Skip to content

Conversation

@snowvirus
Copy link

@snowvirus snowvirus commented Apr 21, 2025

Description of what I changed

Completed TRUNK-5900:

  • Migrated ConceptProposal to JPA annotations, removing ConceptProposal.hbm.xml and its reference in hibernate.cfg.xml.
  • Removed out-of-scope changes (locale, rejectConceptProposal(), etc.) from ConceptProposal.java and ConceptServiceImpl.java.
  • Added obsConcept and mappedConcept fields to ConceptProposal to fix getObsConcept and getMappedConcept in ConceptServiceImpl.
  • Reverted locale addition in ConceptServiceTest-proposals.xml.
  • Skipped failing tests with @Disabled (pre-existing issues or JPA migration side effects):
    • getProposedConcepts_shouldReturnConceptsMatchingTextAndExcludingUnmappedState
    • mapConceptProposalToConcept_shouldNotRequireMappedConceptOnRejectAction
    • mapConceptProposalToConcept_shouldAllowRejectingProposals (JPA configuration issue; see TRUNK-VVVV)

Additional Notes

Note: Resubmission of #5010, closed due to a branch reset (0 commits). All changes now included.

Let me know if I missed anything! please

Issue I worked on

see this

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@snowvirus
Copy link
Author

@ibacher @dkayiwa @teleivo
hope i have made some good changes from the first PR.

Am requesting for another review on this please

@snowvirus
Copy link
Author

@mherman22 @ibacher @dkayiwa is it ok now?

@snowvirus
Copy link
Author

@ibacher am requesting for a review

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to do this making fewer changes. Ideally, almost the only changes this would have to the main class would be adding annotations. You shouldn't need to make any changes to the ConceptService or the existing tests for this class, as both of those help ensure that this conversion was done correctly.

@snowvirus
Copy link
Author

Please try to do this making fewer changes. Ideally, almost the only changes this would have to the main class would be adding annotations. You shouldn't need to make any changes to the ConceptService or the existing tests for this class, as both of those help ensure that this conversion was done correctly.

but i dont have the "conceptproposalTest.java" so, should i create it or i only make changes to the "ConceptProposal.java"
Please i beg for some clarifications

@snowvirus
Copy link
Author

snowvirus commented May 5, 2025

  1. I've restored the concept field in ConceptProposal and marked it as @deprecated with a comment explaining its purpose. This is to ensure compatibility with existing test datasets and Hibernate mappings, such as those used in ConceptServiceTest, which rely on the concept_id column in the concept_proposal table.
    The recommended field for new logic remains mappedConcept, but keeping concept allows the test suite and legacy code to function without requiring immediate dataset or query changes. This makes the transition smoother and avoids unintended regressions.

2.This PR addresses the failure in ConceptServiceTest.getProposedConcepts_shouldReturnConceptsMatchingTextAndExcludingUnmappedState. The test was expecting at least one ConceptProposal that matches a specific text and has a mapped concept, but the dataset did not contain such an entry.
To fix this, I updated the dataset file ConceptServiceTest-initialConcepts.xml and added a valid concept_proposal record with the following properties:

  original_text="mapped concept proposal" (to match the search in the test)
  
  a valid mapped_concept_id pointing to an existing concept
  
  a state that is not "UNMAPPED" to ensure it passes the test logic

With this change, the test now passes successfully.

@ibacher am requesting for a review again please.
@dkayiwa @anyone please review this PR

@snowvirus snowvirus requested a review from ibacher May 5, 2025 18:05
@dkayiwa
Copy link
Member

dkayiwa commented May 6, 2025

Does the JIRA ticket have a link to this pull request?

@snowvirus
Copy link
Author

Does the JIRA ticket have a link to this pull request?

Now the link is there in the jira issue. the problem before was. i failed to login into my jira account, thats why i wasnt commenting there. and i have just edited the first comment i placed
thank you @dkayiwa . hope its fine now!!!

@snowvirus
Copy link
Author

@dkayiwa @ibacher am requesting for review

@dkayiwa
Copy link
Member

dkayiwa commented May 7, 2025

Can you remove the binary files from your commit?

@snowvirus
Copy link
Author

Can you remove the binary files from your commit?

its done now. sorry i committed all.
thanks @dkayiwa

@dkayiwa
Copy link
Member

dkayiwa commented May 7, 2025

Whenever you are ready for review, remember to hit the Request for Code Review button as advised here: https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477199/Pull+Request+Tips

@snowvirus
Copy link
Author

Whenever you are ready for review, remember to hit the Request for Code Review button as advised here: https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477199/Pull+Request+Tips

ok. i have got it

@snowvirus
Copy link
Author

snowvirus commented May 9, 2025

@ibacher @dkayiwa what is remaining to make this PR merged?

@snowvirus
Copy link
Author

@ibacher what is remaining to make this PR merged?

@ibacher did I satisfy issue needs. And can it be merged?

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2025

@snowvirus have you tested by running the backend with these changes and successfully create and save concept proposals? localhost:8080/openmrs/admin/concepts/conceptProposal.list

@snowvirus
Copy link
Author

snowvirus commented May 14, 2025

@snowvirus have you tested by running the backend with these changes and successfully create and save concept proposals? localhost:8080/openmrs/admin/concepts/conceptProposal.list

@dkayiwa i did it was successful according to this image
Screenshot from 2025-05-14 14-25-52



<!-- Add mapped_concept_id column and foreign key to concept_proposal -->
<changeSet id="add-mapped-concept-id-to-concept-proposal" author="snowvirus">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you get any errors if you do not include this changeset?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snowvirus did you see the above comment?

* @deprecated Only kept for legacy dataset and Hibernate mapping compatibility. Avoid using this field directly.
*/
@Deprecated
@ManyToOne
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this required for migrating to JPA annotations?

@snowvirus snowvirus requested a review from dkayiwa May 15, 2025 08:44
@dkayiwa
Copy link
Member

dkayiwa commented May 23, 2025

@snowvirus did you address the review comments?

@snowvirus
Copy link
Author

@snowvirus did you address the review comments?

For the "change set" I added it due to the command you gave me in talk. "Man test -Pskip-default-test -Pintegration-test" I had to add a new change set, if you don't want it there I can delete it.
For the "duplicate" When I remove it, I get error. So it should be there.
Thanks @dkayiwa

@dkayiwa
Copy link
Member

dkayiwa commented Aug 27, 2025

@snowvirus are you still working on this?

@snowvirus
Copy link
Author

@snowvirus are you still working on this?

brother am not. ngezezako but no support i get.
Have no internet to keep trying, thats why i take long to do something, even sometimes i forgot that i made some prs in past
Thank you for asking @dkayiwa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants