From ec91738bbc188dfed36135f81def04e0a78939d0 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Wed, 12 Aug 2026 15:02:37 -0400 Subject: [PATCH 01/12] feat(EDUN-15452): UPI payment_intent.succeeded webhook + orphan recovery Implement end-to-end CT order finalization from Stripe PaymentIntents: - Add CT client methods: get_cart_by_id, create_charge_payment_transaction - Add shared finalize_ct_order_from_stripe_pi module with full parity to customer-twou finalizeStripePayment (charge txn, order creation, line item state transition, Segment Order Completed plan 18, PI metadata backfill) - Add Celery task with bounded retries (CommercetoolsError, max=5, countdown=3) and quarantine log+metric on exhaustion - Add payment_succeeded_commercetools_signal + CC_SIGNALS wiring - Extend WebhookView: route source_system=commercetools to CT signal with SingleInvocation on payment_intent.id; leave legacy + refund paths unchanged - Add recovery management command recover_orphaned_stripe_commercetools_payments with --since, --limit, --dry-run; Stripe Search with list+filter fallback - Quarantine contract: structured log with pi_id, ct_payment_id, ct_cart_id, reason, source fields - 34 new tests covering finalize happy path, charge/order skip, error paths, webhook routing, task quarantine, recovery command dry-run/finalize/fallback - All 140 tests (106 existing + 34 new) pass with zero regressions Co-authored-by: Cursor --- .../apps/commercetools/clients.py | 75 +++++ ..._orphaned_stripe_commercetools_payments.py | 204 +++++++++++++ .../apps/commercetools/signals.py | 14 + .../commercetools/stripe_payment_finalize.py | 278 ++++++++++++++++++ .../apps/commercetools/tasks.py | 92 ++++++ .../commercetools/tests/test_finalize_task.py | 70 +++++ .../tests/test_recovery_command.py | 169 +++++++++++ .../tests/test_stripe_payment_finalize.py | 261 ++++++++++++++++ commerce_coordinator/apps/stripe/signals.py | 1 + .../apps/stripe/tests/test_views.py | 108 +++++++ commerce_coordinator/apps/stripe/views.py | 109 ++++--- commerce_coordinator/settings/base.py | 3 + 12 files changed, 1345 insertions(+), 39 deletions(-) create mode 100644 commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py create mode 100644 commerce_coordinator/apps/commercetools/stripe_payment_finalize.py create mode 100644 commerce_coordinator/apps/commercetools/tests/test_finalize_task.py create mode 100644 commerce_coordinator/apps/commercetools/tests/test_recovery_command.py create mode 100644 commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py diff --git a/commerce_coordinator/apps/commercetools/clients.py b/commerce_coordinator/apps/commercetools/clients.py index 02fc8029d..cc4528a5a 100644 --- a/commerce_coordinator/apps/commercetools/clients.py +++ b/commerce_coordinator/apps/commercetools/clients.py @@ -957,6 +957,67 @@ def create_return_payment_transaction( ) raise err + def create_charge_payment_transaction( + self, + payment_id: str, + payment_version: int, + charge_id: str, + amount_in_cents: int, + currency_code: str, + charge_created: datetime.datetime, + ) -> Payment: + """ + Add a Charge transaction to an existing CT Payment, mirroring + create_return_payment_transaction but for TransactionType.CHARGE. + + Args: + payment_id: CT Payment ID (UUID) + payment_version: Current version of the CT payment + charge_id: Stripe Charge ID (used as interaction_id for idempotency) + amount_in_cents: Charge amount in minor currency units + currency_code: ISO 4217 currency code (e.g. 'USD') + charge_created: Timestamp of the Stripe charge + + Returns: + Updated Payment object with the new Charge transaction + """ + try: + logger.info( + f"[CommercetoolsAPIClient] - Creating charge transaction for " + f"payment {payment_id} with charge {charge_id}" + ) + + amount_as_money = Money( + cent_amount=amount_in_cents, + currency_code=currency_code.upper(), + ) + + transaction_draft = TransactionDraft( + type=TransactionType.CHARGE, + amount=amount_as_money, + timestamp=charge_created, + state=TransactionState.SUCCESS, + interaction_id=charge_id, + ) + + add_transaction_action = PaymentAddTransactionAction( + transaction=transaction_draft + ) + + return self.base_client.payments.update_by_id( + id=payment_id, + version=payment_version, + actions=[add_transaction_action], + ) + except CommercetoolsError as err: + handle_commercetools_error( + "[CommercetoolsAPIClient.create_charge_payment_transaction]", + err, + f"Unable to create charge transaction for payment {payment_id}, " + f"charge {charge_id}", + ) + raise err + def update_line_item_on_fulfillment( self, entitlement_uuid: str, @@ -1342,6 +1403,20 @@ def update_customer( ) raise err + @conditional_retry + def get_cart_by_id(self, cart_id: str) -> Cart: + """ + Fetch a cart by its ID. + + Args: + cart_id (str): Cart ID (UUID) + + Returns: + Cart object + """ + logger.info(f"[CommercetoolsAPIClient] - Attempting to find cart with ID {cart_id}") + return self.base_client.carts.get_by_id(cart_id) + @conditional_retry def get_customer_cart(self, customer_id: str) -> Optional[Cart]: """ diff --git a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py new file mode 100644 index 000000000..8afb3f182 --- /dev/null +++ b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py @@ -0,0 +1,204 @@ +""" +Management command to discover and finalize orphaned Stripe PaymentIntents +that have source_system=commercetools, status=succeeded, but no order_id +in their metadata. + +Intended to run on an external cron (e.g. every 15-30 minutes). +""" + +import datetime +import logging +import time + +import stripe +from commercetools import CommercetoolsError +from django.conf import settings + +from commerce_coordinator.apps.commercetools.management.commands._ct_api_client_command import ( + CommercetoolsAPIClientCommand, +) +from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( + FinalizeError, + finalize_ct_order_from_stripe_pi, +) +from commerce_coordinator.apps.commercetools.tasks import _log_quarantine + +logger = logging.getLogger(__name__) + +stripe.api_key = settings.PAYMENT_PROCESSOR_CONFIG['edx']['stripe']['secret_key'] + + +class Command(CommercetoolsAPIClientCommand): + help = ( + "Discover orphaned Stripe PaymentIntents (succeeded, source_system=commercetools, " + "no order_id) and finalize them into CT orders. Supports --since, --limit, --dry-run." + ) + + def add_arguments(self, parser): + parser.add_argument( + "--since", + type=int, + default=7, + help="Lookback window in days (default: 7)", + ) + parser.add_argument( + "--limit", + type=int, + default=100, + help="Maximum number of orphan candidates to process per run (default: 100)", + ) + parser.add_argument( + "--dry-run", + action="store_true", + default=False, + help="List orphan candidates without calling finalize", + ) + + def handle(self, *args, **options): + since_days = options["since"] + limit = options["limit"] + dry_run = options["dry_run"] + + created_after = int( + (datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta(days=since_days)).timestamp() + ) + + self.stdout.write( + f"Recovery: since={since_days}d limit={limit} dry_run={dry_run}" + ) + + orphans = self._discover_stripe_orphans(created_after, limit) + self.stdout.write(f"Discovered {len(orphans)} Stripe orphan candidate(s)") + + if dry_run: + for pi_id in orphans: + self.stdout.write(f" [dry-run] orphan: {pi_id}") + return + + finalized = 0 + quarantined = 0 + + for pi_id in orphans: + try: + result = finalize_ct_order_from_stripe_pi( + pi_id, source="recovery", client=self.ct_api_client, + ) + if result.already_existed: + self.stdout.write( + f" [skip] {pi_id} -> order {result.order_id} already existed" + ) + else: + self.stdout.write( + f" [finalized] {pi_id} -> order {result.order_id}" + ) + finalized += 1 + except FinalizeError as exc: + self.stderr.write(f" [quarantine] {pi_id}: {exc}") + _log_quarantine( + pi_id=pi_id, + ct_payment_id="unknown", + ct_cart_id="unknown", + reason=str(exc), + source="recovery", + ) + quarantined += 1 + except (CommercetoolsError, Exception) as exc: + self.stderr.write(f" [quarantine] {pi_id}: {exc}") + _log_quarantine( + pi_id=pi_id, + ct_payment_id="unknown", + ct_cart_id="unknown", + reason=str(exc), + source="recovery", + ) + quarantined += 1 + + self.stdout.write( + f"Recovery complete: {finalized} finalized, {quarantined} quarantined, " + f"{len(orphans) - finalized - quarantined} skipped" + ) + + def _discover_stripe_orphans(self, created_after: int, limit: int) -> list[str]: + """ + Query Stripe for PaymentIntents that are succeeded with + source_system=commercetools but missing order_id metadata. + + Uses Stripe Search API with fallback to list+filter. + """ + orphan_ids = [] + + try: + orphan_ids = self._search_stripe_orphans(created_after, limit) + except Exception: + logger.warning( + "[recovery] Stripe Search API failed, falling back to list+filter", + exc_info=True, + ) + orphan_ids = self._list_filter_stripe_orphans(created_after, limit) + + return orphan_ids + + def _search_stripe_orphans(self, created_after: int, limit: int) -> list[str]: + """Use Stripe Search API to find orphaned PIs.""" + query = ( + f"status:'succeeded' " + f"AND metadata['source_system']:'commercetools' " + f"AND created>{created_after}" + ) + + orphan_ids = [] + has_more = True + next_page = None + + while has_more and len(orphan_ids) < limit: + kwargs = {"query": query, "limit": min(100, limit - len(orphan_ids))} + if next_page: + kwargs["page"] = next_page + + result = stripe.PaymentIntent.search(**kwargs) + + for pi in result.data: + metadata = pi.metadata or {} + if not metadata.get("order_id"): + orphan_ids.append(pi.id) + if len(orphan_ids) >= limit: + break + + has_more = result.has_more + next_page = result.next_page if has_more else None + + if has_more and len(orphan_ids) >= limit: + logger.info( + "[recovery] Stripe search truncated at limit=%d, more candidates may exist", + limit, + ) + + return orphan_ids + + def _list_filter_stripe_orphans(self, created_after: int, limit: int) -> list[str]: + """Fallback: list PIs and filter client-side.""" + orphan_ids = [] + + params = { + "limit": 100, + "created": {"gte": created_after}, + } + + for pi in stripe.PaymentIntent.list(**params).auto_paging_iter(): + if pi.status != "succeeded": + continue + + metadata = pi.metadata or {} + if metadata.get("source_system") != "commercetools": + continue + + if not metadata.get("order_id"): + orphan_ids.append(pi.id) + + if len(orphan_ids) >= limit: + logger.info( + "[recovery] List+filter truncated at limit=%d", limit, + ) + break + + return orphan_ids diff --git a/commerce_coordinator/apps/commercetools/signals.py b/commerce_coordinator/apps/commercetools/signals.py index bca8b5850..3724dd269 100644 --- a/commerce_coordinator/apps/commercetools/signals.py +++ b/commerce_coordinator/apps/commercetools/signals.py @@ -6,6 +6,7 @@ from commerce_coordinator.apps.commercetools.catalog_info.constants import TwoUKeys from commerce_coordinator.apps.commercetools.tasks import ( + finalize_commercetools_stripe_payment_task, fulfillment_completed_update_ct_line_item_task, refund_from_mobile_task, refund_from_paypal_task, @@ -94,6 +95,19 @@ def revoke_line_items(**kwargs): return async_result.id +@log_receiver(logger) +def finalize_commercetools_stripe_payment(**kwargs): + """ + Receive the payment_succeeded_commercetools_signal and dispatch + the shared finalize task for a CT Stripe PaymentIntent. + """ + async_result = finalize_commercetools_stripe_payment_task.delay( + payment_intent_id=kwargs["payment_intent_id"], + source="webhook", + ) + return async_result.id + + @log_receiver(logger) def revoke_line_mobile_order(**kwargs): """ diff --git a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py new file mode 100644 index 000000000..8f0549582 --- /dev/null +++ b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py @@ -0,0 +1,278 @@ +""" +Shared finalization logic for CommerceTools orders originating from Stripe +PaymentIntents (UPI webhook + orphan recovery). + +Parity source: customer-twou finalizeStripePayment + runPostPaymentActions. +""" + +import datetime +import logging +from dataclasses import dataclass + +import stripe +from commercetools import CommercetoolsError +from commercetools.platform.models import TransactionType + +from commerce_coordinator.apps.commercetools.catalog_info.constants import TwoUKeys +from commerce_coordinator.apps.commercetools.catalog_info.edx_utils import ( + cents_to_dollars, + get_edx_lms_user_id, + get_product_from_line_item, +) +from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient +from commerce_coordinator.apps.core.segment import track + +logger = logging.getLogger(__name__) + + +class FinalizeError(Exception): + """Non-retryable finalization error (quarantine candidate).""" + + +@dataclass +class FinalizeResult: + order_id: str + order_number: str + payment_id: str + already_existed: bool = False + + +def _payment_has_charge_for(payment, charge_id: str) -> bool: + """Check whether the CT payment already has a Charge txn for this charge.""" + if not payment.transactions: + return False + return any( + t.type == TransactionType.CHARGE and t.interaction_id == charge_id + for t in payment.transactions + ) + + +def finalize_ct_order_from_stripe_pi( + payment_intent_id: str, + *, + source: str, + client: CommercetoolsAPIClient | None = None, +) -> FinalizeResult: + """ + Shared finalize path used by both the webhook Celery task and the + recovery management command. + + Steps (parity with customer-twou finalizeStripePayment): + 1. Retrieve / validate Stripe PaymentIntent + 2. Resolve CT Payment (by key = pi.id or metadata.ct_payment_id) + 3. Resolve CT Cart (by metadata.ct_cart_id) + 4. Add Charge transaction if absent (idempotent by interaction_id) + 5. Skip if order already exists for this payment + 6. Create order from cart → COMPLETE / PAID / SHIPPED + 7. Transition line items → PENDING_FULFILMENT + 8. Emit Segment Order Completed (plan 18, is_mobile=False) + 9. Backfill PI metadata with order_id + ct_payment_id + + Args: + payment_intent_id: Stripe PaymentIntent ID + source: 'webhook' or 'recovery' (for quarantine log context) + client: Optional pre-built CT client (avoids re-init in loops) + + Returns: + FinalizeResult with order details + + Raises: + FinalizeError: on non-retryable problems (missing metadata, etc.) + CommercetoolsError: on transient CT failures (retryable by caller) + """ + if client is None: + client = CommercetoolsAPIClient() + + pi = stripe.PaymentIntent.retrieve(payment_intent_id) + + if pi.status != "succeeded": + raise FinalizeError( + f"PaymentIntent {payment_intent_id} status is '{pi.status}', expected 'succeeded'" + ) + + metadata = pi.metadata or {} + if metadata.get("source_system") != "commercetools": + raise FinalizeError( + f"PaymentIntent {payment_intent_id} source_system is " + f"'{metadata.get('source_system')}', expected 'commercetools'" + ) + + ct_cart_id = metadata.get("ct_cart_id") + if not ct_cart_id: + raise FinalizeError( + f"PaymentIntent {payment_intent_id} missing metadata.ct_cart_id" + ) + + ct_payment_id_from_meta = metadata.get("ct_payment_id") + + # --- Resolve CT Payment --- + if ct_payment_id_from_meta: + try: + payment = client.base_client.payments.get_by_id(ct_payment_id_from_meta) + except CommercetoolsError: + logger.warning( + "[finalize_ct_order] ct_payment_id %s from metadata not found, " + "falling back to key lookup for pi %s", + ct_payment_id_from_meta, payment_intent_id, + ) + payment = client.get_payment_by_key(payment_intent_id) + else: + payment = client.get_payment_by_key(payment_intent_id) + + # --- Add Charge transaction if absent --- + latest_charge = pi.latest_charge + if latest_charge and isinstance(latest_charge, str): + latest_charge = stripe.Charge.retrieve(latest_charge) + + if latest_charge and not _payment_has_charge_for(payment, latest_charge.id): + payment = client.create_charge_payment_transaction( + payment_id=payment.id, + payment_version=payment.version, + charge_id=latest_charge.id, + amount_in_cents=latest_charge.amount, + currency_code=latest_charge.currency, + charge_created=datetime.datetime.fromtimestamp( + latest_charge.created, tz=datetime.timezone.utc + ), + ) + logger.info( + "[finalize_ct_order] Added Charge txn for pi=%s charge=%s", + payment_intent_id, latest_charge.id, + ) + + # --- Check if order already exists --- + try: + existing_order = client.get_order_by_payment_id(payment.id) + logger.info( + "[finalize_ct_order] Order %s already exists for payment %s (pi=%s), skipping creation", + existing_order.id, payment.id, payment_intent_id, + ) + return FinalizeResult( + order_id=existing_order.id, + order_number=existing_order.order_number or "", + payment_id=payment.id, + already_existed=True, + ) + except Exception: + pass + + # --- Load cart and create order --- + cart = client.get_cart_by_id(ct_cart_id) + order = client.create_order_from_cart(cart) + + # --- Transition line items → PENDING_FULFILMENT --- + order = client.update_line_items_transition_state( + order_id=order.id, + order_version=order.version, + line_items=order.line_items, + from_state_id=order.line_items[0].state[0].state.id, + new_state_key=TwoUKeys.PENDING_FULFILMENT_STATE, + use_state_id=True, + ) + + # --- Emit Segment Order Completed (plan 18, web) --- + _emit_web_order_completed(client, order, cart, payment) + + # --- Backfill PI metadata --- + try: + stripe.PaymentIntent.modify( + payment_intent_id, + metadata={ + "order_id": order.id, + "ct_payment_id": payment.id, + }, + ) + except Exception: + logger.warning( + "[finalize_ct_order] Failed to backfill PI metadata for %s", + payment_intent_id, exc_info=True, + ) + + logger.info( + "[finalize_ct_order] Successfully finalized order %s for pi=%s source=%s", + order.id, payment_intent_id, source, + ) + + return FinalizeResult( + order_id=order.id, + order_number=order.order_number or "", + payment_id=payment.id, + ) + + +def _emit_web_order_completed(client, order, cart, payment): + """Emit Segment 'Order Completed' event for the web/UPI path (plan 18).""" + try: + customer = client.get_customer_by_id(order.customer_id) + lms_user_id = get_edx_lms_user_id(customer) + + standalone_price = cart.total_price + products = [ + get_product_from_line_item(item, standalone_price) + for item in cart.line_items + ] + + payment_method = "unknown" + processor_name = "stripe" + if payment.payment_method_info: + payment_method = payment.payment_method_info.method or "unknown" + if payment.payment_method_info.name: + processor_name = payment.payment_method_info.name.get("en", "stripe") + + discount_codes = getattr(cart, "discount_codes", []) or [] + discount_code = None + coupon_name = [] + if discount_codes: + codes_as_dicts = [] + for dc in discount_codes: + if hasattr(dc, "code"): + codes_as_dicts.append({"code": dc.code}) + elif isinstance(dc, dict) and "code" in dc: + codes_as_dicts.append(dc) + if codes_as_dicts: + discount_code = codes_as_dicts[-1].get("code") + coupon_name = [ + d["code"] for d in codes_as_dicts[:-1] + if d.get("code") + ] + + taxed_amount = 0 + if order.taxed_price and order.taxed_price.total_tax: + taxed_amount = cents_to_dollars(order.taxed_price.total_tax) + + discount_amount = 0 + if cart.discount_on_total_price: + discount_amount = cents_to_dollars(cart.discount_on_total_price) + + event_props = { + "track_plan_id": 18, + "trigger_source": "server-side", + "order_id": order.id, + "checkout_id": cart.id, + "currency": standalone_price.currency_code, + "total": cents_to_dollars(standalone_price), + "tax": taxed_amount, + "coupon": discount_code, + "coupon_name": coupon_name, + "discount": discount_amount, + "payment_method": payment_method, + "processor_name": processor_name, + "products": products, + "is_mobile": False, + "multi_item_cart_enabled": len(cart.line_items) > 1, + } + + track( + lms_user_id=lms_user_id, + event="Order Completed", + properties=event_props, + ) + logger.info( + "[finalize_ct_order] Emitted Segment Order Completed for order %s, user %s", + order.id, lms_user_id, + ) + except Exception: + logger.warning( + "[finalize_ct_order] Failed to emit Segment Order Completed for order %s", + order.id, exc_info=True, + ) diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index ce46f335b..2d7ef7086 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -552,3 +552,95 @@ def revoke_line_mobile_order_task(payment_id: str): f"on course {course_run_key} and {logging_data}") return True + + +def _log_quarantine(*, pi_id, ct_payment_id, ct_cart_id, reason, source): + """ + Structured quarantine log for finalize failures that exhaust retries + or hit non-retryable errors. Fixed field contract for ops queries. + """ + logger.error( + "[quarantine] Finalize failure | pi_id=%s ct_payment_id=%s " + "ct_cart_id=%s reason=%s source=%s", + pi_id, ct_payment_id, ct_cart_id, reason, source, + extra={ + "quarantine": True, + "pi_id": pi_id, + "ct_payment_id": ct_payment_id, + "ct_cart_id": ct_cart_id, + "reason": reason, + "source": source, + }, + ) + + +@shared_task( + bind=True, + autoretry_for=(CommercetoolsError,), + retry_kwargs={"max_retries": 5, "countdown": 3}, +) +def finalize_commercetools_stripe_payment_task( + self, + payment_intent_id: str, + source: str = "webhook", +): + """ + Celery task wrapping the shared finalize path for a Stripe + PaymentIntent that originated from a CommerceTools cart. + + Bounded retries on transient CT errors; non-retryable failures + are quarantined via structured log + metric. + """ + from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( + FinalizeError, + finalize_ct_order_from_stripe_pi, + ) + + tag = "finalize_commercetools_stripe_payment_task" + + try: + result = finalize_ct_order_from_stripe_pi( + payment_intent_id, source=source, + ) + if result.already_existed: + logger.info( + "[%s] Order %s already existed for pi=%s, no action taken", + tag, result.order_id, payment_intent_id, + ) + else: + logger.info( + "[%s] Finalized order %s for pi=%s", + tag, result.order_id, payment_intent_id, + ) + return result.order_id + + except FinalizeError as exc: + _log_quarantine( + pi_id=payment_intent_id, + ct_payment_id="unknown", + ct_cart_id="unknown", + reason=str(exc), + source=source, + ) + return None + + except CommercetoolsError: + if self.request.retries >= self.max_retries: + _log_quarantine( + pi_id=payment_intent_id, + ct_payment_id="unknown", + ct_cart_id="unknown", + reason="max retries exhausted on CommercetoolsError", + source=source, + ) + raise + + except Exception as exc: + _log_quarantine( + pi_id=payment_intent_id, + ct_payment_id="unknown", + ct_cart_id="unknown", + reason=f"unexpected error: {exc}", + source=source, + ) + raise diff --git a/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py b/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py new file mode 100644 index 000000000..a7ead3542 --- /dev/null +++ b/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py @@ -0,0 +1,70 @@ +""" +Tests for the finalize_commercetools_stripe_payment_task Celery task. +""" + +from unittest.mock import patch + +from commercetools import CommercetoolsError +from django.test import TestCase + +from commerce_coordinator.apps.commercetools.stripe_payment_finalize import FinalizeError, FinalizeResult +from commerce_coordinator.apps.commercetools.tasks import finalize_commercetools_stripe_payment_task + +FINALIZE_PATH = ( + "commerce_coordinator.apps.commercetools.stripe_payment_finalize" + ".finalize_ct_order_from_stripe_pi" +) + + +class TestFinalizeTask(TestCase): + + @patch(FINALIZE_PATH) + def test_happy_path_returns_order_id(self, mock_finalize): + mock_finalize.return_value = FinalizeResult( + order_id="order-123", + order_number="2U-2026000001", + payment_id="pay-456", + ) + + result = finalize_commercetools_stripe_payment_task("pi_test") + self.assertEqual(result, "order-123") + mock_finalize.assert_called_once_with("pi_test", source="webhook") + + @patch(FINALIZE_PATH) + def test_already_existed_returns_order_id(self, mock_finalize): + mock_finalize.return_value = FinalizeResult( + order_id="order-existing", + order_number="2U-2026000002", + payment_id="pay-789", + already_existed=True, + ) + + result = finalize_commercetools_stripe_payment_task("pi_test") + self.assertEqual(result, "order-existing") + + @patch(FINALIZE_PATH) + @patch("commerce_coordinator.apps.commercetools.tasks._log_quarantine") + def test_finalize_error_quarantines_and_returns_none( + self, mock_quarantine, mock_finalize + ): + mock_finalize.side_effect = FinalizeError("missing ct_cart_id") + + result = finalize_commercetools_stripe_payment_task("pi_bad") + self.assertIsNone(result) + mock_quarantine.assert_called_once() + call_kwargs = mock_quarantine.call_args[1] + self.assertEqual(call_kwargs["pi_id"], "pi_bad") + self.assertEqual(call_kwargs["source"], "webhook") + self.assertIn("missing ct_cart_id", call_kwargs["reason"]) + + @patch(FINALIZE_PATH) + @patch("commerce_coordinator.apps.commercetools.tasks._log_quarantine") + def test_unexpected_error_quarantines_and_reraises( + self, mock_quarantine, mock_finalize + ): + mock_finalize.side_effect = RuntimeError("boom") + + with self.assertRaises(RuntimeError): + finalize_commercetools_stripe_payment_task("pi_boom") + + mock_quarantine.assert_called_once() diff --git a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py new file mode 100644 index 000000000..10abbd748 --- /dev/null +++ b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py @@ -0,0 +1,169 @@ +""" +Tests for the recover_orphaned_stripe_commercetools_payments management command. +""" + +from io import StringIO +from unittest.mock import MagicMock, patch + +from django.test import TestCase + +from commerce_coordinator.apps.commercetools.stripe_payment_finalize import FinalizeError, FinalizeResult + +CMD_MODULE = ( + "commerce_coordinator.apps.commercetools.management.commands" + ".recover_orphaned_stripe_commercetools_payments" +) + + +@patch(f"{CMD_MODULE}.CommercetoolsAPIClientCommand.__init__", return_value=None) +class TestRecoveryCommand(TestCase): + + def _make_command(self): + from commerce_coordinator.apps.commercetools.management.commands.recover_orphaned_stripe_commercetools_payments import Command # noqa: E501 + + cmd = Command() + cmd.ct_api_client = MagicMock() + cmd.stdout = StringIO() + cmd.stderr = StringIO() + return cmd + + @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") + def test_dry_run_lists_candidates(self, mock_finalize, _mock_init): + cmd = self._make_command() + + pi1 = MagicMock() + pi1.id = "pi_orphan1" + pi1.metadata = {"source_system": "commercetools"} + pi2 = MagicMock() + pi2.id = "pi_orphan2" + pi2.metadata = {"source_system": "commercetools"} + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + search_result = MagicMock() + search_result.data = [pi1, pi2] + search_result.has_more = False + mock_stripe.PaymentIntent.search.return_value = search_result + + cmd.handle(since=7, limit=100, dry_run=True) + + output = cmd.stdout.getvalue() + self.assertIn("[dry-run] orphan: pi_orphan1", output) + self.assertIn("[dry-run] orphan: pi_orphan2", output) + mock_finalize.assert_not_called() + + @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") + def test_finalize_happy_path(self, mock_finalize, _mock_init): + cmd = self._make_command() + + pi1 = MagicMock() + pi1.id = "pi_orphan1" + pi1.metadata = {"source_system": "commercetools"} + + mock_finalize.return_value = FinalizeResult( + order_id="order-new", + order_number="2U-2026000001", + payment_id="pay-123", + ) + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + search_result = MagicMock() + search_result.data = [pi1] + search_result.has_more = False + mock_stripe.PaymentIntent.search.return_value = search_result + + cmd.handle(since=7, limit=100, dry_run=False) + + output = cmd.stdout.getvalue() + self.assertIn("[finalized] pi_orphan1 -> order order-new", output) + self.assertIn("1 finalized", output) + + @patch(f"{CMD_MODULE}._log_quarantine") + @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") + def test_finalize_error_quarantines(self, mock_finalize, mock_quarantine, _mock_init): + cmd = self._make_command() + + pi1 = MagicMock() + pi1.id = "pi_bad" + pi1.metadata = {"source_system": "commercetools"} + + mock_finalize.side_effect = FinalizeError("missing cart") + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + search_result = MagicMock() + search_result.data = [pi1] + search_result.has_more = False + mock_stripe.PaymentIntent.search.return_value = search_result + + cmd.handle(since=7, limit=100, dry_run=False) + + err_output = cmd.stderr.getvalue() + self.assertIn("[quarantine] pi_bad", err_output) + mock_quarantine.assert_called_once() + self.assertIn("1 quarantined", cmd.stdout.getvalue()) + + @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") + def test_already_existed_skips(self, mock_finalize, _mock_init): + cmd = self._make_command() + + pi1 = MagicMock() + pi1.id = "pi_existing" + pi1.metadata = {"source_system": "commercetools"} + + mock_finalize.return_value = FinalizeResult( + order_id="order-old", + order_number="2U-2026000002", + payment_id="pay-456", + already_existed=True, + ) + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + search_result = MagicMock() + search_result.data = [pi1] + search_result.has_more = False + mock_stripe.PaymentIntent.search.return_value = search_result + + cmd.handle(since=7, limit=100, dry_run=False) + + output = cmd.stdout.getvalue() + self.assertIn("[skip] pi_existing", output) + + def test_limit_truncates(self, _mock_init): + cmd = self._make_command() + + pis = [] + for i in range(10): + pi = MagicMock() + pi.id = f"pi_orphan_{i}" + pi.metadata = {"source_system": "commercetools"} + pis.append(pi) + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + search_result = MagicMock() + search_result.data = pis + search_result.has_more = True + search_result.next_page = "page2" + mock_stripe.PaymentIntent.search.return_value = search_result + + cmd.handle(since=7, limit=3, dry_run=True) + + output = cmd.stdout.getvalue() + self.assertIn("3 Stripe orphan candidate(s)", output) + + def test_search_fallback_to_list(self, _mock_init): + cmd = self._make_command() + + pi1 = MagicMock() + pi1.id = "pi_list_orphan" + pi1.status = "succeeded" + pi1.metadata = {"source_system": "commercetools"} + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + mock_stripe.PaymentIntent.search.side_effect = Exception("search not available") + list_result = MagicMock() + list_result.auto_paging_iter.return_value = [pi1] + mock_stripe.PaymentIntent.list.return_value = list_result + + cmd.handle(since=7, limit=100, dry_run=True) + + output = cmd.stdout.getvalue() + self.assertIn("[dry-run] orphan: pi_list_orphan", output) diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py new file mode 100644 index 000000000..431bf706f --- /dev/null +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py @@ -0,0 +1,261 @@ +""" +Tests for the shared CT order finalization from Stripe PaymentIntents. +""" + +import datetime +from unittest.mock import MagicMock, Mock, patch + +from commercetools import CommercetoolsError +from commercetools.platform.models import ( + CentPrecisionMoney, + CustomFields, + FieldContainer, + Order, + Payment, + PaymentMethodInfo, + PaymentState, + Transaction, + TransactionState, + TransactionType, + TypeReference, +) +from django.test import TestCase + +from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( + FinalizeError, + FinalizeResult, + _payment_has_charge_for, + finalize_ct_order_from_stripe_pi, +) +from commerce_coordinator.apps.commercetools.tests.conftest import ( + gen_cart, + gen_customer, + gen_order, +) +from commerce_coordinator.apps.core.tests.utils import uuid4_str + + +def _mock_pi( + pi_id="pi_test123", + pi_status="succeeded", + source_system="commercetools", + ct_cart_id="cart-uuid", + ct_payment_id=None, + order_id=None, + latest_charge="ch_test456", +): + pi = MagicMock() + pi.id = pi_id + pi.status = pi_status + pi.metadata = { + "source_system": source_system, + "ct_cart_id": ct_cart_id, + } + if ct_payment_id: + pi.metadata["ct_payment_id"] = ct_payment_id + if order_id: + pi.metadata["order_id"] = order_id + pi.latest_charge = latest_charge + return pi + + +def _mock_charge(charge_id="ch_test456", amount=4900, currency="usd"): + charge = MagicMock() + charge.id = charge_id + charge.amount = amount + charge.currency = currency + charge.created = 1700000000 + return charge + + +def _mock_payment(payment_id=None, version=1, has_charge=False, charge_id="ch_test456"): + txns = [] + if has_charge: + txns.append(Transaction( + id=uuid4_str(), + type=TransactionType.CHARGE, + amount=CentPrecisionMoney(cent_amount=4900, currency_code="USD", fraction_digits=2), + state=TransactionState.SUCCESS, + interaction_id=charge_id, + timestamp=datetime.datetime.now(), + )) + return Payment( + id=payment_id or uuid4_str(), + version=version, + created_at=datetime.datetime.now(), + last_modified_at=datetime.datetime.now(), + amount_planned=CentPrecisionMoney(cent_amount=4900, currency_code="USD", fraction_digits=2), + payment_method_info=PaymentMethodInfo(method="upi"), + payment_status=PaymentState.PAID, + transactions=txns, + interface_interactions=[], + ) + + +class TestPaymentHasChargeFor(TestCase): + def test_no_transactions(self): + payment = _mock_payment() + self.assertFalse(_payment_has_charge_for(payment, "ch_test")) + + def test_has_matching_charge(self): + payment = _mock_payment(has_charge=True, charge_id="ch_match") + self.assertTrue(_payment_has_charge_for(payment, "ch_match")) + + def test_has_different_charge(self): + payment = _mock_payment(has_charge=True, charge_id="ch_other") + self.assertFalse(_payment_has_charge_for(payment, "ch_match")) + + +@patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.stripe") +@patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.track") +@patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.CommercetoolsAPIClient") +class TestFinalizeCTOrderFromStripePI(TestCase): + + def test_happy_path(self, MockClient, mock_track, mock_stripe): + """Full finalize: charge + order + line state + segment + PI metadata.""" + pi = _mock_pi() + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-123") + order = gen_order(uuid4_str()) + cart = gen_cart(cart_id="cart-uuid", customer_id=order.customer_id) + customer = gen_customer("test@example.com", "testuser") + + client = MockClient.return_value + client.get_payment_by_key.return_value = payment + client.create_charge_payment_transaction.return_value = payment + client.get_order_by_payment_id.side_effect = Exception("not found") + client.get_cart_by_id.return_value = cart + client.create_order_from_cart.return_value = order + client.update_line_items_transition_state.return_value = order + client.get_customer_by_id.return_value = customer + + result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + self.assertEqual(result.order_id, order.id) + self.assertFalse(result.already_existed) + client.create_charge_payment_transaction.assert_called_once() + client.create_order_from_cart.assert_called_once_with(cart) + client.update_line_items_transition_state.assert_called_once() + mock_track.assert_called_once() + + def test_order_already_exists_skips(self, MockClient, mock_track, mock_stripe): + """When order already exists for the payment, skip creation.""" + pi = _mock_pi() + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-123", has_charge=True, charge_id="ch_test456") + existing_order = gen_order(uuid4_str()) + + client = MockClient.return_value + client.get_payment_by_key.return_value = payment + client.get_order_by_payment_id.return_value = existing_order + + result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + self.assertTrue(result.already_existed) + self.assertEqual(result.order_id, existing_order.id) + client.create_order_from_cart.assert_not_called() + mock_track.assert_not_called() + + def test_charge_already_present_skips_creation(self, MockClient, mock_track, mock_stripe): + """When charge transaction already exists, don't add another.""" + pi = _mock_pi() + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-123", has_charge=True, charge_id="ch_test456") + order = gen_order(uuid4_str()) + cart = gen_cart(cart_id="cart-uuid", customer_id=order.customer_id) + customer = gen_customer("test@example.com", "testuser") + + client = MockClient.return_value + client.get_payment_by_key.return_value = payment + client.get_order_by_payment_id.side_effect = Exception("not found") + client.get_cart_by_id.return_value = cart + client.create_order_from_cart.return_value = order + client.update_line_items_transition_state.return_value = order + client.get_customer_by_id.return_value = customer + + result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + client.create_charge_payment_transaction.assert_not_called() + self.assertFalse(result.already_existed) + + def test_pi_not_succeeded_raises_finalize_error(self, MockClient, mock_track, mock_stripe): + pi = _mock_pi(pi_status="requires_payment_method") + mock_stripe.PaymentIntent.retrieve.return_value = pi + + with self.assertRaises(FinalizeError) as ctx: + finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") + + self.assertIn("requires_payment_method", str(ctx.exception)) + + def test_wrong_source_system_raises_finalize_error(self, MockClient, mock_track, mock_stripe): + pi = _mock_pi(source_system="edx/commerce_coordinator?v=1") + mock_stripe.PaymentIntent.retrieve.return_value = pi + + with self.assertRaises(FinalizeError): + finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") + + def test_missing_ct_cart_id_raises_finalize_error(self, MockClient, mock_track, mock_stripe): + pi = _mock_pi(ct_cart_id=None) + pi.metadata.pop("ct_cart_id", None) + mock_stripe.PaymentIntent.retrieve.return_value = pi + + with self.assertRaises(FinalizeError): + finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + def test_resolves_payment_by_ct_payment_id(self, MockClient, mock_track, mock_stripe): + """When ct_payment_id is in metadata, use it for lookup.""" + pi = _mock_pi(ct_payment_id="pay-from-meta") + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-from-meta", has_charge=True, charge_id="ch_test456") + existing_order = gen_order(uuid4_str()) + + client = MockClient.return_value + client.base_client.payments.get_by_id.return_value = payment + client.get_order_by_payment_id.return_value = existing_order + + result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + client.base_client.payments.get_by_id.assert_called_once_with("pay-from-meta") + self.assertTrue(result.already_existed) + + def test_segment_event_has_web_properties(self, MockClient, mock_track, mock_stripe): + """Segment Order Completed should have is_mobile=False and plan 18.""" + pi = _mock_pi() + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-123") + order = gen_order(uuid4_str()) + cart = gen_cart(cart_id="cart-uuid", customer_id=order.customer_id) + customer = gen_customer("test@example.com", "testuser") + + client = MockClient.return_value + client.get_payment_by_key.return_value = payment + client.create_charge_payment_transaction.return_value = payment + client.get_order_by_payment_id.side_effect = Exception("not found") + client.get_cart_by_id.return_value = cart + client.create_order_from_cart.return_value = order + client.update_line_items_transition_state.return_value = order + client.get_customer_by_id.return_value = customer + + finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + call_kwargs = mock_track.call_args + props = call_kwargs[1]["properties"] if "properties" in call_kwargs[1] else call_kwargs[0][2] + self.assertFalse(props["is_mobile"]) + self.assertEqual(props["track_plan_id"], 18) + self.assertEqual(props["trigger_source"], "server-side") + self.assertEqual(props["processor_name"], "stripe") diff --git a/commerce_coordinator/apps/stripe/signals.py b/commerce_coordinator/apps/stripe/signals.py index 453a95124..9d230ea9c 100644 --- a/commerce_coordinator/apps/stripe/signals.py +++ b/commerce_coordinator/apps/stripe/signals.py @@ -5,3 +5,4 @@ payment_processed_signal = CoordinatorSignal() payment_refunded_signal = CoordinatorSignal() +payment_succeeded_commercetools_signal = CoordinatorSignal() diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index d5476d843..7750643e9 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -79,6 +79,114 @@ def test_stripe_signature_verification_error(self): ) ) + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') + def test_ct_payment_succeeded_fires_signal(self, mock_ct_signal, mock_construct_event): + """ + Verify payment_succeeded_commercetools_signal is emitted for + payment_intent.succeeded with source_system=commercetools. + """ + pi_id = 'pi_ct_test_123' + self.mock_stripe_event.type = StripeEventType.PAYMENT_SUCCESS.value + metadata = {'source_system': 'commercetools', 'ct_cart_id': 'cart-uuid'} + self.mock_stripe_event.data.object.id = pi_id + self.mock_stripe_event.data.object.metadata = StripeObject() + self.mock_stripe_event.data.object.metadata.update(metadata) + self.mock_stripe_event.data.object.amount = 4900 + mock_construct_event.return_value = self.mock_stripe_event + + response = self.client.post( + self.url, data={}, format='json', **self.mock_header + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_ct_signal.assert_called_once_with( + sender=WebhookView, + payment_intent_id=pi_id, + ) + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') + def test_ct_payment_failed_returns_200_no_signal(self, mock_ct_signal, mock_construct_event): + """ + Verify payment_intent.payment_failed with source_system=commercetools + returns 200 but does NOT fire the CT succeeded signal. + """ + self.mock_stripe_event.type = StripeEventType.PAYMENT_FAILED.value + metadata = {'source_system': 'commercetools', 'ct_cart_id': 'cart-uuid'} + self.mock_stripe_event.data.object.id = 'pi_ct_fail' + self.mock_stripe_event.data.object.metadata = StripeObject() + self.mock_stripe_event.data.object.metadata.update(metadata) + self.mock_stripe_event.data.object.amount = 4900 + mock_construct_event.return_value = self.mock_stripe_event + + response = self.client.post( + self.url, data={}, format='json', **self.mock_header + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_ct_signal.assert_not_called() + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_processed_signal.send_robust') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') + def test_legacy_payment_succeeded_fires_processed_signal( + self, mock_ct_signal, mock_processed_signal, mock_construct_event + ): + """ + Verify legacy source_system still fires payment_processed_signal, + not the CT signal. + """ + pi_id = 'pi_legacy' + source_system = settings.PAYMENT_PROCESSOR_CONFIG['edx']['stripe']['source_system_identifier'] + self.mock_stripe_event.type = StripeEventType.PAYMENT_SUCCESS.value + metadata = { + 'source_system': source_system, + 'edx_lms_user_id': '123', + 'order_number': 'EDX-000001', + 'payment_number': 'PAY-001', + } + self.mock_stripe_event.data.object.id = pi_id + self.mock_stripe_event.data.object.metadata = StripeObject() + self.mock_stripe_event.data.object.metadata.update(metadata) + self.mock_stripe_event.data.object.amount = 4900 + self.mock_stripe_event.data.object.currency = 'usd' + mock_construct_event.return_value = self.mock_stripe_event + + response = self.client.post( + self.url, data={}, format='json', **self.mock_header + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_ct_signal.assert_not_called() + mock_processed_signal.assert_called_once() + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_processed_signal.send_robust') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') + def test_unknown_source_system_skips_both_signals( + self, mock_ct_signal, mock_processed_signal, mock_construct_event + ): + """ + Verify that an unrecognized source_system returns 200 + but does not fire any signal. + """ + self.mock_stripe_event.type = StripeEventType.PAYMENT_SUCCESS.value + metadata = {'source_system': 'unknown_system'} + self.mock_stripe_event.data.object.id = 'pi_unknown' + self.mock_stripe_event.data.object.metadata = StripeObject() + self.mock_stripe_event.data.object.metadata.update(metadata) + self.mock_stripe_event.data.object.amount = 4900 + mock_construct_event.return_value = self.mock_stripe_event + + response = self.client.post( + self.url, data={}, format='json', **self.mock_header + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_ct_signal.assert_not_called() + mock_processed_signal.assert_not_called() + @ddt.data( name_test( "Test 2U order refund and correct source_system", diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 33a778d2f..c367d1e33 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -19,7 +19,11 @@ SignatureVerificationAPIError, UnhandledStripeEventAPIError ) -from commerce_coordinator.apps.stripe.signals import payment_processed_signal, payment_refunded_signal +from commerce_coordinator.apps.stripe.signals import ( + payment_processed_signal, + payment_refunded_signal, + payment_succeeded_commercetools_signal, +) logger = logging.getLogger(__name__) @@ -40,7 +44,6 @@ class WebhookView(SingleInvocationAPIView): http_method_names = ['post'] # accept POST request only authentication_classes = [] permission_classes = [AllowAny] - # TODO: Make this endpoint accessible for Stripe servers only. To be done in SONIC-898. @csrf_exempt def post(self, request): @@ -61,10 +64,71 @@ def post(self, request): raise SignatureVerificationAPIError from e # Handle the event - if event.type == StripeEventType.PAYMENT_SUCCESS: - payment_state = PaymentState.COMPLETED.value - elif event.type == StripeEventType.PAYMENT_FAILED: - payment_state = PaymentState.FAILED.value + if event.type in (StripeEventType.PAYMENT_SUCCESS, StripeEventType.PAYMENT_FAILED): + payment_intent = event.data.object + event_source_system = payment_intent.metadata.get('source_system') + + # --- CommerceTools path (UPI / CT-originated PIs) --- + if event_source_system == 'commercetools': + if event.type == StripeEventType.PAYMENT_SUCCESS: + if self._is_running(tag, payment_intent.id): # pragma no cover + self.meta_should_mark_not_running = False + return Response(status=status.HTTP_200_OK) + else: + self.mark_running(tag, payment_intent.id) + + logger.info( + '[Stripe webhooks] CT payment_intent.succeeded for PI [%s]', + payment_intent.id, + ) + + payment_succeeded_commercetools_signal.send_robust( + sender=self.__class__, + payment_intent_id=payment_intent.id, + ) + else: + logger.info( + '[Stripe webhooks] CT payment_intent.payment_failed for PI [%s], ignoring', + payment_intent.id, + ) + return Response(status=status.HTTP_200_OK) + + # --- Legacy edX path --- + if event.type == StripeEventType.PAYMENT_SUCCESS: + payment_state = PaymentState.COMPLETED.value + else: + payment_state = PaymentState.FAILED.value + + logger.info( + '[Stripe webhooks] event %s with amount %d and payment intent ID [%s], source: [%s].', + event.type, + payment_intent.amount, + payment_intent.id, + event_source_system, + ) + + if event_source_system != source_system_identifier: + logger.info( + '[Stripe webhooks] Skipping event %s with payment intent ID [%s], source: [%s].', + event.type, + payment_intent.id, + event_source_system, + ) + return Response(status=status.HTTP_200_OK) + + payment_processed_signal.send_robust( + sender=self.__class__, + edx_lms_user_id=payment_intent.metadata.edx_lms_user_id, + order_uuid=payment_intent.metadata.order_number, + payment_number=payment_intent.metadata.payment_number, + payment_state=payment_state, + reference_number=payment_intent.id, + amount_in_cents=payment_intent.amount, + currency=payment_intent.currency, + provider_response_body=payload, + ) + return Response(status=status.HTTP_200_OK) + elif event.type == StripeEventType.PAYMENT_REFUNDED: idempotency_key = event.get('request').get('idempotency_key') if self._is_running(tag, idempotency_key): # pragma no cover @@ -110,36 +174,3 @@ def post(self, request): return Response(status=status.HTTP_200_OK) else: raise UnhandledStripeEventAPIError - - payment_intent = event.data.object - - event_source_system_identifier = payment_intent.metadata.get('source_system') - logger.info( - '[Stripe webhooks] event %s with amount %d and payment intent ID [%s], source: [%s].', - event.type, - payment_intent.amount, - payment_intent.id, - event_source_system_identifier, - ) - - if event_source_system_identifier != source_system_identifier: - logger.info( - '[Stripe webhooks] Skipping event %s with payment intent ID [%s], source: [%s].', - event.type, - payment_intent.id, - event_source_system_identifier, - ) - return Response(status=status.HTTP_200_OK) - - payment_processed_signal.send_robust( - sender=self.__class__, - edx_lms_user_id=payment_intent.metadata.edx_lms_user_id, - order_uuid=payment_intent.metadata.order_number, - payment_number=payment_intent.metadata.payment_number, - payment_state=payment_state, - reference_number=payment_intent.id, - amount_in_cents=payment_intent.amount, - currency=payment_intent.currency, - provider_response_body=payload, - ) - return Response(status=status.HTTP_200_OK) diff --git a/commerce_coordinator/settings/base.py b/commerce_coordinator/settings/base.py index 4efb3d31f..835b1cb4d 100644 --- a/commerce_coordinator/settings/base.py +++ b/commerce_coordinator/settings/base.py @@ -338,6 +338,9 @@ def root(*path_fragments): "commerce_coordinator.apps.iap.signals.revoke_line_mobile_order_signal": [ "commerce_coordinator.apps.commercetools.signals.revoke_line_mobile_order", ], + "commerce_coordinator.apps.stripe.signals.payment_succeeded_commercetools_signal": [ + "commerce_coordinator.apps.commercetools.signals.finalize_commercetools_stripe_payment", + ], } # Default timeouts for requests From 86ffb5b8924d70a8b97521a1dd5eea7c5513481d Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Wed, 12 Aug 2026 15:46:36 -0400 Subject: [PATCH 02/12] fix(EDUN-15452): heal PI metadata on existing orders and harden recovery Backfill Stripe order_id when an order already exists so recovery converges; narrow order-lookup exceptions to ValueError; add CT-secondary orphan discovery, quarantine field population, Stripe retries, and missing tests. Co-authored-by: Cursor --- .../apps/commercetools/clients.py | 2 +- ..._orphaned_stripe_commercetools_payments.py | 163 ++++++++++++++++-- .../commercetools/stripe_payment_finalize.py | 109 ++++++++---- .../apps/commercetools/tasks.py | 37 ++-- .../apps/commercetools/tests/test_clients.py | 28 ++- .../tests/test_recovery_command.py | 42 +++++ .../tests/test_stripe_payment_finalize.py | 84 ++++++++- .../apps/stripe/tests/test_views.py | 24 +++ 8 files changed, 425 insertions(+), 64 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/clients.py b/commerce_coordinator/apps/commercetools/clients.py index cc4528a5a..3ac5110b4 100644 --- a/commerce_coordinator/apps/commercetools/clients.py +++ b/commerce_coordinator/apps/commercetools/clients.py @@ -1875,7 +1875,7 @@ def get_order_by_payment_id(self, payment_id: str) -> Order: ) if not response or not response.results: - raise Exception(f"No order found for payment ID {payment_id}") + raise ValueError(f"No order found for payment ID {payment_id}") return response.results[0] diff --git a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py index 8afb3f182..da8c427d2 100644 --- a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py +++ b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py @@ -1,19 +1,25 @@ """ -Management command to discover and finalize orphaned Stripe PaymentIntents -that have source_system=commercetools, status=succeeded, but no order_id -in their metadata. +Management command to discover and finalize orphaned Stripe PaymentIntents / +CommerceTools Payments that have succeeded without a linked Order. + +Discovery: + 1. Stripe primary — succeeded PIs with source_system=commercetools and no order_id + 2. CT secondary — stripe_edx payments with a Success Charge and no Order Intended to run on an external cron (e.g. every 15-30 minutes). """ import datetime import logging -import time import stripe from commercetools import CommercetoolsError +from commercetools.platform.models import TransactionState, TransactionType from django.conf import settings +from commerce_coordinator.apps.commercetools.catalog_info.constants import ( + EDX_STRIPE_PAYMENT_INTERFACE_NAME, +) from commerce_coordinator.apps.commercetools.management.commands._ct_api_client_command import ( CommercetoolsAPIClientCommand, ) @@ -30,8 +36,9 @@ class Command(CommercetoolsAPIClientCommand): help = ( - "Discover orphaned Stripe PaymentIntents (succeeded, source_system=commercetools, " - "no order_id) and finalize them into CT orders. Supports --since, --limit, --dry-run." + "Discover orphaned Stripe PaymentIntents / CT Payments (succeeded, " + "source_system=commercetools / stripe_edx, no Order) and finalize them. " + "Supports --since, --limit, --dry-run." ) def add_arguments(self, parser): @@ -62,13 +69,26 @@ def handle(self, *args, **options): created_after = int( (datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta(days=since_days)).timestamp() ) + created_after_iso = datetime.datetime.fromtimestamp( + created_after, tz=datetime.timezone.utc + ).strftime("%Y-%m-%dT%H:%M:%S.000Z") self.stdout.write( f"Recovery: since={since_days}d limit={limit} dry_run={dry_run}" ) - orphans = self._discover_stripe_orphans(created_after, limit) - self.stdout.write(f"Discovered {len(orphans)} Stripe orphan candidate(s)") + stripe_orphans = self._discover_stripe_orphans(created_after, limit) + self.stdout.write(f"Discovered {len(stripe_orphans)} Stripe orphan candidate(s)") + + remaining = max(0, limit - len(stripe_orphans)) + ct_orphans = [] + if remaining > 0: + ct_orphans = self._discover_ct_orphans( + created_after_iso, remaining, set(stripe_orphans), + ) + self.stdout.write(f"Discovered {len(ct_orphans)} CT-secondary orphan candidate(s)") + + orphans = stripe_orphans + ct_orphans if dry_run: for pi_id in orphans: @@ -79,6 +99,7 @@ def handle(self, *args, **options): quarantined = 0 for pi_id in orphans: + meta = self._pi_metadata(pi_id) try: result = finalize_ct_order_from_stripe_pi( pi_id, source="recovery", client=self.ct_api_client, @@ -96,8 +117,12 @@ def handle(self, *args, **options): self.stderr.write(f" [quarantine] {pi_id}: {exc}") _log_quarantine( pi_id=pi_id, - ct_payment_id="unknown", - ct_cart_id="unknown", + ct_payment_id=getattr(exc, "ct_payment_id", None) + or meta.get("ct_payment_id") + or "unknown", + ct_cart_id=getattr(exc, "ct_cart_id", None) + or meta.get("ct_cart_id") + or "unknown", reason=str(exc), source="recovery", ) @@ -106,8 +131,8 @@ def handle(self, *args, **options): self.stderr.write(f" [quarantine] {pi_id}: {exc}") _log_quarantine( pi_id=pi_id, - ct_payment_id="unknown", - ct_cart_id="unknown", + ct_payment_id=meta.get("ct_payment_id") or "unknown", + ct_cart_id=meta.get("ct_cart_id") or "unknown", reason=str(exc), source="recovery", ) @@ -118,6 +143,13 @@ def handle(self, *args, **options): f"{len(orphans) - finalized - quarantined} skipped" ) + def _pi_metadata(self, pi_id: str) -> dict: + try: + pi = stripe.PaymentIntent.retrieve(pi_id) + return dict(pi.metadata or {}) + except Exception: + return {} + def _discover_stripe_orphans(self, created_after: int, limit: int) -> list[str]: """ Query Stripe for PaymentIntents that are succeeded with @@ -202,3 +234,110 @@ def _list_filter_stripe_orphans(self, created_after: int, limit: int) -> list[st break return orphan_ids + + def _discover_ct_orphans( + self, + created_after_iso: str, + limit: int, + already_found: set[str], + ) -> list[str]: + """ + CT secondary discovery: stripe_edx payments with a Success Charge and + no linked Order. Returns Stripe PaymentIntent IDs (payment.interface_id). + """ + orphan_ids = [] + offset = 0 + page_size = 50 + + while len(orphan_ids) < limit: + try: + result = self.ct_api_client.base_client.payments.query( + where=[ + f'paymentMethodInfo(paymentInterface="{EDX_STRIPE_PAYMENT_INTERFACE_NAME}")', + f'createdAt > "{created_after_iso}"', + ], + sort=["createdAt desc"], + limit=page_size, + offset=offset, + ) + except CommercetoolsError: + logger.warning( + "[recovery] CT payment query failed during secondary discovery", + exc_info=True, + ) + break + + if not result.results: + break + + for payment in result.results: + if len(orphan_ids) >= limit: + break + + pi_id = payment.interface_id + if not pi_id or pi_id in already_found or pi_id in orphan_ids: + continue + + if not self._payment_has_success_charge(payment): + continue + + try: + self.ct_api_client.get_order_by_payment_id(payment.id) + continue # order exists + except ValueError: + pass # no order — candidate + except CommercetoolsError: + logger.warning( + "[recovery] CT order lookup failed for payment %s", + payment.id, + exc_info=True, + ) + continue + + if not self._stripe_pi_still_orphan(pi_id): + continue + + orphan_ids.append(pi_id) + + if len(result.results) < page_size: + break + offset += page_size + + if len(orphan_ids) >= limit: + logger.info( + "[recovery] CT secondary discovery truncated at limit=%d", + limit, + ) + + return orphan_ids + + @staticmethod + def _payment_has_success_charge(payment) -> bool: + if not payment.transactions: + return False + return any( + t.type == TransactionType.CHARGE and t.state == TransactionState.SUCCESS + for t in payment.transactions + ) + + @staticmethod + def _stripe_pi_still_orphan(pi_id: str) -> bool: + """Confirm Stripe PI is succeeded commercetools and still missing order_id.""" + try: + pi = stripe.PaymentIntent.retrieve(pi_id) + except Exception: + logger.warning( + "[recovery] Failed to retrieve Stripe PI %s during CT secondary check", + pi_id, + exc_info=True, + ) + return False + + if pi.status != "succeeded": + return False + + metadata = pi.metadata or {} + if metadata.get("source_system") != "commercetools": + return False + + return not metadata.get("order_id") diff --git a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py index 8f0549582..64819bcdf 100644 --- a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py @@ -28,6 +28,17 @@ class FinalizeError(Exception): """Non-retryable finalization error (quarantine candidate).""" + def __init__( + self, + message: str, + *, + ct_payment_id: str = "unknown", + ct_cart_id: str = "unknown", + ): + super().__init__(message) + self.ct_payment_id = ct_payment_id or "unknown" + self.ct_cart_id = ct_cart_id or "unknown" + @dataclass class FinalizeResult: @@ -47,6 +58,41 @@ def _payment_has_charge_for(payment, charge_id: str) -> bool: ) +def _backfill_pi_metadata(payment_intent_id: str, order_id: str, payment_id: str) -> None: + """Write order_id / ct_payment_id onto the Stripe PaymentIntent (idempotent).""" + try: + stripe.PaymentIntent.modify( + payment_intent_id, + metadata={ + "order_id": order_id, + "ct_payment_id": payment_id, + }, + ) + except Exception: + logger.warning( + "[finalize_ct_order] Failed to backfill PI metadata for %s", + payment_intent_id, + exc_info=True, + ) + + +def _discount_amount_dollars(cart) -> float: + """Extract cart-level discount as dollars from CT cart shapes.""" + discount_on_total = getattr(cart, "discount_on_total_price", None) + if not discount_on_total: + return 0 + + discounted_amount = getattr(discount_on_total, "discounted_amount", None) + if discounted_amount is not None: + return cents_to_dollars(discounted_amount) + + # Fallback if a money-like object was passed directly (tests / older shapes) + if hasattr(discount_on_total, "cent_amount"): + return cents_to_dollars(discount_on_total) + + return 0 + + def finalize_ct_order_from_stripe_pi( payment_intent_id: str, *, @@ -62,7 +108,7 @@ def finalize_ct_order_from_stripe_pi( 2. Resolve CT Payment (by key = pi.id or metadata.ct_payment_id) 3. Resolve CT Cart (by metadata.ct_cart_id) 4. Add Charge transaction if absent (idempotent by interaction_id) - 5. Skip if order already exists for this payment + 5. Skip if order already exists for this payment (still heal PI metadata) 6. Create order from cart → COMPLETE / PAID / SHIPPED 7. Transition line items → PENDING_FULFILMENT 8. Emit Segment Order Completed (plan 18, is_mobile=False) @@ -84,23 +130,28 @@ def finalize_ct_order_from_stripe_pi( client = CommercetoolsAPIClient() pi = stripe.PaymentIntent.retrieve(payment_intent_id) + metadata = pi.metadata or {} if pi.status != "succeeded": raise FinalizeError( - f"PaymentIntent {payment_intent_id} status is '{pi.status}', expected 'succeeded'" + f"PaymentIntent {payment_intent_id} status is '{pi.status}', expected 'succeeded'", + ct_cart_id=metadata.get("ct_cart_id", "unknown"), + ct_payment_id=metadata.get("ct_payment_id", "unknown"), ) - metadata = pi.metadata or {} if metadata.get("source_system") != "commercetools": raise FinalizeError( f"PaymentIntent {payment_intent_id} source_system is " - f"'{metadata.get('source_system')}', expected 'commercetools'" + f"'{metadata.get('source_system')}', expected 'commercetools'", + ct_cart_id=metadata.get("ct_cart_id", "unknown"), + ct_payment_id=metadata.get("ct_payment_id", "unknown"), ) ct_cart_id = metadata.get("ct_cart_id") if not ct_cart_id: raise FinalizeError( - f"PaymentIntent {payment_intent_id} missing metadata.ct_cart_id" + f"PaymentIntent {payment_intent_id} missing metadata.ct_cart_id", + ct_payment_id=metadata.get("ct_payment_id", "unknown"), ) ct_payment_id_from_meta = metadata.get("ct_payment_id") @@ -141,20 +192,27 @@ def finalize_ct_order_from_stripe_pi( ) # --- Check if order already exists --- + # ValueError = not found (aligned with client docstring). CommercetoolsError must + # propagate so Celery can retry instead of creating a duplicate order. try: existing_order = client.get_order_by_payment_id(payment.id) + except ValueError: + existing_order = None + + if existing_order is not None: logger.info( - "[finalize_ct_order] Order %s already exists for payment %s (pi=%s), skipping creation", + "[finalize_ct_order] Order %s already exists for payment %s (pi=%s), " + "skipping creation; ensuring PI metadata is backfilled", existing_order.id, payment.id, payment_intent_id, ) + if not metadata.get("order_id") or metadata.get("ct_payment_id") != payment.id: + _backfill_pi_metadata(payment_intent_id, existing_order.id, payment.id) return FinalizeResult( order_id=existing_order.id, order_number=existing_order.order_number or "", payment_id=payment.id, already_existed=True, ) - except Exception: - pass # --- Load cart and create order --- cart = client.get_cart_by_id(ct_cart_id) @@ -174,19 +232,7 @@ def finalize_ct_order_from_stripe_pi( _emit_web_order_completed(client, order, cart, payment) # --- Backfill PI metadata --- - try: - stripe.PaymentIntent.modify( - payment_intent_id, - metadata={ - "order_id": order.id, - "ct_payment_id": payment.id, - }, - ) - except Exception: - logger.warning( - "[finalize_ct_order] Failed to backfill PI metadata for %s", - payment_intent_id, exc_info=True, - ) + _backfill_pi_metadata(payment_intent_id, order.id, payment.id) logger.info( "[finalize_ct_order] Successfully finalized order %s for pi=%s source=%s", @@ -213,11 +259,8 @@ def _emit_web_order_completed(client, order, cart, payment): ] payment_method = "unknown" - processor_name = "stripe" - if payment.payment_method_info: - payment_method = payment.payment_method_info.method or "unknown" - if payment.payment_method_info.name: - processor_name = payment.payment_method_info.name.get("en", "stripe") + if payment.payment_method_info and payment.payment_method_info.method: + payment_method = payment.payment_method_info.method discount_codes = getattr(cart, "discount_codes", []) or [] discount_code = None @@ -225,10 +268,14 @@ def _emit_web_order_completed(client, order, cart, payment): if discount_codes: codes_as_dicts = [] for dc in discount_codes: - if hasattr(dc, "code"): + code_obj = getattr(dc, "discount_code", None) + if code_obj is not None and hasattr(code_obj, "obj") and code_obj.obj: + codes_as_dicts.append({"code": getattr(code_obj.obj, "code", None)}) + elif hasattr(dc, "code"): codes_as_dicts.append({"code": dc.code}) elif isinstance(dc, dict) and "code" in dc: codes_as_dicts.append(dc) + codes_as_dicts = [d for d in codes_as_dicts if d.get("code")] if codes_as_dicts: discount_code = codes_as_dicts[-1].get("code") coupon_name = [ @@ -240,10 +287,6 @@ def _emit_web_order_completed(client, order, cart, payment): if order.taxed_price and order.taxed_price.total_tax: taxed_amount = cents_to_dollars(order.taxed_price.total_tax) - discount_amount = 0 - if cart.discount_on_total_price: - discount_amount = cents_to_dollars(cart.discount_on_total_price) - event_props = { "track_plan_id": 18, "trigger_source": "server-side", @@ -254,9 +297,9 @@ def _emit_web_order_completed(client, order, cart, payment): "tax": taxed_amount, "coupon": discount_code, "coupon_name": coupon_name, - "discount": discount_amount, + "discount": _discount_amount_dollars(cart), "payment_method": payment_method, - "processor_name": processor_name, + "processor_name": "stripe", "products": products, "is_mobile": False, "multi_item_cart_enabled": len(cart.line_items) > 1, diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index 2d7ef7086..e2a644b47 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -576,7 +576,7 @@ def _log_quarantine(*, pi_id, ct_payment_id, ct_cart_id, reason, source): @shared_task( bind=True, - autoretry_for=(CommercetoolsError,), + autoretry_for=(CommercetoolsError, stripe.error.StripeError), retry_kwargs={"max_retries": 5, "countdown": 3}, ) def finalize_commercetools_stripe_payment_task( @@ -588,8 +588,8 @@ def finalize_commercetools_stripe_payment_task( Celery task wrapping the shared finalize path for a Stripe PaymentIntent that originated from a CommerceTools cart. - Bounded retries on transient CT errors; non-retryable failures - are quarantined via structured log + metric. + Bounded retries on transient CT/Stripe errors; non-retryable failures + are quarantined via structured log. """ from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( FinalizeError, @@ -617,30 +617,45 @@ def finalize_commercetools_stripe_payment_task( except FinalizeError as exc: _log_quarantine( pi_id=payment_intent_id, - ct_payment_id="unknown", - ct_cart_id="unknown", + ct_payment_id=getattr(exc, "ct_payment_id", "unknown"), + ct_cart_id=getattr(exc, "ct_cart_id", "unknown"), reason=str(exc), source=source, ) return None - except CommercetoolsError: + except (CommercetoolsError, stripe.error.StripeError): if self.request.retries >= self.max_retries: + ct_payment_id, ct_cart_id = _quarantine_ids_from_pi(payment_intent_id) _log_quarantine( pi_id=payment_intent_id, - ct_payment_id="unknown", - ct_cart_id="unknown", - reason="max retries exhausted on CommercetoolsError", + ct_payment_id=ct_payment_id, + ct_cart_id=ct_cart_id, + reason="max retries exhausted on transient error", source=source, ) raise except Exception as exc: + ct_payment_id, ct_cart_id = _quarantine_ids_from_pi(payment_intent_id) _log_quarantine( pi_id=payment_intent_id, - ct_payment_id="unknown", - ct_cart_id="unknown", + ct_payment_id=ct_payment_id, + ct_cart_id=ct_cart_id, reason=f"unexpected error: {exc}", source=source, ) raise + + +def _quarantine_ids_from_pi(payment_intent_id: str) -> tuple[str, str]: + """Best-effort ct_payment_id / ct_cart_id from Stripe PI metadata for quarantine logs.""" + try: + pi = stripe.PaymentIntent.retrieve(payment_intent_id) + metadata = pi.metadata or {} + return ( + metadata.get("ct_payment_id") or "unknown", + metadata.get("ct_cart_id") or "unknown", + ) + except Exception: + return "unknown", "unknown" diff --git a/commerce_coordinator/apps/commercetools/tests/test_clients.py b/commerce_coordinator/apps/commercetools/tests/test_clients.py index 0c2889059..eb9c8b150 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_clients.py +++ b/commerce_coordinator/apps/commercetools/tests/test_clients.py @@ -1,6 +1,6 @@ """ Commercetools API Client(s) Testing """ -from datetime import datetime +from datetime import datetime, timezone from unittest.mock import MagicMock, Mock import pytest @@ -1845,12 +1845,36 @@ def test_get_order_by_payment_id_no_order_found(self): status_code=200 ) - with self.assertRaises(Exception) as exc: + with self.assertRaises(ValueError) as exc: self.client_set.client.get_order_by_payment_id(payment_id) # Verify the exception message self.assertEqual(str(exc.exception), f"No order found for payment ID {payment_id}") + def test_create_charge_payment_transaction(self): + """Add a Charge transaction to an existing CT payment.""" + base_url = self.client_set.get_base_url_from_client() + mock_response_payment = gen_payment() + charge_created = datetime.fromtimestamp(1692942318, tz=timezone.utc) + + with requests_mock.Mocker(real_http=True, case_sensitive=False) as mocker: + mocker.post( + f"{base_url}payments/{mock_response_payment.id}", + json=mock_response_payment.serialize(), + status_code=200 + ) + + result = self.client_set.client.create_charge_payment_transaction( + payment_id=mock_response_payment.id, + payment_version=mock_response_payment.version, + charge_id="ch_3P9RWsH4caH7G0X11toRGUJf", + amount_in_cents=4900, + currency_code="usd", + charge_created=charge_created, + ) + + self.assertEqual(result.id, mock_response_payment.id) + def test_get_credit_variant_by_course_run(self): base_url = self.client_set.get_base_url_from_client() course_run_key = "course-v1:edX+DemoX+2025_T1" diff --git a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py index 10abbd748..327a68201 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py +++ b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py @@ -5,6 +5,7 @@ from io import StringIO from unittest.mock import MagicMock, patch +from commercetools.platform.models import TransactionState, TransactionType from django.test import TestCase from commerce_coordinator.apps.commercetools.stripe_payment_finalize import FinalizeError, FinalizeResult @@ -99,6 +100,9 @@ def test_finalize_error_quarantines(self, mock_finalize, mock_quarantine, _mock_ err_output = cmd.stderr.getvalue() self.assertIn("[quarantine] pi_bad", err_output) mock_quarantine.assert_called_once() + quarantine_kwargs = mock_quarantine.call_args.kwargs + self.assertEqual(quarantine_kwargs["pi_id"], "pi_bad") + self.assertEqual(quarantine_kwargs["source"], "recovery") self.assertIn("1 quarantined", cmd.stdout.getvalue()) @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") @@ -167,3 +171,41 @@ def test_search_fallback_to_list(self, _mock_init): output = cmd.stdout.getvalue() self.assertIn("[dry-run] orphan: pi_list_orphan", output) + + @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") + def test_ct_secondary_discovery(self, mock_finalize, _mock_init): + """CT payments with Success Charge and no Order become orphan candidates.""" + cmd = self._make_command() + + payment = MagicMock() + payment.id = "pay-ct-1" + payment.interface_id = "pi_ct_secondary" + charge_txn = MagicMock() + charge_txn.type = TransactionType.CHARGE + charge_txn.state = TransactionState.SUCCESS + payment.transactions = [charge_txn] + + query_result = MagicMock() + query_result.results = [payment] + cmd.ct_api_client.base_client.payments.query.return_value = query_result + cmd.ct_api_client.get_order_by_payment_id.side_effect = ValueError("no order") + + stripe_pi = MagicMock() + stripe_pi.id = "pi_ct_secondary" + stripe_pi.status = "succeeded" + stripe_pi.metadata = {"source_system": "commercetools"} + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + # No Stripe-primary orphans + search_result = MagicMock() + search_result.data = [] + search_result.has_more = False + mock_stripe.PaymentIntent.search.return_value = search_result + mock_stripe.PaymentIntent.retrieve.return_value = stripe_pi + + cmd.handle(since=7, limit=100, dry_run=True) + + output = cmd.stdout.getvalue() + self.assertIn("1 CT-secondary orphan candidate(s)", output) + self.assertIn("[dry-run] orphan: pi_ct_secondary", output) + mock_finalize.assert_not_called() diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py index 431bf706f..07f91364b 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py @@ -126,7 +126,7 @@ def test_happy_path(self, MockClient, mock_track, mock_stripe): client = MockClient.return_value client.get_payment_by_key.return_value = payment client.create_charge_payment_transaction.return_value = payment - client.get_order_by_payment_id.side_effect = Exception("not found") + client.get_order_by_payment_id.side_effect = ValueError("not found") client.get_cart_by_id.return_value = cart client.create_order_from_cart.return_value = order client.update_line_items_transition_state.return_value = order @@ -140,6 +140,77 @@ def test_happy_path(self, MockClient, mock_track, mock_stripe): client.create_order_from_cart.assert_called_once_with(cart) client.update_line_items_transition_state.assert_called_once() mock_track.assert_called_once() + mock_stripe.PaymentIntent.modify.assert_called_once() + + def test_order_already_exists_backfills_missing_pi_metadata(self, MockClient, mock_track, mock_stripe): + """Existing order + missing PI order_id still heals metadata (recovery convergence).""" + pi = _mock_pi() + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-123", has_charge=True, charge_id="ch_test456") + existing_order = gen_order(uuid4_str()) + + client = MockClient.return_value + client.get_payment_by_key.return_value = payment + client.get_order_by_payment_id.return_value = existing_order + + result = finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") + + self.assertTrue(result.already_existed) + self.assertEqual(result.order_id, existing_order.id) + client.create_order_from_cart.assert_not_called() + mock_track.assert_not_called() + mock_stripe.PaymentIntent.modify.assert_called_once_with( + "pi_test123", + metadata={ + "order_id": existing_order.id, + "ct_payment_id": "pay-123", + }, + ) + + def test_order_already_exists_skips_when_metadata_complete(self, MockClient, mock_track, mock_stripe): + """When order exists and PI already has order_id, do not modify again.""" + existing_order = gen_order(uuid4_str()) + pi = _mock_pi(order_id=existing_order.id, ct_payment_id="pay-123") + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-123", has_charge=True, charge_id="ch_test456") + + client = MockClient.return_value + client.base_client.payments.get_by_id.return_value = payment + client.get_order_by_payment_id.return_value = existing_order + + result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + self.assertTrue(result.already_existed) + mock_stripe.PaymentIntent.modify.assert_not_called() + + def test_ct_outage_on_order_lookup_propagates(self, MockClient, mock_track, mock_stripe): + """CommercetoolsError during order lookup must not be treated as not-found.""" + pi = _mock_pi() + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-123", has_charge=True, charge_id="ch_test456") + + client = MockClient.return_value + client.get_payment_by_key.return_value = payment + client.get_order_by_payment_id.side_effect = CommercetoolsError( + message="boom", + errors=[{"code": "ConcurrentModification", "message": "boom"}], + response={}, + correlation_id="corr", + ) + + with self.assertRaises(CommercetoolsError): + finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + client.create_order_from_cart.assert_not_called() def test_order_already_exists_skips(self, MockClient, mock_track, mock_stripe): """When order already exists for the payment, skip creation.""" @@ -176,7 +247,7 @@ def test_charge_already_present_skips_creation(self, MockClient, mock_track, moc client = MockClient.return_value client.get_payment_by_key.return_value = payment - client.get_order_by_payment_id.side_effect = Exception("not found") + client.get_order_by_payment_id.side_effect = ValueError("not found") client.get_cart_by_id.return_value = cart client.create_order_from_cart.return_value = order client.update_line_items_transition_state.return_value = order @@ -208,9 +279,11 @@ def test_missing_ct_cart_id_raises_finalize_error(self, MockClient, mock_track, pi.metadata.pop("ct_cart_id", None) mock_stripe.PaymentIntent.retrieve.return_value = pi - with self.assertRaises(FinalizeError): + with self.assertRaises(FinalizeError) as ctx: finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + self.assertEqual(ctx.exception.ct_cart_id, "unknown") + def test_resolves_payment_by_ct_payment_id(self, MockClient, mock_track, mock_stripe): """When ct_payment_id is in metadata, use it for lookup.""" pi = _mock_pi(ct_payment_id="pay-from-meta") @@ -231,7 +304,7 @@ def test_resolves_payment_by_ct_payment_id(self, MockClient, mock_track, mock_st self.assertTrue(result.already_existed) def test_segment_event_has_web_properties(self, MockClient, mock_track, mock_stripe): - """Segment Order Completed should have is_mobile=False and plan 18.""" + """Segment Order Completed should have is_mobile=False, plan 18, payment_method=upi.""" pi = _mock_pi() charge = _mock_charge() mock_stripe.PaymentIntent.retrieve.return_value = pi @@ -245,7 +318,7 @@ def test_segment_event_has_web_properties(self, MockClient, mock_track, mock_str client = MockClient.return_value client.get_payment_by_key.return_value = payment client.create_charge_payment_transaction.return_value = payment - client.get_order_by_payment_id.side_effect = Exception("not found") + client.get_order_by_payment_id.side_effect = ValueError("not found") client.get_cart_by_id.return_value = cart client.create_order_from_cart.return_value = order client.update_line_items_transition_state.return_value = order @@ -259,3 +332,4 @@ def test_segment_event_has_web_properties(self, MockClient, mock_track, mock_str self.assertEqual(props["track_plan_id"], 18) self.assertEqual(props["trigger_source"], "server-side") self.assertEqual(props["processor_name"], "stripe") + self.assertEqual(props["payment_method"], "upi") diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index 7750643e9..a98acc2e0 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -105,6 +105,30 @@ def test_ct_payment_succeeded_fires_signal(self, mock_ct_signal, mock_construct_ payment_intent_id=pi_id, ) + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') + @mock.patch.object(WebhookView, '_is_running', return_value=True) + def test_ct_payment_succeeded_single_invocation_short_circuits( + self, mock_is_running, mock_ct_signal, mock_construct_event + ): + """Duplicate CT success delivery short-circuits via SingleInvocation.""" + pi_id = 'pi_ct_dup' + self.mock_stripe_event.type = StripeEventType.PAYMENT_SUCCESS.value + metadata = {'source_system': 'commercetools', 'ct_cart_id': 'cart-uuid'} + self.mock_stripe_event.data.object.id = pi_id + self.mock_stripe_event.data.object.metadata = StripeObject() + self.mock_stripe_event.data.object.metadata.update(metadata) + self.mock_stripe_event.data.object.amount = 4900 + mock_construct_event.return_value = self.mock_stripe_event + + response = self.client.post( + self.url, data={}, format='json', **self.mock_header + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_is_running.assert_called() + mock_ct_signal.assert_not_called() + @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') def test_ct_payment_failed_returns_200_no_signal(self, mock_ct_signal, mock_construct_event): From b0cdfed9dc82f01bb16d256e7b4fde723da4eaca Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Thu, 13 Aug 2026 10:44:00 -0400 Subject: [PATCH 03/12] fix(EDUN-15452): satisfy pylint and isort quality gates Co-authored-by: Cursor --- ..._orphaned_stripe_commercetools_payments.py | 8 +- .../commercetools/stripe_payment_finalize.py | 6 +- .../apps/commercetools/tasks.py | 8 +- .../commercetools/tests/test_finalize_task.py | 9 +- .../tests/test_recovery_command.py | 7 +- .../tests/test_stripe_payment_finalize.py | 25 ++- commerce_coordinator/apps/stripe/views.py | 201 +++++++++--------- 7 files changed, 135 insertions(+), 129 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py index da8c427d2..c70316f6e 100644 --- a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py +++ b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py @@ -17,15 +17,13 @@ from commercetools.platform.models import TransactionState, TransactionType from django.conf import settings -from commerce_coordinator.apps.commercetools.catalog_info.constants import ( - EDX_STRIPE_PAYMENT_INTERFACE_NAME, -) +from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME from commerce_coordinator.apps.commercetools.management.commands._ct_api_client_command import ( - CommercetoolsAPIClientCommand, + CommercetoolsAPIClientCommand ) from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( FinalizeError, - finalize_ct_order_from_stripe_pi, + finalize_ct_order_from_stripe_pi ) from commerce_coordinator.apps.commercetools.tasks import _log_quarantine diff --git a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py index 64819bcdf..ab8c926e5 100644 --- a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py @@ -17,7 +17,7 @@ from commerce_coordinator.apps.commercetools.catalog_info.edx_utils import ( cents_to_dollars, get_edx_lms_user_id, - get_product_from_line_item, + get_product_from_line_item ) from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient from commerce_coordinator.apps.core.segment import track @@ -68,7 +68,7 @@ def _backfill_pi_metadata(payment_intent_id: str, order_id: str, payment_id: str "ct_payment_id": payment_id, }, ) - except Exception: + except Exception: # pylint: disable=broad-exception-caught logger.warning( "[finalize_ct_order] Failed to backfill PI metadata for %s", payment_intent_id, @@ -314,7 +314,7 @@ def _emit_web_order_completed(client, order, cart, payment): "[finalize_ct_order] Emitted Segment Order Completed for order %s, user %s", order.id, lms_user_id, ) - except Exception: + except Exception: # pylint: disable=broad-exception-caught logger.warning( "[finalize_ct_order] Failed to emit Segment Order Completed for order %s", order.id, exc_info=True, diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index e2a644b47..18684328b 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -36,6 +36,7 @@ from commerce_coordinator.apps.order_fulfillment.serializers import OrderRevokeLineRequestSerializer from .clients import CommercetoolsAPIClient, Refund +from .stripe_payment_finalize import FinalizeError, finalize_ct_order_from_stripe_pi from .utils import ( convert_ct_cent_amount_to_localized_price, get_lob_from_variant_attr, @@ -591,11 +592,6 @@ def finalize_commercetools_stripe_payment_task( Bounded retries on transient CT/Stripe errors; non-retryable failures are quarantined via structured log. """ - from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( - FinalizeError, - finalize_ct_order_from_stripe_pi, - ) - tag = "finalize_commercetools_stripe_payment_task" try: @@ -657,5 +653,5 @@ def _quarantine_ids_from_pi(payment_intent_id: str) -> tuple[str, str]: metadata.get("ct_payment_id") or "unknown", metadata.get("ct_cart_id") or "unknown", ) - except Exception: + except Exception: # pylint: disable=broad-exception-caught return "unknown", "unknown" diff --git a/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py b/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py index a7ead3542..25602d1e3 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py +++ b/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py @@ -1,22 +1,21 @@ """ Tests for the finalize_commercetools_stripe_payment_task Celery task. """ +# Celery's bind=True self argument is supplied by the task decorator. +# pylint: disable=no-value-for-parameter from unittest.mock import patch -from commercetools import CommercetoolsError from django.test import TestCase from commerce_coordinator.apps.commercetools.stripe_payment_finalize import FinalizeError, FinalizeResult from commerce_coordinator.apps.commercetools.tasks import finalize_commercetools_stripe_payment_task -FINALIZE_PATH = ( - "commerce_coordinator.apps.commercetools.stripe_payment_finalize" - ".finalize_ct_order_from_stripe_pi" -) +FINALIZE_PATH = "commerce_coordinator.apps.commercetools.tasks.finalize_ct_order_from_stripe_pi" class TestFinalizeTask(TestCase): + """Tests for the Celery task wrapping the shared Stripe/CT finalize path.""" @patch(FINALIZE_PATH) def test_happy_path_returns_order_id(self, mock_finalize): diff --git a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py index 327a68201..86aeb6b32 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py +++ b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py @@ -8,6 +8,9 @@ from commercetools.platform.models import TransactionState, TransactionType from django.test import TestCase +from commerce_coordinator.apps.commercetools.management.commands.recover_orphaned_stripe_commercetools_payments import ( + Command +) from commerce_coordinator.apps.commercetools.stripe_payment_finalize import FinalizeError, FinalizeResult CMD_MODULE = ( @@ -18,10 +21,10 @@ @patch(f"{CMD_MODULE}.CommercetoolsAPIClientCommand.__init__", return_value=None) class TestRecoveryCommand(TestCase): + """Tests for the orphaned Stripe/CT payment recovery management command.""" def _make_command(self): - from commerce_coordinator.apps.commercetools.management.commands.recover_orphaned_stripe_commercetools_payments import Command # noqa: E501 - + """Build a command instance with a mocked CT client and captured output streams.""" cmd = Command() cmd.ct_api_client = MagicMock() cmd.stdout = StringIO() diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py index 07f91364b..395c555b3 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py @@ -1,37 +1,30 @@ """ Tests for the shared CT order finalization from Stripe PaymentIntents. """ +# Class-level patch decorators inject every mock into each test method. +# pylint: disable=unused-argument import datetime -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import MagicMock, patch from commercetools import CommercetoolsError from commercetools.platform.models import ( CentPrecisionMoney, - CustomFields, - FieldContainer, - Order, Payment, PaymentMethodInfo, PaymentState, Transaction, TransactionState, - TransactionType, - TypeReference, + TransactionType ) from django.test import TestCase from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( FinalizeError, - FinalizeResult, _payment_has_charge_for, - finalize_ct_order_from_stripe_pi, -) -from commerce_coordinator.apps.commercetools.tests.conftest import ( - gen_cart, - gen_customer, - gen_order, + finalize_ct_order_from_stripe_pi ) +from commerce_coordinator.apps.commercetools.tests.conftest import gen_cart, gen_customer, gen_order from commerce_coordinator.apps.core.tests.utils import uuid4_str @@ -44,6 +37,7 @@ def _mock_pi( order_id=None, latest_charge="ch_test456", ): + """Build a Stripe PaymentIntent stub with CT-linking metadata.""" pi = MagicMock() pi.id = pi_id pi.status = pi_status @@ -60,6 +54,7 @@ def _mock_pi( def _mock_charge(charge_id="ch_test456", amount=4900, currency="usd"): + """Build a Stripe Charge stub for the PaymentIntent's latest charge.""" charge = MagicMock() charge.id = charge_id charge.amount = amount @@ -69,6 +64,7 @@ def _mock_charge(charge_id="ch_test456", amount=4900, currency="usd"): def _mock_payment(payment_id=None, version=1, has_charge=False, charge_id="ch_test456"): + """Build a CT Payment, optionally already carrying a successful Charge transaction.""" txns = [] if has_charge: txns.append(Transaction( @@ -93,6 +89,8 @@ def _mock_payment(payment_id=None, version=1, has_charge=False, charge_id="ch_te class TestPaymentHasChargeFor(TestCase): + """Tests for Charge transaction idempotency detection on a CT payment.""" + def test_no_transactions(self): payment = _mock_payment() self.assertFalse(_payment_has_charge_for(payment, "ch_test")) @@ -110,6 +108,7 @@ def test_has_different_charge(self): @patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.track") @patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.CommercetoolsAPIClient") class TestFinalizeCTOrderFromStripePI(TestCase): + """Tests for finalizing a CT order from a Stripe PaymentIntent.""" def test_happy_path(self, MockClient, mock_track, mock_stripe): """Full finalize: charge + order + line state + segment + PI metadata.""" diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index c367d1e33..7032f1a1c 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -22,7 +22,7 @@ from commerce_coordinator.apps.stripe.signals import ( payment_processed_signal, payment_refunded_signal, - payment_succeeded_commercetools_signal, + payment_succeeded_commercetools_signal ) logger = logging.getLogger(__name__) @@ -68,109 +68,120 @@ def post(self, request): payment_intent = event.data.object event_source_system = payment_intent.metadata.get('source_system') - # --- CommerceTools path (UPI / CT-originated PIs) --- if event_source_system == 'commercetools': - if event.type == StripeEventType.PAYMENT_SUCCESS: - if self._is_running(tag, payment_intent.id): # pragma no cover - self.meta_should_mark_not_running = False - return Response(status=status.HTTP_200_OK) - else: - self.mark_running(tag, payment_intent.id) - - logger.info( - '[Stripe webhooks] CT payment_intent.succeeded for PI [%s]', - payment_intent.id, - ) - - payment_succeeded_commercetools_signal.send_robust( - sender=self.__class__, - payment_intent_id=payment_intent.id, - ) - else: - logger.info( - '[Stripe webhooks] CT payment_intent.payment_failed for PI [%s], ignoring', - payment_intent.id, - ) - return Response(status=status.HTTP_200_OK) - - # --- Legacy edX path --- - if event.type == StripeEventType.PAYMENT_SUCCESS: - payment_state = PaymentState.COMPLETED.value - else: - payment_state = PaymentState.FAILED.value + return self._handle_commercetools_payment_event(tag, event, payment_intent) + return self._handle_legacy_payment_event(event, payment_intent, event_source_system, payload) + + if event.type == StripeEventType.PAYMENT_REFUNDED: + return self._handle_refund_event(tag, event) + + raise UnhandledStripeEventAPIError + + def _handle_commercetools_payment_event(self, tag, event, payment_intent): + """Route CommerceTools-originated PaymentIntents (UPI) to the async finalize path.""" + if event.type != StripeEventType.PAYMENT_SUCCESS: logger.info( - '[Stripe webhooks] event %s with amount %d and payment intent ID [%s], source: [%s].', - event.type, - payment_intent.amount, + '[Stripe webhooks] CT payment_intent.payment_failed for PI [%s], ignoring', payment_intent.id, - event_source_system, ) + return Response(status=status.HTTP_200_OK) - if event_source_system != source_system_identifier: - logger.info( - '[Stripe webhooks] Skipping event %s with payment intent ID [%s], source: [%s].', - event.type, - payment_intent.id, - event_source_system, - ) - return Response(status=status.HTTP_200_OK) + if self._is_running(tag, payment_intent.id): # pragma no cover + self.meta_should_mark_not_running = False + return Response(status=status.HTTP_200_OK) - payment_processed_signal.send_robust( - sender=self.__class__, - edx_lms_user_id=payment_intent.metadata.edx_lms_user_id, - order_uuid=payment_intent.metadata.order_number, - payment_number=payment_intent.metadata.payment_number, - payment_state=payment_state, - reference_number=payment_intent.id, - amount_in_cents=payment_intent.amount, - currency=payment_intent.currency, - provider_response_body=payload, + self.mark_running(tag, payment_intent.id) + + logger.info( + '[Stripe webhooks] CT payment_intent.succeeded for PI [%s]', + payment_intent.id, + ) + + payment_succeeded_commercetools_signal.send_robust( + sender=self.__class__, + payment_intent_id=payment_intent.id, + ) + return Response(status=status.HTTP_200_OK) + + def _handle_legacy_payment_event(self, event, payment_intent, event_source_system, payload): + """Route legacy edX ecommerce PaymentIntents to the existing processed signal.""" + if event.type == StripeEventType.PAYMENT_SUCCESS: + payment_state = PaymentState.COMPLETED.value + else: + payment_state = PaymentState.FAILED.value + + logger.info( + '[Stripe webhooks] event %s with amount %d and payment intent ID [%s], source: [%s].', + event.type, + payment_intent.amount, + payment_intent.id, + event_source_system, + ) + + if event_source_system != source_system_identifier: + logger.info( + '[Stripe webhooks] Skipping event %s with payment intent ID [%s], source: [%s].', + event.type, + payment_intent.id, + event_source_system, ) return Response(status=status.HTTP_200_OK) - elif event.type == StripeEventType.PAYMENT_REFUNDED: - idempotency_key = event.get('request').get('idempotency_key') - if self._is_running(tag, idempotency_key): # pragma no cover - self.meta_should_mark_not_running = False - return Response(status=status.HTTP_200_OK) - else: - self.mark_running(tag, idempotency_key) - - event_object = event.data.object - order_number = event_object.metadata.order_number - is_legacy_order_check = is_legacy_order(order_number) - is_ct_order_check = is_commercetools_stripe_refund(event_object.metadata.get('source_system')) - payment_intent_id = event_object.payment_intent - - if not is_legacy_order_check and is_ct_order_check: - event_source_system_identifier = event_object.metadata.get('source_system') - refunds = event_object.refunds.data - latest_refund = max(refunds, key=lambda refund: refund['created']) - - logger.info( - '[Stripe webhooks] refund event %s with payment intent ID [%s] ' - 'and order number [%s], source: [%s].', - event.type, - payment_intent_id, - order_number, - event_source_system_identifier, - ) - - payment_refunded_signal.send_robust( - sender=self.__class__, - payment_intent_id=payment_intent_id, - stripe_refund=latest_refund, - order_number=order_number, - ) - else: - logger.info( - '[Stripe webhooks] skipping refund event %s with payment intent ID [%s] ' - 'and order number [%s], as it is not a Commercetools order.', - event.type, - payment_intent_id, - order_number, - ) + payment_processed_signal.send_robust( + sender=self.__class__, + edx_lms_user_id=payment_intent.metadata.edx_lms_user_id, + order_uuid=payment_intent.metadata.order_number, + payment_number=payment_intent.metadata.payment_number, + payment_state=payment_state, + reference_number=payment_intent.id, + amount_in_cents=payment_intent.amount, + currency=payment_intent.currency, + provider_response_body=payload, + ) + return Response(status=status.HTTP_200_OK) + + def _handle_refund_event(self, tag, event): + """Route Commercetools refunds to the refund signal, skipping legacy orders.""" + idempotency_key = event.get('request').get('idempotency_key') + if self._is_running(tag, idempotency_key): # pragma no cover + self.meta_should_mark_not_running = False return Response(status=status.HTTP_200_OK) + + self.mark_running(tag, idempotency_key) + + event_object = event.data.object + order_number = event_object.metadata.order_number + is_legacy_order_check = is_legacy_order(order_number) + is_ct_order_check = is_commercetools_stripe_refund(event_object.metadata.get('source_system')) + payment_intent_id = event_object.payment_intent + + if not is_legacy_order_check and is_ct_order_check: + event_source_system_identifier = event_object.metadata.get('source_system') + refunds = event_object.refunds.data + latest_refund = max(refunds, key=lambda refund: refund['created']) + + logger.info( + '[Stripe webhooks] refund event %s with payment intent ID [%s] ' + 'and order number [%s], source: [%s].', + event.type, + payment_intent_id, + order_number, + event_source_system_identifier, + ) + + payment_refunded_signal.send_robust( + sender=self.__class__, + payment_intent_id=payment_intent_id, + stripe_refund=latest_refund, + order_number=order_number, + ) else: - raise UnhandledStripeEventAPIError + logger.info( + '[Stripe webhooks] skipping refund event %s with payment intent ID [%s] ' + 'and order number [%s], as it is not a Commercetools order.', + event.type, + payment_intent_id, + order_number, + ) + return Response(status=status.HTTP_200_OK) From 4030475c2884c0e1b39c64c03947819a65256005 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Fri, 14 Aug 2026 13:51:01 -0400 Subject: [PATCH 04/12] fix(EDUN-15452): address PR review on finalize and webhook paths Merge PI metadata on backfill, only fall back CT payment lookup on ResourceNotFound, set Celery max_retries on the task decorator, and use event.id when refund idempotency_key is missing. Co-authored-by: Cursor --- .../commercetools/stripe_payment_finalize.py | 42 +++++++++--- .../apps/commercetools/tasks.py | 3 +- .../tests/test_stripe_payment_finalize.py | 66 +++++++++++++++++-- .../apps/stripe/tests/test_views.py | 53 +++++++++++++++ commerce_coordinator/apps/stripe/views.py | 10 ++- 5 files changed, 154 insertions(+), 20 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py index ab8c926e5..bd370d9a1 100644 --- a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py @@ -58,15 +58,26 @@ def _payment_has_charge_for(payment, charge_id: str) -> bool: ) -def _backfill_pi_metadata(payment_intent_id: str, order_id: str, payment_id: str) -> None: - """Write order_id / ct_payment_id onto the Stripe PaymentIntent (idempotent).""" +def _backfill_pi_metadata( + payment_intent_id: str, + order_id: str, + payment_id: str, + *, + existing_metadata: dict | None = None, +) -> None: + """ + Write order_id / ct_payment_id onto the Stripe PaymentIntent (idempotent). + + Merges with existing metadata so keys like source_system / ct_cart_id are preserved + even if Stripe treats metadata as a full replacement. + """ try: + merged = dict(existing_metadata or {}) + merged["order_id"] = order_id + merged["ct_payment_id"] = payment_id stripe.PaymentIntent.modify( payment_intent_id, - metadata={ - "order_id": order_id, - "ct_payment_id": payment_id, - }, + metadata=merged, ) except Exception: # pylint: disable=broad-exception-caught logger.warning( @@ -160,7 +171,10 @@ def finalize_ct_order_from_stripe_pi( if ct_payment_id_from_meta: try: payment = client.base_client.payments.get_by_id(ct_payment_id_from_meta) - except CommercetoolsError: + except CommercetoolsError as err: + # Only fall back on true not-found; re-raise transient CT failures for retry. + if err.code != "ResourceNotFound": + raise logger.warning( "[finalize_ct_order] ct_payment_id %s from metadata not found, " "falling back to key lookup for pi %s", @@ -206,7 +220,12 @@ def finalize_ct_order_from_stripe_pi( existing_order.id, payment.id, payment_intent_id, ) if not metadata.get("order_id") or metadata.get("ct_payment_id") != payment.id: - _backfill_pi_metadata(payment_intent_id, existing_order.id, payment.id) + _backfill_pi_metadata( + payment_intent_id, + existing_order.id, + payment.id, + existing_metadata=metadata, + ) return FinalizeResult( order_id=existing_order.id, order_number=existing_order.order_number or "", @@ -232,7 +251,12 @@ def finalize_ct_order_from_stripe_pi( _emit_web_order_completed(client, order, cart, payment) # --- Backfill PI metadata --- - _backfill_pi_metadata(payment_intent_id, order.id, payment.id) + _backfill_pi_metadata( + payment_intent_id, + order.id, + payment.id, + existing_metadata=metadata, + ) logger.info( "[finalize_ct_order] Successfully finalized order %s for pi=%s source=%s", diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index 18684328b..79a6a4be8 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -578,7 +578,8 @@ def _log_quarantine(*, pi_id, ct_payment_id, ct_cart_id, reason, source): @shared_task( bind=True, autoretry_for=(CommercetoolsError, stripe.error.StripeError), - retry_kwargs={"max_retries": 5, "countdown": 3}, + max_retries=5, + retry_kwargs={"countdown": 3}, ) def finalize_commercetools_stripe_payment_task( self, diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py index 395c555b3..89ffb4378 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py @@ -28,6 +28,20 @@ from commerce_coordinator.apps.core.tests.utils import uuid4_str +def _ct_error(code: str, message: str = "boom") -> CommercetoolsError: + """Build a CommercetoolsError whose .code property matches production CT errors.""" + response = MagicMock() + err_obj = MagicMock() + err_obj.code = code + response.errors = [err_obj] + return CommercetoolsError( + message=message, + errors=[{"code": code, "message": message}], + response=response, + correlation_id="corr", + ) + + def _mock_pi( pi_id="pi_test123", pi_status="succeeded", @@ -139,7 +153,15 @@ def test_happy_path(self, MockClient, mock_track, mock_stripe): client.create_order_from_cart.assert_called_once_with(cart) client.update_line_items_transition_state.assert_called_once() mock_track.assert_called_once() - mock_stripe.PaymentIntent.modify.assert_called_once() + mock_stripe.PaymentIntent.modify.assert_called_once_with( + "pi_test123", + metadata={ + "source_system": "commercetools", + "ct_cart_id": "cart-uuid", + "order_id": order.id, + "ct_payment_id": "pay-123", + }, + ) def test_order_already_exists_backfills_missing_pi_metadata(self, MockClient, mock_track, mock_stripe): """Existing order + missing PI order_id still heals metadata (recovery convergence).""" @@ -164,6 +186,8 @@ def test_order_already_exists_backfills_missing_pi_metadata(self, MockClient, mo mock_stripe.PaymentIntent.modify.assert_called_once_with( "pi_test123", metadata={ + "source_system": "commercetools", + "ct_cart_id": "cart-uuid", "order_id": existing_order.id, "ct_payment_id": "pay-123", }, @@ -199,12 +223,7 @@ def test_ct_outage_on_order_lookup_propagates(self, MockClient, mock_track, mock client = MockClient.return_value client.get_payment_by_key.return_value = payment - client.get_order_by_payment_id.side_effect = CommercetoolsError( - message="boom", - errors=[{"code": "ConcurrentModification", "message": "boom"}], - response={}, - correlation_id="corr", - ) + client.get_order_by_payment_id.side_effect = _ct_error("ConcurrentModification") with self.assertRaises(CommercetoolsError): finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") @@ -302,6 +321,39 @@ def test_resolves_payment_by_ct_payment_id(self, MockClient, mock_track, mock_st client.base_client.payments.get_by_id.assert_called_once_with("pay-from-meta") self.assertTrue(result.already_existed) + def test_ct_payment_id_not_found_falls_back_to_key(self, MockClient, mock_track, mock_stripe): + """ResourceNotFound on metadata.ct_payment_id falls back to PI key lookup.""" + pi = _mock_pi(ct_payment_id="pay-stale") + charge = _mock_charge() + mock_stripe.PaymentIntent.retrieve.return_value = pi + mock_stripe.Charge.retrieve.return_value = charge + + payment = _mock_payment(payment_id="pay-by-key", has_charge=True, charge_id="ch_test456") + existing_order = gen_order(uuid4_str()) + + client = MockClient.return_value + client.base_client.payments.get_by_id.side_effect = _ct_error("ResourceNotFound") + client.get_payment_by_key.return_value = payment + client.get_order_by_payment_id.return_value = existing_order + + result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + client.get_payment_by_key.assert_called_once_with("pi_test123") + self.assertTrue(result.already_existed) + + def test_ct_payment_id_transient_error_propagates(self, MockClient, mock_track, mock_stripe): + """Non-not-found CommercetoolsError on ct_payment_id lookup must not fall back.""" + pi = _mock_pi(ct_payment_id="pay-from-meta") + mock_stripe.PaymentIntent.retrieve.return_value = pi + + client = MockClient.return_value + client.base_client.payments.get_by_id.side_effect = _ct_error("ConcurrentModification") + + with self.assertRaises(CommercetoolsError): + finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + + client.get_payment_by_key.assert_not_called() + def test_segment_event_has_web_properties(self, MockClient, mock_track, mock_stripe): """Segment Order Completed should have is_mobile=False, plan 18, payment_method=upi.""" pi = _mock_pi() diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index a98acc2e0..cac6063de 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -283,3 +283,56 @@ def test_payment_refunded_event( ) else: mock_refund_task.assert_not_called() + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + @mock.patch('commerce_coordinator.apps.stripe.views.is_legacy_order', return_value=False) + @mock.patch('commerce_coordinator.apps.stripe.views.is_commercetools_stripe_refund', return_value=True) + @mock.patch.object(WebhookView, 'mark_running') + @mock.patch.object(WebhookView, '_is_running', return_value=False) + def test_refund_falls_back_to_event_id_when_idempotency_key_missing( + self, + mock_is_running, + mock_mark_running, + mock_is_ct_refund, + mock_is_legacy, + mock_refund_task, + mock_construct_event, + ): + """Null Stripe request.idempotency_key must not become the SingleInvocation key.""" + payment_intent_id = 'pi_refund_no_idem' + refund_data = { + 'id': "re_missing_idem", + 'amount': 1000, + 'charge': "ch_missing_idem", + 'created': 1692942318, + 'currency': "usd", + 'payment_intent': payment_intent_id, + 'status': "succeeded", + } + event_id = 'evt_refund_123' + source_system = settings.PAYMENT_PROCESSOR_CONFIG['edx']['stripe']['source_system_identifier'] + self.mock_stripe_event.type = StripeEventType.PAYMENT_REFUNDED.value + self.mock_stripe_event.id = event_id + self.mock_stripe_event.get.side_effect = lambda key, default=None: { + 'request': {'idempotency_key': None}, + 'id': event_id, + }.get(key, default) + self.mock_stripe_event.data.object.payment_intent = payment_intent_id + self.mock_stripe_event.data.object.refunds.data = [refund_data] + metadata = { + 'order_number': '2U-123456', + 'source_system': source_system, + } + self.mock_stripe_event.data.object.metadata = StripeObject() + self.mock_stripe_event.data.object.metadata.update(metadata) + mock_construct_event.return_value = self.mock_stripe_event + + response = self.client.post(self.url, data={}, format='json', **self.mock_header) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_is_running.assert_called_with(WebhookView.__name__, event_id) + mock_mark_running.assert_called_with(WebhookView.__name__, event_id) + mock_refund_task.assert_called_once() + mock_is_ct_refund.assert_called() + mock_is_legacy.assert_called() diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 7032f1a1c..998f9e83f 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -143,12 +143,16 @@ def _handle_legacy_payment_event(self, event, payment_intent, event_source_syste def _handle_refund_event(self, tag, event): """Route Commercetools refunds to the refund signal, skipping legacy orders.""" - idempotency_key = event.get('request').get('idempotency_key') - if self._is_running(tag, idempotency_key): # pragma no cover + request = event.get('request') or {} + idempotency_key = request.get('idempotency_key') if hasattr(request, 'get') else None + # Stripe request.idempotency_key can be null; fall back to event.id so + # unrelated refunds do not collide on a shared None cache key. + invocation_key = idempotency_key or event.get('id') or getattr(event, 'id', None) + if self._is_running(tag, invocation_key): # pragma no cover self.meta_should_mark_not_running = False return Response(status=status.HTTP_200_OK) - self.mark_running(tag, idempotency_key) + self.mark_running(tag, invocation_key) event_object = event.data.object order_number = event_object.metadata.order_number From 3c863a29471283558af7242e8d34e31fe0bb7c16 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Mon, 17 Aug 2026 14:11:33 -0400 Subject: [PATCH 05/12] fix(EDUN-15452): heal fulfillment on existing orders and harden recovery Address Marilyn's review: transition Initial line items on the already-existed path, lock finalize on PI id, cap list+filter scans, and stop quarantining retryable CT/Stripe errors in recovery. Co-authored-by: Cursor --- .../apps/commercetools/clients.py | 12 +- ..._orphaned_stripe_commercetools_payments.py | 81 +++++++++++-- .../commercetools/stripe_payment_finalize.py | 96 ++++++++++++++-- .../apps/commercetools/tasks.py | 18 ++- .../commercetools/tests/test_finalize_task.py | 18 ++- .../tests/test_recovery_command.py | 102 +++++++++++++++-- .../tests/test_stripe_payment_finalize.py | 108 +++++++++++++++--- commerce_coordinator/apps/stripe/views.py | 1 + 8 files changed, 385 insertions(+), 51 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/clients.py b/commerce_coordinator/apps/commercetools/clients.py index 3ac5110b4..ff756f663 100644 --- a/commerce_coordinator/apps/commercetools/clients.py +++ b/commerce_coordinator/apps/commercetools/clients.py @@ -1414,8 +1414,16 @@ def get_cart_by_id(self, cart_id: str) -> Cart: Returns: Cart object """ - logger.info(f"[CommercetoolsAPIClient] - Attempting to find cart with ID {cart_id}") - return self.base_client.carts.get_by_id(cart_id) + try: + logger.info(f"[CommercetoolsAPIClient] - Attempting to find cart with ID {cart_id}") + return self.base_client.carts.get_by_id(cart_id) + except CommercetoolsError as err: + handle_commercetools_error( + "[CommercetoolsAPIClient.get_cart_by_id]", + err, + f"Failed to find cart with ID {cart_id}", + ) + raise err @conditional_retry def get_customer_cart(self, customer_id: str) -> Optional[Cart]: diff --git a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py index c70316f6e..6ed0c87bb 100644 --- a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py +++ b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py @@ -16,6 +16,7 @@ from commercetools import CommercetoolsError from commercetools.platform.models import TransactionState, TransactionType from django.conf import settings +from stripe.error import StripeError from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME from commerce_coordinator.apps.commercetools.management.commands._ct_api_client_command import ( @@ -23,6 +24,7 @@ ) from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( FinalizeError, + FinalizeInProgressError, finalize_ct_order_from_stripe_pi ) from commerce_coordinator.apps.commercetools.tasks import _log_quarantine @@ -31,8 +33,14 @@ stripe.api_key = settings.PAYMENT_PROCESSOR_CONFIG['edx']['stripe']['secret_key'] +# Cap how many PaymentIntents the list+filter fallback will examine so a Search +# API failure cannot walk the entire Stripe account. +DEFAULT_MAX_LIST_EXAMINED = 1000 + class Command(CommercetoolsAPIClientCommand): + """Discover and finalize orphaned Stripe/CT payments that have no Order.""" + help = ( "Discover orphaned Stripe PaymentIntents / CT Payments (succeeded, " "source_system=commercetools / stripe_edx, no Order) and finalize them. " @@ -58,11 +66,21 @@ def add_arguments(self, parser): default=False, help="List orphan candidates without calling finalize", ) + parser.add_argument( + "--max-list-examined", + type=int, + default=DEFAULT_MAX_LIST_EXAMINED, + help=( + "Max PaymentIntents to examine when falling back to list+filter " + f"(default: {DEFAULT_MAX_LIST_EXAMINED})" + ), + ) def handle(self, *args, **options): since_days = options["since"] limit = options["limit"] dry_run = options["dry_run"] + max_list_examined = options["max_list_examined"] created_after = int( (datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta(days=since_days)).timestamp() @@ -75,7 +93,9 @@ def handle(self, *args, **options): f"Recovery: since={since_days}d limit={limit} dry_run={dry_run}" ) - stripe_orphans = self._discover_stripe_orphans(created_after, limit) + stripe_orphans = self._discover_stripe_orphans( + created_after, limit, max_list_examined=max_list_examined, + ) self.stdout.write(f"Discovered {len(stripe_orphans)} Stripe orphan candidate(s)") remaining = max(0, limit - len(stripe_orphans)) @@ -95,24 +115,30 @@ def handle(self, *args, **options): finalized = 0 quarantined = 0 + deferred = 0 for pi_id in orphans: - meta = self._pi_metadata(pi_id) try: result = finalize_ct_order_from_stripe_pi( pi_id, source="recovery", client=self.ct_api_client, ) if result.already_existed: self.stdout.write( - f" [skip] {pi_id} -> order {result.order_id} already existed" + f" [skip] {pi_id} -> order {result.order_id} already existed " + "(fulfillment/metadata heal applied)" ) else: self.stdout.write( f" [finalized] {pi_id} -> order {result.order_id}" ) finalized += 1 + except FinalizeInProgressError as exc: + # Another writer holds the lock; next cron will retry. Do not quarantine. + self.stderr.write(f" [deferred] {pi_id}: {exc}") + deferred += 1 except FinalizeError as exc: self.stderr.write(f" [quarantine] {pi_id}: {exc}") + meta = self._pi_metadata(pi_id) _log_quarantine( pi_id=pi_id, ct_payment_id=getattr(exc, "ct_payment_id", None) @@ -125,8 +151,13 @@ def handle(self, *args, **options): source="recovery", ) quarantined += 1 - except (CommercetoolsError, Exception) as exc: + except (CommercetoolsError, StripeError) as exc: + # Retryable — leave for the next cron run; do not quarantine (avoids NR noise). + self.stderr.write(f" [retryable] {pi_id}: {exc}") + deferred += 1 + except Exception as exc: # pylint: disable=broad-exception-caught self.stderr.write(f" [quarantine] {pi_id}: {exc}") + meta = self._pi_metadata(pi_id) _log_quarantine( pi_id=pi_id, ct_payment_id=meta.get("ct_payment_id") or "unknown", @@ -138,17 +169,25 @@ def handle(self, *args, **options): self.stdout.write( f"Recovery complete: {finalized} finalized, {quarantined} quarantined, " - f"{len(orphans) - finalized - quarantined} skipped" + f"{deferred} deferred, " + f"{len(orphans) - finalized - quarantined - deferred} skipped" ) def _pi_metadata(self, pi_id: str) -> dict: + """Fetch PI metadata only when needed for quarantine logging.""" try: pi = stripe.PaymentIntent.retrieve(pi_id) return dict(pi.metadata or {}) - except Exception: + except Exception: # pylint: disable=broad-exception-caught return {} - def _discover_stripe_orphans(self, created_after: int, limit: int) -> list[str]: + def _discover_stripe_orphans( + self, + created_after: int, + limit: int, + *, + max_list_examined: int = DEFAULT_MAX_LIST_EXAMINED, + ) -> list[str]: """ Query Stripe for PaymentIntents that are succeeded with source_system=commercetools but missing order_id metadata. @@ -159,12 +198,14 @@ def _discover_stripe_orphans(self, created_after: int, limit: int) -> list[str]: try: orphan_ids = self._search_stripe_orphans(created_after, limit) - except Exception: + except Exception: # pylint: disable=broad-exception-caught logger.warning( "[recovery] Stripe Search API failed, falling back to list+filter", exc_info=True, ) - orphan_ids = self._list_filter_stripe_orphans(created_after, limit) + orphan_ids = self._list_filter_stripe_orphans( + created_after, limit, max_examined=max_list_examined, + ) return orphan_ids @@ -205,9 +246,16 @@ def _search_stripe_orphans(self, created_after: int, limit: int) -> list[str]: return orphan_ids - def _list_filter_stripe_orphans(self, created_after: int, limit: int) -> list[str]: - """Fallback: list PIs and filter client-side.""" + def _list_filter_stripe_orphans( + self, + created_after: int, + limit: int, + *, + max_examined: int = DEFAULT_MAX_LIST_EXAMINED, + ) -> list[str]: + """Fallback: list PIs and filter client-side, with a hard examine cap.""" orphan_ids = [] + examined = 0 params = { "limit": 100, @@ -215,6 +263,15 @@ def _list_filter_stripe_orphans(self, created_after: int, limit: int) -> list[st } for pi in stripe.PaymentIntent.list(**params).auto_paging_iter(): + examined += 1 + if examined > max_examined: + logger.warning( + "[recovery] List+filter stopped after examining %d PaymentIntents " + "(max_list_examined=%d); orphans found so far=%d", + examined - 1, max_examined, len(orphan_ids), + ) + break + if pi.status != "succeeded": continue @@ -323,7 +380,7 @@ def _stripe_pi_still_orphan(pi_id: str) -> bool: """Confirm Stripe PI is succeeded commercetools and still missing order_id.""" try: pi = stripe.PaymentIntent.retrieve(pi_id) - except Exception: + except Exception: # pylint: disable=broad-exception-caught logger.warning( "[recovery] Failed to retrieve Stripe PI %s during CT secondary check", pi_id, diff --git a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py index bd370d9a1..5a90f625a 100644 --- a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py @@ -20,10 +20,14 @@ get_product_from_line_item ) from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient +from commerce_coordinator.apps.core.memcache import safe_key from commerce_coordinator.apps.core.segment import track +from commerce_coordinator.apps.core.tasks import acquire_task_lock, release_task_lock logger = logging.getLogger(__name__) +FINALIZE_LOCK_PREFIX = "finalize_ct_order_from_stripe_pi" + class FinalizeError(Exception): """Non-retryable finalization error (quarantine candidate).""" @@ -40,6 +44,10 @@ def __init__( self.ct_cart_id = ct_cart_id or "unknown" +class FinalizeInProgressError(Exception): + """Another webhook/recovery worker holds the finalize lock for this PI.""" + + @dataclass class FinalizeResult: order_id: str @@ -104,6 +112,51 @@ def _discount_amount_dollars(cart) -> float: return 0 +def _line_item_has_state_id(line_item, state_id: str) -> bool: + """True if any ItemState on the line item references the given state ID.""" + for item_state in (getattr(line_item, "state", None) or []): + ref = getattr(item_state, "state", None) + if ref is not None and getattr(ref, "id", None) == state_id: + return True + return False + + +def _ensure_pending_fulfilment(client, order): + """ + Transition line items still in Initial → PENDING_FULFILMENT. + + Uses TwoUKeys.INITIAL_FULFILMENT_STATE (looked up by key) as from_state so we do + not depend on line_items[0].state[0], which is fragile on partial-success retries. + """ + if not order.line_items: + logger.warning( + "[finalize_ct_order] Order %s has no line items; cannot transition fulfillment", + order.id, + ) + return order + + initial_state = client.get_state_by_key(TwoUKeys.INITIAL_FULFILMENT_STATE) + items_needing_transition = [ + item for item in order.line_items + if _line_item_has_state_id(item, initial_state.id) + ] + if not items_needing_transition: + logger.info( + "[finalize_ct_order] Order %s line items already past Initial; skipping transition", + order.id, + ) + return order + + return client.update_line_items_transition_state( + order_id=order.id, + order_version=order.version, + line_items=items_needing_transition, + from_state_id=initial_state.id, + new_state_key=TwoUKeys.PENDING_FULFILMENT_STATE, + use_state_id=True, + ) + + def finalize_ct_order_from_stripe_pi( payment_intent_id: str, *, @@ -119,9 +172,9 @@ def finalize_ct_order_from_stripe_pi( 2. Resolve CT Payment (by key = pi.id or metadata.ct_payment_id) 3. Resolve CT Cart (by metadata.ct_cart_id) 4. Add Charge transaction if absent (idempotent by interaction_id) - 5. Skip if order already exists for this payment (still heal PI metadata) + 5. If order already exists: heal PENDING_FULFILMENT + PI metadata, return 6. Create order from cart → COMPLETE / PAID / SHIPPED - 7. Transition line items → PENDING_FULFILMENT + 7. Transition line items → PENDING_FULFILMENT (from Initial by key) 8. Emit Segment Order Completed (plan 18, is_mobile=False) 9. Backfill PI metadata with order_id + ct_payment_id @@ -135,8 +188,34 @@ def finalize_ct_order_from_stripe_pi( Raises: FinalizeError: on non-retryable problems (missing metadata, etc.) + FinalizeInProgressError: when another writer holds the PI finalize lock CommercetoolsError: on transient CT failures (retryable by caller) """ + lock_key = safe_key( + key=payment_intent_id, + key_prefix=FINALIZE_LOCK_PREFIX, + version="1", + ) + if not acquire_task_lock(lock_key): + raise FinalizeInProgressError( + f"Finalize already in progress for PaymentIntent {payment_intent_id}" + ) + + try: + return _finalize_ct_order_from_stripe_pi_locked( + payment_intent_id, source=source, client=client, + ) + finally: + release_task_lock(lock_key) + + +def _finalize_ct_order_from_stripe_pi_locked( + payment_intent_id: str, + *, + source: str, + client: CommercetoolsAPIClient | None = None, +) -> FinalizeResult: + """Finalize body; caller holds the PI lock.""" if client is None: client = CommercetoolsAPIClient() @@ -216,9 +295,11 @@ def finalize_ct_order_from_stripe_pi( if existing_order is not None: logger.info( "[finalize_ct_order] Order %s already exists for payment %s (pi=%s), " - "skipping creation; ensuring PI metadata is backfilled", + "skipping creation; healing PENDING_FULFILMENT and PI metadata", existing_order.id, payment.id, payment_intent_id, ) + # Partial-success heal: order may exist while line items are still Initial. + _ensure_pending_fulfilment(client, existing_order) if not metadata.get("order_id") or metadata.get("ct_payment_id") != payment.id: _backfill_pi_metadata( payment_intent_id, @@ -238,14 +319,7 @@ def finalize_ct_order_from_stripe_pi( order = client.create_order_from_cart(cart) # --- Transition line items → PENDING_FULFILMENT --- - order = client.update_line_items_transition_state( - order_id=order.id, - order_version=order.version, - line_items=order.line_items, - from_state_id=order.line_items[0].state[0].state.id, - new_state_key=TwoUKeys.PENDING_FULFILMENT_STATE, - use_state_id=True, - ) + order = _ensure_pending_fulfilment(client, order) # --- Emit Segment Order Completed (plan 18, web) --- _emit_web_order_completed(client, order, cart, payment) diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index 79a6a4be8..9904d7a84 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -36,7 +36,7 @@ from commerce_coordinator.apps.order_fulfillment.serializers import OrderRevokeLineRequestSerializer from .clients import CommercetoolsAPIClient, Refund -from .stripe_payment_finalize import FinalizeError, finalize_ct_order_from_stripe_pi +from .stripe_payment_finalize import FinalizeError, FinalizeInProgressError, finalize_ct_order_from_stripe_pi from .utils import ( convert_ct_cent_amount_to_localized_price, get_lob_from_variant_attr, @@ -601,7 +601,7 @@ def finalize_commercetools_stripe_payment_task( ) if result.already_existed: logger.info( - "[%s] Order %s already existed for pi=%s, no action taken", + "[%s] Order %s already existed for pi=%s; fulfillment/metadata heal applied", tag, result.order_id, payment_intent_id, ) else: @@ -611,6 +611,20 @@ def finalize_commercetools_stripe_payment_task( ) return result.order_id + except FinalizeInProgressError: + logger.info( + "[%s] Finalize lock held for pi=%s; retrying in %s seconds", + tag, payment_intent_id, TASK_LOCK_RETRY, + ) + finalize_commercetools_stripe_payment_task.apply_async( + kwargs={ + "payment_intent_id": payment_intent_id, + "source": source, + }, + countdown=TASK_LOCK_RETRY, + ) + return None + except FinalizeError as exc: _log_quarantine( pi_id=payment_intent_id, diff --git a/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py b/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py index 25602d1e3..4fde55007 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py +++ b/commerce_coordinator/apps/commercetools/tests/test_finalize_task.py @@ -8,7 +8,11 @@ from django.test import TestCase -from commerce_coordinator.apps.commercetools.stripe_payment_finalize import FinalizeError, FinalizeResult +from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( + FinalizeError, + FinalizeInProgressError, + FinalizeResult +) from commerce_coordinator.apps.commercetools.tasks import finalize_commercetools_stripe_payment_task FINALIZE_PATH = "commerce_coordinator.apps.commercetools.tasks.finalize_ct_order_from_stripe_pi" @@ -67,3 +71,15 @@ def test_unexpected_error_quarantines_and_reraises( finalize_commercetools_stripe_payment_task("pi_boom") mock_quarantine.assert_called_once() + + @patch.object(finalize_commercetools_stripe_payment_task, "apply_async") + @patch(FINALIZE_PATH) + def test_lock_contention_reschedules(self, mock_finalize, mock_apply_async): + mock_finalize.side_effect = FinalizeInProgressError("locked") + + result = finalize_commercetools_stripe_payment_task("pi_busy") + + self.assertIsNone(result) + mock_apply_async.assert_called_once() + call_kwargs = mock_apply_async.call_args.kwargs + self.assertEqual(call_kwargs["kwargs"]["payment_intent_id"], "pi_busy") diff --git a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py index 86aeb6b32..838fdabb0 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py +++ b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py @@ -5,13 +5,18 @@ from io import StringIO from unittest.mock import MagicMock, patch +from commercetools import CommercetoolsError from commercetools.platform.models import TransactionState, TransactionType from django.test import TestCase from commerce_coordinator.apps.commercetools.management.commands.recover_orphaned_stripe_commercetools_payments import ( Command ) -from commerce_coordinator.apps.commercetools.stripe_payment_finalize import FinalizeError, FinalizeResult +from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( + FinalizeError, + FinalizeInProgressError, + FinalizeResult +) CMD_MODULE = ( "commerce_coordinator.apps.commercetools.management.commands" @@ -48,7 +53,7 @@ def test_dry_run_lists_candidates(self, mock_finalize, _mock_init): search_result.has_more = False mock_stripe.PaymentIntent.search.return_value = search_result - cmd.handle(since=7, limit=100, dry_run=True) + cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=1000) output = cmd.stdout.getvalue() self.assertIn("[dry-run] orphan: pi_orphan1", output) @@ -75,7 +80,7 @@ def test_finalize_happy_path(self, mock_finalize, _mock_init): search_result.has_more = False mock_stripe.PaymentIntent.search.return_value = search_result - cmd.handle(since=7, limit=100, dry_run=False) + cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) output = cmd.stdout.getvalue() self.assertIn("[finalized] pi_orphan1 -> order order-new", output) @@ -98,7 +103,7 @@ def test_finalize_error_quarantines(self, mock_finalize, mock_quarantine, _mock_ search_result.has_more = False mock_stripe.PaymentIntent.search.return_value = search_result - cmd.handle(since=7, limit=100, dry_run=False) + cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) err_output = cmd.stderr.getvalue() self.assertIn("[quarantine] pi_bad", err_output) @@ -129,7 +134,7 @@ def test_already_existed_skips(self, mock_finalize, _mock_init): search_result.has_more = False mock_stripe.PaymentIntent.search.return_value = search_result - cmd.handle(since=7, limit=100, dry_run=False) + cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) output = cmd.stdout.getvalue() self.assertIn("[skip] pi_existing", output) @@ -151,7 +156,7 @@ def test_limit_truncates(self, _mock_init): search_result.next_page = "page2" mock_stripe.PaymentIntent.search.return_value = search_result - cmd.handle(since=7, limit=3, dry_run=True) + cmd.handle(since=7, limit=3, dry_run=True, max_list_examined=1000) output = cmd.stdout.getvalue() self.assertIn("3 Stripe orphan candidate(s)", output) @@ -170,7 +175,7 @@ def test_search_fallback_to_list(self, _mock_init): list_result.auto_paging_iter.return_value = [pi1] mock_stripe.PaymentIntent.list.return_value = list_result - cmd.handle(since=7, limit=100, dry_run=True) + cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=1000) output = cmd.stdout.getvalue() self.assertIn("[dry-run] orphan: pi_list_orphan", output) @@ -206,9 +211,90 @@ def test_ct_secondary_discovery(self, mock_finalize, _mock_init): mock_stripe.PaymentIntent.search.return_value = search_result mock_stripe.PaymentIntent.retrieve.return_value = stripe_pi - cmd.handle(since=7, limit=100, dry_run=True) + cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=1000) output = cmd.stdout.getvalue() self.assertIn("1 CT-secondary orphan candidate(s)", output) self.assertIn("[dry-run] orphan: pi_ct_secondary", output) mock_finalize.assert_not_called() + + @patch(f"{CMD_MODULE}._log_quarantine") + @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") + def test_commercetools_error_deferred_not_quarantined( + self, mock_finalize, mock_quarantine, _mock_init + ): + """Transient CT errors must not quarantine (next cron retries).""" + cmd = self._make_command() + + pi1 = MagicMock() + pi1.id = "pi_transient" + pi1.metadata = {"source_system": "commercetools"} + + mock_finalize.side_effect = CommercetoolsError( + message="blip", + errors=[{"code": "ConcurrentModification", "message": "blip"}], + response={}, + correlation_id="corr", + ) + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + search_result = MagicMock() + search_result.data = [pi1] + search_result.has_more = False + mock_stripe.PaymentIntent.search.return_value = search_result + + cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) + + mock_stripe.PaymentIntent.retrieve.assert_not_called() + + self.assertIn("[retryable] pi_transient", cmd.stderr.getvalue()) + self.assertIn("1 deferred", cmd.stdout.getvalue()) + mock_quarantine.assert_not_called() + + @patch(f"{CMD_MODULE}._log_quarantine") + @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") + def test_lock_contention_deferred_not_quarantined( + self, mock_finalize, mock_quarantine, _mock_init + ): + cmd = self._make_command() + + pi1 = MagicMock() + pi1.id = "pi_busy" + pi1.metadata = {"source_system": "commercetools"} + mock_finalize.side_effect = FinalizeInProgressError("locked") + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + search_result = MagicMock() + search_result.data = [pi1] + search_result.has_more = False + mock_stripe.PaymentIntent.search.return_value = search_result + + cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) + + self.assertIn("[deferred] pi_busy", cmd.stderr.getvalue()) + mock_quarantine.assert_not_called() + + def test_list_filter_respects_max_examined(self, _mock_init): + """List+filter fallback must stop after max_list_examined PIs.""" + cmd = self._make_command() + + pis = [] + for i in range(20): + pi = MagicMock() + pi.id = f"pi_{i}" + pi.status = "succeeded" + # Non-matching source so we keep examining without filling limit early + pi.metadata = {"source_system": "other"} + pis.append(pi) + + with patch(f"{CMD_MODULE}.stripe") as mock_stripe: + mock_stripe.PaymentIntent.search.side_effect = Exception("search down") + list_result = MagicMock() + list_result.auto_paging_iter.return_value = pis + mock_stripe.PaymentIntent.list.return_value = list_result + + cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=5) + + # No orphans found (wrong source_system), but we must not walk past the cap + output = cmd.stdout.getvalue() + self.assertIn("0 Stripe orphan candidate(s)", output) diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py index 89ffb4378..16b1d6bf4 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py @@ -19,8 +19,10 @@ ) from django.test import TestCase +from commerce_coordinator.apps.commercetools.catalog_info.constants import TwoUKeys from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( FinalizeError, + FinalizeInProgressError, _payment_has_charge_for, finalize_ct_order_from_stripe_pi ) @@ -42,6 +44,24 @@ def _ct_error(code: str, message: str = "boom") -> CommercetoolsError: ) +def _stub_initial_matching_order(client, order): + """Make get_state_by_key(Initial) match the order's first line-item state id.""" + initial = MagicMock() + initial.id = order.line_items[0].state[0].state.id + initial.key = TwoUKeys.INITIAL_FULFILMENT_STATE + client.get_state_by_key.return_value = initial + return initial + + +def _stub_initial_unrelated(client): + """Initial state id that will not match order line items (already past Initial).""" + initial = MagicMock() + initial.id = "initial-state-id-unrelated" + initial.key = TwoUKeys.INITIAL_FULFILMENT_STATE + client.get_state_by_key.return_value = initial + return initial + + def _mock_pi( pi_id="pi_test123", pi_status="succeeded", @@ -118,13 +138,18 @@ def test_has_different_charge(self): self.assertFalse(_payment_has_charge_for(payment, "ch_match")) +@patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.release_task_lock") +@patch( + "commerce_coordinator.apps.commercetools.stripe_payment_finalize.acquire_task_lock", + return_value=True, +) @patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.stripe") @patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.track") @patch("commerce_coordinator.apps.commercetools.stripe_payment_finalize.CommercetoolsAPIClient") class TestFinalizeCTOrderFromStripePI(TestCase): """Tests for finalizing a CT order from a Stripe PaymentIntent.""" - def test_happy_path(self, MockClient, mock_track, mock_stripe): + def test_happy_path(self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock): """Full finalize: charge + order + line state + segment + PI metadata.""" pi = _mock_pi() charge = _mock_charge() @@ -144,6 +169,7 @@ def test_happy_path(self, MockClient, mock_track, mock_stripe): client.create_order_from_cart.return_value = order client.update_line_items_transition_state.return_value = order client.get_customer_by_id.return_value = customer + initial = _stub_initial_matching_order(client, order) result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") @@ -151,7 +177,11 @@ def test_happy_path(self, MockClient, mock_track, mock_stripe): self.assertFalse(result.already_existed) client.create_charge_payment_transaction.assert_called_once() client.create_order_from_cart.assert_called_once_with(cart) + client.get_state_by_key.assert_called_with(TwoUKeys.INITIAL_FULFILMENT_STATE) client.update_line_items_transition_state.assert_called_once() + transition_kwargs = client.update_line_items_transition_state.call_args.kwargs + self.assertEqual(transition_kwargs["from_state_id"], initial.id) + self.assertEqual(transition_kwargs["new_state_key"], TwoUKeys.PENDING_FULFILMENT_STATE) mock_track.assert_called_once() mock_stripe.PaymentIntent.modify.assert_called_once_with( "pi_test123", @@ -163,8 +193,10 @@ def test_happy_path(self, MockClient, mock_track, mock_stripe): }, ) - def test_order_already_exists_backfills_missing_pi_metadata(self, MockClient, mock_track, mock_stripe): - """Existing order + missing PI order_id still heals metadata (recovery convergence).""" + def test_order_already_exists_heals_fulfillment_and_metadata( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): + """Existing order with Initial line items still transitions + heals PI metadata.""" pi = _mock_pi() charge = _mock_charge() mock_stripe.PaymentIntent.retrieve.return_value = pi @@ -176,12 +208,18 @@ def test_order_already_exists_backfills_missing_pi_metadata(self, MockClient, mo client = MockClient.return_value client.get_payment_by_key.return_value = payment client.get_order_by_payment_id.return_value = existing_order + client.update_line_items_transition_state.return_value = existing_order + initial = _stub_initial_matching_order(client, existing_order) result = finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") self.assertTrue(result.already_existed) self.assertEqual(result.order_id, existing_order.id) client.create_order_from_cart.assert_not_called() + client.update_line_items_transition_state.assert_called_once() + transition_kwargs = client.update_line_items_transition_state.call_args.kwargs + self.assertEqual(transition_kwargs["from_state_id"], initial.id) + self.assertEqual(transition_kwargs["new_state_key"], TwoUKeys.PENDING_FULFILMENT_STATE) mock_track.assert_not_called() mock_stripe.PaymentIntent.modify.assert_called_once_with( "pi_test123", @@ -193,8 +231,10 @@ def test_order_already_exists_backfills_missing_pi_metadata(self, MockClient, mo }, ) - def test_order_already_exists_skips_when_metadata_complete(self, MockClient, mock_track, mock_stripe): - """When order exists and PI already has order_id, do not modify again.""" + def test_order_already_exists_skips_transition_when_past_initial( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): + """When order exists and line items are past Initial, do not re-transition.""" existing_order = gen_order(uuid4_str()) pi = _mock_pi(order_id=existing_order.id, ct_payment_id="pay-123") charge = _mock_charge() @@ -206,13 +246,25 @@ def test_order_already_exists_skips_when_metadata_complete(self, MockClient, moc client = MockClient.return_value client.base_client.payments.get_by_id.return_value = payment client.get_order_by_payment_id.return_value = existing_order + _stub_initial_unrelated(client) result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") self.assertTrue(result.already_existed) + client.update_line_items_transition_state.assert_not_called() mock_stripe.PaymentIntent.modify.assert_not_called() - def test_ct_outage_on_order_lookup_propagates(self, MockClient, mock_track, mock_stripe): + def test_lock_contention_raises_in_progress( + self, MockClient, mock_track, mock_stripe, mock_lock, _mock_unlock + ): + mock_lock.return_value = False + with self.assertRaises(FinalizeInProgressError): + finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") + MockClient.assert_not_called() + + def test_ct_outage_on_order_lookup_propagates( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): """CommercetoolsError during order lookup must not be treated as not-found.""" pi = _mock_pi() charge = _mock_charge() @@ -230,7 +282,9 @@ def test_ct_outage_on_order_lookup_propagates(self, MockClient, mock_track, mock client.create_order_from_cart.assert_not_called() - def test_order_already_exists_skips(self, MockClient, mock_track, mock_stripe): + def test_order_already_exists_skips( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): """When order already exists for the payment, skip creation.""" pi = _mock_pi() charge = _mock_charge() @@ -243,6 +297,8 @@ def test_order_already_exists_skips(self, MockClient, mock_track, mock_stripe): client = MockClient.return_value client.get_payment_by_key.return_value = payment client.get_order_by_payment_id.return_value = existing_order + client.update_line_items_transition_state.return_value = existing_order + _stub_initial_matching_order(client, existing_order) result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") @@ -251,7 +307,9 @@ def test_order_already_exists_skips(self, MockClient, mock_track, mock_stripe): client.create_order_from_cart.assert_not_called() mock_track.assert_not_called() - def test_charge_already_present_skips_creation(self, MockClient, mock_track, mock_stripe): + def test_charge_already_present_skips_creation( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): """When charge transaction already exists, don't add another.""" pi = _mock_pi() charge = _mock_charge() @@ -270,13 +328,16 @@ def test_charge_already_present_skips_creation(self, MockClient, mock_track, moc client.create_order_from_cart.return_value = order client.update_line_items_transition_state.return_value = order client.get_customer_by_id.return_value = customer + _stub_initial_matching_order(client, order) result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") client.create_charge_payment_transaction.assert_not_called() self.assertFalse(result.already_existed) - def test_pi_not_succeeded_raises_finalize_error(self, MockClient, mock_track, mock_stripe): + def test_pi_not_succeeded_raises_finalize_error( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): pi = _mock_pi(pi_status="requires_payment_method") mock_stripe.PaymentIntent.retrieve.return_value = pi @@ -285,14 +346,18 @@ def test_pi_not_succeeded_raises_finalize_error(self, MockClient, mock_track, mo self.assertIn("requires_payment_method", str(ctx.exception)) - def test_wrong_source_system_raises_finalize_error(self, MockClient, mock_track, mock_stripe): + def test_wrong_source_system_raises_finalize_error( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): pi = _mock_pi(source_system="edx/commerce_coordinator?v=1") mock_stripe.PaymentIntent.retrieve.return_value = pi with self.assertRaises(FinalizeError): finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") - def test_missing_ct_cart_id_raises_finalize_error(self, MockClient, mock_track, mock_stripe): + def test_missing_ct_cart_id_raises_finalize_error( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): pi = _mock_pi(ct_cart_id=None) pi.metadata.pop("ct_cart_id", None) mock_stripe.PaymentIntent.retrieve.return_value = pi @@ -302,7 +367,9 @@ def test_missing_ct_cart_id_raises_finalize_error(self, MockClient, mock_track, self.assertEqual(ctx.exception.ct_cart_id, "unknown") - def test_resolves_payment_by_ct_payment_id(self, MockClient, mock_track, mock_stripe): + def test_resolves_payment_by_ct_payment_id( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): """When ct_payment_id is in metadata, use it for lookup.""" pi = _mock_pi(ct_payment_id="pay-from-meta") charge = _mock_charge() @@ -315,13 +382,17 @@ def test_resolves_payment_by_ct_payment_id(self, MockClient, mock_track, mock_st client = MockClient.return_value client.base_client.payments.get_by_id.return_value = payment client.get_order_by_payment_id.return_value = existing_order + client.update_line_items_transition_state.return_value = existing_order + _stub_initial_matching_order(client, existing_order) result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") client.base_client.payments.get_by_id.assert_called_once_with("pay-from-meta") self.assertTrue(result.already_existed) - def test_ct_payment_id_not_found_falls_back_to_key(self, MockClient, mock_track, mock_stripe): + def test_ct_payment_id_not_found_falls_back_to_key( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): """ResourceNotFound on metadata.ct_payment_id falls back to PI key lookup.""" pi = _mock_pi(ct_payment_id="pay-stale") charge = _mock_charge() @@ -335,13 +406,17 @@ def test_ct_payment_id_not_found_falls_back_to_key(self, MockClient, mock_track, client.base_client.payments.get_by_id.side_effect = _ct_error("ResourceNotFound") client.get_payment_by_key.return_value = payment client.get_order_by_payment_id.return_value = existing_order + client.update_line_items_transition_state.return_value = existing_order + _stub_initial_matching_order(client, existing_order) result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") client.get_payment_by_key.assert_called_once_with("pi_test123") self.assertTrue(result.already_existed) - def test_ct_payment_id_transient_error_propagates(self, MockClient, mock_track, mock_stripe): + def test_ct_payment_id_transient_error_propagates( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): """Non-not-found CommercetoolsError on ct_payment_id lookup must not fall back.""" pi = _mock_pi(ct_payment_id="pay-from-meta") mock_stripe.PaymentIntent.retrieve.return_value = pi @@ -354,7 +429,9 @@ def test_ct_payment_id_transient_error_propagates(self, MockClient, mock_track, client.get_payment_by_key.assert_not_called() - def test_segment_event_has_web_properties(self, MockClient, mock_track, mock_stripe): + def test_segment_event_has_web_properties( + self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock + ): """Segment Order Completed should have is_mobile=False, plan 18, payment_method=upi.""" pi = _mock_pi() charge = _mock_charge() @@ -374,6 +451,7 @@ def test_segment_event_has_web_properties(self, MockClient, mock_track, mock_str client.create_order_from_cart.return_value = order client.update_line_items_transition_state.return_value = order client.get_customer_by_id.return_value = customer + _stub_initial_matching_order(client, order) finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 998f9e83f..702abb224 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -44,6 +44,7 @@ class WebhookView(SingleInvocationAPIView): http_method_names = ['post'] # accept POST request only authentication_classes = [] permission_classes = [AllowAny] + # TODO: Make this endpoint accessible for Stripe servers only. To be done in SONIC-898. @csrf_exempt def post(self, request): From 31ff51108fe0118bfa0544e17f2e3e2cf9d8fa39 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Mon, 17 Aug 2026 14:47:56 -0400 Subject: [PATCH 06/12] fix(EDUN-15452): pass CT payments.query where as a single predicate Combine paymentInterface and createdAt filters with and so the call matches existing payments.query string usage. Co-authored-by: Cursor --- .../recover_orphaned_stripe_commercetools_payments.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py index 6ed0c87bb..bb796c2f7 100644 --- a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py +++ b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py @@ -307,10 +307,10 @@ def _discover_ct_orphans( while len(orphan_ids) < limit: try: result = self.ct_api_client.base_client.payments.query( - where=[ - f'paymentMethodInfo(paymentInterface="{EDX_STRIPE_PAYMENT_INTERFACE_NAME}")', - f'createdAt > "{created_after_iso}"', - ], + where=( + f'paymentMethodInfo(paymentInterface="{EDX_STRIPE_PAYMENT_INTERFACE_NAME}") ' + f'and createdAt > "{created_after_iso}"' + ), sort=["createdAt desc"], limit=page_size, offset=offset, From 2e3f5d12de0d8d1f04926410f9b331744b7fc1bb Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Mon, 17 Aug 2026 14:57:12 -0400 Subject: [PATCH 07/12] fix(EDUN-15452): use 30-minute TTL for Stripe finalize lock Default 60s is too short for Stripe + CT + Segment; match the commercetools views lock expiry so concurrent writers cannot overlap. Co-authored-by: Cursor --- .../apps/commercetools/stripe_payment_finalize.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py index 5a90f625a..b3577e8ab 100644 --- a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py @@ -27,6 +27,8 @@ logger = logging.getLogger(__name__) FINALIZE_LOCK_PREFIX = "finalize_ct_order_from_stripe_pi" +# Stripe + multiple CT calls + Segment can exceed the default 60s lock TTL. +FINALIZE_LOCK_EXPIRE = 1800 # 30 minutes; matches commercetools/views.py class FinalizeError(Exception): @@ -196,7 +198,7 @@ def finalize_ct_order_from_stripe_pi( key_prefix=FINALIZE_LOCK_PREFIX, version="1", ) - if not acquire_task_lock(lock_key): + if not acquire_task_lock(lock_key, FINALIZE_LOCK_EXPIRE): raise FinalizeInProgressError( f"Finalize already in progress for PaymentIntent {payment_intent_id}" ) From 9e8b2d79e9637a515ca9ac70acd646d7b57bb414 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Thu, 20 Aug 2026 09:18:37 -0400 Subject: [PATCH 08/12] fix(EDUN-15452): drop orphan recovery; shorten finalize lock TTL Treat paid-without-enrollment as a support edge case instead of a scheduled job. Cut the PI lock from 30 minutes to 5 minutes so a crashed worker does not block retries. Co-authored-by: Cursor --- ..._orphaned_stripe_commercetools_payments.py | 398 ------------------ .../commercetools/stripe_payment_finalize.py | 14 +- .../tests/test_recovery_command.py | 300 ------------- .../tests/test_stripe_payment_finalize.py | 6 +- 4 files changed, 10 insertions(+), 708 deletions(-) delete mode 100644 commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py delete mode 100644 commerce_coordinator/apps/commercetools/tests/test_recovery_command.py diff --git a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py b/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py deleted file mode 100644 index bb796c2f7..000000000 --- a/commerce_coordinator/apps/commercetools/management/commands/recover_orphaned_stripe_commercetools_payments.py +++ /dev/null @@ -1,398 +0,0 @@ -""" -Management command to discover and finalize orphaned Stripe PaymentIntents / -CommerceTools Payments that have succeeded without a linked Order. - -Discovery: - 1. Stripe primary — succeeded PIs with source_system=commercetools and no order_id - 2. CT secondary — stripe_edx payments with a Success Charge and no Order - -Intended to run on an external cron (e.g. every 15-30 minutes). -""" - -import datetime -import logging - -import stripe -from commercetools import CommercetoolsError -from commercetools.platform.models import TransactionState, TransactionType -from django.conf import settings -from stripe.error import StripeError - -from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME -from commerce_coordinator.apps.commercetools.management.commands._ct_api_client_command import ( - CommercetoolsAPIClientCommand -) -from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( - FinalizeError, - FinalizeInProgressError, - finalize_ct_order_from_stripe_pi -) -from commerce_coordinator.apps.commercetools.tasks import _log_quarantine - -logger = logging.getLogger(__name__) - -stripe.api_key = settings.PAYMENT_PROCESSOR_CONFIG['edx']['stripe']['secret_key'] - -# Cap how many PaymentIntents the list+filter fallback will examine so a Search -# API failure cannot walk the entire Stripe account. -DEFAULT_MAX_LIST_EXAMINED = 1000 - - -class Command(CommercetoolsAPIClientCommand): - """Discover and finalize orphaned Stripe/CT payments that have no Order.""" - - help = ( - "Discover orphaned Stripe PaymentIntents / CT Payments (succeeded, " - "source_system=commercetools / stripe_edx, no Order) and finalize them. " - "Supports --since, --limit, --dry-run." - ) - - def add_arguments(self, parser): - parser.add_argument( - "--since", - type=int, - default=7, - help="Lookback window in days (default: 7)", - ) - parser.add_argument( - "--limit", - type=int, - default=100, - help="Maximum number of orphan candidates to process per run (default: 100)", - ) - parser.add_argument( - "--dry-run", - action="store_true", - default=False, - help="List orphan candidates without calling finalize", - ) - parser.add_argument( - "--max-list-examined", - type=int, - default=DEFAULT_MAX_LIST_EXAMINED, - help=( - "Max PaymentIntents to examine when falling back to list+filter " - f"(default: {DEFAULT_MAX_LIST_EXAMINED})" - ), - ) - - def handle(self, *args, **options): - since_days = options["since"] - limit = options["limit"] - dry_run = options["dry_run"] - max_list_examined = options["max_list_examined"] - - created_after = int( - (datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta(days=since_days)).timestamp() - ) - created_after_iso = datetime.datetime.fromtimestamp( - created_after, tz=datetime.timezone.utc - ).strftime("%Y-%m-%dT%H:%M:%S.000Z") - - self.stdout.write( - f"Recovery: since={since_days}d limit={limit} dry_run={dry_run}" - ) - - stripe_orphans = self._discover_stripe_orphans( - created_after, limit, max_list_examined=max_list_examined, - ) - self.stdout.write(f"Discovered {len(stripe_orphans)} Stripe orphan candidate(s)") - - remaining = max(0, limit - len(stripe_orphans)) - ct_orphans = [] - if remaining > 0: - ct_orphans = self._discover_ct_orphans( - created_after_iso, remaining, set(stripe_orphans), - ) - self.stdout.write(f"Discovered {len(ct_orphans)} CT-secondary orphan candidate(s)") - - orphans = stripe_orphans + ct_orphans - - if dry_run: - for pi_id in orphans: - self.stdout.write(f" [dry-run] orphan: {pi_id}") - return - - finalized = 0 - quarantined = 0 - deferred = 0 - - for pi_id in orphans: - try: - result = finalize_ct_order_from_stripe_pi( - pi_id, source="recovery", client=self.ct_api_client, - ) - if result.already_existed: - self.stdout.write( - f" [skip] {pi_id} -> order {result.order_id} already existed " - "(fulfillment/metadata heal applied)" - ) - else: - self.stdout.write( - f" [finalized] {pi_id} -> order {result.order_id}" - ) - finalized += 1 - except FinalizeInProgressError as exc: - # Another writer holds the lock; next cron will retry. Do not quarantine. - self.stderr.write(f" [deferred] {pi_id}: {exc}") - deferred += 1 - except FinalizeError as exc: - self.stderr.write(f" [quarantine] {pi_id}: {exc}") - meta = self._pi_metadata(pi_id) - _log_quarantine( - pi_id=pi_id, - ct_payment_id=getattr(exc, "ct_payment_id", None) - or meta.get("ct_payment_id") - or "unknown", - ct_cart_id=getattr(exc, "ct_cart_id", None) - or meta.get("ct_cart_id") - or "unknown", - reason=str(exc), - source="recovery", - ) - quarantined += 1 - except (CommercetoolsError, StripeError) as exc: - # Retryable — leave for the next cron run; do not quarantine (avoids NR noise). - self.stderr.write(f" [retryable] {pi_id}: {exc}") - deferred += 1 - except Exception as exc: # pylint: disable=broad-exception-caught - self.stderr.write(f" [quarantine] {pi_id}: {exc}") - meta = self._pi_metadata(pi_id) - _log_quarantine( - pi_id=pi_id, - ct_payment_id=meta.get("ct_payment_id") or "unknown", - ct_cart_id=meta.get("ct_cart_id") or "unknown", - reason=str(exc), - source="recovery", - ) - quarantined += 1 - - self.stdout.write( - f"Recovery complete: {finalized} finalized, {quarantined} quarantined, " - f"{deferred} deferred, " - f"{len(orphans) - finalized - quarantined - deferred} skipped" - ) - - def _pi_metadata(self, pi_id: str) -> dict: - """Fetch PI metadata only when needed for quarantine logging.""" - try: - pi = stripe.PaymentIntent.retrieve(pi_id) - return dict(pi.metadata or {}) - except Exception: # pylint: disable=broad-exception-caught - return {} - - def _discover_stripe_orphans( - self, - created_after: int, - limit: int, - *, - max_list_examined: int = DEFAULT_MAX_LIST_EXAMINED, - ) -> list[str]: - """ - Query Stripe for PaymentIntents that are succeeded with - source_system=commercetools but missing order_id metadata. - - Uses Stripe Search API with fallback to list+filter. - """ - orphan_ids = [] - - try: - orphan_ids = self._search_stripe_orphans(created_after, limit) - except Exception: # pylint: disable=broad-exception-caught - logger.warning( - "[recovery] Stripe Search API failed, falling back to list+filter", - exc_info=True, - ) - orphan_ids = self._list_filter_stripe_orphans( - created_after, limit, max_examined=max_list_examined, - ) - - return orphan_ids - - def _search_stripe_orphans(self, created_after: int, limit: int) -> list[str]: - """Use Stripe Search API to find orphaned PIs.""" - query = ( - f"status:'succeeded' " - f"AND metadata['source_system']:'commercetools' " - f"AND created>{created_after}" - ) - - orphan_ids = [] - has_more = True - next_page = None - - while has_more and len(orphan_ids) < limit: - kwargs = {"query": query, "limit": min(100, limit - len(orphan_ids))} - if next_page: - kwargs["page"] = next_page - - result = stripe.PaymentIntent.search(**kwargs) - - for pi in result.data: - metadata = pi.metadata or {} - if not metadata.get("order_id"): - orphan_ids.append(pi.id) - if len(orphan_ids) >= limit: - break - - has_more = result.has_more - next_page = result.next_page if has_more else None - - if has_more and len(orphan_ids) >= limit: - logger.info( - "[recovery] Stripe search truncated at limit=%d, more candidates may exist", - limit, - ) - - return orphan_ids - - def _list_filter_stripe_orphans( - self, - created_after: int, - limit: int, - *, - max_examined: int = DEFAULT_MAX_LIST_EXAMINED, - ) -> list[str]: - """Fallback: list PIs and filter client-side, with a hard examine cap.""" - orphan_ids = [] - examined = 0 - - params = { - "limit": 100, - "created": {"gte": created_after}, - } - - for pi in stripe.PaymentIntent.list(**params).auto_paging_iter(): - examined += 1 - if examined > max_examined: - logger.warning( - "[recovery] List+filter stopped after examining %d PaymentIntents " - "(max_list_examined=%d); orphans found so far=%d", - examined - 1, max_examined, len(orphan_ids), - ) - break - - if pi.status != "succeeded": - continue - - metadata = pi.metadata or {} - if metadata.get("source_system") != "commercetools": - continue - - if not metadata.get("order_id"): - orphan_ids.append(pi.id) - - if len(orphan_ids) >= limit: - logger.info( - "[recovery] List+filter truncated at limit=%d", limit, - ) - break - - return orphan_ids - - def _discover_ct_orphans( - self, - created_after_iso: str, - limit: int, - already_found: set[str], - ) -> list[str]: - """ - CT secondary discovery: stripe_edx payments with a Success Charge and - no linked Order. Returns Stripe PaymentIntent IDs (payment.interface_id). - """ - orphan_ids = [] - offset = 0 - page_size = 50 - - while len(orphan_ids) < limit: - try: - result = self.ct_api_client.base_client.payments.query( - where=( - f'paymentMethodInfo(paymentInterface="{EDX_STRIPE_PAYMENT_INTERFACE_NAME}") ' - f'and createdAt > "{created_after_iso}"' - ), - sort=["createdAt desc"], - limit=page_size, - offset=offset, - ) - except CommercetoolsError: - logger.warning( - "[recovery] CT payment query failed during secondary discovery", - exc_info=True, - ) - break - - if not result.results: - break - - for payment in result.results: - if len(orphan_ids) >= limit: - break - - pi_id = payment.interface_id - if not pi_id or pi_id in already_found or pi_id in orphan_ids: - continue - - if not self._payment_has_success_charge(payment): - continue - - try: - self.ct_api_client.get_order_by_payment_id(payment.id) - continue # order exists - except ValueError: - pass # no order — candidate - except CommercetoolsError: - logger.warning( - "[recovery] CT order lookup failed for payment %s", - payment.id, - exc_info=True, - ) - continue - - if not self._stripe_pi_still_orphan(pi_id): - continue - - orphan_ids.append(pi_id) - - if len(result.results) < page_size: - break - offset += page_size - - if len(orphan_ids) >= limit: - logger.info( - "[recovery] CT secondary discovery truncated at limit=%d", - limit, - ) - - return orphan_ids - - @staticmethod - def _payment_has_success_charge(payment) -> bool: - if not payment.transactions: - return False - return any( - t.type == TransactionType.CHARGE and t.state == TransactionState.SUCCESS - for t in payment.transactions - ) - - @staticmethod - def _stripe_pi_still_orphan(pi_id: str) -> bool: - """Confirm Stripe PI is succeeded commercetools and still missing order_id.""" - try: - pi = stripe.PaymentIntent.retrieve(pi_id) - except Exception: # pylint: disable=broad-exception-caught - logger.warning( - "[recovery] Failed to retrieve Stripe PI %s during CT secondary check", - pi_id, - exc_info=True, - ) - return False - - if pi.status != "succeeded": - return False - - metadata = pi.metadata or {} - if metadata.get("source_system") != "commercetools": - return False - - return not metadata.get("order_id") diff --git a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py index b3577e8ab..21c884143 100644 --- a/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/stripe_payment_finalize.py @@ -1,6 +1,6 @@ """ Shared finalization logic for CommerceTools orders originating from Stripe -PaymentIntents (UPI webhook + orphan recovery). +PaymentIntents (UPI webhook). Parity source: customer-twou finalizeStripePayment + runPostPaymentActions. """ @@ -27,8 +27,9 @@ logger = logging.getLogger(__name__) FINALIZE_LOCK_PREFIX = "finalize_ct_order_from_stripe_pi" -# Stripe + multiple CT calls + Segment can exceed the default 60s lock TTL. -FINALIZE_LOCK_EXPIRE = 1800 # 30 minutes; matches commercetools/views.py +# Default lock TTL is 60s; this path can exceed that under CT latency. +# 5 minutes covers a slow run without pinning a crashed worker for 30 minutes. +FINALIZE_LOCK_EXPIRE = 300 class FinalizeError(Exception): @@ -47,7 +48,7 @@ def __init__( class FinalizeInProgressError(Exception): - """Another webhook/recovery worker holds the finalize lock for this PI.""" + """Another worker holds the finalize lock for this PI.""" @dataclass @@ -166,8 +167,7 @@ def finalize_ct_order_from_stripe_pi( client: CommercetoolsAPIClient | None = None, ) -> FinalizeResult: """ - Shared finalize path used by both the webhook Celery task and the - recovery management command. + Shared finalize path used by the webhook Celery task. Steps (parity with customer-twou finalizeStripePayment): 1. Retrieve / validate Stripe PaymentIntent @@ -182,7 +182,7 @@ def finalize_ct_order_from_stripe_pi( Args: payment_intent_id: Stripe PaymentIntent ID - source: 'webhook' or 'recovery' (for quarantine log context) + source: quarantine log context (typically 'webhook') client: Optional pre-built CT client (avoids re-init in loops) Returns: diff --git a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py b/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py deleted file mode 100644 index 838fdabb0..000000000 --- a/commerce_coordinator/apps/commercetools/tests/test_recovery_command.py +++ /dev/null @@ -1,300 +0,0 @@ -""" -Tests for the recover_orphaned_stripe_commercetools_payments management command. -""" - -from io import StringIO -from unittest.mock import MagicMock, patch - -from commercetools import CommercetoolsError -from commercetools.platform.models import TransactionState, TransactionType -from django.test import TestCase - -from commerce_coordinator.apps.commercetools.management.commands.recover_orphaned_stripe_commercetools_payments import ( - Command -) -from commerce_coordinator.apps.commercetools.stripe_payment_finalize import ( - FinalizeError, - FinalizeInProgressError, - FinalizeResult -) - -CMD_MODULE = ( - "commerce_coordinator.apps.commercetools.management.commands" - ".recover_orphaned_stripe_commercetools_payments" -) - - -@patch(f"{CMD_MODULE}.CommercetoolsAPIClientCommand.__init__", return_value=None) -class TestRecoveryCommand(TestCase): - """Tests for the orphaned Stripe/CT payment recovery management command.""" - - def _make_command(self): - """Build a command instance with a mocked CT client and captured output streams.""" - cmd = Command() - cmd.ct_api_client = MagicMock() - cmd.stdout = StringIO() - cmd.stderr = StringIO() - return cmd - - @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") - def test_dry_run_lists_candidates(self, mock_finalize, _mock_init): - cmd = self._make_command() - - pi1 = MagicMock() - pi1.id = "pi_orphan1" - pi1.metadata = {"source_system": "commercetools"} - pi2 = MagicMock() - pi2.id = "pi_orphan2" - pi2.metadata = {"source_system": "commercetools"} - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - search_result = MagicMock() - search_result.data = [pi1, pi2] - search_result.has_more = False - mock_stripe.PaymentIntent.search.return_value = search_result - - cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=1000) - - output = cmd.stdout.getvalue() - self.assertIn("[dry-run] orphan: pi_orphan1", output) - self.assertIn("[dry-run] orphan: pi_orphan2", output) - mock_finalize.assert_not_called() - - @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") - def test_finalize_happy_path(self, mock_finalize, _mock_init): - cmd = self._make_command() - - pi1 = MagicMock() - pi1.id = "pi_orphan1" - pi1.metadata = {"source_system": "commercetools"} - - mock_finalize.return_value = FinalizeResult( - order_id="order-new", - order_number="2U-2026000001", - payment_id="pay-123", - ) - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - search_result = MagicMock() - search_result.data = [pi1] - search_result.has_more = False - mock_stripe.PaymentIntent.search.return_value = search_result - - cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) - - output = cmd.stdout.getvalue() - self.assertIn("[finalized] pi_orphan1 -> order order-new", output) - self.assertIn("1 finalized", output) - - @patch(f"{CMD_MODULE}._log_quarantine") - @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") - def test_finalize_error_quarantines(self, mock_finalize, mock_quarantine, _mock_init): - cmd = self._make_command() - - pi1 = MagicMock() - pi1.id = "pi_bad" - pi1.metadata = {"source_system": "commercetools"} - - mock_finalize.side_effect = FinalizeError("missing cart") - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - search_result = MagicMock() - search_result.data = [pi1] - search_result.has_more = False - mock_stripe.PaymentIntent.search.return_value = search_result - - cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) - - err_output = cmd.stderr.getvalue() - self.assertIn("[quarantine] pi_bad", err_output) - mock_quarantine.assert_called_once() - quarantine_kwargs = mock_quarantine.call_args.kwargs - self.assertEqual(quarantine_kwargs["pi_id"], "pi_bad") - self.assertEqual(quarantine_kwargs["source"], "recovery") - self.assertIn("1 quarantined", cmd.stdout.getvalue()) - - @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") - def test_already_existed_skips(self, mock_finalize, _mock_init): - cmd = self._make_command() - - pi1 = MagicMock() - pi1.id = "pi_existing" - pi1.metadata = {"source_system": "commercetools"} - - mock_finalize.return_value = FinalizeResult( - order_id="order-old", - order_number="2U-2026000002", - payment_id="pay-456", - already_existed=True, - ) - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - search_result = MagicMock() - search_result.data = [pi1] - search_result.has_more = False - mock_stripe.PaymentIntent.search.return_value = search_result - - cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) - - output = cmd.stdout.getvalue() - self.assertIn("[skip] pi_existing", output) - - def test_limit_truncates(self, _mock_init): - cmd = self._make_command() - - pis = [] - for i in range(10): - pi = MagicMock() - pi.id = f"pi_orphan_{i}" - pi.metadata = {"source_system": "commercetools"} - pis.append(pi) - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - search_result = MagicMock() - search_result.data = pis - search_result.has_more = True - search_result.next_page = "page2" - mock_stripe.PaymentIntent.search.return_value = search_result - - cmd.handle(since=7, limit=3, dry_run=True, max_list_examined=1000) - - output = cmd.stdout.getvalue() - self.assertIn("3 Stripe orphan candidate(s)", output) - - def test_search_fallback_to_list(self, _mock_init): - cmd = self._make_command() - - pi1 = MagicMock() - pi1.id = "pi_list_orphan" - pi1.status = "succeeded" - pi1.metadata = {"source_system": "commercetools"} - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - mock_stripe.PaymentIntent.search.side_effect = Exception("search not available") - list_result = MagicMock() - list_result.auto_paging_iter.return_value = [pi1] - mock_stripe.PaymentIntent.list.return_value = list_result - - cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=1000) - - output = cmd.stdout.getvalue() - self.assertIn("[dry-run] orphan: pi_list_orphan", output) - - @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") - def test_ct_secondary_discovery(self, mock_finalize, _mock_init): - """CT payments with Success Charge and no Order become orphan candidates.""" - cmd = self._make_command() - - payment = MagicMock() - payment.id = "pay-ct-1" - payment.interface_id = "pi_ct_secondary" - charge_txn = MagicMock() - charge_txn.type = TransactionType.CHARGE - charge_txn.state = TransactionState.SUCCESS - payment.transactions = [charge_txn] - - query_result = MagicMock() - query_result.results = [payment] - cmd.ct_api_client.base_client.payments.query.return_value = query_result - cmd.ct_api_client.get_order_by_payment_id.side_effect = ValueError("no order") - - stripe_pi = MagicMock() - stripe_pi.id = "pi_ct_secondary" - stripe_pi.status = "succeeded" - stripe_pi.metadata = {"source_system": "commercetools"} - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - # No Stripe-primary orphans - search_result = MagicMock() - search_result.data = [] - search_result.has_more = False - mock_stripe.PaymentIntent.search.return_value = search_result - mock_stripe.PaymentIntent.retrieve.return_value = stripe_pi - - cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=1000) - - output = cmd.stdout.getvalue() - self.assertIn("1 CT-secondary orphan candidate(s)", output) - self.assertIn("[dry-run] orphan: pi_ct_secondary", output) - mock_finalize.assert_not_called() - - @patch(f"{CMD_MODULE}._log_quarantine") - @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") - def test_commercetools_error_deferred_not_quarantined( - self, mock_finalize, mock_quarantine, _mock_init - ): - """Transient CT errors must not quarantine (next cron retries).""" - cmd = self._make_command() - - pi1 = MagicMock() - pi1.id = "pi_transient" - pi1.metadata = {"source_system": "commercetools"} - - mock_finalize.side_effect = CommercetoolsError( - message="blip", - errors=[{"code": "ConcurrentModification", "message": "blip"}], - response={}, - correlation_id="corr", - ) - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - search_result = MagicMock() - search_result.data = [pi1] - search_result.has_more = False - mock_stripe.PaymentIntent.search.return_value = search_result - - cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) - - mock_stripe.PaymentIntent.retrieve.assert_not_called() - - self.assertIn("[retryable] pi_transient", cmd.stderr.getvalue()) - self.assertIn("1 deferred", cmd.stdout.getvalue()) - mock_quarantine.assert_not_called() - - @patch(f"{CMD_MODULE}._log_quarantine") - @patch(f"{CMD_MODULE}.finalize_ct_order_from_stripe_pi") - def test_lock_contention_deferred_not_quarantined( - self, mock_finalize, mock_quarantine, _mock_init - ): - cmd = self._make_command() - - pi1 = MagicMock() - pi1.id = "pi_busy" - pi1.metadata = {"source_system": "commercetools"} - mock_finalize.side_effect = FinalizeInProgressError("locked") - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - search_result = MagicMock() - search_result.data = [pi1] - search_result.has_more = False - mock_stripe.PaymentIntent.search.return_value = search_result - - cmd.handle(since=7, limit=100, dry_run=False, max_list_examined=1000) - - self.assertIn("[deferred] pi_busy", cmd.stderr.getvalue()) - mock_quarantine.assert_not_called() - - def test_list_filter_respects_max_examined(self, _mock_init): - """List+filter fallback must stop after max_list_examined PIs.""" - cmd = self._make_command() - - pis = [] - for i in range(20): - pi = MagicMock() - pi.id = f"pi_{i}" - pi.status = "succeeded" - # Non-matching source so we keep examining without filling limit early - pi.metadata = {"source_system": "other"} - pis.append(pi) - - with patch(f"{CMD_MODULE}.stripe") as mock_stripe: - mock_stripe.PaymentIntent.search.side_effect = Exception("search down") - list_result = MagicMock() - list_result.auto_paging_iter.return_value = pis - mock_stripe.PaymentIntent.list.return_value = list_result - - cmd.handle(since=7, limit=100, dry_run=True, max_list_examined=5) - - # No orphans found (wrong source_system), but we must not walk past the cap - output = cmd.stdout.getvalue() - self.assertIn("0 Stripe orphan candidate(s)", output) diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py index 16b1d6bf4..d2ce45a11 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_payment_finalize.py @@ -211,7 +211,7 @@ def test_order_already_exists_heals_fulfillment_and_metadata( client.update_line_items_transition_state.return_value = existing_order initial = _stub_initial_matching_order(client, existing_order) - result = finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") + result = finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") self.assertTrue(result.already_existed) self.assertEqual(result.order_id, existing_order.id) @@ -342,7 +342,7 @@ def test_pi_not_succeeded_raises_finalize_error( mock_stripe.PaymentIntent.retrieve.return_value = pi with self.assertRaises(FinalizeError) as ctx: - finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") + finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") self.assertIn("requires_payment_method", str(ctx.exception)) @@ -353,7 +353,7 @@ def test_wrong_source_system_raises_finalize_error( mock_stripe.PaymentIntent.retrieve.return_value = pi with self.assertRaises(FinalizeError): - finalize_ct_order_from_stripe_pi("pi_test123", source="recovery") + finalize_ct_order_from_stripe_pi("pi_test123", source="webhook") def test_missing_ct_cart_id_raises_finalize_error( self, MockClient, mock_track, mock_stripe, _mock_lock, _mock_unlock From f994609986a4dd52823686e25024d355d48c5a05 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Thu, 20 Aug 2026 09:37:47 -0400 Subject: [PATCH 09/12] fix(EDUN-15452): compare Stripe event types against enum values event.type is a plain string; match StripeEventType.*.value so routing does not miss payment_intent and refund events. Co-authored-by: Cursor --- commerce_coordinator/apps/stripe/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 702abb224..3021df2af 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -65,7 +65,7 @@ def post(self, request): raise SignatureVerificationAPIError from e # Handle the event - if event.type in (StripeEventType.PAYMENT_SUCCESS, StripeEventType.PAYMENT_FAILED): + if event.type in (StripeEventType.PAYMENT_SUCCESS.value, StripeEventType.PAYMENT_FAILED.value): payment_intent = event.data.object event_source_system = payment_intent.metadata.get('source_system') @@ -74,14 +74,14 @@ def post(self, request): return self._handle_legacy_payment_event(event, payment_intent, event_source_system, payload) - if event.type == StripeEventType.PAYMENT_REFUNDED: + if event.type == StripeEventType.PAYMENT_REFUNDED.value: return self._handle_refund_event(tag, event) raise UnhandledStripeEventAPIError def _handle_commercetools_payment_event(self, tag, event, payment_intent): """Route CommerceTools-originated PaymentIntents (UPI) to the async finalize path.""" - if event.type != StripeEventType.PAYMENT_SUCCESS: + if event.type != StripeEventType.PAYMENT_SUCCESS.value: logger.info( '[Stripe webhooks] CT payment_intent.payment_failed for PI [%s], ignoring', payment_intent.id, @@ -107,7 +107,7 @@ def _handle_commercetools_payment_event(self, tag, event, payment_intent): def _handle_legacy_payment_event(self, event, payment_intent, event_source_system, payload): """Route legacy edX ecommerce PaymentIntents to the existing processed signal.""" - if event.type == StripeEventType.PAYMENT_SUCCESS: + if event.type == StripeEventType.PAYMENT_SUCCESS.value: payment_state = PaymentState.COMPLETED.value else: payment_state = PaymentState.FAILED.value From 57d199d83e45a32f2e4947f53fb51780c088deba Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Thu, 20 Aug 2026 10:17:50 -0400 Subject: [PATCH 10/12] fix(EDUN-15452): retry Stripe finalize on 5-minute countdown Match fulfillment CT updates so a Commercetools outage can recover without orphan recovery; the webhook already ACKs Stripe immediately. Co-authored-by: Cursor --- commerce_coordinator/apps/commercetools/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index 9904d7a84..48e9a3efe 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -579,7 +579,7 @@ def _log_quarantine(*, pi_id, ct_payment_id, ct_cart_id, reason, source): bind=True, autoretry_for=(CommercetoolsError, stripe.error.StripeError), max_retries=5, - retry_kwargs={"countdown": 3}, + retry_kwargs={"countdown": 300}, # 5 minutes between retries, to cover time during outage ) def finalize_commercetools_stripe_payment_task( self, From 9c55f2215250453c443bcc15ee7944c4bf4dbb83 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Thu, 20 Aug 2026 10:41:26 -0400 Subject: [PATCH 11/12] fix(EDUN-15452): return 503 if CT webhook enqueue fails send_robust can swallow a Celery broker error while still ACKing Stripe and leaving the 10-minute running flag set. Raise so Stripe retries and SingleInvocation clears the flag. Co-authored-by: Cursor --- .../apps/stripe/exceptions.py | 6 ++++ .../apps/stripe/tests/test_views.py | 31 +++++++++++++++++++ commerce_coordinator/apps/stripe/views.py | 16 +++++++++- 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/commerce_coordinator/apps/stripe/exceptions.py b/commerce_coordinator/apps/stripe/exceptions.py index 54fdd27f0..8c46aff4a 100644 --- a/commerce_coordinator/apps/stripe/exceptions.py +++ b/commerce_coordinator/apps/stripe/exceptions.py @@ -21,6 +21,12 @@ class UnhandledStripeEventAPIError(APIException): default_code = 'unhandled_stripe_event' +class StripeWebhookDispatchAPIError(APIException): + status_code = 503 + default_detail = 'Failed to enqueue Stripe webhook handler.' + default_code = 'stripe_webhook_dispatch_error' + + class StripeIntentCreateAPIError(APIException): status_code = 502 default_detail = 'Error while creating payment intent on payment gateway.' diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index cac6063de..4773bfc02 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -94,6 +94,7 @@ def test_ct_payment_succeeded_fires_signal(self, mock_ct_signal, mock_construct_ self.mock_stripe_event.data.object.metadata.update(metadata) self.mock_stripe_event.data.object.amount = 4900 mock_construct_event.return_value = self.mock_stripe_event + mock_ct_signal.return_value = [(lambda **kwargs: None, 'celery-task-id')] response = self.client.post( self.url, data={}, format='json', **self.mock_header @@ -105,6 +106,36 @@ def test_ct_payment_succeeded_fires_signal(self, mock_ct_signal, mock_construct_ payment_intent_id=pi_id, ) + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') + def test_ct_payment_succeeded_dispatch_failure_returns_503_and_clears_running( + self, mock_ct_signal, mock_construct_event + ): + """ + A failed send_robust must not ACK Stripe or leave the SingleInvocation + flag set; otherwise Stripe will not retry and duplicates are suppressed. + """ + pi_id = 'pi_ct_broker_down' + + def _receiver(**kwargs): + pass + + self.mock_stripe_event.type = StripeEventType.PAYMENT_SUCCESS.value + metadata = {'source_system': 'commercetools', 'ct_cart_id': 'cart-uuid'} + self.mock_stripe_event.data.object.id = pi_id + self.mock_stripe_event.data.object.metadata = StripeObject() + self.mock_stripe_event.data.object.metadata.update(metadata) + self.mock_stripe_event.data.object.amount = 4900 + mock_construct_event.return_value = self.mock_stripe_event + mock_ct_signal.return_value = [(_receiver, RuntimeError('Celery broker down'))] + + response = self.client.post( + self.url, data={}, format='json', **self.mock_header + ) + + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertFalse(WebhookView._is_running(WebhookView.__name__, pi_id)) + @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust') @mock.patch.object(WebhookView, '_is_running', return_value=True) diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 3021df2af..034607461 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -11,12 +11,14 @@ from rest_framework.response import Response from commerce_coordinator.apps.core.constants import PaymentState +from commerce_coordinator.apps.core.signal_helpers import format_signal_results from commerce_coordinator.apps.core.views import SingleInvocationAPIView from commerce_coordinator.apps.rollout.utils import is_commercetools_stripe_refund, is_legacy_order from commerce_coordinator.apps.stripe.constants import StripeEventType from commerce_coordinator.apps.stripe.exceptions import ( InvalidPayloadAPIError, SignatureVerificationAPIError, + StripeWebhookDispatchAPIError, UnhandledStripeEventAPIError ) from commerce_coordinator.apps.stripe.signals import ( @@ -99,12 +101,24 @@ def _handle_commercetools_payment_event(self, tag, event, payment_intent): payment_intent.id, ) - payment_succeeded_commercetools_signal.send_robust( + results = payment_succeeded_commercetools_signal.send_robust( sender=self.__class__, payment_intent_id=payment_intent.id, ) + self._assert_signal_dispatched(results, payment_intent_id=payment_intent.id) return Response(status=status.HTTP_200_OK) + def _assert_signal_dispatched(self, results, *, payment_intent_id): + """Raise so Stripe retries if enqueue failed; handle_exception clears the running flag.""" + formatted = format_signal_results(results) + if not results or any(entry["error"] for entry in formatted.values()): + logger.error( + '[Stripe webhooks] Failed to enqueue CT finalize for PI [%s]: %s', + payment_intent_id, + formatted, + ) + raise StripeWebhookDispatchAPIError + def _handle_legacy_payment_event(self, event, payment_intent, event_source_system, payload): """Route legacy edX ecommerce PaymentIntents to the existing processed signal.""" if event.type == StripeEventType.PAYMENT_SUCCESS.value: From 9edfb40f912aa19f429fb2d77188d035841e4b32 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Thu, 20 Aug 2026 10:49:59 -0400 Subject: [PATCH 12/12] fix(EDUN-15452): silence pylint protected-access in webhook test Co-authored-by: Cursor --- commerce_coordinator/apps/stripe/tests/test_views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index 4773bfc02..2bd9e434c 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -134,7 +134,9 @@ def _receiver(**kwargs): ) self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) - self.assertFalse(WebhookView._is_running(WebhookView.__name__, pi_id)) + self.assertFalse( + WebhookView._is_running(WebhookView.__name__, pi_id) # pylint: disable=protected-access + ) @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.payment_succeeded_commercetools_signal.send_robust')