Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Aug 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a1367c7
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 5, 2024
c84a9ae
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 5, 2024
76327e3
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 5, 2024
26e9ff6
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 6, 2024
c4e92b7
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 6, 2024
00b2de2
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 7, 2024
fd9470d
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 7, 2024
77c0105
EA-198: Add support for configuring Mother Child relationships within…
mogoodrich Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/src/main/java/org/openmrs/module/emrapi/EmrApiConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,6 @@ public class EmrApiConstants {
));

public static final String GP_USE_LEGACY_DIAGNOSIS_SERVICE = "emrapi.useLegacyDiagnosisService";

public static final String METADATA_MAPPING_MOTHER_CHILD_RELATIONSHIP_TYPE = "emrapi.motherChildRelationshipType";
mseaton marked this conversation as resolved.
Show resolved Hide resolved
}
5 changes: 5 additions & 0 deletions api/src/main/java/org/openmrs/module/emrapi/EmrApiProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.openmrs.PatientIdentifierType;
import org.openmrs.PersonAttributeType;
import org.openmrs.Provider;
import org.openmrs.RelationshipType;
import org.openmrs.Role;
import org.openmrs.VisitType;
import org.openmrs.module.emrapi.diagnosis.DiagnosisMetadata;
Expand Down Expand Up @@ -347,4 +348,8 @@ public List<Disposition> getDispositions() {
public DispositionDescriptor getDispositionDescriptor() {
return dispositionService.getDispositionDescriptor();
}

public RelationshipType getMotherChildRelationshipType() {
return getEmrApiMetadataByCode(RelationshipType.class, EmrApiConstants.METADATA_MAPPING_MOTHER_CHILD_RELATIONSHIP_TYPE, false);
}
}
30 changes: 30 additions & 0 deletions api/src/main/java/org/openmrs/module/emrapi/maternal/MaternalService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package org.openmrs.module.emrapi.maternal;

import java.util.List;

import org.openmrs.Patient;
import org.openmrs.api.OpenmrsService;

