feat: add School Roll Number field for Ireland schools Add optional s… (#632)

Mohamed-RPF · web-flow · commit 076b0b8d7008 · 2025-12-11T17:03:11.000Z
Add School Roll Number field for Ireland schools Add optional school_roll_number field with alphanumeric validation, following the same pattern as UK URN and US district fields. Includes database migration, API endpoints, admin dashboard integration, and comprehensive test coverage. ## Status Closes: https://github.com/orgs/RaspberryPiFoundation/projects/51/views/11?pane=issue&itemId=132315256&issue=RaspberryPiFoundation%7Cdigital-editor-issues%7C925 Related to: https://github.com/RaspberryPiFoundation/editor-standalone/pull/686 ## Points for consideration: ### Security - Field has unique index to prevent duplicate entries - Input validation enforces alphanumeric format (numbers followed by letters) - Case-insensitive uniqueness check prevents bypass attempts ### Performance - Database index added on `school_roll_number` column for efficient lookups - Optional field (nullable) - no impact on existing schools ## What's changed? **Database:** - Added `school_roll_number` string column to `schools` table - Added unique index on `school_roll_number` **Model (School):** - Added validation: uniqueness (case-insensitive), alphanumeric format - Added normalisation callback to handle blank values - Added I18n error message for validation **API:** - Added `school_roll_number` to permitted parameters in SchoolsController - Included `school_roll_number` in JSON API responses **Admin Dashboard:** - Added field to admin show page, form, and attributes list - Admins can view School Roll Numbers **Tests:** - 8 comprehensive validation tests (optional, uniqueness, format validation, normalisation) - Updated factory to support the new field **Locales:** - Added validation message to `config/locales/en.yml`
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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: "'%<value>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
@@ -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
@@ -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
diff --git 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
@@ -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
@@ -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
