diff --git a/.rubocop.yml b/.rubocop.yml index 5709cbcfd1..8bf76fb35a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -284,6 +284,10 @@ Style/QuotedSymbols: Exclude: - bin/* +# Keep consistency across attribute writers and readers +Style/RedundantSelf: + Enabled: false + # Files generated by Rails Style/StringLiterals: Exclude: diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index c018867fa9..9947efd7c6 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config --no-exclude-limit` -# on 2026-04-22 15:46:09 UTC using RuboCop version 1.86.1. +# on 2026-05-05 14:43:06 UTC using RuboCop version 1.86.1. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -301,12 +301,11 @@ Lint/UselessOr: - 'app/models/qcable/statemachine.rb' - 'app/models/ui_helper/summary.rb' -# Offense count: 11 +# Offense count: 7 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes, Max. Metrics/AbcSize: Exclude: - 'app/controllers/api/v2/transfers_controller.rb' - - 'app/controllers/studies/information_controller.rb' - 'app/jobs/export_pool_xp_to_traction_job.rb' - 'app/models/accession_service/base_service.rb' - 'app/sample_manifest_excel/sample_manifest_excel/manifest_type_list.rb' @@ -334,7 +333,7 @@ Metrics/CyclomaticComplexity: - 'app/models/accession_service/base_service.rb' - 'lib/limber/helper.rb' -# Offense count: 19 +# Offense count: 18 # Configuration parameters: CountComments, Max, CountAsOne, AllowedMethods, AllowedPatterns. Metrics/MethodLength: Exclude: @@ -617,7 +616,7 @@ RSpec/BeforeAfterAll: - 'spec/sample_manifest_excel/worksheet_spec.rb' - 'spec/sequencescape_excel/worksheet_spec.rb' -# Offense count: 342 +# Offense count: 330 # Configuration parameters: Prefixes, AllowedPatterns. # Prefixes: when, with, without RSpec/ContextWording: @@ -649,7 +648,6 @@ RSpec/ContextWording: - 'spec/models/barcode_spec.rb' - 'spec/models/broadcast_event/lab_event_spec.rb' - 'spec/models/broadcast_event/qc_assay_spec.rb' - - 'spec/models/bulk_submission_spec.rb' - 'spec/models/comment_spec.rb' - 'spec/models/illumina_htp/initial_stock_tube_purpose_spec.rb' - 'spec/models/location_report_form_spec.rb' @@ -840,7 +838,7 @@ RSpec/ExampleWording: - 'spec/sequencescape_excel/validation_spec.rb' - 'spec/sequencescape_excel/worksheet_spec.rb' -# Offense count: 27 +# Offense count: 26 RSpec/ExpectInHook: Exclude: - 'spec/controllers/labwhere_receptions_controller_spec.rb' @@ -1197,7 +1195,7 @@ RSpec/MultipleExpectations: - 'spec/views/labware/show_chromium_chip_spec.rb' - 'spec/views/samples/index_html_erb_spec.rb' -# Offense count: 243 +# Offense count: 249 # Configuration parameters: EnforcedStyle, IgnoreSharedExamples. # SupportedStyles: always, named_only RSpec/NamedSubject: diff --git a/Gemfile b/Gemfile index 2c7017c5a1..bc295f5e31 100644 --- a/Gemfile +++ b/Gemfile @@ -4,7 +4,7 @@ source 'https://rubygems.org' group :default do gem 'bootsnap' - gem 'concurrent-ruby', '1.3.5' + gem 'concurrent-ruby' gem 'configatron' gem 'formtastic' gem 'rails', '~> 8.0.0' @@ -21,10 +21,6 @@ group :default do gem 'faraday-multipart' gem 'rest-client' # Deprecated, but still used in some places, replace with Faraday where possible - # Fix incompatibility with between Ruby 3.1 and Psych 4 (used for yaml) - # see https://stackoverflow.com/a/71192990 - gem 'psych', '< 4' - # State machine gem 'aasm' gem 'after_commit_everywhere', '~> 1.0' # Required by AASM @@ -34,7 +30,7 @@ group :default do # Provides bulk insert capabilities gem 'activerecord-import' - gem 'record_loader', git: 'https://github.com/sanger/record_loader', tag: 'v0.3.0' + gem 'record_loader', git: 'https://github.com/sanger/record_loader', tag: 'v1.1.0' gem 'mysql2', platforms: :mri gem 'will_paginate' diff --git a/Gemfile.lock b/Gemfile.lock index 6a055aa724..93b9cd7fa2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -16,10 +16,11 @@ GIT GIT remote: https://github.com/sanger/record_loader - revision: 238db7fa24fffee5ad413bd9cd4c6b857d1626c9 - tag: v0.3.0 + revision: 9e7f1b67c3eeee1895f1befc6a54682f11c02135 + tag: v1.1.0 specs: - record_loader (0.3.0) + record_loader (1.1.0) + psych (~> 5.0) GIT remote: https://github.com/sanger/sanger_barcode_format.git @@ -170,7 +171,7 @@ GEM choice (0.2.0) chronic (0.10.2) coderay (1.1.3) - concurrent-ruby (1.3.5) + concurrent-ruby (1.3.6) configatron (4.5.1) connection_pool (2.5.5) crack (1.0.1) @@ -233,7 +234,7 @@ GEM factory_bot_rails (6.5.1) factory_bot (~> 6.5) railties (>= 6.1.0) - faraday (2.14.1) + faraday (2.14.2) faraday-net_http (>= 2.0, < 3.5) json logger @@ -244,14 +245,14 @@ GEM ffi (1.17.3-arm64-darwin) ffi (1.17.3-x86_64-darwin) ffi (1.17.3-x86_64-linux-gnu) - flipper (1.4.1) + flipper (1.4.2) concurrent-ruby (< 2) - flipper-active_record (1.4.1) + flipper-active_record (1.4.2) activerecord (>= 4.2, < 9) - flipper (~> 1.4.1) - flipper-ui (1.4.1) + flipper (~> 1.4.2) + flipper-ui (1.4.2) erubi (>= 1.0.0, < 2.0.0) - flipper (~> 1.4.1) + flipper (~> 1.4.2) rack (>= 1.4, < 4) rack-protection (>= 1.5.3, < 5.0.0) rack-session (>= 1.0.2, < 3.0.0) @@ -280,16 +281,16 @@ GEM prism (>= 1.3.0) rdoc (>= 4.0.0) reline (>= 0.4.2) - json (2.19.4) + json (2.19.5) jsonapi-resources (0.9.0) activerecord (>= 4.1) concurrent-ruby railties (>= 4.1) jsonapi-resources-matchers (1.0.0) jsonapi-resources (>= 0.9.0) - knapsack_pro (9.2.3) - logger + knapsack_pro (10.0.0) rake + thor (~> 1.4) language_server-protocol (3.17.0.5) launchy (3.1.1) addressable (~> 2.8) @@ -378,7 +379,9 @@ GEM pry (>= 0.13, < 0.17) pry-rails (0.3.11) pry (>= 0.13.0) - psych (3.3.4) + psych (5.3.1) + date + stringio public_suffix (7.0.5) puma (7.2.0) nio4r (~> 2.0) @@ -499,8 +502,8 @@ GEM rspec-mocks (>= 3.13.0, < 5.0.0) rspec-support (>= 3.13.0, < 5.0.0) rspec-support (3.13.7) - ruboclean (0.7.1) - rubocop (1.86.1) + ruboclean (0.8.0) + rubocop (1.86.2) json (~> 2.3) language_server-protocol (~> 3.17.0.2) lint_roller (~> 1.1.0) @@ -552,7 +555,7 @@ GEM ffi (~> 1.12) logger ruby2_keywords (0.0.5) - rubyzip (3.2.2) + rubyzip (3.3.0) sanger-jsonapi-resources (0.1.2) activerecord (>= 4.1) concurrent-ruby @@ -562,7 +565,7 @@ GEM crass (~> 1.0.2) nokogiri (>= 1.16.8) securerandom (0.4.1) - selenium-webdriver (4.43.0) + selenium-webdriver (4.44.0) base64 (~> 0.2) logger (~> 1.4) rexml (~> 3.2, >= 3.2.5) @@ -589,6 +592,7 @@ GEM rbtree set (~> 1.0) ssrf_filter (1.2.0) + stringio (3.2.0) syntax_tree (6.3.0) prettier_print (>= 1.2.0) syntax_tree-haml (4.0.3) @@ -622,7 +626,7 @@ GEM uri (1.1.1) useragent (0.16.11) uuidtools (3.0.0) - vite_rails (3.10.0) + vite_rails (3.11.0) railties (>= 5.1, < 9) vite_ruby (~> 3.0, >= 3.2.2) vite_ruby (3.10.2) @@ -680,7 +684,7 @@ DEPENDENCIES capybara carrierwave caxlsx - concurrent-ruby (= 1.3.5) + concurrent-ruby configatron csv (~> 3.3) cucumber-rails @@ -716,7 +720,6 @@ DEPENDENCIES prettier_print pry-byebug pry-rails - psych (< 4) puma rack-acceptable rack-cors diff --git a/app/controllers/qc_reports_controller.rb b/app/controllers/qc_reports_controller.rb index 28da03c102..15e458aef1 100644 --- a/app/controllers/qc_reports_controller.rb +++ b/app/controllers/qc_reports_controller.rb @@ -45,12 +45,14 @@ def show # rubocop:todo Metrics/AbcSize def create # rubocop:todo Metrics/AbcSize, Metrics/MethodLength study = Study.find_by(id: params[:qc_report][:study_id]) exclude_existing = params[:qc_report][:exclude_existing] == '1' + plate_barcodes = format_plate_barcodes(params[:qc_report][:plate_barcodes_text]) qc_report = QcReport.new( study: study, product_criteria: @product.stock_criteria, exclude_existing: exclude_existing, - plate_purposes: params[:qc_report][:plate_purposes].try(:reject, &:blank?) + plate_purposes: params[:qc_report][:plate_purposes].try(:reject, &:blank?), + plate_barcodes: plate_barcodes ) if qc_report.save @@ -64,6 +66,13 @@ def create # rubocop:todo Metrics/AbcSize, Metrics/MethodLength end end + def format_plate_barcodes(plate_barcodes) + return nil if plate_barcodes.nil? + + # Split the input string into an array of barcodes + plate_barcodes.split(/\s+/) + end + # On form submit of a qc_file. Strictly speaking this should be an update action # on the qc_report itself. However we don't want to force the user to extract # the report identifier from the file. diff --git a/app/models/broadcast_event.rb b/app/models/broadcast_event.rb index 7493d841e6..146c5571ea 100644 --- a/app/models/broadcast_event.rb +++ b/app/models/broadcast_event.rb @@ -19,7 +19,7 @@ class BroadcastEvent < ApplicationRecord # https://api.rubyonrails.org/classes/ActiveRecord/Inheritance/ClassMethods.html validates :sti_type, presence: true - serialize :properties, coder: YAML + serialize :properties, coder: YAML, yaml: { permitted_classes: [ActionController::Parameters] } self.inheritance_column = 'sti_type' broadcast_with_warren diff --git a/app/models/presenters/qc_report_presenter.rb b/app/models/presenters/qc_report_presenter.rb index d9e83a0d44..f549ece1d1 100644 --- a/app/models/presenters/qc_report_presenter.rb +++ b/app/models/presenters/qc_report_presenter.rb @@ -31,7 +31,7 @@ def product_name end def study_name - qc_report.study.name + qc_report.study&.name || 'N/A' end def study_abbreviation diff --git a/app/models/qc_report.rb b/app/models/qc_report.rb index 771890b6f1..317912a24f 100644 --- a/app/models/qc_report.rb +++ b/app/models/qc_report.rb @@ -82,10 +82,12 @@ module ReportBehaviour # You can trigger a synchronous report manually by calling #generate! # rubocop:todo Metrics/MethodLength def generate_report # rubocop:todo Metrics/AbcSize - study.each_well_for_qc_report_in_batches( + Well.qc_report_in_batches( + study, exclude_existing, product_criteria, - (plate_purposes.empty? ? nil : plate_purposes) + (plate_purposes.empty? ? nil : plate_purposes), + (plate_barcodes.empty? ? nil : plate_barcodes) ) do |assets| # If there are some wells of interest, we get them in a list connected_wells = Well.hash_stock_with_targets(assets, product_criteria.target_plate_purposes) @@ -128,6 +130,7 @@ def generate_report # rubocop:todo Metrics/AbcSize has_many :qc_metrics serialize :plate_purposes, type: Array, coder: YAML + serialize :plate_barcodes, type: Array, coder: YAML before_validation :generate_report_identifier, if: :identifier_required? @@ -135,10 +138,22 @@ def generate_report # rubocop:todo Metrics/AbcSize scope :for_report_page, ->(conditions) { order(id: :desc).where(conditions).joins(:product_criteria) } - validates :product_criteria, :study, :state, presence: true + validates :product_criteria, :state, presence: true validates :exclude_existing, inclusion: { in: [true, false], message: 'should be true or false.' } + validate :check_valid_plate_barcodes, if: -> { plate_barcodes.present? } + + # We allow null values for study_id to allow qc_reports to be created without a study (just plate_barcodes) + validates :study, presence: true, unless: -> { plate_barcodes.present? } + + def check_valid_plate_barcodes + invalid_barcodes = plate_barcodes.reject { |barcode| Plate.find_by_barcode(barcode) } + return unless invalid_barcodes.any? + + errors.add(:plate_barcodes, "contain invalid barcodes: #{invalid_barcodes.join(', ')}") + end + # Reports are handled asynchronously def schedule_report Delayed::Job.enqueue QcReportJob.new(id) @@ -166,9 +181,9 @@ def identifier_required? # same product / study abbreviation combo within one second # of each other. def generate_report_identifier - return true if study.nil? || product_criteria.nil? + return true if product_criteria.nil? - rid = [study.abbreviation, product_criteria.product.name, DateTime.now.to_fs(:number)].compact + rid = [study&.abbreviation, product_criteria.product.name, DateTime.now.to_fs(:number)].compact .join('_') .downcase .gsub(/[^\w]/, '_') diff --git a/app/models/request_type/validator.rb b/app/models/request_type/validator.rb index d863ac909f..db32ee1cba 100644 --- a/app/models/request_type/validator.rb +++ b/app/models/request_type/validator.rb @@ -104,7 +104,14 @@ def valid_options belongs_to :request_type, optional: false validates :request_option, :valid_options, presence: true - serialize :valid_options, coder: YAML + serialize :valid_options, coder: YAML, yaml: { + permitted_classes: [ + Range, + RequestType::Validator::ArrayWithDefault, + RequestType::Validator::FlowcellTypeValidator, + RequestType::Validator::LibraryTypeValidator + ] + } delegate :include?, to: :valid_options diff --git a/app/models/study.rb b/app/models/study.rb index 0d2f9127fc..c752cb02a2 100644 --- a/app/models/study.rb +++ b/app/models/study.rb @@ -446,20 +446,6 @@ def validate_ethically_approved false end - def each_well_for_qc_report_in_batches(exclude_existing, product_criteria, plate_purposes = nil) - # @note We include aliquots here, despite the fact they are only needed if we have to set a poor-quality flag - # as in some cases failures are not as rare as you may imagine, and it can cause major performance issues. - base_scope = - Well - .on_plate_purpose_included(PlatePurpose.where(name: plate_purposes || STOCK_PLATE_PURPOSES)) - .for_study_through_aliquot(self) - .without_blank_samples - .includes(:well_attribute, :aliquots, :map, samples: :sample_metadata) - .readonly(true) - scope = exclude_existing ? base_scope.without_report(product_criteria) : base_scope - scope.find_in_batches { |wells| yield wells } - end - def warnings # These studies are now invalid, but the warning should remain until existing studies are fixed. if study_metadata.managed? && study_metadata.data_access_group.blank? diff --git a/app/models/submission/request_options_behaviour.rb b/app/models/submission/request_options_behaviour.rb index f140271a35..24489254f1 100644 --- a/app/models/submission/request_options_behaviour.rb +++ b/app/models/submission/request_options_behaviour.rb @@ -7,7 +7,14 @@ class HashWrapper def self.load(hash_yaml) return hash_yaml if hash_yaml.nil? - YAML.load(hash_yaml) + YAML.safe_load(hash_yaml, + aliases: true, + permitted_classes: [ + ActiveSupport::HashWithIndifferentAccess, + ActiveSupport::TimeWithZone, + ActiveSupport::TimeZone, + Time + ]) end def self.dump(hash) @@ -28,22 +35,20 @@ def request_options=(options) super end + private + def check_request_options check_multipliers_are_valid end - private :check_request_options - # rubocop:todo Metrics/PerceivedComplexity, Metrics/AbcSize - def check_multipliers_are_valid # rubocop:todo Metrics/CyclomaticComplexity + def check_multipliers_are_valid # rubocop:disable Metrics/CyclomaticComplexity multipliers = request_options.try(:[], :multiplier) return if multipliers.blank? # We're ok with nothing being specified! # TODO[xxx]: should probably error if they've specified a request type that isn't being used - errors.add(:request_options, 'negative multiplier supplied') if multipliers.values.map(&:to_i).any?(&:negative?) - errors.add(:request_options, 'zero multiplier supplied') if multipliers.values.map(&:to_i).any?(&:zero?) + multiplier_values = multipliers.values.map(&:to_i) + errors.add(:request_options, 'negative multiplier supplied') if multiplier_values.any?(&:negative?) + errors.add(:request_options, 'zero multiplier supplied') if multiplier_values.any?(&:zero?) false unless errors.empty? end - - # rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity - private :check_multipliers_are_valid end diff --git a/app/models/well.rb b/app/models/well.rb index 3cc7b94c57..73eedae295 100644 --- a/app/models/well.rb +++ b/app/models/well.rb @@ -91,14 +91,26 @@ def poly_metadata scope :on_plate_purpose, ->(purposes) { joins(:labware).where(labware: { plate_purpose_id: purposes }) } + scope :on_plate_barcode, + lambda { |*barcodes| + if barcodes.flatten.any? + db_barcodes = Barcode.extract_barcodes(barcodes) + joins(labware: :barcodes).where(barcodes: { barcode: db_barcodes }).distinct + end + } + # added version of scope with includes to avoid multiple calls to LabWhere in qc report when getting storage location # for wells in the same plate scope :on_plate_purpose_included, ->(purposes) do - includes(labware: :barcodes).references(:labware).where(labware: { plate_purpose_id: purposes }) + if purposes.any? + includes(labware: :barcodes).references(:labware).where(labware: { plate_purpose_id: purposes }) + end end - scope :for_study_through_aliquot, ->(study) { joins(:aliquots).where(aliquots: { study_id: study }) } + scope :for_study_through_aliquot, ->(study) { + joins(:aliquots).where(aliquots: { study_id: study }) if study.present? + } scope :with_report, ->(product_criteria) do @@ -369,4 +381,21 @@ def library_name def empty? aliquots.blank? end + + def self.qc_report_in_batches(study, exclude_existing, product_criteria, plate_purposes, plate_barcodes, &) + # @note We include aliquots here, despite the fact they are only needed if we have to set a poor-quality flag + # as in some cases failures are not as rare as you may imagine, and it can cause major performance issues. + # Plate purposes don't need to be specified if plate_barcodes are provided. + # If they are not, then we default to the stock plate purposes if not plate_purposes are provided. + default_plate_purposes = plate_barcodes.present? ? nil : Study::STOCK_PLATE_PURPOSES + base_scope = + for_study_through_aliquot(study) + .on_plate_barcode(plate_barcodes) + .on_plate_purpose_included(PlatePurpose.where(name: plate_purposes || default_plate_purposes)) + .without_blank_samples + .includes(:well_attribute, :aliquots, :map, samples: :sample_metadata) + .readonly(true) + scope = exclude_existing ? base_scope.without_report(product_criteria) : base_scope + scope.find_in_batches(&) + end end diff --git a/app/sequencescape_excel/sequencescape_excel/helpers.rb b/app/sequencescape_excel/sequencescape_excel/helpers.rb index 07c60d4513..0317705694 100644 --- a/app/sequencescape_excel/sequencescape_excel/helpers.rb +++ b/app/sequencescape_excel/sequencescape_excel/helpers.rb @@ -5,7 +5,8 @@ module SequencescapeExcel # Helpers module Helpers def load_file(folder, filename) - YAML.load_file(Rails.root.join(folder, "#{filename}.yml")).with_indifferent_access + file_path = Rails.root.join(folder, "#{filename}.yml") + YAML.safe_load_file(file_path, permitted_classes: [Symbol], aliases: true).with_indifferent_access end end end diff --git a/app/views/qc_reports/_qc_report_form.html.erb b/app/views/qc_reports/_qc_report_form.html.erb index 31ef040db3..a62a97c8c0 100644 --- a/app/views/qc_reports/_qc_report_form.html.erb +++ b/app/views/qc_reports/_qc_report_form.html.erb @@ -1,7 +1,7 @@ <%= form_for(qc_report) do |f| %>