EA-198: Add support for configuring Mother Child relationships within…

[mogoodrich]

… the EMR API module

[mogoodrich]

I left this in draft format because the omod REST endpoint stuff still needs to be added but I wanted to put this out for review sooner rather than later, especially to get feedback on the name of the new service and overall structure, see my comments.
@cioan @mseaton @chibongho flagging you all for review.

[mogoodrich]

We can evolve what this object should contain... it likely needs to contain admission information, but I haven't checked out the latest InpatientAdmission / InpatientRequest data structure and I wanted to see how that was evolving before digging too deep into this.

[mseaton]

I'm a little worried about the efficiency of this. I agree we want to support being able to getNewborns for a single mother, but we may need the overall method to be more efficient to operate in bulk in some way (eg. take in a List of mother patient ids and get back a Map<Mother, List> or something.

[mogoodrich]

Fair. It's likely easy enough to support multiple patients. Though I'm a little worried about how we'd hand this on the client side. ie, it's nice to be able to separate the concerns of how to fetch the relevant data needed to display the patient card from fetching the list of the patients on the ward.

[mseaton]

Yeah, agree, just wanted to flag it.

[mogoodrich]

I think it's easy enough to support 1 to n, I will likely add that.

[mseaton]

What is the purpose of the visitLocation parameter here? If "mother" is required, are you saying "but don't return any babies for the mother if the visitLocation doesn't match"? No, you should already have filtered by visitLocation to get the valid mothers.

[mogoodrich]

Good point. I'll sleep on this in case I'm forgetting something, but that does seem to make sense.

[mseaton]

It would be more consistent with the other hql I recently added to connect the tables via joins rather than where criteria.

[mseaton]

I really don't see us ever limiting by visitLocation. Even in the existing adt queries, I don't think we really have that use case...

[mogoodrich]

Moving this out of draft state even though it still doesn't have the code to expose these service methods as REST endpoints, will likely work on that as a separate PR.

I have made changes and added a baby-for-mother inverse service method and added some comments, flagging @mseaton for re-review.

Also, unclear why the tests have now started to fail, but I will look into this and resolve before merging.

[mogoodrich]

Added this since the last commit.

One thing it doesn't do is confirm that the newborns are actually "newborns", so the client side will have to handle this; easy enough to support this if we want to, but it seemed weird to have the method potentially one of the inputs. Practically, this would only make a different if there were a mother and a child in the same hospital and the child is not a newborn... the mother would still show up on the child's card, which certainly isn't the end of the world.

I also considered a single method that, given a patient set, would use a single query to fetch both NewbornsByMother and MothersByNewborn. This seemed like it would be straightfoward to write and would save us a REST call and a query, but it seemed like things would be getting too confusing for the consumer to understand.

[mseaton]

Same comment as above. getMothers(MotherSearchCriteria). But see my other comments below also.

[mogoodrich]

We need the inpatient admission for the newborn so we can should their current location. I followed a similar pattern to what @mseaton for fetching all inpatient requests associated with inpatient admission.

[mseaton]

So Mother and Newborn are inherently admitted patients who just delivered / were born within the facility, but I'm not sure that is necessarily obvious to consumers of a MaternalService that are getting mothers and babies.

I think I might just move everything into the adt domain and AdtService if we are dealing with ADT things - in this case, these are really more specific instances of InpatientAdmission (eg. subclass or wrapper class).

I think it would be more intuitive if Mother -> MaternalAdmission and Newborn -> NewbornAdmission, both of which extend InpatientAdmission.

Then we add methods to AdtService for getMaternalAdmissions(MaternalAdmissionSearchCriteria) and getNewbornAdmissions(NewbornAdmissionSearchCriteria)

  MaternalAdmission extends InpatientAdmission (
    private List<NewbornAdmission> newbornAdmissions;
  )

  NewbornAdmission extends InpatientAdmission (
    private List<MaternalAdmission> maternalAdmission;
  )

