fix: regenerate Utah Invitational tournament orders via blessed helper #485

Merged

forgejo_admin merged 1 commit from 484-regenerate-utah-tournament-orders into main 2026-04-15 20:28:09 +00:00

Conversation 1 Commits 1 Files changed 4 +816

forgejo_admin commented 2026-04-15 20:19:45 +00:00

Owner

Copy link

Summary

The Utah Invitational blast created 21 pending Orders whose Stripe Checkout Sessions were built by ad-hoc stripe.checkout.Session.create calls that omitted order_id metadata. When parents paid, the webhook handler at routes/webhooks.py::_handle_generic_order_completed could not match the session back to its Order row, so the Orders stayed pending even after Stripe took the money.

This PR adds the blessed helper, an idempotent regen script, an end-to-end signed-webhook roundtrip test, and the operator runbook.

Changes

• src/basketball_api/services/tournament_checkout.py — new helper create_tournament_order_checkout_session(). Single source of truth for tournament-fee sessions; always sets order_id / player_id / product_id metadata.
• scripts/regenerate_tournament_orders.py — idempotent regen script. Dry-run default, --tournament-id, --product-ids, --commit, --force. Expires old broken sessions, mints replacements through the helper.
• tests/test_tournament_webhook_roundtrip.py — full-path integration test. Does NOT mock stripe.Webhook.construct_event. Synthesizes a real checkout.session.completed payload, signs it with the test webhook secret via raw HMAC-SHA256 in the Stripe t=..,v1=.. scheme, POSTs to TestClient, asserts Order flips pending -> paid. Companion bad-signature case proves the real validator is in the loop.
• docs/tournament-billing-runbook.md — operator runbook. Prohibition on ad-hoc session creation is scoped to tournament fees only. The five other sanctioned stripe.checkout.Session.create call sites (jersey, generic /create-session, first-payment, tryout registration x2) are documented and unaffected.

QA Review Items Addressed

1. Scope rule to tournament-fee only — runbook enumerates the other five sanctioned call sites explicitly and confirms they are not governed by the new rule.
2. Signed synthesized events, not mocks — construct_event is never patched. Real HMAC-SHA256 signature in Stripe's t=..,v1=.. scheme is validated by the real SDK inside the route. Bad-signature test confirms validator is active.

Test Plan

• pytest tests/test_tournament_webhook_roundtrip.py -v — 2 passed locally
• pytest tests/test_webhooks.py tests/test_checkout.py tests/test_tournament_webhook_roundtrip.py — 37 passed locally (no regressions in adjacent tests)
• ruff format + ruff check — clean on new files
• Manual: python scripts/regenerate_tournament_orders.py --help prints expected usage

Review Checklist

• Tests pass locally (37/37 in scope)
• ruff format + ruff check clean on new files
• No regressions in adjacent tests (webhooks, checkout)
• Webhook signature validation path exercised for real (no construct_event mock)
• Script is idempotent + dry-run by default
• Runbook documents scope of the prohibition (tournament-fee only)
• Secrets/credentials not committed

Related Notes

• Forgejo issue: Closes #484
• Webhook handler: src/basketball_api/routes/webhooks.py::_handle_generic_order_completed
• Related PR: #480 (fresh-pending-session amount match) — adjacent area, independent fix

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

