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

Merged

forgejo_admin merged 2 commits from 498-monthly-payment-links into main 2026-04-18 06:06:42 +00:00

Conversation 2 Commits 2 Files changed 7 +1006 -174

forgejo_admin commented 2026-04-18 05:22:19 +00:00

Owner

Copy link

Summary

Migrate the monthly-fee flow from Stripe Checkout Sessions (24h cap) to Stripe Payment Links (no expiry), mirroring #494's tournament-fee migration. Parents who open a monthly-fee email a day or two late now reach a working Stripe page instead of a dead URL. Three monthly-specific deltas from #494 (setup_future_usage, per-Order amounts, redirect-preserving email template) are carried over verbatim.

Closes #498. Downstream consumer: recovery ticket #497 can use the new helper without additional plumbing.

Changes

• src/basketball_api/services/monthly_checkout.py (new) -- create_monthly_order_payment_link helper. Analog of services/tournament_checkout.py::create_tournament_order_payment_link, with setup_future_usage: "off_session" on the PaymentLink's payment_intent_data, order.amount_cents driving the Price (not product.price_cents), and Payment Link id + URL persisted onto the Order.
• src/basketball_api/routes/checkout.py -- first_payment_checkout now calls the new helper instead of stripe.checkout.Session.create. Fresh-pending reuse redirects straight to Order.stripe_checkout_url (no Stripe round-trip needed since Payment Links don't expire). Non-monthly call sites at checkout.py:247, checkout.py:553, admin.py:2005 are untouched.
• src/basketball_api/routes/webhooks.py -- _handle_generic_order_completed deactivation gate widened additively: ProductCategory.monthly joins ProductCategory.tournament. Jersey / tryout / generic continue to skip deactivation.
• tests/test_first_payment.py -- rewrote mocks for the Payment Link path (stripe.Price.create + stripe.PaymentLink.create live on the new services.monthly_checkout module). Added test_payment_link_metadata_carries_order_id and test_payment_link_uses_order_amount_cents. Preserved test_setup_future_usage_passed_to_stripe for the new path.
• tests/test_monthly_payment_link_real_stripe.py (new) -- opt-in real-Stripe integration test (BASKETBALL_STRIPE_TEST_API_KEY). Re-fetches the PaymentLink from Stripe and asserts payment_intent_data.setup_future_usage == "off_session" from the authoritative server state, not the local helper kwargs. Covers the #490 regression class that mock-only tests missed.
• tests/test_monthly_webhook_roundtrip.py (new) -- real-signature webhook roundtrip for monthly, plus a guard test asserting PaymentLink.modify is NOT called for jersey-category orders (deactivation gate is strictly additive).

Design Decisions

• Stripe SDK path for setup_future_usage: stripe.PaymentLink.PaymentIntentData -- i.e. stripe.PaymentLink.create(payment_intent_data={"setup_future_usage": "off_session", ...}). Verified against stripe==14.4.1 via stripe.PaymentLink.PaymentIntentData.__annotations__. Same shape as the existing Session call's payment_intent_data kwarg, just on a different Stripe object.
• Dropped explicit stripe.Customer.create: PaymentLink does not accept a customer kwarg. When setup_future_usage is set, Stripe auto-creates a Customer and saves the card at click time. The webhook continues to receive checkout.session.completed with the PaymentIntent attached; stripe_customer_id sync, if desired, is a follow-up orthogonal to this PR. Consistent with #494's tournament helper.
• Fresh-pending reuse without Stripe round-trip: Payment Links don't expire, so a pending order with a persisted stripe_checkout_url can redirect straight to that URL. No Session.retrieve equivalent needed. Amount-mismatch path still cancels + mints a fresh link.
• Email template untouched: Email still embeds {base_url}/checkout/first-payment?token=... -- the redirect endpoint now returns the persisted Payment Link URL. Preserves the per-click contract_status re-check required by the issue.
• Backward-compat columns reused: Order.stripe_checkout_session_id stores the plink_... id (no migration required). The webhook matcher keys off metadata.order_id, not this column, so mixed Session / PaymentLink ids are safe.

Test Plan

• ruff format --check + ruff check clean
• tests/test_first_payment.py -- 12/12 passed (8 carried over + 4 new)
• tests/test_monthly_webhook_roundtrip.py -- 2/2 passed (happy path deactivates link, jersey guard does not)
• tests/test_tournament_webhook_roundtrip.py + tests/test_checkout.py + tests/test_webhooks.py + tests/test_first_payment_blast.py -- 41/41 passed (no regression on non-monthly paths)
• tests/test_contract.py + contract flow tests -- 51/51 passed
• Full suite: 1029 passed, 2 skipped (real-Stripe tests opt-in), 9 pre-existing failures unrelated to this PR (verified on main with same failures)
• CI will inject BASKETBALL_STRIPE_TEST_API_KEY so the real-Stripe integration test runs against test-mode Stripe (not mock-only) -- confirms the 24h-cap class of regression is defended server-side

Review Checklist

• Acceptance criteria from #498 all addressed:
  • URLs don't expire (Payment Links replace Checkout Sessions)
  • order_id in Stripe metadata on both the Payment Link and the derived PaymentIntent
  • Payment Link deactivated on payment success (webhook gate widened to include ProductCategory.monthly)
  • Per-parent amount_cents flows through (Price minted from order.amount_cents, not product.price_cents)
  • setup_future_usage: "off_session" carried over via payment_intent_data
  • Email template untouched; redirect preserves contract-status re-check on click
  • Webhook gate additive; tournament branch intact; jersey/tryout/generic skip deactivation
  • Non-monthly call sites untouched
• At least one real-Stripe integration test included (opt-in via BASKETBALL_STRIPE_TEST_API_KEY)
• No CHECKOUT_SESSION_TTL_SECONDS or similar TTL constant introduced (the #490 dead end)
• Webhook handler signature unchanged
• No Stripe URLs embedded directly in email templates
• Ruff clean, no new regressions in full test suite

Related

• Forgejo issue: #498
• Pattern source: #494 (tournament-fee migration, merged)
• Downstream consumer: #497 (monthly-fee recovery)
• Dead-end avoided: #490 (30-day TTL on Sessions -- reverted in #493)

Related Notes

• Memory: feedback_retrieve_before_theorize.md -- first tool on any payment incident is the provider's retrieve API; real-Stripe test enforces this for the monthly path.
• SOP context: tournament-fee migration #494 is the acknowledged pattern; this PR is the direct analog for monthly fees.
• Runbook: no new runbook entries needed; docs/tournament-billing-runbook.md covers the Payment Link operational surface and monthly follows the same conventions.

## Summary Migrate the monthly-fee flow from Stripe Checkout Sessions (24h cap) to Stripe Payment Links (no expiry), mirroring #494's tournament-fee migration. Parents who open a monthly-fee email a day or two late now reach a working Stripe page instead of a dead URL. Three monthly-specific deltas from #494 (`setup_future_usage`, per-Order amounts, redirect-preserving email template) are carried over verbatim. Closes #498. Downstream consumer: recovery ticket #497 can use the new helper without additional plumbing. ## Changes - **`src/basketball_api/services/monthly_checkout.py`** (new) -- `create_monthly_order_payment_link` helper. Analog of `services/tournament_checkout.py::create_tournament_order_payment_link`, with `setup_future_usage: "off_session"` on the PaymentLink's `payment_intent_data`, `order.amount_cents` driving the Price (not `product.price_cents`), and Payment Link id + URL persisted onto the Order. - **`src/basketball_api/routes/checkout.py`** -- `first_payment_checkout` now calls the new helper instead of `stripe.checkout.Session.create`. Fresh-pending reuse redirects straight to `Order.stripe_checkout_url` (no Stripe round-trip needed since Payment Links don't expire). Non-monthly call sites at `checkout.py:247`, `checkout.py:553`, `admin.py:2005` are untouched. - **`src/basketball_api/routes/webhooks.py`** -- `_handle_generic_order_completed` deactivation gate widened additively: `ProductCategory.monthly` joins `ProductCategory.tournament`. Jersey / tryout / generic continue to skip deactivation. - **`tests/test_first_payment.py`** -- rewrote mocks for the Payment Link path (`stripe.Price.create` + `stripe.PaymentLink.create` live on the new `services.monthly_checkout` module). Added `test_payment_link_metadata_carries_order_id` and `test_payment_link_uses_order_amount_cents`. Preserved `test_setup_future_usage_passed_to_stripe` for the new path. - **`tests/test_monthly_payment_link_real_stripe.py`** (new) -- opt-in real-Stripe integration test (`BASKETBALL_STRIPE_TEST_API_KEY`). Re-fetches the PaymentLink from Stripe and asserts `payment_intent_data.setup_future_usage == "off_session"` from the authoritative server state, not the local helper kwargs. Covers the #490 regression class that mock-only tests missed. - **`tests/test_monthly_webhook_roundtrip.py`** (new) -- real-signature webhook roundtrip for monthly, plus a guard test asserting `PaymentLink.modify` is NOT called for jersey-category orders (deactivation gate is strictly additive). ## Design Decisions - **Stripe SDK path for `setup_future_usage`:** `stripe.PaymentLink.PaymentIntentData` -- i.e. `stripe.PaymentLink.create(payment_intent_data={"setup_future_usage": "off_session", ...})`. Verified against `stripe==14.4.1` via `stripe.PaymentLink.PaymentIntentData.__annotations__`. Same shape as the existing Session call's `payment_intent_data` kwarg, just on a different Stripe object. - **Dropped explicit `stripe.Customer.create`:** PaymentLink does not accept a `customer` kwarg. When `setup_future_usage` is set, Stripe auto-creates a Customer and saves the card at click time. The webhook continues to receive `checkout.session.completed` with the PaymentIntent attached; `stripe_customer_id` sync, if desired, is a follow-up orthogonal to this PR. Consistent with #494's tournament helper. - **Fresh-pending reuse without Stripe round-trip:** Payment Links don't expire, so a pending order with a persisted `stripe_checkout_url` can redirect straight to that URL. No `Session.retrieve` equivalent needed. Amount-mismatch path still cancels + mints a fresh link. - **Email template untouched:** Email still embeds `{base_url}/checkout/first-payment?token=...` -- the redirect endpoint now returns the persisted Payment Link URL. Preserves the per-click `contract_status` re-check required by the issue. - **Backward-compat columns reused:** `Order.stripe_checkout_session_id` stores the `plink_...` id (no migration required). The webhook matcher keys off `metadata.order_id`, not this column, so mixed Session / PaymentLink ids are safe. ## Test Plan - [x] `ruff format --check` + `ruff check` clean - [x] `tests/test_first_payment.py` -- 12/12 passed (8 carried over + 4 new) - [x] `tests/test_monthly_webhook_roundtrip.py` -- 2/2 passed (happy path deactivates link, jersey guard does not) - [x] `tests/test_tournament_webhook_roundtrip.py` + `tests/test_checkout.py` + `tests/test_webhooks.py` + `tests/test_first_payment_blast.py` -- 41/41 passed (no regression on non-monthly paths) - [x] `tests/test_contract.py` + contract flow tests -- 51/51 passed - [x] Full suite: 1029 passed, 2 skipped (real-Stripe tests opt-in), 9 pre-existing failures unrelated to this PR (verified on `main` with same failures) - [ ] CI will inject `BASKETBALL_STRIPE_TEST_API_KEY` so the real-Stripe integration test runs against test-mode Stripe (not mock-only) -- confirms the 24h-cap class of regression is defended server-side ## Review Checklist - [x] Acceptance criteria from #498 all addressed: - [x] URLs don't expire (Payment Links replace Checkout Sessions) - [x] `order_id` in Stripe metadata on both the Payment Link and the derived PaymentIntent - [x] Payment Link deactivated on payment success (webhook gate widened to include `ProductCategory.monthly`) - [x] Per-parent `amount_cents` flows through (Price minted from `order.amount_cents`, not `product.price_cents`) - [x] `setup_future_usage: "off_session"` carried over via `payment_intent_data` - [x] Email template untouched; redirect preserves contract-status re-check on click - [x] Webhook gate additive; tournament branch intact; jersey/tryout/generic skip deactivation - [x] Non-monthly call sites untouched - [x] At least one real-Stripe integration test included (opt-in via `BASKETBALL_STRIPE_TEST_API_KEY`) - [x] No `CHECKOUT_SESSION_TTL_SECONDS` or similar TTL constant introduced (the #490 dead end) - [x] Webhook handler signature unchanged - [x] No Stripe URLs embedded directly in email templates - [x] Ruff clean, no new regressions in full test suite ## Related - Forgejo issue: #498 - Pattern source: #494 (tournament-fee migration, merged) - Downstream consumer: #497 (monthly-fee recovery) - Dead-end avoided: #490 (30-day TTL on Sessions -- reverted in #493) ## Related Notes - Memory: `feedback_retrieve_before_theorize.md` -- first tool on any payment incident is the provider's retrieve API; real-Stripe test enforces this for the monthly path. - SOP context: tournament-fee migration #494 is the acknowledged pattern; this PR is the direct analog for monthly fees. - Runbook: no new runbook entries needed; `docs/tournament-billing-runbook.md` covers the Payment Link operational surface and monthly follows the same conventions.

forgejo_admin added 1 commit 2026-04-18 05:22:19 +00:00

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

Mirror the tournament-fee migration from #494 for the monthly-fee flow.
Parents who receive a monthly-fee payment email can now click the link
any time after receipt without hitting a 24h-expired Checkout Session.
Payment Links do not expire Stripe-side.

Three monthly-specific deltas from the tournament helper:

- ``setup_future_usage: "off_session"`` carried via
  ``payment_intent_data`` on ``stripe.PaymentLink.create`` so the card is
  saved to the Customer for recurring off-session charges. Regression
  coverage: ``test_setup_future_usage_passed_to_stripe`` in
  ``tests/test_first_payment.py`` plus a real-Stripe assertion in
  ``tests/test_monthly_payment_link_real_stripe.py`` that re-fetches
  the PaymentLink from Stripe and reads
  ``payment_intent_data.setup_future_usage`` off the server-authoritative
  state.
- Per-Order amount: Price is minted from ``order.amount_cents`` (not
  ``product.price_cents``) so scholarships / prorations / custom
  contracts flow through unchanged. Asserted in
  ``test_payment_link_uses_order_amount_cents``.
- Email redirect preserved: the email template still embeds
  ``{base_url}/checkout/first-payment?token=...``. The
  ``first_payment_checkout`` endpoint redirects to
  ``Order.stripe_checkout_url`` (the persisted Payment Link URL), so
  contract-status re-check on click still fires and the email copy is
  unchanged.

Webhook deactivation gate widened additively in
``routes/webhooks.py:_handle_generic_order_completed`` to include
``ProductCategory.monthly`` alongside ``ProductCategory.tournament``.
Jersey / tryout / generic flows remain untouched (asserted in
``tests/test_monthly_webhook_roundtrip.py::test_jersey_order_does_not_deactivate_payment_link``).

Non-monthly call sites (``routes/checkout.py:247``,
``routes/checkout.py:553``, ``routes/admin.py:2005``) are NOT touched.

Real-Stripe integration test included (opt-in via
``BASKETBALL_STRIPE_TEST_API_KEY``) -- mock-only coverage missed the
24h cap in #490, same rigor applies here.

Stripe SDK decision: ``setup_future_usage`` lives on
``stripe.PaymentLink.PaymentIntentData`` (i.e.
``PaymentLink.create(payment_intent_data={"setup_future_usage":
"off_session"})``), same path as the existing Session call. Verified
against stripe-python 14.4.1 type hints.

Closes #498

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

forgejo_admin commented 2026-04-18 05:29:43 +00:00

Author

Owner

Copy link

PR #499 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Stripe SDK. PEP 484 type hints used throughout. PEP 257 docstrings well-written and explain "why" (load-bearing rationale for Payment Link choice, monthly-specific deltas, scope boundaries). from __future__ import annotations properly used.

Stripe / Payments correctness:

• stripe.Price.create + stripe.PaymentLink.create is the correct primitive — Payment Links don't accept inline price_data (called out in the docstring). Good.
• setup_future_usage: "off_session" is correctly carried via payment_intent_data on the Payment Link — verified by both the unit test (test_setup_future_usage_passed_to_stripe) and the real-Stripe integration test (test_mint_monthly_payment_link_against_real_stripe) which re-fetches from Stripe to inspect authoritative server state. This addresses the #498 AC and prevents a silent recurring-billing regression.
• order_id metadata on both the PaymentLink and the PaymentIntent — belt-and-suspenders matches the tournament helper pattern. Webhook matcher will resolve correctly.
• Per-order amount via order.amount_cents (not product.price_cents) — correctly preserves scholarships/prorations. Asserted in test_payment_link_uses_order_amount_cents.
• Order column reuse: stripe_checkout_session_id now stores plink_.... Documented in the docstring, and the Order model already has stripe_checkout_url (verified at src/basketball_api/models.py:453). No migration required.
• 500-guard for missing URL on RedirectResponse is the right call — silent None redirect would be a worse failure mode.

Webhook gate widening:

• Change in routes/webhooks.py is strictly additive: product.category in (ProductCategory.tournament, ProductCategory.monthly). Tournament branch preserved. Jersey/tryout/generic untouched.
• The test_jersey_order_does_not_deactivate_payment_link test explicitly proves the gate is category-keyed, not presence-keyed, even when a payment_link field is defensively included on a non-monthly/non-tournament event. Excellent regression coverage.
• Log message updated to interpolate product.category.value so observability remains category-aware.

Test coverage:

• All 11 mocked unit tests in test_first_payment.py were rewritten cleanly to patch basketball_api.services.monthly_checkout.stripe instead of the route's stripe ref. The eager-import pattern at the top is necessary for @patch("basketball_api.services.monthly_checkout.stripe") to resolve — comment explains why.
• New test_monthly_payment_link_real_stripe.py follows the lesson learned from #490 (mocks "passed" against an invalid 30-day TTL that Stripe rejected at runtime). It re-fetches the link from Stripe to inspect server-side setup_future_usage, asserts expires_at is absent, and exercises deactivation. Properly skips if BASKETBALL_STRIPE_TEST_API_KEY env var missing — but per feedback_qa_ci_blockers.md, opt-in skips that go silent in CI are a blocker risk. See SOP COMPLIANCE below.
• test_monthly_webhook_roundtrip.py exercises the real stripe.Webhook.construct_event signature validator end-to-end, only mocking outbound calls. The complement test (jersey does not trigger deactivation) is a valuable additive-change guard.

FastAPI / SQLAlchemy:

• Dependency injection pattern preserved.
• Order is db.add + db.flush + then helper commits — consistent with prior pattern, no transaction leak.
• getattr(payment_link, "url", None) is defensive against Stripe SDK shape changes; matches the tournament helper.

BLOCKERS

None.

NITS

1. Near-duplicate of tournament_checkout.py (DRY in payments path). The new services/monthly_checkout.py is structurally ~95% identical to services/tournament_checkout.create_tournament_order_payment_link. Differences:

   • Line-item product name: f"First Monthly Payment – {player.name}" vs product.name
   • Adds setup_future_usage: "off_session" to payment_intent_data
   • Adds parent_id + parent_email keys to PaymentIntent metadata

   Two payment helpers diverging is a legitimate maintenance risk (the next time a Stripe API change lands — e.g. a new mandatory field — both must be updated). Consider a future refactor to a single create_order_payment_link(db, order, *, line_item_name=None, setup_future_usage=None, extra_pi_metadata=None) and have tournament_checkout + monthly_checkout become thin wrappers. Not a blocker because (a) the divergence is currently small + well-documented in both modules' module docstrings, (b) the analog _payment_intent_metadata and _ensure_customer_metadata helpers are scoped private and their signatures already differ. Track as Epilogue subphase per feedback_nits_to_epilogue.md.

2. Real-Stripe test cleanup: test_mint_monthly_payment_link_against_real_stripe deactivates the link at the end (good), but does not delete/archive the test-mode Price or Product it creates. Over time test-mode dashboards will accumulate Integration Test Monthly Fee entries. Tournament analog likely has the same characteristic; tracking-only.

3. Helper accepts success_url kwarg but no test exercises override. Defensible (tournament analog also has it for symmetry), but future readers may wonder if it's load-bearing. Consider one-liner test or noqa comment.

4. Bare getattr(payment_link, "url", None) returns None in two places. PaymentLink.create always returns a URL on success, but the persisted-then-checked path goes:

   • persist whatever getattr returns to order.stripe_checkout_url
   • then check if not result.url: and raise 500

   If Stripe ever returns the object pre-URL (it doesn't today), we'd persist None to the DB before raising. Cosmetic — the order is pending and the next click will replace it. Tracking-only.

SOP COMPLIANCE

• Branch named after issue: 498-monthly-payment-links follows {issue-number}-{kebab-case-purpose} convention
• PR title references parent issue: feat: mint monthly-fee checkout URLs as Stripe Payment Links (#498)
• PR body has Summary / Changes / Primary Benefit; references analog PR #494 + parent #498
• Tests exist for new functionality (mocked unit tests + real-Stripe integration test + signed-webhook roundtrip + complement-guard)
• No secrets committed (test uses env-injected BASKETBALL_STRIPE_TEST_API_KEY, validated to start with sk_test_)
• Webhook change is strictly additive (tournament branch preserved); jersey explicit guard test included
• No scope creep — only routes/checkout.py (first_payment endpoint), routes/webhooks.py (gate widening), new services/monthly_checkout.py, three test files
• Email template unchanged — preserves redirect for contract-status re-check on click (per #498 AC)
• CI service-container verification needed: The real-Stripe test is opt-in skipped without BASKETBALL_STRIPE_TEST_API_KEY. Per feedback_qa_ci_blockers.md, "tests that can't run in CI are blockers, not nits." Confirm CI has this secret wired (Woodpecker repo secret) — otherwise this becomes the same silent-skip pattern that allowed #490's 30-day TTL bug through. The comment in the file says "CI injects this secret for the test step" — please verify a Woodpecker secret exists on forgejo_admin/basketball-api with that name. If it does, this checkbox flips to checked.

PROCESS OBSERVATIONS

Deployment frequency: Clean analog of the tournament migration (#494/#496). Reusing the proven shape — separate helper module, identical webhook gate widening pattern, identical real-Stripe test pattern — is the right move. Cherry-pickable, low-risk change.

Change failure risk: Low.

• Webhook change is strictly additive with explicit complement guard.
• Real-Stripe test catches the failure mode that #490 missed (server-side validation that mocks pass through).
• Stale-amount regen path preserved (test_first_payment_fresh_pending_amount_changed).
• Customer creation path was dropped (Payment Links create the Customer on first payment when setup_future_usage is set) — this is documented in the test module docstring, but it's worth manual validation: the next-month off-session charge needs a Customer attached to the PaymentMethod. Recommend post-merge validation per feedback_validate_before_done.md confirms one full end-to-end click → pay → next-month charge cycle in test mode before mass blast.

Documentation gaps: The monthly billing runbook (analog of docs/tournament-billing-runbook.md referenced in PR #496) is not updated in this PR. If one exists for monthly, refresh; if not, log a follow-up item to mirror the tournament runbook for monthly. Track as Epilogue subphase.

DORA mapping: Resolves a CFR (parents losing 78% of payers from 24h-expiring URLs per #488). Reduces MTTR for the same incident class (Payment Links don't expire, so the recovery loop from #486/#497 becomes unnecessary). Strengthens the payments pipeline observability story Lucas is currently shipping in 290-payment-pipeline-observability.

VERDICT: APPROVED

Conditional on Woodpecker secret BASKETBALL_STRIPE_TEST_API_KEY existing on the basketball-api repo so the real-Stripe test actually runs in CI (per feedback_qa_ci_blockers.md). If it doesn't, treat that as a blocker and wire it before merge. All other findings are nits and can be tracked in the Epilogue.

## PR #499 Review ### DOMAIN REVIEW **Stack:** Python / FastAPI / SQLAlchemy / Stripe SDK. PEP 484 type hints used throughout. PEP 257 docstrings well-written and explain "why" (load-bearing rationale for Payment Link choice, monthly-specific deltas, scope boundaries). `from __future__ import annotations` properly used. **Stripe / Payments correctness:** - `stripe.Price.create` + `stripe.PaymentLink.create` is the correct primitive — Payment Links don't accept inline `price_data` (called out in the docstring). Good. - `setup_future_usage: "off_session"` is correctly carried via `payment_intent_data` on the Payment Link — verified by both the unit test (`test_setup_future_usage_passed_to_stripe`) and the real-Stripe integration test (`test_mint_monthly_payment_link_against_real_stripe`) which re-fetches from Stripe to inspect authoritative server state. This addresses the #498 AC and prevents a silent recurring-billing regression. - `order_id` metadata on both the PaymentLink and the PaymentIntent — belt-and-suspenders matches the tournament helper pattern. Webhook matcher will resolve correctly. - Per-order amount via `order.amount_cents` (not `product.price_cents`) — correctly preserves scholarships/prorations. Asserted in `test_payment_link_uses_order_amount_cents`. - Order column reuse: `stripe_checkout_session_id` now stores `plink_...`. Documented in the docstring, and the Order model already has `stripe_checkout_url` (verified at `src/basketball_api/models.py:453`). No migration required. - 500-guard for missing URL on `RedirectResponse` is the right call — silent `None` redirect would be a worse failure mode. **Webhook gate widening:** - Change in `routes/webhooks.py` is strictly additive: `product.category in (ProductCategory.tournament, ProductCategory.monthly)`. Tournament branch preserved. Jersey/tryout/generic untouched. - The `test_jersey_order_does_not_deactivate_payment_link` test explicitly proves the gate is category-keyed, not presence-keyed, even when a `payment_link` field is defensively included on a non-monthly/non-tournament event. Excellent regression coverage. - Log message updated to interpolate `product.category.value` so observability remains category-aware. **Test coverage:** - All 11 mocked unit tests in `test_first_payment.py` were rewritten cleanly to patch `basketball_api.services.monthly_checkout.stripe` instead of the route's stripe ref. The eager-import pattern at the top is necessary for `@patch("basketball_api.services.monthly_checkout.stripe")` to resolve — comment explains why. - New `test_monthly_payment_link_real_stripe.py` follows the lesson learned from #490 (mocks "passed" against an invalid 30-day TTL that Stripe rejected at runtime). It re-fetches the link from Stripe to inspect server-side `setup_future_usage`, asserts `expires_at` is absent, and exercises deactivation. Properly skips if `BASKETBALL_STRIPE_TEST_API_KEY` env var missing — but **per `feedback_qa_ci_blockers.md`, opt-in skips that go silent in CI are a blocker risk**. See SOP COMPLIANCE below. - `test_monthly_webhook_roundtrip.py` exercises the real `stripe.Webhook.construct_event` signature validator end-to-end, only mocking outbound calls. The complement test (jersey does not trigger deactivation) is a valuable additive-change guard. **FastAPI / SQLAlchemy:** - Dependency injection pattern preserved. - Order is `db.add` + `db.flush` + then helper commits — consistent with prior pattern, no transaction leak. - `getattr(payment_link, "url", None)` is defensive against Stripe SDK shape changes; matches the tournament helper. ### BLOCKERS None. ### NITS 1. **Near-duplicate of `tournament_checkout.py`** (DRY in payments path). The new `services/monthly_checkout.py` is structurally ~95% identical to `services/tournament_checkout.create_tournament_order_payment_link`. Differences: - Line-item product name: `f"First Monthly Payment – {player.name}"` vs `product.name` - Adds `setup_future_usage: "off_session"` to `payment_intent_data` - Adds `parent_id` + `parent_email` keys to PaymentIntent metadata Two payment helpers diverging is a legitimate maintenance risk (the next time a Stripe API change lands — e.g. a new mandatory field — both must be updated). Consider a future refactor to a single `create_order_payment_link(db, order, *, line_item_name=None, setup_future_usage=None, extra_pi_metadata=None)` and have `tournament_checkout` + `monthly_checkout` become thin wrappers. Not a blocker because (a) the divergence is currently small + well-documented in both modules' module docstrings, (b) the analog `_payment_intent_metadata` and `_ensure_customer_metadata` helpers are scoped private and their signatures already differ. **Track as Epilogue subphase** per `feedback_nits_to_epilogue.md`. 2. **Real-Stripe test cleanup**: `test_mint_monthly_payment_link_against_real_stripe` deactivates the link at the end (good), but does not delete/archive the test-mode `Price` or `Product` it creates. Over time test-mode dashboards will accumulate `Integration Test Monthly Fee` entries. Tournament analog likely has the same characteristic; tracking-only. 3. **Helper accepts `success_url` kwarg but no test exercises override.** Defensible (tournament analog also has it for symmetry), but future readers may wonder if it's load-bearing. Consider one-liner test or noqa comment. 4. **Bare `getattr(payment_link, "url", None)` returns `None` in two places.** `PaymentLink.create` always returns a URL on success, but the persisted-then-checked path goes: - persist whatever `getattr` returns to `order.stripe_checkout_url` - then check `if not result.url:` and raise 500 If Stripe ever returns the object pre-URL (it doesn't today), we'd persist `None` to the DB before raising. Cosmetic — the order is `pending` and the next click will replace it. Tracking-only. ### SOP COMPLIANCE - [x] Branch named after issue: `498-monthly-payment-links` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR title references parent issue: `feat: mint monthly-fee checkout URLs as Stripe Payment Links (#498)` - [x] PR body has Summary / Changes / Primary Benefit; references analog PR #494 + parent #498 - [x] Tests exist for new functionality (mocked unit tests + real-Stripe integration test + signed-webhook roundtrip + complement-guard) - [x] No secrets committed (test uses env-injected `BASKETBALL_STRIPE_TEST_API_KEY`, validated to start with `sk_test_`) - [x] Webhook change is strictly additive (tournament branch preserved); jersey explicit guard test included - [x] No scope creep — only `routes/checkout.py` (first_payment endpoint), `routes/webhooks.py` (gate widening), new `services/monthly_checkout.py`, three test files - [x] Email template unchanged — preserves redirect for contract-status re-check on click (per #498 AC) - [ ] **CI service-container verification needed**: The real-Stripe test is opt-in skipped without `BASKETBALL_STRIPE_TEST_API_KEY`. Per `feedback_qa_ci_blockers.md`, "tests that can't run in CI are blockers, not nits." Confirm CI has this secret wired (Woodpecker repo secret) — otherwise this becomes the same silent-skip pattern that allowed #490's 30-day TTL bug through. The comment in the file says "CI injects this secret for the test step" — please verify a Woodpecker secret exists on `forgejo_admin/basketball-api` with that name. If it does, this checkbox flips to checked. ### PROCESS OBSERVATIONS **Deployment frequency:** Clean analog of the tournament migration (#494/#496). Reusing the proven shape — separate helper module, identical webhook gate widening pattern, identical real-Stripe test pattern — is the right move. Cherry-pickable, low-risk change. **Change failure risk:** Low. - Webhook change is strictly additive with explicit complement guard. - Real-Stripe test catches the failure mode that #490 missed (server-side validation that mocks pass through). - Stale-amount regen path preserved (`test_first_payment_fresh_pending_amount_changed`). - Customer creation path was dropped (Payment Links create the Customer on first payment when `setup_future_usage` is set) — this is documented in the test module docstring, but it's worth manual validation: the next-month off-session charge needs a Customer attached to the PaymentMethod. Recommend post-merge validation per `feedback_validate_before_done.md` confirms one full end-to-end click → pay → next-month charge cycle in test mode before mass blast. **Documentation gaps:** The monthly billing runbook (analog of `docs/tournament-billing-runbook.md` referenced in PR #496) is not updated in this PR. If one exists for monthly, refresh; if not, log a follow-up item to mirror the tournament runbook for monthly. Track as Epilogue subphase. **DORA mapping:** Resolves a CFR (parents losing 78% of payers from 24h-expiring URLs per #488). Reduces MTTR for the same incident class (Payment Links don't expire, so the recovery loop from #486/#497 becomes unnecessary). Strengthens the payments pipeline observability story Lucas is currently shipping in `290-payment-pipeline-observability`. ### VERDICT: APPROVED Conditional on Woodpecker secret `BASKETBALL_STRIPE_TEST_API_KEY` existing on the basketball-api repo so the real-Stripe test actually runs in CI (per `feedback_qa_ci_blockers.md`). If it doesn't, treat that as a blocker and wire it before merge. All other findings are nits and can be tracked in the Epilogue.

forgejo_admin referenced this pull request from a commit 2026-04-18 05:32:14 +00:00

ci: wire BASKETBALL_STRIPE_TEST_API_KEY into test step

forgejo_admin added 1 commit 2026-04-18 05:32:14 +00:00

ci: wire BASKETBALL_STRIPE_TEST_API_KEY into test step ... 7e646ae16c

Real-Stripe integration test (tests/test_monthly_payment_link_real_stripe.py)
silently skips without this env var. Per feedback_qa_ci_blockers, tests that
can't run in CI are blockers. Woodpecker repo secret
basketball_stripe_test_api_key wired from ~/secrets/stripe/test-secret-key
by main session before this commit.

Refs #498. QA flag from PR #499 review.

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

forgejo_admin commented 2026-04-18 05:32:33 +00:00

Author

Owner

Copy link

QA blocker fix applied (commit 7e646ae).

The conditional: QA flagged that tests/test_monthly_payment_link_real_stripe.py would silently skip in CI without BASKETBALL_STRIPE_TEST_API_KEY — the #490 pattern feedback_qa_ci_blockers warns against.

Resolution:

1. Created Woodpecker repo secret basketball_stripe_test_api_key on forgejo_admin/basketball-api (events: push, pull_request). Value sourced from ~/secrets/stripe/test-secret-key (verified sk_test_* prefix).
2. Wired BASKETBALL_STRIPE_TEST_API_KEY: { from_secret: basketball_stripe_test_api_key } into the test step environment in .woodpecker.yaml.
3. Validated YAML parse (python3 -c "import yaml; yaml.safe_load(...)").

The real-Stripe integration test will now execute against test-mode Stripe in CI on this push. Watching the pipeline for confirmation the test actually runs (not skipped).

QA blocker fix applied (commit 7e646ae). **The conditional:** QA flagged that `tests/test_monthly_payment_link_real_stripe.py` would silently skip in CI without `BASKETBALL_STRIPE_TEST_API_KEY` — the #490 pattern `feedback_qa_ci_blockers` warns against. **Resolution:** 1. Created Woodpecker repo secret `basketball_stripe_test_api_key` on `forgejo_admin/basketball-api` (events: `push`, `pull_request`). Value sourced from `~/secrets/stripe/test-secret-key` (verified `sk_test_*` prefix). 2. Wired `BASKETBALL_STRIPE_TEST_API_KEY: { from_secret: basketball_stripe_test_api_key }` into the `test` step environment in `.woodpecker.yaml`. 3. Validated YAML parse (`python3 -c "import yaml; yaml.safe_load(...)"`). The real-Stripe integration test will now execute against test-mode Stripe in CI on this push. Watching the pipeline for confirmation the test actually runs (not skipped).

forgejo_admin referenced this pull request 2026-04-18 05:34:51 +00:00

CI red on main — 2 stale test assertions blocking all PR merges #500

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

fix: unblock CI — 2 stale test assertions (#500) #501

forgejo_admin referenced this pull request from a commit 2026-04-18 05:55:10 +00:00

ci: wire BASKETBALL_STRIPE_TEST_API_KEY into test step

forgejo_admin force-pushed 498-monthly-payment-links from 7e646ae16c to 0f6e6cc428 2026-04-18 05:55:10 +00:00 Compare

forgejo_admin merged commit 8747b40057 into main 2026-04-18 06:06:42 +00:00

forgejo_admin deleted branch 498-monthly-payment-links 2026-04-18 06:06:42 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-18 06:06:44 +00:00

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

forgejo_admin referenced this pull request 2026-04-18 06:07:59 +00:00

Refactor: extract shared Payment Link helper from monthly + tournament checkout services #502

forgejo_admin referenced this pull request 2026-04-18 06:08:23 +00:00

Real-Stripe integration tests leave orphan test-mode Price/Product objects #503

forgejo_admin referenced this pull request 2026-04-18 06:09:06 +00:00

Create docs/monthly-billing-runbook.md (analog of tournament-billing-runbook) #504

forgejo_admin referenced this pull request 2026-04-19 23:42:40 +00:00

Recover stranded monthly-fee parents (investigate + apology blast, analog of #486) #497

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

No description provided.