From 4741c4fcdf001966edc2d922725239209d64b99e Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 12 Jun 2026 12:18:56 +0100 Subject: [PATCH] Fix invalid schools in our database We've added validation that makes existing schools we already have in our database. This means that that updates to those schools may fail and cause errors. Generally, when we add validation we should make sure it doesn't invalidate existing records. Here I've done two things: + Add `on: :create` to the new validations where schools have some invalid records + Add `unless: :rejected` to items with a unique condition where new signups may overlap with rejected schools. I've documented the errors in this sheet[1]. Some of the optional invalid codes could be fixed manually by nulling them if they aren't required, but since that can't be done for the required codes, I've handled them all the same way. https://docs.google.com/spreadsheets/d/1ZwkCrRkVeUcgUaDuDxSZljlhntmK1Az8J5--Y0bSBrI/edit?gid=0#gid=0 --- app/models/school.rb | 11 ++++----- spec/models/school_spec.rb | 47 ++++++++++++++++++++++---------------- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/app/models/school.rb b/app/models/school.rb index 0a413cc88..61b8a07f1 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -16,27 +16,26 @@ 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 :administrative_area, presence: true, on: :create 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') }, format: { with: /\A\d{5,6}\z/, allow_nil: true, message: I18n.t('validations.school.reference') }, - if: :united_kingdom? + if: :united_kingdom?, on: :create, unless: :rejected? validates :district_nces_id, format: { with: /\A\d{7}\z/, allow_nil: true, message: I18n.t('validations.school.district_nces_id') }, - if: :united_states? + if: :united_states?, on: :create validates :district_name, presence: true, if: :united_states? validates :school_roll_number, uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true, message: I18n.t('validations.school.school_roll_number_exists') }, format: { with: /\A[0-9]+[A-Z]+\z/, allow_nil: true, message: I18n.t('validations.school.school_roll_number') }, - presence: true, - if: :ireland? + presence: true, on: :create, if: :ireland?, unless: :rejected? validates :creator_id, presence: true, uniqueness: { conditions: -> { where(rejected_at: nil) } - } + }, unless: :rejected? validates :creator_agree_authority, presence: true, acceptance: true validates :creator_agree_terms_and_conditions, presence: true, acceptance: true validates :creator_agree_responsible_safeguarding, presence: true, acceptance: true diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 5074008bf..3ef01a14a 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -135,6 +135,13 @@ expect(another_school.errors[:creator_id]).to include('has already been taken') end + it 'schools can re-use creator_ids if the original school is rejected' do + rejected_school = create(:school, creator_id: SecureRandom.uuid, rejected_at: Time.zone.now) + other_school = build(:school, creator_id: rejected_school.creator_id) + expect(rejected_school).to be_valid + expect(other_school).to be_valid + end + it 'rejects a badly formed url for website' do school.website = 'http://.example.com' expect(school).not_to be_valid @@ -184,20 +191,20 @@ expect(school).to be_valid end - it 'rejects a reference with non-digit characters' do - school.reference = 'URN-123' + it 'rejects new schools with a reference containing non-digit characters' do + school = build(:school, reference: 'URN-123') expect(school).not_to be_valid expect(school.errors[:reference]).to include('must be 5-6 digits (e.g., 100000)') end - it 'rejects a reference with too few digits' do - school.reference = '1234' + it 'rejects new schools with a reference that has too few digits' do + school = build(:school, reference: '1234') expect(school).not_to be_valid expect(school.errors[:reference]).to include('must be 5-6 digits (e.g., 100000)') end - it 'rejects a reference with too many digits' do - school.reference = '1234567' + it 'rejects new schools with a reference that has too many digits' do + school = build(:school, reference: '1234567') expect(school).not_to be_valid expect(school.errors[:reference]).to include('must be 5-6 digits (e.g., 100000)') end @@ -255,14 +262,14 @@ expect(us_school).to be_valid end - it 'rejects a district_nces_id with non-digit characters' do - us_school.district_nces_id = '010000A' + it 'rejects new schools with a district_nces_id containing non-digit characters' do + us_school = build(:school, country_code: 'US', district_nces_id: '010000A') expect(us_school).not_to be_valid expect(us_school.errors[:district_nces_id]).to include('must be 7 digits (e.g., 0100000)') end - it 'rejects a district_nces_id with wrong length' do - us_school.district_nces_id = '123456' + it 'rejects new schools with a district_nces_id with wrong length' do + us_school = build(:school, country_code: 'US', district_nces_id: '123456') expect(us_school).not_to be_valid expect(us_school.errors[:district_nces_id]).to include('must be 7 digits (e.g., 0100000)') end @@ -281,8 +288,8 @@ expect(school).to be_valid end - it 'requires school_roll_number for Ireland schools' do - ireland_school.school_roll_number = nil + it 'requires school_roll_number for Ireland for new schools' do + ireland_school = build(:school, country_code: 'IE', school_roll_number: nil) expect(ireland_school).not_to be_valid expect(ireland_school.errors[:school_roll_number]).to include("can't be blank") end @@ -308,20 +315,20 @@ expect(ireland_school).to be_valid end - it 'rejects a school_roll_number with only numbers' do - ireland_school.school_roll_number = '01572' + it 'rejects new schools with a school_roll_number that contains only numbers' do + ireland_school = build(:school, country_code: 'IE', school_roll_number: '01572') expect(ireland_school).not_to be_valid expect(ireland_school.errors[:school_roll_number]).to include('must be numbers followed by letters (e.g., 01572D)') end - it 'rejects a school_roll_number with only letters' do - ireland_school.school_roll_number = 'ABCDE' + it 'rejects new schools with a school_roll_number containing only letters' do + ireland_school = build(:school, country_code: 'IE', school_roll_number: 'ABCDE') expect(ireland_school).not_to be_valid expect(ireland_school.errors[:school_roll_number]).to include('must be numbers followed by letters (e.g., 01572D)') end - it 'rejects a school_roll_number with special characters' do - ireland_school.school_roll_number = '01572-D' + it 'rejects new schools with a school_roll_number containing special characters' do + ireland_school = build(:school, country_code: 'IE', school_roll_number: '01572-D') expect(ireland_school).not_to be_valid expect(ireland_school.errors[:school_roll_number]).to include('must be numbers followed by letters (e.g., 01572D)') end @@ -356,8 +363,8 @@ expect(school).not_to be_valid end - it 'requires an administrative_area' do - school.administrative_area = ' ' + it 'requires an administrative_area for new schools' do + school = build(:school, administrative_area: ' ') expect(school).not_to be_valid end