Refactor Inactive Teacher Deletion Warning Emailer #69534
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the inactive teacher deletion warning email functionality by extracting the business logic from an ActiveJob into a dedicated service class (InactiveTeacherDeletionWarningEmailer). The job now serves as a thin wrapper that delegates to the new service class.
Key changes:
- Created
InactiveTeacherDeletionWarningEmailerservice class with configurable dry-run mode and limit parameters - Added batching logic (1,000 records at a time) to process teachers incrementally instead of loading all at once
- Updated tests to test the service class directly rather than through the job
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
dashboard/lib/inactive_teacher_deletion_warning_emailer.rb |
New service class containing the extracted email sending logic with batching, dry-run support, and configurable limits |
dashboard/app/jobs/user/inactive_teacher_deletion_warning_job.rb |
Simplified job that now delegates to the new emailer service class |
dashboard/test/lib/services/user/inactive_teacher_deletion_warning_emailer_test.rb |
Updated tests to test the service class directly with new test cases for dry-run mode and metrics tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dashboard/app/jobs/user/inactive_teacher_deletion_warning_job.rb
Outdated
Show resolved
Hide resolved
| rescue StandardError => exception | ||
| report_exception(exception) |
There was a problem hiding this comment.
report_exception call will raise NoMethodError
The new mailer rescues StandardError and calls report_exception, but this plain service object no longer mixes in ActiveJobReporting (where that helper lives) nor defines its own report_exception. Any runtime error during email processing will therefore trigger a NoMethodError inside the rescue block, skipping error reporting and surfacing a different exception instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this might be a valid comment, worth testing it out
There was a problem hiding this comment.
Does the report_exception call still need to be removed or changed? Instead of rescuing do we just allow the error to bubble up and add rescue_from StandardError, with: :report_exception back into the ActiveJob? Haven't tested this out yet to know if it works or not.
Refactors the Inactive Teacher Deletion Warning Job by extracting the core logic into
dashboard/lib/inactive_teacher_deletion_warning_mailer.rb. This keeps the ApplicationJob lightweight.Additionally, this update introduces two new options to the mailer:
dry_run: allows running the job without sending emails or writing to the database.
limit: caps the number of teachers processed in a single run.
Links