Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Mother Child relationships within… #237
EA-198: Add support for configuring Mother Child relationships within… #237
Changes from all commits
a1367c7
c84a9ae
76327e3
26e9ff6
c4e92b7
00b2de2
fd9470d
77c0105
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thoughts about making this to be
private Mother mother;
?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'd rather keep the REST response as simple as possible. You mean have:
Then on a child you've have to child.mother.mother to get the mother?
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.
Hmm. I think given what you have in place it would make sense to just have one object:
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...
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 was thinking of that, though I thought there was a reason it wasn't going to work, will check again.
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.
What is this
fluent = true
? Don't we want these to act and behave like standard Javabean properties?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.
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
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.
No idea why it's called fluent. :)
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 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.
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... let's just stick with the ugly name then, will change.
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.
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.
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.
Thoughts about making this
private Child child;
?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 above
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 above again.
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.
Ditto
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.
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).
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.
They are flexible, but we are saying this is based around have a standardized uni-directional "Mother to Child" relationship type