GSoC 2019: Patient search criteria added in openmrs core #2980
Conversation
| */ | ||
| public List<Patient> getPatients(String query, Date birthdate, Integer start, Integer length, Boolean includeVoided) | ||
| throws DAOException; | ||
|
|
There was a problem hiding this comment.
@Reyano132 There are eight getPatients methods with different input parameters. If you give them a proper name based on the input parameters that will improve the readability. Otherwise it will be difficult to figure out what is the difference between these methods.
There was a problem hiding this comment.
Hey @dilanthas, I changed the names of methods as per inputs
|
|
||
| return patients; | ||
| } | ||
|
|
There was a problem hiding this comment.
There are some code duplication in above methods. Except one line, the rest of the code is same in the above methods. For example the difference between getPatients starts from line 820, getPatients starts from line 847 is what LuceneQuery you are building. I think this can be improved
There was a problem hiding this comment.
Also, I removed code duplication.
| List<Patient> patients = findPatients(query, includeVoided, start, length); | ||
| List<Patient> result=new LinkedList<>(); | ||
| for (Patient p : patients) { | ||
| if (p.getGender().equals(gender)) { |
There was a problem hiding this comment.
This filtering can be done using java8 stream filtering. Example
findPatients(query, includeVoided, start, length).stream().filter(patient -> patient.getGender().equals(gender)).collect(Collectors.toList());
There was a problem hiding this comment.
thanks for the help, I use stream filter as you mention.
| String minChars = Context.getAdministrationService().getGlobalProperty(OpenmrsConstants.GLOBAL_PROPERTY_MIN_SEARCH_CHARACTERS); | ||
|
|
||
| if (minChars == null || !StringUtils.isNumeric(minChars)) { | ||
| minChars = "" + OpenmrsConstants.GLOBAL_PROPERTY_DEFAULT_MIN_SEARCH_CHARACTERS; |
There was a problem hiding this comment.
any reason to append "" in the beginning ?
There was a problem hiding this comment.
For casting int to string, although I removed that part
| } | ||
|
|
||
| @Override | ||
| public List<Patient> getPatients(String query, Date from, Date to, Integer start, Integer length, |
There was a problem hiding this comment.
Since this is a long method It would be better to add some comments in this method body by explaining whats happening .
| } | ||
|
|
||
| @Override | ||
| public List<Patient> getPatients(String query,Date birthdate, Integer start, Integer length, |
There was a problem hiding this comment.
this methods body is quite similar to the above getPatients method body and have lots of duplicate code.
| List<Patient> patients = getPatients(query, from, to, start, length, includeVoided); | ||
| List<Patient> result= new LinkedList<>(); | ||
| for (Patient p : patients) { | ||
| if (p.getGender().equals(gender)) { |
| List<Patient> patients = getPatients(query, birthdate, start, length, includeVoided); | ||
| List<Patient> result=new LinkedList<>(); | ||
| for (Patient p : patients) { | ||
| if (p.getGender().equals(gender)) { |
| if (namesSize > tmpStart) { | ||
| ListPart<Object[]> patientNames = birthdateQuery.listPartProjection(tmpStart, tmpLength, "personId"); | ||
| patientNames.getList().forEach(patientName -> patients.add(getPatient((Integer) patientName[0]))); | ||
| for(Patient p:patients) { |
| */ | ||
|
|
||
| @Override | ||
| public List<Patient> getPatients(String name, String identifier, List<PatientIdentifierType> identifierTypes, |
There was a problem hiding this comment.
I think this method should be break in to several other methods: With all these if and else conditions its really hard to read whats going on.
There was a problem hiding this comment.
I divide this method into two methods, now it becomes easy to understand
|
@Reyano132 did you see the above review comments? |
|
yes, @dkayiwa, I'm working on it |
|
@dkayiwa sir, can you please merge this PR. It's ready and I have to use this feature in core apps module |
|
@Reyano132 have your mentors reviewed and approved it? |
|
hi @Reyano132, this looks impressive at first glance, i need to test it as soon as i can. I have some suggestions but in a meantime, please move all these other changes except for ones about the SEARCH_INDEX_VERSION version, Person.java and Patient.java into our search criteria module (If they already exist in the module, then clear them off this PR). i have created https://github.com/openmrs/openmrs-module-patientsearchcriteria please after migrating these changes into your module, point git to the new and merge it into your local branch, open up one pull request from https://github.com/Reyano132/openmrs-module-patientsearchcriteria pointing to this new repository. I am also looking forward to seeing your functional UI changes. |
Reyano132
force-pushed
the
final
branch
4 times, most recently
from
63bb1f5 to
acf9b3a
Compare
|
@Reyano132 Have you seen the above comments for @k-joseph ? |
|
@kaweesi sir, As you said I changed this PR. Also, I create new PR for patientsearchcriteria, here : openmrs/openmrs-module-patientsearchcriteria#1 |
Reyano132
force-pushed
the
final
branch
3 times, most recently
from
7cd9ae7 to
ae486d1
Compare
| @@ -599,9 +599,9 @@ public static String getOpenmrsProperty(String property) { | |||
| /** | |||
| * Indicates the version of the search index. The index will be rebuilt, if the version changes. | |||
| * | |||
| * @since 1.11 | |||
There was a problem hiding this comment.
when we build the openmrs core, it will generate a person's Lucene index
|
@Reyano132 can you bring this again to the attention of your primary mentor? |
Reyano132
force-pushed
the
final
branch
3 times, most recently
from
16f9fab to
a69a648
Compare
ibacher
force-pushed
the
master
branch
2 times, most recently
from
e8f7850 to
1877e06
Compare
|
tl;dr our action detected no activity on this PR and will close it in 30 days if the stale label is not removed. OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare! This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side. If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :) |
|
tl;dr closing this PR since it has not seen any activity in the last 6 months. OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare! We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs. Your PR has not seen any activity in the last 6 months. This is why we have decided to close this PR for now. This allows us OpenMRS reviewers to focus our limited time to review all other PRs in a timely and professional manner. Please feel free to reassign yourself to the issue you worked on in our JIRA when you have time to focus on it. After that reopen a new PR and we will be glad to work with you to get your contribution merged. Thank you very much for your help and understanding :) |
The functionality of patient search criteria module added in openmrs core. This is part of my GSoC 2019 Project. https://wiki.openmrs.org/display/projects/Patient+Search+Criteria