From 7dfbc34b61930f6746d41f1d7a5cd4af40d57c94 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Fri, 8 Mar 2024 11:25:20 +0000
Subject: [PATCH 01/22] First working version - Can successfully upload a
 manifest with multiple rows per well - Tag depth is set on aliquots, to avoid
 tag clash - When initialising the Rows object, group the rows by plate
 barcode and well, to find the pools - Then assign a tag_depth to each row
 within a pool - To insert the tag_depth against the aliquot, it has to be
 passed through a long chain of method calls - messy - Need to handle the case
 where plate barcode and well position aren't present - TODO: refactor - TODO:
 tests

---
 app/models/sample_manifest.rb                      |  8 +++-----
 app/models/sample_manifest/core_behaviour.rb       |  4 ++--
 app/models/sample_manifest_asset.rb                |  8 ++++----
 .../sample_manifest_excel/upload/row.rb            |  9 +++++++--
 .../sample_manifest_excel/upload/rows.rb           | 14 ++++++++++++++
 5 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/app/models/sample_manifest.rb b/app/models/sample_manifest.rb
index 873f9b124b..e1f76a0228 100644
--- a/app/models/sample_manifest.rb
+++ b/app/models/sample_manifest.rb
@@ -131,9 +131,7 @@ def default_filename
 
   # Use a default value of 1 for rows_per_well if not set
   def rows_per_well
-    1
-    # TODO: replace above line with below line when we want to turn the 'rows_per_well' feature on
-    # @rows_per_well || 1
+    @rows_per_well || 1
   end
 
   scope :pending_manifests,
@@ -158,8 +156,8 @@ def generate
     nil
   end
 
-  def create_sample_and_aliquot(sanger_sample_id, asset)
-    core_behaviour.generate_sample_and_aliquot(sanger_sample_id, asset)
+  def create_sample_and_aliquot(sanger_sample_id, asset, row = nil)
+    core_behaviour.generate_sample_and_aliquot(sanger_sample_id, asset, row)
   end
 
   def create_sample(sanger_sample_id)
diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index 385b11006f..a3fb094ad0 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -34,9 +34,9 @@ def details(&block)
   # The samples get registered in the stock resource table at the end of manifest upload and processing
   # (It used to happen here)
   module StockAssets
-    def generate_sample_and_aliquot(sanger_sample_id, receptacle)
+    def generate_sample_and_aliquot(sanger_sample_id, receptacle, row = nil)
       create_sample(sanger_sample_id).tap do |sample|
-        receptacle.aliquots.create!(sample: sample, study: study)
+        receptacle.aliquots.create!(sample: sample, study: study, tag_depth: row&.tag_depth)
         study.samples << sample
       end
     end
diff --git a/app/models/sample_manifest_asset.rb b/app/models/sample_manifest_asset.rb
index c3f82d9ac9..2ca4881f7e 100644
--- a/app/models/sample_manifest_asset.rb
+++ b/app/models/sample_manifest_asset.rb
@@ -18,8 +18,8 @@ class SampleManifestAsset < ApplicationRecord
   convert_labware_to_receptacle_for :asset
 
   # Returns the existing sample, or generates a new one if it doesn't exist
-  def find_or_create_sample
-    self.sample ||= create_sample
+  def find_or_create_sample(row = nil)
+    self.sample ||= create_sample(row)
   end
 
   def aliquot
@@ -36,7 +36,7 @@ def aliquots
 
   private
 
-  def create_sample
-    sample_manifest.create_sample_and_aliquot(sanger_sample_id, asset)
+  def create_sample(row = nil)
+    sample_manifest.create_sample_and_aliquot(sanger_sample_id, asset, row)
   end
 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 75e61c0f3f..4d5858228f 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
@@ -13,9 +13,12 @@ class Row # rubocop:todo Metrics/ClassLength
       include ActiveModel::Model
       include Converters
 
-      attr_accessor :number, :data, :columns, :cache
+      attr_accessor :number, :data, :columns, :cache, :tag_depth
       attr_reader :sanger_sample_id
 
+      attr_reader :plate_barcode
+      attr_reader :well_position
+
       validates :number, presence: true, numericality: true
       validate :sanger_sample_id_exists?, if: :sanger_sample_id
       validates_presence_of :data, :columns
@@ -30,6 +33,8 @@ def initialize(attributes = {})
         super
         @cache ||= SampleManifestAsset
         @sanger_sample_id ||= value(:sanger_sample_id).presence if columns.present? && data.present?
+        @plate_barcode ||= value(:sanger_plate_id) if columns.present? && data.present? # TODO: do we want a 'presence' here?
+        @well_position ||= value(:well) if columns.present? && data.present?
       end
 
       ##
@@ -132,7 +137,7 @@ def reuploaded?
       end
 
       def sample
-        @sample ||= manifest_asset&.find_or_create_sample if sanger_sample_id.present? && !empty?
+        @sample ||= manifest_asset&.find_or_create_sample(self) if sanger_sample_id.present? && !empty?
       end
 
       def sample_updated?
diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
index 1772a79cb7..79772c032a 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
@@ -21,6 +21,7 @@ def initialize(data, columns, cache = SampleManifestAsset)
         @data = data || []
         @columns = columns
         @items = create_rows(cache)
+        @pools = find_pools_if_present
       end
 
       def each(&block)
@@ -44,6 +45,19 @@ def create_rows(cache)
         end
       end
 
+      def find_pools_if_present
+        # add an 'if plate_barcode and well_position exist' condition here
+        pools = items.group_by { |row| row.plate_barcode + ':' + row.well_position }
+        pools.each do |pool_identifier, rows|
+          rows.each_with_index do |row, index|
+            # within each pool, assign a unique tag_depth to each Row
+            row.tag_depth = index + 1
+          end
+        end
+
+        pools
+      end
+
       def check_rows
         items.each { |row| errors.add(:base, row.errors.full_messages) unless row.valid? }
       end

From cb115223b1ce1f1833ee41e2f63c772540f11b78 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Fri, 8 Mar 2024 12:57:59 +0000
Subject: [PATCH 02/22] rubocop

---
 .../sample_manifest_excel/upload/row.rb            | 14 +++++++-------
 .../sample_manifest_excel/upload/rows.rb           |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

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 4d5858228f..d0d1217444 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
@@ -14,10 +14,7 @@ class Row # rubocop:todo Metrics/ClassLength
       include Converters
 
       attr_accessor :number, :data, :columns, :cache, :tag_depth
-      attr_reader :sanger_sample_id
-
-      attr_reader :plate_barcode
-      attr_reader :well_position
+      attr_reader :sanger_sample_id, :plate_barcode, :well_position
 
       validates :number, presence: true, numericality: true
       validate :sanger_sample_id_exists?, if: :sanger_sample_id
@@ -32,9 +29,12 @@ class Row # rubocop:todo Metrics/ClassLength
       def initialize(attributes = {})
         super
         @cache ||= SampleManifestAsset
-        @sanger_sample_id ||= value(:sanger_sample_id).presence if columns.present? && data.present?
-        @plate_barcode ||= value(:sanger_plate_id) if columns.present? && data.present? # TODO: do we want a 'presence' here?
-        @well_position ||= value(:well) if columns.present? && data.present?
+
+        return unless columns.present? && data.present?
+
+        @sanger_sample_id ||= value(:sanger_sample_id).presence
+        @plate_barcode ||= value(:sanger_plate_id) # TODO: do we want a 'presence' here?
+        @well_position ||= value(:well) # TODO: do we want a 'presence' here?
       end
 
       ##
diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
index 79772c032a..845278e3ee 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
@@ -47,8 +47,8 @@ def create_rows(cache)
 
       def find_pools_if_present
         # add an 'if plate_barcode and well_position exist' condition here
-        pools = items.group_by { |row| row.plate_barcode + ':' + row.well_position }
-        pools.each do |pool_identifier, rows|
+        pools = items.group_by { |row| "#{row.plate_barcode}:#{row.well_position}" }
+        pools.each do |_pool_identifier, rows|
           rows.each_with_index do |row, index|
             # within each pool, assign a unique tag_depth to each Row
             row.tag_depth = index + 1

