From e5c20ea334030845d7a0a56f84e22d20b9ee53eb Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Wed, 22 Apr 2026 12:16:49 +0100 Subject: [PATCH 1/3] Remove capture exception in set status This pattern masks errors in test and creates noise for expected errors Replace it with a test for the expected error case that we want to handle. --- lib/concepts/school_project/set_status.rb | 12 +++-- .../school_project/set_status_spec.rb | 47 ++++++------------- 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/lib/concepts/school_project/set_status.rb b/lib/concepts/school_project/set_status.rb index bccde4d5f..a5a2b102e 100644 --- a/lib/concepts/school_project/set_status.rb +++ b/lib/concepts/school_project/set_status.rb @@ -6,11 +6,15 @@ class << self def call(school_project:, status:, user_id:) response = OperationResponse.new response[:school_project] = school_project + + unless response[:school_project].can_transition_to?(status) + message = "Cannot transition from '#{response[:school_project].status}' to '#{status}'" + response[:error] = message + return response + end + response[:school_project].transition_status_to!(status, user_id) - response - rescue StandardError => e - Sentry.capture_exception(e) - response[:error] = e.message + response end end diff --git a/spec/concepts/school_project/set_status_spec.rb b/spec/concepts/school_project/set_status_spec.rb index b8b5c9829..609ac0834 100644 --- a/spec/concepts/school_project/set_status_spec.rb +++ b/spec/concepts/school_project/set_status_spec.rb @@ -9,42 +9,25 @@ let(:school_project) { create(:school_project, school:, project:) } describe '.call' do - context 'when status transition is successful' do - it 'returns a successful operation response' do - response = described_class.call(school_project:, status: :submitted, user_id: student.id) - expect(response.success?).to be(true) - end - - it 'updates the school project status' do - described_class.call(school_project:, status: :submitted, user_id: student.id) - expect(school_project.status).to eq('submitted') - end - - it 'returns the updated school project in the response' do - response = described_class.call(school_project:, status: :submitted, user_id: student.id) - expect(response[:school_project]).to be_a(SchoolProject) - end + it 'returns a successful operation response' do + response = described_class.call(school_project:, status: :submitted, user_id: student.id) + expect(response.success?).to be(true) end - context 'when status transition fails' do - before do - allow(school_project).to receive(:transition_status_to!).and_raise(StandardError, 'Transition failed') - end - - it 'returns a failed operation response' do - response = described_class.call(school_project:, status: :submitted, user_id: student.id) - expect(response.success?).to be(false) - end + it 'updates the school project status' do + described_class.call(school_project:, status: :submitted, user_id: student.id) + expect(school_project.status).to eq('submitted') + end - it 'does not update the school project status' do - described_class.call(school_project:, status: :submitted, user_id: student.id) - expect(school_project.status).to eq('unsubmitted') - end + it 'returns the updated school project in the response' do + response = described_class.call(school_project:, status: :submitted, user_id: student.id) + expect(response[:school_project]).to be_a(SchoolProject) + end - it 'includes the error message in the response' do - response = described_class.call(school_project:, status: :submitted, user_id: student.id) - expect(response[:error]).to eq('Transition failed') - end + it 'returns an error when transitioning to an invalid status' do + response = described_class.call(school_project:, status: :returned, user_id: student.id) + expect(response.success?).to be(false) + expect(response[:error]).to eq("Cannot transition from #{school_project.status} to returned") end end end From 3fb11332b2fab921e3b5e36922bb66cb7c820246 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Wed, 22 Apr 2026 13:55:01 +0100 Subject: [PATCH 2/3] Don't error when setting a school project to the state it's already in Avoid transitioning the state if it's already in that state. Presumably this might happen if someone double-submits something in the frontend. There are currently errors in sentry for all the controller actions. This is safe to do as we don't perform any actions in the SchoolProjectsController if this was successful (such as sending emails). I've removed the specs around complete - since the complete to complete is guarded against and all other states can transition to complete, there are no invalid transitions to test. Note this won't catch errors that might happen if the school project state has already been updated by process after it is loaded. If we see this happen in Sentry we can catch that specific error. second --- app/models/school_project.rb | 2 +- lib/concepts/school_project/set_status.rb | 3 ++- spec/concepts/school_project/set_status_spec.rb | 9 ++++++++- spec/features/school_project/complete_spec.rb | 15 --------------- 4 files changed, 11 insertions(+), 18 deletions(-) diff --git a/app/models/school_project.rb b/app/models/school_project.rb index 52aceb96b..c8171a743 100644 --- a/app/models/school_project.rb +++ b/app/models/school_project.rb @@ -40,7 +40,7 @@ def returned? state_machine.in_state?(:returned) end - delegate :can_transition_to?, :history, to: :state_machine + delegate :can_transition_to?, :history, :in_state?, to: :state_machine private diff --git a/lib/concepts/school_project/set_status.rb b/lib/concepts/school_project/set_status.rb index a5a2b102e..780ba6318 100644 --- a/lib/concepts/school_project/set_status.rb +++ b/lib/concepts/school_project/set_status.rb @@ -7,6 +7,8 @@ def call(school_project:, status:, user_id:) response = OperationResponse.new response[:school_project] = school_project + return response if response[:school_project].in_state?(status) + unless response[:school_project].can_transition_to?(status) message = "Cannot transition from '#{response[:school_project].status}' to '#{status}'" response[:error] = message @@ -14,7 +16,6 @@ def call(school_project:, status:, user_id:) end response[:school_project].transition_status_to!(status, user_id) - response end end diff --git a/spec/concepts/school_project/set_status_spec.rb b/spec/concepts/school_project/set_status_spec.rb index 609ac0834..9f96baf14 100644 --- a/spec/concepts/school_project/set_status_spec.rb +++ b/spec/concepts/school_project/set_status_spec.rb @@ -27,7 +27,14 @@ it 'returns an error when transitioning to an invalid status' do response = described_class.call(school_project:, status: :returned, user_id: student.id) expect(response.success?).to be(false) - expect(response[:error]).to eq("Cannot transition from #{school_project.status} to returned") + expect(response[:error]).to eq("Cannot transition from '#{school_project.status}' to 'returned'") + end + + it 'is successful when transitioning to the same status' do + school_project.transition_status_to!(:submitted, student.id) + response = described_class.call(school_project:, status: :submitted, user_id: student.id) + expect(response.success?).to be(true) + expect(school_project.status).to eq('submitted') end end end diff --git a/spec/features/school_project/complete_spec.rb b/spec/features/school_project/complete_spec.rb index 7e8afa240..242e1a8e5 100644 --- a/spec/features/school_project/complete_spec.rb +++ b/spec/features/school_project/complete_spec.rb @@ -60,21 +60,6 @@ expect(student_project.school_project).to be_complete end end - - context 'when attempting an invalid status transition' do - before do - student_project.school_project.transition_status_to!(:complete, student.id) - post("/api/projects/#{student_project.identifier}/complete", headers:) - end - - it 'completes unauthorized response' do - expect(response).to have_http_status(:unprocessable_content) - end - - it 'completes error message' do - expect(JSON.parse(response.body)['error']).to eq("Cannot transition from 'complete' to 'complete'") - end - end end context 'when user does not own the project and is not the class teacher' do From aa514e2f02a81c7c082ee573ceaf3cd412358c68 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Wed, 22 Apr 2026 14:39:41 +0100 Subject: [PATCH 3/3] Handle Statesman::TransitionConflictError errors When looking through sentry, the only other error from the school projects controller is a Statesman::TransitionConflictError. This might happen if the object has already transitioned between it being loaded and the request running. Retry the request once to handle this. --- lib/concepts/school_project/set_status.rb | 31 +++++++++++++------ .../school_project/set_status_spec.rb | 22 +++++++++++++ 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/lib/concepts/school_project/set_status.rb b/lib/concepts/school_project/set_status.rb index 780ba6318..aa677443a 100644 --- a/lib/concepts/school_project/set_status.rb +++ b/lib/concepts/school_project/set_status.rb @@ -4,19 +4,32 @@ class SchoolProject class SetStatus class << self def call(school_project:, status:, user_id:) - response = OperationResponse.new - response[:school_project] = school_project + retry_on(Statesman::TransitionConflictError, attempts: 2, record: school_project) do + response = OperationResponse.new + response[:school_project] = school_project - return response if response[:school_project].in_state?(status) + return response if school_project.in_state?(status) - unless response[:school_project].can_transition_to?(status) - message = "Cannot transition from '#{response[:school_project].status}' to '#{status}'" - response[:error] = message - return response + unless school_project.can_transition_to?(status) + message = "Cannot transition from '#{school_project.status}' to '#{status}'" + response[:error] = message + return response + end + + school_project.transition_status_to!(status, user_id) + response end + end + + private - response[:school_project].transition_status_to!(status, user_id) - response + def retry_on(exception_class, attempts:, record:) + yield + rescue exception_class + record.reload + attempts -= 1 + retry if attempts.positive? + raise end end end diff --git a/spec/concepts/school_project/set_status_spec.rb b/spec/concepts/school_project/set_status_spec.rb index 9f96baf14..c98f4ed99 100644 --- a/spec/concepts/school_project/set_status_spec.rb +++ b/spec/concepts/school_project/set_status_spec.rb @@ -36,5 +36,27 @@ expect(response.success?).to be(true) expect(school_project.status).to eq('submitted') end + + it 'retries when transition raises a "Statesman::TransitionConflictError" error' do + call_count = 0 + allow(school_project).to receive(:transition_status_to!).and_wrap_original do |original, *args| + call_count += 1 + raise Statesman::TransitionConflictError if call_count == 1 + + original.call(*args) + end + + response = described_class.call(school_project:, status: :submitted, user_id: student.id) + expect(response.success?).to be(true) + expect(school_project.status).to eq('submitted') + end + + it 'raises the "Statesman::TransitionConflictError" error after 2 attempts' do + allow(school_project).to receive(:transition_status_to!).and_raise(Statesman::TransitionConflictError).twice + + expect do + described_class.call(school_project:, status: :submitted, user_id: student.id) + end.to raise_error(Statesman::TransitionConflictError) + end end end