PDMO-65: fix bug to enable retrieving soft deleted observations. #310

Ewanjiru · 2017-10-23T12:54:01Z

JIRA TICKET NAME:

RESTWS-683: [Enable retrieving soft deleted observations](https://issues.openmrs.org/browse/RESTWS-683

SUMMARY:

Fix bug in the webservice to enable retrieving soft deleted observations.

chibujax · 2017-10-23T13:02:08Z

...in/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ObsResource1_8.java

@@ -479,13 +481,19 @@ protected PageableResult doSearch(RequestContext context) {
 		}

 		String encounterUuid = context.getRequest().getParameter("encounter");
+		boolean includeVoided = Boolean.parseBoolean(context.getRequest().getParameter("includeAll"));


can you try context.getIncludeAll()

This has been corrected

chibujax · 2017-10-23T13:03:37Z

...in/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ObsResource1_8.java

 		if (encounterUuid != null) {
 			Encounter enc = ((EncounterResource1_8) Context.getService(RestService.class).getResourceBySupportedClass(
 			    Encounter.class)).getByUniqueId(encounterUuid);
 			if (enc == null)
 				return new EmptySearchResult();
-			List<Obs> obs = new ArrayList<Obs>(enc.getAllObs());
-			return new NeedsPaging<Obs>(obs, context);
+			if (includeVoided) {


Can you refactor this, in way you may not need to use the if since you have already acquired the value of includeVoided

This has been corrected

coveralls · 2017-10-23T13:08:44Z

Coverage decreased (-0.01%) to 40.322% when pulling 4b8d86e on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

chibujax · 2017-10-23T13:23:01Z

...in/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ObsResource1_8.java

@@ -48,6 +48,8 @@
 import org.openmrs.module.webservices.rest.web.response.IllegalRequestException;
 import org.openmrs.module.webservices.rest.web.response.ObjectNotFoundException;
 import org.openmrs.module.webservices.rest.web.response.ResponseException;
+import org.openmrs.module.webservices.rest.web.v1_0.resource.openmrs1_8.EncounterResource1_8;


Do we really need this?

chibujax · 2017-10-23T13:38:33Z

...in/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ObsResource1_8.java

@@ -48,6 +48,8 @@
 import org.openmrs.module.webservices.rest.web.response.IllegalRequestException;
 import org.openmrs.module.webservices.rest.web.response.ObjectNotFoundException;
 import org.openmrs.module.webservices.rest.web.response.ResponseException;
+import org.openmrs.module.webservices.rest.web.v1_0.resource.openmrs1_8.EncounterResource1_8;
+import org.openmrs.module.webservices.rest.web.v1_0.resource.openmrs1_8.PatientResource1_8;


Do we need this?

These were mistakenly imported and have been removed.

adamsdenniskariuki · 2017-10-23T14:49:32Z

Good work @Ewanjiru

chibujax · 2017-10-23T14:51:56Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ObsController1_9Test.java

+		MockHttpServletResponse allObsIncludingVoidedResponse = handle(getAllObsIncludingVoidedRequest);
+		List<Object> allObsIncludingVoidedList = deserialize(allObsIncludingVoidedResponse).get("results");
+
+		assertEquals(allObsIncludingVoidedList.size(), 6);


The test does not assert that voided obs were retrieved, because AllNonVoidedObsList has the same size with allObsIncludingVoidedList. Can you fix that?

By this I was confirming that the length is the same since before the fix it would only retrieve the non-voided observations hence length would be less but with above test it confirms that even after the delete request the length is still the same.

Ok, just assert that AllNonVoidedObsList is now 5.

Oh, well noted

@adamsdenniskariuki , @andela-cuchendu I have added the assert.
Thanks for the review.

coveralls · 2017-10-23T14:54:06Z

Coverage decreased (-0.003%) to 40.333% when pulling d9c493f on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

coveralls · 2017-10-23T18:38:54Z

Coverage decreased (-0.003%) to 40.333% when pulling acebb94 on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

dkayiwa · 2017-10-23T21:37:28Z

...in/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ObsResource1_8.java

@@ -479,12 +479,14 @@ protected PageableResult doSearch(RequestContext context) {
 		}

 		String encounterUuid = context.getRequest().getParameter("encounter");
+		boolean includeVoided = context.getIncludeAll();


Do you really need the above local variable?

dkayiwa · 2017-10-23T21:40:44Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ObsController1_9Test.java

+
+		MockHttpServletRequest getAllNonVoidedObsRequest2 = newGetRequest(getURI());
+		getAllNonVoidedObsRequest2.addParameter("encounter", "62967e68-96bb-11e0-8d6b-9b9415a91465");
+		MockHttpServletResponse AllNonVoidedObsResponse2 = handle(getAllNonVoidedObsRequest2);


Did you look at our conventions for variable names? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-NamingConventions
The same applies to the above.

dkayiwa · 2017-10-23T21:41:13Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ObsController1_9Test.java

+
+		assertEquals(allObsIncludingVoidedList.size(), 6);
+
+		MockHttpServletRequest getAllNonVoidedObsRequest2 = newGetRequest(getURI());


Is getAllNonVoidedObsRequest2 a function or variable?

dkayiwa · 2017-10-23T21:42:09Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ObsController1_9Test.java

+
+		executeDataSet("encounterWithObsGroup1_9.xml");
+
+		MockHttpServletRequest getAllNonVoidedObsRequest = newGetRequest(getURI());


Is getAllNonVoidedObsRequest a function or variable?
The same applies to the others.

Ewanjiru · 2017-10-24T06:01:37Z

@dkayiwa
Feedback has been implemented. Thanks.

coveralls · 2017-10-24T06:10:30Z

Coverage decreased (-0.003%) to 40.333% when pulling bb5b947 on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

coveralls · 2017-10-24T06:12:23Z

Coverage remained the same at 40.336% when pulling bb5b947 on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

dkayiwa · 2017-10-24T08:01:18Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ObsController1_9Test.java

@@ -84,6 +87,38 @@ public void shouldGetAll() throws Exception {
 	}

 	@Test
+	public void shouldGetallIncludingVoidedObs() throws Exception {


The test method name does not clearly state what you are testing. Can you look at this for an example? 191e0b6

@dkayiwa
Working on it.

This has been corrected.

dkayiwa · 2017-10-24T08:04:13Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ObsController1_9Test.java

+		handle(deleteRequest);
+
+		MockHttpServletRequest allObsIncludingVoidedRequest = newGetRequest(getURI());
+		allObsIncludingVoidedRequest.addParameter("encounter", "62967e68-96bb-11e0-8d6b-9b9415a91465");


Can you avoid using this uuid from more than one place 62967e68-96bb-11e0-8d6b-9b9415a91465?

@dkayiwa
I am not sure I follow what you mean since the uuids above are different;
62967e68-96bb-11e0-8d6b-9b9415a91465 is the encounter uuid
11de743c-96cd-11e0-8d6b-9b9415a91465 is the observation uuid

Let me put it this way, are you aware of the fact that you are using 62967e68-96bb-11e0-8d6b-9b9415a91465 in more than one place?

This has been corrected by assigning the uuid to a variable.

coveralls · 2017-10-24T08:59:58Z

Coverage remained the same at 40.336% when pulling 00bd3b1 on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

dkayiwa · 2017-10-24T10:05:37Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ObsController1_9Test.java

+
+		executeDataSet("encounterWithObsGroup1_9.xml");
+
+		String encounterUuid = "62967e68-96bb-11e0-8d6b-9b9415a91465";


Have you looked at our naming conventions in regards to the above line? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-NamingConventions

@dkayiwa
This has been fixed.

How have you fixed it?

coveralls · 2017-10-24T11:04:57Z

Coverage remained the same at 40.336% when pulling c978e25 on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

coveralls · 2017-10-24T11:14:09Z

Coverage remained the same at 40.336% when pulling c978e25 on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

Ewanjiru · 2017-10-24T12:24:15Z

@dkayiwa
I made some commit mishaps.
I have fixed by having the string as a constant.

coveralls · 2017-10-24T12:34:16Z

Coverage remained the same at 40.336% when pulling 9ff22e7 on Ewanjiru:PDMO-65 into 191e0b6 on openmrs:master.

chibujax reviewed Oct 23, 2017

View reviewed changes

Ewanjiru force-pushed the PDMO-65 branch 2 times, most recently from 43b945b to d9c493f Compare October 23, 2017 14:32

chibujax reviewed Oct 23, 2017

View reviewed changes

Ewanjiru force-pushed the PDMO-65 branch from 394bf8e to acebb94 Compare October 23, 2017 18:14

dkayiwa reviewed Oct 23, 2017

View reviewed changes

Ewanjiru force-pushed the PDMO-65 branch 2 times, most recently from d882b3a to bb5b947 Compare October 24, 2017 06:00

dkayiwa reviewed Oct 24, 2017

View reviewed changes

Ewanjiru force-pushed the PDMO-65 branch from 1b102e4 to 00bd3b1 Compare October 24, 2017 08:47

dkayiwa reviewed Oct 24, 2017

View reviewed changes

Ewanjiru force-pushed the PDMO-65 branch 2 times, most recently from 16a798b to c978e25 Compare October 24, 2017 11:03

[PDMO-65] fix bug to enable retrieving soft deleted observations

9ff22e7

Ewanjiru force-pushed the PDMO-65 branch from c0bb083 to 9ff22e7 Compare October 24, 2017 12:22

dkayiwa merged commit cf06f31 into openmrs:master Oct 24, 2017


		assertEquals(allObsIncludingVoidedList.size(), 6);

		MockHttpServletRequest getAllNonVoidedObsRequest2 = newGetRequest(getURI());


		executeDataSet("encounterWithObsGroup1_9.xml");

		MockHttpServletRequest getAllNonVoidedObsRequest = newGetRequest(getURI());


		executeDataSet("encounterWithObsGroup1_9.xml");

		String encounterUuid = "62967e68-96bb-11e0-8d6b-9b9415a91465";

PDMO-65: fix bug to enable retrieving soft deleted observations. #310

PDMO-65: fix bug to enable retrieving soft deleted observations. #310

Conversation

Ewanjiru commented Oct 23, 2017 • edited by dkayiwa Loading

JIRA TICKET NAME:

SUMMARY:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsdenniskariuki commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 23, 2017

coveralls commented Oct 23, 2017

Choose a reason for hiding this comment

dkayiwa Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

dkayiwa Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ewanjiru commented Oct 24, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

Ewanjiru commented Oct 24, 2017

coveralls commented Oct 24, 2017

Ewanjiru commented Oct 23, 2017 •

edited by dkayiwa

Loading

dkayiwa Oct 23, 2017 •

edited

Loading

dkayiwa Oct 23, 2017 •

edited

Loading