diff --git a/app/controllers/admin/schools_controller.rb b/app/controllers/admin/schools_controller.rb index eebc90f59..5bb5de823 100644 --- a/app/controllers/admin/schools_controller.rb +++ b/app/controllers/admin/schools_controller.rb @@ -28,6 +28,16 @@ def reopen redirect_to admin_school_path(requested_resource) end + def 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 + def default_sorting_attribute :created_at end diff --git a/app/controllers/api/teacher_invitations_controller.rb b/app/controllers/api/teacher_invitations_controller.rb index ec486b871..738ae3252 100644 --- a/app/controllers/api/teacher_invitations_controller.rb +++ b/app/controllers/api/teacher_invitations_controller.rb @@ -13,7 +13,8 @@ def show end def accept - role = Role.teacher.find_or_initialize_by(user_id: current_user.id, school: @invitation.school) + role = Role.unscoped.teacher.find_or_initialize_by(user_id: current_user.id, school: @invitation.school) + role.archived_at = nil if role.save @invitation.update!(accepted_at: Time.current) if @invitation.accepted_at.blank? head :ok diff --git a/app/dashboards/school_dashboard.rb b/app/dashboards/school_dashboard.rb index 213c8da35..5629da864 100644 --- a/app/dashboards/school_dashboard.rb +++ b/app/dashboards/school_dashboard.rb @@ -27,12 +27,14 @@ 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, 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 @@ -64,6 +66,7 @@ class SchoolDashboard < Administrate::BaseDashboard user_origin creator roles + student_count creator_role creator_department reference @@ -84,6 +87,7 @@ class SchoolDashboard < Administrate::BaseDashboard updated_at verified_at rejected_at + archived_at ].freeze # FORM_ATTRIBUTES diff --git a/app/jobs/salesforce/role_sync_job.rb b/app/jobs/salesforce/role_sync_job.rb index 4ffa5503a..a6dc98fb6 100644 --- a/app/jobs/salesforce/role_sync_job.rb +++ b/app/jobs/salesforce/role_sync_job.rb @@ -9,12 +9,13 @@ class RoleSyncJob < SalesforceSyncJob contact__r__pi_accounts_unique_id__c: :user_id, editor__r__editoruuid__c: :school_id, roletype__c: :role, + offboardedat__c: :archived_at, createdat__c: :created_at, updatedat__c: :updated_at }.freeze def perform(role_id:) - role = ::Role.find(role_id) + role = ::Role.unscoped.find(role_id) return if role.student? diff --git a/app/jobs/salesforce/school_sync_job.rb b/app/jobs/salesforce/school_sync_job.rb index 4c3c91fa9..3ef08b604 100644 --- a/app/jobs/salesforce/school_sync_job.rb +++ b/app/jobs/salesforce/school_sync_job.rb @@ -22,7 +22,8 @@ class SchoolSyncJob < SalesforceSyncJob website__c: :website, districtnamesupplied__c: :district_name, ncesid__c: :district_nces_id, - schoolrollnumber__c: :school_roll_number + schoolrollnumber__c: :school_roll_number, + discardedat__c: :archived_at }.freeze def perform(school_id:, is_create: false) diff --git a/app/models/ability.rb b/app/models/ability.rb index 83bf7c693..389852c4d 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -9,7 +9,7 @@ def initialize(user) return unless user define_authenticated_non_student_abilities(user) - user.schools.each do |school| + user.schools.active.each do |school| define_school_student_abilities(user:, school:) if user.school_student?(school) define_school_teacher_abilities(user:, school:) if user.school_teacher?(school) define_school_owner_abilities(school:) if user.school_owner?(school) 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 61b8a07f1..3ddb8c92d 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 @@ -44,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 @@ -59,7 +62,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 +85,10 @@ def verify! update!(verified_at: Time.zone.now) end + def hidden? + rejected? || archived? + end + def generate_code! return code if code.present? @@ -103,6 +110,28 @@ def reopen update(rejected_at: nil) end + 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? + archived_at.present? + end + + def student_count + roles.student.count + end + def postal_code=(str) super(str.to_s.upcase) end @@ -155,6 +184,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/app/views/admin/schools/show.html.erb b/app/views/admin/schools/show.html.erb index 02b182c44..edc4b534d 100644 --- a/app/views/admin/schools/show.html.erb +++ b/app/views/admin/schools/show.html.erb @@ -28,23 +28,32 @@ as well as a link to its edit page. class: "button", ) if accessible_action?(page.resource, :edit) %> - <%= link_to( + <%= button_to( t("administrate.actions.verify_school"), verify_admin_school_path(page.resource), class: "button button--verify", method: :post, style: "background-color: green; margin: 5px;", - data: { confirm: t("administrate.actions.confirm_verify_school") } + data: { turbo_confirm: t("administrate.actions.confirm_verify_school") } ) unless page.resource.verified? || page.resource.rejected? %> - <%= link_to( + <%= button_to( t("administrate.actions.reopen_school"), reopen_admin_school_path(page.resource), class: "button button--verify", method: :patch, style: "background-color: darkorange; margin: 5px;", - data: { confirm: t("administrate.actions.confirm_reopen_school") } + data: { turbo_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;", + data: { turbo_confirm: 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/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/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 952226723..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_06_12_144637) 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" @@ -324,6 +325,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 @@ -348,9 +350,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/features/admin/schools_spec.rb b/spec/features/admin/schools_spec.rb index b5131e35d..2df754e35 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,34 @@ 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 '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)) + end + end + private def sign_in_as(user) diff --git a/spec/features/teacher_invitations/accepting_an_invitation_spec.rb b/spec/features/teacher_invitations/accepting_an_invitation_spec.rb index 38a0bbfee..6970f805e 100644 --- a/spec/features/teacher_invitations/accepting_an_invitation_spec.rb +++ b/spec/features/teacher_invitations/accepting_an_invitation_spec.rb @@ -197,9 +197,7 @@ end context 'when user already has teacher role for the same school' do - before do - Role.teacher.create!(user_id: user.id, school:) - end + let(:role) { Role.teacher.create!(user_id: user.id, school:) } it 'responds 200 OK' do put("/api/teacher_invitations/#{token}/accept", headers:) @@ -213,6 +211,15 @@ expect(user).to be_school_teacher(school) end + it 'makes the user active if they were previously archived' do + role.update!(archived_at: Time.current) + + put("/api/teacher_invitations/#{token}/accept", headers:) + + expect(user).to be_school_teacher(school) + expect(role.reload.archived_at).to be_nil + end + it 'sets the accepted_at timestamp on the invitation' do freeze_time(with_usec: false) do put("/api/teacher_invitations/#{token}/accept", headers:) diff --git a/spec/jobs/salesforce/role_sync_job_spec.rb b/spec/jobs/salesforce/role_sync_job_spec.rb index 2df328a3b..db0cf834d 100644 --- a/spec/jobs/salesforce/role_sync_job_spec.rb +++ b/spec/jobs/salesforce/role_sync_job_spec.rb @@ -17,22 +17,27 @@ ClimateControl.modify(SALESFORCE_ENABLED: 'true') { example.run } end - context 'when the job has run' do - before { perform_job } - - it 'syncs all FIELD_MAPPINGS to the correct role values' do - sf_role = Salesforce::Role.find_by(affiliation_id__c: role.id) - described_class::FIELD_MAPPINGS.each do |sf_field, role_field| - expected = Salesforce::Role.type_for_attribute(sf_field).cast(role.send(role_field)) - expect(sf_role.send(sf_field)).to eq(expected), - "Expected #{sf_field} to equal role.#{role_field}" - end + it 'syncs all FIELD_MAPPINGS to the correct role values' do + perform_job + sf_role = Salesforce::Role.find_by(affiliation_id__c: role.id) + described_class::FIELD_MAPPINGS.each do |sf_field, role_field| + expected = Salesforce::Role.type_for_attribute(sf_field).cast(role.send(role_field)) + expect(sf_role.send(sf_field)).to eq(expected), + "Expected #{sf_field} to equal role.#{role_field}" end + end - it 'sets editor_type__c from the school user_origin' do - sf_role = Salesforce::Role.find_by(affiliation_id__c: role.id) - expect(sf_role.editor_type__c).to eq(role.school.user_origin) - end + it 'sets editor_type__c from the school user_origin' do + perform_job + sf_role = Salesforce::Role.find_by(affiliation_id__c: role.id) + expect(sf_role.editor_type__c).to eq(role.school.user_origin) + end + + it 'syncs archived roles' do + role.update!(archived_at: Time.zone.now) + perform_job + sf_role = Salesforce::Role.find_by(affiliation_id__c: role.id) + expect(sf_role.offboardedat__c.to_date).to eq(role.archived_at.to_date) end context 'when the Salesforce role fails to save' do diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 24efa8095..c0e5a5886 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -4,7 +4,7 @@ require 'rails_helper' RSpec.describe Ability do - subject { described_class.new(user) } + subject(:ability) { described_class.new(user) } let(:user_id) { SecureRandom.uuid } let(:project) { build(:project, user_id:) } @@ -485,6 +485,11 @@ it { is_expected.to be_able_to(:read, school) } it { is_expected.to be_able_to(:update, school) } it { is_expected.to be_able_to(:destroy, school) } + + it 'cannot interact with an inactive school' do + school.update!(archived_at: Time.current) + expect(ability).not_to be_able_to(:read, school) + end end context 'when user is a school teacher' do @@ -495,6 +500,11 @@ it { is_expected.to be_able_to(:read, school) } it { is_expected.not_to be_able_to(:update, school) } it { is_expected.not_to be_able_to(:destroy, school) } + + it 'cannot interact with an inactive school' do + school.update!(archived_at: Time.current) + expect(ability).not_to be_able_to(:read, school) + end end context 'when user is a school student' do @@ -506,6 +516,11 @@ it { is_expected.not_to be_able_to(:update, school) } it { is_expected.not_to be_able_to(:destroy, school) } + it 'cannot interact with an inactive school' do + school.update(archived_at: Time.current) + expect(ability).not_to be_able_to(:read, school) + end + context 'with a starter project' do it { is_expected.not_to be_able_to(:index, starter_project) } it { is_expected.not_to be_able_to(:show, starter_project) } diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 3ef01a14a..cca6f977c 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 @@ -353,6 +361,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 @@ -690,6 +706,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')