RESTWS-920: Endpoint for OrderAttribute and OrderAttributeType. #587

slubwama · 2023-09-08T12:59:37Z

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

Description of what I changed

Added OrderAttribute and OrderAttributeType

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

dkayiwa · 2023-09-10T16:30:20Z

omod-2.5/src/main/java/org/openmrs/module/webservices/rest/InitPathMatcher2_5.java

+ * We should not need to apply this fix in versions where TRUNK-5022 is fixed.
+ */
+@OpenmrsProfile(openmrsPlatformVersion = "2.5.* - 9.*")
+public class InitPathMatcher2_5 implements ServletContextAware {


Is this in any way related to the changes required for your ticket?

dkayiwa · 2023-09-10T16:36:31Z

...st/java/org/openmrs/module/webservices/rest/web/v1_0/controller/RestControllerTestUtils.java

+import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
+import org.xml.sax.InputSource;
+
+public class RestControllerTestUtils extends BaseModuleWebContextSensitiveTest {


What is this for?

dkayiwa · 2023-09-10T16:36:57Z

.../openmrs/module/webservices/rest/web/v1_0/controller/jupiter/MainResourceControllerTest.java

+/**
+ * Facilitates testing controllers.
+ */
+public abstract class MainResourceControllerTest extends RestControllerTestUtils {


What is this for?

dkayiwa · 2023-09-10T16:37:20Z

...org/openmrs/module/webservices/rest/web/v1_0/controller/jupiter/RestControllerTestUtils.java

+import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
+import org.xml.sax.InputSource;
+
+public class RestControllerTestUtils extends BaseModuleWebContextSensitiveTest {


What is this for?

dkayiwa · 2023-09-10T16:38:33Z

...nmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderAttributeResource2_5Test.java

+			return "Dispensing Location: " + new SimpleDateFormat("yyyy-MM-dd").parse("2011-04-25");
+		}
+		catch (ParseException ex) {
+			ex.printStackTrace();


https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-HowTo(Not)Log

@dkayiwa done responding to all comments raised

Can you also add an OrderAttributeController2_5Test?
And an OrderAttributeTypeController2_5Test?

…er2_5Test

dkayiwa · 2023-09-11T17:11:28Z

omod-2.5/pom.xml

+            <scope>test</scope>
+        </dependency>
+
+        <dependency>


Remove this dependency

dkayiwa · 2023-09-11T17:11:39Z

omod-2.5/pom.xml

+
+        <dependency>
+            <groupId>${project.parent.groupId}</groupId>
+            <artifactId>${project.parent.artifactId}-omod-1.8</artifactId>


Remove this dependency

dkayiwa · 2023-09-11T17:11:48Z

omod-2.5/pom.xml

+
+        <dependency>
+            <groupId>${project.parent.groupId}</groupId>
+            <artifactId>${project.parent.artifactId}-omod-1.9</artifactId>


Remove this dependency

dkayiwa · 2023-09-11T17:11:58Z

omod-2.5/pom.xml

+
+        <dependency>
+            <groupId>${project.parent.groupId}</groupId>
+            <artifactId>${project.parent.artifactId}-omod-1.9</artifactId>


Remove this dependency

dkayiwa · 2023-09-11T17:12:46Z

@slubwama for the ClassNotFoundException, in your pom.xml file, remove the webservices.rest 1.8-omod and 1.9-omod dependencies.

dkayiwa · 2023-09-22T10:23:33Z

@slubwama because you extended OrderResource2_2, you need to change its supported platform versions such that they do not overlap with the new class that extends it. Does this make sense?

dkayiwa · 2023-11-28T23:07:25Z

@slubwama did you abandon this?

…s.rest into RESTWS-920

…e-9f4f-5396a31e68fb

ibacher

Few minor things, but this mostly LGTM

ibacher · 2024-06-12T18:12:38Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/BaseAttributeCrudResource2_5.java

+ * @param <P> The parent/owning type for the type T
+ * @param <PR> The Resource for the parent/owning type P
+ */
+public abstract class BaseAttributeCrudResource2_5<T extends Attribute<?, ?>, P, PR> extends DelegatingSubResource<T, P, DelegatingCrudResource<P>> {


Why doesn't this just extend BaseAttributeCrudResource1_9?

It's also not entirely clear to me what this is adding to that class...

ibacher · 2024-06-12T18:14:41Z

...s/module/webservices/rest/web/v1_0/resource/openmrs2_5/BaseAttributeTypeCrudResource2_5.java

+ * 
+ * @param <T>
+ */
+public abstract class BaseAttributeTypeCrudResource2_5<T extends AttributeType<?>> extends MetadataDelegatingCrudResource<T> {


It seems like this should extend BaseAttributeTypeCrudResource1_9.

I'm equally unsure that we even need this class at all.

ibacher · 2024-06-12T18:21:31Z

.../openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderAttributeResource2_5.java

+        for (OrderAttribute orderAttribute : delegate.getOrder().getActiveAttributes()) {
+            if (orderAttribute.equals(delegate)) {
+                needToAdd = false;
+                break;
+            }
+        }


Suggested change

for (OrderAttribute orderAttribute : delegate.getOrder().getActiveAttributes()) {

if (orderAttribute.equals(delegate)) {

needToAdd = false;

break;

}

}

if (delegate.getOrder().getActiveAttributes().contains(delegate)) {

delegate.getOrder().addAttribute(delegate);

}

.../openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderAttributeResource2_5.java

ibacher · 2024-06-12T18:22:41Z

.../openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderAttributeResource2_5.java

+     */
+    @Override
+    protected void delete(OrderAttribute delegate, String reason, RequestContext context) throws ResponseException {
+        throw new UnsupportedOperationException("Cannot purge OrderAttribute");


ibacher · 2024-06-12T18:25:47Z

...nmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderAttributeTypeResource2_5.java

+		List<OrderAttributeType> allAttrs = service().getAllOrderAttributeTypes();
+		List<OrderAttributeType> queryResult = new ArrayList<OrderAttributeType>();
+		for (OrderAttributeType locAttr : allAttrs) {
+			if (Pattern.compile(Pattern.quote(context.getParameter("q")), Pattern.CASE_INSENSITIVE)


We don't really need a regex here. You can just using locAttr.getName().toLowerCase().contains(context.getParameter("q").toLowerCase())

If we do use a Regex here, do not call Pattern.compile() in a loop; lift it to outside the loop context.

ibacher · 2024-06-12T18:27:09Z

.../module/webservices/rest/web/v1_0/controller/openmrs2_5/OrderAttributeController2_5Test.java

+/**
+ * Tests functionality of {@link OrderAttributeController}.
+ */
+public class OrderAttributeController2_5Test extends MainResourceControllerTest {


This is really a test for the OrderResource, so it should be OrderController2_5Test, no?

slubwama · 2024-06-13T09:25:32Z

@dkayiwa need review on this again

dkayiwa · 2024-06-13T09:58:07Z

@slubwama add this to your test class: https://pastebin.com/SRcYACN2

…s.rest into RESTWS-920

…s.rest into RESTWS-920 # Conflicts: # omod-2.5/pom.xml

slubwama · 2024-09-20T19:47:06Z

@dkayiwa @ibacher this is ready for a review again

ibacher · 2024-09-24T13:23:34Z

...nmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderAttributeTypeResource2_5.java

+		for (OrderAttributeType locAttr : allAttrs) {
+			if (locAttr.getName().toLowerCase().contains(context.getParameter("q").toLowerCase())) {
+				queryResult.add(locAttr);
+			}
+		}


It would be ideal, I think, if we could extend the order server to handle this search for use. Anything where we're loading all database entries into memory is going to perform slowly.

We probably want the ability (and to default!) to not returning any retired order attributes without the user explicitly requesting it.

Finally, if possible, it might be nice to order the results so that prefix matches come before "containing" matches. (This shouldn't block the PR)

ibacher · 2024-09-24T13:27:44Z

.../java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderResource2_5.java

+	public String getResourceVersion() {
+		return RestConstants2_5.RESOURCE_VERSION;
+	}
+}


Suggested change

}

}

ibacher · 2024-09-24T13:28:56Z

.../java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs2_5/OrderResource2_5.java

+	 * @param attrs
+	 */
+	@PropertySetter("attributes")
+	public static void setAttributes(Order instance, List<OrderAttribute> attrs) {


I'm a little nervous about this. Shouldn't we try to ensure that we update any attributes that might've been updated? Or do we just delete everything (somewhere) and add everything on request?

CynthiaKamau · 2024-10-31T12:18:20Z

@slubwama looking forward to this so that we can explore using it to store lab orders sample data

RESTWS-920 Create API endpoint for OrderAttribute and OrderAttributeType

90bfbe1

slubwama requested review from ibacher and dkayiwa September 8, 2023 12:59

slubwama mentioned this pull request Sep 8, 2023

TRUNK-6190 Add fulfiller location in orders openmrs/openmrs-core#4382

Closed

5 tasks

slubwama requested review from mseaton and mogoodrich September 8, 2023 13:21

dkayiwa reviewed Sep 10, 2023

View reviewed changes

slubwama added 3 commits September 10, 2023 20:34

removing unnecessary tests and files

51e9927

Changing logging approach in OrderAttributeResource2_5Test

fabd6ac

adding OrderAttributeController2_5Test and OrderAttributeTypeControll…

9f74240

…er2_5Test

dkayiwa reviewed Sep 11, 2023

View reviewed changes

omod-2.5/pom.xml

<scope>test</scope>

</dependency>

<dependency>

Copy link

Member

dkayiwa Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this dependency

dkayiwa reviewed Sep 11, 2023

View reviewed changes

Changing order to include order attribute

9d87b93

slubwama added 6 commits March 3, 2024 16:20

changing test to focus on creation of order with attribute

5f1756a

Merge branch 'master' of github.com:openmrs/openmrs-module-webservice…

48a8078

…s.rest into RESTWS-920

change version in omod-2.5 to match other module poms

e77fb1a

Merge branch 'master' of github.com:openmrs/openmrs-module-webservice…

540c59f

…s.rest into RESTWS-920

change version to 2.44.0-SNAPSHOT

6cd7ba0

change Dispensing Date order_attribute_type uuid to c0de4f5c-6626-418…

8bb76d2

…e-9f4f-5396a31e68fb

ibacher reviewed Jun 12, 2024

View reviewed changes

make changes to test data

67b90a2

response to comments

c0e5447

mks-d changed the title ~~RESTWS-920 Create API endpoint for OrderAttribute and OrderAttributeType~~ RESTWS-920: Endpoint for OrderAttribute and OrderAttributeType. Jun 19, 2024

slubwama added 5 commits July 20, 2024 13:41

response to comments

949c59f

Merge branch 'master' of github.com:openmrs/openmrs-module-webservice…

611ec39

…s.rest into RESTWS-920

change to snapshot 2.45.0-SNAPSHOT

14a16a4

add override for shouldGetAll() to one that throws an exception.

ceff5c5

Merge branch 'master' of github.com:openmrs/openmrs-module-webservice…

a1f0521

…s.rest into RESTWS-920 # Conflicts: # omod-2.5/pom.xml

clean pom

8ef2d27

ibacher reviewed Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RESTWS-920: Endpoint for OrderAttribute and OrderAttributeType. #587

RESTWS-920: Endpoint for OrderAttribute and OrderAttributeType. #587

slubwama commented Sep 8, 2023

dkayiwa Sep 10, 2023

dkayiwa Sep 10, 2023

dkayiwa Sep 10, 2023

dkayiwa Sep 10, 2023

dkayiwa Sep 10, 2023

slubwama Sep 10, 2023

dkayiwa Sep 10, 2023 •

edited

Loading

dkayiwa Sep 11, 2023

dkayiwa Sep 11, 2023

dkayiwa Sep 11, 2023

dkayiwa Sep 11, 2023

dkayiwa commented Sep 11, 2023

dkayiwa commented Sep 22, 2023

dkayiwa commented Nov 28, 2023

ibacher left a comment

ibacher Jun 12, 2024

ibacher Jun 12, 2024

ibacher Jun 12, 2024

ibacher Jun 12, 2024

ibacher Jun 12, 2024

ibacher Jun 12, 2024

ibacher Jun 12, 2024

ibacher Jun 12, 2024

slubwama commented Jun 13, 2024

dkayiwa commented Jun 13, 2024

slubwama commented Sep 20, 2024

ibacher Sep 24, 2024

ibacher Sep 24, 2024

ibacher Sep 24, 2024

ibacher Sep 24, 2024

ibacher Sep 24, 2024

CynthiaKamau commented Oct 31, 2024

RESTWS-920: Endpoint for OrderAttribute and OrderAttributeType. #587

Are you sure you want to change the base?

RESTWS-920: Endpoint for OrderAttribute and OrderAttributeType. #587

Conversation

slubwama commented Sep 8, 2023

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

dkayiwa Sep 10, 2023 • 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

dkayiwa commented Sep 11, 2023

dkayiwa commented Sep 22, 2023

dkayiwa commented Nov 28, 2023

ibacher left a 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

slubwama commented Jun 13, 2024

dkayiwa commented Jun 13, 2024

slubwama commented Sep 20, 2024

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

CynthiaKamau commented Oct 31, 2024

dkayiwa Sep 10, 2023 •

edited

Loading