Still notify participants when a legacy series occurrence cannot be addressed - #386
Merged
Merged
Conversation
…ddressed Production surfaced this as a warning on a real cancellation: Skipping cancellation for session 2d4a5c79-...: series member has no ical_recurrence_id A series session materialized before ical_recurrence_id existed has no valid RECURRENCE-ID, so no invite can address its occurrence. All three notify paths treated that as a reason to send nothing at all, so the coach and coachee were told nothing when their session was scheduled, moved, or cancelled. Those are two different questions. Whether a calendar event can be addressed is about the .ics; whether a human should be told their session moved is not. The guard conflated them because the attachment was built into the send rather than carried as an optional enclosure. The email now always goes out; only the attachment is withheld, via a new SendEmailRequestBuilder::add_optional_ics_attachment mirroring the existing add_optional_variable. Withholding it is still correct: those series were never published to a calendar, so a CANCEL naming the series UID would act on the wrong instance or on nothing at all. Scope, measured against production: 47 future sessions across 6 series, 6 coachees and 2 coaches, draining on 2026-12-28 as those series end. No new session can enter this state. Tested at two levels, because the full notify path cannot be mocked end to end (user::find_by_id uses find_with_related, which MockDatabase cannot express). One test drives the send and asserts the payload carries no attachment; three drive the notify entry points and assert the transaction log is non-empty, the exact inverse of the assertion the old behavior required. The first version of this covered only the send, and stayed green when the early return was restored; the notify-level tests were added after that mutation check exposed the gap.
Contributor
Greptile SummaryThe PR preserves lifecycle email delivery for legacy recurring-session occurrences that lack a calendar occurrence identifier while withholding only the unusable
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| domain/src/emails.rs | Removes legacy-occurrence notification short-circuits and centralizes the decision to omit only an unaddressable calendar attachment. |
| domain/src/emails_tests.rs | Adds discriminating request-count and attachment-absence coverage for all three lifecycle sends and fixes the prior file-scope import violation. |
| domain/src/gateway/resend.rs | Adds a small optional attachment builder method that preserves existing attachment behavior when an .ics body is present. |
Reviews (2): Last reviewed commit: "refactor(emails): share the invite-or-no..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Both from review.
The transaction-log tests only proved the notify entry points stopped
short-circuiting; they never proved an email actually leaves with no attachment,
and the scheduled and rescheduled flows had no send-level coverage at all. Adds
a test driving both of those sends for a series member whose occurrence cannot
be addressed, asserting four sends and no `attachments` key on any of them. It
was confirmed to fail when the unaddressable arm is made to attach an invite
anyway. The cancellation flow already had its equivalent.
Those sends turn out to be testable after all: they take the coach and coachee
directly, so they never reach `user::find_by_id` and its `find_with_related`,
which is what blocks mocking the notify path end to end.
Also hoists 17 identical `use sea_orm::{DatabaseBackend, MockDatabase}` imports
out of function bodies to the top of the file, where the coding standards
require them. Two of those were new here; moving only those would have left the
file inconsistent, and they were all the same import.
Review question: the invite and cancellation orchestrators were carrying the same three-arm match verbatim, differing only in which pair of builders it called. Extracted as build_session_ics, which takes the two builders as closures. It owns the whole decision in one place: a session inside a series is addressed as an override, a standalone one by its own UID, and a series member predating ical_recurrence_id has neither so it goes out with no attachment. `description` is threaded through to whichever builder runs, so it moves exactly once rather than needing a clone to satisfy both closures. Also finishes an inconsistency from #384: send_session_cancelled_email_to_recipient still took recipient, other_user and other_user_role as three positional arguments while its sibling took a Recipient. It only escaped the earlier change because it sat one argument under the clippy limit, not because the transposition hazard was any smaller. Both now take Recipient. No behavior change.
github-project-automation
Bot
moved this from Review
to ✅ Done
in Refactor Coaching Platform
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
1 participant
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.
Description
A coaching session that belongs to a series created before
.icssupport shipped has noical_recurrence_id, so no invite can name its occurrence. All three notification paths treated that as a reason to send nothing at all — the coach and coachee were told nothing when such a session was scheduled, rescheduled, or cancelled.Surfaced in production on a real cancellation:
Follow-up to #384
The confusion
Two different questions had been collapsed into one:
They got merged because the
.icswas built into the send rather than carried as an optional enclosure. So "no valid attachment" silently became "no email."Changes
SendEmailRequestBuilder::add_optional_ics_attachment, mirroring the existingadd_optional_variableidiom.notify_session_scheduled/_rescheduled/_cancelledare gone. The decision moved into the two single-session orchestrators, which now buildOption<String>and log at the point the attachment is dropped rather than at the point the whole notification is abandoned.CANCELnaming the seriesUIDwould address the wrong instance. Series-level paths are untouched — they address the seriesUIDdirectly and never needed aRECURRENCE-ID.Measured scope (production, read-only query)
No new session can enter this state;
bulk_create_recurringhas populatedical_recurrence_idsince #384.Database Migration
None. No schema change, no backfill. Backfilling was considered and rejected: it would invent a
RECURRENCE-IDfor occurrences of a recurring event that was never published to a calendar, so the address would point at nothing.Testing Strategy
Automated. Mock suite green (domain 308 / entity_api 287 / web 162), both clippy invocations clean,
cargo fmt --checkclean.Covered at two levels, deliberately, because the full notify path cannot be mocked end to end —
user::find_by_idusesfind_with_related, whichMockDatabasecannot express:send_session_cancelled_emailand asserts the Resend payload carries noattachmentskey at all.notify_*entry points and assert the transaction log is non-empty — the exact inverse of what the old behavior required.The first version of this covered only the send. Restoring the early return left it green, because it bypassed the guard entirely. The notify-level tests exist because that mutation check exposed the gap, and each was confirmed to fail with the early return put back.
Concerns
.icsonce before fanning out to both recipients. It now readsSending email without an invite for session ... predates ical_recurrence_idrather thanSkipping cancellation ..., which is the material difference for an operator: previously nobody was told anything and manual follow-up was warranted; now only the calendar is out of date. Kept atWARNbecause the session remains permanently unable to carry a calendar update.