ATT-42: AttachmentResource* to be documented in Swagger #48

sherrif10 · 2020-06-12T13:20:05Z

https://issues.openmrs.org/browse/ATT-42

cc @dkayiwa @ibacher @mks-d kindly review thanks alot

sherrif10 · 2020-06-12T13:25:11Z

We need to test this such that we can be on the right truck. If there is more guidance needed, i will humbly welcome them thanks here is the screen shot

reagan-meant · 2020-06-13T16:49:54Z

pom.xml

@@ -44,7 +44,7 @@
    <appframeworkVersion>2.2.1</appframeworkVersion>
    <uiframeworkVersion>3.3.1</uiframeworkVersion>
    <uicommonsVersion>1.4</uicommonsVersion> 
-    <webservices.restVersion>2.17</webservices.restVersion>
+    <webservices.restVersion>2.21.0</webservices.restVersion>


Could you now test this with the latest version

Ohh yeah let me do but i was following @mks-d ideal that it may come up with many dependencies

Unfortunately i tested this on webservices.rest 2.28.0 but i got error invalidActivationKeyException which means that some methods are not yet introduced into 2.28.0, like how @mks-d suggested in talk thread, upgrading to 2.28.0 would bring such an error

@sherrif10 That is alittle strange...How uniques are the pom depepndencies here and the ones i used in this Pull Request

When i run them on my machine i got the similar error i stated above invalidActivationKeyException .

reagan-meant · 2020-06-13T16:52:06Z

omod-2.0/pom.xml

-      <artifactId>webservices.rest-omod</artifactId>
-      <version>${webservices.restVersion}</version>
+      <artifactId>webservices.rest-omod-common</artifactId>
+      <version>2.21.0</version>


Why are you hard coding the version here yet you have already declared it on line 19?

${webservices.restVersion} acts as a variable of its version , inotherwords hardcoding it by replacing the reall version doesnot make much difference. i can replace it back without hardcoding it

reagan-meant · 2020-06-13T16:52:12Z

omod-2.0/pom.xml

    </dependency>
    <dependency>
      <groupId>org.openmrs.module</groupId>
      <artifactId>webservices.rest-omod-common</artifactId>
-      <version>${webservices.restVersion}</version>
+      <version>2.21.0</version>


Why are you hard coding the version here yet you have already declared it on line 19?

${webservices.restVersion} acts as a variable of its version , inotherwords hardcoding it by replacing the reall version doesnot make much difference. i can replace it back without hardcoding it

reagan-meant · 2020-06-13T16:53:51Z

omod-2.0/pom.xml

@@ -16,7 +16,7 @@
  <properties>
    <openMRSVersion>${openMRS2_0Version}</openMRSVersion>
    <emrapiVersion>1.19</emrapiVersion> 
-    <webservices.restVersion>2.17</webservices.restVersion>
+    <webservices.restVersion>2.21.0</webservices.restVersion>


Is it necessary to redeclare this property when it is already in the parent pom unless if you want to override the version?

At first glance i thought of it, however let me try undeclaring it thanks @reagan-meant

@reagan-meant i undeclared that thanks

sherrif10 · 2020-06-15T03:43:44Z

cc @mks-d @ibacher @reagan-meant i think this is ready to be merged,Anything missing please will be highly appreciated

https://issues.openmrs.org/browse/ATT-42

sherrif10 · 2020-06-17T09:16:11Z

We still need this documentation however so i would like more suggestions here cc @mks-d @ibacher

samuelmale

Thanks a bunch for working on this.

This is close to complete, I think we need to add some test coverage, and be sure you run:

> mvn clean package

For proper formatting.

samuelmale · 2020-07-06T13:58:50Z

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

+	// @Override
+	// public Attachment save(Attachment delegate) {
+	// Obs obs = Context.getObsService().saveObs(delegate.getObs(), REASON);
+	// return new Attachment(obs);
+	// }


Did you intend to the leave commented code?

These were earlier in the commit before my changes, except if am to remove it

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

sherrif10 · 2020-09-24T13:41:17Z

@dkayiwa: How does it look now, here is the screen shot of documentation

sherrif10 · 2020-09-28T16:58:12Z

cc @mozzy11 @sacull, @gitcliff @HerbertYiga , @tendomart , feel free to review since reviewing tickets is part of learning thanks

tendomart · 2020-09-28T18:20:48Z

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

