From 47b4cb305cd247f84876a8e976457b49d6494fdc Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Wed, 12 Aug 2026 10:18:59 +0100 Subject: [PATCH 1/2] Surface the underlying error when school onboarding fails Previously SchoolOnboardingService caught every exception and returned false, so School::Create raised a bare 'School onboarding failed' RuntimeError in its place. Sentry grouped every possible cause under that one message - a Profile API timeout, a 5xx, a role that could not be created - with a backtrace pointing at the raise rather than at whatever actually broke. Non-401 failures were also reported twice, once from the service and again as the RuntimeError. This change lets the exception propagate instead. The existing rescue in School::Create receives the real error, so Sentry records its class, message and backtrace once, and the generic RuntimeError is gone entirely. The ProfileApiClient::UnauthorizedError rescue moves up to School::Create so that Profile 401s, which happen when a user is not yet verified, still log a warning without being sent to Sentry. Those registrations now produce no Sentry event at all, so the 'user is unauthorized' log line is the only trace of them. The operation response keeps its shape, so the API still returns 422, but the message is the underlying error rather than 'School onboarding failed'. Co-Authored-By: Claude Opus 5 --- app/services/school_onboarding_service.rb | 12 +---- lib/concepts/school/operations/create.rb | 19 ++++--- spec/concepts/school/create_spec.rb | 49 +++++++++++++++++++ .../school_onboarding_service_spec.rb | 36 +++++--------- 4 files changed, 77 insertions(+), 39 deletions(-) diff --git a/app/services/school_onboarding_service.rb b/app/services/school_onboarding_service.rb index 2bdb9926a..08cc0630e 100644 --- a/app/services/school_onboarding_service.rb +++ b/app/services/school_onboarding_service.rb @@ -14,16 +14,8 @@ def onboard(token:) ProfileApiClient.create_school(token:, id: school.id, code: school.code) end - rescue ProfileApiClient::UnauthorizedError - # Do not log noise to sentry. - # TODO: consider returning a separate error here to distinguish from other errors and return 401 from the API, not 422 - Rails.logger.warn { "Failed to onboard school #{@school.id}: user is unauthorized" } - false rescue StandardError => e - Sentry.capture_exception(e) - Rails.logger.error { "Failed to onboard school #{@school.id}: #{e.message}" } - false - else - true + Rails.logger.error { "Failed to onboard school #{school.id}: #{e.message}" } + raise end end diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index ff8e2521c..9d25ac933 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -10,21 +10,28 @@ def call(school_params:, creator_id:, token:) School.transaction do response[:school].save! - onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:) - raise 'School onboarding failed' unless onboarded + SchoolOnboardingService.new(response[:school]).onboard(token:) end response + rescue ProfileApiClient::UnauthorizedError => e + # Do not log noise to sentry. The Profile API is only available to verified users over 13. + # TODO: consider returning a separate error here to distinguish from other errors and return 401 from the API, not 422 + Rails.logger.warn { "Failed to onboard school #{response[:school].id}: user is unauthorized" } + failure(response, e) rescue StandardError => e Sentry.capture_exception(e) - response[:error] = response[:school].errors.presence || [e.message] - response[:error_types] = response[:school].errors.details - - response + failure(response, e) end private + def failure(response, error) + response[:error] = response[:school].errors.presence || [error.message] + response[:error_types] = response[:school].errors.details + response + end + def build_school(school_params) School.new(school_params) end diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index 7b9927c3e..c924a391f 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -95,4 +95,53 @@ expect(onboarding_service).to have_received(:onboard).with(token:) end end + + context 'when onboarding fails' do + let(:error) { ProfileApiClient::Error.new('Profile API is unavailable') } + + before do + allow(Sentry).to receive(:capture_exception) + allow(ProfileApiClient).to receive(:create_school).and_raise(error) + end + + it 'does not create a school' do + expect { described_class.call(school_params:, creator_id:, token:) }.not_to change(School, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(school_params:, creator_id:, token:) + expect(response.failure?).to be(true) + end + + it 'sends the underlying error to Sentry rather than a generic error' do + described_class.call(school_params:, creator_id:, token:) + expect(Sentry).to have_received(:capture_exception).with(error) + end + + it 'returns the underlying error message in the operation response' do + response = described_class.call(school_params:, creator_id:, token:) + expect(response[:error]).to eq([error.message]) + end + end + + context 'when onboarding fails because the user is unauthorized in Profile' do + before do + allow(Sentry).to receive(:capture_exception) + allow(ProfileApiClient).to receive(:create_school).and_raise(ProfileApiClient::UnauthorizedError) + end + + it 'does not create a school' do + expect { described_class.call(school_params:, creator_id:, token:) }.not_to change(School, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(school_params:, creator_id:, token:) + expect(response.failure?).to be(true) + end + + it 'does not capture the error in Sentry' do + described_class.call(school_params:, creator_id:, token:) + expect(Sentry).not_to have_received(:capture_exception) + end + end end diff --git a/spec/services/school_onboarding_service_spec.rb b/spec/services/school_onboarding_service_spec.rb index d5ca907a0..850e8fade 100644 --- a/spec/services/school_onboarding_service_spec.rb +++ b/spec/services/school_onboarding_service_spec.rb @@ -28,10 +28,6 @@ service.onboard(token:) expect(ProfileApiClient).to have_received(:create_school).with(token:, id: school.id, code: school.code) end - - it 'returns true' do - expect(service.onboard(token:)).to be(true) - end end describe 'when the school cannot be created in Profile API' do @@ -40,43 +36,37 @@ end it 'does not create owner role' do - service.onboard(token:) + suppress(RuntimeError) { service.onboard(token:) } expect(school_creator).not_to be_school_owner(school) end it 'does not create teacher role' do - service.onboard(token:) + suppress(RuntimeError) { service.onboard(token:) } expect(school_creator).not_to be_school_teacher(school) end - it 'returns false' do - expect(service.onboard(token:)).to be(false) + it 'raises the underlying error' do + expect { service.onboard(token:) }.to raise_error(RuntimeError) end end describe 'when Profile API returns unauthorized' do before do allow(ProfileApiClient).to receive(:create_school).and_raise(ProfileApiClient::UnauthorizedError) - allow(Sentry).to receive(:capture_exception) end it 'does not create owner role' do - service.onboard(token:) + suppress(ProfileApiClient::UnauthorizedError) { service.onboard(token:) } expect(school_creator).not_to be_school_owner(school) end it 'does not create teacher role' do - service.onboard(token:) + suppress(ProfileApiClient::UnauthorizedError) { service.onboard(token:) } expect(school_creator).not_to be_school_teacher(school) end - it 'does not capture the error in Sentry' do - service.onboard(token:) - expect(Sentry).not_to have_received(:capture_exception) - end - - it 'returns false' do - expect(service.onboard(token:)).to be(false) + it 'raises the underlying error' do + expect { service.onboard(token:) }.to raise_error(ProfileApiClient::UnauthorizedError) end end @@ -88,22 +78,22 @@ end it 'does not create owner role' do - service.onboard(token:) + suppress(ActiveRecord::RecordInvalid) { service.onboard(token:) } expect(school_creator).not_to be_school_owner(school) end it 'does not create teacher role' do - service.onboard(token:) + suppress(ActiveRecord::RecordInvalid) { service.onboard(token:) } expect(school_creator).not_to be_school_teacher(school) end it 'does not create school in Profile API' do - service.onboard(token:) + suppress(ActiveRecord::RecordInvalid) { service.onboard(token:) } expect(ProfileApiClient).not_to have_received(:create_school) end - it 'returns false' do - expect(service.onboard(token:)).to be(false) + it 'raises the underlying error' do + expect { service.onboard(token:) }.to raise_error(ActiveRecord::RecordInvalid) end end end From c5fc335bf02a3a6398b086d3f552f366350e57b2 Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Wed, 12 Aug 2026 11:08:24 +0100 Subject: [PATCH 2/2] Log school onboarding failures in one place Previously SchoolOnboardingService rescued StandardError purely to log before re-raising. That clause also matched ProfileApiClient::UnauthorizedError, so a Profile 401 produced two log lines: an error from the service, then the warn from School::Create that is meant to record it. This change drops the rescue. The service now performs the work and lets any failure propagate, leaving School::Create as the single place that handles one - a warn for 401s, Sentry for everything else - so each outcome is recorded once. Non-401 failures no longer get an error-level log line. They are reported to Sentry with more detail than that line carried. Co-Authored-By: Claude Opus 5 --- app/services/school_onboarding_service.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/services/school_onboarding_service.rb b/app/services/school_onboarding_service.rb index 08cc0630e..34efec103 100644 --- a/app/services/school_onboarding_service.rb +++ b/app/services/school_onboarding_service.rb @@ -14,8 +14,5 @@ def onboard(token:) ProfileApiClient.create_school(token:, id: school.id, code: school.code) end - rescue StandardError => e - Rails.logger.error { "Failed to onboard school #{school.id}: #{e.message}" } - raise end end