From 09eadc4c3916f5cd4820c39e2bf33049a62842dd Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Fri, 21 Aug 2026 14:04:11 -0400 Subject: [PATCH 01/11] feat(commercetools): reconcile async Stripe refunds (EDUN-15350) Co-authored-by: Cursor --- .../apps/commercetools/clients.py | 38 +- .../apps/commercetools/pipeline.py | 17 +- .../commercetools/stripe_refund_reconcile.py | 380 ++++++++++++++++++ .../apps/commercetools/sub_messages/tasks.py | 29 +- .../apps/commercetools/tasks.py | 62 +-- .../tests/sub_messages/test_tasks.py | 79 +++- .../apps/commercetools/tests/test_clients.py | 20 + .../apps/commercetools/tests/test_pipeline.py | 24 ++ .../tests/test_stripe_refund_reconcile.py | 265 ++++++++++++ .../apps/commercetools/tests/test_tasks.py | 43 +- .../apps/commercetools/tests/test_utils.py | 14 + .../apps/commercetools/utils.py | 18 +- commerce_coordinator/apps/stripe/clients.py | 6 +- commerce_coordinator/apps/stripe/constants.py | 5 + .../apps/stripe/tests/test_clients.py | 24 ++ .../apps/stripe/tests/test_views.py | 181 +++++++++ commerce_coordinator/apps/stripe/views.py | 103 +++-- 17 files changed, 1175 insertions(+), 133 deletions(-) create mode 100644 commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py create mode 100644 commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py diff --git a/commerce_coordinator/apps/commercetools/clients.py b/commerce_coordinator/apps/commercetools/clients.py index ff756f663..e1bb1752e 100644 --- a/commerce_coordinator/apps/commercetools/clients.py +++ b/commerce_coordinator/apps/commercetools/clients.py @@ -9,7 +9,7 @@ from decimal import Decimal from functools import wraps from types import SimpleNamespace -from typing import Generic, List, NamedTuple, Optional, Tuple, TypedDict, TypeVar, Union +from typing import Generic, List, NamedTuple, NotRequired, Optional, Tuple, TypedDict, TypeVar, Union import requests from commercetools import Client, CommercetoolsError @@ -48,6 +48,7 @@ OrderTransitionLineItemStateAction, Payment, PaymentAddTransactionAction, + PaymentChangeTransactionStateAction, PaymentDraft, PaymentMethodInfo, PaymentResourceIdentifier, @@ -142,6 +143,7 @@ class Refund(TypedDict): currency: str created: Union[str, int] status: str + payment_intent: NotRequired[str] class ProcessedRefund(TypedDict): @@ -957,6 +959,40 @@ def create_return_payment_transaction( ) raise err + def change_refund_transaction_state( + self, + payment_id: str, + payment_version: int, + transaction_id: str, + state: TransactionState, + ) -> Payment: + """Change the state of an existing CommerceTools refund transaction.""" + logger.info( + "[CommercetoolsAPIClient] - Changing refund transaction %s on payment %s to %s", + transaction_id, + payment_id, + state, + ) + try: + return self.base_client.payments.update_by_id( + id=payment_id, + version=payment_version, + actions=[ + PaymentChangeTransactionStateAction( + transaction_id=transaction_id, + state=state, + ) + ], + ) + except CommercetoolsError as err: + handle_commercetools_error( + "[CommercetoolsAPIClient.change_refund_transaction_state]", + err, + f"Unable to change refund transaction {transaction_id} " + f"on payment {payment_id} to {state}", + ) + raise + def create_charge_payment_transaction( self, payment_id: str, diff --git a/commerce_coordinator/apps/commercetools/pipeline.py b/commerce_coordinator/apps/commercetools/pipeline.py index 063a586c7..8a97c8a23 100644 --- a/commerce_coordinator/apps/commercetools/pipeline.py +++ b/commerce_coordinator/apps/commercetools/pipeline.py @@ -34,6 +34,7 @@ is_commercetools_line_item_already_refunded ) from commerce_coordinator.apps.rollout.waffle import is_redirect_to_commercetools_enabled_for_user +from commerce_coordinator.apps.stripe.constants import StripeRefundStatus log = getLogger(__name__) @@ -313,7 +314,15 @@ def run_filter( refunded_line_item_refunds = kwargs['refunded_line_item_refunds'] refund_response = kwargs.get('refund_response', {}) - interaction_id = refund_response.get('id') if isinstance(refund_response, dict) else None + interaction_id = refund_response.get('id') if hasattr(refund_response, 'get') else None + refund_status = refund_response.get('status') if hasattr(refund_response, 'get') else None + is_stripe_async_refund = ( + kwargs.get('psp') == EDX_STRIPE_PAYMENT_INTERFACE_NAME + and refund_status in { + StripeRefundStatus.REFUND_PENDING.value, + StripeRefundStatus.REFUND_SUCCESS.value, + } + ) ct_api_client = CommercetoolsAPIClient() updated_order = ct_api_client.update_return_payment_state_after_successful_refund( @@ -324,11 +333,13 @@ def run_filter( refunded_line_item_refunds=refunded_line_item_refunds, payment_intent_id=kwargs['payment_intent_id'], interaction_id=interaction_id, - payment_state=payment_state + payment_state=payment_state, + should_transition_state=not is_stripe_async_refund, ) return { - "returned_order": updated_order + "returned_order": updated_order, + "refund_pending": refund_status == StripeRefundStatus.REFUND_PENDING.value, } diff --git a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py new file mode 100644 index 000000000..4cb1782b2 --- /dev/null +++ b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py @@ -0,0 +1,380 @@ +"""State-aware Stripe refund reconciliation for CommerceTools payments.""" + +import logging +from dataclasses import dataclass + +from commercetools.platform.models import ReturnPaymentState, TransactionState +from django.utils.module_loading import import_string +from iso4217 import Currency + +from commerce_coordinator.apps.commercetools.catalog_info.edx_utils import check_is_bundle, get_edx_lms_user_id +from commerce_coordinator.apps.commercetools.catalog_info.utils import get_product_data +from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient, Refund +from commerce_coordinator.apps.commercetools.utils import ( + convert_ct_cent_amount_to_localized_price, + get_refund_transaction_by_interaction_id, + prepare_segment_event_properties +) +from commerce_coordinator.apps.core.memcache import safe_key +from commerce_coordinator.apps.core.segment import track +from commerce_coordinator.apps.core.signal_helpers import format_signal_results +from commerce_coordinator.apps.core.tasks import acquire_task_lock, release_task_lock +from commerce_coordinator.apps.stripe.constants import StripeRefundStatus + +logger = logging.getLogger(__name__) + +REFUND_RECONCILE_LOCK_PREFIX = "reconcile_stripe_refund" +REFUND_RECONCILE_LOCK_EXPIRE = 300 + +STRIPE_TO_CT_STATE = { + StripeRefundStatus.REFUND_PENDING.value: TransactionState.PENDING, + StripeRefundStatus.REFUND_SUCCESS.value: TransactionState.SUCCESS, + StripeRefundStatus.REFUND_FAILED.value: TransactionState.FAILURE, + StripeRefundStatus.REFUND_CANCELED.value: TransactionState.FAILURE, +} + + +class RefundReconcileInProgressError(Exception): + """Another worker is reconciling the same Stripe refund.""" + + +class RefundSideEffectDispatchError(Exception): + """A refund side effect could not be dispatched safely.""" + + +@dataclass +class RefundReconcileResult: + """Outcome of a refund reconciliation attempt.""" + + payment_id: str | None + refund_id: str + transaction_state: TransactionState | None + side_effects_completed: bool = False + return_found: bool = False + + +def refund_reconcile_lock_key(refund_id: str) -> str: + """Build the shared lock key used by webhook admission and workers.""" + return safe_key( + key=refund_id, + key_prefix=REFUND_RECONCILE_LOCK_PREFIX, + version="1", + ) + + +def reconcile_stripe_refund( + payment_intent_id: str, + stripe_refund: Refund, + *, + order_number: str | None = None, + source: str, + client: CommercetoolsAPIClient | None = None, +) -> RefundReconcileResult: + """Reconcile one Stripe Refund under a lock keyed by Stripe Refund ID.""" + refund_id = stripe_refund["id"] + target_state = STRIPE_TO_CT_STATE.get(stripe_refund.get("status")) + if target_state is None: + logger.warning( + "[stripe_refund_reconcile] Ignoring refund %s with unknown status %s", + refund_id, + stripe_refund.get("status"), + ) + return RefundReconcileResult(None, refund_id, None) + + lock_key = refund_reconcile_lock_key(refund_id) + if not acquire_task_lock(lock_key, REFUND_RECONCILE_LOCK_EXPIRE): + raise RefundReconcileInProgressError( + f"Refund reconciliation already in progress for {refund_id}" + ) + + try: + return _reconcile_stripe_refund_locked( + payment_intent_id, + stripe_refund, + target_state=target_state, + order_number=order_number, + source=source, + client=client, + ) + finally: + release_task_lock(lock_key) + + +def _reconcile_stripe_refund_locked( + payment_intent_id: str, + stripe_refund: Refund, + *, + target_state: TransactionState, + order_number: str | None, + source: str, + client: CommercetoolsAPIClient | None, +) -> RefundReconcileResult: + """Reconcile body; the caller holds the refund-ID lock.""" + if client is None: + client = CommercetoolsAPIClient() + + refund_id = stripe_refund["id"] + payment = client.get_payment_by_key(payment_intent_id) + transaction = get_refund_transaction_by_interaction_id(payment, refund_id) + from_state = transaction.state if transaction else None + + if transaction is None: + payment = client.create_return_payment_transaction( + payment_id=payment.id, + payment_version=payment.version, + refund=stripe_refund, + ) + transaction = get_refund_transaction_by_interaction_id(payment, refund_id) + elif _can_transition(transaction.state, target_state): + payment = client.change_refund_transaction_state( + payment_id=payment.id, + payment_version=payment.version, + transaction_id=transaction.id, + state=target_state, + ) + transaction = get_refund_transaction_by_interaction_id(payment, refund_id) + + if transaction is None or _state_value(transaction.state) != _state_value(target_state): + logger.warning( + "[stripe_refund_reconcile] Ignoring conflicting terminal state for refund %s: " + "ct_state=%s stripe_target=%s", + refund_id, + transaction.state if transaction else None, + target_state, + ) + return RefundReconcileResult( + payment.id, + refund_id, + transaction.state if transaction else None, + ) + + logger.info( + "[stripe_refund_reconcile] payment=%s stripe_refund_id=%s source=%s " + "from_state=%s to_state=%s", + payment.id, + refund_id, + source, + from_state, + target_state, + ) + + order = _find_order(client, payment.id, order_number) + if order is None: + logger.warning( + "[stripe_refund_reconcile] Refund %s reached %s without a CT order; " + "payment transaction updated only", + refund_id, + target_state, + ) + return RefundReconcileResult(payment.id, refund_id, target_state) + + return_items = _resolve_return_items(order, transaction) + if not return_items: + logger.warning( + "[stripe_refund_reconcile] Refund %s reached %s with no CT Return; " + "payment transaction updated only", + refund_id, + target_state, + ) + return RefundReconcileResult(payment.id, refund_id, target_state) + + result = RefundReconcileResult( + payment.id, + refund_id, + target_state, + return_found=True, + ) + if target_state == TransactionState.PENDING: + _update_return_state( + client, + order, + payment, + return_items, + stripe_refund, + should_transition_state=False, + ) + logger.info( + "[stripe_refund_reconcile] Refund %s remains pending; no LMS or Segment side effects", + refund_id, + ) + return result + + if target_state == TransactionState.FAILURE: + if not _all_in_payment_state(return_items, ReturnPaymentState.NOT_REFUNDED): + _update_return_state( + client, + order, + payment, + return_items, + stripe_refund, + payment_state=ReturnPaymentState.NOT_REFUNDED, + ) + logger.warning( + "[stripe_refund_reconcile] Refund %s failed; LMS access preserved and Segment suppressed", + refund_id, + ) + return result + + if _all_in_payment_state(return_items, ReturnPaymentState.REFUNDED): + logger.info( + "[stripe_refund_reconcile] Refund %s side effects already recorded by CT return state", + refund_id, + ) + result.side_effects_completed = True + return result + + _dispatch_revoke(order.id, return_items) + _emit_segment_refund(client, order, stripe_refund, return_items) + _update_return_state( + client, + order, + payment, + return_items, + stripe_refund, + payment_state=ReturnPaymentState.REFUNDED, + ) + result.side_effects_completed = True + logger.info( + "[stripe_refund_reconcile] Refund %s completed lms_revoke=true segment_emitted=true", + refund_id, + ) + return result + + +def _state_value(state): + return getattr(state, "value", state) + + +def _can_transition(current_state, target_state: TransactionState) -> bool: + """Allow only Pending to terminal transitions; never regress terminal CT state.""" + current = _state_value(current_state) + target = _state_value(target_state) + if current == target: + return False + return current == TransactionState.PENDING.value and target in { + TransactionState.SUCCESS.value, + TransactionState.FAILURE.value, + } + + +def _find_order(client, payment_id: str, order_number: str | None): + """Find the order by explicit number or its attached CT payment.""" + try: + if order_number: + return client.get_order_by_number(order_number) + return client.get_order_by_payment_id(payment_id) + except ValueError: + return None + + +def _resolve_return_items(order, transaction) -> list: + """Resolve return items from the transaction marker or open CT returns.""" + all_return_items = [ + item + for return_info in (order.return_info or []) + for item in return_info.items + ] + custom = getattr(transaction, "custom", None) + fields = getattr(custom, "fields", {}) if custom else {} + return_item_ids = { + item_id.strip() + for item_id in (fields.get("returnItemId", "") or "").split(",") + if item_id.strip() + } + if return_item_ids: + return [item for item in all_return_items if item.id in return_item_ids] + return [ + item + for item in all_return_items + if item.payment_state == ReturnPaymentState.INITIAL + ] + + +def _all_in_payment_state(return_items, payment_state: ReturnPaymentState) -> bool: + return all(item.payment_state == payment_state for item in return_items) + + +def _return_item_payload(return_items) -> list[dict]: + return [ + {"id": item.id, "lineItemId": item.line_item_id} + for item in return_items + ] + + +def _dispatch_revoke(order_id: str, return_items) -> None: + """Dispatch the existing retryable LMS revoke task through its signal.""" + revoke_signal = import_string( + "commerce_coordinator.apps.commercetools.signals." + "fulfill_order_returned_send_revoke_line_items_signal" + ) + + results = revoke_signal.send_robust( + sender=reconcile_stripe_refund, + order_id=order_id, + return_items=_return_item_payload(return_items), + ) + formatted = format_signal_results(results) + if not results or any(entry["error"] for entry in formatted.values()): + raise RefundSideEffectDispatchError( + f"Unable to dispatch LMS revoke for order {order_id}: {formatted}" + ) + + +def _emit_segment_refund(client, order, stripe_refund: Refund, return_items) -> None: + """Emit the existing Order Refunded Segment payload for selected returns.""" + customer = client.get_customer_by_id(order.customer_id) + lms_user_id = get_edx_lms_user_id(customer) + line_item_ids = {item.line_item_id for item in return_items} + selected_line_items = [ + item for item in order.line_items if item.id in line_item_ids + ] + fraction_digits = Currency(stripe_refund["currency"].upper()).exponent + total = convert_ct_cent_amount_to_localized_price( + stripe_refund["amount"], + fraction_digits, + ) + properties = prepare_segment_event_properties( + order=order, + total_in_dollars=str(total), + line_item_ids=list(line_item_ids), + return_id=", ".join(item.id for item in return_items), + ) + is_bundle = check_is_bundle(order.line_items) + properties["products"] = [ + get_product_data(item, is_bundle) for item in selected_line_items + ] + if properties["products"]: + properties["title"] = ", ".join( + item.name["en-US"] for item in selected_line_items + ) + track( + lms_user_id=lms_user_id, + event="Order Refunded", + properties=properties, + ) + + +def _update_return_state( + client, + order, + payment, + return_items, + stripe_refund, + *, + payment_state: ReturnPaymentState = ReturnPaymentState.REFUNDED, + should_transition_state: bool = True, +): + """Persist the CT return state and refund-to-return custom markers.""" + return_item_ids = [item.id for item in return_items] + return client.update_return_payment_state_after_successful_refund( + order_id=order.id, + order_version=order.version, + return_line_item_return_ids=return_item_ids, + return_line_entitlement_ids={}, + refunded_line_item_refunds={}, + payment_intent_id=stripe_refund["payment_intent"], + interaction_id=stripe_refund["id"], + payment_state=payment_state, + payment=payment, + should_transition_state=should_transition_state, + ) diff --git a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py index 1d36ea32a..172892329 100644 --- a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py +++ b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py @@ -7,7 +7,6 @@ from commercetools import CommercetoolsError from django.contrib.auth import get_user_model from edx_django_utils.cache import TieredCache -from iso4217 import Currency from requests import RequestException from commerce_coordinator.apps.commercetools.catalog_info.constants import ( @@ -44,8 +43,8 @@ fulfill_order_placed_send_entitlement_signal, fulfill_order_returned_send_revoke_line_items_signal ) +from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import reconcile_stripe_refund from commerce_coordinator.apps.commercetools.utils import ( - convert_ct_cent_amount_to_localized_price, extract_ct_order_information_for_braze_canvas, extract_ct_product_information_for_braze_canvas, get_lob_from_variant_attr, @@ -57,6 +56,7 @@ from commerce_coordinator.apps.core.segment import track from commerce_coordinator.apps.lms.clients import LMSAPIClient from commerce_coordinator.apps.order_fulfillment.clients import OrderFulfillmentAPIClient +from commerce_coordinator.apps.stripe.constants import StripeRefundStatus User = get_user_model() @@ -397,16 +397,25 @@ def _get_product_data(line_item, is_bundle): refunded_line_item_ids = result.get("filtered_line_item_ids", return_line_item_ids) if psp == EDX_STRIPE_PAYMENT_INTERFACE_NAME: - fraction_digits = Currency( - refund_response["currency"].upper() - ).exponent - refund_amount_in_dollars = str( - convert_ct_cent_amount_to_localized_price( - refund_response["amount"], - fraction_digits, + if refund_response.get("status") == StripeRefundStatus.REFUND_PENDING.value: + logger.info( + "[CT-%s] Stripe refund %s is pending; deferring return state, " + "LMS revoke, and Segment", + tag, + refund_response["id"], ) + return True + + reconcile_stripe_refund( + psp_payment_id, + refund_response, + order_number=order.order_number, + source="forward", + client=client, ) - elif psp == EDX_PAYPAL_PAYMENT_INTERFACE_NAME: + return True + + if psp == EDX_PAYPAL_PAYMENT_INTERFACE_NAME: refund_amount_in_dollars = result["refund_response"]["amount"] else: refund_amount_in_dollars = result.get("amount_in_dollars") diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index 48e9a3efe..63b0dad5b 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -7,7 +7,6 @@ from commercetools import CommercetoolsError from commercetools.platform.models import Payment from django.conf import settings -from iso4217 import Currency from requests import RequestException from commerce_coordinator.apps.commercetools.catalog_info.constants import ( @@ -37,8 +36,12 @@ from .clients import CommercetoolsAPIClient, Refund from .stripe_payment_finalize import FinalizeError, FinalizeInProgressError, finalize_ct_order_from_stripe_pi +from .stripe_refund_reconcile import ( + RefundReconcileInProgressError, + RefundSideEffectDispatchError, + reconcile_stripe_refund +) from .utils import ( - convert_ct_cent_amount_to_localized_price, get_lob_from_variant_attr, has_full_refund_transaction, is_transaction_already_refunded, @@ -129,14 +132,19 @@ def _log_info_and_release_lock(log_message): @shared_task( - autoretry_for=(CommercetoolsError,), + autoretry_for=( + CommercetoolsError, + RefundReconcileInProgressError, + RefundSideEffectDispatchError, + ), retry_kwargs={"max_retries": 5, "countdown": 3}, ) def refund_from_stripe_task( payment_intent_id: str, stripe_refund: Refund, - order_number: str | None = None -) -> Payment | None: + order_number: str | None = None, + source: str = "webhook", +): """ Celery task for handling a refund registered in the Stripe dashboard. Creates a refund payment transaction record via the Commercetools API. @@ -145,44 +153,12 @@ def refund_from_stripe_task( refund (dict): Refund object payment_intent_id (str): The Stripe payment intent identifier """ - client = CommercetoolsAPIClient() - try: - logger.info( - f"[refund_from_stripe_task] Initiating creation of CT payment's refund transaction object " - f"for payment Intent ID {payment_intent_id}." - ) - payment = client.get_payment_by_key(payment_intent_id) - if has_full_refund_transaction(payment) or is_transaction_already_refunded( - payment, stripe_refund["id"] - ): - logger.info( - f"[refund_from_stripe_task] Event 'charge.refunded' received, but Payment with ID {payment.id} " - f"already has a full refund. Skipping task to add refund transaction" - ) - return None - - updated_payment = client.create_return_payment_transaction( - payment_id=payment.id, - payment_version=payment.version, - refund=stripe_refund, - ) - total_in_dollars = convert_ct_cent_amount_to_localized_price( - stripe_refund["amount"], - Currency(stripe_refund["currency"].upper()).exponent, - ) - _send_segement_event( - order_number=order_number, - total_in_dollars=str(total_in_dollars), - client=client, - ) - return updated_payment - except CommercetoolsError as err: - logger.error( - f"[refund_from_stripe_task] Unable to create CT payment's refund transaction " - f"object for [ {payment.id} ] on Stripe refund {stripe_refund['id']} " - f"with error {err.errors} and correlation id {err.correlation_id}" - ) - raise err + return reconcile_stripe_refund( + payment_intent_id, + stripe_refund, + order_number=order_number, + source=source, + ) def _send_segement_event(*, order_number, total_in_dollars, client) -> None: diff --git a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py index 2e1e8053e..8d0adc37b 100644 --- a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py +++ b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py @@ -9,7 +9,7 @@ from commercetools.platform.models import ReturnPaymentState as CTReturnPaymentState from edx_django_utils.cache import TieredCache -from commerce_coordinator.apps.commercetools.catalog_info.constants import TwoUKeys +from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME, TwoUKeys from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient from commerce_coordinator.apps.commercetools.constants import SOURCE_SYSTEM from commerce_coordinator.apps.commercetools.sub_messages.tasks import ( @@ -451,6 +451,11 @@ class OrderReturnedMessageSignalTaskTests(TestCase): def setUp(self): super().setUp() self.mock = CommercetoolsAPIClientMock() + reconcile_patcher = patch( + "commerce_coordinator.apps.commercetools.sub_messages.tasks.reconcile_stripe_refund", + ) + self.mock_reconcile_refund = reconcile_patcher.start() + self.addCleanup(reconcile_patcher.stop) revoke_send_patcher = patch( "commerce_coordinator.apps.commercetools.sub_messages.tasks." @@ -508,8 +513,11 @@ def test_correct_arguments_passed_already_refunded_doest_break(self, _stripe_api """ mock_values = self.mock _stripe_api_mock.return_value.refund_payment_intent.return_value = { + "id": "re_existing", + "payment_intent": "mock_payment_intent_id", "currency": "usd", - "amount": 1000 + "amount": 1000, + "status": "succeeded", } ret_val = self.get_uut()(*self.unpack_for_uut(self.mock.example_payload)) @@ -537,8 +545,11 @@ def test_correct_arguments_passed_valid_stripe_refund( mock_values = self.mock mock_values.order_mock.return_value.return_info = [] _stripe_api_mock.return_value.refund_payment_intent.return_value = { + "id": "re_valid", + "payment_intent": "mock_payment_intent_id", "currency": "usd", - "amount": 1000 + "amount": 1000, + "status": "succeeded", } _return_order_mock.return_value = CTOrder.deserialize(mock_values.order_mock.return_value.serialize()) _return_order_mock.return_value.return_info.append( @@ -553,6 +564,7 @@ def test_correct_arguments_passed_valid_stripe_refund( mock_values.order_mock.assert_has_calls([call(mock_values.order_id), call(order_id=mock_values.order_id)]) mock_values.customer_mock.assert_called_once_with(mock_values.customer_id) _stripe_api_mock.return_value.refund_payment_intent.assert_called_once() + self.mock_reconcile_refund.assert_called_once() @patch('commerce_coordinator.apps.commercetools.sub_messages.tasks.get_edx_psp_payment_id') @patch('commerce_coordinator.apps.commercetools.sub_messages.tasks.OrderRefundRequested.run_filter') @@ -659,6 +671,11 @@ class FulfillOrderReturnedSignalTaskTests(TestCase): def setUp(self): super().setUp() + reconcile_patcher = patch( + "commerce_coordinator.apps.commercetools.sub_messages.tasks.reconcile_stripe_refund", + ) + self.mock_reconcile_refund = reconcile_patcher.start() + self.addCleanup(reconcile_patcher.stop) revoke_send_patcher = patch( "commerce_coordinator.apps.commercetools.sub_messages.tasks." "fulfill_order_returned_send_revoke_line_items_signal.send_robust", @@ -744,18 +761,29 @@ def test_refund_successful(self, _ct_client_init: CommercetoolsAPIClientMock, _r Check calling uut when refund is successful. """ mock_values = _ct_client_init.return_value - _run_filter_mock.return_value = {'refund_response': 'succeeded'} + _run_filter_mock.return_value = { + 'refund_response': { + 'id': 're_succeeded', + 'payment_intent': 'pi_succeeded', + 'status': 'succeeded', + }, + 'psp': EDX_STRIPE_PAYMENT_INTERFACE_NAME, + } payload = mock_values.example_payload ret_val = self.get_uut()(*self.unpack_for_uut(payload)) self.assertTrue(ret_val) mock_values.order_mock.assert_called_once_with(mock_values.order_id) mock_values.customer_mock.assert_called_once_with(mock_values.customer_id) - self.mock_revoke_line_send.assert_called_once_with( - sender=fulfill_order_returned_signal_task, - order_id=payload["order_id"], - return_items=payload["return_items"], + self.mock_reconcile_refund.assert_called_once() + reconcile_args, reconcile_kwargs = self.mock_reconcile_refund.call_args + self.assertEqual( + reconcile_args[1], + _run_filter_mock.return_value["refund_response"], ) + self.assertEqual(reconcile_kwargs["source"], "forward") + self.assertEqual(reconcile_kwargs["client"], mock_values) + self.mock_revoke_line_send.assert_not_called() def test_refund_successful_with_segment(self, _ct_client_init: CommercetoolsAPIClientMock, _run_filter_mock): """ @@ -763,7 +791,12 @@ def test_refund_successful_with_segment(self, _ct_client_init: CommercetoolsAPIC """ mock_values = _ct_client_init.return_value _run_filter_mock.return_value = { - 'refund_response': 'succeeded', + 'refund_response': { + 'id': 're_succeeded', + 'payment_intent': 'pi_succeeded', + 'status': 'succeeded', + }, + 'psp': EDX_STRIPE_PAYMENT_INTERFACE_NAME, 'amount_in_cents': 5400, 'filtered_line_item_ids': ['822d77c4-00a6-4fb9-909b-094ef0b8c4b9'], } @@ -773,11 +806,29 @@ def test_refund_successful_with_segment(self, _ct_client_init: CommercetoolsAPIC self.assertTrue(ret_val) mock_values.order_mock.assert_called_once_with(mock_values.order_id) mock_values.customer_mock.assert_called_once_with(mock_values.customer_id) - self.mock_revoke_line_send.assert_called_once_with( - sender=fulfill_order_returned_signal_task, - order_id=payload["order_id"], - return_items=payload["return_items"], - ) + self.mock_reconcile_refund.assert_called_once() + self.mock_revoke_line_send.assert_not_called() + + def test_pending_refund_skips_revoke_segment_and_reconcile( + self, + _ct_client_init: CommercetoolsAPIClientMock, + _run_filter_mock, + ): + mock_values = _ct_client_init.return_value + _run_filter_mock.return_value = { + 'refund_response': { + 'id': 're_pending', + 'payment_intent': 'pi_pending', + 'status': 'pending', + }, + 'psp': EDX_STRIPE_PAYMENT_INTERFACE_NAME, + } + + ret_val = self.get_uut()(*self.unpack_for_uut(mock_values.example_payload)) + + self.assertTrue(ret_val) + self.mock_reconcile_refund.assert_not_called() + self.mock_revoke_line_send.assert_not_called() def test_refund_unsuccessful(self, _ct_client_init: CommercetoolsAPIClientMock, _run_filter_mock): """ diff --git a/commerce_coordinator/apps/commercetools/tests/test_clients.py b/commerce_coordinator/apps/commercetools/tests/test_clients.py index eb9c8b150..0a3662f6e 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_clients.py +++ b/commerce_coordinator/apps/commercetools/tests/test_clients.py @@ -635,6 +635,26 @@ def test_create_refund_transaction(self): self.assertEqual(result.transactions[0].type, mock_response_payment.transactions[0].type) self.assertEqual(result.transactions[0].state, TransactionState.SUCCESS) + def test_change_refund_transaction_state(self): + mock_base_client = MagicMock() + expected_payment = gen_payment() + mock_base_client.payments.update_by_id.return_value = expected_payment + self.client_set.client.base_client = mock_base_client + + result = self.client_set.client.change_refund_transaction_state( + payment_id="payment-1", + payment_version=4, + transaction_id="transaction-1", + state=TransactionState.SUCCESS, + ) + + self.assertEqual(result, expected_payment) + _, kwargs = mock_base_client.payments.update_by_id.call_args + self.assertEqual(kwargs["id"], "payment-1") + self.assertEqual(kwargs["version"], 4) + self.assertEqual(kwargs["actions"][0].transaction_id, "transaction-1") + self.assertEqual(kwargs["actions"][0].state, TransactionState.SUCCESS) + def test_create_refund_transaction_exception(self): base_url = self.client_set.get_base_url_from_client() mock_stripe_refund = stripe.Refund() diff --git a/commerce_coordinator/apps/commercetools/tests/test_pipeline.py b/commerce_coordinator/apps/commercetools/tests/test_pipeline.py index 916f4218f..fd63ec003 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_pipeline.py +++ b/commerce_coordinator/apps/commercetools/tests/test_pipeline.py @@ -299,6 +299,30 @@ def test_pipeline_with_psp_error(self, mock_order_return_update): result_data = ret['returned_order'] self.assertEqual(result_data, self.update_order_response) + @patch( + 'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient' + '.update_return_payment_state_after_successful_refund' + ) + def test_pending_stripe_refund_does_not_transition_return_to_refunded( + self, + mock_order_return_update, + ): + pipe = UpdateCommercetoolsOrderReturnPaymentStatus("test_pipe", None) + mock_order_return_update.return_value = self.update_order_response + + result = pipe.run_filter( + order_data=self.update_order_data, + payment_intent_id="pi_pending", + psp=EDX_STRIPE_PAYMENT_INTERFACE_NAME, + refund_response={"id": "re_pending", "status": "pending"}, + return_line_items={"mock_line_item_id": "mock_return_item_id"}, + refunded_line_item_refunds={}, + return_line_entitlement_ids={}, + ) + + self.assertTrue(result["refund_pending"]) + self.assertFalse(mock_order_return_update.call_args.kwargs["should_transition_state"]) + @patch('commerce_coordinator.apps.commercetools.pipeline.log.info') def test_pipeline_with_free_order(self, mock_logger): """Ensure pipeline is functioning as expected""" diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py new file mode 100644 index 000000000..81599a8b6 --- /dev/null +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -0,0 +1,265 @@ +"""Tests for state-aware Stripe refund reconciliation.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +from commercetools.platform.models import ReturnPaymentState, TransactionState, TransactionType + +from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import reconcile_stripe_refund + + +def _refund(status="succeeded"): + return { + "id": "re_async", + "payment_intent": "pi_async", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": status, + } + + +def _transaction(state): + return SimpleNamespace( + id="txn_refund", + interaction_id="re_async", + type=TransactionType.REFUND, + state=state, + custom=SimpleNamespace(fields={"returnItemId": "return-1"}), + ) + + +def _payment(transaction=None): + return SimpleNamespace( + id="payment-1", + version=3, + transactions=[transaction] if transaction else [], + ) + + +def _order(payment_state=ReturnPaymentState.INITIAL): + return SimpleNamespace( + id="order-1", + order_number="2U-123", + return_info=[ + SimpleNamespace( + items=[ + SimpleNamespace( + id="return-1", + line_item_id="line-1", + payment_state=payment_state, + ) + ] + ) + ], + ) + + +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.release_task_lock") +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.acquire_task_lock", return_value=True) +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile._update_return_state") +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile._emit_segment_refund") +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile._dispatch_revoke") +class TestStripeRefundReconcile: + """Exercise refund state transitions and the shared side-effect gate.""" + + def test_pending_creates_pending_without_terminal_side_effects( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + client.get_payment_by_key.return_value = _payment() + pending_transaction = _transaction(TransactionState.PENDING) + client.create_return_payment_transaction.return_value = _payment(pending_transaction) + client.get_order_by_payment_id.return_value = _order() + + result = reconcile_stripe_refund( + "pi_async", + _refund("pending"), + source="webhook", + client=client, + ) + + client.create_return_payment_transaction.assert_called_once() + mock_update_return.assert_called_once() + assert mock_update_return.call_args.kwargs["should_transition_state"] is False + mock_revoke.assert_not_called() + mock_segment.assert_not_called() + assert result.transaction_state == TransactionState.PENDING + + @pytest.mark.parametrize( + ("stripe_status", "expected_state"), + [ + ("failed", TransactionState.FAILURE), + ("canceled", TransactionState.FAILURE), + ], + ) + def test_failure_updates_pending_and_preserves_access( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + stripe_status, + expected_state, + ): + client = MagicMock() + pending = _transaction(TransactionState.PENDING) + failed = _transaction(TransactionState.FAILURE) + client.get_payment_by_key.return_value = _payment(pending) + client.change_refund_transaction_state.return_value = _payment(failed) + client.get_order_by_payment_id.return_value = _order() + + result = reconcile_stripe_refund( + "pi_async", + _refund(stripe_status), + source="webhook", + client=client, + ) + + client.change_refund_transaction_state.assert_called_once_with( + payment_id="payment-1", + payment_version=3, + transaction_id="txn_refund", + state=expected_state, + ) + assert mock_update_return.call_args.kwargs["payment_state"] == ReturnPaymentState.NOT_REFUNDED + mock_revoke.assert_not_called() + mock_segment.assert_not_called() + assert result.transaction_state == expected_state + + def test_pending_to_success_runs_side_effects_then_records_refunded( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + pending = _transaction(TransactionState.PENDING) + succeeded = _transaction(TransactionState.SUCCESS) + client.get_payment_by_key.return_value = _payment(pending) + client.change_refund_transaction_state.return_value = _payment(succeeded) + client.get_order_by_payment_id.return_value = _order() + + result = reconcile_stripe_refund( + "pi_async", + _refund(), + source="webhook", + client=client, + ) + + mock_revoke.assert_called_once() + mock_segment.assert_called_once() + assert mock_update_return.call_args.kwargs["payment_state"] == ReturnPaymentState.REFUNDED + assert result.side_effects_completed is True + + def test_duplicate_success_uses_refunded_return_as_completion_marker( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + client.get_payment_by_key.return_value = _payment(_transaction(TransactionState.SUCCESS)) + client.get_order_by_payment_id.return_value = _order(ReturnPaymentState.REFUNDED) + + result = reconcile_stripe_refund( + "pi_async", + _refund(), + source="webhook", + client=client, + ) + + client.change_refund_transaction_state.assert_not_called() + mock_revoke.assert_not_called() + mock_segment.assert_not_called() + mock_update_return.assert_not_called() + assert result.side_effects_completed is True + + def test_existing_success_with_incomplete_side_effects_heals( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + client.get_payment_by_key.return_value = _payment(_transaction(TransactionState.SUCCESS)) + client.get_order_by_payment_id.return_value = _order(ReturnPaymentState.INITIAL) + + reconcile_stripe_refund( + "pi_async", + _refund(), + source="forward", + client=client, + ) + + mock_revoke.assert_called_once() + mock_segment.assert_called_once() + mock_update_return.assert_called_once() + + def test_forward_success_then_webhook_does_not_duplicate_side_effects( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + return_order = _order(ReturnPaymentState.INITIAL) + client.get_payment_by_key.return_value = _payment(_transaction(TransactionState.SUCCESS)) + client.get_order_by_payment_id.return_value = return_order + + reconcile_stripe_refund( + "pi_async", + _refund(), + source="forward", + client=client, + ) + return_order.return_info[0].items[0].payment_state = ReturnPaymentState.REFUNDED + reconcile_stripe_refund( + "pi_async", + _refund(), + source="webhook", + client=client, + ) + + mock_revoke.assert_called_once() + mock_segment.assert_called_once() + mock_update_return.assert_called_once() + + def test_unknown_status_does_not_default_to_success( + self, + mock_revoke, + mock_segment, + mock_update_return, + mock_acquire, + _mock_release, + ): + client = MagicMock() + + result = reconcile_stripe_refund( + "pi_async", + _refund("requires_action"), + source="webhook", + client=client, + ) + + client.get_payment_by_key.assert_not_called() + mock_acquire.assert_not_called() + mock_revoke.assert_not_called() + mock_segment.assert_not_called() + mock_update_return.assert_not_called() + assert result.transaction_state is None diff --git a/commerce_coordinator/apps/commercetools/tests/test_tasks.py b/commerce_coordinator/apps/commercetools/tests/test_tasks.py index 2cea4f424..2c86245fb 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_tasks.py +++ b/commerce_coordinator/apps/commercetools/tests/test_tasks.py @@ -4,7 +4,7 @@ import json import logging -from unittest.mock import Mock, call, patch +from unittest.mock import Mock, patch import stripe from commercetools import CommercetoolsError @@ -30,7 +30,6 @@ gen_program_order ) from commerce_coordinator.apps.commercetools.tests.constants import ( - EXAMPLE_RETURNED_ORDER_STRIPE_CLIENT_PAYLOAD, EXAMPLE_RETURNED_ORDER_STRIPE_SIGNAL_PAYLOAD, EXAMPLE_UPDATE_LINE_ITEM_SIGNAL_PAYLOAD ) @@ -96,6 +95,11 @@ def unpack_for_uut(values): def setUp(self): User.objects.create(username='test-user', lms_user_id=4) + reconcile_patcher = patch( + 'commerce_coordinator.apps.commercetools.tasks.reconcile_stripe_refund' + ) + self.mock_reconcile_refund = reconcile_patcher.start() + self.addCleanup(reconcile_patcher.stop) def test_correct_arguments_passed(self, mock_client): ''' @@ -113,10 +117,11 @@ def test_correct_arguments_passed(self, mock_client): _ = returned_uut(*self.unpack_for_uut(EXAMPLE_RETURNED_ORDER_STRIPE_SIGNAL_PAYLOAD)) logger.info('mock_client().mock_calls: %s', mock_client().mock_calls) - mock_client().create_return_payment_transaction.assert_called_once_with( - payment_id=mock_payment.id, - payment_version=mock_payment.version, - refund=mock_stripe_refund + self.mock_reconcile_refund.assert_called_once_with( + EXAMPLE_RETURNED_ORDER_STRIPE_SIGNAL_PAYLOAD['payment_intent_id'], + mock_stripe_refund, + order_number=None, + source="webhook", ) def test_full_refund_already_exists(self, mock_client): @@ -132,22 +137,9 @@ def test_full_refund_already_exists(self, mock_client): mock_client.return_value.get_payment_by_key.return_value = mock_payment - payment_intent_id = EXAMPLE_RETURNED_ORDER_STRIPE_SIGNAL_PAYLOAD['payment_intent_id'] + refund_from_stripe_task(*self.unpack_for_uut(EXAMPLE_RETURNED_ORDER_STRIPE_SIGNAL_PAYLOAD)) - with patch('commerce_coordinator.apps.commercetools.tasks.logger') as mock_logger: - result = refund_from_stripe_task(*self.unpack_for_uut(EXAMPLE_RETURNED_ORDER_STRIPE_SIGNAL_PAYLOAD)) - self.assertIsNone(result) - - # Check that both info messages were logged in the expected order - mock_logger.info.assert_has_calls([ - call( - f"[refund_from_stripe_task] " - f"Initiating creation of CT payment's refund transaction object " - f"for payment Intent ID {payment_intent_id}."), - call(f"[refund_from_stripe_task] Event 'charge.refunded' received, " - f"but Payment with ID {mock_payment.id} " - f"already has a full refund. Skipping task to add refund transaction") - ]) + self.mock_reconcile_refund.assert_called_once() @patch('commerce_coordinator.apps.commercetools.tasks.logger') def test_exception_handling(self, mock_logger, mock_client): @@ -158,7 +150,7 @@ def test_exception_handling(self, mock_logger, mock_client): mock_payment = gen_payment() mock_payment.id = 'f988e0c5-ea44-4111-a7f2-39ecf6af9840' mock_client.return_value.get_payment_by_key.return_value = mock_payment - mock_client().create_return_payment_transaction.side_effect = CommercetoolsError( + self.mock_reconcile_refund.side_effect = CommercetoolsError( message="Could not create return transaction", errors="Some error message", response={}, @@ -168,12 +160,7 @@ def test_exception_handling(self, mock_logger, mock_client): with self.assertRaises(CommercetoolsError): returned_uut(*self.unpack_for_uut(EXAMPLE_RETURNED_ORDER_STRIPE_SIGNAL_PAYLOAD)) - mock_logger.error.assert_called_once_with( - f"[refund_from_stripe_task] Unable to create CT payment's refund transaction " - f"object for [ {mock_payment.id} ] " - f"on Stripe refund {EXAMPLE_RETURNED_ORDER_STRIPE_CLIENT_PAYLOAD['stripe_refund']['id']} " - f"with error Some error message and correlation id 123456" - ) + mock_logger.error.assert_not_called() @patch("commerce_coordinator.apps.commercetools.tasks.CommercetoolsAPIClient") diff --git a/commerce_coordinator/apps/commercetools/tests/test_utils.py b/commerce_coordinator/apps/commercetools/tests/test_utils.py index 3967b6538..640ada2eb 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_utils.py +++ b/commerce_coordinator/apps/commercetools/tests/test_utils.py @@ -366,6 +366,17 @@ def test_has_partial_refund_transaction(self): payment = gen_payment_with_multiple_transactions(TransactionType.CHARGE, 4900, TransactionType.REFUND, 2500) self.assertFalse(has_full_refund_transaction(payment)) + def test_pending_refund_does_not_count_as_fully_refunded(self): + payment = gen_payment_with_multiple_transactions( + TransactionType.CHARGE, + 4900, + TransactionType.REFUND, + 4900, + ) + payment.transactions[-1].state = TransactionState.PENDING + + self.assertFalse(has_full_refund_transaction(payment)) + def test_has_no_refund_transaction(self): payment = gen_payment_with_multiple_transactions(TransactionType.CHARGE, 4900) self.assertFalse(has_full_refund_transaction(payment)) @@ -429,6 +440,9 @@ def test_translate_stripe_refund_status_pending(self): def test_translate_stripe_refund_status_failed(self): self.assertEqual(translate_refund_status_to_transaction_status('failed'), TransactionState.FAILURE) + def test_translate_stripe_refund_status_canceled(self): + self.assertEqual(translate_refund_status_to_transaction_status('canceled'), TransactionState.FAILURE) + def test_translate_stripe_refund_status_other(self): # Test for an unknown status self.assertEqual(translate_refund_status_to_transaction_status('unknown_status'), TransactionState.SUCCESS) diff --git a/commerce_coordinator/apps/commercetools/utils.py b/commerce_coordinator/apps/commercetools/utils.py index db95dd533..d65d38870 100644 --- a/commerce_coordinator/apps/commercetools/utils.py +++ b/commerce_coordinator/apps/commercetools/utils.py @@ -282,7 +282,10 @@ def has_full_refund_transaction(payment: Payment): for transaction in payment.transactions: if transaction.type == TransactionType.CHARGE: charge_amount += cents_to_dollars(transaction.amount) - if transaction.type == TransactionType.REFUND: # pragma no cover + if ( + transaction.type == TransactionType.REFUND + and transaction.state == TransactionState.SUCCESS + ): # pragma no cover refunded_amount += cents_to_dollars(transaction.amount) return refunded_amount == charge_amount @@ -299,6 +302,19 @@ def is_transaction_already_refunded(payment: Payment, psp_refund_transaction_id: return False +def get_refund_transaction_by_interaction_id(payment: Payment, interaction_id: str): + """Return the refund transaction matching a PSP refund identifier.""" + return next( + ( + transaction + for transaction in payment.transactions + if transaction.type == TransactionType.REFUND + and transaction.interaction_id == interaction_id + ), + None, + ) + + def find_refund_transaction(payment: Payment, psp_refund_transaction_id: str): """ Utility to find the refund transaction in a payment diff --git a/commerce_coordinator/apps/stripe/clients.py b/commerce_coordinator/apps/stripe/clients.py index 70904a3be..714a7efdd 100644 --- a/commerce_coordinator/apps/stripe/clients.py +++ b/commerce_coordinator/apps/stripe/clients.py @@ -305,7 +305,11 @@ def refund_payment_intent( logger.exception(msg) raise err - if refund.status != StripeRefundStatus.REFUND_SUCCESS: + accepted_statuses = { + StripeRefundStatus.REFUND_SUCCESS, + StripeRefundStatus.REFUND_PENDING, + } + if refund.status not in accepted_statuses: logger.exception('Refund for order [%s] was unsuccessful', order_uuid) return None diff --git a/commerce_coordinator/apps/stripe/constants.py b/commerce_coordinator/apps/stripe/constants.py index 8bf2d5a63..13094066f 100644 --- a/commerce_coordinator/apps/stripe/constants.py +++ b/commerce_coordinator/apps/stripe/constants.py @@ -9,6 +9,8 @@ class StripeEventType(str, Enum): PAYMENT_SUCCESS = 'payment_intent.succeeded' PAYMENT_FAILED = 'payment_intent.payment_failed' PAYMENT_REFUNDED = 'charge.refunded' + REFUND_UPDATED = 'refund.updated' + REFUND_FAILED = 'refund.failed' class Currency(str, Enum): @@ -21,3 +23,6 @@ class StripeErrorCode(str, Enum): class StripeRefundStatus(str, Enum): REFUND_SUCCESS = 'succeeded' + REFUND_PENDING = 'pending' + REFUND_FAILED = 'failed' + REFUND_CANCELED = 'canceled' diff --git a/commerce_coordinator/apps/stripe/tests/test_clients.py b/commerce_coordinator/apps/stripe/tests/test_clients.py index df9430b68..f4682c068 100644 --- a/commerce_coordinator/apps/stripe/tests/test_clients.py +++ b/commerce_coordinator/apps/stripe/tests/test_clients.py @@ -457,6 +457,30 @@ def test_refund_payment_intent_returns_none(self): expected_output=None ) + def test_refund_payment_intent_returns_pending_refund(self): + self.assertJSONClientResponse( + uut=self.client.refund_payment_intent, + input_kwargs={ + 'order_uuid': '123', + 'payment_intent_id': TEST_PAYMENT_INTENT_ID, + 'amount': 100 + }, + expected_request={ + 'payment_intent': [TEST_PAYMENT_INTENT_ID], + 'amount': ['10000'] + }, + request_type='query_string', + mock_url='https://api.stripe.com/v1/refunds', + mock_response={ + 'id': 'mock_pending_id', + 'status': 'pending' + }, + expected_output={ + 'id': 'mock_pending_id', + 'status': 'pending' + } + ) + def test_refund_payment_intent_error(self): with self.assertRaises(stripe.error.APIError): self.assertJSONClientResponse( diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index 2bd9e434c..f06bd83da 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -303,6 +303,7 @@ def test_payment_refunded_event( mock_construct_event.return_value = self.mock_stripe_event mock_is_legacy.return_value = is_legacy_order mock_is_ct_refund.return_value = is_ct_order + mock_refund_task.return_value = [(lambda **kwargs: None, 'celery-task-id')] response = self.client.post(self.url, data=body, format='json', **self.mock_header) self.assertEqual(response.status_code, expected_status) @@ -360,6 +361,7 @@ def test_refund_falls_back_to_event_id_when_idempotency_key_missing( 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 + mock_refund_task.return_value = [(lambda **kwargs: None, 'celery-task-id')] response = self.client.post(self.url, data={}, format='json', **self.mock_header) @@ -369,3 +371,182 @@ def test_refund_falls_back_to_event_id_when_idempotency_key_missing( mock_refund_task.assert_called_once() mock_is_ct_refund.assert_called() mock_is_legacy.assert_called() + + @ddt.data( + (StripeEventType.REFUND_UPDATED.value, "succeeded"), + (StripeEventType.REFUND_FAILED.value, "failed"), + ) + @ddt.unpack + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.release_task_lock') + @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=True) + @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + def test_refund_object_events_route_to_reconciler( + self, + event_type, + refund_status, + mock_refund_signal, + mock_ct_client, + _mock_acquire, + _mock_release, + mock_construct_event, + ): + refund = StripeObject() + refund.update({ + "id": "re_async", + "payment_intent": "pi_async", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": refund_status, + }) + self.mock_stripe_event.id = f"evt_refund_object_{refund_status}" + self.mock_stripe_event.type = event_type + self.mock_stripe_event.data.object = refund + mock_construct_event.return_value = self.mock_stripe_event + mock_refund_signal.return_value = [(lambda **kwargs: None, "celery-task-id")] + payment = mock_ct_client.return_value.get_payment_by_key.return_value + payment.id = "payment-1" + payment.payment_method_info.payment_interface = "stripe_edx" + mock_ct_client.return_value.get_order_by_payment_id.return_value.order_number = "2U-123" + + response = self.client.post(self.url, data={}, format='json', **self.mock_header) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_refund_signal.assert_called_once_with( + sender=WebhookView, + payment_intent_id="pi_async", + stripe_refund=dict(refund), + order_number="2U-123", + ) + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.release_task_lock') + @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=True) + @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + def test_pending_event_does_not_suppress_later_terminal_event( + self, + mock_refund_signal, + mock_ct_client, + _mock_acquire, + _mock_release, + mock_construct_event, + ): + payment = mock_ct_client.return_value.get_payment_by_key.return_value + payment.id = "payment-1" + payment.payment_method_info.payment_interface = "stripe_edx" + mock_ct_client.return_value.get_order_by_payment_id.return_value.order_number = "2U-123" + mock_refund_signal.return_value = [(lambda **kwargs: None, "celery-task-id")] + + def _event(event_id, refund_status): + refund = StripeObject() + refund.update({ + "id": "re_same", + "payment_intent": "pi_same", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": refund_status, + }) + event = mock.Mock() + event.id = event_id + event.type = StripeEventType.REFUND_UPDATED.value + event.data.object = refund + return event + + mock_construct_event.side_effect = [ + _event("evt_pending", "pending"), + _event("evt_succeeded", "succeeded"), + ] + + first = self.client.post(self.url, data={}, format='json', **self.mock_header) + second = self.client.post(self.url, data={}, format='json', **self.mock_header) + + self.assertEqual(first.status_code, status.HTTP_200_OK) + self.assertEqual(second.status_code, status.HTTP_200_OK) + self.assertEqual(mock_refund_signal.call_count, 2) + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=False) + @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') + def test_refund_lock_contention_returns_503( + self, + mock_ct_client, + _mock_acquire, + mock_construct_event, + ): + refund = StripeObject() + refund.update({ + "id": "re_locked", + "payment_intent": "pi_locked", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": "succeeded", + }) + self.mock_stripe_event.id = "evt_locked" + self.mock_stripe_event.type = StripeEventType.REFUND_UPDATED.value + self.mock_stripe_event.data.object = refund + mock_construct_event.return_value = self.mock_stripe_event + payment = mock_ct_client.return_value.get_payment_by_key.return_value + payment.id = "payment-1" + payment.payment_method_info.payment_interface = "stripe_edx" + mock_ct_client.return_value.get_order_by_payment_id.return_value.order_number = "2U-123" + + 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( # pylint: disable=protected-access + WebhookView.__name__, + "evt_locked", + ) + ) + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.release_task_lock') + @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=True) + @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + def test_refund_dispatch_failure_returns_503_and_clears_event_key( + self, + mock_refund_signal, + mock_ct_client, + _mock_acquire, + _mock_release, + mock_construct_event, + ): + refund = StripeObject() + refund.update({ + "id": "re_dispatch_failure", + "payment_intent": "pi_dispatch_failure", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": "succeeded", + }) + self.mock_stripe_event.id = "evt_refund_dispatch_failure" + self.mock_stripe_event.type = StripeEventType.REFUND_UPDATED.value + self.mock_stripe_event.data.object = refund + mock_construct_event.return_value = self.mock_stripe_event + payment = mock_ct_client.return_value.get_payment_by_key.return_value + payment.id = "payment-1" + payment.payment_method_info.payment_interface = "stripe_edx" + mock_ct_client.return_value.get_order_by_payment_id.return_value.order_number = "2U-123" + + def _receiver(**kwargs): + pass + + mock_refund_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( # pylint: disable=protected-access + WebhookView.__name__, + "evt_refund_dispatch_failure", + ) + ) diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 034607461..558abff4e 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -10,8 +10,15 @@ from rest_framework.permissions import AllowAny from rest_framework.response import Response +from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME +from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient +from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( + REFUND_RECONCILE_LOCK_EXPIRE, + refund_reconcile_lock_key +) from commerce_coordinator.apps.core.constants import PaymentState from commerce_coordinator.apps.core.signal_helpers import format_signal_results +from commerce_coordinator.apps.core.tasks import acquire_task_lock, release_task_lock 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 @@ -76,7 +83,11 @@ def post(self, request): return self._handle_legacy_payment_event(event, payment_intent, event_source_system, payload) - if event.type == StripeEventType.PAYMENT_REFUNDED.value: + if event.type in { + StripeEventType.PAYMENT_REFUNDED.value, + StripeEventType.REFUND_UPDATED.value, + StripeEventType.REFUND_FAILED.value, + }: return self._handle_refund_event(tag, event) raise UnhandledStripeEventAPIError @@ -158,49 +169,77 @@ 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.""" - 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 + event_id = getattr(event, 'id', None) + if self._is_running(tag, event_id): # pragma no cover self.meta_should_mark_not_running = False return Response(status=status.HTTP_200_OK) - self.mark_running(tag, invocation_key) + self.mark_running(tag, event_id) 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 event.type == StripeEventType.PAYMENT_REFUNDED.value: + 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 is_legacy_order_check or not is_ct_order_check: + 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) - 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']) + stripe_refund = max(refunds, key=lambda refund: refund['created']) + else: + stripe_refund = dict(event_object) + payment_intent_id = event_object.payment_intent + client = CommercetoolsAPIClient() + payment = client.get_payment_by_key(payment_intent_id) + payment_interface = getattr(payment.payment_method_info, 'payment_interface', None) + if payment_interface != EDX_STRIPE_PAYMENT_INTERFACE_NAME: + logger.info( + '[Stripe webhooks] skipping refund event %s for non-CT payment intent [%s].', + event.type, + payment_intent_id, + ) + return Response(status=status.HTTP_200_OK) + try: + order = client.get_order_by_payment_id(payment.id) + order_number = order.order_number + except ValueError: + order_number = None - 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, + logger.info( + '[Stripe webhooks] refund event %s with event ID [%s], refund ID [%s], ' + 'payment intent ID [%s], and order number [%s].', + event.type, + event_id, + stripe_refund['id'], + payment_intent_id, + order_number, + ) + + lock_key = refund_reconcile_lock_key(stripe_refund['id']) + if not acquire_task_lock(lock_key, REFUND_RECONCILE_LOCK_EXPIRE): + logger.warning( + '[Stripe webhooks] refund %s is already reconciling; returning retryable failure', + stripe_refund['id'], ) + raise StripeWebhookDispatchAPIError - payment_refunded_signal.send_robust( + try: + results = payment_refunded_signal.send_robust( sender=self.__class__, payment_intent_id=payment_intent_id, - stripe_refund=latest_refund, + stripe_refund=stripe_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, - ) + self._assert_signal_dispatched(results, payment_intent_id=payment_intent_id) + finally: + release_task_lock(lock_key) + return Response(status=status.HTTP_200_OK) From f3ae6f341d26f5824751b8f4dffd2a4735c16e20 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Fri, 21 Aug 2026 15:18:26 -0400 Subject: [PATCH 02/11] fix(commercetools): close refund side-effect retry window (EDUN-15350) Persist CT REFUNDED before LMS/Segment, emit Order Refunded with a stable Stripe refund message_id, and autoretry reconciler errors on the forward return task. Co-authored-by: Cursor --- .../commercetools/stripe_refund_reconcile.py | 23 ++-- .../apps/commercetools/sub_messages/tasks.py | 16 ++- .../tests/sub_messages/test_tasks.py | 26 +++++ .../tests/test_stripe_refund_reconcile.py | 110 ++++++++++++++++-- 4 files changed, 152 insertions(+), 23 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py index 4cb1782b2..3ec596aa0 100644 --- a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py @@ -215,24 +215,18 @@ def _reconcile_stripe_refund_locked( ) return result - if _all_in_payment_state(return_items, ReturnPaymentState.REFUNDED): - logger.info( - "[stripe_refund_reconcile] Refund %s side effects already recorded by CT return state", - refund_id, + if not _all_in_payment_state(return_items, ReturnPaymentState.REFUNDED): + _update_return_state( + client, + order, + payment, + return_items, + stripe_refund, + payment_state=ReturnPaymentState.REFUNDED, ) - result.side_effects_completed = True - return result _dispatch_revoke(order.id, return_items) _emit_segment_refund(client, order, stripe_refund, return_items) - _update_return_state( - client, - order, - payment, - return_items, - stripe_refund, - payment_state=ReturnPaymentState.REFUNDED, - ) result.side_effects_completed = True logger.info( "[stripe_refund_reconcile] Refund %s completed lms_revoke=true segment_emitted=true", @@ -351,6 +345,7 @@ def _emit_segment_refund(client, order, stripe_refund: Refund, return_items) -> lms_user_id=lms_user_id, event="Order Refunded", properties=properties, + message_id=stripe_refund["id"], ) diff --git a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py index 172892329..ec72c9989 100644 --- a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py +++ b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py @@ -43,7 +43,11 @@ fulfill_order_placed_send_entitlement_signal, fulfill_order_returned_send_revoke_line_items_signal ) -from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import reconcile_stripe_refund +from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( + RefundReconcileInProgressError, + RefundSideEffectDispatchError, + reconcile_stripe_refund, +) from commerce_coordinator.apps.commercetools.utils import ( extract_ct_order_information_for_braze_canvas, extract_ct_product_information_for_braze_canvas, @@ -300,7 +304,15 @@ def fulfill_order_sanctioned_message_signal_task( # noinspection DuplicatedCode -@shared_task(autoretry_for=(RequestException, CommercetoolsError), retry_kwargs={'max_retries': 5, 'countdown': 3}) +@shared_task( + autoretry_for=( + RequestException, + CommercetoolsError, + RefundReconcileInProgressError, + RefundSideEffectDispatchError, + ), + retry_kwargs={'max_retries': 5, 'countdown': 3}, +) def fulfill_order_returned_signal_task(order_id, return_items, message_id): """Celery task for an order return (and refunded) message.""" # pylint: disable=too-many-statements diff --git a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py index 8d0adc37b..0082c310b 100644 --- a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py +++ b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py @@ -17,6 +17,10 @@ fulfill_order_returned_signal_task, fulfill_order_sanctioned_message_signal_task ) +from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( + RefundReconcileInProgressError, + RefundSideEffectDispatchError, +) from commerce_coordinator.apps.commercetools.tests.conftest import MonkeyPatch, gen_return_item from commerce_coordinator.apps.commercetools.tests.mocks import ( CTCustomerByIdMock, @@ -830,6 +834,28 @@ def test_pending_refund_skips_revoke_segment_and_reconcile( self.mock_reconcile_refund.assert_not_called() self.mock_revoke_line_send.assert_not_called() + def test_forward_succeeded_retries_reconciler_errors( + self, + _ct_client_init: CommercetoolsAPIClientMock, + _run_filter_mock, + ): + self.assertIn(RefundReconcileInProgressError, fulfill_order_returned_signal_task.autoretry_for) + self.assertIn(RefundSideEffectDispatchError, fulfill_order_returned_signal_task.autoretry_for) + + mock_values = _ct_client_init.return_value + _run_filter_mock.return_value = { + 'refund_response': { + 'id': 're_succeeded', + 'payment_intent': 'pi_succeeded', + 'status': 'succeeded', + }, + 'psp': EDX_STRIPE_PAYMENT_INTERFACE_NAME, + } + self.mock_reconcile_refund.side_effect = RefundReconcileInProgressError("locked") + + with self.assertRaises(RefundReconcileInProgressError): + self.get_uut()(*self.unpack_for_uut(mock_values.example_payload)) + def test_refund_unsuccessful(self, _ct_client_init: CommercetoolsAPIClientMock, _run_filter_mock): """ Check calling uut when refund is unsuccessful. diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py index 81599a8b6..ef9b29d7e 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -134,7 +134,7 @@ def test_failure_updates_pending_and_preserves_access( mock_segment.assert_not_called() assert result.transaction_state == expected_state - def test_pending_to_success_runs_side_effects_then_records_refunded( + def test_pending_to_success_records_refunded_before_side_effects( self, mock_revoke, mock_segment, @@ -148,6 +148,10 @@ def test_pending_to_success_runs_side_effects_then_records_refunded( client.get_payment_by_key.return_value = _payment(pending) client.change_refund_transaction_state.return_value = _payment(succeeded) client.get_order_by_payment_id.return_value = _order() + call_order = [] + mock_update_return.side_effect = lambda *args, **kwargs: call_order.append("update") + mock_revoke.side_effect = lambda *args, **kwargs: call_order.append("revoke") + mock_segment.side_effect = lambda *args, **kwargs: call_order.append("segment") result = reconcile_stripe_refund( "pi_async", @@ -156,12 +160,50 @@ def test_pending_to_success_runs_side_effects_then_records_refunded( client=client, ) + assert call_order == ["update", "revoke", "segment"] + assert mock_update_return.call_args.kwargs["payment_state"] == ReturnPaymentState.REFUNDED + assert result.side_effects_completed is True + + def test_return_update_failure_does_not_emit_side_effects( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + pending = _transaction(TransactionState.PENDING) + succeeded = _transaction(TransactionState.SUCCESS) + client.get_payment_by_key.return_value = _payment(pending) + client.change_refund_transaction_state.return_value = _payment(succeeded) + client.get_order_by_payment_id.return_value = _order() + mock_update_return.side_effect = RuntimeError("ct write failed") + + with pytest.raises(RuntimeError): + reconcile_stripe_refund( + "pi_async", + _refund(), + source="webhook", + client=client, + ) + + mock_revoke.assert_not_called() + mock_segment.assert_not_called() + + mock_update_return.side_effect = None + result = reconcile_stripe_refund( + "pi_async", + _refund(), + source="webhook", + client=client, + ) + mock_revoke.assert_called_once() mock_segment.assert_called_once() - assert mock_update_return.call_args.kwargs["payment_state"] == ReturnPaymentState.REFUNDED assert result.side_effects_completed is True - def test_duplicate_success_uses_refunded_return_as_completion_marker( + def test_already_refunded_retries_idempotent_side_effects( self, mock_revoke, mock_segment, @@ -181,9 +223,9 @@ def test_duplicate_success_uses_refunded_return_as_completion_marker( ) client.change_refund_transaction_state.assert_not_called() - mock_revoke.assert_not_called() - mock_segment.assert_not_called() mock_update_return.assert_not_called() + mock_revoke.assert_called_once() + mock_segment.assert_called_once() assert result.side_effects_completed is True def test_existing_success_with_incomplete_side_effects_heals( @@ -236,8 +278,8 @@ def test_forward_success_then_webhook_does_not_duplicate_side_effects( client=client, ) - mock_revoke.assert_called_once() - mock_segment.assert_called_once() + assert mock_revoke.call_count == 2 + assert mock_segment.call_count == 2 mock_update_return.assert_called_once() def test_unknown_status_does_not_default_to_success( @@ -263,3 +305,57 @@ def test_unknown_status_does_not_default_to_success( mock_segment.assert_not_called() mock_update_return.assert_not_called() assert result.transaction_state is None + + +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.track") +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.get_edx_lms_user_id", return_value="lms-1") +def test_segment_uses_stripe_refund_id_as_message_id(_mock_lms_id, mock_track): + client = MagicMock() + client.get_customer_by_id.return_value = SimpleNamespace(id="cust-1") + order = SimpleNamespace( + id="order-1", + customer_id="cust-1", + line_items=[ + SimpleNamespace( + id="line-1", + name={"en-US": "Course"}, + product_key="course-1", + product_type=SimpleNamespace(obj=SimpleNamespace(name="course")), + variant=SimpleNamespace(sku="sku", images=[], attributes=[]), + price=SimpleNamespace(value=SimpleNamespace(cent_amount=4900, currency_code="USD", fraction_digits=2)), + quantity=1, + ) + ], + return_info=[], + custom=None, + total_price=SimpleNamespace(cent_amount=4900, currency_code="USD"), + taxed_price=None, + discount_on_total_price=None, + discount_codes=[], + payment_info=None, + ) + from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import _emit_segment_refund + + with patch( + "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.prepare_segment_event_properties", + return_value={"products": []}, + ) as mock_props: + mock_props.return_value = {"products": [{"name": "Course"}]} + with patch( + "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.get_product_data", + return_value={"name": "Course"}, + ): + with patch( + "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.check_is_bundle", + return_value=False, + ): + _emit_segment_refund( + client, + order, + _refund(), + [SimpleNamespace(id="return-1", line_item_id="line-1")], + ) + + mock_track.assert_called_once() + assert mock_track.call_args.kwargs["message_id"] == "re_async" + assert mock_track.call_args.kwargs["event"] == "Order Refunded" From de9a0bcca8dcccfc31bbfcd7ef5bf9ff25c6f035 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Mon, 24 Aug 2026 10:17:48 -0400 Subject: [PATCH 03/11] style(commercetools): import refund helper at module top (EDUN-15350) Co-authored-by: Cursor --- .../commercetools/tests/test_stripe_refund_reconcile.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py index ef9b29d7e..1ae2c0499 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -6,7 +6,10 @@ import pytest from commercetools.platform.models import ReturnPaymentState, TransactionState, TransactionType -from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import reconcile_stripe_refund +from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( + _emit_segment_refund, + reconcile_stripe_refund, +) def _refund(status="succeeded"): @@ -334,7 +337,6 @@ def test_segment_uses_stripe_refund_id_as_message_id(_mock_lms_id, mock_track): discount_codes=[], payment_info=None, ) - from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import _emit_segment_refund with patch( "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.prepare_segment_event_properties", From fd84157252b8fb010ffa3bbb0e5bf258f866168c Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Mon, 24 Aug 2026 10:23:20 -0400 Subject: [PATCH 04/11] style(commercetools): fix isort on refund reconcile imports (EDUN-15350) Co-authored-by: Cursor --- .../apps/commercetools/sub_messages/tasks.py | 2 +- .../apps/commercetools/tests/sub_messages/test_tasks.py | 8 ++++---- .../commercetools/tests/test_stripe_refund_reconcile.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py index ec72c9989..7e7075ed6 100644 --- a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py +++ b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py @@ -46,7 +46,7 @@ from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( RefundReconcileInProgressError, RefundSideEffectDispatchError, - reconcile_stripe_refund, + reconcile_stripe_refund ) from commerce_coordinator.apps.commercetools.utils import ( extract_ct_order_information_for_braze_canvas, diff --git a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py index 0082c310b..2e798332e 100644 --- a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py +++ b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py @@ -12,15 +12,15 @@ from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME, TwoUKeys from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient from commerce_coordinator.apps.commercetools.constants import SOURCE_SYSTEM +from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( + RefundReconcileInProgressError, + RefundSideEffectDispatchError +) from commerce_coordinator.apps.commercetools.sub_messages.tasks import ( fulfill_order_placed_message_signal_task, fulfill_order_returned_signal_task, fulfill_order_sanctioned_message_signal_task ) -from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( - RefundReconcileInProgressError, - RefundSideEffectDispatchError, -) from commerce_coordinator.apps.commercetools.tests.conftest import MonkeyPatch, gen_return_item from commerce_coordinator.apps.commercetools.tests.mocks import ( CTCustomerByIdMock, diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py index 1ae2c0499..7cd860909 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -8,7 +8,7 @@ from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( _emit_segment_refund, - reconcile_stripe_refund, + reconcile_stripe_refund ) From ad37d620392384b077d5b8eccbb3258cb5381d08 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Mon, 24 Aug 2026 10:39:24 -0400 Subject: [PATCH 05/11] fix(commercetools): harden refund webhook idempotency (EDUN-15350) Reject refund events without Stripe IDs, compare raw refund status values explicitly, and cache per-refund side-effect completion so retries heal partial work without replaying completed effects. --- .../commercetools/stripe_refund_reconcile.py | 43 +++++++++++++++- .../tests/test_stripe_refund_reconcile.py | 51 +++++++++++++++++-- commerce_coordinator/apps/core/views.py | 3 +- commerce_coordinator/apps/stripe/clients.py | 4 +- .../apps/stripe/tests/test_views.py | 20 ++++++++ commerce_coordinator/apps/stripe/views.py | 4 ++ 6 files changed, 117 insertions(+), 8 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py index 3ec596aa0..fc32a5c1f 100644 --- a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py @@ -5,6 +5,7 @@ from commercetools.platform.models import ReturnPaymentState, TransactionState from django.utils.module_loading import import_string +from edx_django_utils.cache import TieredCache from iso4217 import Currency from commerce_coordinator.apps.commercetools.catalog_info.edx_utils import check_is_bundle, get_edx_lms_user_id @@ -25,6 +26,8 @@ REFUND_RECONCILE_LOCK_PREFIX = "reconcile_stripe_refund" REFUND_RECONCILE_LOCK_EXPIRE = 300 +REFUND_SIDE_EFFECT_CACHE_PREFIX = "stripe_refund_side_effect" +REFUND_SIDE_EFFECT_CACHE_TTL_SECS = 60 * 60 * 24 * 7 STRIPE_TO_CT_STATE = { StripeRefundStatus.REFUND_PENDING.value: TransactionState.PENDING, @@ -215,6 +218,8 @@ def _reconcile_stripe_refund_locked( ) return result + revoke_completed = _side_effect_completed(refund_id, "lms_revoke") + segment_completed = _side_effect_completed(refund_id, "segment") if not _all_in_payment_state(return_items, ReturnPaymentState.REFUNDED): _update_return_state( client, @@ -225,8 +230,20 @@ def _reconcile_stripe_refund_locked( payment_state=ReturnPaymentState.REFUNDED, ) - _dispatch_revoke(order.id, return_items) - _emit_segment_refund(client, order, stripe_refund, return_items) + if revoke_completed and segment_completed: + logger.info( + "[stripe_refund_reconcile] Refund %s side effects already completed", + refund_id, + ) + result.side_effects_completed = True + return result + + if not revoke_completed: + _dispatch_revoke(order.id, return_items) + _mark_side_effect_completed(refund_id, "lms_revoke") + if not segment_completed: + _emit_segment_refund(client, order, stripe_refund, return_items) + _mark_side_effect_completed(refund_id, "segment") result.side_effects_completed = True logger.info( "[stripe_refund_reconcile] Refund %s completed lms_revoke=true segment_emitted=true", @@ -288,6 +305,28 @@ def _all_in_payment_state(return_items, payment_state: ReturnPaymentState) -> bo return all(item.payment_state == payment_state for item in return_items) +def _side_effect_cache_key(refund_id: str, side_effect: str) -> str: + return safe_key( + key=f"{refund_id}_{side_effect}", + key_prefix=REFUND_SIDE_EFFECT_CACHE_PREFIX, + version="1", + ) + + +def _side_effect_completed(refund_id: str, side_effect: str) -> bool: + cache_key = _side_effect_cache_key(refund_id, side_effect) + return TieredCache.get_cached_response(cache_key).is_found + + +def _mark_side_effect_completed(refund_id: str, side_effect: str) -> None: + cache_key = _side_effect_cache_key(refund_id, side_effect) + TieredCache.set_all_tiers( + cache_key, + value="COMPLETED", + django_cache_timeout=REFUND_SIDE_EFFECT_CACHE_TTL_SECS, + ) + + def _return_item_payload(return_items) -> list[dict]: return [ {"id": item.id, "lineItemId": item.line_item_id} diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py index 7cd860909..a4bf7551a 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -5,9 +5,11 @@ import pytest from commercetools.platform.models import ReturnPaymentState, TransactionState, TransactionType +from edx_django_utils.cache import TieredCache from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( _emit_segment_refund, + _side_effect_cache_key, reconcile_stripe_refund ) @@ -59,6 +61,20 @@ def _order(payment_state=ReturnPaymentState.INITIAL): ) +@pytest.fixture(autouse=True) +def clear_refund_side_effect_cache(): + """Keep per-refund completion markers isolated between tests.""" + cache_keys = [ + _side_effect_cache_key("re_async", "lms_revoke"), + _side_effect_cache_key("re_async", "segment"), + ] + for cache_key in cache_keys: + TieredCache.delete_all_tiers(cache_key) + yield + for cache_key in cache_keys: + TieredCache.delete_all_tiers(cache_key) + + @patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.release_task_lock") @patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.acquire_task_lock", return_value=True) @patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile._update_return_state") @@ -206,7 +222,7 @@ def test_return_update_failure_does_not_emit_side_effects( mock_segment.assert_called_once() assert result.side_effects_completed is True - def test_already_refunded_retries_idempotent_side_effects( + def test_already_refunded_dispatches_unmarked_side_effects( self, mock_revoke, mock_segment, @@ -281,10 +297,39 @@ def test_forward_success_then_webhook_does_not_duplicate_side_effects( client=client, ) - assert mock_revoke.call_count == 2 - assert mock_segment.call_count == 2 + mock_revoke.assert_called_once() + mock_segment.assert_called_once() mock_update_return.assert_called_once() + def test_partial_side_effect_completion_only_heals_missing_effect( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + client.get_payment_by_key.return_value = _payment(_transaction(TransactionState.SUCCESS)) + client.get_order_by_payment_id.return_value = _order(ReturnPaymentState.REFUNDED) + TieredCache.set_all_tiers( + _side_effect_cache_key("re_async", "lms_revoke"), + value="COMPLETED", + django_cache_timeout=60, + ) + + result = reconcile_stripe_refund( + "pi_async", + _refund(), + source="webhook", + client=client, + ) + + mock_revoke.assert_not_called() + mock_segment.assert_called_once() + mock_update_return.assert_not_called() + assert result.side_effects_completed is True + def test_unknown_status_does_not_default_to_success( self, mock_revoke, diff --git a/commerce_coordinator/apps/core/views.py b/commerce_coordinator/apps/core/views.py index 1f97331f8..5e661cc13 100644 --- a/commerce_coordinator/apps/core/views.py +++ b/commerce_coordinator/apps/core/views.py @@ -154,5 +154,6 @@ def handle_exception(self, exc): """Mark view as not running on exception""" tag = self.meta_view identifier = self.meta_id - self.mark_running(tag, identifier, False) + if tag is not None and identifier is not None: + self.mark_running(tag, identifier, False) return super().handle_exception(exc) diff --git a/commerce_coordinator/apps/stripe/clients.py b/commerce_coordinator/apps/stripe/clients.py index 714a7efdd..946f2952f 100644 --- a/commerce_coordinator/apps/stripe/clients.py +++ b/commerce_coordinator/apps/stripe/clients.py @@ -306,8 +306,8 @@ def refund_payment_intent( raise err accepted_statuses = { - StripeRefundStatus.REFUND_SUCCESS, - StripeRefundStatus.REFUND_PENDING, + StripeRefundStatus.REFUND_SUCCESS.value, + StripeRefundStatus.REFUND_PENDING.value, } if refund.status not in accepted_statuses: logger.exception('Refund for order [%s] was unsuccessful', order_uuid) diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index f06bd83da..7d62d9bc8 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -372,6 +372,26 @@ def test_refund_falls_back_to_event_id_when_idempotency_key_missing( mock_is_ct_refund.assert_called() mock_is_legacy.assert_called() + @mock.patch('stripe.Webhook.construct_event') + @mock.patch.object(WebhookView, 'mark_running') + @mock.patch.object(WebhookView, '_is_running') + def test_refund_event_without_event_id_returns_400_without_cache_key( + self, + mock_is_running, + mock_mark_running, + mock_construct_event, + ): + """A malformed event must not use a shared None single-invocation key.""" + self.mock_stripe_event.id = None + self.mock_stripe_event.type = StripeEventType.REFUND_UPDATED.value + 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_400_BAD_REQUEST) + mock_is_running.assert_not_called() + mock_mark_running.assert_not_called() + @ddt.data( (StripeEventType.REFUND_UPDATED.value, "succeeded"), (StripeEventType.REFUND_FAILED.value, "failed"), diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 558abff4e..3487665ab 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -170,6 +170,10 @@ 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.""" event_id = getattr(event, 'id', None) + if not event_id: + logger.error('[Stripe webhooks] refund event is missing its Stripe event ID') + raise InvalidPayloadAPIError + if self._is_running(tag, event_id): # pragma no cover self.meta_should_mark_not_running = False return Response(status=status.HTTP_200_OK) From c0e130494f5749b5029803cdae7646008a57680d Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Tue, 25 Aug 2026 10:19:09 -0400 Subject: [PATCH 06/11] fix(commercetools): drop webhook refund lock and optional PI (EDUN-15350) Use the reconciler payment_intent_id when Stripe omits it on the Refund object, and enqueue webhook work without holding the worker lock so Celery does not immediately retry. --- .../commercetools/stripe_refund_reconcile.py | 6 ++- .../tests/test_stripe_refund_reconcile.py | 22 +++++++++++ .../apps/stripe/tests/test_views.py | 37 +++++++------------ commerce_coordinator/apps/stripe/views.py | 31 ++++------------ 4 files changed, 47 insertions(+), 49 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py index fc32a5c1f..7afb7429b 100644 --- a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py @@ -194,6 +194,7 @@ def _reconcile_stripe_refund_locked( payment, return_items, stripe_refund, + payment_intent_id=payment_intent_id, should_transition_state=False, ) logger.info( @@ -210,6 +211,7 @@ def _reconcile_stripe_refund_locked( payment, return_items, stripe_refund, + payment_intent_id=payment_intent_id, payment_state=ReturnPaymentState.NOT_REFUNDED, ) logger.warning( @@ -227,6 +229,7 @@ def _reconcile_stripe_refund_locked( payment, return_items, stripe_refund, + payment_intent_id=payment_intent_id, payment_state=ReturnPaymentState.REFUNDED, ) @@ -395,6 +398,7 @@ def _update_return_state( return_items, stripe_refund, *, + payment_intent_id: str | None = None, payment_state: ReturnPaymentState = ReturnPaymentState.REFUNDED, should_transition_state: bool = True, ): @@ -406,7 +410,7 @@ def _update_return_state( return_line_item_return_ids=return_item_ids, return_line_entitlement_ids={}, refunded_line_item_refunds={}, - payment_intent_id=stripe_refund["payment_intent"], + payment_intent_id=stripe_refund.get("payment_intent") or payment_intent_id or "", interaction_id=stripe_refund["id"], payment_state=payment_state, payment=payment, diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py index a4bf7551a..4c5e252dd 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -10,6 +10,7 @@ from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( _emit_segment_refund, _side_effect_cache_key, + _update_return_state, reconcile_stripe_refund ) @@ -47,6 +48,7 @@ def _order(payment_state=ReturnPaymentState.INITIAL): return SimpleNamespace( id="order-1", order_number="2U-123", + version=1, return_info=[ SimpleNamespace( items=[ @@ -406,3 +408,23 @@ def test_segment_uses_stripe_refund_id_as_message_id(_mock_lms_id, mock_track): mock_track.assert_called_once() assert mock_track.call_args.kwargs["message_id"] == "re_async" assert mock_track.call_args.kwargs["event"] == "Order Refunded" + + +def test_update_return_state_uses_reconciler_payment_intent_when_refund_omits_it(): + client = MagicMock() + refund = _refund() + del refund["payment_intent"] + return_items = [SimpleNamespace(id="return-1", line_item_id="line-1")] + + _update_return_state( + client, + _order(), + _payment(), + return_items, + refund, + payment_intent_id="pi_async", + ) + + assert client.update_return_payment_state_after_successful_refund.call_args.kwargs[ + "payment_intent_id" + ] == "pi_async" diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index 7d62d9bc8..c98f4fe55 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -1,6 +1,7 @@ """ Tests for the stripe views. """ +import inspect import logging import ddt @@ -398,8 +399,6 @@ def test_refund_event_without_event_id_returns_400_without_cache_key( ) @ddt.unpack @mock.patch('stripe.Webhook.construct_event') - @mock.patch('commerce_coordinator.apps.stripe.views.release_task_lock') - @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=True) @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') def test_refund_object_events_route_to_reconciler( @@ -408,8 +407,6 @@ def test_refund_object_events_route_to_reconciler( refund_status, mock_refund_signal, mock_ct_client, - _mock_acquire, - _mock_release, mock_construct_event, ): refund = StripeObject() @@ -442,16 +439,12 @@ def test_refund_object_events_route_to_reconciler( ) @mock.patch('stripe.Webhook.construct_event') - @mock.patch('commerce_coordinator.apps.stripe.views.release_task_lock') - @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=True) @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') def test_pending_event_does_not_suppress_later_terminal_event( self, mock_refund_signal, mock_ct_client, - _mock_acquire, - _mock_release, mock_construct_event, ): payment = mock_ct_client.return_value.get_payment_by_key.return_value @@ -489,27 +482,28 @@ def _event(event_id, refund_status): self.assertEqual(mock_refund_signal.call_count, 2) @mock.patch('stripe.Webhook.construct_event') - @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=False) @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') - def test_refund_lock_contention_returns_503( + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + def test_refund_webhook_does_not_acquire_reconcile_lock( self, + mock_refund_signal, mock_ct_client, - _mock_acquire, mock_construct_event, ): refund = StripeObject() refund.update({ - "id": "re_locked", - "payment_intent": "pi_locked", + "id": "re_unlocked", + "payment_intent": "pi_unlocked", "amount": 4900, "currency": "usd", "created": 1692942318, "status": "succeeded", }) - self.mock_stripe_event.id = "evt_locked" + self.mock_stripe_event.id = "evt_unlocked" self.mock_stripe_event.type = StripeEventType.REFUND_UPDATED.value self.mock_stripe_event.data.object = refund mock_construct_event.return_value = self.mock_stripe_event + mock_refund_signal.return_value = [(lambda **kwargs: None, "celery-task-id")] payment = mock_ct_client.return_value.get_payment_by_key.return_value payment.id = "payment-1" payment.payment_method_info.payment_interface = "stripe_edx" @@ -517,25 +511,20 @@ def test_refund_lock_contention_returns_503( 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( # pylint: disable=protected-access - WebhookView.__name__, - "evt_locked", - ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_refund_signal.assert_called_once() + self.assertNotIn( + 'acquire_task_lock', + inspect.getsource(WebhookView._handle_refund_event), # pylint: disable=protected-access ) @mock.patch('stripe.Webhook.construct_event') - @mock.patch('commerce_coordinator.apps.stripe.views.release_task_lock') - @mock.patch('commerce_coordinator.apps.stripe.views.acquire_task_lock', return_value=True) @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') def test_refund_dispatch_failure_returns_503_and_clears_event_key( self, mock_refund_signal, mock_ct_client, - _mock_acquire, - _mock_release, mock_construct_event, ): refund = StripeObject() diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 3487665ab..78ff4c6b2 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -12,13 +12,8 @@ from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient -from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( - REFUND_RECONCILE_LOCK_EXPIRE, - refund_reconcile_lock_key -) from commerce_coordinator.apps.core.constants import PaymentState from commerce_coordinator.apps.core.signal_helpers import format_signal_results -from commerce_coordinator.apps.core.tasks import acquire_task_lock, release_task_lock 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 @@ -227,23 +222,11 @@ def _handle_refund_event(self, tag, event): order_number, ) - lock_key = refund_reconcile_lock_key(stripe_refund['id']) - if not acquire_task_lock(lock_key, REFUND_RECONCILE_LOCK_EXPIRE): - logger.warning( - '[Stripe webhooks] refund %s is already reconciling; returning retryable failure', - stripe_refund['id'], - ) - raise StripeWebhookDispatchAPIError - - try: - results = payment_refunded_signal.send_robust( - sender=self.__class__, - payment_intent_id=payment_intent_id, - stripe_refund=stripe_refund, - order_number=order_number, - ) - self._assert_signal_dispatched(results, payment_intent_id=payment_intent_id) - finally: - release_task_lock(lock_key) - + results = payment_refunded_signal.send_robust( + sender=self.__class__, + payment_intent_id=payment_intent_id, + stripe_refund=stripe_refund, + order_number=order_number, + ) + self._assert_signal_dispatched(results, payment_intent_id=payment_intent_id) return Response(status=status.HTTP_200_OK) From 76a7f00b75d5529b6ee9574b126886c8bcfc7b25 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Tue, 25 Aug 2026 10:32:30 -0400 Subject: [PATCH 07/11] fix(stripe): ack missing CT payment on refund webhooks (EDUN-15350) Treat Commercetools ResourceNotFound as a non-CT refund and return 200 so legacy Stripe refunds do not retry forever; re-raise other CT errors. Drop the brittle inspect-based webhook lock assertion in favor of behavior tests. Co-authored-by: Cursor --- .../apps/stripe/tests/test_views.py | 81 +++++++++++++++++-- commerce_coordinator/apps/stripe/views.py | 15 +++- 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index c98f4fe55..89dedffca 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -1,11 +1,11 @@ """ Tests for the stripe views. """ -import inspect import logging import ddt import mock +from commercetools import CommercetoolsError from django.conf import settings from django.contrib.auth import get_user_model from django.urls import reverse @@ -23,6 +23,20 @@ log_name = 'commerce_coordinator.apps.stripe.views' +def _ct_error(code: str, message: str = "boom") -> CommercetoolsError: + """Build a CommercetoolsError whose .code property matches production CT errors.""" + response = mock.MagicMock() + err_obj = mock.MagicMock() + err_obj.code = code + response.errors = [err_obj] + return CommercetoolsError( + message=message, + errors=[{"code": code, "message": message}], + response=response, + correlation_id="corr", + ) + + @ddt.ddt class WebhooksViewTests(APITestCase): """ Tests StripeWebhooksView """ @@ -484,7 +498,7 @@ def _event(event_id, refund_status): @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') - def test_refund_webhook_does_not_acquire_reconcile_lock( + def test_refund_updated_dispatches_signal( self, mock_refund_signal, mock_ct_client, @@ -513,10 +527,65 @@ def test_refund_webhook_does_not_acquire_reconcile_lock( self.assertEqual(response.status_code, status.HTTP_200_OK) mock_refund_signal.assert_called_once() - self.assertNotIn( - 'acquire_task_lock', - inspect.getsource(WebhookView._handle_refund_event), # pylint: disable=protected-access - ) + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + def test_refund_webhook_skips_missing_ct_payment( + self, + mock_refund_signal, + mock_ct_client, + mock_construct_event, + ): + refund = StripeObject() + refund.update({ + "id": "re_legacy", + "payment_intent": "pi_legacy", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": "succeeded", + }) + self.mock_stripe_event.id = "evt_legacy_refund" + self.mock_stripe_event.type = StripeEventType.REFUND_UPDATED.value + self.mock_stripe_event.data.object = refund + mock_construct_event.return_value = self.mock_stripe_event + mock_ct_client.return_value.get_payment_by_key.side_effect = _ct_error("ResourceNotFound") + + response = self.client.post(self.url, data={}, format='json', **self.mock_header) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_refund_signal.assert_not_called() + + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + def test_refund_webhook_rethrows_ct_outage( + self, + mock_refund_signal, + mock_ct_client, + mock_construct_event, + ): + refund = StripeObject() + refund.update({ + "id": "re_ct_outage", + "payment_intent": "pi_ct_outage", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": "succeeded", + }) + self.mock_stripe_event.id = "evt_ct_outage" + self.mock_stripe_event.type = StripeEventType.REFUND_UPDATED.value + self.mock_stripe_event.data.object = refund + mock_construct_event.return_value = self.mock_stripe_event + mock_ct_client.return_value.get_payment_by_key.side_effect = _ct_error("ConcurrentModification") + + # Django's test client re-raises; production surfaces this as 5xx so Stripe retries. + with self.assertRaises(CommercetoolsError): + self.client.post(self.url, data={}, format='json', **self.mock_header) + + mock_refund_signal.assert_not_called() @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 78ff4c6b2..27c9347c5 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -4,6 +4,7 @@ import logging import stripe +from commercetools import CommercetoolsError from django.conf import settings from django.views.decorators.csrf import csrf_exempt from rest_framework import status @@ -197,7 +198,19 @@ def _handle_refund_event(self, tag, event): stripe_refund = dict(event_object) payment_intent_id = event_object.payment_intent client = CommercetoolsAPIClient() - payment = client.get_payment_by_key(payment_intent_id) + try: + payment = client.get_payment_by_key(payment_intent_id) + except CommercetoolsError as err: + # Legacy Stripe refunds have no CT Payment; ack so Stripe does not retry forever. + if err.code != "ResourceNotFound": + raise + logger.info( + '[Stripe webhooks] skipping refund event %s for payment intent [%s] ' + 'with no Commercetools payment.', + event.type, + payment_intent_id, + ) + return Response(status=status.HTTP_200_OK) payment_interface = getattr(payment.payment_method_info, 'payment_interface', None) if payment_interface != EDX_STRIPE_PAYMENT_INTERFACE_NAME: logger.info( From dc935da269ca5003dbeb23d1230fa9f1c6bb2e06 Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Tue, 25 Aug 2026 10:35:06 -0400 Subject: [PATCH 08/11] fix(commercetools): fail Segment emit when refund products are missing (EDUN-15350) Raise RefundSideEffectDispatchError if Order Refunded cannot include products so the side-effect cache is not marked complete. Update refund_from_stripe_task docs to describe full reconciliation, not only CT transaction creation. Co-authored-by: Cursor --- .../commercetools/stripe_refund_reconcile.py | 22 ++++--- .../apps/commercetools/tasks.py | 11 +++- .../tests/test_stripe_refund_reconcile.py | 61 +++++++++++++++++++ 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py index 7afb7429b..bdc80c9c6 100644 --- a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py @@ -379,16 +379,20 @@ def _emit_segment_refund(client, order, stripe_refund: Refund, return_items) -> properties["products"] = [ get_product_data(item, is_bundle) for item in selected_line_items ] - if properties["products"]: - properties["title"] = ", ".join( - item.name["en-US"] for item in selected_line_items - ) - track( - lms_user_id=lms_user_id, - event="Order Refunded", - properties=properties, - message_id=stripe_refund["id"], + if not properties["products"]: + raise RefundSideEffectDispatchError( + f"Unable to emit Order Refunded for refund {stripe_refund['id']}: " + "no matching line items to include as products" ) + properties["title"] = ", ".join( + item.name["en-US"] for item in selected_line_items + ) + track( + lms_user_id=lms_user_id, + event="Order Refunded", + properties=properties, + message_id=stripe_refund["id"], + ) def _update_return_state( diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index 63b0dad5b..077f09d2c 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -146,12 +146,17 @@ def refund_from_stripe_task( source: str = "webhook", ): """ - Celery task for handling a refund registered in the Stripe dashboard. - Creates a refund payment transaction record via the Commercetools API. + Celery task that reconciles a Stripe refund against CommerceTools. + + Delegates to ``reconcile_stripe_refund``, which creates or updates the CT + refund transaction, may update Return payment state, and on confirmed + success dispatches LMS revoke and Segment ``Order Refunded`` side effects. Args: - refund (dict): Refund object payment_intent_id (str): The Stripe payment intent identifier + stripe_refund (dict): Stripe Refund object + order_number (str | None): Optional CT order number + source (str): Caller identifier (for example ``webhook``) """ return reconcile_stripe_refund( payment_intent_id, diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py index 4c5e252dd..6607c3dae 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -8,8 +8,10 @@ from edx_django_utils.cache import TieredCache from commerce_coordinator.apps.commercetools.stripe_refund_reconcile import ( + RefundSideEffectDispatchError, _emit_segment_refund, _side_effect_cache_key, + _side_effect_completed, _update_return_state, reconcile_stripe_refund ) @@ -224,6 +226,32 @@ def test_return_update_failure_does_not_emit_side_effects( mock_segment.assert_called_once() assert result.side_effects_completed is True + def test_segment_emit_failure_does_not_mark_segment_complete( + self, + mock_revoke, + mock_segment, + mock_update_return, + _mock_acquire, + _mock_release, + ): + client = MagicMock() + client.get_payment_by_key.return_value = _payment(_transaction(TransactionState.SUCCESS)) + client.get_order_by_payment_id.return_value = _order(ReturnPaymentState.REFUNDED) + mock_segment.side_effect = RefundSideEffectDispatchError("no products") + + with pytest.raises(RefundSideEffectDispatchError): + reconcile_stripe_refund( + "pi_async", + _refund(), + source="webhook", + client=client, + ) + + mock_revoke.assert_called_once() + mock_update_return.assert_not_called() + assert _side_effect_completed("re_async", "lms_revoke") + assert not _side_effect_completed("re_async", "segment") + def test_already_refunded_dispatches_unmarked_side_effects( self, mock_revoke, @@ -410,6 +438,39 @@ def test_segment_uses_stripe_refund_id_as_message_id(_mock_lms_id, mock_track): assert mock_track.call_args.kwargs["event"] == "Order Refunded" +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.track") +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.get_edx_lms_user_id", return_value="lms-1") +def test_segment_raises_when_no_products_can_be_built(_mock_lms_id, mock_track): + client = MagicMock() + client.get_customer_by_id.return_value = SimpleNamespace(id="cust-1") + order = SimpleNamespace( + id="order-1", + customer_id="cust-1", + line_items=[], + return_info=[], + custom=None, + total_price=SimpleNamespace(cent_amount=4900, currency_code="USD"), + taxed_price=None, + discount_on_total_price=None, + discount_codes=[], + payment_info=None, + ) + + with patch( + "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.prepare_segment_event_properties", + return_value={"products": []}, + ): + with pytest.raises(RefundSideEffectDispatchError, match="no matching line items"): + _emit_segment_refund( + client, + order, + _refund(), + [SimpleNamespace(id="return-1", line_item_id="line-missing")], + ) + + mock_track.assert_not_called() + + def test_update_return_state_uses_reconciler_payment_intent_when_refund_omits_it(): client = MagicMock() refund = _refund() From 005008418b989b9431a74119df59249dae5c1f7d Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Tue, 25 Aug 2026 10:39:05 -0400 Subject: [PATCH 09/11] test(stripe): assert refund webhook skips reconcile lock at runtime Patch acquire_task_lock in both the core tasks and reconciler modules and assert it is never called during the webhook request, replacing the source-inspection assertion with a behavioral check. Co-authored-by: Cursor --- commerce_coordinator/apps/stripe/tests/test_views.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index 89dedffca..860348a0e 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -498,8 +498,12 @@ def _event(event_id, refund_status): @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') - def test_refund_updated_dispatches_signal( + @mock.patch('commerce_coordinator.apps.commercetools.stripe_refund_reconcile.acquire_task_lock') + @mock.patch('commerce_coordinator.apps.core.tasks.acquire_task_lock') + def test_refund_webhook_dispatches_without_acquiring_reconcile_lock( self, + mock_core_acquire_lock, + mock_reconcile_acquire_lock, mock_refund_signal, mock_ct_client, mock_construct_event, @@ -527,6 +531,9 @@ def test_refund_updated_dispatches_signal( self.assertEqual(response.status_code, status.HTTP_200_OK) mock_refund_signal.assert_called_once() + # The worker owns the refund lock; the webhook must only enqueue. + mock_core_acquire_lock.assert_not_called() + mock_reconcile_acquire_lock.assert_not_called() @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') From 90cb4bb5ffaae96f7d8576868155c115c157d1af Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Tue, 25 Aug 2026 10:44:51 -0400 Subject: [PATCH 10/11] fix(stripe): log unsuccessful refunds without a stack trace (EDUN-15350) Use warning for expected non-success refund statuses instead of exception so failed/canceled do not look like unhandled errors. Compute Segment is_bundle from refunded line items only so sibling bundle items do not change product_id. Co-authored-by: Cursor --- .../commercetools/stripe_refund_reconcile.py | 2 +- .../tests/test_stripe_refund_reconcile.py | 62 ++++++++++++++++++- commerce_coordinator/apps/stripe/clients.py | 6 +- 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py index bdc80c9c6..d4c815d40 100644 --- a/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/stripe_refund_reconcile.py @@ -375,7 +375,7 @@ def _emit_segment_refund(client, order, stripe_refund: Refund, return_items) -> line_item_ids=list(line_item_ids), return_id=", ".join(item.id for item in return_items), ) - is_bundle = check_is_bundle(order.line_items) + is_bundle = check_is_bundle(selected_line_items) properties["products"] = [ get_product_data(item, is_bundle) for item in selected_line_items ] diff --git a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py index 6607c3dae..30200aed8 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py +++ b/commerce_coordinator/apps/commercetools/tests/test_stripe_refund_reconcile.py @@ -421,11 +421,11 @@ def test_segment_uses_stripe_refund_id_as_message_id(_mock_lms_id, mock_track): with patch( "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.get_product_data", return_value={"name": "Course"}, - ): + ) as mock_product_data: with patch( "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.check_is_bundle", return_value=False, - ): + ) as mock_is_bundle: _emit_segment_refund( client, order, @@ -436,6 +436,64 @@ def test_segment_uses_stripe_refund_id_as_message_id(_mock_lms_id, mock_track): mock_track.assert_called_once() assert mock_track.call_args.kwargs["message_id"] == "re_async" assert mock_track.call_args.kwargs["event"] == "Order Refunded" + mock_is_bundle.assert_called_once_with(order.line_items) + mock_product_data.assert_called_once() + assert mock_product_data.call_args.args[1] is False + + +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.track") +@patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.get_edx_lms_user_id", return_value="lms-1") +def test_segment_computes_is_bundle_from_refunded_line_items(_mock_lms_id, mock_track): + client = MagicMock() + client.get_customer_by_id.return_value = SimpleNamespace(id="cust-1") + refunded_item = SimpleNamespace( + id="line-course", + name={"en-US": "Course"}, + product_key="course-1", + product_type=SimpleNamespace(obj=SimpleNamespace(name="course")), + variant=SimpleNamespace(sku="sku", images=[], attributes=[]), + price=SimpleNamespace(value=SimpleNamespace(cent_amount=4900, currency_code="USD", fraction_digits=2)), + quantity=1, + ) + bundled_item = SimpleNamespace(id="line-bundle") + order = SimpleNamespace( + id="order-1", + customer_id="cust-1", + line_items=[bundled_item, refunded_item], + return_info=[], + custom=None, + total_price=SimpleNamespace(cent_amount=4900, currency_code="USD"), + taxed_price=None, + discount_on_total_price=None, + discount_codes=[], + payment_info=None, + ) + + with patch( + "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.prepare_segment_event_properties", + return_value={"products": []}, + ) as mock_props: + mock_props.return_value = {"products": [{"name": "Course"}]} + with patch( + "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.get_product_data", + return_value={"name": "Course"}, + ) as mock_product_data: + with patch( + "commerce_coordinator.apps.commercetools.stripe_refund_reconcile.check_is_bundle", + return_value=False, + ) as mock_is_bundle: + _emit_segment_refund( + client, + order, + _refund(), + [SimpleNamespace(id="return-1", line_item_id="line-course")], + ) + + mock_is_bundle.assert_called_once_with([refunded_item]) + mock_product_data.assert_called_once() + assert mock_product_data.call_args.args[0] is refunded_item + assert mock_product_data.call_args.args[1] is False + mock_track.assert_called_once() @patch("commerce_coordinator.apps.commercetools.stripe_refund_reconcile.track") diff --git a/commerce_coordinator/apps/stripe/clients.py b/commerce_coordinator/apps/stripe/clients.py index 946f2952f..7b094b3ee 100644 --- a/commerce_coordinator/apps/stripe/clients.py +++ b/commerce_coordinator/apps/stripe/clients.py @@ -310,7 +310,11 @@ def refund_payment_intent( StripeRefundStatus.REFUND_PENDING.value, } if refund.status not in accepted_statuses: - logger.exception('Refund for order [%s] was unsuccessful', order_uuid) + logger.warning( + 'Refund for order [%s] was unsuccessful with status [%s]', + order_uuid, + refund.status, + ) return None return refund From e7094fc816842e0bc53ba3570f03f309a51bc67a Mon Sep 17 00:00:00 2001 From: Bianca Severino Date: Tue, 25 Aug 2026 11:13:34 -0400 Subject: [PATCH 11/11] fix(stripe): skip refund webhooks without a payment intent (EDUN-15350) Ack refund.updated/failed with no payment_intent so Stripe does not retry a lookup that cannot succeed. Log dispatch failures as refund reconcile vs CT finalize, and retry OpenEdxFilterException on both Stripe and CT-return tasks. Co-authored-by: Cursor --- .../apps/commercetools/sub_messages/tasks.py | 2 + .../apps/commercetools/tasks.py | 2 + .../tests/sub_messages/test_tasks.py | 2 + .../apps/commercetools/tests/test_tasks.py | 5 +++ .../apps/stripe/tests/test_views.py | 37 ++++++++++++++++++- commerce_coordinator/apps/stripe/views.py | 27 +++++++++++--- 6 files changed, 69 insertions(+), 6 deletions(-) diff --git a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py index 7e7075ed6..a2c60bd4f 100644 --- a/commerce_coordinator/apps/commercetools/sub_messages/tasks.py +++ b/commerce_coordinator/apps/commercetools/sub_messages/tasks.py @@ -7,6 +7,7 @@ from commercetools import CommercetoolsError from django.contrib.auth import get_user_model from edx_django_utils.cache import TieredCache +from openedx_filters.exceptions import OpenEdxFilterException from requests import RequestException from commerce_coordinator.apps.commercetools.catalog_info.constants import ( @@ -308,6 +309,7 @@ def fulfill_order_sanctioned_message_signal_task( autoretry_for=( RequestException, CommercetoolsError, + OpenEdxFilterException, RefundReconcileInProgressError, RefundSideEffectDispatchError, ), diff --git a/commerce_coordinator/apps/commercetools/tasks.py b/commerce_coordinator/apps/commercetools/tasks.py index 077f09d2c..2fc4ec44b 100644 --- a/commerce_coordinator/apps/commercetools/tasks.py +++ b/commerce_coordinator/apps/commercetools/tasks.py @@ -7,6 +7,7 @@ from commercetools import CommercetoolsError from commercetools.platform.models import Payment from django.conf import settings +from openedx_filters.exceptions import OpenEdxFilterException from requests import RequestException from commerce_coordinator.apps.commercetools.catalog_info.constants import ( @@ -134,6 +135,7 @@ def _log_info_and_release_lock(log_message): @shared_task( autoretry_for=( CommercetoolsError, + OpenEdxFilterException, RefundReconcileInProgressError, RefundSideEffectDispatchError, ), diff --git a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py index 2e798332e..29de40433 100644 --- a/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py +++ b/commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py @@ -8,6 +8,7 @@ from commercetools.platform.models import ReturnInfo as CTReturnInfo from commercetools.platform.models import ReturnPaymentState as CTReturnPaymentState from edx_django_utils.cache import TieredCache +from openedx_filters.exceptions import OpenEdxFilterException from commerce_coordinator.apps.commercetools.catalog_info.constants import EDX_STRIPE_PAYMENT_INTERFACE_NAME, TwoUKeys from commerce_coordinator.apps.commercetools.clients import CommercetoolsAPIClient @@ -841,6 +842,7 @@ def test_forward_succeeded_retries_reconciler_errors( ): self.assertIn(RefundReconcileInProgressError, fulfill_order_returned_signal_task.autoretry_for) self.assertIn(RefundSideEffectDispatchError, fulfill_order_returned_signal_task.autoretry_for) + self.assertIn(OpenEdxFilterException, fulfill_order_returned_signal_task.autoretry_for) mock_values = _ct_client_init.return_value _run_filter_mock.return_value = { diff --git a/commerce_coordinator/apps/commercetools/tests/test_tasks.py b/commerce_coordinator/apps/commercetools/tests/test_tasks.py index 2c86245fb..9d280a6f5 100644 --- a/commerce_coordinator/apps/commercetools/tests/test_tasks.py +++ b/commerce_coordinator/apps/commercetools/tests/test_tasks.py @@ -10,6 +10,7 @@ from commercetools import CommercetoolsError from commercetools.platform.models import Money, TransactionType from django.test import TestCase +from openedx_filters.exceptions import OpenEdxFilterException from requests.exceptions import RequestException from commerce_coordinator.apps.commercetools.catalog_info.constants import EdXFieldNames @@ -141,6 +142,10 @@ def test_full_refund_already_exists(self, mock_client): self.mock_reconcile_refund.assert_called_once() + def test_retries_filter_and_reconciler_errors(self, _mock_client): + self.assertIn(OpenEdxFilterException, refund_from_stripe_task.autoretry_for) + self.assertIn(CommercetoolsError, refund_from_stripe_task.autoretry_for) + @patch('commerce_coordinator.apps.commercetools.tasks.logger') def test_exception_handling(self, mock_logger, mock_client): ''' diff --git a/commerce_coordinator/apps/stripe/tests/test_views.py b/commerce_coordinator/apps/stripe/tests/test_views.py index 860348a0e..2757f7745 100644 --- a/commerce_coordinator/apps/stripe/tests/test_views.py +++ b/commerce_coordinator/apps/stripe/tests/test_views.py @@ -564,6 +564,34 @@ def test_refund_webhook_skips_missing_ct_payment( self.assertEqual(response.status_code, status.HTTP_200_OK) mock_refund_signal.assert_not_called() + @mock.patch('stripe.Webhook.construct_event') + @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') + @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') + def test_refund_webhook_skips_missing_payment_intent( + self, + mock_refund_signal, + mock_ct_client, + mock_construct_event, + ): + refund = StripeObject() + refund.update({ + "id": "re_no_pi", + "amount": 4900, + "currency": "usd", + "created": 1692942318, + "status": "succeeded", + }) + self.mock_stripe_event.id = "evt_no_pi" + self.mock_stripe_event.type = StripeEventType.REFUND_UPDATED.value + self.mock_stripe_event.data.object = refund + 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_client.return_value.get_payment_by_key.assert_not_called() + mock_refund_signal.assert_not_called() + @mock.patch('stripe.Webhook.construct_event') @mock.patch('commerce_coordinator.apps.stripe.views.CommercetoolsAPIClient') @mock.patch('commerce_coordinator.apps.stripe.views.payment_refunded_signal.send_robust') @@ -626,7 +654,8 @@ def _receiver(**kwargs): mock_refund_signal.return_value = [(_receiver, RuntimeError("Celery broker down"))] - response = self.client.post(self.url, data={}, format='json', **self.mock_header) + with LogCapture(log_name) as log_capture: + response = self.client.post(self.url, data={}, format='json', **self.mock_header) self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) self.assertFalse( @@ -635,3 +664,9 @@ def _receiver(**kwargs): "evt_refund_dispatch_failure", ) ) + self.assertTrue( + any( + "Failed to enqueue refund reconcile" in rec.getMessage() + for rec in log_capture.records + ) + ) diff --git a/commerce_coordinator/apps/stripe/views.py b/commerce_coordinator/apps/stripe/views.py index 27c9347c5..722fb9f71 100644 --- a/commerce_coordinator/apps/stripe/views.py +++ b/commerce_coordinator/apps/stripe/views.py @@ -112,15 +112,20 @@ def _handle_commercetools_payment_event(self, tag, event, payment_intent): sender=self.__class__, payment_intent_id=payment_intent.id, ) - self._assert_signal_dispatched(results, payment_intent_id=payment_intent.id) + self._assert_signal_dispatched( + results, + payment_intent_id=payment_intent.id, + action="CT finalize", + ) return Response(status=status.HTTP_200_OK) - def _assert_signal_dispatched(self, results, *, payment_intent_id): + def _assert_signal_dispatched(self, results, *, payment_intent_id, action): """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', + '[Stripe webhooks] Failed to enqueue %s for PI [%s]: %s', + action, payment_intent_id, formatted, ) @@ -196,7 +201,15 @@ def _handle_refund_event(self, tag, event): stripe_refund = max(refunds, key=lambda refund: refund['created']) else: stripe_refund = dict(event_object) - payment_intent_id = event_object.payment_intent + payment_intent_id = stripe_refund.get("payment_intent") + if not payment_intent_id: + logger.info( + '[Stripe webhooks] skipping refund event %s with refund ID [%s] ' + 'because it has no payment intent.', + event.type, + stripe_refund.get("id"), + ) + return Response(status=status.HTTP_200_OK) client = CommercetoolsAPIClient() try: payment = client.get_payment_by_key(payment_intent_id) @@ -241,5 +254,9 @@ def _handle_refund_event(self, tag, event): stripe_refund=stripe_refund, order_number=order_number, ) - self._assert_signal_dispatched(results, payment_intent_id=payment_intent_id) + self._assert_signal_dispatched( + results, + payment_intent_id=payment_intent_id, + action="refund reconcile", + ) return Response(status=status.HTTP_200_OK)