Conversation
Test coverage90.1% line coverage reported by SimpleCov. |
There was a problem hiding this comment.
Pull request overview
Removes the ENABLE_IMMEDIATE_SCHOOL_ONBOARDING feature flag and updates the application and specs to assume immediate onboarding behavior unconditionally.
Changes:
- Deleted the
FeatureFlags.immediate_school_onboarding?flag and removed related ENV/config/spec coverage. - Made school code generation and onboarding paths unconditional (and removed conditional validation/guardrails that depended on the flag).
- Simplified multiple specs by removing flag-on/flag-off branches.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/services/school_verification_service_spec.rb | Removes feature-flag branches; now expects verification to succeed without flag toggling. |
| spec/models/teacher_invitation_spec.rb | Removes flag-dependent validity expectations for unverified schools. |
| spec/models/school_spec.rb | Removes flag setup around code validation/generation specs. |
| spec/lib/feature_flags_spec.rb | Removes tests for the deleted flag method (file is now empty aside from requires). |
| spec/concepts/school/create_spec.rb | Updates expectations to always call onboarding service. |
| spec/concepts/school_teacher/invite_spec.rb | Removes flag-dependent behavior checks for unverified schools. |
| spec/concepts/school_student/create_spec.rb | Removes flag-dependent behavior checks for unverified schools. |
| lib/feature_flags.rb | Removes the immediate onboarding flag method (module now empty). |
| lib/concepts/school/operations/create.rb | Always runs SchoolOnboardingService after saving the school. |
| lib/concepts/school_student/create.rb | Removes verification gating from student creation validation. |
| app/services/school_verification_service.rb | Removes feature-flag logic and now calls onboarding unconditionally during verification. |
| app/models/teacher_invitation.rb | Removes the “school must be verified” validation guard. |
| app/models/school.rb | Makes code generation unconditional and removes flag-based code generation in verify!. |
| .env.example | Removes the ENABLE_IMMEDIATE_SCHOOL_ONBOARDING example entry. |
Comments suppressed due to low confidence (1)
lib/concepts/school/operations/create.rb:21
SchoolOnboardingServiceintentionally avoids sendingProfileApiClient::UnauthorizedErrorto Sentry (it logs a warning and returnsfalse), but this operation converts anyfalseinto a raisedRuntimeError('School onboarding failed') which is then captured bySentry.capture_exception(e). That defeats the “no noise to sentry” intent and will likely increase Sentry volume now that onboarding is unconditional. Suggestion: handleonboarded == falsewithout raising/capturing (e.g., set an error on the response and roll back the transaction) or propagate a structured failure reason so the rescue can decide whether to report to Sentry.
School.transaction do
response[:school].save!
onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:)
raise 'School onboarding failed' unless onboarded
end
response
rescue StandardError => e
Sentry.capture_exception(e)
response[:error] = response[:school].errors.presence || [e.message]
response[:error_types] = response[:school].errors.details
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
spec/lib/feature_flags_spec.rb:2
- This spec file no longer contains any examples and can be removed to keep the spec suite tidy and avoid implying there is still behavior under test for feature flags.
# frozen_string_literal: true
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| School.transaction do | ||
| response[:school].save! | ||
|
|
||
| # TODO: Remove this conditional once the feature flag is retired | ||
| if FeatureFlags.immediate_school_onboarding? | ||
| onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:) | ||
| raise 'School onboarding failed' unless onboarded | ||
| end | ||
| onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:) | ||
| raise 'School onboarding failed' unless onboarded |
There was a problem hiding this comment.
School::Create.call wraps save+onboarding in a School.transaction but then rescues the raised error and returns a failure response. If School::Create.call is invoked inside an outer transaction (e.g. SchoolImportJob wraps it in School.transaction), this can prevent the outer transaction from rolling back while still returning failure, leaving the created school (and possibly roles) committed. To keep the operation atomic regardless of caller context, consider using School.transaction(requires_new: true) (savepoint) or otherwise ensuring the DB changes are rolled back when onboarding fails before returning a failure response.
| describe '#verify' do | ||
| describe 'when immediate onboarding is enabled' do | ||
| # TODO: Remove this block once the feature flag is retired | ||
| around do |example| | ||
| ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do | ||
| example.run | ||
| end | ||
| describe 'when school can be saved' do | ||
| it 'saves the school' do | ||
| service.verify(token:) | ||
| expect(school).to be_persisted |
There was a problem hiding this comment.
SchoolVerificationService#verify now always performs onboarding and rolls back on onboarding failure, but the spec no longer covers the onboarding-failure path (e.g., when SchoolOnboardingService#onboard returns false/raises) to assert the transaction is rolled back (school not persisted / verified_at not set). Adding an example for this failure case would help prevent regressions in the new always-onboarding behavior.
Status
What's changed?