Omission of patient from CohortMembership model

@darius @wyclif @teleivo @burke
In this model https://github.com/openmrs/openmrs-core/blob/31456b307c922f1db0797191407a38ff28494571/api/src/main/java/org/openmrs/CohortMembership.java We don’t have the patient property which was there in the previous CohortMember model which means any reources using the model does not have access to the patient. Is this by design and if so what was the rationale.

I can see patientId property, can’t you use that to get patient object? see https://github.com/openmrs/openmrs-core/blob/31456b307c922f1db0797191407a38ff28494571/api/src/main/java/org/openmrs/CohortMembership.java#L27, moreover, I think cohort comprises only of set of patient ids.

@ningosi I think it would be sub-optimal to do that, mainly because we are using it in a list. Meaning if a cohort contains X members, we will have to do X requests in order to get the other patient details that we require like, age, gender, identifiers, meaning the API becomes too chatty. It was there initially. Looks like the process of migrating the CohortMember resource into OpenMRS core(renaming it to CohortMembership in the process) led to its omission, or was intentionally done for other reasons.

We are trying to fix this so this is what we have so far WIP. https://github.com/enyachoke/openmrs-core/commit/5712443f44ef777fc75821144cdd904d68deb601

https://github.com/enyachoke/openmrs-module-webservices.rest/commit/fc473c731f7e481317958a59808439f42557ea06.

These are very large commits, and honestly I don’t have time to read it all now. Please highlight the relevant parts. :slight_smile:

@achachiez, can you please clarify where there used to be a patient property? The old Cohort.java class does not contain a List or anything like that.

If you are talking about the REST module, here is the background reading for why this happened:

It is intentional to not expose the patient directly.

Generally the best way to get many fields for a cohort of patients is with the Reporting module (and Reporting REST).

If you need to do this without the reporting module, I suggest introducing a convenience method like getPatients(Cohort), rather than trying to rewrite anything.

I think it’s a little problematic that CohortMembership has a patientId field rather than a uuid, it makes the resource useless to a rest client since they can’t work with database ids.

1 Like

@darius are you able to guide me on how I can achieve the same using reporting module(plus reporting rest module). I want a way to fetch the members of a cohort by uuid. It needs to return the members containing the patient object. E.g. Request: https://demo.openmrs.org/openmrs/ws/rest/v1/cohort/some-cohort-uuid

