fix: handle pending orders gracefully in first-payment checkout #474

Merged
forgejo_admin merged 1 commit from 473-fix-first-payment-409 into main 2026-04-13 21:47:46 +00:00

Summary

Parents were permanently locked out with 409 after a pending order was created (by first click or link-preview bots). The duplicate order guard blocked on both paid AND pending statuses, so once any pending order existed, the parent could never complete checkout.

Changes

  • src/basketball_api/routes/checkout.py — Replaced the blanket 409 for pending+paid orders with three-way logic:
    • Paid order → 409 (unchanged)
    • Fresh pending order (<30 min) → redirect to existing Stripe checkout session URL
    • Stale pending order (>30 min) → cancel the order (set status to canceled) and create a new checkout session
  • tests/test_first_payment.py — Replaced test_duplicate_order_returns_409 with three targeted tests:
    • test_first_payment_paid_still_blocked — paid order returns 409
    • test_first_payment_stale_pending_replaced — stale pending gets canceled, new session created
    • test_first_payment_fresh_pending_returns_redirect — fresh pending redirects to existing Stripe session

Test Plan

  • pytest tests/test_first_payment.py -v — 11/11 passing
  • pytest tests/ -x — 567 passed, 1 pre-existing failure in test_reconciliation.py (missing groupme_sdk module, unrelated)
  • ruff check and ruff format clean

Review Checklist

  • No changes to webhooks.py, email.py, or subscriptions.py
  • Orders are canceled, not deleted (audit trail preserved)
  • Existing code patterns in checkout.py followed
  • All tests pass
  • Linting clean

None.

Closes #473

