From b689ea0aca13356042cf1ac8b5bf6e8a4d612fc3 Mon Sep 17 00:00:00 2001 From: Alexander Dusenbery Date: Wed, 4 Sep 2024 11:22:23 -0400 Subject: [PATCH] feat: failed notifications for assignments leave state as allocated When the assignment email notification task fails, the corresponding assignment is now NOT moved to an errored state, but instead left in an allocated state. Furthermore, the admin-serializer for the assignment now indicates a "failed" _learner_ state, and an error reason related to the failed notification attempt. ENT-9037 --- .../content_assignments/assignment.py | 8 +- .../api/v1/tests/test_assignment_views.py | 48 ++++++++ .../apps/content_assignments/models.py | 14 +++ .../apps/content_assignments/tasks.py | 8 ++ .../content_assignments/tests/test_tasks.py | 111 +++++++++++++----- 5 files changed, 158 insertions(+), 31 deletions(-) diff --git a/enterprise_access/apps/api/serializers/content_assignments/assignment.py b/enterprise_access/apps/api/serializers/content_assignments/assignment.py index b2eddc72..77b8503f 100644 --- a/enterprise_access/apps/api/serializers/content_assignments/assignment.py +++ b/enterprise_access/apps/api/serializers/content_assignments/assignment.py @@ -184,8 +184,12 @@ def get_error_reason(self, assignment): Resolves the error reason for the assignment, if any, for display purposes based on any associated actions. """ - # If the assignment is not in an errored state, there should be no error reason. - if assignment.state != LearnerContentAssignmentStateChoices.ERRORED: + # If the assignment is not in an errored or allocated state, + # there should be no error reason. + if assignment.state not in ( + LearnerContentAssignmentStateChoices.ERRORED, + LearnerContentAssignmentStateChoices.ALLOCATED, + ): return None # Assignment is an errored state, so determine the appropriate error reason so clients don't need to. diff --git a/enterprise_access/apps/api/v1/tests/test_assignment_views.py b/enterprise_access/apps/api/v1/tests/test_assignment_views.py index 327fe9e0..c156d214 100644 --- a/enterprise_access/apps/api/v1/tests/test_assignment_views.py +++ b/enterprise_access/apps/api/v1/tests/test_assignment_views.py @@ -12,6 +12,7 @@ from enterprise_access.apps.content_assignments.constants import ( NUM_DAYS_BEFORE_AUTO_EXPIRATION, + AssignmentActionErrors, AssignmentActions, AssignmentAutomaticExpiredReason, AssignmentLearnerStates, @@ -448,6 +449,53 @@ def test_retrieve(self, role_context_dict, mock_subsidy_record, mock_catalog_cli }, } + @ddt.data( + # A good admin role, and with a context matching the main testing customer. + {'system_wide_role': SYSTEM_ENTERPRISE_ADMIN_ROLE, 'context': str(TEST_ENTERPRISE_UUID)}, + # A good operator role, and with a context matching the main testing customer. + {'system_wide_role': SYSTEM_ENTERPRISE_OPERATOR_ROLE, 'context': str(TEST_ENTERPRISE_UUID)}, + ) + @mock.patch('enterprise_access.apps.content_metadata.api.EnterpriseCatalogApiClient', autospec=True) + @mock.patch.object(SubsidyAccessPolicy, 'subsidy_record', autospec=True) + def test_retrieve_allocated_with_notification_error( + self, role_context_dict, mock_subsidy_record, mock_catalog_client + ): + assignment = LearnerContentAssignmentFactory( + state=LearnerContentAssignmentStateChoices.ALLOCATED, + lms_user_id=98123, + transaction_uuid=None, + assignment_configuration=self.assignment_configuration, + content_key='edX+edXPrivacy101', + content_quantity=-321, + content_title='edx: Privacy 101' + ) + assignment.add_errored_notified_action(Exception('foo')) + + # Set the JWT-based auth that we'll use for every request. + self.set_jwt_cookie([role_context_dict]) + + # Mock results from the catalog content metadata API endpoint. + mock_catalog_client.return_value.catalog_content_metadata.return_value = self.mock_catalog_result + + # Mock results from the subsidy record. + mock_subsidy_record.return_value = self.mock_subsidy_record + + # Setup and call the retrieve endpoint. + detail_kwargs = { + 'assignment_configuration_uuid': str(TEST_ASSIGNMENT_CONFIG_UUID), + 'uuid': str(assignment.uuid), + } + detail_url = reverse('api:v1:admin-assignments-detail', kwargs=detail_kwargs) + response = self.client.get(detail_url) + + assert response.status_code == status.HTTP_200_OK + assert response.json().get('state') == LearnerContentAssignmentStateChoices.ALLOCATED + assert response.json().get('learner_state') == AssignmentLearnerStates.FAILED + assert response.json().get('error_reason') == { + 'action_type': AssignmentActions.NOTIFIED, + 'error_reason': AssignmentActionErrors.EMAIL_ERROR, + } + @ddt.data( # A good admin role, and with a context matching the main testing customer. {'system_wide_role': SYSTEM_ENTERPRISE_ADMIN_ROLE, 'context': str(TEST_ENTERPRISE_UUID)}, diff --git a/enterprise_access/apps/content_assignments/models.py b/enterprise_access/apps/content_assignments/models.py index 1d8a41a3..9b381bed 100644 --- a/enterprise_access/apps/content_assignments/models.py +++ b/enterprise_access/apps/content_assignments/models.py @@ -784,9 +784,23 @@ def annotate_dynamic_fields_onto_queryset(cls, queryset): error_reason__isnull=True, completed_at__isnull=False, ) + ), + # ... or if they have an errored notification. + has_errored_notification=Exists( + LearnerContentAssignmentAction.objects.filter( + assignment=OuterRef('uuid'), + action_type=AssignmentActions.NOTIFIED, + error_reason__isnull=False, + ) ) ).annotate( learner_state=Case( + When( + Q(state=LearnerContentAssignmentStateChoices.ALLOCATED) & + Q(has_errored_notification=True) & + Q(has_notification=False), + then=Value(AssignmentLearnerStates.FAILED), + ), When( Q(state=LearnerContentAssignmentStateChoices.ALLOCATED) & Q(has_notification=False), then=Value(AssignmentLearnerStates.NOTIFYING), diff --git a/enterprise_access/apps/content_assignments/tasks.py b/enterprise_access/apps/content_assignments/tasks.py index 0ae95208..2e2a1bff 100644 --- a/enterprise_access/apps/content_assignments/tasks.py +++ b/enterprise_access/apps/content_assignments/tasks.py @@ -458,6 +458,14 @@ class SendNotificationEmailTask(BaseAssignmentRetryAndErrorActionTask): def add_errored_action(self, assignment, exc): assignment.add_errored_notified_action(exc) + def progress_state_on_failure(self, assignment): + """ + Skip progressing the assignment state to `failed` (keeping it `allocated`) + so that the assignment remains functional and redeemable + for learners and appears as actionable to admins. + """ + logger.info('NOT progressing the assignment state to failed for notification failures.') + @shared_task(base=SendNotificationEmailTask) def send_email_for_new_assignment(new_assignment_uuid): diff --git a/enterprise_access/apps/content_assignments/tests/test_tasks.py b/enterprise_access/apps/content_assignments/tests/test_tasks.py index c8f2ed5b..8e037799 100644 --- a/enterprise_access/apps/content_assignments/tests/test_tasks.py +++ b/enterprise_access/apps/content_assignments/tests/test_tasks.py @@ -39,6 +39,8 @@ from enterprise_access.utils import get_automatic_expiration_date_and_reason from test_utils import APITestWithMocks +TEST_CONTENT_KEY = 'course:test_content' +TEST_ENTERPRISE_CUSTOMER_NAME = 'test-customer-name' TEST_ENTERPRISE_UUID = uuid4() TEST_EMAIL = 'foo@bar.com' TEST_LMS_USER_ID = 2 @@ -215,13 +217,37 @@ def setUpTestData(cls): assignment_configuration=cls.assignment_configuration, spend_limit=10000000, ) + cls.mock_enterprise_customer_data = { + 'uuid': TEST_ENTERPRISE_UUID, + 'slug': 'test-slug', + 'admin_users': [{ + 'email': 'test@admin.com', + 'lms_user_id': 1 + }], + 'name': TEST_ENTERPRISE_CUSTOMER_NAME, + } + cls.mock_content_metadata = { + 'key': TEST_CONTENT_KEY, + 'normalized_metadata': { + 'start_date': '2020-01-01T12:00:00Z', + 'end_date': '2022-01-01 12:00:00Z', + 'enroll_by_date': '2021-01-01T12:00:00Z', + 'content_price': 123, + }, + 'owners': [ + {'name': 'Smart Folks', 'logo_image_url': 'http://pictures.yes'}, + {'name': 'Good People', 'logo_image_url': 'http://pictures.nice'}, + ], + 'card_image_url': 'https://itsanimage.com', + } def setUp(self): super().setUp() self.course_name = 'test-course-name' - self.enterprise_customer_name = 'test-customer-name' + self.enterprise_customer_name = TEST_ENTERPRISE_CUSTOMER_NAME self.assignment = LearnerContentAssignmentFactory( uuid=TEST_ASSIGNMENT_UUID, + content_key=TEST_CONTENT_KEY, learner_email='TESTING THIS EMAIL', lms_user_id=TEST_LMS_USER_ID, assignment_configuration=self.assignment_configuration, @@ -386,36 +412,13 @@ def test_send_email_for_new_assignment( """ Verify send_email_for_new_assignment hits braze client with expected args """ - admin_email = 'test@admin.com' - mock_lms_client.return_value.get_enterprise_customer_data.return_value = { - 'uuid': TEST_ENTERPRISE_UUID, - 'slug': 'test-slug', - 'admin_users': [{ - 'email': admin_email, - 'lms_user_id': 1 - }], - 'name': self.enterprise_customer_name, - } + mock_lms_client.return_value.get_enterprise_customer_data.return_value = self.mock_enterprise_customer_data mock_recipient = { 'external_user_id': 1 } - mock_metadata = { - 'key': self.assignment.content_key, - 'normalized_metadata': { - 'start_date': '2020-01-01T12:00:00Z', - 'end_date': '2022-01-01 12:00:00Z', - 'enroll_by_date': '2021-01-01T12:00:00Z', - 'content_price': 123, - }, - 'owners': [ - {'name': 'Smart Folks', 'logo_image_url': 'http://pictures.yes'}, - {'name': 'Good People', 'logo_image_url': 'http://pictures.nice'}, - ], - 'card_image_url': 'https://itsanimage.com', - } mock_catalog_client.return_value.catalog_content_metadata.return_value = { 'count': 1, - 'results': [mock_metadata] + 'results': [self.mock_content_metadata] } # Set the subsidy expiration time to tomorrow @@ -425,7 +428,8 @@ def test_send_email_for_new_assignment( } mock_subsidy_client.retrieve_subsidy.return_value = mock_subsidy - mock_admin_mailto = f'mailto:{admin_email}' + admin_email = self.mock_enterprise_customer_data['admin_users'][0]['email'] + mock_admin_mailto = f"mailto:{admin_email}" mock_braze_client.return_value.create_recipient.return_value = mock_recipient mock_braze_client.return_value.generate_mailto_link.return_value = mock_admin_mailto @@ -446,13 +450,62 @@ def test_send_email_for_new_assignment( 'enrollment_deadline': 'Jan 01, 2021', 'start_date': 'Jan 01, 2020', 'course_partner': 'Smart Folks and Good People', - 'course_card_image': 'https://itsanimage.com', - 'learner_portal_link': '{}/{}'.format(settings.ENTERPRISE_LEARNER_PORTAL_URL, 'test-slug'), + 'course_card_image': self.mock_content_metadata['card_image_url'], + 'learner_portal_link': '{}/{}'.format( + settings.ENTERPRISE_LEARNER_PORTAL_URL, + self.mock_enterprise_customer_data['slug'] + ), 'action_required_by_timestamp': '2021-01-01T12:00:00Z' }, ) assert mock_braze_client.return_value.send_campaign_message.call_count == 1 + @mock.patch('enterprise_access.apps.subsidy_access_policy.models.SubsidyAccessPolicy.subsidy_client') + @mock.patch('enterprise_access.apps.content_metadata.api.EnterpriseCatalogApiClient') + @mock.patch('enterprise_access.apps.content_assignments.tasks.LmsApiClient') + @mock.patch('enterprise_access.apps.content_assignments.tasks.BrazeApiClient') + def test_send_email_for_new_assignment_failure( + self, + mock_braze_client, + mock_lms_client, + mock_catalog_client, + mock_subsidy_client, + ): + """ + Verify send_email_for_new_assignment does not change the state of the + assignment record on failure. + """ + mock_lms_client.return_value.get_enterprise_customer_data.return_value = self.mock_enterprise_customer_data + mock_recipient = { + 'external_user_id': 1 + } + mock_catalog_client.return_value.catalog_content_metadata.return_value = { + 'count': 1, + 'results': [self.mock_content_metadata] + } + + # Set the subsidy expiration time to tomorrow + mock_subsidy = { + 'uuid': self.policy.subsidy_uuid, + 'expiration_datetime': (now() + timedelta(days=1)).strftime('%Y-%m-%d %H:%M:%SZ'), + } + mock_subsidy_client.retrieve_subsidy.return_value = mock_subsidy + + braze_client_instance = mock_braze_client.return_value + braze_client_instance.send_campaign_message.side_effect = Exception('foo') + + admin_email = self.mock_enterprise_customer_data['admin_users'][0]['email'] + mock_admin_mailto = f"mailto:{admin_email}" + braze_client_instance.create_recipient.return_value = mock_recipient + braze_client_instance.generate_mailto_link.return_value = mock_admin_mailto + + send_email_for_new_assignment.delay(self.assignment.uuid) + self.assertEqual(self.assignment.state, LearnerContentAssignmentStateChoices.ALLOCATED) + self.assertTrue(self.assignment.actions.filter( + error_reason=AssignmentActionErrors.EMAIL_ERROR, + action_type=AssignmentActions.NOTIFIED, + ).exists()) + @mock.patch('enterprise_access.apps.subsidy_access_policy.models.SubsidyAccessPolicy.objects') @mock.patch('enterprise_access.apps.content_assignments.tasks.LmsApiClient') @mock.patch('enterprise_access.apps.content_assignments.tasks.BrazeApiClient')