Expected response: { members: [ { uuid:’…’, patient: { … } ],

other cohort details… }

It’s the equivalence of doing a request in 2.0.x and earlier versions, as follows https://demo.openmrs.org/openmrs/ws/rest/v1/cohort/some-cohort-uuid/member

The CohortMembership Java class has a patientId, but the REST resource has a patientUuid property. I don’t see anything problematic here.

The reportingrest module is documented at Home · openmrs/openmrs-module-reportingrest Wiki · GitHub. You can’t actually get back the Patient object, but you’d need to specify the individual data points you want, either using a dataset, or using the ad hoc analysis resource.

I can also think of two alternatives, that we could introduce in the core webservices.rest module:

  1. allow some way to provide a list of uuids, and get back all those patients in a single response. (Maybe GET .../patients?uuids=1,2,3) (This sounds like it’s defining a new convention, so we’d want to discuss a bit.)
  2. add an additional representation on the cohort resource which would include a new currentPatients property, which would determine the current cohort membership, and give more or less your expected response. A probably with this is that it could allow a DOS attack on the server if you cause it to try to fetch thousands of patient objects from hibernate.

Then that’s fine, Sorry I didn’t look at the code first to make that comment I assumed the rest API was similar.

It may help to give context of our primary use case at AMPATH for using cohorts. We do not use this feature for reporting needs. Rather, as a clinician, it’s very useful to have the ability to manually manage a list of patients. So we made use of the cohort framework in 1.11 to do so. A user is able to manually create a “patient list” (= a cohort) and add and remove members to that list.

In upgrading to 2.1.x, we discovered that our way of using the cohort model no longer worked. Specifically, we wrote code to pull a cohort and display all the members of a cohort to the user. It was relatively simple before as with a single rest call, we could get all the information we needed about the patients

My understanding from our team, is that the rest endpoint now no longer contains the patient object but only the id (I can’t remember if it’s the uuid or patient_id). Just for my curiosity, what was the reason this was removed?

We can certainly switch our code to get the patient_uuids from the cohort endpoint and then make a rest request to get each patient resource. It seems less efficient then righting a single query to get the data we need for all patients in the cohort. But, I can understand if there are reasons for this.

@darius, you suggest

You can’t actually get back the Patient object, but you’d need to specify the individual data points you want, either using a dataset, or using the ad hoc analysis resource.

This seems to me to suggest that cohorts have become more tightly linked with it’s use in reports (which is quite reasonable, we were hacking the feature for a unintended use). Perhaps creating datasets is simple enough but seems to be moving further from how we are actually using cohorts.

I wonder if we are simply misusing this OpenMRS feature and whether we should simply commit to creating a new module to support this. I believe Muzima is using cohorts in a way more akin to our use case - are there others out there doing the same?

I’m curious to hear what others think. If appropriate, we can begin working on this as a new module for openmrs.

JJ

Perhaps, but this did not come from a requirement in the reporting module. It has introduced it’s fair share of compatibility issues in how we have used Cohort in reporting as well. In retrospect, we could ask whether we should have have made this change to Cohort, or rather have created a new Domain object (maybe a subclass) that added the ability to date constrain cohort membership and left the original Cohort class completely backwards-compatible.

Of course, if no one is actually using the new CohortMembership construct for anything in reality, maybe revisiting this is worthwhile?

Mike

This was very helpful!

Using cohorts for a “My Patients” feature seems perfectly reasonable to me, and you should definitely be able to use cohorts in core without reporting.

These changes to the core representation of cohorts were actually triggered by muzima: @ayeung created the project that @vshankar worked on that turned into TRUNK-4906.

The changes to REST were done by me in early 2017 because I needed to get that change fully merged so that Bahmni could release openmrs-core 2.1.0. I posted on talk about this here (and I mentioned you!) but my post was long, involved, and somewhat out of the blue, so I’m not surprised few people read/responded. Specifically I said this:

I still think this is philosophically correct, but I hadn’t really thought about your use case at the time, and we should make it easier to do what you’re trying to do.

I propose either of two options (or both I guess):

(Option 1) Add new property to the cohort\membership resource called patient. This would not be included by default, but we can add a new named representation called withdefaultpatient to include this, or you could use a custom representation to get it.

  • This feels more “correct” to me, because the patients are only fetched in pages, which would help avoid an inadvertent DOS attack if someone has created a huge My Patients list.
  • it preserves the new parts of the data model (i.e. memberships have start/end dates)
  • It might be a bit less convenient for you to work with

(Option 2) Add a new property to the cohort resource called currentPatients. This would not be included by default, but we can add a new named representation, or you could use a custom representation to get it.

  • This is closest to the previous API you were using so requires the least changes on your side
  • We’d be hiding the new data model elements (membership start/end dates) and just calculating the current cohort membership.
  • feels hackier to me

It would be great if someone from muzima can review the current REST functionality, as well as these two options, since they’re supposedly the ones being served by these changes! (@jdick, maybe you can ping someone offline about this?)

I think that either of my two options could be done in the core webservices.rest module and you wouldn’t need a new module.

Thanks @mseaton and @darius for your responses.

We don’t anticipate having patient lists to have length anywhere near the same magnitude as cohorts. The patient lists really are for tracking patients a provider is actively following which probably will be no more than a 100 or 200. On our side, I’m not too worried about a DOS attack.

We generally use the patient name and identifier in the patient list. @darius, it’s not clear to me from your descpription but would Option (1) provide this info for every patient in the list. If so, that works for us.

I think it is valuable to track start and end dates of the cohort membership so you can theoretically find patients previously on your list who are no longer there. We would want to take advantage of this info.

Additionally, we have been testing out an addition to this patient list feature which allows users to share patient lists with each other. In order to facilitate this, we added a table on the backend which tracks which user has access to the list and which rights the user has (admin, edit, view). Here’s a quick video showing the feature: https://www.useloom.com/share/a79b2892b9c24ff3be89a98546680675

The thought here is that this would allow users to co-manage a list of patients. To be perfectly honest, it has not caught on all that much here but we haven’t done a great job marketing the feature either.

JJ

And I’ll follow up with @mssavai to discuss the affect on Muzima of the recommendations above.

@jdick, @mssavai, I’m curious where this went?

FYI see this other discussion where I’m proposing that we simplify the new cohort model a bit by allowing (and defaulting to) null start dates: More changes to the new Cohort Membership model

And I still support making either/both of the changes to the REST API I mentioned in this thread, but someone else would need to take the lead on this.

Hi @darius, I think it makes perfect sense to support backwards compatibility. Both options suggested here wouldn’t adversely affect mUzima.

Thanks @mssavai!

@jdick, does your team have a preference between the options?

(I don’t personally have time to work on this now, but I’d like to ensure we get a ticket created for, so we don’t lose the fact that this created difficulties for you!)


Option 1: you can do GET .../cohort/membership?v=withdefaultpatient

(i.e. it’s the new representation, where these would have the extra details like startDate and endDate, but would also include the patient)


Option 2: you can do GET .../cohort?v=currentPatients

(this would look more like the old model)

Thanks @darius for your support so far! We really appreciate it.

I think Option 1, is good since that’s the new way. Would be good to get opinion from the other implementing members. @mssavai What do you think?

@darius Just following up on this. Was the ticket ever created?

Thanks for the ping @nkimaina. I said I didn’t have time to work on this myself back in March, and I still don’t. :slight_smile:

However I think we should create a ticket now and add it to the Help Upgrade to 2.x sprint. I think we can go with Option 1. Can you create that ticket, and share it here (and I can review/edit)?

@nkimaina since we currently have a sprint going on with the topic of helping people upgrade to the latest platforms, I think this piece of work would fit in it.

Do you have time to create the ticket?