-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add new study groups for human #235
Conversation
377418a
to
e7365d6
Compare
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.
The main thing still needed is that the existing test for xahuman should be updated to test for the new permissions. It currently just tests that the old permissions are removed (see test_updates_xahuman_permissions_mx
).
One more thing that I forgot - all the modified files will need their copyright statement (if they have one) updated to add the year 2024 to the list of years. |
I think David said that the '_human' group should just be used for the human subset |
I checked JIRA and there's no mention of xahuman, so I agree - let's interpret that at literally just |
I've implemented this so a irods group of ss__human get's added when the component has a subset of "human". I'm not sure if this is the best way to do it so open to feedback :D
I was also thinking for line 249 in
src/npg_irods/illumina.py
elif any(c.contains_nonconsented_human() for c in components): # Illumina specific
does It make sense to still implement it this way as now the logic for a subset of "human" and "xahuman" is different so maybe we make the distinction here and use two separate methods instead of having the logic inside the method? Hopefully that makes sense.