Skip to content

fix: move revoke signal call to return view #536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bseverino wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: move revoke signal call to return view #536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions commerce_coordinator/apps/commercetools/sub_messages/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@
)
from commerce_coordinator.apps.commercetools.signals import (
fulfill_order_placed_send_enroll_in_course_signal,
fulfill_order_placed_send_entitlement_signal,
fulfill_order_returned_send_revoke_line_items_signal
fulfill_order_placed_send_entitlement_signal
)
from commerce_coordinator.apps.commercetools.utils import (
convert_ct_cent_amount_to_localized_price,
Expand Down Expand Up @@ -443,13 +442,6 @@ def _get_product_data(line_item, is_bundle):
event='Order Refunded',
properties=segment_event_properties
)

# revoke line items
fulfill_order_returned_send_revoke_line_items_signal.send_robust(
sender=fulfill_order_returned_signal_task,
order_id=order_id,
return_items=return_items,
)
else: # pragma no cover
logger.info(f'[CT-{tag}] payment {psp_payment_id} not refunded, '
f'sending Slack notification, message id: {message_id}')
Expand Down
35 changes: 0 additions & 35 deletions commerce_coordinator/apps/commercetools/tests/sub_messages/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,13 +452,6 @@ def setUp(self):
super().setUp()
self.mock = CommercetoolsAPIClientMock()

revoke_send_patcher = patch(
"commerce_coordinator.apps.commercetools.sub_messages.tasks."
"fulfill_order_returned_send_revoke_line_items_signal.send_robust",
)
self.mock_revoke_line_send = revoke_send_patcher.start()
self.addCleanup(revoke_send_patcher.stop)

# Force reset nested mocked return object
order_return = self.mock.order_mock.return_value
if hasattr(order_return, "custom") and hasattr(order_return.custom, "fields"):
Expand Down Expand Up @@ -657,15 +650,6 @@ def test_mobile_order_triggers_update_return_payment_state_for_mobile_order(
class FulfillOrderReturnedSignalTaskTests(TestCase):
"""Tests for the fulfill_order_returned_signal_task"""

def setUp(self):
super().setUp()
revoke_send_patcher = patch(
"commerce_coordinator.apps.commercetools.sub_messages.tasks."
"fulfill_order_returned_send_revoke_line_items_signal.send_robust",
)
self.mock_revoke_line_send = revoke_send_patcher.start()
self.addCleanup(revoke_send_patcher.stop)

@staticmethod
def unpack_for_uut(values):
""" Unpack the dictionary in the order required for the UUT """
Expand All @@ -692,11 +676,6 @@ def test_correct_arguments_passed(self, _ct_client_init: CommercetoolsAPIClientM
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"],
)

def test_order_not_found(self, _ct_client_init: CommercetoolsAPIClientMock, _run_filter_mock):
"""
Expand All @@ -708,7 +687,6 @@ def test_order_not_found(self, _ct_client_init: CommercetoolsAPIClientMock, _run
with self.assertRaises(CommercetoolsError):
self.get_uut()(*self.unpack_for_uut(mock_values.example_payload))

self.mock_revoke_line_send.assert_not_called()
mock_values.order_mock.assert_called_once_with(mock_values.order_id)

def test_customer_not_found(self, _ct_client_init: CommercetoolsAPIClientMock, _run_filter_mock):
Expand All @@ -721,7 +699,6 @@ def test_customer_not_found(self, _ct_client_init: CommercetoolsAPIClientMock, _
with self.assertRaises(CommercetoolsError):
self.get_uut()(*self.unpack_for_uut(mock_values.example_payload))

self.mock_revoke_line_send.assert_not_called()
mock_values.order_mock.assert_called_once_with(mock_values.order_id)
mock_values.customer_mock.assert_called_once_with(mock_values.customer_id)

Expand All @@ -735,7 +712,6 @@ def test_not_edx_order(self, _ct_client_init: CommercetoolsAPIClientMock, _run_f
ret_val = self.get_uut()(*self.unpack_for_uut(mock_values.example_payload))

self.assertTrue(ret_val)
self.mock_revoke_line_send.assert_not_called()
mock_values.order_mock.assert_called_once_with(mock_values.order_id)
mock_values.customer_mock.assert_called_once_with(mock_values.customer_id)

Expand All @@ -751,11 +727,6 @@ def test_refund_successful(self, _ct_client_init: CommercetoolsAPIClientMock, _r
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"],
)

def test_refund_successful_with_segment(self, _ct_client_init: CommercetoolsAPIClientMock, _run_filter_mock):
"""
Expand All @@ -773,11 +744,6 @@ 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"],
)

