[Bug]Failure to change preferred name, or identifier using a POST req… #305

chibujax · 2017-09-29T10:45:54Z

JIRA TICKET NAME:

RESTWS-677: Failure to change preferred name, or identifier using a POST request to the REST API

SUMMARY:

Fixes the Failure to change preferred name, or identifier using a POST request to the REST API

chibujax · 2017-09-29T10:47:30Z

This is what I came up with, but fails test with "'Patient#2' failed to validate with reason: error.preferredIdentifier" I have not written the unit test for this, but for you to guide.

dkayiwa · 2017-09-29T11:31:37Z

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

-		return service().savePatientIdentifier(delegate);
+		}
+		service().savePatientIdentifier(delegate);
+		//service().savePatient(delegate.getPatient());


What is the use of the above commented out line?

My bad, will remove

dkayiwa · 2017-09-29T11:33:53Z

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

+			for (PatientIdentifier pi : delegate.getPatient().getActiveIdentifiers()) {
+				if (!pi.equals(delegate)) {
+					pi.setPreferred(false);
+				}


The changes you have in PatientIdentifierResource1_8.java look more than the ones for PersonAddressResource1_8.java in this commit d23b7dd
Can you please do exactly the changes like Darius committed?

ok. Thank you. !Great tip

dkayiwa · 2017-09-29T11:34:30Z

You also need to add a test for these changes like Darius did for the commit you are copying from.

chibujax · 2017-09-29T16:26:54Z

I have updated but still with error: 'Patient#2' failed to validate with reason: error.preferredIdentifier and this is caused by line 230 to 236. Please how may I proceed?

dkayiwa · 2017-10-01T12:43:40Z

@andela-cuchendu replace service.savePatient() with service().savePatientIdentifier(delegate)

chibujax · 2017-10-03T09:29:21Z

Changes made, two tests added, tests passing 😄

dkayiwa · 2017-10-03T09:38:14Z

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

+
+		String preferredUuid = (String) PropertyUtils.getProperty(newPatientIdentifier, "uuid");
+		PatientIdentifier preferredIdentifer = service.getPatientIdentifierByUuid(preferredUuid);
+		System.out.print("none prefered is " + nonPreferred.isPreferred());


Have you looked at this? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotprintout

coveralls · 2017-10-03T09:40:29Z

Coverage decreased (-0.02%) to 40.381% when pulling 8600a98 on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

chibujax · 2017-10-03T09:41:01Z

I shot myself on the leg, corrected 😄

dkayiwa · 2017-10-03T09:44:58Z

Did you use an AK-47? 🔫

chibujax · 2017-10-03T09:51:49Z

No. With my action 😄 Is there any more work to be done on this? 😄

coveralls · 2017-10-03T09:52:22Z

Coverage decreased (-0.02%) to 40.381% when pulling 8600a98 on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

dkayiwa · 2017-10-03T10:10:15Z

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

+		Boolean originalPreferredStatus = patientIdentifier.getPreferred();
+		Boolean postPreferredStatus = !originalPreferredStatus;
+		SimpleObject postPatientIdentifier = new SimpleObject();
+		postPatientIdentifier.add("preferred", postPreferredStatus);


Does this test fail before your changes?

There was no test for editing preferred. So I added it to test if it could edit preferred

In other words, this test is not related to the ticket. Correct?

dkayiwa · 2017-10-04T09:24:33Z

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

+	public void shouldEditAPatientIdentifierWithPreferredAttibuite() throws Exception {
+		PatientIdentifier patientIdentifier = service.getPatientIdentifierByUuid(getUuid());
+		Boolean originalPreferredStatus = patientIdentifier.getPreferred();
+		Boolean postPreferredStatus = !originalPreferredStatus;


What is the use of the above variable? postPreferredStatus

The identifier to be changed. I guess I should change the name

dkayiwa · 2017-10-04T09:26:59Z

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

+		MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
+		req.setContent(json.getBytes());
+		handle(req);
+		assertNotEquals(originalPreferredStatus, patientIdentifier.getPreferred());


After posting, how do you confirm that the preferred was persisted?

assertNotEquals(originalPreferredStatus, patientIdentifier.getPreferred()); Assert that the new value is not the same with old one.

I toggled the value on line 120 newPreferredStatus = !originalPreferredStatus;

So assuming patientIdentifier.getPreferred() is TRUE, then posting with toggled value will make it FALSE. then I will assert that the new value which is now FALSE is no longer the original value(TRUE)

coveralls · 2017-10-04T09:41:00Z

Coverage decreased (-0.02%) to 40.381% when pulling 5986a30 on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

chibujax · 2017-10-04T10:07:43Z

I have removed the unwanted test. Also, I have tested on the app and postman locally.

dkayiwa · 2017-10-04T10:10:10Z

The test is still useful. Though you would need to create a new ticket and hence a separate pull request for it. Does it make sense?

chibujax · 2017-10-04T10:14:53Z

Ok I will. Is there anything more to be done on this?

coveralls · 2017-10-04T10:18:58Z

Coverage decreased (-0.02%) to 40.381% when pulling 3c489de on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

dkayiwa · 2017-10-04T10:24:46Z

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

+
+		String preferredUuid = (String) PropertyUtils.getProperty(newPatientIdentifier, "uuid");
+		PatientIdentifier preferredIdentifer = service.getPatientIdentifierByUuid(preferredUuid);
+		assertEquals(nonPreferred.isPreferred(), false);


How sure are you that nonPreferred.isPreferred() did not have a value of false even before the POST rest call?

Because, since I have new preferred(preferredIdentifer), the nonPreferred must become false. This is the fix I worked on, setting others false when we have new preferred.

If you say that it must become false, how sure are you that it was true?

I have added assertTrue(nonPreferred.isPreferred()); to prove that nonPreferred.isPreferred() == false.

dkayiwa · 2017-10-04T10:25:50Z

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

+		String preferredUuid = (String) PropertyUtils.getProperty(newPatientIdentifier, "uuid");
+		PatientIdentifier preferredIdentifer = service.getPatientIdentifierByUuid(preferredUuid);
+		assertEquals(nonPreferred.isPreferred(), false);
+		assertEquals(preferredIdentifer.isPreferred(), true);


Also instead of assertEquals, assertTrue or assertFalse is clearer.

chibujax · 2017-10-04T10:47:07Z

I have added assertTrue(nonPreferred.isPreferred()); before the call to prove that nonPreferred.isPreferred() == true and I have used assertTrue and assertFalse instead of assertEquals. I have also proved the outcome. 😄

chibujax · 2017-10-04T10:49:52Z

So line 121 which is assertTrue(nonPreferred.isPreferred()); proves that nonPreferred was the preferred identifier before the call. Line 140 to 141 (assertFalse(nonPreferred.isPreferred()); and assertTrue(preferredIdentifer.isPreferred());) Proves that nonPrefered after the call is now false and the preferredIdentifer is True

coveralls · 2017-10-04T10:55:30Z

Coverage decreased (-0.02%) to 40.381% when pulling d41bf40 on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

dkayiwa · 2017-10-04T11:38:48Z

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

+		assertNotNull(PropertyUtils.getProperty(newPatientIdentifier, "uuid"));
+		assertEquals(originalCount + 1, getAllCount());
+
+		String preferredUuid = (String) PropertyUtils.getProperty(newPatientIdentifier, "uuid");


Isn't this the same call being repeated as online 135?

On line 135, I just assert that the property is not null, then on 138, I only got the uuid (which I made sure in 135 that it is not null), used the uuid on line 139 to get the preferredIdentifier. So I don't think its a repeated call

Are you sure PropertyUtils.getProperty(newPatientIdentifier, "uuid") is not repeated?

chibujax · 2017-10-04T11:47:07Z

Changed

dkayiwa · 2017-10-04T11:54:00Z

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

+		assertNotNull(uuid);
+		assertEquals(originalCount + 1, getAllCount());
+
+		String preferredUuid = (String) uuid;


No need of the above variable. You could just service.getPatientIdentifierByUuid(uuid.toString())

chibujax · 2017-10-04T11:56:00Z

Ok, fixed. thank you 😄

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

Coverage decreased (-0.02%) to 40.381% when pulling 861ff8a on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

dkayiwa · 2017-10-04T11:59:06Z

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

+		assertEquals(originalCount + 1, getAllCount());
+
+		PatientIdentifier preferredIdentifer = service.getPatientIdentifierByUuid(uuid.toString());
+		assertFalse(nonPreferred.isPreferred());


Can you add another assertion or two to confirm that this is the one you have just posted?

I have added assertEquals(preferredIdentifer.getIdentifier(), "abc123ez"); which was set on line 123. I guess this confirms it.

coveralls · 2017-10-04T12:00:30Z

Coverage decreased (-0.02%) to 40.381% when pulling 861ff8a on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

coveralls · 2017-10-04T12:07:17Z

Coverage decreased (-0.02%) to 40.381% when pulling 861ff8a on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

chibujax · 2017-10-04T12:13:21Z

I have added assertEquals(preferredIdentifer.getIdentifier(), "abc123ez"); which was set on line 123. I guess this confirms it.

coveralls · 2017-10-04T12:24:02Z

Coverage decreased (-0.02%) to 40.381% when pulling f84b5e9 on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

dkayiwa · 2017-10-04T12:25:59Z

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

@@ -114,6 +115,33 @@ public void shouldEditAPatientIdentifier() throws Exception {
 	}

 	@Test
+	public void shouldUnsetOtherPreferredIdentifiers() throws Exception {
+		PatientIdentifier nonPreferred = service.getPatientIdentifierByUuid(getUuid());


The nonPreferred variable name is a bit confusing. Can you think of better name?
For instance, look at this on line 121 assertTrue(nonPreferred.isPreferred());
In other words, how can non preferred be expected to be preferred? 😊
Do you agree?

Yes, makes sense. Changed to exisitingIdentifer

chibujax · 2017-10-04T12:37:43Z

Yes, makes sense. Changed to exisitingIdentifer

dkayiwa · 2017-10-04T12:39:40Z

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

+		assertNotNull(uuid);
+		assertEquals(originalCount + 1, getAllCount());
+
+		PatientIdentifier preferredIdentifer = service.getPatientIdentifierByUuid(uuid.toString());


Then in that case preferredIdentifer becomes newIdentifer

…uest to the REST API

chibujax · 2017-10-04T12:43:02Z

Ok, done.

coveralls · 2017-10-04T12:49:09Z

Coverage decreased (-0.02%) to 40.381% when pulling eeb1749 on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

coveralls · 2017-10-04T12:56:26Z

Coverage decreased (-0.02%) to 40.381% when pulling eeb1749 on andela-cuchendu:RESTWS-677 into f0711b8 on openmrs:master.

dkayiwa reviewed Sep 29, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch 2 times, most recently from 786a511 to 671863d Compare September 29, 2017 16:22

chibujax force-pushed the RESTWS-677 branch from 671863d to 620b85e Compare October 3, 2017 09:28

dkayiwa reviewed Oct 3, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch from 620b85e to 8600a98 Compare October 3, 2017 09:40

dkayiwa reviewed Oct 3, 2017

View reviewed changes

dkayiwa reviewed Oct 4, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch from 8600a98 to 5986a30 Compare October 4, 2017 09:28

chibujax force-pushed the RESTWS-677 branch from 5986a30 to 3c489de Compare October 4, 2017 10:06

dkayiwa reviewed Oct 4, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch from 3c489de to d41bf40 Compare October 4, 2017 10:43

dkayiwa reviewed Oct 4, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch from d41bf40 to 7228cc2 Compare October 4, 2017 11:46

chibujax force-pushed the RESTWS-677 branch from 7228cc2 to db9f8cf Compare October 4, 2017 11:48

dkayiwa reviewed Oct 4, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch from db9f8cf to 861ff8a Compare October 4, 2017 11:55

dkayiwa reviewed Oct 4, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch from 861ff8a to f84b5e9 Compare October 4, 2017 12:10

dkayiwa reviewed Oct 4, 2017

View reviewed changes

chibujax force-pushed the RESTWS-677 branch from f84b5e9 to 295b0a3 Compare October 4, 2017 12:36

dkayiwa reviewed Oct 4, 2017

View reviewed changes

[Bug]Failure to change preferred name, or identifier using a POST req…

eeb1749

…uest to the REST API

chibujax force-pushed the RESTWS-677 branch from 295b0a3 to eeb1749 Compare October 4, 2017 12:42

dkayiwa merged commit 799057d into openmrs:master Oct 4, 2017

[Bug]Failure to change preferred name, or identifier using a POST req… #305

[Bug]Failure to change preferred name, or identifier using a POST req… #305

Conversation

chibujax commented Sep 29, 2017

JIRA TICKET NAME:

SUMMARY:

chibujax commented Sep 29, 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

dkayiwa commented Sep 29, 2017

chibujax commented Sep 29, 2017

dkayiwa commented Oct 1, 2017

chibujax commented Oct 3, 2017

Choose a reason for hiding this comment

coveralls commented Oct 3, 2017

chibujax commented Oct 3, 2017

dkayiwa commented Oct 3, 2017

chibujax commented Oct 3, 2017

coveralls commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibujax Oct 4, 2017 • edited Loading

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 4, 2017

chibujax commented Oct 4, 2017 • edited Loading

dkayiwa commented Oct 4, 2017

chibujax commented Oct 4, 2017

coveralls commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Oct 4, 2017 • edited Loading

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

chibujax commented Oct 4, 2017

chibujax commented Oct 4, 2017 • edited Loading

coveralls commented Oct 4, 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

chibujax commented Oct 4, 2017

Choose a reason for hiding this comment

chibujax commented Oct 4, 2017

coveralls commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

chibujax commented Oct 4, 2017

coveralls commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibujax commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibujax commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

dkayiwa Oct 4, 2017 •

edited

Loading

chibujax Oct 4, 2017 •

edited

Loading

chibujax commented Oct 4, 2017 •

edited

Loading

dkayiwa Oct 4, 2017 •

edited

Loading

chibujax commented Oct 4, 2017 •

edited

Loading