feat: Modify License Revocation Endpoint Behavior

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

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 [email protected] exists in plan "
             "{} with a status in ['activated', 'assigned']. user_email: [email protected]".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: [email protected]',
-        )
+        expected_error_msg = {'unsuccessful_revocations': [{
+            'error': "Action: license revocation failed for license: {} because: {}".format(
+                alice_license.uuid,
+                'floor is lava. user_email: [email protected]',
+            ),
+            'error_response_status': 400,
+            'user_email': '[email protected]'
+        }]}
         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(

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 [email protected] 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):