Y26-125 dual index tags update in mlwh

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
 

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

app/sequencescape_excel/sequencescape_excel/specialised_field/dual_index_tag_well.rb

@@ -26,30 +26,36 @@ 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. however we do not use `aliquots.first` as this triggers a select and gets a new instance of the aliquot,
+        # which then is not same aliquot instance when saved_changes? works to detect substitutions for TagSubstitution
+        aliquots.each { |aliquot| aliquot.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
+        # added because this code was running before the validation in DualIndexTagSet catches the missing value
+        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
+        # added because this code was running before the validation in DualIndexTagSet catches the missing value
+        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
@@ -77,33 +83,11 @@ def tag2
 
       # Validation to ensure that the well has a single aliquot
       def well_has_single_aliquot?
-        return true if asset.aliquots.one?
+        return true if aliquots.one?
 
-        msg = "Expecting well #{asset.map.description} to have a single aliquot, but it has #{asset.aliquots.count}"
+        msg = "Expecting well #{asset.map.description} to have a single aliquot, but it has #{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

spec/sequencescape_excel/specialised_field_spec.rb

@@ -741,17 +741,23 @@ def self.name
             end
 
             it 'will apply the two tags associated with the map_id' do
-              sf_dual_index_tag_well.update(aliquot: aliquot, tag_group: nil)
+              sf_dual_index_tag_well.update(aliquot:)
               # 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
-              expect(asset.reload.aliquots.first.tag2.map_id).to eq 1
-              expect(asset.reload.aliquots.first.tag2.tag_group).to eq tag_group2
+              expect(asset.aliquots).to all(
+                have_attributes(
+                  tag: have_attributes(
+                    map_id: 1, tag_group: tag_group1
+                  ),
+                  tag2: have_attributes(
+                    map_id: 1, tag_group: tag_group2
+                  )
+                )
+              )
 
               tag_set =
                 TagSet.find_by(
-                  tag_group_id: asset.reload.aliquots.first.tag.tag_group.id,
-                  tag2_group_id: asset.reload.aliquots.first.tag2.tag_group.id
+                  tag_group_id: sf_dual_index_tag_well.aliquots.first.tag.tag_group.id,
+                  tag2_group_id: sf_dual_index_tag_well.aliquots.first.tag2.tag_group.id
                 )
               expect(tag_set).to eq dual_index_tag_set
             end
@@ -762,56 +768,13 @@ def self.name
             let(:dual_index_tag_well) { 'd1' }
 
             it 'will apply the 2 tags associated with the updated map_id' do
-              sf_dual_index_tag_well.update(aliquot: aliquot, tag_group: nil)
+              sf_dual_index_tag_well.update(aliquot:)
               # 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
-            end
-          end
-
-          context 'when a tag change is applied in a re-upload that already has downstream labware' do
-            let(:asset) { create(:tagged_well, map: map, aliquot_count: 1) }
-            let(:dual_index_tag_well) { 'd1' }
-
-            let(:downstream_aliquot1) do
-              create(
-                :aliquot,
-                sample: asset.aliquots.first.sample,
-                tag: asset.aliquots.first.tag,
-                tag2: asset.aliquots.first.tag2,
-                library_id: asset.aliquots.first.library_id
-              )
-            end
-            let(:downstream_aliquot2) do
-              create(
-                :aliquot,
-                sample: asset.aliquots.first.sample,
-                tag: asset.aliquots.first.tag,
-                tag2: asset.aliquots.first.tag2,
-                library_id: asset.aliquots.first.library_id
-              )
-            end
-
-            before do
-              asset.aliquots.first.library_id = 1
-              asset.aliquots.first.save!
-              downstream_aliquot1
-              downstream_aliquot2
-            end
-
-            it 'will apply the 2 tags associated with the updated map_id' do
-              sf_dual_index_tag_well.update(aliquot: aliquot, tag_group: nil)
-
-              # 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(sf_dual_index_tag_well.aliquots.first.tag.map_id).to eq 4
+              expect(sf_dual_index_tag_well.aliquots.first.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
+              # NB. in reality downstream aliquots would get updated too by the TagSubstitution code but
+              # that isn't triggered here because we are only testing the specialised field code
             end
           end