From 7999bdfbe90e8d9729a1e236e431f2bc74b5954d Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Fri, 20 Mar 2020 11:03:39 -0400 Subject: [PATCH] Fix error in verification email command We are seeing an error in the send_verification_expiry_email job because one verification model instance had a null expiry_date but a non-null expery_email_date. This makes us more robust to that odd data and makes the job more robust by having it still send other emails out even if one fails. AA-70 --- .../send_verification_expiry_email.py | 40 ++++++++++++------- .../test_send_verification_expiry_email.py | 31 ++++++++++++++ 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/verify_student/management/commands/send_verification_expiry_email.py b/lms/djangoapps/verify_student/management/commands/send_verification_expiry_email.py index fee92b27ff..e4a59e33d9 100644 --- a/lms/djangoapps/verify_student/management/commands/send_verification_expiry_email.py +++ b/lms/djangoapps/verify_student/management/commands/send_verification_expiry_email.py @@ -101,6 +101,7 @@ class Command(BaseCommand): # Adding an order_by() clause will override the class meta ordering as we don't need ordering here query = SoftwareSecurePhotoVerification.objects.filter(Q(status='approved') & + Q(expiry_date__isnull=False) & (Q(expiry_date__gte=start_date, expiry_date__lt=end_date) | Q(expiry_email_date__lte=date_resend_days_ago) @@ -124,18 +125,22 @@ class Command(BaseCommand): 'default_emails': default_emails } + success = True for verification in sspv: if not verification.expiry_email_date or verification.expiry_email_date <= date_resend_days_ago: batch_verifications.append(verification) if len(batch_verifications) == batch_size: - self.send_verification_expiry_email(batch_verifications, email_config) + success = self.send_verification_expiry_email(batch_verifications, email_config) and success time.sleep(sleep_time) batch_verifications = [] # If selected verification in batch are less than batch_size if batch_verifications: - self.send_verification_expiry_email(batch_verifications, email_config) + success = self.send_verification_expiry_email(batch_verifications, email_config) and success + + if not success: + raise CommandError('One or more email attempts failed. Search for "Could not send" above.') def send_verification_expiry_email(self, batch_verifications, email_config): """ @@ -152,7 +157,7 @@ class Command(BaseCommand): u"This was a dry run, no email was sent. For the actual run email would have been sent " u"to {} learner(s)".format(len(batch_verifications)) ) - return + return True site = Site.objects.get_current() message_context = get_base_template_context(site) @@ -165,18 +170,25 @@ class Command(BaseCommand): expiry_email = VerificationExpiry(context=message_context) users = User.objects.filter(pk__in=[verification.user_id for verification in batch_verifications]) + success = True for verification in batch_verifications: - user = users.get(pk=verification.user_id) - with emulate_http_request(site=site, user=user): - msg = expiry_email.personalize( - recipient=Recipient(user.username, user.email), - language=get_user_preference(user, LANGUAGE_KEY), - user_context={ - 'full_name': user.profile.name, - } - ) - ace.send(msg) - self._set_email_expiry_date(verification, user, email_config) + try: + user = users.get(pk=verification.user_id) + with emulate_http_request(site=site, user=user): + msg = expiry_email.personalize( + recipient=Recipient(user.username, user.email), + language=get_user_preference(user, LANGUAGE_KEY), + user_context={ + 'full_name': user.profile.name, + } + ) + ace.send(msg) + self._set_email_expiry_date(verification, user, email_config) + except Exception: # pylint: disable=broad-except + logger.exception('Could not send email for verification id %d', verification.id) + success = False + + return success def _set_email_expiry_date(self, verification, user, email_config): """ diff --git a/lms/djangoapps/verify_student/management/commands/tests/test_send_verification_expiry_email.py b/lms/djangoapps/verify_student/management/commands/tests/test_send_verification_expiry_email.py index bde25e958e..e7215c06de 100644 --- a/lms/djangoapps/verify_student/management/commands/tests/test_send_verification_expiry_email.py +++ b/lms/djangoapps/verify_student/management/commands/tests/test_send_verification_expiry_email.py @@ -101,6 +101,8 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): outdated_verification.status = 'approved' outdated_verification.save() + call_command('send_verification_expiry_email') + # Check that the expiry_email_date is not set for the outdated verification expiry_email_date = SoftwareSecurePhotoVerification.objects.get(pk=outdated_verification.pk).expiry_email_date self.assertIsNone(expiry_email_date) @@ -236,3 +238,32 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): u"emails use --dry-run flag instead." with self.assertRaisesRegex(CommandError, err_string): call_command('send_verification_expiry_email') + + def test_one_failed_but_others_succeeded(self): + """ + Test that if the first verification fails to send, the rest still do. + """ + verifications = [] + for _i in range(2): + user = UserFactory.create() + verification = self.create_and_submit(user) + verification.status = 'approved' + verification.expiry_date = now() - timedelta(days=self.days) + verification.save() + verifications.append(verification) + + with patch('lms.djangoapps.verify_student.management.commands.send_verification_expiry_email.ace') as mock_ace: + mock_ace.send.side_effect = (Exception('Aw shucks'), None) + with self.assertRaisesRegex(CommandError, 'One or more email attempts failed.*'): + with LogCapture(LOGGER_NAME) as logger: + call_command('send_verification_expiry_email') + + logger.check_present( + (LOGGER_NAME, 'ERROR', 'Could not send email for verification id {}'.format(verifications[0].id)), + ) + + for verification in verifications: + verification.refresh_from_db() + self.assertIsNone(verifications[0].expiry_email_date) + self.assertIsNotNone(verifications[1].expiry_email_date) + self.assertEqual(mock_ace.send.call_count, 2)