BED-17: Queries for BedPatientAssignment should account for the voided field#90
BED-17: Queries for BedPatientAssignment should account for the voided field#90jwnasambu wants to merge 53 commits intoopenmrs:masterfrom
voided field#90Conversation
jwnasambu
changed the title
voided field
|
Code looks good. Can you include what you did to test this change @jwnasambu? |
|
@jwnasambu can you update the ticket with the link to your pull request as advised at? https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477199/Pull+Request+Tips |
|
@dkayiwa I have updated the ticket with the link to my pull request. |
|
Do you mind also adding some tests? |
|
@dkayiwa, @chibongho, I’m really sorry for overlooking the requested changes. I’ve just noticed them now I think it might have been due to fatigue. Please allow me to fix them over the weekend. |
Sorry for the delayed update. @chibongho , @dkayiwa I have implemented the proposed changes. |
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertThrows; | ||
| import static org.mockito.ArgumentMatchers.*; |
There was a problem hiding this comment.
|
I thought that you would create two new tests where one demonstrates what is returned when the voided flag is set and the other when the voided flag is not set. |
|
@dkayiwa feel free to review my PR please! |
|
@jwnasambu when committing, can you always include the ticket id in the commit message as advised at? https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477199/Pull+Request+Tips |
| @@ -67,8 +67,8 @@ public Bed getBedByUuid(String uuid) { | |||
| public Bed getBedByPatient(Patient patient) { | |||
There was a problem hiding this comment.
Can we have the getBedByPatient method called/not mocked, in a test?
| @@ -89,7 +89,7 @@ public Location getWardForBed(Bed bed) { | |||
| @Override | |||
| public BedPatientAssignment getBedPatientAssignmentByUuid(String uuid) { | |||
There was a problem hiding this comment.
Can we have the getBedPatientAssignmentByUuid method called/not mocked, in a test?
| @@ -129,8 +129,9 @@ public List<BedPatientAssignment> getBedPatientAssignmentByVisit(String visitUui | |||
| public List<BedPatientAssignment> getCurrentAssignmentsByBed(Bed bed) { | |||
There was a problem hiding this comment.
Can we have the getCurrentAssignmentsByBed method called/not mocked, in a test?
| @@ -139,7 +140,8 @@ public Bed getLatestBedByVisit(String visitUuid) { | |||
| Session session = sessionFactory.getCurrentSession(); | |||
| Bed bed = (Bed) session | |||
There was a problem hiding this comment.
Can we have the getLatestBedByVisit method called/not mocked, in a test?
| bedManagementService = new BedManagementServiceImpl(); | ||
| bedManagementService.setDao(bedManagementDao); | ||
| } | ||
There was a problem hiding this comment.
Can we discard these formatting changes?
|
|
||
| defaultLocation = Context.getLocationService() | ||
| .getLocationByUuid("8d6c993e-c2cc-11de-8d13-0010c6dffd0f"); | ||
| assertNotNull("Default location should not be null", defaultLocation); |
There was a problem hiding this comment.
I do not think we need this assertion because we are sure that we have this location in the test dataset.
| defaultEncounterType = Context.getEncounterService() | ||
| .getEncounterType("Scheduled"); | ||
|
|
||
| assertNotNull("Default encounter type should not be null", defaultEncounterType); |
|
|
||
| private Visit getTestVisit(Patient patient) { | ||
| List<Visit> visits = Context.getVisitService().getVisitsByPatient(patient); | ||
| if (visits.isEmpty()) { |
There was a problem hiding this comment.
Do you need to test if visits is empty when you are dealing with visits in a known test dataset file?
| private Encounter getTestEncounter(Patient patient) { | ||
| List<Encounter> encounters = Context.getEncounterService().getEncountersByPatient(patient); | ||
|
|
||
| if (encounters.isEmpty()) { |
|
|
||
| Encounter encounter = encounters.get(0); | ||
|
|
||
| if (encounter.getEncounterType() == null) { |
There was a problem hiding this comment.
To make these tests simpler, you can fetch a visit or encounter from the test dataset file, which you know is not null and has all the expected properties filled in.
| } | ||
|
|
||
| private Encounter getTestEncounter(Patient patient) { | ||
| return Context.getEncounterService().getEncountersByPatient(patient).get(0); |
There was a problem hiding this comment.
You can make this test more efficient by directly fetching this encounter with EncounterService.getEncounter(id)
| } | ||
|
|
||
| private Visit getTestVisit(Patient patient) { | ||
| return Context.getVisitService().getVisitsByPatient(patient).get(0); |
There was a problem hiding this comment.
You can make this test more efficient by directly fetching this encounter with VisitService.getVisit(id)
| } | ||
|
|
||
| private Visit getTestVisit(Patient patient) { | ||
| List<Visit> visits = Context.getVisitService().getVisitsByPatient(patient); |
There was a problem hiding this comment.
What i mean is Context.getVisitService().getVisit(visitId)
| } | ||
|
|
||
| private Encounter getTestEncounter(Patient patient) { | ||
| List<Encounter> encounters = Context.getEncounterService().getEncountersByPatient(patient); |
There was a problem hiding this comment.
What i mean is Context.getEncounterService().getEncounter(encounterId)
|
@dkayiwa Kindly feel free to review my PR at your convinient time please! |
|
|
||
| private Bed createBed(String bedNumber) { | ||
| BedType bedType = new BedType(); | ||
| bedType.setName("Test BedType"); |
There was a problem hiding this comment.
Why don't we just load this bed from the test dataset?
|
|
||
| private Visit getTestVisit(Patient patient, Integer visitId) { | ||
| Visit visit = Context.getVisitService().getVisit(visitId); | ||
| assertNotNull(visit); |
There was a problem hiding this comment.
If we know that this visit exists in the test dataset, then we do not need to assertNotNull.
There was a problem hiding this comment.
In fact we do not need a method for this. The caller should just do Context.getVisitService().getVisit(visitId)
|
|
||
| private Encounter getTestEncounter(Patient patient, Integer encounterId) { | ||
| Encounter encounter = Context.getEncounterService().getEncounter(encounterId); | ||
| assertNotNull(encounter); |
|
|
||
| BedPatientAssignment assignment = new BedPatientAssignment(); | ||
| assignment.setBed(bed); | ||
| assignment.setPatient(patient); |
There was a problem hiding this comment.
Instead of doing all this, just have this data in the test dataset file and then it gets loaded as expected.
| BedPatientAssignment assignment = new BedPatientAssignment(); | ||
| assignment.setBed(bed); | ||
| assignment.setPatient(patient); | ||
| assignment.setEncounter(encounter); |
There was a problem hiding this comment.
Thanks for the review @dkayiwa. I am fixing the issue now.
| <person person_id="678" gender="" birthdate="1975-06-30" birthdate_estimated="false" dead="false" creator="1" date_created="2005-01-01 00:00:00.0" changed_by="1" date_changed="2007-09-20 21:54:12.0" voided="false" void_reason="" uuid="6adb7c42-cfd2-4301-b53b-ff17c5654dd7"/> | ||
|
|
||
| <encounter encounter_id="1001" visit_id="1001" encounter_type="2" patient_id="1001" location_id="1" form_id="1" encounter_datetime="2008-08-01 00:00:00.0" creator="1" date_created="2008-08-18 14:09:05.0" voided="false" uuid="12345678-393b-4118-9c83-a3715b82d4de"/> |
There was a problem hiding this comment.
Is there a patient with the ID 1001??
There was a problem hiding this comment.
Thanks for the point out. Though still fixing the change format issues with the help of Daniel
|
Sorry @jwnasambu , I'm taking over this ticket as we need it soon for an upcoming release. New PR here: #102. Closing this one. |
4 participants
Description of what I changed
Updated BedPatientAssignment queries in BedManagementDaoImpl to exclude voided records.
Issue I worked on
https://openmrs.atlassian.net/browse/BED-17