Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DPL-823 [Part 1 - download] Import PBMC pool plates into Sequencescape #4017

Merged
merged 18 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/sdb/sample_manifests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ def show
@study_id = @sample_manifest.study_id
@samples = @sample_manifest.samples.paginate(page: params[:page])
end

def new # rubocop:todo Metrics/AbcSize
params[:only_first_label] ||= false
@sample_manifest = SampleManifest.new(new_manifest_params)
Expand Down
22 changes: 14 additions & 8 deletions app/jobs/sample_manifest/generate_wells_job.rb
Original file line number Diff line number Diff line change
@@ -1,25 +1,31 @@
# frozen_string_literal: true
# Generates wells for plate sample manifests
# Generates wells and sample manifest assets, for a plate sample manifest.
SampleManifest::GenerateWellsJob =
Struct.new(:sample_manifest_id, :map_ids_to_sample_ids, :plate_id) do
Struct.new(:sample_manifest_id, :map_ids_to_sanger_sample_ids, :plate_id) do
def perform
ActiveRecord::Base.transaction do
# Ensure the order of the wells are maintained
maps = Map.find(map_ids).index_by(&:id)
well_data = map_ids_to_sample_ids.map { |map_id, sample_id| [maps[map_id], sample_id] }
map_ids_to_sanger_sample_ids.each { |map_id, sanger_sample_id| create_well(map_id, sanger_sample_id) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the work around for this variable renaming pain is to use that Ruby syntax where you define a method to pass the values to instead of write it out in full like you have here. I don't know if this works when you have two values coming in, but I know you can push the values through another method when you write it something like:

Suggested change
map_ids_to_sanger_sample_ids.each { |map_id, sanger_sample_id| create_well(map_id, sanger_sample_id) }
map_ids_to_sanger_sample_ids.each &:create_well


sample_manifest.core_behaviour.generate_wells_job(well_data, plate)
RequestFactory.create_assets_requests(plate.wells, sample_manifest.study)

plate.events.created_using_sample_manifest!(sample_manifest.user)
end
end

def map_ids
map_ids_to_sample_ids.map(&:first)
def create_well(map_id, sanger_sample_ids)
plate.wells.create!(map: Map.find(map_id)) { |well| create_sample_manifest_assets(well, sanger_sample_ids) }
end

def plate
Plate.find(plate_id)
end

def create_sample_manifest_assets(well, sanger_sample_ids)
sanger_sample_ids.each do |sanger_sample_id|
SampleManifestAsset.create(sanger_sample_id: sanger_sample_id, asset: well, sample_manifest: sample_manifest)
end
end

def sample_manifest
SampleManifest.find(sample_manifest_id)
end
Expand Down
10 changes: 9 additions & 1 deletion app/models/sample_manifest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
# for the potential samples. It also generates a {SampleManifestExcel}
# spreadsheet which gets sent to the customer.
#
# The labware that gets generate is determined by the {#asset_type} which
# The labware that gets generated is determined by the {#asset_type} which
# switches out the {#core_behaviour} module {SampleManifest::CoreBehaviour}.
# This is concerned with generating {Labware} and {Receptacle receptacles},
# generating any event specific to the asset type, and setting manifest specific
Expand Down Expand Up @@ -49,6 +49,7 @@ def self.included(base)
has_uploaded_document :generated, differentiator: 'generated'

attr_accessor :override, :only_first_label
attr_writer :rows_per_well
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SampleManifest#rows_per_well is a writable attribute


class_attribute :spreadsheet_offset
class_attribute :spreadsheet_header_row
Expand Down Expand Up @@ -128,6 +129,13 @@ def default_filename
"#{study_id}stdy_manifest_#{id}_#{created_at.to_formatted_s(:dmy)}"
end

# Use a default value of 1 for rows_per_well if not set
def rows_per_well
1
# TODO: replace above line with below line to turn the rows_per_well feature on, when DPL-823 is complete
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO found

# @rows_per_well || 1
end

scope :pending_manifests,
-> {
order(id: :desc).includes(:uploaded_document).references(:uploaded_document).where(documents: { id: nil })
Expand Down
10 changes: 9 additions & 1 deletion app/models/sample_manifest/generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,21 @@ def execute_print_job
end

def attributes
params.except(:template, :barcode_printer, :only_first_label).merge(user: user, asset_type: asset_type)
params
.except(:template, :barcode_printer, :only_first_label)
.merge(user: user, asset_type: asset_type, rows_per_well: rows_per_well)
end

def asset_type
configuration.manifest_types.find_by(params[:template]).asset_type
end

# Retrieves the value of the rows_per_well attribute from the manifest_types.yml config.
# If the attribute is not set, it returns nil.
def rows_per_well
configuration.manifest_types.find_by(params[:template]).rows_per_well
end

def only_first_label
ActiveRecord::Type::Boolean.new.cast(params[:only_first_label])
end
Expand Down
108 changes: 47 additions & 61 deletions app/models/sample_manifest/plate_behaviour.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ def initialize(manifest)

def generate
@plates = generate_plates(purpose)

sanger_sample_ids = insert_sanger_sample_ids
well_data = build_well_data(sanger_sample_ids)

build_wells_async(well_data)

@details_array = build_details_array(well_data)

@manifest.update!(barcodes: @plates.map(&:human_barcode))
end

def acceptable_purposes
Expand All @@ -27,24 +36,6 @@ def included_resources
[{ sample: :sample_metadata, asset: { plate: :barcodes } }]
end

def generate_wells(well_data, plates)
# Generate the wells, samples & requests asynchronously.
generate_wells_for_plates(well_data, plates) do |this_plates_well_data, plate|
generate_wells_asynchronously(this_plates_well_data.map { |map, sample_id| [map.id, sample_id] }, plate.id)
end

# Ensure we maintain the information we need for printing labels and generating
# the CSV file
@plates = plates.sort_by(&:human_barcode)

@details_array =
plates.flat_map do |plate|
well_data
.slice!(0, plate.size)
.map { |map, sample_id| { barcode: plate.human_barcode, position: map.description, sample_id: sample_id } }
end
end

def io_samples
samples.map do |sample|
container = sample.primary_receptacle
Expand Down Expand Up @@ -83,65 +74,60 @@ def labware=(labware)

# We use the barcodes here as we may need to reference the plates before the delayed job has passed
def labware
plates | labware_from_barcodes
plates | Labware.with_barcode(barcodes)
end
alias printables labware

# Called by {SampleManifest::GenerateWellsJob} and builds the wells
def generate_wells_job(wells_for_plate, plate)
wells_for_plate.map do |map, sanger_sample_id|
plate
.wells
.create!(map: map) do |well|
SampleManifestAsset.create(sanger_sample_id: sanger_sample_id, asset: well, sample_manifest: @manifest)
end
end
RequestFactory.create_assets_requests(plate.wells, study)
plate.events.created_using_sample_manifest!(@manifest.user)
end

private

# This method ensures that each of the plates is handled by an individual job. If it doesn't do this we run
# the risk that the 'handler' column in the database for the delayed job will not be large enough and will
# truncate the data.
def generate_wells_for_plates(well_data, plates)
cloned_well_data = well_data.dup
plates.each { |plate| yield(cloned_well_data.slice!(0, plate.size), plate) }
end

def labware_from_barcodes
Labware.with_barcode(barcodes)
def generate_plates(purpose)
Array.new(count) { purpose.create!(:without_wells) }.sort_by(&:human_barcode)
end

def generate_wells_asynchronously(map_ids_to_sample_ids, plate_id)
Delayed::Job.enqueue SampleManifest::GenerateWellsJob.new(@manifest.id, map_ids_to_sample_ids, plate_id)
def insert_sanger_sample_ids
sanger_sample_ids = generate_sanger_ids(@plates.sum(&:size) * @manifest.rows_per_well)
sanger_sample_ids.map do |sanger_sample_id|
SangerSampleId.generate_sanger_sample_id!(study.abbreviation, sanger_sample_id)
end
end

# rubocop:todo Metrics/MethodLength
def generate_plates(purpose) # rubocop:todo Metrics/AbcSize
study_abbreviation = study.abbreviation

well_data = []
plates = Array.new(count) { purpose.create!(:without_wells) }.sort_by(&:human_barcode)

plates.each do |plate|
sanger_sample_ids = generate_sanger_ids(plate.size)
# output:
# plate_id => { map_id => [sanger_sample_id, sanger_sample_id, ...] }
def build_well_data(sanger_sample_ids)
@plates.each_with_object({}) do |plate, well_data|
well_data[plate.id] = {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SampleManifest::PlateBehaviour::Base#build_well_data calls 'plate.id' 2 times

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SampleManifest::PlateBehaviour::Base#build_well_data refers to 'plate' more than self (maybe move it to another class?)


plate.maps.in_column_major_order.each do |well_map|
sanger_sample_id = sanger_sample_ids.shift
generated_sanger_sample_id = SangerSampleId.generate_sanger_sample_id!(study_abbreviation, sanger_sample_id)

well_data << [well_map, generated_sanger_sample_id]
well_data[plate.id][well_map.id] = sanger_sample_ids.shift(@manifest.rows_per_well)
end
end
end

generate_wells(well_data, plates)
@manifest.update!(barcodes: plates.map(&:human_barcode))
# Each of the plates is handled by an individual job.
# If it doesn't do this we run the risk that the 'handler' column in the database
# for the delayed job will not be large enough and will truncate the data.
def build_wells_async(well_data)
@plates.each do |plate|
Delayed::Job.enqueue SampleManifest::GenerateWellsJob.new(@manifest.id, well_data[plate.id], plate.id)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SampleManifest::PlateBehaviour::Base#build_wells_async calls 'plate.id' 2 times

end
end

plates
# output:
# [{barcode, position, sanger_sample_id}, {barcode, position, sanger_sample_id}, ...]
def build_details_array(well_data)
@details_array =
@plates.flat_map do |plate|
well_data[plate.id].flat_map do |map_id, sanger_sample_ids|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SampleManifest::PlateBehaviour::Base#build_details_array refers to 'plate' more than self (maybe move it to another class?)

sanger_sample_ids.map do |sanger_sample_id|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SampleManifest::PlateBehaviour::Base#build_details_array contains iterators nested 3 deep

{
barcode: plate.human_barcode,
position: plate.maps.find(map_id).description,
sample_id: sanger_sample_id
}
end
end
end
sdjmchattie marked this conversation as resolved.
Show resolved Hide resolved
end
# rubocop:enable Metrics/MethodLength
end

class Core < Base
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def each(&block)
manifest_types.each(&block)
end

# Hash version of the manifest_types.yml config file
def manifest_types
@manifest_types ||= {}
end
Expand Down Expand Up @@ -49,7 +50,7 @@ def <=>(other)
class ManifestType
include SequencescapeExcel::Helpers::Attributes

setup_attributes :name, :columns, :heading, :asset_type
setup_attributes :name, :columns, :heading, :asset_type, :rows_per_well

def initialize(attributes = {})
super
Expand All @@ -62,7 +63,8 @@ def to_a
def ==(other)
return false unless other.is_a?(self.class)

name == other.name && columns == other.columns && heading == other.heading && asset_type == other.asset_type
name == other.name && columns == other.columns && heading == other.heading && asset_type == other.asset_type &&
rows_per_well == other.rows_per_well
end
end

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
# scRNA Core pipeline purposes
# Most are defined in the Limber config, but the below also need to be defined in Sequencescape.
# They are in a separate file so it can be 'feature flagged off' until needed.
---
# The 'LRC PBMC Pools' purpose is controlled by Limber. However, it has been
# added here to create submission and request type records for scRNA Core cDNA
# Prep stage.
---
LRC PBMC Pools:
stock_plate: false
cherrypickable_target: false
# The 'LRC PBMC Pools Input' purpose is included here so that it's available for
# sample manifests.
LRC PBMC Pools Input:
stock_plate: true
cherrypickable_target: false
10 changes: 10 additions & 0 deletions config/sample_manifest_excel/manifest_types.yml
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,16 @@ plate_bioscan:
- :sample_type
- :donor_id
- :bioscan_control_type
plate_scrna_core_pools:
heading: "scRNA Core Pools Plate"
asset_type: "plate"
rows_per_well: 20
columns:
- :sanger_plate_id
- :well
- :sanger_sample_id
- :supplier_name
- :donor_id_mandatory
tube_default:
heading: "Default Tube"
asset_type: "1dtube"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true
class AddRowsPerWellToSampleManifests < ActiveRecord::Migration[6.0]
def change
add_column :sample_manifests, :rows_per_well, :integer
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2023_12_04_163029) do
ActiveRecord::Schema.define(version: 2024_02_14_144515) do

create_table "aliquot_indices", id: :integer, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci ROW_FORMAT=DYNAMIC", force: :cascade do |t|
t.integer "aliquot_id", null: false
Expand Down Expand Up @@ -1327,6 +1327,7 @@
t.string "password"
t.integer "purpose_id"
t.integer "tube_rack_purpose_id"
t.integer "rows_per_well"
t.index ["purpose_id"], name: "fk_rails_5627ab4aaa"
t.index ["study_id"], name: "index_sample_manifests_on_study_id"
t.index ["supplier_id"], name: "index_sample_manifests_on_supplier_id"
Expand Down
1 change: 1 addition & 0 deletions spec/data/sample_manifest_excel/extract/manifest_types.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ test_2:
test_3:
heading: "Yet Another test"
asset_type: "plate"
rows_per_well: 2
columns:
- :sanger_plate_id
- :well
Expand Down
9 changes: 9 additions & 0 deletions spec/data/sample_manifest_excel/manifest_types.yml
Original file line number Diff line number Diff line change
Expand Up @@ -701,3 +701,12 @@ tube_extraction:
- :sample_ebi_accession_number
- :donor_id
- :phenotype
pools_plate:
heading: "Pools Plate"
asset_type: "plate"
rows_per_well: 2
columns:
- :sanger_plate_id
- :well
- :sanger_sample_id
- :supplier_name
4 changes: 2 additions & 2 deletions spec/features/sample_manifests/create_manifest_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def load_manifest_spec
select(study.name, from: 'Study')
select(supplier.name, from: 'Supplier')
within('#sample_manifest_template') do
expect(page).to have_css('option', count: 7)
expect(page).to have_css('option', count: 8)
expect(page).not_to have_css('option', text: 'Default Tube')
end
select('Default Plate', from: 'Template')
Expand Down Expand Up @@ -66,7 +66,7 @@ def load_manifest_spec

it 'indicate the purpose field is used for plates only' do
visit(new_sample_manifest_path)
within('#sample_manifest_template') { expect(page).to have_css('option', count: 22) }
within('#sample_manifest_template') { expect(page).to have_css('option', count: 23) }
select(created_purpose.name, from: 'Purpose')
expect(page).to have_text('Used for plate manifests only')
end
Expand Down
Loading
Loading