More changes to the new Cohort Membership model

Development
1

Continuing the discussion from Cohort builder error on openmrs 2.1.2.:

I reviewed and didn’t approve. :slight_smile:

My counterproposal is that we change the new cohort behavior slightly so that cohort membership start date is not required, and defaults to null when you instantiate a new cohort/membership.

(We also need to fix Cohort.intersect to behave in a way that makes sense if you have two cohorts where the same member joined with different start dates.)

Edit: and I think we should do this fast and release it as Platform 2.1.3

2

@dkayiwa I suggest that we go ahead an do what I’m proposing here, and get TRUNK-5331 fixed as a community priority, so that Cohort Builder can again work with the latest OpenMRS releases.

So to be more precise I’m saying that the acceptance criteria for this are:

  1. drop the not-null constraint on CohortMembership.startDate
  2. remove any Java logic that defaults CohortMembership.startDate to new Date(), and instead default it to null.
  3. Cohort.intersect should behave in a way that makes sense if you give it {Patient 123, startDate Jan 1} and {Patient 123, startDate Feb 15}.
    • I would be satisfied if the output is either a cohort with {Patient 123, startDate Feb 15}, or else a cohort with {Patient 123, startDate null}
    • but the dev who works on this can propose anything else that makes sense.
3

Thanks @darius for defining it clearly. I already assigned it to one of the Andela developers. :slight_smile:

4

@wyclif expressed concern about this approach on the ticket (TRUNK-5331), so I’m bringing that discussion back here.

Personally, am not sold in on making start date nullable, I feel like a null startDate is bad data, a null start date doesn’t make sense.

I still think we can fix the bugs without making start date null by just fixing the implementation of Cohort.intersect.

I should have shared my complete thought process earlier. My bad. (And it’s fair to say that I have not thought this through as well as I should, so I’d like some confirmation here one way or the other!)

My thinking is that in the TRUNK-4906 (Expanded Cohort Details) ticket, we introduced some new complexity, without properly addressing backwards-compatibility, or maintaining the simpler use case.

In fact @mseaton complained about this here: Omission of patient from CohortMembership model - #11 by mseaton. There’s a use case for “simple cohorts” which are just a list of patients, and you don’t indicate the start or end date, in addition to these new “expanded” cohorts. (@jdick mentioned it here.)

So, I’m trying to respond to those complains and introduce some backwards-compatibility retrospectively, without removing changes that have already been released.

My proposal is that if you write your code without using dates, then Cohorts work in the old way. For example:

Cohort favorites = service.getCohort("Favorites");
favorites.addMember(selectedPatientId);

Alternately, if you want the complexity, then you specify dates:

Cohort nutritionStudy = new Cohort();
nutritionStudy.addMembership(new CohortMembership(selectedPatientId, enrollmentDate));

(If we had a time machine I would suggest we go back and leave Cohort as-is, and introduce a new DateRangeCohort. But we’ve already released a platform version with the new code.)

5

I agree with you @darius, both with the pragmatic approach of making start date nullable and the wish that we had a time machine so that we could have avoided this altogether by creating a new type of Cohort (personally I’d be willing to consider reverting if we can determine that the small number of implementations that might be using this feature can be migrated to a DateRangeCohort instead).

When I confronted this issue with the reporting module, I ended up having to do a bunch of refactoring to work around these changes. You can see the ticket in which I addressed that here, specifically this commit which might be instructive.

I may or may not have approached this the right way, but I generally had to avoid using the Cohort operations from core and modified the module to operate upon Cohorts as simply sets of ids (see PatientIdSet and CohortUtil), as the reporting module usage expects.

Mike

6

At this point, we removed the feature that depended on Cohorts in order to complete the upgrade to v2.1. Given the significant amount of overhead to get this feature working, we have delayed at this time.

We are happy to assign a developer to work on this ticket but we’d need a good amount of mentorship about how to go about this.

Reverting is fine with us but also happy to work on updating our angular services to adopt to a different api.

This is tangential but if we do proceed with a new code route, we’d like to figure out how to add a table which tracks which users are allowed access to each cohort and permissions for modifying members of the cohort. This is critical for us as, for our Patient List" feature.

JJ

7

@jdick this ticket (making startDate nullable) is already being worked on: https://issues.openmrs.org/browse/TRUNK-5331

The other thread, that involves some REST changes, has not resulted in a ticket yet. I asked you a question over there, to help get to a ticket.

On this tangent, Bahmni is also thinking about a My Favorite Patients feature. These would be limited to the user, but it could be nice to build this on top of something that can be expanded to shared Patient Lists.

I’m not sure if we’d want this in openmrs-core, or a shared module, but if you propose a design forum to discuss this then I would be happy to join.

2 Likes