From 8aad520c85033424c0348d18b98bd3b3f16dcc6e Mon Sep 17 00:00:00 2001 From: Andrew Sparkes Date: Fri, 1 May 2026 14:39:48 +0100 Subject: [PATCH 1/3] removed aliquot update code from dual index tag well, added guard clauses to prevent NoMethod errors, and updated specialised fields to pass in row to enab --- app/models/aliquot/data_for_substitution.rb | 2 + .../upload/processor/base.rb | 3 +- .../sample_manifest_excel/upload/row.rb | 6 ++- .../specialised_field/base.rb | 2 +- .../specialised_field/dual_index_tag_well.rb | 45 ++++++------------- 5 files changed, 24 insertions(+), 34 deletions(-) diff --git a/app/models/aliquot/data_for_substitution.rb b/app/models/aliquot/data_for_substitution.rb index 90bb1b2bd4..ec545850a9 100644 --- a/app/models/aliquot/data_for_substitution.rb +++ b/app/models/aliquot/data_for_substitution.rb @@ -7,6 +7,8 @@ module Aliquot::DataForSubstitution def substitution_hash return if id_previously_changed? + # Take care with saved_changes? - the Aliquot object in memory must be the same one that you called save on, + # otherwise you can't detect the changes. generate_substitution_hash if saved_changes? 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..de5f8cac0e 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 @@ -107,7 +107,8 @@ def disable_match_expectation true end - # if manifest is reuploaded, only aliquots, that are in 'fake' library tubes will be updated + # Comment is re: a multiplexed library tubes scenario, but this method is used for plates too. + # if manifest is re-uploaded, only aliquots, that are in 'fake' library tubes will be updated # actual aliquots in multiplexed library tube and other aliquots downstream are updated by this method # library updates all aliquots in one go, doing it row by row is inefficient and may trigger tag clash def update_downstream_aliquots 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 f85c9c7643..79c9c5ce82 100644 --- a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb +++ b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb @@ -295,7 +295,11 @@ def create_specialised_fields specialised_fields = columns.with_specialised_fields.map do |column| - column.specialised_field.new(value: at(column.number), sample_manifest_asset: manifest_asset) + column.specialised_field.new( + value: at(column.number), + sample_manifest_asset: manifest_asset, + row: self + ) end specialised_fields.tap { |fields| link_tag_groups_and_indexes(fields) } diff --git a/app/sequencescape_excel/sequencescape_excel/specialised_field/base.rb b/app/sequencescape_excel/sequencescape_excel/specialised_field/base.rb index 343c87b474..45e4c9addb 100644 --- a/app/sequencescape_excel/sequencescape_excel/specialised_field/base.rb +++ b/app/sequencescape_excel/sequencescape_excel/specialised_field/base.rb @@ -15,7 +15,7 @@ module Base end end - attr_accessor :value, :sample_manifest_asset + attr_accessor :value, :sample_manifest_asset, :row delegate :present?, to: :value, prefix: true delegate :asset, :sample, :sample_manifest, to: :sample_manifest_asset diff --git a/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb b/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb index a79fe7be96..2b0b2187dc 100644 --- a/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb +++ b/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb @@ -26,30 +26,35 @@ class DualIndexTagWell def update(_attributes = {}) return unless valid? - # NB. asset here is a well, and a the well_has_single_aliquot? validation ensures there is only one aliquot - stock_aliquot = asset.aliquots.first - - # Update all downstream aliquots as well as current aliquot - matching_aliquots = identify_all_matching_aliquots(stock_aliquot) - update_all_relevant_aliquots(matching_aliquots, tag, tag2) + # NB. the asset here is a well, and the well_has_single_aliquot? validation ensures there is only one aliquot + # NB. we are fetching via row so this is the same aliquot instance so saved_changes? works to detect substitutions + row.aliquots.first.assign_attributes(tag:, tag2:) end def link(other_fields) self.sf_dual_index_tag_set = other_fields[SequencescapeExcel::SpecialisedField::DualIndexTagSet] end - # From the validation in DualIndexTagSet, we know this tag set is a valid dual index tag set + # From the validation in DualIndexTagSet, we will know if this tag set is a valid dual index tag set # with a visible tag group and visible tag2 group def dual_index_tag_set @dual_index_tag_set = TagSet.find(sf_dual_index_tag_set.tag_set_id) if sf_dual_index_tag_set&.tag_set_id end def tag_group_id - @tag_group_id ||= ::TagGroup.find_by(id: dual_index_tag_set.tag_group_id, visible: true).id + # defensive guard to avoid NoMethodError being thrown when dual_index_tag_set is nil, caught by validation + # in DualIndexTagSet + return unless dual_index_tag_set + + @tag_group_id ||= ::TagGroup.find_by(id: dual_index_tag_set.tag_group_id, visible: true)&.id end def tag2_group_id - @tag2_group_id ||= ::TagGroup.find_by(id: dual_index_tag_set.tag2_group_id, visible: true).id + # defensive guard to avoid NoMethodError being thrown when dual_index_tag_set is nil, caught by validation + # in DualIndexTagSet + return unless dual_index_tag_set + + @tag2_group_id ||= ::TagGroup.find_by(id: dual_index_tag_set.tag2_group_id, visible: true)&.id end private @@ -82,28 +87,6 @@ def well_has_single_aliquot? msg = "Expecting well #{asset.map.description} to have a single aliquot, but it has #{asset.aliquots.count}" errors.add(:base, msg) end - - # Find all aliquots that need updating - # Aliquots must have a matching sample_id, library_id, tag_id and tag2_id to the given stock_aliquot. - def identify_all_matching_aliquots(stock_aliquot) - attributes = { - sample_id: stock_aliquot.sample_id, - library_id: stock_aliquot.library_id, - tag_id: stock_aliquot.tag_id, - tag2_id: stock_aliquot.tag2_id - } - - Aliquot.where(attributes).ids - end - - # Update the tags in all the matching aliquots - # NB. if active record sees that nothing has changed it will update the record - def update_all_relevant_aliquots(matching_aliquots, i7_tag, i5_tag) - Aliquot.where(id: matching_aliquots).find_each do |aq| - aq.update(tag: i7_tag, tag2: i5_tag) - aq.save! - end - end end end end From 110d784e38e6dc72423bd12975e59db7ee35ca15 Mon Sep 17 00:00:00 2001 From: Andrew Sparkes Date: Fri, 1 May 2026 14:41:30 +0100 Subject: [PATCH 2/3] linted --- .../specialised_field/dual_index_tag_well.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb b/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb index 2b0b2187dc..820ded80e4 100644 --- a/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb +++ b/app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb @@ -27,7 +27,8 @@ def update(_attributes = {}) return unless valid? # NB. the asset here is a well, and the well_has_single_aliquot? validation ensures there is only one aliquot - # NB. we are fetching via row so this is the same aliquot instance so saved_changes? works to detect substitutions + # NB. we are fetching via row so this is the same aliquot instance so saved_changes? works to detect + # substitutions row.aliquots.first.assign_attributes(tag:, tag2:) end From d3256a3609b4ab3e779b085b0ccb6cc02a5fda8c Mon Sep 17 00:00:00 2001 From: Andrew Sparkes Date: Fri, 1 May 2026 16:35:11 +0100 Subject: [PATCH 3/3] fix unit tests --- .../sequencescape_excel/specialised_field_spec.rb | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/spec/sequencescape_excel/specialised_field_spec.rb b/spec/sequencescape_excel/specialised_field_spec.rb index e036cde4b1..8c0f2edc16 100644 --- a/spec/sequencescape_excel/specialised_field_spec.rb +++ b/spec/sequencescape_excel/specialised_field_spec.rb @@ -718,8 +718,9 @@ def self.name describe SequencescapeExcel::SpecialisedField::DualIndexTagWell do let(:sf_dual_index_tag_well) do - described_class.new(value: dual_index_tag_well, sample_manifest_asset: sample_manifest_asset) + described_class.new(value: dual_index_tag_well, sample_manifest_asset: sample_manifest_asset, row: row) end + let(:row) { instance_double(SampleManifestExcel::Upload::Row, aliquots: asset.aliquots) } let(:sf_dual_index_tag_set) do SequencescapeExcel::SpecialisedField::DualIndexTagSet.new( value: dual_index_tag_set.name, @@ -742,6 +743,7 @@ def self.name it 'will apply the two tags associated with the map_id' do sf_dual_index_tag_well.update(aliquot: aliquot, tag_group: nil) + row.aliquots.first.save! # well location 'A1' => map_id '1' expect(asset.reload.aliquots.first.tag.map_id).to eq 1 expect(asset.reload.aliquots.first.tag.tag_group).to eq tag_group1 @@ -763,6 +765,7 @@ def self.name it 'will apply the 2 tags associated with the updated map_id' do sf_dual_index_tag_well.update(aliquot: aliquot, tag_group: nil) + row.aliquots.first.save! # well location 'D1' => map_id '4' expect(asset.reload.aliquots.first.tag.map_id).to eq 4 expect(asset.reload.aliquots.first.tag2.map_id).to eq 4 @@ -799,19 +802,13 @@ def self.name downstream_aliquot2 end - it 'will apply the 2 tags associated with the updated map_id' do + it 'will apply the 2 tags associated with the updated map_id to the source aliquot' do sf_dual_index_tag_well.update(aliquot: aliquot, tag_group: nil) + row.aliquots.first.save! # well location 'D1' => map_id '4' expect(asset.reload.aliquots.first.tag.map_id).to eq 4 expect(asset.reload.aliquots.first.tag2.map_id).to eq 4 - - # check downstream aliquots updated too - expect(downstream_aliquot1.reload.tag.map_id).to eq 4 - expect(downstream_aliquot1.reload.tag2.map_id).to eq 4 - - expect(downstream_aliquot2.reload.tag.map_id).to eq 4 - expect(downstream_aliquot2.reload.tag2.map_id).to eq 4 end end