From 7ffdae90be5b6d65c50c0da41d7139cfd066ca22 Mon Sep 17 00:00:00 2001 From: michaelroytman Date: Thu, 28 Oct 2021 12:47:49 -0400 Subject: [PATCH] feat: Add Management Command to Re-Emit SoftwareSecurePhotoVerification post_save Signal The verify_student Django app contains a Signal receiver that receives the SoftwareSecurePhotoVerification post_save signal. It then emits an idv_update_signal to communicate that a change to IDV has occured. This Signal is received by the nameaffirmation app, which uses it to start a Celery task to create or update VerifiedName records based on the changes to the SoftwareSecurePhotoVerification model. During the phased roll out of the Verified Name feature, due to the way percentage rollout is implemented by django-waffle and the way SoftwareSecurePhotoVerifications are updated, a mismatch of states occured where created VerifiedNames stayed in the "pending" state, even as the corresponding SoftwareSecurePhotoVerifications moved into "submitted", "approved", or "denied". Please see below for additional details. This management commands takes as an argument a list of SoftwareSecurePhotoVerification IDs verification-ids, as well as a batch-size and sleep-time. In batches of batch-size, the command saves the SoftwareSecurePhotoVerification associated with the IDs argument. Each batch is separated by a pause of sleep_time in seconds. By saving each SoftwareSecurePhotoVerification, the post_save signal is re-emitted, thereby re-emitting the idv_update_signal. Now that the aforementioned bug has been fixed, this will correctly trigger the Celery task and sync the SoftwareSecurePhotoVerification and VerifiedName objects. Please find additional details about the bug below. The release reached a percentage of 50% before it was observed that, due to the way percentage roll out works in django-waffle, the code to create or update VerifiedName records was not working properly. The code was written such that any change to a SoftwareSecurePhotoVerification model instance sent a signal, which was received and handled by the Name Affirmation application. If the VERIFIED_NAME_FLAG was on for the requesting user, a Celery task was launched from the Name Affirmation application to perform the creation of or update to the appropriate VerifiedName model instances based on the verify_student application signal. However, we observed that when SoftwareSecurePhotoVerification records were moved into the "created" or "ready" status, a Celery task in Name Affirmation was created, but when SoftwareSecurePhotoVerification records were moved into the "submitted" status, the corresponding Celery task in Name Affirmation was not created. This caused VerifiedName records to stay in the "pending" state. The django-waffle waffle flag used by the edx-toggle library implements percentage rollout by setting a cookie in a learner's browser session to assign them to the enabled or disabled group. It turns out that the code that submits a SoftwareSecurePhotoVerification record, which moves it into the "submitted" state, happens as part of a Celery task in the verify_student application in the edx-platform. Therefore, we believe that because there is no request object in a Celery task, the edx-toggle code is defaulting to the case where there is no request object. In this case, the code checks whether the flag is enabled for everyone when determining whether the flag is enabled. Because of the percentage rollout (i.e. waffle flag not enabled for everyone), the Celery task in Name Affirmation is not created. This behavior was confirmed by logging added as part of https://github.com/edx/edx-name-affirmation/pull/62. [MST-1130](https://openedx.atlassian.net/browse/MST-1130) --- ...curephotoverifications_post_save_signal.py | 73 +++++++++++++++++++ ...curephotoverifications_post_save_signal.py | 68 +++++++++++++++++ 2 files changed, 141 insertions(+) create mode 100644 lms/djangoapps/verify_student/management/commands/tests/test_trigger_softwaresecurephotoverifications_post_save_signal.py create mode 100644 lms/djangoapps/verify_student/management/commands/trigger_softwaresecurephotoverifications_post_save_signal.py diff --git a/lms/djangoapps/verify_student/management/commands/tests/test_trigger_softwaresecurephotoverifications_post_save_signal.py b/lms/djangoapps/verify_student/management/commands/tests/test_trigger_softwaresecurephotoverifications_post_save_signal.py new file mode 100644 index 0000000000..3ea911651b --- /dev/null +++ b/lms/djangoapps/verify_student/management/commands/tests/test_trigger_softwaresecurephotoverifications_post_save_signal.py @@ -0,0 +1,73 @@ +""" +Tests for django admin command `trigger_softwaresecurephotoverifications_post_save_signal` +in the verify_student module +""" + + +from django.conf import settings +from django.core.management import call_command + +from freezegun import freeze_time +from unittest.mock import call, patch, ANY + +from common.test.utils import MockS3BotoMixin +from lms.djangoapps.verify_student.tests import TestVerificationBase +from lms.djangoapps.verify_student.tests.test_models import ( + FAKE_SETTINGS, + mock_software_secure_post, +) + + +# Lots of patching to stub in our own settings, and HTTP posting +@patch.dict(settings.VERIFY_STUDENT, FAKE_SETTINGS) +@patch.dict(settings.FEATURES, {'AUTOMATIC_VERIFY_STUDENT_IDENTITY_FOR_TESTING': True}) +@patch('lms.djangoapps.verify_student.models.requests.post', new=mock_software_secure_post) +class TestTriggerSoftwareSecurePhotoVerificationsPostSaveSignal(MockS3BotoMixin, TestVerificationBase): + """ + Tests for django admin command `trigger_softwaresecurephotoverifications_post_save_signal` + in the verify_student module + """ + def setUp(self): + super().setUp() + with freeze_time("2021-10-30 12:00:00"): + self._create_attempts(4) + with freeze_time("2021-11-01 03:00:00"): + self._create_attempts(4) + + def _create_attempts(self, num_attempts): + for _ in range(num_attempts): + self.create_and_submit_attempt_for_user() + + @patch('lms.djangoapps.verify_student.signals.idv_update_signal.send') + def test_command(self, send_idv_update_mock): + call_command('trigger_softwaresecurephotoverifications_post_save_signal', start_date_time='2021-10-31 06:00:00') + + # The UserFactory instantiates first_name and last_name using a Sequence, which provide integers to + # generate unique values. A Sequence maintains its state across test runs, so the value of a given Sequence, + # and therefore, the value of photo_id_name and full_name, depends on the order that tests using the UserFactory + # run, as the Sequence is not reset after each test suite. This makes asserting on the exact value + # of photo_id_name and full_name, derived from first_name and last_name, difficult. + # Resetting the Sequence with UserFactory.reset_sequence causes failures in other tests that rely on + # these values. In any case, we primarily care about the signal being called with the correct sender, + # attempt_id, and user_id, as the signal is tested elsewhere. + expected_calls = [ + call( + sender='idv_update', attempt_id=5, user_id=5, status='must_retry', + photo_id_name=ANY, full_name=ANY + ), + call( + sender='idv_update', attempt_id=6, user_id=6, status='must_retry', + photo_id_name=ANY, full_name=ANY + ), + call( + sender='idv_update', attempt_id=7, user_id=7, status='must_retry', + photo_id_name=ANY, full_name=ANY + ), + call( + sender='idv_update', attempt_id=8, user_id=8, status='must_retry', + photo_id_name=ANY, full_name=ANY + ), + ] + print(send_idv_update_mock.mock_calls) + self.assertEqual(send_idv_update_mock.call_count, 4) + send_idv_update_mock.assert_has_calls(expected_calls, any_order=True) diff --git a/lms/djangoapps/verify_student/management/commands/trigger_softwaresecurephotoverifications_post_save_signal.py b/lms/djangoapps/verify_student/management/commands/trigger_softwaresecurephotoverifications_post_save_signal.py new file mode 100644 index 0000000000..715ed0bf68 --- /dev/null +++ b/lms/djangoapps/verify_student/management/commands/trigger_softwaresecurephotoverifications_post_save_signal.py @@ -0,0 +1,68 @@ +""" +Django admin command to save SoftwareSecurePhotoVerifications given an iterable of +verification_ids, thereby re-emitting the post_save signal. +""" + +import datetime +import logging +import time + +from django.core.management.base import BaseCommand + +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification + +logger = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + Command to save SoftwareSecurePhotoVerification model instances based on a provided interable of verifiction_ids. + This re-emits the post_save signal. + """ + + def add_arguments(self, parser): + parser.add_argument( + '--start_date_time', + action='store', + dest='start_date_time', + type=str, + help='First date time for SoftwareSecurePhotoVerifications to resave; ' + 'should be formatted as 2020-12-02 00:00:00.' + ) + parser.add_argument( + '--batch_size', + action='store', + dest='batch_size', + type=int, + default=300, + help='Maximum number of SoftwareSecurePhotoVerifications to process. ' + 'This helps avoid overloading the database while updating large amount of data.' + ) + parser.add_argument( + '--sleep_time', + action='store', + dest='sleep_time', + type=int, + default=10, + help='Sleep time in seconds between update of batches' + ) + + def handle(self, *args, **options): + start_date = datetime.datetime.strptime(options['start_date_time'], '%Y-%m-%d %H:%M:%S') + batch_size = options['batch_size'] + sleep_time = options['sleep_time'] + + count = 0 + verifications_after_start = SoftwareSecurePhotoVerification.objects.filter(created_at__gte=start_date) + + for verification in verifications_after_start: + logger.info( + 'Saving SoftwareSecurePhotoVerification with id=%(id)s', + {'id': verification.id} + ) + verification.save() + count += 1 + + if count == batch_size: + time.sleep(sleep_time) + count = 0