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/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 5a915f903b..fc2feb8b47 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,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 diff --git a/spec/sequencescape_excel/specialised_field_spec.rb b/spec/sequencescape_excel/specialised_field_spec.rb index e036cde4b1..837b3226de 100644 --- a/spec/sequencescape_excel/specialised_field_spec.rb +++ b/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