Skip to content

Commit

Permalink
feat: Modify License Revocation Endpoint Behavior (#698)
Browse files Browse the repository at this point in the history 
* feat: continue revocation job instead of breaking on error
* fix: consistent response objects
* feat: standardize bulk-revoke endpoint
* chore: update docstring
  • Loading branch information
@hamzawaleed01
hamzawaleed01 authored Sep 10, 2024
1 parent 4cb33cc commit 99289ff
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 38 deletions.
2 changes: 1 addition & 1 deletion license_manager/apps/api/v1/tests/test_api_eventing.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 31 additions & 15 deletions license_manager/apps/api/v1/tests/test_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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([
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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.
Expand All @@ -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')
Expand All @@ -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)

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
99 changes: 77 additions & 22 deletions license_manager/apps/api/v1/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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:
Expand All @@ -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):
Expand Down

0 comments on commit 99289ff

Please sign in to comment.