Remove immediate onboarding feature flag by jamiebenstead · Pull Request #741 · RaspberryPiFoundation/editor-api

jamiebenstead · 2026-03-20T11:37:35Z

Status

Closes

What's changed?

Removed all uses of the immediate onboarding feature flag

github-actions · 2026-03-20T11:40:35Z

Test coverage

90.09% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/23439686914

Copilot

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

SchoolOnboardingService intentionally avoids sending ProfileApiClient::UnauthorizedError to Sentry (it logs a warning and returns false), but this operation converts any false into a raised RuntimeError ('School onboarding failed') which is then captured by Sentry.capture_exception(e). That defeats the “no noise to sentry” intent and will likely increase Sentry volume now that onboarding is unconditional. Suggestion: handle onboarded == false without 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.

Copilot

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.

Copilot

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.

Copilot · 2026-03-20T13:59:16Z

        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


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.

Copilot · 2026-03-20T13:59:17Z

  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


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.

zetter-rpf · 2026-03-23T09:29:41Z

+      success = SchoolOnboardingService.new(school).onboard(token: token)
      raise ActiveRecord::Rollback unless success


Is this right? my reading of the old code is when FeatureFlags.immediate_school_onboarding? was true SchoolOnboardingService.new(school).onboard(token: token) would not be called because of the ||.

But also maybe we want to call to call onboard when an admin clicks verify if they signed up before immediate onboarding?

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

spec/models/teacher_invitation_spec.rb:27

After removing the feature-flagged verification rule from TeacherInvitation, this spec no longer asserts the intended behavior for invitations on unverified schools. Add a focused example that documents whether an invitation should be valid or invalid when school.verified_at is nil, so the new always-on behavior can’t regress silently.

  it 'is invalid with an incorrectly formatted email address' do
    invitation = build(:teacher_invitation, email_address: 'not-an-email-address')

    expect(invitation).not_to be_valid
  end

  it 'sends an invitation email after create' do
    school = create(:verified_school)

    invitation = described_class.create!(email_address: 'teacher@example.com', school:)

    assert_enqueued_email_with InvitationMailer, :invite_teacher, params: { invitation: }
  end

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T13:02:21Z

        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
        end


SchoolOnboardingService#onboard intentionally avoids reporting ProfileApiClient::UnauthorizedError to Sentry (it returns false and logs a warning). Here, when onboard returns false (including unauthorized), the code raises and the rescue block reports the raised exception to Sentry anyway, which reintroduces Sentry noise and loses the original failure reason. Consider handling unauthorized (and possibly other expected failures) explicitly without raising/capturing, or have the onboarding service return a richer result so the caller can set response[:error] without generating a new exception.

Copilot · 2026-03-23T13:02:21Z

+          onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:)
+          raise 'School onboarding failed' unless onboarded
        end


School::Create now always performs onboarding and fails the operation if onboarding returns false. There’s no spec coverage for the onboarding-failure path (e.g., SchoolOnboardingService#onboard returns false) to ensure the transaction rolls back and the response is marked as failure with a useful error. Add a unit test that stubs the onboarding service to return false and asserts the school is not persisted and response.failure? is true.

Copilot · 2026-03-23T13:02:22Z

      def create_student(school, school_student_params, token)
        school_id = school.id
        username = school_student_params.fetch(:username)
        encrypted_password = school_student_params.fetch(:password)
        password = DecryptionHelpers.decrypt_password(encrypted_password)
        name = school_student_params.fetch(:name)

        validate(
          username:,
          password:,
-          name:,
-          school: (FeatureFlags.immediate_school_onboarding? ? nil : school)
+          name:
        )

        response = ProfileApiClient.create_school_student(token:, username:, password:, name:, school_id:)
        user_id = response[:created].first
        Role.student.create!(school:, user_id:)
        user_id
      end

-      def validate(username:, password:, name:, school: nil)
+      def validate(username:, password:, name:)
        raise ArgumentError, "username '#{username}' is invalid" if username.blank?
        raise ArgumentError, "password '#{password}' is invalid" if password.size < 8
        raise ArgumentError, "name '#{name}' is invalid" if name.blank?
-
-        return unless school
-        raise ArgumentError, 'school must be verified' unless school.verified?
      end


The feature-flag-dependent verification requirement for creating students was removed from SchoolStudent::Create#validate, but specs only exercise creation with create(:verified_school). Add an example using an unverified school (create(:school) / verified_at: nil) to pin down the intended behavior now that the feature flag is gone (either allowing creation or asserting a specific error).

zetter-rpf

Nice one, great tidy up 🧹

Remove immediate onboarding feature flag

8f7edd1

Copilot AI review requested due to automatic review settings March 20, 2026 11:37

cla-bot Bot added the cla-signed label Mar 20, 2026

Copilot started reviewing on behalf of jamiebenstead March 20, 2026 11:38 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

Comment thread app/services/school_verification_service.rb Outdated

Comment thread spec/lib/feature_flags_spec.rb Outdated

Comment thread lib/feature_flags.rb Outdated

jamiebenstead added 2 commits March 20, 2026 11:47

Update verify func

f6ec5d0

Remove unneeded required

41e69a7

jamiebenstead requested a review from Copilot March 20, 2026 12:54

Copilot started reviewing on behalf of jamiebenstead March 20, 2026 12:55 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

Comment thread app/services/school_verification_service.rb Outdated

Comment thread spec/services/school_verification_service_spec.rb

Comment thread lib/concepts/school/operations/create.rb

Comment thread lib/feature_flags.rb Outdated

jamiebenstead added 2 commits March 20, 2026 13:18

Remove empty files

21f3b39

Change token to not be nil, update tests

ecfbf29

jamiebenstead requested a review from Copilot March 20, 2026 13:52

Copilot started reviewing on behalf of jamiebenstead March 20, 2026 13:52 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

zetter-rpf reviewed Mar 23, 2026

View reviewed changes

Update verify method, no longer pass token down

868563f

jamiebenstead requested a review from Copilot March 23, 2026 12:55

Copilot started reviewing on behalf of jamiebenstead March 23, 2026 12:56 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

Remove unused

688c8d2

zetter-rpf approved these changes Mar 23, 2026

View reviewed changes

jamiebenstead merged commit a2eb1f6 into main Mar 24, 2026
6 checks passed

jamiebenstead deleted the 1231-remove-immediate-onboarding-feature-flag branch March 24, 2026 10:24

		success = SchoolOnboardingService.new(school).onboard(token: token)
		raise ActiveRecord::Rollback unless success

Uh oh!

Conversation

jamiebenstead commented Mar 20, 2026

Status

What's changed?

Uh oh!

github-actions Bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test coverage

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

zetter-rpf Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zetter-rpf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Mar 20, 2026 •

edited

Loading