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

Author: zetter-rpf

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

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