Improve student removal service

app/services/student_removal_service.rb

@@ -1,42 +1,63 @@
 # frozen_string_literal: true
 
 class StudentRemovalService
-  def initialize(students:, school:, remove_from_profile: false, token: nil)
-    @students = students
+  class NoSchoolError < StandardError; end
+  class NoClassesError < StandardError; end
+  class StudentHasProjectsError < StandardError; end
+  class NoopError < StandardError; end
+
+  def initialize(school:, remove_from_profile: false, token: nil, raise_on_noop: false)
     @school = school
     @remove_from_profile = remove_from_profile
     @token = token
+    @raise_on_noop = raise_on_noop
   end
 
-  # Returns an array of hashes, one per student, with details of what was removed
-  def remove_students
-    results = []
-    @students.each do |user_id|
-      result = { user_id: }
-      begin
-        # Skip if student has projects
-        projects = Project.where(user_id: user_id)
-        result[:skipped] = true if projects.length.positive?
-
-        unless result[:skipped]
-          ActiveRecord::Base.transaction do
-            # 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?
-        end
-      rescue StandardError => e
-        result[:error] = "#{e.class}: #{e.message}"
-      end
-      results << result
+  # rubocop:disable Metrics/CyclomaticComplexity
+  def remove_student(student_id)
+    raise NoSchoolError, 'School not found' if @school.nil?
+    raise NoClassesError, 'School has no classes' if @school.classes.empty?
+    raise StudentHasProjectsError, 'Student has existing projects' if Project.exists?(user_id: student_id)
+
+    ActiveRecord::Base.transaction do
+      roles_destroyed = remove_roles(student_id)
+      classes_destroyed = remove_from_classes(student_id)
+      remove_from_profile(student_id) if should_remove_from_profile?
+
+      raise NoopError, 'Student has no roles or class assignments to remove' if roles_destroyed.zero? && classes_destroyed.zero? && should_raise_on_noop?
     end
-    results
+  end
+  # rubocop:enable Metrics/CyclomaticComplexity
+
+  private
+
+  def remove_from_classes(student_id)
+    ClassStudent.where(
+      school_class_id: @school.classes.pluck(:id),
+      student_id: student_id
+    ).destroy_all.length
+  end
+
+  def remove_roles(student_id)
+    Role.student.where(
+      user_id: student_id,
+      school_id: @school.id
+    ).destroy_all.length
+  end
+
+  def remove_from_profile(student_id)
+    ProfileApiClient.delete_school_student(
+      token: @token,
+      school_id: @school.id,
+      student_id: student_id
+    )
+  end
+
+  def should_remove_from_profile?
+    @remove_from_profile && @token.present?
+  end
+
+  def should_raise_on_noop?
+    @raise_on_noop && !should_remove_from_profile?
   end
 end

lib/tasks/remove_students.rake

@@ -6,14 +6,16 @@ require 'csv'
 namespace :remove_students do
   desc 'Remove students listed in a CSV file'
   task run: :environment do
+    Rails.logger.level = Logger::WARN
+
     students = []
     school_id = ENV.fetch('SCHOOL_ID', nil)
     remove_from_profile = ENV.fetch('REMOVE_FROM_PROFILE', 'false') == 'true'
     token = ENV.fetch('TOKEN', nil)
 
     school = School.find(school_id)
     if school.nil?
-      Rails.logger.error 'Please provide a school ID with SCHOOL_ID=your_school_id'
+      Rails.logger.error 'Please provide a valid school ID with SCHOOL_ID=your_school_id'
       exit 1
     end
 
@@ -40,36 +42,33 @@ namespace :remove_students do
     puts "Students to remove: #{students.size}"
     puts "====================\n\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): '
+    puts 'Are you sure you want to continue? (yes/no): '
     confirmation = $stdin.gets.strip.downcase
     unless confirmation == 'yes'
       puts 'Aborted. No students were removed.'
       exit 0
     end
 
     service = StudentRemovalService.new(
-      students: students,
       school: school,
       remove_from_profile: remove_from_profile,
-      token: token
+      token: token,
+      raise_on_noop: true
     )
 
-    results = service.remove_students.map do |res|
-      if res[:error]
-        "Student: #{res[:user_id]} | Error: #{res[:error]}"
-      elsif res[:skipped]
-        "Student: #{res[:user_id]} | Skipped: has project(s)"
-      else
-        "Student: #{res[:user_id]} | Removed successfully"
-      end
+    results = []
+    students.each do |student_id|
+      service.remove_student(student_id)
+      results << "Student: #{student_id} | Removed successfully"
+    rescue StandardError => e
+      results << "Student: #{student_id} | Error: #{e.message}"
     end
 
     puts "\n===================="
