-
Notifications
You must be signed in to change notification settings - Fork 143
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.
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 (and fetching) Mother Child relat… #239
Conversation
…ionships within the EMR API module
@@ -216,5 +216,5 @@ 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"; | |||
public static final String METADATA_MAPPING_MOTHER_CHILD_RELATIONSHIP_TYPE = "emr.motherChildRelationshipType"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other EMR API properties started with "emr", so I renamed this for consistency.
@@ -47,7 +47,9 @@ public List<MotherAndChild> getMothersAndChildren(MothersAndChildrenSearchCriter | |||
|
|||
Map<String, Object> parameters = new HashMap<>(); | |||
parameters.put("motherUuids", criteria.getMotherUuids()); | |||
parameters.put("restrictByMothers", criteria.getMotherUuids() != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see below
((:motherUuids) is null or mother.uuid in (:motherUuids)) | ||
and ((:childUuids) is null or child.uuid in (:childUuids)) | ||
(:restrictByMothers = false or mother.uuid in (:motherUuids)) | ||
and (:restrictByChildren = false or child.uuid in (:childUuids)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was stumped by this for a while... the "(:motherUuids) is null" was throwing an error when motherUuids was an array, which in-and-of-itself isn't necessarily surprising, but it was working in the tests, it was only failing when trying to use it for "real". No idea what the difference was. I figured I was inadvertently passing in a slightly different datatype, but debugging they looked the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I ran into the same thing. I assume you saw how I ended up handling this in the other hql queries, but if not we arrived at the same solution :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I guess I missed that
criteria.setChildRequiredToHaveActiveVisit(requireChildHasActiveVisit); | ||
criteria.setChildRequiredToBeBornDuringMothersActiveVisit(requireChildBornDuringMothersActiveVisit); | ||
List<MotherAndChild> motherAndChildList = maternalService.getMothersAndChildren(criteria); | ||
return new NeedsPaging<>(motherAndChildList, context).toSimpleObject(new SimpleBeanConverter<MotherAndChild>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to create a custom converter, but at the end-of-the-day the buildt in converter seemed to give reps with reasonable defaults. I tested full, default, rep, and a custom rep.
RequestContext context = RestUtil.getRequestContext(request, response, Representation.DEFAULT); | ||
MothersAndChildrenSearchCriteria criteria = new MothersAndChildrenSearchCriteria(); | ||
criteria.setMotherUuids(StringUtils.isNotBlank(motherUuids) ? Arrays.asList(motherUuids.split(",")) : null); | ||
criteria.setChildUuids(StringUtils.isNotBlank(childUuids) ? Arrays.asList(childUuids.split(",")) : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there is a built-in way to map a parameter to an array, but the few ways I tried didn't seem to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn't supported out of the box, I'd assume it isn't considered best practice. The way to accomplish this is typically to just pass in the request parameter multiple times. This is one of the reasons I was thinking the singular tense maybe: mother=xxx&mother=yyy&mother=zzz
. I agree that mothers=xxx,yyy,zzz
is more concise, I just don't know if it is considered typical practice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use:
@RequestParam(required = false, value = "mother") List<String> motherUuids,
@RequestParam(required = false, value = "child") List<String> childUuids,
((:motherUuids) is null or mother.uuid in (:motherUuids)) | ||
and ((:childUuids) is null or child.uuid in (:childUuids)) | ||
(:restrictByMothers = false or mother.uuid in (:motherUuids)) | ||
and (:restrictByChildren = false or child.uuid in (:childUuids)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I ran into the same thing. I assume you saw how I ended up handling this in the other hql queries, but if not we arrived at the same solution :)
HttpServletRequest request, | ||
HttpServletResponse response, | ||
@RequestParam(required = false, value = "motherUuids") String motherUuids, | ||
@RequestParam(required = false, value = "childUuids") String childUuids, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call these properties mothers
and children
? Or limitToMothers
and limitToChildren
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If best practice is not to use the comments, I would change this probably to "mother" and "child" like to specified below, let me fiddle around with this.
RequestContext context = RestUtil.getRequestContext(request, response, Representation.DEFAULT); | ||
MothersAndChildrenSearchCriteria criteria = new MothersAndChildrenSearchCriteria(); | ||
criteria.setMotherUuids(StringUtils.isNotBlank(motherUuids) ? Arrays.asList(motherUuids.split(",")) : null); | ||
criteria.setChildUuids(StringUtils.isNotBlank(childUuids) ? Arrays.asList(childUuids.split(",")) : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn't supported out of the box, I'd assume it isn't considered best practice. The way to accomplish this is typically to just pass in the request parameter multiple times. This is one of the reasons I was thinking the singular tense maybe: mother=xxx&mother=yyy&mother=zzz
. I agree that mothers=xxx,yyy,zzz
is more concise, I just don't know if it is considered typical practice...
…ionships within the EMR API module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated based on code review, merging in, thanks @mseaton
…ionships within the EMR API module