Merge pull request #4393 from sanger/develop

Develop into master

.release-version

@@ -1 +1 @@
-14.43.1
+14.43.2

app/models/asset_link.rb

@@ -76,4 +76,81 @@ def has_#{name}?
       end
     end
   end
+
+  # Creates an edge between the ancestor and descendant nodes using save.
+  #
+  # This method first attempts to find an existing link between the ancestor
+  # and descendant. If no link is found, it builds a new edge and saves it.
+  # If a link is found, it makes the link an edge and saves it.
+  #
+  # This method is overridden to handle race conditions in finding an
+  # existing link and has_duplicates validation. It also assumes that there
+  # is a unique-together index on ancestor_id and descendant_id columns.
+  #
+  # @param ancestor [Dag::Standard::EndPoint] The ancestor node.
+  # @param descendant [Dag::Standard::EndPoint] The descendant node.
+  # @return [Boolean] Returns true if the edge is successfully created or
+  #   already exists, false otherwise.
+  # @raise [ActiveRecord::RecordNotUnique] Re-raises any exception if it is
+  #   not a constraint violation that involves ancestor_id and descendant_id
+  #   columns.
+  def self.create_edge(ancestor, descendant)
+    # Two processes try to find an existing link.
+    link = find_link(ancestor, descendant)
+    # Either or both may find no link and try to create a new edge.
+    if link.nil?
+      edge = build_edge(ancestor, descendant)
+      result = save_edge_or_handle_error(edge)
+      return result unless result.nil? # Bubble up.
+      # Losing process finds the edge created by the winning process.
+      link = find_link(ancestor, descendant)
+    end
+
+    return if link.nil?
+
+    link.make_direct
+    link.changed? ? link.save : true
+  end
+
+  # Saves the edge between the ancestor and descendant nodes or handles errors.
+  #
+  # @param edge [AssetLink] The edge object containing the errors.
+  # @return [Boolean] Returns true if the edge is successfully saved,
+  #   nil if the error is unique validation or constraint violation,
+  #   false if the error is another validation error.
+  # @raise [ActiveRecord::RecordNotUnique] Re-raises an exception if the
+  #   exception caught is not a unique constraint violation.
+  def self.save_edge_or_handle_error(edge)
+    # Winning process successfully saves the edge (direct link).
+    return true if edge.save
+    # has_duplicate validation may see it for the losing process before
+    # hitting the DB.
+    return false unless unique_validation_error?(edge) # Bubble up.
+    edge.errors.clear # Clear all errors and use the existing link.
+  rescue ActiveRecord::RecordNotUnique => e
+    # Unique constraint violation is triggered for the losing process after
+    # hitting the DB.
+    raise unless unique_violation_error?(edge, e) # Bubble up.
+  end
+
+  # Checks if the validation error includes a specific message indicating a
+  # unique link already exists.
+  #
+  # @param edge [AssetLink] The edge object containing the errors.
+  # @return [Boolean] Returns true if the errors include the message "Link
+  #   already exists between these points", false otherwise.
+  def self.unique_validation_error?(edge)
+    edge.errors[:base].include?('Link already exists between these points')
+  end
+
+  # Checks if the unique constraint violation involves the specified columns.
+  #
+  # @param edge [AssetLink] The edge object containing the column names.
+  # @param exception [ActiveRecord::RecordNotUnique] The exception raised due
+  #   to the unique constraint violation.
+  # @return [Boolean] Returns true if the exception message includes both the
+  #   ancestor and descendant column names, false otherwise.
+  def self.unique_violation_error?(edge, exception)
+    [edge.ancestor_id_column_name, edge.descendant_id_column_name].all? { |col| exception.message.include?(col) }
+  end
 end

app/models/barcode/format_handlers.rb

@@ -578,4 +578,11 @@ class IbdResponse < BaseRegExBarcode
   class Rvi < BaseRegExBarcode
     self.format = /\A(?<prefix>RVI)-(?<number>[0-9]{6,})\z/
   end
+
+  #  add AkerBarcode class here as it could be called in
+  #  Barcode::FormatHandlers.const_get in app/models/barcode.rb to avoid
+  #  uninitialized constant Barcode::FormatHandlers::AkerBarcode (NameError)
+  class AkerBarcode < BaseRegExBarcode
+    self.format = /\A(?<prefix>Aker)-(?<number>[0-9]{2,})\z/
+  end
 end

