Archive schools#908
Conversation
Test coverage92.09% line coverage reported by SimpleCov. |
raspberrypiherokubot
temporarily deployed
to
editor-api-p-archive-sc-i9snzs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds “archive school” functionality to the Rails admin area, allowing admins to offboard a school (and its owner/teacher roles) without deleting data, while preventing archiving if students still exist.
Changes:
- Introduces
archived_atonschoolsandroles, plus School/Role model logic to archive and hide archived records by default. - Adds an admin UI action (button + controller route/action) to archive a school, including validation that blocks archiving when students exist.
- Updates unique constraints/validations to treat archived schools as inactive, allowing key identifiers to be reused.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/models/school_spec.rb | Adds specs for archived reuse and School#archive behavior. |
| spec/features/admin/schools_spec.rb | Adds request specs for the admin archive action/link. |
| db/schema.rb | Reflects new archived_at columns and updated partial indexes. |
| db/migrate/20260703151018_add_archived_at_to_roles.rb | Adds roles.archived_at. |
| db/migrate/20260703150521_update_school_index_conditions.rb | Updates schools partial unique indexes to exclude archived schools. |
| db/migrate/20260703145125_add_archived_at_to_schools.rb | Adds schools.archived_at. |
| config/routes.rb | Adds archive member route for admin schools. |
| config/locales/admin/en.yml | Adds i18n strings for archive action/flash messaging. |
| app/views/admin/schools/show.html.erb | Adds Archive School button to the admin school show page. |
| app/models/school.rb | Adds active scope, hidden?, archive validation, and #archive implementation. |
| app/models/role.rb | Adds default scope to hide archived roles and #archive!. |
| app/jobs/salesforce/school_sync_job.rb | Syncs archived_at to Salesforce (discardedat__c). |
| app/jobs/salesforce/role_sync_job.rb | Syncs role archived_at to Salesforce (offboardedat__c). |
| app/dashboards/school_dashboard.rb | Exposes student_count and archived_at in Administrate. |
| app/controllers/admin/schools_controller.rb | Adds the archive action with success/error flashes and redirect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
It looks good overall, i think some of the copilot comments may apply have a look and we can merge it |
zetter-rpf
force-pushed
the
archive-schools
branch
from
1488359 to
74aab2f
Compare
The rails JS API has changed
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.
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.
Having shared setup makes tests harder to change
zetter-rpf
force-pushed
the
archive-schools
branch
from
74aab2f to
ee41784
Compare
I spoke to Liesl about the best way to model this. Mo models in Salesforce already have a discarded at field (or offboarded for people) that we can use for this purpose. Liesl explained that records with these set are already ignored and it's fine to toggle this back and forth if needed.
zetter-rpf
force-pushed
the
archive-schools
branch
from
ee41784 to
3fd7e80
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3fd7e80. Configure here.
| validate :students_cannot_have_additional_roles | ||
| validate :users_can_only_have_roles_in_one_school | ||
|
|
||
| default_scope { where(archived_at: nil) } |
There was a problem hiding this comment.
Archived schools accept join enrollments
High Severity
The school archiving process isn't fully effective because the class join flow (in JoinStatusService and JoinController) doesn't check if a school is archived. This allows new student roles to be created on an archived school, keeping it active for new enrollments despite the archiving workflow.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3fd7e80. Configure here.
| validate :students_cannot_have_additional_roles | ||
| validate :users_can_only_have_roles_in_one_school | ||
|
|
||
| default_scope { where(archived_at: nil) } |
There was a problem hiding this comment.
Hidden roles break re-create
Medium Severity
Adding a default_scope that hides archived roles while the unique index on user_id, school_id, and role still includes archived rows means flows like teacher invitation accept that use find_or_initialize_by attempt an insert for a row that already exists. That can surface as an unhandled database uniqueness error instead of a validation response.
Reviewed by Cursor Bugbot for commit 3fd7e80. Configure here.
|
|
||
| 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) |
There was a problem hiding this comment.
Role lookup ignores active schools
Medium Severity
School.find_for_user! now resolves creators through School.active, but it still loads a school from any non-archived Role without requiring the school to be active. A student role on an archived school (for example after join enrolment) makes MySchoolController return that archived school.
Reviewed by Cursor Bugbot for commit 3fd7e80. Configure here.
Status
Context
Currently educators can only belong to one school. We often have requests from educators to remove their old School so they can create/join a new one. We've been following a manual process to do this.
What's changed?
This adds a button to the admin panel so any admin can 'archive' a school. This marks the school and all owner/teacher roles as archived.
Archiving a School does not delete any data so we could recover from it if it was done accidentally.
To prevent us from archiving a school that is in use, archiving requires there be no students in the school. Teachers will need to remove any students before we can archive.
I haven't removed any safeguarding flags as part of this - the safeguarding flags should remain as we may still need to link usage to an individual later.
See commits for more.