diff --git a/app/controllers/sample_manifest_upload_with_tag_sequences_controller.rb b/app/controllers/sample_manifest_upload_with_tag_sequences_controller.rb index b863ccc488..6b1d0be464 100644 --- a/app/controllers/sample_manifest_upload_with_tag_sequences_controller.rb +++ b/app/controllers/sample_manifest_upload_with_tag_sequences_controller.rb @@ -19,7 +19,10 @@ def create end def create_uploader - SampleManifest::Uploader.new(params[:upload], SampleManifestExcel.configuration, current_user, params[:override]) + samples = params[:override_samples].present? + + overrides = { samples:, exclude_fields: } + SampleManifest::Uploader.new(params[:upload], SampleManifestExcel.configuration, current_user, overrides) end def upload_manifest @@ -82,4 +85,11 @@ def prepare_manifest_pagination # rubocop:todo Metrics/MethodLength @display_manifests = pending_sample_manifests | completed_sample_manifests @sample_manifests = SampleManifest.paginate(page: params[:page]) end + + def exclude_fields + excluded_fields = [] + excluded_fields << :volume if params[:overwrite_volume].blank? + excluded_fields << :concentration if params[:overwrite_concentration].blank? + excluded_fields.compact + end end diff --git a/app/models/sample_manifest.rb b/app/models/sample_manifest.rb index 7b9525715a..17b29c231a 100644 --- a/app/models/sample_manifest.rb +++ b/app/models/sample_manifest.rb @@ -51,7 +51,7 @@ def self.included(base) has_uploaded_document :uploaded, differentiator: 'uploaded' has_uploaded_document :generated, differentiator: 'generated' - attr_accessor :override, :only_first_label, :barcode_type + attr_accessor :overrides, :only_first_label, :barcode_type attr_writer :rows_per_well, :invalid_wells class_attribute :spreadsheet_offset diff --git a/app/models/sample_manifest/uploader.rb b/app/models/sample_manifest/uploader.rb index 4a165aa5a9..6198e61de7 100644 --- a/app/models/sample_manifest/uploader.rb +++ b/app/models/sample_manifest/uploader.rb @@ -9,7 +9,7 @@ class SampleManifest::Uploader include ActiveModel::Validations - attr_reader :file, :configuration, :tag_group, :upload, :user, :override + attr_reader :file, :configuration, :tag_group, :upload, :user validates :tag_group, presence: { message: 'is not correctly configured for manifest generation' } validates :file, :configuration, :user, presence: true @@ -18,14 +18,12 @@ class SampleManifest::Uploader delegate :processed?, :study, to: :upload - def initialize(file, configuration, user, override) + def initialize(file, configuration, user, overrides) @file = file @configuration = configuration || SequencescapeExcel::NullObjects::NullConfiguration.new @user = user - @override = override @tag_group = create_tag_group - @upload = - SampleManifestExcel::Upload::Base.new(file: file, column_list: self.configuration.columns.all, override: override) + @upload = SampleManifestExcel::Upload::Base.new(file:, column_list:, overrides:) end def run! # rubocop:disable Metrics/MethodLength @@ -54,6 +52,10 @@ def run! # rubocop:disable Metrics/MethodLength private + def column_list + configuration.columns.all + end + def process_upload_and_callbacks return false unless upload.process(tag_group) diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/base.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/base.rb index 3035f244a6..9d58f41c79 100644 --- a/app/sample_manifest_excel/sample_manifest_excel/upload/base.rb +++ b/app/sample_manifest_excel/sample_manifest_excel/upload/base.rb @@ -19,7 +19,7 @@ class Base # rubocop:todo Metrics/ClassLength include AccessionHelper include ActiveModel::Model - attr_accessor :file, :column_list, :start_row, :override + attr_accessor :file, :column_list, :start_row, :overrides # rubocop:todo Layout/LineLength attr_reader :spreadsheet, :columns, :sanger_sample_id_column, :rows, :sample_manifest, :data, :processor, :cache # TODO: probably shouldn't add the cache here, do it another way @@ -47,7 +47,7 @@ def initialize(attributes = {}) # rubocop:todo Metrics/AbcSize @cache = Cache.new(self) @rows = Upload::Rows.new(data, columns, @cache) @sample_manifest = derive_sample_manifest - @override = override || false + @overrides = overrides || { samples: false, exclude_fields: [] } @processor = create_processor end diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/processor/base.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/processor/base.rb index e1e4156fa4..4119e7d51d 100644 --- a/app/sample_manifest_excel/sample_manifest_excel/upload/processor/base.rb +++ b/app/sample_manifest_excel/sample_manifest_excel/upload/processor/base.rb @@ -49,7 +49,7 @@ def samples_valid? all_valid = true upload.rows.each do |row| - unless row.validate_sample + unless row.validate_sample(upload.overrides) upload.errors.add(:base, row.errors.full_messages.join(', ').to_s) all_valid = false end @@ -60,7 +60,7 @@ def samples_valid? def update_samples_and_aliquots(tag_group) upload.rows.each do |row| - row.update_sample(tag_group, upload.override) + row.update_sample(tag_group, upload.overrides) substitutions.concat(row.aliquots.filter_map(&:substitution_hash)) if row.reuploaded? end update_downstream_aliquots unless no_substitutions? diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/processor/multiplexed_library_tube.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/processor/multiplexed_library_tube.rb index faff473a9c..8eb3025cd2 100644 --- a/app/sample_manifest_excel/sample_manifest_excel/upload/processor/multiplexed_library_tube.rb +++ b/app/sample_manifest_excel/sample_manifest_excel/upload/processor/multiplexed_library_tube.rb @@ -24,7 +24,7 @@ def run(tag_group) def update_samples_and_aliquots(tag_group) upload.rows.each do |row| - row.update_sample(tag_group, upload.override) + row.update_sample(tag_group, upload.overrides) row.transfer_aliquot # Requests are smart enough to only transfer once substitutions.concat(row.aliquots.filter_map(&:substitution_hash)) if row.reuploaded? end diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb index cd83d4218a..3cc1783e9c 100644 --- a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb +++ b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb @@ -88,17 +88,18 @@ def specialised_fields # - Updating all of the specialised fields in the aliquot # - Updating the sample metadata # - Saving the asset, metadata and sample - def update_sample(tag_group, override) + def update_sample(tag_group, overrides) return unless valid? @reuploaded = sample.updated_by_manifest - if skip_sample_update?(override) + if skip_sample_update?(overrides) @sample_skipped = true return end - @sample_updated = save_sample(tag_group) + exclude_fields = overrides[:exclude_fields] + @sample_updated = save_sample(tag_group, exclude_fields) end def changed? @@ -106,12 +107,22 @@ def changed? aliquots.any? { |a| a.previous_changes.present? } end - def update_specialised_fields(tag_group) - specialised_fields.each { |specialised_field| specialised_field.update(tag_group:) } + def update_specialised_fields(tag_group, exclude_fields) + specialised_fields.each do |specialised_field| + puts "Exclude specialised fields: #{exclude_fields.inspect}, Processing column: #{specialised_field.name.to_sym}" + next if exclude_fields.include?(specialised_field.name.to_sym) + + puts "Updating specialised field: #{specialised_field.name}" + + specialised_field.update(tag_group:) + end end - def update_metadata_fields + def update_metadata_fields(exclude_fields) columns.with_metadata_fields.each do |column| + puts "Exclude metadata fields: #{exclude_fields.inspect}, Processing column: #{column.name}" + next if exclude_fields.include?(column.name.to_sym) + value = at(column.number) column.update_metadata(metadata, value) if value.present? end @@ -172,9 +183,9 @@ def labware sample.primary_receptacle.labware end - def validate_sample + def validate_sample(overrides) check_sample_present - sample_can_be_updated + sample_can_be_updated(overrides) errors.empty? end @@ -247,12 +258,14 @@ def sanger_sample_id_exists? # # This confirms that the sample can be updated by attempting to apply the updates # and raising errors if any of the updates are invalid. It does not (seem to!) save any changes to the database. - def sample_can_be_updated + def sample_can_be_updated(overrides) return unless errors.empty? + exclude_fields = overrides[:exclude_fields] + check_primary_receptacle - check_specialised_fields - check_sample_metadata + check_specialised_fields(exclude_fields) + check_sample_metadata(exclude_fields) end def check_primary_receptacle @@ -261,19 +274,21 @@ def check_primary_receptacle errors.add(:base, "#{row_title} Does not have a primary receptacle.") end - def check_specialised_fields + def check_specialised_fields(exclude_fields) return unless errors.empty? specialised_fields.each do |specialised_field| + next if exclude_fields.include?(specialised_field.name.to_sym) + unless specialised_field.valid? errors.add(:base, "#{row_title} #{specialised_field.errors.full_messages.join(', ')}") end end end - def check_sample_metadata + def check_sample_metadata(exclude_fields) # it has to be called here, otherwise metadata errors will not appear - update_metadata_fields + update_metadata_fields(exclude_fields) return if metadata.valid? errors.add(:base, "#{row_title} #{metadata.errors.full_messages.join(', ')}") @@ -302,10 +317,10 @@ def link_tag_groups_and_indexes(fields) fields.each { |field| field.link(indexed_fields) } end - def save_sample(tag_group) - update_specialised_fields(tag_group) + def save_sample(tag_group, exclude_fields) + update_specialised_fields(tag_group, exclude_fields) asset.save! - update_metadata_fields + update_metadata_fields(exclude_fields) metadata.save! sample.updated_by_manifest = true sample.empty_supplier_sample_name = false diff --git a/app/views/sdb/sample_manifests/_upload.html.erb b/app/views/sdb/sample_manifests/_upload.html.erb index c3bfdcfba6..448c16d31f 100644 --- a/app/views/sdb/sample_manifests/_upload.html.erb +++ b/app/views/sdb/sample_manifests/_upload.html.erb @@ -6,7 +6,21 @@ <%= file_field_tag :upload, required: true %>
- <%= check_box_tag :override %> <%= label_tag :override, 'Override previously uploaded samples' %> +
+ <%= check_box_tag :override_samples, class: 'form-check-input'%> + <%= label_tag :override_samples, 'Override previously uploaded samples', class: 'form-check-label' %> +
+ +
<%# Indent the volume and concentration checkboxes, since they depend on override_samples %> +
+ <%= check_box_tag :overwrite_volume, class: 'form-check-input', disabled: true %> + <%= label_tag :overwrite_volume, 'Overwrite volume', class: 'form-check-label' %> +
+
+ <%= check_box_tag :overwrite_concentration, class: 'form-check-input', disabled: true %> + <%= label_tag :overwrite_concentration, 'Overwrite concentration', class: 'form-check-label' %> +
+
<%= submit_tag 'Upload manifest', class: 'btn btn-success' %> @@ -14,3 +28,21 @@ <% end -%> <% end %> + + diff --git a/app/views/shared/metadata/show/_sample.html.erb b/app/views/shared/metadata/show/_sample.html.erb index 17cb2a0196..8b7721a9b3 100644 --- a/app/views/shared/metadata/show/_sample.html.erb +++ b/app/views/shared/metadata/show/_sample.html.erb @@ -9,12 +9,15 @@ <%= metadata_fields.plain_value(:geographical_region) %> <%= metadata_fields.plain_value(:ethnicity) %> <%= metadata_fields.plain_value(:dna_source) %> - <%= metadata_fields.plain_value(:volume) %> <%= metadata_fields.plain_value(:mother) %> <%= metadata_fields.plain_value(:father) %> <%= metadata_fields.plain_value(:replicate) %> <%= metadata_fields.plain_value(:collected_by) %> + + <%= metadata_fields.plain_value(:volume) %> + <%= metadata_fields.plain_value(:concentration) %> + <%# Fields for 'Next-gen sequencing' %> <%= metadata_fields.plain_value(:organism) %> <%= metadata_fields.plain_value(:gc_content) %> diff --git a/spec/sample_manifest_excel/upload/processor_spec.rb b/spec/sample_manifest_excel/upload/processor_spec.rb index 8f8a97b21e..e50f13d24b 100644 --- a/spec/sample_manifest_excel/upload/processor_spec.rb +++ b/spec/sample_manifest_excel/upload/processor_spec.rb @@ -38,6 +38,7 @@ def cell(row, column) let(:new_test_file_name) { 'new_test_file.xlsx' } let(:test_file) { Rack::Test::UploadedFile.new(Rails.root.join(test_file_name), '') } let(:tag_group) { create(:tag_group) } + let(:overrides) { { samples:, exclude_fields: } } before do allow(PlateBarcode).to receive(:create_barcode).and_return(build(:plate_barcode)) @@ -47,7 +48,7 @@ def cell(row, column) after { FileUtils.rm_f(test_file_name) } shared_examples 'it updates downstream aliquots' do |rows, columns| - it 'will update the aliquots downstream if aliquots data has changed and override is set to true' do + it 'will update the aliquots downstream if aliquots data has changed and override is set to overall all' do cell(rows.first, columns[:insert_size_from]).value = '100' cell(rows.last, columns[:insert_size_to]).value = '1000' download.save(new_test_file_name) @@ -56,7 +57,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload2) processor.update_samples_and_aliquots(tag_group) @@ -65,7 +66,7 @@ def cell(row, column) expect(processor).to be_downstream_aliquots_updated end - it 'will update the aliquots downstream if tags were swapped and override is set to true' do + it 'will update the aliquots downstream if tags were swapped and override is set to overall all' do i7_tag1 = cell(rows.first, columns[:i7]).value i7_tag2 = cell(rows.last, columns[:i7]).value cell(rows.first, columns[:i7]).value = i7_tag2 @@ -76,7 +77,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload2) processor.update_samples_and_aliquots(tag_group) @@ -95,7 +96,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload2) processor.update_samples_and_aliquots(tag_group) @@ -110,7 +111,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload2) processor.update_samples_and_aliquots(tag_group) @@ -120,7 +121,7 @@ def cell(row, column) end shared_examples 'it updates chromium aliquots' do |rows, columns| - it 'will update the aliquots downstream if aliquots data has changed and override is set to true' do + it 'will update the aliquots downstream if aliquots data has changed and override is set to overall all' do cell(rows.first, columns[:insert_size_from]).value = '100' cell(rows.last, columns[:insert_size_to]).value = '1000' @@ -130,7 +131,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload2) processor.update_samples_and_aliquots(tag_group) @@ -140,7 +141,7 @@ def cell(row, column) expect(processor).to be_downstream_aliquots_updated end - it 'will update the aliquots downstream if tags were swapped and override is set to true' do + it 'will update the aliquots downstream if tags were swapped and override is set to overall all' do chromium_tag1 = cell(rows.first, columns[:chromium_tag_well]).value chromium_tag2 = cell(rows.last, columns[:chromium_tag_well]).value cell(rows.first, columns[:chromium_tag_well]).value = chromium_tag2 @@ -151,7 +152,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload2) processor.update_samples_and_aliquots(tag_group) @@ -174,7 +175,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload2) @@ -231,7 +232,7 @@ def cell(row, column) it_behaves_like 'it updates downstream aliquots', [10, 11], insert_size_from: 6, insert_size_to: 7, i7: 2 context 'when override is true' do - let(:override) { true } + let(:override_samples) { true } context 'when updating sample data' do let(:reupload) do @@ -262,7 +263,7 @@ def cell(row, column) end context 'when override is false' do - let(:override) { false } + let(:override_samples) { false } let(:reupload) do SampleManifestExcel::Upload::Base.new( file: new_test_file, @@ -511,7 +512,7 @@ def cell(row, column) after { FileUtils.rm_f(new_test_file_name) } - it 'will update the aliquots downstream if aliquots data has changed and override is set to true' do + it 'will update the aliquots downstream if aliquots data has changed and override is set to overall all' do cell(10, 7).value = '100' cell(11, 8).value = '1000' download.save(new_test_file_name) @@ -520,7 +521,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload) processor.update_samples_and_aliquots(nil) @@ -529,7 +530,7 @@ def cell(row, column) expect(processor).to be_downstream_aliquots_updated end - it 'will update the aliquots downstream if tag indexes were swapped and override is set to true' do + it 'will update the aliquots downstream if tag indexes were swapped and override is set to overall all' do tag_group1 = cell(10, 2).value tag_index1 = cell(10, 3).value tag_group2 = cell(11, 2).value @@ -544,7 +545,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload) processor.update_samples_and_aliquots(nil) @@ -558,7 +559,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload) processor.update_samples_and_aliquots(nil) @@ -693,7 +694,7 @@ def cell(row, column) file: new_test_file, column_list: column_list, start_row: 9, - override: true + override: overrides ) processor = described_class.new(reupload) processor.update_samples_and_aliquots(nil)