From 00b8e18728c891e1b87766ee8d49aee91c4cf34a Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Fri, 8 Mar 2024 13:03:10 +0000
Subject: [PATCH 03/22] Only try to add tag_depth if certain conditions are
 met: - the manifest has plate barcode and well columns in it (not a tube or
 tube rack) - there are multiple rows per well - TODO: need to also avoid
 adding it if the samples are tagged

---
 .../sample_manifest_excel/upload/rows.rb                    | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
index 845278e3ee..f4e5f4a723 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
@@ -46,8 +46,12 @@ def create_rows(cache)
       end
 
       def find_pools_if_present
-        # add an 'if plate_barcode and well_position exist' condition here
+        return unless row.plate_barcode && row.well_position
+
         pools = items.group_by { |row| "#{row.plate_barcode}:#{row.well_position}" }
+        return if pools.values.all?(&:one?)
+
+        # TODO: only add tag_depth if there are no tags being assigned
         pools.each do |_pool_identifier, rows|
           rows.each_with_index do |row, index|
             # within each pool, assign a unique tag_depth to each Row

From 15a4c6c6cf24637d2cce6aca82de0fc90dcfda3f Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Fri, 8 Mar 2024 13:15:22 +0000
Subject: [PATCH 04/22] fix failing tests

---
 app/models/sample_manifest/core_behaviour.rb                   | 2 +-
 app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index a3fb094ad0..a19d73d2b9 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -48,7 +48,7 @@ def stocks?
 
   # Used for read-made libraries. Ensures that the library_id gets set
   module LibraryAssets
-    def generate_sample_and_aliquot(sanger_sample_id, receptacle)
+    def generate_sample_and_aliquot(sanger_sample_id, receptacle, row = nil)
       create_sample(sanger_sample_id).tap do |sample|
         receptacle.aliquots.create!(sample: sample, study: study, library: receptacle)
         study.samples << sample
diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
index f4e5f4a723..0303c9a3d8 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
@@ -46,7 +46,7 @@ def create_rows(cache)
       end
 
       def find_pools_if_present
-        return unless row.plate_barcode && row.well_position
+        return unless items.first.plate_barcode && items.first.well_position
 
         pools = items.group_by { |row| "#{row.plate_barcode}:#{row.well_position}" }
         return if pools.values.all?(&:one?)

From 03d5eef6316ed89cfaaa58be54e28057acce38c5 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Fri, 8 Mar 2024 14:59:03 +0000
Subject: [PATCH 05/22] Only set tag_depth if there are no tags provided in the
 manifest - the reason for tag_depth is to avoid tag clash, when there are
 multiple untagged samples in a pool together - not sure of the
 implementation, might refactor - also add some null safety due to failing
 tests, where items.first is nil (think because the file is invalid)

---
 .../sample_manifest_excel/upload/row.rb                      | 3 ++-
 .../sample_manifest_excel/upload/rows.rb                     | 5 +++--
 app/sequencescape_excel/sequencescape_excel/column_list.rb   | 5 +++++
 3 files changed, 10 insertions(+), 3 deletions(-)

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 d0d1217444..160044b77f 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
@@ -14,7 +14,7 @@ class Row # rubocop:todo Metrics/ClassLength
       include Converters
 
       attr_accessor :number, :data, :columns, :cache, :tag_depth
-      attr_reader :sanger_sample_id, :plate_barcode, :well_position
+      attr_reader :sanger_sample_id, :plate_barcode, :well_position, :has_tags
 
       validates :number, presence: true, numericality: true
       validate :sanger_sample_id_exists?, if: :sanger_sample_id
@@ -35,6 +35,7 @@ def initialize(attributes = {})
         @sanger_sample_id ||= value(:sanger_sample_id).presence
         @plate_barcode ||= value(:sanger_plate_id) # TODO: do we want a 'presence' here?
         @well_position ||= value(:well) # TODO: do we want a 'presence' here?
+        @has_tags ||= columns.has_tags?
       end
 
       ##
diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
index 0303c9a3d8..c487a6189c 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
@@ -46,12 +46,13 @@ def create_rows(cache)
       end
 
       def find_pools_if_present
-        return unless items.first.plate_barcode && items.first.well_position
+        return unless items&.first&.plate_barcode && items&.first&.well_position
+
+        return if items&.first&.has_tags
 
         pools = items.group_by { |row| "#{row.plate_barcode}:#{row.well_position}" }
         return if pools.values.all?(&:one?)
 
-        # TODO: only add tag_depth if there are no tags being assigned
         pools.each do |_pool_identifier, rows|
           rows.each_with_index do |row, index|
             # within each pool, assign a unique tag_depth to each Row
diff --git a/app/sequencescape_excel/sequencescape_excel/column_list.rb b/app/sequencescape_excel/sequencescape_excel/column_list.rb
index e704d4a6bf..0cda552c42 100644
--- a/app/sequencescape_excel/sequencescape_excel/column_list.rb
+++ b/app/sequencescape_excel/sequencescape_excel/column_list.rb
@@ -96,6 +96,11 @@ def inspect
       "<#{self.class}: @columns:#{columns.map(&:name).inspect}...>"
     end
 
+    def has_tags?
+      names = columns.map(&:name)
+      names.any? { |name| name.include? 'tag' }
+    end
+
     private
 
     # You can add a hash of columns, a hash of attributes or an array of columns.

From 300416109587dc56663df67eb786035134a6191d Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Mon, 11 Mar 2024 14:13:21 +0000
Subject: [PATCH 06/22] Add new method to null column list to fix test

---
 .../sequencescape_excel/null_objects/null_column_list.rb      | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb b/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb
index a33bbdb830..ee10a912c0 100644
--- a/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb
+++ b/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb
@@ -26,6 +26,10 @@ def errors
       def with_specialised_fields
         []
       end
+
+      def has_tags?
+        false
+      end
     end
   end
 end

From 41c6cdbee0d0bb2c7890f1f25e9d57598565448c Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Wed, 13 Mar 2024 15:21:47 +0000
Subject: [PATCH 07/22] Change implementation - avoids passing extra parameter
 through long method chain - finds pools on sample manifest by querying sample
 manifest assets - assigns tag depth based on position in those pools - only
 assigns tag depth in StockAssets module, not LibraryAssets - this is a proxy
 for determining whether tags are present or not - we only want to assign
 tag_depth if tags are not present - remove previous implementation (mainly in
 Row and Rows)

---
 app/models/sample_manifest.rb                 | 17 +++++++++++++++--
 app/models/sample_manifest/core_behaviour.rb  | 16 ++++++++++++----
 app/models/sample_manifest_asset.rb           |  6 +++---
 .../sample_manifest_excel/upload/row.rb       |  2 +-
 .../sample_manifest_excel/upload/rows.rb      | 19 -------------------
 5 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/app/models/sample_manifest.rb b/app/models/sample_manifest.rb
index e1f76a0228..f399196c78 100644
--- a/app/models/sample_manifest.rb
+++ b/app/models/sample_manifest.rb
@@ -134,6 +134,19 @@ def rows_per_well
     @rows_per_well || 1
   end
 
