feat: continue revocation job instead of breaking on error

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

@@ -2930,8 +2930,9 @@ 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_204_NO_CONTENT
+        assert response.status_code == status.HTTP_200_OK
+        assert response.json()['revocation_results'][0]['user_email'] == '[email protected]'
+        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),
@@ -2980,7 +2981,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 +3013,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 +3057,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
@@ -3146,8 +3147,9 @@ def test_bulk_revoke_not_enough_revocations_remaining(self, mock_revoke_license)
         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.
@@ -3164,17 +3166,27 @@ def test_bulk_revoke_license_not_found(self, mock_revoke_license):
             'user_emails': [
                 '[email protected]',
                 '[email protected]',  # There's no license for bob
+                '[email protected]',  # There's no license for john
             ],
         }
-        response = self.api_client.post(self.bulk_revoke_license_url, request_payload)
-
-        assert response.status_code == status.HTTP_404_NOT_FOUND
+        response = self.api_client.post(
+            self.bulk_revoke_license_url, request_payload)
+        assert response.status_code == status.HTTP_200_OK
         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)
+            "No license for email {} exists in plan "
+            "{} with a status in ['activated', 'assigned']. user_email: {}"
         )
-        self.assertEqual(expected_error_msg, response.json())
+        self.assertEqual(expected_error_msg.format(
+            '[email protected]',
+            self.subscription_plan.uuid,
+            '[email protected]'
+        ), response.json()
+            ['error_messages'][0]['error'])
+        self.assertEqual(expected_error_msg.format(
+            '[email protected]',
+            self.subscription_plan.uuid,
+            '[email protected]'
+        ), response.json()['error_messages'][1]['error'])
         mock_revoke_license.assert_called_once_with(alice_license)
 
     @mock.patch('license_manager.apps.api.v1.views.revoke_license')
@@ -3205,7 +3217,7 @@ def test_bulk_revoke_license_revocation_error(self, mock_revoke_license):
             alice_license.uuid,
             'floor is lava. user_email: [email protected]',
         )
-        self.assertEqual(expected_error_msg, response.json())
+        self.assertEqual(expected_error_msg, response.json()['error_messages'][0]['error'])
         mock_revoke_license.assert_called_once_with(alice_license)
 
     def test_revoke_all_no_valid_subscription_plan_superuser(self):
@@ -3252,7 +3264,7 @@ def test_revoke_all_happy_path(self, mock_revoke_all_licenses_task):
         """
         self._setup_request_jwt(user=self.super_user)
         response = self.api_client.post(self.revoke_all_licenses_url, {})
-        assert response.status_code == status.HTTP_204_NO_CONTENT
+        assert response.status_code == status.HTTP_200_OK
         mock_revoke_all_licenses_task.assert_called()
 
     @ddt.data(
@@ -3281,7 +3293,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 +3354,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

@@ -1355,25 +1355,48 @@ def bulk_revoke(self, request, subscription_uuid=None):
             return Response(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
+                    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)
 
-        if error_response_status:
-            return Response(error_message, status=error_response_status)
+        # Case 1: if all revocations failed; return only the error messages list
+        if error_response_status and not revocation_results:
+            return Response({
+                'error_messages': 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 = {
+            'revocation_results': revocation_succeeded,
+            'error_messages': error_messages
+        }
+        return Response(data=results, status=status.HTTP_200_OK)
 
     @action(detail=False, methods=['post'], url_path='revoke-all')
     def revoke_all(self, _, subscription_uuid=None):
@@ -1405,7 +1428,7 @@ def revoke_all(self, _, subscription_uuid=None):
                             status=status.HTTP_422_UNPROCESSABLE_ENTITY)
 
         revoke_all_licenses_task.delay(subscription_uuid)
-        return Response(status=status.HTTP_204_NO_CONTENT)
+        return Response(status=status.HTTP_200_OK)
 
     @action(detail=False, methods=['get'])
     def overview(self, request, subscription_uuid=None):  # pylint: disable=unused-argument