## Summary The Utah Invitational blast created 21 pending Orders whose Stripe Checkout Sessions were built by ad-hoc `stripe.checkout.Session.create` calls that omitted `order_id` metadata. When parents paid, the webhook handler at `routes/webhooks.py::_handle_generic_order_completed` could not match the session back to its Order row, so the Orders stayed `pending` even after Stripe took the money. This PR adds the blessed helper, an idempotent regen script, an end-to-end signed-webhook roundtrip test, and the operator runbook. ## Changes - `src/basketball_api/services/tournament_checkout.py` — new helper `create_tournament_order_checkout_session()`. Single source of truth for tournament-fee sessions; always sets `order_id` / `player_id` / `product_id` metadata. - `scripts/regenerate_tournament_orders.py` — idempotent regen script. Dry-run default, `--tournament-id`, `--product-ids`, `--commit`, `--force`. Expires old broken sessions, mints replacements through the helper. - `tests/test_tournament_webhook_roundtrip.py` — full-path integration test. **Does NOT mock `stripe.Webhook.construct_event`.** Synthesizes a real `checkout.session.completed` payload, signs it with the test webhook secret via raw HMAC-SHA256 in the Stripe `t=..,v1=..` scheme, POSTs to TestClient, asserts Order flips `pending -> paid`. Companion bad-signature case proves the real validator is in the loop. - `docs/tournament-billing-runbook.md` — operator runbook. Prohibition on ad-hoc session creation is **scoped to tournament fees only**. The five other sanctioned `stripe.checkout.Session.create` call sites (jersey, generic /create-session, first-payment, tryout registration x2) are documented and unaffected. ## QA Review Items Addressed 1. **Scope rule to tournament-fee only** — runbook enumerates the other five sanctioned call sites explicitly and confirms they are not governed by the new rule. 2. **Signed synthesized events, not mocks** — `construct_event` is never patched. Real HMAC-SHA256 signature in Stripe's `t=..,v1=..` scheme is validated by the real SDK inside the route. Bad-signature test confirms validator is active. ## Test Plan - `pytest tests/test_tournament_webhook_roundtrip.py -v` — 2 passed locally - `pytest tests/test_webhooks.py tests/test_checkout.py tests/test_tournament_webhook_roundtrip.py` — 37 passed locally (no regressions in adjacent tests) - `ruff format` + `ruff check` — clean on new files - Manual: `python scripts/regenerate_tournament_orders.py --help` prints expected usage ## Review Checklist - [x] Tests pass locally (37/37 in scope) - [x] `ruff format` + `ruff check` clean on new files - [x] No regressions in adjacent tests (webhooks, checkout) - [x] Webhook signature validation path exercised for real (no `construct_event` mock) - [x] Script is idempotent + dry-run by default - [x] Runbook documents scope of the prohibition (tournament-fee only) - [x] Secrets/credentials not committed ## Related Notes - Forgejo issue: Closes #484 - Webhook handler: `src/basketball_api/routes/webhooks.py::_handle_generic_order_completed` - Related PR: #480 (fresh-pending-session amount match) — adjacent area, independent fix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin added 4 commits 2026-04-15 20:19:45 +00:00

feat: add skip_proration flag to first-payment endpoint and checkout route ... 857d10a149

When skip_proration=true on POST /admin/email/first-payment, the email
shows the flat monthly fee instead of the prorated amount. The checkout
URL includes &prorate=false so GET /checkout/first-payment charges the
exact monthly_fee * 100 cents.

Closes #458

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: plain-text fee_label bug + add skip_proration tests ... 880a2faaf4

Fix plain-text body to use fee_label instead of hardcoded "Prorated
April fee". Add 3 tests: email skip_proration, blast param forwarding,
checkout prorate=false charging.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