app/models/cherrypick_task.rb

@@ -21,8 +21,16 @@ class CherrypickTask < Task # rubocop:todo Metrics/ClassLength
   #
   # @return [CherrypickTask::ControlLocator] A generator of control locations
   #
-  def new_control_locator(batch_id, total_wells, num_control_wells, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE)
-    CherrypickTask::ControlLocator.new(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:)
+
+  def new_control_locator(params)
+    CherrypickTask::ControlLocator.new(
+      batch_id: params[:batch_id],
+      total_wells: params[:total_wells],
+      num_control_wells: params[:num_control_wells],
+      wells_to_leave_free: params[:wells_to_leave_free] || DEFAULT_WELLS_TO_LEAVE_FREE,
+      control_source_plate: params[:control_source_plate],
+      template: params[:template]
+    )
   end
 
   #
@@ -38,7 +46,7 @@ def can_link_directly?
   # rubocop:todo Metrics/ParameterLists
   def pick_new_plate(requests, template, robot, plate_purpose, control_source_plate = nil, workflow_controller = nil)
     target_type = PickTarget.for(plate_purpose)
-    perform_pick(requests, robot, control_source_plate, workflow_controller) do
+    perform_pick(requests, robot, control_source_plate, workflow_controller, template) do
       target_type.new(template, plate_purpose.try(:asset_shape))
     end
   end
