From 4fc33fd07b2483c39f9a2768e8a8d08a4b3a8c48 Mon Sep 17 00:00:00 2001 From: Stephen Hulme Date: Wed, 4 Mar 2026 10:36:05 +0000 Subject: [PATCH 1/3] style: fix typo --- spec/lib/accession/submission_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/accession/submission_spec.rb b/spec/lib/accession/submission_spec.rb index b4e3f011fe..253084e64f 100644 --- a/spec/lib/accession/submission_spec.rb +++ b/spec/lib/accession/submission_spec.rb @@ -55,7 +55,7 @@ allow(described_class).to receive(:client).and_return(mock_client) end - context 'when th sample has not yet been accessioned' do + context 'when the sample has not yet been accessioned' do context 'when the submission is successful' do let(:accession_number) { 'EGA00001000240' } let(:mock_client) do From 43473c696ecff10013d6d7665fbd4b7fca7fa4f5 Mon Sep 17 00:00:00 2001 From: Stephen Hulme Date: Wed, 4 Mar 2026 13:11:18 +0000 Subject: [PATCH 2/3] fix: add additional gender validations --- app/models/sample.rb | 3 ++ lib/accession/sample.rb | 3 ++ spec/lib/accession/sample_spec.rb | 49 +++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/app/models/sample.rb b/app/models/sample.rb index f6af730ddb..51b99ed776 100644 --- a/app/models/sample.rb +++ b/app/models/sample.rb @@ -27,6 +27,7 @@ class Current < ActiveSupport::CurrentAttributes GC_CONTENTS = ['Neutral', 'High AT', 'High GC'].freeze GENDERS = ['Male', 'Female', 'Mixed', 'Hermaphrodite', 'Unknown', 'Not Applicable'].freeze + EGA_GENDERS = %w[Female Male Unknown].freeze DNA_SOURCES = [ 'Genomic', 'Whole Genome Amplified', @@ -194,6 +195,8 @@ class Current < ActiveSupport::CurrentAttributes with_options(on: :EGA) do validates :gender, presence: { message: 'is required' } + one_of_ega_genders = EGA_GENDERS.to_sentence(last_word_connector: ' or ', two_words_connector: ' or ') + validates :gender, inclusion: { in: EGA_GENDERS, message: "must be #{one_of_ega_genders}" } validates :phenotype, presence: { message: 'is required' } validates :donor_id, presence: { message: 'is required' } end diff --git a/lib/accession/sample.rb b/lib/accession/sample.rb index 55aae8a35e..214e9c2759 100644 --- a/lib/accession/sample.rb +++ b/lib/accession/sample.rb @@ -116,6 +116,9 @@ def check_required_fields unless tags.meets_service_requirements?(service, standard_tags) errors.add(:sample, "does not have the required metadata: #{tags.missing.sort.to_sentence.dasherize}.") end + + service_context = service.ena? ? :ENA : :EGA + sample.errors.full_messages.each { |msg| errors.add(:sample, msg) } unless sample.valid?(service_context) end def check_studies diff --git a/spec/lib/accession/sample_spec.rb b/spec/lib/accession/sample_spec.rb index 6c96fb590b..0b2f81a47b 100644 --- a/spec/lib/accession/sample_spec.rb +++ b/spec/lib/accession/sample_spec.rb @@ -90,6 +90,55 @@ def find_value_at_tag(xml_received, tag_name) expect(described_class.new(tag_list, sample)).not_to be_valid end + it 'can have a gender of Female' do + sample.sample_metadata.gender = 'Female' + expect(described_class.new(tag_list, sample)).to be_valid + end + + it 'can not have a leading space in gender' do + sample.sample_metadata.gender = ' Female' + expect(described_class.new(tag_list, sample)).not_to be_valid + end + + it 'can not have a trailing space in gender' do + sample.sample_metadata.gender = 'Female ' + expect(described_class.new(tag_list, sample)).not_to be_valid + end + + it 'can have a gender of Male' do + sample.sample_metadata.gender = 'Male' + expect(described_class.new(tag_list, sample)).to be_valid + end + + it 'can have a gender of Unknown' do + sample.sample_metadata.gender = 'Unknown' + expect(described_class.new(tag_list, sample)).to be_valid + end + + it 'cannot have a gender of Mixed' do + sample.sample_metadata.gender = 'Mixed' + expect(described_class.new(tag_list, sample)).not_to be_valid + end + + it 'cannot have a gender of Hermaphrodite' do + sample.sample_metadata.gender = 'Hermaphrodite' + expect(described_class.new(tag_list, sample)).not_to be_valid + end + + it 'cannot have a gender of Not Applicable' do + sample.sample_metadata.gender = 'Not Applicable' + expect(described_class.new(tag_list, sample)).not_to be_valid + end + + it 'provides a clear error message about valid gender options' do + sample.sample_metadata.gender = 'Invalid Gender' + accession_sample = described_class.new(tag_list, sample) + accession_sample.validate + expect(accession_sample.errors[:sample]).to include( + 'Sample metadata gender must be Female, Male or Unknown' + ) + end + it 'is required to define phenotype' do sample.sample_metadata.phenotype = nil expect(described_class.new(tag_list, sample)).not_to be_valid From e2d54458d241372e2e55df50633bb12f269f77c7 Mon Sep 17 00:00:00 2001 From: Stephen Hulme Date: Wed, 4 Mar 2026 13:29:51 +0000 Subject: [PATCH 3/3] refactor: reduce complexity --- lib/accession/sample.rb | 18 ++++++++++++------ spec/lib/accession/sample_spec.rb | 6 +++--- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/lib/accession/sample.rb b/lib/accession/sample.rb index 214e9c2759..96cce5969d 100644 --- a/lib/accession/sample.rb +++ b/lib/accession/sample.rb @@ -113,12 +113,8 @@ def check_required_fields # EBI will still perform its own validation on submission. return if Flipper.enabled?(:y25_714_skip_accessioning_tag_validation) - unless tags.meets_service_requirements?(service, standard_tags) - errors.add(:sample, "does not have the required metadata: #{tags.missing.sort.to_sentence.dasherize}.") - end - - service_context = service.ena? ? :ENA : :EGA - sample.errors.full_messages.each { |msg| errors.add(:sample, msg) } unless sample.valid?(service_context) + check_tags + check_samples end def check_studies @@ -128,5 +124,15 @@ def check_studies errors.add(:sample, 'can only be accessioned if linked to a releasable, accessioned study.') end end + + def check_tags + unless tags.meets_service_requirements?(service, standard_tags) + errors.add(:sample, "does not have the required metadata: #{tags.missing.sort.to_sentence.dasherize}.") + end + end + + def check_samples + sample.errors.full_messages.each { |msg| errors.add(:sample, msg) } unless sample.valid?(service.provider) + end end end diff --git a/spec/lib/accession/sample_spec.rb b/spec/lib/accession/sample_spec.rb index 0b2f81a47b..8654c382f1 100644 --- a/spec/lib/accession/sample_spec.rb +++ b/spec/lib/accession/sample_spec.rb @@ -95,12 +95,12 @@ def find_value_at_tag(xml_received, tag_name) expect(described_class.new(tag_list, sample)).to be_valid end - it 'can not have a leading space in gender' do + it 'cannot have a leading space in gender' do sample.sample_metadata.gender = ' Female' expect(described_class.new(tag_list, sample)).not_to be_valid end - it 'can not have a trailing space in gender' do + it 'cannot have a trailing space in gender' do sample.sample_metadata.gender = 'Female ' expect(described_class.new(tag_list, sample)).not_to be_valid end @@ -134,7 +134,7 @@ def find_value_at_tag(xml_received, tag_name) sample.sample_metadata.gender = 'Invalid Gender' accession_sample = described_class.new(tag_list, sample) accession_sample.validate - expect(accession_sample.errors[:sample]).to include( + expect(accession_sample.errors.full_messages.join).to include( 'Sample metadata gender must be Female, Male or Unknown' ) end