TRUNK-5350: Introduce new REST APIs for the Condition domain object. #327

Arthur236 · 2018-02-27T07:44:14Z

Description of what I changed

Changed 1 file for this task:

omod-2.2/src/test/java/org/openmrs/module/webservices/rest/web/v1_0/RestTestConstants2_2.java

Added 5 files for this task:

omod-2.2/src/main/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java
omod-2.2/src/test/java/org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java
omod-2.2/src/test/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2Test.java
omod-2.2/src/test/resources/conditionTestDataset.xml
omod-2.2/src/main/java/org/openmrs/module/webservices/rest/web/v1_0/converter/openmrs2_2/CodedOrFreeTextConverter.java

Issue I worked on

See https://issues.openmrs.org/browse/TRUNK-5350

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
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

dkayiwa · 2018-02-27T08:05:47Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+	@Test
+	public void shouldCreateACondition() throws Exception {
+		long originalCount = getAllCount();
+


Can you add more tests for voiding, unvoiding, and purging a condition?

Added 👍🏼

esirK · 2018-02-27T08:08:15Z

omod-2.2/src/test/resources/conditionTestDataset.xml

@@ -0,0 +1,9 @@
+<?xml version='1.0' encoding='UTF-8'?>


Remember to add License header to this file too

Noted. Thanks.

dkayiwa · 2018-02-27T10:58:02Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+	 */
+	@Override
+	public DelegatingResourceDescription getRepresentationDescription(Representation representation) {
+		if (representation instanceof DefaultRepresentation) {


Do you wanna add one for the RefRepresentation?

dkayiwa · 2018-02-27T10:59:38Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+
+		SimpleObject attributes = new SimpleObject();
+		attributes.add("voided", false);
+		attributes.add("void_reason", "");


Why do you pass these if they just contain empty strings?

Add a voided condition to the test dataset. Then unvoid it.

dkayiwa · 2018-02-27T11:00:17Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+		req.addParameter("purge", "");
+		handle(req);
+
+		Assert.assertNull(conditionService.getConditionByUuid(getUuid()));


Can you assertNotNull before purging?

dkayiwa · 2018-02-27T13:05:46Z

...g/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2Test.java

+
+	@Override
+	public void validateDefaultRepresentation() throws Exception {
+		super.validateDefaultRepresentation();


Can you also add a test for the ref representation?

dkayiwa · 2018-02-27T13:07:55Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+			DelegatingResourceDescription description = new DelegatingResourceDescription();
+			description.addProperty("uuid");
+			description.addProperty("condition");
+			description.addProperty("clinicalStatus");


Can you also add the display property to all the representations?

Can you also add the voided property to the representations?

Did you see my comment about the display property?

dkayiwa · 2018-02-27T13:09:22Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+			DelegatingResourceDescription description = new DelegatingResourceDescription();
+			description.addProperty("uuid");
+			description.addProperty("condition");
+			description.addProperty("clinicalStatus");


Can you also add the auditInfo property to the full representation?

dkayiwa · 2018-02-27T19:00:46Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+			description.addProperty("previousVersion");
+			description.addProperty("onsetDate");
+			description.addProperty("voided");
+			description.addProperty("void_reason");


Remove void reason

dkayiwa · 2018-02-27T19:00:52Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+			description.addProperty("verificationStatus");
+			description.addProperty("previousVersion");
+			description.addProperty("onsetDate");
+			description.addProperty("voided");


Remove void reason

dkayiwa · 2018-02-27T19:01:10Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+			description.addProperty("onsetDate");
+			description.addProperty("additionalDetail");
+			description.addProperty("voided");
+			description.addProperty("void_reason");


Remove void reason and date voided

dkayiwa · 2018-02-27T19:03:03Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+		SimpleObject newConditionSource = deserialize(handle(req));
+
+		Assert.assertNotNull(PropertyUtils.getProperty(newConditionSource, "uuid"));
+		Assert.assertEquals(originalCount + 1, getAllCount());


Can you also assert the property values you created above? That way, it is more than just creating a condition, but with the expected values too.

dkayiwa · 2018-02-27T19:06:44Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+		handle(req);
+
+		Condition unvoidedCondition = conditionService.getConditionByUuid(getUuid());
+		Assert.assertEquals(false, unvoidedCondition.getVoided());


I have not seen any assertion proving that this was originally voided.

dkayiwa · 2018-02-27T19:11:16Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+	 */
+	@Override
+	public DelegatingResourceDescription getRepresentationDescription(Representation representation) {
+		if (representation instanceof RefRepresentation) {


For now, let us drop the branch for the RefRepresentation because we are not adding any value. 😊

dkayiwa · 2018-02-28T06:51:33Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+
+		SimpleObject conditionSource = new SimpleObject();
+		conditionSource.add("condition_id", 2);
+		conditionSource.add("condition_coded", 1);


The property names you are using here do not match those in the default or full representations

dkayiwa · 2018-02-28T10:31:26Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+	@Test
+	public void unvoidCondition_shouldUnvoidACondition() throws Exception {
+
+		Condition condition = conditionService.getConditionByUuid(getUuid());


How can this same condition be voided here and then not voided in the previous test?

dkayiwa · 2018-02-28T11:09:32Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+			description.addProperty("previousVersion");
+			description.addProperty("onsetDate");
+			description.addProperty("additionalDetail");
+			description.addProperty("voided");


Remove voided by

dkayiwa · 2018-02-28T11:20:48Z

...g/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2Test.java

+		assertPropEquals("onsetDate", getObject().getOnsetDate());
+		assertPropEquals("additionalDetail", getObject().getAdditionalDetail());
+		assertPropEquals("voided", getObject().getVoided());
+		assertPropEquals("voided_by", getObject().getVoidedBy());


Are you sure this test will pass?

dkayiwa · 2018-02-28T15:46:10Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+	 */
+	@Override
+	public void purge(Condition condition, RequestContext requestContext) throws ResponseException {
+		Context.getConditionService().purgeService(condition);


Can you use the real method in the ConditionService?

dkayiwa · 2018-03-01T12:53:50Z

omod-2.2/src/test/java/org/openmrs/module/webservices/rest/web/v1_0/RestTestConstants2_2.java

+
+	public final static String CONDITION_UUID = "c804ee60-ecbc-4d70-abda-1e4f6f64e5b5";
+
+	public static final String CONDITION_TEST_DATA_XML = "conditionTestDataSet.xml";


This file will not be found. It needs the full path as here openmrs/openmrs-core@d4981cd#diff-0d16b7abc7a5851414804ac8d1b34bb9R31

dkayiwa · 2018-03-13T09:17:55Z

omod-2.2/src/test/resources/conditionTestDataset.xml

+	<concept_name concept_id="111" name="mg" concept_name_id="1111" locale="en" creator="1"
+				  date_created="2008-08-15 13:52:53.0" concept_name_type="FULLY_SPECIFIED" locale_preferred="1"
+				  voided="false" uuid="cf215350-7863-11e3-981f-0800200c9a66"/>
+	<patient patient_id="2" creator="1" date_created="2005-01-01 00:00:00.0" voided="false"


The patient table does not have a uuid property

dkayiwa · 2018-03-14T08:40:11Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+	@Override
+	protected PageableResult doGetAll(RequestContext context) {
+		String patientUuid = context.getRequest().getParameter("patient");
+


Getting a patient from uuid is as simple as Context.getPatientService().getPatientByUuid(uuid)

Can we get rid of this method? I mean the doGetAll() method as i said on email.

dkayiwa · 2018-03-14T09:34:29Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+
+		List<Condition> activeConditions = new ArrayList<Condition>();
+
+		if (patient != null)


If patient is null, then you should not silently ignore this. Throw an exception

dkayiwa · 2018-03-15T08:55:05Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+		        .property("verificationStatus", new StringProperty())
+		        .property("previousVersion", new StringProperty())
+		        .property("onsetDate", new StringProperty())
+		        .required("condition").required("onsetDate");


What does the above line mean?

It specifies the required properties that must be set

dkayiwa · 2018-03-15T08:55:28Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_2/ConditionResource2_2.java

+		        .property("previousVersion", new StringProperty())
+		        .property("onsetDate", new StringProperty())
+		        .property("voided", new StringProperty())
+		        .required("condition").required("onsetDate");


What does the above line mean?

coveralls · 2018-03-15T09:50:48Z

Coverage increased (+0.2%) to 41.399% when pulling 8ec507e on Arthur236:TRUNK-5350 into 3aab806 on openmrs:master.

dkayiwa · 2018-03-19T14:44:44Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+		String uuid = newConditionSource.get("uuid");
+		Assert.assertNotNull(conditionService.getConditionByUuid(uuid));
+
+		Condition condition = conditionService.getConditionByUuid(uuid);


Can you add assertions for all the properties you set before saving?

dkayiwa · 2018-03-19T14:48:39Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+		req.setContent(json.getBytes());
+
+		SimpleObject newConditionSource = deserialize(handle(req));
+		Util.log("Created condition", newConditionSource);


Are you using the above log for anything?

I was using it to show that the Condition was being created correctly.

dkayiwa · 2018-03-22T08:21:35Z

@Arthur236 have you looked at the above travis failure?

dkayiwa · 2018-03-22T09:40:37Z

Can you also make the CodedOrFreeTextConverter look like this? https://pastebin.com/MMC2cPy5

Arthur236 · 2018-03-22T11:59:09Z

Yes I have @dkayiwa. Currently working on fixing it.

dkayiwa · 2018-03-22T14:42:34Z

...enmrs/module/webservices/rest/web/v1_0/controller/openmrs2_2/ConditionController2_2Test.java

+
+		Condition condition = conditionService.getConditionByUuid(uuid);
+
+		Assert.assertEquals(nonCoded.get("nonCoded"), condition.getCondition().getNonCoded());


Shouldn't the above assert be using the newConditionSource as the rest?

Oh seen it. :)

dkayiwa reviewed Feb 27, 2018

View reviewed changes

esirK reviewed Feb 27, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch 2 times, most recently from 70f80a6 to ead3fb0 Compare February 27, 2018 09:25

dkayiwa reviewed Feb 27, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from ead3fb0 to 4577cfd Compare February 27, 2018 12:31

dkayiwa reviewed Feb 27, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from 4577cfd to 568ccac Compare February 27, 2018 18:29

dkayiwa reviewed Feb 27, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from 568ccac to 44d9ad2 Compare February 28, 2018 06:28

dkayiwa reviewed Feb 28, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from 16ec2b0 to 45c8273 Compare February 28, 2018 08:43

dkayiwa reviewed Feb 28, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from 45c8273 to 058d7dc Compare February 28, 2018 11:06

dkayiwa reviewed Feb 28, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from 058d7dc to d23f901 Compare February 28, 2018 11:17

dkayiwa reviewed Feb 28, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from d23f901 to 5ffd858 Compare February 28, 2018 12:10

dkayiwa reviewed Feb 28, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from 5ffd858 to 400501d Compare March 1, 2018 06:12

dkayiwa reviewed Mar 1, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch 3 times, most recently from f3783c3 to 46898d8 Compare March 13, 2018 09:01

dkayiwa reviewed Mar 13, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch 4 times, most recently from 04ac1e1 to a1c1a3d Compare March 14, 2018 08:35

dkayiwa reviewed Mar 14, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from a1c1a3d to 0ef6d60 Compare March 14, 2018 09:25

dkayiwa reviewed Mar 14, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch 4 times, most recently from 7d0e9b0 to a753ba6 Compare March 14, 2018 17:43

dkayiwa reviewed Mar 15, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from a753ba6 to 4f1c7e3 Compare March 15, 2018 09:38

dkayiwa reviewed Mar 19, 2018

View reviewed changes

Arthur236 force-pushed the TRUNK-5350 branch from 4f1c7e3 to bd918c1 Compare March 21, 2018 09:28

TRUNK-5350: Introduce new REST APIs for the Condition domain object.

8ec507e

Arthur236 force-pushed the TRUNK-5350 branch from bd918c1 to 8ec507e Compare March 22, 2018 13:25

dkayiwa reviewed Mar 22, 2018

View reviewed changes

dkayiwa merged commit 97fe285 into openmrs:master Mar 22, 2018

Arthur236 deleted the TRUNK-5350 branch March 23, 2018 08:24


		public final static String CONDITION_UUID = "c804ee60-ecbc-4d70-abda-1e4f6f64e5b5";

		public static final String CONDITION_TEST_DATA_XML = "conditionTestDataSet.xml";


		List<Condition> activeConditions = new ArrayList<Condition>();

		if (patient != null)


		Condition condition = conditionService.getConditionByUuid(uuid);

		Assert.assertEquals(nonCoded.get("nonCoded"), condition.getCondition().getNonCoded());

TRUNK-5350: Introduce new REST APIs for the Condition domain object. #327

TRUNK-5350: Introduce new REST APIs for the Condition domain object. #327

Conversation

Arthur236 commented Feb 27, 2018 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Feb 27, 2018 • 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

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

dkayiwa Mar 14, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Mar 22, 2018

dkayiwa commented Mar 22, 2018

Arthur236 commented Mar 22, 2018

dkayiwa Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arthur236 commented Feb 27, 2018 •

edited

Loading

dkayiwa Feb 27, 2018 •

edited

Loading

dkayiwa Mar 14, 2018 •

edited

Loading

coveralls commented Mar 15, 2018 •

edited

Loading

dkayiwa Mar 22, 2018 •

edited

Loading