RESTWS-702 : Add support for unvoiding/unretiring resources by Ruhanga · Pull Request #373 · openmrs/openmrs-module-webservices.rest

Ruhanga · 2019-01-04T12:09:42Z

RESTWS-702 Add support for unvoiding/unretiring resources

Description of what I changed

Updated "Deletable" interface to also support: undelete(String uuid, RequestContext context) throws ResponseException;

Made DelegatingCrudeResource class to implement this method (undelete(String uuid, RequestContext context) throws ResponseException;)

Updated BaseDelegatingResource class to have a protected unDelete(Object, RequestContext) which temporarily/by-default provides for an alternative message for the classes that have not yet overridden this method. This pattern follows the design used by delete(Object, String, RequestContext) throws ResponseException; method in the same class.

Overrode the unDelete(Object, RequestContext) for the following Resources:

-> PatientResource1_8
-> PersonResource1_8
-> ObsResource1_8
-> OrderResource1_8
-> EncounterResource1_8

I am welcoming comments and enhancements on resources connected to Voidable Openmrs Objects to include.

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-702

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
My IDE is configured to follow the code style of this project.
I have added tests to cover my changes.
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.

mhawila · 2019-01-04T15:53:49Z

Hi @Ruhanga I was skimming really quick through your code and I see you have named your method unDelete. I think undelete is simply one word hence the method names do not need to be camelized they way you did. My two cents ( or just one 😄 )

djazayeri

Made some comments.

djazayeri · 2019-01-04T19:36:40Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/resource/api/Deletable.java

+	 * @return unvoided/unretired object
+	 * @since 2.2.0
+	 */
+	Object unDelete(String uuid, RequestContext context) throws ResponseException;


should be capitalized like undelete (no capital D). E.g. for consistency with unvoid, unretire, etc.

I made changes accordingly

djazayeri · 2019-01-04T19:38:49Z

.../main/java/org/openmrs/module/webservices/rest/web/resource/impl/BaseDelegatingResource.java

+	 * @return Object
+	 */
+
+	// TODO make this method an abstruct method when all subclasses have overriden it


Can't merge code that has a TODO in it. Remove this comment (and the blank lines).

If there is no base implementation of this the method should be abstract, just like delete. Are there many other resources that would need this fixed?

I deleted the TODO and empty line but left the default implementation in place because I think this will help the end user trouble shoot problems that may come with existing resources that may not be part of the rest.webservices module and don't have their own implementation for this already.

djazayeri · 2019-01-04T19:40:26Z

...ain/java/org/openmrs/module/webservices/rest/web/v1_0/controller/MainResourceController.java