+  def pools
+    @pools ||= begin
+      receptacle_to_smas = sample_manifest_assets.group_by(&:asset)
+
+      # if all receptacles only contain 0 or 1 sample, there are no pools so return nil
+      if receptacle_to_smas.values.all?{ |smas| smas.size <= 1 }
+        nil
+      else
+        receptacle_to_smas
+      end
+    end
+  end
+
   scope :pending_manifests,
         -> {
           order(id: :desc).includes(:uploaded_document).references(:uploaded_document).where(documents: { id: nil })
@@ -156,8 +169,8 @@ def generate
     nil
   end
 
-  def create_sample_and_aliquot(sanger_sample_id, asset, row = nil)
-    core_behaviour.generate_sample_and_aliquot(sanger_sample_id, asset, row)
+  def create_sample_and_aliquot(sanger_sample_id, asset)
+    core_behaviour.generate_sample_and_aliquot(sanger_sample_id, asset)
   end
 
   def create_sample(sanger_sample_id)
diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index a19d73d2b9..6982460d5d 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -34,9 +34,16 @@ def details(&block)
   # The samples get registered in the stock resource table at the end of manifest upload and processing
   # (It used to happen here)
   module StockAssets
-    def generate_sample_and_aliquot(sanger_sample_id, receptacle, row = nil)
+    # Assume no tags?
+    def generate_sample_and_aliquot(sanger_sample_id, receptacle)
       create_sample(sanger_sample_id).tap do |sample|
-        receptacle.aliquots.create!(sample: sample, study: study, tag_depth: row&.tag_depth)
+        tag_depth = if @manifest.pools
+                      @manifest.pools[receptacle].find_index { |sma| sma.sample.sanger_sample_id == sanger_sample_id }
+                    else
+                      nil
+                    end
+
+        receptacle.aliquots.create!(sample: sample, study: study, tag_depth: tag_depth)
         study.samples << sample
       end
     end
@@ -46,9 +53,10 @@ def stocks?
     end
   end
 
-  # Used for read-made libraries. Ensures that the library_id gets set
+  # Used for ready-made libraries. Ensures that the library_id gets set
   module LibraryAssets
-    def generate_sample_and_aliquot(sanger_sample_id, receptacle, row = nil)
+    # Assume tags?
+    def generate_sample_and_aliquot(sanger_sample_id, receptacle)
       create_sample(sanger_sample_id).tap do |sample|
         receptacle.aliquots.create!(sample: sample, study: study, library: receptacle)
         study.samples << sample
diff --git a/app/models/sample_manifest_asset.rb b/app/models/sample_manifest_asset.rb
index 2ca4881f7e..11a5f973fd 100644
--- a/app/models/sample_manifest_asset.rb
+++ b/app/models/sample_manifest_asset.rb
@@ -18,8 +18,8 @@ class SampleManifestAsset < ApplicationRecord
   convert_labware_to_receptacle_for :asset
 
   # Returns the existing sample, or generates a new one if it doesn't exist
-  def find_or_create_sample(row = nil)
-    self.sample ||= create_sample(row)
+  def find_or_create_sample()
+    self.sample ||= create_sample
   end
 
   def aliquot
@@ -37,6 +37,6 @@ def aliquots
   private
 
   def create_sample(row = nil)
-    sample_manifest.create_sample_and_aliquot(sanger_sample_id, asset, row)
+    sample_manifest.create_sample_and_aliquot(sanger_sample_id, asset)
   end
 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 160044b77f..c06f1e25f8 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
@@ -138,7 +138,7 @@ def reuploaded?
       end
 
       def sample
-        @sample ||= manifest_asset&.find_or_create_sample(self) if sanger_sample_id.present? && !empty?
+        @sample ||= manifest_asset&.find_or_create_sample if sanger_sample_id.present? && !empty?
       end
 
       def sample_updated?
diff --git a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
index c487a6189c..1772a79cb7 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/rows.rb
@@ -21,7 +21,6 @@ def initialize(data, columns, cache = SampleManifestAsset)
         @data = data || []
         @columns = columns
         @items = create_rows(cache)
-        @pools = find_pools_if_present
       end
 
       def each(&block)
@@ -45,24 +44,6 @@ def create_rows(cache)
         end
       end
 
-      def find_pools_if_present
-        return unless items&.first&.plate_barcode && items&.first&.well_position
-
-        return if items&.first&.has_tags
-
-        pools = items.group_by { |row| "#{row.plate_barcode}:#{row.well_position}" }
-        return if pools.values.all?(&:one?)
-
-        pools.each do |_pool_identifier, rows|
-          rows.each_with_index do |row, index|
-            # within each pool, assign a unique tag_depth to each Row
-            row.tag_depth = index + 1
-          end
-        end
-
-        pools
-      end
-
       def check_rows
         items.each { |row| errors.add(:base, row.errors.full_messages) unless row.valid? }
       end

From 22478d158b77f55c581801920b389bf41c6a69a6 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Wed, 13 Mar 2024 15:24:25 +0000
Subject: [PATCH 08/22] remove more bits from the previous implementation

---
 app/models/sample_manifest_asset.rb                  |  4 ++--
 .../sample_manifest_excel/upload/row.rb              | 12 +++---------
 .../sequencescape_excel/column_list.rb               |  5 -----
 .../null_objects/null_column_list.rb                 |  4 ----
 4 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/app/models/sample_manifest_asset.rb b/app/models/sample_manifest_asset.rb
index 11a5f973fd..c3f82d9ac9 100644
--- a/app/models/sample_manifest_asset.rb
+++ b/app/models/sample_manifest_asset.rb
@@ -18,7 +18,7 @@ class SampleManifestAsset < ApplicationRecord
   convert_labware_to_receptacle_for :asset
 
   # Returns the existing sample, or generates a new one if it doesn't exist
-  def find_or_create_sample()
+  def find_or_create_sample
     self.sample ||= create_sample
   end
 
@@ -36,7 +36,7 @@ def aliquots
 
   private
 
-  def create_sample(row = nil)
+  def create_sample
     sample_manifest.create_sample_and_aliquot(sanger_sample_id, asset)
   end
 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 c06f1e25f8..75e61c0f3f 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/upload/row.rb
@@ -13,8 +13,8 @@ class Row # rubocop:todo Metrics/ClassLength
       include ActiveModel::Model
       include Converters
 
-      attr_accessor :number, :data, :columns, :cache, :tag_depth
-      attr_reader :sanger_sample_id, :plate_barcode, :well_position, :has_tags
+      attr_accessor :number, :data, :columns, :cache
+      attr_reader :sanger_sample_id
 
       validates :number, presence: true, numericality: true
       validate :sanger_sample_id_exists?, if: :sanger_sample_id
@@ -29,13 +29,7 @@ class Row # rubocop:todo Metrics/ClassLength
       def initialize(attributes = {})
         super
         @cache ||= SampleManifestAsset
-
-        return unless columns.present? && data.present?
-
-        @sanger_sample_id ||= value(:sanger_sample_id).presence
-        @plate_barcode ||= value(:sanger_plate_id) # TODO: do we want a 'presence' here?
-        @well_position ||= value(:well) # TODO: do we want a 'presence' here?
-        @has_tags ||= columns.has_tags?
+        @sanger_sample_id ||= value(:sanger_sample_id).presence if columns.present? && data.present?
       end
 
       ##
diff --git a/app/sequencescape_excel/sequencescape_excel/column_list.rb b/app/sequencescape_excel/sequencescape_excel/column_list.rb
index 0cda552c42..e704d4a6bf 100644
--- a/app/sequencescape_excel/sequencescape_excel/column_list.rb
+++ b/app/sequencescape_excel/sequencescape_excel/column_list.rb
@@ -96,11 +96,6 @@ def inspect
       "<#{self.class}: @columns:#{columns.map(&:name).inspect}...>"
     end
 
-    def has_tags?
-      names = columns.map(&:name)
-      names.any? { |name| name.include? 'tag' }
-    end
-
     private
 
     # You can add a hash of columns, a hash of attributes or an array of columns.
diff --git a/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb b/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb
index ee10a912c0..a33bbdb830 100644
--- a/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb
+++ b/app/sequencescape_excel/sequencescape_excel/null_objects/null_column_list.rb
@@ -26,10 +26,6 @@ def errors
       def with_specialised_fields
         []
       end
-
-      def has_tags?
-        false
-      end
     end
   end
 end

From f506cb49b1cb8251ae7871baa3aacc2f1b0fb8e3 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Thu, 14 Mar 2024 09:28:55 +0000
Subject: [PATCH 09/22] prettier

---
 app/models/sample_manifest.rb                | 15 ++++++---------
 app/models/sample_manifest/core_behaviour.rb | 11 ++++++-----
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/app/models/sample_manifest.rb b/app/models/sample_manifest.rb
index f399196c78..90c1f08f01 100644
--- a/app/models/sample_manifest.rb
+++ b/app/models/sample_manifest.rb
@@ -135,16 +135,13 @@ def rows_per_well
   end
 
   def pools
-    @pools ||= begin
-      receptacle_to_smas = sample_manifest_assets.group_by(&:asset)
-
-      # if all receptacles only contain 0 or 1 sample, there are no pools so return nil
-      if receptacle_to_smas.values.all?{ |smas| smas.size <= 1 }
-        nil
-      else
-        receptacle_to_smas
+    @pools ||=
+      begin
+        receptacle_to_smas = sample_manifest_assets.group_by(&:asset)
+
+        # if all receptacles only contain 0 or 1 sample, there are no pools so return nil
+        receptacle_to_smas.values.all? { |smas| smas.size <= 1 } ? nil : receptacle_to_smas
       end
-    end
   end
 
   scope :pending_manifests,
diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index 6982460d5d..765293c082 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -37,11 +37,12 @@ module StockAssets
     # Assume no tags?
     def generate_sample_and_aliquot(sanger_sample_id, receptacle)
       create_sample(sanger_sample_id).tap do |sample|
-        tag_depth = if @manifest.pools
-                      @manifest.pools[receptacle].find_index { |sma| sma.sample.sanger_sample_id == sanger_sample_id }
-                    else
-                      nil
-                    end
+        tag_depth =
+          if @manifest.pools
+            @manifest.pools[receptacle].find_index { |sma| sma.sample.sanger_sample_id == sanger_sample_id }
+          else
+            nil
+          end
 
         receptacle.aliquots.create!(sample: sample, study: study, tag_depth: tag_depth)
         study.samples << sample

From a06a08db9354bee8bd4ed9551ec8037d80182ab0 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Thu, 14 Mar 2024 10:06:10 +0000
Subject: [PATCH 10/22] refactor

---
 app/models/sample_manifest/core_behaviour.rb | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index 765293c082..07c07a462c 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -37,14 +37,10 @@ module StockAssets
     # Assume no tags?
     def generate_sample_and_aliquot(sanger_sample_id, receptacle)
       create_sample(sanger_sample_id).tap do |sample|
-        tag_depth =
-          if @manifest.pools
-            @manifest.pools[receptacle].find_index { |sma| sma.sample.sanger_sample_id == sanger_sample_id }
-          else
-            nil
-          end
+        tag_depth = tag_depth_for_sample(@manifest.pools, receptacle, sanger_sample_id)
 
         receptacle.aliquots.create!(sample: sample, study: study, tag_depth: tag_depth)
+
         study.samples << sample
       end
     end
@@ -52,6 +48,12 @@ def generate_sample_and_aliquot(sanger_sample_id, receptacle)
     def stocks?
       true
     end
+
+    def tag_depth_for_sample(pools, receptacle, sanger_sample_id)
+      return nil unless pools
+
+      pools[receptacle].find_index { |sma| sma.sample.sanger_sample_id == sanger_sample_id }
+    end
   end
 
   # Used for ready-made libraries. Ensures that the library_id gets set

From f5d541b10c74dda8cdfd6b0aed2d76c75c0ddd67 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Thu, 14 Mar 2024 11:55:33 +0000
Subject: [PATCH 11/22] Add documentation comments

---
 app/models/sample_manifest.rb                | 17 ++++++++++++++++-
 app/models/sample_manifest/core_behaviour.rb | 10 ++++++++--
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/app/models/sample_manifest.rb b/app/models/sample_manifest.rb
index 90c1f08f01..c38ee25346 100644
--- a/app/models/sample_manifest.rb
+++ b/app/models/sample_manifest.rb
@@ -129,11 +129,26 @@ def default_filename
     "#{study_id}stdy_manifest_#{id}_#{created_at.to_formatted_s(:dmy)}"
   end
 
-  # Use a default value of 1 for rows_per_well if not set
+  # Number of rows per well in the manifest.
+  # Specified in the manifest_types.yml config.
+  # Used to pre-populate the spreadsheet with a sufficient number of rows per well,
+  # in the case where we are importing a plate containing pools, and we want to provide
+  # sample-level information for the pools.
+  # Developed initially for the scRNA Core pipeline.
+  #
+  # Uses a default value of 1 if not set.
   def rows_per_well
     @rows_per_well || 1
   end
 
+  # Used in manifest upload code to determine if pools are present,
+  # so that tag_depth can be set on the aliquots if needed.
+  # Checks if at least one receptacle has more than one sample manifest asset.
+  # Sample manifest assets are a join table between sample manifest and receptacle,
+  # created upfront when a manifest is generated.
+  #
+  # Returns a hash, where key is receptacle and value is array of sample manifest assets.
+  # Returns nil if all receptacles only contain 0 or 1 sample.
   def pools
     @pools ||=
       begin
diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index 07c07a462c..d4bc1eb1e8 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -34,7 +34,11 @@ def details(&block)
   # The samples get registered in the stock resource table at the end of manifest upload and processing
   # (It used to happen here)
   module StockAssets
-    # Assume no tags?
+    # Used in manifest upload code to insert the sample and aliquot the database.
+    # The receptacle and sanger_sample_id already exist as they are inserted upfront when the manifest is generated.
+    # tag_depth is set on the aliquot if pools are present, and if the samples are not tagged, to avoid tag clash.
+    # The assumption is made that the fact we're in the 'StockAssets' module means tags are never present,
+    # and if we are in the LibraryAssets module, then tags are always present (hence no tag_depth setting in the equivalent method there).
     def generate_sample_and_aliquot(sanger_sample_id, receptacle)
       create_sample(sanger_sample_id).tap do |sample|
         tag_depth = tag_depth_for_sample(@manifest.pools, receptacle, sanger_sample_id)
@@ -49,6 +53,9 @@ def stocks?
       true
     end
 
+    # Assigns a tag_depth to a sample in a pool.
+    # Tag_depth just needs to be a unique integer for each sample in the pool,
+    # So we just use the index in the list of sample manifest assets in this receptacle.
     def tag_depth_for_sample(pools, receptacle, sanger_sample_id)
       return nil unless pools
 
@@ -58,7 +65,6 @@ def tag_depth_for_sample(pools, receptacle, sanger_sample_id)
 
   # Used for ready-made libraries. Ensures that the library_id gets set
   module LibraryAssets
-    # Assume tags?
     def generate_sample_and_aliquot(sanger_sample_id, receptacle)
       create_sample(sanger_sample_id).tap do |sample|
         receptacle.aliquots.create!(sample: sample, study: study, library: receptacle)

From 5db1e5e6e443124ff67452576ff40551f52e2ecd Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Thu, 14 Mar 2024 13:32:59 +0000
Subject: [PATCH 12/22] reword comment

---
 app/models/sample_manifest/core_behaviour.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index d4bc1eb1e8..31e4100528 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -37,8 +37,8 @@ module StockAssets
     # Used in manifest upload code to insert the sample and aliquot the database.
     # The receptacle and sanger_sample_id already exist as they are inserted upfront when the manifest is generated.
     # tag_depth is set on the aliquot if pools are present, and if the samples are not tagged, to avoid tag clash.
-    # The assumption is made that the fact we're in the 'StockAssets' module means tags are never present,
-    # and if we are in the LibraryAssets module, then tags are always present (hence no tag_depth setting in the equivalent method there).
+    # The assumption is made that samples passed to the below method are never tagged,
+    # because we're in the 'StockAssets' module rather than the 'LibraryAssets' module.
     def generate_sample_and_aliquot(sanger_sample_id, receptacle)
       create_sample(sanger_sample_id).tap do |sample|
         tag_depth = tag_depth_for_sample(@manifest.pools, receptacle, sanger_sample_id)

From a146950c771c421c829afd3d016030dd11912fa5 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Thu, 14 Mar 2024 14:30:53 +0000
Subject: [PATCH 13/22] correction - sanger_sample_id is directly on sample
 manifest asset

---
 app/models/sample_manifest/core_behaviour.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index 31e4100528..b5c78b7e2a 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -59,7 +59,7 @@ def stocks?
     def tag_depth_for_sample(pools, receptacle, sanger_sample_id)
       return nil unless pools
 
-      pools[receptacle].find_index { |sma| sma.sample.sanger_sample_id == sanger_sample_id }
+      pools[receptacle].find_index { |sma| sma.sanger_sample_id == sanger_sample_id }
     end
   end
 

From 674596b045eeaec2d5efafd9d78bd4bb57acce4a Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Mon, 18 Mar 2024 11:33:01 +0000
Subject: [PATCH 14/22] Enable the tests for the rows_per_well feature - this
 feature was merged in 'feature flagged off', but we are enabling it in this
 PR

---
 spec/models/sample_manifest/generator_spec.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/spec/models/sample_manifest/generator_spec.rb b/spec/models/sample_manifest/generator_spec.rb
index f6b9ccbdac..8adcb1c38c 100644
--- a/spec/models/sample_manifest/generator_spec.rb
+++ b/spec/models/sample_manifest/generator_spec.rb
@@ -128,7 +128,6 @@
   context 'with rows_per_well set' do
     let(:template) { 'pools_plate' }
 
-    # TODO: un-skip when rows_per_well feature is enabled
     skip 'generates a details array with more than one entry per well' do
       generator = described_class.new(attributes, user, configuration)
       generator.execute
@@ -137,7 +136,6 @@
   end
 
   context 'with rows_per_well not set' do
-    # TODO: un-skip when rows_per_well feature is enabled
     skip 'generates a details array with one entry per well' do
       generator = described_class.new(attributes, user, configuration)
       generator.execute

From 52871a3637d8ffc60a3bb76035457e241aa6e8c3 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Fri, 22 Mar 2024 14:40:17 +0000
Subject: [PATCH 15/22] Add tests for the new 'pools' method on sample manifest
 - Adapt existing factory to allow multiple samples per well (pools)

---
 spec/factories/sample_manifest_factories.rb | 19 ++++---
 spec/models/sample_manifest_spec.rb         | 55 ++++++++++++++-------
 2 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/spec/factories/sample_manifest_factories.rb b/spec/factories/sample_manifest_factories.rb
index ea4488f0ac..00be33ee6b 100644
--- a/spec/factories/sample_manifest_factories.rb
+++ b/spec/factories/sample_manifest_factories.rb
@@ -15,8 +15,9 @@
     factory :plate_sample_manifest_with_manifest_assets do
       transient do
         num_plates { 1 }
-        num_samples_per_plate { 1 }
-        plates { create_list :plate, num_plates, well_factory: :empty_well, well_count: num_samples_per_plate }
+        num_wells_per_plate { 1 }
+        num_samples_per_well { 1 }
+        plates { create_list :plate, num_plates, well_factory: :empty_well, well_count: num_wells_per_plate }
       end
 
       barcodes { plates.map(&:human_barcode) }
@@ -26,12 +27,14 @@
           .plates
           .flat_map(&:wells)
           .each do |well|
-            create(
-              :sample_manifest_asset,
-              sanger_sample_id: generate(:sanger_sample_id),
-              asset: well,
-              sample_manifest: sample_manifest
-            )
+            evaluator.num_samples_per_well.times do
+              create(
+                :sample_manifest_asset,
+                sanger_sample_id: generate(:sanger_sample_id),
+                asset: well,
+                sample_manifest: sample_manifest
+              )
+            end
           end
         sample_manifest.barcodes = sample_manifest.labware.map(&:human_barcode)
       end
diff --git a/spec/models/sample_manifest_spec.rb b/spec/models/sample_manifest_spec.rb
index a03fa5cd9c..1ceaf1dbd5 100644
--- a/spec/models/sample_manifest_spec.rb
+++ b/spec/models/sample_manifest_spec.rb
@@ -37,13 +37,13 @@
         context "count: #{count}" do
           let(:count) { count }
 
-          it "create #{count} plate(s), #{count * 96} wells" do
+          it "creates #{count} plate(s), #{count * 96} wells" do
             expect { manifest.generate }.to change(Plate, :count).by(count).and change(Well, :count).by(count * 96)
             expect(manifest.labware.count).to eq(count)
             expect(manifest.labware.first).to be_a(Plate)
           end
 
-          it 'create sample manifest assets' do
+          it 'creates sample manifest assets' do
             expect { manifest.generate }.to change(SampleManifestAsset, :count).by(count * 96)
             wells = Plate.includes(:wells).with_barcode(manifest.barcodes).flat_map(&:wells)
             expect(manifest.assets).to eq(wells)
@@ -62,7 +62,7 @@
               )
             end
 
