Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4e0bf0f
ATT-42: AttachmentResource* to be documented in Swagger
sherrif10 Jun 12, 2020
afc7922
Removing unwanted commits
sherrif10 Sep 24, 2020
f05a918
Resolving representations in swagger
sherrif10 Sep 24, 2020
67b1577
Removing unncessary comments
sherrif10 Sep 24, 2020
937e7c4
resolving commit
sherrif10 Sep 29, 2020
7af99d6
removing unwanted commits
sherrif10 Sep 30, 2020
9cc8876
adding some tests
sherrif10 Oct 6, 2020
59928cb
Adding some unit tests in rest controller
sherrif10 Oct 7, 2020
4a05f33
resolving deserialised objects with loglevels
sherrif10 Oct 26, 2020
809fb57
Merge branch 'master' into ATT-42
sherrif10 Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions omod-2.0/pom.xml
View file
Open in desktop
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
181 changes: 181 additions & 0 deletions omod-2.0/src/main/java/org/openmrs/module/attachments/rest/AttachmentResource2_0.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,197 @@
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 •
edited
Loading

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

/**
* @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("display");
description.addProperty("comment");
description.addProperty("dateTime");

description.addRequiredProperty("complexData");

return description;
}

@Override
public Model getGETModel(Representation rep) {
ModelImpl model = (ModelImpl) super.getGETModel(rep);
if (rep instanceof DefaultRepresentation || rep instanceof FullRepresentation) {
model.property("uuid", new StringProperty()).property("display", new StringProperty())

.property("comment", new BooleanProperty()).property("Datetime", new DateProperty())
.property("complexData", new StringProperty())

.addProperty("voided", new BooleanProperty());
}
if (rep instanceof DefaultRepresentation) {
model.property("complexData", new RefProperty("#/definitions/AttachmentComplexDataGetRef")).property("comment",
new RefProperty("#/definitions/AttachmentCommentGetRef"));

} else if (rep instanceof FullRepresentation) {
model.property("display", new RefProperty("#/definitions/AttachmentDisplayGet"))
.property("comment", new RefProperty("#/definitions/AttachmentCommentGet"))
.property("datetime", new ArrayProperty(new RefProperty("#/definitions/AttachmentDateTimeGet")))
.property("complexData", new ArrayProperty(new RefProperty("#/definitions/AttachmentComplexDataGet")));
}
return model;
}

@Override
public Model getCREATEModel(Representation representation) {
ModelImpl model = new ModelImpl()
.property("display", new ArrayProperty(new RefProperty("#/definitions/AttachmentDisplayCreate")))

.property("dataTime", new DateProperty())

.property("comment", new ArrayProperty(new RefProperty("#/definitions/AttachmentCommentCreate")))
.property("complexData", new ArrayProperty(new RefProperty("#/definitions/AttachmentComplexDataCreate")));

model.setRequired(Arrays.asList("comment", "datetime"));
return model;
}

@Override
public Model getUPDATEModel(Representation representation) {
return new ModelImpl().property("uuid", new BooleanProperty()).property("comment", new StringProperty())
.property("dateTime", new DateProperty())

Copy link

Choose a reason for hiding this comment

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

Is this space necessary ?

.property("complexData", new ArrayProperty(new RefProperty("#/definitions/AttachmentComplexDataCreate")));

}

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

@Override
public Attachment newDelegate() {
return new Attachment();
}

// @Override
// public Attachment save(Attachment delegate) {
// Obs obs = Context.getObsService().saveObs(delegate.getObs(), REASON);
// return new Attachment(obs);
// }
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 intend to the leave commented code?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@SuppressWarnings("deprecation")
@Override
public Attachment getByUniqueId(String uniqueId) {
Obs obs = Context.getObsService().getObsByUuid(uniqueId);
if (!obs.isComplex())
throw new GenericRestException(uniqueId + " does not identify a complex obs.", null);
else {
obs = Context.getObsService().getComplexObs(obs.getId(), AttachmentsConstants.ATT_VIEW_CRUD);
return new Attachment(obs);
}
}

@Override
protected void delete(Attachment delegate, String reason, RequestContext context) throws ResponseException {
String encounterUuid = delegate.getObs().getEncounter() != null ? delegate.getObs().getEncounter().getUuid() : null;
Context.getObsService().voidObs(delegate.getObs(), REASON);
voidEncounterIfEmpty(Context.getEncounterService(), encounterUuid);
}

@Override
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);
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@samuelmale thanks , let me cross check that code

Copy link
Member Author

Choose a reason for hiding this comment

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

I have resolved that , take a look


@Override
public Attachment save(Attachment delegate) {
FlushMode flushMode = DbSessionUtil.getCurrentFlushMode();
Expand All @@ -26,4 +206,5 @@ public Attachment save(Attachment delegate) {
}
return attachment;
}

}
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
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.12</emrapiVersion>
<reportingVersion>0.9.2.1</reportingVersion>
<calculationVersion>1.1</calculationVersion>
Expand Down