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..beedfea21 100644 --- a/lib/tasks/remove_students.rake +++ b/lib/tasks/remove_students.rake @@ -6,6 +6,8 @@ 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' @@ -13,7 +15,7 @@ namespace :remove_students do 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,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