-		return RestUtil.updated(response, updated);
+
+		if (post.get("deleted") != null && "false".equals(post.get("deleted")) && post.size() == 1) {
+			//This branching provides for unvoiding or unretiring a resource 


This comment is obvious from context, so you don't need to explicitly state this.

I deleted the comment

djazayeri · 2019-01-04T19:43:14Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/EncounterResource1_8.java

+	protected Object unDelete(Encounter enc, RequestContext context) {
+		Encounter unvoidedEnc = null;
+		if (!enc.isVoided()) {
+			unvoidedEnc = Context.getEncounterService().getEncounter(enc.getEncounterId());


You already have "Encounter enc" passed into the method. Why do you need to fetch it again? Can't you just do

if (enc.isVoided()) { // unvoid it } return // convert

Thank you for this observation, I have adjusted the code accordingly.

Ruhanga · 2019-01-06T09:47:53Z

I am going ahead to include the rest of the resources that are Deletable .

Ruhanga · 2019-01-10T16:01:04Z

What I have done

-Included remaining resources to unvoid/unretire that are unretireable including MetaData related resources
-Added more Unit tests for the changes made for the following resources....

DelegatingCrudeResource s

-EncounterResource1_8
-ObsResource1_8
-OrderResource1-8
-PatientResource1_8
-PersonResource1_8
-ProgramEnrollment1_8
-RelationshipResource1_8
-VisitResource1_8

MetaDataDelegatingCrudResource s

-ConceptClassResource1_8
-ConceptSourceResource1_8
-EncounterType1_8
-FieldTypeResource1_8
-FormResource1_8

dkayiwa · 2019-01-17T17:23:58Z

@Ruhanga did you address all the changes requested by @djazayeri ?

Ruhanga · 2019-01-18T10:12:24Z

@dkayiwa , yes I addressed the requests for changes from @djazayeri .

dkayiwa · 2019-01-21T18:58:54Z

.../main/java/org/openmrs/module/webservices/rest/web/resource/impl/DelegatingCrudResource.java

+	 *      org.openmrs.module.webservices.rest.web.RequestContext)
+	 */
+	@Override
+	public Object unDelete(String uuid, RequestContext context) throws ResponseException {


Didn't i see comments about renaming these methods?

@dkayiwa , sorry for the late response, yes you did see the comments and I made the changes in this second commit cd12521 after the first commit (054593f). I think I have to squash the commits into one. But just for reference purposes I left this out.

@Ruhanga can you please squash your commits into one?

Thank you @samuelmale , commits have been squashed into one.

samuelmale · 2019-02-06T17:59:28Z

.../main/java/org/openmrs/module/webservices/rest/web/resource/impl/BaseDelegatingResource.java

+	 * @return Object
+	 */
+	protected T undelete(T delegate, RequestContext context) throws ResponseException {
+		//Default implimentation of this method if not overriden by sub-class is to raise an 


Can you fix this typo "implimentation"?

Thank you @samuelmale , I've got this resolved.

dkayiwa · 2019-02-22T11:32:40Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/resource/api/Deletable.java

+	 * @param context
+	 * @throws ResponseException
+	 * @return unvoided/unretired object
+	 * @since 2.2.0


2.2.0 is a version of what?

Thank you very much for pointing out this @dkayiwa , it was supposed to be 2.25.0 (release version to contain these changes). I have updated the commit to reflect the correction. I initially thought the version to be compared against openmrs-core.

dkayiwa · 2019-03-05T10:30:02Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/resource/api/Deletable.java

+	 * Undeletes the specified resource, which in the OpenMRS context means either unvoiding or
+	 * unretiring it
+	 * 
+	 * @param uuid


Did you just forget to document these params?

You mean uuid and context ? If so what in their documentation is missing?

That was quick, the description is missing. But This is how the other params I have seen in this module being documented.

Did you do some googling about javadoc?

Definitely yes, and in this case I had to remind myself. Good convention is to include the @params's description. Gona do just that. Thank you @dkayiwa .

dkayiwa · 2019-03-05T10:39:29Z

...rs/module/webservices/rest/web/v1_0/controller/openmrs1_8/ConceptClassController1_8Test.java

+		conceptClass.setDateRetired(new Date());
+		conceptClass.setRetireReason("random reason");
+		service.saveConceptClass(conceptClass);
+		Assert.assertTrue(conceptClass.isRetired());


Will the above assertion fail if line 105 is commented out?

Definitely not, what was in my mind though was conceptClass = service.saveConceptClass(conceptClass);. I am refactoring to reflect this.

dkayiwa · 2019-03-05T10:52:42Z

...ain/java/org/openmrs/module/webservices/rest/web/v1_0/controller/MainResourceController.java

+			return RestUtil.updated(response, undeletedRes);
+		}
+		else {
+			Updatable res = (Updatable) restService.getResourceByName(buildResourceName(resource));


What happens if you have the deleted property together with others?

Also what happens when the property deleted has a value of true?

An Exception similar to all other invalid properties is thrown, i.e conversion Exception.

Do you have a test to confirm so?

This test should be part of work done earlier since this is not the first time this possibility exists.

dkayiwa · 2019-03-05T11:12:51Z

Did you confirm that you have added this for all resources and a corresponding test for each?

Ruhanga · 2019-03-05T13:03:20Z

Yes, apart from Condition and Diagnosis which have a different implementation.

dkayiwa · 2019-03-05T20:42:35Z

...s/module/webservices/rest/web/v1_0/controller/openmrs1_8/EncounterTypeController1_8Test.java

+	public void shouldUnRetireAnEncounterType() throws Exception {
+		EncounterType encounterType = service.getEncounterTypeByUuid(getUuid());
+		encounterType.setRetired(true);
+		encounterType.setRetiredBy(Context.getAuthenticatedUser());


I guess you do not need to set retiredBy, retireReason, and dateRetired.

Yes, thank you for pointing out this @dkayiwa , these methods are deprecated apart from setRetireReason().

dkayiwa · 2019-03-05T20:43:30Z

...le/webservices/rest/web/v1_0/controller/openmrs1_8/PersonAttributeTypeController1_8Test.java

+		obj.setRetiredBy(Context.getAuthenticatedUser());
+		obj.setDateRetired(new Date());
+		obj.setRetireReason("random reason");
+		service.savePersonAttributeType(obj);


What happens if you comment out the above line?

This has been resolve.

dkayiwa · 2019-03-05T20:44:09Z

...odule/webservices/rest/web/v1_0/controller/openmrs1_8/RelationshipTypeController1_8Test.java

+		relationshipType.setRetiredBy(Context.getAuthenticatedUser());
+		relationshipType.setDateRetired(new Date());
+		relationshipType.setRetireReason("random reason");
+		service.saveRelationshipType(relationshipType);


What happens if you comment out the above line?

This has been resolved.

dkayiwa · 2019-03-06T20:09:20Z

.../webservices/rest/web/v1_0/controller/openmrs1_8/PatientIdentifierTypeController1_8Test.java

+		patientIdentifierType.setRetiredBy(Context.getAuthenticatedUser());
+		patientIdentifierType.setDateRetired(new Date());
+		patientIdentifierType.setRetireReason("random reason");
+		service.savePatientIdentifierType(patientIdentifierType);


What happens when the above line is commented out?
Am now asking this same question for the third time. Do you take time to review your changes?

Apologies on this @dkayiwa , I think I have been following my not up-to date list of the changes and resources affected as I have made on this pull request. The next commit will consider everything is fine. Thank you very much.

dkayiwa · 2019-03-13T11:11:02Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/resource/api/Deletable.java

+	 * Undeletes the specified resource, which in the OpenMRS context means either unvoiding or
+	 * unretiring it
+	 * 
+	 * @param uuid Uuid to the delegate to unvoid/unretire


What does "delegate" mean?

Delegate in my thinking as I have seen it being used with respect to this module, is a domain object.

What has that got to do with the API consumer?

I think to make this doc safe, I'l replace "delegate" with "resource" instead.

dkayiwa · 2019-03-13T11:14:55Z

...g/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/VisitController1_9Test.java


+	@Test
+	public void shouldUnVoidAVisit() throws Exception {
+		Visit visit = service.getVisitByUuid(RestTestConstants1_9.VISIT_UUID);


Can you try void an existing visit in any of the datasets used for VisitServiceTest in the core platform?

I think it's not possible since this setting is in a Web and Module context sensitive environment. Unless I duplicate the datasets into this module.

When documenting, avoid exposing internal implementation details to the client.

Thank you @dkayiwa , I've taken note of this.

dkayiwa · 2019-03-13T11:17:42Z

...openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/ProgramController1_9Test.java

+		program.setRetireReason("random reason");
+		program = service.saveProgram(program);
+		Assert.assertTrue(program.isRetired());
+


Can you first make a REST call and confirm that it is indeed retired?

I have used Service.get.......ByUuid() which is implicitly called on any GET request for a given resource as suggested somewhere on this PR.

dkayiwa · 2019-03-13T11:21:45Z

...le/webservices/rest/web/v1_0/controller/openmrs1_8/PersonAttributeTypeController1_8Test.java

+	public void shouldUnRetireAPersonAttributeType() throws Exception {
+
+		final String nonRetiredAttribute = "a0f5521c-dbbd-4c10-81b2-1b7ab18330df";
+		PersonAttributeType obj = service.getPersonAttributeTypeByUuid(nonRetiredAttribute);


What does obj mean?

Just a variable name. Meaning comes from its type definition → PersonAttributeType .

Can you do some googling about good variable names?

Thank you @dkayiwa , this helped as it usually does. I going to rename this for better meaning.

dkayiwa · 2019-03-13T11:23:19Z

.../webservices/rest/web/v1_0/controller/openmrs1_8/PatientIdentifierTypeController1_8Test.java

+		PatientIdentifierType patientIdentifierType = service.getPatientIdentifierTypeByUuid(getUuid());
+		patientIdentifierType.setRetired(true);
+		patientIdentifierType.setRetireReason("random reason");
+		patientIdentifierType = service.savePatientIdentifierType(patientIdentifierType);


Generally, instead of asserting using the returned object, i would prefer fetching it afresh using getPatientIdentifierTypeByUuid and then assert that it is retired

I agree @dkayiwa . Let me refactor to reflect these changes.

dkayiwa · 2019-03-19T14:18:40Z

@Ruhanga did you try run your changes and test using an interface like this to unretire a concept?

Ruhanga · 2019-03-19T16:52:32Z

@dkayiwa , this was not implemented for the Concept resource because there is currently no Service layer method to un-retire a Concept.

dkayiwa · 2019-03-19T17:04:10Z

Do we have a ticket for it?

Ruhanga · 2019-03-19T17:26:41Z

@dkayiwa , there's actually never been any ticket for or against this, I guess I have to create or rather seek consent from talk to create a ticket that addresses the Unretiring of a Concept .

dkayiwa · 2019-03-19T18:11:44Z

@Ruhanga did you try run your changes and test using an interface like this https://demo.openmrs.org/openmrs/module/webservices/rest/test.htm to unretire a patient?

Ruhanga · 2019-03-20T05:48:14Z

Okay, I hadn't @dkayiwa . Let me hard test these changes on a running instance. Thank you.

Ruhanga · 2019-03-20T12:57:36Z

@dkayiwa , for the far I have gone unretiring a resource is possible including the patient one.

dkayiwa · 2019-03-20T13:01:06Z

Can you share some screenshots on the JIRA ticket?

Ruhanga · 2019-03-20T15:24:32Z

Cool @dkayiwa .

djazayeri requested changes Jan 4, 2019

View reviewed changes

Ruhanga force-pushed the RESTWS-702 branch from be34131 to cd12521 Compare January 10, 2019 15:47

dkayiwa reviewed Jan 21, 2019

View reviewed changes

Ruhanga force-pushed the RESTWS-702 branch from cd12521 to 883510f Compare February 6, 2019 13:51

samuelmale reviewed Feb 6, 2019

View reviewed changes

dkayiwa reviewed Feb 22, 2019

View reviewed changes

Ruhanga force-pushed the RESTWS-702 branch from 883510f to 87b00d2 Compare February 26, 2019 14:07

dkayiwa reviewed Mar 5, 2019

View reviewed changes

Ruhanga force-pushed the RESTWS-702 branch from 87b00d2 to e9b8c7d Compare March 5, 2019 14:28

dkayiwa reviewed Mar 5, 2019

View reviewed changes

Ruhanga force-pushed the RESTWS-702 branch from b1c74ba to e2ca79a Compare March 6, 2019 13:41

dkayiwa reviewed Mar 6, 2019

View reviewed changes

Ruhanga force-pushed the RESTWS-702 branch from e2ca79a to 3022a5a Compare March 12, 2019 06:30

dkayiwa reviewed Mar 13, 2019

View reviewed changes

RESTWS-702 : Add support for unvoiding/unretiring resources

e287235

Ruhanga force-pushed the RESTWS-702 branch from 9e2a78f to e287235 Compare March 14, 2019 14:30

dkayiwa merged commit bff28f9 into openmrs:master Mar 21, 2019

Conversation

Ruhanga commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

mhawila commented Jan 4, 2019

Uh oh!

djazayeri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ruhanga commented Jan 6, 2019

Uh oh!

Ruhanga commented Jan 10, 2019

Uh oh!

dkayiwa commented Jan 17, 2019

Uh oh!

Ruhanga commented Jan 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkayiwa commented Mar 5, 2019

Uh oh!

Ruhanga commented Mar 5, 2019

Uh oh!

Ruhanga commented Jan 4, 2019 •

edited

Loading