Be careful to only delete students with a role in the current school.

Author: fspeirs

app/services/student_removal_service.rb

@@ -11,21 +11,25 @@ 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)
+      next if student_roles.empty?
+
       result = { user_id: }
       begin
         ActiveRecord::Base.transaction do
-          # Delete all projects
+          # Delete all projects for this user
           projects = Project.where(user_id: user_id)
           projects.destroy_all
 
           # Remove from classes
-          class_assignments = ClassStudent.where(student_id: user_id)
+          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
+          student_roles.destroy_all
         end
 
         # Remove from profile if requested

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) }
@@ -15,32 +16,149 @@
     create(:class_student, student_id: student.id, school_class: school_class)
   end
 
-  it 'removes student from classes, roles, and deletes all projects' do
-    create(:project, user_id: student.id)
+  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)
 
-    expect(Role.where(user_id: student.id, role: :student)).to exist
-    expect(ClassStudent.where(student_id: student.id)).to exist
-    expect(Project.where(user_id: student.id)).to exist
+        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
 
-    results = service.remove_students
+        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)).not_to exist
-    expect(Project.where(user_id: student.id)).not_to exist
-  end
+        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 '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 '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 'skips the student and does not remove anything' do
+        results = service.remove_students
+
+        expect(results).to be_empty
+      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 skips 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(2)
+        expect(results.pluck(:user_id)).to contain_exactly(student.id, second_student.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
+      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
+    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