Surface the underlying error when school onboarding fails - #962
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves observability and error reporting for school onboarding failures by letting the original exceptions propagate to School::Create, so Sentry captures the real error class/message/backtrace once and the API returns the underlying error message (still as a 422 via the existing OperationResponse shape).
Changes:
- Stop swallowing exceptions in
SchoolOnboardingService; re-raise after logging so callers can handle the real error. - Update
School::Createto handleProfileApiClient::UnauthorizedErrorspecially (warn-only, no Sentry), and to capture all other onboarding failures in Sentry with the original exception. - Update and extend specs to assert raising behavior in the service and that
School::Createreports the underlying error (and suppresses Sentry for Profile 401s).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/services/school_onboarding_service.rb |
Stops returning booleans and re-raises errors so upstream can capture/report the original exception. |
lib/concepts/school/operations/create.rb |
Removes the generic “School onboarding failed” raise; captures real exceptions in Sentry and handles Profile 401s without Sentry. |
spec/services/school_onboarding_service_spec.rb |
Updates expectations from boolean returns to raised exceptions; ensures side effects don’t persist on failures. |
spec/concepts/school/create_spec.rb |
Adds coverage for Sentry capturing the underlying error and for suppressing Sentry on Profile unauthorized onboarding failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+17
to
+19
| 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 |
Test coverage92.29% line coverage reported by SimpleCov. |
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
zetter-rpf
reviewed
Aug 17, 2026
|
|
||
| response | ||
| rescue ProfileApiClient::UnauthorizedError => e | ||
| # Do not log noise to sentry. The Profile API is only available to verified users over 13. |
Contributor
There was a problem hiding this comment.
You've added 'The Profile API is only available to verified users over 13' to this comment. What does this mean - that all the instances from this error are from users under 13? How do we know that?
Contributor
Author
There was a problem hiding this comment.
@zetter-rpf You're right, we don't know that, so have removed it now. Claude lifted the comment from profile_api_client.rb:264
zetter-rpf
approved these changes
Aug 17, 2026
zetter-rpf
left a comment
zetter-rpf
left a comment
Contributor
There was a problem hiding this comment.
Great simplification, I've asked a clarifying question about a comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Status