From 1c656274f410c01c62823a7a97cecc9ce2e9cee3 Mon Sep 17 00:00:00 2001 From: Muhammad Abdullah Waheed Date: Mon, 25 Nov 2024 11:28:28 +0500 Subject: [PATCH 1/5] fix: handled exception gracefully and returned proper response --- .../apps/frontend_app_ecommerce/views.py | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/commerce_coordinator/apps/frontend_app_ecommerce/views.py b/commerce_coordinator/apps/frontend_app_ecommerce/views.py index bd818c6f..29aa5fa3 100644 --- a/commerce_coordinator/apps/frontend_app_ecommerce/views.py +++ b/commerce_coordinator/apps/frontend_app_ecommerce/views.py @@ -10,7 +10,7 @@ from edx_rest_framework_extensions.permissions import LoginRedirectIfUnauthenticated from rest_framework.exceptions import PermissionDenied from rest_framework.response import Response -from rest_framework.status import HTTP_303_SEE_OTHER +from rest_framework.status import HTTP_303_SEE_OTHER, HTTP_400_BAD_REQUEST from rest_framework.throttling import UserRateThrottle from rest_framework.views import APIView @@ -94,18 +94,21 @@ def get(self, request): if not request.user.lms_user_id: # pragma: no cover raise PermissionDenied(detail="Could not detect LMS user id.") - order_data = OrderHistoryRequested.run_filter(request, params) + try: + order_data = OrderHistoryRequested.run_filter(request, params) - output_orders = [] + output_orders = [] - for order_set in order_data: - output_orders.extend(order_set['results']) + for order_set in order_data: + output_orders.extend(order_set['results']) - output = { - "count": request.query_params['page_size'], # This suppresses the ecomm mfe Order History Pagination ctrl - "next": None, - "previous": None, - "results": sorted(output_orders, key=lambda item: date_conv(item["date_placed"]), reverse=True) - } + output = { + "count": request.query_params['page_size'], # This suppresses the ecomm mfe Order History Pagination ctrl + "next": None, + "previous": None, + "results": sorted(output_orders, key=lambda item: date_conv(item["date_placed"]), reverse=True) + } - return Response(output) + return Response(output) + except Exception as exc: + return Response(status=HTTP_400_BAD_REQUEST, data=str(exc)) From 6f6f6aa875605a59449293ae0aa0f6ff2bbcfeaa Mon Sep 17 00:00:00 2001 From: Muhammad Abdullah Waheed Date: Mon, 25 Nov 2024 13:10:07 +0500 Subject: [PATCH 2/5] fix: liniting issues --- commerce_coordinator/apps/frontend_app_ecommerce/views.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/commerce_coordinator/apps/frontend_app_ecommerce/views.py b/commerce_coordinator/apps/frontend_app_ecommerce/views.py index 29aa5fa3..86dd5512 100644 --- a/commerce_coordinator/apps/frontend_app_ecommerce/views.py +++ b/commerce_coordinator/apps/frontend_app_ecommerce/views.py @@ -103,12 +103,13 @@ def get(self, request): output_orders.extend(order_set['results']) output = { - "count": request.query_params['page_size'], # This suppresses the ecomm mfe Order History Pagination ctrl + # This suppresses the ecomm mfe Order History Pagination control + "count": request.query_params['page_size'], "next": None, "previous": None, "results": sorted(output_orders, key=lambda item: date_conv(item["date_placed"]), reverse=True) } return Response(output) - except Exception as exc: + except Exception as exc: # pylint: disable=broad-except return Response(status=HTTP_400_BAD_REQUEST, data=str(exc)) From 5b7c74925fdc043961ba84c08eb9f369581523c0 Mon Sep 17 00:00:00 2001 From: Muhammad Abdullah Waheed Date: Mon, 25 Nov 2024 13:15:15 +0500 Subject: [PATCH 3/5] refactor: logged error and returned appropriate message --- commerce_coordinator/apps/frontend_app_ecommerce/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/commerce_coordinator/apps/frontend_app_ecommerce/views.py b/commerce_coordinator/apps/frontend_app_ecommerce/views.py index 86dd5512..3ff1a7bf 100644 --- a/commerce_coordinator/apps/frontend_app_ecommerce/views.py +++ b/commerce_coordinator/apps/frontend_app_ecommerce/views.py @@ -112,4 +112,5 @@ def get(self, request): return Response(output) except Exception as exc: # pylint: disable=broad-except - return Response(status=HTTP_400_BAD_REQUEST, data=str(exc)) + logger.error(f'UserOrdersView threw following exception: [{exc}]') + return Response(status=HTTP_400_BAD_REQUEST, data='Something went wrong!') From 19a371b50c26ffa2b50cbeb8177c0a37d62fcd60 Mon Sep 17 00:00:00 2001 From: Muhammad Abdullah Waheed Date: Wed, 27 Nov 2024 11:38:55 +0500 Subject: [PATCH 4/5] refactor: updated log message --- commerce_coordinator/apps/frontend_app_ecommerce/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commerce_coordinator/apps/frontend_app_ecommerce/views.py b/commerce_coordinator/apps/frontend_app_ecommerce/views.py index 3ff1a7bf..bb1a078d 100644 --- a/commerce_coordinator/apps/frontend_app_ecommerce/views.py +++ b/commerce_coordinator/apps/frontend_app_ecommerce/views.py @@ -112,5 +112,5 @@ def get(self, request): return Response(output) except Exception as exc: # pylint: disable=broad-except - logger.error(f'UserOrdersView threw following exception: [{exc}]') + logger.error(f'[UserOrdersView] An error occured while fetching Order History.\n Data: [{exc}]') return Response(status=HTTP_400_BAD_REQUEST, data='Something went wrong!') From 360711cacc344ba36d4612ce415ac7225ad1a60c Mon Sep 17 00:00:00 2001 From: Muhammad Abdullah Waheed Date: Wed, 27 Nov 2024 14:04:30 +0500 Subject: [PATCH 5/5] test: added unit tests for graceful exception handling --- .../tests/test_views.py | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/commerce_coordinator/apps/frontend_app_ecommerce/tests/test_views.py b/commerce_coordinator/apps/frontend_app_ecommerce/tests/test_views.py index 3a90be94..ee1c9823 100644 --- a/commerce_coordinator/apps/frontend_app_ecommerce/tests/test_views.py +++ b/commerce_coordinator/apps/frontend_app_ecommerce/tests/test_views.py @@ -44,6 +44,7 @@ class OrdersViewTests(TestCase): test_user_username = 'test' # Different from ORDER_HISTORY_GET_PARAMETERS username. test_user_email = 'test@example.com' test_user_password = 'secret' + url = reverse('frontend_app_ecommerce:order_history') def setUp(self): """Create test user before test starts.""" @@ -69,7 +70,7 @@ def test_view_rejects_post(self, _mock_ctorders, _mock_ecommerce_client): self.client.login(username=self.test_user_username, password=self.test_user_password) # Perform POST - response = self.client.post(reverse('frontend_app_ecommerce:order_history'), ORDER_HISTORY_GET_PARAMETERS) + response = self.client.post(self.url, ORDER_HISTORY_GET_PARAMETERS) # Check 405 Method Not Allowed self.assertEqual(response.status_code, 405) @@ -78,7 +79,7 @@ def test_view_rejects_unauthorized(self, _mock_ctorders, _mock_ecommerce_client) """Check unauthorized users querying orders are redirected to login page.""" # Perform GET without logging in. - response = self.client.get(reverse('frontend_app_ecommerce:order_history'), ORDER_HISTORY_GET_PARAMETERS) + response = self.client.get(self.url, ORDER_HISTORY_GET_PARAMETERS) # Check 302 Found with redirect to login page. self.assertEqual(response.status_code, 302) @@ -91,7 +92,7 @@ def test_view_returns_ok(self, _mock_ctorders, _mock_ecommerce_client): self.client.login(username=self.test_user_username, password=self.test_user_password) # Perform GET - response = self.client.get(reverse('frontend_app_ecommerce:order_history'), ORDER_HISTORY_GET_PARAMETERS) + response = self.client.get(self.url, ORDER_HISTORY_GET_PARAMETERS) # Check 200 OK self.assertEqual(response.status_code, 200) @@ -104,7 +105,7 @@ def test_view_returns_expected_ecommerce_response(self, is_redirect_mock, _mock_ self.client.login(username=self.test_user_username, password=self.test_user_password) # Perform GET - response = self.client.get(reverse('frontend_app_ecommerce:order_history'), ORDER_HISTORY_GET_PARAMETERS) + response = self.client.get(self.url, ORDER_HISTORY_GET_PARAMETERS) # Check expected response self.assertEqual(response.json()['results'][1], ECOMMERCE_REQUEST_EXPECTED_RESPONSE['results'][0]) @@ -116,7 +117,7 @@ def test_view_passes_username(self, _mock_ctorders, mock_ecommerce_client): self.client.login(username=self.test_user_username, password=self.test_user_password) # Perform GET - self.client.get(reverse('frontend_app_ecommerce:order_history'), ORDER_HISTORY_GET_PARAMETERS) + self.client.get(self.url, ORDER_HISTORY_GET_PARAMETERS) # Get username sent to ecommerce client request_username = mock_ecommerce_client.call_args.args[0]['username'] @@ -124,6 +125,25 @@ def test_view_passes_username(self, _mock_ctorders, mock_ecommerce_client): # Check username is passed to ecommerce client self.assertEqual(request_username, self.test_user_username) + @patch( + 'commerce_coordinator.apps.commercetools.pipeline.GetCommercetoolsOrders.run_filter', + side_effect=Exception('Something went wrong') + ) + def test_view_crashes_with_some_error(self, _mock_pipeline, _mock_ctorders, _mock_ecommerce_client): + """Check exception is handled gracefully if pipeline crashes.""" + + # Login + self.client.login(username=self.test_user_username, password=self.test_user_password) + + # Perform GET request + response = self.client.get(self.url, ORDER_HISTORY_GET_PARAMETERS) + + # Assert the response status is 400 + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + # Assert the response content matches the error message + self.assertEqual(response.data, 'Something went wrong!') + @ddt.ddt class ReceiptRedirectViewTests(APITestCase):