Merge pull request #15996 from edx/ssemenova/ed-1319

ED-1319 Cert not accessible by learner

lms/djangoapps/certificates/signals.py

@@ -14,6 +14,7 @@ from certificates.models import (
 )
 from certificates.tasks import generate_certificate
 from lms.djangoapps.grades.new.course_grade_factory import CourseGradeFactory
+from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification
 from openedx.core.djangoapps.certificates.api import auto_certificate_generation_enabled
 from openedx.core.djangoapps.certificates.config import waffle
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
@@ -22,6 +23,7 @@ from student.models import CourseEnrollment
 
 
 log = logging.getLogger(__name__)
+CERTIFICATE_DELAY_SECONDS = 2
 
 
 @receiver(post_save, sender=CertificateWhitelist, dispatch_uid="append_certificate_whitelist")
@@ -55,7 +57,7 @@ def _listen_for_passing_grade(sender, user, course_id, **kwargs):  # pylint: dis
 
 
 @receiver(LEARNER_NOW_VERIFIED, dispatch_uid="learner_track_changed")
-def _listen_for_track_change(sender, user, **kwargs):  # pylint: disable=unused-argument
+def _listen_for_id_verification_status_changed(sender, user, **kwargs):  # pylint: disable=unused-argument
     """
     Catches a track change signal, determines user status,
     calls fire_ungenerated_certificate_task for passing grades
@@ -65,16 +67,22 @@ def _listen_for_track_change(sender, user, **kwargs):  # pylint: disable=unused-
 
     user_enrollments = CourseEnrollment.enrollments_for_user(user=user)
     grade_factory = CourseGradeFactory()
+    expected_verification_status, _ = SoftwareSecurePhotoVerification.user_status(user)
     for enrollment in user_enrollments:
         if grade_factory.read(user=user, course=enrollment.course_overview).passed:
-            if fire_ungenerated_certificate_task(user, enrollment.course_id):
-                log.info(u'Certificate generation task initiated for {user} : {course} via track change'.format(
+            if fire_ungenerated_certificate_task(user, enrollment.course_id, expected_verification_status):
+                message = (
+                    u'Certificate generation task initiated for {user} : {course} via track change ' +
+                    u'with verification status of {status}'
+                )
+                log.info(message.format(
                     user=user.id,
-                    course=enrollment.course_id
+                    course=enrollment.course_id,
+                    status=expected_verification_status
                 ))
 
 
-def fire_ungenerated_certificate_task(user, course_key):
+def fire_ungenerated_certificate_task(user, course_key, expected_verification_status=None):
     """
     Helper function to fire un-generated certificate tasks
 
@@ -87,8 +95,11 @@ def fire_ungenerated_certificate_task(user, course_key):
     mode_is_verified = enrollment_mode in GeneratedCertificate.VERIFIED_CERTS_MODES
     cert = GeneratedCertificate.certificate_for_student(user, course_key)
     if mode_is_verified and (cert is None or cert.status == 'unverified'):
-        generate_certificate.apply_async(kwargs={
+        kwargs = {
             'student': unicode(user.id),
-            'course_key': unicode(course_key),
-        })
+            'course_key': unicode(course_key)
+        }
+        if expected_verification_status:
+            kwargs['expected_verification_status'] = unicode(expected_verification_status)
+        generate_certificate.apply_async(countdown=CERTIFICATE_DELAY_SECONDS, kwargs=kwargs)
         return True

lms/djangoapps/certificates/tasks.py

@@ -4,6 +4,7 @@ from logging import getLogger
 from celery_utils.logged_task import LoggedTask
 from celery_utils.persist_on_failure import PersistOnFailureTask
 from django.contrib.auth.models import User
+from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification
 from opaque_keys.edx.keys import CourseKey
 
 from .api import generate_user_certificates
@@ -18,11 +19,27 @@ class _BaseCertificateTask(PersistOnFailureTask, LoggedTask):  # pylint: disable
     abstract = True
 
 
-@task(base=_BaseCertificateTask)
-def generate_certificate(**kwargs):
+@task(base=_BaseCertificateTask, bind=True, default_retry_delay=30, max_retries=2)
+def generate_certificate(self, **kwargs):
     """
     Generates a certificate for a single user.
+
+    kwargs:
+        - student: The student for whom to generate a certificate.
+        - course_key: The course key for the course that the student is
+            receiving a certificate in.
+        - expected_verification_status: The expected verification status
+            for the user.  When the status has changed, we double check
+            that the actual verification status is as expected before
+            generating a certificate, in the off chance that the database
+            has not yet updated with the user's new verification status.
     """
