pal-e-docs

Review: First monthly payment email + Stripe checkout for signed contracts

review-872-2026-04-06 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Standalone, scoped during 2026-04-06 session
  • [x] Repo — forgejo_admin/basketball-api
  • [x] User Story — clear parent-facing payment story
  • [x] Context — detailed: proration formula, redirect approach, email tone, deadline
  • [x] File Targets — 4 create, 4 modify, 2 explicitly excluded
  • [x] Acceptance Criteria — 9 testable criteria
  • [x] Test Expectations — 5 unit tests with run command
  • [x] Constraints — 6 constraints including pattern references
  • [x] Checklist — standard 3-item checklist
  • [x] Related — project, spec, and plan references
All required Feature template sections are present and well-populated.

Traceability

  • [x] story:WS-S7 label — "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)
  • [ ] arch:basketball-api — arch note MISSING. [SCOPE] Create architecture note arch-basketball-api for component basketball-api
  • [ ] arch:email — arch note MISSING. [SCOPE] Create architecture note arch-email for component email
  • [x] Forgejo issue — forgejo_admin/basketball-api#366, open

File Targets

  • [x] src/basketball_api/models.py — verified: ProductCategory enum at line 123 (no monthly yet), EmailType enum at line 63 (no first_payment yet), monthly_fee field on Player at line 239, contract_token at line 232, contract_status at line 226. All referenced patterns exist.
  • [x] src/basketball_api/routes/checkout.py — verified: file exists, will add new GET endpoint
  • [x] src/basketball_api/services/email.py — verified: _brand_wrapper() at line 365, multiple existing email functions follow same pattern
  • [x] src/basketball_api/routes/admin.py — verified: jersey-reminder blast pattern at line 847 with test_email param, good reference for new blast endpoint
  • [x] src/basketball_api/routes/webhooks.py (NOT touch) — verified: _handle_generic_order_completed at line 164, handles order checkout generically. Correct to exclude.
  • [x] alembic/versions/031_... (create) — verified: latest numbered migration is 030, so 031 is correct sequence
  • [x] tests/test_first_payment.py (create) — new file, OK
  • [x] tests/test_first_payment_email.py (create) — new file, OK
  • [x] tests/test_first_payment_blast.py (create) — new file, OK
All file targets verified. Targets are specific and actionable.

Repo Placement

OK. Issue filed on basketball-api, all file targets are in basketball-api. Single-repo scope. The westside-contracts proration formula is referenced for correctness but no cross-repo code changes needed.

Dependencies

  • [x] Stripe integration — satisfied (existing checkout.py patterns)
  • [x] Gmail OAuth — satisfied (existing email.py patterns)
  • [x] Contract signing flow — satisfied (contract_status and contract_token already on Player model)
  • [x] Generic order webhook handler — satisfied (_handle_generic_order_completed already handles this pattern)
No unresolved dependencies. No blocking board items in in_progress.

Acceptance Criteria

9 AC, all testable and specific:
  • Migration apply — testable via alembic upgrade
  • 302 redirect — HTTP status assertion
  • Invalid/unsigned token 404 — HTTP status assertion
  • Duplicate order 409 — HTTP status assertion
  • Proration formula with concrete values ($200→$165, $180→$150, $160→$135, null→$165) — unit test
  • Email HTML content — mock send, inspect output
  • Blast to signed contracts — DB query + send assertion
  • test_email filter — integration test
  • EmailLog entry — DB assertion
No ambiguous language. All AC are agent-verifiable.

Blast Radius

  • Adding enum values to ProductCategory and EmailType is additive — no existing queries break
  • New admin blast endpoint follows existing jersey-reminder pattern — consistent and low risk
  • Webhook handler explicitly excluded — good boundary
  • No downstream consumers affected (new feature, not modification)
  • Rollback: revert migration + remove new code. Straightforward.

Decomposition Assessment

NEEDS DECOMPOSITION.
  • 8 files total (4 create + 4 modify) — exceeds 3-file threshold
  • 9 acceptance criteria — exceeds 5 AC threshold
  • 5 test expectations across 3 test files
  • Estimated agent work: 10-15 minutes — exceeds 5-minute rule
Recommended 3-ticket split:
  • Ticket A: Migration + model enum changes. Create migration file, add monthly to ProductCategory, first_payment to EmailType, seed product. (1 create, 1 modify, 2 AC)
  • Ticket B: Checkout endpoint + tests. Add GET /checkout/first-payment with proration logic, Stripe session creation, 302 redirect. (1 modify, 1 create, 5 AC)
  • Ticket C: Email function + blast endpoint + tests. Add send_first_payment_email(), admin blast endpoint, EmailLog writing. (2 modify, 2 create, 3 AC)
Tickets A and B+C have a dependency: B and C depend on A (enum values and product must exist). B and C are independent of each other. Route to skill-decompose-ticket.

Recommendation

  • [SCOPE] Create architecture note arch-basketball-api for component basketball-api
  • [SCOPE] Create architecture note arch-email for component email
  • [DECOMPOSE] 9 AC across 8 files, route to skill-decompose-ticket. Suggested 3-ticket split: (A) migration+models, (B) checkout endpoint+tests, (C) email+blast+tests. A is prerequisite; B and C can parallelize.