From 82e8592fbed28fd7bfd27813e6c6ea6479b25fea Mon Sep 17 00:00:00 2001 From: Bianca Severino <biancasev@gmail.com> Date: Tue, 10 Nov 2020 14:24:07 -0500 Subject: [PATCH] Revert "Set expiration_date field in IDVerificationAttempt model" --- .../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, 148 insertions(+), 370 deletions(-) delete mode 100644 lms/djangoapps/verify_student/management/commands/populate_expiration_date.py delete mode 100644 lms/djangoapps/verify_student/management/commands/tests/test_populate_expiration_date.py delete 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 21caaef850b..e437a1bc0ca 100644 --- a/common/djangoapps/student/tests/test_verification_status.py +++ b/common/djangoapps/student/tests/test_verification_status.py @@ -136,11 +136,7 @@ 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', - expiration_date=now() + timedelta(days=1) - ) + SoftwareSecurePhotoVerification.objects.create(user=self.user, status='approved', expiry_date=now()) response = self.client.get(self.dashboard_url) self.assertContains(response, "Resubmit Verification") @@ -166,7 +162,7 @@ class TestCourseVerificationStatus(UrlResetMixin, ModuleStoreTestCase): attempt.mark_ready() attempt.submit() attempt.approve() - attempt.expiration_date = self.DATES[self.PAST] - timedelta(days=900) + attempt.created_at = self.DATES[self.PAST] - timedelta(days=900) attempt.save() # The student didn't have an approved verification at the deadline, @@ -182,7 +178,7 @@ class TestCourseVerificationStatus(UrlResetMixin, ModuleStoreTestCase): attempt.mark_ready() attempt.submit() attempt.approve() - attempt.expiration_date = self.DATES[self.PAST] - timedelta(days=900) + attempt.created_at = 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 deleted file mode 100644 index 8bf38a495bd..00000000000 --- a/lms/djangoapps/verify_student/management/commands/populate_expiration_date.py +++ /dev/null @@ -1,103 +0,0 @@ -""" -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 cc4317fa76b..0ed1c116c1a 100644 --- a/lms/djangoapps/verify_student/management/commands/populate_expiry_date.py +++ b/lms/djangoapps/verify_student/management/commands/populate_expiry_date.py @@ -1,6 +1,5 @@ """ -(DEPRECATED) Django admin command to populate expiry_date for -approved verifications in SoftwareSecurePhotoVerification +Django admin command to populate expiry_date for approved verifications in SoftwareSecurePhotoVerification """ @@ -21,11 +20,14 @@ 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 @@ -53,6 +55,7 @@ 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 9ae840a2c96..259eeb01217 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 `expiration_datetime` + The expiry email is sent when the date represented by SoftwareSecurePhotoVerification's field `expiry_date` 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_email_date + Since a user can have multiple verification all the previous verifications have expiry_date and 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,21 +101,12 @@ 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(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() + 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() sspv = use_read_replica_if_available(query) @@ -239,7 +230,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.expiration_datetime).days + days_since_expiry = (now() - verification.expiry_date).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 deleted file mode 100644 index 39ce50b4d85..00000000000 --- a/lms/djangoapps/verify_student/management/commands/tests/test_populate_expiration_date.py +++ /dev/null @@ -1,65 +0,0 @@ -""" -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 d01e44c4bcc..964fe38c05e 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.expiration_date = now() - timedelta(days=self.days) + verification.expiry_date = now() - timedelta(days=self.days) verification.save() return verification - def test_expiration_date_range(self): + def test_expiry_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.expiration_date = now() - timedelta(days=self.days) + verification_in_range.expiry_date = now() - timedelta(days=self.days) verification_in_range.save() user = UserFactory.create() verification = self.create_and_submit(user) verification.status = 'approved' - verification.expiration_date = now() - timedelta(days=self.days + 1) + verification.expiry_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.expiration_date = today - timedelta(days=self.days + 1) + verification_in_range.expiry_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 expiration_date range but matches - # the criteria to resend email + # Check that email is sent even if the verification is not in expiry_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 its expiry_email_date is set NULL + outdated if it's expiry_date and expiry_email_date is set NULL """ - # For outdated verification the expiry_email_date is set NULL verify_student/views.py:1164 + # For outdated verification the expiry_date and 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.expiration_date = now() - timedelta(days=self.days) + verification.expiry_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.expiration_date = now() - timedelta(days=self.days) + verification.expiry_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.expiration_date = now() - timedelta(days=self.days) + verification.expiry_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.expiration_date = now() - timedelta(days=self.resend_days * (self.default_no_of_emails - 1)) + verification.expiry_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.expiration_date = now() - timedelta(days=1) + verification.expiry_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.expiration_date = today - timedelta(days=self.resend_days * i + 1) + verification.expiry_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.expiration_date = now() - timedelta(days=self.days) + verification.expiry_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 deleted file mode 100644 index cbacb9bb76e..00000000000 --- a/lms/djangoapps/verify_student/migrations/0013_add_expiration_date_field.py +++ /dev/null @@ -1,29 +0,0 @@ -# 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 7f53bc0222e..e7a9eb95de5 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -113,17 +113,6 @@ 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 @@ -131,9 +120,7 @@ class IDVerificationAttempt(StatusModel): @property def expiration_datetime(self): - """Account for old DB entries which have `expiration_date` set to NULL.""" - if self.expiration_date: - return self.expiration_date + """Datetime that the verification will expire. """ days_good_for = settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] return self.created_at + timedelta(days=days_good_for) @@ -448,9 +435,6 @@ 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( @@ -640,13 +624,30 @@ 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) - # DEPRECATED: the `expiry_date` field has been replaced by `expiration_date` - expiry_date = models.DateTimeField(null=True, blank=True, db_index=True) - - # This field is used to maintain a check for learners to which email + # 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. + 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) + @classmethod def get_initial_verification(cls, user, earliest_allowed_date=None): """Get initial verification for a user with the 'photo_id_key'. @@ -968,7 +969,9 @@ class SoftwareSecurePhotoVerification(PhotoVerification): Returns: SoftwareSecurePhotoVerification (object) or None """ - recent_verification = SoftwareSecurePhotoVerification.objects.filter(status='approved', user_id=user.id) + recent_verification = SoftwareSecurePhotoVerification.objects.filter(status='approved', + user_id=user.id, + expiry_date__isnull=False) return recent_verification.latest('updated_at') if recent_verification.exists() else None @@ -988,7 +991,7 @@ class SoftwareSecurePhotoVerification(PhotoVerification): verification = SoftwareSecurePhotoVerification.get_recent_verification(user) - if verification and verification.expiration_datetime < recently_expired_date and not verification.expiry_email_date: + if verification and verification.expiry_date < 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 78e09bbed78..a57df2321ab 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -3,12 +3,11 @@ 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.timezone import now +from django.utils import timezone from django.utils.translation import ugettext as _ from common.djangoapps.course_modes.models import CourseMode @@ -58,16 +57,23 @@ class IDVerificationService(object): """ @classmethod - def user_is_verified(cls, user): + def user_is_verified(cls, user, earliest_allowed_date=None): """ 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. """ - expiration_datetime = cls.get_expiration_datetime(user, ['approved']) - if expiration_datetime: - return expiration_datetime >= now() - return False + 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()) @classmethod def verifications_for_user(cls, user): @@ -89,7 +95,7 @@ class IDVerificationService(object): filter_kwargs = { 'user__in': users, 'status': 'approved', - 'created_at__gt': now() - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + 'created_at__gte': (earliest_allowed_verification_date()) } return chain( SoftwareSecurePhotoVerification.objects.filter(**filter_kwargs).values_list('user_id', flat=True), @@ -137,10 +143,17 @@ class IDVerificationService(object): Returns: bool: True or False according to existence of valid verifications """ - expiration_datetime = cls.get_expiration_datetime(user, ['submitted', 'approved', 'must_retry']) - if expiration_datetime: - return expiration_datetime >= now() - return False + 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() + ) @classmethod def user_status(cls, user): @@ -167,12 +180,15 @@ class IDVerificationService(object): attempt = None - verifications = cls.verifications_for_user(user) + verifications = active_verifications( + cls.verifications_for_user(user), + timezone.now(), + ) if verifications: attempt = verifications[0] for verification in verifications: - if verification.expiration_datetime > now() and verification.status == 'approved': + if 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 @@ -183,8 +199,7 @@ class IDVerificationService(object): return user_status user_status['should_display'] = attempt.should_display_status_to_user() - - if attempt.expiration_datetime < now() and attempt.status == 'approved': + if attempt.created_at < earliest_allowed_verification_date(): if user_status['should_display']: user_status['status'] = 'expired' user_status['error'] = _(u"Your {platform_name} verification has expired.").format( @@ -204,8 +219,8 @@ class IDVerificationService(object): elif attempt.status == 'approved': user_status['status'] = 'approved' expiration_datetime = cls.get_expiration_datetime(user, ['approved']) - if is_verification_expiring_soon(expiration_datetime): - user_status['verification_expiry'] = attempt.expiration_datetime.date().strftime("%m/%d/%Y") + 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") 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 130bdd94086..e6ec9d043a6 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(minutes=1) + before = attempt.created_at - timedelta(seconds=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.expiration_datetime + expiration = attempt.created_at + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) before_expiration = expiration - timedelta(seconds=1) self.assertTrue(attempt.active_at_datetime(before_expiration)) # Not active after the expiration date - attempt.expiration_date = now() - timedelta(days=1) + attempt.created_at = attempt.created_at - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) attempt.save() - self.assertFalse(attempt.active_at_datetime(now())) + self.assertFalse(attempt.active_at_datetime(now() + timedelta(days=1))) 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 e1984a43849..71a44deb72b 100644 --- a/lms/djangoapps/verify_student/tests/factories.py +++ b/lms/djangoapps/verify_student/tests/factories.py @@ -20,6 +20,8 @@ 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 4177b391e68..654b9b1926f 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.expiration_date = datetime.now() + attempt.expiry_date = datetime.now() attempt.save() # Test method 'get_recent_verification' returns the most recent @@ -351,6 +351,25 @@ 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 @@ -370,7 +389,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.expiration_date = now() - timedelta(days=FAKE_SETTINGS['DAYS_GOOD_FOR']) + verification.expiry_date = now() - timedelta(days=FAKE_SETTINGS['DAYS_GOOD_FOR']) verification.status = PhotoVerification.STATUS.approved verification.save() @@ -381,21 +400,6 @@ 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 0a12c07699a..39c703bf925 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -3,26 +3,23 @@ Tests for the service classes in verify_student. """ -from datetime import datetime, timedelta +from datetime import datetime 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 openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory FAKE_SETTINGS = { - "DAYS_GOOD_FOR": 365, + "DAYS_GOOD_FOR": 10, } @@ -147,7 +144,7 @@ class TestIDVerificationServiceUserStatus(TestCase): self.assertDictEqual(status, expected_status) def test_denied_software_secure_verification(self): - with freeze_time('2015-2-02'): + with freeze_time('2015-02-02'): # create denied photo verification for the user, make sure the denial # is handled properly SoftwareSecurePhotoVerification.objects.create( @@ -214,27 +211,6 @@ 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', @@ -243,21 +219,13 @@ class TestIDVerificationServiceUserStatus(TestCase): 'ready', 'must_retry' ) - def test_reverify_after_expired(self, new_status): + def test_expired_verification(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', - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) - ) - frozen_datetime.move_to('2016-07-12') + SoftwareSecurePhotoVerification.objects.create(user=self.user, status='approved') + frozen_datetime.move_to('2015-07-22') # create another according to status passed in. - SoftwareSecurePhotoVerification.objects.create( - user=self.user, - status=new_status, - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) - ) + SoftwareSecurePhotoVerification.objects.create(user=self.user, status=new_status) check_status = new_status status_date = '' @@ -268,7 +236,7 @@ class TestIDVerificationServiceUserStatus(TestCase): elif new_status == 'denied': check_status = 'must_reverify' else: - status_date = now() + status_date = datetime.now(utc) expected_status = {'status': check_status, 'error': '', 'should_display': True, 'verification_expiry': '', 'status_date': status_date} @@ -282,20 +250,12 @@ 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', - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) - ) + SoftwareSecurePhotoVerification.objects.create(user=self.user, status='approved') frozen_datetime.move_to('2015-07-14') - verification_type.objects.create( - user=self.user, - status='approved', - expiration_date=now() + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) - ) + verification_type.objects.create(user=self.user, status='approved') expected_status = { 'status': 'approved', 'error': '', 'should_display': False, - 'verification_expiry': '', 'status_date': now() + 'verification_expiry': '', 'status_date': datetime.now(utc) } 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 f9f237ddc90..9adb9bf0523 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.expiration_datetime + timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) + expiration = attempt.created_at + 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.expiration_date = now - timedelta(seconds=1) + attempt.created_at = attempt.created_at - timedelta(days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"]) 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 4f392901b0b..81403620196 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.expiration_date = now() - timedelta(days=1) + attempt.created_at = now() - timedelta(days=(days_good_for + 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. """ - expiration_datetime = now() + timedelta( + expiry_date = now() + timedelta( days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] ) verification = self.create_and_submit_attempt_for_user(self.user) verification.approve() - verification.expiration_date = now() + verification.expiry_date = now() verification.expiry_email_date = now() verification.save() @@ -1571,7 +1571,8 @@ 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.expiration_datetime.date(), expiration_datetime.date()) + self.assertEqual(attempt.expiry_date.date(), expiry_date.date()) + self.assertIsNone(old_verification.expiry_date) self.assertIsNone(old_verification.expiry_email_date) self.assertEqual(response.content.decode('utf-8'), 'OK!') self._assert_verification_approved_email() @@ -1586,7 +1587,7 @@ class TestPhotoVerificationResultsCallback(ModuleStoreTestCase, TestVerification """ Test for verification passed if the learner does not have any previous verification """ - expiration_datetime = now() + timedelta( + expiry_date = now() + timedelta( days=settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] ) @@ -1606,7 +1607,7 @@ class TestPhotoVerificationResultsCallback(ModuleStoreTestCase, TestVerification attempt = SoftwareSecurePhotoVerification.objects.get(receipt_id=self.receipt_id) self.assertEqual(attempt.status, u'approved') - self.assertEqual(attempt.expiration_datetime.date(), expiration_datetime.date()) + self.assertEqual(attempt.expiry_date.date(), expiry_date.date()) self.assertEqual(response.content.decode('utf-8'), 'OK!') self._assert_verification_approved_email() @@ -1727,7 +1728,7 @@ class TestReverifyView(TestVerificationBase): attempt.approve() days_good_for = settings.VERIFY_STUDENT["DAYS_GOOD_FOR"] - attempt.expiration_date = now() - timedelta(days=(days_good_for + 1)) + attempt.created_at = 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 052957939f3..7e3aded42a6 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 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 + # 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 # 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 email date of previous approved verification NULL for {}'.format(attempt.user_id)) + log.info(u'Making expiry 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_email_date=None) + ).update(expiry_date=None, 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 dffcbd502d3..7be48113a76 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 {expiration_datetime}").format(expiration_date=expiration_datetime)} +${_("Your verification is effective for one year. It will expire on {expiry_date}").format(expiry_date=expiry_date)} ${_("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 544865157fd..83ff7a5cbd1 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 {{ expiration_datetime }}.{% endblocktrans %} + {% blocktrans %}Your approval status remains valid for one year, and it will expire {{ expiry_date }}.{% 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 7476929d61d..f3ffad937dd 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 {{ expiration_datetime }}.{% endblocktrans %} +{% blocktrans %}Your approval status remains valid for one year, and it will expire {{ expiry_date }}.{% endblocktrans %} {% trans "Enjoy your studies," %} {% blocktrans %}The {{ platform_name }} Team {% endblocktrans %} -- GitLab