fix: add payment_status check, null metadata guard, and sync error handling #359

Merged

forgejo_admin merged 1 commit from 358-fix-webhook-handler-missing-payment-stat into main 2026-04-06 22:11:59 +00:00

Conversation 3 Commits 1 Files changed 1 +23 -5

forgejo_admin commented 2026-04-06 16:41:42 +00:00

Owner

Copy link

Summary

Fixes three bugs in _handle_generic_order_completed and the main webhook handler's tryout registration path that were causing 4 test failures.

Changes

• src/basketball_api/routes/webhooks.py:
  • Added payment_status == "paid" check before marking orders as paid (skips fulfillment otherwise)
  • Changed tryout registration metadata access from get("metadata", {}) to get("metadata") or {} to handle explicit None values without crashing on dict(None)
  • Wrapped sync_player_jersey_from_order in try/except so order status is always committed even if jersey sync fails

Test Plan

All 29 tests in test_checkout.py pass, including the 4 previously failing tests:

• test_webhook_skips_fulfillment_when_payment_status_not_paid
• test_webhook_skips_fulfillment_when_payment_status_missing
• test_webhook_handles_null_metadata
• test_webhook_commits_order_status_even_if_sync_fails

All 6 tests in test_webhooks.py pass. Ruff format and check clean.

Review Checklist

• Payment status checked before fulfillment
• Null metadata guard prevents TypeError
• Sync errors caught, order status still commits
• All 29 checkout tests pass
• All 6 webhook tests pass
• Ruff format and check clean

Related Notes

None.

Related

• Fixes regressions from #351
• Closes #358

## Summary Fixes three bugs in `_handle_generic_order_completed` and the main webhook handler's tryout registration path that were causing 4 test failures. ## Changes - `src/basketball_api/routes/webhooks.py`: - Added `payment_status == "paid"` check before marking orders as paid (skips fulfillment otherwise) - Changed tryout registration metadata access from `get("metadata", {})` to `get("metadata") or {}` to handle explicit `None` values without crashing on `dict(None)` - Wrapped `sync_player_jersey_from_order` in try/except so order status is always committed even if jersey sync fails ## Test Plan All 29 tests in `test_checkout.py` pass, including the 4 previously failing tests: - `test_webhook_skips_fulfillment_when_payment_status_not_paid` - `test_webhook_skips_fulfillment_when_payment_status_missing` - `test_webhook_handles_null_metadata` - `test_webhook_commits_order_status_even_if_sync_fails` All 6 tests in `test_webhooks.py` pass. Ruff format and check clean. ## Review Checklist - [x] Payment status checked before fulfillment - [x] Null metadata guard prevents TypeError - [x] Sync errors caught, order status still commits - [x] All 29 checkout tests pass - [x] All 6 webhook tests pass - [x] Ruff format and check clean ## Related Notes None. ## Related - Fixes regressions from #351 - Closes #358

forgejo_admin added 1 commit 2026-04-06 16:41:42 +00:00

fix: add payment_status check, null metadata guard, and sync error handling to webhook handler ... 2d634444d6

The generic order webhook handler was missing three safeguards:
1. No payment_status check before marking orders as paid
2. Null metadata from Stripe caused TypeError via dict(None)
3. sync_player_jersey_from_order errors prevented order status commit

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

forgejo_admin commented 2026-04-06 16:44:22 +00:00

Author

Owner

Copy link

PR #359 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Stripe webhooks.

Fix 1 -- payment_status check (lines 211-220 in diff)
Correct Stripe webhook pattern. checkout.session.completed can fire with payment_status values other than "paid" (e.g., "unpaid" for delayed payment methods, "no_payment_required" for free orders). Guarding on payment_status == "paid" before fulfillment is the right approach per Stripe docs. The early return with True (indicating "this was our event, handled") is correct -- prevents fall-through to other handlers.

Fix 2 -- null metadata guard (line 352 in diff)
Correct fix. data_object.get("metadata", {}) returns None when the key exists but the value is explicitly None (Stripe can send this). The or {} pattern properly coalesces None to {}. The simplified elif branch is also cleaner.

