pal-e-docs

Review Pass 2: #498 Migrate monthly-fee flow to Payment Links (analog of #494)

review-1032-2026-04-17-pass-2 Review

review approved
Project

Verdict: APPROVED

Pass-2 review of basketball-api#498. All three pass-1 NEEDS_REFINEMENT deltas have landed in the issue body as first-class acceptance criteria, constraints, test expectations, and checklist items — not as narrative asides. No scope creep beyond the required deltas plus the mint-site citation and the #497 related-link add. The email-URL architectural question is now locked in as a constraint, not left open.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — carries pass-1 provenance + pointer to `review-1032-2026-04-17`
  • [x] Repo — `forgejo_admin/basketball-api`
  • [x] User Story — parent-framed
  • [x] Context — now carries the three explicit monthly-vs-tournament deltas
  • [x] File Targets — "dev agent's decision" with verified mint-site citation (`routes/checkout.py:305`)
  • [x] Acceptance Criteria — 8 bullets, all testable
  • [x] Test Expectations — real-Stripe integration test mandated, mock + deactivation + `setup_future_usage` coverage all named
  • [x] Constraints — includes new "do not embed Stripe URLs in email" constraint
  • [x] Checklist — 7 items, four directly enforcing pass-1 deltas
  • [x] Related — `#494`, `#486`, `#493`, `#497`, memory `feedback_retrieve_before_theorize.md`

Pass-1 Delta Coverage

Delta 1 — `setup_future_usage: "off_session"` preservation

  • [x] Context — item 2: "`setup_future_usage: "off_session"` is required on the monthly flow for card-save on recurring billing. The existing Session call sets this via `payment_intent_data` (checkout.py:423). #494's tournament helper does NOT set this, so a straight copy will silently break recurring charges."
  • [x] Acceptance Criteria — bullet: "`setup_future_usage: "off_session"` carried over from the existing Session call. The regression test `tests/test_first_payment.py::test_setup_future_usage_passed_to_stripe` must continue to pass (and ideally an analogous assertion added for the Payment Link path)."
  • [x] Test Expectations — bullet: "`setup_future_usage` presence asserted on the Payment Link path (extend or add alongside `tests/test_first_payment.py::test_setup_future_usage_passed_to_stripe`)."
  • [x] Checklist — bullet: "`setup_future_usage` carry-over explicitly asserted."
  • [x] Live-code anchor verified — `payment_intent_data={"setup_future_usage": "off_session"}` sits at `src/basketball_api/routes/checkout.py:423` on origin/main. Regression test `tests/test_first_payment.py::test_setup_future_usage_passed_to_stripe` exists at line 477.

Delta 2 — Webhook deactivation gate widening (additive, not replacement)

  • [x] Acceptance Criteria — bullet: "Webhook deactivation gate widened: `webhooks.py:246-267` currently deactivates Payment Links only when `product.category == ProductCategory.tournament`. Widen to include `ProductCategory.monthly` WITHOUT removing the tournament branch — jersey/tryout/generic must continue to skip deactivation."
  • [x] Test Expectations — bullet: "Deactivation on payment asserted for monthly category (and existing tournament assertion still green)."
  • [x] Checklist — bullet: "Webhook deactivation gate widened to include monthly, tournament branch still intact."
  • [x] Live-code anchor verified — the gate at `src/basketball_api/routes/webhooks.py:246-267` on origin/main matches exactly: `if (payment_link_id and product is not None and product.category == ProductCategory.tournament)`. Widening to include monthly without removing tournament is the correct transform.

Delta 3 — Email-URL architectural choice, stated as a constraint

  • [x] Context — item 3 is unambiguous: "Keep the redirect — the `first_payment_checkout` endpoint should return the persisted Payment Link URL from `Order.stripe_checkout_url`, preserving the contract-status re-check that happens on click. Do NOT embed `https://buy.stripe.com/...` directly in the email template."
  • [x] Acceptance Criteria — bullet: "Email continues to embed `{base_url}/checkout/first-payment?token=...` — no change to the email template. The redirect endpoint returns the persisted Payment Link URL from `Order.stripe_checkout_url`."
  • [x] Constraints — bullet: "Do not embed Stripe URLs directly in the email template — keep the `/checkout/first-payment?token=...` redirect so contract-status re-check on click is preserved."
  • [x] Checklist — bullet: "Email template unchanged (redirect preserved)."
  • [x] Live-code anchor verified — `src/basketball_api/services/email.py` still builds `checkout_url = f"{settings.base_url}/checkout/first-payment?token={player.contract_token}"` (around the lines the pass-1 review cited). The body's instruction matches the live shape.
  • [x] Decision is phrased as a directive ("Keep the redirect…Do NOT embed"), not as a question or an option list. Dev agent has no ambiguity.

