RESTWS-647: Searching for Orderable Concepts #274

vtuwei · 2017-03-21T13:33:46Z

No description provided.

mention-bot · 2017-03-21T13:33:47Z

@vtuwei, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dkayiwa and @teleivo to be potential reviewers.

dkayiwa · 2017-03-22T20:51:38Z

...org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_11/OrderableResource1_11.java

+ */
+@Resource(name = RestConstants.VERSION_1 + "/orderable", supportedClass = Concept.class, supportedOpenmrsVersions = {
+        "1.11.*", "1.12.*", "2.0.*" })
+public class OrderableResource1_11 extends BaseDelegatingResource<ConceptSearchResult> implements Searchable {


What happens if this was put in the 1.9 sub module? It is using any api that requires a minimum of platform 1.11?

@dkayiwa We cannot move it since OrderType is deprecated in 1.9

dkayiwa · 2017-03-22T21:09:47Z

...openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_11/OrderableResource1_11Test.java

+import org.openmrs.module.webservices.rest.web.RestTestConstants1_8;
+import org.openmrs.module.webservices.rest.web.resource.impl.BaseDelegatingResourceTest;
+
+public class OrderableResource1_11Test extends BaseDelegatingResourceTest<OrderableResource1_11, ConceptSearchResult> {


Can you also add a controller test for this resource? Just like ConceptController1_8Test for the concept resource.

dkayiwa · 2017-03-22T21:27:47Z

...org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_11/OrderableResource1_11.java

+			}
+		}
+
+		if (conceptClasses == null | (conceptClasses != null && conceptClasses.size() == 0))


On the above line after == null, was the next supposed to be "|" or "||"?

also if the first case (conceptClasses == null) is not true you dont need to check again that its not null.

so this is enough

if (conceptClasses == null || conceptClasses.size() == 0)

and I think you should use this

if (conceptClasses == null || conceptClasses.isEmpty())

since this shows your intent more clearly and isEmpty might also be faster than computing the size and checking if that is 0

@teleivo changed to that.

dkayiwa · 2017-03-22T21:32:55Z

...org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_11/OrderableResource1_11.java

