New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trunk 2768 #150
Trunk 2768 #150
Conversation
Fixed problem causing failed unit test.
Liquibase update Added property deathdateEstimated to Person.hbm.xml
Added deathdateEstimated
@@ -223,6 +226,29 @@ public void setBirthdateEstimated(Boolean birthdateEstimated) { | |||
} | |||
|
|||
/** | |||
* @return true if person's deathdate is estimated | |||
*/ | |||
public Boolean isDeathdateEstimated() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should return boolean and not Boolean. If deathdateEsitmate == null return false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this Rafal? shouldn't we leave it null when unset, e.g. in the case where the patient is alive? And thus this method should be getDeathdateEstimated, not isDeathdateEstimated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a convention that isSomething should be returning boolean and getSomething should be returning Boolean. I know it's wrong in other places in this class, but let's do it right for new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying that the property is correctly a Boolean, because it should be nullable, and therefore we should have a getDeathdateEstimated method that returns Boolean (and not isDeathdateEstimated method).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with removing isDeathdateEstimated. We have the correct getDeathEstimated a few lines below.
Removed method isDeathDateEstimated() from Person.java
Got following comment from Rafal: In api/src/main/java/org/openmrs/api/impl/PatientServiceImpl.java:
I don't follow. What did you want to say here? I don't follow it either. Again, just following instructions - change like birthdateEstimated. I tried replacing the if statement with:
This caused the following: at org.openmrs.api.PatientServiceTest.mergePatients_shouldAuditPriorDateOfDeath(PatientServiceTest.java:2866) PatientServiceTest.java:2866 Any suggestions on how to handle this? |
Changes re Rafal's suggestions.
Changed deathdateEstimated = person.isDeathdateEstimated(); To deathdateEstimated = person.getDeathdateEstimated() isDeathdateEstimated() has been removed
preferred.setDeathDate(notPreferred.getDeathDate()); | ||
preferred.setDeathdateEstimated(notPreferred.getDeathdateEstimated()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try with:
{code}
mergedData.setPriorDateOfDeathEstimated(preferred.getDeathdateEstimated());
if (preferred.getDeathdateEstimated() == null)
preferred.setDeathdateEstimated(notPreferred.getDeathdateEstimated());
mergedData.setPriorDateOfDeath(preferred.getDeathDate());
if (preferred.getDeathDate() == null)
preferred.setDeathDate(notPreferred.getDeathDate());
{code}
Fix code causing error in test Add checkbox for deathdateEstimated to editPersonInfo.jsp
Changed shortPatientForm.jsp, editPersonInfo.jsp for deathdateEstimated.
Added test for deathdateEstimated to PatientServiceTest.java
Fix problem preventing deathdate_estimated column in Person table from getting set.
Cleaned up comments
Cleaned up Comments
@@ -219,6 +220,7 @@ public void shouldCreatePatient() throws Exception { | |||
// patient.removeAddress(pAddress); | |||
|
|||
patient.setDeathDate(new Date()); | |||
patient.setBirthdateEstimated(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to call setDeathdateEstimated
* Found modules with unresolved backend dependencies. - Current implementation compares the required with incorrect installed version - Fixing it * Found modules with unresolved backend dependencies. -- Removed getInstalledVersion in favor of find() * Found modules with unresolved backend dependencies. -- Removed getInstalledVersion in favor of find() * Found modules with unresolved backend dependencies. -- Removed getInstalledVersion in favor of find() --- formatted Co-authored-by: amuj <alaboso@uonbi.ac.ke>
Still getting errors on 28 tests in PatientServiceTest - all dealing with mergePatients.
Controllers:
PersonFormController.java
Don't know if we need change here - no deathDate info on person is captured
birthDate is and birthdateEstimated is - see method getMiniPerson(person, name, gender, date, age).
deathDate is mentioned but not set on person set on Obs
!!! Maybe deathdate info needs to be added here and gotten from request
No changes needed for PatientFormController.java
There are some comments I put in the code - I need them - they need to stay till the code is finished.