+    original_kwargs = kwargs.copy()
     student = User.objects.get(id=kwargs.pop('student'))
     course_key = CourseKey.from_string(kwargs.pop('course_key'))
+    expected_verification_status = kwargs.pop('expected_verification_status', None)
+    if expected_verification_status:
+        actual_verification_status, _ = SoftwareSecurePhotoVerification.user_status(student)
+        if expected_verification_status != actual_verification_status:
+            raise self.retry(kwargs=original_kwargs)
     generate_user_certificates(student=student, course_key=course_key, **kwargs)

lms/djangoapps/certificates/tests/test_signals.py

@@ -15,6 +15,7 @@ from lms.djangoapps.grades.new.course_grade_factory import CourseGradeFactory
 from lms.djangoapps.grades.tests.utils import mock_passing_grade
 from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification
 from openedx.core.djangoapps.certificates.config import waffle
+from lms.djangoapps.certificates.signals import CERTIFICATE_DELAY_SECONDS
 from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration
 from student.tests.factories import CourseEnrollmentFactory, UserFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
@@ -91,10 +92,13 @@ class WhitelistGeneratedCertificatesTest(ModuleStoreTestCase):
                     user=self.user,
                     course_id=self.course.id
                 )
-                mock_generate_certificate_apply_async.assert_called_with(kwargs={
-                    'student': unicode(self.user.id),
-                    'course_key': unicode(self.course.id),
-                })
+                mock_generate_certificate_apply_async.assert_called_with(
+                    countdown=CERTIFICATE_DELAY_SECONDS,
+                    kwargs={
+                        'student': unicode(self.user.id),
+                        'course_key': unicode(self.course.id),
+                    }
+                )
 
     def test_cert_generation_on_whitelist_append_instructor_paced(self):
         """
@@ -116,10 +120,13 @@ class WhitelistGeneratedCertificatesTest(ModuleStoreTestCase):
                     user=self.user,
                     course_id=self.ip_course.id
                 )
-                mock_generate_certificate_apply_async.assert_called_with(kwargs={
-                    'student': unicode(self.user.id),
-                    'course_key': unicode(self.ip_course.id),
-                })
+                mock_generate_certificate_apply_async.assert_called_with(
+                    countdown=CERTIFICATE_DELAY_SECONDS,
+                    kwargs={
+                        'student': unicode(self.user.id),
+                        'course_key': unicode(self.ip_course.id),
+                    }
+                )
 
 
 class PassingGradeCertsTest(ModuleStoreTestCase):
@@ -164,10 +171,13 @@ class PassingGradeCertsTest(ModuleStoreTestCase):
                 # Certs fired after passing
                 with mock_passing_grade():
                     grade_factory.update(self.user, self.course)
-                    mock_generate_certificate_apply_async.assert_called_with(kwargs={
-                        'student': unicode(self.user.id),
-                        'course_key': unicode(self.course.id),
-                    })
+                    mock_generate_certificate_apply_async.assert_called_with(
+                        countdown=CERTIFICATE_DELAY_SECONDS,
+                        kwargs={
+                            'student': unicode(self.user.id),
+                            'course_key': unicode(self.course.id),
+                        }
+                    )
 
     def test_cert_generation_on_passing_instructor_paced(self):
         with mock.patch(
@@ -182,10 +192,13 @@ class PassingGradeCertsTest(ModuleStoreTestCase):
                 # Certs fired after passing
                 with mock_passing_grade():
                     grade_factory.update(self.user, self.ip_course)
-                    mock_generate_certificate_apply_async.assert_called_with(kwargs={
-                        'student': unicode(self.user.id),
-                        'course_key': unicode(self.ip_course.id),
-                    })
+                    mock_generate_certificate_apply_async.assert_called_with(
+                        countdown=CERTIFICATE_DELAY_SECONDS,
+                        kwargs={
+                            'student': unicode(self.user.id),
+                            'course_key': unicode(self.ip_course.id),
+                        }
+                    )
 
     def test_cert_already_generated(self):
         with mock.patch(
@@ -244,10 +257,14 @@ class LearnerTrackChangeCertsTest(ModuleStoreTestCase):
                     status='submitted'
                 )
                 attempt.approve()
-                mock_generate_certificate_apply_async.assert_called_with(kwargs={
-                    'student': unicode(self.user_one.id),
-                    'course_key': unicode(self.course_one.id),
-                })
+                mock_generate_certificate_apply_async.assert_called_with(
+                    countdown=CERTIFICATE_DELAY_SECONDS,
+                    kwargs={
+                        'student': unicode(self.user_one.id),
+                        'course_key': unicode(self.course_one.id),
+                        'expected_verification_status': SoftwareSecurePhotoVerification.STATUS.approved
+                    }
+                )
 
     def test_cert_generation_on_photo_verification_instructor_paced(self):
         with mock.patch(
@@ -261,7 +278,11 @@ class LearnerTrackChangeCertsTest(ModuleStoreTestCase):
                     status='submitted'
                 )
                 attempt.approve()
-                mock_generate_certificate_apply_async.assert_called_with(kwargs={
-                    'student': unicode(self.user_two.id),
-                    'course_key': unicode(self.course_two.id),
-                })
+                mock_generate_certificate_apply_async.assert_called_with(
+                    countdown=CERTIFICATE_DELAY_SECONDS,
+                    kwargs={
+                        'student': unicode(self.user_two.id),
+                        'course_key': unicode(self.course_two.id),
+                        'expected_verification_status': SoftwareSecurePhotoVerification.STATUS.approved
+                    }
+                )

lms/djangoapps/certificates/tests/test_tasks.py

 from unittest import TestCase
 
 import ddt
-from mock import patch
+from mock import call, patch
 from opaque_keys.edx.keys import CourseKey
+from nose.tools import assert_true
 
 from lms.djangoapps.certificates.tasks import generate_certificate
+from student.tests.factories import UserFactory
 
 
 @ddt.ddt
 class GenerateUserCertificateTest(TestCase):
     @patch('lms.djangoapps.certificates.tasks.generate_user_certificates')
     @patch('lms.djangoapps.certificates.tasks.User.objects.get')
-    def test_cert_task(self, user_get_mock, generate_user_certs_mock):
+    def test_generate_user_certs(self, user_get_mock, generate_user_certs_mock):
         course_key = 'course-v1:edX+CS101+2017_T2'
-
-        generate_certificate(student='student-id', course_key=course_key, otherarg='c', otherotherarg='d')
+        kwargs = {
+            'student': 'student-id',
+            'course_key': course_key,
+            'otherarg': 'c',
+            'otherotherarg': 'd'
+        }
+        generate_certificate.apply_async(kwargs=kwargs).get()
 
         expected_student = user_get_mock.return_value
         generate_user_certs_mock.assert_called_with(
@@ -26,10 +33,36 @@ class GenerateUserCertificateTest(TestCase):
         user_get_mock.assert_called_once_with(id='student-id')
 
     @ddt.data('student', 'course_key')
-    def test_cert_task_missing_args(self, missing_arg):
+    def test_missing_args(self, missing_arg):
         kwargs = {'student': 'a', 'course_key': 'b', 'otherarg': 'c'}
         del kwargs[missing_arg]
 
         with patch('lms.djangoapps.certificates.tasks.User.objects.get'):
             with self.assertRaisesRegexp(KeyError, missing_arg):
-                generate_certificate(**kwargs)
+                generate_certificate.apply_async(kwargs=kwargs).get()
+
+    @patch('lms.djangoapps.certificates.tasks.generate_user_certificates')
+    @patch('lms.djangoapps.verify_student.models.SoftwareSecurePhotoVerification.user_status')
+    def test_retry_until_verification_status_updates(self, user_status_mock, generate_user_certs_mock):
+        course_key = 'course-v1:edX+CS101+2017_T2'
+        student = UserFactory()
+
+        kwargs = {
+            'student': student.id,
+            'course_key': course_key,
+            'expected_verification_status': 'approved'
+        }
+
+        user_status_mock.side_effect = [('pending', ''), ('approved', '')]
+
+        generate_certificate.apply_async(kwargs=kwargs).get()
+
+        user_status_mock.assert_has_calls([
+            call(student),
+            call(student)
+        ])
+
+        generate_user_certs_mock.assert_called_once_with(
+            student=student,
+            course_key=CourseKey.from_string(course_key)
+        )