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

BAH-460 Add search similar patients API [WIP] #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eyalgo
Copy link

@eyalgo eyalgo commented May 16, 2018

  • Add API for search similar patients in BahmniPatientService
  • Add API for similar patients in PatientDao
  • The search is done using Lucene

@eyalgo
Copy link
Author

eyalgo commented May 16, 2018

There was previously a discussion to create a special class (or use existing one) instead of sending parameters.
We didn't add it now, as we preferred to do the minimal necessary code changes.
Not to add code "in case we need it in the future".

@teleivo
Copy link

teleivo commented May 17, 2018

@eyalgo please merge the commits into one (we do not need the commits in between as the state was not clean)

@eyalgo eyalgo force-pushed the master branch 2 times, most recently from 5a04180 to 114c540 Compare May 17, 2018 16:03
@eyalgo
Copy link
Author

eyalgo commented May 17, 2018

@angshu @djazayeri
hi,
can you please review the PR (I think I don't have permission to add reviewers).

Thanks !

/cc @teleivo , @mduemcke

Copy link

@djazayeri djazayeri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -19,6 +20,12 @@
String programAttributeFieldName, String[] addressSearchResultFields,
String[] patientSearchResultFields, String loginLocationUuid, Boolean filterPatientsByLocation, Boolean filterOnAllIdentifiers);

List<PatientResponse> getSimilarPatientsUsingLuceneSearch(String identifer, String name, String gender, String customAttribute,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really have to add another method here? The DAO isn't an external-facing interface so we should just be able to add a parameter to the above, right?

@@ -67,4 +69,5 @@ public void shouldGetPatientByPartialIdentifier() throws Exception {
bahmniPatientService.get("partial_identifier", shouldMatchExactPatientId);
verify(patientDao).getPatients("partial_identifier", shouldMatchExactPatientId);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spurious change

@Override
public List<PatientResponse> getSimilarPatientsUsingLuceneSearch(String identifier, String name, String gender, String customAttribute,
String addressFieldName, String addressFieldValue, Integer length,
Integer offset, String[] customAttributeFields, String programAttributeFieldValue,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look closely but I think you're ignoring the length and offset parameters.

@@ -71,6 +73,14 @@ public void setName(String name) {
this.name = name;
}

public String getGender() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djazayeri what do you mean by respected?

PatientResponse patient1 = patients.get(0);

for(PatientResponse response: patients) {
System.out.println(response.getGivenName() + " " + response.getMiddleName() + " " + response.getFamilyName());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this before merging.


@RequestMapping(value="similar", method = RequestMethod.GET)
@ResponseBody
public ResponseEntity<AlreadyPaged<PatientResponse>> searchSimilarPerson(HttpServletRequest request,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that this is the right place to add this search (and that would have a big impact on the rest of the code).

See this comment: https://bahmni.atlassian.net/browse/BAH-460?focusedCommentId=12001&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-12001

This code needs to be called from the Register New Patient screen, asking the question "are there any similar patients to this one that I'm about to create?" Therefor I would expect that the client is allowed to submit an incomplete version of the patient creation object, instead of needing to construct a search that's in a different format.

If you prefer not to do it that for some reason, please give a justification.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Darius, I might be missing some context (just joined the team) but I thought that this search needs to be called as the user inputs first name / last name, and wouldn't require them to hit the submit button before checking for matches.
Did I misunderstand your comment? Do you still think we should change this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djazayeri cc @mduemcke

Therefor I would expect that the client is allowed to submit an incomplete version of the patient creation object, instead of needing to construct a search that's in a different format

Hi Darius, I'm trying to figure this out (I've also just joined and took over from Martina). If we were to send an incomplete patient representation to the, say BahmniPatientProfileResource we'd still have to construct input from that representation in order to be able to search for the potentially similar patient using Lucene. As far as I understood it was suggested/agreed upon that Lucene is to be utilized for this search. Ultimately we don't want to create new patients from the lookup, but just aid the creation process, trying to avoid creation of duplicates, no?

Thus, wouldn't that mean adding a responsibility to BahmniPatientProfileResource which it shouldn't have? As is, the responsibilities of patient creation and searching seem more properly separated.

Though a different approach just came to mind while writing this — when creating a new patient object from the given, incomplete data, said object could obtain a new functionality searchSimilar() and conduct a Lucene search from there, e.g. something along BahmniPatient::searchSimilar()... was that what you're after? I'd probably still would want to keep that in the search controller.

Let me know what you think (or had in mind).. thanks!

@eyalgo eyalgo force-pushed the master branch 2 times, most recently from 767f916 to 5a04180 Compare May 22, 2018 10:00
@eyalgo eyalgo changed the title BAH-460 Add search similar patients API BAH-460 Add search similar patients API [WIP] May 22, 2018

import static java.util.stream.Collectors.reducing;

import java.util.*;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please dont use star imports

String query = LuceneQuery.escapeQuery(name);
PersonLuceneQuery personLuceneQuery = new PersonLuceneQuery(sessionFactory);
LuceneQuery<PersonName> nameQuery = personLuceneQuery.getPatientNameQueryWithOrParser(query, false);
/* person.gender does not work somehow in LuceneQuery, so the dirty way is to filter result with person's gender */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this comment, we should explain in the JIRA ticket that we decided to go this route as to not have to adapt the openmrs lucene index (since this is where gender would need to be included in the index)

@Test
public void shouldSearchSimilarPatientByPatientName() {
List<PatientResponse> patients = patientDao.getSimilarPatientsUsingLuceneSearch("Peet", "", "c36006e5-9fbb-4f20-866b-0ece245615a1", 5);
PatientResponse patient1 = patients.get(0);
Copy link

@teleivo teleivo May 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these 2 lines should move below the assertEquals(2, patients.size()), so we get a clean error message in case the patients returned are empty or only of length 1

import static org.mockito.Mockito.anyInt;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.*;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not use star imports, please replace

assertEquals(patient1.getFamilyName(), "Sinha");
}

//TODO missing tests: by limit, parameters verification
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the TODO/replace it with missing tests.
you tested limit in shouldSearchSimilarPatientByPatientNameAndUseLimitResult

what do you want to test in parameters verification ?

patientSearchParameters.setName("John");
patientSearchParameters.setGender("M");
patientSearchParameters.setLoginLocationUuid("someUUid");

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove newline

@mduemcke
Copy link

mduemcke commented Jun 8, 2018

amended the pull request - still work in progress! Going to hand over to next pair

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2019

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants