Add Rest Resource For ActorResponse #1

sherrif10 · 2022-10-07T10:28:14Z

Conversations are on talk https://talk.openmrs.org/t/get-api-responses-onto-dashboard-from-actorresponsetable/37656/5
Add Rest Endpoint for ActorResponse table. This feature is needed in one our CFL distribution.
Kindly help me review
cc @pwargulak @aagrawa4

druchniewicz · 2022-10-10T12:40:21Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+    public DelegatingResourceDescription getRepresentationDescription(Representation rep) {
+        if (rep instanceof DefaultRepresentation) {
+            final DelegatingResourceDescription description = new DelegatingResourceDescription();
+            description.addProperty("id");


please extract all these hard-coded values into consts. You're using several times the same values like "id", "patient", "actor", "person" etc. etc. Please go through whole file and extract to consts where possible

Hello @druchniewicz . Sorry, i was supposed to use UUID instead of Id.
Let me resolve this quickly.

druchniewicz · 2022-10-10T12:47:12Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+
+    @Override
+    public DelegatingResourceDescription getRepresentationDescription(Representation rep) {
+        if (rep instanceof DefaultRepresentation) {


in each 'if' block you create 'DelegatingResourceDescription description' variable
Please create this variable at the beginning of the method and assign null 'DelegatingResourceDescription description = null;' and then in each if block you can create proper object and assign it to this value. Then instead of return statement in each if block you return 'description' value once at the end of file. (and you won't have to return null value separately)

Hello @druchniewicz . Kindly am i didnot get your comment above well .

I mean that we can extract 'description' variable at the beginning of the method and assign 'null' value initially. DelegatingResourceDescription description = null;
and then in 'if-else' block, create a object and assign it to 'description' variable.
if (rep instanceof DefaultRepresentation) {
description = new DelegatingResourceDescription();
[...]
else if (rep instanceof FullRepresentation) {
description = new DelegatingResourceDescription();
[...]
else if (rep instanceof RefRepresentation) {
description = new DelegatingResourceDescription();

Thanks to this you have only one 'return' statement at the end of method, instead of in each if-else block. And also you don't have to return null at the end separately.
return description

druchniewicz · 2022-10-10T12:54:37Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+                .property("patient", new ArrayProperty(new RefProperty("#/definitions/PatientCreate")))
+                .property("person", new ArrayProperty(new RefProperty("#/definitions/PersonCreate")))
+                .property("id", new IntegerProperty())
+                .property("answereTime", new DateProperty())


I guess there was supposed to be "answeredTime" -> this is the one of the biggest benefit storing repeated multiple times values in consts - small chance to make a typo. But even if make a typo, we only correct it in one place and that's it

druchniewicz · 2022-10-10T12:55:09Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+                .property("sourceId", new StringProperty())
+                .property("textResponse", new StringProperty())
+                .property("id", new IntegerProperty())
+                .property("answereTime", new StringProperty())


same typo as above

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

druchniewicz · 2022-10-10T13:02:22Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+    }
+
+    @Override
+    protected void delete(ActorResponse delegate, String textQuestion, RequestContext textResponse ) throws ResponseException {


why textQuestion and textResponse params are here? They are not used

Resolved thanks

You changed only param names. You still have two unsed params - they are required because you override delete method from interface, right?
Second remark - you have condition "if response != null" then "do nothing". It does it make sense. In this case you always delete null entity

druchniewicz · 2022-10-10T13:02:48Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+    }
+
+    @Override
+    public void purge(ActorResponse delegate, RequestContext context ) throws ResponseException {


context param is not used

druchniewicz · 2022-10-10T13:04:50Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+		"2.3.*", "2.4.*", "2.5.*", "2.6.*" })
+public class ActorResponseRestResource extends DelegatingCrudResource<ActorResponse> {
+
+    protected static final String REASON = "REST web service";


is it used somewhere?

druchniewicz · 2022-10-10T13:09:03Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

@@ -0,0 +1,243 @@
+package org.openmrs.module.messages.resources;


please use google-java-format plugin (https://github.com/google/google-java-format) to reformat code, because there are a lot of ugly indents, missing spaces etc. You can install such plugin directly in your IDE

druchniewicz · 2022-10-10T13:10:17Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

@@ -0,0 +1,243 @@
+package org.openmrs.module.messages.resources;


@sherrif10 I guess you tested this whole code and it works fine, right?

Thank you so much @druchniewicz , i will address to the above comments.

druchniewicz · 2022-10-10T13:15:25Z

omod/src/test/java/org/openmrs/module/messages/resources/ActorResponseRestResourceTest.java

+    private final ActorResponseRestResource actorResponseRestResource = new ActorResponseRestResource();
+
+    @Mock
+    private org.openmrs.module.messages.api.service.ActorResponseService ActorResponseService;


don't use whole package path if you don't need.
and change ActorResponseService field name to actorResponseService

druchniewicz · 2022-10-17T09:41:56Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+
+    @Override
+    public DelegatingResourceDescription getRepresentationDescription(Representation rep) {
+        if (rep instanceof DefaultRepresentation) {


I mean that we can extract 'description' variable at the beginning of the method and assign 'null' value initially. DelegatingResourceDescription description = null;
and then in 'if-else' block, create a object and assign it to 'description' variable.
if (rep instanceof DefaultRepresentation) {
description = new DelegatingResourceDescription();
[...]
else if (rep instanceof FullRepresentation) {
description = new DelegatingResourceDescription();
[...]
else if (rep instanceof RefRepresentation) {
description = new DelegatingResourceDescription();

Thanks to this you have only one 'return' statement at the end of method, instead of in each if-else block. And also you don't have to return null at the end separately.
return description

druchniewicz · 2022-10-17T10:02:00Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

-                    .property("preferredTextQuestion", new RefProperty("#/definitions/TextQuestionGetRef"))
-                    .property("preferredActor", new RefProperty("#/definitions/ActorGetRef"))
-                    .property("preferreQuestion", new RefProperty("#/definitions/QuestionGetRef"));
+                    .property("Response", new RefProperty("#/definitions/ResponseGetRef"))


Why you changed these properties names and first character to upper case?
Should not these names be a class fields names or can by any names?

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

druchniewicz · 2022-10-17T10:04:40Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

@@ -209,7 +219,7 @@ public Model getUPDATEModel(Representation representation) {
 	 */
 	@Override
    public List<String> getPropertiesToExposeAsSubResources() {
-        return Arrays.asList("actorResponseType", "question", "attributes");
+        return Arrays.asList("actorResponseType", "concept", "patient","p");


what does this change mean? Why suddenly other fields and one more field?

druchniewicz · 2022-10-17T10:10:24Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

+    }
+
+    @Override
+    protected void delete(ActorResponse delegate, String textQuestion, RequestContext textResponse ) throws ResponseException {


You changed only param names. You still have two unsed params - they are required because you override delete method from interface, right?
Second remark - you have condition "if response != null" then "do nothing". It does it make sense. In this case you always delete null entity

druchniewicz · 2022-10-17T10:12:30Z

omod/src/main/java/org/openmrs/module/messages/resources/ActorResponseRestResource.java

    }

    @Override
-    public ActorResponse getByUniqueId(String uuid) {
-        return Context.getService(ActorResponseService.class).getByUuid(uuid);
+    public ActorResponse getByUniqueId(String uniqueid) {


I think 'uuid' was a good name, but if you want to use 'uniqueid' name then please change it to 'uniqueId' (camel-case convention)

pwargulak · 2023-12-15T13:48:13Z

It's going to be included in the next release.

…act time Merge in ITX-BPV/openmrs-module-messages from feature/AGRE-2167 to development * commit 'b8b5dd2f2549731d93a95918afaae2d799e52993': AGRE-2167: Fixed receiving calls after the best contact time

Add Rest Resource For ActorResponse

34bd5ee

druchniewicz suggested changes Oct 10, 2022

View reviewed changes

aggieshami added 2 commits October 10, 2022 15:53

Add Tests to Rest Resource

8ce6bc1

add resources

2ea2f40

druchniewicz suggested changes Oct 17, 2022

View reviewed changes

Resolve the commits

33c82cc

pwargulak closed this Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rest Resource For ActorResponse #1

Add Rest Resource For ActorResponse #1

sherrif10 commented Oct 7, 2022

druchniewicz Oct 10, 2022

sherrif10 Oct 14, 2022

druchniewicz Oct 10, 2022

sherrif10 Oct 14, 2022

druchniewicz Oct 17, 2022

druchniewicz Oct 10, 2022

druchniewicz Oct 10, 2022

druchniewicz Oct 10, 2022

sherrif10 Oct 14, 2022

druchniewicz Oct 17, 2022

druchniewicz Oct 10, 2022

druchniewicz Oct 10, 2022

druchniewicz Oct 10, 2022

druchniewicz Oct 10, 2022

sherrif10 Oct 10, 2022

druchniewicz Oct 10, 2022

druchniewicz Oct 17, 2022

druchniewicz Oct 17, 2022

druchniewicz Oct 17, 2022

druchniewicz Oct 17, 2022

druchniewicz Oct 17, 2022

pwargulak commented Dec 15, 2023

		@@ -0,0 +1,243 @@
		package org.openmrs.module.messages.resources;

Add Rest Resource For ActorResponse #1

Add Rest Resource For ActorResponse #1

Conversation

sherrif10 commented Oct 7, 2022

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

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

pwargulak commented Dec 15, 2023