From 19d1105d0f4f6f1fd1dfb86f1fe506c7c8775b35 Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Tue, 13 Apr 2021 13:12:02 -0400 Subject: [PATCH] feat: skip bulk certificate invalidation during bulk regeneration in v2 certs [MICROBA-1077] * Skip bulk certificate invalidation during bulk regeneration. I couldn't come up with a reason to continue to invalidate certificates right before we attempted regeneration. --- lms/djangoapps/certificates/api.py | 8 +++ .../certificates/generation_handler.py | 14 +++--- .../tests/test_generation_handler.py | 6 +-- .../instructor_task/tasks_helper/certs.py | 41 +++++++++------- .../tests/test_tasks_helper.py | 49 ++++++++++++++++++- 5 files changed, 90 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index e059370c9b..ea9e337e20 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -24,6 +24,7 @@ from lms.djangoapps.certificates.generation_handler import ( generate_certificate_task as _generate_certificate_task, generate_user_certificates as _generate_user_certificates, is_using_certificate_allowlist as _is_using_certificate_allowlist, + is_using_v2_course_certificates as _is_using_v2_course_certificates, regenerate_user_certificates as _regenerate_user_certificates ) from lms.djangoapps.certificates.models import ( @@ -705,3 +706,10 @@ def get_certificate_invalidation_entry(certificate): return None return certificate_invalidation_entry + + +def is_using_v2_course_certificates(course_key): + """ + Determines if the given course run is using V2 of course certificates + """ + return _is_using_v2_course_certificates(course_key) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index 6710e92b4a..2eab86fe58 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -74,7 +74,7 @@ def can_generate_certificate_task(user, course_key): """ if is_using_certificate_allowlist_and_is_on_allowlist(user, course_key): return True - elif _is_using_v2_course_certificates(course_key): + elif is_using_v2_course_certificates(course_key): return True return False @@ -93,7 +93,7 @@ def generate_certificate_task(user, course_key, generation_mode=None): f'will be made to generate an allowlist certificate.') return generate_allowlist_certificate_task(user, course_key, generation_mode) - elif _is_using_v2_course_certificates(course_key): + elif is_using_v2_course_certificates(course_key): log.info(f'{course_key} is using v2 course certificates. Attempt will be made to generate a certificate for ' f'user {user.id}.') return generate_regular_certificate_task(user, course_key, generation_mode) @@ -154,7 +154,7 @@ def _can_generate_allowlist_certificate(user, course_key): f'for {user.id}.') return False - if not _is_on_certificate_allowlist(user, course_key): + if not is_on_certificate_allowlist(user, course_key): log.info(f'{user.id} : {course_key} is not on the certificate allowlist. Allowlist certificate cannot be ' f'generated.') return False @@ -175,7 +175,7 @@ def _can_generate_v2_certificate(user, course_key): Check if a v2 course certificate can be generated (created if it doesn't already exist, or updated if it does exist) for this user, in this course run. """ - if not _is_using_v2_course_certificates(course_key): + if not is_using_v2_course_certificates(course_key): # This course run is not using the v2 course certificate feature log.info(f'{course_key} is not using v2 course certificates. Certificate cannot be generated.') return False @@ -245,7 +245,7 @@ def is_using_certificate_allowlist_and_is_on_allowlist(user, course_key): 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) + return is_using_certificate_allowlist(course_key) and is_on_certificate_allowlist(user, course_key) def is_using_certificate_allowlist(course_key): @@ -255,14 +255,14 @@ def is_using_certificate_allowlist(course_key): return CERTIFICATES_USE_ALLOWLIST.is_enabled(course_key) -def _is_using_v2_course_certificates(course_key): +def is_using_v2_course_certificates(course_key): """ Return True if the course run is using v2 course certificates """ return CERTIFICATES_USE_UPDATED.is_enabled(course_key) -def _is_on_certificate_allowlist(user, course_key): +def is_on_certificate_allowlist(user, course_key): """ Check if the user is on the allowlist, and is enabled for the allowlist, for this course run """ diff --git a/lms/djangoapps/certificates/tests/test_generation_handler.py b/lms/djangoapps/certificates/tests/test_generation_handler.py index 818e060396..502badd607 100644 --- a/lms/djangoapps/certificates/tests/test_generation_handler.py +++ b/lms/djangoapps/certificates/tests/test_generation_handler.py @@ -12,7 +12,7 @@ from lms.djangoapps.certificates.generation_handler import ( CERTIFICATES_USE_ALLOWLIST, CERTIFICATES_USE_UPDATED, is_using_certificate_allowlist, - _is_using_v2_course_certificates, + is_using_v2_course_certificates, _can_generate_allowlist_certificate, _can_generate_certificate_for_status, _can_generate_v2_certificate, @@ -309,14 +309,14 @@ class CertificateTests(ModuleStoreTestCase): """ Test the updated flag """ - assert _is_using_v2_course_certificates(self.course_run_key) + assert is_using_v2_course_certificates(self.course_run_key) @override_waffle_flag(CERTIFICATES_USE_UPDATED, active=False) def test_is_using_updated_false(self): """ Test the updated flag without the override """ - assert not _is_using_v2_course_certificates(self.course_run_key) + assert not is_using_v2_course_certificates(self.course_run_key) @ddt.data( (CertificateStatuses.deleted, True), diff --git a/lms/djangoapps/instructor_task/tasks_helper/certs.py b/lms/djangoapps/instructor_task/tasks_helper/certs.py index 03dc3cf5b8..d9299b9c4c 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/certs.py +++ b/lms/djangoapps/instructor_task/tasks_helper/certs.py @@ -15,7 +15,8 @@ from lms.djangoapps.certificates.api import ( can_generate_certificate_task, generate_certificate_task, generate_user_certificates, - get_allowlisted_users + get_allowlisted_users, + is_using_v2_course_certificates, ) from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from xmodule.modulestore.django import modulestore @@ -140,27 +141,33 @@ def _invalidate_generated_certificates(course_id, enrolled_students, certificate Invalidate generated certificates for all enrolled students in the given course having status in 'certificate_statuses', if the student is not on the course's allowlist. - Generated Certificates are invalidated by marking its status 'unavailable' and updating verify_uuid, download_uuid, + Generated Certificates are invalidated by marking its status 'unavailable' and updating error_reason, download_uuid, download_url and grade with empty string. + If V2 of Course Certificates is enabled for this course-run, this step will be skipped. + :param course_id: Course Key for the course whose generated certificates need to be removed :param enrolled_students: (queryset or list) students enrolled in the course :param certificate_statuses: certificates statuses for whom to remove generated certificate """ - certificates = GeneratedCertificate.objects.filter( - user__in=enrolled_students, - course_id=course_id, - status__in=certificate_statuses, - ) + if is_using_v2_course_certificates(course_id): + log.info(f"Course {course_id} is using V2 certificates. Skipping certificate invalidation step of bulk " + "regeneration.") + else: + certificates = GeneratedCertificate.objects.filter( + user__in=enrolled_students, + course_id=course_id, + status__in=certificate_statuses, + ) - allowlisted_users = get_allowlisted_users(course_id) + allowlisted_users = get_allowlisted_users(course_id) - # Invalidate each cert that is not allowlisted. We loop over the certs and invalidate each individually in order to - # save a history of the change. - for c in certificates: - if c.user in allowlisted_users: - log.info(f'Certificate for user {c.user.id} will not be invalidated because they are on the allowlist for ' - f'course {course_id}') - else: - log.info(f'About to invalidate certificate for user {c.user.id} in course {course_id}') - c.invalidate() + # Invalidate each cert that is not allowlisted. We loop over the certs and invalidate each individually in order + # to save a history of the change. + for c in certificates: + if c.user in allowlisted_users: + log.info(f'Certificate for user {c.user.id} will not be invalidated because they are on the allowlist ' + f'for course {course_id}') + else: + log.info(f'About to invalidate certificate for user {c.user.id} in course {course_id}') + c.invalidate() diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 4d5b37c91e..801600fe41 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -30,7 +30,10 @@ from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentAllowed 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 ( + CERTIFICATES_USE_ALLOWLIST, + CERTIFICATES_USE_UPDATED +) from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.certificates.tests.factories import CertificateWhitelistFactory, GeneratedCertificateFactory from lms.djangoapps.courseware.models import StudentModule @@ -2529,6 +2532,50 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): invalidated_cert = certs.first() assert invalidated_cert.status == CertificateStatuses.unavailable + @override_waffle_flag(CERTIFICATES_USE_UPDATED, active=False) + def test_invalidation_v2_certificates_disabled(self): + """ + Test that ensures the bulk invalidation step (as part of bulk certificate regeneration) continues to occur when + the v2 certificates feature is disabled for a course run. + """ + students = self._create_students(2) + + for s in students: + GeneratedCertificateFactory.create( + user=s, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + mode='verified' + ) + + _invalidate_generated_certificates(self.course.id, students, [CertificateStatuses.downloadable]) + + for s in students: + cert = GeneratedCertificate.objects.get(user=s, course_id=self.course.id) + assert cert.status == CertificateStatuses.unavailable + + @override_waffle_flag(CERTIFICATES_USE_UPDATED, active=True) + def test_invalidation_v2_certificates_enabled(self): + """ + Test that ensures the bulk invalidation step (as part of bulk certificate regeneration) is skipped when the v2 + certificates feature is enabled for a course run. + """ + students = self._create_students(2) + + for s in students: + GeneratedCertificateFactory.create( + user=s, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + mode='verified' + ) + + _invalidate_generated_certificates(self.course.id, students, [CertificateStatuses.downloadable]) + + for s in students: + cert = GeneratedCertificate.objects.get(user=s, course_id=self.course.id) + assert cert.status == CertificateStatuses.downloadable + def assertCertificatesGenerated(self, task_input, expected_results): """ Generate certificates for the given task_input and compare with expected_results.