Implement Endpoint to allow Mass Student Deletion #637
Conversation
fspeirs
changed the title
fspeirs
force-pushed
the
excs-1597-implement-student-removal-back-end-features
branch
from
123e4f3 to
8811c27
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements a mass student deletion endpoint and modifies the student deletion behavior to unconditionally delete students and their projects, rather than skipping students with projects. The single deletion endpoint now delegates to the batch endpoint, resulting in a status code change from 204 to 200.
Key changes:
- New
DELETE /api/schools/:school_id/students/batchendpoint for batch deletion of students (owner-only permission) - Modified
StudentRemovalServiceto delete all student projects instead of skipping students with projects - Single student deletion now wraps batch deletion, changing response from 204 No Content to 200 OK with JSON body
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/controllers/api/school_students_controller.rb | Implements destroy_batch endpoint and refactors destroy to delegate to it; adds student_ids_params method |
| app/models/ability.rb | Grants destroy_batch permission to school owners (not teachers) |
| app/services/student_removal_service.rb | Removes skip logic and unconditionally deletes projects, class assignments, and student roles |
| config/routes.rb | Adds DELETE batch route for students collection |
| spec/features/school_student/batch_deleting_school_students_spec.rb | Tests batch deletion success and validation error cases |
| spec/features/school_student/deleting_a_school_student_spec.rb | Updates expected status code from 204 to 200 |
| spec/services/student_removal_service_spec.rb | Removes skip test and updates to verify project deletion |
| lib/tasks/remove_students.rake | Adds warning message about permanent project deletion |
| # Delete all projects | ||
| projects = Project.where(user_id: user_id) | ||
| projects.destroy_all | ||
|
|
||
| unless result[:skipped] | ||
| ActiveRecord::Base.transaction do | ||
| # Remove from classes | ||
| class_assignments = ClassStudent.where(student_id: user_id) | ||
| class_assignments.destroy_all | ||
| # Remove from classes | ||
| class_assignments = ClassStudent.where(student_id: user_id) | ||
| class_assignments.destroy_all | ||
|
|
||
| # Remove roles | ||
| roles = Role.student.where(user_id: user_id) | ||
| roles.destroy_all | ||
| end | ||
|
|
||
| # Remove from profile if requested | ||
| ProfileApiClient.delete_school_student(token: @token, school_id: @school.id, student_id: user_id) if @remove_from_profile && @token.present? | ||
| # Remove roles | ||
| roles = Role.student.where(user_id: user_id) | ||
| roles.destroy_all |
There was a problem hiding this comment.
Critical security issue: The StudentRemovalService deletes ALL projects for a user (line 19), removes them from ALL classes (line 23), and removes ALL student roles (line 27), regardless of which school they belong to. This means a school owner could delete projects and remove students from other schools by providing student IDs that don't belong to their school.
The deletions should be scoped to the specific school:
- Projects:
Project.where(user_id: user_id, school_id: @school.id) - Classes:
ClassStudent.joins(:school_class).where(student_id: user_id, school_classes: { school_id: @school.id }) - Roles:
Role.student.where(user_id: user_id, school_id: @school.id)
Additionally, consider validating that the provided student IDs actually have student roles in the school before attempting deletion.
There was a problem hiding this comment.
I think the correct solution here is to ensure that (a) the student ID definitely has the Student role in the given school and (b) we only delete ClassStudent records which relate to this school. In the latter case, it is enforced elsewhere that students can only have a role in one school but it's defensive to check that here in case that constraint is removed in the future.
If we only delete Projects where the school_id is set, we'll miss deleting the user's personal projects.
fspeirs
force-pushed
the
excs-1597-implement-student-removal-back-end-features
branch
from
8382c91 to
17ac6bc
Compare
This code: 1. Modifies StudentRemovalService to mass-delete users and their projects instead of skipping users who have projects. 2. Adds an endpoint for school owners to mass-delete students.
fspeirs
force-pushed
the
excs-1597-implement-student-removal-back-end-features
branch
from
17ac6bc to
29c877a
Compare
Status
Points for consideration:
StudentRemovalServicefrom "skips deleting users that have projects" to "unconditionally deletes users and their projects" (see Slack thread from 2025-12-03)What's changed?
SchoolStudentsController#destroy_batchendpoint for batch deletion of students, only accessible to school owners (not teachers).SchoolStudentsController#destroyendpoint to wrapSchoolStudentsController#destroy_batch, which leads its return code to change from204to200.Steps to perform after deploying to production
Nothing additional required.
Tasks Remaining Before Merge