Implement Endpoint to allow Mass Student Deletion

app/controllers/api/school_students_controller.rb

@@ -110,17 +110,63 @@ def update
     end
 
     def destroy
-      result = SchoolStudent::Delete.call(school: @school, student_id: params[:id], token: current_user.token)
+      remove_students([params[:id]])
+    end
 
-      if result.success?
-        head :no_content
-      else
-        render json: { error: result[:error] }, status: :unprocessable_entity
+    def destroy_batch
+      # DELETE /api/schools/:school_id/students/batch
+      # Params: { student_ids: ["uuid1", "uuid2", ...] }
+      #
+      # Returns 200 OK with one of:
+      # - Success: { results: [{ user_id: "..." }, ...] }
+      # - Partial failure: { results: [...], error: "N student(s) failed to be removed" }
+      #
+      # Each result may contain:
+      # - { user_id:, error: } - deletion failed
+      # - { user_id:, skipped:, reason: } - student skipped (e.g., not in this school)
+      # - { user_id: } - deletion succeeded
+
+      student_ids = student_ids_params
+
+      if student_ids.blank?
+        render json: {
+                 error: 'No student IDs provided',
+                 error_type: :unprocessable_entity
+               },
+               status: :unprocessable_entity
+        return
       end
+
+      # Remove duplicates to avoid redundant processing
+      unique_student_ids = student_ids.uniq
+      remove_students(unique_student_ids)
     end
 
     private
 
+    def remove_students(student_ids)
+      # Invoke StudentRemovalService
+      service = StudentRemovalService.new(
+        students: student_ids,
+        school: @school,
+        remove_from_profile: true,
+        token: current_user.token
+      )
+
+      results = service.remove_students
+
+      # Check if any errors occurred
+      errors = results.select { |r| r[:error] }
+      if errors.any?
+        render json: {
+          results: results,
+          error: "#{errors.size} student(s) failed to be removed"
+        }, status: :ok
+      else
+        render json: { results: results }, status: :ok
+      end
+    end
+
     def school_student_params
       params.require(:school_student).permit(:username, :password, :name)
     end
@@ -135,6 +181,10 @@ def school_students_params
       end
     end
 
+    def student_ids_params
+      params.fetch(:student_ids, [])
+    end
+
     def create_safeguarding_flags
       create_teacher_safeguarding_flag
       create_owner_safeguarding_flag

app/models/ability.rb

@@ -66,7 +66,7 @@ def define_school_owner_abilities(school:)
     can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } })
     can(%i[read create destroy], :school_owner)
     can(%i[read create destroy], :school_teacher)
-    can(%i[read create create_batch update destroy], :school_student)
+    can(%i[read create create_batch update destroy destroy_batch], :school_student)
     can(%i[create create_copy], Lesson, school_id: school.id)
     can(%i[read update destroy], Lesson, school_id: school.id, visibility: %w[teachers students public])
     can(%i[read destroy], Feedback, school_project: { school_id: school.id })

app/services/student_removal_service.rb

@@ -11,25 +11,31 @@ def initialize(students:, school:, remove_from_profile: false, token: nil)
   # Returns an array of hashes, one per student, with details of what was removed
   def remove_students
     results = []
+
     @students.each do |user_id|
+      # Ensure that the student has a role in this school and skip if not.
+      student_roles = Role.student.where(user_id:, school_id: @school.id)
+      if student_roles.empty?
+        results << { user_id:, skipped: true, reason: 'no_role_in_school' }
+        next
+      end
+
       result = { user_id: }
       begin
-        # Skip if student has projects
-        projects = Project.where(user_id: user_id)
-        result[:skipped] = true if projects.length.positive?
+        ActiveRecord::Base.transaction do
+          # Delete all projects for this user
+          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.joins(:school_class).where(student_id: user_id, school_class: { school_id: @school.id })
+          class_assignments.destroy_all
 
-            # Remove roles
-            roles = Role.student.where(user_id: user_id)
-            roles.destroy_all
-          end
+          # Remove roles
+          student_roles.destroy_all
 
-          # Remove from profile if requested
+          # Remove from profile if requested - inside transaction so it can be rolled back
+          # If this call fails, the entire transaction will be rolled back
           ProfileApiClient.delete_school_student(token: @token, school_id: @school.id, student_id: user_id) if @remove_from_profile && @token.present?
         end
       rescue StandardError => e

config/routes.rb

@@ -69,6 +69,7 @@
       resources :teachers, only: %i[index create], controller: 'school_teachers'
       resources :students, only: %i[index create update destroy], controller: 'school_students' do
         post :batch, on: :collection, to: 'school_students#create_batch'
+        delete :batch, on: :collection, to: 'school_students#destroy_batch'
       end
     end
 

lib/tasks/remove_students.rake

@@ -39,6 +39,7 @@ namespace :remove_students do
     puts "REMOVE_FROM_PROFILE: #{remove_from_profile}"
     puts "Students to remove: #{students.size}"
     puts "====================\n\n"