+
+		if (classUuids != null) {
+			for (String uuid : classUuids) {
+				ConceptClass cc = getConceptClassByUuid(uuid, orderableConceptClasses);


For the above, did you think of using? (ConceptClass) ConversionUtil.convert(uuid, ConceptClass.class)

Switched to this. I was using this function to search for a concept class from the already loaded concept classes instead of loading the class from the database again.

djazayeri · 2017-03-27T09:56:19Z

...org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_11/OrderableResource1_11.java

+ * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
+ * graphic logo is a trademark of OpenMRS Inc.
+ */
+package org.openmrs.module.webservices.rest.web.v1_0.resource.openmrs1_11;


I would think these should be for 1.10 (since that's when we completely rewrote the order entry API)

@djazayeri moved it to 1.1

djazayeri · 2017-03-27T09:57:00Z

...org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_11/OrderableResource1_11.java

+ * orderable. It returns ConceptSearchResults instead of Concepts
+ */
+@Resource(name = RestConstants.VERSION_1 + "/orderable", supportedClass = Concept.class, supportedOpenmrsVersions = {
+        "1.11.*", "1.12.*" })


should also include 2.0.* and 2.1.*, right?

@djazayeri i have included them

vtuwei · 2017-03-29T11:08:22Z

@djazayeri Can you advise me on how to handle the tests? They are failing because of Conversion Exceptions

dkayiwa · 2017-03-29T19:58:53Z

@vtuwei the conversion exceptions are happening because on your OrderableResource1_10 you have the supportedClass as Concept.class instead of ConceptSearchResult.class

coveralls · 2017-03-30T09:53:06Z

Coverage decreased (-0.3%) to 45.252% when pulling f3da11c on vtuwei:RESTWS-647 into 9d8cbf6 on openmrs:master.

vtuwei · 2017-03-30T11:19:07Z

Thank you @dkayiwa i will add more tests.

dkayiwa · 2017-03-30T13:31:29Z

@vtuwei will review again after you have added the missing tests.

coveralls · 2017-04-03T08:48:57Z

Coverage increased (+0.2%) to 45.765% when pulling 457d658 on vtuwei:RESTWS-647 into 9d8cbf6 on openmrs:master.

vtuwei · 2017-04-03T08:50:07Z

@dkayiwa please review

dkayiwa · 2017-04-04T14:26:02Z

...mrs/module/webservices/rest/web/v1_0/controller/openmrs1_10/OrderableController1_10Test.java

+
+	private ConceptService service;
+
+	private boolean isIndexUpToDate = false;


Is the above variable necessary if you used http://junit.sourceforge.net/javadoc/org/junit/BeforeClass.html?

@dkayiwa onlyOnce expects a static modifier. That means the concept service has to be a static instance.

Does it even have to be an instance variable? 😊

@dkayiwa not really. Do i switch to the beforeclass annotation?

If what you are trying to do is setup once for the whole class instead of for each method, then that is exactly what this annotation is meant for. 😊

@dkayiwa ConceptService cannot be a static instance. The test fails when it is a static instance.

coveralls · 2017-04-10T08:50:22Z

Coverage increased (+0.2%) to 45.786% when pulling f341eeb on vtuwei:RESTWS-647 into 5202e54 on openmrs:master.

vtuwei force-pushed the RESTWS-647 branch from 963cb5a to 67f0675 Compare March 21, 2017 13:35

dkayiwa reviewed Mar 22, 2017

View reviewed changes

vtuwei force-pushed the RESTWS-647 branch 8 times, most recently from 056e516 to 91b94af Compare March 27, 2017 08:56

djazayeri reviewed Mar 27, 2017

View reviewed changes

vtuwei force-pushed the RESTWS-647 branch from 91b94af to 56d9f36 Compare March 29, 2017 09:20

vtuwei force-pushed the RESTWS-647 branch 2 times, most recently from 1cb26af to f3da11c Compare March 30, 2017 09:38

vtuwei force-pushed the RESTWS-647 branch 2 times, most recently from 085f704 to 457d658 Compare April 3, 2017 08:35

dkayiwa reviewed Apr 4, 2017

View reviewed changes

vtuwei force-pushed the RESTWS-647 branch from 457d658 to c293768 Compare April 10, 2017 08:27

RESTWS-647: Searching for Orderable Concepts

f341eeb

vtuwei force-pushed the RESTWS-647 branch from c293768 to f341eeb Compare April 10, 2017 08:35

dkayiwa merged commit 850202b into openmrs:master Apr 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RESTWS-647: Searching for Orderable Concepts #274

RESTWS-647: Searching for Orderable Concepts #274

vtuwei commented Mar 21, 2017

mention-bot commented Mar 21, 2017

dkayiwa Mar 22, 2017

vtuwei Mar 27, 2017 •

edited

Loading

dkayiwa Mar 22, 2017 •

edited

Loading

dkayiwa Mar 22, 2017

teleivo Mar 23, 2017

vtuwei Mar 27, 2017

dkayiwa Mar 22, 2017

vtuwei Mar 27, 2017

djazayeri Mar 27, 2017

vtuwei Mar 29, 2017

djazayeri Mar 27, 2017

vtuwei Mar 29, 2017

vtuwei commented Mar 29, 2017

dkayiwa commented Mar 29, 2017

coveralls commented Mar 30, 2017

vtuwei commented Mar 30, 2017

dkayiwa commented Mar 30, 2017

coveralls commented Apr 3, 2017

vtuwei commented Apr 3, 2017

dkayiwa Apr 4, 2017

vtuwei Apr 10, 2017 •

edited

Loading

dkayiwa Apr 10, 2017

vtuwei Apr 10, 2017

dkayiwa Apr 10, 2017

vtuwei Apr 10, 2017

coveralls commented Apr 10, 2017


		private ConceptService service;

		private boolean isIndexUpToDate = false;

RESTWS-647: Searching for Orderable Concepts #274

RESTWS-647: Searching for Orderable Concepts #274

Conversation

vtuwei commented Mar 21, 2017

mention-bot commented Mar 21, 2017

Choose a reason for hiding this comment

vtuwei Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

dkayiwa Mar 22, 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

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

vtuwei commented Mar 29, 2017

dkayiwa commented Mar 29, 2017

coveralls commented Mar 30, 2017

vtuwei commented Mar 30, 2017

dkayiwa commented Mar 30, 2017

coveralls commented Apr 3, 2017

vtuwei commented Apr 3, 2017

Choose a reason for hiding this comment

vtuwei Apr 10, 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

Choose a reason for hiding this comment

coveralls commented Apr 10, 2017

vtuwei Mar 27, 2017 •

edited

Loading

dkayiwa Mar 22, 2017 •

edited

Loading

vtuwei Apr 10, 2017 •

edited

Loading