Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1ac271c
fix: add missing accessioning-enabled guard clauses
StephenHulme Feb 9, 2026
597434b
refactor: improve reability of if statement
StephenHulme Feb 9, 2026
641aeea
style: add reminder that some checks don't belong in a model
StephenHulme Feb 9, 2026
71757ff
fix: add more missing accessioning-enabled guard clauses
StephenHulme Feb 9, 2026
55b09b9
refactor: move accession_all_samples out of study model
StephenHulme Feb 9, 2026
5d77642
fix: repair missing model reference
StephenHulme Feb 9, 2026
546042e
test: update tests to match code changes
StephenHulme Feb 9, 2026
5af76cf
support '/' in plate purpose names
sabrine33 Feb 9, 2026
d1fcaf4
run rubocop
sabrine33 Feb 9, 2026
b9dab24
Update faraday to version 2.14.1
depfu[bot] Feb 9, 2026
df2a07c
Merge pull request #5535 from sanger/depfu/update/faraday-2.14.1
StephenHulme Feb 10, 2026
9fe5331
tests: add additional helper specs
StephenHulme Feb 10, 2026
3fa9e63
tests: add additional controller specs
StephenHulme Feb 10, 2026
03e9f4f
test: remove extraneous lets
StephenHulme Feb 10, 2026
2f9b4d6
Merge pull request #5531 from sanger/y25-286-add-missing-accessioning…
StephenHulme Feb 11, 2026
6c0ad46
fix: add missing safe navigation operators to fix humanize on nil error
StephenHulme Feb 11, 2026
8fc8826
Merge pull request #5539 from sanger/y25-286-fix-humanize-bug
StephenHulme Feb 11, 2026
bac092d
support '/' in plate purpose names
sabrine33 Feb 9, 2026
ca08e0a
run rubocop
sabrine33 Feb 9, 2026
410d19f
add an intermediate state 'processed' for labware between started and…
sabrine33 Feb 13, 2026
58828d9
Merge remote-tracking branch 'origin/Y25-682' into Y25-682
sabrine33 Feb 13, 2026
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
6 changes: 3 additions & 3 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ GEM
factory_bot_rails (6.5.1)
factory_bot (~> 6.5)
railties (>= 6.1.0)
faraday (2.14.0)
faraday (2.14.1)
faraday-net_http (>= 2.0, < 3.5)
json
logger
Expand Down Expand Up @@ -338,8 +338,8 @@ GEM
mutex_m (0.3.0)
mysql2 (0.5.7)
bigdecimal
net-http (0.7.0)
uri
net-http (0.9.1)
uri (>= 0.11.1)
net-imap (0.5.9)
date
net-protocol
Expand Down
32 changes: 29 additions & 3 deletions app/controllers/studies_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -253,18 +253,44 @@ def accession
end
end

# Accession all samples in the study.
#
# If the study does not have an accession number, adds an error to the study and returns.
# Otherwise, iterates through each sample in the study and attempts to accession it,
# unless the sample already has an accession number.
# If an Accession::Error occurs for a sample, adds the error message to the study's errors.
#
# NOTE: this does not check if the current user has permission to accession samples in this study
#
# @return [void]
def accession_all_samples # rubocop:disable Metrics/AbcSize,Metrics/MethodLength
@study = Study.find(params[:id])
return accessioning_not_enabled_redirect unless accessioning_enabled?

# TODO: Y26-026 - Enforce accessioning permissions
# return accession_permission_denied_redirect unless permitted_to_accession?(@study)

@study.accession_all_samples(current_user)
unless @study.accession_number?
flash[:error] = 'Please accession the study before accessioning samples'
return redirect_to(study_path(@study))
end
unless @study.samples_accessionable?
flash[:error] = 'Study cannot accession samples, see Study Accessioning tab for details'
return redirect_to(study_path(@study))
end

@study.samples.find_each do |sample|
next if sample.accession_number?

begin
Accession.accession_sample(sample, current_user)
rescue Accession::Error => e
@study.errors.add(:base, e.message)
end
end

