From 2875111c2769f8a4e349a2e29ad5ecb6788bab51 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Wed, 3 Sep 2025 11:23:38 +0100 Subject: [PATCH 1/2] 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. --- app/services/student_removal_service.rb | 83 ++++++----- lib/tasks/remove_students.rake | 33 +++-- spec/lib/tasks/remove_students_spec.rb | 130 ++++++++++++++++++ spec/services/student_removal_service_spec.rb | 110 +++++++++++---- 4 files changed, 282 insertions(+), 74 deletions(-) create mode 100644 spec/lib/tasks/remove_students_spec.rb diff --git a/app/services/student_removal_service.rb b/app/services/student_removal_service.rb index 0437fd3ba..212a8738a 100644 --- a/app/services/student_removal_service.rb +++ b/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 diff --git a/lib/tasks/remove_students.rake b/lib/tasks/remove_students.rake index 134e0f92e..9a2faaa21 100644 --- a/lib/tasks/remove_students.rake +++ b/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,7 +42,7 @@ 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.' @@ -48,28 +50,25 @@ namespace :remove_students do 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 diff --git a/spec/lib/tasks/remove_students_spec.rb b/spec/lib/tasks/remove_students_spec.rb new file mode 100644 index 000000000..a309b68bd --- /dev/null +++ b/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 diff --git a/spec/services/student_removal_service_spec.rb b/spec/services/student_removal_service_spec.rb index 5db27ad4a..de2ee6876 100644 --- a/spec/services/student_removal_service_spec.rb +++ b/spec/services/student_removal_service_spec.rb @@ -8,40 +8,98 @@ let(:teacher) { create(:teacher, school: school) } let(:student) { create(:student, school: school) } let(:school_class) { create(:school_class, teacher_ids: [teacher.id, owner.id], school: school) } - let(:service) { described_class.new(students: [student.id], school: school) } + let(:service) { described_class.new(school: school) } + let(:empty_school) { create(:school) } before do - allow(Project).to receive(:where).and_return([]) + allow(Project).to receive(:exists?).and_return(false) 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_student' do + 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 - 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 + service.remove_student(student.id) - 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 + expect(ClassStudent.where(student_id: student.id)).not_to exist + expect(Role.where(user_id: student.id, role: :student)).not_to exist + end + + it 'raises NoSchoolError when school is nil' do + service = described_class.new(school: nil) + + expect { service.remove_student(student.id) }.to raise_error( + StudentRemovalService::NoSchoolError, 'School not found' + ) + end + + it 'raises NoClassesError when school has no classes' do + service = described_class.new(school: empty_school) + + expect { service.remove_student(student.id) }.to raise_error( + StudentRemovalService::NoClassesError, 'School has no classes' + ) + end + + it 'raises StudentHasProjectsError when student has projects' do + allow(Project).to receive(:exists?).with(user_id: student.id).and_return(true) + + expect { service.remove_student(student.id) }.to raise_error( + StudentRemovalService::StudentHasProjectsError, 'Student has existing projects' + ) + end + + it 'raises NoopError when student has no roles or classes and raise_on_noop is true' do + student_without_associations = create(:student, school: empty_school) + service = described_class.new(school: school, raise_on_noop: true) + + expect { service.remove_student(student_without_associations.id) }.to raise_error( + StudentRemovalService::NoopError, 'Student has no roles or class assignments to remove' + ) + end + + it 'does not raise NoopError when remove_from_profile is true (even with raise_on_noop true)' do + student_without_associations = create(:student, school: empty_school) + service = described_class.new( + school: school, + raise_on_noop: true, + remove_from_profile: true, + token: 'abc123' + ) + + expect { service.remove_student(student_without_associations.id) }.not_to raise_error + end + + it 'calls ProfileApiClient when remove_from_profile is true and token is present' do + token = 'abc123' + service = described_class.new(school: school, remove_from_profile: true, token: token) + + service.remove_student(student.id) + + 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 when remove_from_profile is false' do + service = described_class.new(school: school, remove_from_profile: false, token: 'abc123') + + service.remove_student(student.id) + + expect(ProfileApiClient).not_to have_received(:delete_school_student) + end + + it 'does not call ProfileApiClient when token is missing' do + service = described_class.new(school: school, remove_from_profile: true, token: nil) + + service.remove_student(student.id) - 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(ProfileApiClient).not_to have_received(:delete_school_student) + end end end From 166fcf9963eac3af3a3efc28e5e64a8111bce120 Mon Sep 17 00:00:00 2001 From: Dan Halson Date: Mon, 8 Sep 2025 10:58:19 +0100 Subject: [PATCH 2/2] Change find_by to find for school lookup --- lib/tasks/remove_students.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/remove_students.rake b/lib/tasks/remove_students.rake index 9a2faaa21..beedfea21 100644 --- a/lib/tasks/remove_students.rake +++ b/lib/tasks/remove_students.rake @@ -13,7 +13,7 @@ namespace :remove_students do remove_from_profile = ENV.fetch('REMOVE_FROM_PROFILE', 'false') == 'true' token = ENV.fetch('TOKEN', nil) - school = School.find_by(id: school_id) + school = School.find(school_id) if school.nil? Rails.logger.error 'Please provide a valid school ID with SCHOOL_ID=your_school_id' exit 1