Then the ward app can just get these directly, without making any additional queries...

[mogoodrich]

I did/do worry that it's not 100% obvious what the new service methods do, but I don't follow exactly what you are suggesting? I think I understand the idea, but in practice I'm not following. Isn't the above infinitely recursive?

We could just bundle the mother/child with InpatientAdmission, but we'd want to flag when to populate these fields to not do the search for the case of most wards where inpatient/outpatient isn't a thing:

    private Visit visit;
    private Patient patient;
    private Set<Encounter> admissionEncounters = new TreeSet<>(getEncounterComparator());
    private Set<Encounter> transferEncounters = new TreeSet<>(getEncounterComparator());
    private Set<Encounter> dischargeEncounters = new TreeSet<>(getEncounterComparator());

    private List<Patient> admittedNewborns;
    private Patient admittedMother;

But this seems way less than ideal the more I look at it.

I'd be up for discussion other ways to make this clearer, but I'm not sure your suggestion does, or perhaps I'm not understanding correctly? Post-standup discussion?

This is also why I like putting all this in a separate service/package, so people that want to learn about how ADT works, but don't care about maternal care, can just ignore this package.

We could change "getMothersForNewborns" to "getAdmittedMothersForNewborns" to clarify, the annoying this is the method really gets mothers with active visits, not admitted, so "admitted" isn't really clear either, I guess the clearest would be:

getNewbornsWithActiveVisitByMother(patientIds)
getMothersWithActiveVisitByChild(patientIds)

Generally, thinking more about this, this does feel like a targeted case that it seems hard to pre-generalize, and there's still a question of how consumption will work, so I'm still leaning towards getting something working and isolating this in it's own class. But let's talk through your idea in case I'm missing an element of it.

[mogoodrich]

I added this restriction (mother and baby visits must be at the same location). This might be problematic for implementations that collect visit location at a greater level of granularity than at the facility level, but it seemed like something we should do in preparation for a more cloud-based model where there are multiple facilities in the same database.

[mseaton]

I feel like this is more likely to cause data to be excluded that shouldn't be, than include too much data, at least in the short term. We can always add this restriction in, but I'd be a little wary about it. I don't think there are a lot of use cases where we want to exclude babies from mothers who were born after the mothers visit start date, because they are in different facilities in a federated system. Seems unlikely.

[mogoodrich]

Fair, I could see going either way, I can remove this.

[mogoodrich]

And, oh, I converted this somewhat to inner joins, but needed help on how do this when can't just chain the joins... ie relationship links to person/patient, but then visit links to patient not the other way around... hope that made some sense and I'm not just missing something obvious. :)

[mseaton]

This is where I found a lot of value (and really like the pattern) of using SearchCriteria. So if we continue with these methods (and I have other feedback that may supercede this), it might be preferable to do something like getNewborns(NewbornSearchCriteria), and then one of the properties of NewbornSearchCriteria might be which mothers to search on, but other use cases can emerge to get Newborns by something other than their mothers (or in addition).

Also, it is potentially more expensive to take in a List here, if you need to look up a bunch of patient by id or uuid before passing them in.

[mogoodrich]

Passing in uuids/ids likely makes sense here, good point...

I would favor switching to a search criteria as soon as we have more than one input parameter... would be trivial to deprecate the old method and delegate to the new.

[mseaton]

Same comment as above. getMothers(MotherSearchCriteria). But see my other comments below also.

[mseaton]

So Mother and Newborn are inherently admitted patients who just delivered / were born within the facility, but I'm not sure that is necessarily obvious to consumers of a MaternalService that are getting mothers and babies.

I think I might just move everything into the adt domain and AdtService if we are dealing with ADT things - in this case, these are really more specific instances of InpatientAdmission (eg. subclass or wrapper class).

I think it would be more intuitive if Mother -> MaternalAdmission and Newborn -> NewbornAdmission, both of which extend InpatientAdmission.

