Updates the service to only process a single student at a time and to raise on errors, allowing it to be more adaptable. Updated the task to handle iterating over students and passing to service, and handling errors, allowing better output. More comprehensive tests also added.

Author: danhalson

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)
+    school = School.find_by(id: 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