def test_refund_unsuccessful(self, _ct_client_init: CommercetoolsAPIClientMock, _run_filter_mock):
"""
Expand All @@ -788,6 +754,5 @@ def test_refund_unsuccessful(self, _ct_client_init: CommercetoolsAPIClientMock,
with self.assertRaises(Exception):
self.get_uut()(*self.unpack_for_uut(mock_values.example_payload))

self.mock_revoke_line_send.assert_not_called()
mock_values.order_mock.assert_called_once_with(mock_values.order_id)
mock_values.customer_mock.assert_called_once_with(mock_values.customer_id)
31 changes: 22 additions & 9 deletions commerce_coordinator/apps/commercetools/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,16 @@ def test_view_returns_ok_missing_order_state(self, _mock_customer, _mock_order,


@ddt.ddt
@patch('commerce_coordinator.apps.commercetools.sub_messages.signals_dispatch'
'.fulfill_order_returned_signal.send_robust',
new_callable=SendRobustSignalMock)
@patch(
'commerce_coordinator.apps.commercetools.views.'
'fulfill_order_returned_send_revoke_line_items_signal.send_robust',
new_callable=SendRobustSignalMock,
)
@patch(
'commerce_coordinator.apps.commercetools.sub_messages.signals_dispatch'
'.fulfill_order_returned_signal.send_robust',
new_callable=SendRobustSignalMock,
)
class OrderReturnedViewTests(APITestCase):
"""Tests for order sanctioned view"""
url = reverse('commercetools:returned')
Expand Down Expand Up @@ -379,7 +386,7 @@ def tearDown(self):
'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient.get_customer_by_id',
new_callable=CTCustomerByIdMock
)
def test_view_returns_ok(self, _mock_customer, _mock_order, _mock_signal):
def test_view_returns_ok(self, _mock_customer, _mock_order, _mock_signal, _mock_revoke_signal):
"""Check authorized user requesting sanction receives a HTTP 200 OK."""

# Login
Expand All @@ -390,6 +397,8 @@ def test_view_returns_ok(self, _mock_customer, _mock_order, _mock_signal):

# Check 200 OK
self.assertEqual(response.status_code, 200)
_mock_signal.assert_called_once()
_mock_revoke_signal.assert_called_once()

@patch(
'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient.get_order_by_id',
Expand All @@ -399,7 +408,7 @@ def test_view_returns_ok(self, _mock_customer, _mock_order, _mock_signal):
'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient.get_customer_by_id',
new_callable=CTCustomerByIdMock
)
def test_view_returns_expected_error(self, _mock_customer, _mock_order, _mock_signal):
def test_view_returns_expected_error(self, _mock_customer, _mock_order, _mock_signal, _mock_revoke_signal):
"""Check an authorized account requesting fulfillment with bad inputs receive an expected error."""

# Login
Expand All @@ -417,6 +426,8 @@ def test_view_returns_expected_error(self, _mock_customer, _mock_order, _mock_si
'detail': ['This field is required.'],
}
self.assertEqual(response.json(), expected_response)
_mock_signal.assert_not_called()
_mock_revoke_signal.assert_not_called()

@patch(
'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient.get_order_by_id',
Expand All @@ -426,7 +437,7 @@ def test_view_returns_expected_error(self, _mock_customer, _mock_order, _mock_si
'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient.get_customer_by_id',
new_callable=CTCustomerByIdMock
)
def test_view_returns_expected_error_no_order(self, mock_customer, _mock_order, _mock_signal):
def test_view_returns_expected_error_no_order(self, mock_customer, _mock_order, _mock_signal, _mock_revoke_signal):
"""Check an authorized account requesting fulfillment unable to get customer receive an expected error."""
mock_customer.return_value = None
# Login
Expand All @@ -445,7 +456,7 @@ def test_view_returns_expected_error_no_order(self, mock_customer, _mock_order,
'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient.get_customer_by_id',
new_callable=CTCustomerByIdMock
)
def test_view_returns_ok_bad_order_state(self, _mock_customer, _mock_order, _mock_signal):
def test_view_returns_ok_bad_order_state(self, _mock_customer, _mock_order, _mock_signal, _mock_revoke_signal):
"""Check authorized user requesting sanction receives a HTTP 200 OK."""

# Login
Expand All @@ -465,7 +476,7 @@ def test_view_returns_ok_bad_order_state(self, _mock_customer, _mock_order, _moc
'commerce_coordinator.apps.commercetools.clients.CommercetoolsAPIClient.get_customer_by_id',
new_callable=CTCustomerByIdMock
)
def test_view_returns_ok_missing_order_state(self, _mock_customer, _mock_order, _mock_signal):
def test_view_returns_ok_missing_order_state(self, _mock_customer, _mock_order, _mock_signal, _mock_revoke_signal):
"""Check authorized with missing order user requesting sanction receives a HTTP 200 OK."""

# Login
Expand All @@ -477,7 +488,7 @@ def test_view_returns_ok_missing_order_state(self, _mock_customer, _mock_order,
# Check 200 OK
self.assertEqual(response.status_code, 200)

def test_unauthorized_user(self, _mock_signal):
def test_unauthorized_user(self, _mock_signal, _mock_revoke_signal):
"""Check unauthorized user is forbidden."""

# Login
Expand All @@ -488,3 +499,5 @@ def test_unauthorized_user(self, _mock_signal):

# Check 403 Forbidden
self.assertEqual(response.status_code, 403)
_mock_signal.assert_not_called()
_mock_revoke_signal.assert_not_called()
8 changes: 8 additions & 0 deletions commerce_coordinator/apps/commercetools/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
OrderReturnedViewMessageInputSerializer,
OrderSanctionedViewMessageInputSerializer
)
from commerce_coordinator.apps.commercetools.signals import fulfill_order_returned_send_revoke_line_items_signal
from commerce_coordinator.apps.commercetools.sub_messages.signals_dispatch import (
fulfill_order_placed_message_signal,
fulfill_order_returned_signal,
Expand Down Expand Up @@ -151,4 +152,11 @@ def post(self, request):
message_id=message_id
)

# revoke line items
fulfill_order_returned_send_revoke_line_items_signal.send_robust(
sender=self,
order_id=order_id,
return_items=return_items,
Comment on lines +155 to +159

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_return_line_items() defaults to []. If a return message ever arrives with an empty returnInfo.items, this will still dispatch the revoke signal and enqueue a Celery task that immediately no-ops. Consider short-circuiting when return_items is empty (log + return 200) to reduce unnecessary queue traffic.

Copilot uses AI. Check for mistakes.

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For observability/correlation, consider passing message_id through this revoke signal as well (it’s available just above and is already passed to fulfill_order_returned_signal). The receiver can ignore unknown kwargs, but logs will then include the CT subscription message id.

Suggested change
return_items=return_items,
return_items=return_items,
message_id=message_id,

Copilot uses AI. Check for mistakes.
)

return Response(status=status.HTTP_200_OK)
Loading