-            it 'create sample and aliquots' do
+            it 'creates sample and aliquots' do
               sma1 = manifest.sample_manifest_assets.first
               expect { manifest.create_sample_and_aliquot(sma1.sanger_sample_id, sma1.asset) }.to change(Sample, :count)
                 .by(1).and change { study.samples.count }.by(1)
@@ -82,7 +82,7 @@
 
         before { manifest.generate }
 
-        it 'create a plate of the correct purpose' do
+        it 'creates a plate of the correct purpose' do
           expect(Plate.last.purpose).to eq(purpose)
         end
       end
@@ -96,19 +96,19 @@
 
       teardown { Delayed::Worker.delay_jobs = true }
 
-      it 'create 1 plate(s), 96 wells' do
+      it 'creates 1 plate(s), 96 wells' do
         expect { manifest.generate }.to change(Plate, :count).by(count).and change(Well, :count).by(count * 96)
         expect(manifest.labware.count).to eq(count)
         expect(manifest.labware.first).to be_a(Plate)
       end
 
-      it 'create sample manifest assets' do
+      it 'creates sample manifest assets' do
         expect { manifest.generate }.to change(SampleManifestAsset, :count).by(count * 96)
         wells = Plate.includes(:wells).with_barcode(manifest.barcodes).flat_map(&:wells)
         expect(manifest.assets).to eq(wells)
       end
 
