Skip to content
Open
Show file tree
Hide file tree
Changes from all 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/location_reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def location_report_params # rubocop:todo Metrics/MethodLength
:end_date,
:barcodes,
:barcodes_text,
Comment on lines 63 to 64
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

location_report_params permits :barcodes and :barcodes_text twice. This duplication is redundant and can confuse future edits; remove the duplicates so each permitted key appears only once.

Suggested change
:barcodes,
:barcodes_text,

Copilot uses AI. Check for mistakes.
retention_instructions: [],
faculty_sponsor_ids: [],
plate_purpose_ids: []
)
Expand Down
32 changes: 26 additions & 6 deletions app/models/location_report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class LocationReport < ApplicationRecord
serialize :faculty_sponsor_ids, type: Array, coder: YAML
serialize :plate_purpose_ids, type: Array, coder: YAML
serialize :barcodes, type: Array, coder: YAML
serialize :retention_instructions, type: Array, coder: YAML
self.per_page = 20
enum :report_type, { type_selection: 0, type_labwhere: 1 }

Expand Down Expand Up @@ -49,7 +50,7 @@ def check_location_barcode
end

def check_any_select_field_present
attr_list = %i[faculty_sponsor_ids study_id start_date end_date plate_purpose_ids barcodes]
attr_list = %i[faculty_sponsor_ids study_id start_date end_date plate_purpose_ids barcodes retention_instructions]
if attr_list.all? { |attr| send(attr).blank? }
errors.add(:base, I18n.t('location_reports.errors.no_selection_fields_filled'))
end
Expand Down Expand Up @@ -119,7 +120,6 @@ def generate_report_rows # rubocop:todo Metrics/MethodLength
end

yield column_headers

labware_list.each do |cur_labware|
if cur_labware.studies.present?
cur_labware.studies.each { |cur_study| yield(generate_report_row(cur_labware, cur_study)) }
Expand Down Expand Up @@ -176,14 +176,30 @@ def generate_study_cols_for_row(cur_study)
end

def search_for_labware_by_selection
params = { faculty_sponsor_ids:, study_id:, start_date:, end_date:, plate_purpose_ids:, barcodes: }
params = { faculty_sponsor_ids:, study_id:, start_date:, end_date:, plate_purpose_ids:, barcodes:,
retention_instructions: }
validate_result_size(params)

# Only plates and tubes are currently supported by this report
labwares = Labware.search_for_labware(params).filter { |labware| labware.is_a?(Plate) || labware.is_a?(Tube) }

filter_labware_by_retention_instructions(labwares)
Comment on lines 178 to +186
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

validate_result_size(params) returns an empty array when the result set is too large, but its return value is ignored here. That means the code will still execute Labware.search_for_labware(params) even after adding the too_many_labwares_found error, defeating the safeguard and potentially loading a very large dataset. Short-circuit the method (eg return [] immediately) when the validation fails.

Copilot uses AI. Check for mistakes.
end

def filter_labware_by_retention_instructions(labwares)
return labwares if retention_instructions.blank?

labwares.select do |lw|
retention_instructions.include?(normalize_retention_instruction(lw.retention_instructions))
end
end

def validate_result_size(params)
count = Labware.search_for_count_of_labware(params)
if count > configatron.fetch(:location_reports_fetch_count_max, 25000)
errors.add(:base, I18n.t('location_reports.errors.too_many_labwares_found', count:))
return []
[]
end
# Only plates and tubes are currently supported by this report
Labware.search_for_labware(params).filter { |labware| labware.is_a?(Plate) || labware.is_a?(Tube) }
end
Comment on lines +197 to 203
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

validate_result_size currently adds an error and returns [], but does not explicitly halt callers (and returns nil on success). Consider making this method return a boolean (eg true/false) or raising/returning consistently so callers can reliably short-circuit when the limit is exceeded.

Copilot uses AI. Check for mistakes.

def search_for_labware_by_labwhere_locn_bc
Expand All @@ -208,5 +224,9 @@ def get_labwares_per_location(curr_locn_bc)
curr_locn_children = LabWhereClient::Location.children(curr_locn_bc)
curr_locn_children.each { |curr_locn| get_labwares_per_location(curr_locn.barcode) } if curr_locn_children.present?
end

def normalize_retention_instruction(value)
value.to_s.parameterize(separator: '_')
end
end
# rubocop:enable Metrics/ClassLength
6 changes: 4 additions & 2 deletions app/models/location_report/location_report_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ class LocationReport::LocationReportForm
include ActiveModel::AttributeMethods

# Attributes
attr_accessor :user, :report_type, :faculty_sponsor_ids, :study_id, :start_date, :end_date, :plate_purpose_ids
attr_accessor :user, :report_type, :faculty_sponsor_ids, :study_id, :start_date, :end_date, :retention_instructions,
:plate_purpose_ids

attr_accessor :barcodes_text
attr_reader :name, :location_barcode
Expand Down Expand Up @@ -44,7 +45,8 @@ def location_report
start_date: start_date&.to_datetime,
end_date: end_date&.to_datetime,
plate_purpose_ids: plate_purpose_ids,
barcodes: barcodes
barcodes: barcodes,
retention_instructions: retention_instructions
)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
</div>
</div>

<div class='form-group'>
<%= f.label 'retention_instructions', 'Retention instructions' %>
<%= f.select :retention_instructions, options_for_select(retention_instruction_option_for_select, location_report_form.retention_instructions), { include_hidden: false }, { class: 'form-control select2', :multiple => true } %>
</div>

<div class='form-group'>
<%= f.label 'plate_purpose_ids', 'Labware purposes (can select multiple)' %>
<%= f.select :plate_purpose_ids, options_for_select(Purpose.alphabetical.pluck(:name, :id), location_report_form.plate_purpose_ids), { include_hidden: false }, { class: 'form-control select2', :multiple => true } %>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true
class AddRetentionInstructionsToLocationReports < ActiveRecord::Migration[7.2]
def change
add_column :location_reports, :retention_instructions, :text
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[7.2].define(version: 2026_03_17_142326) do
ActiveRecord::Schema[7.2].define(version: 2026_03_25_134057) do
create_table "accession_sample_statuses", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
t.integer "sample_id", null: false
t.string "status", null: false
Expand Down Expand Up @@ -637,6 +637,7 @@
t.string "report_filename"
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.text "retention_instructions"
t.index ["study_id"], name: "index_location_reports_on_study_id"
t.index ["user_id"], name: "index_location_reports_on_user_id"
end
Expand Down
22 changes: 21 additions & 1 deletion spec/models/location_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@
start_date:,
end_date:,
plate_purpose_ids:,
barcodes:
barcodes:,
retention_instructions:
)
end
let(:report_type) { nil }
Expand All @@ -157,6 +158,7 @@
let(:end_date) { nil }
let(:plate_purpose_ids) { nil }
let(:barcodes) { nil }
let(:retention_instructions) { nil }

describe 'validations' do
context 'when no report type is set' do
Expand Down Expand Up @@ -473,6 +475,24 @@

it_behaves_like 'a successful report'
end

context 'with retention instructions' do
let(:start_date) { '2016-01-01 00:00:00' }
let(:end_date) { '2016-11-01 00:00:00' }
let(:retention_instructions) { ['long_term_storage'] }

# Only plate_1, plate_3, and tube_1 have this metadata, and the value is 'Long term storage' for all
let(:expected_lines) do
[
headers_line,
plt_1_line,
plt_3_line,
tube_1_line
]
end

it_behaves_like 'a successful report'
end
end

context 'by labwhere location' do
Expand Down
Loading