Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions omod-2.0/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@reagan-meant i undeclared that thanks

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved thanks

<reportingVersion>0.10.4</reportingVersion>
<serialization.xstreamVersion>0.2.12</serialization.xstreamVersion>
</properties>
Expand Down Expand Up @@ -110,7 +110,7 @@

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved thanks

<artifactId>webservices.rest-omod-common</artifactId>
<version>${webservices.restVersion}</version>
<scope>provided</scope>
</dependency>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,169 @@
package org.openmrs.module.attachments.rest;

import java.util.Arrays;
import java.util.List;
import org.hibernate.FlushMode;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import org.hibernate.FlushMode;

You have added an import that you are not using.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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. ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sherrif10 sherrif10 Sep 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Did you do as @sacull suggested above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i responded to his comments above

import org.openmrs.Obs;
import org.openmrs.api.context.Context;
import org.openmrs.module.attachments.AttachmentsService;
import org.openmrs.module.attachments.AttachmentsConstants;
import org.openmrs.module.attachments.obs.Attachment;
import org.openmrs.module.emrapi.db.DbSessionUtil;
Copy link
Member

Choose a reason for hiding this comment

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

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

import org.openmrs.module.webservices.rest.web.RequestContext;
import org.openmrs.module.attachments.AttachmentsService;
import org.openmrs.module.webservices.rest.web.RestConstants;
import org.openmrs.module.webservices.rest.web.annotation.Resource;
import org.openmrs.module.webservices.rest.web.representation.DefaultRepresentation;
import org.openmrs.module.webservices.rest.web.representation.FullRepresentation;
import org.openmrs.module.webservices.rest.web.representation.RefRepresentation;
import org.openmrs.module.webservices.rest.web.representation.Representation;
import org.openmrs.module.webservices.rest.web.resource.impl.DelegatingResourceDescription;
import org.openmrs.module.webservices.rest.web.response.GenericRestException;
import org.openmrs.module.webservices.rest.web.response.ResourceDoesNotSupportOperationException;
import org.openmrs.module.webservices.rest.web.response.ResponseException;
import io.swagger.models.Model;
import io.swagger.models.ModelImpl;
import io.swagger.models.properties.ArrayProperty;
import io.swagger.models.properties.BooleanProperty;
import io.swagger.models.properties.DateProperty;
import io.swagger.models.properties.DateTimeProperty;
import io.swagger.models.properties.IntegerProperty;
import io.swagger.models.properties.RefProperty;
import io.swagger.models.properties.StringProperty;

