Feat/dbgap options #702
Feat/dbgap options #702
Conversation
Pull Request Test Coverage Report for Build 7944
💛 - Coveralls |
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
| @@ -370,12 +370,26 @@ dbGaP: | |||
| proxy_user: '' | |||
| protocol: 'sftp' | |||
| decrypt_key: '' | |||
| # parse out the consent from the dbgap accession number such that something | |||
| # like "phs000123.v1.p1.c2" becomes "phs000123.c2". | |||
There was a problem hiding this comment.
what does .v1.p1 mean? :D
There was a problem hiding this comment.
| ] | ||
|
|
||
| assert "phs000179" in resource_to_parent_paths | ||
| assert resource_to_parent_paths["phs000179"] == ["/orgA/programs/"] |
There was a problem hiding this comment.
phs000179 is not in study_to_resource_namespaces so where does the mapping come from?
There was a problem hiding this comment.
gets put in the _default namespace
…s no overarching common exchange area, just parent study common exchange areas
…ecate existing handling giving access to phsid
| # Will NOT MATCH forms like: phs000123 | ||
| # | ||
| # WARNING: Do not change this without consulting the code that uses it | ||
| DBGAP_ACCESSION_WITH_CONSENT_REGEX: '(?P<phsid>phs[0-9]+)(.(?P<version>v[0-9]+)){0,1}(.(?P<participant_set>p[0-9]+)){0,1}.(?P<consent>c[0-9]+)' |
There was a problem hiding this comment.
why did you choose to make this configurable?
There was a problem hiding this comment.
in case dbgap projects we receive change format, we don't have to edit code. there are possibilities where the first 3 chars are not phs. I'd rather be able to change it in configuration in the future than edit code 🤷♂️ idk, I could be convinced otherwise, but since this is an external format we are expected, figured it'd feel more at home in configuration
There was a problem hiding this comment.
but instead of editing code in 1 place, we would have to edit the default config + edit the value in the config of all the Commons that use it 😬
There was a problem hiding this comment.
not necessarily, just edit the default and don't supply it in the configurations in the commons so they use the default
There was a problem hiding this comment.
and then if a specific commons expects something different they can override it
There was a problem hiding this comment.
like if for some commons we need phs123 but others we start getting phg123, better to have that in config. I see your point though, it's not of immediate use now, I just feel like this is something that should be configurable
There was a problem hiding this comment.
true!! assuming we remember to leave it out of the commons' configs 😅
fence/sync/sync_users.py
Outdated
| # go through existing access and find any c999's and make sure to give access to | ||
| # all accessions with consent for that phsid | ||
| for username, user_project_info in copy.deepcopy(user_projects).items(): | ||
| for project, permissions in copy.deepcopy(user_project_info).items(): |
There was a problem hiding this comment.
permissions is not used - either use for project in copy.deepcopy(user_project_info) here, or use permissions instead of read-storage later
tests/dbgap_sync/test_user_sync.py
Outdated
| assert "phs000178.c999" in resource_to_parent_paths | ||
| assert resource_to_parent_paths["phs000178.c999"] == ["/orgA/programs/"] | ||
|
|
||
| assert "test_common_exchange_area" in resource_to_parent_paths | ||
| assert resource_to_parent_paths["test_common_exchange_area"] == [ | ||
| "/dbgap/programs/" | ||
| ] | ||
| else: |
There was a problem hiding this comment.
did you forget to paste this somewhere? 🤔
There was a problem hiding this comment.
no, this logic was confusing, this is already checked below, no need to do it twice
New Features
c999consent group nowc999consent group" featureBreaking Changes
c999is no longer converted to justphsidwhen parsing consent codes (which causes issues when switching the configuration on/off in terms of data access). Now, when consent code parsing is on, usersync handlesc999by providing access to all consents explicitly (including itself,.c999, to represent the study-specific exchange area)Bug Fixes
Improvements
Dependency updates
Deployment changes
c999the same as before. If using dbgap sync w/ consent code parsing and indexd records have acls with justphsidto represent objects someone w/c999should be able to see, you will need to ensure that the records also have the phsid.consent_code as well or users will lose access