Skip to content
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

Merged
merged 14 commits into from
Jan 25, 2024
Merged

Conversation

zb32
Copy link
Contributor

@zb32 zb32 commented Jan 12, 2024

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.

src/npg_irods/metadata/lims.py Outdated Show resolved Hide resolved
src/npg_irods/ont.py Outdated Show resolved Hide resolved
src/npg_irods/illumina.py Outdated Show resolved Hide resolved
src/npg_irods/metadata/lims.py Outdated Show resolved Hide resolved
src/npg_irods/metadata/lims.py Outdated Show resolved Hide resolved
@zb32 zb32 force-pushed the add_new_study_groups_for_human branch from 377418a to e7365d6 Compare January 24, 2024 10:54
@zb32 zb32 requested a review from kjsanger January 24, 2024 14:28
Copy link
Member

@kjsanger kjsanger left a 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).

src/npg_irods/illumina.py Outdated Show resolved Hide resolved
src/npg_irods/metadata/lims.py Outdated Show resolved Hide resolved
tests/test_illumina.py Outdated Show resolved Hide resolved
@kjsanger
Copy link
Member

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.

@zb32
Copy link
Contributor Author

zb32 commented Jan 25, 2024

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).

I think David said that the '_human' group should just be used for the human subset

@kjsanger
Copy link
Member

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).

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 human -> _human for the scope.

@kjsanger kjsanger merged commit 2d4413c into devel Jan 25, 2024
10 checks passed
@kjsanger kjsanger deleted the add_new_study_groups_for_human branch January 25, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants