From 5bc22bf9a04afc1b02e0285477ff3d048a60084f Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Tue, 3 Dec 2024 14:19:48 +0000 Subject: [PATCH 01/16] feat(studies): Updates new/edit study questions --- app/models/study.rb | 31 +++++++++++++++---- .../shared/metadata/edit/_study.html.erb | 2 +- config/locales/metadata/en.yml | 7 ++--- ...eeds_to_be_reverted_to_old_version.feature | 2 +- .../8447221_data_release_help_text.feature | 9 ++---- features/studies/data_release_timings.feature | 10 +++--- spec/features/studies/create_study_spec.rb | 6 ++-- spec/features/studies/edit_study_spec.rb | 4 +-- spec/features/studies/manage_study_spec.rb | 4 +-- .../studies/view_study_properties_spec.rb | 2 +- 10 files changed, 46 insertions(+), 31 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index ebfd589446..0a291fba0c 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -82,11 +82,25 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength DATA_RELEASE_TIMING_IMMEDIATE, DATA_RELEASE_TIMING_DELAYED ].freeze - DATA_RELEASE_PREVENTION_REASONS = ['data validity', 'legal', 'replication of data subset'].freeze + + OLD_DATA_RELEASE_PREVENTION_REASONS = ['data validity', 'legal', 'replication of data subset'].freeze + DATA_RELEASE_PREVENTION_REASONS = [ + 'Pilot or validation studies - DAC approval not required', + 'Collaborators will share data in a research repository - DAC approval not required', + 'Prevent harm (e.g sensitive studies or biosecurity) - DAC approval required', + 'Protecting IP - DAC approval required', + 'Other (please specify)' + ].freeze DATA_RELEASE_DELAY_FOR_OTHER = 'other' - DATA_RELEASE_DELAY_REASONS_STANDARD = ['phd study', DATA_RELEASE_DELAY_FOR_OTHER].freeze DATA_RELEASE_DELAY_REASONS_ASSAY = ['phd study', 'assay of no other use', DATA_RELEASE_DELAY_FOR_OTHER].freeze + DATA_RELEASE_DELAY_REASONS_STANDARD = [ + 'PhD study', + 'Capacity building', + 'Intellectual property protection', + 'Additional time to make data FAIR', + DATA_RELEASE_DELAY_FOR_OTHER + ].freeze DATA_RELEASE_DELAY_PERIODS = ['3 months', '6 months', '9 months', '12 months', '18 months'].freeze @@ -217,6 +231,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength in: DATA_RELEASE_DELAY_REASONS_ASSAY, if: :delayed_release? ) + custom_attribute(:data_release_delay_period, required: true, in: DATA_RELEASE_DELAY_PERIODS, if: :delayed_release?) custom_attribute(:bam, default: true) @@ -235,10 +250,14 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength custom_attribute(:data_release_delay_approval, in: YES_OR_NO, default: NO) end - with_options(if: :never_release?, required: true) do - custom_attribute(:data_release_prevention_reason, in: DATA_RELEASE_PREVENTION_REASONS) - custom_attribute(:data_release_prevention_approval, in: YES_OR_NO) - custom_attribute(:data_release_prevention_reason_comment) + with_options(if: :never_release?) do + custom_attribute( + :data_release_prevention_reason, + in: DATA_RELEASE_PREVENTION_REASONS + OLD_DATA_RELEASE_PREVENTION_REASONS, + required: true + ) + custom_attribute(:data_release_prevention_reason_comment, required: true) + custom_attribute(:data_release_prevention_approval) end # NOTE: Additional validation in Study::Metadata Class to validate_presence_of :data_access_group, if: :managed diff --git a/app/views/shared/metadata/edit/_study.html.erb b/app/views/shared/metadata/edit/_study.html.erb index 4f16c90e52..aa1e599762 100644 --- a/app/views/shared/metadata/edit/_study.html.erb +++ b/app/views/shared/metadata/edit/_study.html.erb @@ -60,7 +60,7 @@ <% metadata_fields.related_fields(to: :data_release_strategy, when: Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE) do %> <%= group.select(:data_release_prevention_reason, Study::DATA_RELEASE_PREVENTION_REASONS) %> - <%= group.radio_select(:data_release_prevention_approval, Study::YES_OR_NO) %> + <%= group.text_area(:data_release_prevention_approval) %> <%= group.text_area(:data_release_prevention_reason_comment) %> <% end %> <% metadata_fields.related_fields(to: :data_release_strategy, in: Study::DATA_RELEASE_STRATEGIES, not: Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE) do %> diff --git a/config/locales/metadata/en.yml b/config/locales/metadata/en.yml index 2abb785013..85e636230a 100644 --- a/config/locales/metadata/en.yml +++ b/config/locales/metadata/en.yml @@ -422,7 +422,7 @@ en: values: open: "Open (ENA)" managed: "Managed (EGA)" - not_applicable: "Not Applicable (Contact Datasharing)" + not_applicable: "Not Applicable" data_release_standard_agreement: label: "Will you be using WTSI's standard access agreement?" @@ -432,13 +432,13 @@ en: data_release_timing: label: How is the data release to be timed? - help: "Choose from:

Immediate: To be released as soon as possible.

Standard: To be released:

Delayed:

Never: This option is only available if the data release strategy is set to 'Not applicable.'

" + help: "Choose from:

Immediate: To be released as soon as possible.

Standard: To be released:

Delayed:

Never: This option is only available if the data release strategy is set to 'Not applicable.'