-      context 'follwing generation' do
+      context 'following generation' do
         before { manifest.generate }
 
         it 'returns the details of the created samples' do
@@ -121,7 +121,7 @@
           )
         end
 
-        it 'create sample and aliquots' do
+        it 'creates sample and aliquots' do
           sma1 = manifest.sample_manifest_assets.first
           expect { manifest.create_sample_and_aliquot(sma1.sanger_sample_id, sma1.asset) }.to change(Sample, :count).by(
             1
@@ -141,7 +141,7 @@
 
         before { manifest.generate }
 
-        it 'create a plate of the correct purpose' do
+        it 'creates a plate of the correct purpose' do
           expect(Plate.last.purpose).to eq(purpose)
         end
       end
@@ -172,7 +172,7 @@
                                                       ).by(1).and change(BroadcastEvent, :count).by(1)
           end
 
-          it 'create sample manifest asset' do
+          it 'creates sample manifest assets' do
             expect { manifest.generate }.to change(SampleManifestAsset, :count).by(count)
             expect(manifest.assets).to match_array(LibraryTube.with_barcode(manifest.barcodes).map(&:receptacle))
           end
@@ -189,7 +189,7 @@
               )
             end
 
-            it 'create sample and aliquots' do
+            it 'creates sample and aliquots' do
               sma = manifest.sample_manifest_assets.last
               expect { manifest.create_sample_and_aliquot(sma.sanger_sample_id, sma.asset) }.to change(Sample, :count)
                 .by(1).and change { study.samples.count }.by(1)
@@ -219,7 +219,7 @@
       let(:count) { 1 }
 
       context 'library tubes' do
-        it 'create 1 tube' do
+        it 'creates 1 tube' do
           # We need to create library tubes as we have downstream dependencies that assume a unique library tube
           expect { manifest.generate }.to change(LibraryTube, :count).by(count)
           expect { manifest.generate }.not_to change(MultiplexedLibraryTube, :count)
@@ -237,12 +237,12 @@
             expect(manifest.details_array.first).to eq(barcode: manifest.barcodes.first, sample_id: "WTCCC#{sample_id}")
           end
 
-          it 'create sample manifest asset' do
+          it 'creates sample manifest asset' do
             expect(manifest.assets.count).to eq(count)
             expect(manifest.assets).to eq(LibraryTube.with_barcode(manifest.barcodes).map(&:receptacle))
           end
 
