RestWS-950-Encounter Search Handler should support a visits parameter by sadamhussain-m · Pull Request #616 · openmrs/openmrs-module-webservices.rest

sadamhussain-m · 2024-08-02T21:35:51Z

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

mogoodrich

Thanks @sadamhussain-m , generally looks good, but see my comments... there seem to be changes from other recent commits in your PR, and there is one place where I believe we need a null chek.

mogoodrich · 2024-08-05T16:25:56Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/PatientIdentifierResource1_8.java

 			delegate.getPatient().addIdentifier(delegate);
 		}
-
-		service().savePatientIdentifier(delegate);


This looks unrelated, I think you are modifying another recent commit unintentionally?

I don't know how other commits are tagged up on this issue branch.Before I'm raising the PR,I tried to skip the other unrelated commits.But I can't find a way for it.Could you help me on this?.

mogoodrich · 2024-08-05T16:27:02Z

.../openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_9/CustomDatatypeResource1_9.java

 	private List<CustomDatatypeRepresentation> getAllCustomDatatypes() {
+		if(!Context.isAuthenticated()) {
+			throw new APIAuthenticationException("User must be authenticated!");
+		}


Same here, this looks unrelated, I think you are modifying another recent commit unintentionally?

mogoodrich · 2024-08-05T16:27:17Z

...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java

 	}
+
+	@Test
+	public void shouldNotAddIdentifierInUseByAnotherPatient() throws Exception {


Same here as well, this looks unrelated, I think you are modifying another recent commit unintentionally?

mogoodrich · 2024-08-05T16:27:57Z

...rg/openmrs/module/webservices/rest/web/v1_0/search/openmrs2_0/EncounterSearchHandler2_0.java

-import java.util.Collections;
-import java.util.Date;
-import java.util.List;
+import java.util.*;


As a convention, we don't use wildcard imports. You made need to configure your IDE not to automatically substitute with wildcard imports?

I'll rectify this.

mogoodrich · 2024-08-05T16:28:40Z

...rg/openmrs/module/webservices/rest/web/v1_0/search/openmrs2_0/EncounterSearchHandler2_0.java

 		String patientUuid = context.getRequest().getParameter("patient");
 		String encounterTypeUuid = context.getRequest().getParameter("encounterType");
-
+		String[] visitsUuid=context.getRequest().getParameter("visits").split(",");


We will need to do a null check here, as visits is not a required parameter.

sure,I'll add it up.

mseaton · 2024-08-05T16:18:03Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/PatientIdentifierResource1_8.java

 		}
-
-		service().savePatientIdentifier(delegate);
+


This seems unrelated to this ticket, was this done to support a different issue?

I don't know how the unrelated commits are tagged up on this issue branch.

mseaton · 2024-08-05T16:18:08Z

.../openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_9/CustomDatatypeResource1_9.java

 	private List<CustomDatatypeRepresentation> getAllCustomDatatypes() {
+		if(!Context.isAuthenticated()) {
+			throw new APIAuthenticationException("User must be authenticated!");
+		}


This seems unrelated to this ticket, was this done to support a different issue?

mseaton · 2024-08-05T16:18:17Z

...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java


 		assertNotNull(PropertyUtils.getProperty(result, "auditInfo"));
 	}
+


This seems unrelated to this ticket, was this done to support a different issue?

mseaton · 2024-08-05T16:19:03Z

...rg/openmrs/module/webservices/rest/web/v1_0/search/openmrs2_0/EncounterSearchHandler2_0.java

 		String patientUuid = context.getRequest().getParameter("patient");
 		String encounterTypeUuid = context.getRequest().getParameter("encounterType");
-
+		String[] visitsUuid=context.getRequest().getParameter("visits").split(",");


The name of this parameter should be singular visit, not plural visits.

Sorry, @sadamhussain-m my bad, I think I said to use "visits", but let's go with "visit"

okay fine,I'll modify it.

mseaton · 2024-08-05T16:37:14Z