if @study.errors.any?
error_messages = compile_accession_errors(@study.errors)
flash[:error] = error_messages
flash[:error] = compile_accession_errors(@study.errors)
else
flash[:notice] = 'All of the samples in this study have been sent for accessioning. ' \
'Please check back in 5 minutes to confirm that accessioning was successful.'
Expand Down
4 changes: 3 additions & 1 deletion app/helpers/samples_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ module SamplesHelper
# Indicate to the user that saving the sample will also accession it
# This will not happen if the study has not been accessioned
def save_text(sample)
return 'Save and Accession' if sample.should_be_accessioned?
if [accessioning_enabled?, sample.should_be_accessioned?, permitted_to_accession?(sample)].all?
return 'Save and Accession'
end

'Save Sample'
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/purpose.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class Purpose < ApplicationRecord

before_validation :set_default_barcode_prefix

validates :name, format: { with: /\A\w[\s\w.-]+\w\z/i }, presence: true, uniqueness: { case_sensitive: false }
validates :name, format: { with: %r{\A\w[\s\w.\-/]+\w\z}i }, presence: true, uniqueness: { case_sensitive: false }

# NOTE: We should validate against valid asset subclasses, but running into some issues with
# subclass loading while seeding.
Expand Down
3 changes: 2 additions & 1 deletion app/models/sample.rb
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,8 @@ def should_be_accessioned?
false
end

# NOTE: this does not check whether the current user is permitted to accession the sample
# NOTE: this does not check whether the current user is permitted to accession the sample,
# nor if accessioning is enabled, as these belong in a controller or library, rather than the model.
def accession_and_handle_validation_errors
event_user = current_user # the event_user for this sample must be set from the calling controller
Accession.accession_sample(self, event_user, perform_now: true)
Expand Down
25 changes: 0 additions & 25 deletions app/models/study.rb
Original file line number Diff line number Diff line change
Expand Up @@ -583,31 +583,6 @@ def samples_accessionable?
].all?
end

# Accession all samples in the study.
#
# If the study does not have an accession number, adds an error to the study and returns.
# Otherwise, iterates through each sample in the study and attempts to accession it,
# unless the sample already has an accession number.
# If an Accession::Error occurs for a sample, adds the error message to the study's errors.
#
# NOTE: this does not check if the current user has permission to accession samples in this study
#
# @return [void]
def accession_all_samples(event_user)
return errors.add(:base, 'Please accession the study before accessioning samples') unless accession_number?

unless samples_accessionable?
return errors.add(:base,
'Study cannot accession samples, see Study Accessioning tab for details')
end

samples.find_each do |sample|
Accession.accession_sample(sample, event_user) unless sample.accession_number?
rescue Accession::Error => e
errors.add(:base, e.message)
end
end

def abbreviation
abbreviation = study_metadata.study_name_abbreviation
abbreviation.presence || "#{id}STDY"
Expand Down
7 changes: 6 additions & 1 deletion app/models/transfer_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ class TransferRequest < ApplicationRecord # rubocop:todo Metrics/ClassLength
# as being more concise as it has fewer states.
state :pending, initial: true
state :started
state :processed
state :processed_1
state :processed_2
state :processed_3
Expand All @@ -85,6 +86,10 @@ class TransferRequest < ApplicationRecord # rubocop:todo Metrics/ClassLength
transitions to: :started, from: [:pending], after: :on_started
end

event :progress do
transitions to: :processed, from: [:started], after: :on_started
end

event :process_1 do
transitions to: :processed_1, from: [:pending], after: :on_started
end
Expand All @@ -104,7 +109,7 @@ class TransferRequest < ApplicationRecord # rubocop:todo Metrics/ClassLength
event :pass do
# Jumping straight to passed moves through an implied started state.
transitions to: :passed, from: :pending, after: :on_started
transitions to: :passed, from: %i[started failed processed_2 processed_3 processed_4]
transitions to: :passed, from: %i[started failed processed processed_2 processed_3 processed_4]
end