-          it 'create sample and aliquots' do
+          it 'creates sample and aliquots' do
             sma = manifest.sample_manifest_assets.last
             expect { manifest.create_sample_and_aliquot(sma.sanger_sample_id, sma.asset) }.to change(Sample, :count).by(
               1
@@ -275,7 +275,7 @@
         context "#{count} tubes(s)" do
           let(:count) { count }
 
-          it "create #{count} tubes(s)" do
+          it "creates #{count} tubes(s)" do
             expect { manifest.generate }.to change(SampleTube, :count).by(count).and change { manifest.assets.count }
                                                        .by(count)
             expect(manifest.assets).to eq(SampleTube.with_barcode(manifest.barcodes).map(&:receptacle))
@@ -284,7 +284,7 @@
           context 'when generation has completed' do
             before { manifest.generate }
 
-            it 'create sample and aliquots' do
+            it 'creates sample and aliquots' do
               sma = manifest.sample_manifest_assets.last
               expect { manifest.create_sample_and_aliquot(sma.sanger_sample_id, sma.asset) }.to change(Sample, :count)
                 .by(1).and change { study.samples.count }.by(1)
@@ -293,7 +293,7 @@
               expect(manifest.samples.first.primary_aliquot.study).to eq(study)
             end
 
-            it 'create create asset requests when jobs are processed' do
+            it 'creates create asset requests when jobs are processed' do
               # Not entirely certain this behaviour is all that useful to us.
               Delayed::Worker.new.work_off
 
@@ -346,4 +346,25 @@
       expect(Delayed::Job.count).to eq(manifest.count)
     end
   end
+
+  describe '#pools' do
+    let(:manifest) { create :plate_sample_manifest_with_manifest_assets, study: study, asset_type: 'plate', num_samples_per_well: num_samples_per_well }
+
+    context 'when there is only one sample per well' do
+      let(:num_samples_per_well) { 1 }
+
+      it 'returns nil' do
+        expect(manifest.pools).to be_nil
+      end
+    end
+
+    context 'when there are multiple samples per well' do
+      let(:num_samples_per_well) { 2 }
+
+      it 'returns a hash of pools' do
+        expect(manifest.pools).to be_a(Hash)
+        expect(manifest.pools.size).to eq(manifest.labware.size)
+      end
+    end
+  end
 end

From 918e828cba31af273eabcbf0ab4dbc3a74c9955d Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Mon, 25 Mar 2024 15:44:44 +0000
Subject: [PATCH 16/22] Add a test to uploader_spec to check the new
 functionality - tests  method in sample_manifest - tests  in sample_manifest
 > core_behaviour - had to add rows_per_well functionality to the factories
 and test worksheet

---
 .../worksheet/test_worksheet.rb               |  12 +-
 .../test_download_plates.rb                   |   6 +-
 spec/factories/sample_manifest_factories.rb   |   9 +-
 spec/models/sample_manifest/uploader_spec.rb  | 107 ++++++++++++------
 spec/sample_manifest_excel/worksheet_spec.rb  |   4 +-
 5 files changed, 94 insertions(+), 44 deletions(-)

diff --git a/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb b/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb
index 9f24f297b8..ddac847632 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb
@@ -21,7 +21,8 @@ class TestWorksheet < SequencescapeExcel::Worksheet::Base # rubocop:todo Metrics
                     :partial,
                     :cgap,
                     :num_plates,
-                    :num_samples_per_plate
+                    :num_filled_wells_per_plate,
+                    :num_rows_per_well
       attr_reader :dynamic_attributes, :tags, :study
       attr_writer :manifest_type
 
@@ -60,7 +61,7 @@ def last_row
 
       def compute_last_row
         if %w[plate_default plate_full plate_rnachip].include? manifest_type
-          computed_first_row + (num_plates * num_samples_per_plate) - 1
+          computed_first_row + (num_plates * num_filled_wells_per_plate * num_rows_per_well) - 1
         else
           computed_first_row + no_of_rows
         end
@@ -88,7 +89,8 @@ def create_sample_manifest # rubocop:todo Metrics/MethodLength
           FactoryBot.create(
             :pending_plate_sample_manifest,
             num_plates: num_plates,
-            num_samples_per_plate: num_samples_per_plate,
+            num_filled_wells_per_plate: num_filled_wells_per_plate,
+            num_rows_per_well: num_rows_per_well,
             study: study
           )
         when /tube_library/, /tube_chromium_library/
@@ -329,6 +331,10 @@ def chromium_tags
       def computed_first_row
         type == 'Tube Racks' ? first_row + count + 1 : first_row
       end
+
+      def num_rows_per_well
+        @num_rows_per_well ||= 1
+      end
     end
   end
 end
diff --git a/spec/factories/sample_manifest_excel/test_download_plates.rb b/spec/factories/sample_manifest_excel/test_download_plates.rb
index 487e4457d7..83bb6ed682 100644
--- a/spec/factories/sample_manifest_excel/test_download_plates.rb
+++ b/spec/factories/sample_manifest_excel/test_download_plates.rb
@@ -5,7 +5,8 @@
     columns { build :column_list }
     validation_errors { [] }
     num_plates { 2 }
-    num_samples_per_plate { 2 }
+    num_filled_wells_per_plate { 2 }
+    num_rows_per_well { 1 }
     study { 'WTCCC' }
     supplier { 'Test Supplier' }
     partial { false }
@@ -41,7 +42,8 @@
         study: study,
         supplier: supplier,
         num_plates: num_plates,
-        num_samples_per_plate: num_samples_per_plate,
+        num_filled_wells_per_plate: num_filled_wells_per_plate,
+        num_rows_per_well: num_rows_per_well,
         type: type,
         manifest_type: manifest_type
       )
diff --git a/spec/factories/sample_manifest_factories.rb b/spec/factories/sample_manifest_factories.rb
index 00be33ee6b..bedff1ca32 100644
--- a/spec/factories/sample_manifest_factories.rb
+++ b/spec/factories/sample_manifest_factories.rb
@@ -78,8 +78,9 @@
     factory :pending_plate_sample_manifest do
       transient do
         num_plates { 2 }
-        num_samples_per_plate { 2 }
-        plates { create_list :plate, num_plates, well_factory: :empty_well, well_count: num_samples_per_plate }
+        num_filled_wells_per_plate { 2 }
+        num_rows_per_well { 1 }
+        plates { create_list :plate, num_plates, well_factory: :empty_well, well_count: num_filled_wells_per_plate }
       end
 
       barcodes { plates.map(&:human_barcode) }
@@ -89,7 +90,9 @@
         evaluator
           .plates
           .flat_map(&:wells)
-          .each { |well| create(:sample_manifest_asset, asset: well, sample_manifest: sample_manifest) }
+          .each do |well|
+            evaluator.num_rows_per_well.times { create(:sample_manifest_asset, asset: well, sample_manifest: sample_manifest) }
+          end
       end
     end
 
diff --git a/spec/models/sample_manifest/uploader_spec.rb b/spec/models/sample_manifest/uploader_spec.rb
index 7487f6f7ba..c0a17acbc1 100644
--- a/spec/models/sample_manifest/uploader_spec.rb
+++ b/spec/models/sample_manifest/uploader_spec.rb
@@ -20,42 +20,44 @@
 
   after { File.delete(test_file_name) if File.exist?(test_file_name) }
 
-  it 'will not be valid without a filename' do
-    expect(described_class.new(nil, SampleManifestExcel.configuration, user, false)).not_to be_valid
-  end
+  describe '#initialize' do
+    it 'will not be valid without a filename' do
+      expect(described_class.new(nil, SampleManifestExcel.configuration, user, false)).not_to be_valid
+    end
 