feat: add skip_proration flag to first-payment endpoint and checkout (#470) bb7723a844

fix: regenerate Utah Invitational tournament orders via blessed helper ... e720a88f6c

The Utah Invitational blast created 21 pending Orders whose Stripe
Checkout Sessions were built by ad-hoc stripe.checkout.Session.create
calls that omitted order_id metadata. When parents paid, the webhook
handler at routes/webhooks.py::_handle_generic_order_completed could
not match the session back to its Order row, so the Orders stayed
pending forever.

Changes:
- services/tournament_checkout.py: new blessed helper
  create_tournament_order_checkout_session() that always sets
  order_id / player_id / product_id metadata.
- scripts/regenerate_tournament_orders.py: idempotent regen script
  with --dry-run default, --tournament-id and --product-ids filters,
  --force override. Expires old broken sessions and mints replacements
  through the helper.
- tests/test_tournament_webhook_roundtrip.py: full-path integration
  test that does NOT mock stripe.Webhook.construct_event. Synthesizes
  a real checkout.session.completed event, signs it with the test
  webhook secret via raw HMAC-SHA256 in the t=..,v1=.. scheme, POSTs
  to TestClient, and asserts Order flips pending -> paid. Companion
  bad-signature case proves the real validator is in the loop.
- docs/tournament-billing-runbook.md: operator runbook. Tournament-fee
  flow must go through the helper. Prohibition is scoped to tournament
  fees; the five other sanctioned stripe.checkout.Session.create call
  sites (jersey, first-payment, tryout registration) are documented
  and unaffected.

Forgejo: #484

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented 2026-04-15 20:21:24 +00:00

Author

Owner

Copy link

PR #485 Review

DOMAIN REVIEW (Python / FastAPI / SQLAlchemy / Stripe)

src/basketball_api/services/tournament_checkout.py — solid

• Clean single-source helper. Always sets order_id / player_id / product_id metadata.
• CheckoutSessionResult dataclass is a nice typed boundary.
• _ensure_customer creates-or-reuses correctly and persists stripe_customer_id.
• Raises ValueError when relationships missing — good defensive check.
• Minor: _SUCCESS_URL / _CANCEL_URL captured at import time from _settings.frontend_url. If settings ever mutate in tests this could bite, but current codebase treats settings as immutable at boot — acceptable.

scripts/regenerate_tournament_orders.py — meets spec

• Dry-run is default; --commit required to write. Correct safety posture.
• --tournament-id, --product-ids, --force all present as requested.
• Idempotent via _session_has_order_id() guard; --force bypass.
• _expire_session() is best-effort with typed exception handling (InvalidRequestError) and broad fallback. Good.
• Uses the blessed helper (create_tournament_order_checkout_session) — no ad-hoc Session.create. Correct.
• Order.custom_data["tournament_id"].as_integer() — non-portable (PG-specific JSON accessor). Fine for a prod-run script, but if you ever want this script under pytest with SQLite it will error. Not a blocker.
• Rolls back on dry-run, commits on --commit. Correct.

tests/test_tournament_webhook_roundtrip.py — excellent

• Real HMAC-SHA256 signature in Stripe t=..,v1=.. scheme. No stripe.Webhook.construct_event patch. Exactly what the incident review demanded.
• Bad-signature companion test proves the validator is in the loop, not silently bypassed.
• Asserts stripe_webhook_secret == TEST_WEBHOOK_SECRET before running — fails loud if conftest env drifts.
• Only mocks outbound Stripe calls (Customer.create, checkout.Session.create). Correct surface.
• Asserts Order.status flip pending->paid AND stripe_payment_intent_id captured. Complete assertion.

docs/tournament-billing-runbook.md — scoped correctly

• Prohibition explicitly scoped to tournament-fee flow.
• Five sanctioned stripe.checkout.Session.create call sites enumerated with file:line references (jersey, generic /create-session, first-payment, tryout x2). Operators cannot confuse scope.
• Regeneration procedure, verification steps, post-incident checklist all present.

BLOCKERS

1. Scope creep — skip_proration changes are unrelated to issue #484. The following changes belong in a separate PR tied to the Jaxon skip_proration work (session memory references #458):

   • src/basketball_api/routes/admin.py — skip_proration query param on /admin/email/first-payment
   • src/basketball_api/routes/checkout.py — prorate query param on /checkout/first-payment
   • src/basketball_api/services/email.py — send_first_payment_email(skip_proration=...)
   • tests/test_first_payment.py, test_first_payment_blast.py, test_first_payment_email.py — tests for the above

   Issue #484 is "regenerate Utah tournament orders." Bundling the skip_proration feature here violates per-convention "one ticket = one PR" (feedback_smaller_scopes_parallel). Pull skip_proration into its own branch/PR referencing its own ticket.

2. Python 3.12+ f-string syntax risk in services/email.py. Two places now use nested f-strings with the same quote character:

   f"{\n        f'''<p ...>...</p>'''\n        if not skip_proration\n        else \"\"\n    }"
   

   and in plain_body:

   f"{\"Since you're joining...\" + chr(10) + chr(10) if not skip_proration else ''}"
   

   This requires PEP 701 (Python 3.12+). If CI or prod runs 3.11, this is a syntax error. Verify the runtime Python version is >=3.12 OR refactor to build the conditional fragment in a plain variable before the f-string. The chr(10) usage is also a code smell — prefer a "\n\n" literal held in a variable.

NITS

• services/email.py: the conditional HTML and plain-text branches duplicate logic. Consider extracting a small _build_fee_copy(skip_proration, monthly_fee) helper returning a tuple so the templates stay readable. Tied to the blocker above — fix when you split the scope.
• regenerate_tournament_orders.py: stripe.error.InvalidRequestError import pattern uses # type: ignore[attr-defined]. Fine, but a top-of-file from stripe.error import InvalidRequestError would be cleaner and let you drop the ignore.
• Test file has a top-level TEST_WEBHOOK_SECRET = "whsec_test" — consider reading it from settings directly to remove the duplication (the assert guards drift, but DRY is cheap here).
• PR body says Closes #484 in "Related Notes" — good. Confirm Forgejo auto-closes on merge.

SOP COMPLIANCE

• Branch named after issue (484-regenerate-utah-tournament-orders)
• PR body has Summary / Changes / Test Plan / Related
• Tests exist and pass locally (37/37 claimed)
• No secrets committed
• No migration files (no collision risk against main's 047)
• Scope discipline violated — skip_proration work piggybacked onto tournament PR
• Webhook signature validation path exercised for real
• Runbook prohibition scoped to tournament-fee only
• Tournament helper uses create_player_checkout_session()-equivalent pattern (actually introduces create_tournament_order_checkout_session() as the blessed helper — naming is clearer than the issue's suggested name, consistent with scope)

PROCESS OBSERVATIONS

• Change failure risk: The scope-creep bundle increases blast radius. If skip_proration introduces a regression, rolling back the tournament fix via git revert also reverts proration changes (and vice versa). DORA CFR suffers. Split now, merge separately.
• Mergeable flag is false in the PR API response — likely needs rebase against main before merge. Pull local main first (feedback_pull_local_after_merge).
• Documentation: Runbook is exemplary. This is the standard — post-incident write-up + scoped prohibition + enumerated exceptions. Worth citing as a reference for future runbooks.
• Test quality: The "real HMAC, no construct_event mock" pattern should be promoted to a convention for any webhook handler. Consider a sop-webhook-roundtrip-testing note in pal-e-docs.

VERDICT: NOT APPROVED

Split the skip_proration changes into a separate PR tied to its own issue (#458 or a new ticket). After split, verify the remaining Python 3.12+ f-string risk is mitigated (either confirm runtime is 3.12+, or — cleanest — the email changes leave this PR entirely). The tournament regeneration work itself is well-executed and ready to merge once scoped cleanly.

## PR #485 Review ### DOMAIN REVIEW (Python / FastAPI / SQLAlchemy / Stripe) **`src/basketball_api/services/tournament_checkout.py` — solid** - Clean single-source helper. Always sets `order_id` / `player_id` / `product_id` metadata. - `CheckoutSessionResult` dataclass is a nice typed boundary. - `_ensure_customer` creates-or-reuses correctly and persists `stripe_customer_id`. - Raises `ValueError` when relationships missing — good defensive check. - Minor: `_SUCCESS_URL` / `_CANCEL_URL` captured at import time from `_settings.frontend_url`. If settings ever mutate in tests this could bite, but current codebase treats settings as immutable at boot — acceptable. **`scripts/regenerate_tournament_orders.py` — meets spec** - Dry-run is default; `--commit` required to write. Correct safety posture. - `--tournament-id`, `--product-ids`, `--force` all present as requested. - Idempotent via `_session_has_order_id()` guard; `--force` bypass. - `_expire_session()` is best-effort with typed exception handling (InvalidRequestError) and broad fallback. Good. - Uses the blessed helper (`create_tournament_order_checkout_session`) — no ad-hoc Session.create. Correct. - `Order.custom_data["tournament_id"].as_integer()` — non-portable (PG-specific JSON accessor). Fine for a prod-run script, but if you ever want this script under pytest with SQLite it will error. Not a blocker. - Rolls back on dry-run, commits on `--commit`. Correct. **`tests/test_tournament_webhook_roundtrip.py` — excellent** - Real HMAC-SHA256 signature in Stripe `t=..,v1=..` scheme. No `stripe.Webhook.construct_event` patch. Exactly what the incident review demanded. - Bad-signature companion test proves the validator is in the loop, not silently bypassed. - Asserts `stripe_webhook_secret == TEST_WEBHOOK_SECRET` before running — fails loud if conftest env drifts. - Only mocks outbound Stripe calls (`Customer.create`, `checkout.Session.create`). Correct surface. - Asserts `Order.status` flip pending->paid AND `stripe_payment_intent_id` captured. Complete assertion. **`docs/tournament-billing-runbook.md` — scoped correctly** - Prohibition explicitly scoped to tournament-fee flow. - Five sanctioned `stripe.checkout.Session.create` call sites enumerated with file:line references (jersey, generic /create-session, first-payment, tryout x2). Operators cannot confuse scope. - Regeneration procedure, verification steps, post-incident checklist all present. ### BLOCKERS 1. **Scope creep — `skip_proration` changes are unrelated to issue #484.** The following changes belong in a separate PR tied to the Jaxon skip_proration work (session memory references #458): - `src/basketball_api/routes/admin.py` — `skip_proration` query param on `/admin/email/first-payment` - `src/basketball_api/routes/checkout.py` — `prorate` query param on `/checkout/first-payment` - `src/basketball_api/services/email.py` — `send_first_payment_email(skip_proration=...)` - `tests/test_first_payment.py`, `test_first_payment_blast.py`, `test_first_payment_email.py` — tests for the above Issue #484 is "regenerate Utah tournament orders." Bundling the skip_proration feature here violates per-convention "one ticket = one PR" (feedback_smaller_scopes_parallel). Pull skip_proration into its own branch/PR referencing its own ticket. 2. **Python 3.12+ f-string syntax risk in `services/email.py`.** Two places now use nested f-strings with the same quote character: ```python f"{\n f'''<p ...>...</p>'''\n if not skip_proration\n else \"\"\n }" ``` and in `plain_body`: ```python f"{\"Since you're joining...\" + chr(10) + chr(10) if not skip_proration else ''}" ``` This requires PEP 701 (Python 3.12+). If CI or prod runs 3.11, this is a syntax error. Verify the runtime Python version is >=3.12 OR refactor to build the conditional fragment in a plain variable before the f-string. The `chr(10)` usage is also a code smell — prefer a `"\n\n"` literal held in a variable. ### NITS - `services/email.py`: the conditional HTML and plain-text branches duplicate logic. Consider extracting a small `_build_fee_copy(skip_proration, monthly_fee)` helper returning a tuple so the templates stay readable. Tied to the blocker above — fix when you split the scope. - `regenerate_tournament_orders.py`: `stripe.error.InvalidRequestError` import pattern uses `# type: ignore[attr-defined]`. Fine, but a top-of-file `from stripe.error import InvalidRequestError` would be cleaner and let you drop the ignore. - Test file has a top-level `TEST_WEBHOOK_SECRET = "whsec_test"` — consider reading it from settings directly to remove the duplication (the assert guards drift, but DRY is cheap here). - PR body says `Closes #484` in "Related Notes" — good. Confirm Forgejo auto-closes on merge. ### SOP COMPLIANCE - [x] Branch named after issue (`484-regenerate-utah-tournament-orders`) - [x] PR body has Summary / Changes / Test Plan / Related - [x] Tests exist and pass locally (37/37 claimed) - [x] No secrets committed - [x] No migration files (no collision risk against main's 047) - [ ] **Scope discipline violated** — skip_proration work piggybacked onto tournament PR - [x] Webhook signature validation path exercised for real - [x] Runbook prohibition scoped to tournament-fee only - [x] Tournament helper uses `create_player_checkout_session()`-equivalent pattern (actually introduces `create_tournament_order_checkout_session()` as the blessed helper — naming is clearer than the issue's suggested name, consistent with scope) ### PROCESS OBSERVATIONS - **Change failure risk**: The scope-creep bundle increases blast radius. If skip_proration introduces a regression, rolling back the tournament fix via `git revert` also reverts proration changes (and vice versa). DORA CFR suffers. Split now, merge separately. - **Mergeable flag is false** in the PR API response — likely needs rebase against main before merge. Pull local main first (feedback_pull_local_after_merge). - **Documentation**: Runbook is exemplary. This is the standard — post-incident write-up + scoped prohibition + enumerated exceptions. Worth citing as a reference for future runbooks. - **Test quality**: The "real HMAC, no construct_event mock" pattern should be promoted to a convention for any webhook handler. Consider a `sop-webhook-roundtrip-testing` note in pal-e-docs. ### VERDICT: NOT APPROVED Split the `skip_proration` changes into a separate PR tied to its own issue (#458 or a new ticket). After split, verify the remaining Python 3.12+ f-string risk is mitigated (either confirm runtime is 3.12+, or — cleanest — the email changes leave this PR entirely). The tournament regeneration work itself is well-executed and ready to merge once scoped cleanly.

forgejo_admin force-pushed 484-regenerate-utah-tournament-orders from e720a88f6c to f16fd1e9ef 2026-04-15 20:27:39 +00:00 Compare

forgejo_admin merged commit a4047d2c62 into main 2026-04-15 20:28:09 +00:00

forgejo_admin deleted branch 484-regenerate-utah-tournament-orders 2026-04-15 20:28:09 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-15 20:28:10 +00:00

fix: regenerate Utah Invitational tournament orders via blessed helper (#485)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/basketball-api!485

No description provided.