From 014c32a4b64f40797f6adcec861bd8c8d672817d Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Fri, 19 Dec 2025 10:13:21 +0000 Subject: [PATCH 01/16] 1069: Add immediate onboarding --- .env.example | 3 + app/controllers/api/schools_controller.rb | 2 +- app/jobs/school_import_job.rb | 3 +- app/models/school.rb | 49 +++- app/services/school_onboarding_service.rb | 24 ++ app/services/school_verification_service.rb | 7 +- lib/concepts/school/operations/create.rb | 15 +- lib/concepts/school/operations/update.rb | 4 + lib/feature_flags.rb | 7 + lib/tasks/seeds_helper.rb | 4 +- spec/concepts/school/create_spec.rb | 28 ++- spec/factories/school.rb | 10 +- .../features/school/creating_a_school_spec.rb | 2 + spec/features/school/showing_a_school_spec.rb | 5 +- spec/jobs/school_import_job_spec.rb | 6 + spec/lib/feature_flags_spec.rb | 37 +++ spec/models/school_spec.rb | 225 ++++++++++++++---- .../school_onboarding_service_spec.rb | 84 +++++++ .../school_verification_service_spec.rb | 117 +-------- 19 files changed, 445 insertions(+), 187 deletions(-) create mode 100644 app/services/school_onboarding_service.rb create mode 100644 lib/feature_flags.rb create mode 100644 spec/lib/feature_flags_spec.rb create mode 100644 spec/services/school_onboarding_service_spec.rb diff --git a/.env.example b/.env.example index 14cf08ae5..f3c82fe1d 100644 --- a/.env.example +++ b/.env.example @@ -51,3 +51,6 @@ EDITOR_ENCRYPTION_KEY=a1b2c3d4e5f67890123456789abcdef0123456789abcdef0123456789a # The sandbox creds can be found in 1password under "Google Cloud Console: CEfE Sandbox" GOOGLE_CLIENT_ID=changeme.apps.googleusercontent.com GOOGLE_CLIENT_SECRET=changeme + +# Enable immediate onboarding for schools +ENABLE_IMMEDIATE_SCHOOL_ONBOARDING=true diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index a33ac6fba..0fce16d39 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -16,7 +16,7 @@ def show end def create - result = School::Create.call(school_params:, creator_id: current_user.id) + result = School::Create.call(school_params:, creator_id: current_user.id, token: current_user.token) if result.success? @school = result[:school] diff --git a/app/jobs/school_import_job.rb b/app/jobs/school_import_job.rb index 19e4aee1b..58f332a37 100644 --- a/app/jobs/school_import_job.rb +++ b/app/jobs/school_import_job.rb @@ -71,7 +71,8 @@ def import_school(school_data) School.transaction do result = School::Create.call( school_params: school_params, - creator_id: proposed_owner[:id] + creator_id: proposed_owner[:id], + token: @token ) if result.success? diff --git a/app/models/school.rb b/app/models/school.rb index a1b1beb57..33857cd7f 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class School < ApplicationRecord + class DuplicateSchoolError < StandardError; end + has_many :classes, class_name: :SchoolClass, inverse_of: :school, dependent: :destroy has_many :lessons, dependent: :nullify has_many :projects, dependent: :nullify @@ -15,6 +17,8 @@ class School < ApplicationRecord validates :website, presence: true, format: { with: VALID_URL_REGEX, message: I18n.t('validations.school.website') } validates :address_line_1, presence: true validates :municipality, presence: true + validates :administrative_area, presence: true + validates :postal_code, presence: true validates :country_code, presence: true, inclusion: { in: ISO3166::Country.codes } validates :reference, uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true, message: I18n.t('validations.school.reference_urn_exists') }, @@ -39,8 +43,6 @@ class School < ApplicationRecord validates :verified_at, absence: { if: proc { |school| school.rejected? } } validates :code, uniqueness: { allow_nil: true }, - presence: { if: proc { |school| school.verified? } }, - absence: { unless: proc { |school| school.verified? } }, format: { with: /\d\d-\d\d-\d\d/, allow_nil: true } validate :verified_at_cannot_be_changed validate :code_cannot_be_changed @@ -49,8 +51,11 @@ class School < ApplicationRecord before_validation :normalize_district_fields before_validation :normalize_school_roll_number + before_save :prevent_duplicate_school before_save :format_uk_postal_code, if: :should_format_uk_postal_code? + after_commit :generate_code!, on: :create, if: -> { FeatureFlags.immediate_school_onboarding? } + def self.find_for_user!(user) school = Role.find_by(user_id: user.id)&.school || find_by(creator_id: user.id) raise ActiveRecord::RecordNotFound unless school @@ -71,9 +76,18 @@ def rejected? end def verify! + generate_code! if ENV['ENABLE_IMMEDIATE_SCHOOL_ONBOARDING'].blank? + + update!(verified_at: Time.zone.now) + end + + def generate_code! + return code if code.present? + attempts = 0 begin - update!(verified_at: Time.zone.now, code: ForEducationCodeGenerator.generate) + new_code = ForEducationCodeGenerator.generate + update!(code: new_code) rescue ActiveRecord::RecordInvalid => e raise unless e.record.errors[:code].include?('has already been taken') && attempts <= 5 @@ -87,6 +101,8 @@ def reject end def reopen + return false unless rejected? + update(rejected_at: nil) end @@ -131,7 +147,7 @@ def rejected_at_cannot_be_changed end def code_cannot_be_changed - errors.add(:code, 'cannot be changed after verification') if code_was.present? && code_changed? + errors.add(:code, 'cannot be changed after onboarding') if code_was.present? && code_changed? end def should_format_uk_postal_code? @@ -156,4 +172,29 @@ def format_uk_postal_code # ensures UK postcodes are always formatted correctly (as the inward code is always 3 chars long) self.postal_code = "#{cleaned_postal_code[0..-4]} #{cleaned_postal_code[-3..]}" end + + def prevent_duplicate_school + name_threshold = 0.6 + municipality_threshold = 0.7 + postal_code_threshold = 0.8 + max_edit_distance = 3 + + normalized_postal = postal_code.to_s.gsub(/\s+/, '') + + duplicate_school = School + .where.not(id:) + .where(country_code:) + .where( + 'similarity(lower(name), ?) >= ? AND levenshtein(lower(name), ?) <= ?', + name.to_s, name_threshold, name.to_s, max_edit_distance + ) + .where('similarity(lower(municipality), ?) >= ?', municipality.to_s, municipality_threshold) + .where( + "similarity(lower(replace(postal_code, ' ', '')), ?) >= ?", + normalized_postal, postal_code_threshold + ) + .first + + raise DuplicateSchoolError, I18n.t('validations.school.duplicate_school') if duplicate_school + end end diff --git a/app/services/school_onboarding_service.rb b/app/services/school_onboarding_service.rb new file mode 100644 index 000000000..ea82e439e --- /dev/null +++ b/app/services/school_onboarding_service.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class SchoolOnboardingService + attr_reader :school + + def initialize(school) + @school = school + end + + def onboard(token:) + School.transaction do + Role.owner.create!(user_id: school.creator_id, school:) + Role.teacher.create!(user_id: school.creator_id, school:) + + ProfileApiClient.create_school(token:, id: school.id, code: school.code) + end + 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/app/services/school_verification_service.rb b/app/services/school_verification_service.rb index 579bfdfdc..6b78ee59d 100644 --- a/app/services/school_verification_service.rb +++ b/app/services/school_verification_service.rb @@ -7,12 +7,11 @@ def initialize(school) @school = school end - def verify(token:) + def verify(token: nil) School.transaction do school.verify! - Role.owner.create!(user_id: school.creator_id, school:) - Role.teacher.create!(user_id: school.creator_id, school:) - ProfileApiClient.create_school(token:, id: school.id, code: school.code) + + SchoolOnboardingService.new(school).onboard(token: token) unless FeatureFlags.immediate_school_onboarding? end rescue StandardError => e Sentry.capture_exception(e) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index b9662486d..8fd531cca 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -3,14 +3,23 @@ class School class Create class << self - def call(school_params:, creator_id:) + def call(school_params:, creator_id:, token:) response = OperationResponse.new response[:school] = build_school(school_params.merge!(creator_id:)) - response[:school].save! + + School.transaction do + response[:school].save! + + SchoolOnboardingService.new(response[:school]).onboard(token:) if FeatureFlags.immediate_school_onboarding? + end + + response + rescue School::DuplicateSchoolError => e + response[:error] = e.message response rescue StandardError => e Sentry.capture_exception(e) - response[:error] = response[:school].errors + response[:error] = response[:school].errors.presence || [e.message] response[:error_types] = response[:school].errors.details response diff --git a/lib/concepts/school/operations/update.rb b/lib/concepts/school/operations/update.rb index 439f30d64..4140483bb 100644 --- a/lib/concepts/school/operations/update.rb +++ b/lib/concepts/school/operations/update.rb @@ -9,6 +9,10 @@ def call(school:, school_params:) response[:school].assign_attributes(school_params) response[:school].save! response + rescue School::DuplicateSchoolError => e + Sentry.capture_exception(e) + response[:error] = e.message + response rescue StandardError => e Sentry.capture_exception(e) errors = response[:school].errors.full_messages.join(',') diff --git a/lib/feature_flags.rb b/lib/feature_flags.rb new file mode 100644 index 000000000..86a76d637 --- /dev/null +++ b/lib/feature_flags.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module FeatureFlags + def self.immediate_school_onboarding? + ENV['ENABLE_IMMEDIATE_SCHOOL_ONBOARDING'] == 'true' + end +end diff --git a/lib/tasks/seeds_helper.rb b/lib/tasks/seeds_helper.rb index 6a03bcb36..67058fc83 100644 --- a/lib/tasks/seeds_helper.rb +++ b/lib/tasks/seeds_helper.rb @@ -20,8 +20,10 @@ def create_school(creator_id, school_id = nil) school.name = Faker::Educator.secondary_school school.website = Faker::Internet.url(scheme: 'https') school.address_line_1 = Faker::Address.street_address + school.administrative_area = "#{Faker::Address.city}shire" school.municipality = Faker::Address.city - school.country_code = country_code + school.postal_code = Faker::Address.postcode + school.country_code = Faker::Address.country_code school.creator_id = creator_id school.creator_agree_authority = true school.creator_agree_terms_and_conditions = true diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index f8375c7d9..5d6d428b0 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -8,7 +8,9 @@ name: 'Test School', website: 'http://www.example.com', address_line_1: 'Address Line 1', + administrative_area: 'Greater London', municipality: 'Greater London', + postal_code: 'SW1A 1AA', country_code: 'GB', reference: '100000', creator_agree_authority: true, @@ -21,27 +23,31 @@ let(:token) { UserProfileMock::TOKEN } let(:creator_id) { SecureRandom.uuid } + before do + allow(ProfileApiClient).to receive(:create_school).and_return(true) + end + it 'returns a successful operation response' do - response = described_class.call(school_params:, creator_id:) + response = described_class.call(school_params:, creator_id:, token:) expect(response.success?).to be(true) end it 'creates a school' do - expect { described_class.call(school_params:, creator_id:) }.to change(School, :count).by(1) + expect { described_class.call(school_params:, creator_id:, token:) }.to change(School, :count).by(1) end it 'returns the school in the operation response' do - response = described_class.call(school_params:, creator_id:) + response = described_class.call(school_params:, creator_id:, token:) expect(response[:school]).to be_a(School) end it 'assigns the name' do - response = described_class.call(school_params:, creator_id:) + response = described_class.call(school_params:, creator_id:, token:) expect(response[:school].name).to eq('Test School') end it 'assigns the creator_id' do - response = described_class.call(school_params:, creator_id:) + response = described_class.call(school_params:, creator_id:, token:) expect(response[:school].creator_id).to eq(creator_id) end @@ -53,26 +59,26 @@ end it 'does not create a school' do - expect { described_class.call(school_params:, creator_id:) }.not_to change(School, :count) + 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:) + response = described_class.call(school_params:, creator_id:, token:) expect(response.failure?).to be(true) end it 'returns the correct number of objects in the operation response' do - response = described_class.call(school_params:, creator_id:) - expect(response[:error].count).to eq(9) + response = described_class.call(school_params:, creator_id:, token:) + expect(response[:error].count).to eq(11) end it 'returns the correct type of object in the operation response' do - response = described_class.call(school_params:, creator_id:) + response = described_class.call(school_params:, creator_id:, token:) expect(response[:error].first).to be_a(ActiveModel::Error) end it 'sent the exception to Sentry' do - described_class.call(school_params:, creator_id:) + described_class.call(school_params:, creator_id:, token:) expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) end end diff --git a/spec/factories/school.rb b/spec/factories/school.rb index d9bdaf6b6..9378df265 100644 --- a/spec/factories/school.rb +++ b/spec/factories/school.rb @@ -2,10 +2,12 @@ FactoryBot.define do factory :school do - sequence(:name) { |n| "School #{n}" } - website { 'http://www.example.com' } - address_line_1 { 'Address Line 1' } - municipality { 'Greater London' } + name { "#{Faker::Educator.primary_school} #{Faker::Address.city}" } + website { Faker::Internet.url } + address_line_1 { Faker::Address.street_address } + administrative_area { "#{Faker::Address.city}shire" } + municipality { Faker::Address.city } + postal_code { Faker::Address.postcode } country_code { 'GB' } sequence(:reference) { |n| format('%06d', 100_000 + n) } school_roll_number { nil } diff --git a/spec/features/school/creating_a_school_spec.rb b/spec/features/school/creating_a_school_spec.rb index aafa2f388..ca5a4b69f 100644 --- a/spec/features/school/creating_a_school_spec.rb +++ b/spec/features/school/creating_a_school_spec.rb @@ -17,7 +17,9 @@ name: 'Test School', website: 'http://www.example.com', address_line_1: 'Address Line 1', + administrative_area: 'Greater London', municipality: 'Greater London', + postal_code: 'SW1A 1AA', country_code: 'GB', reference: '100000', creator_agree_authority: true, diff --git a/spec/features/school/showing_a_school_spec.rb b/spec/features/school/showing_a_school_spec.rb index edf1c9fc7..0af82ac78 100644 --- a/spec/features/school/showing_a_school_spec.rb +++ b/spec/features/school/showing_a_school_spec.rb @@ -34,10 +34,9 @@ end it 'responds 403 Forbidden when the user belongs to a different school' do - Role.owner.find_by(user_id: owner.id, school:).delete - school.update!(id: SecureRandom.uuid) + other_school = create(:school) - get("/api/schools/#{school.id}", headers:) + get("/api/schools/#{other_school.id}", headers:) expect(response).to have_http_status(:forbidden) end end diff --git a/spec/jobs/school_import_job_spec.rb b/spec/jobs/school_import_job_spec.rb index b74b470d8..4ad9c9455 100644 --- a/spec/jobs/school_import_job_spec.rb +++ b/spec/jobs/school_import_job_spec.rb @@ -15,7 +15,9 @@ name: 'Test School 1', website: 'https://test1.example.com', address_line_1: '123 Main St', + administrative_area: 'Massachusetts', municipality: 'Springfield', + postal_code: '01101', country_code: 'US', owner_email: 'owner1@example.com', district_name: 'Some District', @@ -25,7 +27,9 @@ name: 'Test School 2', website: 'https://test2.example.com', address_line_1: '456 Oak Ave', + administrative_area: 'Massachusetts', municipality: 'Boston', + postal_code: '02101', country_code: 'US', owner_email: 'owner2@example.com', district_name: 'Other District', @@ -126,7 +130,9 @@ 'name' => 'Test School 1', 'website' => 'https://test1.example.com', 'address_line_1' => '123 Main St', + 'administrative_area' => 'Massachusetts', 'municipality' => 'Springfield', + 'postal_code' => '01101', 'country_code' => 'us', 'owner_email' => 'owner1@example.com', 'district_name' => 'Some District', diff --git a/spec/lib/feature_flags_spec.rb b/spec/lib/feature_flags_spec.rb new file mode 100644 index 000000000..b7fa7b085 --- /dev/null +++ b/spec/lib/feature_flags_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe FeatureFlags do + describe '.immediate_school_onboarding?' do + it 'returns true when ENV is set to "true"' do + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do + expect(described_class.immediate_school_onboarding?).to be(true) + end + end + + it 'returns false when ENV is not set' do + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: nil) do + expect(described_class.immediate_school_onboarding?).to be(false) + end + end + + it 'returns false when ENV is set to empty string' do + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: '') do + expect(described_class.immediate_school_onboarding?).to be(false) + end + end + + it 'returns false when ENV is set to "false"' do + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'false') do + expect(described_class.immediate_school_onboarding?).to be(false) + end + end + + it 'returns false when ENV is set to any other value' do + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: '1') do + expect(described_class.immediate_school_onboarding?).to be(false) + end + end + end +end diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 9b878e369..ec6e30011 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -5,7 +5,7 @@ RSpec.describe School do let(:student) { create(:student, school:) } let(:teacher) { create(:teacher, school:) } - let(:school) { create(:school, creator_id: SecureRandom.uuid) } + let(:school) { create(:school) } let!(:us_school) { create(:school, country_code: 'US', district_name: 'Some District', district_nces_id: '010000000001', creator_id: SecureRandom.uuid) } let!(:ireland_school) { create(:school, country_code: 'IE', school_roll_number: '01572D', creator_id: SecureRandom.uuid) } @@ -355,6 +355,16 @@ expect(school).not_to be_valid end + it 'requires an administrative_area' do + school.administrative_area = ' ' + expect(school).not_to be_valid + end + + it 'requires a postal_code' do + school.postal_code = ' ' + expect(school).not_to be_valid + end + it 'requires a country_code' do school.country_code = ' ' expect(school).not_to be_valid @@ -413,32 +423,30 @@ expect(school.errors[:verified_at]).to include('cannot be changed after verification') end - it 'requires #code to be unique' do - school.update!(code: '00-00-00', verified_at: Time.current) - another_school = build(:school, code: '00-00-00') - another_school.valid? - expect(another_school.errors[:code]).to include('has already been taken') - end - - it 'requires #code to be set when the school is verified' do - school.update(verified_at: Time.current) - expect(school.errors[:code]).to include("can't be blank") - end + context 'code validations' do + around do |example| + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do + example.run + end + end - it 'requires code to be blank until the school is verified' do - school.update(code: 'school-code') - expect(school.errors[:code]).to include('must be blank') - end + it 'requires #code to be unique' do + school # ensure existing school has a code + another_school = build(:school) + another_school.code = school.code + another_school.valid? + expect(another_school.errors[:code]).to include('has already been taken') + end - it 'requires code to be formatted as 3 pairs of digits separated by hyphens' do - school.update(code: 'invalid', verified_at: Time.current) - expect(school.errors[:code]).to include('is invalid') - end + it 'requires code to be formatted as 3 pairs of digits separated by hyphens' do + school.update(code: 'invalid') + expect(school.errors[:code]).to include('is invalid') + end - it "cannot change #code once it's been set" do - school.verify! - school.update(code: '00-00-00') - expect(school.errors[:code]).to include('cannot be changed after verification') + it "cannot change #code once it's been set" do + school.update(code: '00-00-00') + expect(school.errors[:code]).to include('cannot be changed after onboarding') + end end it 'requires a user_origin' do @@ -449,6 +457,105 @@ it 'sets the user_origin to for_education by default' do expect(school.user_origin).to eq('for_education') end + + describe 'duplicate detection' do + let(:existing_school) do + create(:school, + name: 'Riverside Academy', + municipality: 'Greenville', + postal_code: 'GV1 2XY', + administrative_area: 'Greenshire') + end + + before { existing_school } + + it 'allows schools with the same name in different countries' do + duplicate = build(:school, + name: existing_school.name, + municipality: existing_school.municipality, + postal_code: existing_school.postal_code, + administrative_area: existing_school.administrative_area, + country_code: 'US') + expect(duplicate).to be_valid + end + + it 'allows schools with the same name in different cities' do + duplicate = build(:school, + name: existing_school.name, + municipality: 'Bluetown', + postal_code: 'BT3 4ZW', + administrative_area: 'Blueshire') + expect(duplicate).to be_valid + end + + it 'allows schools with the same name but very different postal codes' do + duplicate = build(:school, + name: existing_school.name, + municipality: existing_school.municipality, + postal_code: 'NE1 7RU', + administrative_area: existing_school.administrative_area) + expect(duplicate).to be_valid + end + + it 'blocks duplicate with missing apostrophe in name' do + duplicate = build(:school, + name: 'Riverside Acadmy', + municipality: existing_school.municipality, + postal_code: existing_school.postal_code, + administrative_area: existing_school.administrative_area) + expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) + end + + it 'blocks duplicate with typo in name' do + duplicate = build(:school, + name: 'Riversid Academy', + municipality: existing_school.municipality, + postal_code: existing_school.postal_code, + administrative_area: existing_school.administrative_area) + expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) + end + + it 'blocks duplicate with similar municipality spelling' do + duplicate = build(:school, + name: existing_school.name, + municipality: 'Greenvile', + postal_code: existing_school.postal_code, + administrative_area: existing_school.administrative_area) + expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) + end + + it 'blocks duplicate with slightly different postal code spacing' do + duplicate = build(:school, + name: existing_school.name, + municipality: existing_school.municipality, + postal_code: 'GV12XY', + administrative_area: existing_school.administrative_area) + expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) + end + + it 'allows updating existing school without triggering duplicate detection' do + existing_school.update(address_line_1: Faker::Address.street_address) + expect(existing_school).to be_valid + end + + it 'allows school with very different name despite same location' do + duplicate = build(:school, + name: 'Completely Different Academy', + municipality: existing_school.municipality, + postal_code: existing_school.postal_code, + administrative_area: existing_school.administrative_area) + expect(duplicate).to be_valid + end + + it 'allows schools with similar but sufficiently different names in same location' do + duplicate = build(:school, + name: 'Riverside Institute', + municipality: existing_school.municipality, + postal_code: existing_school.postal_code, + administrative_area: existing_school.administrative_area) + expect(duplicate).to be_valid + end + end end describe '#creator' do @@ -520,34 +627,58 @@ expect(school.verified_at).to be_within(1.second).of(Time.zone.now) end - it 'uses the school code generator to generates and set the code' do + it 'returns true on successful verification' do + expect(school.verify!).to be(true) + end + + it 'raises ActiveRecord::RecordInvalid if verification fails' do + school.rejected_at = Time.zone.now + expect { school.verify! }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + describe 'code generation' do + around do |example| + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do + example.run + end + end + + it 'automatically generates a code after creation' do + new_school = described_class.create!(build(:school).attributes.except('id', 'created_at', 'updated_at')) + expect(new_school.reload.code).to be_present + end + + it 'uses the school code generator to generate the code' do allow(ForEducationCodeGenerator).to receive(:generate).and_return('00-00-00') - school.verify! - expect(school.code).to eq('00-00-00') + new_school = described_class.create!(build(:school).attributes.except('id', 'created_at', 'updated_at', 'code')) + expect(new_school.reload.code).to eq('00-00-00') end it 'retries 5 times if the school code is not unique' do - school.verify! - allow(ForEducationCodeGenerator).to receive(:generate).and_return(school.code, school.code, school.code, school.code, '00-00-00') - another_school = create(:school) - another_school.verify! - expect(another_school.code).to eq('00-00-00') + existing_school = school + allow(ForEducationCodeGenerator).to receive(:generate).and_return(existing_school.code, existing_school.code, existing_school.code, existing_school.code, '00-00-00') + another_school = described_class.create!(build(:school).attributes.except('id', 'created_at', 'updated_at', 'code')) + expect(another_school.reload.code).to eq('00-00-00') end it 'raises exception if unique code cannot be generated in 5 retries' do - school.verify! - allow(ForEducationCodeGenerator).to receive(:generate).and_return(school.code, school.code, school.code, school.code, school.code) - another_school = create(:school) - expect { another_school.verify! }.to raise_error(ActiveRecord::RecordInvalid) + existing_school = school + allow(ForEducationCodeGenerator).to receive(:generate).and_return(existing_school.code) + another_school_attrs = build(:school).attributes.except('id', 'created_at', 'updated_at', 'code') + expect { described_class.create!(another_school_attrs) }.to raise_error(ActiveRecord::RecordInvalid, /Code has already been taken/) end + end - it 'returns true on successful verification' do - expect(school.verify!).to be(true) - end + describe '#generate_code!' do + it 'does not regenerate the code once it has been set' do + allow(ForEducationCodeGenerator).to receive(:generate) - it 'raises ActiveRecord::RecordInvalid if verification fails' do - school.rejected_at = Time.zone.now - expect { school.verify! }.to raise_error(ActiveRecord::RecordInvalid) + school = create(:school) + existing_code = school.code + + expect { school.generate_code! }.not_to change(school, :code) + expect(school.code).to eq(existing_code) end end @@ -605,25 +736,21 @@ it 'returns true on successful rejection' do expect(school.reject).to be(true) end - - it 'returns false on unsuccessful rejection' do - school.verified_at = Time.zone.now - expect(school.reject).to be(false) - end end describe '#reopen' do it 'sets rejected_at to nil' do + school.reject school.reopen expect(school.rejected_at).to be_nil end it 'returns true on successful reopening' do + school.reject expect(school.reopen).to be(true) end - it 'returns false on unsuccessful reopening' do - school.verified_at = Time.zone.now + it 'returns false when trying to reopen a non-rejected school' do expect(school.reopen).to be(false) end end diff --git a/spec/services/school_onboarding_service_spec.rb b/spec/services/school_onboarding_service_spec.rb new file mode 100644 index 000000000..e00a34970 --- /dev/null +++ b/spec/services/school_onboarding_service_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe SchoolOnboardingService do + let(:token) { UserProfileMock::TOKEN } + let(:school) { create(:verified_school, creator_id: school_creator.id) } + let(:school_creator) { create(:user) } + let(:service) { described_class.new(school) } + + before do + allow(ProfileApiClient).to receive(:create_school) + end + + describe '#onboard' do + describe 'when onboarding is successful' do + it 'grants the creator the owner role for the school' do + service.onboard(token:) + expect(school_creator).to be_school_owner(school) + end + + it 'grants the creator the teacher role for the school' do + service.onboard(token:) + expect(school_creator).to be_school_teacher(school) + end + + it 'creates the school in Profile API' do + 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 + before do + allow(ProfileApiClient).to receive(:create_school).and_raise(RuntimeError) + end + + it 'does not create owner role' do + service.onboard(token:) + expect(school_creator).not_to be_school_owner(school) + end + + it 'does not create teacher role' do + service.onboard(token:) + expect(school_creator).not_to be_school_teacher(school) + end + + it 'returns false' do + expect(service.onboard(token:)).to be(false) + end + end + + describe 'when teacher and owner roles cannot be created because they already have a role in another school' do + let(:another_school) { create(:school) } + + before do + create(:role, user_id: school.creator_id, school: another_school) + end + + it 'does not create owner role' do + service.onboard(token:) + expect(school_creator).not_to be_school_owner(school) + end + + it 'does not create teacher role' do + 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:) + expect(ProfileApiClient).not_to have_received(:create_school) + end + + it 'returns false' do + expect(service.onboard(token:)).to be(false) + end + end + end +end diff --git a/spec/services/school_verification_service_spec.rb b/spec/services/school_verification_service_spec.rb index 28c4dd3ae..1f7fdee4d 100644 --- a/spec/services/school_verification_service_spec.rb +++ b/spec/services/school_verification_service_spec.rb @@ -5,50 +5,29 @@ RSpec.describe SchoolVerificationService do let(:website) { 'http://example.com' } let(:school) { build(:school, creator_id: school_creator.id, website:) } - let(:user) { create(:user) } let(:school_creator) { create(:user) } let(:service) { described_class.new(school) } - let(:organisation_id) { SecureRandom.uuid } - let(:token) { 'token' } - before do - allow(ProfileApiClient).to receive(:create_school) + around do |example| + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do + example.run + end end describe '#verify' do describe 'when school can be saved' do it 'saves the school' do - service.verify(token:) + service.verify expect(school).to be_persisted end it 'sets verified_at to a date' do - service.verify(token:) + service.verify expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone) end - it 'generates school code' do - service.verify(token:) - expect(school.reload.code).to be_present - end - - it 'grants the creator the owner role for the school' do - service.verify(token:) - expect(school_creator).to be_school_owner(school) - end - - it 'grants the creator the teacher role for the school' do - service.verify(token:) - expect(school_creator).to be_school_teacher(school) - end - - it 'creates the school in Profile API' do - service.verify(token:) - expect(ProfileApiClient).to have_received(:create_school).with(token:, id: school.id, code: school.code) - end - it 'returns true' do - expect(service.verify(token:)).to be(true) + expect(service.verify).to be(true) end end @@ -56,86 +35,12 @@ let(:website) { 'invalid' } it 'does not save the school' do - service.verify(token:) + service.verify expect(school).not_to be_persisted end - it 'does not create owner role' do - service.verify(token:) - expect(school_creator).not_to be_school_owner(school) - end - - it 'does not create teacher role' do - service.verify(token:) - expect(school_creator).not_to be_school_teacher(school) - end - - it 'does not create school in Profile API' do - expect(ProfileApiClient).not_to have_received(:create_school) - end - - it 'returns false' do - expect(service.verify(token:)).to be(false) - end - end - - describe 'when the school cannot be created in Profile API' do - before do - allow(ProfileApiClient).to receive(:create_school).and_raise(RuntimeError) - end - - it 'does not save the school' do - service.verify(token:) - expect { school.reload }.to raise_error(ActiveRecord::RecordNotFound) - end - - it 'does not create owner role' do - service.verify(token:) - expect(school_creator).not_to be_school_owner(school) - end - - it 'does not create teacher role' do - service.verify(token:) - expect(school_creator).not_to be_school_teacher(school) - end - - it 'does not create school in Profile API' do - expect(ProfileApiClient).not_to have_received(:create_school) - end - - it 'returns false' do - expect(service.verify(token:)).to be(false) - end - end - - describe 'when teacher and owner roles cannot be created because they already have a role in another school' do - let(:another_school) { create(:school) } - - before do - create(:role, user_id: school.creator_id, school: another_school) - end - - it 'does not save the school' do - service.verify(token:) - expect { school.reload }.to raise_error(ActiveRecord::RecordNotFound) - end - - it 'does not create owner role' do - service.verify(token:) - expect(school_creator).not_to be_school_owner(school) - end - - it 'does not create teacher role' do - service.verify(token:) - expect(school_creator).not_to be_school_teacher(school) - end - - it 'does not create school in Profile API' do - expect(ProfileApiClient).not_to have_received(:create_school) - end - it 'returns false' do - expect(service.verify(token:)).to be(false) + expect(service.verify).to be(false) end end end @@ -157,12 +62,12 @@ describe 'when the school was previously verified' do before do - service.verify(token:) + service.verify service.reject school.reload end - it 'does not reset verified_at' do + it 'does not clear verified_at' do expect(school.verified_at).to be_present end From cfd967997ef51923d51a2f163ee511e25e708981 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Fri, 19 Dec 2025 10:29:29 +0000 Subject: [PATCH 02/16] Use describe for rubocop --- spec/models/school_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index ec6e30011..3e8f4cfd5 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -423,7 +423,7 @@ expect(school.errors[:verified_at]).to include('cannot be changed after verification') end - context 'code validations' do + describe 'code validations' do around do |example| ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do example.run From e77228256f31c7dfb161e5cd6fe317a0f0ff1cbb Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Fri, 19 Dec 2025 10:53:10 +0000 Subject: [PATCH 03/16] 1069: Add error_type to duplication response --- app/controllers/api/schools_controller.rb | 2 +- lib/concepts/school/operations/create.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index 0fce16d39..922403d0b 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -24,7 +24,7 @@ def create else render json: { error: result[:error], - error_types: result[:error_types] + error_type: result[:error_type] || :unknown_error }, status: :unprocessable_entity end end diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 8fd531cca..77aaa4e70 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -16,6 +16,7 @@ def call(school_params:, creator_id:, token:) response rescue School::DuplicateSchoolError => e response[:error] = e.message + response[:error_type] = :duplication_error response rescue StandardError => e Sentry.capture_exception(e) From 40ff1009eef25f2e3bd03a167eaff0f80043cdb1 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Tue, 6 Jan 2026 14:59:40 +0000 Subject: [PATCH 04/16] 1069: Descoped duplication --- app/models/school.rb | 28 ------- db/schema.rb | 2 +- lib/concepts/school/operations/create.rb | 4 - lib/concepts/school/operations/update.rb | 4 - spec/models/school_spec.rb | 99 ------------------------ 5 files changed, 1 insertion(+), 136 deletions(-) diff --git a/app/models/school.rb b/app/models/school.rb index 33857cd7f..6672278f1 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class School < ApplicationRecord - class DuplicateSchoolError < StandardError; end - has_many :classes, class_name: :SchoolClass, inverse_of: :school, dependent: :destroy has_many :lessons, dependent: :nullify has_many :projects, dependent: :nullify @@ -51,7 +49,6 @@ class DuplicateSchoolError < StandardError; end before_validation :normalize_district_fields before_validation :normalize_school_roll_number - before_save :prevent_duplicate_school before_save :format_uk_postal_code, if: :should_format_uk_postal_code? after_commit :generate_code!, on: :create, if: -> { FeatureFlags.immediate_school_onboarding? } @@ -172,29 +169,4 @@ def format_uk_postal_code # ensures UK postcodes are always formatted correctly (as the inward code is always 3 chars long) self.postal_code = "#{cleaned_postal_code[0..-4]} #{cleaned_postal_code[-3..]}" end - - def prevent_duplicate_school - name_threshold = 0.6 - municipality_threshold = 0.7 - postal_code_threshold = 0.8 - max_edit_distance = 3 - - normalized_postal = postal_code.to_s.gsub(/\s+/, '') - - duplicate_school = School - .where.not(id:) - .where(country_code:) - .where( - 'similarity(lower(name), ?) >= ? AND levenshtein(lower(name), ?) <= ?', - name.to_s, name_threshold, name.to_s, max_edit_distance - ) - .where('similarity(lower(municipality), ?) >= ?', municipality.to_s, municipality_threshold) - .where( - "similarity(lower(replace(postal_code, ' ', '')), ?) >= ?", - normalized_postal, postal_code_threshold - ) - .first - - raise DuplicateSchoolError, I18n.t('validations.school.duplicate_school') if duplicate_school - end end diff --git a/db/schema.rb b/db/schema.rb index 447f05824..1c8004d31 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_12_08_134354) do +ActiveRecord::Schema[7.2].define(version: 2025_12_04_132605) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 77aaa4e70..5282249fd 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -13,10 +13,6 @@ def call(school_params:, creator_id:, token:) SchoolOnboardingService.new(response[:school]).onboard(token:) if FeatureFlags.immediate_school_onboarding? end - response - rescue School::DuplicateSchoolError => e - response[:error] = e.message - response[:error_type] = :duplication_error response rescue StandardError => e Sentry.capture_exception(e) diff --git a/lib/concepts/school/operations/update.rb b/lib/concepts/school/operations/update.rb index 4140483bb..439f30d64 100644 --- a/lib/concepts/school/operations/update.rb +++ b/lib/concepts/school/operations/update.rb @@ -9,10 +9,6 @@ def call(school:, school_params:) response[:school].assign_attributes(school_params) response[:school].save! response - rescue School::DuplicateSchoolError => e - Sentry.capture_exception(e) - response[:error] = e.message - response rescue StandardError => e Sentry.capture_exception(e) errors = response[:school].errors.full_messages.join(',') diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 3e8f4cfd5..9d7dda871 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -457,105 +457,6 @@ it 'sets the user_origin to for_education by default' do expect(school.user_origin).to eq('for_education') end - - describe 'duplicate detection' do - let(:existing_school) do - create(:school, - name: 'Riverside Academy', - municipality: 'Greenville', - postal_code: 'GV1 2XY', - administrative_area: 'Greenshire') - end - - before { existing_school } - - it 'allows schools with the same name in different countries' do - duplicate = build(:school, - name: existing_school.name, - municipality: existing_school.municipality, - postal_code: existing_school.postal_code, - administrative_area: existing_school.administrative_area, - country_code: 'US') - expect(duplicate).to be_valid - end - - it 'allows schools with the same name in different cities' do - duplicate = build(:school, - name: existing_school.name, - municipality: 'Bluetown', - postal_code: 'BT3 4ZW', - administrative_area: 'Blueshire') - expect(duplicate).to be_valid - end - - it 'allows schools with the same name but very different postal codes' do - duplicate = build(:school, - name: existing_school.name, - municipality: existing_school.municipality, - postal_code: 'NE1 7RU', - administrative_area: existing_school.administrative_area) - expect(duplicate).to be_valid - end - - it 'blocks duplicate with missing apostrophe in name' do - duplicate = build(:school, - name: 'Riverside Acadmy', - municipality: existing_school.municipality, - postal_code: existing_school.postal_code, - administrative_area: existing_school.administrative_area) - expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) - end - - it 'blocks duplicate with typo in name' do - duplicate = build(:school, - name: 'Riversid Academy', - municipality: existing_school.municipality, - postal_code: existing_school.postal_code, - administrative_area: existing_school.administrative_area) - expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) - end - - it 'blocks duplicate with similar municipality spelling' do - duplicate = build(:school, - name: existing_school.name, - municipality: 'Greenvile', - postal_code: existing_school.postal_code, - administrative_area: existing_school.administrative_area) - expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) - end - - it 'blocks duplicate with slightly different postal code spacing' do - duplicate = build(:school, - name: existing_school.name, - municipality: existing_school.municipality, - postal_code: 'GV12XY', - administrative_area: existing_school.administrative_area) - expect { duplicate.save! }.to raise_error(School::DuplicateSchoolError, I18n.t('validations.school.duplicate_school')) - end - - it 'allows updating existing school without triggering duplicate detection' do - existing_school.update(address_line_1: Faker::Address.street_address) - expect(existing_school).to be_valid - end - - it 'allows school with very different name despite same location' do - duplicate = build(:school, - name: 'Completely Different Academy', - municipality: existing_school.municipality, - postal_code: existing_school.postal_code, - administrative_area: existing_school.administrative_area) - expect(duplicate).to be_valid - end - - it 'allows schools with similar but sufficiently different names in same location' do - duplicate = build(:school, - name: 'Riverside Institute', - municipality: existing_school.municipality, - postal_code: existing_school.postal_code, - administrative_area: existing_school.administrative_area) - expect(duplicate).to be_valid - end - end end describe '#creator' do From 3d2c773f12bcb5492dfd5b56efd4e79b57456044 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Tue, 6 Jan 2026 16:10:07 +0000 Subject: [PATCH 05/16] 1069: Update flag usage --- app/models/school.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/school.rb b/app/models/school.rb index 6672278f1..9a96d6386 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -73,7 +73,7 @@ def rejected? end def verify! - generate_code! if ENV['ENABLE_IMMEDIATE_SCHOOL_ONBOARDING'].blank? + generate_code! unless FeatureFlags.immediate_school_onboarding? update!(verified_at: Time.zone.now) end From c3d60187cb4c185f296b11c943f46e9020ecbdea Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Tue, 6 Jan 2026 16:28:04 +0000 Subject: [PATCH 06/16] 1069: Ensure old coverage exists until feature flag removed --- .../school_verification_service_spec.rb | 185 +++++++++++++++--- 1 file changed, 163 insertions(+), 22 deletions(-) diff --git a/spec/services/school_verification_service_spec.rb b/spec/services/school_verification_service_spec.rb index 1f7fdee4d..6068f1589 100644 --- a/spec/services/school_verification_service_spec.rb +++ b/spec/services/school_verification_service_spec.rb @@ -8,39 +8,180 @@ let(:school_creator) { create(:user) } let(:service) { described_class.new(school) } - around do |example| - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - example.run - end - end - describe '#verify' do - describe 'when school can be saved' do - it 'saves the school' do - service.verify - expect(school).to be_persisted + 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 end - it 'sets verified_at to a date' do - service.verify - expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone) + describe 'when school can be saved' do + it 'saves the school' do + service.verify + expect(school).to be_persisted + end + + it 'sets verified_at to a date' do + service.verify + expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone) + end + + it 'returns true' do + expect(service.verify).to be(true) + end end - it 'returns true' do - expect(service.verify).to be(true) + describe 'when school cannot be saved' do + let(:website) { 'invalid' } + + it 'does not save the school' do + service.verify + expect(school).not_to be_persisted + end + + it 'returns false' do + expect(service.verify).to be(false) + end end end - describe 'when school cannot be saved' do - let(:website) { 'invalid' } + # TODO: Remove these examples once the feature flag is retired + describe 'when immediate onboarding is disabled' do + let(:token) { 'token' } - it 'does not save the school' do - service.verify - expect(school).not_to be_persisted + around do |example| + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: nil) do + example.run + end end - it 'returns false' do - expect(service.verify).to be(false) + before do + allow(ProfileApiClient).to receive(:create_school) + end + + describe 'when school can be saved' do + it 'saves the school' do + service.verify(token:) + expect(school).to be_persisted + end + + it 'sets verified_at to a date' do + service.verify(token:) + expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone) + end + + it 'generates school code' do + service.verify(token:) + expect(school.reload.code).to be_present + end + + it 'grants the creator the owner role for the school' do + service.verify(token:) + expect(school_creator).to be_school_owner(school) + end + + it 'grants the creator the teacher role for the school' do + service.verify(token:) + expect(school_creator).to be_school_teacher(school) + end + + it 'creates the school in Profile API' do + service.verify(token:) + expect(ProfileApiClient).to have_received(:create_school).with(token:, id: school.id, code: school.code) + end + + it 'returns true' do + expect(service.verify(token:)).to be(true) + end + end + + describe 'when school cannot be saved' do + let(:website) { 'invalid' } + + it 'does not save the school' do + service.verify(token:) + expect(school).not_to be_persisted + end + + it 'does not create owner role' do + service.verify(token:) + expect(school_creator).not_to be_school_owner(school) + end + + it 'does not create teacher role' do + service.verify(token:) + expect(school_creator).not_to be_school_teacher(school) + end + + it 'does not create school in Profile API' do + expect(ProfileApiClient).not_to have_received(:create_school) + end + + it 'returns false' do + expect(service.verify(token:)).to be(false) + end + end + + describe 'when the school cannot be created in Profile API' do + before do + allow(ProfileApiClient).to receive(:create_school).and_raise(RuntimeError) + end + + it 'does not save the school' do + service.verify(token:) + expect { school.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'does not create owner role' do + service.verify(token:) + expect(school_creator).not_to be_school_owner(school) + end + + it 'does not create teacher role' do + service.verify(token:) + expect(school_creator).not_to be_school_teacher(school) + end + + it 'does not create school in Profile API' do + expect(ProfileApiClient).not_to have_received(:create_school) + end + + it 'returns false' do + expect(service.verify(token:)).to be(false) + end + end + + describe 'when teacher and owner roles cannot be created because they already have a role in another school' do + let(:another_school) { create(:school) } + + before do + create(:role, user_id: school.creator_id, school: another_school) + end + + it 'does not save the school' do + service.verify(token:) + expect { school.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'does not create owner role' do + service.verify(token:) + expect(school_creator).not_to be_school_owner(school) + end + + it 'does not create teacher role' do + service.verify(token:) + expect(school_creator).not_to be_school_teacher(school) + end + + it 'does not create school in Profile API' do + expect(ProfileApiClient).not_to have_received(:create_school) + end + + it 'returns false' do + expect(service.verify(token:)).to be(false) + end end end end From e93a856b25d423553e8501d47219c4318c8f91a3 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Tue, 6 Jan 2026 16:28:24 +0000 Subject: [PATCH 07/16] 1069: Comment what needs to be removed once the flag is retired --- app/models/school.rb | 2 ++ app/services/school_verification_service.rb | 1 + lib/concepts/school/operations/create.rb | 1 + 3 files changed, 4 insertions(+) diff --git a/app/models/school.rb b/app/models/school.rb index 9a96d6386..e2c7f7ded 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -51,6 +51,7 @@ class School < ApplicationRecord before_save :format_uk_postal_code, if: :should_format_uk_postal_code? + # TODO: Remove the conditional once the feature flag is retired after_commit :generate_code!, on: :create, if: -> { FeatureFlags.immediate_school_onboarding? } def self.find_for_user!(user) @@ -73,6 +74,7 @@ def rejected? end def verify! + # TODO: Remove this line once the feature flag is retired generate_code! unless FeatureFlags.immediate_school_onboarding? update!(verified_at: Time.zone.now) diff --git a/app/services/school_verification_service.rb b/app/services/school_verification_service.rb index 6b78ee59d..68cceb5a0 100644 --- a/app/services/school_verification_service.rb +++ b/app/services/school_verification_service.rb @@ -11,6 +11,7 @@ def verify(token: nil) School.transaction do school.verify! + # TODO: Remove this line once the feature flag is retired SchoolOnboardingService.new(school).onboard(token: token) unless FeatureFlags.immediate_school_onboarding? end rescue StandardError => e diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 5282249fd..b582b8f21 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -10,6 +10,7 @@ def call(school_params:, creator_id:, token:) School.transaction do response[:school].save! + # TODO: Remove this conditional once the feature flag is retired SchoolOnboardingService.new(response[:school]).onboard(token:) if FeatureFlags.immediate_school_onboarding? end From 5ad7647111f11a0a274a33bf327021d31a598065 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Tue, 6 Jan 2026 17:04:30 +0000 Subject: [PATCH 08/16] 1069: Ensure we're rolling back based on the result of onboarding to mimic current behaviour --- app/services/school_verification_service.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/app/services/school_verification_service.rb b/app/services/school_verification_service.rb index 68cceb5a0..aaa3b7559 100644 --- a/app/services/school_verification_service.rb +++ b/app/services/school_verification_service.rb @@ -8,18 +8,23 @@ def initialize(school) end def verify(token: nil) + success = false School.transaction do school.verify! - # TODO: Remove this line once the feature flag is retired - SchoolOnboardingService.new(school).onboard(token: token) unless FeatureFlags.immediate_school_onboarding? + # TODO: Remove this line, once the feature flag is retired + success = FeatureFlags.immediate_school_onboarding? || SchoolOnboardingService.new(school).onboard(token: token) + + # TODO: Remove this line, once the feature flag is retired + raise ActiveRecord::Rollback unless success end rescue StandardError => e Sentry.capture_exception(e) Rails.logger.error { "Failed to verify school #{@school.id}: #{e.message}" } false else - true + # TODO: Return 'true', once the feature flag is retired + success end delegate :reject, to: :school From 9741bb7c61b54447c82120493533e8be6620a213 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Tue, 6 Jan 2026 17:30:02 +0000 Subject: [PATCH 09/16] 1069: Fix test failure --- spec/services/school_verification_service_spec.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/spec/services/school_verification_service_spec.rb b/spec/services/school_verification_service_spec.rb index 6068f1589..93ea0289b 100644 --- a/spec/services/school_verification_service_spec.rb +++ b/spec/services/school_verification_service_spec.rb @@ -7,6 +7,11 @@ let(:school) { build(:school, creator_id: school_creator.id, website:) } let(:school_creator) { create(:user) } let(:service) { described_class.new(school) } + let(:token) { 'token' } + + before do + allow(ProfileApiClient).to receive(:create_school) + end describe '#verify' do describe 'when immediate onboarding is enabled' do @@ -49,18 +54,12 @@ # TODO: Remove these examples once the feature flag is retired describe 'when immediate onboarding is disabled' do - let(:token) { 'token' } - around do |example| ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: nil) do example.run end end - before do - allow(ProfileApiClient).to receive(:create_school) - end - describe 'when school can be saved' do it 'saves the school' do service.verify(token:) From 12f5e0ad5d11164557132b22a8e713206d8b8e99 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 12 Jan 2026 09:24:17 +0000 Subject: [PATCH 10/16] Bring schema version up to date --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 1c8004d31..447f05824 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_12_04_132605) do +ActiveRecord::Schema[7.2].define(version: 2025_12_08_134354) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" From 35bcce7788c596803f0f30789a731e4ba72e1c7c Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 12 Jan 2026 09:30:11 +0000 Subject: [PATCH 11/16] Update schools_controller.rb --- app/controllers/api/schools_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index 922403d0b..0fce16d39 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -24,7 +24,7 @@ def create else render json: { error: result[:error], - error_type: result[:error_type] || :unknown_error + error_types: result[:error_types] }, status: :unprocessable_entity end end From afe2d6334133c1d050f2404f05752a9235a20b1e Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 12 Jan 2026 12:42:30 +0000 Subject: [PATCH 12/16] Create the code after_create to ensure it rolls back the transaction if the code fails This is really an edge case, but because of that in the unlikely case it happens we're better off erroring and fixing it. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- app/models/school.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/school.rb b/app/models/school.rb index e2c7f7ded..eef37c520 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -52,7 +52,7 @@ class School < ApplicationRecord before_save :format_uk_postal_code, if: :should_format_uk_postal_code? # TODO: Remove the conditional once the feature flag is retired - after_commit :generate_code!, on: :create, if: -> { FeatureFlags.immediate_school_onboarding? } + after_create :generate_code!, if: -> { FeatureFlags.immediate_school_onboarding? } def self.find_for_user!(user) school = Role.find_by(user_id: user.id)&.school || find_by(creator_id: user.id) From bf3c6b63ed48bb268d293c54aa0c5786361ee64e Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 12 Jan 2026 12:44:07 +0000 Subject: [PATCH 13/16] Ensure we're using the same country_code This was the result of a merge conflict, the original behaviour needs to be preserved Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- lib/tasks/seeds_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/seeds_helper.rb b/lib/tasks/seeds_helper.rb index 67058fc83..b73a8d3f5 100644 --- a/lib/tasks/seeds_helper.rb +++ b/lib/tasks/seeds_helper.rb @@ -23,7 +23,7 @@ def create_school(creator_id, school_id = nil) school.administrative_area = "#{Faker::Address.city}shire" school.municipality = Faker::Address.city school.postal_code = Faker::Address.postcode - school.country_code = Faker::Address.country_code + school.country_code = country_code school.creator_id = creator_id school.creator_agree_authority = true school.creator_agree_terms_and_conditions = true From 8978fe35be43b99e8d86ac5e7be428a9924610aa Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 12 Jan 2026 12:46:18 +0000 Subject: [PATCH 14/16] Ensure we raise if onboarding was to fail for any reason Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- lib/concepts/school/operations/create.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index b582b8f21..79c67eb51 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -11,7 +11,10 @@ def call(school_params:, creator_id:, token:) response[:school].save! # TODO: Remove this conditional once the feature flag is retired - SchoolOnboardingService.new(response[:school]).onboard(token:) if FeatureFlags.immediate_school_onboarding? + if FeatureFlags.immediate_school_onboarding? + onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:) + raise "School onboarding failed" unless onboarded + end end response From 8f4804caf3a443f304e8cd8fd3e1a8bc99b7b8b9 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 12 Jan 2026 14:01:57 +0000 Subject: [PATCH 15/16] 1069: Fix quotes --- lib/concepts/school/operations/create.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 79c67eb51..ac6cdf96c 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -13,7 +13,7 @@ def call(school_params:, creator_id:, token:) # 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 + raise 'School onboarding failed' unless onboarded end end From 96d14f8c60ba2db00b6d217f0cf78b3d94da8d7d Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 12 Jan 2026 14:17:02 +0000 Subject: [PATCH 16/16] 1069: Add coverage to ensure the SchoolOnboardingService is called / not called int he school concept depending on the feature flag --- spec/concepts/school/create_spec.rb | 40 +++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index 5d6d428b0..fd098540d 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -82,4 +82,44 @@ expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) end end + + 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 + end + + let(:onboarding_service) { instance_spy(SchoolOnboardingService, onboard: true) } + + before do + allow(SchoolOnboardingService).to receive(:new).and_return(onboarding_service) + end + + it 'calls the onboarding service' do + described_class.call(school_params:, creator_id:, token:) + expect(onboarding_service).to have_received(:onboard).with(token:) + end + end + + # TODO: Remove these examples once the feature flag is retired + describe 'when immediate onboarding is disabled' do + around do |example| + ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: nil) do + example.run + end + end + + let(:onboarding_service) { instance_spy(SchoolOnboardingService) } + + before do + allow(SchoolOnboardingService).to receive(:new).and_return(onboarding_service) + end + + it 'does not call the onboarding service' do + described_class.call(school_params:, creator_id:, token:) + expect(onboarding_service).not_to have_received(:onboard) + end + end end