diff --git a/app/services/school_onboarding_service.rb b/app/services/school_onboarding_service.rb index 2bdb9926a..34efec103 100644 --- a/app/services/school_onboarding_service.rb +++ b/app/services/school_onboarding_service.rb @@ -14,16 +14,5 @@ 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 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