-
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-2: Add support for configuring (and fetching) Mother Child rel… #238
Conversation
…ationships within the EMR API module
Okay, took a while to rename all the classes and variables in the test case, but, mircalously all the tests passed first time after it compiled. I probably need to give this one more once-over before merging, and add a couple tests around the weird cases where you are restricting by both mother and child in a single request. Will take a look tomorrow, but wanted to get this open for review. |
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.
A few minor comments, but I think this looks like a good improvement, simplifies things and also makes things cleaner and clearer
|
||
for (Object req : l) { | ||
Object[] row = (Object[]) req; | ||
Child child = new Child(); | ||
MotherAndChild child = new 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.
Probably should rename this local variable to motherAndChild
, though you could also just have a constructor that takes in both and not need a local variable at all...
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.
Whoops, yep... missed this one.
private List<String> childUuids; // restrict to mothers of these children | ||
private boolean requireMotherHasActiveVisit = false; // restrict to mothers who have an active visit | ||
private boolean requireChildHasActiveVisit = false; // restrict to mothers of children who have an active visit | ||
private boolean requireChildBornDuringMothersActiveVisit = false; // restrict to mothers who had a child born during their active visit |
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 you did want these to read better, you could go with something like:
motherRequiredToHaveActiveVisit
-> isMotherRequiredToHaveActiveVisit()
childRequiredToHaveActiveVisit
-> isChildRequiredToHaveActiveVisit()
childRequiredToBeBornDuringMothersActiveVisit
-> isChildRequiredToBeBornDuringMothersActiveVisit()
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 like that.
…ionships within the EMR API module (update with code review comments)
…ationships within the EMR API module