From 8d6d55e41c4f233391bf9356699f3d31e974f728 Mon Sep 17 00:00:00 2001 From: yoldas Date: Thu, 12 Sep 2024 09:59:21 +0100 Subject: [PATCH 01/82] Add migration for unique-together index on ancestor and descendant --- ...2000109_add_unique_index_to_asset_links.rb | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 db/migrate/20240912000109_add_unique_index_to_asset_links.rb diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb new file mode 100644 index 0000000000..631d249400 --- /dev/null +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'csv' +# 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 recovery if necessary. +# +# If the migration is rolled back, the index will be removed. The duplicate +# records removed before can be restored from a CSV using the rake task: +# +# bundle exec rake 'support:restore_removed_asset_links[csv_file_path]' +# +class AddUniqueIndexToAssetLinks < ActiveRecord::Migration[6.1] + def change + add_index :asset_links, [:ancestor_id, :descendant_id], unique: true, name: 'index_asset_links_on_ancestor_and_descendant' + end +end From 5896e32b06bda9e97b091be368a2624fcb303e4a Mon Sep 17 00:00:00 2001 From: yoldas Date: Thu, 12 Sep 2024 16:27:25 +0100 Subject: [PATCH 02/82] Applied migration to db schema --- db/migrate/20240912000109_add_unique_index_to_asset_links.rb | 5 ++++- db/schema.rb | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index 631d249400..09c3e418a2 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -22,6 +22,9 @@ # class AddUniqueIndexToAssetLinks < ActiveRecord::Migration[6.1] def change - add_index :asset_links, [:ancestor_id, :descendant_id], unique: true, name: 'index_asset_links_on_ancestor_and_descendant' + add_index :asset_links, + %i[ancestor_id descendant_id], + unique: true, + name: 'index_asset_links_on_ancestor_and_descendant' end end diff --git a/db/schema.rb b/db/schema.rb index 207c08263d..647e3fd66b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2024_08_13_130010) do +ActiveRecord::Schema.define(version: 2024_09_12_000109) do create_table "aliquot_indices", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.integer "aliquot_id", null: false @@ -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_and_descendant", 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 From b9661b027b299cfc88d1504ca5305842efdfe265 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:35:33 +0100 Subject: [PATCH 03/82] Pass through control assets to control locator --- app/models/cherrypick_task.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index 13304cb801..d957874547 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -21,12 +21,13 @@ 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) + def new_control_locator(batch_id, total_wells, num_control_wells, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE, control_assets: nil) CherrypickTask::ControlLocator.new( batch_id: batch_id, total_wells: total_wells, num_control_wells: num_control_wells, - wells_to_leave_free: wells_to_leave_free + wells_to_leave_free: wells_to_leave_free, + control_assets: control_assets ) end @@ -85,7 +86,8 @@ 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, current_destination_plate.size, control_assets.count, control_assets:) 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 @@ -176,7 +178,7 @@ def build_plate_wells_from_requests(requests, workflow_controller = nil) # ruboc [ barcodes_sorted_by_location.index(request.asset.plate.human_barcode), request.asset.plate.id, - request.asset.map.column_order + request.asset.map.column_order, ] end From 8a58ffd19117c646265e9e8ac7122578b7685468 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:36:00 +0100 Subject: [PATCH 04/82] Add function to inversely populate control wells on the destination plate --- app/models/cherrypick_task/control_locator.rb | 65 +++++++++++++++++-- 1 file changed, 61 insertions(+), 4 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 91313fb121..c5d990e039 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -29,7 +29,7 @@ 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_assets # @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 @@ -40,12 +40,13 @@ class CherrypickTask::ControlLocator # @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: []) + def initialize(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free: [], control_assets:) @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 + @control_assets = control_assets end # @@ -66,8 +67,12 @@ 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) + + # initial_positions = random_positions_from_available(seed) + + initial_positions = fixed_positions_from_available + + #control_positions_for_plate(num_plate, initial_positions) end private @@ -89,6 +94,7 @@ def control_positions_for_plate(num_plate, initial_positions) initial_positions.map do |pos| available_positions[(available_positions.index(pos) + offset) % total_available_positions] + end end @@ -96,6 +102,57 @@ def random_positions_from_available(seed) available_positions.sample(num_control_wells, random: Random.new(seed)) end + def convert_control_assets(control_assets) + rows = ('A'..'H').to_a + columns = (1..12).to_a + + + valid_map = {} + map_id = 1 + + rows.each do |row| + columns.each do |col| + valid_map[map_id] = "#{row}#{col}" + map_id += 1 + end + end + + + rows = ('A'..'H').to_a + columns = (1..12).to_a + + + invalid_map = {} + map_id = 1 + + columns.each do |col| + rows.each do |row| + invalid_map[map_id] = "#{row}#{col}" + map_id += 1 + end + end + converted_assets = [] + control_assets.map do |id| + # Find the location on the invalid grid + invalid_location = valid_map[id] + + # Find the corresponding map_id on the valid grid + binding.pry + converted_assets.push(invalid_map.key(invalid_location) - 1) + + end + converted_assets + + end + + def fixed_positions_from_available + wells = control_assets.map { |asset| asset.map_id } + converted_assets = convert_control_assets(wells) + puts converted_assets + converted_assets + 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 From 4e9934dddbaa88f4a15e158b6a4a1e09374fec10 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:58:31 +0100 Subject: [PATCH 05/82] Rubocop --- app/models/cherrypick_task.rb | 6 ++- app/models/cherrypick_task/control_locator.rb | 46 ++++--------------- 2 files changed, 12 insertions(+), 40 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index d957874547..cdeb164712 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -21,7 +21,8 @@ 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, control_assets: nil) + def new_control_locator(batch_id, total_wells, num_control_wells, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE, +control_assets: nil) CherrypickTask::ControlLocator.new( batch_id: batch_id, total_wells: total_wells, @@ -87,7 +88,8 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller) # r 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_assets:) + control_locator = new_control_locator(batch.id, current_destination_plate.size, control_assets.count, +control_assets:) 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 c5d990e039..92a016bfbd 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -40,7 +40,7 @@ class CherrypickTask::ControlLocator # @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: [], control_assets:) + def initialize(batch_id:, total_wells:, num_control_wells:, control_assets:, wells_to_leave_free: []) @batch_id = batch_id @total_wells = total_wells @num_control_wells = num_control_wells @@ -66,11 +66,11 @@ 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) + seed_for(num_plate) # initial_positions = random_positions_from_available(seed) - initial_positions = fixed_positions_from_available + fixed_positions_from_available #control_positions_for_plate(num_plate, initial_positions) end @@ -105,50 +105,20 @@ def random_positions_from_available(seed) def convert_control_assets(control_assets) rows = ('A'..'H').to_a columns = (1..12).to_a - - - valid_map = {} - map_id = 1 - rows.each do |row| - columns.each do |col| - valid_map[map_id] = "#{row}#{col}" - map_id += 1 - end - end - - - rows = ('A'..'H').to_a - columns = (1..12).to_a - - - invalid_map = {} - map_id = 1 + 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}"] } - columns.each do |col| - rows.each do |row| - invalid_map[map_id] = "#{row}#{col}" - map_id += 1 - end - end - converted_assets = [] control_assets.map do |id| - # Find the location on the invalid grid invalid_location = valid_map[id] - - # Find the corresponding map_id on the valid grid - binding.pry - converted_assets.push(invalid_map.key(invalid_location) - 1) - + invalid_map.key(invalid_location) - 1 end - converted_assets - end def fixed_positions_from_available - wells = control_assets.map { |asset| asset.map_id } + wells = control_assets.map(&:map_id) converted_assets = convert_control_assets(wells) - puts converted_assets + Rails.logger.debug converted_assets converted_assets end From 2a76fb5b8de3605807a7b5f966e7dd6f274b0a3c Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Thu, 19 Sep 2024 14:06:45 +0100 Subject: [PATCH 06/82] Prettier --- app/models/cherrypick_task.rb | 15 ++++++++++----- app/models/cherrypick_task/control_locator.rb | 12 +++++------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index cdeb164712..54b28b02c8 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -21,8 +21,13 @@ 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, -control_assets: nil) + def new_control_locator( + batch_id, + total_wells, + num_control_wells, + wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE, + control_assets: nil + ) CherrypickTask::ControlLocator.new( batch_id: batch_id, total_wells: total_wells, @@ -88,8 +93,8 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller) # r 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_assets:) + control_locator = + new_control_locator(batch.id, current_destination_plate.size, control_assets.count, control_assets:) 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 @@ -180,7 +185,7 @@ def build_plate_wells_from_requests(requests, workflow_controller = nil) # ruboc [ barcodes_sorted_by_location.index(request.asset.plate.human_barcode), request.asset.plate.id, - request.asset.map.column_order, + request.asset.map.column_order ] end diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 92a016bfbd..84284a0b76 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -67,7 +67,7 @@ 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_for(num_plate) - + # initial_positions = random_positions_from_available(seed) fixed_positions_from_available @@ -94,7 +94,6 @@ def control_positions_for_plate(num_plate, initial_positions) initial_positions.map do |pos| available_positions[(available_positions.index(pos) + offset) % total_available_positions] - end end @@ -105,24 +104,23 @@ def random_positions_from_available(seed) def convert_control_assets(control_assets) 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}"] } - + control_assets.map do |id| invalid_location = valid_map[id] invalid_map.key(invalid_location) - 1 end end - + def fixed_positions_from_available wells = control_assets.map(&:map_id) converted_assets = convert_control_assets(wells) Rails.logger.debug converted_assets converted_assets 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 From 6c3ef2c6338ef324726e24271588d4fca30b4c29 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Thu, 19 Sep 2024 16:22:36 +0100 Subject: [PATCH 07/82] Added dev notes for converstion function --- app/models/cherrypick_task/control_locator.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 84284a0b76..fbce59d537 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -101,6 +101,9 @@ def random_positions_from_available(seed) available_positions.sample(num_control_wells, random: Random.new(seed)) 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_control_assets(control_assets) rows = ('A'..'H').to_a columns = (1..12).to_a @@ -115,10 +118,8 @@ def convert_control_assets(control_assets) end def fixed_positions_from_available - wells = control_assets.map(&:map_id) - converted_assets = convert_control_assets(wells) - Rails.logger.debug converted_assets - converted_assets + control_wells = control_assets.map(&:map_id) + convert_control_assets(control_wells) end # Works out which offset to use based on the number of available wells and ensures we use From ab1f587025415ae8f53978e8bdf5933fd8d049ac Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 02:46:48 +0100 Subject: [PATCH 08/82] Include column names in the unique index --- db/migrate/20240912000109_add_unique_index_to_asset_links.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index 09c3e418a2..a5639500e5 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -20,11 +20,13 @@ # # bundle exec rake 'support:restore_removed_asset_links[csv_file_path]' # +# 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_and_descendant' + name: 'index_asset_links_on_ancestor_id_and_descendant_id' end end From 775d85eecb334d0c90dcd5c177fb2a7c71d81e46 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 02:56:00 +0100 Subject: [PATCH 09/82] Apply migration to schema --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 647e3fd66b..e5f088aa24 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -115,7 +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_and_descendant", unique: true + 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 From 1b0fb4e41008f281ae2416eb683610a21438af17 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 02:59:26 +0100 Subject: [PATCH 10/82] Override create_edge on asset_links to handle race conditions --- app/models/asset_link.rb | 73 ++++ .../asset_links_race_conditions_spec.rb | 346 ++++++++++++++++++ 2 files changed, 419 insertions(+) create mode 100644 spec/models/asset_links_race_conditions_spec.rb diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index c3191408d7..d1c98adf90 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -76,4 +76,77 @@ 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] Raises an exception if the unique + # constraint violation does not involve the expected 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) + return true if save_edge_or_handle_error(edge) + # Losing process finds the edge created by the winning process. + link = find_link(ancestor, descendant) + end + + 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, false + # otherwise. + def self.save_edge_or_handle_error(edge) + begin + # 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 + false + 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 diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb new file mode 100644 index 0000000000..f79465ac32 --- /dev/null +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -0,0 +1,346 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Exception raised when a socket operation times out. +class SocketTimeoutError < StandardError +end + +# Exception raised when a process operation times out. +class ProcessTimeoutError < StandardError +end + +# Exception raised when a process exits with a non-zero status. +class ProcessStatusError < StandardError +end + +RSpec.describe AssetLink, type: :model do + describe '.create_edge' do + # Used in IPC when one end of the duplex pipe is waiting for the other end + # to send a message with a timeout in seconds. + # + # @param socket [Socket] The socket to wait for. + # @param length [Integer] The length of the message to wait for. + # @param timeout [Integer] The timeout in seconds, default is 10. + # @param message [String] The message to raise if times out, default is 'socket timeout'. + # @return [String] The message received from the socket. + # @raise SocketTimeoutError + + def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket timeout') + raise SocketTimeoutError, message unless socket.wait_readable(timeout) + socket.recv(length) + end + + # Used to reap a child process with a timeout in seconds. + # + # @param pid [Integer] The process ID to wait for. + # @param timeout [Integer] The timeout in seconds, default is 10. + # @param message [String] The message to raise if times out, default is 'process timeout'. + # @return [Process::Status] The status of the process. + # @raise ProcessTimeoutError + # rubocop:disable Metrics/MethodLength + def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') + start_time = Time.zone.now + loop do + begin + pid2, status = Process.waitpid2(pid, Process::WNOHANG) + return status if pid2 + rescue Errno::ECHILD # No child process + return nil + end + if Time.zone.now - start_time > timeout + begin + Process.kill('TERM', pid) # Send TERM signal. + sleep 1 # Wait for the process to terminate. + Process.kill('KILL', pid) # Send KILL signal. + Process.waitpid(pid) # Reap + rescue Errno::ECHILD + # No child process + end + raise ProcessTimeoutError, message + end + sleep 0.1 + end + end + # rubocop:enable Metrics/MethodLength + + # Wait for the given processes to finish with a timeout in seconds. + # @param pids [Array] The process IDs to wait for. + # @return [void] + def wait_for_processes(pids) + pids.each do |pid| + Process.waitpid(pid) + # status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") + # raise ProcessStatusError, + # "Forked process #{pid} failed with exit status #{status.exitstatus}" if status.exitstatus != 0 + end + end + + # rubocop:disable RSpec/ExampleLength + it 'handles race condition at find_link' do + # In this example, the first and second processes are forked from the + # main process and they are in race condition to find an existing link + # between the ancestor and descendant and create an edge if no link is + # found. Both first and second processes finds no link, but the second + # process creates the edge first. The first also tries to create the + # edge based on the result of the find_link method. However, it must + # not be able to create it. + + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + # Create a duplex pipe for inter-process communication. + first_socket, second_socket = Socket.pair(:UNIX, :STREAM) + + # Fork the first process. + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + + call_count = 0 # Track the calls to the find_link method. + + # Patch the find_link method in the first process. + allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| + call_count += 1 + link = method.call(*args) + if call_count == 1 + expect(link).to be_nil # The first process should not find any link initally. + + signal = 'paused' + first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. + + # Wait for the second process to send 'resume'. + wait_readable_with_timeout( + first_socket, + 'resume'.length, + 10, + 'Timeout waiting for the second process to send resume.' + ) + elsif call_count == 2 + # The first process now should find the link created by the second process. + expect(link).not_to be_nil + end + link + end + + described_class.create_edge(ancestor, descendant) + ActiveRecord::Base.connection.close + end + + # Fork the second process. + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. + + # Wait for the first process to signal that it is paused after calling find_link. + wait_readable_with_timeout( + second_socket, + 'paused'.length, + 10, + 'Timeout waiting for the first process to send paused.' + ) + + described_class.create_edge(ancestor, descendant) + # Signal the first process to resume now. Although it has found no link + # in its last method call, actually there is one now. + signal = 'resume' + second_socket.send(signal, 0) # Zero flags. + ActiveRecord::Base.connection.close + end + + # Wait for both processes to finish and check their exit statuses + wait_for_processes([pid1, pid2]) + + # Check that the edge was created as expected. + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) + + ActiveRecord::Base.connection.close + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it 'handles race condition at has_duplicates' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the has_duplicates validation. + # Failing process should use the existing link. + + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + # Create a duplex pipe for inter-process communication. + first_socket, second_socket = Socket.pair(:UNIX, :STREAM) + + # Fork the first process. + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + call_count = 0 # Track the calls to the find_link method. + # Patch the find_link method in the first process. + # Check link, trigger the second process, pause, and then wait for'resume' signal. + allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| + call_count += 1 + link = method.call(*args) + if call_count == 1 + expect(link).to be_nil # The first process should not find any link initally. + signal = 'paused' + first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. + # Wait for the second process to send 'resume'. + wait_readable_with_timeout( + first_socket, + 'resume'.length, + 10, + 'Timeout waiting for the second process to send resume.' + ) + end + link + end + # Now the first process should be prevented from saving because of has_duplicates validation. + result = described_class.create_edge(ancestor, descendant) + expect(result).to be_truthy + expect(described_class).to have_received(:unique_validation_error?) + + ActiveRecord::Base.connection.close + end + + # Fork the second process. + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + # Wait for the first process to signal that it is paused after calling find_link. + wait_readable_with_timeout( + second_socket, + 'paused'.length, + 10, + 'Timeout waiting for the first process to send paused.' + ) + described_class.create_edge(ancestor, descendant) + signal = 'resume' + second_socket.send(signal, 0) # Zero flags. + ActiveRecord::Base.connection.close + end + + # Wait for both processes to finish and check their exit statuses + wait_for_processes([pid1, pid2]) + + # Check that the edge was created as expected. + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) # How many different paths between the nodes. + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it 'handles unique constraint violation' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the unique constraint + # violation. Failing process should use the existing link. + + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + # Create a duplex pipe for inter-process communication. + first_socket, second_socket = Socket.pair(:UNIX, :STREAM) + + # Fork the first process. + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + call_count = 0 # Track the calls to the find_link method. + # Patch the find_link method in the first process. + # Check link, trigger the second process, pause, and then wait for'resume' signal. + allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| + call_count += 1 + link = method.call(*args) + if call_count == 1 + expect(link).to be_nil # The first process should not find any link initally. + signal = 'paused' + first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. + # Wait for the second process to send 'resume'. + wait_readable_with_timeout( + first_socket, + 'resume'.length, + 10, + 'Timeout waiting for the second process to send resume.' + ) + end + link + end + # Validation error is not triggered in this case because the other + # process has not saved the link yet. Therefore, the save call will + # pass the validations and hit the database to trigger the unique + # constraint violation just after the other process saves the link. + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) + # rubocop:enable RSpec/AnyInstance + + allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| + edge = args[0] + message = + "Duplicate entry '#{ancestor.id}-#{descendant.id}' " \ + "for key 'index_asset_links_on_ancestor_id_and_descendant_id'" + exception = ActiveRecord::RecordNotUnique.new(message) + allow(edge).to receive(:save).and_raise(exception) + + method.call(*args) + end + + described_class.create_edge(ancestor, descendant) + expect(described_class).to have_received(:unique_violation_error?) + ActiveRecord::Base.connection.close + end + + # Fork the second process. + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. + + # Wait for the first process to signal that it is paused after calling find_link. + wait_readable_with_timeout( + second_socket, + 'paused'.length, + 10, + 'Timeout waiting for the first process to send paused.' + ) + + described_class.create_edge(ancestor, descendant) + # Signal the first process to resume now. Although it has found no link + # in its last method call, actually there is one now. + signal = 'resume' + second_socket.send(signal, 0) # Zero flags. + ActiveRecord::Base.connection.close + end + + # Wait for both processes to finish and check their exit statuses + wait_for_processes([pid1, pid2]) + + # Check that the edge was created as expected. + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) + + ActiveRecord::Base.connection.close + end + # rubocop:enable RSpec/ExampleLength + end +end From d720c8e9b4f24721ad8532a37fdc04262b10f674 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 18:20:48 +0100 Subject: [PATCH 11/82] Bubble up non-duplication errors --- app/models/asset_link.rb | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index d1c98adf90..dd175b6468 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -99,7 +99,8 @@ def self.create_edge(ancestor, descendant) # Either or both may find no link and try to create a new edge. if link.nil? edge = build_edge(ancestor, descendant) - return true if save_edge_or_handle_error(edge) + 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 @@ -114,19 +115,16 @@ def self.create_edge(ancestor, descendant) # @return [Boolean] Returns true if the edge is successfully saved, false # otherwise. def self.save_edge_or_handle_error(edge) - begin - # 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 - false + # 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 From 6b9b83d3530e00a5cc928e11fe37968cbfbfe010 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 21:14:27 +0100 Subject: [PATCH 12/82] Remove custom exception class definitions from test --- .../asset_links_race_conditions_spec.rb | 36 ++++++++----------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index f79465ac32..5f96890a3a 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -2,20 +2,10 @@ require 'rails_helper' -# Exception raised when a socket operation times out. -class SocketTimeoutError < StandardError -end - -# Exception raised when a process operation times out. -class ProcessTimeoutError < StandardError -end - -# Exception raised when a process exits with a non-zero status. -class ProcessStatusError < StandardError -end - RSpec.describe AssetLink, type: :model do describe '.create_edge' do + # Helpers + # Used in IPC when one end of the duplex pipe is waiting for the other end # to send a message with a timeout in seconds. # @@ -24,10 +14,9 @@ class ProcessStatusError < StandardError # @param timeout [Integer] The timeout in seconds, default is 10. # @param message [String] The message to raise if times out, default is 'socket timeout'. # @return [String] The message received from the socket. - # @raise SocketTimeoutError - + # @raise StandardError def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket timeout') - raise SocketTimeoutError, message unless socket.wait_readable(timeout) + raise StandardError, message unless socket.wait_readable(timeout) socket.recv(length) end @@ -37,7 +26,7 @@ def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket t # @param timeout [Integer] The timeout in seconds, default is 10. # @param message [String] The message to raise if times out, default is 'process timeout'. # @return [Process::Status] The status of the process. - # @raise ProcessTimeoutError + # @raise StandardError # rubocop:disable Metrics/MethodLength def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') start_time = Time.zone.now @@ -57,7 +46,7 @@ def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') rescue Errno::ECHILD # No child process end - raise ProcessTimeoutError, message + raise StandardError, message end sleep 0.1 end @@ -69,13 +58,15 @@ def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') # @return [void] def wait_for_processes(pids) pids.each do |pid| - Process.waitpid(pid) - # status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") - # raise ProcessStatusError, - # "Forked process #{pid} failed with exit status #{status.exitstatus}" if status.exitstatus != 0 + status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") + if status.exitstatus != 0 + raise StandardError, "Forked process #{pid} failed with exit status #{status.exitstatus}" + end end end + # Examples + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the @@ -205,6 +196,8 @@ def wait_for_processes(pids) end link end + allow(described_class).to receive(:unique_validation_error?).and_call_original + # Now the first process should be prevented from saving because of has_duplicates validation. result = described_class.create_edge(ancestor, descendant) expect(result).to be_truthy @@ -301,6 +294,7 @@ def wait_for_processes(pids) method.call(*args) end + allow(described_class).to receive(:unique_violation_error?).and_call_original described_class.create_edge(ancestor, descendant) expect(described_class).to have_received(:unique_violation_error?) From 480e3d368437428faa5967f9b305aadecaa079f7 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 21:28:31 +0100 Subject: [PATCH 13/82] Update comment about return values --- app/models/asset_link.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index dd175b6468..1def9ab0c4 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -91,8 +91,9 @@ def has_#{name}? # @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] Raises an exception if the unique - # constraint violation does not involve the expected columns. + # @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) @@ -112,8 +113,11 @@ def self.create_edge(ancestor, descendant) # 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, false - # otherwise. + # @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 From dc6a704698d0d89489e80eaa681d062548eb8a35 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 18:20:51 +0100 Subject: [PATCH 14/82] Add guard against nil before converting link to edge --- app/models/asset_link.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 1def9ab0c4..8267b761f6 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -106,8 +106,10 @@ def self.create_edge(ancestor, descendant) link = find_link(ancestor, descendant) end - link.make_direct - link.changed? ? link.save : true + unless link.nil? + link.make_direct + link.changed? ? link.save : true + end end # Saves the edge between the ancestor and descendant nodes or handles errors. From 80d377ad34bcba93b783fe9e3589711ee753b318 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 18:26:14 +0100 Subject: [PATCH 15/82] Replace the recover proecedure with auditing as it is wrong to put duplicate records back into the table. --- .../20240912000109_add_unique_index_to_asset_links.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index a5639500e5..13a495bbaa 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -13,12 +13,7 @@ # 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 recovery if necessary. -# -# If the migration is rolled back, the index will be removed. The duplicate -# records removed before can be restored from a CSV using the rake task: -# -# bundle exec rake 'support:restore_removed_asset_links[csv_file_path]' +# 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. From bac2c98d498e5a1954abbd24301b014120aca34b Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 18:29:41 +0100 Subject: [PATCH 16/82] Rubocop --- app/models/asset_link.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 8267b761f6..837d25080d 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -106,10 +106,10 @@ def self.create_edge(ancestor, descendant) link = find_link(ancestor, descendant) end - unless link.nil? - link.make_direct - link.changed? ? link.save : true - 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. From 03124f48d84171c9f36903c3a5e878ae84b4e185 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 19:07:09 +0100 Subject: [PATCH 17/82] Clean the database after tests because of the new DB connections in the main and forked child processes --- spec/models/asset_links_race_conditions_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 5f96890a3a..b208c639c8 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -67,6 +67,11 @@ def wait_for_processes(pids) # Examples + # We need to explicitly clean the database after examples here because we + # are establishing new database connections in the main and forked child + # processes. + after { DatabaseCleaner.clean_with(:truncation) } + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the From dee9a7e549ede3a6ba397bf29e209002ba584236 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 20:42:26 +0100 Subject: [PATCH 18/82] Removed after block --- spec/models/asset_links_race_conditions_spec.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index b208c639c8..5f96890a3a 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -67,11 +67,6 @@ def wait_for_processes(pids) # Examples - # We need to explicitly clean the database after examples here because we - # are establishing new database connections in the main and forked child - # processes. - after { DatabaseCleaner.clean_with(:truncation) } - # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the From c4a13a86f43ccbeea0086eecb0535560b7dfd455 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 20:54:18 +0100 Subject: [PATCH 19/82] Add after all cleanup --- spec/models/asset_links_race_conditions_spec.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 5f96890a3a..1513b305fc 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -67,6 +67,15 @@ def wait_for_processes(pids) # Examples + # We need to explicitly clean the database after examples here because we + # are establishing new database connections in the main and forked child + # processes. + # rubocop:disable RSpec/BeforeAfterAll + after(:all) do + DatabaseCleaner.clean_with(:truncation) + end + # rubocop:enable RSpec/BeforeAfterAll + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the From 165dd5eeeb1b911332a8accc135a91c6ccd86331 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 21:15:31 +0100 Subject: [PATCH 20/82] Delete test asset link and labware records --- spec/models/asset_links_race_conditions_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 1513b305fc..3604f812e0 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -72,7 +72,8 @@ def wait_for_processes(pids) # processes. # rubocop:disable RSpec/BeforeAfterAll after(:all) do - DatabaseCleaner.clean_with(:truncation) + AssetLink.delete_all + Labware.delete_all end # rubocop:enable RSpec/BeforeAfterAll From 6f50b2c37938da7df0550bafc254ee83ee1777c8 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 21:33:06 +0100 Subject: [PATCH 21/82] Debug side effect --- spec/models/asset_links_race_conditions_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 3604f812e0..cc804dcce8 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -RSpec.describe AssetLink, type: :model do +RSpec.xdescribe AssetLink, type: :model do describe '.create_edge' do # Helpers From f04d1cf2210fbd8049aaa0d42ddc1c8d5cc8998a Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 21:45:47 +0100 Subject: [PATCH 22/82] Debug 2 --- app/models/asset_link.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 837d25080d..5f75a4fc28 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -94,7 +94,7 @@ def has_#{name}? # @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) + def self.xcreate_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. From bc857a28f633703deec1b708cf52df48ddf7fef5 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 22:04:47 +0100 Subject: [PATCH 23/82] Revert debug --- app/models/asset_link.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 5f75a4fc28..837d25080d 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -94,7 +94,7 @@ def has_#{name}? # @raise [ActiveRecord::RecordNotUnique] Re-raises any exception if it is # not a constraint violation that involves ancestor_id and descendant_id # columns. - def self.xcreate_edge(ancestor, descendant) + 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. From 0aec8882965d0b6cb858492f937349d6dc64a65b Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 23:55:47 +0100 Subject: [PATCH 24/82] Debug handles race condition at find_link --- spec/models/asset_links_create_edge_spec.rb | 79 +++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 spec/models/asset_links_create_edge_spec.rb diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb new file mode 100644 index 0000000000..a432a6a54c --- /dev/null +++ b/spec/models/asset_links_create_edge_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe AssetLink, type: :model do + describe '.create_edge' do + # rubocop:disable RSpec/ExampleLength + it 'handles race condition at find_link' do + # Parent + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + first_socket, second_socket = UNIXSocket.pair + + # First child + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + ancestor = Labware.find(ancestor.id) + descendant = Labware.find(descendant.id) + find_link_call_count = 0 + allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| + find_link_call_count += 1 + link = m.call(*args) + if find_link_call_count == 1 + expect(link).to be_nil + message = 'paused' + first_socket.send(message, 0) + message = 'child1: Timeout waiting for resume message' + raise StandardError, message unless first_socket.wait_readable(10) + message = 'resume' + first_socket.recv(message.size) + elsif find_link_call_count == 2 + expect(link).not_to be_nil + end + link + end + described_class.create_edge(ancestor, descendant) + ActiveRecord::Base.connection.close + end + + # Second child + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + ancestor = Labware.find(ancestor.id) + descendant = Labware.find(descendant.id) + message = 'child2: Timeout waiting for paused message' + raise StandardError, message unless second_socket.wait_readable(10) + message = 'paused' + second_socket.recv(message.size) + described_class.create_edge(ancestor, descendant) + message = 'resume' + second_socket.send(message, 0) + ActiveRecord::Base.connection.close + end + + # Parent + [pid1, pid2].each + .with_index(1) do |pid, index| + Timeout.timeout(10) { Process.waitpid(pid) } + rescue Timeout::Error + raise StandardError, "parent: Timeout waiting for child#{index} to finish" + end + + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) + + ActiveRecord::Base.connection.close + end + # rubocop:enable RSpec/ExampleLength + end +end From a69265a32a5ccd364593d8fabd40df1e75a013cf Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 00:18:18 +0100 Subject: [PATCH 25/82] Debug2 handles race condition at find_link --- spec/models/asset_links_create_edge_spec.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index a432a6a54c..7ba45792c7 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -4,12 +4,18 @@ RSpec.describe AssetLink, type: :model do describe '.create_edge' do + + after do + @ancestor.destroy if @ancestor + @descendant.destroy if @descendant + end + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # Parent ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) + @ancestor = ancestor = create(:labware) + @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction first_socket, second_socket = UNIXSocket.pair From 4ebd702b5a0f35588f642bcafd400eb0d69e3547 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 01:31:39 +0100 Subject: [PATCH 26/82] Rewrite test for unique validation error --- spec/models/asset_links_create_edge_spec.rb | 111 +++++++++++++++++--- 1 file changed, 96 insertions(+), 15 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index 7ba45792c7..c94cb2689f 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -3,14 +3,14 @@ require 'rails_helper' RSpec.describe AssetLink, type: :model do + # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable describe '.create_edge' do - after do - @ancestor.destroy if @ancestor - @descendant.destroy if @descendant + @ancestor&.destroy + @descendant&.destroy + @edge&.destroy end - # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # Parent ActiveRecord::Base.connection.reconnect! @@ -24,8 +24,6 @@ pid1 = fork do ActiveRecord::Base.connection.reconnect! - ancestor = Labware.find(ancestor.id) - descendant = Labware.find(descendant.id) find_link_call_count = 0 allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| find_link_call_count += 1 @@ -44,6 +42,7 @@ link end described_class.create_edge(ancestor, descendant) + ActiveRecord::Base.connection.close end @@ -51,8 +50,6 @@ pid2 = fork do ActiveRecord::Base.connection.reconnect! - ancestor = Labware.find(ancestor.id) - descendant = Labware.find(descendant.id) message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) message = 'paused' @@ -64,22 +61,106 @@ end # Parent + exits = [] [pid1, pid2].each .with_index(1) do |pid, index| - Timeout.timeout(10) { Process.waitpid(pid) } + Timeout.timeout(10) do + _pid, status = Process.wait2(pid) + exits << status.exitstatus + end rescue Timeout::Error raise StandardError, "parent: Timeout waiting for child#{index} to finish" end + expect(exits).to eq([0, 0]) + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) + @edge = edge = described_class.last + expect(edge.ancestor).to eq(ancestor) + expect(edge.descendant).to eq(descendant) + expect(edge.direct).to be_truthy + expect(edge.count).to eq(1) ActiveRecord::Base.connection.close end - # rubocop:enable RSpec/ExampleLength + + it 'handles unique validation error' do + # Parent + ActiveRecord::Base.connection.reconnect! + @ancestor = ancestor = create(:labware) + @descendant = descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + first_socket, second_socket = UNIXSocket.pair + + # First child + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + find_link_call_count = 0 + allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| + find_link_call_count += 1 + link = m.call(*args) + if find_link_call_count == 1 + expect(link).to be_nil + message = 'paused' + first_socket.send(message, 0) + message = 'child1: Timeout waiting for resume message' + raise StandardError, message unless first_socket.wait_readable(10) + message = 'resume' + first_socket.recv(message.size) + end + link + end + + unique_validation_error_return_value = nil + allow(described_class).to receive(:unique_validation_error?).and_wrap_original do |m, *args| + unique_validation_error_return_value = m.call(*args) + end + + result = described_class.create_edge(ancestor, descendant) + expect(result).to be_truthy + expect(described_class).to have_received(:unique_validation_error?) + expect(unique_validation_error_return_value).to be_truthy + + ActiveRecord::Base.connection.close + end + + # Second child + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + message = 'child2: Timeout waiting for paused message' + raise StandardError, message unless second_socket.wait_readable(10) + message = 'paused' + second_socket.recv(message.size) + described_class.create_edge(ancestor, descendant) + message = 'resume' + second_socket.send(message, 0) + ActiveRecord::Base.connection.close + end + + # Parent + exits = [] + [pid1, pid2].each + .with_index(1) do |pid, index| + Timeout.timeout(10) do + _pid, status = Process.wait2(pid) + exits << status.exitstatus + end + rescue Timeout::Error + raise StandardError, "parent: Timeout waiting for child#{index} to finish" + end + + expect(exits).to eq([0, 0]) + + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + @edge = edge = described_class.last + expect(edge.ancestor).to eq(ancestor) + expect(edge.descendant).to eq(descendant) + expect(edge.direct).to be_truthy + expect(edge.count).to eq(1) + end end + # rubocop:enable RSpec/ExampleLength,RSpec/InstanceVariable end From 9ea98ae8f0e609ccb027330e42b3d50f5a925990 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 02:12:38 +0100 Subject: [PATCH 27/82] Rewrite example handles unique constraint violation --- spec/models/asset_links_create_edge_spec.rb | 120 ++++++++++++++++---- 1 file changed, 99 insertions(+), 21 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index c94cb2689f..56be4a5d70 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -5,6 +5,22 @@ RSpec.describe AssetLink, type: :model do # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable describe '.create_edge' do + def wait_for_child_processes(pids) + exits = [] + pids + .each + .with_index(1) do |pid, index| + Timeout.timeout(10) do + _pid, status = Process.wait2(pid) + exits << status.exitstatus + end + rescue Timeout::Error + raise StandardError, "parent: Timeout waiting for child#{index} to finish" + end + + expect(exits).to eq([0, 0]) + end + after do @ancestor&.destroy @descendant&.destroy @@ -61,18 +77,7 @@ end # Parent - exits = [] - [pid1, pid2].each - .with_index(1) do |pid, index| - Timeout.timeout(10) do - _pid, status = Process.wait2(pid) - exits << status.exitstatus - end - rescue Timeout::Error - raise StandardError, "parent: Timeout waiting for child#{index} to finish" - end - - expect(exits).to eq([0, 0]) + wait_for_child_processes([pid1, pid2]) expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) @edge = edge = described_class.last @@ -141,18 +146,89 @@ end # Parent - exits = [] - [pid1, pid2].each - .with_index(1) do |pid, index| - Timeout.timeout(10) do - _pid, status = Process.wait2(pid) - exits << status.exitstatus + wait_for_child_processes([pid1, pid2]) + + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + @edge = edge = described_class.last + expect(edge.ancestor).to eq(ancestor) + expect(edge.descendant).to eq(descendant) + expect(edge.direct).to be_truthy + expect(edge.count).to eq(1) + end + + it 'handles unique constraint violation' do + # Parent + ActiveRecord::Base.connection.reconnect! + @ancestor = ancestor = create(:labware) + @descendant = descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + first_socket, second_socket = UNIXSocket.pair + + # First child + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + find_link_call_count = 0 + allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| + find_link_call_count += 1 + link = m.call(*args) + if find_link_call_count == 1 + expect(link).to be_nil + message = 'paused' + first_socket.send(message, 0) + message = 'child1: Timeout waiting for resume message' + raise StandardError, message unless first_socket.wait_readable(10) + message = 'resume' + first_socket.recv(message.size) + end + link end - rescue Timeout::Error - raise StandardError, "parent: Timeout waiting for child#{index} to finish" + + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) + # rubocop:enable RSpec/AnyInstance + + allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| + edge = args[0] + message = + "Duplicate entry '#{ancestor.id}-#{descendant.id}' " \ + "for key 'index_asset_links_on_ancestor_id_and_descendant_id'" + exception = ActiveRecord::RecordNotUnique.new(message) + allow(edge).to receive(:save).and_raise(exception) + + method.call(*args) + end + + unique_violation_error_return_value = nil + allow(described_class).to receive(:unique_violation_error?).and_wrap_original do |m, *args| + unique_violation_error_return_value = m.call(*args) + end + + result = described_class.create_edge(ancestor, descendant) + expect(result).to be_truthy + expect(described_class).to have_received(:unique_violation_error?) + expect(unique_violation_error_return_value).to be_truthy + + ActiveRecord::Base.connection.close end - expect(exits).to eq([0, 0]) + # Second child + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + message = 'child2: Timeout waiting for paused message' + raise StandardError, message unless second_socket.wait_readable(10) + message = 'paused' + second_socket.recv(message.size) + described_class.create_edge(ancestor, descendant) + message = 'resume' + second_socket.send(message, 0) + ActiveRecord::Base.connection.close + end + + # Parent + wait_for_child_processes([pid1, pid2]) expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) @edge = edge = described_class.last @@ -160,6 +236,8 @@ expect(edge.descendant).to eq(descendant) expect(edge.direct).to be_truthy expect(edge.count).to eq(1) + + ActiveRecord::Base.connection.close end end # rubocop:enable RSpec/ExampleLength,RSpec/InstanceVariable From b393fddeb16286dc9544bd60c823e35e02ec5ee1 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 02:33:15 +0100 Subject: [PATCH 28/82] Update tests for create_edge method --- spec/models/asset_links_create_edge_spec.rb | 66 ++++++++++++++++----- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index 56be4a5d70..cffbf33376 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -4,7 +4,13 @@ RSpec.describe AssetLink, type: :model do # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable + # Test the overridden create_edge class method. describe '.create_edge' do + # Wait for child processes to finish. + # + # @param pids [Array] Process IDs + # @raise [Timeout::Error] If a child process does not finish within 10 seconds. + # @return [void] def wait_for_child_processes(pids) exits = [] pids @@ -21,6 +27,7 @@ def wait_for_child_processes(pids) expect(exits).to eq([0, 0]) end + # Delete created records. after do @ancestor&.destroy @descendant&.destroy @@ -28,32 +35,43 @@ def wait_for_child_processes(pids) end it 'handles race condition at find_link' do + # In this example, the first and second processes are forked from the + # main process and they are in race condition to find an existing link + # between the ancestor and descendant and create an edge if no link is + # found. Both first and second processes finds no link, but the second + # process creates the edge first. The first also tries to create the + # edge based on the result of the find_link method. Only one link must + # be created. + # Parent ActiveRecord::Base.connection.reconnect! @ancestor = ancestor = create(:labware) @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction + # Create a duplex pipe for IPC. first_socket, second_socket = UNIXSocket.pair # First child pid1 = fork do ActiveRecord::Base.connection.reconnect! + find_link_call_count = 0 + # Patch find_link method allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| find_link_call_count += 1 link = m.call(*args) if find_link_call_count == 1 - expect(link).to be_nil + expect(link).to be_nil # No link found message = 'paused' - first_socket.send(message, 0) + first_socket.send(message, 0) # Notify the second child message = 'child1: Timeout waiting for resume message' raise StandardError, message unless first_socket.wait_readable(10) - message = 'resume' + message = 'resume' # Wait for the second child to create the edge first_socket.recv(message.size) elsif find_link_call_count == 2 - expect(link).not_to be_nil + expect(link).not_to be_nil # Found one now end link end @@ -68,10 +86,10 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.reconnect! message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) - message = 'paused' + message = 'paused' # Wait for the first child to call find_link second_socket.recv(message.size) described_class.create_edge(ancestor, descendant) - message = 'resume' + message = 'resume' # Notify the first child second_socket.send(message, 0) ActiveRecord::Base.connection.close end @@ -90,12 +108,19 @@ def wait_for_child_processes(pids) end it 'handles unique validation error' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the has_duplicates validation. + # Failing process should use the existing link. + # Parent ActiveRecord::Base.connection.reconnect! @ancestor = ancestor = create(:labware) @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction + # Create a duplex pipe for IPC. first_socket, second_socket = UNIXSocket.pair # First child @@ -107,17 +132,18 @@ def wait_for_child_processes(pids) find_link_call_count += 1 link = m.call(*args) if find_link_call_count == 1 - expect(link).to be_nil - message = 'paused' + expect(link).to be_nil # No link found + message = 'paused' # Notify the second child first_socket.send(message, 0) message = 'child1: Timeout waiting for resume message' raise StandardError, message unless first_socket.wait_readable(10) - message = 'resume' + message = 'resume' # Wait for the second child to create the edge first_socket.recv(message.size) end link end + # Patch unique_validation_error? method unique_validation_error_return_value = nil allow(described_class).to receive(:unique_validation_error?).and_wrap_original do |m, *args| unique_validation_error_return_value = m.call(*args) @@ -137,10 +163,10 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.reconnect! message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) - message = 'paused' + message = 'paused' # Wait for the first child to call find_link second_socket.recv(message.size) described_class.create_edge(ancestor, descendant) - message = 'resume' + message = 'resume' # Notify the first child second_socket.send(message, 0) ActiveRecord::Base.connection.close end @@ -157,12 +183,19 @@ def wait_for_child_processes(pids) end it 'handles unique constraint violation' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the database unique constraint + # violation. Failing process should use the existing link. + # Parent ActiveRecord::Base.connection.reconnect! @ancestor = ancestor = create(:labware) @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction + # Create a duplex pipe for IPC. first_socket, second_socket = UNIXSocket.pair # First child @@ -175,20 +208,22 @@ def wait_for_child_processes(pids) link = m.call(*args) if find_link_call_count == 1 expect(link).to be_nil - message = 'paused' + message = 'paused' # Notify the second child first_socket.send(message, 0) message = 'child1: Timeout waiting for resume message' raise StandardError, message unless first_socket.wait_readable(10) - message = 'resume' + message = 'resume' # Wait for the second child to create the edge first_socket.recv(message.size) end link end + # Patch has_duplicates method. # rubocop:disable RSpec/AnyInstance allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) # rubocop:enable RSpec/AnyInstance + # Patch save_edge_or_handle_error method. allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| edge = args[0] message = @@ -200,6 +235,7 @@ def wait_for_child_processes(pids) method.call(*args) end + # Patch unique_violation_error? method. unique_violation_error_return_value = nil allow(described_class).to receive(:unique_violation_error?).and_wrap_original do |m, *args| unique_violation_error_return_value = m.call(*args) @@ -219,10 +255,10 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.reconnect! message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) - message = 'paused' + message = 'paused' # Wait for the first child to call find_link second_socket.recv(message.size) described_class.create_edge(ancestor, descendant) - message = 'resume' + message = 'resume' # Notify the first child second_socket.send(message, 0) ActiveRecord::Base.connection.close end From 0ac25f7227f305c21e394e470d7afc9b1300bf69 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 02:34:05 +0100 Subject: [PATCH 29/82] Deleted old version of the tests --- .../asset_links_race_conditions_spec.rb | 350 ------------------ 1 file changed, 350 deletions(-) delete mode 100644 spec/models/asset_links_race_conditions_spec.rb diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb deleted file mode 100644 index cc804dcce8..0000000000 --- a/spec/models/asset_links_race_conditions_spec.rb +++ /dev/null @@ -1,350 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.xdescribe AssetLink, type: :model do - describe '.create_edge' do - # Helpers - - # Used in IPC when one end of the duplex pipe is waiting for the other end - # to send a message with a timeout in seconds. - # - # @param socket [Socket] The socket to wait for. - # @param length [Integer] The length of the message to wait for. - # @param timeout [Integer] The timeout in seconds, default is 10. - # @param message [String] The message to raise if times out, default is 'socket timeout'. - # @return [String] The message received from the socket. - # @raise StandardError - def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket timeout') - raise StandardError, message unless socket.wait_readable(timeout) - socket.recv(length) - end - - # Used to reap a child process with a timeout in seconds. - # - # @param pid [Integer] The process ID to wait for. - # @param timeout [Integer] The timeout in seconds, default is 10. - # @param message [String] The message to raise if times out, default is 'process timeout'. - # @return [Process::Status] The status of the process. - # @raise StandardError - # rubocop:disable Metrics/MethodLength - def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') - start_time = Time.zone.now - loop do - begin - pid2, status = Process.waitpid2(pid, Process::WNOHANG) - return status if pid2 - rescue Errno::ECHILD # No child process - return nil - end - if Time.zone.now - start_time > timeout - begin - Process.kill('TERM', pid) # Send TERM signal. - sleep 1 # Wait for the process to terminate. - Process.kill('KILL', pid) # Send KILL signal. - Process.waitpid(pid) # Reap - rescue Errno::ECHILD - # No child process - end - raise StandardError, message - end - sleep 0.1 - end - end - # rubocop:enable Metrics/MethodLength - - # Wait for the given processes to finish with a timeout in seconds. - # @param pids [Array] The process IDs to wait for. - # @return [void] - def wait_for_processes(pids) - pids.each do |pid| - status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") - if status.exitstatus != 0 - raise StandardError, "Forked process #{pid} failed with exit status #{status.exitstatus}" - end - end - end - - # Examples - - # We need to explicitly clean the database after examples here because we - # are establishing new database connections in the main and forked child - # processes. - # rubocop:disable RSpec/BeforeAfterAll - after(:all) do - AssetLink.delete_all - Labware.delete_all - end - # rubocop:enable RSpec/BeforeAfterAll - - # rubocop:disable RSpec/ExampleLength - it 'handles race condition at find_link' do - # In this example, the first and second processes are forked from the - # main process and they are in race condition to find an existing link - # between the ancestor and descendant and create an edge if no link is - # found. Both first and second processes finds no link, but the second - # process creates the edge first. The first also tries to create the - # edge based on the result of the find_link method. However, it must - # not be able to create it. - - ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) - ActiveRecord::Base.connection.commit_db_transaction - - # Create a duplex pipe for inter-process communication. - first_socket, second_socket = Socket.pair(:UNIX, :STREAM) - - # Fork the first process. - pid1 = - fork do - ActiveRecord::Base.connection.reconnect! - - call_count = 0 # Track the calls to the find_link method. - - # Patch the find_link method in the first process. - allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| - call_count += 1 - link = method.call(*args) - if call_count == 1 - expect(link).to be_nil # The first process should not find any link initally. - - signal = 'paused' - first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. - - # Wait for the second process to send 'resume'. - wait_readable_with_timeout( - first_socket, - 'resume'.length, - 10, - 'Timeout waiting for the second process to send resume.' - ) - elsif call_count == 2 - # The first process now should find the link created by the second process. - expect(link).not_to be_nil - end - link - end - - described_class.create_edge(ancestor, descendant) - ActiveRecord::Base.connection.close - end - - # Fork the second process. - pid2 = - fork do - ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. - - # Wait for the first process to signal that it is paused after calling find_link. - wait_readable_with_timeout( - second_socket, - 'paused'.length, - 10, - 'Timeout waiting for the first process to send paused.' - ) - - described_class.create_edge(ancestor, descendant) - # Signal the first process to resume now. Although it has found no link - # in its last method call, actually there is one now. - signal = 'resume' - second_socket.send(signal, 0) # Zero flags. - ActiveRecord::Base.connection.close - end - - # Wait for both processes to finish and check their exit statuses - wait_for_processes([pid1, pid2]) - - # Check that the edge was created as expected. - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) - - ActiveRecord::Base.connection.close - end - # rubocop:enable RSpec/ExampleLength - - # rubocop:disable RSpec/ExampleLength - it 'handles race condition at has_duplicates' do - # In this example, the first and second processes are forked from the - # main process. Neither of them finds and existing link between the - # ancestor and descendant. Both try to create the edge. One of them - # succeeds and the other one fails due to the has_duplicates validation. - # Failing process should use the existing link. - - ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) - ActiveRecord::Base.connection.commit_db_transaction - - # Create a duplex pipe for inter-process communication. - first_socket, second_socket = Socket.pair(:UNIX, :STREAM) - - # Fork the first process. - pid1 = - fork do - ActiveRecord::Base.connection.reconnect! - call_count = 0 # Track the calls to the find_link method. - # Patch the find_link method in the first process. - # Check link, trigger the second process, pause, and then wait for'resume' signal. - allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| - call_count += 1 - link = method.call(*args) - if call_count == 1 - expect(link).to be_nil # The first process should not find any link initally. - signal = 'paused' - first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. - # Wait for the second process to send 'resume'. - wait_readable_with_timeout( - first_socket, - 'resume'.length, - 10, - 'Timeout waiting for the second process to send resume.' - ) - end - link - end - allow(described_class).to receive(:unique_validation_error?).and_call_original - - # Now the first process should be prevented from saving because of has_duplicates validation. - result = described_class.create_edge(ancestor, descendant) - expect(result).to be_truthy - expect(described_class).to have_received(:unique_validation_error?) - - ActiveRecord::Base.connection.close - end - - # Fork the second process. - pid2 = - fork do - ActiveRecord::Base.connection.reconnect! - # Wait for the first process to signal that it is paused after calling find_link. - wait_readable_with_timeout( - second_socket, - 'paused'.length, - 10, - 'Timeout waiting for the first process to send paused.' - ) - described_class.create_edge(ancestor, descendant) - signal = 'resume' - second_socket.send(signal, 0) # Zero flags. - ActiveRecord::Base.connection.close - end - - # Wait for both processes to finish and check their exit statuses - wait_for_processes([pid1, pid2]) - - # Check that the edge was created as expected. - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) # How many different paths between the nodes. - end - # rubocop:enable RSpec/ExampleLength - - # rubocop:disable RSpec/ExampleLength - it 'handles unique constraint violation' do - # In this example, the first and second processes are forked from the - # main process. Neither of them finds and existing link between the - # ancestor and descendant. Both try to create the edge. One of them - # succeeds and the other one fails due to the unique constraint - # violation. Failing process should use the existing link. - - ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) - ActiveRecord::Base.connection.commit_db_transaction - - # Create a duplex pipe for inter-process communication. - first_socket, second_socket = Socket.pair(:UNIX, :STREAM) - - # Fork the first process. - pid1 = - fork do - ActiveRecord::Base.connection.reconnect! - call_count = 0 # Track the calls to the find_link method. - # Patch the find_link method in the first process. - # Check link, trigger the second process, pause, and then wait for'resume' signal. - allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| - call_count += 1 - link = method.call(*args) - if call_count == 1 - expect(link).to be_nil # The first process should not find any link initally. - signal = 'paused' - first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. - # Wait for the second process to send 'resume'. - wait_readable_with_timeout( - first_socket, - 'resume'.length, - 10, - 'Timeout waiting for the second process to send resume.' - ) - end - link - end - # Validation error is not triggered in this case because the other - # process has not saved the link yet. Therefore, the save call will - # pass the validations and hit the database to trigger the unique - # constraint violation just after the other process saves the link. - # rubocop:disable RSpec/AnyInstance - allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) - # rubocop:enable RSpec/AnyInstance - - allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| - edge = args[0] - message = - "Duplicate entry '#{ancestor.id}-#{descendant.id}' " \ - "for key 'index_asset_links_on_ancestor_id_and_descendant_id'" - exception = ActiveRecord::RecordNotUnique.new(message) - allow(edge).to receive(:save).and_raise(exception) - - method.call(*args) - end - allow(described_class).to receive(:unique_violation_error?).and_call_original - - described_class.create_edge(ancestor, descendant) - expect(described_class).to have_received(:unique_violation_error?) - ActiveRecord::Base.connection.close - end - - # Fork the second process. - pid2 = - fork do - ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. - - # Wait for the first process to signal that it is paused after calling find_link. - wait_readable_with_timeout( - second_socket, - 'paused'.length, - 10, - 'Timeout waiting for the first process to send paused.' - ) - - described_class.create_edge(ancestor, descendant) - # Signal the first process to resume now. Although it has found no link - # in its last method call, actually there is one now. - signal = 'resume' - second_socket.send(signal, 0) # Zero flags. - ActiveRecord::Base.connection.close - end - - # Wait for both processes to finish and check their exit statuses - wait_for_processes([pid1, pid2]) - - # Check that the edge was created as expected. - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) - - ActiveRecord::Base.connection.close - end - # rubocop:enable RSpec/ExampleLength - end -end From ca872412d15d7bbc707ae643fc88ba47075c02b0 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 17:29:09 +0100 Subject: [PATCH 30/82] Remove redundant require --- db/migrate/20240912000109_add_unique_index_to_asset_links.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index 13a495bbaa..786eb1b9b1 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require 'csv' # 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. From 7a691cfc48c52a765e3d47eee103b80055693f51 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 11:58:35 +0100 Subject: [PATCH 31/82] Pass control source plate to control locator --- app/models/cherrypick_task.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index 54b28b02c8..7a8156b1c8 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -26,14 +26,14 @@ def new_control_locator( total_wells, num_control_wells, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE, - control_assets: nil + control_source_plate: nil ) CherrypickTask::ControlLocator.new( batch_id: batch_id, total_wells: total_wells, num_control_wells: num_control_wells, wells_to_leave_free: wells_to_leave_free, - control_assets: control_assets + control_source_plate: control_source_plate ) end @@ -92,9 +92,8 @@ 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_assets:) + new_control_locator(batch.id, current_destination_plate.size, control_assets.count, control_source_plate:) 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 From 868631e3d7601587468bdd230e203ff57b4e49f6 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 11:59:12 +0100 Subject: [PATCH 32/82] Added control placement type attr from custom metadata --- app/models/cherrypick_task/control_locator.rb | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index fbce59d537..0c19baff69 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -29,7 +29,7 @@ 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_assets + attr_reader :batch_id, :total_wells, :wells_to_leave_free, :num_control_wells, :available_positions, :control_assets, :control_placement_type # @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 @@ -40,13 +40,13 @@ class CherrypickTask::ControlLocator # @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:, control_assets:, wells_to_leave_free: []) + def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate:, 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 - @control_assets = control_assets + @control_source_plate = control_source_plate end # @@ -57,6 +57,11 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_assets:, wel # # @return [Array] The indexes of the control well positions # + + def control_placement_type + @control_placement_type ||= control_source_plate.custom_metadatum_collection.metadata['control_placement_type'] + + def control_positions(num_plate) raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions @@ -108,6 +113,8 @@ def convert_control_assets(control_assets) rows = ('A'..'H').to_a columns = (1..12).to_a + # 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. 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}"] } @@ -118,6 +125,7 @@ def convert_control_assets(control_assets) end def fixed_positions_from_available + control_assets = @control_source_plate.wells.joins(:samples) control_wells = control_assets.map(&:map_id) convert_control_assets(control_wells) end From aa3955c85157bb47777fe14f1baa9339e6051404 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:08:13 +0100 Subject: [PATCH 33/82] Added logic for handling random and fixed placement --- app/models/cherrypick_task/control_locator.rb | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 0c19baff69..41ac196e0b 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -59,7 +59,8 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate # def control_placement_type - @control_placement_type ||= control_source_plate.custom_metadatum_collection.metadata['control_placement_type'] + @control_source_plate.custom_metadatum_collection.metadata['control_placement_type'] + end def control_positions(num_plate) @@ -71,13 +72,15 @@ 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_for(num_plate) - - # initial_positions = random_positions_from_available(seed) - - fixed_positions_from_available - - #control_positions_for_plate(num_plate, initial_positions) + seed = seed_for(num_plate) + + @control_placement_type = control_placement_type + if @control_placement_type == 'random' + initial_positions = random_positions_from_available(seed) + control_positions_for_plate(num_plate, initial_positions) + else + fixed_positions_from_available + end end private @@ -125,6 +128,7 @@ def convert_control_assets(control_assets) end def fixed_positions_from_available + binding.pry control_assets = @control_source_plate.wells.joins(:samples) control_wells = control_assets.map(&:map_id) convert_control_assets(control_wells) From a1864ff00aa570e558b7d1190bd7522d1edd1206 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:19:50 +0100 Subject: [PATCH 34/82] Added control placement type to control plate options in UI --- app/models/cherrypick_task/control_locator.rb | 10 +++++----- app/views/workflows/_plate_template_batches.html.erb | 9 +++++++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 41ac196e0b..2141ef34d3 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -29,7 +29,7 @@ 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_assets, :control_placement_type + 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 @@ -72,10 +72,11 @@ 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) - @control_placement_type = control_placement_type - if @control_placement_type == 'random' + placement_type = control_placement_type + + if placement_type == 'random' + seed = seed_for(num_plate) initial_positions = random_positions_from_available(seed) control_positions_for_plate(num_plate, initial_positions) else @@ -128,7 +129,6 @@ def convert_control_assets(control_assets) end def fixed_positions_from_available - binding.pry control_assets = @control_source_plate.wells.joins(:samples) control_wells = control_assets.map(&:map_id) convert_control_assets(control_wells) diff --git a/app/views/workflows/_plate_template_batches.html.erb b/app/views/workflows/_plate_template_batches.html.erb index dac8d76df3..5dcba51bf1 100644 --- a/app/views/workflows/_plate_template_batches.html.erb +++ b/app/views/workflows/_plate_template_batches.html.erb @@ -19,8 +19,13 @@ <%= 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 {|p| [ "#{p.human_barcode} - #{p.name}", p.id ] }, { include_blank: true }, class: 'form-control select2') %> + + <%= select("Control", "plate_id", + ControlPlate.all.collect do |p| + placement_type = p.custom_metadatum_collection.metadata['control_placement_type'] + [ "#{p.human_barcode} - #{p.name} (#{placement_type.capitalize})", p.id ] + end, + { include_blank: true }, class: 'form-control select2') %> From f37337dfacd0d762a73c01ae361ea81068668b18 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:21:10 +0100 Subject: [PATCH 35/82] Rubocop --- app/models/cherrypick_task/control_locator.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 2141ef34d3..1e29610be2 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -29,7 +29,8 @@ 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, +: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 @@ -117,8 +118,8 @@ def convert_control_assets(control_assets) rows = ('A'..'H').to_a columns = (1..12).to_a - # 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. + # 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. 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}"] } From 51913bf9aef17ac15c338dd1a21c0bfc97dcd56f Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:24:35 +0100 Subject: [PATCH 36/82] Prettier --- app/models/cherrypick_task/control_locator.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 1e29610be2..6b97bebdef 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -29,8 +29,12 @@ 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, + :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 @@ -63,7 +67,6 @@ def control_placement_type @control_source_plate.custom_metadatum_collection.metadata['control_placement_type'] end - def control_positions(num_plate) raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions @@ -75,7 +78,7 @@ def control_positions(num_plate) # 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 == 'random' seed = seed_for(num_plate) initial_positions = random_positions_from_available(seed) From e0d0b01ee620ac4b15c3ea6ddc5298550acd66f6 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:59:13 +0100 Subject: [PATCH 37/82] Control plates won't show without required metadata --- app/views/workflows/_plate_template_batches.html.erb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/views/workflows/_plate_template_batches.html.erb b/app/views/workflows/_plate_template_batches.html.erb index 5dcba51bf1..e3fbeb9b77 100644 --- a/app/views/workflows/_plate_template_batches.html.erb +++ b/app/views/workflows/_plate_template_batches.html.erb @@ -22,9 +22,11 @@ <%= select("Control", "plate_id", ControlPlate.all.collect do |p| - placement_type = p.custom_metadatum_collection.metadata['control_placement_type'] - [ "#{p.human_barcode} - #{p.name} (#{placement_type.capitalize})", p.id ] - end, + 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') %> From e3a4a33f9d841d3b43c9e6f81a80ff3ff93cbca2 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 13:03:01 +0100 Subject: [PATCH 38/82] Added exception handling for unknown placement type --- app/models/cherrypick_task/control_locator.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 6b97bebdef..ad810ea619 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -78,7 +78,9 @@ def control_positions(num_plate) # To avoid it, every num_plate=available_positions we start a new cycle with a new seed. placement_type = control_placement_type - + raise StandardError, 'Control placement type is not set or is invalid' if placement_type.nil? || ["Fixed", + "Random"].exclude?(placement_type) + if placement_type == 'random' seed = seed_for(num_plate) initial_positions = random_positions_from_available(seed) From 5f0323401624b6ac3503a3984b655c4a3d0ecf7f Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:39:25 +0100 Subject: [PATCH 39/82] 'control_source_plate' is now a required attribute --- app/models/cherrypick_task.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index 7a8156b1c8..26371c8043 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -26,7 +26,7 @@ def new_control_locator( total_wells, num_control_wells, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE, - control_source_plate: nil + control_source_plate ) CherrypickTask::ControlLocator.new( batch_id: batch_id, From 440f1dfa5797928fda132a3fbf437f51b04b4351 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:41:33 +0100 Subject: [PATCH 40/82] Fixed syntax --- app/models/cherrypick_task.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index 26371c8043..e365b7c09c 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -25,8 +25,8 @@ def new_control_locator( batch_id, total_wells, num_control_wells, - wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE, - control_source_plate + control_source_plate, + wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE ) CherrypickTask::ControlLocator.new( batch_id: batch_id, From c29b516825f825708101c6ebfbc53b4ae47c1123 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 16:47:05 +0100 Subject: [PATCH 41/82] Changed control placement type values --- app/models/cherrypick_task/control_locator.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index ad810ea619..519038f65d 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -78,8 +78,8 @@ def control_positions(num_plate) # To avoid it, every num_plate=available_positions we start a new cycle with a new seed. placement_type = control_placement_type - raise StandardError, 'Control placement type is not set or is invalid' if placement_type.nil? || ["Fixed", - "Random"].exclude?(placement_type) + raise StandardError, 'Control placement type is not set or is invalid' if placement_type.nil? || ["fixed", + "random"].exclude?(placement_type) if placement_type == 'random' seed = seed_for(num_plate) From 23dd8f178c8c3c16c84e71d36fe44bc01b9360f9 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 16:47:22 +0100 Subject: [PATCH 42/82] Added control placement type to control plate factory --- spec/factories/plate_factories.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/spec/factories/plate_factories.rb b/spec/factories/plate_factories.rb index a700925935..1b44773861 100644 --- a/spec/factories/plate_factories.rb +++ b/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? From 1abbc05b671cc727ce17450a3b72d5c14ff54d23 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 16:48:15 +0100 Subject: [PATCH 43/82] Create control plate with new metadata in spec --- spec/models/cherrypick_task/control_locator_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 37cd0645f1..6ccd945752 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe CherrypickTask::ControlLocator do - let(:instance) { described_class.new(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:) } + let(:instance) { described_class.new(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:, control_source_plate: create(:control_plate) ) } shared_examples 'an invalid ControlLocator' do |plate_number, error = 'More controls than free wells'| it 'throws a "More controls than free wells" exception' do @@ -126,7 +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_positions(0).first + described_class.new(batch_id: batch_id, total_wells: 96, num_control_wells: 1, control_source_plate: create(:control_plate)).control_positions(0).first end end From 4dd1e2a0fc7dd862d3d70779190e24a53109ff40 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 16:59:08 +0100 Subject: [PATCH 44/82] Added control plate to cherrypick task spec --- spec/models/tasks/cherrypick_task_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/models/tasks/cherrypick_task_spec.rb b/spec/models/tasks/cherrypick_task_spec.rb index 887de3e81f..615d977ad8 100644 --- a/spec/models/tasks/cherrypick_task_spec.rb +++ b/spec/models/tasks/cherrypick_task_spec.rb @@ -36,7 +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 + wells_to_leave_free: wells_to_leave_free, + control_source_plate: control_plate ).and_return(locator) end From bf60f4bd146baed18f3516b3c3bc9a89c23f4617 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 18:09:33 +0100 Subject: [PATCH 45/82] Make control source plate optional for control locator --- app/models/cherrypick_task.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index e365b7c09c..9cd0c17660 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -25,15 +25,15 @@ def new_control_locator( batch_id, total_wells, num_control_wells, - control_source_plate, + control_source_plate: nil, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE ) CherrypickTask::ControlLocator.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: control_source_plate + batch_id:, + total_wells:, + num_control_wells:, + wells_to_leave_free:, + control_source_plate: ) end From c54563a01bc74ef0ed40f1c80024797215fab794 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 18:09:38 +0100 Subject: [PATCH 46/82] Rubocop --- app/models/cherrypick_task/control_locator.rb | 50 +++++++++++-------- .../cherrypick_task/control_locator_spec.rb | 7 ++- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 519038f65d..adb07e6ef6 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -63,11 +63,7 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate # @return [Array] The indexes of the control well positions # - def control_placement_type - @control_source_plate.custom_metadatum_collection.metadata['control_placement_type'] - end - - def control_positions(num_plate) + def control_positions(_num_plate) raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions # Check that all elements in wells_to_leave_free fall within the acceptable range @@ -80,16 +76,11 @@ def control_positions(num_plate) placement_type = control_placement_type raise StandardError, 'Control placement type is not set or is invalid' if placement_type.nil? || ["fixed", "random"].exclude?(placement_type) - - if placement_type == 'random' - seed = seed_for(num_plate) - initial_positions = random_positions_from_available(seed) - control_positions_for_plate(num_plate, initial_positions) - else - fixed_positions_from_available - end + + handle_control_placement_type(placement_type) end + private # If num plate is equal to the available positions, the cycle is going to be repeated. @@ -116,17 +107,23 @@ 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) + 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_control_assets(control_assets) - rows = ('A'..'H').to_a - columns = (1..12).to_a - - # 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. - 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 = create_plate_maps control_assets.map do |id| invalid_location = valid_map[id] @@ -140,6 +137,19 @@ def fixed_positions_from_available convert_control_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/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 6ccd945752..75f36ebfa0 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -3,7 +3,9 @@ require 'rails_helper' RSpec.describe CherrypickTask::ControlLocator do - let(:instance) { described_class.new(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:, control_source_plate: create(:control_plate) ) } + 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) ) end shared_examples 'an invalid ControlLocator' do |plate_number, error = 'More controls than free wells'| it 'throws a "More controls than free wells" exception' do @@ -126,7 +128,8 @@ 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)).control_positions(0).first + described_class.new(batch_id: batch_id, total_wells: 96, num_control_wells: 1, +control_source_plate: create(:control_plate)).control_positions(0).first end end From 27fde60fb00b9083f2d6eda0138d2ac9809080ad Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 18:13:23 +0100 Subject: [PATCH 47/82] Modification to inline documenation --- app/models/cherrypick_task/control_locator.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index adb07e6ef6..e2e56a7f1a 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -61,9 +61,8 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate # @param num_plate [Integer] The plate number within the batch # # @return [Array] The indexes of the control well positions - # - def control_positions(_num_plate) + def control_positions(num_plate) raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions # Check that all elements in wells_to_leave_free fall within the acceptable range From 6b9cad6a72be14e75dc64efc0cf026de67b46aa5 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 18:14:04 +0100 Subject: [PATCH 48/82] Rubocop --- app/models/cherrypick_task/control_locator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index e2e56a7f1a..2be4b51127 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -62,7 +62,7 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate # # @return [Array] The indexes of the control well positions - def control_positions(num_plate) + def control_positions(_num_plate) raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions # Check that all elements in wells_to_leave_free fall within the acceptable range From 4d5822babc0aeba16c2d0dc0a216cfe16f1ea182 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 18:14:46 +0100 Subject: [PATCH 49/82] Rubocop.... --- app/models/cherrypick_task/control_locator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 2be4b51127..c31e9ed1bf 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -58,7 +58,7 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate # Returns a list with the destination positions for the control wells distributed randomly # using batch_id as seed and num_plate to increase position with plates in same batch. # - # @param num_plate [Integer] The plate number within the batch + # @param _num_plate [Integer] The plate number within the batch # # @return [Array] The indexes of the control well positions From a4e60ee243f089bcea2feb757b675b677c6503ad Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 18:42:35 +0100 Subject: [PATCH 50/82] Syntax --- app/models/cherrypick_task/control_locator.rb | 12 +++++------ spec/factories/plate_factories.rb | 2 +- .../cherrypick_task/control_locator_spec.rb | 21 +++++++++++++++---- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index c31e9ed1bf..08301bd5d6 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -73,13 +73,13 @@ def control_positions(_num_plate) # To avoid it, every num_plate=available_positions we start a new cycle with a new seed. placement_type = control_placement_type - raise StandardError, 'Control placement type is not set or is invalid' if placement_type.nil? || ["fixed", - "random"].exclude?(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) + handle_control_placement_type(placement_type, num_plate) end - private # If num plate is equal to the available positions, the cycle is going to be repeated. @@ -110,7 +110,7 @@ def control_placement_type @control_source_plate.custom_metadatum_collection.metadata['control_placement_type'] end - def handle_control_placement_type(placement_type) + 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 @@ -138,7 +138,7 @@ def fixed_positions_from_available # 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 diff --git a/spec/factories/plate_factories.rb b/spec/factories/plate_factories.rb index 1b44773861..455d2b9a3f 100644 --- a/spec/factories/plate_factories.rb +++ b/spec/factories/plate_factories.rb @@ -251,7 +251,7 @@ 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 75f36ebfa0..cb7383bdb5 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -4,8 +4,14 @@ 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) ) end + 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) + ) + end shared_examples 'an invalid ControlLocator' do |plate_number, error = 'More controls than free wells'| it 'throws a "More controls than free wells" exception' do @@ -128,8 +134,15 @@ 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)).control_positions(0).first + described_class + .new( + batch_id: batch_id, + total_wells: 96, + num_control_wells: 1, + control_source_plate: create(:control_plate) + ) + .control_positions(0) + .first end end From 49dc78c53c870cb2e499b54af31c97f4f89de806 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 19:06:30 +0100 Subject: [PATCH 51/82] Syntax (again) --- app/models/cherrypick_task/control_locator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 08301bd5d6..861b5d76c5 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -62,7 +62,7 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate # # @return [Array] The indexes of the control well positions - def control_positions(_num_plate) + def control_positions(num_plate) raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions # Check that all elements in wells_to_leave_free fall within the acceptable range From 506b49a6320927982b54954e9817459c9dc814d2 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Fri, 27 Sep 2024 19:18:53 +0100 Subject: [PATCH 52/82] Yard doc --- app/models/cherrypick_task/control_locator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 861b5d76c5..b99d162886 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -58,7 +58,7 @@ def initialize(batch_id:, total_wells:, num_control_wells:, control_source_plate # Returns a list with the destination positions for the control wells distributed randomly # using batch_id as seed and num_plate to increase position with plates in same batch. # - # @param _num_plate [Integer] The plate number within the batch + # @param num_plate [Integer] The plate number within the batch # # @return [Array] The indexes of the control well positions From efdf4da09ffb0b59fa2580b90eb1f19873d908ba Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 10:45:53 +0100 Subject: [PATCH 53/82] Pass plate template to control locator --- app/models/cherrypick_task.rb | 11 +++++++---- app/models/cherrypick_task/control_locator.rb | 3 ++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index 9cd0c17660..af59a5cae0 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -25,6 +25,7 @@ def new_control_locator( batch_id, total_wells, num_control_wells, + template, control_source_plate: nil, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE ) @@ -33,7 +34,8 @@ def new_control_locator( total_wells:, num_control_wells:, wells_to_leave_free:, - control_source_plate: + control_source_plate:, + template: ) end @@ -50,7 +52,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 @@ -78,7 +80,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? @@ -93,7 +95,8 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller) # r 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_source_plate:) + new_control_locator(batch.id, current_destination_plate.size, control_assets.count, template, +control_source_plate:) 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 b99d162886..d3ee7b9fbd 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -45,13 +45,14 @@ class CherrypickTask::ControlLocator # @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:, control_source_plate:, wells_to_leave_free: []) + def initialize(batch_id:, total_wells:, num_control_wells:, template:, control_source_plate:, 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 @control_source_plate = control_source_plate + @plate_template = template end # From 1a1930f9528bcecba5c363748f7140c929016325 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:11:29 +0100 Subject: [PATCH 54/82] Added method to handle incompatible plates --- app/models/cherrypick_task.rb | 16 ++++++++++++---- app/models/cherrypick_task/control_locator.rb | 18 +++++++++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index af59a5cae0..db09320c70 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -21,12 +21,14 @@ class CherrypickTask < Task # rubocop:todo Metrics/ClassLength # # @return [CherrypickTask::ControlLocator] A generator of control locations # + + # rubocop:disable Metrics/ParameterLists def new_control_locator( batch_id, total_wells, num_control_wells, template, - control_source_plate: nil, + control_source_plate:, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE ) CherrypickTask::ControlLocator.new( @@ -38,6 +40,7 @@ def new_control_locator( template: ) end + # rubocop:enable Metrics/ParameterLists # # Cherrypick tasks are directly coupled to the previous task, due to the awkward @@ -68,7 +71,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 @@ -95,8 +98,13 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller, tem 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, template, -control_source_plate:) + new_control_locator( + batch.id, + current_destination_plate.size, + control_assets.count, + template, + control_source_plate: + ) 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 d3ee7b9fbd..3ffdf72199 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -45,6 +45,8 @@ class CherrypickTask::ControlLocator # @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 + + # rubocop:disable Metrics/ParameterLists def initialize(batch_id:, total_wells:, num_control_wells:, template:, control_source_plate:, wells_to_leave_free: []) @batch_id = batch_id @total_wells = total_wells @@ -54,6 +56,7 @@ def initialize(batch_id:, total_wells:, num_control_wells:, template:, control_s @control_source_plate = control_source_plate @plate_template = template end + # rubocop:enable Metrics/ParameterLists # # Returns a list with the destination positions for the control wells distributed randomly @@ -131,10 +134,23 @@ def convert_control_assets(control_assets) end end + def handle_incompatible_plates(converted_control_assets) + template_wells = @plate_template.wells.map(&:map_id) + converted_template_assets = convert_control_assets(template_wells) + + converted_control_assets.intersect?(converted_template_assets) + end + def fixed_positions_from_available control_assets = @control_source_plate.wells.joins(:samples) control_wells = control_assets.map(&:map_id) - convert_control_assets(control_wells) + converted_assets = convert_control_assets(control_wells) + + if handle_incompatible_plates(converted_assets) + raise StandardError, 'The control plate and plate template are incompatible' + end + + converted_assets end # The invalid and valid maps are hash maps to represent a plate that maps A1 -> 1, A2 -> 2, etc, From ccbdc13cc1d23828ff554b2e14b0a7aa2743de6e Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:11:43 +0100 Subject: [PATCH 55/82] Modifications to existing tests --- spec/models/cherrypick_task/control_locator_spec.rb | 6 ++++-- spec/models/tasks/cherrypick_task_spec.rb | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index cb7383bdb5..a2880f84ff 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -9,7 +9,8 @@ total_wells: total_wells, num_control_wells: num_control_wells, wells_to_leave_free: wells_to_leave_free, - control_source_plate: create(:control_plate) + control_source_plate: create(:control_plate), + template: create(:plate_template) ) end @@ -139,7 +140,8 @@ batch_id: batch_id, total_wells: 96, num_control_wells: 1, - control_source_plate: create(:control_plate) + control_source_plate: create(:control_plate), + template: create(:plate_template) ) .control_positions(0) .first diff --git a/spec/models/tasks/cherrypick_task_spec.rb b/spec/models/tasks/cherrypick_task_spec.rb index 615d977ad8..79f112340b 100644 --- a/spec/models/tasks/cherrypick_task_spec.rb +++ b/spec/models/tasks/cherrypick_task_spec.rb @@ -37,7 +37,8 @@ def requests_for_plate(plate) total_wells: 6, num_control_wells: 2, wells_to_leave_free: wells_to_leave_free, - control_source_plate: control_plate + control_source_plate: control_plate, + template: template ).and_return(locator) end From b5f1d4888ae086fe75eb51278f840dbfae8e4657 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:22:29 +0100 Subject: [PATCH 56/82] Added test for invalid placement type --- spec/models/cherrypick_task/control_locator_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index a2880f84ff..87f8d5cc6e 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -59,6 +59,11 @@ 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 From ef144f9dfbaec6c5367ec56bef96c2103439b7f8 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:28:30 +0100 Subject: [PATCH 57/82] Added test for plate compatibilty check --- spec/models/cherrypick_task/control_locator_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 87f8d5cc6e..089820c0df 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -64,6 +64,13 @@ placement_type = instance.send(:control_placement_type) expect(placement_type.nil? || %w[fixed random].exclude?(placement_type)).to be_falsey end + + it 'validates compatibility with the plate template' do + placement_type = instance.send(:control_placement_type) + if placement_type == 'fixed' + expect(error).not_to eq 'The control plate and plate template are incompatible' + end + end end # Control positions will be our only public method, sand perhaps some attr_readers From 5cbdc32f1a811b9e8f7edbfa1a5c3046267944d6 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:33:20 +0100 Subject: [PATCH 58/82] Linting --- spec/models/cherrypick_task/control_locator_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 089820c0df..1edc671d28 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -67,9 +67,7 @@ it 'validates compatibility with the plate template' do placement_type = instance.send(:control_placement_type) - if placement_type == 'fixed' - expect(error).not_to eq 'The control plate and plate template are incompatible' - end + expect(error).not_to eq 'The control plate and plate template are incompatible' if placement_type == 'fixed' end end From 63c00b82b1f5f00740bdfd24f206c99c75c6a1f3 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 13:05:27 +0100 Subject: [PATCH 59/82] Added test for invalid placement types --- .../cherrypick_task/control_locator_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 1edc671d28..4fdf25b3fb 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -173,5 +173,23 @@ 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 do + # Stub the `control_placement_type` to return an invalid type + allow(instance).to receive(:control_placement_type).and_return('invalid_type') + end + + it 'raises an error about invalid placement type' do + expect do + instance.control_positions(0) + end.to raise_error(StandardError, 'Control placement type is not set or is invalid') + end + end end end From a761d3702d5e0c84719f1542fef43823ec3d09ec Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 14:05:28 +0100 Subject: [PATCH 60/82] Refactored test fror checking plate compatibility --- .../cherrypick_task/control_locator_spec.rb | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 4fdf25b3fb..6c84d8bbf4 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -64,11 +64,6 @@ placement_type = instance.send(:control_placement_type) expect(placement_type.nil? || %w[fixed random].exclude?(placement_type)).to be_falsey end - - it 'validates compatibility with the plate template' do - placement_type = instance.send(:control_placement_type) - expect(error).not_to eq 'The control plate and plate template are incompatible' if placement_type == 'fixed' - end end # Control positions will be our only public method, sand perhaps some attr_readers @@ -180,15 +175,29 @@ let(:num_control_wells) { 2 } let(:wells_to_leave_free) { [] } - before do - # Stub the `control_placement_type` to return an invalid type - allow(instance).to receive(:control_placement_type).and_return('invalid_type') - end - + before { allow(instance).to receive(:control_placement_type).and_return('invalid_type') } + it 'raises an error about invalid placement type' do - expect do - instance.control_positions(0) - end.to raise_error(StandardError, 'Control placement type is not set or is invalid') + expect do instance.control_positions(0) end.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 { allow(instance).to receive_messages(control_placement_type: 'fixed', convert_control_assets: [1, 2, 3]) } + + it 'raises an error about incompatibility' do + expect do instance.control_positions(0) end.to raise_error( + StandardError, + 'The control plate and plate template are incompatible' + ) end end end From 6a1850af04e551d0c1d9ca6494f66835f8ebd665 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 14:29:31 +0100 Subject: [PATCH 61/82] Added test coverage for asset map conversion --- spec/models/cherrypick_task/control_locator_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 6c84d8bbf4..e257b9fb15 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -200,5 +200,18 @@ ) 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_control_assets, [94, 95, 96])).to eq([79, 87, 95]) + end + end end end From 230b4d09371083ad5260c49cab329d9f10cd06a1 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 14:39:11 +0100 Subject: [PATCH 62/82] Fixed weird rubocop/prettier loop --- spec/models/cherrypick_task/control_locator_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index e257b9fb15..2257862af7 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -178,7 +178,7 @@ before { allow(instance).to receive(:control_placement_type).and_return('invalid_type') } it 'raises an error about invalid placement type' do - expect do instance.control_positions(0) end.to raise_error( + expect { instance.control_positions(0) }.to raise_error( StandardError, 'Control placement type is not set or is invalid' ) @@ -194,7 +194,7 @@ before { allow(instance).to receive_messages(control_placement_type: 'fixed', convert_control_assets: [1, 2, 3]) } it 'raises an error about incompatibility' do - expect do instance.control_positions(0) end.to raise_error( + expect { instance.control_positions(0) }.to raise_error( StandardError, 'The control plate and plate template are incompatible' ) From 5bdc03ea5e21e9fd39959d90c8bfa4ec5bf45f03 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 17:09:56 +0100 Subject: [PATCH 63/82] Refactored method for handling incompatible plates --- app/models/cherrypick_task/control_locator.rb | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 3ffdf72199..a8ec754010 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -84,6 +84,18 @@ def control_positions(num_plate) 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 # If num plate is equal to the available positions, the cycle is going to be repeated. @@ -125,7 +137,7 @@ def handle_control_placement_type(placement_type, num_plate) # 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_control_assets(control_assets) + def convert_assets(control_assets) valid_map, invalid_map = create_plate_maps control_assets.map do |id| @@ -134,23 +146,10 @@ def convert_control_assets(control_assets) end end - def handle_incompatible_plates(converted_control_assets) - template_wells = @plate_template.wells.map(&:map_id) - converted_template_assets = convert_control_assets(template_wells) - - converted_control_assets.intersect?(converted_template_assets) - end - def fixed_positions_from_available control_assets = @control_source_plate.wells.joins(:samples) control_wells = control_assets.map(&:map_id) - converted_assets = convert_control_assets(control_wells) - - if handle_incompatible_plates(converted_assets) - raise StandardError, 'The control plate and plate template are incompatible' - end - - converted_assets + convert_assets(control_wells) end # The invalid and valid maps are hash maps to represent a plate that maps A1 -> 1, A2 -> 2, etc, From 0e8b3aef5964862bbae204191dc0418763e6ca72 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 17:10:22 +0100 Subject: [PATCH 64/82] Display an error to user if plates are incompatible --- app/models/cherrypick_task.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index db09320c70..c74169a021 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -105,6 +105,11 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller, tem template, 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 From 59d9c5b1c2dc2a153d58d0d476b44a9bf577b0d8 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 17:10:52 +0100 Subject: [PATCH 65/82] Modified existing tests --- spec/models/tasks/cherrypick_task_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/models/tasks/cherrypick_task_spec.rb b/spec/models/tasks/cherrypick_task_spec.rb index 79f112340b..97e9f98796 100644 --- a/spec/models/tasks/cherrypick_task_spec.rb +++ b/spec/models/tasks/cherrypick_task_spec.rb @@ -40,6 +40,7 @@ def requests_for_plate(plate) control_source_plate: control_plate, template: template ).and_return(locator) + allow(locator).to receive(:handle_incompatible_plates) end let(:instance) { described_class.new } @@ -91,6 +92,7 @@ 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 @@ -119,6 +121,7 @@ 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 } @@ -147,6 +150,7 @@ 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 } From 36a9dc0157b9c2a84022340db243584c7c0766e9 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 17:11:16 +0100 Subject: [PATCH 66/82] Added test coverage for expecting a displayed error --- spec/models/cherrypick_task/control_locator_spec.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 2257862af7..bbd24683ad 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -191,13 +191,10 @@ let(:num_control_wells) { 2 } let(:wells_to_leave_free) { [] } - before { allow(instance).to receive_messages(control_placement_type: 'fixed', convert_control_assets: [1, 2, 3]) } + before { allow(instance).to receive_messages(control_placement_type: 'fixed', convert_assets: [1, 2, 3]) } - it 'raises an error about incompatibility' do - expect { instance.control_positions(0) }.to raise_error( - StandardError, - 'The control plate and plate template are incompatible' - ) + it 'displays an error about incompatibility' do + expect(instance.handle_incompatible_plates).to be_falsey end end @@ -210,7 +207,7 @@ before { allow(instance).to receive_messages(control_placement_type: 'fixed') } it 'they are given the correct position IDs' do - expect(instance.send(:convert_control_assets, [94, 95, 96])).to eq([79, 87, 95]) + expect(instance.send(:convert_assets, [94, 95, 96])).to eq([79, 87, 95]) end end end From c2dda46fd3bf946b47d8607bd57e3a19396e9b40 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 17:28:10 +0100 Subject: [PATCH 67/82] Added a test for valid fixed control placement --- spec/models/cherrypick_task/control_locator_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index bbd24683ad..a0b153b293 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -185,6 +185,19 @@ end end + context 'when the control placement type is set to 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 'does not raise an error' do + expect(instance.control_positions(0)).to eq([]) + end + end + context 'when the control plate and plate template are incompatible' do let(:batch_id) { 1 } let(:total_wells) { 96 } From 00d5d9b2d174cd27a5573c27d058ae1da75bf581 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 20:03:32 +0100 Subject: [PATCH 68/82] Improved test coverage for the control locator --- .../cherrypick_task/control_locator_spec.rb | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index a0b153b293..4b3ad8f7e1 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -10,7 +10,7 @@ num_control_wells: num_control_wells, wells_to_leave_free: wells_to_leave_free, control_source_plate: create(:control_plate), - template: create(:plate_template) + template: create(:plate_template_with_well) ) end @@ -185,29 +185,22 @@ end end - context 'when the control placement type is set to 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 'does not raise an error' do - expect(instance.control_positions(0)).to eq([]) - 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 { allow(instance).to receive_messages(control_placement_type: 'fixed', convert_assets: [1, 2, 3]) } + before do + allow(instance).to receive_messages( + control_placement_type: 'fixed', + convert_assets: [1, 2, 3], + control_source_plate: create(:control_plate) + ) + end it 'displays an error about incompatibility' do - expect(instance.handle_incompatible_plates).to be_falsey + expect(instance.handle_incompatible_plates).to be_truthy end end From 11650906218ab1d3a8019438bbbafc2c3b8165fa Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Tue, 1 Oct 2024 20:45:17 +0100 Subject: [PATCH 69/82] Improved test coverage (again) --- .../cherrypick_task/control_locator_spec.rb | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/spec/models/cherrypick_task/control_locator_spec.rb b/spec/models/cherrypick_task/control_locator_spec.rb index 4b3ad8f7e1..ee13130967 100644 --- a/spec/models/cherrypick_task/control_locator_spec.rb +++ b/spec/models/cherrypick_task/control_locator_spec.rb @@ -199,7 +199,7 @@ ) end - it 'displays an error about incompatibility' do + it 'returns and displays an error message' do expect(instance.handle_incompatible_plates).to be_truthy end end @@ -216,5 +216,18 @@ 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 From 80954a053c26c2e29e05ced6eec8a06fe7b39fb3 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 09:55:35 +0100 Subject: [PATCH 70/82] Add UAT action to generate randomised FluidX barcodes --- .../uat_actions/generate_fluidx_barcodes.rb | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 app/uat_actions/uat_actions/generate_fluidx_barcodes.rb diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb new file mode 100644 index 0000000000..bd5e18854c --- /dev/null +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +# UAT action to generate randomised FluidX barcodes. +class UatActions::GenerateFluidxBarcodes < UatActions + self.title = 'Generate FluidX Barcodes' + self.description = 'Generate randomised FluidX barcodes' + self.category = :auxiliary_data + + validates :barcode_count, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 96 } + validates :barcode_prefix, + presence: true, + length: { + is: 2 + }, + format: { + with: /\A[A-Z]{2}\z/, + message: 'only allows two uppercase letters' + } + validates :barcode_index, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 900 } + + form_field :barcode_count, + :number_field, + label: 'Number of barcodes', + help: 'The number of FluidX barcodes that should be generated', + options: { + min: 1, + max: 96, + value: 1 + } + form_field :barcode_prefix, + :text_field, + label: 'Barcode prefix', + help: 'The prefix to be used for the barcodes', + options: { + maxlength: 2, + oninput: 'javascript:this.value=this.value.toUpperCase().replace(/[^A-Z]/g, "")', + value: 'TS' + } + form_field :barcode_index, + :number_field, + label: 'Barcode index', + help: 'The starting index to make a sequential tail for the barcodes', + options: { + min: 1, + max: 900, + value: 1 + } + + def perform + random = Array.new(6) { rand(0..9) }.join # uniform distribution of digits + report['barcodes'] = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) + true + end + + private + + # Generates an array of barcodes with the specified count, prefix, common + # random part, and starting index for the sequential tail. It attempts to + # generate unique barcodes, iterating up to 5 times before giving up. + # + # @param count [Integer] the number of barcodes to generate + # @param prefix [String] the prefix to be used for the barcodes + # @param random [String] a common random part for the barcodes + # @param index [Integer] the starting index for the sequential tail + # @return [Array] an array of unique barcodes + # @raise [StandardError] if it fails to generate unique barcodes + def generate_barcodes(count, prefix, random, index) + barcodes = [] + 5.times do # Max 5 iterations to generate unique barcodes. + barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) + return barcodes if barcodes.size == barcode_count.to_i + count = barcode_count.to_i - barcodes.size + index += count + end + raise StandardError, 'Failed to generate unique barcodes' + end + + # Filters out the barcodes that already exist in the database. + # + # @param barcodes [Array] an array of barcodes + # @return [Array] an array of unique barcodes + def filter_barcodes(barcodes) + barcodes - Barcode.where(barcode: barcodes).pluck(:barcode) + end + + # Builds an array of barcodes with the specified count, prefix, common + # random part, and starting index for the sequential tail. + # + # @param count [Integer] the number of barcodes to generate + # @param prefix [String] the prefix to be used for the barcodes + # @param random [String] a common random part for the barcodes + # @param index [Integer] the starting index for the suffix + # @return [Array] an array of barcodes + def build_barcodes(count, prefix, random, index) + (index...(index + count)).map do |counter| + suffix = counter.to_s.rjust(2, '0') # Min 2 suffix digits + random = random[0, 8 - suffix.length] # 8 FluidX digits minus suffix + "#{prefix}#{random}#{suffix}" + end + end +end From 5bb6bb8c41596fe68e05fbf7f59d9924c656f2f2 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 10:22:42 +0100 Subject: [PATCH 71/82] Fill errors collection instead of raising exception --- .../uat_actions/generate_fluidx_barcodes.rb | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb index bd5e18854c..b11cb6e713 100644 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -46,10 +46,22 @@ class UatActions::GenerateFluidxBarcodes < UatActions value: 1 } + # This method is called from the save method after validations have passed. + # If the return value is true, the report hash populated by the action is + # used for rendering the response. If the return value is false, the errors + # collection is used. + # + # @return [Boolean] true if the action was successful; false otherwise def perform random = Array.new(6) { rand(0..9) }.join # uniform distribution of digits - report['barcodes'] = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) - true + barcodes = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) + if barcodes.size == barcode_count.to_i + report['barcodes'] = barcodes + true + else + errors.add(:base, 'Failed to generate unique barcodes') + false + end end private @@ -72,7 +84,6 @@ def generate_barcodes(count, prefix, random, index) count = barcode_count.to_i - barcodes.size index += count end - raise StandardError, 'Failed to generate unique barcodes' end # Filters out the barcodes that already exist in the database. From 8531eae25bacaad6c09896bacb31c4dbf36ac260 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 10:27:42 +0100 Subject: [PATCH 72/82] Continue index in the next iteration --- app/uat_actions/uat_actions/generate_fluidx_barcodes.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb index b11cb6e713..84cd8bec58 100644 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -81,8 +81,8 @@ def generate_barcodes(count, prefix, random, index) 5.times do # Max 5 iterations to generate unique barcodes. barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) return barcodes if barcodes.size == barcode_count.to_i - count = barcode_count.to_i - barcodes.size - index += count + count = barcode_count.to_i - barcodes.size # More to generate. + index += barcode_count.to_i # Continue index. end end From 693f6b1d355c4569ece45110e217fed342c6b4df Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Wed, 2 Oct 2024 10:49:55 +0100 Subject: [PATCH 73/82] Converted control locator init method to use single arg params --- app/models/cherrypick_task.rb | 36 ++++++++----------- app/models/cherrypick_task/control_locator.rb | 32 +++++++++-------- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/app/models/cherrypick_task.rb b/app/models/cherrypick_task.rb index c74169a021..031a599eb0 100644 --- a/app/models/cherrypick_task.rb +++ b/app/models/cherrypick_task.rb @@ -22,25 +22,16 @@ class CherrypickTask < Task # rubocop:todo Metrics/ClassLength # @return [CherrypickTask::ControlLocator] A generator of control locations # - # rubocop:disable Metrics/ParameterLists - def new_control_locator( - batch_id, - total_wells, - num_control_wells, - template, - control_source_plate:, - wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE - ) + def new_control_locator(params) CherrypickTask::ControlLocator.new( - batch_id:, - total_wells:, - num_control_wells:, - wells_to_leave_free:, - control_source_plate:, - template: + 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 - # rubocop:enable Metrics/ParameterLists # # Cherrypick tasks are directly coupled to the previous task, due to the awkward @@ -99,12 +90,15 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller, tem control_assets = control_source_plate.wells.joins(:samples) control_locator = new_control_locator( - batch.id, - current_destination_plate.size, - control_assets.count, - template, - control_source_plate: + { + 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? diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index a8ec754010..42f3e250e3 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -41,22 +41,24 @@ class CherrypickTask::ControlLocator # 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 - - # rubocop:disable Metrics/ParameterLists - def initialize(batch_id:, total_wells:, num_control_wells:, template:, control_source_plate:, 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 - @control_source_plate = control_source_plate - @plate_template = template + # @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 + # - :available_positions [Enumerable] Array or range indicating the available positions for 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 - # rubocop:enable Metrics/ParameterLists # # Returns a list with the destination positions for the control wells distributed randomly From c326657a519dc2f8b48625b1198401d97a5544d1 Mon Sep 17 00:00:00 2001 From: Shiv <44001656+SHIV5T3R@users.noreply.github.com> Date: Wed, 2 Oct 2024 11:02:25 +0100 Subject: [PATCH 74/82] Removed 'available_positions' from param documentation --- app/models/cherrypick_task/control_locator.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/cherrypick_task/control_locator.rb b/app/models/cherrypick_task/control_locator.rb index 42f3e250e3..2811dc3b89 100644 --- a/app/models/cherrypick_task/control_locator.rb +++ b/app/models/cherrypick_task/control_locator.rb @@ -46,7 +46,6 @@ class CherrypickTask::ControlLocator # - :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 - # - :available_positions [Enumerable] Array or range indicating the available positions for controls # - :control_source_plate [ControlPlate] The plate to source controls from # - :template [PlateTemplate] The template of the destination plate From f7a918abb6ae4d341bc2e2f3cc359f42b88b4600 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 22:23:53 +0100 Subject: [PATCH 75/82] Separate random part and index with zero --- .../uat_actions/generate_fluidx_barcodes.rb | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb index 84cd8bec58..7f0441e756 100644 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -1,10 +1,17 @@ # frozen_string_literal: true # UAT action to generate randomised FluidX barcodes. +# : Ten characters in total +# : two uppercase letters +# : six random digits; may be truncated from the end to fit the length +# : +# : one digit that separates random part and index, i.e. '0' +# : sequential tail, 1 to 3 digits, e.g., '9', '99', and '999' class UatActions::GenerateFluidxBarcodes < UatActions self.title = 'Generate FluidX Barcodes' self.description = 'Generate randomised FluidX barcodes' self.category = :auxiliary_data + self.max_number_of_iterations = 5 validates :barcode_count, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 96 } validates :barcode_prefix, @@ -64,21 +71,27 @@ def perform end end + # Returns a default copy of the UatAction which will be used to fill in the form. + # + # @return [UatActions::GenerateFluidxBarcodes] a default object for rendering a form + def self.default + new(barcode_count: '1', barcode_prefix: 'TS', barcode_index: '1') + end + private - # Generates an array of barcodes with the specified count, prefix, common - # random part, and starting index for the sequential tail. It attempts to - # generate unique barcodes, iterating up to 5 times before giving up. + # Generates an array of barcodes with the specified count, prefix, random + # part, and starting index for the sequential tail. It attempts to generate + # unique barcodes, iterating up to max_number_of_iterations before giving up. # # @param count [Integer] the number of barcodes to generate # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] a common random part for the barcodes + # @param random [String] random part for the barcodes # @param index [Integer] the starting index for the sequential tail # @return [Array] an array of unique barcodes - # @raise [StandardError] if it fails to generate unique barcodes def generate_barcodes(count, prefix, random, index) barcodes = [] - 5.times do # Max 5 iterations to generate unique barcodes. + max_number_of_iterations.times do barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) return barcodes if barcodes.size == barcode_count.to_i count = barcode_count.to_i - barcodes.size # More to generate. @@ -94,17 +107,17 @@ def filter_barcodes(barcodes) barcodes - Barcode.where(barcode: barcodes).pluck(:barcode) end - # Builds an array of barcodes with the specified count, prefix, common - # random part, and starting index for the sequential tail. + # Builds an array of barcodes with the specified count, prefix, random + # part, and starting index for the sequential tail. # # @param count [Integer] the number of barcodes to generate # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] a common random part for the barcodes + # @param random [String] random part for the barcodes # @param index [Integer] the starting index for the suffix # @return [Array] an array of barcodes def build_barcodes(count, prefix, random, index) (index...(index + count)).map do |counter| - suffix = counter.to_s.rjust(2, '0') # Min 2 suffix digits + suffix = '0' + counter.to_s random = random[0, 8 - suffix.length] # 8 FluidX digits minus suffix "#{prefix}#{random}#{suffix}" end From e81b4a0e78d6119ce668be94cf66cf10257b1115 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 22:53:35 +0100 Subject: [PATCH 76/82] Deleted file as it does not belong to this PR --- .../uat_actions/generate_fluidx_barcodes.rb | 125 ------------------ 1 file changed, 125 deletions(-) delete mode 100644 app/uat_actions/uat_actions/generate_fluidx_barcodes.rb diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb deleted file mode 100644 index 7f0441e756..0000000000 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ /dev/null @@ -1,125 +0,0 @@ -# frozen_string_literal: true - -# UAT action to generate randomised FluidX barcodes. -# : Ten characters in total -# : two uppercase letters -# : six random digits; may be truncated from the end to fit the length -# : -# : one digit that separates random part and index, i.e. '0' -# : sequential tail, 1 to 3 digits, e.g., '9', '99', and '999' -class UatActions::GenerateFluidxBarcodes < UatActions - self.title = 'Generate FluidX Barcodes' - self.description = 'Generate randomised FluidX barcodes' - self.category = :auxiliary_data - self.max_number_of_iterations = 5 - - validates :barcode_count, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 96 } - validates :barcode_prefix, - presence: true, - length: { - is: 2 - }, - format: { - with: /\A[A-Z]{2}\z/, - message: 'only allows two uppercase letters' - } - validates :barcode_index, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 900 } - - form_field :barcode_count, - :number_field, - label: 'Number of barcodes', - help: 'The number of FluidX barcodes that should be generated', - options: { - min: 1, - max: 96, - value: 1 - } - form_field :barcode_prefix, - :text_field, - label: 'Barcode prefix', - help: 'The prefix to be used for the barcodes', - options: { - maxlength: 2, - oninput: 'javascript:this.value=this.value.toUpperCase().replace(/[^A-Z]/g, "")', - value: 'TS' - } - form_field :barcode_index, - :number_field, - label: 'Barcode index', - help: 'The starting index to make a sequential tail for the barcodes', - options: { - min: 1, - max: 900, - value: 1 - } - - # This method is called from the save method after validations have passed. - # If the return value is true, the report hash populated by the action is - # used for rendering the response. If the return value is false, the errors - # collection is used. - # - # @return [Boolean] true if the action was successful; false otherwise - def perform - random = Array.new(6) { rand(0..9) }.join # uniform distribution of digits - barcodes = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) - if barcodes.size == barcode_count.to_i - report['barcodes'] = barcodes - true - else - errors.add(:base, 'Failed to generate unique barcodes') - false - end - end - - # Returns a default copy of the UatAction which will be used to fill in the form. - # - # @return [UatActions::GenerateFluidxBarcodes] a default object for rendering a form - def self.default - new(barcode_count: '1', barcode_prefix: 'TS', barcode_index: '1') - end - - private - - # Generates an array of barcodes with the specified count, prefix, random - # part, and starting index for the sequential tail. It attempts to generate - # unique barcodes, iterating up to max_number_of_iterations before giving up. - # - # @param count [Integer] the number of barcodes to generate - # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] random part for the barcodes - # @param index [Integer] the starting index for the sequential tail - # @return [Array] an array of unique barcodes - def generate_barcodes(count, prefix, random, index) - barcodes = [] - max_number_of_iterations.times do - barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) - return barcodes if barcodes.size == barcode_count.to_i - count = barcode_count.to_i - barcodes.size # More to generate. - index += barcode_count.to_i # Continue index. - end - end - - # Filters out the barcodes that already exist in the database. - # - # @param barcodes [Array] an array of barcodes - # @return [Array] an array of unique barcodes - def filter_barcodes(barcodes) - barcodes - Barcode.where(barcode: barcodes).pluck(:barcode) - end - - # Builds an array of barcodes with the specified count, prefix, random - # part, and starting index for the sequential tail. - # - # @param count [Integer] the number of barcodes to generate - # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] random part for the barcodes - # @param index [Integer] the starting index for the suffix - # @return [Array] an array of barcodes - def build_barcodes(count, prefix, random, index) - (index...(index + count)).map do |counter| - suffix = '0' + counter.to_s - random = random[0, 8 - suffix.length] # 8 FluidX digits minus suffix - "#{prefix}#{random}#{suffix}" - end - end -end From c9a058a87c68603da5317d8d7703430ad329c951 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 22:58:42 +0100 Subject: [PATCH 77/82] Rubocop --- spec/models/asset_links_create_edge_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index cffbf33376..63bff1b4b5 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe AssetLink, type: :model do - # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable + # rubocop:disable RSpec/InstanceVariable,Metrics/MethodLength,RSpec/ExampleLength,RSpec/MultipleExpectations # Test the overridden create_edge class method. describe '.create_edge' do # Wait for child processes to finish. @@ -97,7 +97,7 @@ def wait_for_child_processes(pids) # Parent wait_for_child_processes([pid1, pid2]) - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + expect(described_class.where(ancestor:, descendant:).count).to eq(1) @edge = edge = described_class.last expect(edge.ancestor).to eq(ancestor) expect(edge.descendant).to eq(descendant) @@ -174,7 +174,7 @@ def wait_for_child_processes(pids) # Parent wait_for_child_processes([pid1, pid2]) - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + expect(described_class.where(ancestor:, descendant:).count).to eq(1) @edge = edge = described_class.last expect(edge.ancestor).to eq(ancestor) expect(edge.descendant).to eq(descendant) @@ -266,7 +266,7 @@ def wait_for_child_processes(pids) # Parent wait_for_child_processes([pid1, pid2]) - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + expect(described_class.where(ancestor:, descendant:).count).to eq(1) @edge = edge = described_class.last expect(edge.ancestor).to eq(ancestor) expect(edge.descendant).to eq(descendant) @@ -276,5 +276,5 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.close end end - # rubocop:enable RSpec/ExampleLength,RSpec/InstanceVariable + # rubocop:enable RSpec/InstanceVariable,Metrics/MethodLength,RSpec/ExampleLength,RSpec/MultipleExpectations end From a9ff092137bf82aa40e0b4e689f479de05a8d4ab Mon Sep 17 00:00:00 2001 From: yoldas Date: Thu, 3 Oct 2024 09:25:44 +0100 Subject: [PATCH 78/82] Deleting the part-1 test as it is conflicting with part-2 --- .../remove_duplicate_asset_links_spec.rb | 51 ------------------- 1 file changed, 51 deletions(-) delete mode 100644 spec/tasks/support/remove_duplicate_asset_links_spec.rb diff --git a/spec/tasks/support/remove_duplicate_asset_links_spec.rb b/spec/tasks/support/remove_duplicate_asset_links_spec.rb deleted file mode 100644 index b34eebdc98..0000000000 --- a/spec/tasks/support/remove_duplicate_asset_links_spec.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -# rubocop:disable RSpec/ExampleLength, RSpec/MultipleExpectations,RSpec/MultipleMemoizedHelpers -RSpec.describe 'support:remove_duplicate_asset_links', type: :task do - let(:clear_tasks) { Rake.application.clear } - let(:load_tasks) { Rails.application.load_tasks } - let(:task_reenable) { Rake::Task[self.class.top_level_description].reenable } - let(:task_invoke) { Rake::Task[self.class.top_level_description].invoke(csv_file_path) } - let(:csv_file_path) { 'tmp/deleted_asset_links.csv' } - let(:links) { create_list(:asset_link, 5) } - let(:duplicate_links) do - links.map do |link| - duplicate = AssetLink.new(ancestor: link.ancestor, descendant: link.descendant, created_at: 1.day.ago) - duplicate.save!(validate: false) # Needs to be saved without validation - duplicate - end - end - - before do - clear_tasks - load_tasks - task_reenable - links - duplicate_links - end - - after { File.delete(csv_file_path) if File.exist?(csv_file_path) } - - it 'removes all duplicate links except the most recently created ones' do - expect(AssetLink.count).to eq(links.size + duplicate_links.size) - task_invoke - expect(AssetLink.count).to eq(links.size) # most recent links should be kept - expect(AssetLink.exists?(links.first.id)).to be true - expect(AssetLink.exists?(duplicate_links.first.id)).to be false - end - - it 'exports the removed duplicates to a CSV file' do - task_invoke - expect(File.exist?(csv_file_path)).to be true - csv = CSV.read(Rails.root.join(csv_file_path)) - expect(csv.size).to eq(duplicate_links.size + 1) # With header. - expect(csv.first).to eq(AssetLink.column_names) - (1..duplicate_links.size).each do |i| - expected_row = duplicate_links[i - 1].attributes.values.map { |value| value&.to_s } - expect(csv[i]).to eq(expected_row) - end - end -end -# rubocop:enable RSpec/ExampleLength, RSpec/MultipleExpectations,RSpec/MultipleMemoizedHelpers From 7cd7d9521426e899b57588e096e30c7b4796ec0c Mon Sep 17 00:00:00 2001 From: Wendy Yang Date: Thu, 3 Oct 2024 11:18:12 +0100 Subject: [PATCH 79/82] add class AkerBarcode to handle calling this method elsewhere --- app/models/barcode/format_handlers.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/app/models/barcode/format_handlers.rb b/app/models/barcode/format_handlers.rb index 3ccaad49a3..2976750659 100644 --- a/app/models/barcode/format_handlers.rb +++ b/app/models/barcode/format_handlers.rb @@ -3,6 +3,22 @@ require 'sanger_barcode_format' # A collection of supported formats module Barcode::FormatHandlers + # Define the AkerBarcode class or module + class AkerBarcode + # Define necessary methods and attributes + def initialize(barcode) + @barcode = barcode + end + + def human_barcode + @barcode + end + + def machine_barcode + human_barcode + end + end + # Include in barcode formats which can not be rendered as EAN13s module Ean13Incompatible def ean13_barcode? From 4cbfdfe3a095daca5561c23c109eb52ed7632dac Mon Sep 17 00:00:00 2001 From: yoldas Date: Thu, 3 Oct 2024 13:47:21 +0100 Subject: [PATCH 80/82] Update .release-version to 14.43.2 --- .release-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-version b/.release-version index e7d355aaa2..a63a66348d 100644 --- a/.release-version +++ b/.release-version @@ -1 +1 @@ -14.43.0 +14.43.2 From bf10f847d34d89e6fbdc2f5bb30fa3dcab1e7fd6 Mon Sep 17 00:00:00 2001 From: Wendy Yang Date: Thu, 3 Oct 2024 14:05:47 +0100 Subject: [PATCH 81/82] change the method & add test for the method --- app/models/barcode/format_handlers.rb | 20 ++++---------------- spec/models/barcode/format_handlers_spec.rb | 7 +++++++ 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/app/models/barcode/format_handlers.rb b/app/models/barcode/format_handlers.rb index 2976750659..6eb42646db 100644 --- a/app/models/barcode/format_handlers.rb +++ b/app/models/barcode/format_handlers.rb @@ -3,22 +3,6 @@ require 'sanger_barcode_format' # A collection of supported formats module Barcode::FormatHandlers - # Define the AkerBarcode class or module - class AkerBarcode - # Define necessary methods and attributes - def initialize(barcode) - @barcode = barcode - end - - def human_barcode - @barcode - end - - def machine_barcode - human_barcode - end - end - # Include in barcode formats which can not be rendered as EAN13s module Ean13Incompatible def ean13_barcode? @@ -594,4 +578,8 @@ class IbdResponse < BaseRegExBarcode class Rvi < BaseRegExBarcode self.format = /\A(?RVI)-(?[0-9]{6,})\z/ end + + class AkerBarcode < BaseRegExBarcode + self.format = /\A(?Aker)-(?[0-9]{2,})\z/ + end end diff --git a/spec/models/barcode/format_handlers_spec.rb b/spec/models/barcode/format_handlers_spec.rb index b363388fe1..c6cdb1958f 100644 --- a/spec/models/barcode/format_handlers_spec.rb +++ b/spec/models/barcode/format_handlers_spec.rb @@ -420,5 +420,12 @@ def self.it_has_an_invalid_barcode(barcode) it_has_an_invalid_barcode 'IRVI-123456' end + describe Barcode::FormatHandlers::AkerBarcode do + it_has_a_valid_barcode 'Aker-11', prefix: 'Aker', number: 11 + it_has_an_invalid_barcode 'SQPD-12345678-234233890-WD' + it_has_an_invalid_barcode 'RVI123456' + it_has_an_invalid_barcode 'IRVI-123456' + end + # rubocop:enable RSpec/EmptyExampleGroup end From 7eb412f149e5d58eb71ebe189500682712925f11 Mon Sep 17 00:00:00 2001 From: Wendy Yang Date: Thu, 3 Oct 2024 15:28:03 +0100 Subject: [PATCH 82/82] add comment to the added method --- app/models/barcode/format_handlers.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/barcode/format_handlers.rb b/app/models/barcode/format_handlers.rb index 6eb42646db..9517412b8c 100644 --- a/app/models/barcode/format_handlers.rb +++ b/app/models/barcode/format_handlers.rb @@ -579,6 +579,9 @@ class Rvi < BaseRegExBarcode self.format = /\A(?RVI)-(?[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(?Aker)-(?[0-9]{2,})\z/ end