[GEN-1636] add IDs from retraction form in the retraction steps for creating clinical release files #164
Conversation
There was a problem hiding this comment.
I glanced at this and the overall logic looks good, but I'll leave it to @rxu17 for final review.
We should add tests to any new code that is written even if we don't have CI/CD set up.
There was a problem hiding this comment.
LGTM! Just a comment about including staging for the retraction table update.
I think eventually we want to support staging versions of the folders/tables used in the create_release_files.R as well but that would require us to support the input and output and would be more time than we can do this time.
| release_dat <- dataset %>% | ||
| filter(cohort_internal==selected_cohort) %>% | ||
| filter(!record_id %in% retracted_patient) %>% | ||
| select(all_of(release_cols)) | ||
|
|
||
| # remove retracted sample |
There was a problem hiding this comment.
I think it might be best to have some unit tests for this function since it does the retraction
EDIT: Based on coordination meeting discussion, this has to wait due to deadline tmrw 12/11
| parser.add_argument("-s", "--synapse_config", default=synapseclient.client.CONFIG_FILE, help="Path to Synapse credentials file") | ||
| parser.add_argument("-m", "--message", default="", help="Version comment for the table update") | ||
| parser.add_argument("-d", "--dry_run", action="store_true", help="Flag for dry run (no updates will be made)") | ||
|
|
There was a problem hiding this comment.
- Let's add an additional argument called
--productionand create a staging version of the retraction table here. - Then convert
RETRACTION_TABLE_IDto like a dictionary or something to hold the two sets of synapse ids. - Then add logic here to use the correct synapse id depending on the
productionflag.
This way we can run things on staging first to see if the retractions are as expected/test the synapse upload. Thoughts?
No description provided.