+		        .property("preferredName", new StringProperty().example("uuid"))
+		        .property("preferredAddress", new StringProperty().example("uuid"))
+		        .property("attributes", new ArrayProperty(new RefProperty("#/definitions/AttachmentAttributeCreate")))
+


Is this space necessary ?

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

debicki · 2020-09-28T18:56:52Z

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

+
+import java.util.Arrays;
+import java.util.List;
+import org.hibernate.FlushMode;


Suggested change

import org.hibernate.FlushMode;

You have added an import that you are not using.

thanks @sacull ,Actually am using this import, let me know if there is something else please

Oddly, I'm sure these two imports weren't used before and the save(Attachement) method looked different, however, I can't find it in history. Maybe something just happened to me. Bravo for vigilance. ;)

Thank you too @sacull , if you approve this , then we can ping @dkayiwa for final remarks ,actually after this merge, am releasing attachment module asap

After the last commit (464f5aa) the save(Attachment) method looks like I remembered it, so again these imports are not used:

Hey @sacull, shall we go with this idea??, I included screenshots for verification

@sherrif10 It's not my decision whether to leave it or not, I'm not important enough here. ;)

In addition, I would like to take a better look at this PR to be able to click "Approve" with full responsibility, unfortunately, until the end of the week, I am not able to do it due to lack of time. I'm sorry.

haa you are more important @sacull, we have learnt clean code from you , keep up

Did you do as @sacull suggested above?

Yes i responded to his comments above

debicki · 2020-09-28T18:57:29Z

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

+import org.hibernate.FlushMode;
+import org.openmrs.Obs;
+import org.openmrs.module.attachments.AttachmentsConstants;
+import org.openmrs.module.emrapi.db.DbSessionUtil;


Suggested change

import org.openmrs.module.emrapi.db.DbSessionUtil;

Same as above.

Thanks @sacull, actually i used this package on line 214

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

samuelmale · 2020-09-30T20:38:47Z

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

+	public void purge(Attachment delegate, RequestContext context) throws ResponseException {
+		String encounterUuid = delegate.getObs().getEncounter() != null ? delegate.getObs().getEncounter().getUuid() : null;
+		Context.getObsService().purgeObs(delegate.getObs());
+		voidEncounterIfEmpty(Context.getEncounterService(), encounterUuid);
+	}


Is this also required for Swagger documentation? If not, why did you override this? I see your doing great stuff here but I think it maybe out of scope.

Same case for: newDelegate(...), delete(...), getByUniqueId(...)

@samuelmale thanks , let me cross check that code

I have resolved that , take a look

samuelmale · 2020-09-30T22:06:35Z

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

+	@Override
+	public List<String> getPropertiesToExposeAsSubResources() {
+		return Arrays.asList("comment", "complexData", "Attributes");
+	}
+


I'm not sure about this method, is the override required for Swagger docs?

Yes , @samuelmale , this method exposes subresource classes as a resource in swagger docs

Only one approval from either @sacull @tendomart ,will rescue me

samuelmale

LGTM

Akayeshmantha

LGTM

Nice work @sherrif10

sherrif10 · 2020-10-02T15:11:49Z

Thanks @Akayeshmantha @dkayiwa any cool response from you, can we merge this please

mozzy11 · 2020-10-04T18:14:32Z

@sherrif10 great work done.
i can see some tests already written for the initial work that was done around the Attachments Resource, can you also write more test units for the extra work done here??

mozzy11 · 2020-10-04T18:32:41Z

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

