From 3342524699d0d5b747fad9783c986b1ff2003b32 Mon Sep 17 00:00:00 2001 From: Bianca Severino <biancasev@gmail.com> Date: Fri, 30 Oct 2020 15:16:13 -0400 Subject: [PATCH] Update IDVerificationAttempt to use expiration_date field --- .../student/tests/test_verification_status.py | 10 +- .../commands/populate_expiration_date.py | 103 ++++++++++++++++++ .../commands/populate_expiry_date.py | 7 +- .../send_verification_expiry_email.py | 27 +++-- .../tests/test_populate_expiration_date.py | 65 +++++++++++ .../test_send_verification_expiry_email.py | 32 +++--- .../0013_add_expiration_date_field.py | 29 +++++ lms/djangoapps/verify_student/models.py | 49 ++++----- lms/djangoapps/verify_student/services.py | 51 +++------ .../verify_student/tests/__init__.py | 8 +- .../verify_student/tests/factories.py | 2 - .../verify_student/tests/test_models.py | 38 +++---- .../verify_student/tests/test_services.py | 64 +++++++++-- .../verify_student/tests/test_utils.py | 4 +- .../verify_student/tests/test_views.py | 15 ++- lms/djangoapps/verify_student/views.py | 8 +- .../emails/passed_verification_email.txt | 2 +- .../verificationapproved/email/body.html | 2 +- .../verificationapproved/email/body.txt | 2 +- 19 files changed, 370 insertions(+), 148 deletions(-) create mode 100644 lms/djangoapps/verify_student/management/commands/populate_expiration_date.py create mode 100644 lms/djangoapps/verify_student/management/commands/tests/test_populate_expiration_date.py create mode 100644 lms/djangoapps/verify_student/migrations/0013_add_expiration_date_field.py diff --git a/common/djangoapps/student/tests/test_verification_status.py b/common/djangoapps/student/tests/test_verification_status.py index e437a1bc0ca..21caaef850b 100644 --- a/common/djangoapps/student/tests/test_verification_status.py +++ b/common/djangoapps/student/tests/test_verification_status.py @@ -136,7 +136,11 @@ class TestCourseVerificationStatus(UrlResetMixin, ModuleStoreTestCase): @patch("lms.djangoapps.verify_student.services.is_verification_expiring_soon") def test_verify_resubmit_button_on_dashboard(self, mock_expiry): mock_expiry.return_value = True - SoftwareSecurePhotoVerification.objects.create(user=self.user, status='approved', expiry_date=now()) + SoftwareSecurePhotoVerification.objects.create( + user=self.user, + status='approved', + expiration_date=now() + timedelta(days=1) + ) response = self.client.get(self.dashboard_url) self.assertContains(response, "Resubmit Verification") @@ -162,7 +166,7 @@ class TestCourseVerificationStatus(UrlResetMixin, ModuleStoreTestCase): attempt.mark_ready() attempt.submit() attempt.approve() - attempt.created_at = self.DATES[self.PAST] - timedelta(days=900) + attempt.expiration_date = self.DATES[self.PAST] - timedelta(days=900) attempt.save() # The student didn't have an approved verification at the deadline, @@ -178,7 +182,7 @@ class TestCourseVerificationStatus(UrlResetMixin, ModuleStoreTestCase): attempt.mark_ready() attempt.submit() attempt.approve() - attempt.created_at = self.DATES[self.PAST] - timedelta(days=900) + attempt.expiration_date = self.DATES[self.PAST] - timedelta(days=900) attempt.save() # The student didn't have an approved verification at the deadline, diff --git a/lms/djangoapps/verify_student/management/commands/populate_expiration_date.py b/lms/djangoapps/verify_student/management/commands/populate_expiration_date.py new file mode 100644 index 00000000000..8bf38a495bd --- /dev/null +++ b/lms/djangoapps/verify_student/management/commands/populate_expiration_date.py @@ -0,0 +1,103 @@ +""" +Django admin command to populate expiration_date for approved verifications in SoftwareSecurePhotoVerification +""" + + +import logging +import time +from datetime import timedelta + +from django.conf import settings +from django.core.management.base import BaseCommand +from django.db.models import F + +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification +from util.query import use_read_replica_if_available + +logger = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + This command sets the `expiration_date` for users for which the deprecated field `expiry_date` is set + The task is performed in batches with maximum number of rows to process given in argument `batch_size` + and a sleep time between each batch given by `sleep_time` + Default values: + `batch_size` = 1000 rows + `sleep_time` = 10 seconds + Example usage: + $ ./manage.py lms populate_expiration_date --batch_size=1000 --sleep_time=5 + OR + $ ./manage.py lms populate_expiration_date + """ + help = 'Populate expiration_date for approved verifications' + + def add_arguments(self, parser): + parser.add_argument( + '--batch_size', + action='store', + dest='batch_size', + type=int, + default=1000, + help='Maximum number of database rows to process. ' + 'This helps avoid locking 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): + """ + Handler for the command + It filters approved Software Secure Photo Verification and then for each distinct user it finds the most + recent approved verification and set its expiration_date + """ + batch_size = options['batch_size'] + sleep_time = options['sleep_time'] + + query = SoftwareSecurePhotoVerification.objects.filter(status='approved').order_by() + sspv = use_read_replica_if_available(query) + + if not sspv.count(): + logger.info("No approved entries found in SoftwareSecurePhotoVerification") + return + + distinct_user_ids = set() + update_verification_ids = [] + update_verification_count = 0 + + for verification in sspv: + if verification.user_id not in distinct_user_ids: + distinct_user_ids.add(verification.user_id) + + recent_verification = self.find_recent_verification(sspv, verification.user_id) + if recent_verification.expiry_date: + update_verification_ids.append(recent_verification.pk) + update_verification_count += 1 + + if update_verification_count == batch_size: + self.bulk_update(update_verification_ids) + update_verification_count = 0 + update_verification_ids = [] + time.sleep(sleep_time) + + if update_verification_ids: + self.bulk_update(update_verification_ids) + + def bulk_update(self, verification_ids): + """ + It updates the expiration_date and sets the expiry_date to NULL for all the + verifications whose ids lie in verification_ids + """ + recent_verification_qs = SoftwareSecurePhotoVerification.objects.filter(pk__in=verification_ids) + recent_verification_qs.update(expiration_date=F('expiry_date')) + recent_verification_qs.update(expiry_date=None) + + def find_recent_verification(self, model, user_id): + """ + Returns the most recent approved verification for a user + """ + return model.filter(user_id=user_id).latest('updated_at') diff --git a/lms/djangoapps/verify_student/management/commands/populate_expiry_date.py b/lms/djangoapps/verify_student/management/commands/populate_expiry_date.py index 0ed1c116c1a..cc4317fa76b 100644 --- a/lms/djangoapps/verify_student/management/commands/populate_expiry_date.py +++ b/lms/djangoapps/verify_student/management/commands/populate_expiry_date.py @@ -1,5 +1,6 @@ """ -Django admin command to populate expiry_date for approved verifications in SoftwareSecurePhotoVerification +(DEPRECATED) Django admin command to populate expiry_date for +approved verifications in SoftwareSecurePhotoVerification """ @@ -20,14 +21,11 @@ logger = logging.getLogger(__name__) class Command(BaseCommand): """ This command sets the expiry_date for users for which the verification is approved - The task is performed in batches with maximum number of rows to process given in argument `batch_size` and a sleep time between each batch given by `sleep_time` - Default values: `batch_size` = 1000 rows `sleep_time` = 10 seconds - Example usage: $ ./manage.py lms populate_expiry_date --batch_size=1000 --sleep_time=5 OR @@ -55,7 +53,6 @@ class Command(BaseCommand): def handle(self, *args, **options): """ Handler for the command - It filters approved Software Secure Photo Verification and then for each distinct user it finds the most recent approved verification and set its expiry_date """ 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 259eeb01217..9ae840a2c96 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 @@ -35,12 +35,12 @@ class Command(BaseCommand): """ This command sends email to learners for which the Software Secure Photo Verification has expired - The expiry email is sent when the date represented by SoftwareSecurePhotoVerification's field `expiry_date` + The expiry email is sent when the date represented by SoftwareSecurePhotoVerification's field `expiration_datetime` lies within the date range provided by command arguments. If the email is already sent indicated by field `expiry_email_date` then filter if the specified number of days given in settings as VERIFICATION_EXPIRY_EMAIL['RESEND_DAYS'] have passed since the last email. - Since a user can have multiple verification all the previous verifications have expiry_date and expiry_email_date + Since a user can have multiple verification all the previous verifications have expiry_email_date set to None so that they are not filtered. See lms/djangoapps/verify_student/views.py:1174 The range to filter expired verification is selected based on VERIFICATION_EXPIRY_EMAIL['DAYS_RANGE']. This @@ -101,12 +101,21 @@ class Command(BaseCommand): start_date = end_date - timedelta(days=days) # 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) - )).order_by() + query = SoftwareSecurePhotoVerification.objects.filter( + Q(status='approved') & + ( + Q(expiration_date__isnull=False) & ( + Q(expiration_date__gte=start_date, expiration_date__lt=end_date) | + Q(expiry_email_date__lte=date_resend_days_ago) + ) | + # Account for old entries still using `expiry_date` rather than`expiration_date` + # (this will be deprecated) + 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) + ) + ) + ).order_by() sspv = use_read_replica_if_available(query) @@ -230,7 +239,7 @@ class Command(BaseCommand): """ send_expiry_email_again = True email_duration = email_config['resend_days'] * (email_config['default_emails'] - 1) - days_since_expiry = (now() - verification.expiry_date).days + days_since_expiry = (now() - verification.expiration_datetime).days if days_since_expiry >= email_duration: send_expiry_email_again = False diff --git a/lms/djangoapps/verify_student/management/commands/tests/test_populate_expiration_date.py b/lms/djangoapps/verify_student/management/commands/tests/test_populate_expiration_date.py new file mode 100644 index 00000000000..39ce50b4d85 --- /dev/null +++ b/lms/djangoapps/verify_student/management/commands/tests/test_populate_expiration_date.py @@ -0,0 +1,65 @@ +""" +Tests for django admin command `populate_expiration_date` in the verify_student module +""" + + +from datetime import timedelta + +from django.conf import settings +from django.core.management import call_command +from django.test import TestCase +from django.utils.timezone import now +from mock import patch +from testfixtures import LogCapture + +from common.test.utils import MockS3BotoMixin +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification +from lms.djangoapps.verify_student.tests.test_models import FAKE_SETTINGS, mock_software_secure_post +from student.tests.factories import UserFactory + +LOGGER_NAME = 'lms.djangoapps.verify_student.management.commands.populate_expiration_date' + + +@patch.dict(settings.VERIFY_STUDENT, FAKE_SETTINGS) +@patch('lms.djangoapps.verify_student.models.requests.post', new=mock_software_secure_post) +class TestPopulateExpiryationDate(MockS3BotoMixin, TestCase): + """ Tests for django admin command `populate_expiration_date` in the verify_student module """ + + def create_and_submit(self, user): + """ Helper method that lets us create new SoftwareSecurePhotoVerifications """ + attempt = SoftwareSecurePhotoVerification(user=user) + attempt.upload_face_image("Fake Data") + attempt.upload_photo_id_image("More Fake Data") + attempt.mark_ready() + attempt.submit() + attempt.expiry_date = now() + timedelta(days=FAKE_SETTINGS["DAYS_GOOD_FOR"]) + return attempt + + def test_no_expiry_date(self): + """ + Test that the `expiration_date` for most recent approved verification is updated only when the + deprecated field `expiry_date` is still present + """ + user = UserFactory.create() + verification = self.create_and_submit(user) + verification.status = 'approved' + verification.expiry_date = None + verification.save() + + expiration_date = verification.expiration_date + call_command('populate_expiration_date') + + # Check that the `expiration_date` for approved verification is not changed + verification_expiration_date = SoftwareSecurePhotoVerification.objects.get(pk=verification.pk).expiration_date + + self.assertEqual(verification_expiration_date, expiration_date) + + def test_no_approved_verification_found(self): + """ + Test that if no approved verifications are found the management command terminates gracefully + """ + with LogCapture(LOGGER_NAME) as logger: + call_command('populate_expiration_date') + logger.check( + (LOGGER_NAME, 'INFO', "No approved entries found in SoftwareSecurePhotoVerification") + ) 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 964fe38c05e..d01e44c4bcc 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 @@ -52,11 +52,11 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): user = UserFactory.create() verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=self.days) + verification.expiration_date = now() - timedelta(days=self.days) verification.save() return verification - def test_expiry_date_range(self): + def test_expiration_date_range(self): """ Test that the verifications are filtered on the given range. Email is not sent for any verification with expiry date out of range @@ -64,13 +64,13 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): user = UserFactory.create() verification_in_range = self.create_and_submit(user) verification_in_range.status = 'approved' - verification_in_range.expiry_date = now() - timedelta(days=self.days) + verification_in_range.expiration_date = now() - timedelta(days=self.days) verification_in_range.save() user = UserFactory.create() verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=self.days + 1) + verification.expiration_date = now() - timedelta(days=self.days + 1) verification.save() call_command('send_verification_expiry_email') @@ -91,22 +91,22 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): today = now().replace(hour=0, minute=0, second=0, microsecond=0) verification_in_range = self.create_and_submit(user) verification_in_range.status = 'approved' - verification_in_range.expiry_date = today - timedelta(days=self.days + 1) + verification_in_range.expiration_date = today - timedelta(days=self.days + 1) verification_in_range.expiry_email_date = today - timedelta(days=self.resend_days) verification_in_range.save() call_command('send_verification_expiry_email') - # Check that email is sent even if the verification is not in expiry_date range but matches the criteria - # to resend email + # Check that email is sent even if the verification is not in expiration_date range but matches + # the criteria to resend email self.assertEqual(len(mail.outbox), 1) def test_most_recent_verification(self): """ Test that the SoftwareSecurePhotoVerification object is not filtered if it is outdated. A verification is - outdated if it's expiry_date and expiry_email_date is set NULL + outdated if its expiry_email_date is set NULL """ - # For outdated verification the expiry_date and expiry_email_date is set NULL verify_student/views.py:1164 + # For outdated verification the expiry_email_date is set NULL verify_student/views.py:1164 user = UserFactory.create() outdated_verification = self.create_and_submit(user) outdated_verification.status = 'approved' @@ -125,7 +125,7 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): user = UserFactory.create() verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=self.days) + verification.expiration_date = now() - timedelta(days=self.days) verification.save() call_command('send_verification_expiry_email') @@ -167,7 +167,7 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): user = UserFactory.create() verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=self.days) + verification.expiration_date = now() - timedelta(days=self.days) verification.expiry_email_date = now() verification.save() @@ -195,7 +195,7 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): user = UserFactory.create() verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=self.days) + verification.expiration_date = now() - timedelta(days=self.days) verification.save() start_date = now() - timedelta(days=self.days) # using default days @@ -226,7 +226,7 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): today = now().replace(hour=0, minute=0, second=0, microsecond=0) verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=self.resend_days * (self.default_no_of_emails - 1)) + verification.expiration_date = now() - timedelta(days=self.resend_days * (self.default_no_of_emails - 1)) verification.expiry_email_date = today - timedelta(days=self.resend_days) verification.save() @@ -247,7 +247,7 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=1) + verification.expiration_date = now() - timedelta(days=1) verification.save() call_command('send_verification_expiry_email') @@ -256,7 +256,7 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): for i in range(1, self.default_no_of_emails + 1): if SoftwareSecurePhotoVerification.objects.get(pk=verification.id).expiry_email_date: today = now().replace(hour=0, minute=0, second=0, microsecond=0) - verification.expiry_date = today - timedelta(days=self.resend_days * i + 1) + verification.expiration_date = today - timedelta(days=self.resend_days * i + 1) verification.expiry_email_date = today - timedelta(days=self.resend_days) verification.save() call_command('send_verification_expiry_email') @@ -283,7 +283,7 @@ class TestSendVerificationExpiryEmail(MockS3BotoMixin, TestCase): user = UserFactory.create() verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiry_date = now() - timedelta(days=self.days) + verification.expiration_date = now() - timedelta(days=self.days) verification.save() verifications.append(verification) diff --git a/lms/djangoapps/verify_student/migrations/0013_add_expiration_date_field.py b/lms/djangoapps/verify_student/migrations/0013_add_expiration_date_field.py new file mode 100644 index 00000000000..cbacb9bb76e --- /dev/null +++ b/lms/djangoapps/verify_student/migrations/0013_add_expiration_date_field.py @@ -0,0 +1,29 @@ +# Generated by Django 2.2.16 on 2020-11-06 21:29 + +from django.db import migrations, models +import lms.djangoapps.verify_student.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('verify_student', '0012_sspverificationretryconfig'), + ] + + operations = [ + migrations.AddField( + model_name='manualverification', + name='expiration_date', + field=models.DateTimeField(blank=True, db_index=True, default=lms.djangoapps.verify_student.models.IDVerificationAttempt.expiration_default, null=True), + ), + migrations.AddField( + model_name='softwaresecurephotoverification', + name='expiration_date', + field=models.DateTimeField(blank=True, db_index=True, default=lms.djangoapps.verify_student.models.IDVerificationAttempt.expiration_default, null=True), + ), + migrations.AddField( + model_name='ssoverification', + name='expiration_date', + field=models.DateTimeField(blank=True, db_index=True, default=lms.djangoapps.verify_student.models.IDVerificationAttempt.expiration_default, null=True), + ), + ] diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index e7a9eb95de5..7f53bc0222e 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -113,6 +113,17 @@ class IDVerificationAttempt(StatusModel): created_at = models.DateTimeField(auto_now_add=True, db_index=True) updated_at = models.DateTimeField(auto_now=True, db_index=True) + def expiration_default(): + return now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + + # Datetime that the verification will expire. + expiration_date = models.DateTimeField( + null=True, + blank=True, + db_index=True, + default=expiration_default + ) + class Meta(object): app_label = "verify_student" abstract = True @@ -120,7 +131,9 @@ class IDVerificationAttempt(StatusModel): @property def expiration_datetime(self): - """Datetime that the verification will expire. """ + """Account for old DB entries which have `expiration_date` set to NULL.""" + if self.expiration_date: + return self.expiration_date days_good_for = settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] return self.created_at + timedelta(days=days_good_for) @@ -435,6 +448,9 @@ class PhotoVerification(IDVerificationAttempt): self.reviewing_user = user_id self.reviewing_service = service self.status = self.STATUS.approved + self.expiration_date = now() + timedelta( + days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] + ) self.save() # Emit signal to find and generate eligible certificates LEARNER_NOW_VERIFIED.send_robust( @@ -624,29 +640,12 @@ class SoftwareSecurePhotoVerification(PhotoVerification): IMAGE_LINK_DURATION = 5 * 60 * 60 * 24 # 5 days in seconds copy_id_photo_from = models.ForeignKey("self", null=True, blank=True, on_delete=models.CASCADE) - # Fields for functionality of sending email when verification expires - # expiry_date: The date when the SoftwareSecurePhotoVerification will expire - # expiry_email_date: This field is used to maintain a check for learners to which email - # to notify for expired verification is already sent. + # DEPRECATED: the `expiry_date` field has been replaced by `expiration_date` expiry_date = models.DateTimeField(null=True, blank=True, db_index=True) - expiry_email_date = models.DateTimeField(null=True, blank=True, db_index=True) - - @status_before_must_be("must_retry", "submitted", "approved", "denied") - def approve(self, user_id=None, service=""): - """ - Approve the verification attempt for user - - Valid attempt statuses when calling this method: - `submitted`, `approved`, `denied` - After method completes: - status is set to `approved` - expiry_date is set to one year from now - """ - self.expiry_date = now() + timedelta( - days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] - ) - super(SoftwareSecurePhotoVerification, self).approve(user_id, service) + # This field is used to maintain a check for learners to which email + # to notify for expired verification is already sent. + expiry_email_date = models.DateTimeField(null=True, blank=True, db_index=True) @classmethod def get_initial_verification(cls, user, earliest_allowed_date=None): @@ -969,9 +968,7 @@ class SoftwareSecurePhotoVerification(PhotoVerification): Returns: SoftwareSecurePhotoVerification (object) or None """ - recent_verification = SoftwareSecurePhotoVerification.objects.filter(status='approved', - user_id=user.id, - expiry_date__isnull=False) + recent_verification = SoftwareSecurePhotoVerification.objects.filter(status='approved', user_id=user.id) return recent_verification.latest('updated_at') if recent_verification.exists() else None @@ -991,7 +988,7 @@ class SoftwareSecurePhotoVerification(PhotoVerification): verification = SoftwareSecurePhotoVerification.get_recent_verification(user) - if verification and verification.expiry_date < recently_expired_date and not verification.expiry_email_date: + if verification and verification.expiration_datetime < recently_expired_date and not verification.expiry_email_date: expiry_email_date = today - timedelta(days=email_config['RESEND_DAYS']) SoftwareSecurePhotoVerification.objects.filter(pk=verification.pk).update( expiry_email_date=expiry_email_date) diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index a57df2321ab..78e09bbed78 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -3,11 +3,12 @@ Implementation of abstraction layer for other parts of the system to make querie """ import logging +from datetime import timedelta from itertools import chain from django.conf import settings from django.urls import reverse -from django.utils import timezone +from django.utils.timezone import now from django.utils.translation import ugettext as _ from common.djangoapps.course_modes.models import CourseMode @@ -57,23 +58,16 @@ class IDVerificationService(object): """ @classmethod - def user_is_verified(cls, user, earliest_allowed_date=None): + def user_is_verified(cls, user): """ Return whether or not a user has satisfactorily proved their identity. Depending on the policy, this can expire after some period of time, so a user might have to renew periodically. - - This will check for the user's *initial* verification. """ - filter_kwargs = { - 'user': user, - 'status': 'approved', - 'created_at__gte': (earliest_allowed_date or earliest_allowed_verification_date()) - } - - return (SoftwareSecurePhotoVerification.objects.filter(**filter_kwargs).exists() or - SSOVerification.objects.filter(**filter_kwargs).exists() or - ManualVerification.objects.filter(**filter_kwargs).exists()) + expiration_datetime = cls.get_expiration_datetime(user, ['approved']) + if expiration_datetime: + return expiration_datetime >= now() + return False @classmethod def verifications_for_user(cls, user): @@ -95,7 +89,7 @@ class IDVerificationService(object): filter_kwargs = { 'user__in': users, 'status': 'approved', - 'created_at__gte': (earliest_allowed_verification_date()) + 'created_at__gt': now() - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) } return chain( SoftwareSecurePhotoVerification.objects.filter(**filter_kwargs).values_list('user_id', flat=True), @@ -143,17 +137,10 @@ class IDVerificationService(object): Returns: bool: True or False according to existence of valid verifications """ - filter_kwargs = { - 'user': user, - 'status__in': ['submitted', 'approved', 'must_retry'], - 'created_at__gte': earliest_allowed_verification_date() - } - - return ( - SoftwareSecurePhotoVerification.objects.filter(**filter_kwargs).exists() or - SSOVerification.objects.filter(**filter_kwargs).exists() or - ManualVerification.objects.filter(**filter_kwargs).exists() - ) + expiration_datetime = cls.get_expiration_datetime(user, ['submitted', 'approved', 'must_retry']) + if expiration_datetime: + return expiration_datetime >= now() + return False @classmethod def user_status(cls, user): @@ -180,15 +167,12 @@ class IDVerificationService(object): attempt = None - verifications = active_verifications( - cls.verifications_for_user(user), - timezone.now(), - ) + verifications = cls.verifications_for_user(user) if verifications: attempt = verifications[0] for verification in verifications: - if verification.status == 'approved': + if verification.expiration_datetime > now() and verification.status == 'approved': # Always select the LATEST non-expired approved verification if there is such if attempt.status != 'approved' or ( attempt.expiration_datetime < verification.expiration_datetime @@ -199,7 +183,8 @@ class IDVerificationService(object): return user_status user_status['should_display'] = attempt.should_display_status_to_user() - if attempt.created_at < earliest_allowed_verification_date(): + + if attempt.expiration_datetime < now() and attempt.status == 'approved': if user_status['should_display']: user_status['status'] = 'expired' user_status['error'] = _(u"Your {platform_name} verification has expired.").format( @@ -219,8 +204,8 @@ class IDVerificationService(object): elif attempt.status == 'approved': user_status['status'] = 'approved' expiration_datetime = cls.get_expiration_datetime(user, ['approved']) - if getattr(attempt, 'expiry_date', None) and is_verification_expiring_soon(expiration_datetime): - user_status['verification_expiry'] = attempt.expiry_date.date().strftime("%m/%d/%Y") + if is_verification_expiring_soon(expiration_datetime): + user_status['verification_expiry'] = attempt.expiration_datetime.date().strftime("%m/%d/%Y") user_status['status_date'] = attempt.status_changed elif attempt.status in ['submitted', 'approved', 'must_retry']: diff --git a/lms/djangoapps/verify_student/tests/__init__.py b/lms/djangoapps/verify_student/tests/__init__.py index e6ec9d043a6..130bdd94086 100644 --- a/lms/djangoapps/verify_student/tests/__init__.py +++ b/lms/djangoapps/verify_student/tests/__init__.py @@ -42,7 +42,7 @@ class TestVerificationBase(TestCase): Tests to ensure the Verification is active or inactive at the appropriate datetimes. """ # Not active before the created date - before = attempt.created_at - timedelta(seconds=1) + before = attempt.created_at - timedelta(minutes=1) self.assertFalse(attempt.active_at_datetime(before)) # Active immediately after created date @@ -50,14 +50,14 @@ class TestVerificationBase(TestCase): self.assertTrue(attempt.active_at_datetime(after_created)) # Active immediately before expiration date - expiration = attempt.created_at + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + expiration = attempt.expiration_datetime before_expiration = expiration - timedelta(seconds=1) self.assertTrue(attempt.active_at_datetime(before_expiration)) # Not active after the expiration date - attempt.created_at = attempt.created_at - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + attempt.expiration_date = now() - timedelta(days=1) attempt.save() - self.assertFalse(attempt.active_at_datetime(now() + timedelta(days=1))) + self.assertFalse(attempt.active_at_datetime(now())) def submit_attempt(self, attempt): with self.immediate_on_commit(): diff --git a/lms/djangoapps/verify_student/tests/factories.py b/lms/djangoapps/verify_student/tests/factories.py index 71a44deb72b..e1984a43849 100644 --- a/lms/djangoapps/verify_student/tests/factories.py +++ b/lms/djangoapps/verify_student/tests/factories.py @@ -20,8 +20,6 @@ class SoftwareSecurePhotoVerificationFactory(DjangoModelFactory): model = SoftwareSecurePhotoVerification status = 'approved' - if hasattr(settings, 'VERIFY_STUDENT'): - expiry_date = now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) class SSOVerificationFactory(DjangoModelFactory): diff --git a/lms/djangoapps/verify_student/tests/test_models.py b/lms/djangoapps/verify_student/tests/test_models.py index 654b9b1926f..4177b391e68 100644 --- a/lms/djangoapps/verify_student/tests/test_models.py +++ b/lms/djangoapps/verify_student/tests/test_models.py @@ -342,7 +342,7 @@ class TestPhotoVerification(TestVerificationBase, MockS3BotoMixin, ModuleStoreTe # Make an approved verification attempt = SoftwareSecurePhotoVerification(user=user) attempt.status = PhotoVerification.STATUS.approved - attempt.expiry_date = datetime.now() + attempt.expiration_date = datetime.now() attempt.save() # Test method 'get_recent_verification' returns the most recent @@ -351,25 +351,6 @@ class TestPhotoVerification(TestVerificationBase, MockS3BotoMixin, ModuleStoreTe self.assertIsNotNone(recent_verification) self.assertEqual(recent_verification.id, attempt.id) - def test_get_recent_verification_expiry_null(self): - """Test that method 'get_recent_verification' of model - 'SoftwareSecurePhotoVerification' will return None when expiry_date - is NULL for 'approved' verifications based on updated_at value. - """ - user = UserFactory.create() - attempt = None - - for _ in range(2): - # Make an approved verification - attempt = SoftwareSecurePhotoVerification(user=user) - attempt.status = PhotoVerification.STATUS.approved - attempt.save() - - # Test method 'get_recent_verification' returns None - # as attempts don't have an expiry_date - recent_verification = SoftwareSecurePhotoVerification.get_recent_verification(user=user) - self.assertIsNone(recent_verification) - def test_no_approved_verification(self): """Test that method 'get_recent_verification' of model 'SoftwareSecurePhotoVerification' returns None if no @@ -389,7 +370,7 @@ class TestPhotoVerification(TestVerificationBase, MockS3BotoMixin, ModuleStoreTe email_config = getattr(settings, 'VERIFICATION_EXPIRY_EMAIL', {'DAYS_RANGE': 1, 'RESEND_DAYS': 15}) user = UserFactory.create() verification = SoftwareSecurePhotoVerification(user=user) - verification.expiry_date = now() - timedelta(days=FAKE_SETTINGS['DAYS_GOOD_FOR']) + verification.expiration_date = now() - timedelta(days=FAKE_SETTINGS['DAYS_GOOD_FOR']) verification.status = PhotoVerification.STATUS.approved verification.save() @@ -400,6 +381,21 @@ class TestPhotoVerification(TestVerificationBase, MockS3BotoMixin, ModuleStoreTe self.assertIsNotNone(result.expiry_email_date) + def test_expiration_date_null(self): + """ + Test if the `expiration_date` field is null, `expiration_datetime` returns a + default expiration date based on the time the entry was created. + """ + user = UserFactory.create() + verification = SoftwareSecurePhotoVerification(user=user) + verification.expiration_date = None + verification.save() + + self.assertEqual( + verification.expiration_datetime, + verification.created_at + timedelta(days=FAKE_SETTINGS["DAYS_GOOD_FOR"]) + ) + class SSOVerificationTest(TestVerificationBase): """ diff --git a/lms/djangoapps/verify_student/tests/test_services.py b/lms/djangoapps/verify_student/tests/test_services.py index 39c703bf925..0a12c07699a 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -3,23 +3,26 @@ Tests for the service classes in verify_student. """ -from datetime import datetime +from datetime import datetime, timedelta import ddt from django.conf import settings from django.test import TestCase +from django.utils.timezone import now +from django.utils.translation import ugettext as _ from freezegun import freeze_time from mock import patch from pytz import utc +from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.verify_student.models import ManualVerification, SoftwareSecurePhotoVerification, SSOVerification from lms.djangoapps.verify_student.services import IDVerificationService -from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory FAKE_SETTINGS = { - "DAYS_GOOD_FOR": 10, + "DAYS_GOOD_FOR": 365, } @@ -144,7 +147,7 @@ class TestIDVerificationServiceUserStatus(TestCase): self.assertDictEqual(status, expected_status) def test_denied_software_secure_verification(self): - with freeze_time('2015-02-02'): + with freeze_time('2015-2-02'): # create denied photo verification for the user, make sure the denial # is handled properly SoftwareSecurePhotoVerification.objects.create( @@ -211,6 +214,27 @@ class TestIDVerificationServiceUserStatus(TestCase): status = IDVerificationService.user_status(self.user) self.assertDictEqual(status, expected_status) + def test_expired_verification(self): + with freeze_time('2015-07-11') as frozen_datetime: + # create approved photo verification for the user + SoftwareSecurePhotoVerification.objects.create( + user=self.user, + status='approved', + expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + ) + frozen_datetime.move_to('2016-07-11') + expected_status = { + 'status': 'expired', + 'error': _(u"Your {platform_name} verification has expired.").format( + platform_name=configuration_helpers.get_value('platform_name', settings.PLATFORM_NAME) + ), + 'should_display': True, + 'verification_expiry': '', + 'status_date': '' + } + status = IDVerificationService.user_status(self.user) + self.assertDictEqual(status, expected_status) + @ddt.data( 'submitted', 'denied', @@ -219,13 +243,21 @@ class TestIDVerificationServiceUserStatus(TestCase): 'ready', 'must_retry' ) - def test_expired_verification(self, new_status): + def test_reverify_after_expired(self, new_status): with freeze_time('2015-07-11') as frozen_datetime: # create approved photo verification for the user - SoftwareSecurePhotoVerification.objects.create(user=self.user, status='approved') - frozen_datetime.move_to('2015-07-22') + SoftwareSecurePhotoVerification.objects.create( + user=self.user, + status='approved', + expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + ) + frozen_datetime.move_to('2016-07-12') # create another according to status passed in. - SoftwareSecurePhotoVerification.objects.create(user=self.user, status=new_status) + SoftwareSecurePhotoVerification.objects.create( + user=self.user, + status=new_status, + expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + ) check_status = new_status status_date = '' @@ -236,7 +268,7 @@ class TestIDVerificationServiceUserStatus(TestCase): elif new_status == 'denied': check_status = 'must_reverify' else: - status_date = datetime.now(utc) + status_date = now() expected_status = {'status': check_status, 'error': '', 'should_display': True, 'verification_expiry': '', 'status_date': status_date} @@ -250,12 +282,20 @@ class TestIDVerificationServiceUserStatus(TestCase): def test_override_verification(self, verification_type): with freeze_time('2015-07-11') as frozen_datetime: # create approved photo verification for the user - SoftwareSecurePhotoVerification.objects.create(user=self.user, status='approved') + SoftwareSecurePhotoVerification.objects.create( + user=self.user, + status='approved', + expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + ) frozen_datetime.move_to('2015-07-14') - verification_type.objects.create(user=self.user, status='approved') + verification_type.objects.create( + user=self.user, + status='approved', + expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + ) expected_status = { 'status': 'approved', 'error': '', 'should_display': False, - 'verification_expiry': '', 'status_date': datetime.now(utc) + 'verification_expiry': '', 'status_date': now() } status = IDVerificationService.user_status(self.user) self.assertDictEqual(status, expected_status) diff --git a/lms/djangoapps/verify_student/tests/test_utils.py b/lms/djangoapps/verify_student/tests/test_utils.py index 9adb9bf0523..f9f237ddc90 100644 --- a/lms/djangoapps/verify_student/tests/test_utils.py +++ b/lms/djangoapps/verify_student/tests/test_utils.py @@ -70,14 +70,14 @@ class TestVerifyStudentUtils(unittest.TestCase): self.assertEqual(result, attempt) # Immediately before the expiration date, should get the attempt - expiration = attempt.created_at + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + expiration = attempt.expiration_datetime + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) before_expiration = expiration - timedelta(seconds=1) query = SoftwareSecurePhotoVerification.objects.filter(user=user) result = verification_for_datetime(before_expiration, query) self.assertEqual(result, attempt) # Immediately after the expiration date, should not get the attempt - attempt.created_at = attempt.created_at - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + attempt.expiration_date = now - timedelta(seconds=1) attempt.save() after = now + timedelta(days=1) query = SoftwareSecurePhotoVerification.objects.filter(user=user) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 81403620196..4f392901b0b 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -815,7 +815,7 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase, XssTestMixin, Tes if status == "expired": days_good_for = settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] - attempt.created_at = now() - timedelta(days=(days_good_for + 1)) + attempt.expiration_date = now() - timedelta(days=1) attempt.save() def _set_deadlines(self, course_key, upgrade_deadline=None, verification_deadline=None, mode_slug="verified"): @@ -1546,12 +1546,12 @@ class TestPhotoVerificationResultsCallback(ModuleStoreTestCase, TestVerification """ Test for verification passed. """ - expiry_date = now() + timedelta( + expiration_datetime = now() + timedelta( days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] ) verification = self.create_and_submit_attempt_for_user(self.user) verification.approve() - verification.expiry_date = now() + verification.expiration_date = now() verification.expiry_email_date = now() verification.save() @@ -1571,8 +1571,7 @@ class TestPhotoVerificationResultsCallback(ModuleStoreTestCase, TestVerification attempt = SoftwareSecurePhotoVerification.objects.get(receipt_id=self.receipt_id) old_verification = SoftwareSecurePhotoVerification.objects.get(pk=verification.pk) self.assertEqual(attempt.status, u'approved') - self.assertEqual(attempt.expiry_date.date(), expiry_date.date()) - self.assertIsNone(old_verification.expiry_date) + self.assertEqual(attempt.expiration_datetime.date(), expiration_datetime.date()) self.assertIsNone(old_verification.expiry_email_date) self.assertEqual(response.content.decode('utf-8'), 'OK!') self._assert_verification_approved_email() @@ -1587,7 +1586,7 @@ class TestPhotoVerificationResultsCallback(ModuleStoreTestCase, TestVerification """ Test for verification passed if the learner does not have any previous verification """ - expiry_date = now() + timedelta( + expiration_datetime = now() + timedelta( days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] ) @@ -1607,7 +1606,7 @@ class TestPhotoVerificationResultsCallback(ModuleStoreTestCase, TestVerification attempt = SoftwareSecurePhotoVerification.objects.get(receipt_id=self.receipt_id) self.assertEqual(attempt.status, u'approved') - self.assertEqual(attempt.expiry_date.date(), expiry_date.date()) + self.assertEqual(attempt.expiration_datetime.date(), expiration_datetime.date()) self.assertEqual(response.content.decode('utf-8'), 'OK!') self._assert_verification_approved_email() @@ -1728,7 +1727,7 @@ class TestReverifyView(TestVerificationBase): attempt.approve() days_good_for = settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] - attempt.created_at = now() - timedelta(days=(days_good_for + 1)) + attempt.expiration_date = now() - timedelta(days=(days_good_for + 1)) attempt.save() # Allow the student to re-verify diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 7e3aded42a6..052957939f3 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -1105,13 +1105,13 @@ def results_callback(request): 'platform_name': settings.PLATFORM_NAME, } if result == "PASS": - # If this verification is not an outdated version then make expiry date of previous approved verification NULL - # Setting expiry date to NULL is important so that it does not get filtered in the management command + # If this verification is not an outdated version then make expiry email date of previous approved verification NULL + # Setting expiry email date to NULL is important so that it does not get filtered in the management command # that sends email when verification expires : verify_student/send_verification_expiry_email if attempt.status != 'approved': verification = SoftwareSecurePhotoVerification.objects.filter(status='approved', user_id=attempt.user_id) if verification: - log.info(u'Making expiry date of previous approved verification NULL for {}'.format(attempt.user_id)) + log.info(u'Making expiry email date of previous approved verification NULL for {}'.format(attempt.user_id)) # The updated_at field in sspv model has auto_now set to True, which means any time save() is called on # the model instance, `updated_at` will change. Some of the existing functionality of verification # (showing your verification has expired on dashboard) relies on updated_at. @@ -1119,7 +1119,7 @@ def results_callback(request): # functionality update() is called instead of save() previous_verification = verification.latest('updated_at') SoftwareSecurePhotoVerification.objects.filter(pk=previous_verification.pk - ).update(expiry_date=None, expiry_email_date=None) + ).update(expiry_email_date=None) log.debug(u'Approving verification for {}'.format(receipt_id)) attempt.approve() diff --git a/lms/templates/emails/passed_verification_email.txt b/lms/templates/emails/passed_verification_email.txt index 7be48113a76..dffcbd502d3 100644 --- a/lms/templates/emails/passed_verification_email.txt +++ b/lms/templates/emails/passed_verification_email.txt @@ -4,7 +4,7 @@ ${_("Hi {full_name}").format(full_name=full_name)} ${_("Congratulations! Your ID verification process was successful.")} -${_("Your verification is effective for one year. It will expire on {expiry_date}").format(expiry_date=expiry_date)} +${_("Your verification is effective for one year. It will expire on {expiration_datetime}").format(expiration_date=expiration_datetime)} ${_("Thank you,")} ${_("The {platform_name} team").format(platform_name=platform_name)} \ No newline at end of file diff --git a/lms/templates/verify_student/edx_ace/verificationapproved/email/body.html b/lms/templates/verify_student/edx_ace/verificationapproved/email/body.html index 83ff7a5cbd1..544865157fd 100644 --- a/lms/templates/verify_student/edx_ace/verificationapproved/email/body.html +++ b/lms/templates/verify_student/edx_ace/verificationapproved/email/body.html @@ -20,7 +20,7 @@ {% endfilter %} {% filter force_escape %} - {% blocktrans %}Your approval status remains valid for one year, and it will expire {{ expiry_date }}.{% endblocktrans %} + {% blocktrans %}Your approval status remains valid for one year, and it will expire {{ expiration_datetime }}.{% endblocktrans %} {% endfilter %} <br/> </p> diff --git a/lms/templates/verify_student/edx_ace/verificationapproved/email/body.txt b/lms/templates/verify_student/edx_ace/verificationapproved/email/body.txt index f3ffad937dd..7476929d61d 100644 --- a/lms/templates/verify_student/edx_ace/verificationapproved/email/body.txt +++ b/lms/templates/verify_student/edx_ace/verificationapproved/email/body.txt @@ -1,7 +1,7 @@ {% load i18n %}{% autoescape off %} {% blocktrans %}Hello {{full_name}}, {% endblocktrans %} {% blocktrans %}Your {{ platform_name }} ID verification photos have been approved.{% endblocktrans %} -{% blocktrans %}Your approval status remains valid for one year, and it will expire {{ expiry_date }}.{% endblocktrans %} +{% blocktrans %}Your approval status remains valid for one year, and it will expire {{ expiration_datetime }}.{% endblocktrans %} {% trans "Enjoy your studies," %} {% blocktrans %}The {{ platform_name }} Team {% endblocktrans %} -- GitLab