Skip to content
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

RCM-109 Added patient verification to return Cohorts objects for the serviceMethods #39

Closed
wants to merge 1 commit into from

Conversation

@suthagar23
Copy link
Member

@suthagar23 suthagar23 commented Aug 6, 2018

Descriptions

In reporting compatibility module used the org.openmrs.cohort.Cohort class instead of org.openmrs.Cohort class. So other modules which are using these service methods needed to add the reporting compatibility module as the required module.

To avoid this requirement, A patient verification is added to the Cohort Membership methods.

Ticket Information

Ticket : https://issues.openmrs.org/browse/RCM-109

@@ -299,4 +300,28 @@ public static PatientFilter toPatientFilter(PatientSearch search, CohortSearchHi
c.setMemberIds(cohort.getMemberIds());
return c;
}

public static Cohort verifyPatients(Cohort cohort) {

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

What does verifyPatients mean?

This comment has been minimized.

@suthagar23

suthagar23 Aug 6, 2018
Author Member

Just verify the given patients in the cohort is exist or not.

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

The method name does not look meaningful because verify can mean very many things.

This comment has been minimized.

@suthagar23

suthagar23 Aug 6, 2018
Author Member

Thus, Could we think of "removeNullPatientsFromCohort" ?

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

What are null patients? Does this mean the patient collection has a list of nulls

This comment has been minimized.

@suthagar23

suthagar23 Aug 6, 2018
Author Member

Actually, patient ids collection contains the patientIds which are not reachable(by some restrictions eg. location). So it should remove those ids which have the null patient objects at that time.
What would be the most appropriate name for this method?

if(patientIds != null) {
Iterator<Integer> iterator = patientIds.iterator();
while (iterator.hasNext()) {
Integer patientId = iterator.next();

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

Does the above line mean that the iterator can return an element which is null?

This comment has been minimized.

@suthagar23

suthagar23 Aug 6, 2018
Author Member

Actually patientId will have the value, the patient object which is pointed by the patientId could be null at some times (by the location restrictions)

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

Are you explaining this in reference to the next line which checks if patientId != null?

This comment has been minimized.

@suthagar23

suthagar23 Aug 6, 2018
Author Member

Nope, I addressed the line 315 and line 317. In most cases, patientId will not be null (Since Im checking this against null for the confirmation).
But the patient points by that patientId can be null according to the location restrictions. So the line 315 will have the value (not null), but the line 317 may have the null object.

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

Am not yet talking about location restrictions. Am simply talking about the return value of iterator.next()
Do we expect it to have null, when hasNext() returned true?

This comment has been minimized.

@suthagar23

suthagar23 Aug 6, 2018
Author Member

Ah yes, Just got your point 😄. No need to have this.
I have updated the code

@suthagar23 suthagar23 force-pushed the suthagar23:RCM-109 branch from 8d9d334 to d65154b Aug 6, 2018
@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Aug 6, 2018

Instead of removing the patient ids from the collection, can't we just filter them at source? Where they are loaded from the database?

@suthagar23
Copy link
Member Author

@suthagar23 suthagar23 commented Aug 6, 2018

Instead of adding this to the DAO resource (we might also need to add for multiple lines), why we do not add this to the Cohort constructor here (just some places will be affected - Constructor,setMemberIds and addMember methods) ?
https://github.com/openmrs/openmrs-module-reportingcompatibility/blob/master/api/src/main/java/org/openmrs/cohort/Cohort.java#L70

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Aug 6, 2018

Looks fine to me for now. As long as those who do not have our module installed will not be affected.

@suthagar23
Copy link
Member Author

@suthagar23 suthagar23 commented Aug 6, 2018

@dkayiwa Do we only use the Cohort objects for Patients? If we are using the Cohorts for other members then We can't add this patient verification to the Cohort Class. Because it will caused the problems for other members.
If we are using Cohort for other members(like for persons and users) , then we should add these to the DAO resource, otherwise, we can add this to the Cohort class. What do you think?

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Aug 6, 2018

Are there any major disadvantages of adding this to the DAO layer?

@suthagar23
Copy link
Member Author

@suthagar23 suthagar23 commented Aug 6, 2018

We need to change multiple lines to make the changes in multiple class files. But going for the Cohort will only need to change few lines.
But the parameters and the methods are contained "patients" in their names in the Cohort Class. So I think mostly we will use the Cohorts for the patients. Is it Right?

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Aug 6, 2018

Yes that is right.

@suthagar23 suthagar23 force-pushed the suthagar23:RCM-109 branch from d65154b to 0350c2a Aug 6, 2018
@suthagar23
Copy link
Member Author

@suthagar23 suthagar23 commented Aug 6, 2018

@dkayiwa I have changed the implementation. Please review again THanks.


public static Collection<Integer> removeNullPatients(Collection<Integer> patientIds) {
PatientService patientService = Context.getPatientService();
if(patientIds != null) {

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

To reduce nesting, just return if the list is null.

This comment has been minimized.

@suthagar23

suthagar23 Aug 7, 2018
Author Member

Changed


public static Boolean doesPatientExist(Integer patientId) {
PatientService patientService = Context.getPatientService();
if(patientId != null && patientService.getPatient(patientId) != null) {

This comment has been minimized.

@dkayiwa

dkayiwa Aug 6, 2018
Member

Are there cases when this is called with a null patientId?

This comment has been minimized.

@suthagar23

suthagar23 Aug 7, 2018
Author Member

Yes, If I created a Cohort, and tried to insert a member with addMember(null). Then it can be null

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Aug 6, 2018

Is it worth making all these extra calls when our module is not installed?

@suthagar23
Copy link
Member Author

@suthagar23 suthagar23 commented Aug 7, 2018

Yes, I also thought this.
I have added Global Property("Cohorts.checkForNullPatients") to track this extra work. If the GP points to true, then do the extra works else just ignore them by default.
The GP value should be created/set to True while starting our module.

@suthagar23 suthagar23 force-pushed the suthagar23:RCM-109 branch 2 times, most recently from 4e441df to aa5e391 Aug 7, 2018
RCM-109 Added patient verification to the Cohorts

RCM-109 Added patient verification to the Cohorts

RCM-109 Added patient verification to the Cohorts
@suthagar23 suthagar23 force-pushed the suthagar23:RCM-109 branch from aa5e391 to 8f613ff Aug 7, 2018
@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Aug 7, 2018

Why create an extra gp? Can't you use the same logic like you used for the reference application?

@suthagar23
Copy link
Member Author

@suthagar23 suthagar23 commented Aug 7, 2018

In the reference application also, we have created a GP through our module and used that. What do you mean by the logic?
If you have meant about the same GP, then we might not use that here since the name of that GP doesn't relate to this requirement (See here).

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Aug 15, 2018

Closing this for now until when we get feedback from the 0.1.0 release

@dkayiwa dkayiwa closed this Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants