Jian | BAH-460 | add api for search similar patient using PersonService #29
Conversation
| @Override | ||
| public List<PatientResponse> searchSimilarPatients(PatientSearchParameters searchParameters, PatientResponseMapper patientResponseMapper) { | ||
| List<PatientResponse> patients = new ArrayList<PatientResponse>(); | ||
| Set<Person> persons = personService.getSimilarPeople(searchParameters.getName(), null, searchParameters.getGender()); |
There was a problem hiding this comment.
We need to actually use Lucene here. OpenMRS's personService.getSimilarPeople does a handcrafted SQL query which doesn't perform well against large databases: https://github.com/openmrs/openmrs-core/blob/2.1.3/api/src/main/java/org/openmrs/api/db/hibernate/HibernatePersonDAO.java#L82
(Also FYI not every Person is a Patient.)
| @@ -71,6 +73,14 @@ public void setName(String name) { | |||
| this.name = name; | |||
| } | |||
|
|
|||
| public String getGender() { | |||
There was a problem hiding this comment.
If we add this field here we also need to make sure it's respected by existing API methods that use PatientSearchParameters.
(It would be okay to skip the gender field when searching for similar patients, if your timeline to finish this is tight.)
| @@ -15,6 +17,8 @@ | |||
|
|
|||
| List<PatientResponse> luceneSearch(PatientSearchParameters searchParameters); | |||
|
|
|||
| List<PatientResponse> searchSimilarPatients(PatientSearchParameters searchParameters, PatientResponseMapper patientResponseMapper); | |||
There was a problem hiding this comment.
I think the input parameter to this should actually be PatientProfile (which is https://github.com/openmrs/openmrs-module-emrapi/blob/master/api/src/main/java/org/openmrs/module/emrapi/patient/PatientProfile.java)
The registration create patient actually posts to PatientProfileResource: https://github.com/Bahmni/openmrs-module-bahmniapps/blob/15d0175b14585c7bca47d63f62e61fbf8a412ceb/ui/app/registration/services/defaultPatientServiceStrategy.js#L41
This method should take the same type, so that the UI that is building up a PatientProfile to submit can just submit a partial version.
| @@ -83,6 +89,23 @@ public PatientConfigResponse getConfig() { | |||
| searchParameters.getFilterPatientsByLocation(), searchParameters.getFilterOnAllIdentifiers()); | |||
| } | |||
|
|
|||
| @Override | |||
| public List<PatientResponse> searchSimilarPatients(PatientSearchParameters searchParameters, PatientResponseMapper patientResponseMapper) { | |||
There was a problem hiding this comment.
If we're going to use PatientResponseMapper in this service we should autowire it in the constructor, not ask for it to be passed in by the caller.
| @@ -100,6 +105,66 @@ public PatientDaoImpl(SessionFactory sessionFactory) { | |||
| return patientResponses; | |||
| } | |||
|
|
|||
| @Override | |||
| public List<PatientResponse> getSimilarPatientsUsingLuceneSearch(String identifier, String name, String gender, String customAttribute, | |||
There was a problem hiding this comment.
This should really take some kind of search object (or a PatientProfile or BahmniPatient as I've suggested elsewhere) as its single parameter. As written here this seems like this method sign will need to be frequently modified in the future.
There was a problem hiding this comment.
Also, I'm surprised to see so much logic here, e.g. for program attributes. Is that required to address similar patient search when creating a new patient?
| @@ -83,6 +89,25 @@ public PatientConfigResponse getConfig() { | |||
| searchParameters.getFilterPatientsByLocation(), searchParameters.getFilterOnAllIdentifiers()); | |||
| } | |||
|
|
|||
| @Override | |||
| public List<PatientResponse> searchSimilarPatients(PatientSearchParameters searchParameters) { | |||
| return patientDao.getSimilarPatientsUsingLuceneSearch(searchParameters.getIdentifier(), | |||
There was a problem hiding this comment.
why not just pass PatientSearchParameters to the DAO?
| PatientResponse patient1 = patients.get(0); | ||
|
|
||
| for(PatientResponse response: patients) { | ||
| System.out.println(response.getGivenName() + " " + response.getMiddleName() + " " + response.getFamilyName()); |
| @@ -1,5 +1,7 @@ | |||
| package org.bahmni.module.bahmnicore.service.impl; | |||
|
|
|||
| import org.bahmni.module.bahmnicore.contract.patient.PatientSearchParameters; | |||
There was a problem hiding this comment.
presumably this change isn't necessary since there's no changes to the class itself
| && checkGender(personName.getPerson(), gender) | ||
| ).collect(toList()); | ||
| persons = persons.subList(0, Math.min(length, persons.size())); | ||
| persons.forEach(person -> patients.add(patientDAO.getPatient(person.getPerson().getPersonId()))); |
There was a problem hiding this comment.
@angshu @djazayeri we do not want to hit the database just to get the Patients from the Persons. We used the lucence index to get the PersonName's of similar Patients via personLuceneQuery.getPatientNameQueryWithOrParser, we can access the Person via personName.getPerson() but here are accessing the database to get Patients. Do you have a hint for us on how to get Patients without hitting the DB? Can we simply cast them?
cc @eyalgo (who is picking up on the work done by Jian with Martina)
There was a problem hiding this comment.
answering this late, I haven't ever used OpenMRS's lucene searching, so I don't know offhand how this works.
|
this PR can be closed as its replaced by #33 |
|
Jian seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
as indicated above by @teleivo - closing this PR |
No description provided.