-  it 'will not be valid without some configuration' do
-    download =
-      build(
-        :test_download_tubes,
-        manifest_type: 'tube_library_with_tag_sequences',
-        columns: SampleManifestExcel.configuration.columns.tube_library_with_tag_sequences.dup
-      )
-    download.save(test_file_name)
-    expect(described_class.new(test_file, nil, user, false)).not_to be_valid
-  end
+    it 'will not be valid without some configuration' do
+      download =
+        build(
+          :test_download_tubes,
+          manifest_type: 'tube_library_with_tag_sequences',
+          columns: SampleManifestExcel.configuration.columns.tube_library_with_tag_sequences.dup
+        )
+      download.save(test_file_name)
+      expect(described_class.new(test_file, nil, user, false)).not_to be_valid
+    end
 
-  it 'will not be valid without a tag group' do
-    SampleManifestExcel.configuration.tag_group = nil
-    download =
-      build(
-        :test_download_tubes,
-        manifest_type: 'tube_library_with_tag_sequences',
-        columns: SampleManifestExcel.configuration.columns.tube_library_with_tag_sequences.dup
-      )
-    download.save(test_file_name)
-    expect(described_class.new(test_file, SampleManifestExcel.configuration, user, false)).not_to be_valid
-    SampleManifestExcel.configuration.tag_group = 'My Magic Tag Group'
-  end
+    it 'will not be valid without a tag group' do
+      SampleManifestExcel.configuration.tag_group = nil
+      download =
+        build(
+          :test_download_tubes,
+          manifest_type: 'tube_library_with_tag_sequences',
+          columns: SampleManifestExcel.configuration.columns.tube_library_with_tag_sequences.dup
+        )
+      download.save(test_file_name)
+      expect(described_class.new(test_file, SampleManifestExcel.configuration, user, false)).not_to be_valid
+      SampleManifestExcel.configuration.tag_group = 'My Magic Tag Group'
+    end
 
-  it 'will not be valid without a user' do
-    download =
-      build(
-        :test_download_tubes,
-        columns: SampleManifestExcel.configuration.columns.tube_library_with_tag_sequences.dup
-      )
-    download.save(test_file_name)
-    expect(described_class.new(test_file, SampleManifestExcel.configuration, nil, false)).not_to be_valid
+    it 'will not be valid without a user' do
+      download =
+        build(
+          :test_download_tubes,
+          columns: SampleManifestExcel.configuration.columns.tube_library_with_tag_sequences.dup
+        )
+      download.save(test_file_name)
+      expect(described_class.new(test_file, SampleManifestExcel.configuration, nil, false)).not_to be_valid
+    end
   end
 
   context 'when checking uploads' do
@@ -168,7 +170,7 @@
         build(
           :test_download_plates,
           num_plates: number_of_plates,
-          num_samples_per_plate: samples_per_plate,
+          num_filled_wells_per_plate: samples_per_plate,
           manifest_type: 'plate_full',
           columns: SampleManifestExcel.configuration.columns.plate_full.dup,
           study: create(:open_study, accession_number: 'acc')
@@ -302,5 +304,42 @@
       uploader.run!
       expect(uploader).to be_processed
     end
+
+    context 'pools plate manifest' do
+      let(:uploader) { described_class.new(test_file, SampleManifestExcel.configuration, user, false) }
+
+      before do
+        # create a test manifest file with 2 plates, 2 wells per plate, and 2 rows per well
+        download = build(:test_download_plates, num_rows_per_well: 2, columns: SampleManifestExcel.configuration.columns.pools_plate.dup)
+        download.save(test_file_name)
+        Delayed::Worker.delay_jobs = false
+        uploader.run!
+      end
+
+      it 'will upload the manifest' do
+        expect(uploader).to be_processed
+      end
+
+      it 'will understand there is one pool per well' do
+        expect(uploader.upload.sample_manifest.pools).not_to be_nil
+        expect(uploader.upload.sample_manifest.pools.count).to eq(4) # one pool per well
+      end
+
+      it 'will set tag_depth on aliquots' do
+        labware = uploader.upload.sample_manifest.labware
+        expect(labware.count).to eq(2)
+
+        labware.each do |plate|
+          wells = plate.wells
+          expect(wells.count).to eq(2)
+
+          wells.each do |well|
+            aliquots = well.aliquots
+            # within a well, each aliquot should have its own tag_depth
+            expect(aliquots.map { |w| w.tag_depth }.uniq.count).to eq(aliquots.count)
+          end
+        end
+      end
+    end
   end
 end
diff --git a/spec/sample_manifest_excel/worksheet_spec.rb b/spec/sample_manifest_excel/worksheet_spec.rb
index 0c0b7bd512..7910a82b6b 100644
--- a/spec/sample_manifest_excel/worksheet_spec.rb
+++ b/spec/sample_manifest_excel/worksheet_spec.rb
@@ -598,7 +598,7 @@ def save_file
         count: 1,
         type: 'Plates',
         num_plates: 2,
-        num_samples_per_plate: 3,
+        num_filled_wells_per_plate: 3,
         manifest_type: 'plate_default'
       }
     end
@@ -615,7 +615,7 @@ def save_file
 
       it 'last row should be correct' do
         expect(worksheet.last_row).to eq(
-          worksheet.first_row + (attributes[:num_plates] * attributes[:num_samples_per_plate]) - 1
+          worksheet.first_row + (attributes[:num_plates] * attributes[:num_filled_wells_per_plate]) - 1
         )
       end
 

From 72eef8ccb7807b082fd2958a28ae24da7b671867 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Tue, 26 Mar 2024 08:48:06 +0000
Subject: [PATCH 17/22] prettier

---
 spec/factories/sample_manifest_factories.rb  | 4 +++-
 spec/models/sample_manifest/uploader_spec.rb | 8 +++++++-
 spec/models/sample_manifest_spec.rb          | 7 ++++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/spec/factories/sample_manifest_factories.rb b/spec/factories/sample_manifest_factories.rb
index bedff1ca32..21b2e041f7 100644
--- a/spec/factories/sample_manifest_factories.rb
+++ b/spec/factories/sample_manifest_factories.rb
@@ -91,7 +91,9 @@
           .plates
           .flat_map(&:wells)
           .each do |well|
-            evaluator.num_rows_per_well.times { create(:sample_manifest_asset, asset: well, sample_manifest: sample_manifest) }
+            evaluator.num_rows_per_well.times do
+              create(:sample_manifest_asset, asset: well, sample_manifest: sample_manifest)
+            end
           end
       end
     end
diff --git a/spec/models/sample_manifest/uploader_spec.rb b/spec/models/sample_manifest/uploader_spec.rb
index c0a17acbc1..15cc03c02e 100644
--- a/spec/models/sample_manifest/uploader_spec.rb
+++ b/spec/models/sample_manifest/uploader_spec.rb
@@ -310,7 +310,12 @@
 
       before do
         # create a test manifest file with 2 plates, 2 wells per plate, and 2 rows per well
-        download = build(:test_download_plates, num_rows_per_well: 2, columns: SampleManifestExcel.configuration.columns.pools_plate.dup)
+        download =
+          build(
+            :test_download_plates,
+            num_rows_per_well: 2,
+            columns: SampleManifestExcel.configuration.columns.pools_plate.dup
+          )
         download.save(test_file_name)
         Delayed::Worker.delay_jobs = false
         uploader.run!
@@ -335,6 +340,7 @@
 
           wells.each do |well|
             aliquots = well.aliquots
+
             # within a well, each aliquot should have its own tag_depth
             expect(aliquots.map { |w| w.tag_depth }.uniq.count).to eq(aliquots.count)
           end
diff --git a/spec/models/sample_manifest_spec.rb b/spec/models/sample_manifest_spec.rb
index 1ceaf1dbd5..b951a69760 100644
--- a/spec/models/sample_manifest_spec.rb
+++ b/spec/models/sample_manifest_spec.rb
@@ -348,7 +348,12 @@
   end
 
   describe '#pools' do
-    let(:manifest) { create :plate_sample_manifest_with_manifest_assets, study: study, asset_type: 'plate', num_samples_per_well: num_samples_per_well }
+    let(:manifest) do
+      create :plate_sample_manifest_with_manifest_assets,
+             study: study,
+             asset_type: 'plate',
+             num_samples_per_well: num_samples_per_well
+    end
 
     context 'when there is only one sample per well' do
       let(:num_samples_per_well) { 1 }

