diff --git a/license_manager/apps/api/v1/tests/test_api_eventing.py b/license_manager/apps/api/v1/tests/test_api_eventing.py index 94d5f67e..7f962c0d 100644 --- a/license_manager/apps/api/v1/tests/test_api_eventing.py +++ b/license_manager/apps/api/v1/tests/test_api_eventing.py @@ -159,7 +159,7 @@ def test_bulk_revoked_event(self, *_): with mock.patch('license_manager.apps.subscriptions.models.track_event') as mock_revoke_track_event, \ mock.patch('license_manager.apps.subscriptions.event_utils.track_event') as mock_create_track_event: response = self.api_client.post(self.bulk_revoke_license_url, request_payload) - assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.status_code == status.HTTP_200_OK assert mock_revoke_track_event.call_count == 2 assert mock_create_track_event.call_count == 2 diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index 158cccf9..4a4ef486 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -2931,7 +2931,7 @@ def test_bulk_revoke_happy_path(self, mock_revoke_license, mock_execute_post_rev response = self.api_client.post(self.bulk_revoke_license_url, request_payload) - assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.status_code == status.HTTP_200_OK # Since alice has multiple licenses, we should only revoke her assigned one. mock_revoke_license.assert_has_calls([ mock.call(alice_assigned_license), @@ -2980,7 +2980,7 @@ def test_bulk_revoke_set_custom_tags( } # Verify that set_tags util was called with right arguments mock_set_tags_util.assert_called_with(tags_dict) - assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.status_code == status.HTTP_200_OK @mock.patch('license_manager.apps.api.v1.views.execute_post_revocation_tasks') @mock.patch('license_manager.apps.api.v1.views.revoke_license') @@ -3012,7 +3012,7 @@ def test_bulk_revoke_multiple_activated_same_email(self, mock_revoke_license, mo response = self.api_client.post(self.bulk_revoke_license_url, request_payload) - assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.status_code == status.HTTP_200_OK # Since alice has multiple licenses, we should only revoke the first one (which is arbitrarily # the one with the smallest uuid). mock_revoke_license.assert_has_calls([ @@ -3056,7 +3056,7 @@ def test_bulk_revoke_with_filters_happy_path( } response = self.api_client.post(self.bulk_revoke_license_url, request_payload) - assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.status_code == status.HTTP_200_OK revoked_emails = [call_arg[0][0].user_email for call_arg in mock_revoke_license.call_args_list] assert sorted(revoked_emails) == expected_revoked_emails @@ -3111,7 +3111,8 @@ 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 = {'unsuccessful_revocations': [ + {'error': 'No SubscriptionPlan identified by {} exists'.format(non_existent_uuid)}]} self.assertEqual(expected_response_message, response.json()) self.assertFalse(mock_revoke_license.called) @@ -3142,12 +3143,14 @@ 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 = {'unsuccessful_revocations': [ + {'error': 'Plan does not have enough revocations remaining.'}]} self.assertEqual(expected_response_message, response.json()) self.assertFalse(mock_revoke_license.called) + @mock.patch('license_manager.apps.api.v1.views.execute_post_revocation_tasks') @mock.patch('license_manager.apps.api.v1.views.revoke_license') - def test_bulk_revoke_license_not_found(self, mock_revoke_license): + def test_bulk_revoke_license_not_found(self, mock_revoke_license, mock_execute_post_revocation_tasks): # pylint: disable=unused-argument """ Test that calls to bulk_revoke fail with a 404 if the plan does not have enough revocations remaining. @@ -3168,13 +3171,22 @@ def test_bulk_revoke_license_not_found(self, mock_revoke_license): } response = self.api_client.post(self.bulk_revoke_license_url, request_payload) - assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.status_code == status.HTTP_207_MULTI_STATUS expected_error_msg = ( "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, response.json()) + response_data = response.json() + self.assertIn('successful_revocations', response_data) + self.assertIn('unsuccessful_revocations', response_data) + + self.assertEqual(len(response_data['successful_revocations']), 1) + self.assertIsInstance(response_data['successful_revocations'][0]['user_email'], str) + + self.assertEqual(len(response_data['unsuccessful_revocations']), 1) + self.assertIsInstance(response_data['unsuccessful_revocations'][0]['user_email'], str) + self.assertEqual(response_data['unsuccessful_revocations'][0]['error'], expected_error_msg) mock_revoke_license.assert_called_once_with(alice_license) @mock.patch('license_manager.apps.api.v1.views.revoke_license') @@ -3201,10 +3213,14 @@ 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', - ) + expected_error_msg = {'unsuccessful_revocations': [{ + '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) @@ -3281,7 +3297,7 @@ def test_assign_after_license_revoke_end_to_end( self.subscription_plan.save() response = self.api_client.post(self.bulk_revoke_license_url, {'user_emails': [self.test_email]}) - assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.status_code == status.HTTP_200_OK mock_revoke_course_enrollments_for_user_task.assert_called() if is_revocation_cap_enabled: mock_send_revocation_cap_notification_email_task.assert_called_with( @@ -3342,7 +3358,7 @@ def test_revoke_total_and_allocated_count_end_to_end( # Revoke the activated license and verify the counts change appropriately revoke_response = self.api_client.post(self.bulk_revoke_license_url, {'user_emails': [self.test_email]}) - assert revoke_response.status_code == status.HTTP_204_NO_CONTENT + assert revoke_response.status_code == status.HTTP_200_OK mock_revoke_course_enrollments_for_user_task.assert_called() second_detail_response = _subscriptions_detail_request( diff --git a/license_manager/apps/api/v1/views.py b/license_manager/apps/api/v1/views.py index a758c1eb..24c99388 100644 --- a/license_manager/apps/api/v1/views.py +++ b/license_manager/apps/api/v1/views.py @@ -1316,19 +1316,36 @@ def bulk_revoke(self, request, subscription_uuid=None): Response: - 204 No Content - All revocations were successful. - 400 Bad Request - Some error occurred when processing one of the revocations, no revocations - were committed. An error message is provided. - 404 Not Found - No license exists in the plan for one of the given email addresses, - or the license is not in an assigned or activated state. - An error message is provided. + 200 OK - All revocations were successful. Returns a list of successful revocations. + 207 Multi-Status - Some revocations were successful, but others failed. + Returns both successful and failed revocations. + 400 Bad Request - An error occurred when processing the request (e.g., invalid data format). + 404 Not Found - The subscription plan was not found. - Error Response Message: - "No license for email carol@example.com exists in plan {subscription_plan_uuid} " - "with a status in ['activated', 'assigned']" + Response Body: + { + "successful_revocations": [ + { + "license_uuid": "string", + "original_status": "string", + "user_email": "string" + } + ], + "unsuccessful_revocations": [ + { + "error": "string", + "error_response_status": "integer", + "user_email": "string" + } + ] + } - The revocation of licenses is atomic: if an error occurs while processing any of the license revocations, - no status change is committed for any of the licenses. + The revocation process attempts to revoke all requested licenses. If any revocations fail, + the successful revocations are still committed, and details of both successful and failed + revocations are returned in the response. + + If the number of requested revocations exceeds the remaining revocations for the plan, + a 400 Bad Request response is returned without processing any revocations. """ # to capture custom metrics custom_tags = { @@ -1347,7 +1364,13 @@ def bulk_revoke(self, request, subscription_uuid=None): error_message = 'No SubscriptionPlan identified by {} exists'.format( subscription_uuid, ) - return Response(error_message, status=status.HTTP_404_NOT_FOUND) + return Response({ + 'unsuccessful_revocations': [ + {'error': error_message} + ] + }, + status=status.HTTP_404_NOT_FOUND + ) user_emails = request.data.get('user_emails', []) if not user_emails: @@ -1356,28 +1379,60 @@ 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_message, status=status.HTTP_400_BAD_REQUEST) + return Response({ + 'unsuccessful_revocations': [ + {'error': error_message} + ] + }, status=status.HTTP_400_BAD_REQUEST) revocation_results = [] + error_messages = [] with transaction.atomic(): for user_email in user_emails: try: - revocation_result = self._revoke_by_email_and_plan(user_email, subscription_plan) + revocation_result = self._revoke_by_email_and_plan( + user_email, subscription_plan) + revocation_result['user_email'] = user_email revocation_results.append(revocation_result) except (LicenseNotFoundError, LicenseRevocationError) as exc: error_message = f'{str(exc)}. user_email: {user_email}' - error_response_status = utils.get_http_status_for_exception(exc) - logger.error(f'{error_message}, error_response_status:{error_response_status}') - break - - if error_response_status: - return Response(error_message, status=error_response_status) + error_response_status = utils.get_http_status_for_exception( + exc) + error_object = { + 'error': error_message, + 'error_response_status': error_response_status, + 'user_email': user_email, + } + logger.error(error_object) + error_messages.append(error_object) + + # Case 1: if all revocations failed; return only the error messages list + if error_response_status and not revocation_results: + return Response({ + 'unsuccessful_revocations': error_messages + }, + status=error_response_status + ) + # Case 2: if all or few revocations succeded; return error messages list & the succeeeded revocations list + revocation_succeeded = [] for revocation_result in revocation_results: + user_email = revocation_result.pop('user_email', None) execute_post_revocation_tasks(**revocation_result) - - return Response(status=status.HTTP_204_NO_CONTENT) + revocation_succeeded.append({ + 'license_uuid': str(revocation_result['revoked_license'].uuid), + 'original_status': str(revocation_result['original_status']), + 'user_email': str(user_email) + }) + results = { + 'successful_revocations': revocation_succeeded, + 'unsuccessful_revocations': error_messages + } + 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):