Review: Bug: Generic checkout webhook doesn't sync jersey status to players

Verdict: NEEDS_REFINEMENT

Template Completeness

• [x] Type — present (Bug)
• [x] Lineage — present (standalone — discovered during operations/data audit)
• [x] Repo — present (forgejo_admin/basketball-api)
• [x] What Broke — present, thorough root cause analysis with confirmed affected players
• [x] Repro Steps — present, 5 clear steps
• [x] Expected Behavior — present
• [x] Environment — present (prod / basketball-api)
• [x] Acceptance Criteria — present, 6 items
• [x] Related — present
• [ ] User Story — MISSING (template requires "As a ___ I want ___ So that ___")
• [ ] File Targets — MISSING as dedicated section. Files are mentioned inline in "What Broke" but not in the template-required format listing files to modify vs. files NOT to touch
• [ ] Test Expectations — MISSING. No test commands or descriptions of what tests to write/update
• [ ] Constraints — MISSING. No mention of patterns to follow, dependencies, or safety considerations for the data migration
• [ ] Checklist — MISSING (PR opened / Tests pass / No unrelated changes)

File Targets

• [x] webhooks.py — verified at src/basketball_api/routes/webhooks.py (NOT src/routes/webhooks.py as implied). _handle_generic_order_completed confirmed at lines 137-169. Only sets order.status = paid and stripe_payment_intent_id — does NOT touch player fields. Line reference "webhooks.py:137-169" is accurate.
• [x] checkout.py — verified at src/basketball_api/routes/checkout.py. Opt-out path (lines 141-160) creates Order with status=paid but does NOT sync to player jersey fields. Bug confirmed.
• [x] jersey.py — verified at src/basketball_api/routes/jersey.py. Legacy path sets jersey_order_status = pending at checkout start (line 314), webhook handler syncs full jersey fields on payment (webhooks.py lines 200-214).
• [x] webhooks.py:256-262 — verified. Routing at lines 256-263: generic handler checked first, returns early if order_id present, legacy handler never reached. Root cause confirmed.
• [x] alembic/versions/013_generic_checkout_system.py — verified. Creates products and orders tables. No unique constraint on (player_id, product_id) — duplicate prevention gap confirmed.
• [x] Player model fields — verified in models.py: jersey_option (line 208), jersey_order_status (line 209), jersey_size (line 212), jersey_number (line 213) all exist.
• [ ] File paths in issue use shorthand (webhooks.py, checkout.py) — should use full paths from repo root (src/basketball_api/routes/webhooks.py, etc.) for agent clarity

Repo Placement

Dependencies

• No blocking dependencies found on the board. Related completed items: #265 (jersey number in checkout), #262 (jersey size in checkout), #260 (jersey_option set before payment fix) — all in done column.
• No in-progress items that conflict.
• The ticket's AC #6 ("Decision: retire legacy /jersey/checkout or keep as redirect") is an architectural decision, not a code change. This should be a separate ticket or explicitly marked as a decision to document, not an acceptance criterion for the agent.

Acceptance Criteria

• [x] AC1 — Testable: webhook handler syncs jersey fields. Existing test test_checkout.py:TestCheckoutWebhook.test_webhook_updates_order_to_paid (line 321) already tests the webhook but only asserts order.status and stripe_payment_intent_id — needs to be extended to assert player jersey fields.
• [x] AC2 — Testable: opt-out path syncs to players table. Can verify with unit test against checkout.py create-session endpoint for opt_out product type.
• [ ] AC3 — "One-time data migration" — AMBIGUOUS. Is this an alembic migration? A one-off script? Needs clarification on mechanism and safety (dry-run first? backup?). Also: how does the migration determine jersey_option/size/number from the Order record when custom_data JSONB structure varies?
• [ ] AC4 — "Baby Betty contradictory state resolved (manual data fix)" — this is manual ops work, not agent-verifiable code. Should be split to a separate ops ticket or documented as a post-deploy manual step.
• [x] AC5 — Testable: duplicate order prevention. Clear enough for implementation.
• [ ] AC6 — "Decision: retire legacy /jersey/checkout or keep as redirect" — this is a decision, not an acceptance criterion. An agent cannot verify a decision. Should be extracted to a separate architectural decision ticket.

Blast Radius

• westside-app — reads jersey_number for display on team pages (src/routes/teams/[id]/+page.svelte:89) and coach dashboard (src/routes/coach/+page.svelte:111). Fixing the sync bug will make these displays accurate — no code changes needed downstream, but positive downstream impact.
• westside-contracts — references jersey info in contract display only. Not affected by this fix.
• Admin email — admin.py has a jersey reminder email endpoint (line 792) that presumably queries player jersey status. If admin queries rely on players.jersey_order_status to determine who hasn't ordered, the current bug means reminders may be sent to parents who already paid via the generic path. The fix will resolve this.
• No similar pattern found in other webhook handlers — the subscription webhook handler is separate and doesn't have this two-path problem.
• Test gap — tests/test_checkout.py:TestCheckoutWebhook has only 2 tests (paid + invalid order_id). The jersey legacy webhook has 6 tests covering all field syncs. The generic path needs equivalent coverage.

Recommendation

• Add a File Targets section listing full paths from repo root:
• Add Test Expectations section — describe what tests to write/extend, include run command (pytest tests/test_checkout.py -v)
• Clarify AC3 — specify the data migration mechanism (alembic migration vs. one-off script) and how to derive jersey_option/size/number from the orders.custom_data JSONB
• Split AC4 (Baby Betty manual fix) to a separate ops ticket — it is not agent-verifiable code
• Split AC6 (legacy path retirement decision) to a separate architectural decision ticket — agents cannot verify decisions
• Add Constraints section — mention the data migration safety requirements (dry-run, backup, rollback plan)