event :fail do
Expand Down
2 changes: 1 addition & 1 deletion app/views/sdb/sample_manifests/_samples.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
<td><%= accession_status&.status&.capitalize %></td>
<td><%= accession_status&.updated_at&.in_time_zone&.strftime('%Y-%m-%d %H:%M:%S %Z') %></td>
<td>
<% if can?(:accession, sample) && configatron.accession_samples %>
<% if [accessioning_enabled?, sample.should_be_accessioned?, permitted_to_accession?(sample)].all? %>
<%# Add link to perform sample accessioning %>
<% if sample.current_accession_status %>
<%# If status exists, offer retry %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/studies/information/_accession_statuses.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
<td><%= accession_status&.status&.capitalize %></td>
<td><%= accession_status&.updated_at&.in_time_zone&.strftime('%Y-%m-%d %H:%M:%S %Z') %></td>
<td>
<% if permitted_to_accession?(sample) & sample.should_be_accessioned? & accessioning_enabled? %>
<% if [accessioning_enabled?, sample.should_be_accessioned?, permitted_to_accession?(sample)].all? %>
<%# Add link to perform sample accessioning %>
<% if sample.current_accession_status %>
<%# If status exists, offer retry %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

<%= checklist_item(
condition: !@study.study_metadata.strategy_not_applicable?,
good: "Study release strategy is #{Study::DATA_RELEASE_STRATEGY_OPEN.humanize} or #{Study::DATA_RELEASE_STRATEGY_MANAGED.humanize} #{tag.strong { "(#{@study.study_metadata.data_release_strategy.humanize})" }}".html_safe,
good: "Study release strategy is #{Study::DATA_RELEASE_STRATEGY_OPEN.humanize} or #{Study::DATA_RELEASE_STRATEGY_MANAGED.humanize} #{tag.strong { "(#{@study.study_metadata.data_release_strategy&.humanize})" }}".html_safe,
bad: "Study release strategy is <strong>#{Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE.humanize}</strong>".html_safe,
action: link_to('Change release strategy', edit_study_path(@study)),
action_permission: can?(:edit, @study),
Expand All @@ -34,7 +34,7 @@

<%= checklist_item(
condition: !@study.study_metadata.never_release?,
good: "Study release timing is <strong>#{@study.study_metadata.data_release_timing.humanize}</strong>".html_safe,
good: "Study release timing is <strong>#{@study.study_metadata.data_release_timing&.humanize}</strong>".html_safe,
bad: "Study release timing is <strong>#{Study::DATA_RELEASE_TIMING_NEVER.humanize}</strong>".html_safe,
action: link_to('Change release settings', edit_study_path(@study)),
action_permission: can?(:edit, @study),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,15 @@ limber_lcm_triomics_wgs:
- LCMT DNA Adp Lig
library_types:
- LCMB
limber_lcm_triomics_rnaseq:
name: LCM Triomics RNA-seq
asset_type: Well
order: 1
request_class_name: IlluminaHtp::Requests::StdLibraryRequest
for_multiplexing: false
billable: true
product_line_name: Short Read
acceptable_purposes:
- LCMT Lysate
library_types:
- Combined LCM RNA
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,10 @@ Limber-Htp - LCM Triomics WGS:
request_type_keys: ["limber_lcm_triomics_wgs"]
product_line_name: LCM Triomics
product_catalogue_name: LCM Triomics

Limber-Htp - LCM Triomics RNA-seq:
submission_class_name: "LinearSubmission"
related_records:
request_type_keys: ["limber_lcm_triomics_rnaseq"]
product_line_name: LCM Triomics
product_catalogue_name: LCM Triomics
99 changes: 99 additions & 0 deletions spec/controllers/studies_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,48 @@
end

context 'when the accessioning succeeds' do
it 'accessions all samples in the study' do
study.samples.each do |sample|
expect(sample.reload.sample_metadata.sample_ebi_accession_number).to eq('EGA00001000240')
end
end

it 'redirects to the accession-statuses tab of the study page' do
expect(subject).to redirect_to(study_path(study, anchor: 'accession-statuses'))
end

it 'does not set a flash error message' do
expect(flash[:error]).to be_nil
end

it 'does not set a flash warning message' do
expect(flash[:warning]).to be_nil
end

it 'sets a flash notice message' do
expect(flash[:notice]).to eq(
'All of the samples in this study have been sent for accessioning. ' \
'Please check back in 5 minutes to confirm that accessioning was successful.'
)
end

it 'does not set a flash info message' do
expect(flash[:info]).to be_nil
end
end

context 'when a sample already has an accession number' do
# add a 6th already accessioned sample to the study
let(:samples) { create_list(:sample_for_accessioning, number_of_samples) + create_list(:accessioned_sample, 1) }
let(:study) { create(:open_study, accession_number: 'ENA123', samples: samples) }

it 'does not attempt to accession accessioned samples' do
# confirm that only 5 calls were made to the accession client, not 6
expect(Accession::Submission.client)
.to have_received(:submit_and_fetch_accession_number)
.exactly(number_of_samples).times
end

it 'redirects to the accession-statuses tab of the study page' do
expect(subject).to redirect_to(study_path(study, anchor: 'accession-statuses'))
end
Expand Down Expand Up @@ -308,6 +350,63 @@
end
end
end

context 'when the study does not have an accession number' do
let(:study) { create(:managed_study, samples:) }

it 'does not attempt to accession samples' do
expect(Accession::Submission.client).not_to have_received(:submit_and_fetch_accession_number)
end

it 'redirects to the study page' do
expect(subject).to redirect_to(study_path(study))
end

it 'does not set a flash warning message' do
expect(flash[:warning]).to be_nil
end

it 'does not set a flash notice message' do
expect(flash[:notice]).to be_nil
end

it 'sets a flash error message' do
expect(flash[:error]).to eq('Please accession the study before accessioning samples')
end

it 'does not set a flash info message' do
expect(flash[:info]).to be_nil
end
end

context 'when a study is not longer accessionable' do
let(:study_metadata) { create(:study_metadata_for_accessioning, study_ebi_accession_number: 'EGA123') }
let(:study) { create(:study, study_metadata:, samples:) }

it 'does not attempt to accession samples' do
expect(Accession::Submission.client).not_to have_received(:submit_and_fetch_accession_number)
end

it 'redirects to the study page' do
expect(subject).to redirect_to(study_path(study))
end

it 'does not set a flash warning message' do
expect(flash[:warning]).to be_nil
end

it 'does not set a flash notice message' do
expect(flash[:notice]).to be_nil
end

it 'sets a flash error message' do
expect(flash[:error]).to eq('Study cannot accession samples, see Study Accessioning tab for details')
end

it 'does not set a flash info message' do
expect(flash[:info]).to be_nil
end
end
end
end
end
41 changes: 41 additions & 0 deletions spec/helpers/samples_helper_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe SamplesHelper, type: :helper do
subject { helper.save_text(sample) }

let(:sample) { create(:sample) }

let(:accessioning_enabled) { true }
let(:should_be_accessioned) { true }
let(:permitted) { true }

before do
allow(helper).to receive(:accessioning_enabled?).and_return(accessioning_enabled)
allow(helper).to receive(:permitted_to_accession?).with(sample).and_return(permitted)
allow(sample).to receive(:should_be_accessioned?).and_return(should_be_accessioned)
end

context 'when accessioning is enabled, sample should be accessioned, and user is permitted' do
it { is_expected.to eq('Save and Accession') }
end

context 'when accessioning is disabled' do
let(:accessioning_enabled) { false }

it { is_expected.to eq('Save Sample') }
end

context 'when sample should not be accessioned' do
let(:should_be_accessioned) { false }

it { is_expected.to eq('Save Sample') }
end

context 'when user is not permitted to accession' do
let(:permitted) { false }

it { is_expected.to eq('Save Sample') }
end
end
Loading
Loading