COH-50 : Cohort resources cannot be voided #26

ManojLL · 2023-03-10T17:05:46Z

Issue worked on:

https://issues.openmrs.org/browse/COH-50?filter=-1

Description

This PR fixed the issue in (COH-50) DELETE Cohort endpoint.

Piumal1999

Hi @ibacher, could you please review this PR? I added some comments as well.

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortTypeServiceImpl.java

dkayiwa · 2023-03-12T19:25:34Z

@ManojLL can you add some tests for this?

ManojLL · 2023-03-13T17:09:48Z

@dkayiwa I added some tests for voidCohort method. Can you review?

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortServiceImpl.java

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortTypeServiceImpl.java

ManojLL · 2023-03-20T09:10:24Z

@ibacher @dkayiwa can you review this PR?

dkayiwa · 2023-03-20T09:19:12Z

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortServiceImpl.java

+		cohort.setVoided(true);
+		cohort.setVoidReason(reason);
+
+		for (CohortMember cohortMember : cohort.getCohortMembers()) {


Is this needed?

cohort won't voided automatically. That is why manually set voided true.

Do you have a test to demonstrate it?

Here I attached test screenshots.

I mean tests in this pull request. Not a screenshot.

In other words, tests which would fail if you removed that for loop

In other words, tests which would fail if you removed that for loop

If I remove for loop tests wont fail

Which means that you do not have tests to demonstrate the problem that you claim to require the for loop

@dkayiwa I updated the test can you review ?

api/src/test/java/org/openmrs/module/cohort/api/impl/CohortServiceImplTest.java

ibacher · 2023-03-21T15:03:35Z

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortServiceImpl.java

+			cohortMember.setVoided(true);
+			cohortMember.setVoidReason("Cohort voided");


Is there a reason we need these two explicit setVoided() and setVoidReason() calls rather than calling the service method?

@ibacher I thought that, it would be better to call the two explicit would be better than calling Context.getService multiple times inside the for loop considering the performance, that is why I use setVoided and setVoidReasin methods.

I'd tend to prefer the service call. You could always do:

CohortMemberService cms = Context.getService(CohortMemberService.class); // loop cms.voidCohortMember(); // end loop

That way, it's the CohortMemberService's responsibility to know how to void a cohort member (and also it fixes the real problem which is that calling these methods depends on Hibernate's implicit context to flush those changes to the DB whereas calling the service method will actually ensure that those changes are always written.

@ibacher After rename the voidChort method to voidCohortM ,no need to call cohortMemberservice void method to make cohortmember to voided. when delete a cohort , the child cohortmember getting Voided.

ibacher · 2023-03-21T15:04:36Z

api/src/test/java/org/openmrs/module/cohort/api/impl/CohortServiceImplTest.java

@@ -167,9 +167,11 @@ public void shouldVoidCohort() {
 		cohortMember.setPatient(patient);
 		cohortMember.setUuid("4834jk3-n34nm30-34nm34-348nl");
 		cohortMember.setCohort(cohortM);
+		cohortM.addMemberships(cohortMember);


It would be better to write a separate test for this. The test here isn't really valid because ultimately we mock the createOrUpdate() call...

@ibacher This test is to check whether the child cohortMember is voided when the parent cohort is deleted. So I added the cohortMember to the cohort and used assertions to check the cohort and cohortMember is voided after deleting the cohort

So, what I was trying to get at is the way in which the mock changes this from how the code would run in production because the key thing about voiding the memberships is that the voiding gets written to the database, but because we've mocked the call to createOrUpdate() nothing is getting written to a database, so, ultimately, this test ends up verifying that cohortMember.setVoided() was called, but not that the cohortMember was actually saved in a voided state.

dkayiwa · 2023-03-21T17:53:35Z

@ManojLL the method needs renaming to voidCohortM for this to work automatically as seen here: https://github.com/openmrs/openmrs-core/blob/2.6.0/api/src/main/java/org/openmrs/aop/RequiredDataAdvice.java#L191-L218

If we want other platform AOP things to work for other methods like saveCohort, etc, we would need to rename those methods too. But any way, that would be a different ticket.

ManojLL · 2023-03-21T20:31:15Z

@dkayiwa rename the voidCohort metod to voidcohortM, Can you review the changes?

dkayiwa · 2023-03-21T20:56:33Z

@ManojLL do you think you can add a test?

ManojLL · 2023-03-21T21:20:23Z

@ManojLL do you think you can add a test?

@dkayiwa I tried to write test for this method, but unable to find a way to check whether the cohort or cohortMember is voided after mocking voidCohortM method.

dkayiwa · 2023-03-21T21:29:51Z

@ManojLL would it be easier for you to just put un mocked tests here? https://github.com/openmrs/openmrs-module-cohort/blob/master/api/src/test/java/org/openmrs/module/cohort/api/dao/CohortGenericDaoTest.java

ManojLL · 2023-03-21T21:52:06Z

@ManojLL would it be easier for you to just put un mocked tests here? https://github.com/openmrs/openmrs-module-cohort/blob/master/api/src/test/java/org/openmrs/module/cohort/api/dao/CohortGenericDaoTest.java

@dkayiwa I added tests for voidcohortM method and voidCohortType method, you can review.

dkayiwa · 2023-03-21T22:28:07Z

I do not see your test calling the service.voidCohortM method of interest.

ManojLL · 2023-03-22T03:54:28Z

@dkayiwa I updated the test

dkayiwa · 2023-03-22T12:41:53Z

The test that you added passes even when you do not fix the reported problem.
Can you use the real service without mocking it?

ManojLL · 2023-03-22T13:01:47Z

@dkayiwa updated the tests, You can review.

dkayiwa · 2023-03-22T14:25:49Z

api/src/test/java/org/openmrs/module/cohort/api/dao/CohortGenericDaoTest.java

+	@Test
+	public void shouldVoidCohortM() {
+		CohortM cohortToVoid = dao.get(COHORT_UUID);
+		cohortToVoid.setVoided(true);


Why are you setting the void properties of the cohort and then at the same time call the voidCohort service method? It is the service method that is supposed to take care of that.

@dkayiwa I thought that, the cohort which is passing to the voidCoghortM method needs be voided true.
btw I updated test.

api/src/test/java/org/openmrs/module/cohort/api/dao/CohortGenericDaoTest.java

Piumal1999 · 2023-03-22T21:23:23Z

Thank you all for getting this done.

@dkayiwa @ibacher will it be possible to do a release with this fix?
Because the error with the delete cohort is a blocker for completing this PR: openmrs/openmrs-esm-patient-management#578.

dkayiwa · 2023-03-22T21:32:16Z

@Piumal1999 feel empowered to do the release by yourself. Here are the instructions: https://wiki.openmrs.org/display/docs/Releasing+a+Module+from+Bamboo

Piumal1999 · 2023-03-22T21:48:12Z

Sure, I would love to. I just created a help desk ticket to get access to the bamboo dashboard.
Thank you @dkayiwa

ibacher · 2023-03-22T22:23:26Z

I’d prefer if we hold off on releasing this just yet. There’s no reason we need to do that to unblock the issue; we can just update the O3 release to use the snapshot and then, once we inevitably find another problem or two, we can fix things without all the hassle. We’ll cut a release of this the next time we need to release the 3.x RefApp (which will likely be soon anyways).

Piumal1999 · 2023-03-23T07:48:57Z

I’d prefer if we hold off on releasing this just yet. There’s no reason we need to do that to unblock the issue; we can just update the O3 release to use the snapshot and then, once we inevitably find another problem or two, we can fix things without all the hassle. We’ll cut a release of this the next time we need to release the 3.x RefApp (which will likely be soon anyways).

Oh sorry @ibacher. I already did the release.

we can just update the O3 release to use the snapshot

Btw, how do we change the module versions used in dev3? Is it done by updating this?

dkayiwa · 2023-03-23T10:05:53Z

Is it done by updating this?

Yes.

COH-50 : fixed cohort delete issue

4869537

ManojLL marked this pull request as ready for review March 10, 2023 17:06

ManojLL marked this pull request as draft March 10, 2023 17:07

ManojLL marked this pull request as ready for review March 12, 2023 11:54

Piumal1999 reviewed Mar 12, 2023

View reviewed changes

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortTypeServiceImpl.java Show resolved Hide resolved

ManojLL added 2 commits March 13, 2023 01:32

delete cohort members when deleteing cohort

de15c83

add unit test for voidCohort method

1c5701b

ibacher requested changes Mar 13, 2023

View reviewed changes

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortServiceImpl.java Outdated Show resolved Hide resolved

api/src/main/java/org/openmrs/module/cohort/api/impl/CohortTypeServiceImpl.java Show resolved Hide resolved

remove all imports

adbcf0d

ManojLL requested review from ibacher and removed request for ibacher March 14, 2023 16:26

optimized the void cohort test

b96ed28

ManojLL requested review from ibacher and removed request for ibacher March 16, 2023 19:37

dkayiwa reviewed Mar 20, 2023

View reviewed changes

api/src/test/java/org/openmrs/module/cohort/api/impl/CohortServiceImplTest.java Outdated Show resolved Hide resolved

ManojLL added 2 commits March 20, 2023 14:59

fixed formatting issue

7638ad3

improve voidCohort unit tests

5482e7e

ibacher reviewed Mar 21, 2023

View reviewed changes

Change voidCohort method name to voidCohortM

f286bf3

added test for voidCohortM method and voidCohortType method

385901a

updated tests on voidCohortM method and voidCohortType method

42e7be0

use services without mocking on tests

a05e5d3

dkayiwa reviewed Mar 22, 2023

View reviewed changes

remove voided from VoidCohortM tests

90dd4c0

dkayiwa reviewed Mar 22, 2023

View reviewed changes

api/src/test/java/org/openmrs/module/cohort/api/dao/CohortGenericDaoTest.java Outdated Show resolved Hide resolved

update unit test on voidCohort

82c8111

dkayiwa reviewed Mar 22, 2023

View reviewed changes

api/src/test/java/org/openmrs/module/cohort/api/dao/CohortGenericDaoTest.java Show resolved Hide resolved

dkayiwa reviewed Mar 22, 2023

View reviewed changes

api/src/test/java/org/openmrs/module/cohort/api/dao/CohortGenericDaoTest.java Show resolved Hide resolved

ManojLL added 2 commits March 22, 2023 23:10

update unit test on voidCohort

34cb146

update unit test on voidCohort

ba9b291

dkayiwa merged commit d73dd40 into openmrs:master Mar 22, 2023

		cohortMember.setVoided(true);
		cohortMember.setVoidReason("Cohort voided");

COH-50 : Cohort resources cannot be voided #26

COH-50 : Cohort resources cannot be voided #26

Conversation

ManojLL commented Mar 10, 2023

Issue worked on:

Description

Piumal1999 left a comment

Choose a reason for hiding this comment

dkayiwa commented Mar 12, 2023

ManojLL commented Mar 13, 2023

ManojLL commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManojLL Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManojLL Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Mar 21, 2023

ManojLL commented Mar 21, 2023

dkayiwa commented Mar 21, 2023

ManojLL commented Mar 21, 2023

dkayiwa commented Mar 21, 2023

ManojLL commented Mar 21, 2023

dkayiwa commented Mar 21, 2023

ManojLL commented Mar 22, 2023

dkayiwa commented Mar 22, 2023

ManojLL commented Mar 22, 2023

Choose a reason for hiding this comment

ManojLL Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Piumal1999 commented Mar 22, 2023

dkayiwa commented Mar 22, 2023

Piumal1999 commented Mar 22, 2023

ibacher commented Mar 22, 2023

Piumal1999 commented Mar 23, 2023

dkayiwa commented Mar 23, 2023

dkayiwa Mar 20, 2023 •

edited

Loading

ManojLL Mar 21, 2023 •

edited

Loading

ManojLL Mar 21, 2023 •

edited

Loading

ManojLL Mar 22, 2023 •

edited

Loading