Then we add methods to AdtService for getMaternalAdmissions(MaternalAdmissionSearchCriteria) and getNewbornAdmissions(NewbornAdmissionSearchCriteria)

  MaternalAdmission extends InpatientAdmission (
    private List<NewbornAdmission> newbornAdmissions;
  )

  NewbornAdmission extends InpatientAdmission (
    private List<MaternalAdmission> maternalAdmission;
  )

Then the ward app can just get these directly, without making any additional queries...

[mseaton]

See comment above where we could make Newborn -> NewbornAdmission

[mseaton]

I feel like this is more likely to cause data to be excluded that shouldn't be, than include too much data, at least in the short term. We can always add this restriction in, but I'd be a little wary about it. I don't think there are a lot of use cases where we want to exclude babies from mothers who were born after the mothers visit start date, because they are in different facilities in a federated system. Seems unlikely.

[mseaton]

Maybe this is going over the same ground as my other comments, but I don't love all of these restrictions here in a "generic" mothers by newborn method. If someone were to see a getMothersByNewborn method, passing in a Newborn, and got null back for the mother because the mother no longer had an active visit, that would seem...unexpected. Another reason why I like being more explicit with our naming around XxxAdmission

[mseaton]

Same comment as before. Consumers will wonder why no babies are returned for a mother just because they no longer have an active visit. That seems more like a front-end concern than the concern of a back-end service method, unless we make this much more explicitly specific.

[mogoodrich]

Okay, ready for re-review @mseaton ... I still need to add the REST endpoint and add new javadocs for thee new service but this updated commit now:

• Uses search criteria objects for Mother and Child searches, enabling configuring whether to limit to patients with actvive visits
• Uses a list of patient uuids for the search instead of patient objects
• Standardizing naming convention on child (instead of "baby" and "newborn")

[mseaton]

The datatype and naming of these variables imply that you can have null, true, and false values, and that true and false both restrict, but in opposite ways. If you do it this way, I'd expect these to be null by default, and null would mean that there is no restriction, and true and false would both lead to filtering of results. eg. motherHasActiveVisi = true would do what I'd expect, but motherHasActiveVisit = false would enforce that the mother does not have an active visit, rather than ignoring the condition.

The alternative would be to make these boolean rather than Boolean, and name them something more clearly to indicate that false means no filtering, and true activates the filter. Something like requireMotherToHaveActiveVisit, requireChildToHaveActiveVisit, requireChildBornDuringMothersActiveVisit.

[mogoodrich]

I don't thing we need to support searching for patients that don't have active visits, but your commentary about the naming being confusion is fair, will fix.

[mseaton]

I believe there is a lombok annotation to add a full-argument constructor that you could use instead, if you really need this.

[mogoodrich]

Neat, done.

[mseaton]

This should be renamed getChildren I would think. There is nothing in the method signature any longer that implies that you must search by mother. I know the implementation of this currently requires setting mother uuids, but this may not always be the case, so I don't think we should name it like this.

[mogoodrich]

So I changed this to getChildren, but (almost) back to the original, making the mothers plural: "getChildrenByMothers". I did remove making setting mother uuids mandatroy, but I think the name should stay, reflecting the case that this returns child-mother pairs, (and, in particular, would return two rows for single child if they had two or mother mothers... admittedly, not a standard case, but certainly a standard case for the inverse, getMothersByChild).

[mseaton]

This should be renamed getMothers. Same comments as above.

[mogoodrich]

See above (and I did remove making setting child uuids mandatory)

[mseaton]

Does this need to be required? I think it likely does to perform and not crash the system if none of the other properties are set. If some of the other properties are set, it might not really be strictly required.

[mogoodrich]

Yep, looks possible remove this restriction, will do.

[mseaton]

Ditto on above.

[mseaton]

Same comments apply here that I made in the ChildSearchCriteria

[mogoodrich]

Done

[mseaton]

