feat: mint tournament-fee checkout URLs as Stripe Payment Links (#494) #496

Merged

forgejo_admin merged 1 commit from 494-payment-links-tournament into main 2026-04-17 22:33:59 +00:00

Conversation 1 Commits 1 Files changed 6 +639 -212

forgejo_admin commented 2026-04-17 22:15:11 +00:00

Owner

Copy link

Summary

Replaces the tournament-fee mint path in services/tournament_checkout.py with stripe.PaymentLink.create so blast URLs no longer hit Stripe's 24h Checkout Session cap that stranded 17 Utah Invitational orders in PR #490. The webhook handler is wired to deactivate the Payment Link on checkout.session.completed so leaked URLs can't double-charge. Scope is strictly tournament-fee — jersey, monthly, tryout, generic checkout, and admin-regen flows are untouched (they'll be handled separately; their latency profile tolerates the 24h cap).

Addresses Forgejo issue forgejo_admin/basketball-api#494. First consumer is the #486 Utah Invitational recovery ticket, which can now mint 17 Payment Links through the same blessed helper.

Changes

• src/basketball_api/services/tournament_checkout.py — rewrote helper to mint stripe.Price.create + stripe.PaymentLink.create with order_id / player_id / product_id metadata. Exposed as create_tournament_order_payment_link; kept create_tournament_order_checkout_session as a back-compat alias (recovery ticket #486 uses the old name). Result dataclass gained a session_id alias property so pre-existing destructuring keeps working.
• src/basketball_api/routes/webhooks.py — _handle_generic_order_completed now deactivates the Stripe Payment Link (stripe.PaymentLink.modify(id, active=False)) after marking an Order paid, gated on (a) session.payment_link being populated and (b) product.category == ProductCategory.tournament so non-tournament flows are untouched. Deactivation failure is logged-and-swallowed so the fulfillment commit never blocks.
• scripts/regenerate_tournament_orders.py — updated to retire either a cs_ or plink_ stored id (prefix-dispatched), re-mints through the new helper. Remains idempotent + dry-run by default.
• tests/test_checkout_session_ttl.py — tournament helper test rewritten to assert PaymentLink minting with correct metadata and no expires_at. The other 7 Checkout-Session call-site tests are untouched and still enforce the 30d TTL invariant for non-tournament flows.
• tests/test_tournament_webhook_roundtrip.py — now patches stripe.PaymentLink.create / Price.create / PaymentLink.modify (not Session.create). Synthesized event now carries payment_link so the deactivation branch runs; asserts mock_modify.assert_called_once_with(plink_id, active=False). Still uses the real stripe.Webhook.construct_event HMAC validator.
• tests/test_payment_link_real_stripe.py — NEW. Real-Stripe integration test required by #494 AC. Talks to Stripe test-mode API, verifies no expires_at on the minted link, verifies metadata propagation, deactivates and asserts active=False. Opt-in via BASKETBALL_STRIPE_TEST_API_KEY env var. Guards against live keys — refuses anything not prefixed sk_test_.
• docs/tournament-billing-runbook.md — updated TL;DR + call-site table + "Adding a new tournament-fee call site" to reflect the PaymentLink pivot. Kept legacy TTL section for the still-Checkout-Session flows.

Test Plan

• ruff check . — all checks passed
• ruff format --check . — 125 files formatted
• pytest tests/test_checkout_session_ttl.py tests/test_tournament_webhook_roundtrip.py -v — 10/10 pass locally (Postgres backend, real HMAC signature validator in loop).
• pytest tests/test_payment_link_real_stripe.py -v — skips cleanly without the test key; will exercise the real Stripe path once CI injects BASKETBALL_STRIPE_TEST_API_KEY.
• Full pytest — 1033 pass, 1 skipped, 5 failures pre-existing on main and unrelated to this PR (test_jersey_reminder.py, test_westside_streamlit_ro_role.py, test_first_payment_email.py::test_send_first_payment_email; verified by stashing changes and re-running).
• CI (Woodpecker) to run ruff + pytest on push — standard pipeline.
• Manual check post-merge: run scripts/regenerate_tournament_orders.py --tournament-id 2 in dry-run against prod, confirm it proposes PaymentLink mints, then --commit to recover the 17 Utah Invitational orders (dogfood for #486).

AC Traceability (issue #494)

• Tournament-fee email blasts carry URLs that remain clickable indefinitely — PaymentLinks don't expire.
• Each URL is bound 1:1 to an Order, order_id in Stripe metadata — set on PaymentLink + payment_intent_data.metadata belt-and-suspenders.
• On successful payment, Payment Link is deactivated programmatically — webhook calls stripe.PaymentLink.modify(id, active=False) gated on tournament category.
• Recovery ticket #486 can mint 17 links via this helper — create_tournament_order_checkout_session alias + updated regen script preserve call signature.
• Non-tournament flows untouched — no changes to routes/jersey.py, routes/checkout.py, routes/register.py, routes/admin.py checkout-session call sites. They're left for #493 (which will revert the broken 30d TTL).
• Webhook fulfillment match still works (order_id → Order.status=paid) — covered by roundtrip test.
• Deactivation-on-payment path covered — roundtrip test asserts PaymentLink.modify called once with active=False.
• Real-Stripe integration test included — tests/test_payment_link_real_stripe.py.
• Webhook handler signature unchanged — _handle_generic_order_completed(db, session_data) still matches via metadata, no new parameters.

Review Checklist

• Ruff check passes
• Ruff format check passes
• Tournament helper + webhook roundtrip tests pass locally (10/10)
• Full test suite run — no new failures introduced (5 pre-existing failures verified on unmodified main)
• No non-tournament call sites modified (jersey, monthly, tryout, generic checkout, admin regen all untouched)
• Webhook handler signature unchanged
• No database migration required (reuses existing stripe_checkout_session_id column to store plink_... id)
• Deactivation path swallows Stripe errors so fulfillment commit never blocks
• Real-Stripe integration test guarded against live keys (sk_test_ prefix assertion)
• Docs updated (docs/tournament-billing-runbook.md)
• CI passes (Woodpecker pipeline on push)
• Post-merge: dogfood recovery script against the 17 Utah Invitational orders (#486)

Related Notes

• project-pal-e-platform — basketball-api is a downstream consumer of the platform; no platform changes required
• feedback_retrieve_before_theorize.md — #490 was mocked-only coverage missing Stripe's server-side 24h cap; this PR ships a real-Stripe integration test so the same class of bug can't recur
• feedback_migration_slot_coordination.md — reused stripe_checkout_session_id column to avoid alembic migration collision with any parallel branches
• project_seven_pillar_validation.md — payment pipeline observability hardening continues (ties to recent #290 alert work)

Related

• Closes #494
• Depends on / complements: #493 (revert of broken 30d TTL on non-tournament flows; different files — no conflict)
• Architecture spike: #489
• First consumer: #486 (Utah Invitational recovery — dogfood)
• Original (misdiagnosed) ticket: #488

## Summary Replaces the tournament-fee mint path in `services/tournament_checkout.py` with `stripe.PaymentLink.create` so blast URLs no longer hit Stripe's 24h Checkout Session cap that stranded 17 Utah Invitational orders in PR #490. The webhook handler is wired to deactivate the Payment Link on `checkout.session.completed` so leaked URLs can't double-charge. Scope is strictly tournament-fee — jersey, monthly, tryout, generic checkout, and admin-regen flows are untouched (they'll be handled separately; their latency profile tolerates the 24h cap). Addresses Forgejo issue `forgejo_admin/basketball-api#494`. First consumer is the `#486` Utah Invitational recovery ticket, which can now mint 17 Payment Links through the same blessed helper. ## Changes - `src/basketball_api/services/tournament_checkout.py` — rewrote helper to mint `stripe.Price.create` + `stripe.PaymentLink.create` with `order_id` / `player_id` / `product_id` metadata. Exposed as `create_tournament_order_payment_link`; kept `create_tournament_order_checkout_session` as a back-compat alias (recovery ticket #486 uses the old name). Result dataclass gained a `session_id` alias property so pre-existing destructuring keeps working. - `src/basketball_api/routes/webhooks.py` — `_handle_generic_order_completed` now deactivates the Stripe Payment Link (`stripe.PaymentLink.modify(id, active=False)`) after marking an Order `paid`, gated on (a) `session.payment_link` being populated and (b) `product.category == ProductCategory.tournament` so non-tournament flows are untouched. Deactivation failure is logged-and-swallowed so the fulfillment commit never blocks. - `scripts/regenerate_tournament_orders.py` — updated to retire either a `cs_` or `plink_` stored id (prefix-dispatched), re-mints through the new helper. Remains idempotent + dry-run by default. - `tests/test_checkout_session_ttl.py` — tournament helper test rewritten to assert PaymentLink minting with correct metadata and no `expires_at`. The other 7 Checkout-Session call-site tests are untouched and still enforce the 30d TTL invariant for non-tournament flows. - `tests/test_tournament_webhook_roundtrip.py` — now patches `stripe.PaymentLink.create` / `Price.create` / `PaymentLink.modify` (not `Session.create`). Synthesized event now carries `payment_link` so the deactivation branch runs; asserts `mock_modify.assert_called_once_with(plink_id, active=False)`. Still uses the real `stripe.Webhook.construct_event` HMAC validator. - `tests/test_payment_link_real_stripe.py` — **NEW**. Real-Stripe integration test required by #494 AC. Talks to Stripe test-mode API, verifies no `expires_at` on the minted link, verifies metadata propagation, deactivates and asserts `active=False`. Opt-in via `BASKETBALL_STRIPE_TEST_API_KEY` env var. Guards against live keys — refuses anything not prefixed `sk_test_`. - `docs/tournament-billing-runbook.md` — updated TL;DR + call-site table + "Adding a new tournament-fee call site" to reflect the PaymentLink pivot. Kept legacy TTL section for the still-Checkout-Session flows. ## Test Plan - [x] `ruff check .` — all checks passed - [x] `ruff format --check .` — 125 files formatted - [x] `pytest tests/test_checkout_session_ttl.py tests/test_tournament_webhook_roundtrip.py -v` — **10/10 pass** locally (Postgres backend, real HMAC signature validator in loop). - [x] `pytest tests/test_payment_link_real_stripe.py -v` — skips cleanly without the test key; will exercise the real Stripe path once CI injects `BASKETBALL_STRIPE_TEST_API_KEY`. - [x] Full `pytest` — 1033 pass, 1 skipped, 5 failures pre-existing on main and unrelated to this PR (`test_jersey_reminder.py`, `test_westside_streamlit_ro_role.py`, `test_first_payment_email.py::test_send_first_payment_email`; verified by stashing changes and re-running). - [ ] CI (Woodpecker) to run ruff + pytest on push — standard pipeline. - [ ] Manual check post-merge: run `scripts/regenerate_tournament_orders.py --tournament-id 2` in dry-run against prod, confirm it proposes PaymentLink mints, then `--commit` to recover the 17 Utah Invitational orders (dogfood for #486). ## AC Traceability (issue #494) - [x] Tournament-fee email blasts carry URLs that remain clickable indefinitely — PaymentLinks don't expire. - [x] Each URL is bound 1:1 to an Order, `order_id` in Stripe metadata — set on PaymentLink + `payment_intent_data.metadata` belt-and-suspenders. - [x] On successful payment, Payment Link is deactivated programmatically — webhook calls `stripe.PaymentLink.modify(id, active=False)` gated on tournament category. - [x] Recovery ticket #486 can mint 17 links via this helper — `create_tournament_order_checkout_session` alias + updated regen script preserve call signature. - [x] Non-tournament flows untouched — no changes to `routes/jersey.py`, `routes/checkout.py`, `routes/register.py`, `routes/admin.py` checkout-session call sites. They're left for #493 (which will revert the broken 30d TTL). - [x] Webhook fulfillment match still works (`order_id` → `Order.status=paid`) — covered by roundtrip test. - [x] Deactivation-on-payment path covered — roundtrip test asserts `PaymentLink.modify` called once with `active=False`. - [x] Real-Stripe integration test included — `tests/test_payment_link_real_stripe.py`. - [x] Webhook handler signature unchanged — `_handle_generic_order_completed(db, session_data)` still matches via metadata, no new parameters. ## Review Checklist - [x] Ruff check passes - [x] Ruff format check passes - [x] Tournament helper + webhook roundtrip tests pass locally (10/10) - [x] Full test suite run — no new failures introduced (5 pre-existing failures verified on unmodified main) - [x] No non-tournament call sites modified (jersey, monthly, tryout, generic checkout, admin regen all untouched) - [x] Webhook handler signature unchanged - [x] No database migration required (reuses existing `stripe_checkout_session_id` column to store `plink_...` id) - [x] Deactivation path swallows Stripe errors so fulfillment commit never blocks - [x] Real-Stripe integration test guarded against live keys (`sk_test_` prefix assertion) - [x] Docs updated (`docs/tournament-billing-runbook.md`) - [ ] CI passes (Woodpecker pipeline on push) - [ ] Post-merge: dogfood recovery script against the 17 Utah Invitational orders (#486) ## Related Notes - `project-pal-e-platform` — basketball-api is a downstream consumer of the platform; no platform changes required - `feedback_retrieve_before_theorize.md` — #490 was mocked-only coverage missing Stripe's server-side 24h cap; this PR ships a real-Stripe integration test so the same class of bug can't recur - `feedback_migration_slot_coordination.md` — reused `stripe_checkout_session_id` column to avoid alembic migration collision with any parallel branches - `project_seven_pillar_validation.md` — payment pipeline observability hardening continues (ties to recent #290 alert work) ## Related - Closes #494 - Depends on / complements: #493 (revert of broken 30d TTL on non-tournament flows; different files — no conflict) - Architecture spike: #489 - First consumer: #486 (Utah Invitational recovery — dogfood) - Original (misdiagnosed) ticket: #488

forgejo_admin added 1 commit 2026-04-17 22:15:11 +00:00

feat: mint tournament-fee checkout URLs as Stripe Payment Links (#494) ... a8bc0bf7e0

Replaces the tournament-fee mint path in
services/tournament_checkout.py with stripe.PaymentLink.create so
blast URLs no longer hit Stripe's 24h Checkout Session cap that
stranded 17 Utah Invitational orders (#490). Payment Links carry
order_id metadata that propagates onto the checkout.session.completed
event, so the existing webhook matcher flips Order.status to paid
without any handler-signature changes. On successful payment the
webhook deactivates the link (stripe.PaymentLink.modify active=False)
so a leaked URL can't double-charge.

Non-tournament flows (monthly, jersey, tryout, generic checkout,
admin regen) remain on Checkout Sessions -- their latency profile
tolerates 24h and #493 will handle the TTL revert there.

Tests: tournament helper TTL check rewritten to assert PaymentLink
minting + metadata; webhook roundtrip now asserts deactivation on
payment; new test_payment_link_real_stripe.py talks to real Stripe
in test mode (opt-in via BASKETBALL_STRIPE_TEST_API_KEY, guarded
against live keys) per #494 integration-test AC.

Helper #486 recovery callers keep working via
create_tournament_order_checkout_session back-compat alias.

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

forgejo_admin commented 2026-04-17 22:18:27 +00:00

Author

Owner

Copy link

PR #496 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Stripe SDK. Payment-integration code on the hot path — correctness matters more than style here.

Scope audit (tournament-only invariant) — verified clean:

• routes/jersey.py, routes/checkout.py, routes/register.py, routes/admin.py are untouched. Their Checkout Session call sites remain. Good.
• services/tournament_checkout.py is the only PaymentLink.create call site in src/. Good — the helper stays the single source of truth, now for a different primitive.
• The webhook deactivation block is correctly gated on BOTH session.payment_link being present AND product.category == ProductCategory.tournament. Non-tournament orders cannot accidentally trigger PaymentLink.modify. Good.

PaymentLink.create metadata correctness:

• order_id / player_id / product_id set on the PaymentLink itself (line 136 of the new helper) — this is what Stripe propagates onto checkout.session.completed.metadata when a parent pays.
• Belt-and-suspenders: same metadata also stamped on payment_intent_data.metadata with additional parent_email. This matches the existing webhook matcher semantics at _handle_generic_order_completed (line 194 of webhooks.py) which keys off session_data["metadata"]["order_id"]. Matcher will still fire. Good.
• success_url wired through via after_completion.redirect.url. Cancel URL intentionally dropped (PaymentLinks have no cancel concept) — doc comment on the alias explains this clearly. Accurate.

Deactivation placement:

• Correctly lives in _handle_generic_order_completed — fired synchronously after order.status = paid but before db.commit(). That's the right sequence: we want the Order row committed even if deactivation fails, and the exception handler does exactly that (logged-and-swallowed). No race-prone side-channel. Good.
• One subtle correctness nit: the deactivation sits between jersey-sync and db.commit(). If PaymentLink.modify succeeds but the commit later fails, we end up with a deactivated Stripe link for an un-fulfilled order. That's a VERY narrow window (nothing between the two can realistically fail at this point) and the worse alternative (deactivate after commit, fail, leak a charged URL) is also ugly. Acceptable tradeoff — parents don't double-pay on a paid order, so deactivating early is the right call.

Real-Stripe integration test (tests/test_payment_link_real_stripe.py):

• Correctly opt-in via BASKETBALL_STRIPE_TEST_API_KEY — pytest.mark.skipif at module level means local pytest runs skip cleanly. Good.
• Live-key guard (assert key.startswith("sk_test_")) is load-bearing. This is the safety net the #490 incident was missing. Keep this assertion — it is what makes the test shippable to CI without fear.
• Actually re-fetches the link via stripe.PaymentLink.retrieve(...) and asserts against server-side representation (not just the local return value). This is the coverage #490 lacked. Good.
• Asserts active is False after modify — this is the contract that backs the webhook deactivation hook. Good.
• Minor concern: the test does not clean up the Payment Link explicitly beyond deactivation. Deactivated test-mode Payment Links clutter the Stripe dashboard over time but do not incur cost. Acceptable for now; a followup to add an after/finally that also deactivates price could tidy. NIT only.

Webhook roundtrip test (tests/test_tournament_webhook_roundtrip.py):

• Patches updated from Session.create / Customer.create to PaymentLink.create / Price.create / PaymentLink.modify. Correct set.
• _build_checkout_completed_event now carries payment_link top-level field when supplied — this matches how Stripe actually populates sessions that materialized from PaymentLink clicks. The webhook's deactivation gate reads session_data.get("payment_link") so this fixture exercises the real branch. Good.
• mock_modify.assert_called_once_with(fake_payment_link_id, active=False) is the contract assertion that mocks-only #490 missed. Present and correct.
• Still uses real stripe.Webhook.construct_event HMAC validation — the one mock boundary is outbound Stripe API, exactly right.
• Back-compat result.session_id == fake_payment_link_id asserted. Good — catches any regression in the alias property.

Regen script (scripts/regenerate_tournament_orders.py):

• Prefix-dispatched retirement (plink_ → PaymentLink.modify(active=False), cs_ → Session.expire) handles the mixed state correctly during the #486 recovery — some of the 17 Utah Invitational orders have cs_... ids from the original blast, some have plink_... if this PR has been partially applied.
• Idempotency preserved: _stripe_object_has_order_id handles both object types. Good.
• Dry-run default preserved.

Runbook (docs/tournament-billing-runbook.md):

• TL;DR correctly flips the narrative: the TTL is no longer the tournament-flow problem (it's irrelevant for PaymentLinks). Good.
• Call-site table gains a "Primitive" column distinguishing tournament (PaymentLink) from all others (Checkout Session). Clear.
• "Adding a new tournament-fee call site" section updated to call create_tournament_order_payment_link with legacy alias explicitly documented. Accurate.
• Legacy TTL section kept for non-tournament call sites — correct, since #495 is the revert for #490 and will land first.

Model coverage check:

• Helper now writes order.stripe_checkout_url = payment_link.url. Verified stripe_checkout_url is a real String(500) column on the Order model (migration 047, models.py:453) and is used by services/email_queries.py for the blast URL join. The blast pipeline will therefore see the Payment Link URL without further changes. Good — no new migration needed.

stripe_checkout_session_id column holding plink_... ids:

• Acknowledged in docstring. The webhook matcher keys off metadata.order_id, not this column, so the column is purely for human/script lookup. Prefix-dispatched retirement in the regen script handles both id shapes. A future rename to stripe_checkout_object_id (or similar) is NOT required for this PR — noting as a possible follow-up if the schema stays heterogeneous long term.

BLOCKERS

None.

NITS

• Scope-level merge sequencing: This PR's diff was cut from a base that still contains #490's broken 30d expires_at on the 7 non-tournament call sites. The merge sequence #495 → #496 is correct, and #496 will need a rebase after #495 lands. Expect trivial conflicts in docs/tournament-billing-runbook.md (legacy TTL section) and possibly in tests/test_checkout_session_ttl.py around the non-tournament assertions. Please rebase and re-run the full suite post-#495.
• tests/test_payment_link_real_stripe.py leaves the minted Price object undeactivated. Prices cannot be "deleted" via the public API, only archived. Consider stripe.Price.modify(price.id, active=False) in a teardown so the Stripe test-mode dashboard stays tidy over many CI runs. Non-blocking.
• after_completion.redirect.url only supports one URL — no cancel_url branch. That's a Stripe constraint, not a PR issue, but worth a one-line note in the helper docstring so future readers don't hunt for a missing cancel redirect.
• The _ensure_customer_metadata helper loses the Stripe Customer creation path from the old helper. That was previously caching player.stripe_customer_id. We now never create a Stripe Customer for tournament-fee orders. That's consistent with PaymentLinks accepting a guest checkout — but if any downstream reconciliation code (Stripe dashboard filtering, accounting exports) relied on stripe_customer_id being set on Player, this is a silent behavior change. Please confirm no reporting/filter code upstream depends on player.stripe_customer_id being populated by the tournament path. (Jersey/monthly/tryout paths still set it, so steady-state players will still have one — this only affects tournament-only parents.)
• Minor: log line formatting logger.info(" unknown stripe object prefix for %s -- skipping", obj_id) uses leading-space indentation for visual nesting. Harmless but unconventional — consider logger.info("regen: unknown stripe object prefix %s, skipping", obj_id).

SOP COMPLIANCE

• Branch named after issue: 494-payment-links-tournament ✓
• PR body has ## Summary, ## Changes, ## Test Plan, ## Related ✓
• Related section references issues (#494, #493, #489, #486, #488) ✓
• Tests exist for new functionality (mocked + roundtrip + real-Stripe) ✓
• No secrets committed — BASKETBALL_STRIPE_TEST_API_KEY is env-var referenced, never written ✓
• Live-key guard (sk_test_ prefix assertion) ✓
• No DB migration (reuses stripe_checkout_session_id + pre-existing stripe_checkout_url) ✓
• Back-compat alias preserved for #486 consumer (create_tournament_order_checkout_session → create_tournament_order_payment_link) ✓
• Ruff check + format pass (per PR body) ✓
• Full pytest run noted (1033 pass, 5 pre-existing failures unrelated) ✓

PROCESS OBSERVATIONS

• DORA MTTR impact: This PR directly addresses the #490 regression with a real-Stripe integration test so the same class of bug cannot recur. That's a textbook MTTR-to-CFR conversion — the mocked-only coverage gap that caused the 24h-TTL strand is now closed with a contract that talks to the real Stripe server-side validator. Exactly the right shape of fix.
• Change failure risk: LOW. Scope is surgical (one helper + one webhook gate + one script + tests + docs), back-compat alias preserves the #486 recovery path, webhook signature unchanged, no migration. Non-tournament flows provably untouched.
• Deployment frequency impact: Neutral to positive. The recovery dogfood (regenerating the 17 Utah Invitational orders) can happen immediately post-merge.
• Documentation compliance: Runbook updated in the same PR as the code change. Good.
• Feedback loop on #490: The real-Stripe test is the institutional learning from #490 made executable. That's the correct response to an incident — don't just fix the symptom, build the canary.

VERDICT: APPROVED

Merge-order caveat: land #495 first, then rebase #496 onto the new main, re-run the suite, then merge. No code changes requested on #496 itself.

## PR #496 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Stripe SDK. Payment-integration code on the hot path — correctness matters more than style here. **Scope audit (tournament-only invariant)** — verified clean: - `routes/jersey.py`, `routes/checkout.py`, `routes/register.py`, `routes/admin.py` are untouched. Their Checkout Session call sites remain. Good. - `services/tournament_checkout.py` is the only `PaymentLink.create` call site in `src/`. Good — the helper stays the single source of truth, now for a different primitive. - The webhook deactivation block is correctly gated on BOTH `session.payment_link` being present AND `product.category == ProductCategory.tournament`. Non-tournament orders cannot accidentally trigger `PaymentLink.modify`. Good. **`PaymentLink.create` metadata correctness**: - `order_id` / `player_id` / `product_id` set on the PaymentLink itself (line 136 of the new helper) — this is what Stripe propagates onto `checkout.session.completed.metadata` when a parent pays. - Belt-and-suspenders: same metadata also stamped on `payment_intent_data.metadata` with additional `parent_email`. This matches the existing webhook matcher semantics at `_handle_generic_order_completed` (line 194 of `webhooks.py`) which keys off `session_data["metadata"]["order_id"]`. Matcher will still fire. Good. - `success_url` wired through via `after_completion.redirect.url`. Cancel URL intentionally dropped (PaymentLinks have no cancel concept) — doc comment on the alias explains this clearly. Accurate. **Deactivation placement**: - Correctly lives in `_handle_generic_order_completed` — fired synchronously after `order.status = paid` but before `db.commit()`. That's the right sequence: we want the Order row committed even if deactivation fails, and the exception handler does exactly that (logged-and-swallowed). No race-prone side-channel. Good. - One subtle correctness nit: the deactivation sits between jersey-sync and `db.commit()`. If `PaymentLink.modify` succeeds but the commit later fails, we end up with a deactivated Stripe link for an un-fulfilled order. That's a VERY narrow window (nothing between the two can realistically fail at this point) and the worse alternative (deactivate after commit, fail, leak a charged URL) is also ugly. Acceptable tradeoff — parents don't double-pay on a paid order, so deactivating early is the right call. **Real-Stripe integration test** (`tests/test_payment_link_real_stripe.py`): - Correctly opt-in via `BASKETBALL_STRIPE_TEST_API_KEY` — `pytest.mark.skipif` at module level means local `pytest` runs skip cleanly. Good. - Live-key guard (`assert key.startswith("sk_test_")`) is load-bearing. This is the safety net the #490 incident was missing. Keep this assertion — it is what makes the test shippable to CI without fear. - Actually re-fetches the link via `stripe.PaymentLink.retrieve(...)` and asserts against server-side representation (not just the local return value). This is the coverage #490 lacked. Good. - Asserts `active is False` after `modify` — this is the contract that backs the webhook deactivation hook. Good. - Minor concern: the test does not clean up the Payment Link explicitly beyond deactivation. Deactivated test-mode Payment Links clutter the Stripe dashboard over time but do not incur cost. Acceptable for now; a followup to add an `after`/`finally` that also deactivates `price` could tidy. NIT only. **Webhook roundtrip test** (`tests/test_tournament_webhook_roundtrip.py`): - Patches updated from `Session.create` / `Customer.create` to `PaymentLink.create` / `Price.create` / `PaymentLink.modify`. Correct set. - `_build_checkout_completed_event` now carries `payment_link` top-level field when supplied — this matches how Stripe actually populates sessions that materialized from PaymentLink clicks. The webhook's deactivation gate reads `session_data.get("payment_link")` so this fixture exercises the real branch. Good. - `mock_modify.assert_called_once_with(fake_payment_link_id, active=False)` is the contract assertion that mocks-only #490 missed. Present and correct. - Still uses real `stripe.Webhook.construct_event` HMAC validation — the one mock boundary is outbound Stripe API, exactly right. - Back-compat `result.session_id == fake_payment_link_id` asserted. Good — catches any regression in the alias property. **Regen script (`scripts/regenerate_tournament_orders.py`)**: - Prefix-dispatched retirement (`plink_` → `PaymentLink.modify(active=False)`, `cs_` → `Session.expire`) handles the mixed state correctly during the #486 recovery — some of the 17 Utah Invitational orders have `cs_...` ids from the original blast, some have `plink_...` if this PR has been partially applied. - Idempotency preserved: `_stripe_object_has_order_id` handles both object types. Good. - Dry-run default preserved. **Runbook (`docs/tournament-billing-runbook.md`)**: - TL;DR correctly flips the narrative: the TTL is no longer the tournament-flow problem (it's irrelevant for PaymentLinks). Good. - Call-site table gains a "Primitive" column distinguishing tournament (PaymentLink) from all others (Checkout Session). Clear. - "Adding a new tournament-fee call site" section updated to call `create_tournament_order_payment_link` with legacy alias explicitly documented. Accurate. - Legacy TTL section kept for non-tournament call sites — correct, since #495 is the revert for #490 and will land first. **Model coverage check**: - Helper now writes `order.stripe_checkout_url = payment_link.url`. Verified `stripe_checkout_url` is a real `String(500)` column on the `Order` model (migration 047, `models.py:453`) and is used by `services/email_queries.py` for the blast URL join. The blast pipeline will therefore see the Payment Link URL without further changes. Good — no new migration needed. **`stripe_checkout_session_id` column holding `plink_...` ids**: - Acknowledged in docstring. The webhook matcher keys off `metadata.order_id`, not this column, so the column is purely for human/script lookup. Prefix-dispatched retirement in the regen script handles both id shapes. A future rename to `stripe_checkout_object_id` (or similar) is NOT required for this PR — noting as a possible follow-up if the schema stays heterogeneous long term. ### BLOCKERS None. ### NITS - **Scope-level merge sequencing**: This PR's diff was cut from a base that still contains #490's broken 30d `expires_at` on the 7 non-tournament call sites. The merge sequence #495 → #496 is correct, and #496 will need a rebase after #495 lands. Expect trivial conflicts in `docs/tournament-billing-runbook.md` (legacy TTL section) and possibly in `tests/test_checkout_session_ttl.py` around the non-tournament assertions. Please rebase and re-run the full suite post-#495. - `tests/test_payment_link_real_stripe.py` leaves the minted `Price` object undeactivated. Prices cannot be "deleted" via the public API, only archived. Consider `stripe.Price.modify(price.id, active=False)` in a teardown so the Stripe test-mode dashboard stays tidy over many CI runs. Non-blocking. - `after_completion.redirect.url` only supports one URL — no `cancel_url` branch. That's a Stripe constraint, not a PR issue, but worth a one-line note in the helper docstring so future readers don't hunt for a missing cancel redirect. - The `_ensure_customer_metadata` helper loses the Stripe `Customer` creation path from the old helper. That was previously caching `player.stripe_customer_id`. We now never create a Stripe Customer for tournament-fee orders. That's consistent with PaymentLinks accepting a guest checkout — but if any downstream reconciliation code (Stripe dashboard filtering, accounting exports) relied on `stripe_customer_id` being set on `Player`, this is a silent behavior change. Please confirm no reporting/filter code upstream depends on `player.stripe_customer_id` being populated by the tournament path. (Jersey/monthly/tryout paths still set it, so steady-state players will still have one — this only affects tournament-only parents.) - Minor: log line formatting `logger.info(" unknown stripe object prefix for %s -- skipping", obj_id)` uses leading-space indentation for visual nesting. Harmless but unconventional — consider `logger.info("regen: unknown stripe object prefix %s, skipping", obj_id)`. ### SOP COMPLIANCE - [x] Branch named after issue: `494-payment-links-tournament` ✓ - [x] PR body has ## Summary, ## Changes, ## Test Plan, ## Related ✓ - [x] Related section references issues (#494, #493, #489, #486, #488) ✓ - [x] Tests exist for new functionality (mocked + roundtrip + real-Stripe) ✓ - [x] No secrets committed — `BASKETBALL_STRIPE_TEST_API_KEY` is env-var referenced, never written ✓ - [x] Live-key guard (`sk_test_` prefix assertion) ✓ - [x] No DB migration (reuses `stripe_checkout_session_id` + pre-existing `stripe_checkout_url`) ✓ - [x] Back-compat alias preserved for #486 consumer (`create_tournament_order_checkout_session` → `create_tournament_order_payment_link`) ✓ - [x] Ruff check + format pass (per PR body) ✓ - [x] Full pytest run noted (1033 pass, 5 pre-existing failures unrelated) ✓ ### PROCESS OBSERVATIONS - **DORA MTTR impact**: This PR directly addresses the #490 regression with a real-Stripe integration test so the same class of bug cannot recur. That's a textbook MTTR-to-CFR conversion — the mocked-only coverage gap that caused the 24h-TTL strand is now closed with a contract that talks to the real Stripe server-side validator. Exactly the right shape of fix. - **Change failure risk**: LOW. Scope is surgical (one helper + one webhook gate + one script + tests + docs), back-compat alias preserves the #486 recovery path, webhook signature unchanged, no migration. Non-tournament flows provably untouched. - **Deployment frequency impact**: Neutral to positive. The recovery dogfood (regenerating the 17 Utah Invitational orders) can happen immediately post-merge. - **Documentation compliance**: Runbook updated in the same PR as the code change. Good. - **Feedback loop on #490**: The real-Stripe test is the institutional learning from #490 made executable. That's the correct response to an incident — don't just fix the symptom, build the canary. ### VERDICT: APPROVED Merge-order caveat: land #495 first, then rebase #496 onto the new main, re-run the suite, then merge. No code changes requested on #496 itself.

forgejo_admin force-pushed 494-payment-links-tournament from a8bc0bf7e0 to 4c579ee984 2026-04-17 22:24:32 +00:00 Compare

forgejo_admin merged commit 4a4b88a43f into main 2026-04-17 22:33:59 +00:00

forgejo_admin deleted branch 494-payment-links-tournament 2026-04-17 22:34:00 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-17 22:34:01 +00:00

feat: mint tournament-fee checkout URLs as Stripe Payment Links (#494) (#496)

forgejo_admin referenced this pull request 2026-04-18 05:29:43 +00:00

feat: mint monthly-fee checkout URLs as Stripe Payment Links (#498) #499

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!496

No description provided.