@@ -54,7 +62,7 @@ def pick_onto_partial_plate(
     purpose = partial_plate.plate_purpose
     target_type = PickTarget.for(purpose)
 
-    perform_pick(requests, robot, control_source_plate, workflow_controller) do
+    perform_pick(requests, robot, control_source_plate, workflow_controller, template) do
       target_type
         .new(template, purpose.try(:asset_shape), partial_plate)
         .tap do
@@ -66,7 +74,7 @@ def pick_onto_partial_plate(
   # rubocop:enable Metrics/ParameterLists
 
   # rubocop:todo Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength
-  def perform_pick(requests, robot, control_source_plate, workflow_controller) # rubocop:todo Metrics/AbcSize
+  def perform_pick(requests, robot, control_source_plate, workflow_controller, template) # rubocop:todo Metrics/AbcSize
     max_plates = robot.max_beds
     raise StandardError, 'The chosen robot has no beds!' if max_plates.zero?
 
@@ -80,7 +88,22 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller) # r
       num_plate = 0
       batch = requests.first.batch
       control_assets = control_source_plate.wells.joins(:samples)
-      control_locator = new_control_locator(batch.id, current_destination_plate.size, control_assets.count)
+      control_locator =
+        new_control_locator(
+          {
+            batch_id: batch.id,
+            total_wells: current_destination_plate.size,
+            num_control_wells: control_assets.count,
+            template: template,
+            control_source_plate: control_source_plate
+          }
+        )
+
+      if control_locator.handle_incompatible_plates
+        message = 'The control plate and plate template are incompatible'
+        workflow_controller.send(:flash)[:error] = message unless workflow_controller.nil?
+        workflow_controller.redirect_to action: 'stage', batch_id: batch.id, workflow_id: workflow.id
+      end
       control_posns = control_locator.control_positions(num_plate)
 
       # If is an incomplete plate, or a plate with a template applied, copy all the controls missing into the

app/models/cherrypick_task/control_locator.rb

@@ -29,23 +29,34 @@ class CherrypickTask::ControlLocator
   # limit ourself to primes to simplify validation.
   BETWEEN_PLATE_OFFSETS = [53, 59].freeze
 
-  attr_reader :batch_id, :total_wells, :wells_to_leave_free, :num_control_wells, :available_positions
+  attr_reader :batch_id,
+              :total_wells,
+              :wells_to_leave_free,
+              :num_control_wells,
+              :available_positions,
+              :control_source_plate
 
   # @note wells_to_leave_free was originally hardcoded for 96 well plates at 24, in order to avoid
   # control wells being missed in cDNA quant QC. This requirement was removed in
   # https://github.com/sanger/sequencescape/issues/2967 however I've avoided stripping out the behaviour
   # completely in case controls are used in other pipelines.
   #
-  # @param batch_id [Integer] The id of the batch, used to generate a starting position
-  # @param total_wells [Integer] The total number of wells on the plate
-  # @param num_control_wells [Integer] The number of control wells to lay out
-  # @param wells_to_leave_free [Enumerable] Array or range indicating the wells to leave free from controls
-  def initialize(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free: [])
-    @batch_id = batch_id
-    @total_wells = total_wells
-    @num_control_wells = num_control_wells
-    @wells_to_leave_free = wells_to_leave_free.to_a
-    @available_positions = (0...total_wells).to_a - @wells_to_leave_free
+  # @param params [Hash] A hash containing the following keys:
+  #   - :batch_id [Integer] The id of the batch, used to generate a starting position
+  #   - :total_wells [Integer] The total number of wells on the plate
+  #   - :num_control_wells [Integer] The number of control wells to lay out
+  #   - :wells_to_leave_free [Enumerable] Array or range indicating the wells to leave free from controls
+  #   - :control_source_plate [ControlPlate] The plate to source controls from
+  #   - :template [PlateTemplate] The template of the destination plate
+
+  def initialize(params)
+    @batch_id = params[:batch_id]
+    @total_wells = params[:total_wells]
+    @num_control_wells = params[:num_control_wells]
+    @wells_to_leave_free = params[:wells_to_leave_free].to_a || []
+    @available_positions = (0...@total_wells).to_a - @wells_to_leave_free
+    @control_source_plate = params[:control_source_plate]
+    @plate_template = params[:template]
   end
 
   #
@@ -55,7 +66,7 @@ def initialize(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:
   # @param num_plate [Integer] The plate number within the batch
   #
   # @return [Array<Integer>] The indexes of the control well positions
-  #
+
   def control_positions(num_plate)
     raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions
 
@@ -65,9 +76,25 @@ def control_positions(num_plate)
 
     # If num plate is equal to the available positions, the cycle is going to be repeated.
     # To avoid it, every num_plate=available_positions we start a new cycle with a new seed.
-    seed = seed_for(num_plate)
-    initial_positions = random_positions_from_available(seed)
-    control_positions_for_plate(num_plate, initial_positions)
+
+    placement_type = control_placement_type
+    if placement_type.nil? || %w[fixed random].exclude?(placement_type)
+      raise StandardError, 'Control placement type is not set or is invalid'
+    end
+
+    handle_control_placement_type(placement_type, num_plate)
+  end
+
+  def handle_incompatible_plates
+    return false if control_placement_type == 'random'
+    return false if @plate_template.wells.empty?
+
+    control_assets = control_source_plate.wells.joins(:samples)
+
+    converted_control_assets = convert_assets(control_assets.map(&:map_id))
+    converted_template_assets = convert_assets(@plate_template.wells.map(&:map_id))
+
+    converted_control_assets.intersect?(converted_template_assets)
   end
 
   private
@@ -96,6 +123,49 @@ def random_positions_from_available(seed)
     available_positions.sample(num_control_wells, random: Random.new(seed))
   end
 
+  def control_placement_type
+    @control_source_plate.custom_metadatum_collection.metadata['control_placement_type']
+  end
+
+  def handle_control_placement_type(placement_type, num_plate)
+    if placement_type == 'random'
+      control_positions_for_plate(num_plate, random_positions_from_available(seed_for(num_plate)))
+    else
+      fixed_positions_from_available
+    end
+  end
+
+  # Because the control source plate wells are ordered inversely to the destination plate wells,
+  # the control asset ids need to be converted to the corresponding destination plate well indexes.
+
+  def convert_assets(control_assets)
+    valid_map, invalid_map = create_plate_maps
+
+    control_assets.map do |id|
+      invalid_location = valid_map[id]
+      invalid_map.key(invalid_location) - 1
+    end
+  end
+
+  def fixed_positions_from_available
+    control_assets = @control_source_plate.wells.joins(:samples)
+    control_wells = control_assets.map(&:map_id)
+    convert_assets(control_wells)
+  end
+
+  # The invalid and valid maps are hash maps to represent a plate that maps A1 -> 1, A2 -> 2, etc,
+  # whereas the other map is the inverse of this, mapping 1 -> A1, 2 -> B1, etc.
+
+  def create_plate_maps
+    rows = ('A'..'H').to_a
+    columns = (1..12).to_a
+
+    valid_map = rows.product(columns).each_with_index.to_h { |(row, col), i| [i + 1, "#{row}#{col}"] }
+    invalid_map = columns.product(rows).each_with_index.to_h { |(col, row), i| [i + 1, "#{row}#{col}"] }
+
+    [valid_map, invalid_map]
+  end
+
   # Works out which offset to use based on the number of available wells and ensures we use
   # all wells before looping. Will select the first suitable value from BETWEEN_PLATE_OFFSETS
   # excluding any numbers that are a factor of the available wells. In the incredibly unlikely

app/views/workflows/_plate_template_batches.html.erb

@@ -19,8 +19,15 @@
             <%= select(:plate_template, "0", @plate_templates.map { |pt| [pt.name, pt.id ] }, {}, class: 'form-control select2')  %>
             <p class='help-block'>Templates define which wells to leave empty on a plate when you cherrypick samples. You can add to an existing partial plate by scanning the barcode, or entering the plate ID.  The plate must have been previously picked in Sequencescape.  Wells can be rearranged in the next step.</p>
 
-            <label for="Control_plate_id">Control plate</label>
-            <%= select("Control", "plate_id", ControlPlate.all.collect {|p| [ "#{p.human_barcode} - #{p.name}", p.id ] }, { include_blank: true }, class: 'form-control select2') %>
+            <label for="Control_plate_id">Control plate & placement type</label>
+            <%= select("Control", "plate_id", 
+              ControlPlate.all.collect do |p| 
+                placement_type = p.custom_metadatum_collection&.metadata&.[]('control_placement_type')
+                if placement_type.present?
+                  [ "#{p.human_barcode} - #{p.name} (#{placement_type.capitalize})", p.id ]
+                end
+              end.compact, 
+              { include_blank: true }, class: 'form-control select2') %>
           </fieldset>
         </div>
 

db/migrate/20240912000109_add_unique_index_to_asset_links.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+# This migration adds a unique-together index on ancestor_id and descendant_id
+# in order to prevent duplicate links between the same ancestor and descendant
+# labware.
+#
+# Before this migration, the database allowed duplicate asset_links between the
+# same ancestor and descendant labware. Therefore, the migration will fail if
+# there are any duplicate links in the database. To fix this, they must be
+# removed before running the migration using the rake task:
+#
+# bundle exec rake 'support:remove_duplicate_asset_links[csv_file_path]'
+#
+# The rake task will write the removed records into a CSV file that can be used
+# for auditing purposes.
+#
+# Note that the column names in the index name below is used for finding the
+# reason of the database unique constraint violation by the AssetLink model.
+class AddUniqueIndexToAssetLinks < ActiveRecord::Migration[6.1]
+  def change
+    add_index :asset_links,
+              %i[ancestor_id descendant_id],
+              unique: true,
+              name: 'index_asset_links_on_ancestor_id_and_descendant_id'
+  end
+end

db/schema.rb

@@ -115,6 +115,7 @@
     t.integer "count"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.index ["ancestor_id", "descendant_id"], name: "index_asset_links_on_ancestor_id_and_descendant_id", unique: true
     t.index ["ancestor_id", "direct"], name: "index_asset_links_on_ancestor_id_and_direct"
     t.index ["descendant_id", "direct"], name: "index_asset_links_on_descendant_id_and_direct"
   end

spec/factories/plate_factories.rb

@@ -242,6 +242,16 @@
     transient { well_factory { :untagged_well } }
 
     after(:create) do |plate, _evaluator|
+      custom_metadatum = CustomMetadatum.new
+      custom_metadatum.key = 'control_placement_type'
+      custom_metadatum.value = 'random'
+      custom_metadatum_collection = CustomMetadatumCollection.new
+      custom_metadatum_collection.custom_metadata = [custom_metadatum]
+      custom_metadatum_collection.asset = plate
+      custom_metadatum_collection.user = User.new(id: 1)
+      custom_metadatum_collection.save!
+      custom_metadatum.save!
+
       plate.wells.each_with_index do |well, index|
         next if well.aliquots.empty?