From e6122c53832f93933dd6c89f57ebe766c652ca16 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 3 Jul 2026 16:03:08 +0100 Subject: [PATCH 1/4] Allow schools to be archived Many school owners contact us because they have created a school that is not being used. Because they are an owner/teacher on this school it prevents them from joining a new school. Add the ability for admins to archive a school. I considered using the existing rejected_at flag but this was modellling a different that we want to keep historical data for. Instead I'll add a 'hidden?' method to group this behaviour together. --- app/controllers/admin/schools_controller.rb | 6 +++++ app/dashboards/school_dashboard.rb | 2 ++ app/models/school.rb | 8 +++++++ app/views/admin/schools/show.html.erb | 11 +++++++++ config/locales/admin/en.yml | 5 ++++ config/routes.rb | 1 + ...260703145125_add_archived_at_to_schools.rb | 5 ++++ db/schema.rb | 3 ++- spec/features/admin/schools_spec.rb | 23 +++++++++++++++++++ 9 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260703145125_add_archived_at_to_schools.rb diff --git a/app/controllers/admin/schools_controller.rb b/app/controllers/admin/schools_controller.rb index eebc90f59..fe298b851 100644 --- a/app/controllers/admin/schools_controller.rb +++ b/app/controllers/admin/schools_controller.rb @@ -28,6 +28,12 @@ def reopen redirect_to admin_school_path(requested_resource) end + def archive + requested_resource.archive! + + redirect_to admin_school_path(requested_resource) + end + def default_sorting_attribute :created_at end diff --git a/app/dashboards/school_dashboard.rb b/app/dashboards/school_dashboard.rb index 213c8da35..fb3dfb013 100644 --- a/app/dashboards/school_dashboard.rb +++ b/app/dashboards/school_dashboard.rb @@ -33,6 +33,7 @@ class SchoolDashboard < Administrate::BaseDashboard school_roll_number: Field::String, verified_at: Field::DateTime, rejected_at: Field::DateTime, + archived_at: Field::DateTime, created_at: Field::DateTime, updated_at: Field::DateTime, user_origin: EnumField @@ -84,6 +85,7 @@ class SchoolDashboard < Administrate::BaseDashboard updated_at verified_at rejected_at +archived_at ].freeze # FORM_ATTRIBUTES diff --git a/app/models/school.rb b/app/models/school.rb index 61b8a07f1..ce9ef7cba 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -103,6 +103,14 @@ def reopen update(rejected_at: nil) end + def archive! + update!(archived_at: Time.zone.now, verified_at: nil) + end + + def archived? + archived_at.present? + end + def postal_code=(str) super(str.to_s.upcase) end diff --git a/app/views/admin/schools/show.html.erb b/app/views/admin/schools/show.html.erb index 02b182c44..68dac0726 100644 --- a/app/views/admin/schools/show.html.erb +++ b/app/views/admin/schools/show.html.erb @@ -45,6 +45,17 @@ as well as a link to its edit page. style: "background-color: darkorange; margin: 5px;", data: { confirm: t("administrate.actions.confirm_reopen_school") } ) unless page.resource.verified? || !page.resource.rejected? %> + + <%= button_to( + t("administrate.actions.archive_school"), + archive_admin_school_path(page.resource), + class: "button button--archive", + method: :patch, + style: "background-color: firebrick; margin: 5px;", + form: { + onsubmit: "return confirm('#{j(t("administrate.actions.confirm_archive_school"))}')" + } + ) unless page.resource.archived? %> diff --git a/config/locales/admin/en.yml b/config/locales/admin/en.yml index 36bba6442..726eb40f6 100644 --- a/config/locales/admin/en.yml +++ b/config/locales/admin/en.yml @@ -8,6 +8,8 @@ en: confirm_verify_school: Are you sure you want to verify this school? reopen_school: Reopen School confirm_reopen_school: Are you sure you want to reopen this school? + archive_school: Archive School + confirm_archive_school: Are you sure you want to archive this school? controller: verify_school: success: "Successfully verified." @@ -15,6 +17,9 @@ en: reopen_school: success: "Successfully reopened." error: "There was an error reopening the school" + archive_school: + success: "Successfully archived." + error: "There was an error archiving the school" activerecord: attributes: school: diff --git a/config/routes.rb b/config/routes.rb index 581c92710..a59a64057 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -16,6 +16,7 @@ member do post :verify patch :reopen + patch :archive end end diff --git a/db/migrate/20260703145125_add_archived_at_to_schools.rb b/db/migrate/20260703145125_add_archived_at_to_schools.rb new file mode 100644 index 000000000..b8554e4e3 --- /dev/null +++ b/db/migrate/20260703145125_add_archived_at_to_schools.rb @@ -0,0 +1,5 @@ +class AddArchivedAtToSchools < ActiveRecord::Migration[8.1] + def change + add_column :schools, :archived_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 952226723..a8f8a94bf 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[8.1].define(version: 2026_06_12_144637) do +ActiveRecord::Schema[8.1].define(version: 2026_07_03_145125) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -324,6 +324,7 @@ t.string "address_line_1", null: false t.string "address_line_2" t.string "administrative_area" + t.datetime "archived_at" t.string "code" t.string "country_code", null: false t.datetime "created_at", null: false diff --git a/spec/features/admin/schools_spec.rb b/spec/features/admin/schools_spec.rb index b5131e35d..be402ad04 100644 --- a/spec/features/admin/schools_spec.rb +++ b/spec/features/admin/schools_spec.rb @@ -36,6 +36,10 @@ expect(response.body).to include(I18n.t('administrate.actions.verify_school')) end + it 'includes link to archive school' do + expect(response.body).to include(I18n.t('administrate.actions.archive_school')) + end + it 'does not include a link to search for this school by its ZIP code in the NCES public schools database' do expect(response.body).not_to include('Search for this school in the NCES database') end @@ -201,6 +205,25 @@ end end + describe 'PUT #archive' do + let(:creator) { create(:user) } + let(:school) { create(:school, creator_id: creator.id) } + + before do + stub_user_info_api_for(creator) + end + + it 'marks the school as archived' do + patch archive_admin_school_path(school) + expect(school.reload).to be_archived + end + + it 'redirects to school path' do + patch archive_admin_school_path(school) + expect(response).to redirect_to(admin_school_path(school)) + end + end + private def sign_in_as(user) From 85aa0da6a25c82b6dd92cb16a4b7cac2e10ec333 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 3 Jul 2026 16:10:00 +0100 Subject: [PATCH 2/4] Update validation to allow for archived schools --- app/models/school.rb | 20 ++++++++++------ ...03150521_update_school_index_conditions.rb | 12 ++++++++++ db/schema.rb | 8 +++---- spec/models/school_spec.rb | 24 +++++++++++++++++++ 4 files changed, 53 insertions(+), 11 deletions(-) create mode 100644 db/migrate/20260703150521_update_school_index_conditions.rb diff --git a/app/models/school.rb b/app/models/school.rb index ce9ef7cba..d5e2eaf99 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -12,6 +12,8 @@ class School < ApplicationRecord enum :user_origin, { for_education: 0, experience_cs: 1 }, default: :for_education, validate: true + scope :active, -> { where(rejected_at: nil, archived_at: nil) } + validates :name, presence: true validates :website, presence: true, format: { with: VALID_URL_REGEX, message: I18n.t('validations.school.website') } validates :address_line_1, presence: true @@ -20,22 +22,22 @@ class School < ApplicationRecord 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') }, + uniqueness: { conditions: -> { active }, 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?, on: :create, unless: :rejected? + if: :united_kingdom?, on: :create, unless: :hidden? validates :district_nces_id, format: { with: /\A\d{7}\z/, allow_nil: true, message: I18n.t('validations.school.district_nces_id') }, 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') }, + uniqueness: { conditions: -> { active }, 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, on: :create, if: :ireland?, unless: :rejected? + presence: true, on: :create, if: :ireland?, unless: :hidden? validates :creator_id, presence: true, uniqueness: { - conditions: -> { where(rejected_at: nil) } - }, unless: :rejected? + conditions: -> { active } + }, unless: :hidden? 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 @@ -59,7 +61,7 @@ class School < ApplicationRecord after_commit -> { do_salesforce_sync(is_create: false) }, on: :update, if: -> { FeatureFlags.salesforce_sync? } def self.find_for_user!(user) - school = Role.find_by(user_id: user.id)&.school || find_by(creator_id: user.id, rejected_at: nil) + school = Role.find_by(user_id: user.id)&.school || active.find_by(creator_id: user.id) raise ActiveRecord::RecordNotFound unless school school @@ -82,6 +84,10 @@ def verify! update!(verified_at: Time.zone.now) end + def hidden? + rejected? || archived? + end + def generate_code! return code if code.present? diff --git a/db/migrate/20260703150521_update_school_index_conditions.rb b/db/migrate/20260703150521_update_school_index_conditions.rb new file mode 100644 index 000000000..df611432c --- /dev/null +++ b/db/migrate/20260703150521_update_school_index_conditions.rb @@ -0,0 +1,12 @@ +class UpdateSchoolIndexConditions < ActiveRecord::Migration[8.1] + def change + remove_index :schools, :creator_id + add_index :schools, :creator_id, unique: true, where: "rejected_at IS NULL AND archived_at IS NULL" + + remove_index :schools, :reference + add_index :schools, :reference, unique: true, where: "rejected_at IS NULL AND archived_at IS NULL" + + remove_index :schools, :school_roll_number + add_index :schools, :school_roll_number, unique: true, where: "rejected_at IS NULL AND archived_at IS NULL" + end +end diff --git a/db/schema.rb b/db/schema.rb index a8f8a94bf..9031ffa81 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[8.1].define(version: 2026_07_03_145125) do +ActiveRecord::Schema[8.1].define(version: 2026_07_03_150521) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -349,9 +349,9 @@ t.datetime "verified_at" t.string "website", null: false t.index ["code"], name: "index_schools_on_code", unique: true - t.index ["creator_id"], name: "index_schools_on_creator_id_active_only", 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)" + t.index ["creator_id"], name: "index_schools_on_creator_id", unique: true, where: "((rejected_at IS NULL) AND (archived_at IS NULL))" + t.index ["reference"], name: "index_schools_on_reference", unique: true, where: "((rejected_at IS NULL) AND (archived_at IS NULL))" + t.index ["school_roll_number"], name: "index_schools_on_school_roll_number", unique: true, where: "((rejected_at IS NULL) AND (archived_at IS NULL))" end create_table "scratch_assets", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 3ef01a14a..5bce08de8 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -217,6 +217,14 @@ expect { new_school.save! }.not_to raise_error end + it 'allows reference reuse when original school is archived' do + school.update!(reference: '100000', archived_at: Time.zone.now) + + 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 @@ -282,6 +290,14 @@ expect { new_school.save! }.not_to raise_error end + it 'allows district_nces_id reuse when original school is archived' do + us_school.update!(archived_at: Time.zone.now) + + new_school = build(:school, country_code: 'US', district_name: 'Some District', district_nces_id: '0100000') + 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 @@ -353,6 +369,14 @@ expect { new_school.save! }.not_to raise_error end + it 'allows school_roll_number reuse when original school is archived' do + ireland_school.update!(archived_at: Time.zone.now) + + 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 + it 'requires an address_line_1' do school.address_line_1 = ' ' expect(school).not_to be_valid From e01cc8d33f341227e1e55bf01d48ed9247220817 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 3 Jul 2026 16:49:15 +0100 Subject: [PATCH 3/4] Archive roles in schools When we are manually doing this we tend to delete all the roles for the school. This isn't ideal as it's hard to recover from or later use that data in the datawarehouse or sync to salesforce. Instead add an 'archived_at' column that we can use to keep the data around. I think this is a good use of a default scope - it would be dangerous if we were allowing archived roles to access data so it's safer to block them. I've chosen not to archive students as we already have a deletion process for them that the owner can do before requesting the school to be removed. Also, most of the requests for deletions are duplicate schools that have no students. --- app/controllers/admin/schools_controller.rb | 6 +++- app/models/role.rb | 6 ++++ app/models/school.rb | 19 ++++++++++-- ...20260703151018_add_archived_at_to_roles.rb | 5 ++++ db/schema.rb | 3 +- spec/features/admin/schools_spec.rb | 9 ++++++ spec/models/school_spec.rb | 29 +++++++++++++++++++ 7 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20260703151018_add_archived_at_to_roles.rb diff --git a/app/controllers/admin/schools_controller.rb b/app/controllers/admin/schools_controller.rb index fe298b851..5bb5de823 100644 --- a/app/controllers/admin/schools_controller.rb +++ b/app/controllers/admin/schools_controller.rb @@ -29,7 +29,11 @@ def reopen end def archive - requested_resource.archive! + if requested_resource.archive + flash[:notice] = t('administrate.controller.archive_school.success') + else + flash[:error] = requested_resource.errors.full_messages.to_sentence.presence || t('administrate.controller.archive_school.error') + end redirect_to admin_school_path(requested_resource) end diff --git a/app/models/role.rb b/app/models/role.rb index db6dc67cd..ac9d9c19e 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -10,6 +10,8 @@ class Role < ApplicationRecord validate :students_cannot_have_additional_roles validate :users_can_only_have_roles_in_one_school + default_scope { where(archived_at: nil) } + has_paper_trail( meta: { meta_school_id: ->(cm) { cm.school&.id } @@ -18,6 +20,10 @@ class Role < ApplicationRecord after_commit :do_salesforce_sync, on: %i[create update], if: -> { FeatureFlags.salesforce_sync? && !student? } + def archive! + update!(archived_at: Time.zone.now) + end + private def students_cannot_have_additional_roles diff --git a/app/models/school.rb b/app/models/school.rb index d5e2eaf99..098e7f6f6 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -46,6 +46,7 @@ class School < ApplicationRecord validates :code, uniqueness: { allow_nil: true }, format: { with: /\d\d-\d\d-\d\d/, allow_nil: true } + validate :cannot_archive_with_students, on: :archive validate :verified_at_cannot_be_changed validate :code_cannot_be_changed @@ -109,8 +110,18 @@ def reopen update(rejected_at: nil) end - def archive! - update!(archived_at: Time.zone.now, verified_at: nil) + def archive + return true if archived? + + transaction do + self.archived_at = Time.zone.now + save!(context: :archive) + roles.where(role: %i[owner teacher]).find_each(&:archive!) + end + + true + rescue ActiveRecord::RecordInvalid + false end def archived? @@ -169,6 +180,10 @@ def normalize_school_roll_number self.school_roll_number = (school_roll_number.presence&.upcase) end + def cannot_archive_with_students + errors.add(:base, 'Cannot archive a school with students') if roles.student.exists? + 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 diff --git a/db/migrate/20260703151018_add_archived_at_to_roles.rb b/db/migrate/20260703151018_add_archived_at_to_roles.rb new file mode 100644 index 000000000..bbadf5505 --- /dev/null +++ b/db/migrate/20260703151018_add_archived_at_to_roles.rb @@ -0,0 +1,5 @@ +class AddArchivedAtToRoles < ActiveRecord::Migration[8.1] + def change + add_column :roles, :archived_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 9031ffa81..74dc5e40b 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[8.1].define(version: 2026_07_03_150521) do +ActiveRecord::Schema[8.1].define(version: 2026_07_03_151018) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -253,6 +253,7 @@ end create_table "roles", force: :cascade do |t| + t.datetime "archived_at" t.datetime "created_at", null: false t.integer "role" t.uuid "school_id" diff --git a/spec/features/admin/schools_spec.rb b/spec/features/admin/schools_spec.rb index be402ad04..2df754e35 100644 --- a/spec/features/admin/schools_spec.rb +++ b/spec/features/admin/schools_spec.rb @@ -218,6 +218,15 @@ expect(school.reload).to be_archived end + it 'displays the validation error when the school has students' do + create(:student_role, school:) + + patch archive_admin_school_path(school) + follow_redirect! + + expect(response.body).to include('Cannot archive a school with students') + end + it 'redirects to school path' do patch archive_admin_school_path(school) expect(response).to redirect_to(admin_school_path(school)) diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 5bce08de8..475c6a370 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -714,6 +714,35 @@ end end + describe '#archive' do + it 'sets archived_at and returns true' do + result = nil + + expect do + result = school.archive + end.to change { school.reload.archived? }.from(false).to(true) + + expect(result).to be(true) + end + + it 'archives owner and teacher roles' do + owner_role = create(:owner_role, school:) + teacher_role = create(:teacher_role, school:) + + school.archive + + expect(Role.unscoped.find(owner_role.id).archived_at).to be_present + expect(Role.unscoped.find(teacher_role.id).archived_at).to be_present + end + + it 'does not archive school if students exist and returns false' do + create(:student_role, school:) + + expect(school.archive).to be(false) + expect(school.reload).not_to be_archived + end + end + describe '#auto_join_enabled?' do it 'returns true when the school has at least one registered email domain' do SchoolEmailDomain.create!(school:, domain: 'valid.edu') From eaa75f381530ce3cef2c29c1f3624f33f79ed46a Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:01:16 +0100 Subject: [PATCH 4/4] Add student count --- app/dashboards/school_dashboard.rb | 4 +++- app/models/school.rb | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/dashboards/school_dashboard.rb b/app/dashboards/school_dashboard.rb index fb3dfb013..5629da864 100644 --- a/app/dashboards/school_dashboard.rb +++ b/app/dashboards/school_dashboard.rb @@ -27,6 +27,7 @@ class SchoolDashboard < Administrate::BaseDashboard lessons: Field::HasMany, projects: Field::HasMany, roles: SchoolRolesField, + student_count: Field::Number, reference: Field::String, district_name: Field::String, district_nces_id: Field::String, @@ -65,6 +66,7 @@ class SchoolDashboard < Administrate::BaseDashboard user_origin creator roles + student_count creator_role creator_department reference @@ -85,7 +87,7 @@ class SchoolDashboard < Administrate::BaseDashboard updated_at verified_at rejected_at -archived_at + archived_at ].freeze # FORM_ATTRIBUTES diff --git a/app/models/school.rb b/app/models/school.rb index 098e7f6f6..3ddb8c92d 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -128,6 +128,10 @@ def archived? archived_at.present? end + def student_count + roles.student.count + end + def postal_code=(str) super(str.to_s.upcase) end