Skip to content

Commit

Permalink
feat: continue revocation job instead of breaking on error
Browse files Browse the repository at this point in the history
  • Loading branch information
@hamzawaleed01
hamzawaleed01 committed Aug 20, 2024
1 parent 678a0a9 commit 392ece9
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 23 deletions.
41 changes: 26 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_201_CREATED
# 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_201_CREATED

@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_201_CREATED
# 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_201_CREATED

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 @@ -3146,8 +3146,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.
Expand All @@ -3164,17 +3165,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_201_CREATED
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')
Expand Down Expand Up @@ -3205,7 +3216,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):
Expand Down Expand Up @@ -3281,7 +3292,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_201_CREATED
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 +3353,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_201_CREATED
mock_revoke_course_enrollments_for_user_task.assert_called()

second_detail_response = _subscriptions_detail_request(
Expand Down
36 changes: 28 additions & 8 deletions license_manager/apps/api/v1/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1355,25 +1355,45 @@ 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_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
}
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:
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'])
}
)
results = {
'revocation_results': revocation_succeeded,
'error_messages': error_messages
}
return Response(data=results, status=status.HTTP_201_CREATED)

@action(detail=False, methods=['post'], url_path='revoke-all')
def revoke_all(self, _, subscription_uuid=None):
Expand Down

0 comments on commit 392ece9

Please sign in to comment.