fix: handle pending orders gracefully in first-payment checkout #474
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo_admin/basketball-api!474
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "473-fix-first-payment-409"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
paidANDpendingstatuses, 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:canceled) and create a new checkout sessiontests/test_first_payment.py— Replacedtest_duplicate_order_returns_409with three targeted tests:test_first_payment_paid_still_blocked— paid order returns 409test_first_payment_stale_pending_replaced— stale pending gets canceled, new session createdtest_first_payment_fresh_pending_returns_redirect— fresh pending redirects to existing Stripe sessionTest Plan
pytest tests/test_first_payment.py -v— 11/11 passingpytest tests/ -x— 567 passed, 1 pre-existing failure intest_reconciliation.py(missinggroupme_sdkmodule, unrelated)ruff checkandruff formatcleanReview Checklist
Related Notes
None.
Related
Closes #473
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:
Stripe session URL can be None on expired sessions. Stripe Checkout Sessions expire after 24 hours. When expired,
session.urlreturnsNone, which would produce aRedirectResponse(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:Severity: NIT -- the 30-minute window makes this unlikely in practice, but it is a latent edge case.
stripe_checkout_session_idis nullable. The Order model declaresstripe_checkout_session_idasString(200), nullable=True. If a pending order somehow exists without a session ID (e.g., a crash betweendb.add(order)andorder.stripe_checkout_session_id = checkout_session.id),stripe.checkout.Session.retrieve(None)will raise an unhandled StripeInvalidRequestError. The query already filters forOrderStatus.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.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). Usingdatetime.now()is correct here given the existing convention. No issue.skip_proration/proratefeature (admin.py, email.py, checkout.py)The
proratequery parameter on the checkout endpoint and theskip_prorationflag on the admin blast endpoint are well-implemented:*) insend_first_payment_emailprevents positional misuseBLOCKERS
1. Scope creep: skip_proration bundled with 409 fix
This PR bundles two distinct features:
skip_prorationflag on admin blast + checkout + emailIssue #473 is about stale pending orders blocking checkout. The
skip_prorationchanges touchadmin.py,email.py, and addproratetocheckout.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_prorationcode 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_prorationwork 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
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 = 30for readability and future tuning.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:A simple
if/elseblock building the string in parts would be clearer.Test naming consistency. The existing test was renamed from
test_duplicate_order_returns_409totest_first_payment_paid_still_blocked. The new tests followtest_first_payment_*convention, which is good. No action needed.SOP COMPLIANCE
473-fix-first-payment-409matches issue #473skip_prorationchanges in admin.py, email.py, and their tests belong to #458, not #473PROCESS OBSERVATIONS
skip_prorationlets the critical fix merge immediately.skip_prorationfeature is also low risk but coupling it increases revert blast radius unnecessarily.skip_proration. These should be closed when the final version lands to avoid confusion.VERDICT: NOT APPROVED
Reason: Scope creep. The
skip_prorationfeature (#458) is bundled with the 409 bug fix (#473). Split into two PRs: one for the three-way pending order logic (closes #473), one forskip_proration(closes #458). The 409 fix itself is well-implemented and would likely pass review on its own.8f70b506210d72068539PR #474 Review
DOMAIN REVIEW
Tech stack: Python / FastAPI / SQLAlchemy / Stripe SDK
Scope verification: The previous review flagged scope creep (
skip_prorationbundled 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.pylines 357-368 on branch):The three-way branch is correct and well-structured:
Session.retrieve()-> 307 redirect to existing session URLThe 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 betweendb.flush()at line 391 anddb.commit()at line 418), the fresh-pending path would callstripe.checkout.Session.retrieve(None)which raises a StripeInvalidRequestError. 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'sserver_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 oldtest_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 IDtest_first_payment_fresh_pending_returns_redirect-- verifies 307 redirect, assertsSession.retrievecalled with correct ID, assertsSession.createNOT called, asserts order remains pendingAll three tests use
follow_redirects=Falsecorrectly to inspect the 307. Mocking is targeted and appropriate. The stale test setscreated_atto 45 minutes ago (well past threshold), fresh test sets it to 5 minutes ago (well within). Good boundary separation.BLOCKERS
None.
NITS
Defensive guard on
stripe_checkout_session_id: In the fresh-pending branch, consider checkingexisting_order.stripe_checkout_session_id is not Nonebefore callingSession.retrieve(). If it's None, treat the order as stale (cancel and recreate). Low probability but would prevent a confusing 500.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_FEEand_SUCCESS_URLconstants in the same file.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
473-fix-first-payment-409follows{issue-number}-{kebab-case-purpose}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