## Summary Parents were permanently locked out with 409 after a pending order was created (by first click or link-preview bots). The duplicate order guard blocked on both `paid` AND `pending` statuses, so once any pending order existed, the parent could never complete checkout. ## Changes - `src/basketball_api/routes/checkout.py` — Replaced the blanket 409 for pending+paid orders with three-way logic: - **Paid order** → 409 (unchanged) - **Fresh pending order (<30 min)** → redirect to existing Stripe checkout session URL - **Stale pending order (>30 min)** → cancel the order (set status to `canceled`) and create a new checkout session - `tests/test_first_payment.py` — Replaced `test_duplicate_order_returns_409` with three targeted tests: - `test_first_payment_paid_still_blocked` — paid order returns 409 - `test_first_payment_stale_pending_replaced` — stale pending gets canceled, new session created - `test_first_payment_fresh_pending_returns_redirect` — fresh pending redirects to existing Stripe session ## Test Plan - `pytest tests/test_first_payment.py -v` — 11/11 passing - `pytest tests/ -x` — 567 passed, 1 pre-existing failure in `test_reconciliation.py` (missing `groupme_sdk` module, unrelated) - `ruff check` and `ruff format` clean ## Review Checklist - [x] No changes to webhooks.py, email.py, or subscriptions.py - [x] Orders are canceled, not deleted (audit trail preserved) - [x] Existing code patterns in checkout.py followed - [x] All tests pass - [x] Linting clean ## Related Notes None. ## Related Closes #473
feat: add skip_proration flag to first-payment endpoint and checkout route
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
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
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
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>
fix: handle pending orders gracefully in first-payment checkout (#473)
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
8f70b50621
Parents were permanently locked out with 409 after a pending order was
created (by first click or link-preview bots). Now: paid orders still
return 409, fresh pending orders (<30 min) redirect to the existing
Stripe session, and stale pending orders (>30 min) are canceled so a
new checkout can proceed.

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

PR #474 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Stripe SDK. Applying Python/FastAPI domain checklist.

Three-way pending order logic (checkout.py, lines 360-375 in diff)

The core fix is sound: paid orders still 409, fresh pending orders redirect to the existing Stripe session, stale pending orders get canceled and recreated. This directly addresses the revenue-blocking bug where any pending order permanently locked parents out.

Correctness concerns:

  1. Stripe session URL can be None on expired sessions. Stripe Checkout Sessions expire after 24 hours. When expired, session.url returns None, which would produce a RedirectResponse(url=None). The 30-minute freshness threshold protects against this in the common case (a 5-minute-old session is not expired), but there is no defensive guard. If for any reason a "fresh" session has a null URL, the parent gets a broken redirect instead of a useful error. Recommend adding a guard:

    if not session.url:
        existing_order.status = OrderStatus.canceled
        db.commit()
        # fall through to create new session
    

    Severity: NIT -- the 30-minute window makes this unlikely in practice, but it is a latent edge case.

  2. stripe_checkout_session_id is nullable. The Order model declares stripe_checkout_session_id as String(200), nullable=True. If a pending order somehow exists without a session ID (e.g., a crash between db.add(order) and order.stripe_checkout_session_id = checkout_session.id), stripe.checkout.Session.retrieve(None) will raise an unhandled Stripe InvalidRequestError. The query already filters for OrderStatus.pending, and normal flow always sets the session ID, so this is a defensive-coding concern rather than a production risk.
    Severity: NIT -- add a guard or filter Order.stripe_checkout_session_id.isnot(None) to the query.

  3. datetime.now() vs timezone-aware timestamps. The code comment says "created_at is naive local time from DB" which is consistent with the existing codebase pattern (func.now() in SQLAlchemy models producing naive timestamps). Using datetime.now() is correct here given the existing convention. No issue.

skip_proration / prorate feature (admin.py, email.py, checkout.py)

The prorate query parameter on the checkout endpoint and the skip_proration flag on the admin blast endpoint are well-implemented:

  • Keyword-only argument (*) in send_first_payment_email prevents positional misuse
  • Email template conditionally omits proration explanation when skipping
  • Tests verify the full chain: admin endpoint forwards the flag, email includes correct URL and label, checkout charges flat fee

BLOCKERS

1. Scope creep: skip_proration bundled with 409 fix

This PR bundles two distinct features:

  • Fix #473: Three-way pending order handling (the 409 bug)
  • Fix #458: skip_proration flag on admin blast + checkout + email

Issue #473 is about stale pending orders blocking checkout. The skip_proration changes touch admin.py, email.py, and add prorate to checkout.py -- none of which are related to the pending order bug. There are already open PRs (#470, #472) targeting #458.

This violates the one-ticket-one-PR convention. If the skip_proration code introduces a regression, it cannot be reverted without also reverting the critical 409 fix. These should be separate PRs.

Recommendation: Split into two PRs. The 409 fix (checkout.py three-way logic + its 3 tests) should land alone since it is revenue-blocking. The skip_proration work should go through its own PR against #458, closing the duplicate open PRs (#470, #472).

Why this is a blocker and not a nit: The PR is marked mergeable: false, suggesting merge conflicts (likely from the competing #470/#472 PRs). Splitting resolves both the scope issue and the merge conflict.

NITS

  1. Hardcoded 30-minute threshold. The staleness cutoff timedelta(minutes=30) is a magic number inline in the route handler. Consider extracting to a module-level constant like _PENDING_ORDER_STALE_MINUTES = 30 for readability and future tuning.

  2. Plain-text email f-string readability. The conditional expression using chr(10) for newlines in the plain-text body (email.py) is hard to read:

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

    A simple if/else block building the string in parts would be clearer.

  3. Test naming consistency. The existing test was renamed from test_duplicate_order_returns_409 to test_first_payment_paid_still_blocked. The new tests follow test_first_payment_* convention, which is good. No action needed.

SOP COMPLIANCE

  • Branch named after issue: 473-fix-first-payment-409 matches issue #473
  • PR body follows template: Summary, Changes, Test Plan, Related all present
  • Related references plan slug: No plan slug (stated upfront as N/A -- acceptable for a bug fix)
  • No secrets committed
  • No unnecessary file changes (scope creep): skip_proration changes in admin.py, email.py, and their tests belong to #458, not #473
  • Commit messages are descriptive (inferred from PR title)
  • Tests exist: 4 new tests covering all three branches + skip_proration
  • Ruff clean per PR body

PROCESS OBSERVATIONS

  • Deployment frequency: This is a revenue-blocking bug. The 409 fix should land as fast as possible. Splitting out skip_proration lets the critical fix merge immediately.
  • Change failure risk: The three-way logic is low risk -- it narrows an existing over-broad guard. The skip_proration feature is also low risk but coupling it increases revert blast radius unnecessarily.
  • Competing PRs: PRs #468, #469, #470, #472 all appear to be earlier attempts at skip_proration. These should be closed when the final version lands to avoid confusion.

VERDICT: NOT APPROVED

Reason: Scope creep. The skip_proration feature (#458) is bundled with the 409 bug fix (#473). Split into two PRs: one for the three-way pending order logic (closes #473), one for skip_proration (closes #458). The 409 fix itself is well-implemented and would likely pass review on its own.

## PR #474 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Stripe SDK. Applying Python/FastAPI domain checklist. **Three-way pending order logic (checkout.py, lines 360-375 in diff)** The core fix is sound: paid orders still 409, fresh pending orders redirect to the existing Stripe session, stale pending orders get canceled and recreated. This directly addresses the revenue-blocking bug where any pending order permanently locked parents out. **Correctness concerns:** 1. **Stripe session URL can be None on expired sessions.** Stripe Checkout Sessions expire after 24 hours. When expired, `session.url` returns `None`, which would produce a `RedirectResponse(url=None)`. The 30-minute freshness threshold protects against this in the common case (a 5-minute-old session is not expired), but there is no defensive guard. If for any reason a "fresh" session has a null URL, the parent gets a broken redirect instead of a useful error. Recommend adding a guard: ``` if not session.url: existing_order.status = OrderStatus.canceled db.commit() # fall through to create new session ``` **Severity: NIT** -- the 30-minute window makes this unlikely in practice, but it is a latent edge case. 2. **`stripe_checkout_session_id` is nullable.** The Order model declares `stripe_checkout_session_id` as `String(200), nullable=True`. If a pending order somehow exists without a session ID (e.g., a crash between `db.add(order)` and `order.stripe_checkout_session_id = checkout_session.id`), `stripe.checkout.Session.retrieve(None)` will raise an unhandled Stripe `InvalidRequestError`. The query already filters for `OrderStatus.pending`, and normal flow always sets the session ID, so this is a defensive-coding concern rather than a production risk. **Severity: NIT** -- add a guard or filter `Order.stripe_checkout_session_id.isnot(None)` to the query. 3. **`datetime.now()` vs timezone-aware timestamps.** The code comment says "created_at is naive local time from DB" which is consistent with the existing codebase pattern (`func.now()` in SQLAlchemy models producing naive timestamps). Using `datetime.now()` is correct here given the existing convention. No issue. **`skip_proration` / `prorate` feature (admin.py, email.py, checkout.py)** The `prorate` query parameter on the checkout endpoint and the `skip_proration` flag on the admin blast endpoint are well-implemented: - Keyword-only argument (`*`) in `send_first_payment_email` prevents positional misuse - Email template conditionally omits proration explanation when skipping - Tests verify the full chain: admin endpoint forwards the flag, email includes correct URL and label, checkout charges flat fee ### BLOCKERS **1. Scope creep: skip_proration bundled with 409 fix** This PR bundles two distinct features: - **Fix #473:** Three-way pending order handling (the 409 bug) - **Fix #458:** `skip_proration` flag on admin blast + checkout + email Issue #473 is about stale pending orders blocking checkout. The `skip_proration` changes touch `admin.py`, `email.py`, and add `prorate` to `checkout.py` -- none of which are related to the pending order bug. There are already open PRs (#470, #472) targeting #458. This violates the one-ticket-one-PR convention. If the `skip_proration` code introduces a regression, it cannot be reverted without also reverting the critical 409 fix. These should be separate PRs. **Recommendation:** Split into two PRs. The 409 fix (checkout.py three-way logic + its 3 tests) should land alone since it is revenue-blocking. The `skip_proration` work should go through its own PR against #458, closing the duplicate open PRs (#470, #472). **Why this is a blocker and not a nit:** The PR is marked `mergeable: false`, suggesting merge conflicts (likely from the competing #470/#472 PRs). Splitting resolves both the scope issue and the merge conflict. ### NITS 1. **Hardcoded 30-minute threshold.** The staleness cutoff `timedelta(minutes=30)` is a magic number inline in the route handler. Consider extracting to a module-level constant like `_PENDING_ORDER_STALE_MINUTES = 30` for readability and future tuning. 2. **Plain-text email f-string readability.** The conditional expression using `chr(10)` for newlines in the plain-text body (email.py) is hard to read: ```python f"{\"Since you're joining...\" + chr(10) + chr(10) if not skip_proration else ''}" ``` A simple `if/else` block building the string in parts would be clearer. 3. **Test naming consistency.** The existing test was renamed from `test_duplicate_order_returns_409` to `test_first_payment_paid_still_blocked`. The new tests follow `test_first_payment_*` convention, which is good. No action needed. ### SOP COMPLIANCE - [x] Branch named after issue: `473-fix-first-payment-409` matches issue #473 - [x] PR body follows template: Summary, Changes, Test Plan, Related all present - [ ] Related references plan slug: No plan slug (stated upfront as N/A -- acceptable for a bug fix) - [x] No secrets committed - [ ] No unnecessary file changes (scope creep): `skip_proration` changes in admin.py, email.py, and their tests belong to #458, not #473 - [x] Commit messages are descriptive (inferred from PR title) - [x] Tests exist: 4 new tests covering all three branches + skip_proration - [x] Ruff clean per PR body ### PROCESS OBSERVATIONS - **Deployment frequency:** This is a revenue-blocking bug. The 409 fix should land as fast as possible. Splitting out `skip_proration` lets the critical fix merge immediately. - **Change failure risk:** The three-way logic is low risk -- it narrows an existing over-broad guard. The `skip_proration` feature is also low risk but coupling it increases revert blast radius unnecessarily. - **Competing PRs:** PRs #468, #469, #470, #472 all appear to be earlier attempts at `skip_proration`. These should be closed when the final version lands to avoid confusion. ### VERDICT: NOT APPROVED **Reason:** Scope creep. The `skip_proration` feature (#458) is bundled with the 409 bug fix (#473). Split into two PRs: one for the three-way pending order logic (closes #473), one for `skip_proration` (closes #458). The 409 fix itself is well-implemented and would likely pass review on its own.
forgejo_admin force-pushed 473-fix-first-payment-409 from 8f70b50621
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
to 0d72068539
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
2026-04-13 20:14:19 +00:00
Compare
Author
Owner

PR #474 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Stripe SDK

Scope verification: The previous review flagged scope creep (skip_proration bundled in). Confirmed all skip_proration references are gone -- grep across the entire repo returns zero matches. The diff touches exactly two files (checkout.py, test_first_payment.py) and contains only the three-way pending order logic. Scope is clean.

Logic review (checkout.py lines 357-368 on branch):

The three-way branch is correct and well-structured:

  1. Paid -> 409 (unchanged behavior)
  2. Fresh pending (<30 min) -> Session.retrieve() -> 307 redirect to existing session URL
  3. Stale pending (>30 min) -> cancel order, fall through to create new session

The 30-minute threshold is reasonable -- Stripe Checkout sessions expire after 24 hours, so a fresh session will always have a valid URL. The stale path cancels (not deletes) the order, preserving audit trail. Both are good decisions.

Defensive gap (NIT, not blocker): If a pending Order exists with stripe_checkout_session_id = None (e.g., a prior crash between db.flush() at line 391 and db.commit() at line 418), the fresh-pending path would call stripe.checkout.Session.retrieve(None) which raises a Stripe InvalidRequestError. This is an unlikely edge case (would require a crash in a very narrow window), and the unhandled Stripe exception would surface as a 500, which is acceptable degraded behavior. Worth a guard clause in a follow-up but not a blocker.

Datetime handling: Uses datetime.now() (naive) which matches the model's server_default=func.now() (also naive). Consistent. The comment "created_at is naive local time from DB" is helpful documentation.

Test review (test_first_payment.py):

Three new tests cover all three branches of the new logic:

  • test_first_payment_paid_still_blocked -- verifies 409 for paid orders (was the old test_duplicate_order_returns_409, correctly updated)
  • test_first_payment_stale_pending_replaced -- verifies cancellation + new session creation, asserts old order canceled AND new order created with correct session ID
  • test_first_payment_fresh_pending_returns_redirect -- verifies 307 redirect, asserts Session.retrieve called with correct ID, asserts Session.create NOT called, asserts order remains pending

All three tests use follow_redirects=False correctly to inspect the 307. Mocking is targeted and appropriate. The stale test sets created_at to 45 minutes ago (well past threshold), fresh test sets it to 5 minutes ago (well within). Good boundary separation.

BLOCKERS

None.

NITS

  1. Defensive guard on stripe_checkout_session_id: In the fresh-pending branch, consider checking existing_order.stripe_checkout_session_id is not None before calling Session.retrieve(). If it's None, treat the order as stale (cancel and recreate). Low probability but would prevent a confusing 500.

  2. Magic number: The 30-minute threshold is hardcoded inline. Consider extracting to a module-level constant (e.g., _PENDING_ORDER_STALE_MINUTES = 30) for discoverability and consistency with the existing _DEFAULT_MONTHLY_FEE and _SUCCESS_URL constants in the same file.

  3. Boundary test: No test at exactly 30 minutes. The two tests use 5 min and 45 min, which is fine for confidence, but a boundary test at 29 min and 31 min would catch off-by-one issues if the threshold ever changes. Non-blocking.

SOP COMPLIANCE

  • Branch named after issue: 473-fix-first-payment-409 follows {issue-number}-{kebab-case-purpose}
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related references plan slug -- N/A, no plan (standalone bug fix)
  • No secrets committed
  • No unnecessary file changes (scope creep resolved from prior review)
  • Tests exist and pass (11/11 per PR body, 3 new tests covering all branches)
  • Commit messages are descriptive (PR title follows fix: convention)

PROCESS OBSERVATIONS

Clean scope recovery from the previous review. The dev agent stripped the skip_proration changes completely without leaving any artifacts. The PR is now tightly scoped to the 409 fix with full test coverage of all three branches. Change failure risk is low -- the logic is straightforward conditional branching with good test isolation.

VERDICT: APPROVED

## PR #474 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Stripe SDK **Scope verification**: The previous review flagged scope creep (`skip_proration` bundled in). Confirmed **all skip_proration references are gone** -- grep across the entire repo returns zero matches. The diff touches exactly two files (`checkout.py`, `test_first_payment.py`) and contains only the three-way pending order logic. Scope is clean. **Logic review** (`checkout.py` lines 357-368 on branch): The three-way branch is correct and well-structured: 1. **Paid** -> 409 (unchanged behavior) 2. **Fresh pending (<30 min)** -> `Session.retrieve()` -> 307 redirect to existing session URL 3. **Stale pending (>30 min)** -> cancel order, fall through to create new session The 30-minute threshold is reasonable -- Stripe Checkout sessions expire after 24 hours, so a fresh session will always have a valid URL. The stale path cancels (not deletes) the order, preserving audit trail. Both are good decisions. **Defensive gap (NIT, not blocker)**: If a pending Order exists with `stripe_checkout_session_id = None` (e.g., a prior crash between `db.flush()` at line 391 and `db.commit()` at line 418), the fresh-pending path would call `stripe.checkout.Session.retrieve(None)` which raises a Stripe `InvalidRequestError`. This is an unlikely edge case (would require a crash in a very narrow window), and the unhandled Stripe exception would surface as a 500, which is acceptable degraded behavior. Worth a guard clause in a follow-up but not a blocker. **Datetime handling**: Uses `datetime.now()` (naive) which matches the model's `server_default=func.now()` (also naive). Consistent. The comment "created_at is naive local time from DB" is helpful documentation. **Test review** (`test_first_payment.py`): Three new tests cover all three branches of the new logic: - `test_first_payment_paid_still_blocked` -- verifies 409 for paid orders (was the old `test_duplicate_order_returns_409`, correctly updated) - `test_first_payment_stale_pending_replaced` -- verifies cancellation + new session creation, asserts old order canceled AND new order created with correct session ID - `test_first_payment_fresh_pending_returns_redirect` -- verifies 307 redirect, asserts `Session.retrieve` called with correct ID, asserts `Session.create` NOT called, asserts order remains pending All three tests use `follow_redirects=False` correctly to inspect the 307. Mocking is targeted and appropriate. The stale test sets `created_at` to 45 minutes ago (well past threshold), fresh test sets it to 5 minutes ago (well within). Good boundary separation. ### BLOCKERS None. ### NITS 1. **Defensive guard on `stripe_checkout_session_id`**: In the fresh-pending branch, consider checking `existing_order.stripe_checkout_session_id is not None` before calling `Session.retrieve()`. If it's None, treat the order as stale (cancel and recreate). Low probability but would prevent a confusing 500. 2. **Magic number**: The 30-minute threshold is hardcoded inline. Consider extracting to a module-level constant (e.g., `_PENDING_ORDER_STALE_MINUTES = 30`) for discoverability and consistency with the existing `_DEFAULT_MONTHLY_FEE` and `_SUCCESS_URL` constants in the same file. 3. **Boundary test**: No test at exactly 30 minutes. The two tests use 5 min and 45 min, which is fine for confidence, but a boundary test at 29 min and 31 min would catch off-by-one issues if the threshold ever changes. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `473-fix-first-payment-409` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related references plan slug -- N/A, no plan (standalone bug fix) - [x] No secrets committed - [x] No unnecessary file changes (scope creep resolved from prior review) - [x] Tests exist and pass (11/11 per PR body, 3 new tests covering all branches) - [x] Commit messages are descriptive (PR title follows `fix:` convention) ### PROCESS OBSERVATIONS Clean scope recovery from the previous review. The dev agent stripped the skip_proration changes completely without leaving any artifacts. The PR is now tightly scoped to the 409 fix with full test coverage of all three branches. Change failure risk is low -- the logic is straightforward conditional branching with good test isolation. ### VERDICT: APPROVED
forgejo_admin deleted branch 473-fix-first-payment-409 2026-04-13 21:47:46 +00:00
Sign in to join this conversation.
No description provided.