+    puts "WARNING: All student projects will be deleted permanently and this operation is not reversible.\n"
     puts "Please confirm deletion of #{students.size} user(s), and that recent Postgres backups have been captured for all services affected (https://devcenter.heroku.com/articles/heroku-postgres-backups#manual-backups)"
     print 'Are you sure you want to continue? (yes/no): '
     confirmation = $stdin.gets.strip.downcase
@@ -58,7 +59,7 @@ namespace :remove_students do
       if res[:error]
         "Student: #{res[:user_id]} | Error: #{res[:error]}"
       elsif res[:skipped]
-        "Student: #{res[:user_id]} | Skipped: has project(s)"
+        "Student: #{res[:user_id]} | Skipped: #{res[:reason]}"
       else
         "Student: #{res[:user_id]} | Removed successfully"
       end

spec/features/school_student/batch_deleting_school_students_spec.rb

@@ -0,0 +1,236 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Batch deleting school students', type: :request do
+  let(:school) { create(:school) }
+  let(:owner) { create(:owner, school: school) }
+  let(:teacher) { create(:teacher, school: school) }
+  let(:school_class) { create(:school_class, school: school, teacher_ids: [teacher.id]) }
+  let(:student_1) { create(:student, school: school) }
+  let(:student_2) { create(:student, school: school) }
+  let(:headers) { { Authorization: UserProfileMock::TOKEN } }
+
+  before do
+    authenticated_in_hydra_as(owner)
+    stub_profile_api_create_safeguarding_flag
+    create(:class_student, student_id: student_1.id, school_class: school_class)
+    create(:class_student, student_id: student_2.id, school_class: school_class)
+  end
+
+  describe 'DELETE /api/schools/:school_id/students/batch' do
+    before do
+      stub_profile_api_delete_school_student
+    end
+
+    it 'calls ProfileApiClient to delete each student from the profile service' do
+      delete "/api/schools/#{school.id}/students/batch",
+             params: { student_ids: [student_1.id, student_2.id] },
+             headers: headers
+
+      expect(response).to have_http_status(:ok)
+      expect(ProfileApiClient).to have_received(:delete_school_student).with(token: UserProfileMock::TOKEN, school_id: school.id, student_id: student_1.id)
+      expect(ProfileApiClient).to have_received(:delete_school_student).with(token: UserProfileMock::TOKEN, school_id: school.id, student_id: student_2.id)
+    end
+
+    it 'deletes all students and their projects' do
+      project_1 = create(:project, user_id: student_1.id)
+      project_2 = create(:project, user_id: student_2.id)
+
+      expect(ClassStudent.where(student_id: student_1.id)).to exist
+      expect(ClassStudent.where(student_id: student_2.id)).to exist
+      expect(Project.where(id: project_1.id)).to exist
+      expect(Project.where(id: project_2.id)).to exist
+
+      delete "/api/schools/#{school.id}/students/batch",
+             params: { student_ids: [student_1.id, student_2.id] },
+             headers: headers
+
+      expect(response).to have_http_status(:ok)
+
+      json = JSON.parse(response.body)
+      expect(json['results'].size).to eq(2)
+      expect(json['results'].all? { |r| r['error'].nil? }).to be true
+
+      # Verify students removed from classes
+      expect(ClassStudent.where(student_id: student_1.id)).not_to exist
+      expect(ClassStudent.where(student_id: student_2.id)).not_to exist
+
+      # Verify projects deleted
+      expect(Project.where(id: project_1.id)).not_to exist
+      expect(Project.where(id: project_2.id)).not_to exist
+    end
+
+    it 'responds 403 Forbidden when the user is a school-teacher' do
+      authenticated_in_hydra_as(teacher)
+
+      delete "/api/schools/#{school.id}/students/batch",
+             params: { student_ids: [student_1.id, student_2.id] },
+             headers: headers
+
+      expect(response).to have_http_status(:forbidden)
+    end
+
+    it 'returns error when no student IDs provided' do
+      delete "/api/schools/#{school.id}/students/batch",
+             headers: headers
+
+      expect(response).to have_http_status(:unprocessable_entity)
+      json = JSON.parse(response.body)
+      expect(json['error']).to eq('No student IDs provided')
+    end
+
+    context 'when validating input parameters' do
+      it 'removes duplicate student IDs before processing' do
+        duplicate_ids = [student_1.id, student_1.id, student_2.id, student_2.id, student_1.id]
+
+        delete "/api/schools/#{school.id}/students/batch",
+               params: { student_ids: duplicate_ids },
+               headers: headers
+
+        expect(response).to have_http_status(:ok)
+        json = JSON.parse(response.body)
+        # Should only process 2 unique students
+        expect(json['results'].size).to eq(2)
+        expect(json['results'].pluck('user_id')).to contain_exactly(student_1.id, student_2.id)
+      end
+    end
+
+    context 'when handling non-existent student IDs' do
+      it 'skips non-existent students and processes valid ones' do
+        non_existent_id = SecureRandom.uuid
+        project_1 = create(:project, user_id: student_1.id)
+
+        delete "/api/schools/#{school.id}/students/batch",
+               params: { student_ids: [student_1.id, non_existent_id] },
+               headers: headers
+
+        expect(response).to have_http_status(:ok)
+
+        json = JSON.parse(response.body)
+        expect(json['results'].size).to eq(2)
+
+        # Valid student should be removed
+        valid_result = json['results'].find { |r| r['user_id'] == student_1.id }
+        expect(valid_result['error']).to be_nil
+        expect(Project.exists?(project_1.id)).to be false
+        expect(ClassStudent.where(student_id: student_1.id)).not_to exist
+
+        # Non-existent student should be skipped
+        skipped_result = json['results'].find { |r| r['user_id'] == non_existent_id }
+        expect(skipped_result['skipped']).to be true
+        expect(skipped_result['reason']).to eq('no_role_in_school')
+      end
+
+      it 'returns success when all IDs are non-existent' do
+        non_existent_id_1 = SecureRandom.uuid
+        non_existent_id_2 = SecureRandom.uuid
+
+        delete "/api/schools/#{school.id}/students/batch",
+               params: { student_ids: [non_existent_id_1, non_existent_id_2] },
+               headers: headers
+
+        expect(response).to have_http_status(:ok)
+
+        json = JSON.parse(response.body)
+        expect(json['results'].size).to eq(2)
+        expect(json['results'].all? { |r| r['skipped'] == true }).to be true
+        expect(json['results'].all? { |r| r['reason'] == 'no_role_in_school' }).to be true
+      end
+    end
+
+    context 'when handling students from different schools' do
+      let(:other_school) { create(:school) }
+      let(:other_school_owner) { create(:owner, school: other_school) }
+      let(:other_student) { create(:student, school: other_school) }
+
+      before do
+        other_school_class = create(:school_class, school: other_school)
+        create(:class_student, student_id: other_student.id, school_class: other_school_class)
+      end
+
+      it 'skips students from different schools and processes own students' do
+        project_1 = create(:project, user_id: student_1.id)
+        other_project = create(:project, user_id: other_student.id)
+
+        delete "/api/schools/#{school.id}/students/batch",
+               params: { student_ids: [student_1.id, other_student.id] },
+               headers: headers
+
+        expect(response).to have_http_status(:ok)
+
+        json = JSON.parse(response.body)
+        expect(json['results'].size).to eq(2)
+
+        # Own student should be removed
+        valid_result = json['results'].find { |r| r['user_id'] == student_1.id }
+        expect(valid_result['error']).to be_nil
+        expect(Project.exists?(project_1.id)).to be false
+        expect(ClassStudent.where(student_id: student_1.id)).not_to exist
+
+        # Other school's student should be skipped
+        skipped_result = json['results'].find { |r| r['user_id'] == other_student.id }
+        expect(skipped_result['skipped']).to be true
+        expect(skipped_result['reason']).to eq('no_role_in_school')
+
+        # Other school's student data should remain intact
+        expect(Project.exists?(other_project.id)).to be true
+        expect(Role.where(user_id: other_student.id, school_id: other_school.id, role: :student)).to exist
+      end
+    end
+
+    context 'when handling partial failures' do
+      it 'returns 200 OK with error details when some deletions fail' do
+        project_2 = create(:project, user_id: student_2.id)
+
+        # Simulate ProfileApiClient failure for one student
+        allow(ProfileApiClient).to receive(:delete_school_student).with(
+          token: UserProfileMock::TOKEN,
+          school_id: school.id,
+          student_id: student_1.id
+        ).and_raise(StandardError, 'Profile API error')
+
+        allow(ProfileApiClient).to receive(:delete_school_student).with(
+          token: UserProfileMock::TOKEN,
+          school_id: school.id,
+          student_id: student_2.id
+        ).and_return(true)
+
+        delete "/api/schools/#{school.id}/students/batch",
+               params: { student_ids: [student_1.id, student_2.id] },
+               headers: headers
+
+        expect(response).to have_http_status(:ok)
+
+        json = JSON.parse(response.body)
+        expect(json['results'].size).to eq(2)
+        expect(json['error']).to eq('1 student(s) failed to be removed')
+
+        # First student should have error
+        failed_result = json['results'].find { |r| r['user_id'] == student_1.id }
+        expect(failed_result['error']).to match(/StandardError: Profile API error/)
+
+        # Second student should succeed
+        success_result = json['results'].find { |r| r['user_id'] == student_2.id }
+        expect(success_result['error']).to be_nil
+        expect(Project.exists?(project_2.id)).to be false
+        expect(ClassStudent.where(student_id: student_2.id)).not_to exist
+      end
+
+      it 'reports correct error count when multiple deletions fail' do
+        allow(ProfileApiClient).to receive(:delete_school_student).and_raise(StandardError, 'Profile API down')
+
+        delete "/api/schools/#{school.id}/students/batch",
+               params: { student_ids: [student_1.id, student_2.id] },
+               headers: headers
+
+        expect(response).to have_http_status(:ok)
+
+        json = JSON.parse(response.body)
+        expect(json['results'].size).to eq(2)
+        expect(json['error']).to eq('2 student(s) failed to be removed')
+        expect(json['results'].all? { |r| r['error'].present? }).to be true
+      end
+    end
+  end
+end