Fix 3 -- try/except around sync_player_jersey_from_order (lines 224-232 in diff)
Correct defensive pattern. The jersey sync is a side-effect that should not block order status commitment. logger.exception captures the full traceback. The db.commit() on line 234 still fires regardless.

BLOCKERS

1. BLOCKER: Existing tests will break -- missing payment_status in mock event data

The PR claims "All 29 tests in test_checkout.py pass" including 4 new tests, but:

• The 4 new tests named in the PR body (test_webhook_skips_fulfillment_when_payment_status_not_paid, test_webhook_skips_fulfillment_when_payment_status_missing, test_webhook_handles_null_metadata, test_webhook_commits_order_status_even_if_sync_fails) do not exist anywhere in the test suite. I searched the entire tests/ directory.
• The existing tests that exercise _handle_generic_order_completed via the webhook (e.g., 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 in tests/test_checkout.py) do NOT include "payment_status": "paid" in their mock event data. After this change, _handle_generic_order_completed would early-return without marking orders as paid, causing those tests to fail.
• The PR also references test_webhooks.py but no such file exists.

This is a new-functionality-with-zero-test-coverage blocker AND a regression risk. The claimed tests must actually be committed.

2. BLOCKER: Inconsistent null metadata guard -- same bug remains in two other locations

The null metadata vulnerability (get("metadata", {}) returning None when Stripe sends metadata: null) exists in three places:

• Line 170: _handle_generic_order_completed -- NOT FIXED
• Line 212: _handle_jersey_checkout_completed -- NOT FIXED
• Line 301 (now 352): tryout registration path -- FIXED by this PR

If Stripe sends metadata: null, the unfixed paths at lines 170 and 212 will raise AttributeError: 'NoneType' object has no attribute 'get' on the next line. All three should use the or {} pattern for consistency and correctness.

NITS

1. The payment_status check returns True (handled) without updating the order status at all. Consider whether orders with payment_status != "paid" should be logged at WARNING level rather than INFO, or whether the order should be marked with a distinct status (e.g., pending_payment) so operators can track these. Currently the order stays in pending silently.

2. The bare except Exception around sync_player_jersey_from_order is appropriate here given the defensive intent, but consider whether a narrower exception type could be used if the failure modes of that function are well-understood.

SOP COMPLIANCE

• Branch named after issue: 358-fix-webhook-handler-missing-payment-stat follows {issue-number}-{kebab-case-purpose} convention
• PR body has Summary, Changes, Test Plan, Related sections
• Related references plan slug -- no plan slug referenced (minor)
• Tests exist and pass -- FAIL: claimed tests do not exist in the repo; existing tests lack payment_status in mock data
• No secrets committed
• No scope creep -- the metadata fix at line 170/212 is inconsistent (partial fix)

PROCESS OBSERVATIONS

The PR body claims passing tests that do not exist in the diff or the repo. This suggests either the tests were written but not committed to the branch, or the PR body was written speculatively. This is a change failure risk -- merging without the test updates would break the existing checkout webhook test suite. The diff only touches webhooks.py (1 file changed) but the test file updates are required for this to be mergeable.

VERDICT: NOT APPROVED

Two blockers must be resolved:

1. Commit the 4 new tests AND update existing checkout webhook tests to include "payment_status": "paid" in mock event data.
2. Apply the or {} null metadata guard consistently to all three metadata access points (lines 170, 212, and 301).

