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.

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

Jump to bottom

Bug when retrieving non-active(soft deleted) patient identifiers #304

Merged
merged 1 commit into from
Sep 28, 2017
Merged

Bug when retrieving non-active(soft deleted) patient identifiers #304

merged 1 commit into from
Sep 28, 2017

Conversation

chibujax
Copy link
Contributor

JIRA TICKET NAME:

RESTWS-675: Fix inability to retrieve soft deleted patient identifiers

SUMMARY:

This PR fixes Bug when retrieving non-active(soft deleted) patient identifiers

@chibujax
Copy link
Contributor Author

The 1.9 does not accept ?includeAll=true
So fails the test.

dkayiwa
dkayiwa reviewed Sep 26, 2017
View reviewed changes
...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/PatientIdentifierResource1_8.java Outdated
@@ -248,7 +250,13 @@ public void purge(PatientIdentifier delegate, RequestContext context) throws Res
*/
@Override
public NeedsPaging<PatientIdentifier> doGetAll(Patient parent, RequestContext context) throws ResponseException {
return new NeedsPaging<PatientIdentifier>(parent.getActiveIdentifiers(), context);
List<PatientIdentifier> newPatientIdentifier;
Copy link
Member

Choose a reason for hiding this comment

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

Don't you think patientIdentifiers looks better than newPatientIdentifier?

dkayiwa
dkayiwa reviewed Sep 26, 2017
View reviewed changes
...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java Outdated
public void shouldListAllPatientIdentifiersWithVoidedIdentifiersForAPatient() throws Exception {
String urlString = getURI();
urlString += "?includeAll=true";
MockHttpServletRequest req = request(RequestMethod.GET, urlString);
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 do the includeAll parameter as here? https://github.com/openmrs/openmrs-module-webservices.rest/blob/master/omod-1.9/src/test/java/org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/VisitAttributeTypeController1_9Test.java#L118-L119

@chibujax chibujax force-pushed the RESTWS-675 branch 2 times, most recently from 52cfa64 to 7869de8 Compare September 27, 2017 11:50
@chibujax
Copy link
Contributor Author

Feedback has been implemented.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 40.385% when pulling 7869de8 on andela-cuchendu:RESTWS-675 into bba5380 on openmrs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 40.385% when pulling 7869de8 on andela-cuchendu:RESTWS-675 into bba5380 on openmrs:master.

dkayiwa
dkayiwa reviewed Sep 27, 2017
View reviewed changes
...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java Outdated
assertEquals(false, service.getPatientIdentifierByUuid(getUuid()).isVoided());
MockHttpServletRequest req = request(RequestMethod.DELETE, getURI() + "/" + getUuid());
req.addParameter("!purge", "");
final String reason = "none";
Copy link
Member

Choose a reason for hiding this comment

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

If your changes in PatientIdentifierResource1_8.java were removed, would this test fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because getAllCount would return 1

dkayiwa
dkayiwa reviewed Sep 27, 2017
View reviewed changes
...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java Outdated
MockHttpServletRequest req = request(RequestMethod.DELETE, getURI() + "/" + getUuid());
req.addParameter("!purge", "");
final String reason = "none";
req.addParameter("testing voided", reason);
Copy link
Member

Choose a reason for hiding this comment

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

What is the name of the above parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean "Testing voided" ?

Copy link
Member

Choose a reason for hiding this comment

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

Am referring to line 134

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason.

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 look at the documentation of this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

dkayiwa
dkayiwa reviewed Sep 27, 2017
View reviewed changes
...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java Outdated
public void shouldListAllPatientIdentifiersWithVoidedIdentifiersForAPatient() throws Exception {
assertEquals(false, service.getPatientIdentifierByUuid(getUuid()).isVoided());
MockHttpServletRequest req = request(RequestMethod.DELETE, getURI() + "/" + getUuid());
req.addParameter("!purge", "");
Copy link
Member

Choose a reason for hiding this comment

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

What is the use of the above parameter on line 132?

Copy link
Contributor Author

@chibujax chibujax Sep 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Void, I guess it can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean it is the way of making the request void the data?

dkayiwa
dkayiwa reviewed Sep 27, 2017
View reviewed changes
...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java Outdated
assertEquals(true, service.getPatientIdentifierByUuid(getUuid()).isVoided());
SimpleObject result = deserialize(handle(newGetRequest(getURI(), new Parameter(
RestConstants.REQUEST_PROPERTY_FOR_INCLUDE_ALL, "true"))));
assertNotEquals(getAllCount(), Util.getResultsSize(result));
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 make another call and corresponding assertion where the includeAll parameter has a value of false?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 40.385% when pulling 6e3e174 on andela-cuchendu:RESTWS-675 into bba5380 on openmrs:master.

@chibujax
Copy link
Contributor Author

I have implemented that by calling with true then testing

assertNotEquals(getAllCount(),Util.getResultsSize(result));

since getAllCount() will return 1 and getAllWithVoided will return 2 which includes the voided

for false

assertEquals(getAllCount(),Util.getResultsSize(nextResult));

since getAllCount() will return one and getAllWithVoided(false) will be 1 excluding the voided

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 40.385% when pulling f8c389e on andela-cuchendu:RESTWS-675 into bba5380 on openmrs:master.

dkayiwa
dkayiwa reviewed Sep 28, 2017
View reviewed changes
...dule/webservices/rest/web/v1_0/controller/openmrs1_9/PatientIdentifierController1_9Test.java
assertNotEquals(getAllCount(), Util.getResultsSize(result));

SimpleObject nextResult = deserialize(handle(newGetRequest(getURI(), new Parameter(
RestConstants.REQUEST_PROPERTY_FOR_INCLUDE_ALL, "false"))));
Copy link
Member

Choose a reason for hiding this comment

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

Isn't getAllCount() the one that has all, including the voided ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Get all returns only the active Identifiers

return service.getPatientByUuid(RestTestConstants1_8.PATIENT_UUID).getActiveIdentifiers().size();

That is why getAllCount() returns 1 after we void an identifier instead of two but REQUEST_PROPERTY_FOR_INCLUDE_ALL returns 2 including the voided.

Copy link
Member

Choose a reason for hiding this comment

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

Oh i see! 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes 😄 Is there any other thing I should do?

@dkayiwa dkayiwa merged commit 49ed4dc into openmrs:master Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkayiwa dkayiwa dkayiwa left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chibujax @coveralls @dkayiwa