Merge pull request #67 from edx/alangsto/reduce_celery_tasks

fix: reduce number of celery tasks for idv and proctoring updates

CHANGELOG.rst

@@ -14,6 +14,11 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[2.0.1] - 2021-11-15
+~~~~~~~~~~~~~~~~~~~~
+* If we receive a non-relevant status for either IDV or proctoring, do not
+  trigger a celery task.
+
 [2.0.0] - 2021-10-27
 ~~~~~~~~~~~~~~~~~~~~~
 * Remove VERIFIED_NAME_FLAG and all references to it.

edx_name_affirmation/__init__.py

@@ -2,6 +2,6 @@
 Django app housing name affirmation logic.
 """
 
-__version__ = '2.0.0'
+__version__ = '2.0.1'
 
 default_app_config = 'edx_name_affirmation.apps.EdxNameAffirmationConfig'  # pylint: disable=invalid-name

edx_name_affirmation/handlers.py

@@ -42,16 +42,28 @@ def idv_attempt_handler(attempt_id, user_id, status, photo_id_name, full_name, *
         photo_id_name(str): name to be used as verified name
         full_name(str): user's pending name change or current profile name
     """
+    trigger_status = VerifiedNameStatus.trigger_state_change_from_idv(status)
 
-    log.info('VerifiedName: idv_attempt_handler triggering Celery task for user %(user_id)s '
-             'with photo_id_name %(photo_id_name)s and status %(status)s',
-             {
-                 'user_id': user_id,
-                 'photo_id_name': photo_id_name,
-                 'status': status
-             }
-             )
-    idv_update_verified_name.delay(attempt_id, user_id, status, photo_id_name, full_name)
+    # only trigger celery task if status is relevant to name affirmation
+    if trigger_status:
+        log.info('VerifiedName: idv_attempt_handler triggering Celery task for user %(user_id)s '
+                 'with photo_id_name %(photo_id_name)s and status %(status)s',
+                 {
+                     'user_id': user_id,
+                     'photo_id_name': photo_id_name,
+                     'status': status
+                 }
+                 )
+        idv_update_verified_name.delay(attempt_id, user_id, status, photo_id_name, full_name)
+    else:
+        log.info('VerifiedName: idv_attempt_handler will not trigger Celery task for user %(user_id)s '
+                 'with photo_id_name %(photo_id_name)s because of status %(status)s',
+                 {
+                     'user_id': user_id,
+                     'photo_id_name': photo_id_name,
+                     'status': status
+                 }
+                 )
 
 
 def proctoring_attempt_handler(
@@ -78,13 +90,35 @@ def proctoring_attempt_handler(
         is_proctored(boolean): if the exam attempt is for a proctored exam
         backend_supports_onboarding(boolean): if the exam attempt is for an exam with a backend that supports onboarding
     """
-    proctoring_update_verified_name.delay(
-        attempt_id,
-        user_id,
-        status,
-        full_name,
-        profile_name,
-        is_practice_exam,
-        is_proctored,
-        backend_supports_onboarding
-    )
+
+    # We only care about updates from onboarding exams, or from non-practice proctored exams with a backend that
+    # does not support onboarding. This is because those two event types are guaranteed to contain verification events,
+    # whereas timed exams and proctored exams with a backend that does support onboarding are not guaranteed
+    is_onboarding_exam = is_practice_exam and is_proctored and backend_supports_onboarding
+    reviewable_proctored_exam = is_proctored and not is_practice_exam and not backend_supports_onboarding
+    if not (is_onboarding_exam or reviewable_proctored_exam):
+        return
+
+    trigger_status = VerifiedNameStatus.trigger_state_change_from_proctoring(status)
+
+    # only trigger celery task if status is relevant to name affirmation
+    if trigger_status:
+        proctoring_update_verified_name.delay(
+            attempt_id,
+            user_id,
+            status,
+            full_name,
+            profile_name,
+            is_practice_exam,
+            is_proctored,
+            backend_supports_onboarding
+        )
+    else:
+        log.info('VerifiedName: proctoring_attempt_handler will not trigger Celery task for user %(user_id)s '
+                 'with profile_name %(profile_name)s because of status %(status)s',
+                 {
+                     'user_id': user_id,
+                     'profile_name': profile_name,
+                     'status': status,
+                 }
+                 )

edx_name_affirmation/tasks.py

@@ -96,6 +96,79 @@ def idv_update_verified_name(self, attempt_id, user_id, status, photo_id_name, f
         )
 
 
+@shared_task(
+    bind=True, autoretry_for=(Exception,), default_retry_delay=DEFAULT_RETRY_SECONDS, max_retries=MAX_RETRIES,
+)
+@set_code_owner_attribute
+def idv_update_verified_name_task(self, attempt_id, user_id, name_affirmation_status, photo_id_name, full_name):
+    """
+    Celery task for updating a verified name based on an IDV attempt
+    """
+    log.info('VerifiedName: idv_update_verified_name triggering Celery task started for user %(user_id)s '
+             'with attempt_id %(attempt_id)s and status %(status)s',
+             {
+                'user_id': user_id,
+                'attempt_id': attempt_id,
+                'status': name_affirmation_status
+             }
+             )
+    verified_names = VerifiedName.objects.filter(user__id=user_id, verified_name=photo_id_name).order_by('-created')
+    if verified_names:
+        # if there are VerifiedName objects, we want to update existing entries
+        # for each attempt with no attempt id (either proctoring or idv), update attempt id
+        updated_for_attempt_id = verified_names.filter(
+            proctored_exam_attempt_id=None,
+            verification_attempt_id=None
+        ).update(verification_attempt_id=attempt_id)
+
+        if updated_for_attempt_id:
+            log.info(
+                'Updated VerifiedNames for user={user_id} to verification_attempt_id={attempt_id}'.format(
+                    user_id=user_id,
+                    attempt_id=attempt_id,
+                )
+            )
+
+        # then for all matching attempt ids, update the status
+        verified_name_qs = verified_names.filter(
+            verification_attempt_id=attempt_id,
+            proctored_exam_attempt_id=None
+        )
+
+        # Individually update to ensure that post_save signals send
+        for verified_name_obj in verified_name_qs:
+            verified_name_obj.status = name_affirmation_status
+            verified_name_obj.save()
+
+        log.info(
+            'Updated VerifiedNames for user={user_id} with verification_attempt_id={attempt_id} to '
+            'have status={status}'.format(
+                user_id=user_id,
+                attempt_id=attempt_id,
+                status=name_affirmation_status
+            )
+        )
+    else:
+        # otherwise if there are no entries, we want to create one.
+        user = User.objects.get(id=user_id)
+        verified_name = VerifiedName.objects.create(
+            user=user,
+            verified_name=photo_id_name,
+            profile_name=full_name,
+            verification_attempt_id=attempt_id,
+            status=name_affirmation_status,
+        )
+        log.error(
+            'Created VerifiedName for user={user_id} to have status={status} '
+            'and verification_attempt_id={attempt_id}, because no matching '
+            'attempt_id or verified_name were found.'.format(
+                user_id=user_id,
+                attempt_id=attempt_id,
+                status=verified_name.status
+            )
+        )
+
+
 @shared_task(
     bind=True, autoretry_for=(Exception,), default_retry_delay=DEFAULT_RETRY_SECONDS, max_retries=MAX_RETRIES,
 )
@@ -187,3 +260,81 @@ def proctoring_update_verified_name(
                     attempt_id=attempt_id,
                 )
             )
+
+
+@shared_task(
+    bind=True, autoretry_for=(Exception,), default_retry_delay=DEFAULT_RETRY_SECONDS, max_retries=MAX_RETRIES,
+)
+@set_code_owner_attribute
+def proctoring_update_verified_name_task(
+    self,
+    attempt_id,
+    user_id,
+    name_affirmation_status,
+    full_name,
+    profile_name
+):
+    """
+    Celery task for updating a verified name based on a proctoring attempt
+    """
+
+    # check if approved VerifiedName already exists for the user
+    verified_name = VerifiedName.objects.filter(
+        user__id=user_id,
+        status=VerifiedNameStatus.APPROVED
+    ).order_by('-created').first()
+    if verified_name:
+        approved_verified_name = verified_name.verified_name
+        is_full_name_approved = approved_verified_name == full_name
+        if not is_full_name_approved:
+            log.warning(
+                'Full name for proctored_exam_attempt_id={attempt_id} is not equal '
+                'to the most recent verified name verified_name_id={name_id}.'.format(
+                    attempt_id=attempt_id,
+                    name_id=verified_name.id
+                )
+            )
+        return
+
+    verified_name = VerifiedName.objects.filter(
+        user__id=user_id,
+        proctored_exam_attempt_id=attempt_id
+    ).order_by('-created').first()
+    if verified_name:
+        verified_name.status = name_affirmation_status
+        verified_name.save()
+        log.info(
+            'Updated VerifiedName for user={user_id} with proctored_exam_attempt_id={attempt_id} '
+            'to have status={status}'.format(
+                user_id=user_id,
+                attempt_id=attempt_id,
+                status=name_affirmation_status
+            )
+        )
+    else:
+        if full_name and profile_name:
+            # if they do not already have an approved VerifiedName, create one
+            user = User.objects.get(id=user_id)
+            VerifiedName.objects.create(
+                user=user,
+                verified_name=full_name,
+                proctored_exam_attempt_id=attempt_id,
+                status=name_affirmation_status,
+                profile_name=profile_name
+            )
+            log.info(
+                'Created VerifiedName for user={user_id} to have status={status} '
+                'and proctored_exam_attempt_id={attempt_id}'.format(
+                    user_id=user_id,
+                    attempt_id=attempt_id,
+                    status=name_affirmation_status
+                )
+            )
+        else:
+            log.error(
+                'Cannot create VerifiedName for user={user_id} for proctored_exam_attempt_id={attempt_id} '
+                'because neither profile name nor full name were provided'.format(
+                    user_id=user_id,
+                    attempt_id=attempt_id,
+                )
+            )

edx_name_affirmation/tests/test_handlers.py

@@ -90,7 +90,6 @@ def test_idv_create_verified_name(self):
 
     @ddt.data(
         ('created', VerifiedNameStatus.PENDING),
-        ('must_retry', VerifiedNameStatus.PENDING),
         ('submitted', VerifiedNameStatus.SUBMITTED),
         ('approved', VerifiedNameStatus.APPROVED),
         ('denied', VerifiedNameStatus.DENIED)
@@ -163,7 +162,6 @@ def test_idv_does_not_update_verified_name_by_proctoring(self):
 
     @ddt.data(
         ('created', VerifiedNameStatus.PENDING),
-        ('must_retry', VerifiedNameStatus.PENDING),
         ('submitted', VerifiedNameStatus.SUBMITTED),
         ('approved', VerifiedNameStatus.APPROVED),
         ('denied', VerifiedNameStatus.DENIED)
@@ -197,8 +195,26 @@ def test_idv_update_one_verified_name(self, idv_status, expected_status):
             if expected_status == VerifiedNameStatus.APPROVED:
                 mock_signal.assert_called()
             else:
-                with self.assertRaises(AssertionError):
-                    mock_signal.assert_called()
+                mock_signal.assert_not_called()
+
+    @ddt.data(
+        'ready',
+        'must_retry',
+    )
+    @patch('edx_name_affirmation.tasks.idv_update_verified_name.delay')
+    def test_idv_non_trigger_status(self, status, mock_task):
+        """
+        Test that a celery task is not triggered if a non-relevant status is received
+        """
+        idv_attempt_handler(
+            self.idv_attempt_id,
+            self.user.id,
+            status,
+            self.verified_name,
+            self.profile_name
+        )
+
+        mock_task.assert_not_called()
 
 
 @ddt.ddt
@@ -209,7 +225,6 @@ class ProctoringSignalTests(SignalTestCase):
 
     @ddt.data(
         ('created', VerifiedNameStatus.PENDING),
-        ('started', VerifiedNameStatus.PENDING),
         ('submitted', VerifiedNameStatus.SUBMITTED),
         ('verified', VerifiedNameStatus.APPROVED),
         ('rejected', VerifiedNameStatus.DENIED)
@@ -265,8 +280,6 @@ def test_proctoring_create_verified_name(self):
         verified_name = verified_name_query.first()
         self.assertEqual(verified_name.status, VerifiedNameStatus.PENDING)
 
-    # test for log
-
     @ddt.data(
         (None, None, True, True, True),
         ('John', 'John', False, False, False),
@@ -345,3 +358,28 @@ def test_proctoring_log_with_existing_approved_verified_name(self, should_names_
             # check that log is not called if the names do not differ
             with self.assertRaises(AssertionError):
                 mock_logger.assert_called_with(log_str)
+
+    @ddt.data(
+        'download_software_clicked',
+        'ready_to_start',
+        'started',
+        'ready_to_submit',
+        'error',
+    )
+    @patch('edx_name_affirmation.tasks.proctoring_update_verified_name.delay')
+    def test_proctoring_non_trigger_status(self, status, mock_task):
+        """
+        Test that a celery task is not triggered if a non-relevant status is received
+        """
+        proctoring_attempt_handler(
+            self.proctoring_attempt_id,
+            self.user.id,
+            status,
+            self.verified_name,
+            self.profile_name,
+            True,
+            True,
+            True
+        )
+
+        mock_task.assert_not_called()