diff --git a/.release-version b/.release-version index a63a66348d..f817227823 100644 --- a/.release-version +++ b/.release-version @@ -1 +1 @@ -14.43.2 +14.43.3 diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index 031a599eb0..7fb4b9ce50 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -21,16 +21,8 @@ class CherrypickTask < Task # rubocop:todo Metrics/ClassLength # # @return [CherrypickTask::ControlLocator] A generator of control locations # - - 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] - ) + 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:) end # @@ -46,7 +38,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, template) do + perform_pick(requests, robot, control_source_plate, workflow_controller) do target_type.new(template, plate_purpose.try(:asset_shape)) end end @@ -62,7 +54,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, template) do + perform_pick(requests, robot, control_source_plate, workflow_controller) do target_type .new(template, purpose.try(:asset_shape), partial_plate) .tap do @@ -74,7 +66,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, template) # rubocop:todo Metrics/AbcSize + def perform_pick(requests, robot, control_source_plate, workflow_controller) # rubocop:todo Metrics/AbcSize max_plates = robot.max_beds raise StandardError, 'The chosen robot has no beds!' if max_plates.zero? @@ -88,22 +80,7 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller, tem num_plate = 0 batch = requests.first.batch control_assets = control_source_plate.wells.joins(:samples) - 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_locator = new_control_locator(batch.id, current_destination_plate.size, control_assets.count) 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 diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 2811dc3b89..91313fb121 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -29,34 +29,23 @@ 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, - :control_source_plate + attr_reader :batch_id, :total_wells, :wells_to_leave_free, :num_control_wells, :available_positions # @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 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] + # @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 end # @@ -66,7 +55,7 @@ def initialize(params) # @param num_plate [Integer] The plate number within the batch # # @return [Array] 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 @@ -76,25 +65,9 @@ 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. - - 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) + seed = seed_for(num_plate) + initial_positions = random_positions_from_available(seed) + control_positions_for_plate(num_plate, initial_positions) end private @@ -123,49 +96,6 @@ 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 diff --git a/app/views/workflows/_plate_template_batches.html.erb b/app/views/workflows/_plate_template_batches.html.erb index e3fbeb9b77..dac8d76df3 100644 --- a/app/views/workflows/_plate_template_batches.html.erb +++ b/app/views/workflows/_plate_template_batches.html.erb @@ -19,15 +19,8 @@ <%= select(:plate_template, "0", @plate_templates.map { |pt| [pt.name, pt.id ] }, {}, class: 'form-control select2') %>

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.