" data_release_prevention_reason: label: What is the reason for preventing data release? data_release_prevention_approval: - label: Has this been approved? + label: If reason for exemption requires DAC approval, what is the approval number? help: "If this is for data validity reasons: approval from the sponsor is required
If this is for legal reasons: approval from the Data Sharing Committee is required (please contact sd4)
" data_release_prevention_reason_comment: @@ -446,7 +446,6 @@ en: data_release_delay_reason: label: Reason for delaying release - help: 'To apply for a delay, please contact <%= configatron.data_sharing_contact.email %>' data_release_delay_other_comment: label: Please explain the reason for delaying release diff --git a/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature b/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature index 331c614d46..59e640bd67 100644 --- a/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature +++ b/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature @@ -115,7 +115,7 @@ Feature: The XML for the sequencescape API Not specified - Has this been approved? + If reason for exemption requires DAC approval, what is the approval number? diff --git a/features/studies/8447221_data_release_help_text.feature b/features/studies/8447221_data_release_help_text.feature index a245f126d6..0b20d4bab8 100644 --- a/features/studies/8447221_data_release_help_text.feature +++ b/features/studies/8447221_data_release_help_text.feature @@ -23,10 +23,7 @@ Feature: Update the data release fields for creating a study When I choose "" from "What is the data release strategy for this study?" When I select "delayed" from "How is the data release to be timed?" When I select "other" from "Reason for delaying release" - Then the help text for "Reason for delaying release" should contain: - """ - To apply for a delay, please contact datasharing@example.com - """ + Then I should exactly see "Reason for delaying release" Examples: | release strategy | @@ -34,9 +31,9 @@ Feature: Update the data release fields for creating a study | Open (ENA) | Scenario: Add help text to has this been approved for never release (4044343) - When I choose "Not Applicable (Contact Datasharing)" from "What is the data release strategy for this study?" + When I choose "Not Applicable" from "What is the data release strategy for this study?" When I select "never" from "How is the data release to be timed?" - Then the help text for "Has this been approved?" should contain: + Then the help text for "If reason for exemption requires DAC approval, what is the approval number?" should contain: """ If this is for data validity reasons: approval from the sponsor is required If this is for legal reasons: approval from the Data Sharing Committee is required (please contact sd4) diff --git a/features/studies/data_release_timings.feature b/features/studies/data_release_timings.feature index c8e210b365..15ba11a340 100644 --- a/features/studies/data_release_timings.feature +++ b/features/studies/data_release_timings.feature @@ -28,7 +28,7 @@ Feature: Studies have timings for release of their data Scenario: When the data release is delayed for PhD study Given I select "delayed" from "How is the data release to be timed?" - And I select "phd study" from "Reason for delaying release" + And I select "PhD study" from "Reason for delaying release" Then the "Comment regarding data release timing and approval" field is hidden When I select "6 months" from "Delay for" And I press "Create" @@ -70,19 +70,19 @@ Feature: Studies have timings for release of their data | 12 months | Scenario: When the data release is never but the comment is not supplied - When I choose "Not Applicable (Contact Datasharing)" from "What is the data release strategy for this study?" + When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" - And I choose "Yes" from "Has this been approved?" + And I fill in "12345" from "If reason for exemption requires DAC approval, what is the approval number?" When I press "Create" Then I should be on the studies page # Again, ideally without study metadata And I should see "Study metadata data release prevention reason comment can't be blank" Scenario: When the data release is never and the comment is supplied - When I choose "Not Applicable (Contact Datasharing)" from "What is the data release strategy for this study?" + When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" And I fill in "Comment regarding prevention of data release and approval" with "Some reason" - And I choose "Yes" from "Has this been approved?" + And I fill in "12345" from "If reason for exemption requires DAC approval, what is the approval number? When I press "Create" Then I should be on the study information page for "Testing data release strategies" And I should see "Your study has been created" diff --git a/spec/features/studies/create_study_spec.rb b/spec/features/studies/create_study_spec.rb index 4f63079c5b..e4545f5182 100644 --- a/spec/features/studies/create_study_spec.rb +++ b/spec/features/studies/create_study_spec.rb @@ -40,7 +40,7 @@ within_fieldset('What is the data release strategy for this study?') do expect(page).to have_field('Open (ENA)', type: :radio) expect(page).to have_field('Managed (EGA)', type: :radio) - expect(page).to have_field('Not Applicable (Contact Datasharing)', type: :radio) + expect(page).to have_field('Not Applicable', type: :radio) end within_fieldset('Study Visibility') do @@ -132,8 +132,8 @@ expect(page).to have_field('HuMFre approval number', type: :text) end - it 'displays HuMFre approval number when Not Applicable (Contact Datasharing) is clicked' do - choose('Not Applicable (Contact Datasharing)', allow_label_click: true) + it 'displays HuMFre approval number when Not Applicable is clicked' do + choose('Not Applicable', allow_label_click: true) expect(page).to have_field('HuMFre approval number', type: :text) end end diff --git a/spec/features/studies/edit_study_spec.rb b/spec/features/studies/edit_study_spec.rb index c8a29b8138..a21ac64c62 100644 --- a/spec/features/studies/edit_study_spec.rb +++ b/spec/features/studies/edit_study_spec.rb @@ -69,8 +69,8 @@ expect(page).to have_field('HuMFre approval number', type: :text) end - it 'displays HuMFre approval number when Not Applicable (Contact Datasharing) is clicked' do - choose('Not Applicable (Contact Datasharing)', allow_label_click: true) + it 'displays HuMFre approval number when Not Applicable is clicked' do + choose('Not Applicable', allow_label_click: true) expect(page).to have_field('HuMFre approval number', type: :text) end end diff --git a/spec/features/studies/manage_study_spec.rb b/spec/features/studies/manage_study_spec.rb index a6a1a094c3..a6e4b6f5de 100644 --- a/spec/features/studies/manage_study_spec.rb +++ b/spec/features/studies/manage_study_spec.rb @@ -34,8 +34,8 @@ expect(page).to have_field('HuMFre approval number', type: :text) end - it 'displays HuMFre approval number when Not Applicable (Contact Datasharing) is clicked' do - choose('Not Applicable (Contact Datasharing)', allow_label_click: true) + it 'displays HuMFre approval number when Not Applicable is clicked' do + choose('Not Applicable', allow_label_click: true) expect(page).to have_field('HuMFre approval number', type: :text) end end diff --git a/spec/features/studies/view_study_properties_spec.rb b/spec/features/studies/view_study_properties_spec.rb index a3359f72ac..b64305bacd 100644 --- a/spec/features/studies/view_study_properties_spec.rb +++ b/spec/features/studies/view_study_properties_spec.rb @@ -52,7 +52,7 @@ expect(page).to have_content('HuMFre approval number: 12345') end - it 'displays HuMFre approval number for Not Applicable (Contact Datasharing) data release strategy' do + it 'displays HuMFre approval number for Not Applicable data release strategy' do study.study_metadata.data_release_strategy = Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE study.study_metadata.data_release_timing = Study::DATA_RELEASE_TIMING_NEVER study.study_metadata.data_release_prevention_reason = Study::DATA_RELEASE_PREVENTION_REASONS[0] From 9f6cbcd84154d45e5f082fd256930b644cc26f9c Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Wed, 4 Dec 2024 10:10:21 +0000 Subject: [PATCH 02/16] feat(studies): corrects undefined cucumber tests in data release timing --- app/models/study.rb | 2 +- features/studies/data_release_timings.feature | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index 0a291fba0c..f891e86125 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -228,7 +228,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength custom_attribute( :data_release_delay_reason, required: true, - in: DATA_RELEASE_DELAY_REASONS_ASSAY, + in: DATA_RELEASE_DELAY_REASONS_ASSAY + DATA_RELEASE_DELAY_REASONS_STANDARD, if: :delayed_release? ) diff --git a/features/studies/data_release_timings.feature b/features/studies/data_release_timings.feature index 15ba11a340..b59a19fc87 100644 --- a/features/studies/data_release_timings.feature +++ b/features/studies/data_release_timings.feature @@ -72,7 +72,7 @@ Feature: Studies have timings for release of their data Scenario: When the data release is never but the comment is not supplied When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" - And I fill in "12345" from "If reason for exemption requires DAC approval, what is the approval number?" + And I fill in "If reason for exemption requires DAC approval, what is the approval number?" with "12345" When I press "Create" Then I should be on the studies page # Again, ideally without study metadata @@ -82,7 +82,7 @@ Feature: Studies have timings for release of their data When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" And I fill in "Comment regarding prevention of data release and approval" with "Some reason" - And I fill in "12345" from "If reason for exemption requires DAC approval, what is the approval number? + And I fill in "If reason for exemption requires DAC approval, what is the approval number?" with "12345" When I press "Create" Then I should be on the study information page for "Testing data release strategies" And I should see "Your study has been created" From 12d0368ef42cd37b04bfa680d18156accff610e0 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Wed, 4 Dec 2024 14:39:57 +0000 Subject: [PATCH 03/16] feat(study): removes redundant old data release prevention reasons --- app/models/study.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index f891e86125..0f50fdc6c0 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -83,7 +83,6 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength DATA_RELEASE_TIMING_DELAYED ].freeze - OLD_DATA_RELEASE_PREVENTION_REASONS = ['data validity', 'legal', 'replication of data subset'].freeze DATA_RELEASE_PREVENTION_REASONS = [ 'Pilot or validation studies - DAC approval not required', 'Collaborators will share data in a research repository - DAC approval not required', @@ -93,7 +92,6 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength ].freeze DATA_RELEASE_DELAY_FOR_OTHER = 'other' - DATA_RELEASE_DELAY_REASONS_ASSAY = ['phd study', 'assay of no other use', DATA_RELEASE_DELAY_FOR_OTHER].freeze DATA_RELEASE_DELAY_REASONS_STANDARD = [ 'PhD study', 'Capacity building', @@ -101,6 +99,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength 'Additional time to make data FAIR', DATA_RELEASE_DELAY_FOR_OTHER ].freeze + DATA_RELEASE_DELAY_REASONS_ASSAY = ['assay of no other use', *DATA_RELEASE_DELAY_REASONS_STANDARD].freeze DATA_RELEASE_DELAY_PERIODS = ['3 months', '6 months', '9 months', '12 months', '18 months'].freeze @@ -228,7 +227,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength custom_attribute( :data_release_delay_reason, required: true, - in: DATA_RELEASE_DELAY_REASONS_ASSAY + DATA_RELEASE_DELAY_REASONS_STANDARD, + in: DATA_RELEASE_DELAY_REASONS_ASSAY, if: :delayed_release? ) @@ -251,11 +250,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength end with_options(if: :never_release?) do - custom_attribute( - :data_release_prevention_reason, - in: DATA_RELEASE_PREVENTION_REASONS + OLD_DATA_RELEASE_PREVENTION_REASONS, - required: true - ) + custom_attribute(:data_release_prevention_reason, in: DATA_RELEASE_PREVENTION_REASONS, required: true) custom_attribute(:data_release_prevention_reason_comment, required: true) custom_attribute(:data_release_prevention_approval) end From ea5deb33d50de2bcd60b47d7929d621430eb857f Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Wed, 4 Dec 2024 16:36:52 +0000 Subject: [PATCH 04/16] tests(studies): updates study metadata test data to reflect data release changes --- spec/factories/study_factories.rb | 2 +- spec/models/study_spec.rb | 4 ++-- test/unit/data_release_test.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/factories/study_factories.rb b/spec/factories/study_factories.rb index 8ab0fcae97..9603449b12 100644 --- a/spec/factories/study_factories.rb +++ b/spec/factories/study_factories.rb @@ -54,7 +54,7 @@ new_field_values = { data_release_strategy: 'not applicable', data_release_timing: 'never', - data_release_prevention_reason: 'data validity', + data_release_prevention_reason: 'Protecting IP - DAC approval required', data_release_prevention_approval: 'Yes', data_release_prevention_reason_comment: 'This is the reason context' } diff --git a/spec/models/study_spec.rb b/spec/models/study_spec.rb index e6547b6917..3b146ee736 100644 --- a/spec/models/study_spec.rb +++ b/spec/models/study_spec.rb @@ -445,7 +445,7 @@ data_release_strategy: 'open', data_release_standard_agreement: 'Yes', data_release_timing: 'standard', - data_release_delay_reason: 'phd study', + data_release_delay_reason: 'PhD study', data_release_delay_period: '3 months', bam: true, data_release_delay_other_comment: 'Data Release delay other comment', @@ -456,7 +456,7 @@ ega_policy_accession_number: 'POL123456', array_express_accession_number: 'ARR123456', data_release_delay_approval: 'Yes', - data_release_prevention_reason: 'data validity', + data_release_prevention_reason: 'Protecting IP - DAC approval required', data_release_prevention_approval: 'Yes', data_release_prevention_reason_comment: 'Data Release prevention reason comment', data_access_group: 'something', diff --git a/test/unit/data_release_test.rb b/test/unit/data_release_test.rb index f1dee7e4af..328b3bc9d4 100644 --- a/test/unit/data_release_test.rb +++ b/test/unit/data_release_test.rb @@ -93,7 +93,7 @@ class DataReleaseTest < ActiveSupport::TestCase setup do @study.study_metadata.data_release_strategy = 'never' @study.study_metadata.data_release_timing = 'never' - @study.study_metadata.data_release_prevention_reason = 'legal' + @study.study_metadata.data_release_prevention_reason = 'Protecting IP - DAC approval required' @study.study_metadata.data_release_prevention_approval = 'Yes' @study.study_metadata.data_release_prevention_reason_comment = 'It just is' end @@ -106,7 +106,7 @@ class DataReleaseTest < ActiveSupport::TestCase context 'and release timing is delayed' do setup do @study.study_metadata.data_release_timing = 'delayed' - @study.study_metadata.data_release_delay_reason = 'phd study' + @study.study_metadata.data_release_delay_reason = 'PhD study' end data_release_strategies.each do |strategy| From 5ffaa681784da12565e5594e2d18bdb91c75d767 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Mon, 9 Dec 2024 11:01:58 +0000 Subject: [PATCH 05/16] fix(studies): adds blank values to changed study data release values in edit form to fail validation by default when editing an old study --- app/views/shared/metadata/edit/_study.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/shared/metadata/edit/_study.html.erb b/app/views/shared/metadata/edit/_study.html.erb index aa1e599762..70942e2638 100644 --- a/app/views/shared/metadata/edit/_study.html.erb +++ b/app/views/shared/metadata/edit/_study.html.erb @@ -59,13 +59,13 @@ %> <% metadata_fields.related_fields(to: :data_release_strategy, when: Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE) do %> - <%= group.select(:data_release_prevention_reason, Study::DATA_RELEASE_PREVENTION_REASONS) %> + <%= group.select(:data_release_prevention_reason, [''] + Study::DATA_RELEASE_PREVENTION_REASONS) %> <%= group.text_area(:data_release_prevention_approval) %> <%= group.text_area(:data_release_prevention_reason_comment) %> <% end %> <% metadata_fields.related_fields(to: :data_release_strategy, in: Study::DATA_RELEASE_STRATEGIES, not: Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE) do %> <% metadata_fields.related_fields(to: :data_release_timing, when: Study::DATA_RELEASE_TIMING_DELAYED) do %> - <%= group.select(:data_release_delay_reason, Study::DATA_RELEASE_DELAY_REASONS_STANDARD) %> + <%= group.select(:data_release_delay_reason, [''] + Study::DATA_RELEASE_DELAY_REASONS_STANDARD) %> <% 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, From 6966d2a3a0dade1d9bbc42f230e13781aef0fc34 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Mon, 9 Dec 2024 11:46:09 +0000 Subject: [PATCH 06/16] tests(studies): updates cucumber studies data release test after removal of valid default values --- features/studies/data_release_timings.feature | 2 ++ 1 file changed, 2 insertions(+) diff --git a/features/studies/data_release_timings.feature b/features/studies/data_release_timings.feature index b59a19fc87..4c0c2f3961 100644 --- a/features/studies/data_release_timings.feature +++ b/features/studies/data_release_timings.feature @@ -72,6 +72,7 @@ Feature: Studies have timings for release of their data Scenario: When the data release is never but the comment is not supplied When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" + And I select "Protecting IP - DAC approval required" from "What is the reason for preventing data release?" And I fill in "If reason for exemption requires DAC approval, what is the approval number?" with "12345" When I press "Create" Then I should be on the studies page @@ -81,6 +82,7 @@ Feature: Studies have timings for release of their data Scenario: When the data release is never and the comment is supplied When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" + And I select "Protecting IP - DAC approval required" from "What is the reason for preventing data release?" And I fill in "Comment regarding prevention of data release and approval" with "Some reason" And I fill in "If reason for exemption requires DAC approval, what is the approval number?" with "12345" When I press "Create" From 380d1615e43449256b551d0c8ab4663c43362f35 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Mon, 16 Dec 2024 15:41:28 +0000 Subject: [PATCH 07/16] feat(study-metadata): removes data_release_delay_approval for new/edited studies --- app/models/study.rb | 4 ---- app/views/shared/metadata/edit/_study.html.erb | 3 --- .../support/step_definitions/study_steps.rb | 1 - spec/models/study_spec.rb | 17 ----------------- 4 files changed, 25 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index 0f50fdc6c0..dca73ee792 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -245,10 +245,6 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength custom_attribute(:ega_policy_accession_number) custom_attribute(:array_express_accession_number) - with_options(if: :delayed_for_long_time?, required: true) do - custom_attribute(:data_release_delay_approval, in: YES_OR_NO, default: NO) - end - with_options(if: :never_release?) do custom_attribute(:data_release_prevention_reason, in: DATA_RELEASE_PREVENTION_REASONS, required: true) custom_attribute(:data_release_prevention_reason_comment, required: true) diff --git a/app/views/shared/metadata/edit/_study.html.erb b/app/views/shared/metadata/edit/_study.html.erb index 70942e2638..d754aaddb4 100644 --- a/app/views/shared/metadata/edit/_study.html.erb +++ b/app/views/shared/metadata/edit/_study.html.erb @@ -74,9 +74,6 @@ %> <%= group.select(:data_release_delay_period, Study::DATA_RELEASE_DELAY_PERIODS) %> - <% metadata_fields.related_fields(to: :data_release_delay_period, in: Study::DATA_RELEASE_DELAY_PERIODS) do %> - <%= group.radio_select(:data_release_delay_approval, Study::YES_OR_NO) %> - <% end %> <% metadata_fields.related_fields(to: :data_release_delay_reason, when: Study::DATA_RELEASE_DELAY_FOR_OTHER) do %> <%= group.text_area(:data_release_delay_other_comment) %> <% metadata_fields.related_fields(to: :data_release_delay_period, in: Study::DATA_RELEASE_DELAY_PERIODS) do %> diff --git a/features/support/step_definitions/study_steps.rb b/features/support/step_definitions/study_steps.rb index e3bacbb72b..466e42627b 100644 --- a/features/support/step_definitions/study_steps.rb +++ b/features/support/step_definitions/study_steps.rb @@ -162,7 +162,6 @@ def given_study_metadata(attribute, regexp) data_release_delay_reason: 'other', data_release_delay_other_comment: reason, data_release_delay_period: "#{period} months", - data_release_delay_approval: 'Yes', data_release_delay_reason_comment: reason } ) diff --git a/spec/models/study_spec.rb b/spec/models/study_spec.rb index 3b146ee736..a3e27ab54e 100644 --- a/spec/models/study_spec.rb +++ b/spec/models/study_spec.rb @@ -739,23 +739,6 @@ end end - context 'delayed for long time' do - let(:study) do - create( - :study, - study_metadata: - create( - :study_metadata, - metadata.merge(data_release_timing: 'delayed', data_release_delay_period: '6 months') - ) - ) - end - - it 'will have a data_release_delay_approval' do - expect(study.study_metadata.data_release_delay_approval).to eq(metadata[:data_release_delay_approval]) - end - end - context 'never released' do let(:never_release_fields) do { From bac3e8b6744fd6d1a7e59b26449d8cbac1c34f84 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Mon, 16 Dec 2024 15:52:40 +0000 Subject: [PATCH 08/16] tests(study-metadata): removes test references to deprecated release delay approval study metadata --- ...5391_study_xml_needs_to_be_reverted_to_old_version.feature | 4 ---- features/studies/8447221_data_release_help_text.feature | 1 - 2 files changed, 5 deletions(-) diff --git a/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature b/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature index 59e640bd67..8057e24450 100644 --- a/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature +++ b/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature @@ -150,10 +150,6 @@ Feature: The XML for the sequencescape API Are all the samples to be used in this study commercially available, unlinked anonymised cell-lines? No - - Has the delay period been approved by the data sharing committee for this project? - - ENA Project ID diff --git a/features/studies/8447221_data_release_help_text.feature b/features/studies/8447221_data_release_help_text.feature index 0b20d4bab8..603b2c0b4c 100644 --- a/features/studies/8447221_data_release_help_text.feature +++ b/features/studies/8447221_data_release_help_text.feature @@ -43,7 +43,6 @@ Feature: Update the data release fields for creating a study When I select "delayed" from "How is the data release to be timed?" And I select "other" from "Reason for delaying release" And I select "" from "Delay for" - Then I should exactly see "Has the delay period been approved by the data sharing committee for this project?" And I should exactly see "Comment regarding data release timing and approval" When I fill in the following: From b5e5c4a41427248d3309c3dac5ae99c085dad4ff Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Tue, 17 Dec 2024 11:50:24 +0000 Subject: [PATCH 09/16] feat(studies): updates data release other option --- app/models/study.rb | 2 +- app/views/shared/metadata/_related_fields.html.erb | 2 +- features/studies/8447221_data_release_help_text.feature | 4 ++-- features/studies/data_release_timings.feature | 4 ++-- features/support/step_definitions/study_steps.rb | 2 +- spec/models/study_spec.rb | 5 ++++- 6 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index dca73ee792..92ea477ce2 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -91,7 +91,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength 'Other (please specify)' ].freeze - DATA_RELEASE_DELAY_FOR_OTHER = 'other' + DATA_RELEASE_DELAY_FOR_OTHER = 'Other (with free text box)' DATA_RELEASE_DELAY_REASONS_STANDARD = [ 'PhD study', 'Capacity building', diff --git a/app/views/shared/metadata/_related_fields.html.erb b/app/views/shared/metadata/_related_fields.html.erb index d748e8d430..9d59fb9bdc 100644 --- a/app/views/shared/metadata/_related_fields.html.erb +++ b/app/views/shared/metadata/_related_fields.html.erb @@ -23,7 +23,7 @@ var valueFrom = function(element) { if (element === null) { return null } - var value = element.value.toLowerCase().replace(/[^a-z0-9]+/, '_'); + var value = element.value.toLowerCase().replaceAll(/[^a-z0-9]+/g, '_'); return (value.length == 0) ? 'blank' : value; }; diff --git a/features/studies/8447221_data_release_help_text.feature b/features/studies/8447221_data_release_help_text.feature index 603b2c0b4c..dd79263d3e 100644 --- a/features/studies/8447221_data_release_help_text.feature +++ b/features/studies/8447221_data_release_help_text.feature @@ -22,7 +22,7 @@ Feature: Update the data release fields for creating a study Scenario Outline: Add help text opposite delay drop down (4044305) When I choose "" from "What is the data release strategy for this study?" When I select "delayed" from "How is the data release to be timed?" - When I select "other" from "Reason for delaying release" + When I select "Other (with free text box)" from "Reason for delaying release" Then I should exactly see "Reason for delaying release" Examples: @@ -41,7 +41,7 @@ Feature: Update the data release fields for creating a study Scenario Outline: Delaying for 3 months should have the same questions as all other delays (4044273) When I select "delayed" from "How is the data release to be timed?" - And I select "other" from "Reason for delaying release" + And I select "Other (with free text box)" from "Reason for delaying release" And I select "" from "Delay for" And I should exactly see "Comment regarding data release timing and approval" diff --git a/features/studies/data_release_timings.feature b/features/studies/data_release_timings.feature index 4c0c2f3961..cc6562c638 100644 --- a/features/studies/data_release_timings.feature +++ b/features/studies/data_release_timings.feature @@ -37,7 +37,7 @@ Feature: Studies have timings for release of their data Scenario Outline: When the data release is delayed but no reasons are provided Given I select "delayed" from "How is the data release to be timed?" - And I select "other" from "Reason for delaying release" + And I select "Other (with free text box)" from "Reason for delaying release" And I fill in "Please explain the reason for delaying release" with "Some reason" And I select "" from "Delay for" When I press "Create" @@ -54,7 +54,7 @@ Feature: Studies have timings for release of their data Scenario Outline: When the data release is delayed and the reasons are provided Given I select "delayed" from "How is the data release to be timed?" - And I select "other" from "Reason for delaying release" + And I select "Other (with free text box)" from "Reason for delaying release" And I fill in "Please explain the reason for delaying release" with "Some reason" And I select "" from "Delay for" And I fill in "Comment regarding data release timing and approval" with "Because it is ok?" diff --git a/features/support/step_definitions/study_steps.rb b/features/support/step_definitions/study_steps.rb index 466e42627b..86f220fab3 100644 --- a/features/support/step_definitions/study_steps.rb +++ b/features/support/step_definitions/study_steps.rb @@ -159,7 +159,7 @@ def given_study_metadata(attribute, regexp) study.update!( study_metadata_attributes: { data_release_timing: 'delayed', - data_release_delay_reason: 'other', + data_release_delay_reason: 'Other (with free text box)', data_release_delay_other_comment: reason, data_release_delay_period: "#{period} months", data_release_delay_reason_comment: reason diff --git a/spec/models/study_spec.rb b/spec/models/study_spec.rb index a3e27ab54e..696170a97f 100644 --- a/spec/models/study_spec.rb +++ b/spec/models/study_spec.rb @@ -713,7 +713,10 @@ create( :study, study_metadata: - create(:study_metadata, metadata.merge(data_release_timing: 'delayed', data_release_delay_reason: 'other')) + create( + :study_metadata, + metadata.merge(data_release_timing: 'delayed', data_release_delay_reason: 'Other (with free text box)') + ) ) end From fdc2e1ae33a337194727f9ceaa94ff5d6e819de4 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Wed, 8 Jan 2025 11:34:52 +0000 Subject: [PATCH 10/16] feat(study): updates study edit dropdown options to display old options if present --- app/models/study.rb | 39 +++++++++++++++++-- .../shared/metadata/edit/_study.html.erb | 10 ++--- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index 92ea477ce2..2fd7aadf7c 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -83,6 +83,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength DATA_RELEASE_TIMING_DELAYED ].freeze + OLD_DATA_RELEASE_PREVENTION_REASONS = ['data validity', 'legal', 'replication of data subset'].freeze DATA_RELEASE_PREVENTION_REASONS = [ 'Pilot or validation studies - DAC approval not required', 'Collaborators will share data in a research repository - DAC approval not required', @@ -91,6 +92,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength 'Other (please specify)' ].freeze + OLD_DATA_RELEASE_DELAY_REASONS = ['other', 'phd study'].freeze DATA_RELEASE_DELAY_FOR_OTHER = 'Other (with free text box)' DATA_RELEASE_DELAY_REASONS_STANDARD = [ 'PhD study', @@ -99,7 +101,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength 'Additional time to make data FAIR', DATA_RELEASE_DELAY_FOR_OTHER ].freeze - DATA_RELEASE_DELAY_REASONS_ASSAY = ['assay of no other use', *DATA_RELEASE_DELAY_REASONS_STANDARD].freeze + DATA_RELEASE_DELAY_REASONS_ASSAY = ['assay of no other use'].freeze DATA_RELEASE_DELAY_PERIODS = ['3 months', '6 months', '9 months', '12 months', '18 months'].freeze @@ -227,7 +229,7 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength custom_attribute( :data_release_delay_reason, required: true, - in: DATA_RELEASE_DELAY_REASONS_ASSAY, + in: [*DATA_RELEASE_DELAY_REASONS_STANDARD, *DATA_RELEASE_DELAY_REASONS_ASSAY, *OLD_DATA_RELEASE_DELAY_REASONS], if: :delayed_release? ) @@ -246,7 +248,11 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength custom_attribute(:array_express_accession_number) with_options(if: :never_release?) do - custom_attribute(:data_release_prevention_reason, in: DATA_RELEASE_PREVENTION_REASONS, required: true) + custom_attribute( + :data_release_prevention_reason, + in: [*DATA_RELEASE_PREVENTION_REASONS, *OLD_DATA_RELEASE_PREVENTION_REASONS], + required: true + ) custom_attribute(:data_release_prevention_reason_comment, required: true) custom_attribute(:data_release_prevention_approval) end @@ -577,6 +583,33 @@ def poly_metadatum_by_key(key) poly_metadata.find { |pm| pm.key == key.to_s } end + # Helper method for edit dropdowns to support backwards compatibility with old options. + # + # @return [Array] the list of options for the data release prevention reason dropdown + def data_release_prevention_options + additional_options = [] + if OLD_DATA_RELEASE_PREVENTION_REASONS.include? study_metadata.data_release_prevention_reason + additional_options << study_metadata.data_release_prevention_reason + end + + additional_options + DATA_RELEASE_PREVENTION_REASONS + end + + # Helper method for edit dropdowns to support backwards compatibility with old options. + # + # @param [Boolean] assay_option - whether to include assay-specific options + # @return [Array] the list of options for the data release delay reason dropdown + def data_release_delay_options(assay_option: false) + # If the current value is an old one, then we need to include it in the list of options + additional_options = [] + if OLD_DATA_RELEASE_DELAY_REASONS.include? study_metadata.data_release_delay_reason + additional_options << study_metadata.data_release_delay_reason + end + + additional_options << DATA_RELEASE_DELAY_REASONS_ASSAY if assay_option + additional_options + DATA_RELEASE_DELAY_REASONS_STANDARD + end + private def valid_ethically_approved? diff --git a/app/views/shared/metadata/edit/_study.html.erb b/app/views/shared/metadata/edit/_study.html.erb index d754aaddb4..3012fbb9e2 100644 --- a/app/views/shared/metadata/edit/_study.html.erb +++ b/app/views/shared/metadata/edit/_study.html.erb @@ -59,22 +59,22 @@ %> <% metadata_fields.related_fields(to: :data_release_strategy, when: Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE) do %> - <%= group.select(:data_release_prevention_reason, [''] + Study::DATA_RELEASE_PREVENTION_REASONS) %> + <%= group.select(:data_release_prevention_reason, study.data_release_prevention_options) %> <%= group.text_area(:data_release_prevention_approval) %> <%= group.text_area(:data_release_prevention_reason_comment) %> <% end %> <% metadata_fields.related_fields(to: :data_release_strategy, in: Study::DATA_RELEASE_STRATEGIES, not: Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE) do %> <% metadata_fields.related_fields(to: :data_release_timing, when: Study::DATA_RELEASE_TIMING_DELAYED) do %> - <%= group.select(:data_release_delay_reason, [''] + Study::DATA_RELEASE_DELAY_REASONS_STANDARD) %> + <%= group.select(:data_release_delay_reason, study.data_release_delay_options) %> <% 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), + DataReleaseStudyType.non_assay_types.map(&:id) => [ '' ] + study.data_release_delay_options }) %> <%= group.select(:data_release_delay_period, Study::DATA_RELEASE_DELAY_PERIODS) %> - <% metadata_fields.related_fields(to: :data_release_delay_reason, when: Study::DATA_RELEASE_DELAY_FOR_OTHER) do %> + <% metadata_fields.related_fields(to: :data_release_delay_reason, in: [Study::DATA_RELEASE_DELAY_FOR_OTHER, 'other']) do %> <%= group.text_area(:data_release_delay_other_comment) %> <% metadata_fields.related_fields(to: :data_release_delay_period, in: Study::DATA_RELEASE_DELAY_PERIODS) do %> <%= group.text_area(:data_release_delay_reason_comment) %> From c400f4e949594a23dc27c4624ddc177605dd14f9 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Wed, 8 Jan 2025 13:37:27 +0000 Subject: [PATCH 11/16] tests(study): adds tests for dropdown option generation functions --- app/models/study.rb | 10 ++++---- spec/models/study_spec.rb | 52 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index 2fd7aadf7c..5b6e30bda0 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -584,7 +584,7 @@ def poly_metadatum_by_key(key) end # Helper method for edit dropdowns to support backwards compatibility with old options. - # + # # @return [Array] the list of options for the data release prevention reason dropdown def data_release_prevention_options additional_options = [] @@ -592,7 +592,7 @@ def data_release_prevention_options additional_options << study_metadata.data_release_prevention_reason end - additional_options + DATA_RELEASE_PREVENTION_REASONS + DATA_RELEASE_PREVENTION_REASONS + additional_options end # Helper method for edit dropdowns to support backwards compatibility with old options. @@ -605,9 +605,9 @@ def data_release_delay_options(assay_option: false) if OLD_DATA_RELEASE_DELAY_REASONS.include? study_metadata.data_release_delay_reason additional_options << study_metadata.data_release_delay_reason end - - additional_options << DATA_RELEASE_DELAY_REASONS_ASSAY if assay_option - additional_options + DATA_RELEASE_DELAY_REASONS_STANDARD + + additional_options.concat(DATA_RELEASE_DELAY_REASONS_ASSAY) if assay_option + DATA_RELEASE_DELAY_REASONS_STANDARD + additional_options end private diff --git a/spec/models/study_spec.rb b/spec/models/study_spec.rb index 696170a97f..5f63d476fa 100644 --- a/spec/models/study_spec.rb +++ b/spec/models/study_spec.rb @@ -426,6 +426,58 @@ end end + describe '#data_release_prevention_options' do + subject { study.data_release_prevention_options } + + let(:study) { create(:study) } + + context 'when there is no existing data release prevention reason' do + it { is_expected.to eq Study::DATA_RELEASE_PREVENTION_REASONS } + end + + context 'when there is an existing data release prevention reason' do + before { study.study_metadata.data_release_prevention_reason = 'Protecting IP - DAC approval required' } + + it { is_expected.to eq Study::DATA_RELEASE_PREVENTION_REASONS } + end + + context 'when there is an existing legacy data release prevention reason' do + before { study.study_metadata.data_release_prevention_reason = 'data validity' } + + it { is_expected.to eq [*Study::DATA_RELEASE_PREVENTION_REASONS, 'data validity'] } + end + end + + describe '#data_release_delay_options' do + subject { study.data_release_delay_options } + + let(:study) { create(:study) } + + context 'when there is no existing data release delay reason' do + it { is_expected.to eq Study::DATA_RELEASE_DELAY_REASONS_STANDARD } + end + + context 'when there is an existing data release delay reason' do + before { study.study_metadata.data_release_delay_reason = 'Capacity building' } + + it { is_expected.to eq Study::DATA_RELEASE_DELAY_REASONS_STANDARD } + end + + context 'when there is an existing legacy data release delay reason' do + before { study.study_metadata.data_release_delay_reason = 'phd study' } + + it { is_expected.to eq [*Study::DATA_RELEASE_DELAY_REASONS_STANDARD, 'phd study'] } + end + + context 'when the data release delay options include assays' do + it 'includes the assay options' do + expect(study.data_release_delay_options(assay_option: true)).to eq( + [*Study::DATA_RELEASE_DELAY_REASONS_STANDARD, *Study::DATA_RELEASE_DELAY_REASONS_ASSAY] + ) + end + end + end + describe 'metadata' do let(:metadata) do { From 108220151cecc236a0e96c0ba4debf52988252ab Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Wed, 8 Jan 2025 13:48:42 +0000 Subject: [PATCH 12/16] fix(study): fixes old other option not firing mandatory validation --- app/models/study.rb | 5 +++-- app/views/shared/metadata/edit/_study.html.erb | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/study.rb b/app/models/study.rb index 5b6e30bda0..bc00c799da 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -92,8 +92,9 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength 'Other (please specify)' ].freeze - OLD_DATA_RELEASE_DELAY_REASONS = ['other', 'phd study'].freeze + OLD_DATA_RELEASE_DELAY_FOR_OTHER = 'other' DATA_RELEASE_DELAY_FOR_OTHER = 'Other (with free text box)' + OLD_DATA_RELEASE_DELAY_REASONS = ['other', 'phd study'].freeze DATA_RELEASE_DELAY_REASONS_STANDARD = [ 'PhD study', 'Capacity building', @@ -644,7 +645,7 @@ def never_release? end def delayed_for_other_reasons? - data_release_delay_reason == DATA_RELEASE_DELAY_FOR_OTHER + [DATA_RELEASE_DELAY_FOR_OTHER, OLD_DATA_RELEASE_DELAY_FOR_OTHER].include?(data_release_delay_reason) end def delayed_for_long_time? diff --git a/app/views/shared/metadata/edit/_study.html.erb b/app/views/shared/metadata/edit/_study.html.erb index 3012fbb9e2..b174425fd9 100644 --- a/app/views/shared/metadata/edit/_study.html.erb +++ b/app/views/shared/metadata/edit/_study.html.erb @@ -74,7 +74,7 @@ %> <%= group.select(:data_release_delay_period, Study::DATA_RELEASE_DELAY_PERIODS) %> - <% metadata_fields.related_fields(to: :data_release_delay_reason, in: [Study::DATA_RELEASE_DELAY_FOR_OTHER, 'other']) do %> + <% metadata_fields.related_fields(to: :data_release_delay_reason, in: [Study::DATA_RELEASE_DELAY_FOR_OTHER, Study::OLD_DATA_RELEASE_DELAY_FOR_OTHER]) do %> <%= group.text_area(:data_release_delay_other_comment) %> <% metadata_fields.related_fields(to: :data_release_delay_period, in: Study::DATA_RELEASE_DELAY_PERIODS) do %> <%= group.text_area(:data_release_delay_reason_comment) %> From ec2447376518bed8198e46075eefd1d784acfffe Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Tue, 14 Jan 2025 15:34:56 +0000 Subject: [PATCH 13/16] feat(study): adds data_release_prevention_other_comment to study metadata, removes redundant fields --- app/models/study.rb | 18 +++++-- .../shared/metadata/edit/_study.html.erb | 6 +-- .../shared/metadata/show/_study.html.erb | 1 + config/locales/metadata/en.yml | 4 +- ...vention_other_comment_to_study_metadata.rb | 6 +++ db/schema.rb | 3 +- .../8447221_data_release_help_text.feature | 6 +-- features/studies/data_release_timings.feature | 52 +++++-------------- .../support/step_definitions/study_steps.rb | 5 +- spec/models/study_spec.rb | 11 +++- 10 files changed, 56 insertions(+), 56 deletions(-) create mode 100644 db/migrate/20250114135342_add_data_release_prevention_other_comment_to_study_metadata.rb diff --git a/app/models/study.rb b/app/models/study.rb index bc00c799da..f893ad111b 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -84,16 +84,17 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength ].freeze OLD_DATA_RELEASE_PREVENTION_REASONS = ['data validity', 'legal', 'replication of data subset'].freeze + DATA_RELEASE_PREVENTION_REASON_OTHER = 'Other (please specify)' DATA_RELEASE_PREVENTION_REASONS = [ 'Pilot or validation studies - DAC approval not required', 'Collaborators will share data in a research repository - DAC approval not required', 'Prevent harm (e.g sensitive studies or biosecurity) - DAC approval required', 'Protecting IP - DAC approval required', - 'Other (please specify)' + DATA_RELEASE_PREVENTION_REASON_OTHER ].freeze OLD_DATA_RELEASE_DELAY_FOR_OTHER = 'other' - DATA_RELEASE_DELAY_FOR_OTHER = 'Other (with free text box)' + DATA_RELEASE_DELAY_FOR_OTHER = 'Other (please specify below)' OLD_DATA_RELEASE_DELAY_REASONS = ['other', 'phd study'].freeze DATA_RELEASE_DELAY_REASONS_STANDARD = [ 'PhD study', @@ -237,8 +238,8 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength custom_attribute(:data_release_delay_period, required: true, in: DATA_RELEASE_DELAY_PERIODS, if: :delayed_release?) custom_attribute(:bam, default: true) - with_options(required: true, if: :delayed_for_other_reasons?) do - custom_attribute(:data_release_delay_other_comment) + with_options(if: :delayed_for_other_reasons?) do + custom_attribute(:data_release_delay_other_comment, required: true) custom_attribute(:data_release_delay_reason_comment) end @@ -254,6 +255,11 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength in: [*DATA_RELEASE_PREVENTION_REASONS, *OLD_DATA_RELEASE_PREVENTION_REASONS], required: true ) + custom_attribute( + :data_release_prevention_other_comment, + required: true, + if: :data_release_prevention_reason_other? + ) custom_attribute(:data_release_prevention_reason_comment, required: true) custom_attribute(:data_release_prevention_approval) end @@ -648,6 +654,10 @@ def delayed_for_other_reasons? [DATA_RELEASE_DELAY_FOR_OTHER, OLD_DATA_RELEASE_DELAY_FOR_OTHER].include?(data_release_delay_reason) end + def data_release_prevention_reason_other? + data_release_prevention_reason == DATA_RELEASE_PREVENTION_REASON_OTHER + end + def delayed_for_long_time? DATA_RELEASE_DELAY_PERIODS.include?(data_release_delay_period) end diff --git a/app/views/shared/metadata/edit/_study.html.erb b/app/views/shared/metadata/edit/_study.html.erb index b174425fd9..8741a9f4aa 100644 --- a/app/views/shared/metadata/edit/_study.html.erb +++ b/app/views/shared/metadata/edit/_study.html.erb @@ -60,6 +60,9 @@ <% metadata_fields.related_fields(to: :data_release_strategy, when: Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE) do %> <%= group.select(:data_release_prevention_reason, study.data_release_prevention_options) %> + <% metadata_fields.related_fields(to: :data_release_prevention_reason, in: [Study::DATA_RELEASE_PREVENTION_REASON_OTHER]) do %> + <%= group.text_area(:data_release_prevention_other_comment) %> + <% end %> <%= group.text_area(:data_release_prevention_approval) %> <%= group.text_area(:data_release_prevention_reason_comment) %> <% end %> @@ -76,9 +79,6 @@ <%= group.select(:data_release_delay_period, Study::DATA_RELEASE_DELAY_PERIODS) %> <% metadata_fields.related_fields(to: :data_release_delay_reason, in: [Study::DATA_RELEASE_DELAY_FOR_OTHER, Study::OLD_DATA_RELEASE_DELAY_FOR_OTHER]) do %> <%= group.text_area(:data_release_delay_other_comment) %> - <% metadata_fields.related_fields(to: :data_release_delay_period, in: Study::DATA_RELEASE_DELAY_PERIODS) do %> - <%= group.text_area(:data_release_delay_reason_comment) %> - <% end %> <% end %> <% end %> <% end %> diff --git a/app/views/shared/metadata/show/_study.html.erb b/app/views/shared/metadata/show/_study.html.erb index d6ebe29a7c..1030ceeb5d 100644 --- a/app/views/shared/metadata/show/_study.html.erb +++ b/app/views/shared/metadata/show/_study.html.erb @@ -57,6 +57,7 @@ <%= group.plain_value(:data_release_prevention_reason) %> <%= group.plain_value(:data_release_prevention_approval) %> <%= group.plain_value(:data_release_prevention_reason_comment) %> + <%= group.plain_value(:data_release_prevention_other_comment) %> <% end %> <%= metadata_fields.plain_value(:dac_policy_title) %> <%= metadata_fields.plain_value(:dac_policy) %> diff --git a/config/locales/metadata/en.yml b/config/locales/metadata/en.yml index 6e0890bb9d..ae7b3a5084 100644 --- a/config/locales/metadata/en.yml +++ b/config/locales/metadata/en.yml @@ -441,7 +441,9 @@ en: data_release_prevention_approval: label: If reason for exemption requires DAC approval, what is the approval number? - help: "If this is for data validity reasons: approval from the sponsor is required
If this is for legal reasons: approval from the Data Sharing Committee is required (please contact sd4)
" + + data_release_prevention_other_comment: + label: Please explain the reason for preventing data release data_release_prevention_reason_comment: label: Comment regarding prevention of data release and approval diff --git a/db/migrate/20250114135342_add_data_release_prevention_other_comment_to_study_metadata.rb b/db/migrate/20250114135342_add_data_release_prevention_other_comment_to_study_metadata.rb new file mode 100644 index 0000000000..1caaa449b2 --- /dev/null +++ b/db/migrate/20250114135342_add_data_release_prevention_other_comment_to_study_metadata.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddDataReleasePreventionOtherCommentToStudyMetadata < ActiveRecord::Migration[7.0] + def change + add_column :study_metadata, :data_release_prevention_other_comment, :string, default: nil + end +end diff --git a/db/schema.rb b/db/schema.rb index 187e3c4e3e..0174acc85c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_12_11_143636) do +ActiveRecord::Schema[7.0].define(version: 2025_01_14_135342) do create_table "aliquot_indices", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.integer "aliquot_id", null: false t.integer "lane_id", null: false @@ -1548,6 +1548,7 @@ t.string "s3_email_list" t.string "data_deletion_period" t.string "contaminated_human_data_access_group" + t.string "data_release_prevention_other_comment" t.index ["faculty_sponsor_id"], name: "index_study_metadata_on_faculty_sponsor_id" t.index ["study_id"], name: "index_study_metadata_on_study_id" end diff --git a/features/studies/8447221_data_release_help_text.feature b/features/studies/8447221_data_release_help_text.feature index dd79263d3e..e5ab02f06a 100644 --- a/features/studies/8447221_data_release_help_text.feature +++ b/features/studies/8447221_data_release_help_text.feature @@ -22,7 +22,7 @@ Feature: Update the data release fields for creating a study Scenario Outline: Add help text opposite delay drop down (4044305) When I choose "" from "What is the data release strategy for this study?" When I select "delayed" from "How is the data release to be timed?" - When I select "Other (with free text box)" from "Reason for delaying release" + When I select "Other (please specify below)" from "Reason for delaying release" Then I should exactly see "Reason for delaying release" Examples: @@ -41,15 +41,13 @@ Feature: Update the data release fields for creating a study Scenario Outline: Delaying for 3 months should have the same questions as all other delays (4044273) When I select "delayed" from "How is the data release to be timed?" - And I select "Other (with free text box)" from "Reason for delaying release" + And I select "Other (please specify below)" from "Reason for delaying release" And I select "" from "Delay for" - And I should exactly see "Comment regarding data release timing and approval" When I fill in the following: | Study name | new study | | Study description | writing cukes | | Please explain the reason for delaying release | some comment | - | Comment regarding data release timing and approval | another comment | And I select "Jack Sponsor" from "Faculty Sponsor" And I choose "Yes" from "Do any of the samples in this study contain human DNA?" diff --git a/features/studies/data_release_timings.feature b/features/studies/data_release_timings.feature index cc6562c638..38458bf41a 100644 --- a/features/studies/data_release_timings.feature +++ b/features/studies/data_release_timings.feature @@ -29,57 +29,33 @@ Feature: Studies have timings for release of their data Scenario: When the data release is delayed for PhD study Given I select "delayed" from "How is the data release to be timed?" And I select "PhD study" from "Reason for delaying release" - Then the "Comment regarding data release timing and approval" field is hidden When I select "6 months" from "Delay for" And I press "Create" Then I should be on the study information page for "Testing data release strategies" And I should see "Your study has been created" - Scenario Outline: When the data release is delayed but no reasons are provided - Given I select "delayed" from "How is the data release to be timed?" - And I select "Other (with free text box)" from "Reason for delaying release" - And I fill in "Please explain the reason for delaying release" with "Some reason" - And I select "" from "Delay for" + Scenario: When the data release is never but the prevention other comment is not supplied + When I choose "Not Applicable" from "What is the data release strategy for this study?" + And I select "never" from "How is the data release to be timed?" + And I select "Other (please specify)" from "What is the reason for preventing data release?" + And I fill in "If reason for exemption requires DAC approval, what is the approval number?" with "12345" When I press "Create" Then I should be on the studies page - # Ideally this should be without the study metadata qualification - And I should see "Study metadata data release delay reason comment can't be blank" - - Examples: - | period | - | 3 months | - | 6 months | - | 9 months | - | 12 months | - - Scenario Outline: When the data release is delayed and the reasons are provided - Given I select "delayed" from "How is the data release to be timed?" - And I select "Other (with free text box)" from "Reason for delaying release" - And I fill in "Please explain the reason for delaying release" with "Some reason" - And I select "" from "Delay for" - And I fill in "Comment regarding data release timing and approval" with "Because it is ok?" - When I press "Create" - Then I should be on the study information page for "Testing data release strategies" - And I should see "Your study has been created" - - Examples: - | period | - | 3 months | - | 6 months | - | 9 months | - | 12 months | + # Again, ideally without study metadata + And I should see "Study metadata data release prevention other comment can't be blank" - Scenario: When the data release is never but the comment is not supplied + Scenario: When the data release is never and the prevention other comment is supplied When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" - And I select "Protecting IP - DAC approval required" from "What is the reason for preventing data release?" + And I select "Other (please specify)" from "What is the reason for preventing data release?" + And I fill in "Please explain the reason for preventing data release" with "Some reason" + And I fill in "Comment regarding prevention of data release and approval" with "Some reason" And I fill in "If reason for exemption requires DAC approval, what is the approval number?" with "12345" When I press "Create" - Then I should be on the studies page - # Again, ideally without study metadata - And I should see "Study metadata data release prevention reason comment can't be blank" + Then I should be on the study information page for "Testing data release strategies" + And I should see "Your study has been created" - Scenario: When the data release is never and the comment is supplied + Scenario: When the data release is never and the prevention comment is supplied When I choose "Not Applicable" from "What is the data release strategy for this study?" And I select "never" from "How is the data release to be timed?" And I select "Protecting IP - DAC approval required" from "What is the reason for preventing data release?" diff --git a/features/support/step_definitions/study_steps.rb b/features/support/step_definitions/study_steps.rb index 86f220fab3..05798b7dfb 100644 --- a/features/support/step_definitions/study_steps.rb +++ b/features/support/step_definitions/study_steps.rb @@ -159,10 +159,9 @@ def given_study_metadata(attribute, regexp) study.update!( study_metadata_attributes: { data_release_timing: 'delayed', - data_release_delay_reason: 'Other (with free text box)', + data_release_delay_reason: 'Other (please specify below)', data_release_delay_other_comment: reason, - data_release_delay_period: "#{period} months", - data_release_delay_reason_comment: reason + data_release_delay_period: "#{period} months" } ) end diff --git a/spec/models/study_spec.rb b/spec/models/study_spec.rb index 5f63d476fa..0b26b961e3 100644 --- a/spec/models/study_spec.rb +++ b/spec/models/study_spec.rb @@ -508,9 +508,10 @@ ega_policy_accession_number: 'POL123456', array_express_accession_number: 'ARR123456', data_release_delay_approval: 'Yes', - data_release_prevention_reason: 'Protecting IP - DAC approval required', + data_release_prevention_reason: 'Other (please specify)', data_release_prevention_approval: 'Yes', data_release_prevention_reason_comment: 'Data Release prevention reason comment', + data_release_prevention_other_comment: 'Data Release prevention other comment', data_access_group: 'something', snp_study_id: 123_456, snp_parent_study_id: 123_456, @@ -767,7 +768,7 @@ study_metadata: create( :study_metadata, - metadata.merge(data_release_timing: 'delayed', data_release_delay_reason: 'Other (with free text box)') + metadata.merge(data_release_timing: 'delayed', data_release_delay_reason: 'Other (please specify below)') ) ) end @@ -818,6 +819,12 @@ ) end + it 'will have a data_release_prevention_other_comment' do + expect(study.study_metadata.data_release_prevention_other_comment).to eq( + metadata[:data_release_prevention_other_comment] + ) + end + context 'when the data_release_timing validation is switched on' do before { Flipper.enable :y24_052_enable_data_release_timing_validation } From befe5d4fdf08b4e1fe242eb48e9a8300fa325686 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Tue, 14 Jan 2025 15:54:09 +0000 Subject: [PATCH 14/16] tests(cucumber): fixes test looking for removed text --- features/studies/8447221_data_release_help_text.feature | 9 --------- 1 file changed, 9 deletions(-) diff --git a/features/studies/8447221_data_release_help_text.feature b/features/studies/8447221_data_release_help_text.feature index e5ab02f06a..bfda315bda 100644 --- a/features/studies/8447221_data_release_help_text.feature +++ b/features/studies/8447221_data_release_help_text.feature @@ -30,15 +30,6 @@ Feature: Update the data release fields for creating a study | Managed (EGA) | | Open (ENA) | - Scenario: Add help text to has this been approved for never release (4044343) - When I choose "Not Applicable" from "What is the data release strategy for this study?" - When I select "never" from "How is the data release to be timed?" - Then the help text for "If reason for exemption requires DAC approval, what is the approval number?" should contain: - """ - If this is for data validity reasons: approval from the sponsor is required - If this is for legal reasons: approval from the Data Sharing Committee is required (please contact sd4) - """ - Scenario Outline: Delaying for 3 months should have the same questions as all other delays (4044273) When I select "delayed" from "How is the data release to be timed?" And I select "Other (please specify below)" from "Reason for delaying release" From 7b18b96bb2774649c35287257ea84e21f95408bf Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Tue, 14 Jan 2025 16:19:50 +0000 Subject: [PATCH 15/16] tests(cucumber): fixes study xml api response --- ...5391_study_xml_needs_to_be_reverted_to_old_version.feature | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature b/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature index 8057e24450..83ce342e94 100644 --- a/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature +++ b/features/studies/4295391_study_xml_needs_to_be_reverted_to_old_version.feature @@ -174,6 +174,10 @@ Feature: The XML for the sequencescape API Please explain the reason for delaying release + + Please explain the reason for preventing data release + + SNP parent study ID From b37c5a0c56898eaca0a454be8951270fa8282460 Mon Sep 17 00:00:00 2001 From: Ben Topping Date: Wed, 15 Jan 2025 10:02:11 +0000 Subject: [PATCH 16/16] misc(study): remove helper text for study description --- config/locales/metadata/en.yml | 1 - .../studies/8447221_data_release_help_text.feature | 13 ------------- 2 files changed, 14 deletions(-) diff --git a/config/locales/metadata/en.yml b/config/locales/metadata/en.yml index ae7b3a5084..0a3610d094 100644 --- a/config/locales/metadata/en.yml +++ b/config/locales/metadata/en.yml @@ -351,7 +351,6 @@ en: study_description: label: Study description edit_info: "ENA requirement" - help: Please choose one of the following 2 standard statements to be included with your data submissions (one or the other, depending on the study). If you use the second statement, replace [doi or ref] by a reference or doi for your publication:

