From 29c877a665111c669929c897b5a768b10e6957ef Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Wed, 3 Dec 2025 12:04:09 +0000 Subject: [PATCH] Add endpoint for mass user deletion. 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. --- .../api/school_students_controller.rb | 60 ++++- app/models/ability.rb | 2 +- app/services/student_removal_service.rb | 32 ++- config/routes.rb | 1 + lib/tasks/remove_students.rake | 3 +- .../batch_deleting_school_students_spec.rb | 236 ++++++++++++++++++ .../deleting_a_school_student_spec.rb | 10 +- spec/services/student_removal_service_spec.rb | 193 ++++++++++++-- 8 files changed, 490 insertions(+), 47 deletions(-) create mode 100644 spec/features/school_student/batch_deleting_school_students_spec.rb diff --git a/app/controllers/api/school_students_controller.rb b/app/controllers/api/school_students_controller.rb index 7045c1493..a67189d2c 100644 --- a/app/controllers/api/school_students_controller.rb +++ b/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 diff --git a/app/models/ability.rb b/app/models/ability.rb index 65bc1987a..8686e2067 100644 --- a/app/models/ability.rb +++ b/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 }) diff --git a/app/services/student_removal_service.rb b/app/services/student_removal_service.rb index 0437fd3ba..e168e37a4 100644 --- a/app/services/student_removal_service.rb +++ b/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 diff --git a/config/routes.rb b/config/routes.rb index 813ad8737..8d777047f 100644 --- a/config/routes.rb +++ b/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 diff --git a/lib/tasks/remove_students.rake b/lib/tasks/remove_students.rake index 134e0f92e..8c0202c32 100644 --- a/lib/tasks/remove_students.rake +++ b/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 diff --git a/spec/features/school_student/batch_deleting_school_students_spec.rb b/spec/features/school_student/batch_deleting_school_students_spec.rb new file mode 100644 index 000000000..b70861a29 --- /dev/null +++ b/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 diff --git a/spec/features/school_student/deleting_a_school_student_spec.rb b/spec/features/school_student/deleting_a_school_student_spec.rb index f6cd818f2..04f188574 100644 --- a/spec/features/school_student/deleting_a_school_student_spec.rb +++ b/spec/features/school_student/deleting_a_school_student_spec.rb @@ -14,6 +14,12 @@ let(:student_id) { SecureRandom.uuid } let(:owner) { create(:owner, school:) } + it 'calls ProfileApiClient to delete the student from the profile service' do + student = create(:student, school:) + delete("/api/schools/#{school.id}/students/#{student.id}", headers:) + expect(ProfileApiClient).to have_received(:delete_school_student).with(token: UserProfileMock::TOKEN, school_id: school.id, student_id: student.id) + end + it 'creates the school owner safeguarding flag' do delete("/api/schools/#{school.id}/students/#{student_id}", headers:) expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner], email: owner.email) @@ -24,9 +30,9 @@ expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher], email: owner.email) end - it 'responds 204 No Content' do + it 'responds 200 OK' do delete("/api/schools/#{school.id}/students/#{student_id}", headers:) - expect(response).to have_http_status(:no_content) + expect(response).to have_http_status(:ok) end it 'responds 401 Unauthorized when no token is given' do diff --git a/spec/services/student_removal_service_spec.rb b/spec/services/student_removal_service_spec.rb index 5db27ad4a..00002488a 100644 --- a/spec/services/student_removal_service_spec.rb +++ b/spec/services/student_removal_service_spec.rb @@ -4,6 +4,7 @@ describe StudentRemovalService do let(:school) { create(:school) } + let(:other_school) { create(:school) } let(:owner) { create(:owner, school: school) } let(:teacher) { create(:teacher, school: school) } let(:student) { create(:student, school: school) } @@ -11,37 +12,179 @@ let(:service) { described_class.new(students: [student.id], school: school) } before do - allow(Project).to receive(:where).and_return([]) allow(ProfileApiClient).to receive(:delete_school_student) create(:class_student, student_id: student.id, school_class: school_class) end - it 'removes student from classes and roles' do - expect(Role.where(user_id: student.id, role: :student)).to exist - expect(ClassStudent.where(student_id: student.id)).to exist - results = service.remove_students - expect(results.first[:user_id]).to eq(student.id) - expect(results.first[:skipped]).to be_nil - expect(ClassStudent.where(student_id: student.id)).not_to exist - expect(Role.where(user_id: student.id, role: :student)).not_to exist - end + describe '#remove_students' do + context 'when student has a role in the school' do + it 'removes student from classes, roles, and deletes all projects' do + school_project = create(:project, user_id: student.id, school: school) + personal_project = create(:project, user_id: student.id, school: nil) - it 'skips removal if student has projects' do - allow(Project).to receive(:where).and_return([instance_double(Project)]) - results = service.remove_students - expect(results.first[:skipped]).to be true - end + expect(Role.where(user_id: student.id, role: :student, school_id: school.id)).to exist + expect(ClassStudent.where(student_id: student.id)).to exist + expect(Project.where(user_id: student.id)).to exist - it 'calls ProfileApiClient if remove_from_profile is true and token is present' do - token = 'abc123' - service = described_class.new(students: [student.id], school: school, remove_from_profile: true, token: token) - service.remove_students - expect(ProfileApiClient).to have_received(:delete_school_student).with(token: token, school_id: school.id, student_id: student.id) - end + results = service.remove_students + + expect(results.first[:user_id]).to eq(student.id) + expect(results.first[:error]).to be_nil + expect(ClassStudent.where(student_id: student.id)).not_to exist + expect(Role.where(user_id: student.id, role: :student, school_id: school.id)).not_to exist + expect(Project.exists?(school_project.id)).to be false + expect(Project.exists?(personal_project.id)).to be false + end + + it 'only deletes class assignments for the current school' do + # Create a different student for the other school + other_student = create(:student, school: other_school) + other_school_class = create(:school_class, school: other_school) + other_class_assignment = create(:class_student, student_id: other_student.id, school_class: other_school_class) + + # Original student should be removed from this school only + results = service.remove_students + + expect(results.first[:error]).to be_nil + expect(ClassStudent.joins(:school_class).where(student_id: student.id, school_class: { school_id: school.id })).not_to exist + # Other student's assignment should remain + expect(ClassStudent.exists?(other_class_assignment.id)).to be true + end + + it 'only deletes roles for the current school' do + # Create a different student for the other school + other_student = create(:student, school: other_school) + + results = service.remove_students + + expect(results.first[:error]).to be_nil + expect(Role.where(user_id: student.id, role: :student, school_id: school.id)).not_to exist + # Other student's role should remain + expect(Role.where(user_id: other_student.id, school_id: other_school.id)).to exist + end + + it 'deletes all projects for the user regardless of school' do + school_project = create(:project, user_id: student.id, school: school) + personal_project = create(:project, user_id: student.id, school: nil) + + # Create a different student for the other school + other_student = create(:student, school: other_school) + other_school_project = create(:project, user_id: other_student.id, school: other_school) + + results = service.remove_students + + expect(results.first[:error]).to be_nil + expect(Project.exists?(school_project.id)).to be false + expect(Project.exists?(personal_project.id)).to be false + # Other student's project should remain + expect(Project.exists?(other_school_project.id)).to be true + end + end + + context 'when student does not have a role in the school' do + let(:student_without_role) { create(:user) } + let(:service) { described_class.new(students: [student_without_role.id], school: school) } + + it 'returns a skipped entry with reason' do + results = service.remove_students + + expect(results.length).to eq(1) + expect(results.first[:user_id]).to eq(student_without_role.id) + expect(results.first[:skipped]).to be true + expect(results.first[:reason]).to eq('no_role_in_school') + end + end + + context 'when processing multiple students' do + let(:second_student) { create(:student, school: school) } + let(:student_without_role) { create(:user) } + let(:service) { described_class.new(students: [student.id, second_student.id, student_without_role.id], school: school) } + + before do + create(:class_student, student_id: second_student.id, school_class: school_class) + end + + it 'processes students with roles and returns skipped entry for those without' do + create(:project, user_id: student.id, school: school) + create(:project, user_id: second_student.id, school: school) + + results = service.remove_students + + expect(results.length).to eq(3) + expect(results.pluck(:user_id)).to contain_exactly(student.id, second_student.id, student_without_role.id) + expect(Role.where(user_id: student.id, school: school, role: :student)).not_to exist + expect(Role.where(user_id: second_student.id, school: school, role: :student)).not_to exist + + skipped_result = results.find { |r| r[:user_id] == student_without_role.id } + expect(skipped_result[:skipped]).to be true + expect(skipped_result[:reason]).to eq('no_role_in_school') + end + end + + context 'with profile API integration' do + it 'calls ProfileApiClient if remove_from_profile is true and token is present' do + token = 'abc123' + service = described_class.new(students: [student.id], school: school, remove_from_profile: true, token: token) + service.remove_students + expect(ProfileApiClient).to have_received(:delete_school_student).with(token: token, school_id: school.id, student_id: student.id) + end + + it 'does not call ProfileApiClient if remove_from_profile is false' do + service = described_class.new(students: [student.id], school: school, remove_from_profile: false, token: 'token') + service.remove_students + expect(ProfileApiClient).not_to have_received(:delete_school_student) + end + + it 'does not call ProfileApiClient if token is not present' do + service = described_class.new(students: [student.id], school: school, remove_from_profile: true, token: nil) + service.remove_students + expect(ProfileApiClient).not_to have_received(:delete_school_student) + end + + it 'rolls back database changes if ProfileApiClient call fails' do + token = 'abc123' + project = create(:project, user_id: student.id, school: school) + + # Stub ProfileApiClient to raise an error + allow(ProfileApiClient).to receive(:delete_school_student).and_raise(StandardError, 'Profile API failure') + + service = described_class.new(students: [student.id], school: school, remove_from_profile: true, token: token) + results = service.remove_students + + # Should have error in result + expect(results.first[:error]).to match(/Profile API failure/) + + # Database changes should have been rolled back + expect(Project.exists?(project.id)).to be true + expect(ClassStudent.where(student_id: student.id)).to exist + expect(Role.where(user_id: student.id, school_id: school.id, role: :student)).to exist + end + end + + context 'when handling errors' do + it 'handles errors gracefully' do + allow(ClassStudent).to receive(:joins).and_raise(StandardError, 'fail') + results = service.remove_students + expect(results.first[:error]).to match(/StandardError: fail/) + end + + it 'continues processing other students after an error' do + second_student = create(:student, school: school) + service = described_class.new(students: [student.id, second_student.id], school: school) + + # Mock to raise error on first student's projects + projects_relation = instance_double(ActiveRecord::Relation) + allow(projects_relation).to receive(:destroy_all).and_raise(StandardError, 'fail') + allow(Project).to receive(:where).with(user_id: student.id).and_return(projects_relation) + allow(Project).to receive(:where).with(user_id: second_student.id).and_call_original + + results = service.remove_students - it 'handles errors gracefully' do - allow(ClassStudent).to receive(:where).and_raise(StandardError, 'fail') - results = service.remove_students - expect(results.first[:error]).to match(/StandardError: fail/) + expect(results.length).to eq(2) + expect(results.first[:error]).to match(/StandardError/) + # Second student should succeed + expect(Role.where(user_id: second_student.id, school: school, role: :student)).not_to exist + end + end end end