-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor families + add family participants to graphql #740
Conversation
…-family-participants-to-graphql
…articipants-to-graphql' of github.com:populationgenomics/metamist into add-family-participants-to-graphql
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #740 +/- ##
==========================================
+ Coverage 76.47% 77.15% +0.68%
==========================================
Files 148 157 +9
Lines 11919 12945 +1026
==========================================
+ Hits 9115 9988 +873
- Misses 2804 2957 +153 ☔ View full report in Codecov by Sentry. |
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 looks like there's a lot of cleaning up of the db typehints and changes to the graphql filters & loaders - how much do these reviewed? I can see the tests cover a fair chunk of it, but obviously Codecov has noted there are aspects missing from tests.
Otherwise, this is my understanding:
The entries in the family_participant table will now be query-able through GraphQL, either as a child of a query for participant
or family
. They won't be query-able at the root level, because a family_participant function hasn't been added to api/graphql/schema.py
under the Query
class.
When queried, a FamilyParticipant node will have the affected status as an integer field under affected
(alongside notes
, participant_id
, family_id
).
The family layers/tables updates also let you query families and filter the results by family IDs and/or family external IDs.
Is my understanding correct? And what do you think about the test coverage?
All in all, this looks pretty good! 👍
Thanks @EddieLF! Yeah, a bit of extra refactoring here to bring it in line with other classes. I'm going to merge this one after #615, because I'll need to update a few things that I've changed here - so if you want to re-review, feel free to do it after that. Let me know if you're not comfy with approving this (though I think you're in a great position too :) ). Thanks for the 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.
Looks good to me, nice that you improved the test coverage and also updated the participant layer typing
typehints to align with the other typehint changes ➕ 👍
…-family-participants-to-graphql
I got carried away, but broadly:
Will self-review, then assign out soon.