diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index 422a47b70..3daab8de2 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -80,6 +80,7 @@ def school_params :reference, :district_name, :district_nces_id, + :school_roll_number, :address_line_1, :address_line_2, :municipality, diff --git a/app/dashboards/school_dashboard.rb b/app/dashboards/school_dashboard.rb index 8ee71136f..7a1be6ee1 100644 --- a/app/dashboards/school_dashboard.rb +++ b/app/dashboards/school_dashboard.rb @@ -28,6 +28,7 @@ class SchoolDashboard < Administrate::BaseDashboard reference: Field::String, district_name: Field::String, district_nces_id: Field::String, + school_roll_number: Field::String, verified_at: Field::DateTime, rejected_at: Field::DateTime, created_at: Field::DateTime, @@ -62,6 +63,7 @@ class SchoolDashboard < Administrate::BaseDashboard reference district_name district_nces_id + school_roll_number website address_line_1 address_line_2 @@ -86,6 +88,7 @@ class SchoolDashboard < Administrate::BaseDashboard reference district_name district_nces_id + school_roll_number website address_line_1 address_line_2 diff --git a/app/models/school.rb b/app/models/school.rb index a798b994c..4e9d35f60 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -18,6 +18,10 @@ class School < ApplicationRecord 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 :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') } 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 @@ -34,6 +38,7 @@ class School < ApplicationRecord before_validation :normalize_reference before_validation :normalize_district_fields + before_validation :normalize_school_roll_number before_save :format_uk_postal_code, if: :should_format_uk_postal_code? @@ -102,6 +107,12 @@ def normalize_district_fields self.district_nces_id = nil if district_nces_id.blank? end + # Ensure the school_roll_number is nil, not an empty string + # Also normalize to uppercase for consistent validation + def normalize_school_roll_number + self.school_roll_number = school_roll_number.blank? ? nil : school_roll_number.upcase + end + def verified_at_cannot_be_changed errors.add(:verified_at, 'cannot be changed after verification') if verified_at_was.present? && verified_at_changed? end @@ -118,6 +129,10 @@ def should_format_uk_postal_code? country_code == 'GB' && postal_code.to_s.length >= 5 end + def ireland? + country_code == 'IE' + end + def format_uk_postal_code cleaned_postal_code = postal_code.delete(' ') # insert a space as the third-from-last character in the postcode, eg. SW1A1AA -> SW1A 1AA diff --git a/app/views/api/schools/_school.json.jbuilder b/app/views/api/schools/_school.json.jbuilder index 74ea46775..a2ee65eff 100644 --- a/app/views/api/schools/_school.json.jbuilder +++ b/app/views/api/schools/_school.json.jbuilder @@ -8,6 +8,7 @@ json.call( :reference, :district_name, :district_nces_id, + :school_roll_number, :address_line_1, :address_line_2, :municipality, diff --git a/config/locales/en.yml b/config/locales/en.yml index df0bccc02..765ef5650 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -15,6 +15,7 @@ en: validations: school: website: "must be a valid URL" + school_roll_number: "must be numbers followed by letters (e.g., 01572D)" invitation: email_address: "'%s' is invalid" activerecord: diff --git a/db/migrate/20251128102719_add_school_roll_number_to_schools.rb b/db/migrate/20251128102719_add_school_roll_number_to_schools.rb new file mode 100644 index 000000000..dc0f5b7e4 --- /dev/null +++ b/db/migrate/20251128102719_add_school_roll_number_to_schools.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddSchoolRollNumberToSchools < ActiveRecord::Migration[7.2] + def change + add_column :schools, :school_roll_number, :string + + add_index :schools, :school_roll_number, unique: true + end +end diff --git a/db/migrate/20251204132605_change_school_roll_number_index_to_partial.rb b/db/migrate/20251204132605_change_school_roll_number_index_to_partial.rb new file mode 100644 index 000000000..704567d9a --- /dev/null +++ b/db/migrate/20251204132605_change_school_roll_number_index_to_partial.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class ChangeSchoolRollNumberIndexToPartial < ActiveRecord::Migration[7.2] + def change + remove_index :schools, :school_roll_number + add_index :schools, :school_roll_number, unique: true, where: 'rejected_at IS NULL' + end +end diff --git a/db/schema.rb b/db/schema.rb index 188db48cc..f851998e9 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_11_20_145032) 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" @@ -308,10 +308,12 @@ t.integer "user_origin", default: 0 t.string "district_name" t.string "district_nces_id" + 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 ["school_roll_number"], name: "index_schools_on_school_roll_number", unique: true, where: "(rejected_at IS NULL)" end create_table "teacher_invitations", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/spec/factories/school.rb b/spec/factories/school.rb index ce447c43b..64107ffcb 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' } + school_roll_number { nil } creator_id { SecureRandom.uuid } creator_agree_authority { true } creator_agree_terms_and_conditions { true } diff --git a/spec/features/my_school/showing_my_school_spec.rb b/spec/features/my_school/showing_my_school_spec.rb index f25d0ea5e..4818cd789 100644 --- a/spec/features/my_school/showing_my_school_spec.rb +++ b/spec/features/my_school/showing_my_school_spec.rb @@ -18,7 +18,7 @@ it "includes the school details and user's roles in the JSON" do school_json = school.to_json(only: %i[ - id name website reference address_line_1 address_line_2 municipality administrative_area postal_code country_code code verified_at created_at updated_at district_name district_nces_id + id name website reference address_line_1 address_line_2 municipality administrative_area postal_code country_code code verified_at created_at updated_at district_name district_nces_id school_roll_number ]) expected_data = JSON.parse(school_json, symbolize_names: true).merge(roles: ['owner'], import_in_progress: school.import_in_progress?) diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 85db38cf9..93ec09018 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -147,6 +147,77 @@ expect(duplicate_school).not_to be_valid end + it 'does not require a school_roll_number for non-Ireland schools' do + school.country_code = 'GB' + school.school_roll_number = nil + expect(school).to be_valid + 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") + 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') + 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 + end + + it 'accepts a school_roll_number with one or more letters' do + school.school_roll_number = '12345ABC' + expect(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)') + 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)') + 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)') + end + + it 'normalizes blank school_roll_number to nil' do + school.school_roll_number = ' ' + expect(school).to be_valid + expect(school.school_roll_number).to be_nil + end + + it 'normalizes school_roll_number to uppercase' do + school.school_roll_number = '01572d' + expect(school).to be_valid + expect(school.school_roll_number).to eq('01572D') + end + + it 'allows school_roll_number reuse when original school is rejected' do + school.school_roll_number = '01572D' + school.save! + school.reject + + new_school = build(:school, school_roll_number: '01572D') + expect(new_school).to be_valid + expect { new_school.save! }.not_to raise_error + end + it 'requires an address_line_1' do school.address_line_1 = ' ' expect(school).not_to be_valid