diff --git a/Gemfile.lock b/Gemfile.lock index 5f754041a..2b8327168 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -558,6 +558,7 @@ GEM PLATFORMS aarch64-linux arm64-darwin-24 + arm64-darwin-25 x86_64-linux DEPENDENCIES diff --git a/app/jobs/school_import_job.rb b/app/jobs/school_import_job.rb index 85a54823e..19e4aee1b 100644 --- a/app/jobs/school_import_job.rb +++ b/app/jobs/school_import_job.rb @@ -128,7 +128,10 @@ def build_school_params(school_data) creator_agree_authority: true, creator_agree_terms_and_conditions: true, creator_agree_responsible_safeguarding: true, - user_origin: 'experience_cs' + user_origin: 'experience_cs', + district_name: school_data[:district_name], + district_nces_id: school_data[:district_nces_id], + school_roll_number: school_data[:school_roll_number] }.compact end diff --git a/app/models/school.rb b/app/models/school.rb index 4e9d35f60..831b9f655 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -16,12 +16,21 @@ class School < ApplicationRecord validates :address_line_1, presence: true validates :municipality, presence: true validates :country_code, presence: true, inclusion: { in: ISO3166::Country.codes } - validates :reference, uniqueness: { case_sensitive: false, allow_nil: true }, presence: false - validates :district_nces_id, uniqueness: { case_sensitive: false, allow_nil: true }, presence: false + validates :reference, + uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true }, + format: { with: /\A\d{5,6}\z/, allow_nil: true, message: I18n.t('validations.school.reference') }, + if: :united_kingdom? + validates :district_nces_id, + uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true }, + format: { with: /\A\d{12}\z/, allow_nil: true, message: I18n.t('validations.school.district_nces_id') }, + presence: true, + if: :united_states? + validates :district_name, presence: true, if: :united_states? validates :school_roll_number, - uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_nil: true }, - presence: { if: :ireland? }, - format: { with: /\A[0-9]+[A-Z]+\z/, allow_nil: true, message: I18n.t('validations.school.school_roll_number') } + uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true }, + format: { with: /\A[0-9]+[A-Z]+\z/, allow_nil: true, message: I18n.t('validations.school.school_roll_number') }, + presence: true, + if: :ireland? validates :creator_id, presence: true, uniqueness: true validates :creator_agree_authority, presence: true, acceptance: true validates :creator_agree_terms_and_conditions, presence: true, acceptance: true @@ -129,6 +138,14 @@ def should_format_uk_postal_code? country_code == 'GB' && postal_code.to_s.length >= 5 end + def united_kingdom? + country_code == 'GB' + end + + def united_states? + country_code == 'US' + end + def ireland? country_code == 'IE' end diff --git a/config/locales/en.yml b/config/locales/en.yml index 765ef5650..7811f7084 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -15,6 +15,8 @@ en: validations: school: website: "must be a valid URL" + reference: "must be 5-6 digits (e.g., 100000)" + district_nces_id: "must be 12 digits (e.g., 010000000001)" school_roll_number: "must be numbers followed by letters (e.g., 01572D)" invitation: email_address: "'%s' is invalid" diff --git a/db/migrate/20251208134354_change_reference_and_nces_id_indexes_to_partial.rb b/db/migrate/20251208134354_change_reference_and_nces_id_indexes_to_partial.rb new file mode 100644 index 000000000..88e2987c4 --- /dev/null +++ b/db/migrate/20251208134354_change_reference_and_nces_id_indexes_to_partial.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class ChangeReferenceAndNcesIdIndexesToPartial < ActiveRecord::Migration[7.2] + def change + # Convert reference (UK URN) index to partial index + # This allows rejected schools to release their URN for reuse + remove_index :schools, :reference + add_index :schools, :reference, unique: true, where: 'rejected_at IS NULL' + + # Convert district_nces_id (US NCES ID) index to partial index + # This allows rejected schools to release their NCES ID for reuse + remove_index :schools, :district_nces_id + add_index :schools, :district_nces_id, unique: true, where: 'rejected_at IS NULL' + end +end + diff --git a/db/schema.rb b/db/schema.rb index f851998e9..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" @@ -311,8 +311,8 @@ t.string "school_roll_number" t.index ["code"], name: "index_schools_on_code", unique: true t.index ["creator_id"], name: "index_schools_on_creator_id", unique: true - t.index ["district_nces_id"], name: "index_schools_on_district_nces_id", unique: true - t.index ["reference"], name: "index_schools_on_reference", unique: true + t.index ["district_nces_id"], name: "index_schools_on_district_nces_id", unique: true, where: "(rejected_at IS NULL)" + t.index ["reference"], name: "index_schools_on_reference", unique: true, where: "(rejected_at IS NULL)" t.index ["school_roll_number"], name: "index_schools_on_school_roll_number", unique: true, where: "(rejected_at IS NULL)" end diff --git a/lib/tasks/seeds_helper.rb b/lib/tasks/seeds_helper.rb index 2955e6590..b834b0a15 100644 --- a/lib/tasks/seeds_helper.rb +++ b/lib/tasks/seeds_helper.rb @@ -16,15 +16,20 @@ module SeedsHelper def create_school(creator_id, school_id = nil) School.find_or_create_by!(creator_id:, id: school_id) do |school| Rails.logger.info 'Seeding a school...' + country_code = Faker::Address.country_code school.name = Faker::Educator.secondary_school school.website = Faker::Internet.url(scheme: 'https') school.address_line_1 = Faker::Address.street_address school.municipality = Faker::Address.city - 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 school.creator_agree_to_ux_contact = true + # Country-specific required fields + school.reference = format('%06d', rand(100_000..999_999)) if country_code == 'GB' + school.district_nces_id = format('%012d', rand(10**12)) if country_code == 'US' + school.school_roll_number = "#{rand(10_000..99_999)}#{('A'..'Z').to_a.sample}" if country_code == 'IE' end end diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index 26a5226dc..f8375c7d9 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -10,6 +10,7 @@ address_line_1: 'Address Line 1', municipality: 'Greater London', country_code: 'GB', + reference: '100000', creator_agree_authority: true, creator_agree_terms_and_conditions: true, creator_agree_to_ux_contact: true, diff --git a/spec/factories/school.rb b/spec/factories/school.rb index 64107ffcb..d9bdaf6b6 100644 --- a/spec/factories/school.rb +++ b/spec/factories/school.rb @@ -7,6 +7,7 @@ address_line_1 { 'Address Line 1' } municipality { 'Greater London' } country_code { 'GB' } + sequence(:reference) { |n| format('%06d', 100_000 + n) } school_roll_number { nil } creator_id { SecureRandom.uuid } creator_agree_authority { true } diff --git a/spec/features/admin/schools_spec.rb b/spec/features/admin/schools_spec.rb index bde35d5ba..acba1d55c 100644 --- a/spec/features/admin/schools_spec.rb +++ b/spec/features/admin/schools_spec.rb @@ -79,7 +79,7 @@ describe 'when the school is in the United States and has a postal code' do before do - school.update(country_code: 'US', postal_code: '90210') + school.update(country_code: 'US', postal_code: '90210', district_nces_id: '010000000001', reference: nil) get admin_school_path(school) end diff --git a/spec/features/school/creating_a_school_spec.rb b/spec/features/school/creating_a_school_spec.rb index fd51863ed..aafa2f388 100644 --- a/spec/features/school/creating_a_school_spec.rb +++ b/spec/features/school/creating_a_school_spec.rb @@ -19,6 +19,7 @@ address_line_1: 'Address Line 1', municipality: 'Greater London', country_code: 'GB', + reference: '100000', creator_agree_authority: true, creator_agree_terms_and_conditions: true, creator_agree_to_ux_contact: true, diff --git a/spec/jobs/school_import_job_spec.rb b/spec/jobs/school_import_job_spec.rb index a66e073af..a783da2ba 100644 --- a/spec/jobs/school_import_job_spec.rb +++ b/spec/jobs/school_import_job_spec.rb @@ -17,7 +17,8 @@ address_line_1: '123 Main St', municipality: 'Springfield', country_code: 'US', - owner_email: 'owner1@example.com' + owner_email: 'owner1@example.com', + district_nces_id: '010000000001' }, { name: 'Test School 2', diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 93ec09018..374d83634 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -6,6 +6,8 @@ let(:student) { create(:student, school:) } let(:teacher) { create(:teacher, school:) } let(:school) { create(:school, creator_id: SecureRandom.uuid) } + 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) } describe 'associations' do it 'has many classes' do @@ -132,21 +134,131 @@ expect(school).to be_valid end - it 'does not require a reference' do - create(:school, id: SecureRandom.uuid, reference: nil) + it 'does not require a reference for non-UK schools' do + school.country_code = 'DE' + school.reference = nil + expect(school).to be_valid + end + it 'does not requires reference for UK schools' do + school.country_code = 'GB' school.reference = nil expect(school).to be_valid end it 'requires references to be unique if provided' do + school.reference = '100000' + school.save! + + duplicate_school = build(:school, reference: '100000') + expect(duplicate_school).not_to be_valid + end + + it 'accepts a valid reference format (5-6 digits)' do + school.reference = '100000' + expect(school).to be_valid + end + + it 'accepts a 5-digit reference' do + school.reference = '20000' + expect(school).to be_valid + end + + it 'rejects a reference with non-digit characters' do 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' + 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' + expect(school).not_to be_valid + expect(school.errors[:reference]).to include('must be 5-6 digits (e.g., 100000)') + end + + it 'allows reference reuse when original school is rejected' do + school.reference = '100000' school.save! + school.reject + + new_school = build(:school, reference: '100000') + expect(new_school).to be_valid + expect { new_school.save! }.not_to raise_error + end + + it 'does not require a district_nces_id for UK schools' do + school.country_code = 'GB' + school.district_nces_id = nil + expect(school).to be_valid + end + + it 'does not require a district_nces_id for CA schools' do + school.country_code = 'CA' + school.district_name = 'Some District' + school.district_nces_id = nil + expect(school).to be_valid + end + + it 'requires district_nces_id for US schools' do + us_school.district_nces_id = nil + expect(us_school).not_to be_valid + expect(us_school.errors[:district_nces_id]).to include("can't be blank") + end + + it 'requires district_name for US schools' do + us_school.district_name = nil + expect(us_school).not_to be_valid + expect(us_school.errors[:district_name]).to include("can't be blank") + end + + it 'does not require district_name for non-US schools' do + school.district_name = nil + expect(school).to be_valid + end - duplicate_school = build(:school, reference: 'urn-123') + it 'does not require district_name for CA schools' do + school.country_code = 'CA' + school.district_name = nil + expect(school).to be_valid + end + + it 'requires district_nces_id to be unique if provided' do + duplicate_school = build(:school, country_code: 'US', district_nces_id: '010000000001') expect(duplicate_school).not_to be_valid end + it 'accepts a valid district_nces_id format (12 digits)' do + us_school.district_nces_id = '010000000001' + expect(us_school).to be_valid + end + + it 'rejects a district_nces_id with non-digit characters' do + us_school.district_nces_id = '01000000000A' + expect(us_school).not_to be_valid + expect(us_school.errors[:district_nces_id]).to include('must be 12 digits (e.g., 010000000001)') + end + + it 'rejects a district_nces_id with wrong length' do + us_school.district_nces_id = '12345678901' + expect(us_school).not_to be_valid + expect(us_school.errors[:district_nces_id]).to include('must be 12 digits (e.g., 010000000001)') + end + + it 'allows district_nces_id reuse when original school is rejected' do + us_school.district_nces_id = '010000000001' + us_school.reject + + new_school = build(:school, district_nces_id: '010000000001') + expect(new_school).to be_valid + expect { new_school.save! }.not_to raise_error + end + it 'does not require a school_roll_number for non-Ireland schools' do school.country_code = 'GB' school.school_roll_number = nil @@ -154,46 +266,42 @@ end it 'requires school_roll_number for Ireland schools' do - school.country_code = 'IE' - school.school_roll_number = nil - expect(school).not_to be_valid - expect(school.errors[:school_roll_number]).to include("can't be blank") + ireland_school.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 it 'requires school_roll_number to be unique if provided' do - school.school_roll_number = '01572D' - school.save! - - duplicate_school = build(:school, school_roll_number: '01572d') + duplicate_school = build(:school, school_roll_number: '01572D', country_code: 'IE') expect(duplicate_school).not_to be_valid end it 'accepts a valid alphanumeric school_roll_number' do - school.school_roll_number = '01572D' - expect(school).to be_valid + ireland_school.school_roll_number = '01572D' + expect(ireland_school).to be_valid end it 'accepts a school_roll_number with one or more letters' do - school.school_roll_number = '12345ABC' - expect(school).to be_valid + ireland_school.school_roll_number = '12345ABC' + expect(ireland_school).to be_valid end it 'rejects a school_roll_number with only numbers' do - school.school_roll_number = '01572' - expect(school).not_to be_valid - expect(school.errors[:school_roll_number]).to include('must be numbers followed by letters (e.g., 01572D)') + ireland_school.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 - school.school_roll_number = 'ABCDE' - expect(school).not_to be_valid - expect(school.errors[:school_roll_number]).to include('must be numbers followed by letters (e.g., 01572D)') + ireland_school.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 - school.school_roll_number = '01572-D' - expect(school).not_to be_valid - expect(school.errors[:school_roll_number]).to include('must be numbers followed by letters (e.g., 01572D)') + ireland_school.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 it 'normalizes blank school_roll_number to nil' do @@ -209,11 +317,9 @@ end it 'allows school_roll_number reuse when original school is rejected' do - school.school_roll_number = '01572D' - school.save! - school.reject + ireland_school.reject - new_school = build(:school, school_roll_number: '01572D') + new_school = build(:school, school_roll_number: '01572D', country_code: 'IE') expect(new_school).to be_valid expect { new_school.save! }.not_to raise_error end