pal-e-docs

Review: Generic send_templated_email() + EmailType migration

review-751-2026-04-03 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — references MJML email system ticket dependency
  • [x] Repo — forgejo_admin/basketball-api
  • [x] User Story — clear developer-facing enabler story
  • [x] Context — thorough explanation of current pattern and rationale for change
  • [x] File Targets — 3 specific paths with "Files NOT to touch" section
  • [x] Acceptance Criteria — 6 testable conditions
  • [x] Test Expectations — 4 unit tests with run command
  • [x] Constraints — function signature, OAuth, template rendering, plaintext fallback
  • [x] Checklist — 4 items
  • [x] Related — links to project page and MJML dependency
All required sections for Feature type are present and filled.

Traceability

  • [x] story:WS-S7 — "As an admin, I want to send branded email announcements so that parent comms are professional and consistent" — verified in project-westside-basketball user-stories section (Admin list)
  • [x] story:WS-S22 — "As a parent, I want to receive clear email communications with action links so that I never miss a deadline" — verified in project-westside-basketball user-stories section (Parent list)
  • [x] arch:email — references email architecture component
  • [ ] arch note MISSING — no arch-email note found in pal-e-docs via search_notes. [SCOPE] Create architecture note arch-email for the email subsystem.
  • [x] Forgejo issue — forgejo_admin/basketball-api#294, state: open

File Targets

  • [x] src/basketball_api/services/email.py — verified exists. Contains 7 existing send functions (lines 65-1342). load_email_template() at line 1107 and get_gmail_client() at line 29 both confirmed present. Constraints references are accurate.
  • [x] src/basketball_api/models.py — verified exists. EmailType enum at line 63 with 7 values (registration, admin_registration, reminder, roster_export, announcement, contract_signed, interest_notification). No contract_offer or contract_reminder yet — ticket is accurate.
  • [x] alembic/versions/ — verified exists. 30 migrations present. Pattern for adding enum values established in 027_add_email_type_enum_values.py (uses ALTER TYPE emailtype ADD VALUE IF NOT EXISTS). Next migration would be 031.
All targets are specific enough for an agent to act on without guessing.

Repo Placement

OK. Issue filed on forgejo_admin/basketball-api, all file targets are in that repo. Single-repo change, no cross-repo scope.

Dependencies

  • [x] Issue #293 / board item #750 (MJML email system) — status: pending (backlog, open). Lineage section references this. send_templated_email() calls load_email_template() which reads compiled HTML from disk. Templates must exist for integration testing, but unit tests can mock the template loader. Soft dependency: not blocking code merge, but blocking production use.
  • [x] Issue #295 / board item #752 (Admin blast endpoint) — status: downstream consumer. Correctly identified as separate ticket in File Targets ("blast endpoint is a separate ticket").
  • [x] Board item #724 (Contract reminder email endpoint) — in needs_approval column. Future consumer of send_templated_email() + EmailType.contract_reminder.
No unresolved blocking dependencies. Execution order is correct.

Acceptance Criteria

  • [x] AC1: Function signature is concrete with all parameters specified. Testable via unit test.
  • [x] AC2: EmailLog fields are well-defined in the models (tenant_id, parent_id, player_id, email_type, recipient_email, gmail_message_id). Testable.
  • [x] AC3: Plain text fallback algorithm specified in Constraints ("strip HTML tags, preserve link URLs"). Testable.
  • [x] AC4: Enum values are specific strings. Testable via assertion on enum members.
  • [x] AC5: Alembic up+down testable via alembic upgrade/downgrade. Note: downgrade for enum values is a no-op per PostgreSQL limitation — consistent with migration 027 pattern.
  • [x] AC6: "Existing functions unchanged" — testable by running full test suite before and after.
All AC are testable and specific. No ambiguous language.

Blast Radius

  • Additive only — no existing functions modified. Low blast radius.
  • Alembic enum migration uses ADD VALUE IF NOT EXISTS — safe, idempotent, proven pattern from migration 027.
  • No downstream consumers yet — blast endpoint (#295) and contract emails are separate tickets.
  • Rollback: straightforward. New function is unused until a caller is added. Enum values cannot be removed from PostgreSQL but are harmless if unused.

Decomposition Assessment

Apply the three-thing limit and five-minute rule:
  • 3 discrete changes: (1) new function in email.py, (2) enum values in models.py, (3) alembic migration. Exactly at the 3-thing limit.
  • 6 AC + 4 test expectations. All in one repo, one cohesive feature.
  • Estimated agent time: ~5 minutes. At the boundary but acceptable — the work is cohesive and cannot be meaningfully parallelized (the function depends on the enum values, tests depend on both).
No decomposition needed.

Recommendation

  • [SCOPE] Create architecture note arch-email for the email subsystem. This note should document: send functions, EmailType enum, EmailLog model, template loading via load_email_template(), Gmail OAuth via get_gmail_client(), and the MJML compiled template pipeline. Referenced by 8+ board items with arch:email label — the backing note is overdue.