diff --git a/Gemfile.lock b/Gemfile.lock index eace765b67..27cf5c9182 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -236,7 +236,7 @@ GEM factory_bot_rails (6.5.1) factory_bot (~> 6.5) railties (>= 6.1.0) - faraday (2.14.0) + faraday (2.14.1) faraday-net_http (>= 2.0, < 3.5) json logger @@ -338,8 +338,8 @@ GEM mutex_m (0.3.0) mysql2 (0.5.7) bigdecimal - net-http (0.7.0) - uri + net-http (0.9.1) + uri (>= 0.11.1) net-imap (0.5.9) date net-protocol diff --git a/app/controllers/studies_controller.rb b/app/controllers/studies_controller.rb index 7598cc2899..8fbb039d2d 100644 --- a/app/controllers/studies_controller.rb +++ b/app/controllers/studies_controller.rb @@ -253,6 +253,16 @@ def accession end end + # Accession all samples in the study. + # + # If the study does not have an accession number, adds an error to the study and returns. + # Otherwise, iterates through each sample in the study and attempts to accession it, + # unless the sample already has an accession number. + # If an Accession::Error occurs for a sample, adds the error message to the study's errors. + # + # NOTE: this does not check if the current user has permission to accession samples in this study + # + # @return [void] def accession_all_samples # rubocop:disable Metrics/AbcSize,Metrics/MethodLength @study = Study.find(params[:id]) return accessioning_not_enabled_redirect unless accessioning_enabled? @@ -260,11 +270,27 @@ def accession_all_samples # rubocop:disable Metrics/AbcSize,Metrics/MethodLength # TODO: Y26-026 - Enforce accessioning permissions # return accession_permission_denied_redirect unless permitted_to_accession?(@study) - @study.accession_all_samples(current_user) + unless @study.accession_number? + flash[:error] = 'Please accession the study before accessioning samples' + return redirect_to(study_path(@study)) + end + unless @study.samples_accessionable? + flash[:error] = 'Study cannot accession samples, see Study Accessioning tab for details' + return redirect_to(study_path(@study)) + end + + @study.samples.find_each do |sample| + next if sample.accession_number? + + begin + Accession.accession_sample(sample, current_user) + rescue Accession::Error => e + @study.errors.add(:base, e.message) + end + end if @study.errors.any? - error_messages = compile_accession_errors(@study.errors) - flash[:error] = error_messages + flash[:error] = compile_accession_errors(@study.errors) else flash[:notice] = 'All of the samples in this study have been sent for accessioning. ' \ 'Please check back in 5 minutes to confirm that accessioning was successful.' diff --git a/app/helpers/samples_helper.rb b/app/helpers/samples_helper.rb index 03f3753c72..6413215b32 100644 --- a/app/helpers/samples_helper.rb +++ b/app/helpers/samples_helper.rb @@ -4,7 +4,9 @@ module SamplesHelper # Indicate to the user that saving the sample will also accession it # This will not happen if the study has not been accessioned def save_text(sample) - return 'Save and Accession' if sample.should_be_accessioned? + if [accessioning_enabled?, sample.should_be_accessioned?, permitted_to_accession?(sample)].all? + return 'Save and Accession' + end 'Save Sample' end diff --git a/app/models/purpose.rb b/app/models/purpose.rb index 56fd0c76b5..7359357854 100644 --- a/app/models/purpose.rb +++ b/app/models/purpose.rb @@ -44,7 +44,7 @@ class Purpose < ApplicationRecord before_validation :set_default_barcode_prefix - validates :name, format: { with: /\A\w[\s\w.-]+\w\z/i }, presence: true, uniqueness: { case_sensitive: false } + validates :name, format: { with: %r{\A\w[\s\w.\-/]+\w\z}i }, presence: true, uniqueness: { case_sensitive: false } # NOTE: We should validate against valid asset subclasses, but running into some issues with # subclass loading while seeding. diff --git a/app/models/sample.rb b/app/models/sample.rb index b4cfbf7937..de8a013572 100644 --- a/app/models/sample.rb +++ b/app/models/sample.rb @@ -553,7 +553,8 @@ def should_be_accessioned? false end - # NOTE: this does not check whether the current user is permitted to accession the sample + # NOTE: this does not check whether the current user is permitted to accession the sample, + # nor if accessioning is enabled, as these belong in a controller or library, rather than the model. def accession_and_handle_validation_errors event_user = current_user # the event_user for this sample must be set from the calling controller Accession.accession_sample(self, event_user, perform_now: true) diff --git a/app/models/study.rb b/app/models/study.rb index 84bf3ac864..ca504dac2b 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -583,31 +583,6 @@ def samples_accessionable? ].all? end - # Accession all samples in the study. - # - # If the study does not have an accession number, adds an error to the study and returns. - # Otherwise, iterates through each sample in the study and attempts to accession it, - # unless the sample already has an accession number. - # If an Accession::Error occurs for a sample, adds the error message to the study's errors. - # - # NOTE: this does not check if the current user has permission to accession samples in this study - # - # @return [void] - def accession_all_samples(event_user) - return errors.add(:base, 'Please accession the study before accessioning samples') unless accession_number? - - unless samples_accessionable? - return errors.add(:base, - 'Study cannot accession samples, see Study Accessioning tab for details') - end - - samples.find_each do |sample| - Accession.accession_sample(sample, event_user) unless sample.accession_number? - rescue Accession::Error => e - errors.add(:base, e.message) - end - end - def abbreviation abbreviation = study_metadata.study_name_abbreviation abbreviation.presence || "#{id}STDY" diff --git a/app/models/transfer_request.rb b/app/models/transfer_request.rb index 624330b9c2..37bf570f7a 100644 --- a/app/models/transfer_request.rb +++ b/app/models/transfer_request.rb @@ -71,6 +71,7 @@ class TransferRequest < ApplicationRecord # rubocop:todo Metrics/ClassLength # as being more concise as it has fewer states. state :pending, initial: true state :started + state :processed state :processed_1 state :processed_2 state :processed_3 @@ -85,6 +86,10 @@ class TransferRequest < ApplicationRecord # rubocop:todo Metrics/ClassLength transitions to: :started, from: [:pending], after: :on_started end + event :progress do + transitions to: :processed, from: [:started], after: :on_started + end + event :process_1 do transitions to: :processed_1, from: [:pending], after: :on_started end @@ -104,7 +109,7 @@ class TransferRequest < ApplicationRecord # rubocop:todo Metrics/ClassLength event :pass do # Jumping straight to passed moves through an implied started state. transitions to: :passed, from: :pending, after: :on_started - transitions to: :passed, from: %i[started failed processed_2 processed_3 processed_4] + transitions to: :passed, from: %i[started failed processed processed_2 processed_3 processed_4] end event :fail do diff --git a/app/views/sdb/sample_manifests/_samples.html.erb b/app/views/sdb/sample_manifests/_samples.html.erb index 535e9d9b11..5dc50d480c 100644 --- a/app/views/sdb/sample_manifests/_samples.html.erb +++ b/app/views/sdb/sample_manifests/_samples.html.erb @@ -38,7 +38,7 @@ <%= accession_status&.status&.capitalize %> <%= accession_status&.updated_at&.in_time_zone&.strftime('%Y-%m-%d %H:%M:%S %Z') %> - <% if can?(:accession, sample) && configatron.accession_samples %> + <% if [accessioning_enabled?, sample.should_be_accessioned?, permitted_to_accession?(sample)].all? %> <%# Add link to perform sample accessioning %> <% if sample.current_accession_status %> <%# If status exists, offer retry %> diff --git a/app/views/studies/information/_accession_statuses.html.erb b/app/views/studies/information/_accession_statuses.html.erb index 33852b716e..cccbf5536e 100644 --- a/app/views/studies/information/_accession_statuses.html.erb +++ b/app/views/studies/information/_accession_statuses.html.erb @@ -36,7 +36,7 @@ <%= accession_status&.status&.capitalize %> <%= accession_status&.updated_at&.in_time_zone&.strftime('%Y-%m-%d %H:%M:%S %Z') %> - <% if permitted_to_accession?(sample) & sample.should_be_accessioned? & accessioning_enabled? %> + <% if [accessioning_enabled?, sample.should_be_accessioned?, permitted_to_accession?(sample)].all? %> <%# Add link to perform sample accessioning %> <% if sample.current_accession_status %> <%# If status exists, offer retry %> diff --git a/app/views/studies/information/_study_accession_checklist.html.erb b/app/views/studies/information/_study_accession_checklist.html.erb index c5ece5c113..3425e42975 100644 --- a/app/views/studies/information/_study_accession_checklist.html.erb +++ b/app/views/studies/information/_study_accession_checklist.html.erb @@ -25,7 +25,7 @@ <%= checklist_item( condition: !@study.study_metadata.strategy_not_applicable?, - good: "Study release strategy is #{Study::DATA_RELEASE_STRATEGY_OPEN.humanize} or #{Study::DATA_RELEASE_STRATEGY_MANAGED.humanize} #{tag.strong { "(#{@study.study_metadata.data_release_strategy.humanize})" }}".html_safe, + good: "Study release strategy is #{Study::DATA_RELEASE_STRATEGY_OPEN.humanize} or #{Study::DATA_RELEASE_STRATEGY_MANAGED.humanize} #{tag.strong { "(#{@study.study_metadata.data_release_strategy&.humanize})" }}".html_safe, bad: "Study release strategy is #{Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE.humanize}".html_safe, action: link_to('Change release strategy', edit_study_path(@study)), action_permission: can?(:edit, @study), @@ -34,7 +34,7 @@ <%= checklist_item( condition: !@study.study_metadata.never_release?, - good: "Study release timing is #{@study.study_metadata.data_release_timing.humanize}".html_safe, + good: "Study release timing is #{@study.study_metadata.data_release_timing&.humanize}".html_safe, bad: "Study release timing is #{Study::DATA_RELEASE_TIMING_NEVER.humanize}".html_safe, action: link_to('Change release settings', edit_study_path(@study)), action_permission: can?(:edit, @study), diff --git a/config/default_records/request_types/018_limber_lcm_triomics_request_types.yml b/config/default_records/request_types/018_limber_lcm_triomics_request_types.yml index cdc3f13ce4..e2ff4ee696 100644 --- a/config/default_records/request_types/018_limber_lcm_triomics_request_types.yml +++ b/config/default_records/request_types/018_limber_lcm_triomics_request_types.yml @@ -24,3 +24,15 @@ limber_lcm_triomics_wgs: - LCMT DNA Adp Lig library_types: - LCMB +limber_lcm_triomics_rnaseq: + name: LCM Triomics RNA-seq + asset_type: Well + order: 1 + request_class_name: IlluminaHtp::Requests::StdLibraryRequest + for_multiplexing: false + billable: true + product_line_name: Short Read + acceptable_purposes: + - LCMT Lysate + library_types: + - Combined LCM RNA diff --git a/config/default_records/submission_templates/014_lcm_triomics_submission_templates.yml b/config/default_records/submission_templates/014_lcm_triomics_submission_templates.yml index 7a55ace723..c4d1933df9 100644 --- a/config/default_records/submission_templates/014_lcm_triomics_submission_templates.yml +++ b/config/default_records/submission_templates/014_lcm_triomics_submission_templates.yml @@ -14,3 +14,10 @@ Limber-Htp - LCM Triomics WGS: request_type_keys: ["limber_lcm_triomics_wgs"] product_line_name: LCM Triomics product_catalogue_name: LCM Triomics + +Limber-Htp - LCM Triomics RNA-seq: + submission_class_name: "LinearSubmission" + related_records: + request_type_keys: ["limber_lcm_triomics_rnaseq"] + product_line_name: LCM Triomics + product_catalogue_name: LCM Triomics diff --git a/spec/controllers/studies_controller_spec.rb b/spec/controllers/studies_controller_spec.rb index ba3f93b587..6ac4bd4257 100644 --- a/spec/controllers/studies_controller_spec.rb +++ b/spec/controllers/studies_controller_spec.rb @@ -192,6 +192,48 @@ end context 'when the accessioning succeeds' do + it 'accessions all samples in the study' do + study.samples.each do |sample| + expect(sample.reload.sample_metadata.sample_ebi_accession_number).to eq('EGA00001000240') + end + end + + it 'redirects to the accession-statuses tab of the study page' do + expect(subject).to redirect_to(study_path(study, anchor: 'accession-statuses')) + end + + it 'does not set a flash error message' do + expect(flash[:error]).to be_nil + end + + it 'does not set a flash warning message' do + expect(flash[:warning]).to be_nil + end + + it 'sets a flash notice message' do + expect(flash[:notice]).to eq( + 'All of the samples in this study have been sent for accessioning. ' \ + 'Please check back in 5 minutes to confirm that accessioning was successful.' + ) + end + + it 'does not set a flash info message' do + expect(flash[:info]).to be_nil + end + end + + context 'when a sample already has an accession number' do + # add a 6th already accessioned sample to the study + let(:samples) { create_list(:sample_for_accessioning, number_of_samples) + create_list(:accessioned_sample, 1) } + let(:study) { create(:open_study, accession_number: 'ENA123', samples: samples) } + + it 'does not attempt to accession accessioned samples' do + # confirm that only 5 calls were made to the accession client, not 6 + expect(Accession::Submission.client) + .to have_received(:submit_and_fetch_accession_number) + .exactly(number_of_samples).times + end + it 'redirects to the accession-statuses tab of the study page' do expect(subject).to redirect_to(study_path(study, anchor: 'accession-statuses')) end @@ -308,6 +350,63 @@ end end end + + context 'when the study does not have an accession number' do + let(:study) { create(:managed_study, samples:) } + + it 'does not attempt to accession samples' do + expect(Accession::Submission.client).not_to have_received(:submit_and_fetch_accession_number) + end + + it 'redirects to the study page' do + expect(subject).to redirect_to(study_path(study)) + end + + it 'does not set a flash warning message' do + expect(flash[:warning]).to be_nil + end + + it 'does not set a flash notice message' do + expect(flash[:notice]).to be_nil + end + + it 'sets a flash error message' do + expect(flash[:error]).to eq('Please accession the study before accessioning samples') + end + + it 'does not set a flash info message' do + expect(flash[:info]).to be_nil + end + end + + context 'when a study is not longer accessionable' do + let(:study_metadata) { create(:study_metadata_for_accessioning, study_ebi_accession_number: 'EGA123') } + let(:study) { create(:study, study_metadata:, samples:) } + + it 'does not attempt to accession samples' do + expect(Accession::Submission.client).not_to have_received(:submit_and_fetch_accession_number) + end + + it 'redirects to the study page' do + expect(subject).to redirect_to(study_path(study)) + end + + it 'does not set a flash warning message' do + expect(flash[:warning]).to be_nil + end + + it 'does not set a flash notice message' do + expect(flash[:notice]).to be_nil + end + + it 'sets a flash error message' do + expect(flash[:error]).to eq('Study cannot accession samples, see Study Accessioning tab for details') + end + + it 'does not set a flash info message' do + expect(flash[:info]).to be_nil + end + end end end end diff --git a/spec/helpers/samples_helper_spec.rb b/spec/helpers/samples_helper_spec.rb new file mode 100644 index 0000000000..881e25b8df --- /dev/null +++ b/spec/helpers/samples_helper_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe SamplesHelper, type: :helper do + subject { helper.save_text(sample) } + + let(:sample) { create(:sample) } + + let(:accessioning_enabled) { true } + let(:should_be_accessioned) { true } + let(:permitted) { true } + + before do + allow(helper).to receive(:accessioning_enabled?).and_return(accessioning_enabled) + allow(helper).to receive(:permitted_to_accession?).with(sample).and_return(permitted) + allow(sample).to receive(:should_be_accessioned?).and_return(should_be_accessioned) + end + + context 'when accessioning is enabled, sample should be accessioned, and user is permitted' do + it { is_expected.to eq('Save and Accession') } + end + + context 'when accessioning is disabled' do + let(:accessioning_enabled) { false } + + it { is_expected.to eq('Save Sample') } + end + + context 'when sample should not be accessioned' do + let(:should_be_accessioned) { false } + + it { is_expected.to eq('Save Sample') } + end + + context 'when user is not permitted to accession' do + let(:permitted) { false } + + it { is_expected.to eq('Save Sample') } + end +end diff --git a/spec/lib/accession/study_spec.rb b/spec/lib/accession/study_spec.rb deleted file mode 100644 index 9c10e3e4cf..0000000000 --- a/spec/lib/accession/study_spec.rb +++ /dev/null @@ -1,129 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -MISSING_METADATA = { - managed_study: %w[sample-taxon-id sample-common-name gender phenotype donor-id].sort.to_sentence, - open_study: %w[sample-taxon-id sample-common-name].sort.to_sentence -}.freeze -STUDY_TYPES = %i[open_study managed_study].freeze - -RSpec.describe Study, :accession, :accessioning_enabled, :un_delay_jobs, type: :model do - include AccessionV1ClientHelper - - let(:current_user) { create(:user) } - let(:accession_number) { 'SAMPLE123456' } - let(:accessionable_samples) { create_list(:sample_for_accessioning, 5) } - let(:non_accessionable_samples) { create_list(:sample, 3) } - - before do - create(:user, api_key: configatron.accession_local_key) - allow(Accession::Submission).to receive(:client).and_return( - stub_accession_client(:submit_and_fetch_accession_number, return_value: accession_number) - ) - end - - after do - SampleManifestExcel.reset! - end - - STUDY_TYPES.each do |study_type| - context "in a #{study_type}" do - let(:missing_metadata_for_study) { MISSING_METADATA[study_type] } - - context 'when all samples in a study are accessionable' do - let(:study) { create(study_type, accession_number: 'ENA123', samples: accessionable_samples) } - - before do - study.accession_all_samples(current_user) - study.reload - end - - it 'accessions only the samples with accession numbers' do - expect(study.samples.count { |sample| sample.sample_metadata.sample_ebi_accession_number.present? }).to eq( - accessionable_samples.count - ) - end - end - - context 'with studies missing accession numbers' do - let(:study) { create(study_type, samples: create_list(:sample_for_accessioning, 5)) } - - before do - # Verify expectation before running the method - expect(Accession).not_to receive(:accession_sample).with(study.samples.first, anything) - study.accession_all_samples(current_user) - study.reload - end - - it 'does not accession any samples' do - expect(study.samples).to be_all { |sample| sample.sample_metadata.sample_ebi_accession_number.nil? } - end - end - - context 'when some samples in a study are not accessionable' do - let(:study) do - create(study_type, accession_number: 'ENA123', samples: accessionable_samples + non_accessionable_samples) - end - - before do - study.accession_all_samples(current_user) - study.reload - end - - it 'adds errors to the sample model' do - expect(study.errors.full_messages).to eq( - [ - "Sample 'Sample6' cannot be accessioned: " \ - "Sample does not have the required metadata: #{missing_metadata_for_study}.", - "Sample 'Sample7' cannot be accessioned: " \ - "Sample does not have the required metadata: #{missing_metadata_for_study}.", - "Sample 'Sample8' cannot be accessioned: " \ - "Sample does not have the required metadata: #{missing_metadata_for_study}." - ] - ) - end - - it 'accessions only the samples with accession numbers' do - expect(study.samples.count { |sample| sample.sample_metadata.sample_ebi_accession_number.present? }).to eq( - accessionable_samples.count - ) - end - - it 'does not accession samples without accession numbers' do - expect(study.samples.count { |sample| sample.sample_metadata.sample_ebi_accession_number.nil? }).to eq( - non_accessionable_samples.count - ) - end - end - - context 'when none of the samples in a study are accessionable' do - let(:study) { create(study_type, accession_number: 'ENA123', samples: non_accessionable_samples) } - - before do - study.accession_all_samples(current_user) - study.reload - end - - it 'adds errors to the sample model' do - expect(study.errors.full_messages).to eq( - [ - "Sample 'Sample1' cannot be accessioned: " \ - "Sample does not have the required metadata: #{missing_metadata_for_study}.", - "Sample 'Sample2' cannot be accessioned: " \ - "Sample does not have the required metadata: #{missing_metadata_for_study}.", - "Sample 'Sample3' cannot be accessioned: " \ - "Sample does not have the required metadata: #{missing_metadata_for_study}." - ] - ) - end - - it 'does not accession samples without accession numbers' do - expect(study.samples.count { |sample| sample.sample_metadata.sample_ebi_accession_number.nil? }).to eq( - non_accessionable_samples.count - ) - end - end - end - end -end