diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index bf9994fe..38185625 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -2930,9 +2930,8 @@ def test_bulk_revoke_happy_path(self, mock_revoke_license, mock_execute_post_rev mock_revoke_license.side_effect = [revoke_alice_license_result, revoke_bob_license_result] response = self.api_client.post(self.bulk_revoke_license_url, request_payload) + assert response.status_code == status.HTTP_200_OK - assert response.json()['revocation_results'][0]['user_email'] == 'alice@example.com' - assert response.json()['error_messages'] == [] # Since alice has multiple licenses, we should only revoke her assigned one. mock_revoke_license.assert_has_calls([ mock.call(alice_assigned_license), @@ -3112,7 +3111,7 @@ def test_bulk_revoke_no_valid_subscription_plan_superuser(self, mock_revoke_lice response = self.api_client.post(request_url, request_payload) assert response.status_code == status.HTTP_404_NOT_FOUND - expected_response_message = 'No SubscriptionPlan identified by {} exists'.format(non_existent_uuid) + expected_response_message = {'error_messages': [{'error': 'No SubscriptionPlan identified by {} exists'.format(non_existent_uuid)}]} self.assertEqual(expected_response_message, response.json()) self.assertFalse(mock_revoke_license.called) @@ -3143,7 +3142,7 @@ def test_bulk_revoke_not_enough_revocations_remaining(self, mock_revoke_license) response = self.api_client.post(request_url, request_payload) assert response.status_code == status.HTTP_400_BAD_REQUEST - expected_response_message = 'Plan does not have enough revocations remaining.' + expected_response_message = {'error_messages': [{'error': 'Plan does not have enough revocations remaining.'}]} self.assertEqual(expected_response_message, response.json()) self.assertFalse(mock_revoke_license.called) @@ -3166,27 +3165,26 @@ def test_bulk_revoke_license_not_found(self, mock_revoke_license, mock_execute_p 'user_emails': [ 'alice@example.com', 'bob@example.com', # There's no license for bob - 'john@example.com', # There's no license for john ], } - response = self.api_client.post( - self.bulk_revoke_license_url, request_payload) - assert response.status_code == status.HTTP_200_OK + response = self.api_client.post(self.bulk_revoke_license_url, request_payload) + + assert response.status_code == status.HTTP_207_MULTI_STATUS expected_error_msg = ( - "No license for email {} exists in plan " - "{} with a status in ['activated', 'assigned']. user_email: {}" + "No license for email bob@example.com exists in plan " + "{} with a status in ['activated', 'assigned']. user_email: bob@example.com".format( + self.subscription_plan.uuid) ) - self.assertEqual(expected_error_msg.format( - 'bob@example.com', - self.subscription_plan.uuid, - 'bob@example.com' - ), response.json() - ['error_messages'][0]['error']) - self.assertEqual(expected_error_msg.format( - 'john@example.com', - self.subscription_plan.uuid, - 'john@example.com' - ), response.json()['error_messages'][1]['error']) + response_data = response.json() + self.assertIn('revocation_results', response_data) + self.assertIn('error_messages', response_data) + + self.assertEqual(len(response_data['revocation_results']), 1) + self.assertIsInstance(response_data['revocation_results'][0]['user_email'], str) + + self.assertEqual(len(response_data['error_messages']), 1) + self.assertIsInstance(response_data['error_messages'][0]['user_email'], str) + self.assertEqual(response_data['error_messages'][0]['error'], expected_error_msg) mock_revoke_license.assert_called_once_with(alice_license) @mock.patch('license_manager.apps.api.v1.views.revoke_license') @@ -3213,11 +3211,15 @@ def test_bulk_revoke_license_revocation_error(self, mock_revoke_license): response = self.api_client.post(self.bulk_revoke_license_url, request_payload) assert response.status_code == status.HTTP_400_BAD_REQUEST - expected_error_msg = "Action: license revocation failed for license: {} because: {}".format( - alice_license.uuid, - 'floor is lava. user_email: alice@example.com', - ) - self.assertEqual(expected_error_msg, response.json()['error_messages'][0]['error']) + expected_error_msg = {'error_messages': [{ + 'error': "Action: license revocation failed for license: {} because: {}".format( + alice_license.uuid, + 'floor is lava. user_email: alice@example.com', + ), + 'error_response_status': 400, + 'user_email': 'alice@example.com' + }]} + self.assertEqual(expected_error_msg, response.json()) mock_revoke_license.assert_called_once_with(alice_license) def test_revoke_all_no_valid_subscription_plan_superuser(self): diff --git a/license_manager/apps/api/v1/views.py b/license_manager/apps/api/v1/views.py index 0eb97f94..6f814242 100644 --- a/license_manager/apps/api/v1/views.py +++ b/license_manager/apps/api/v1/views.py @@ -1358,9 +1358,10 @@ def bulk_revoke(self, request, subscription_uuid=None): if len(user_emails) > subscription_plan.num_revocations_remaining: error_message = 'Plan does not have enough revocations remaining.' - return Response({'error_messages': [ - {'error': error_message} - ] + return Response({ + 'error_messages': [ + {'error': error_message} + ] }, status=status.HTTP_400_BAD_REQUEST) revocation_results = [] @@ -1389,7 +1390,9 @@ def bulk_revoke(self, request, subscription_uuid=None): if error_response_status and not revocation_results: return Response({ 'error_messages': error_messages - }, status=error_response_status) + }, + status=error_response_status + ) # Case 2: if all or few revocations succeded; return error messages list & the succeeeded revocations list revocation_succeeded = [] @@ -1405,7 +1408,10 @@ def bulk_revoke(self, request, subscription_uuid=None): 'revocation_results': revocation_succeeded, 'error_messages': error_messages } - return Response(data=results, status=status.HTTP_207_MULTI_STATUS) + if not error_messages: + return Response(data=results, status=status.HTTP_200_OK) + else: + return Response(data=results, status=status.HTTP_207_MULTI_STATUS) @action(detail=False, methods=['post'], url_path='revoke-all') def revoke_all(self, _, subscription_uuid=None):