Skip to content

Conversation

@rushtong
Copy link
Contributor

@rushtong rushtong commented Oct 30, 2024

Addresses

https://broadworkbench.atlassian.net/browse/DT-945

Summary

This PR makes some changes to the Update Study API to fix a few bugs. As a result of updating a production study (see DT-941), there was a non-trivial amount of hand-editing the JSON to get around incorrect validation rules. This PR fixes that to make updates a lot more smoother in the future.

  • Phenotype is not a required field
  • Data Custodian Email is not a required field
  • If the consent group (i.e. dataset) is being updated:
    • All data use related fields are ignored
    • Access management changes are ignored
  • If the consent group (i.e. dataset) is a new entity, all data use and access management values are preserved.
  • Everything else provided in the study is valid for update.

Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rushtong rushtong marked this pull request as ready for review October 30, 2024 20:55
@rushtong rushtong requested a review from a team as a code owner October 30, 2024 20:55
@rushtong rushtong requested review from pshapiro4broad and rjohanek and removed request for a team October 30, 2024 20:55
@Override
public boolean shouldSkipField(FieldAttributes fieldAttributes) {
final HashSet<String> exclusions = new HashSet<>(List.of(
"accessManagement",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are data use fields an enum somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha ... they are defined as static strings in DatasetRegistrationSchemaV1Builder, thank you! Will make that update.

Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a great improvement to make updating studies easier for users in the future!

Copy link
Contributor

@snf2ye snf2ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time wrapping my head around using GSON to convert between json and java objects, but as far as I can tell, LGTM!

@rushtong
Copy link
Contributor Author

I'm having a hard time wrapping my head around using GSON to convert between json and java objects, but as far as I can tell, LGTM!

To be honest, this was super painful. If I were to re-implement this, I might choose a very different set of updates to support.

@rushtong rushtong merged commit 81fe1c0 into develop Oct 31, 2024
@rushtong rushtong deleted the gr-DT-945-better-study-update branch October 31, 2024 18:23
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.

4 participants