From bb460c9fb67f29db9790ed966de47f4430ad0d7f Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 30 Mar 2026 01:28:00 +0100 Subject: [PATCH 1/5] fix: track task execution to drive redirect behavior --- app/controllers/workflows_controller.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index 8495e54ae5..ff3b9a98d4 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -41,6 +41,11 @@ def stage # rubocop:todo Metrics/CyclomaticComplexity @stage = params[:id].to_i @task = @workflow.tasks[@stage] + # Track whether the current task execution succeeded; defaults to true when + # just rendering so we only redirect on "Update" after a successful do_task + # call. + task_success = true + # If params[:next_stage] is nil then just render the current task # else actually execute the task. unless params[:next_stage].nil? @@ -67,7 +72,7 @@ def stage # rubocop:todo Metrics/CyclomaticComplexity end end - if params[:commit] == 'Update' + if params[:commit] == 'Update' && task_success redirect_to batch_path(@batch) elsif @stage >= @workflow.tasks.size # All requests have finished all tasks: finish workflow From cbef03ee306f135957b0313c74b4605853cc7185 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 30 Mar 2026 02:03:28 +0100 Subject: [PATCH 2/5] feat: validate required and date-based inputs --- app/models/descriptor.rb | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/app/models/descriptor.rb b/app/models/descriptor.rb index ed37873ea1..7dc9e9d447 100644 --- a/app/models/descriptor.rb +++ b/app/models/descriptor.rb @@ -3,6 +3,9 @@ class Descriptor < ApplicationRecord belongs_to :task serialize :selection, coder: YAML + DATE_YEAR_MIN = 1990 + DATE_YEAR_MAX = 2100 + def is_required? required end @@ -11,4 +14,40 @@ def matches?(search) search.descriptors.each { |descriptor| return true if descriptor.name == name && descriptor.value == value } false end + + # Returns an array of validation errors for the submitted descriptor value. + # The value comes from the Task Details form for a workflow task on a batch. + # @return [Array] An array of error messages, empty if the value is valid + def validate_value(submitted_value) + return ["#{name} is required"] if submitted_value.blank? && is_required? + return [] if submitted_value.blank? + return validate_date_value(submitted_value) if kind == 'Date' + + [] + end + + private + + # Validates that the submitted value is a valid date string in the format + # YYYY-MM-DD, and that the year is within a reasonable range. + # @return [Array] An array of error messages, empty if the value is valid + def validate_date_value(submitted_value) + unless submitted_value.match?(/\A\d{4}-\d{2}-\d{2}\z/) + return ["'#{submitted_value}' is not a valid date for #{name} (expected YYYY-MM-DD)"] + end + + parsed_date = Date.iso8601(submitted_value) + validate_date_year(parsed_date) + rescue ArgumentError + ["'#{submitted_value}' is not a valid date for #{name} (expected YYYY-MM-DD)"] + end + + # Validates that the year of the submitted date is within a reasonable range + # to catch common data entry errors (e.g. 62026 instead of 2026). + # @return [Array] An array of error messages, empty if the year is valid + def validate_date_year(parsed_date) + return [] if parsed_date.year.between?(DATE_YEAR_MIN, DATE_YEAR_MAX) + + ["Date year for #{name} must be between #{DATE_YEAR_MIN} and #{DATE_YEAR_MAX} (got #{parsed_date.year})"] + end end From 05a4146bad4de7de69248fe8bd9c1d86e646468c Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 30 Mar 2026 02:14:38 +0100 Subject: [PATCH 3/5] feat: validate task details inputs in set descriptors handler --- app/models/tasks/set_descriptors_handler.rb | 32 +++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/app/models/tasks/set_descriptors_handler.rb b/app/models/tasks/set_descriptors_handler.rb index 62a501e45d..7ae7e56005 100644 --- a/app/models/tasks/set_descriptors_handler.rb +++ b/app/models/tasks/set_descriptors_handler.rb @@ -8,6 +8,9 @@ def render end def perform + errors = validate_descriptor_inputs + return [false, errors.join(', ')] if errors.any? + # Process each request that has been selected in the front end # by default all requests are selected, but in rare circumstances the user # can uncheck a request to exclude it from the step @@ -28,10 +31,39 @@ def perform private + # Iterates every descriptor on the task and asks each one to validate the + # submitted value. + # @return [Array] An array of error messages, empty if all values are valid + def validate_descriptor_inputs + return [] if task.descriptors.empty? + + selected_requests_for_validation.each_with_object([]) do |request, errors| + errors.concat(descriptor_errors_for(request)) + end.uniq + end + def params @params.respond_to?(:permit!) ? @params.permit!.to_h : @params end + # Returns an array of the requests that have been selected in the UI for + # the Task Details form. + # @return [Array] An array of selected requests + def selected_requests_for_validation + requests.select { |request| selected_requests.include?(request.id) } + end + + # For a given request, returns an array of error messages for any descriptors + # that fail validation. + # @return [Array] An array of error messages, empty if all values are valid + def descriptor_errors_for(request) + submitted = descriptors(request) + + task.descriptors.each_with_object([]) do |descriptor, errors| + errors.concat(descriptor.validate_value(submitted[descriptor.name])) + end + end + def process_request(request) LabEvent.create!( batch: batch, From 71ac87872dc79d32cd516295fb2d715bc73e3928 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 30 Mar 2026 02:19:57 +0100 Subject: [PATCH 4/5] test: add descriptor required and date validation tests --- spec/models/descriptor_spec.rb | 127 +++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 spec/models/descriptor_spec.rb diff --git a/spec/models/descriptor_spec.rb b/spec/models/descriptor_spec.rb new file mode 100644 index 0000000000..296041e233 --- /dev/null +++ b/spec/models/descriptor_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Descriptor do + describe '#validate_value' do + subject(:errors) { descriptor.validate_value(value) } + + context 'when kind is Date' do + context 'when required is true' do + let(:descriptor) { described_class.new(name: 'OTR carrier expiry', kind: 'Date', required: true) } + + context 'with a valid ISO 8601 date' do + let(:value) { '2026-06-01' } + + it { is_expected.to be_empty } + end + + context 'with a blank value' do + let(:value) { '' } + + it { is_expected.to contain_exactly('OTR carrier expiry is required') } + end + + context 'with an invalid date string' do + let(:value) { 'not-a-date' } + + it { + is_expected.to contain_exactly( + "'not-a-date' is not a valid date for OTR carrier expiry (expected YYYY-MM-DD)" + ) + } + end + + context 'with a date in the wrong format (DD/MM/YYYY)' do + let(:value) { '01/06/2026' } + + it { + is_expected.to contain_exactly( + "'01/06/2026' is not a valid date for OTR carrier expiry (expected YYYY-MM-DD)" + ) + } + end + + context 'with a year too far in the past' do + let(:value) { '1989-06-01' } + + it { + is_expected.to contain_exactly( + 'Date year for OTR carrier expiry must be between 1990 and 2100 (got 1989)' + ) + } + end + + context 'with a year too far in the future' do + let(:value) { '2101-06-01' } + + it { + is_expected.to contain_exactly( + 'Date year for OTR carrier expiry must be between 1990 and 2100 (got 2101)' + ) + } + end + + context 'with a typo in the year (e.g., 62026 instead of 2026)' do + let(:value) { '62026-06-01' } + + it { + is_expected.to contain_exactly( + "'62026-06-01' is not a valid date for OTR carrier expiry (expected YYYY-MM-DD)" + ) + } + end + end + + context 'when required is false' do + let(:descriptor) { described_class.new(name: 'OTR carrier expiry', kind: 'Date', required: false) } + + context 'with a blank value' do + let(:value) { '' } + + it { is_expected.to be_empty } + end + + context 'with a valid ISO 8601 date' do + let(:value) { '2026-06-01' } + + it { is_expected.to be_empty } + end + + context 'with an invalid date string' do + let(:value) { 'not-a-date' } + + it { + is_expected.to contain_exactly( + "'not-a-date' is not a valid date for OTR carrier expiry (expected YYYY-MM-DD)" + ) + } + end + + context 'with a year sanity check failure' do + let(:value) { '62026-06-01' } + + it { + is_expected.to contain_exactly( + "'62026-06-01' is not a valid date for OTR carrier expiry (expected YYYY-MM-DD)" + ) + } + end + end + end + + context 'when kind is Text' do + let(:descriptor) { described_class.new(name: 'Comment', kind: 'Text', required: false) } + let(:value) { 'any free text' } + + it { is_expected.to be_empty } + end + + context 'when kind is Selection' do + let(:descriptor) { described_class.new(name: 'Workflow', kind: 'Selection', required: false) } + let(:value) { 'Standard' } + + it { is_expected.to be_empty } + end + end +end From 623db61f0e7629d5128cc67857f51af8f7470594 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 30 Mar 2026 02:25:24 +0100 Subject: [PATCH 5/5] test: add handler tests for descriptor validations --- .../set_descriptors_handler/handler_spec.rb | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/spec/models/tasks/set_descriptors_handler/handler_spec.rb b/spec/models/tasks/set_descriptors_handler/handler_spec.rb index 816d97a1ec..d247a7f66c 100644 --- a/spec/models/tasks/set_descriptors_handler/handler_spec.rb +++ b/spec/models/tasks/set_descriptors_handler/handler_spec.rb @@ -11,7 +11,7 @@ let(:request) { batch.requests.first } let(:controller) { instance_double(WorkflowsController) } let(:user) { create(:user) } - let(:task) { instance_double(SetDescriptorsTask, name: 'Step 1', id: 1) } + let(:task) { instance_double(SetDescriptorsTask, name: 'Step 1', id: 1, descriptors: []) } describe '#perform' do context 'with all requests selected' do @@ -46,5 +46,48 @@ ) end end + + # The handler delegates validation entirely to Descriptor#validate_value. + # Full Date validation rules are covered in spec/models/descriptor_spec.rb. + context 'when a descriptor returns no errors' do + let(:passing_descriptor) { instance_double(Descriptor, name: 'OTR carrier expiry') } + let(:task) { instance_double(SetDescriptorsTask, name: 'Step 1', id: 1, descriptors: [passing_descriptor]) } + let(:params) do + { + batch_id: batch.id.to_s, + descriptors: { 'OTR carrier expiry' => '2026-06-01' }, + request: { request.id.to_s => 'on' } + } + end + + before { allow(passing_descriptor).to receive(:validate_value).with('2026-06-01').and_return([]) } + + it 'returns true' do + expect(handler.perform).to be true + end + end + + context 'when a descriptor returns a validation error' do + let(:error_message) { "'not-a-date' is not a valid date for OTR carrier expiry (expected YYYY-MM-DD)" } + let(:failing_descriptor) { instance_double(Descriptor, name: 'OTR carrier expiry') } + let(:task) { instance_double(SetDescriptorsTask, name: 'Step 1', id: 1, descriptors: [failing_descriptor]) } + let(:params) do + { + batch_id: batch.id.to_s, + descriptors: { 'OTR carrier expiry' => 'not-a-date' }, + request: { request.id.to_s => 'on' } + } + end + + before { allow(failing_descriptor).to receive(:validate_value).with('not-a-date').and_return([error_message]) } + + it 'returns [false, error_message]' do + expect(handler.perform).to eq([false, error_message]) + end + + it 'does not create any lab events' do + expect { handler.perform }.not_to change(LabEvent, :count) + end + end end end