Improve student removal service #588

danhalson · 2025-09-03T10:31:43Z

Status

Closes https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/864

What's changed?

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.
Ensures only students from passed school are removed (previously this was implied from the user_ids passed)
Adds usage to top of task
More comprehensive tests also added.

Steps to perform after deploying to production

N/A

… 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.

Copilot

Pull Request Overview

This PR refactors the student removal service to process students individually rather than in batches, with improved error handling and more granular control. The main purpose is to make the service more adaptable and provide better error reporting.

Refactored StudentRemovalService to process one student at a time with proper exception handling
Updated the Rake task to handle iteration and error management externally
Added comprehensive validation and custom error classes for different failure scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
app/services/student_removal_service.rb	Completely refactored to process single students with custom exceptions and improved validation
lib/tasks/remove_students.rake	Updated to iterate over students and handle service exceptions with better output formatting
spec/services/student_removal_service_spec.rb	Comprehensive test rewrite covering new error scenarios and single-student processing
spec/lib/tasks/remove_students_spec.rb	New test file covering Rake task functionality including validation and error handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

app/services/student_removal_service.rb

Copilot · 2025-09-05T08:39:54Z

lib/tasks/remove_students.rake

+    school = School.find_by(id: school_id)
    if school.nil?


Using find_by here could be inconsistent with expectations. Since the code already handles the nil case explicitly on line 17-20, consider using School.find(school_id) and rescuing ActiveRecord::RecordNotFound instead, which would provide clearer intent and more specific error handling.

Suggested change

school = School.find_by(id: school_id)

if school.nil?

begin

school = School.find(school_id)

rescue ActiveRecord::RecordNotFound

Copilot · 2025-09-05T08:39:55Z

lib/tasks/remove_students.rake

+    rescue StandardError => e
+      results << "Student: #{student_id} | Error: #{e.message}"


Catching StandardError is too broad and could mask unexpected errors. Consider catching specific exceptions like StudentRemovalService::StudentHasProjectsError, StudentRemovalService::NoopError, etc., and re-raising unexpected errors to avoid silently handling system errors.

Suggested change

rescue StandardError => e

results << "Student: #{student_id} | Error: #{e.message}"

rescue StudentRemovalService::StudentHasProjectsError, StudentRemovalService::NoopError => e

results << "Student: #{student_id} | Error: #{e.message}"

rescue => e

raise

adrian-rpf

This all makes sense and some nice tweak improvements.
Worth looking at the Copilot suggestions some of them make sense.

patch0 · 2025-09-08T10:21:52Z

app/services/student_removal_service.rb

+      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?


Does anyone care if it is a noop? Seems a bit extra, and would simplify code if it didn't need to check this.

I added this because I was struggling without it, when running the task I had no idea if anything was actually being done when should_remove_from_profile was false, so I think it helps to get a summary of what has happened even if it's nothing

patch0 · 2025-09-08T10:23:03Z

app/services/student_removal_service.rb

-      results << result
+  # rubocop:disable Metrics/CyclomaticComplexity
+  def remove_student(student_id)
+    raise NoSchoolError, 'School not found' if @school.nil?


This might be better to go in the initializer. Also the error message isn't correct "School not found" isn't what's going on here. Maybe the caller (where the school lookup is happening) should be sorting this.

patch0 · 2025-09-08T10:26:45Z

app/services/student_removal_service.rb

+  # 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?


Is this something that is important to check? I don't think the caller cares if there are no classes. They might want the student removing from the school roles, or from profile.

patch0 · 2025-09-08T10:28:09Z

app/services/student_removal_service.rb

+  end
+
+  def should_remove_from_profile?
+    @remove_from_profile && @token.present?


I'd probably just move this into the remove_from_profile method, and return early if no token is given, or @remove_from_profile is set.

patch0 · 2025-09-08T10:29:33Z

spec/lib/tasks/remove_students_spec.rb

+      end
+
+      it 'exits when school is not found' do
+        ENV['SCHOOL_ID'] = 'non-existent-id'


Use ClimateControl to manage ENV in testing.

patch0 · 2025-09-08T10:37:22Z

app/services/student_removal_service.rb

+  class NoSchoolError < StandardError; end
+  class NoClassesError < StandardError; end
+  class StudentHasProjectsError < StandardError; end
+  class NoopError < StandardError; end


If you set up a BaseError class, then in the rake task you can just rescue anything

# Set up a BaseError class so we can rescue all errors BaseError = Class.new(StandardError) ## Specific errors NoSchoolError = Class.new(BaseError) NoClassesError = Class.new(BaseError) StudentHasProjectsError = Class.new(BaseError) NoopError = Class.new(BaseError)

then in the rake task

begin ... rescue StudentRemovalService::BaseError => e # whatever it is here end

patch0 · 2025-09-08T10:40:53Z

app/services/student_removal_service.rb

+  class StudentHasProjectsError < StandardError; end
+  class NoopError < StandardError; end
+
+  def initialize(school:, remove_from_profile: false, token: nil, raise_on_noop: false)


I guess a fundamental question is why is this asking for a school when the service is a "student removal service"? I'd suggest maybe renaming it to SchoolStudentsRemovalService or let it handle student IDs directly, and pass them in one at a time.

it's because you can only remove them from profile from a school context (i.e. by passing a school), and that is also how support requests will come in - a student is unique to the school, not globally

but yes, that's good call about altering the name - that'll help make it clearer

Updates the service to only process a single student at a time and to…

2875111

… 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.

cla-bot bot added the cla-signed label Sep 3, 2025

danhalson self-assigned this Sep 3, 2025

adrian-rpf requested a review from Copilot September 5, 2025 08:39

Copilot AI reviewed Sep 5, 2025

View reviewed changes

adrian-rpf previously approved these changes Sep 5, 2025

View reviewed changes

Change find_by to find for school lookup

166fcf9

danhalson dismissed adrian-rpf’s stale review via 166fcf9 September 8, 2025 09:58

patch0 reviewed Sep 8, 2025

View reviewed changes

		rescue StandardError => e
		results << "Student: #{student_id} \| Error: #{e.message}"

Improve student removal service #588

Are you sure you want to change the base?

Improve student removal service #588

Conversation

danhalson commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status

What's changed?

Steps to perform after deploying to production

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

adrian-rpf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patch0 Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danhalson commented Sep 3, 2025 •

edited

Loading

patch0 Sep 8, 2025 •

edited

Loading