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

COH-55: Changed Boolean Conversion Code #32

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Conversation

WodPachua
Copy link
Contributor

@WodPachua WodPachua commented Aug 31, 2023

@WodPachua
Copy link
Contributor Author

Hello @dkayiwa Requesting an initial review of the changes made

@reagan-meant
Copy link

Thank you @WodPachua for the fix. Are you in position to ensure this works by testing if the purge works. You could use postman for this

@@ -134,7 +134,7 @@ public CohortMember getByUniqueId(String uuid) {

@Override
public void purge(CohortMember cohortMember, RequestContext context) throws ResponseException {
boolean purge = Boolean.getBoolean(context.getParameter("purge"));
boolean purge = Boolean.parseBoolean(context.getParameter("purge"));
Copy link
Member

Choose a reason for hiding this comment

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

Fix should be much simpler: just unconditionally call purgeCohortMember. The framework already handles the purge=true bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, okay, let me fix that. Thanks @ibacher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibacher I have made the changes here, requesting a review if the fix is done correctly

@WodPachua
Copy link
Contributor Author

Thank you @WodPachua for the fix. Are you in position to ensure this works by testing if the purge works. You could use postman for this

Let me find out how to do this also. Thank you @reagan-meant

@WodPachua WodPachua changed the title COH-55: Changes Boolean Conversion Code COH-55: Changed Boolean Conversion Code Aug 31, 2023
Copy link
Member

@wikumChamith wikumChamith left a comment

Choose a reason for hiding this comment

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

Did you manually test the changes?

@@ -134,12 +134,7 @@ public CohortMember getByUniqueId(String uuid) {

@Override
public void purge(CohortMember cohortMember, RequestContext context) throws ResponseException {
boolean purge = Boolean.getBoolean(context.getParameter("purge"));
if (purge) {
Context.getService(CohortMemberService.class).purgeCohortMember(cohortMember);
Copy link
Member

Choose a reason for hiding this comment

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

Should also be formatted so this is no longer indented.

Copy link
Contributor Author

@WodPachua WodPachua Sep 5, 2023

Choose a reason for hiding this comment

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

Oh sure @ibacher . Though after testing, manually with postman & the legacy UI Webservices Test page for REST URIs, its returns a "success" response but still does the normal delete and the cohort member remains in the database. Have tried tracing the code flow and can't seem to figure out why it still isn't purging.
image
image

Copy link
Member

Choose a reason for hiding this comment

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

So that seems to mean either:

  1. purge() isn't getting called
  2. purgeCohortMember() isn't calling delete()

Can you show what you tested in Postman?

Copy link
Contributor Author

@WodPachua WodPachua Sep 5, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @ibacher , any further assistance on this? Seems am stuck how to move this forward.

Copy link
Member

@ibacher ibacher Oct 3, 2023

Choose a reason for hiding this comment

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

@WodPachua Sorry, I missed this. When you do that via Postman, is the member deleted? More exactly, I'm not completely sure that that Testing REST URIs works with query parameters...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibacher Yes, the delete is successful, at least the database shows so.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so I think unindent the code, so that it's formatted correctly and we can merge this in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright @ibacher , I have fixed that.

@WodPachua
Copy link
Contributor Author

Did you manually test the changes?

@wikumChamith Have done so, created a cohort(s) with members, but seems the purge isn't still being effected. Still trying to figure out why...

@dkayiwa dkayiwa merged commit 3a98ef1 into openmrs:master Oct 5, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants