From 65a9531a4ec290729fb1b06f17079fc8317fb1da Mon Sep 17 00:00:00 2001 From: Alie Langston Date: Mon, 15 Nov 2021 09:31:16 -0500 Subject: [PATCH] fix: reduce number of celery tasks for idv and proctoring updates --- CHANGELOG.rst | 5 + edx_name_affirmation/__init__.py | 2 +- edx_name_affirmation/handlers.py | 72 +++++++--- edx_name_affirmation/tasks.py | 151 ++++++++++++++++++++ edx_name_affirmation/tests/test_handlers.py | 52 ++++++- 5 files changed, 255 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4dbeaea..ed1a130 100644 --- a/CHANGELOG.rst +++ b/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. diff --git a/edx_name_affirmation/__init__.py b/edx_name_affirmation/__init__.py index 15089ce..f682cf3 100644 --- a/edx_name_affirmation/__init__.py +++ b/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 diff --git a/edx_name_affirmation/handlers.py b/edx_name_affirmation/handlers.py index 6d1a6ac..2c9c949 100644 --- a/edx_name_affirmation/handlers.py +++ b/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, + } + ) diff --git a/edx_name_affirmation/tasks.py b/edx_name_affirmation/tasks.py index 1de4405..87936a0 100644 --- a/edx_name_affirmation/tasks.py +++ b/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, + ) + ) diff --git a/edx_name_affirmation/tests/test_handlers.py b/edx_name_affirmation/tests/test_handlers.py index 4edb9f4..235ded9 100644 --- a/edx_name_affirmation/tests/test_handlers.py +++ b/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()