+	 * @see org.openmrs.module.webservices.rest.web.resource.impl.BaseDelegatingResource#getUpdatableProperties()
+	 */
+	@Override
+	public DelegatingResourceDescription getUpdatableProperties() throws ResourceDoesNotSupportOperationException {


@sherrif10 , can you create test for this Update method ??

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java

sherrif10 · 2020-10-06T14:28:03Z

cc @mozzy11 @dkayiwa , i added some tests to confirm . kindly take a look FYI we need to release attachments with this work thanks

mozzy11 · 2020-10-07T06:47:17Z

well done @sherrif10 .

can you also write some more tests units for the Attachment Resource Rest Controller ?? like here,
especially for Creating and Updating the attachment with complex data ,

mozzy11 · 2020-10-07T17:57:48Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+		String uuid = (String) attachment.get("uuid");
+		Assert.assertNotNull("uuid", obs.getUuid());
+		Assert.assertNotNull("comment", obs.getComment());
+		Assert.assertEquals("complexData", obs.getComplexData(), null);


@sherrif10 , you instead need to check the deserialised response , not the very json object you posted

mozzy11 · 2020-10-07T18:01:30Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+		req.setContent(json.getBytes());
+
+		String editedAttachment = obs.getUuid();
+		Assert.assertNotEquals("Updated complexData ", editedAttachment.equalsIgnoreCase(editedAttachment));


@sherrif10 , same here , you need to check the deserialized response , not the OBS object you posted

@mozzy11 ok no problem

sherrif10 · 2020-10-19T20:26:14Z

cc @mozzy11 this LGTM

sherrif10 · 2020-10-22T08:14:06Z

cc @dkayiwa @mozzy11 This ticket has been the one delaying the release of attachment module even now. We have done necessary work for it to be merged at least. The work being done can enable this ticket to be merged. So am suggesting we finish this by today because personally am too fixed with other tickets in current ongoing sprint thanks

dkayiwa · 2020-10-22T08:28:59Z

@sherrif10 have u responded to all the review comments?

sherrif10 · 2020-10-22T09:03:37Z

@dkayiwa i have responded to every comment above

dkayiwa · 2020-10-22T20:39:53Z

omod-2.0/pom.xml

@@ -110,7 +110,7 @@

    <dependency>
      <groupId>org.openmrs.module</groupId>
-      <artifactId>webservices.rest-omod</artifactId>


Did you change this accidentally? If not, what was the reason behind the change?

Resolved thanks

dkayiwa · 2020-10-22T20:49:12Z

omod-2.0/pom.xml

@@ -16,7 +16,7 @@
  <properties>
    <openMRSVersion>${openMRS2_0Version}</openMRSVersion>
    <emrapiVersion>1.19</emrapiVersion> 
-    <webservices.restVersion>2.17</webservices.restVersion>
+    <webservices.restVersion>2.21.0</webservices.restVersion>


I already saw this in the root pom. Do you need to duplicate it here?

Resolved thanks

dkayiwa · 2020-10-22T20:50:51Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

@@ -124,10 +217,10 @@ public void doSearch_shouldReturnResults() {
 		RequestContext requestContext = new RequestContext();
 		requestContext.setRequest(request);

-		// Replay
+		// // Replay


Why this change?

dkayiwa · 2020-10-22T20:51:04Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

 		BasePageableResult response = (BasePageableResult) res.doSearch(requestContext);
 		List<Attachment> actualAttachments = response.getPageOfResults();
-
+		//


Why this change?

dkayiwa · 2020-10-22T20:52:55Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+			result = deserialize(handle(req));
+		}
+		catch (Exception e) {
+			// TODO Auto-generated catch block


https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotprintout

dkayiwa · 2020-10-22T20:53:27Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+			json = new ObjectMapper().writeValueAsString(attributes);
+		}
+		catch (IOException e) {
+			// TODO Auto-generated catch block


https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotprintout

dkayiwa · 2020-10-22T20:53:50Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+			result = deserialize(handle(req));
+		}
+		catch (Exception e) {
+			// TODO Auto-generated catch block


https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotprintout

dkayiwa · 2020-10-22T20:53:54Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+			json = new ObjectMapper().writeValueAsString(attachment);
+		}
+		catch (IOException e) {
+			// TODO Auto-generated catch block


https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotprintout

Hope using logs to catch exception would work here

dkayiwa · 2020-10-22T20:54:34Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentResource2_0Test.java

+
+	@Override
+	public String getDisplayProperty() {
+		// TODO Auto-generated method stub


Can you remove the above?

Are you suggesting i remove these methods above or whole class ?

Resolved thanks

dkayiwa · 2020-10-22T20:55:56Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentResource2_0Test.java

+
+	@Override
+	public Attachment newObject() {
+		return new Attachment(Context.getObsService().getObsByUuid("9b6639b2-5785-4603-a364-075c2d61cd51"));


Don't you already have a constant for this?

True dat, will resolve it soon

sherrif10 · 2020-10-26T17:08:55Z

cc @dkayiwa @mozzy11 feel free to have a look at my new commits

sherrif10 · 2020-10-27T10:26:19Z

cc @dkayiwa, @mozzy11 : kindly review this ticket thanks

mozzy11 · 2020-10-28T08:49:05Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+		String uuid = (String) attachment.get("uuid");
+		Assert.assertNull(result);
+		String createdAttachment = obs.getUuid();
+		Assert.assertNotEquals("Created complexData ", createdAttachment.equalsIgnoreCase(createdAttachment));


@sherrif10 what are you trying to test here

The created Attachment , so basically this tests above string

mozzy11 · 2020-10-28T08:49:46Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+		// Check existence in database
+		String uuid = (String) attachment.get("uuid");
+		Assert.assertNull(result);
+		String createdAttachment = obs.getUuid();


@sherrif10 ,is this really the created attachment obj??

Yeah i actually tested the created attachment below with ignorecase character

mozzy11 · 2020-10-28T08:51:10Z

omod-2.0/src/test/java/org/openmrs/module/attachments/rest/AttachmentRestController2_0Test.java

+		}
+
+		Assert.assertNull(result);
+		String editedAttachment = obs.getUuid();


@sherrif10 ,is this the edited attachment object ??

Yes this is the editted attachment, the editted attachment was initialised as a getUuid() then i tested it ignoring the case case character, if i had not used the ignoreCaseMethod, the test would fail in this context

sherrif10 · 2020-10-28T19:04:48Z

cc @dkayiwa @mozzy11 any more comments on this

Muta-Jonathan · 2021-11-10T10:44:45Z

Is this still available.

sherrif10 · 2021-11-10T11:00:30Z

Yes thanks. WIll look through and update the need branch.

reagan-meant reviewed Jun 14, 2020

View reviewed changes

ATT-42: AttachmentResource* to be documented in Swagger

4e0bf0f

https://issues.openmrs.org/browse/ATT-42

mks-d requested a review from samuelmale July 2, 2020 10:27

samuelmale requested changes Jul 6, 2020

View reviewed changes

sherrif10 added 2 commits September 24, 2020 11:58

Removing unwanted commits

afc7922

Resolving representations in swagger

f05a918

Removing unncessary comments

67b1577

tendomart reviewed Sep 28, 2020

View reviewed changes

omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java Outdated Show resolved Hide resolved

debicki suggested changes Sep 28, 2020

View reviewed changes

resolving commit

937e7c4

samuelmale reviewed Sep 30, 2020

View reviewed changes

removing unwanted commits

7af99d6

samuelmale reviewed Sep 30, 2020

View reviewed changes

samuelmale approved these changes Sep 30, 2020

View reviewed changes

Akayeshmantha approved these changes Oct 2, 2020

View reviewed changes

mozzy11 reviewed Oct 4, 2020

View reviewed changes

adding some tests

9cc8876

mozzy11 reviewed Oct 7, 2020

View reviewed changes

dkayiwa reviewed Oct 22, 2020

View reviewed changes

sherrif10 and others added 3 commits October 26, 2020 15:20

Adding some unit tests in rest controller

59928cb

resolving deserialised objects with loglevels

4a05f33

Merge branch 'master' into ATT-42

809fb57

mozzy11 reviewed Oct 28, 2020

View reviewed changes

ATT-42: AttachmentResource* to be documented in Swagger #48

Are you sure you want to change the base?

ATT-42: AttachmentResource* to be documented in Swagger #48

Conversation

sherrif10 commented Jun 12, 2020

sherrif10 commented Jun 12, 2020

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

sherrif10 commented Jun 15, 2020 • edited Loading

sherrif10 commented Jun 17, 2020

samuelmale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherrif10 commented Sep 24, 2020

sherrif10 commented Sep 28, 2020

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

sherrif10 Sep 30, 2020 • 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

sherrif10 Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelmale left a comment

Choose a reason for hiding this comment

Akayeshmantha left a comment

Choose a reason for hiding this comment

sherrif10 commented Oct 2, 2020

mozzy11 commented Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

sherrif10 commented Oct 6, 2020

mozzy11 commented Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherrif10 commented Oct 19, 2020

sherrif10 commented Oct 22, 2020

dkayiwa commented Oct 22, 2020 • edited Loading

sherrif10 commented Oct 22, 2020

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

sherrif10 commented Oct 26, 2020 • edited Loading

sherrif10 commented Jun 15, 2020 •

edited

Loading

sherrif10 Sep 30, 2020 •

edited

Loading

sherrif10 Sep 30, 2020 •

edited

Loading

mozzy11 commented Oct 4, 2020 •

edited

Loading

mozzy11 commented Oct 7, 2020 •

edited

Loading

dkayiwa commented Oct 22, 2020 •

edited

Loading

sherrif10 commented Oct 26, 2020 •

edited

Loading