...rg/openmrs/module/webservices/rest/web/v1_0/search/openmrs2_0/EncounterSearchHandler2_0.java

 		String patientUuid = context.getRequest().getParameter("patient");
 		String encounterTypeUuid = context.getRequest().getParameter("encounterType");
-
+		String[] visitsUuid=context.getRequest().getParameter("visits").split(",");


Better to use existing method: String[] visitUuids = context.getRequest().getParameterValues("visit");

okay,I'll use this.

mseaton · 2024-08-05T16:37:52Z

...rg/openmrs/module/webservices/rest/web/v1_0/search/openmrs2_0/EncounterSearchHandler2_0.java

-
+			if (!visits.isEmpty()){
+				encounterSearchCriteriaBuilder.setVisits(visits);
+			}


if (visitUuids != null && visitUuids.length > 0) { List<Visit> visits = new ArrayList<>(); for (String visitUuid : visitUuids) { visits.add(Context.getVisitService().getVisitByUuid(visitUuid)); } encounterSearchCriteriaBuilder.setVisits(visits); }

mseaton · 2024-08-05T16:39:17Z

...rg/openmrs/module/webservices/rest/web/v1_0/search/openmrs2_0/EncounterSearchHandler2_0.java

+				Visit visit = ((VisitResource1_9) Context.getService(RestService.class).getResourceBySupportedClass(Visit.class)).getByUniqueId(uuid);
+				visits.add(visit);
+			}
+		}


Might as well wait to do all this once you are actually doing the query below, rather than pre-emptively, since patient is current required. If we want to make patient no longer required, and query on visit only, then we need to make sure the patient null check does not wrap everything. See suggested code below.

Okay.I'll add up those validations for visits before the query itself.

mseaton · 2024-08-05T16:40:05Z

...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java

+		assertEquals(patientIdentifierUuidThatShouldNotChange, uuid);
+		assertEquals(patientIdentifierNewValue, identifierValue);
+		assertEquals(patientIdentifierNewValue, patientIdentifier.getIdentifier());
+	}


We do need to add a unit test for this feature, which should be placed in the `EncounterController2_0Test.java" class, which is where the existing tests of the Encounter 2.0 search functionality live.

can you give me a test data for the visits as input and the expected output for the encounters for a patient to check for it?

mogoodrich

Looks like you were merging against the wrong branch, Sadam, but @ibacher was able fix it, thanks @ibacher ... I am no longer seeing the changes to the other files. If you can address the comments @mseaton and I made we should be good to merge, thanks!

mogoodrich · 2024-08-05T21:40:24Z

...rg/openmrs/module/webservices/rest/web/v1_0/search/openmrs2_0/EncounterSearchHandler2_0.java

 		String patientUuid = context.getRequest().getParameter("patient");
 		String encounterTypeUuid = context.getRequest().getParameter("encounterType");
-
+		String[] visitsUuid=context.getRequest().getParameter("visits").split(",");


Sorry, @sadamhussain-m my bad, I think I said to use "visits", but let's go with "visit"

mogoodrich · 2024-08-08T18:56:56Z

@sadamhussain-m thanks so much for working on this! I'm actually going to go ahead and merge this in, as we have need for it right now for a feature we are building. I will handle the changes @mseaton and I suggested and tag you on them, thanks!

RESTWS-950:Encounter search handler supporting visits parameter

f263bf1

mseaton requested review from ibacher, mogoodrich and mseaton August 5, 2024 16:16

mogoodrich requested changes Aug 5, 2024

View reviewed changes

mseaton requested changes Aug 5, 2024

View reviewed changes

ibacher changed the base branch from RESTWS-950-Encounter-Search-Handler-should-support-a-visits-parameter to master August 5, 2024 17:54

mogoodrich requested changes Aug 5, 2024

View reviewed changes

mogoodrich merged commit a0c09b2 into openmrs:master Aug 8, 2024


		assertNotNull(PropertyUtils.getProperty(result, "auditInfo"));
		}

Conversation

sadamhussain-m commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

mogoodrich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogoodrich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogoodrich commented Aug 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sadamhussain-m commented Aug 2, 2024 •

edited

Loading