LBAC-3 Added implementation for assigning the patients to the locations on registration #4
Conversation
f7baf60
to
3a86fc1
@dkayiwa I have changed this PR with the whole code base for the location assignment while patients registering on the system. Please have a look at here. |
Have you tested this from the end user's perspective and proved that it works? |
Yes @dkayiwa, This implementation is enough to create a personAttribute with accessLocation value in the database. This implementation will work as shown below,
We can improve this to select the locations by the user for the patients, and can improve to find a default location as well. |
Can you list for me the steps to test this out (from the end user's perspective)? |
Sure, |
How do i specify a location, as an end user? |
@@ -46,5 +46,10 @@ | |||
<file>messages_es.properties</file> | |||
</messages> | |||
<!-- /Internationalization --> | |||
|
|||
<advice> | |||
<point>org.openmrs.api.PatientService</point> |
dkayiwa
May 27, 2018
Member
Shouldn't this be on PersonService?
Shouldn't this be on PersonService?
suthagar23
May 28, 2018
Author
Member
Yes correct, Actually I plan to change it to PersonService while implementing the user location management(To address Patients and Users). I addressed this PR as only for Patients, So I handle only with high-level PatientService.
I will change it to Persons and will update the PR content.
Yes correct, Actually I plan to change it to PersonService while implementing the user location management(To address Patients and Users). I addressed this PR as only for Patients, So I handle only with high-level PatientService.
I will change it to Persons and will update the PR content.
dkayiwa
May 29, 2018
Member
I thought PersonService would automatically deal with patients because they are persons. Not so?
I thought PersonService would automatically deal with patients because they are persons. Not so?
suthagar23
Jun 18, 2018
Author
Member
Nope, AFAIK We need to access this PatientService
Nope, AFAIK We need to access this PatientService
/** | ||
* It will used to implement the AOP Methods for PatientServiceImpl class | ||
*/ | ||
public class PatientControlImpl extends StaticMethodMatcherPointcutAdvisor implements Advisor { |
dkayiwa
May 27, 2018
Member
Shouldn't this be named PersonServiceAdvisor?
Shouldn't this be named PersonServiceAdvisor?
suthagar23
May 28, 2018
Author
Member
I will change it to Persons and will update the PR content as I addressed above
I will change it to Persons and will update the PR content as I addressed above
|
||
public static final String PATIENT_OBJ_NAME = "PATIENT"; | ||
|
||
public static final String SAVE_PATIENT_METHOD_NAME = "savePatient"; |
suthagar23
May 28, 2018
Author
Member
This need to be updated as saveUser
to make those changes.
This need to be updated as saveUser
to make those changes.
dkayiwa
May 29, 2018
Member
We have not yet got to the ticket which deals with users.
We have not yet got to the ticket which deals with users.
|
||
public static final String ACCESS_LOCATION_PERSON_ATTRIBUTE_TYPE_NAME = "accessLocation"; | ||
|
||
public static final String PATIENT_OBJ_NAME = "PATIENT"; |
suthagar23
May 28, 2018
Author
Member
This need to be updated as USER
to make those changes(To deal with PersonService).
This need to be updated as USER
to make those changes(To deal with PersonService).
dkayiwa
May 29, 2018
Member
Not handled users yet
Not handled users yet
@dkayiwa Finally I came up with this PR,
Please use this App definition to customize the patient dashboard - https://gist.github.com/suthagar23/f0b92381aec5d0ad6d70e4eb40eb308d You need the updated appui module to run this part! |
0bb457c
to
d8fccd4
You are still doing more than what i asked, for the first pass. |
5e8a9e2
to
eeeef83
Oh @dkayiwa I have removed the AOP implementation from this PR. It has a simple implementation to assign the access locations through patient registration dashboard now. Please use this App definition to customize the patient dashboard - https://gist.github.com/suthagar23/f0b92381aec5d0ad6d70e4eb40eb308d |
No need of any AOP to assign access locations. I think you are still complicating the use case. Let me state this again.
As simple as that. I do not want to see code which implements any thing apart from just that. This is a very simple use case which we can even demonstrate to the client, as concrete progress update. |
Yes @dkayiwa. I totally understood. It will be a solution for the Case-1. Then shall I create another ticket for case-2? |
When you ask me about use cases 2 when you are not yet even done with use case 1, i start to think that you are not prioritizing as you should. |
Oh, I think, you missed my point in the last comment I have mentioned And Usecase-2 as, This PR will be a solution for usecase-1 and, I will work to create another PR for usecase-2. Is it alright? |
You pull request title "Added implementation for assigning the patients to the locations on registration" says it is assigning patients to locations. Isn't that supposed to be automatically done by the patient registration form? Do you need it in your pull request? |
Ohh @dkayiwa , So then, I have updated this PR with filters for PatientSearch. Please have a look again here. |
This pull request still has too much more than the first use case that we are dealing with. |
1ed360a
to
0122c94
return false; | ||
} | ||
else { | ||
if(!personAttribute.getValue().equals(location.getUuid())) { |
dkayiwa
Jun 24, 2018
Member
Is it efficient to call location.getUuid() for each and every patient?
Is it efficient to call location.getUuid() for each and every patient?
suthagar23
Jun 24, 2018
Author
Member
We can improve it by replacing UUID.
We can improve it by replacing UUID.
suthagar23
Jun 24, 2018
Author
Member
Changed
Changed
return patientList; | ||
} | ||
|
||
public Boolean isPatientConstainsLocation(Location location, PersonAttribute personAttribute ) { |
dkayiwa
Jun 24, 2018
Member
This method name is still confusing based on its parameters. The grammar of the name is also not correct.
Just get rid of it.
This method name is still confusing based on its parameters. The grammar of the name is also not correct.
Just get rid of it.
suthagar23
Jun 24, 2018
Author
Member
Oh, Sorry small mistake on Contains
. Let me know if there are any issues with this fixed method name.
Oh, Sorry small mistake on Contains
. Let me know if there are any issues with this fixed method name.
cf38324
to
f3dc711
return patientList; | ||
} | ||
|
||
public Boolean isPatientContainsLocation(String locationUuid, PersonAttribute personAttribute ) { |
dkayiwa
Jun 24, 2018
Member
Looking at the method name, is there anything in this method that has to do with patient?
Looking at the method name, is there anything in this method that has to do with patient?
suthagar23
Jun 24, 2018
Author
Member
That's why I thought to replace by isPatientAttributeContainsLocation()
. Wha do you think about this name?
That's why I thought to replace by isPatientAttributeContainsLocation()
. Wha do you think about this name?
dkayiwa
Jun 24, 2018
Member
Can you just do a 10 minutes google search about good method names?
Can you just do a 10 minutes google search about good method names?
return patientList; | ||
} | ||
|
||
public Boolean compare(String value1, String value2) { |
dkayiwa
Jun 25, 2018
Member
Do we have to make this public?
Can we also reduce the number of return statements in this method?
Do we have to make this public?
Can we also reduce the number of return statements in this method?
suthagar23
Jun 25, 2018
Author
Member
We can make it as private.
I have fixed it as well and reduced the number of returns.
We can make it as private.
I have fixed it as well and reduced the number of returns.
|
||
private Boolean compare(String value1, String value2) { | ||
if (StringUtils.isNotBlank((value1)) && StringUtils.isNotBlank((value2))) { | ||
if(value1.equals(value2)) { |
dkayiwa
Jun 25, 2018
Member
Can you also reduce the number of if statements?
Can you also reduce the number of if statements?
suthagar23
Jun 25, 2018
Author
Member
Reduced by removing on IF condition
Reduced by removing on IF condition
} | ||
|
||
private Boolean compare(String value1, String value2) { | ||
if (StringUtils.isNotBlank((value1)) && StringUtils.isNotBlank((value2))) { |
dkayiwa
Jun 25, 2018
Member
Can you reduce to one return statement?
Can you reduce to one return statement?
suthagar23
Jun 25, 2018
Author
Member
Changed
Changed
import org.apache.commons.lang3.StringUtils; | ||
import org.apache.commons.logging.Log; | ||
import org.apache.commons.logging.LogFactory; | ||
import org.openmrs.*; |
suthagar23
Jun 25, 2018
Author
Member
oh, Sorry for the mistake. I have fixed. Please check again
oh, Sorry for the mistake. I have fixed. Please check again
dkayiwa
Jun 25, 2018
Member
Did you see my latest comments on JIRA?
Did you see my latest comments on JIRA?
|
||
for (Iterator<Patient> iterator = patientList.iterator(); iterator.hasNext(); ) { | ||
Patient patient = iterator.next(); | ||
if (!compare(patient.getAttribute(personAttributeType).getValue(), sessionLocationUuid)) { |
dkayiwa
Jun 26, 2018
Member
Did you actually test these changes by compiling the module and try to search for patients?
Did you actually test these changes by compiling the module and try to search for patients?
suthagar23
Jun 26, 2018
Author
Member
Yes @dkayiwa, I have tested with my local deployment. it's working for me (I already created that person attribute type and global property)
Yes @dkayiwa, I have tested with my local deployment. it's working for me (I already created that person attribute type and global property)
dkayiwa
Jun 26, 2018
Member
Have you tried searching when you have patients who did not save any value in the location person attribute?
Have you tried searching when you have patients who did not save any value in the location person attribute?
suthagar23
Jun 27, 2018
Author
Member
Yah, there was an issue on fetching personAttribute for them. I have fixed it and updated the PR.
Yah, there was an issue on fetching personAttribute for them. I have fixed it and updated the PR.
Patient patient = iterator.next(); | ||
PersonAttribute personAttribute = patient.getAttribute(personAttributeType); | ||
if( personAttribute != null) { | ||
if (!compare(personAttribute.getValue(), sessionLocationUuid)) { |
dkayiwa
Jun 27, 2018
Member
Combine these two if statements such that you do not duplicate iterator.remove
Combine these two if statements such that you do not duplicate iterator.remove
suthagar23
Jun 27, 2018
Author
Member
Changed.
Changed.
6f00b92
to
ab9d05a
|
||
for (Iterator<Patient> iterator = patientList.iterator(); iterator.hasNext(); ) { | ||
Patient patient = iterator.next(); | ||
PersonAttribute personAttribute = patient.getAttribute(personAttributeType); |
dkayiwa
Jun 29, 2018
Member
While testing, i have noticed something. If some one installs the module, but with some patients who do not yet have locations, they will not be listed on search. So how are they supposed to be listed, in order to be able to edit their locations? Any ideas?
While testing, i have noticed something. If some one installs the module, but with some patients who do not yet have locations, they will not be listed on search. So how are they supposed to be listed, in order to be able to edit their locations? Any ideas?
suthagar23
Jul 2, 2018
Author
Member
Updated.
So now, System Administrators can get the patients lists for the logged-in location + patients lists who haven't location attribute. So System Administrators can edit the patient location.
The code base is updated along with this check.
Updated.
So now, System Administrators can get the patients lists for the logged-in location + patients lists who haven't location attribute. So System Administrators can edit the patient location.
The code base is updated along with this check.
…ns on registration Removed personAttribute creation Added fixes for PR comments PR Review fixes Added minor fix Added some fixes according to the PR reviews Minor fix Changed the locationCheck method structure minor fix Changed method name Added fixes for PR reviews Added fixes for PR reviews Changed method to compare Changed Compare method Minor improvements Minor modifications Minor fix Minor fix in the methods Merged IF conditions Removed unwanted dependencies Added appUi to required module Added user role check for viewing patients
Can you also add some unit tests for this functionality? |
I will follow the unit test methods for AOP Advices and update the unit testing for my work later. |
Description
Added implementation for assigning the users to the locations on registration.
Ticket
Ticekt : https://issues.openmrs.org/browse/LBAC-3
Others