TRUNK-5492 Replace Person and Patient hbm mapping file with annotations #2900

wluyima · 2019-05-17T05:36:48Z

TRUNK-5492 Replace Patient hbm mapping file with annotations

TRUNK-5492 Fixed failing tests

TRUNK-5492 Added special auditable fields to Patient table

TRUNK-5492 Updated Auditable interceptor to set patient auditable fields instead of PatientSaveHandler

TRUNK-5492 Renamed properties back to their original names

TRUNK-5492 Removed unused code

TRUNK-5492 Removed changes in BaseOpenmrsData

TRUNK-5492 Removed changes in PersonServiceTest

TRUNK-5492 Removed explicit lazy loading from JPA mappings

TRUNK-5492 Removed more explicit lazy loading from JPA mappings

TRUNK-5492 Replaced @sort with @SortNatural

TRUNK-5492 Removed more explicit lazy loading from JPA mappings

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5492

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
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

@sort

TRUNK-5492 Replace Patient hbm mapping file with annotations TRUNK-5492 Fixed failing tests TRUNK-5492 Added special auditable fields to Patient table TRUNK-5492 Updated Auditable interceptor to set patient auditable fields instead of PatientSaveHandler TRUNK-5492 Renamed properties back to their original names TRUNK-5492 Removed unused code TRUNK-5492 Removed unused code TRUNK-5492 Removed changes in BaseOpenmrsData TRUNK-5492 Removed changes in PersonServiceTest TRUNK-5492 Removed explicit lazy loading from JPA mappings TRUNK-5492 Removed more explicit lazy loading from JPA mappings TRUNK-5492 Replaced @sort with @SortNatural TRUNK-5492 Removed more explicit lazy loading from JPA mappings

coveralls · 2019-05-17T05:53:48Z

Coverage increased (+0.03%) to 59.862% when pulling 21f54ca on wluyima:TRUNK-5492 into 61178e8 on openmrs:master.

wluyima · 2019-05-17T06:00:05Z

api/src/test/java/org/openmrs/test/BaseContextSensitiveTest.java

@@ -583,6 +584,10 @@ public void initializeInMemoryDatabase() throws SQLException {
 			throw new RuntimeException(
 			        "You shouldn't be initializing a NON in-memory database. Consider unoverriding useInMemoryDatabase");

+		//Because creator property in the superclass is mapped as nullable by the autoddl tool, we need to first


I need to rephrase this, meant to say:

Because creator property in the superclass is mapped with optional set to false, the autoddl tool marks the column as not nullable

wluyima · 2019-05-17T13:11:42Z

api/src/main/java/org/openmrs/Patient.java

@@ -172,7 +222,13 @@ public void setPersonId(Integer personId) {
 	 * @see org.openmrs.PatientIdentifier
 	 */
 	public void setIdentifiers(Set<PatientIdentifier> identifiers) {
-		this.identifiers = identifiers;
+		if (identifiers == null) {


Probably this is no longer necessary

…extSensitiveTest

… property

dkayiwa · 2019-05-19T10:37:16Z

The link to the JIRA ticket does not look correct.

wluyima · 2019-05-23T02:24:22Z

Fixed

dkayiwa · 2019-05-23T16:50:31Z

api/src/main/java/org/openmrs/Patient.java

@@ -99,6 +156,9 @@ public Patient(Patient patient) {
 	 * @return internal identifier for patient
 	 */
 	public Integer getPatientId() {
+		if (this.patientId == null) {


Do you mind explaining why you needed to do this?

In-memory, this field is null when you create the patient record since it is technically mapped to a FK column which is why it is mapped as not insertable and not updatable, I wonder why hibernate was setting it with xml but not with annotations though, my guess is there could be a hibernate specific annotations to fix it that JPA doesn't support, will look into it a little a bit to see if there is another work around

What do you think of adding this comment to the code?

I can but I see that you already merged the PR

You can do it as a followup commit.

dkayiwa · 2019-05-23T16:53:38Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernatePatientDAO.java

@@ -608,7 +608,7 @@ public boolean isIdentifierInUseByAnotherPatient(PatientIdentifier patientIdenti
 		        && patientIdentifier.getIdentifierType().getUniquenessBehavior() == UniquenessBehavior.LOCATION;

 		// switched this to an hql query so the hibernate cache can be considered as well as the database
-		String hql = "select count(*) from PatientIdentifier pi, Patient p where pi.patient.patientId = p.patient.patientId "


Is this a bug fix? Or is it a change caused by switching to annotations?

This is a bug fix because it was wrong before, annotations just exposed it, p is an alias for patient, and patient for sure doesn't have a patient field

dkayiwa · 2019-05-23T16:54:25Z

api/src/main/java/org/openmrs/api/handler/PatientSaveHandler.java

@@ -45,5 +46,10 @@ public void handle(Patient patient, User creator, Date dateCreated, String other
 				}
 			}
 		}
+
+		if(patient.getPatientId() == null){
+			patient.setCreator(Context.getAuthenticatedUser());


Is this a bug fix? Or is it a change caused by switching to annotations?

Not really but sort of, more like bad design, this is needed because creator and dateCreated fields exist both in BaseOpenmrsData and Patient, this is actually bad practice because it defeats the purpose of inheritance, and this where xml and annotations differ, because annotations are pure java whereas xml isn't, with xml you get away with bad things just like we were doing before which is why I say sort of a bug.

Anyways, long story short, hibernate is unable to set these fields via the AuditableInterceptor mechanism because based on the ClassMedata it has built for Patient class, it sees the same fields on the superclass and the subclass and treats them as different because they are declared by different classes anyways, it only sets the values of the fields in the superclass and not the subclass so we have to manually set them from here as we used to do before to get around it.

What do you think of adding this comment to the code?

dkayiwa · 2019-05-23T16:54:47Z

api/src/main/java/org/openmrs/api/handler/PersonSaveHandler.java

 			for (PersonAddress pAddress : person.getAddresses()) {
-				if (pAddress.isBlank()){
-					person.removeAddress(pAddress);
+				if (pAddress.isBlank()) {


Is this a bug fix? Or is it a change caused by switching to annotations?

Definitely a bug, it's java 101, you can't remove an element from the same Iterable you are looping over, you get a ConcurrentModificationException

wluyima · 2019-05-24T03:38:10Z

api/src/test/java/org/openmrs/api/PatientServiceTest.java

@@ -2096,7 +2094,7 @@ public void savePatientIdentifier_shouldAllowLocationToBeNullWhenLocationBehavio
 	 */
 	@Test(expected = ValidationException.class)
 	public void savePatientIdentifier_shouldAllowLocationToBeNullWhenLocationBehaviourIsRequired() {
-		PatientIdentifier patientIdentifier = patientService.getPatientIdentifier(7);
+		PatientIdentifier patientIdentifier = patientService.getPatientIdentifier(9);


This was a bad patient, they are voided but it worked before because they had a non-voided identifier which I changed to voided and it got broken, so I had to pick another non-voided patient with a non voided identifier

wluyima · 2019-05-24T03:41:27Z

...src/test/resources/org/openmrs/api/db/hibernate/include/HibernatePatientDAOTest-patients.xml

@@ -68,7 +68,7 @@
        Voided patient with non-voided name
    -->
    <person person_id="60" gender="M" birthdate="1998-09-30 12:34:56.7" birthdate_estimated="0" dead="false" creator="1" date_created="2014-01-09 00:00:00.0" voided="false" uuid="7443D9DD-CA51-4BD9-9E68-7C43628B82DF"/>
-    <person_name person_name_id="60" preferred="true" person_id="60" prefix="Mr." given_name="Meriadoc" middle_name="" family_name="Brandybuck" family_name2="" creator="1" date_created="2014-01-09 00:00:00.0" voided="false" void_reason="" uuid="61ACD71F-989B-4B4A-B00E-0503B756099F"/>
+    <person_name person_name_id="60" preferred="true" person_id="60" prefix="Mr." given_name="Meriadoc" middle_name="" family_name="Brandybuck" family_name2="" creator="1" date_created="2014-01-09 00:00:00.0" voided="true" void_reason="" uuid="61ACD71F-989B-4B4A-B00E-0503B756099F"/>


This was a construed scenario that should never arise based on the semantics of our API because a voided patient should never have a non-voided name, If an admin tinkers with data in the DB, they better know what they are doing.

wluyima · 2019-05-24T03:43:02Z

api/src/test/resources/org/openmrs/api/db/hibernate/include/HibernatePersonDAOTest-people.xml

@@ -56,7 +56,7 @@
        Voided person with non-voided name
    -->
    <person person_id="50" gender="M" birthdate="1998-09-30 12:34:56.7" birthdate_estimated="0" dead="false" creator="1" date_created="2013-04-25 12:34:56.7" voided="true" uuid="2B856CBF-23D4-4D37-B2F6-FF8BD23A0434"/>
-    <person_name person_name_id="50" preferred="true" person_id="50" prefix="Mr." given_name="voided-bravo" middle_name="voided-foxtrot" family_name="voided-kilo" family_name2="voided-oscar" creator="1" date_created="2013-04-18 12:34:56.7" voided="false" void_reason="" uuid="3CCEB428-2B96-48AF-AAA3-36DB80601C9B"/>
+    <person_name person_name_id="50" preferred="true" person_id="50" prefix="Mr." given_name="voided-bravo" middle_name="voided-foxtrot" family_name="voided-kilo" family_name2="voided-oscar" creator="1" date_created="2013-04-18 12:34:56.7" voided="true" void_reason="" uuid="3CCEB428-2B96-48AF-AAA3-36DB80601C9B"/>


Same here, name has to be voided if patient is voided

wluyima · 2019-05-24T03:43:51Z

api/src/test/resources/org/openmrs/include/standardTestDataset.xml

@@ -302,7 +302,7 @@
  <patient_identifier patient_identifier_id="4" patient_id="7" identifier="6TS-4" identifier_type="1" preferred="1" location_id="1" creator="1" date_created="2006-01-18 00:00:00.0" voided="false" void_reason="" uuid="489edb7f-0786-4726-88a3-f0fcd2505d1a"/>
  <patient_identifier patient_identifier_id="5" patient_id="8" identifier="7TU-8" identifier_type="1" preferred="0" location_id="1" creator="1" date_created="2006-01-18 00:00:00.0" voided="false" void_reason="" uuid="2c23455b-dee3-450c-914f-fe4bd0748d81"/>
  <patient_identifier patient_identifier_id="6" patient_id="8" identifier="ABC123" identifier_type="2" preferred="0" location_id="1" creator="1" date_created="2006-01-18 00:00:00.0" voided="true" void_reason="Incorrect"/>
-  <patient_identifier patient_identifier_id="7" patient_id="999" identifier="XYZ" identifier_type="2" preferred="1" location_id="1" creator="1" date_created="2006-01-18 00:00:00.0" voided="false" void_reason=""/>
+  <patient_identifier patient_identifier_id="7" patient_id="999" identifier="XYZ" identifier_type="2" preferred="1" location_id="1" creator="1" date_created="2006-01-18 00:00:00.0" voided="true" void_reason=""/>


Same here, which is why I changed the patient for the test I mentioned earlier

dkayiwa · 2019-05-24T08:43:01Z

I take the switch from xml to annotations as a non simple change, which may have un expected effects. And therefore, i would feel more comfortable if we separated bug fixes that have nothing to do with xml vs annotations, in their own ticket and pull request. That would also simplify the reviews by easily telling what exactly was needed to change for the sake of annotations.

wluyima · 2019-05-24T22:48:05Z

I see what you are saying, and am not opposed to it, the reasons why I did it this way are:

These are 'bugs', I want to think of them as flaws, that are not all easily reproducible before changing to annotations, xml was just masking them, I think in terms of TDD how will you create a failing test to reproduce the issue and fix the issue?
If I create separate tickets which imply separate PR then you can't merge this PR before they all merged and you can't reproduce them before you merge this, kind of a chicken-egg dilemma there.

I made sure I added some quick notes to explain what each change attempts to fix to make it easier for the reviewer

wluyima · 2019-05-29T00:17:28Z

@dkayiwa did you see my response?

wluyima · 2019-05-30T21:42:23Z

Sounds good!

…nnotations (#2900)" This reverts commit 9c3a95f.

* Revert "TRUNK-5492: Replace Person and Patient hbm mapping file with annotation" This reverts commit 5377688. * Revert "TRUNK-5685: In BaseContextSenstiveTest, make dropNotNullConstraint method protected" This reverts commit ebb491d. * Revert "TRUNK-5685: In BaseContextSenstiveTest, make dropNotNullConstraint method public" This reverts commit 1a38e83. * Revert "TRUNK-5492 Replace Person and Patient hbm mapping file with annotations (#2900)" This reverts commit 9c3a95f.

wluyima commented May 17, 2019

View reviewed changes

mks-d requested review from dkayiwa, rkorytkowski and bmamlin May 17, 2019 14:05

wluyima20 added 2 commits May 17, 2019 10:54

TRUNK-5492 Removed unnecessary tests and updated comments on BaseCont…

de6fbfe

…extSensitiveTest

TRUNK-5492 Removed getter/setter overrides and changed access type to…

21f54ca

… property

dkayiwa reviewed May 23, 2019

View reviewed changes

wluyima commented May 24, 2019

View reviewed changes

dkayiwa merged commit 9c3a95f into openmrs:master May 29, 2019

Ruhanga added a commit that referenced this pull request Jan 23, 2020

Revert "TRUNK-5492 Replace Person and Patient hbm mapping file with a…

49e79b6

…nnotations (#2900)" This reverts commit 9c3a95f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRUNK-5492 Replace Person and Patient hbm mapping file with annotations #2900

TRUNK-5492 Replace Person and Patient hbm mapping file with annotations #2900

wluyima commented May 17, 2019 •

edited

Loading

coveralls commented May 17, 2019 •

edited

Loading

wluyima May 17, 2019

wluyima May 17, 2019

dkayiwa commented May 19, 2019

wluyima commented May 23, 2019

dkayiwa May 23, 2019

wluyima May 24, 2019 •

edited

Loading

dkayiwa May 29, 2019

wluyima May 30, 2019

dkayiwa May 30, 2019

dkayiwa May 23, 2019

wluyima May 24, 2019

dkayiwa May 23, 2019

wluyima May 24, 2019

dkayiwa May 29, 2019

dkayiwa May 23, 2019

wluyima May 24, 2019

wluyima May 24, 2019

wluyima May 24, 2019 •

edited

Loading

wluyima May 24, 2019

wluyima May 24, 2019

dkayiwa commented May 24, 2019

wluyima commented May 24, 2019

wluyima commented May 29, 2019

wluyima commented May 30, 2019

TRUNK-5492 Replace Person and Patient hbm mapping file with annotations #2900

TRUNK-5492 Replace Person and Patient hbm mapping file with annotations #2900

Conversation

wluyima commented May 17, 2019 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

coveralls commented May 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented May 19, 2019

wluyima commented May 23, 2019

Choose a reason for hiding this comment

wluyima May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wluyima May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented May 24, 2019

wluyima commented May 24, 2019

wluyima commented May 29, 2019

wluyima commented May 30, 2019

wluyima commented May 17, 2019 •

edited

Loading

coveralls commented May 17, 2019 •

edited

Loading

wluyima May 24, 2019 •

edited

Loading

wluyima May 24, 2019 •

edited

Loading