Y24-481 - Study setup page changes #4541
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4541 +/- ##
===========================================
+ Coverage 89.29% 89.30% +0.01%
===========================================
Files 1406 1406
Lines 30010 30022 +12
===========================================
+ Hits 26797 26811 +14
+ Misses 3213 3211 -2 ☔ View full report in Codecov by Sentry. |
…in edit form to fail validation by default when editing an old study
…val of valid default values
There was a problem hiding this comment.
Minor comment re the code; code looks good otherwise.
But I think it would be worth discussing how to handle the Edit Study page though. Left some thoughts below.
And as discussed, would you mind keeping in Liz Cook in the loop about the final version of the Study setup questions for her Google Form
A few things regarding this:
Happy to discuss further. |
|
I forgot to update that PR description, I have got around a few of the issues but yes its a little bit tricky to balance. We have to validate against the new data release agreement and we cant backfill. The current setup keeps the old data release data present but uneditable and if a user needs to edit a study they will need to update the data release to current standards. I think this is something to ask Liz.
Its not broken as such, they can be edited but they may need to update their data release values to match the current standard.
Yes information around data release agreement. E.G. They won't be able to edit the old fields that have been removed, such as data delay approval.
The studies are still valid, just when they go to edit it they may need to update their release data.
Good idea 👍
|
|
I haven't really looked at this PR (can do if you'd like me to) but I just had a thought - there are 2 study edit pages in SS - the normal one and the admin one. e.g. https://uat.sequencescape.psd.sanger.ac.uk/studies/5895/edit and https://uat.sequencescape.psd.sanger.ac.uk/admin/studies (search for the study) I think they share code, but have behaved differently in the past, which has caused problems. Just checking whether you're aware of them both, and whether you've tested your changes on both. Thanks |
Ah interesting. I didn't know that. I'll have a look into the admin one and check the behaviour 👍 |
|
Note: I would like to mention that radio to textbox conversion is supported by the sample_metadata schema as the fields are defined as varchar(255). The longest strings in the options fit as well. |
…o y24-481-study-setup-page
| <% | ||
| group.change_select_options_for(:data_release_delay_reason, when: :data_release_study_type_id, values: { | ||
| DataReleaseStudyType.assay_types.map(&:id) => [ '' ] + Study::DATA_RELEASE_DELAY_REASONS_ASSAY, | ||
| DataReleaseStudyType.non_assay_types.map(&:id) => [ '' ] + Study::DATA_RELEASE_DELAY_REASONS_STANDARD | ||
| DataReleaseStudyType.assay_types.map(&:id) => [ '' ] + study.data_release_delay_options(assay_option: true), |
There was a problem hiding this comment.
Do we still need to keep the empty option?
There was a problem hiding this comment.
Hmm not sure. These empty options were already here, I guess its for a similar reason I added them elsewhere - to force a user selection.
| DataReleaseStudyType.assay_types.map(&:id) => [ '' ] + Study::DATA_RELEASE_DELAY_REASONS_ASSAY, | ||
| DataReleaseStudyType.non_assay_types.map(&:id) => [ '' ] + Study::DATA_RELEASE_DELAY_REASONS_STANDARD | ||
| DataReleaseStudyType.assay_types.map(&:id) => [ '' ] + study.data_release_delay_options(assay_option: true), | ||
| DataReleaseStudyType.non_assay_types.map(&:id) => [ '' ] + study.data_release_delay_options |
There was a problem hiding this comment.
Do we still need to keep the empty option?
Closes #4534
Changes proposed in this pull request
Instructions for Reviewers
Please pull down and confirm the question and options now visible match those required in the story.
One thing to note - previous studies with answers for 'What is the reason for preventing data release' and 'Reason for delaying release' will need to update these fields if they are edited to pass the new validation options.