pal-e-docs

Review: Generic email blast system with pluggable audience queries

review-993-2026-04-12 Review

review needs-refinement
Project

Verdict: APPROVED

Re-review of board item #993. Previous review flagged 5 items. 2 were valid disputes (arch-email and sop-email-send exist). 3 were claimed as fixed but are NOT present in the current issue body.

Disputed Items (Resolved)

  • [x] arch-email — EXISTS. Full architecture note with 6 sections (Overview, Components, Layouts, Email Flow, Preview & Approval, Decisions). Previous review's search_notes call returned zero spuriously. Confirmed via get_note_toc.
  • [x] sop-email-send — EXISTS (note id 1108). Full SOP with 5 sections (Purpose, Steps, Contract Emails gate, New Email Type Checklist, Rules). Confirmed via get_note_toc.

Claimed Fixes — NOT Applied

All 3 claimed fixes are absent from the current Forgejo issue #456 body (verified 2026-04-12 via WebFetch):
  • [ ] Migration number — still says alembic/versions/031_add_email_types.py. Local checkout tops at 030, remote main likely at 044+ (per #449 CrashLoopBackOff from migration 044). Should say NNN with instruction to determine next slot from remote main.
  • [ ] brand.py not in File Targets as a separate modify target. The description mentions "using brand.py tokens" inside the email.py bullet, but brand.py itself has zero pink color tokens (only COLOR_RED = "#d42026"). A COLOR_PINK constant must be added to brand.py for Queens branding, and brand.py must appear as its own file target.
  • [ ] Dependencies #457 and #448 not added to Related. Related section has only 4 bullets: project-westside-basketball, story:WS-S32, arch-email, sop-email-send. No cross-references to downstream consumers.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Standalone, discovered during Utah Invitational tournament fee billing
  • [x] Repo — forgejo_admin/basketball-api
  • [x] User Story — present, matches WS-S32
  • [x] Context — thorough, references existing endpoints and architecture
  • [x] File Targets — present with should-modify and should-not-touch
  • [x] Acceptance Criteria — 7 criteria, all testable
  • [x] Test Expectations — unit + integration + run command
  • [x] Constraints — present, includes Gmail inline-style requirement
  • [x] Checklist — present
  • [x] Related — present but incomplete (missing dependency cross-refs)

Traceability

  • [x] story:WS-S32 label — verified in project-westside-basketball Admin stories list
  • [x] story note verified — "As an admin, I want a generic email blast system with pluggable audience queries so that new email types ship as config, not code changes"
  • [x] arch:email label — present on board item
  • [x] arch note verified — arch-email exists with full architecture (6 sections)
  • [x] Forgejo issue — basketball-api#456, open

File Targets

  • [x] src/basketball_api/services/email_queries.py — NEW file, confirmed does not exist yet. Correct.
  • [x] src/basketball_api/services/email.py — verified exists. Has 9 send functions and _brand_wrapper as described.
  • [x] src/basketball_api/routes/admin.py — verified exists. Has 4 hardcoded email endpoints. No existing /email/blast endpoint.
  • [x] src/basketball_api/models.py — verified. EmailType enum at L63-70 with 7 values as described.
  • [ ] alembic/versions/031_add_email_types.py — ISSUE: Migration number 031 is wrong. Local checkout has up to 030. Remote main has 040-044 (per #449 context). Next slot is likely 045+. Must say NNN with agent instruction to determine from remote main.
  • [ ] brand.py — MISSING from file targets. brand.py has no pink tokens. Must be listed as a modify target with instruction to add COLOR_PINK for Queens branding.

Repo Placement

OK — issue filed on basketball-api, all file targets in basketball-api. Single repo.

Dependencies

  • #994 / basketball-api#457 (Tournament product creation) — downstream consumer of tournament_committed query. Not documented in Related.
  • #966 / basketball-api#448 (Add player_ids filter to blast endpoint) — depends on this ticket creating the blast endpoint. Not documented in Related.
  • #965 / basketball-api#447 (email_log.player_id NULL on blast sends) — related to blast sends. Dependency unclear but relevant.

Acceptance Criteria

7 AC, all agent-verifiable. Test commands are real. Same missing criteria as initial review:
  • No AC for alembic migration success (alembic upgrade head)
  • No AC for idempotency / double-send protection

Blast Radius

Same as initial review. EmailType enum expansion via alembic is the primary risk (migration number collision = CrashLoopBackOff). brand.py changes are safe — additive only.

Decomposition Assessment

No decomposition needed if refinements are applied. 5 file targets in one repo, 7 AC, single agent pass.

Recommendation

3 items remain from the initial review. The 2 disputed items are resolved (notes exist).
  • [BODY] Fix migration filename: replace 031 with NNN and instruct agent to determine next slot from remote main HEAD.
  • [BODY] Add src/basketball_api/brand.py as a separate file target: "Add COLOR_PINK token for Queens branding."
  • [BODY] Add dependency cross-references to Related: #457 (downstream consumer), #448 (depends on blast endpoint).