- - <%= 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') %> + + <%= select("Control", "plate_id", ControlPlate.all.collect {|p| [ "#{p.human_barcode} - #{p.name}", p.id ] }, { include_blank: true }, class: 'form-control select2') %> diff --git a/spec/factories/plate_factories.rb b/spec/factories/plate_factories.rb index 455d2b9a3f..a700925935 100644 --- a/spec/factories/plate_factories.rb +++ b/spec/factories/plate_factories.rb @@ -242,16 +242,6 @@ 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? diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index ee13130967..37cd0645f1 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -3,16 +3,7 @@ require 'rails_helper' RSpec.describe CherrypickTask::ControlLocator do - let(:instance) do - described_class.new( - batch_id: batch_id, - total_wells: total_wells, - num_control_wells: num_control_wells, - wells_to_leave_free: wells_to_leave_free, - control_source_plate: create(:control_plate), - template: create(:plate_template_with_well) - ) - end + let(:instance) { described_class.new(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:) } shared_examples 'an invalid ControlLocator' do |plate_number, error = 'More controls than free wells'| it 'throws a "More controls than free wells" exception' do @@ -59,11 +50,6 @@ this_plate.each_with_index { |position, index| expect(position).not_to be_within(5).of(previous_plate[index]) } end end - - it 'uses a control plate that is valid' do - placement_type = instance.send(:control_placement_type) - expect(placement_type.nil? || %w[fixed random].exclude?(placement_type)).to be_falsey - end end # Control positions will be our only public method, sand perhaps some attr_readers @@ -140,16 +126,7 @@ let(:range) { (1...1000) } let(:control_positions) do range.map do |batch_id| - described_class - .new( - batch_id: batch_id, - total_wells: 96, - num_control_wells: 1, - control_source_plate: create(:control_plate), - template: create(:plate_template) - ) - .control_positions(0) - .first + described_class.new(batch_id: batch_id, total_wells: 96, num_control_wells: 1).control_positions(0).first end end @@ -168,66 +145,5 @@ expect(tally.values).to all be_between(2, 25) end end - - context 'when the control placement type is not valid' do - let(:batch_id) { 1 } - let(:total_wells) { 96 } - let(:num_control_wells) { 2 } - let(:wells_to_leave_free) { [] } - - before { allow(instance).to receive(:control_placement_type).and_return('invalid_type') } - - it 'raises an error about invalid placement type' do - expect { instance.control_positions(0) }.to raise_error( - StandardError, - 'Control placement type is not set or is invalid' - ) - end - end - - context 'when the control plate and plate template are incompatible' do - let(:batch_id) { 1 } - let(:total_wells) { 96 } - let(:num_control_wells) { 2 } - let(:wells_to_leave_free) { [] } - - before do - allow(instance).to receive_messages( - control_placement_type: 'fixed', - convert_assets: [1, 2, 3], - control_source_plate: create(:control_plate) - ) - end - - it 'returns and displays an error message' do - expect(instance.handle_incompatible_plates).to be_truthy - end - end - - context 'when assets are converted using the maps' do - let(:batch_id) { 1 } - let(:total_wells) { 96 } - let(:num_control_wells) { 2 } - let(:wells_to_leave_free) { [] } - - before { allow(instance).to receive_messages(control_placement_type: 'fixed') } - - it 'they are given the correct position IDs' do - expect(instance.send(:convert_assets, [94, 95, 96])).to eq([79, 87, 95]) - end - end - - context 'when the control placement type is fixed' do - let(:batch_id) { 1 } - let(:total_wells) { 96 } - let(:num_control_wells) { 2 } - let(:wells_to_leave_free) { [] } - - before { allow(instance).to receive_messages(control_placement_type: 'fixed') } - - it 'passes as intended' do - expect(instance.send(:fixed_positions_from_available)).to eq([]) - end - end end end diff --git a/spec/models/tasks/cherrypick_task_spec.rb b/spec/models/tasks/cherrypick_task_spec.rb index 97e9f98796..887de3e81f 100644 --- a/spec/models/tasks/cherrypick_task_spec.rb +++ b/spec/models/tasks/cherrypick_task_spec.rb @@ -36,11 +36,8 @@ def requests_for_plate(plate) batch_id: 1235, total_wells: 6, num_control_wells: 2, - wells_to_leave_free: wells_to_leave_free, - control_source_plate: control_plate, - template: template + wells_to_leave_free: wells_to_leave_free ).and_return(locator) - allow(locator).to receive(:handle_incompatible_plates) end let(:instance) { described_class.new } @@ -92,7 +89,6 @@ def requests_for_plate(plate) locator = instance_double(CherrypickTask::ControlLocator) allow(locator).to receive(:control_positions).and_return([2, 5], [0, 2]) allow(CherrypickTask::ControlLocator).to receive(:new).and_return(locator) - allow(locator).to receive(:handle_incompatible_plates) end it 'places controls in a different position' do @@ -121,7 +117,6 @@ def requests_for_plate(plate) before do locator = instance_double(CherrypickTask::ControlLocator, control_positions: [2, 3]) allow(CherrypickTask::ControlLocator).to receive(:new).and_return(locator) - allow(locator).to receive(:handle_incompatible_plates) end let(:instance) { described_class.new } @@ -150,7 +145,6 @@ def requests_for_plate(plate) locator = instance_double(CherrypickTask::ControlLocator) allow(locator).to receive(:control_positions).and_return([2, 4], [0, 2]) allow(CherrypickTask::ControlLocator).to receive(:new).and_return(locator) - allow(locator).to receive(:handle_incompatible_plates) end let(:instance) { described_class.new }