fix: stripe webhook not syncing checkout.session.completed to order status (#264) #266

Merged

forgejo_admin merged 1 commit from 264-fix-webhook-sync into main 2026-03-31 00:21:24 +00:00

Conversation 2 Commits 1 Files changed 2 +291 -6

forgejo_admin commented 2026-03-31 00:12:50 +00:00

Contributor

Copy link

Summary

Fixes three issues in the Stripe webhook handler that could cause order status to stay pending after successful payment. Adds payment_status checking per Stripe best practices, defensive metadata null handling, error isolation for jersey sync, and structured logging for debugging.

Changes

• src/basketball_api/routes/webhooks.py

  • Added payment_status == "paid" guard in _handle_generic_order_completed -- Stripe best practice to prevent fulfillment before payment is confirmed
  • Changed session_data.get("metadata", {}) to session_data.get("metadata") or {} in all three checkout handlers -- prevents AttributeError when Stripe sends metadata: null
  • Wrapped sync_player_jersey_from_order in try/except so a sync failure does not prevent the order status db.commit() from executing
  • Added logger.info at webhook entry with event type and session ID
  • Added logger.info on fallthrough to registration handler (no handler matched)
  • Enhanced order completion log to include payment_intent

• tests/test_checkout.py

  • Added payment_status: "paid" to all existing webhook test event data (required by new guard)
  • Added test_webhook_skips_fulfillment_when_payment_status_not_paid -- verifies order stays pending when payment_status != "paid"
  • Added test_webhook_skips_fulfillment_when_payment_status_missing -- verifies order stays pending when payment_status key is absent
  • Added test_webhook_handles_null_metadata -- verifies no crash when metadata: null
  • Added test_webhook_updates_player_on_upgrade_from_opt_out -- verifies player jersey fields update when upgrading from opt-out to real jersey
  • Added test_webhook_commits_order_status_even_if_sync_fails -- verifies order marked paid even when sync_player_jersey_from_order raises

Test Plan

• Full test suite: 694 passed (25 in test_checkout.py, up from 20)
• ruff format and ruff check clean
• New tests cover: payment_status guard, null metadata, opt-out upgrade path, sync exception resilience

Review Checklist

• Tests pass locally (694 passed)
• Ruff format and lint clean
• No unrelated changes
• Existing test event data updated with payment_status: "paid" to match new guard
• Logging added at webhook entry, handler decisions, and fallthrough paths

Related Notes

None -- standalone bug fix.

Related

Closes #264

• Board: board-westside-basketball (item #719)

## Summary Fixes three issues in the Stripe webhook handler that could cause order status to stay `pending` after successful payment. Adds `payment_status` checking per Stripe best practices, defensive `metadata` null handling, error isolation for jersey sync, and structured logging for debugging. ## Changes - `src/basketball_api/routes/webhooks.py` - Added `payment_status == "paid"` guard in `_handle_generic_order_completed` -- Stripe best practice to prevent fulfillment before payment is confirmed - Changed `session_data.get("metadata", {})` to `session_data.get("metadata") or {}` in all three checkout handlers -- prevents `AttributeError` when Stripe sends `metadata: null` - Wrapped `sync_player_jersey_from_order` in try/except so a sync failure does not prevent the order status `db.commit()` from executing - Added `logger.info` at webhook entry with event type and session ID - Added `logger.info` on fallthrough to registration handler (no handler matched) - Enhanced order completion log to include `payment_intent` - `tests/test_checkout.py` - Added `payment_status: "paid"` to all existing webhook test event data (required by new guard) - Added `test_webhook_skips_fulfillment_when_payment_status_not_paid` -- verifies order stays pending when `payment_status != "paid"` - Added `test_webhook_skips_fulfillment_when_payment_status_missing` -- verifies order stays pending when `payment_status` key is absent - Added `test_webhook_handles_null_metadata` -- verifies no crash when `metadata: null` - Added `test_webhook_updates_player_on_upgrade_from_opt_out` -- verifies player jersey fields update when upgrading from opt-out to real jersey - Added `test_webhook_commits_order_status_even_if_sync_fails` -- verifies order marked paid even when `sync_player_jersey_from_order` raises ## Test Plan - Full test suite: 694 passed (25 in test_checkout.py, up from 20) - `ruff format` and `ruff check` clean - New tests cover: payment_status guard, null metadata, opt-out upgrade path, sync exception resilience ## Review Checklist - [x] Tests pass locally (694 passed) - [x] Ruff format and lint clean - [x] No unrelated changes - [x] Existing test event data updated with `payment_status: "paid"` to match new guard - [x] Logging added at webhook entry, handler decisions, and fallthrough paths ## Related Notes None -- standalone bug fix. ## Related Closes #264 - Board: board-westside-basketball (item #719)

forgejo_admin added 1 commit 2026-03-31 00:12:50 +00:00

fix: stripe webhook not syncing checkout.session.completed to order status (#264) ... 8285dd5c76

The webhook handler for checkout.session.completed had three issues that
could cause order status to stay pending after successful payment:

1. No payment_status check -- Stripe best practice requires verifying
   payment_status == "paid" before fulfilling. Without this, async payment
   methods could trigger premature fulfillment, and the lack of the check
   meant the handler had no guard against partial payment states.

2. Defensive metadata handling -- session_data.get("metadata", {}) returns
   None (not {}) when the key exists with a null value. Changed to
   `get("metadata") or {}` to prevent AttributeError on None.get().

3. No error isolation for jersey sync -- if sync_player_jersey_from_order
   raised an exception, the entire handler would fail and db.commit() would
   never execute, leaving the order as pending. Wrapped sync in try/except
   so order status is always committed even if jersey sync fails.

Also added structured logging at webhook entry, handler decisions, and
fallthrough paths to make future debugging possible without guessing which
code path was taken.

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

forgejo_admin commented 2026-03-31 00:13:28 +00:00

Author

Contributor

Copy link

QA Review -- PR #266

Scope Check

PR touches exactly 2 files (webhooks.py + test_checkout.py), both directly related to issue #264. No unrelated changes.

Code Quality

webhooks.py changes (3 fixes + logging):

1. payment_status guard (lines 186-197) -- Correct implementation. Checks payment_status != "paid" before fulfilling. Returns True (claims the event) so it doesn't fall through to other handlers. Good: uses warning-level log with order ID and the unexpected status value.

2. Defensive metadata (or {} pattern, lines 170, 237, 332) -- Fixes the None metadata edge case where .get("metadata", {}) returns None when the key exists with a null value. Applied consistently across all three checkout handler paths.

3. Jersey sync error isolation (lines 206-215) -- try/except Exception wrapping sync_player_jersey_from_order ensures db.commit() on line 217 always executes. Uses logger.exception which captures the full traceback. Order status update is decoupled from jersey sync success.

4. Structured logging (lines 316-320, 417-420) -- Entry log on every webhook with event type and session ID. Fallthrough log when no handler matched. Good for production debugging.

Test Quality

5 new tests added (20 -> 25 total):

• test_webhook_skips_fulfillment_when_payment_status_not_paid -- verifies unpaid status is rejected
• test_webhook_skips_fulfillment_when_payment_status_missing -- verifies missing key is rejected
• test_webhook_handles_null_metadata -- verifies metadata: None doesn't crash
• test_webhook_updates_player_on_upgrade_from_opt_out -- the exact production scenario from issue #264
• test_webhook_commits_order_status_even_if_sync_fails -- verifies error isolation

All 4 existing webhook tests updated with "payment_status": "paid" to match the new guard. No test logic changed, only event data enriched.

Nits

None. Changes are minimal, focused, and well-tested.

Full Suite

694 tests pass. Ruff clean.

---

VERDICT: APPROVED

## QA Review -- PR #266 ### Scope Check PR touches exactly 2 files (`webhooks.py` + `test_checkout.py`), both directly related to issue #264. No unrelated changes. ### Code Quality **webhooks.py changes (3 fixes + logging):** 1. **`payment_status` guard** (lines 186-197) -- Correct implementation. Checks `payment_status != "paid"` before fulfilling. Returns `True` (claims the event) so it doesn't fall through to other handlers. Good: uses warning-level log with order ID and the unexpected status value. 2. **Defensive metadata** (`or {}` pattern, lines 170, 237, 332) -- Fixes the `None` metadata edge case where `.get("metadata", {})` returns `None` when the key exists with a null value. Applied consistently across all three checkout handler paths. 3. **Jersey sync error isolation** (lines 206-215) -- `try/except Exception` wrapping `sync_player_jersey_from_order` ensures `db.commit()` on line 217 always executes. Uses `logger.exception` which captures the full traceback. Order status update is decoupled from jersey sync success. 4. **Structured logging** (lines 316-320, 417-420) -- Entry log on every webhook with event type and session ID. Fallthrough log when no handler matched. Good for production debugging. ### Test Quality 5 new tests added (20 -> 25 total): - `test_webhook_skips_fulfillment_when_payment_status_not_paid` -- verifies `unpaid` status is rejected - `test_webhook_skips_fulfillment_when_payment_status_missing` -- verifies missing key is rejected - `test_webhook_handles_null_metadata` -- verifies `metadata: None` doesn't crash - `test_webhook_updates_player_on_upgrade_from_opt_out` -- the exact production scenario from issue #264 - `test_webhook_commits_order_status_even_if_sync_fails` -- verifies error isolation All 4 existing webhook tests updated with `"payment_status": "paid"` to match the new guard. No test logic changed, only event data enriched. ### Nits None. Changes are minimal, focused, and well-tested. ### Full Suite 694 tests pass. Ruff clean. --- **VERDICT: APPROVED**

forgejo_admin commented 2026-03-31 00:16:17 +00:00

Author

Contributor

Copy link

PR #266 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy -- applying Python/FastAPI domain checklist.

Stripe webhook correctness:

1. payment_status == "paid" guard (webhooks.py line ~187 in diff): Correctly placed AFTER the order lookup but BEFORE setting order.status = OrderStatus.paid. This prevents fulfillment on async payment methods (bank debits, etc.) where checkout.session.completed fires before payment clears. The default of "" when the key is absent means missing payment_status also blocks fulfillment -- correct and safe.

2. Null metadata fix (session_data.get("metadata") or {}): Applied to all three metadata access points:

   • _handle_generic_order_completed (line 170)
   • _handle_jersey_checkout_completed (line 234 post-diff)
   • stripe_webhook inline registration handler (line 329 post-diff)

   The pattern get("metadata") or {} correctly handles both metadata key absent (returns None -> {}) AND metadata key present with explicit null value (returns None -> {}). The old pattern .get("metadata", {}) only handled the absent-key case. This is the correct fix.

3. Jersey sync isolation (try/except around sync_player_jersey_from_order): The db.commit() is positioned AFTER the try/except block at line ~203 in the diff, so the order status update (order.status = OrderStatus.paid) is always committed regardless of sync failure. logger.exception captures the full traceback. The bare except Exception is appropriate here -- this is a fire-and-forget sync, not a control flow decision.

4. Structured logging: Entry-point log with event_type and session_id, enhanced completion log with payment_intent, and fallthrough log are all useful for production debugging. Uses logger.info for happy path and logger.warning/logger.exception for anomalies -- correct severity levels.

5. Webhook signature verification: Untouched. stripe.Webhook.construct_event with stripe_signature and settings.stripe_webhook_secret remains intact at the top of stripe_webhook(). No security regression.

Test coverage (5 new tests):

Test	Root Cause Covered	Verdict
test_webhook_skips_fulfillment_when_payment_status_not_paid	RC1: no payment_status check	Verifies order stays pending when payment_status="unpaid"
test_webhook_skips_fulfillment_when_payment_status_missing	RC1: no payment_status check	Verifies order stays pending when key absent
test_webhook_handles_null_metadata	RC2: null metadata crash	Verifies no crash with metadata: None
test_webhook_updates_player_on_upgrade_from_opt_out	Regression coverage for jersey sync	Verifies player fields overwritten on upgrade
test_webhook_commits_order_status_even_if_sync_fails	RC3: sync exception blocks commit	Mocks sync_player_jersey_from_order to raise, verifies order still paid

All three root causes have direct test coverage. The sync failure test correctly patches the sync function at the module level (basketball_api.routes.webhooks.sync_player_jersey_from_order) and verifies both order.status and stripe_payment_intent_id.

Existing tests updated: All 4 existing webhook tests (test_webhook_updates_order_to_paid, test_webhook_syncs_jersey_fields_to_player, test_webhook_syncs_warmup_jersey_option, test_webhook_skips_player_sync_for_non_jersey_product) have payment_status: "paid" added to their event data, which is required by the new guard.

BLOCKERS

None.

NITS

1. NIT: _handle_jersey_checkout_completed lacks payment_status guard -- This legacy handler at line 205 also marks player.jersey_order_status = JerseyOrderStatus.paid and commits, but does not check payment_status == "paid" first. While it is documented as "backwards compatibility" and the generic order handler is the primary path now, it is still called for any checkout session with jersey_option metadata. This is a consistency gap. Recommend tracking as discovered scope for a follow-up ticket.

2. NIT: PR body "Related Notes" section says "None -- standalone bug fix" -- Convention expects the Related section to reference a plan slug. Since this is a standalone bug fix (not plan-driven), "None" is acceptable, but it could reference the board item directly (board-westside-basketball item #719 is already mentioned at the bottom, which covers this).

SOP COMPLIANCE

• Branch named after issue: 264-fix-webhook-sync follows {issue-number}-{kebab-case-purpose}
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
• Related references plan slug: N/A (standalone bug fix), closes #264 and references board item #719
• Tests exist and pass: 5 new tests, 694 total passed per PR body
• No secrets committed: No credentials, .env files, or API keys in diff
• No scope creep: All changes are directly related to the three root causes
• Commit messages descriptive: PR title follows conventional commits (fix:)
• Ruff format and lint clean per PR body

PROCESS OBSERVATIONS

This is a well-scoped bug fix with clear root cause analysis. The three bugs are related (all in the same webhook handler path), so bundling them in one PR is appropriate. The PR body is thorough -- it documents each change, explains the Stripe best practice for payment_status, and includes a test plan with concrete numbers.

The _handle_jersey_checkout_completed consistency gap (NIT #1) should become a Forgejo issue to close the loop on legacy handler hardening.

VERDICT: APPROVED

## PR #266 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy -- applying Python/FastAPI domain checklist. **Stripe webhook correctness:** 1. **`payment_status == "paid"` guard** (`webhooks.py` line ~187 in diff): Correctly placed AFTER the order lookup but BEFORE setting `order.status = OrderStatus.paid`. This prevents fulfillment on async payment methods (bank debits, etc.) where `checkout.session.completed` fires before payment clears. The default of `""` when the key is absent means missing `payment_status` also blocks fulfillment -- correct and safe. 2. **Null metadata fix** (`session_data.get("metadata") or {}`): Applied to all three metadata access points: - `_handle_generic_order_completed` (line 170) - `_handle_jersey_checkout_completed` (line 234 post-diff) - `stripe_webhook` inline registration handler (line 329 post-diff) The pattern `get("metadata") or {}` correctly handles both `metadata` key absent (returns `None` -> `{}`) AND `metadata` key present with explicit `null` value (returns `None` -> `{}`). The old pattern `.get("metadata", {})` only handled the absent-key case. This is the correct fix. 3. **Jersey sync isolation** (try/except around `sync_player_jersey_from_order`): The `db.commit()` is positioned AFTER the try/except block at line ~203 in the diff, so the order status update (`order.status = OrderStatus.paid`) is always committed regardless of sync failure. `logger.exception` captures the full traceback. The bare `except Exception` is appropriate here -- this is a fire-and-forget sync, not a control flow decision. 4. **Structured logging**: Entry-point log with `event_type` and `session_id`, enhanced completion log with `payment_intent`, and fallthrough log are all useful for production debugging. Uses `logger.info` for happy path and `logger.warning`/`logger.exception` for anomalies -- correct severity levels. 5. **Webhook signature verification**: Untouched. `stripe.Webhook.construct_event` with `stripe_signature` and `settings.stripe_webhook_secret` remains intact at the top of `stripe_webhook()`. No security regression. **Test coverage (5 new tests):** | Test | Root Cause Covered | Verdict | |------|-------------------|---------| | `test_webhook_skips_fulfillment_when_payment_status_not_paid` | RC1: no payment_status check | Verifies order stays `pending` when `payment_status="unpaid"` | | `test_webhook_skips_fulfillment_when_payment_status_missing` | RC1: no payment_status check | Verifies order stays `pending` when key absent | | `test_webhook_handles_null_metadata` | RC2: null metadata crash | Verifies no crash with `metadata: None` | | `test_webhook_updates_player_on_upgrade_from_opt_out` | Regression coverage for jersey sync | Verifies player fields overwritten on upgrade | | `test_webhook_commits_order_status_even_if_sync_fails` | RC3: sync exception blocks commit | Mocks `sync_player_jersey_from_order` to raise, verifies order still `paid` | All three root causes have direct test coverage. The sync failure test correctly patches the sync function at the module level (`basketball_api.routes.webhooks.sync_player_jersey_from_order`) and verifies both `order.status` and `stripe_payment_intent_id`. **Existing tests updated**: All 4 existing webhook tests (`test_webhook_updates_order_to_paid`, `test_webhook_syncs_jersey_fields_to_player`, `test_webhook_syncs_warmup_jersey_option`, `test_webhook_skips_player_sync_for_non_jersey_product`) have `payment_status: "paid"` added to their event data, which is required by the new guard. ### BLOCKERS None. ### NITS 1. **NIT: `_handle_jersey_checkout_completed` lacks `payment_status` guard** -- This legacy handler at line 205 also marks `player.jersey_order_status = JerseyOrderStatus.paid` and commits, but does not check `payment_status == "paid"` first. While it is documented as "backwards compatibility" and the generic order handler is the primary path now, it is still called for any checkout session with `jersey_option` metadata. This is a consistency gap. Recommend tracking as discovered scope for a follow-up ticket. 2. **NIT: PR body "Related Notes" section says "None -- standalone bug fix"** -- Convention expects the Related section to reference a plan slug. Since this is a standalone bug fix (not plan-driven), "None" is acceptable, but it could reference the board item directly (board-westside-basketball item #719 is already mentioned at the bottom, which covers this). ### SOP COMPLIANCE - [x] Branch named after issue: `264-fix-webhook-sync` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references plan slug: N/A (standalone bug fix), closes #264 and references board item #719 - [x] Tests exist and pass: 5 new tests, 694 total passed per PR body - [x] No secrets committed: No credentials, .env files, or API keys in diff - [x] No scope creep: All changes are directly related to the three root causes - [x] Commit messages descriptive: PR title follows conventional commits (`fix:`) - [x] Ruff format and lint clean per PR body ### PROCESS OBSERVATIONS This is a well-scoped bug fix with clear root cause analysis. The three bugs are related (all in the same webhook handler path), so bundling them in one PR is appropriate. The PR body is thorough -- it documents each change, explains the Stripe best practice for `payment_status`, and includes a test plan with concrete numbers. The `_handle_jersey_checkout_completed` consistency gap (NIT #1) should become a Forgejo issue to close the loop on legacy handler hardening. ### VERDICT: APPROVED

forgejo_admin merged commit 8303cf3cc3 into main 2026-03-31 00:21:24 +00:00

forgejo_admin deleted branch 264-fix-webhook-sync 2026-03-31 00:21:24 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-31 00:21:24 +00:00

fix: stripe webhook not syncing checkout.session.completed to order status (#264) (#266)

forgejo_admin referenced this pull request 2026-03-31 00:21:49 +00:00

Legacy jersey webhook handler missing payment_status guard #267

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

ldraney/basketball-api!266

No description provided.