public interface MaternalService extends OpenmrsService {
mseaton marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns all "newborns" of the specified patients, where "newborn" is defined as a patient who is:
* - linked to the specified patient by a relationship of type emrapi.motherChildRelationshipType
* - has as an active visit at the same visit location of the mother
* - has a birthdate that is on or after the start date of the mother's active visit (at the visitLocation, if specified) (note matches on date, not datetime to account for retrospective data entry or only have a date component of birthdate)
*
* @param mothers
* @return
*/
public List<Newborn> getNewbornsByMother(List<Patient> mothers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@mogoodrich mogoodrich Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Returns all mothers of the specified patients(), where "mother" is defined as a patient who is:
* - linked to the specified patient by a relationship of typ emrapi.motherChildRelationshipType
* - has an active visit at the same visit location of the newborn
*
* @param newborns (assumption: these are newborns, method does *not* confirm this)
* @return
*/
public List<Mother> getMothersByNewborn(List<Patient> newborns);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
134 changes: 134 additions & 0 deletions api/src/main/java/org/openmrs/module/emrapi/maternal/MaternalServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package org.openmrs.module.emrapi.maternal;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import org.openmrs.Location;
import org.openmrs.Patient;
import org.openmrs.RelationshipType;
import org.openmrs.Visit;
import org.openmrs.api.APIException;
import org.openmrs.api.impl.BaseOpenmrsService;
import org.openmrs.module.emrapi.EmrApiProperties;
import org.openmrs.module.emrapi.adt.AdtService;
import org.openmrs.module.emrapi.adt.InpatientAdmission;
import org.openmrs.module.emrapi.adt.InpatientAdmissionSearchCriteria;
import org.openmrs.module.emrapi.db.EmrApiDAO;

public class MaternalServiceImpl extends BaseOpenmrsService implements MaternalService {

private EmrApiProperties emrApiProperties;

private AdtService adtService;

private EmrApiDAO emrApiDAO;

public void setEmrApiProperties(EmrApiProperties emrApiProperties) {
this.emrApiProperties = emrApiProperties;
}

public void setEmrApiDAO(EmrApiDAO emrApiDAO) {
this.emrApiDAO = emrApiDAO;
}

public void setAdtService(AdtService adtService) {
this.adtService = adtService;
}

public List<Newborn> getNewbornsByMother(List<Patient> mothers) {

RelationshipType motherChildRelationshipType = emrApiProperties.getMotherChildRelationshipType();

if (motherChildRelationshipType == null) {
throw new APIException("Mother-Child relationship type has not been configured");
}

if (mothers == null || mothers.isEmpty()) {
throw new APIException("No mothers provided");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks possible remove this restriction, will do.


Map<String, Object> parameters = new HashMap<>();
parameters.put("mothers", mothers);
parameters.put("motherChildRelationshipType", motherChildRelationshipType);

List<?> l = emrApiDAO.executeHqlFromResource("hql/newborns_by_mother.hql", parameters, List.class);

List<Newborn> ret = new ArrayList<>();
List<Visit> visits = new ArrayList<>();

for (Object req : l) {
Object[] row = (Object[]) req;
Newborn newborn = new Newborn();
newborn.setNewborn((Patient) row[0]);
newborn.setMother((Patient) row[1]);
visits.add((Visit) row[2]);
ret.add(newborn);
}

// now fetch all the admissions for newborns in the result set
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

InpatientAdmissionSearchCriteria criteria = new InpatientAdmissionSearchCriteria();
criteria.setVisitIds(visits.stream().map(Visit::getId).collect(Collectors.toList()));
List<InpatientAdmission> admissions = adtService.getInpatientAdmissions(criteria);
Map<Patient, InpatientAdmission> admissionsByPatient = new HashMap<>();
if (admissions != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this can ever be null, so you should be able to remove this null check.

for (InpatientAdmission admission : admissions) {
admissionsByPatient.put(admission.getVisit().getPatient(), admission);
}
}
for (Newborn newborn : ret) {
newborn.setNewbornAdmission(admissionsByPatient.get(newborn.getNewborn()));
}

return ret;
}

public List<Mother> getMothersByNewborn(List<Patient> newborns) {
RelationshipType motherChildRelationshipType = emrApiProperties.getMotherChildRelationshipType();

if (motherChildRelationshipType == null) {
throw new APIException("Mother-Child relationship type has not been configured");
}

if (newborns == null || newborns.isEmpty()) {
throw new APIException("No newborns provided");
}

Map<String, Object> parameters = new HashMap<>();
parameters.put("babies", newborns);
parameters.put("motherChildRelationshipType", motherChildRelationshipType);

List<?> l = emrApiDAO.executeHqlFromResource("hql/mothers_by_newborn.hql", parameters, List.class);

List<Mother> ret = new ArrayList<>();
List<Visit> visits = new ArrayList<>();

for (Object req : l) {
Object[] row = (Object[]) req;
Mother mother = new Mother();
mother.setMother((Patient) row[0]);
mother.setNewborn((Patient) row[1]);
visits.add((Visit) row[2]);
ret.add(mother);
}

// now fetch all the admissions for mothers in the result set
InpatientAdmissionSearchCriteria criteria = new InpatientAdmissionSearchCriteria();
criteria.setVisitIds(visits.stream().map(Visit::getId).collect(Collectors.toList()));
List<InpatientAdmission> admissions = adtService.getInpatientAdmissions(criteria);
Map<Patient, InpatientAdmission> admissionsByPatient = new HashMap<>();
if (admissions != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on above.

for (InpatientAdmission admission : admissions) {
admissionsByPatient.put(admission.getVisit().getPatient(), admission);
}
}
for (Mother mother : ret) {
mother.setMotherAdmission(admissionsByPatient.get(mother.getMother()));
}


return ret;
}
}
13 changes: 13 additions & 0 deletions api/src/main/java/org/openmrs/module/emrapi/maternal/Mother.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.openmrs.module.emrapi.maternal;

import lombok.Data;
import org.openmrs.Patient;
import org.openmrs.module.emrapi.adt.InpatientAdmission;


@Data
public class Mother {
private Patient mother;
private Patient newborn;
private InpatientAdmission motherAdmission;
mseaton marked this conversation as resolved.
Show resolved Hide resolved
}
12 changes: 12 additions & 0 deletions api/src/main/java/org/openmrs/module/emrapi/maternal/Newborn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package org.openmrs.module.emrapi.maternal;

import lombok.Data;
import org.openmrs.Patient;
import org.openmrs.module.emrapi.adt.InpatientAdmission;

@Data
public class Newborn {
private Patient newborn;
private Patient mother;
private InpatientAdmission newbornAdmission;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above where we could make Newborn -> NewbornAdmission

}
18 changes: 18 additions & 0 deletions api/src/main/resources/hql/mothers_by_newborn.hql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
select
mother,
baby,
motherVisit
from
Relationship as motherChildRelationship, Visit as motherVisit, Visit as babyVisit
inner join motherChildRelationship.personA as mother
inner join motherChildRelationship.personB as baby
where
baby in (:babies)
and motherChildRelationship.relationshipType = :motherChildRelationshipType
and motherVisit.patient = mother and motherVisit.stopDatetime is null
and babyVisit.patient = baby and babyVisit.stopDatetime is null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

and babyVisit.location = motherVisit.location
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

and mother.voided = false and baby.voided = false and motherChildRelationship.voided = false and motherVisit.voided = false and babyVisit.voided = false



21 changes: 21 additions & 0 deletions api/src/main/resources/hql/newborns_by_mother.hql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
select
baby,
mother,
babyVisit
from
Relationship as motherChildRelationship, Visit as motherVisit, Visit as babyVisit
inner join motherChildRelationship.personA as mother
inner join motherChildRelationship.personB as baby
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

where
mother in (:mothers)
and motherChildRelationship.relationshipType = :motherChildRelationshipType
and motherVisit.patient = mother and motherVisit.stopDatetime is null
and babyVisit.patient = baby and babyVisit.stopDatetime is null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

and babyVisit.location = motherVisit.location
and year(baby.birthdate) >= year(motherVisit.startDatetime)
and month(baby.birthdate) >= month(motherVisit.startDatetime)
and day(baby.birthdate) >= day(motherVisit.startDatetime)
mseaton marked this conversation as resolved.
Show resolved Hide resolved
and mother.voided = false and baby.voided = false and motherChildRelationship.voided = false and motherVisit.voided = false and babyVisit.voided = false



28 changes: 28 additions & 0 deletions api/src/main/resources/moduleApplicationContext.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,34 @@
</property>
</bean>

<bean id="maternalService" class="org.springframework.transaction.interceptor.TransactionProxyFactoryBean">
<property name="transactionManager">
<ref bean="transactionManager"/>
</property>
<property name="target">
<bean class="org.openmrs.module.emrapi.maternal.MaternalServiceImpl">
<property name="adtService" ref="adtService"/>
<property name="emrApiProperties" ref="emrApiProperties"/>
<property name="emrApiDAO" ref="emrApiDAOImpl"/>
</bean>
</property>
<property name="preInterceptors">
<ref bean="serviceInterceptors"/>
</property>
<property name="transactionAttributeSource">
<ref bean="transactionAttributeSource"/>
</property>
</bean>

<bean parent="serviceContext">
<property name="moduleService">
<list merge="true">
<value>org.openmrs.module.emrapi.maternal.MaternalService</value>
<ref bean="maternalService"/>
</list>
</property>
</bean>

<bean id="exitFromCareService" class="org.springframework.transaction.interceptor.TransactionProxyFactoryBean">
<property name="transactionManager">
<ref bean="transactionManager"/>
Expand Down
Loading
Loading