/**
* {@link Resource} for Attachment, supporting standard CRUD operations
*/
@Resource(name = RestConstants.VERSION_1 + "/attachment", supportedClass = Attachment.class, supportedOpenmrsVersions = {
"2.0.0" })
public class AttachmentResource2_0 extends AttachmentResource1_10 {

/**
* @see org.openmrs.module.webservices.rest.web.resource.impl.DelegatingCrudResource#getRepresentationDescription(org.openmrs.module.webservices.rest.web.representation.Representation)
*/
@Override
public DelegatingResourceDescription getRepresentationDescription(Representation rep) {
if (rep instanceof DefaultRepresentation) {
DelegatingResourceDescription description = new DelegatingResourceDescription();
description.addProperty("uuid");
description.addProperty("dateTime");
description.addProperty("comment");
description.addProperty("complexData");
description.addSelfLink();
description.addLink("full", ".?v=" + RestConstants.REPRESENTATION_FULL);
return description;
} else if (rep instanceof FullRepresentation) {
DelegatingResourceDescription description = new DelegatingResourceDescription();
description.addProperty("uuid");
description.addProperty("dateTime");
description.addProperty("comment");
description.addProperty("auditInfo");
description.addProperty("complexData");
description.addSelfLink();
return description;
} else if (rep instanceof RefRepresentation) {
DelegatingResourceDescription description = new DelegatingResourceDescription();
description.addProperty("uuid");
description.addProperty("comment");
description.addSelfLink();
return description;
}
return null;
sherrif10 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @see org.openmrs.module.webservices.rest.web.resource.impl.BaseDelegatingResource#getCreatableProperties()
*/
@Override
public DelegatingResourceDescription getCreatableProperties() {
DelegatingResourceDescription description = new DelegatingResourceDescription();
description.addRequiredProperty("uuid");
description.addRequiredProperty("display");
description.addProperty("comment");
description.addProperty("dateTime");
description.addProperty("complexData");

return description;
}

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

Choose a reason for hiding this comment

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

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

DelegatingResourceDescription description = new DelegatingResourceDescription();
description.addProperty("uuid");
description.addProperty("gender");
description.addProperty("display");
description.addProperty("comment");
description.addProperty("dateTime");
description.addRequiredProperty("complexData");
return description;
}

@Override
public Model getGETModel(Representation rep) {
sherrif10 marked this conversation as resolved.
Show resolved Hide resolved
ModelImpl model = (ModelImpl) super.getGETModel(rep);
if (rep instanceof DefaultRepresentation || rep instanceof FullRepresentation) {
model.property("uuid", new StringProperty()).property("display", new StringProperty())
.property("gender", new StringProperty()._enum("M")._enum("F")).property("age", new IntegerProperty())
.property("birthdate", new DateTimeProperty()).property("birthdateEstimated", new BooleanProperty())
.property("dead", new BooleanProperty()).property("deathDate", new DateProperty())
.property("causeOfDeath", new StringProperty())
.property("attributes", new ArrayProperty(new RefProperty("#/definitions/AttachmentAttributeGetRef")))
.property("voided", new BooleanProperty());
}
if (rep instanceof DefaultRepresentation) {
model.property("preferredName", new RefProperty("#/definitions/AttachementNameGetRef"))
.property("preferredAddress", new RefProperty("#/definitions/AttachementAddressGetRef"));

} else if (rep instanceof FullRepresentation) {
model.property("preferredName", new RefProperty("#/definitions/AttachmentNameGet"))
.property("preferredAddress", new RefProperty("#/definitions/AttachmentAddressGet"))
.property("names", new ArrayProperty(new RefProperty("#/definitions/AttachmentNameGet")))
.property("addresses", new ArrayProperty(new RefProperty("#/definitions/AttachmentAddressGet")));
}
return model;
}

@Override
public Model getCREATEModel(Representation representation) {
ModelImpl model = new ModelImpl()
.property("names", new ArrayProperty(new RefProperty("#/definitions/AttachmentNameCreate")))
.property("gender", new StringProperty()._enum("M")._enum("F")).property("age", new IntegerProperty())
.property("birthdate", new DateProperty())
.property("birthdateEstimated", new BooleanProperty()._default(false))
.property("dead", new BooleanProperty()._default(false)).property("deathDate", new DateProperty())
.property("causeOfDeath", new StringProperty())
.property("addresses", new ArrayProperty(new RefProperty("#/definitions/AttachmentAddressCreate")))
.property("attributes", new ArrayProperty(new RefProperty("#/definitions/AttachmentAttributeCreate")));

model.setRequired(Arrays.asList("names", "gender"));
return model;
}

@Override
public Model getUPDATEModel(Representation representation) {
return new ModelImpl().property("dead", new BooleanProperty()).property("causeOfDeath", new StringProperty())
.property("deathDate", new DateProperty()).property("age", new IntegerProperty())
.property("gender", new StringProperty()._enum("M")._enum("F")).property("birthdate", new DateProperty())
.property("birthdateEstimated", new BooleanProperty()._default(false))
.property("preferredName", new StringProperty().example("uuid"))
.property("preferredAddress", new StringProperty().example("uuid"))
.property("attributes", new ArrayProperty(new RefProperty("#/definitions/AttachmentAttributeCreate")))
.required("dead").required("causeOfDeath");
}

/**
* @see org.openmrs.module.webservices.rest.web.resource.impl.BaseDelegatingResource#getPropertiesToExposeAsSubResources()
*/
@Override
public List<String> getPropertiesToExposeAsSubResources() {
return Arrays.asList("comment", "complexData", "Attributes");
}

Comment on lines +159 to +163
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sherrif10 sherrif10 Sep 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@Override
public Attachment save(Attachment delegate) {
return Context.getService(AttachmentsService.class).save(delegate, AttachmentResource1_10.REASON);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package org.openmrs.module.attachments.rest;

import org.junit.Before;
import org.mockito.Mock;
import org.openmrs.api.ObsService;
import org.openmrs.api.context.Context;
import org.openmrs.module.attachments.AttachmentsService;
import org.openmrs.module.attachments.obs.Attachment;
import org.openmrs.module.webservices.rest.web.RequestContext;
import org.openmrs.module.webservices.rest.web.resource.impl.BaseDelegatingResourceTest;
import org.springframework.beans.factory.annotation.Autowired;

public class AttachmentResource2_0Test extends BaseDelegatingResourceTest<AttachmentResource2_0, Attachment> {

private static final String ATTACHMENTRESOURCE_UUID = "9b6639b2-5785-4603-a364-075c2d61cd51";

@Mock
AttachmentsService attachmentService;

Attachment attachment;

@Autowired
private ObsService obsService;

@Mock
RequestContext requestContext;

@Before
public void before() throws Exception {
executeDataSet("org/openmrs/api/include/ObsServiceTest-complex.xml");
}

@Override
public String getDisplayProperty() {
// TODO Auto-generated method stub
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved thanks

return null;
}

@Override
public String getUuidProperty() {
return ATTACHMENTRESOURCE_UUID;
}

@Override
public Attachment newObject() {
return new Attachment(Context.getObsService().getObsByUuid(ATTACHMENTRESOURCE_UUID));

}

@Override
public void validateDefaultRepresentation() throws Exception {
super.validateDefaultRepresentation();
assertPropEquals("uuid", getObject().getUuid());
assertPropEquals("dateTime", getObject().getDateTime());
assertPropEquals("comment", getObject().getComment());
assertPropEquals("complexData", getObject().getComplexData());

}

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

assertPropEquals("uuid", getObject().getUuid());
assertPropEquals("dateTime", getObject().getDateTime());
assertPropEquals("comment", getObject().getComment());
assertPropEquals("datechanged", getObject().getDateChanged());
assertPropEquals("complexData", getObject().getComplexData());

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import java.util.stream.Collectors;

import org.apache.commons.beanutils.PropertyUtils;
import org.codehaus.jackson.map.ObjectMapper;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.openmrs.Obs;
Expand All @@ -20,16 +22,24 @@
import org.openmrs.module.attachments.obs.Attachment;
import org.openmrs.module.attachments.obs.TestAttachmentBytesViewHandler;
import org.openmrs.module.attachments.obs.TestHelper;
import org.openmrs.module.webservices.rest.SimpleObject;
import org.openmrs.module.webservices.rest.test.Util;
import org.openmrs.module.webservices.rest.web.RequestContext;
import org.openmrs.module.webservices.rest.web.resource.impl.BasePageableResult;
import org.openmrs.module.webservices.rest.web.response.ResourceDoesNotSupportOperationException;
import org.openmrs.module.webservices.rest.web.v1_0.controller.MainResourceControllerTest;
import org.openmrs.obs.ComplexObsHandler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.web.bind.annotation.RequestMethod;

public class AttachmentRestController2_0Test extends MainResourceControllerTest {

private static final Logger log = LoggerFactory.getLogger(Attachment.class);

@Autowired
private AttachmentsService as;

Expand Down Expand Up @@ -94,6 +104,90 @@ public void shouldGetRefByUuid() {
public void shouldGetFullByUuid() {
}

@Test
public void createAttachment_shouldCreateANewAttachment() throws Exception {

SimpleObject attachment = new SimpleObject();
attachment.add("uuid", obs.getUuid());
attachment.add("comment", obs.getComment());
attachment.add("complexData", obs.getComplexData());

String json = "{\"uuid\":\"" + getUuid() + "\",\"attachment\":\"" + attachment + "\"}";

try {
json = new ObjectMapper().writeValueAsString(attachment);
}
catch (IOException e) {
log.error("attachment created", e);
}

MockHttpServletRequest req = request(RequestMethod.POST, getURI());
req.setContent(json.getBytes());

SimpleObject result = null;
try {
result = deserialize(handle(req));
}
catch (Exception e) {
log.error("Attachment failed to be created!", e);
}

Util.log("Created attachment", result);

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah i actually tested the created attachment below with ignorecase character

Assert.assertNotEquals("Created complexData ", createdAttachment.equalsIgnoreCase(createdAttachment));
Copy link
Member

Choose a reason for hiding this comment

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

@sherrif10 what are you trying to test here

Copy link
Member Author

Choose a reason for hiding this comment

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

The created Attachment , so basically this tests above string


}

@Test
public void getAttachment_shouldGetADefaultRepresentationOfAttachment() throws Exception {

MockHttpServletRequest req = request(RequestMethod.GET, getURI() + "/" + getUuid());
SimpleObject result = deserialize(handle(req));

Assert.assertNotNull(result);
Util.log("Attachment fetched (default)", result);
Assert.assertEquals(getUuid(), result.get("uuid"));
}

@Test
public void updateAttachment_shouldChangeAPropertyOnAttachment() throws Exception {

SimpleObject attributes = new SimpleObject();
attributes.add("complexData", "update complextData");
attributes.add("deathDate", "Updated deathDate");
attributes.add("prefferedName", "Updated prefferedName");
attributes.add("gender", "updated gender");
attributes.add("preferredAddress", "updated preferredAddress");

String json = "{\"uuid\":\"" + getUuid() + "\",\"attributes\":\"" + attributes + "\"}";

try {
json = new ObjectMapper().writeValueAsString(attributes);
}
catch (IOException e) {
log.error("Attachments deserialised!", e);
}

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());

SimpleObject result = null;
try {
result = deserialize(handle(req));
}
catch (Exception e) {
log.error("attachment failed to be updated!", e);
}

Assert.assertNull(result);
String editedAttachment = obs.getUuid();
Copy link
Member

Choose a reason for hiding this comment

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

@sherrif10 ,is this the edited attachment object ??

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Assert.assertNotEquals("Updated complexData ", editedAttachment.equalsIgnoreCase(editedAttachment));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@mozzy11 ok no problem

}

@Test
public void postAttachment_shouldUpdateObsComment() throws Exception {
// Setup
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you now test this with the latest version

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

<emrapiVersion>1.19</emrapiVersion>
<reportingVersion>0.9.2.1</reportingVersion>
<calculationVersion>1.1</calculationVersion>
Expand Down