-
Notifications
You must be signed in to change notification settings - Fork 228
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
BAH-273 #27
base: master
Are you sure you want to change the base?
BAH-273 #27
Changes from 18 commits
077b026
b368e4d
727a353
f028cd1
83ccc4c
bfe7cdf
c2805cc
964089b
3e344c9
8d7d20c
0cac042
ac2de0a
ac0eda1
dfeb59a
67d3db7
0cc1d8a
bf119fe
5e56b8d
6c6e16d
724d06a
d45ab53
0b9563e
62c1f5a
b1ab8fa
6798949
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,11 @@ | |
import org.openmrs.PatientIdentifier; | ||
import org.openmrs.PatientIdentifierType; | ||
import org.openmrs.RelationshipType; | ||
import org.openmrs.api.PatientService; | ||
import org.openmrs.api.context.Context; | ||
import org.openmrs.module.bahmniemrapi.visitlocation.BahmniVisitLocationServiceImpl; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.beans.factory.annotation.Qualifier; | ||
import org.springframework.stereotype.Repository; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
|
@@ -42,10 +44,12 @@ | |
public class PatientDaoImpl implements PatientDao { | ||
|
||
private SessionFactory sessionFactory; | ||
private PatientService patientService; | ||
|
||
@Autowired | ||
public PatientDaoImpl(SessionFactory sessionFactory) { | ||
public PatientDaoImpl(SessionFactory sessionFactory, PatientService patientService) { | ||
this.sessionFactory = sessionFactory; | ||
this.patientService = patientService; | ||
} | ||
|
||
@Override | ||
|
@@ -77,24 +81,35 @@ public List<PatientResponse> getPatientsUsingLuceneSearch(String identifier, Str | |
String programAttributeFieldName, String[] addressSearchResultFields, | ||
String[] patientSearchResultFields, String loginLocationUuid, | ||
Boolean filterPatientsByLocation, Boolean filterOnAllIdentifiers) { | ||
|
||
validateSearchParams(customAttributeFields, programAttributeFieldName, addressFieldName); | ||
|
||
List<PatientIdentifier> patientIdentifiers = getPatientIdentifiers(identifier, filterOnAllIdentifiers, offset, length); | ||
List<Integer> patientIds = patientIdentifiers.stream().map(patientIdentifier -> patientIdentifier.getPatient().getPatientId()).collect(toList()); | ||
Map<Object, Object> programAttributes = Context.getService(BahmniProgramWorkflowService.class).getPatientProgramAttributeByAttributeName(patientIds, programAttributeFieldName); | ||
Set<Patient> patients = new HashSet<>(); | ||
|
||
if(identifier != null && !identifier.isEmpty()){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer to use org.apache.commons.lang3.StringUtils.isNotEmpty to combine both these checks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this whole block can be refactored so as not to duplicate the code blocks about calling name and customAttribute search. |
||
patients.addAll(patientService.getPatients(identifier, false, offset, length)); | ||
if(name != null && !name.isEmpty()){ | ||
patients.retainAll( patientService.getPatients(name, false, offset, length)); | ||
} | ||
if(customAttribute != null && !customAttribute.isEmpty()){ | ||
patients.retainAll( patientService.getPatients(customAttribute, false, offset, length)); | ||
} | ||
}else if(name != null && !name.isEmpty()){ | ||
patients.addAll(patientService.getPatients(name, false, offset, length)); | ||
if(customAttribute!=null && !customAttribute.isEmpty()){ | ||
patients.retainAll(patientService.getPatients(customAttribute, false, offset, length)); | ||
} | ||
}else if((customAttribute != null && !customAttribute.isEmpty())){ | ||
patients.addAll(patientService.getPatients(customAttribute, false, offset, length)); | ||
} | ||
|
||
List<Integer> patientIds = patients.stream().map(Patient::getPatientId).collect(toList()); | ||
PatientResponseMapper patientResponseMapper = new PatientResponseMapper(Context.getVisitService(),new BahmniVisitLocationServiceImpl(Context.getLocationService())); | ||
Set<Integer> uniquePatientIds = new HashSet<>(); | ||
List<PatientResponse> patientResponses = patientIdentifiers.stream() | ||
.map(patientIdentifier -> { | ||
Patient patient = patientIdentifier.getPatient(); | ||
if(!uniquePatientIds.contains(patient.getPatientId())) { | ||
PatientResponse patientResponse = patientResponseMapper.map(patient, loginLocationUuid, patientSearchResultFields, addressSearchResultFields, | ||
programAttributes.get(patient.getPatientId())); | ||
uniquePatientIds.add(patient.getPatientId()); | ||
return patientResponse; | ||
}else | ||
return null; | ||
Map<Object, Object> programAttributes = Context.getService(BahmniProgramWorkflowService.class).getPatientProgramAttributeByAttributeName(patientIds, programAttributeFieldName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should autowire this service in the constructor instead of doing Context.getService |
||
|
||
List<PatientResponse> patientResponses = patients.stream() | ||
.map(patient -> { | ||
PatientResponse patientResponse = patientResponseMapper.map(patient,loginLocationUuid,patientSearchResultFields,addressSearchResultFields,programAttributes.get(patient.getPatientId())); | ||
return patientResponse; | ||
}).filter(Objects::nonNull) | ||
.collect(toList()); | ||
return patientResponses; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
package org.bahmni.module.bahmnicore.model; | ||
|
||
import org.hibernate.search.annotations.Indexed; | ||
|
||
import java.util.LinkedHashMap; | ||
|
||
@Indexed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this going to do anything here? This is not a hibernate managed class. |
||
public class BahmniAddress { | ||
|
||
private String address1; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,4 +18,6 @@ public interface BahmniPatientService { | |
public List<Patient> get(String partialIdentifier, boolean shouldMatchExactPatientId); | ||
|
||
public List<RelationshipType> getByAIsToB(String aIsToB); | ||
List<PatientResponse> luceneHibernateSearch(PatientSearchParameters searchParameters); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a very detailed Javadoc header for this one given the apparently confusing name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Provided java doc for the method in the service class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this method name need to say this. It's possible that the under-the-hood implementation does in fact use lucene and then hibernate, but is that actually part of the interface contract? Isn't this just "searchBy" or something like that? |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
package org.bahmni.module.bahmnicore.service.impl; | ||
|
||
import org.apache.commons.lang3.StringUtils; | ||
import org.bahmni.module.bahmnicore.contract.patient.PatientSearchParameters; | ||
import org.bahmni.module.bahmnicore.contract.patient.response.PatientConfigResponse; | ||
import org.bahmni.module.bahmnicore.contract.patient.response.PatientResponse; | ||
|
@@ -10,11 +11,14 @@ | |
import org.openmrs.PersonAttributeType; | ||
import org.openmrs.RelationshipType; | ||
import org.openmrs.api.ConceptService; | ||
import org.openmrs.api.PatientService; | ||
import org.openmrs.api.PersonService; | ||
import org.openmrs.api.context.Context; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.context.annotation.Lazy; | ||
import org.springframework.stereotype.Service; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
@Service | ||
|
@@ -23,13 +27,15 @@ public class BahmniPatientServiceImpl implements BahmniPatientService { | |
private PersonService personService; | ||
private ConceptService conceptService; | ||
private PatientDao patientDao; | ||
private PatientService patientService; | ||
|
||
@Autowired | ||
public BahmniPatientServiceImpl(PersonService personService, ConceptService conceptService, | ||
PatientDao patientDao) { | ||
PatientDao patientDao, PatientService patientService) { | ||
this.personService = personService; | ||
this.conceptService = conceptService; | ||
this.patientDao = patientDao; | ||
this.patientService = patientService; | ||
} | ||
|
||
@Override | ||
|
@@ -93,4 +99,30 @@ public List<RelationshipType> getByAIsToB(String aIsToB) { | |
return patientDao.getByAIsToB(aIsToB); | ||
} | ||
|
||
@Override | ||
public List<PatientResponse> luceneHibernateSearch(PatientSearchParameters searchParameters) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment: I find that the method naming here makes this very hard to understand. How would anyone possibly know what luceneSearch, lucenseHibernateSearch, and search are suppose to do, and which ones handle which parameters? |
||
List<PatientResponse> luceneHibernateResponse = new ArrayList<>(); | ||
if(searchParameters.getIdentifier() != null || searchParameters.getName() != null || searchParameters.getCustomAttribute() != null){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You will probably want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes you are right. Strings will be always empty if they contain nothing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If all arguments are empty strings, then we don't search for the patient objects There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If none of the first 3 parameters are missing and others like program attributes and address are provided, it will get to hibernate search and if nothing is supplied at all, then no search will be called |
||
List<PatientResponse> luceneResponse = luceneSearch(searchParameters); | ||
|
||
for(PatientResponse patientResponse: luceneResponse){ | ||
Patient patient = patientService.getPatient(patientResponse.getPersonId()); | ||
searchParameters.setIdentifier(patient.getPatientIdentifier().getIdentifier()); | ||
if(StringUtils.isNotEmpty(searchParameters.getName())){ | ||
searchParameters.setName(""); | ||
} | ||
if( StringUtils.isNotEmpty(searchParameters.getCustomAttribute())){ | ||
searchParameters.setCustomAttribute(""); | ||
} | ||
luceneHibernateResponse.addAll(search(searchParameters)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're firing a hibernate/sql search for every result that comes back from the lucene search? Are we guaranteed that the number of these searches will be small? |
||
} | ||
}else{ | ||
luceneHibernateResponse = search(searchParameters); | ||
} | ||
|
||
|
||
|
||
return luceneHibernateResponse; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please checkout this file from upstream/master, I suspect it should not have been changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked upstream and corrected