-    puts "Student removal results summary\n"
-    puts "SCHOOL: #{school.name} (#{school.id})"
-    puts "REMOVE_FROM_PROFILE: #{remove_from_profile}"
+    puts "Results\n"
+    puts "Students processed: #{results.size}"
     puts '===================='
-    results.each { |summary| puts summary }
+    results.each { |result| puts result }
     puts "====================\n\n"
   end
 end

spec/lib/tasks/remove_students_spec.rb

@@ -0,0 +1,130 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+require 'rake'
+require 'tempfile'
+
+RSpec.describe 'remove_students', type: :task do
+  describe ':run' do
+    let(:task) { Rake::Task['remove_students:run'] }
+    let(:school) { create(:school) }
+    let(:student_1) { create(:student, school: school) }
+    let(:student_2) { create(:student, school: school) }
+    let(:mock_service) { instance_double(StudentRemovalService) }
+
+    before do
+      # Clear the task to avoid "already invoked" errors
+      task.reenable
+
+      # Mock the confirmation input to avoid interactive prompts
+      allow($stdin).to receive(:gets).and_return("yes\n")
+
+      # Mock StudentRemovalService to avoid actual student removal
+      allow(StudentRemovalService).to receive(:new).and_return(mock_service)
+      allow(mock_service).to receive(:remove_student)
+
+      # Silence console output during tests
+      allow($stdout).to receive(:puts)
+      allow($stdout).to receive(:print)
+      allow(Rails.logger).to receive(:error)
+    end
+
+    describe 'envvar validation' do
+      it 'exits when SCHOOL_ID is missing' do
+        expect { task.invoke }.to raise_error(SystemExit)
+      end
+
+      it 'exits when school is not found' do
+        ENV['SCHOOL_ID'] = 'non-existent-id'
+
+        expect { task.invoke }.to raise_error(SystemExit)
+      ensure
+        ENV.delete('SCHOOL_ID')
+      end
+
+      it 'exits when neither CSV nor STUDENTS is provided' do
+        ENV['SCHOOL_ID'] = school.id
+
+        expect { task.invoke }.to raise_error(SystemExit)
+      ensure
+        ENV.delete('SCHOOL_ID')
+      end
+    end
+
+    describe 'CSV handling' do
+      let(:csv_file) { Tempfile.new(['students', '.csv']) }
+
+      before do
+        ENV['SCHOOL_ID'] = school.id
+      end
+
+      after do
+        csv_file.close
+        csv_file.unlink
+        ENV.delete('SCHOOL_ID')
+        ENV.delete('CSV')
+      end
+
+      it 'exits when CSV file does not exist' do
+        ENV['CSV'] = '/non/existent/file.csv'
+
+        expect { task.invoke }.to raise_error(SystemExit)
+      end
+
+      it 'processes valid CSV file' do
+        csv_file.write("user_id\n#{student_1.id}\n#{student_2.id}\n")
+        csv_file.rewind
+        ENV['CSV'] = csv_file.path
+
+        expect { task.invoke }.not_to raise_error
+        expect(mock_service).to have_received(:remove_student).with(student_1.id)
+        expect(mock_service).to have_received(:remove_student).with(student_2.id)
+      end
+    end
+
+    describe 'STUDENTS handling' do
+      before do
+        ENV['SCHOOL_ID'] = school.id
+      end
+
+      after do
+        ENV.delete('SCHOOL_ID')
+        ENV.delete('STUDENTS')
+      end
+
+      it 'processes csv student list' do
+        ENV['STUDENTS'] = "#{student_1.id}, #{student_2.id}"
+
+        expect { task.invoke }.not_to raise_error
+        expect(mock_service).to have_received(:remove_student).with(student_1.id)
+        expect(mock_service).to have_received(:remove_student).with(student_2.id)
+      end
+    end
+
+    describe 'user confirmation' do
+      before do
+        ENV['SCHOOL_ID'] = school.id
+        ENV['STUDENTS'] = student_1.id
+      end
+
+      after do
+        ENV.delete('SCHOOL_ID')
+        ENV.delete('STUDENTS')
+      end
+
+      it 'exits when user does not confirm' do
+        allow($stdin).to receive(:gets).and_return("no\n")
+
+        expect { task.invoke }.to raise_error(SystemExit)
+        expect(mock_service).not_to have_received(:remove_student)
+      end
+
+      it 'proceeds when user confirms with "yes"' do
+        allow($stdin).to receive(:gets).and_return("yes\n")
+
+        expect { task.invoke }.not_to raise_error
+        expect(mock_service).to have_received(:remove_student).with(student_1.id)
+      end
+    end
+  end
+end