Design Forum 2018-10-15: Resolving design inconsistencies in Cohort Membership (TRUNK-5379)

Monday’s design forum is from 4-5 pm UTC:

Resolving design inconsistencies in Cohort Membership

Description: There are various design inconsistencies in the CohortMembership model that need resolution as described in TRUNK-5379. We will use time in the design forum to walk through these inconsistencies and aim, by the end of the call, to create or identify ready-for-work tickets to resolve them

Notes: https://notes.openmrs.org/2018-10-15-Design-Forum

Attention: @mogoodrich, @burke, @wyclif and/or @dkayiwa, @darius. All others interested are welcome!

To join the call using Audio, Chat, & Screen Sharing please use a WebRTC-compatible browser (Chrome recommended) and join the OpenMRS UberConference call:


The next design forum on Wednesday, October 10 @ 6-7 pm UTC - OPEN FORUM

If you have a topic you would like to discuss during the call please post it at http://om.rs/designtime

Thanks @darius and @mseaton for helping get TRUNK-5379 ready for work during today’s design forum. Darius & I started out disagreeing with most of @mogoodrich’s assertions until Mike joined, agreed with Mark’s assertions, and helped understand Mark’s brilliance. It came down to how one approaches Cohort: do you see it as a Java collection of memberships or do you see it as a clinical cohort of patients. The former expects methods to behave like any other collection regardless of whether or not memberships are active; the latter expects methods to take into account only active memberships. In the end, we realized (as Mark had) the default collection methods are better behaving as typical collection methods and new methods could be introduced to work only with active cohort memberships.

In short, rather than making standard collection methods (contains(Integer), size(), and isEmpty()) take active membership into account, we agreed these methods should behave as a Java programmer would expect a collection of CohortMemberships to behave (except, ignoring voided memberships). We would create new, non-ambiguous methods to get the same information for active memberships (hasActiveMembership(Integer), activeMembershipSize(), and hasNoActiveMemberships()).

By the end of the call, we were able to make TRUNK-5379 ready for work.

1 Like

Thanks! Apologies for missing the call… as for “brilliance”, this Cohort stuff was probably Mike’s idea to begin with, and, if not, I probably would have forgotten what I had been thinking, so better for Mike to be there than me… :slight_smile:

+1 Mark’s brilliance +1 Burke’s humour

1 Like

@jthomas (cc @burke @mseaton @mogoodrich ) Is the design decision clear now and the following ticket is ready to be worked on? https://issues.openmrs.org/browse/TRUNK-5379?filter=-1

Any documentation or decision I should know about if I want to start working on this?

Thank you!

@fruether, thanks for offering to work on this (I’ve moved your comment from the topic discussing when to have the call to here.)

Just the summary above and the ticket’s description, which we updated to reflect consensus. Since there are several tasks described, please feel free to create subtask tickets for individual chunks of work (splitting it up into a few subtasks would, at a minimum help make reviewing the pull requests easier).

If you have any further questions about the ticket or lack privileges to make subtasks in JIRA, let us know here and we’ll try to help.

One small question @burke @mogoodrich regarding the size method. Its current state is:

	public int size() {
	return getMemberships().stream().filter(m -> !m.getVoided()).collect(Collectors.toList())
	        .size();
}  

The node says:

The size() method only returns the count of active, non-voided members. I would think this should not take active into account, and potentially not take voided into account The size() method only returns the count of active, non-voided members. I would think this should not take active into account, and potentially not take voided into account

So my assumption is: This method does not have to be changed since it already only counts non voided members. Is there anything I oversee?

I documented the expected behaviour in the test case for the PR: https://github.com/openmrs/openmrs-core/pull/2790/commits/cb95fb9a064e7b9d7b560cdd8e1d98dc03267c4a#diff-89be6a5420bc3e8a75fcb43bc5c95ff4

@burke @mogoodrich I have the following issue withTRUNK-5451. It refers to

There’s getMemberships(Boolean includeVoided) and a getMemberships() method… the getMemberships() method includes voided and non-voided members, which is what I think we actually want, but isn’t the default behaviour in OpenMRS to exclude voided members?

The result of this discussion was (at least when I look into the issue) that this function should exclude all non voided members.

Issue

Hibernate is using the getMembers() method. If I change it to only return a subset, namely the members that are non-voided then I get the following excpetion:

org.hibernate.HibernateException: A collection with cascade=“all-delete-orphan” was no longer referenced by the owning entity instance: org.openmrs.Cohort.memberships

That’s why I think the issue can’t be implemented.