fix(automation): Mark survey reminders sent after sending emails, not before - #63
Merged
tylermenezes merged 1 commit intoSep 18, 2026
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Bug
emailDueSurveysReminder(a 5-minute cron task) emails mentors/students a "due soon" reminder when aSurveyOccurencebecomes visible and an "OVERDUE" reminder when it passesdueAt, tracking each viasentVisibleReminder/sentOverdueReminderso each fires at most once.For each selected occurrence the task persisted the flag before the per-recipient
email.sendMailloop, and the loop had notry/catch. WhensendMailrejected for one recipient (network error, SMTP 5xx, bad address), the throw aborted the rest of that occurrence's cohort and the rest of the batch, while the flag was alreadytrue. For the overdue arm this is permanent loss: oncesentOverdueReminderis set, the occurrence matches neither branch of the selection query and nothing resets the flag, so recipients ordered after the failing one never receive the final[Action Required]notice.Fix
In
src/automation/tasks/emailDueSurveysReminder.ts, per occurrence:email.sendMailin atry/catch(trackingsentAtLeastOne/sentAll), so one failing recipient no longer aborts the rest of the cohort or the rest of the batch.updateto after the send loop, with a per-arm predicate:falseand the occurrence is re-selected on the next tick (self-heals; accepts duplicate emails to already-delivered recipients as a bounded tradeoff).Zero-target occurrences are still marked on both arms (avoids re-query churn). The query, subject line, and template selection are unchanged.
Testing
emailDueSurveysReminder.test.ts, colocated and excluded from the cron loader) stubbing the Prisma client, nodemailer transport, and template loader — no DB, SMTP, or env required. It covers: a failing recipient not aborting the cohort on both arms; the overdue arm not marking on partial failure and recovering on the next tick (the durable-loss case); the visible arm marking on any success to avoid window duplicates; zero-target marking on both arms; a failing occurrence not aborting the rest of the batch; and the global "flag update always happens after sends" invariant.npx tsc --skipLibCheck --noEmit(incl. the new test file), the new unit tests under bothtsxandts-node, and the pre-existingsyncAlumniInteractions.test.tsregression suite.smtp-serverstub scriptable to reject specific recipients with550 5.1.1, seeded an active event with one visible and one overdue occurrence and three students, then ran the task across simulated ticks. Verified: the happy path sends 6 emails on tick 1 and 0 on tick 2; an overdue transient failure for one recipient leavessentOverdueReminder=falseand the recipient receives the overdue reminder on the next tick once the failure clears (no permanent loss); a visible-arm failure marks on any success and does not re-send during the window; a failing recipient in one occurrence does not abort the other occurrence in the batch; a permanently bad address keeps the occurrence unmarked and retrying while deliverable recipients still receive the reminder each tick. Reverting the fix reproduces the bug — the throw aborts the run andsentOverdueReminder=trueis persisted though the recipient never received the email — confirming the E2E is a valid regression test.npx eslint) could not be run: the repo's@typescript-eslint/parser@3.10.1is incompatible withtypescript@5.2.2and fails to parse every.tsfile (including untouched ones) with aDeprecationError: 'originalKeywordKind'. This is a pre-existing toolchain issue independent of this change, and lint is not part of the package's verification commands.Automatic Fixes PRs can be configured here.