This data is part of a pre-publication release. For information on the proper use of pre-publication data shared by the Wellcome Trust Sanger Institute (including details of any publication moratoria), please see http://www.sanger.ac.uk/datasharing/

OR

This data has been described in the following article [doi or ref] and its further analysis can be freely submitted for publication. For information on the proper use of data shared by the Wellcome Trust Sanger Institute (including information on acknowledgement), please see http://www.sanger.ac.uk/datasharing/

If applicable, include a brief description of any restrictions on data usage, e.g. 'For AIDS-related research only'

contaminated_human_dna: label: Does this study contain samples that are contaminated with human DNA which must be removed prior to analysis? diff --git a/features/studies/8447221_data_release_help_text.feature b/features/studies/8447221_data_release_help_text.feature index bfda315bda..673c5cd658 100644 --- a/features/studies/8447221_data_release_help_text.feature +++ b/features/studies/8447221_data_release_help_text.feature @@ -6,19 +6,6 @@ Feature: Update the data release fields for creating a study Given a faculty sponsor called "Jack Sponsor" exists And I am on the new study page - Scenario: Add help text to study description (8348119) - Then the help text for "Study description" should contain: - """ - Please choose one of the following 2 standard statements to be included with your data submissions (one or the other, depending on the study). If you use the second statement, replace [doi or ref] by a reference or doi for your publication: - - This data is part of a pre-publication release. For information on the proper use of pre-publication data shared by the Wellcome Trust Sanger Institute (including details of any publication moratoria), please see http://www.sanger.ac.uk/datasharing/ - - OR - - This data has been described in the following article [doi or ref] and its further analysis can be freely submitted for publication. For information on the proper use of data shared by the Wellcome Trust Sanger Institute (including information on acknowledgement), please see http://www.sanger.ac.uk/datasharing/ - If applicable, include a brief description of any restrictions on data usage, e.g. 'For AIDS-related research only' - """ - Scenario Outline: Add help text opposite delay drop down (4044305) When I choose "" from "What is the data release strategy for this study?" When I select "delayed" from "How is the data release to be timed?"