There is so much similarity between the two queries. Can we accomplish this with only one query that we have to maintain which is used to both getMothers and getChildren? I could see both supporting either motherUuids and/or childUuids, for example. And I could see both supporting childBornDuringMothersActiveVisit. Would seem to have value in consolidating.

[mogoodrich]

Done. And now both support childBornDuringMothersActiveVisit

[mseaton]

Thoughts about making this to be private Mother mother;?

[mogoodrich]

I'd rather keep the REST response as simple as possible. You mean have:

public class Mother {
    private Patient mother;
    private Child child;
    private InpatientAdmission motherAdmission;
}


public class Child {
    private Patient child;
    private Mother mother;
    private InpatientAdmission childAdmission;
}

Then on a child you've have to child.mother.mother to get the mother?

[mseaton]

Hmm. I think given what you have in place it would make sense to just have one object:

public class MotherAndChild {
    private Patient mother;
    private Child child;
    private InpatientAdmission motherAdmission;
    private InpatientAdmission childAdmission;
}

And then one service method to getMothersAndChildren(MothersAndChildrenSearchCriteria) that returns a List of these.

That list would contain all children of interest if you pass it a set of child uuids, and would contain all mothers of interest if you pass it was set of mother uuids, and everyone of interest if you just pass it criteria about the visit constraints expected...

[mogoodrich]

Yeah, I was thinking of that, though I thought there was a reason it wasn't going to work, will check again.

[mseaton]

Thoughts about making this private Child child;?

[mogoodrich]

See above

[mseaton]

See above again.

[mogoodrich]

@mseaton I made most, but not all of the suggested changes, see my comments and updated PR.

Going to go ahead and merge this in.

[mogoodrich]

I don't thing we need to support searching for patients that don't have active visits, but your commentary about the naming being confusion is fair, will fix.

[mogoodrich]

Neat, done.

[mogoodrich]

Yep, looks possible remove this restriction, will do.

[mogoodrich]

I'd rather keep the REST response as simple as possible. You mean have:

public class Mother {
    private Patient mother;
    private Child child;
    private InpatientAdmission motherAdmission;
}


public class Child {
    private Patient child;
    private Mother mother;
    private InpatientAdmission childAdmission;
}

Then on a child you've have to child.mother.mother to get the mother?

[mogoodrich]

See above

[mogoodrich]

Done

[mogoodrich]

So I changed this to getChildren, but (almost) back to the original, making the mothers plural: "getChildrenByMothers". I did remove making setting mother uuids mandatroy, but I think the name should stay, reflecting the case that this returns child-mother pairs, (and, in particular, would return two rows for single child if they had two or mother mothers... admittedly, not a standard case, but certainly a standard case for the inverse, getMothersByChild).

[mogoodrich]

See above (and I did remove making setting child uuids mandatory)

[mogoodrich]

Done. And now both support childBornDuringMothersActiveVisit

[mseaton]

What is this fluent = true? Don't we want these to act and behave like standard Javabean properties?

[mogoodrich]

It makes the getter/setter "requireMotherHasActiveVisit" instead of "isRequireMotherHasActiveVisit"... "is" is the default for "boolean" types and that read horribly. https://projectlombok.org/features/experimental/Accessors

[mogoodrich]

No idea why it's called fluent. :)

[mseaton]

I understand what it does, but nothing else that works with JavaBean objects will be able to deal with this. We should make it standard, even if it reads poorly, or try to change the variable name so it reads better with an "is" or "get" prefix.

[mogoodrich]

ah... let's just stick with the ugly name then, will change.

[mseaton]

Regarding these method names, see my suggestion above. These aren't "ByMothers" or "ByChildren" at all. They are really lists of MotherAndChild objects. They aren't "by" anything.

[mseaton]

Ditto

[ibacher]

Are relationships always uni-directional like this, i.e., with a fixed A/B? (The flexibility around this is one of the things I've always disliked about relationships).

[mogoodrich]

They are flexible, but we are saying this is based around have a standardized uni-directional "Mother to Child" relationship type