From 3d6ab52b586f3bb31037472d0e004e1a2c0bde7d Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Mon, 1 Feb 2021 13:12:10 -0500 Subject: [PATCH] MICROBA-934 Trigger allowlist certificate generation from signals (#26257) --- .../certificates/generation_handler.py | 21 ++++++- lms/djangoapps/certificates/signals.py | 30 +++++++--- lms/djangoapps/certificates/tasks.py | 2 +- .../tests/test_generation_handler.py | 42 ++++++++++++-- .../certificates/tests/test_signals.py | 58 ++++++++++++++++++- 5 files changed, 134 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index 81f0f16150..f62ff82207 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -19,8 +19,7 @@ from lms.djangoapps.certificates.models import ( CertificateWhitelist, GeneratedCertificate ) -from lms.djangoapps.certificates.signals import CERTIFICATE_DELAY_SECONDS -from lms.djangoapps.certificates.tasks import generate_certificate +from lms.djangoapps.certificates.tasks import CERTIFICATE_DELAY_SECONDS, generate_certificate from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.certificates.api import auto_certificate_generation_enabled from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag @@ -108,7 +107,7 @@ def can_generate_allowlist_certificate(user, course_key): )) return False - if not CertificateWhitelist.objects.filter(user=user, course_id=course_key, whitelist=True).exists(): + if not _is_on_certificate_allowlist(user, course_key): log.info('{user} : {course} is not on the certificate allowlist. Certificate cannot be generated.'.format( user=user.id, course=course_key @@ -123,6 +122,15 @@ def can_generate_allowlist_certificate(user, course_key): return _can_generate_allowlist_certificate_for_status(cert) +def is_using_certificate_allowlist_and_is_on_allowlist(user, course_key): + """ + Return True if both: + 1) the course run is using the allowlist, and + 2) if the user is on the allowlist for this course run + """ + return _is_using_certificate_allowlist(course_key) and _is_on_certificate_allowlist(user, course_key) + + def _is_using_certificate_allowlist(course_key): """ Check if the course run is using the allowlist, aka V2 of certificate whitelisting @@ -130,6 +138,13 @@ def _is_using_certificate_allowlist(course_key): return CERTIFICATES_USE_ALLOWLIST.is_enabled(course_key) +def _is_on_certificate_allowlist(user, course_key): + """ + Check if the user is on the allowlist for this course run + """ + return CertificateWhitelist.objects.filter(user=user, course_id=course_key, whitelist=True).exists() + + def _can_generate_allowlist_certificate_for_status(cert): """ Check if the user's certificate status allows certificate generation diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 8fa8c9efe9..f25cb79e28 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -2,7 +2,6 @@ Signal handler for enabling/disabling self-generated certificates based on the course-pacing. """ - import logging import six @@ -10,13 +9,18 @@ from django.db.models.signals import post_save from django.dispatch import receiver from common.djangoapps.course_modes.models import CourseMode +from common.djangoapps.student.models import CourseEnrollment +from lms.djangoapps.certificates.generation_handler import ( + generate_allowlist_certificate_task, + is_using_certificate_allowlist_and_is_on_allowlist +) from lms.djangoapps.certificates.models import ( CertificateGenerationCourseSetting, CertificateStatuses, CertificateWhitelist, GeneratedCertificate ) -from lms.djangoapps.certificates.tasks import generate_certificate +from lms.djangoapps.certificates.tasks import CERTIFICATE_DELAY_SECONDS, generate_certificate from lms.djangoapps.grades.api import CourseGradeFactory from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.certificates.api import auto_certificate_generation_enabled @@ -27,10 +31,8 @@ from openedx.core.djangoapps.signals.signals import ( COURSE_GRADE_NOW_PASSED, LEARNER_NOW_VERIFIED ) -from common.djangoapps.student.models import CourseEnrollment log = logging.getLogger(__name__) -CERTIFICATE_DELAY_SECONDS = 2 @receiver(COURSE_PACING_CHANGED, dispatch_uid="update_cert_settings_on_pacing_change") @@ -54,11 +56,11 @@ def _listen_for_certificate_whitelist_append(sender, instance, **kwargs): # pyl if not auto_certificate_generation_enabled(): return - fire_ungenerated_certificate_task(instance.user, instance.course_id) - log.info(u'Certificate generation task initiated for {user} : {course} via whitelist'.format( - user=instance.user.id, - course=instance.course_id - )) + if fire_ungenerated_certificate_task(instance.user, instance.course_id): + log.info(u'Certificate generation task initiated for {user} : {course} via whitelist'.format( + user=instance.user.id, + course=instance.course_id + )) @receiver(COURSE_GRADE_NOW_PASSED, dispatch_uid="new_passing_learner") @@ -149,6 +151,15 @@ def fire_ungenerated_certificate_task(user, course_key, expected_verification_st message = u'Entered into Ungenerated Certificate task for {user} : {course}' log.info(message.format(user=user.id, course=course_key)) + if is_using_certificate_allowlist_and_is_on_allowlist(user, course_key): + log.info('{course} is using allowlist certificates, and the user {user} is on its allowlist. Attempt will be ' + 'made to generate an allowlist certificate.'.format(course=course_key, user=user.id)) + generate_allowlist_certificate_task(user, course_key) + return True + + log.info('{course} is not using allowlist certificates (or user {user} is not on its allowlist). The normal ' + 'generation logic will be followed.'.format(course=course_key, user=user.id)) + allowed_enrollment_modes_list = [ CourseMode.VERIFIED, CourseMode.CREDIT_MODE, @@ -176,3 +187,4 @@ def fire_ungenerated_certificate_task(user, course_key, expected_verification_st message = u'Certificate Generation task failed for {user} : {course}' log.info(message.format(user=user.id, course=course_key)) + return False diff --git a/lms/djangoapps/certificates/tasks.py b/lms/djangoapps/certificates/tasks.py index e0370ab2c5..49ddea2761 100644 --- a/lms/djangoapps/certificates/tasks.py +++ b/lms/djangoapps/certificates/tasks.py @@ -2,7 +2,6 @@ Tasks that generate a course certificate for a user """ - from logging import getLogger from celery import shared_task @@ -16,6 +15,7 @@ from lms.djangoapps.certificates.generation import generate_allowlist_certificat from lms.djangoapps.verify_student.services import IDVerificationService logger = getLogger(__name__) +CERTIFICATE_DELAY_SECONDS = 2 @shared_task(base=LoggedPersistOnFailureTask, bind=True, default_retry_delay=30, max_retries=2) diff --git a/lms/djangoapps/certificates/tests/test_generation_handler.py b/lms/djangoapps/certificates/tests/test_generation_handler.py index c0d1574cc8..7cbc9d9c8e 100644 --- a/lms/djangoapps/certificates/tests/test_generation_handler.py +++ b/lms/djangoapps/certificates/tests/test_generation_handler.py @@ -14,12 +14,19 @@ from xmodule.modulestore.tests.factories import CourseFactory from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from lms.djangoapps.certificates.generation_handler import CERTIFICATES_USE_ALLOWLIST -from lms.djangoapps.certificates.generation_handler import _is_using_certificate_allowlist, \ - _can_generate_allowlist_certificate_for_status, generate_allowlist_certificate_task, \ - can_generate_allowlist_certificate +from lms.djangoapps.certificates.generation_handler import ( + _can_generate_allowlist_certificate_for_status, + _is_using_certificate_allowlist, + can_generate_allowlist_certificate, + generate_allowlist_certificate_task, + is_using_certificate_allowlist_and_is_on_allowlist +) from lms.djangoapps.certificates.models import GeneratedCertificate, CertificateStatuses -from lms.djangoapps.certificates.tests.factories import CertificateWhitelistFactory, GeneratedCertificateFactory, \ +from lms.djangoapps.certificates.tests.factories import ( + CertificateWhitelistFactory, + GeneratedCertificateFactory, CertificateInvalidationFactory +) from openedx.core.djangoapps.certificates.config import waffle log = logging.getLogger(__name__) @@ -70,6 +77,33 @@ class AllowlistTests(ModuleStoreTestCase): """ self.assertFalse(_is_using_certificate_allowlist(self.course_run_key)) + def test_is_using_allowlist_and_is_on_list(self): + """ + Test the allowlist flag and the presence of the user on the list + """ + self.assertTrue(is_using_certificate_allowlist_and_is_on_allowlist(self.user, self.course_run_key)) + + @override_waffle_flag(CERTIFICATES_USE_ALLOWLIST, active=False) + def test_is_using_allowlist_and_is_on_list_with_flag_off(self): + """ + Test the allowlist flag and the presence of the user on the list when the flag is off + """ + self.assertFalse(is_using_certificate_allowlist_and_is_on_allowlist(self.user, self.course_run_key)) + + def test_is_using_allowlist_and_is_on_list_true(self): + """ + Test the allowlist flag and the presence of the user on the list when the user is not on the list + """ + u = UserFactory() + CourseEnrollmentFactory( + user=u, + course_id=self.course_run_key, + is_active=True, + mode="verified", + ) + CertificateWhitelistFactory.create(course_id=self.course_run_key, user=u, whitelist=False) + self.assertFalse(is_using_certificate_allowlist_and_is_on_allowlist(u, self.course_run_key)) + @ddt.data( (CertificateStatuses.deleted, True), (CertificateStatuses.deleting, True), diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index a75922c0e5..7b15dbef00 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/lms/djangoapps/certificates/tests/test_signals.py @@ -8,19 +8,22 @@ import ddt import mock import six from edx_toggles.toggles import LegacyWaffleSwitch +from edx_toggles.toggles.testutils import override_waffle_flag from edx_toggles.toggles.testutils import override_waffle_switch -from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from lms.djangoapps.certificates import api as certs_api +from lms.djangoapps.certificates.generation_handler import CERTIFICATES_USE_ALLOWLIST from lms.djangoapps.certificates.models import ( CertificateGenerationConfiguration, CertificateStatuses, CertificateWhitelist, GeneratedCertificate ) -from lms.djangoapps.certificates.signals import CERTIFICATE_DELAY_SECONDS, fire_ungenerated_certificate_task +from lms.djangoapps.certificates.signals import fire_ungenerated_certificate_task +from lms.djangoapps.certificates.tasks import CERTIFICATE_DELAY_SECONDS from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory from lms.djangoapps.grades.tests.utils import mock_passing_grade from lms.djangoapps.verify_student.models import IDVerificationAttempt, SoftwareSecurePhotoVerification @@ -135,6 +138,57 @@ class WhitelistGeneratedCertificatesTest(ModuleStoreTestCase): } ) + @override_waffle_flag(CERTIFICATES_USE_ALLOWLIST, active=True) + def test_fire_task_allowlist_enabled(self): + """ + Test that the allowlist generation is invoked if the allowlist is enabled for a user on the list + """ + with mock.patch( + 'lms.djangoapps.certificates.signals.generate_certificate.apply_async', + return_value=None + ) as mock_generate_certificate_apply_async: + with mock.patch( + 'lms.djangoapps.certificates.signals.generate_allowlist_certificate_task', + return_value=None + ) as mock_generate_allowlist_task: + CertificateWhitelist.objects.create( + user=self.user, + course_id=self.ip_course.id, + whitelist=True + ) + + fire_ungenerated_certificate_task(self.user, self.ip_course.id) + mock_generate_certificate_apply_async.assert_not_called() + mock_generate_allowlist_task.assert_called_with(self.user, self.ip_course.id) + + def test_fire_task_allowlist_disabled(self): + """ + Test that the normal logic is followed if the allowlist is disabled for a user on the list + """ + with mock.patch( + 'lms.djangoapps.certificates.signals.generate_certificate.apply_async', + return_value=None + ) as mock_generate_certificate_apply_async: + with mock.patch( + 'lms.djangoapps.certificates.signals.generate_allowlist_certificate_task', + return_value=None + ) as mock_generate_allowlist_task: + CertificateWhitelist.objects.create( + user=self.user, + course_id=self.ip_course.id, + whitelist=True + ) + + fire_ungenerated_certificate_task(self.user, self.ip_course.id) + mock_generate_certificate_apply_async.assert_called_with( + countdown=CERTIFICATE_DELAY_SECONDS, + kwargs={ + 'student': six.text_type(self.user.id), + 'course_key': six.text_type(self.ip_course.id), + } + ) + mock_generate_allowlist_task.assert_not_called() + class PassingGradeCertsTest(ModuleStoreTestCase): """