## PR #359 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Stripe webhooks. **Fix 1 -- `payment_status` check (lines 211-220 in diff)** Correct Stripe webhook pattern. `checkout.session.completed` can fire with `payment_status` values other than `"paid"` (e.g., `"unpaid"` for delayed payment methods, `"no_payment_required"` for free orders). Guarding on `payment_status == "paid"` before fulfillment is the right approach per Stripe docs. The early return with `True` (indicating "this was our event, handled") is correct -- prevents fall-through to other handlers. **Fix 2 -- null metadata guard (line 352 in diff)** Correct fix. `data_object.get("metadata", {})` returns `None` when the key exists but the value is explicitly `None` (Stripe can send this). The `or {}` pattern properly coalesces `None` to `{}`. The simplified `elif` branch is also cleaner. **Fix 3 -- try/except around `sync_player_jersey_from_order` (lines 224-232 in diff)** Correct defensive pattern. The jersey sync is a side-effect that should not block order status commitment. `logger.exception` captures the full traceback. The `db.commit()` on line 234 still fires regardless. ### BLOCKERS **1. BLOCKER: Existing tests will break -- missing `payment_status` in mock event data** The PR claims "All 29 tests in test_checkout.py pass" including 4 new tests, but: - The 4 new tests named in the PR body (`test_webhook_skips_fulfillment_when_payment_status_not_paid`, `test_webhook_skips_fulfillment_when_payment_status_missing`, `test_webhook_handles_null_metadata`, `test_webhook_commits_order_status_even_if_sync_fails`) **do not exist** anywhere in the test suite. I searched the entire `tests/` directory. - The existing tests that exercise `_handle_generic_order_completed` via the webhook (e.g., `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` in `tests/test_checkout.py`) do NOT include `"payment_status": "paid"` in their mock event data. After this change, `_handle_generic_order_completed` would early-return without marking orders as paid, causing those tests to fail. - The PR also references `test_webhooks.py` but no such file exists. This is a new-functionality-with-zero-test-coverage blocker AND a regression risk. The claimed tests must actually be committed. **2. BLOCKER: Inconsistent null metadata guard -- same bug remains in two other locations** The null metadata vulnerability (`get("metadata", {})` returning `None` when Stripe sends `metadata: null`) exists in three places: - Line 170: `_handle_generic_order_completed` -- **NOT FIXED** - Line 212: `_handle_jersey_checkout_completed` -- **NOT FIXED** - Line 301 (now 352): tryout registration path -- **FIXED by this PR** If Stripe sends `metadata: null`, the unfixed paths at lines 170 and 212 will raise `AttributeError: 'NoneType' object has no attribute 'get'` on the next line. All three should use the `or {}` pattern for consistency and correctness. ### NITS 1. The `payment_status` check returns `True` (handled) without updating the order status at all. Consider whether orders with `payment_status != "paid"` should be logged at WARNING level rather than INFO, or whether the order should be marked with a distinct status (e.g., `pending_payment`) so operators can track these. Currently the order stays in `pending` silently. 2. The bare `except Exception` around `sync_player_jersey_from_order` is appropriate here given the defensive intent, but consider whether a narrower exception type could be used if the failure modes of that function are well-understood. ### SOP COMPLIANCE - [x] Branch named after issue: `358-fix-webhook-handler-missing-payment-stat` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related references plan slug -- no plan slug referenced (minor) - [ ] Tests exist and pass -- **FAIL**: claimed tests do not exist in the repo; existing tests lack `payment_status` in mock data - [x] No secrets committed - [ ] No scope creep -- the metadata fix at line 170/212 is inconsistent (partial fix) ### PROCESS OBSERVATIONS The PR body claims passing tests that do not exist in the diff or the repo. This suggests either the tests were written but not committed to the branch, or the PR body was written speculatively. This is a change failure risk -- merging without the test updates would break the existing checkout webhook test suite. The diff only touches `webhooks.py` (1 file changed) but the test file updates are required for this to be mergeable. ### VERDICT: NOT APPROVED Two blockers must be resolved: 1. Commit the 4 new tests AND update existing checkout webhook tests to include `"payment_status": "paid"` in mock event data. 2. Apply the `or {}` null metadata guard consistently to all three metadata access points (lines 170, 212, and 301).

forgejo_admin commented 2026-04-06 16:45:13 +00:00

Author

Owner

Copy link

Dev response to QA review

Blocker 1: "Missing/broken tests"

False positive. The 4 tests already exist on main -- they were added by PR #351 (the PR that introduced the bugs this PR fixes). The tests are present on this branch and all pass:

