Merge pull request #4050 from sanger/DPL-823-upload

DPL-823 [Part 2 - upload] Import PBMC pool plates into Sequencescape

app/models/sample_manifest.rb

@@ -129,11 +129,32 @@ 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 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.
   def rows_per_well
-    1
-    # TODO: replace above line with below line to turn the rows_per_well feature on, when DPL-823 is complete
-    # @rows_per_well || 1
+    @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.
+  #
+  # 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)
+
+        receptacle_to_smas.values.all? { |smas| smas.size <= 1 } ? nil : receptacle_to_smas
+      end
   end
 
   scope :pending_manifests,

app/models/sample_manifest/core_behaviour.rb

@@ -34,19 +34,36 @@ 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 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 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)
       create_sample(sanger_sample_id).tap do |sample|
-        receptacle.aliquots.create!(sample: sample, study: study)
+        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
 
     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
+
+      pools[receptacle].find_index { |sma| sma.sanger_sample_id == sanger_sample_id }
+    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)
       create_sample(sanger_sample_id).tap do |sample|

app/sample_manifest_excel/sample_manifest_excel/worksheet/test_worksheet.rb

@@ -21,9 +21,9 @@ class TestWorksheet < SequencescapeExcel::Worksheet::Base # rubocop:todo Metrics
                     :partial,
                     :cgap,
                     :num_plates,
-                    :num_samples_per_plate
+                    :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
@@ -60,7 +60,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 +88,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 +330,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

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
       )

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
@@ -75,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) }
@@ -86,7 +90,11 @@
         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 do
+              create(:sample_manifest_asset, asset: well, sample_manifest: sample_manifest)
+            end
+          end
       end
     end
 

spec/models/sample_manifest/generator_spec.rb

@@ -128,17 +128,15 @@
   context 'with rows_per_well set' do
     let(:template) { 'pools_plate' }
 
-    # TODO: un-skip when rows_per_well feature is enabled (when DPL-823 is complete)
-    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)
     end
   end
 
   context 'with rows_per_well not set' do
-    # TODO: un-skip when rows_per_well feature is enabled (when DPL-823 is complete)
-    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)

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')
@@ -294,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)
@@ -302,5 +307,48 @@
       uploader.run!
       expect(uploader).to be_processed
     end
+
+    context 'with a 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(&:tag_depth).uniq.count).to eq(aliquots.count)
+          end
+        end
+      end
+    end
   end
 end