Traceability

  • [x] story:WS-S11 label — verified on project-westside-basketball Admin section (pass 1 verified)
  • [x] arch:dataflow-westside-basketball label — arch-dataflow-westside-basketball note exists (pass 1 verified)
  • [x] Forgejo issue — `forgejo_admin/basketball-api#498`, open, title unchanged

File Targets

  • [x] `src/basketball_api/routes/checkout.py:305` — `first_payment_checkout` endpoint (sole monthly-fee mint path) — verified present on origin/main
  • [x] `src/basketball_api/routes/checkout.py:423` — `payment_intent_data={"setup_future_usage": "off_session"}` — verified present
  • [x] `src/basketball_api/routes/webhooks.py:246-267` — tournament-only deactivation gate — verified present, ready to widen
  • [x] `src/basketball_api/services/email.py` (~line 1041) — `{base_url}/checkout/first-payment?token=...` URL construction — verified present
  • [x] `tests/test_first_payment.py::test_setup_future_usage_passed_to_stripe` — regression test exists (line 477)

Repo Placement

OK — single repo, `forgejo_admin/basketball-api`. No cross-repo work. Frontend redirect stays as-is (parents click email URL, no SPA involvement). Unchanged from pass 1.

Dependencies

  • Not blocked by #497 (monthly recovery). #497 is the downstream consumer of this helper, not a predecessor. Confirmed in pass 1.
  • Depends on #494 having landed (it has — commit `4a4b88a` on origin/main). `services/tournament_checkout.py` provides the dogfooded pattern to copy with the three monthly-specific deltas above.
  • No blockers in `in_progress`. This ticket is the enabler for `#497`.

Acceptance Criteria

8 bullets, all testable. Four are net-new work (Payment Link URL creation with metadata, webhook gate widening, `setup_future_usage` carry-over, email redirect plumbing). Four are pass-through assertions (amount_cents flow-through, `#497` compatibility, non-monthly flows untouched, email template unchanged). Agent-verifiable. Fits the 5-minute rule — no decomposition needed.

Blast Radius

  • Webhook handler gate (webhooks.py:250) — additive widening only. Tournament branch untouched. Jersey/tryout/generic untouched. No risk.
  • Email template — explicitly unchanged by constraint. No risk.
  • Stale-pending reuse path (checkout.py:358-377) — will simplify with Payment Links (no expiry, no 30-min reuse gymnastics). Acceptable cleanup within scope.
  • Non-monthly flows (jersey.py, admin.py, register.py, generic checkout paths) — unchanged by constraint. 24h-safe for their latency profiles.

Scope Creep Check

Diff vs pass-1 body, counted narratively:
  • Lineage — one sentence added pointing at `review-1032-2026-04-17`. Audit trail. Acceptable housekeeping.
  • Context — three numbered sub-items added (per-parent amount already present conceptually; `setup_future_usage`; email redirect). All three trace to pass-1 recommendations.
  • File Targets — single mint-site citation added (`routes/checkout.py:305` `first_payment_checkout`). Explicitly within pass-1 allowed deltas.
  • Acceptance Criteria — +3 bullets (webhook gate, `setup_future_usage`, email redirect). All three map 1:1 to required deltas.
  • Test Expectations — +1 bullet (`setup_future_usage` on Payment Link path). Maps to delta 1.
  • Constraints — +1 bullet (do not embed Stripe URLs in email). Maps to delta 3.
  • Checklist — +4 bullets (`setup_future_usage`, webhook gate, email template, helper availability for `#497`). All four map to the three deltas or the `#497` consumer note.
  • Related — `#497` link added (explicitly allowed).
No new repos, no new file targets beyond the cited mint site, no expanded blast radius, no new user stories, no real-Stripe scope expansion. Body grew only within the pass-1 allowed deltas.

Decomposition Assessment

File count: 3 (checkout.py, email.py or first_payment_checkout redirect body, webhooks.py gate). AC count: 8 — on the edge of the >5 threshold, but four of those eight are pass-through assertions (email template unchanged, non-monthly flows untouched, recovery-ticket compatibility, metadata flow-through) rather than net-new work. Estimated agent work: 30-45 min. No decomposition needed — fits a single agent pass.

Recommendation

APPROVED. Board item #1032 is ready to advance from `backlog` to `todo` via `update_board_item`. No further body edits required. No `[BODY]`, `[LABEL]`, `[SCOPE]`, or `[DECOMPOSE]` recommendations.
  • Pass-1 review: `review-1032-2026-04-17` (verdict NEEDS_REFINEMENT)
  • Forgejo issue: `forgejo_admin/basketball-api#498`
  • Board item: `board-westside-basketball#1032`
  • Skill: `skill-review-ticket`