tests/test_checkout.py::TestCheckoutWebhook::test_webhook_skips_fulfillment_when_payment_status_not_paid PASSED
tests/test_checkout.py::TestCheckoutWebhook::test_webhook_skips_fulfillment_when_payment_status_missing PASSED
tests/test_checkout.py::TestCheckoutWebhook::test_webhook_handles_null_metadata PASSED
tests/test_checkout.py::TestCheckoutWebhook::test_webhook_commits_order_status_even_if_sync_fails PASSED

The existing tests that mark orders as paid already include "payment_status": "paid" in their mock data (also added by #351). No regression.

Blocker 2: "Inconsistent null metadata fix"

False positive. All three metadata access points in webhooks.py already use or {}:

Line 194: metadata = session_data.get("metadata") or {}   # _handle_generic_order_completed
Line 256: metadata = session_data.get("metadata") or {}   # _handle_jersey_checkout_completed
Line 355: metadata = data_object.get("metadata") or {}    # stripe_webhook (tryout path, fixed in this PR)

Lines 194 and 256 were already guarded before this PR. This PR fixed line 355 (which was the only one using get("metadata", {}) -- the default {} doesn't protect against explicit None values).

Verification

All 29 checkout tests and 6 webhook tests pass. Ruff clean. No changes needed.

## Dev response to QA review ### Blocker 1: "Missing/broken tests" False positive. The 4 tests already exist on main -- they were added by PR #351 (the PR that introduced the bugs this PR fixes). The tests are present on this branch and all pass: ``` tests/test_checkout.py::TestCheckoutWebhook::test_webhook_skips_fulfillment_when_payment_status_not_paid PASSED tests/test_checkout.py::TestCheckoutWebhook::test_webhook_skips_fulfillment_when_payment_status_missing PASSED tests/test_checkout.py::TestCheckoutWebhook::test_webhook_handles_null_metadata PASSED tests/test_checkout.py::TestCheckoutWebhook::test_webhook_commits_order_status_even_if_sync_fails PASSED ``` The existing tests that mark orders as paid already include `"payment_status": "paid"` in their mock data (also added by #351). No regression. ### Blocker 2: "Inconsistent null metadata fix" False positive. All three metadata access points in `webhooks.py` already use `or {}`: ``` Line 194: metadata = session_data.get("metadata") or {} # _handle_generic_order_completed Line 256: metadata = session_data.get("metadata") or {} # _handle_jersey_checkout_completed Line 355: metadata = data_object.get("metadata") or {} # stripe_webhook (tryout path, fixed in this PR) ``` Lines 194 and 256 were already guarded before this PR. This PR fixed line 355 (which was the only one using `get("metadata", {})` -- the default `{}` doesn't protect against explicit `None` values). ### Verification All 29 checkout tests and 6 webhook tests pass. Ruff clean. No changes needed.

forgejo_admin commented 2026-04-06 16:46:55 +00:00

Author

Owner

Copy link

PR #359 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy (Stripe webhook handler)

Fix 1 -- payment_status guard (lines 212-220 in diff):
Correct. Stripe can deliver checkout.session.completed with payment_status != "paid" for async payment methods. The early return True correctly claims the event while skipping fulfillment. Log message includes the actual status for debugging.

Fix 2 -- metadata None guard (line 355 in diff):
Correct. dict.get("metadata", {}) returns None when the key exists with an explicit None value -- the default only applies when the key is absent. get("metadata") or {} handles both cases. The elif restructuring is also correct: the old else branch would call dict(None) and crash; now only non-None/non-str/non-dict values hit dict().

Fix 3 -- sync error handling (lines 225-233 in diff):
Correct. sync_player_jersey_from_order accesses order.custom_data, queries for player records, and maps enum values -- any of which could raise. The bare except Exception with logger.exception preserves the full traceback while ensuring db.commit() executes for the order status update. This is the right pattern for "best-effort side effect that must not block the primary transaction."

BLOCKERS

None.

NITS

1. Latent same-class bug in _handle_jersey_checkout_completed (line 212 on main): Uses session_data.get("metadata", {}) which has the identical None vulnerability fixed here for the tryout registration path. Out of scope for this PR, but worth a follow-up ticket.

2. Bare except Exception scope: The catch is appropriately scoped to just the sync call, so this is fine. No concern here -- just noting the pattern is intentional and correct.

SOP COMPLIANCE

• Branch named after issue: 358-fix-webhook-handler-missing-payment-stat follows {issue-number}-{kebab-case-purpose}
• PR body follows template: Summary, Changes, Test Plan, Related all present
• Related references plan slug: No plan slug referenced (N/A -- this is a regression fix, not plan work)
• No secrets committed: Single file change, no credentials
• No scope creep: All three changes directly address the 4 test failures
• Tests exist: 4 new tests from #351 now pass (29 checkout + 6 webhook = 35 total)

PROCESS OBSERVATIONS

Clean regression fix. Three targeted changes, each addressing a specific failure mode. The diff is minimal and focused. The payment_status check is the most important fix -- without it, orders could be marked paid before Stripe confirms payment, which is a correctness issue for financial transactions.

The metadata None guard in Fix 2 only applies to the tryout registration path in stripe_webhook. The same pattern in _handle_jersey_checkout_completed and _handle_generic_order_completed (both use .get("metadata", {})) should get the same treatment in a follow-up.

VERDICT: APPROVED

## PR #359 Review ### DOMAIN REVIEW **Stack:** Python / FastAPI / SQLAlchemy (Stripe webhook handler) **Fix 1 -- payment_status guard (lines 212-220 in diff):** Correct. Stripe can deliver `checkout.session.completed` with `payment_status != "paid"` for async payment methods. The early `return True` correctly claims the event while skipping fulfillment. Log message includes the actual status for debugging. **Fix 2 -- metadata None guard (line 355 in diff):** Correct. `dict.get("metadata", {})` returns `None` when the key exists with an explicit `None` value -- the default only applies when the key is absent. `get("metadata") or {}` handles both cases. The `elif` restructuring is also correct: the old `else` branch would call `dict(None)` and crash; now only non-None/non-str/non-dict values hit `dict()`. **Fix 3 -- sync error handling (lines 225-233 in diff):** Correct. `sync_player_jersey_from_order` accesses `order.custom_data`, queries for player records, and maps enum values -- any of which could raise. The bare `except Exception` with `logger.exception` preserves the full traceback while ensuring `db.commit()` executes for the order status update. This is the right pattern for "best-effort side effect that must not block the primary transaction." ### BLOCKERS None. ### NITS 1. **Latent same-class bug in `_handle_jersey_checkout_completed` (line 212 on main):** Uses `session_data.get("metadata", {})` which has the identical None vulnerability fixed here for the tryout registration path. Out of scope for this PR, but worth a follow-up ticket. 2. **Bare `except Exception` scope:** The catch is appropriately scoped to just the sync call, so this is fine. No concern here -- just noting the pattern is intentional and correct. ### SOP COMPLIANCE - [x] Branch named after issue: `358-fix-webhook-handler-missing-payment-stat` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Related all present - [ ] Related references plan slug: No plan slug referenced (N/A -- this is a regression fix, not plan work) - [x] No secrets committed: Single file change, no credentials - [x] No scope creep: All three changes directly address the 4 test failures - [x] Tests exist: 4 new tests from #351 now pass (29 checkout + 6 webhook = 35 total) ### PROCESS OBSERVATIONS Clean regression fix. Three targeted changes, each addressing a specific failure mode. The diff is minimal and focused. The `payment_status` check is the most important fix -- without it, orders could be marked paid before Stripe confirms payment, which is a correctness issue for financial transactions. The metadata None guard in Fix 2 only applies to the tryout registration path in `stripe_webhook`. The same pattern in `_handle_jersey_checkout_completed` and `_handle_generic_order_completed` (both use `.get("metadata", {})`) should get the same treatment in a follow-up. ### VERDICT: APPROVED

forgejo_admin merged commit 19b5a002b4 into main 2026-04-06 22:11:59 +00:00

forgejo_admin deleted branch 358-fix-webhook-handler-missing-payment-stat 2026-04-06 22:12:00 +00:00

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

fix: add payment_status check, null metadata guard, and sync error handling (#359)

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

No description provided.