From 72470d3cafedef85e011566dd1820798e540213e Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Tue, 26 Mar 2024 08:57:16 +0000
Subject: [PATCH 18/22] rubocop

---
 .../sample_manifest_excel/worksheet/test_worksheet.rb        | 5 ++---
 spec/models/sample_manifest/uploader_spec.rb                 | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb b/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb
index ddac847632..8b0e7ebbc7 100644
--- a/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb
+++ b/app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb
@@ -21,10 +21,9 @@ class TestWorksheet < SequencescapeExcel::Worksheet::Base # rubocop:todo Metrics
                     :partial,
                     :cgap,
                     :num_plates,
-                    :num_filled_wells_per_plate,
-                    :num_rows_per_well
+                    :num_filled_wells_per_plate
       attr_reader :dynamic_attributes, :tags, :study
-      attr_writer :manifest_type
+      attr_writer :manifest_type, :num_rows_per_well
 
       def initialize(attributes = {}) # rubocop:todo Metrics/MethodLength
         super
diff --git a/spec/models/sample_manifest/uploader_spec.rb b/spec/models/sample_manifest/uploader_spec.rb
index 15cc03c02e..6d468650ab 100644
--- a/spec/models/sample_manifest/uploader_spec.rb
+++ b/spec/models/sample_manifest/uploader_spec.rb
@@ -305,7 +305,7 @@
       expect(uploader).to be_processed
     end
 
-    context 'pools plate manifest' do
+    context 'with a pools plate manifest' do
       let(:uploader) { described_class.new(test_file, SampleManifestExcel.configuration, user, false) }
 
       before do
@@ -342,7 +342,7 @@
             aliquots = well.aliquots
 
             # within a well, each aliquot should have its own tag_depth
-            expect(aliquots.map { |w| w.tag_depth }.uniq.count).to eq(aliquots.count)
+            expect(aliquots.map(&:tag_depth).uniq.count).to eq(aliquots.count)
           end
         end
       end

From 4f7be9218803476d264308b3c45bff8f69912874 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Tue, 26 Mar 2024 09:10:18 +0000
Subject: [PATCH 19/22] Tidy comments

---
 app/models/sample_manifest.rb | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/app/models/sample_manifest.rb b/app/models/sample_manifest.rb
index c38ee25346..01464768a6 100644
--- a/app/models/sample_manifest.rb
+++ b/app/models/sample_manifest.rb
@@ -129,11 +129,10 @@ def default_filename
     "#{study_id}stdy_manifest_#{id}_#{created_at.to_formatted_s(:dmy)}"
   end
 
-  # Number of rows per well in the manifest.
-  # Specified in the manifest_types.yml config.
-  # Used to pre-populate the spreadsheet with a sufficient number of rows per well,
-  # in the case where we are importing a plate containing pools, and we want to provide
-  # sample-level information for the pools.
+  # Number of rows per well in the manifest file, specified in manifest_types.yml.
+  # Used to pre-populate the spreadsheet with a sufficient number of rows,
+  # in the case where we are importing a plate containing pools,
+  # and want to provide sample-level information for each pool.
   # Developed initially for the scRNA Core pipeline.
   #
   # Uses a default value of 1 if not set.
@@ -143,18 +142,17 @@ def rows_per_well
 
   # Used in manifest upload code to determine if pools are present,
   # so that tag_depth can be set on the aliquots if needed.
-  # Checks if at least one receptacle has more than one sample manifest asset.
-  # Sample manifest assets are a join table between sample manifest and receptacle,
-  # created upfront when a manifest is generated.
   #
-  # Returns a hash, where key is receptacle and value is array of sample manifest assets.
+  # Returns a hash of receptacle to array of sample manifest assets.
   # Returns nil if all receptacles only contain 0 or 1 sample.
   def pools
     @pools ||=
       begin
+        # Sample manifest assets are a join table between sample manifests and samples,
+        # created upfront when the manifest is generated.
+        # Here we use them to see how many samples are in each receptacle.
         receptacle_to_smas = sample_manifest_assets.group_by(&:asset)
 
-        # if all receptacles only contain 0 or 1 sample, there are no pools so return nil
         receptacle_to_smas.values.all? { |smas| smas.size <= 1 } ? nil : receptacle_to_smas
       end
   end

From f3c676006f55f5d9125074843bcb42d11b9b6a90 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Tue, 26 Mar 2024 09:11:41 +0000
Subject: [PATCH 20/22] tidy up more comments

---
 app/models/sample_manifest/core_behaviour.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/sample_manifest/core_behaviour.rb b/app/models/sample_manifest/core_behaviour.rb
index b5c78b7e2a..0e22d429c5 100644
--- a/app/models/sample_manifest/core_behaviour.rb
+++ b/app/models/sample_manifest/core_behaviour.rb
@@ -34,9 +34,9 @@ def details(&block)
   # The samples get registered in the stock resource table at the end of manifest upload and processing
   # (It used to happen here)
   module StockAssets
-    # Used in manifest upload code to insert the sample and aliquot the database.
+    # Used in manifest upload code to insert the sample and aliquot into the database.
     # The receptacle and sanger_sample_id already exist as they are inserted upfront when the manifest is generated.
-    # tag_depth is set on the aliquot if pools are present, and if the samples are not tagged, to avoid tag clash.
+    # tag_depth is set on the aliquot to avoid tag clash if a) pools are present, and b) if the samples are not tagged.
     # The assumption is made that samples passed to the below method are never tagged,
     # because we're in the 'StockAssets' module rather than the 'LibraryAssets' module.
     def generate_sample_and_aliquot(sanger_sample_id, receptacle)

From bc645172bb1ee4a65cbcedfd94775fed20faba56 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Tue, 26 Mar 2024 09:14:11 +0000
Subject: [PATCH 21/22] Unskip tests, because we've enabled the rows_per_well
 feature

---
 spec/models/sample_manifest/generator_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/models/sample_manifest/generator_spec.rb b/spec/models/sample_manifest/generator_spec.rb
index 8adcb1c38c..32d6a976f0 100644
--- a/spec/models/sample_manifest/generator_spec.rb
+++ b/spec/models/sample_manifest/generator_spec.rb
@@ -128,7 +128,7 @@
   context 'with rows_per_well set' do
     let(:template) { 'pools_plate' }
 
-    skip 'generates a details array with more than one entry per well' do
+    it 'generates a details array with more than one entry per well' do
       generator = described_class.new(attributes, user, configuration)
       generator.execute
       expect(generator.sample_manifest.details_array.size).to eq(4 * 96 * 2)
@@ -136,7 +136,7 @@
   end
 
   context 'with rows_per_well not set' do
-    skip 'generates a details array with one entry per well' do
+    it 'generates a details array with one entry per well' do
       generator = described_class.new(attributes, user, configuration)
       generator.execute
       expect(generator.sample_manifest.details_array.size).to eq(4 * 96)

From bb858ab6c3833a6f9fc255e797dff9a016e4baf3 Mon Sep 17 00:00:00 2001
From: Katy Taylor <kt17@sanger.ac.uk>
Date: Wed, 3 Apr 2024 15:24:43 +0100
Subject: [PATCH 22/22] document one of the tests

---
 spec/models/sample_manifest/uploader_spec.rb | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/spec/models/sample_manifest/uploader_spec.rb b/spec/models/sample_manifest/uploader_spec.rb
index 6d468650ab..83a30e20c5 100644
--- a/spec/models/sample_manifest/uploader_spec.rb
+++ b/spec/models/sample_manifest/uploader_spec.rb
@@ -296,6 +296,9 @@
       expect(uploader).to be_processed
     end
 
+    # The manifest is generated for 2 plates, with 2 wells each, with 1 sample per well.
+    # The test_download_plates_partial factory leaves the last 2 rows of the manifest empty.
+    # So 2 samples are uploaded for the first plate, and 0 samples for the second plate.
     it 'will upload a valid partial plate sample manifest' do
       download = build(:test_download_plates_partial, columns: SampleManifestExcel.configuration.columns.plate_full.dup)
       download.save(test_file_name)