diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 08971de42f..7372ef7e81 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/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 diff --git a/lms/djangoapps/certificates/tasks.py b/lms/djangoapps/certificates/tasks.py index c297d2f516..7c5988d99a 100644 --- a/lms/djangoapps/certificates/tasks.py +++ b/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) diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index 77fea0e382..5fba7260a0 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/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 + } + ) diff --git a/lms/djangoapps/certificates/tests/test_tasks.py b/lms/djangoapps/certificates/tests/test_tasks.py index 2f391b682e..2538cf97e0 100644 --- a/lms/djangoapps/certificates/tests/test_tasks.py +++ b/lms/djangoapps/certificates/tests/test_tasks.py @@ -1,20 +1,27 @@ 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) + )