From 4dae1abd6763b0a1b7b95fba1ca9cec17f557235 Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Mon, 10 May 2021 15:04:11 -0400 Subject: [PATCH] fix: Remove unused allowlist flag (#27576) MICROBA-1073 --- lms/djangoapps/certificates/api.py | 4 -- .../certificates/generation_handler.py | 58 ++----------------- lms/djangoapps/certificates/services.py | 7 +-- lms/djangoapps/certificates/signals.py | 14 ++--- .../tests/test_generation_handler.py | 21 +++---- 5 files changed, 23 insertions(+), 81 deletions(-) diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 7a49846a8a..1e4ed3979d 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -25,7 +25,6 @@ from lms.djangoapps.certificates.generation_handler import ( can_generate_certificate_task as _can_generate_certificate_task, 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 ) @@ -612,9 +611,6 @@ def get_allowlisted_users(course_key): """ Return the users who are on the allowlist for this course run """ - if not _is_using_certificate_allowlist(course_key): - return User.objects.none() - return User.objects.filter(certificatewhitelist__course_id=course_key, certificatewhitelist__whitelist=True) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index 7c370a17f4..c6328b2080 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -34,28 +34,11 @@ log = logging.getLogger(__name__) WAFFLE_FLAG_NAMESPACE = LegacyWaffleFlagNamespace(name='certificates_revamp') -# .. toggle_name: certificates_revamp.use_allowlist -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Waffle flag to enable the course certificates allowlist (aka v2 of the certificate whitelist) -# on a per-course run basis. -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2021-01-27 -# .. toggle_target_removal_date: 2022-01-27 -# .. toggle_tickets: MICROBA-918 -CERTIFICATES_USE_ALLOWLIST = CourseWaffleFlag( - waffle_namespace=WAFFLE_FLAG_NAMESPACE, - flag_name='use_allowlist', - module_name=__name__, -) - - # .. toggle_name: certificates_revamp.use_updated # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False # .. toggle_description: Waffle flag to enable the updated regular (non-allowlist) course certificate logic on a -# per-course run basis. Note that if this flag is enabled for a course run, certificates_revamp.use_allowlist -# should also be enabled for that course run. +# per-course run basis. # .. toggle_use_cases: temporary # .. toggle_creation_date: 2021-03-05 # .. toggle_target_removal_date: 2022-03-05 @@ -72,10 +55,10 @@ def can_generate_certificate_task(user, course_key): Determine if we can create a task to generate a certificate for this user in this course run. This will return True if either: - - the course run is using the allowlist and the user is on the allowlist, or + - the user is on the allowlist for the course run, or - the course run is using v2 course certificates """ - if is_using_certificate_allowlist_and_is_on_allowlist(user, course_key): + if is_on_certificate_allowlist(user, course_key): return True elif is_using_v2_course_certificates(course_key): return True @@ -91,9 +74,9 @@ def generate_certificate_task(user, course_key, generation_mode=None): If the allowlist is enabled for this course run and the user is on the allowlist, the allowlist logic will be used. Otherwise, the regular course certificate generation logic will be used. """ - if is_using_certificate_allowlist_and_is_on_allowlist(user, course_key): - log.info(f'{course_key} is using allowlist certificates, and the user {user.id} is on its allowlist. Attempt ' - f'will be made to generate an allowlist certificate.') + if is_on_certificate_allowlist(user, course_key): + log.info(f'User {user.id} is on the allowlist for {course_key}. Attempt will be made to generate an allowlist ' + f'certificate.') return generate_allowlist_certificate_task(user, course_key, generation_mode) elif is_using_v2_course_certificates(course_key): @@ -157,12 +140,6 @@ def _can_generate_allowlist_certificate(user, course_key): Check if an allowlist 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_certificate_allowlist(course_key): - # This course run is not using the allowlist feature - log.info(f'{course_key} is not using the certificate allowlist. Allowlist certificate cannot be generated' - f'for {user.id}.') - return False - 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.') @@ -315,9 +292,6 @@ def _can_set_allowlist_cert_status(user, course_key): """ Determine whether we can set a custom (non-downloadable) cert status for an allowlist certificate """ - if not is_using_certificate_allowlist(course_key): - return False - if not is_on_certificate_allowlist(user, course_key): return False @@ -361,26 +335,6 @@ def _can_set_cert_status_common(user, course_key): return True -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 - - Currently this returns True for all course runs, as we're enabling the allowlist for everyone. - If all goes well, we'll come back and remove this method and the flag entirely in MICROBA-1073. - """ - return True -# return CERTIFICATES_USE_ALLOWLIST.is_enabled(course_key) - - def is_using_v2_course_certificates(course_key): """ Return True if the course run is using v2 course certificates diff --git a/lms/djangoapps/certificates/services.py b/lms/djangoapps/certificates/services.py index 44510efefb..77823e0ff3 100644 --- a/lms/djangoapps/certificates/services.py +++ b/lms/djangoapps/certificates/services.py @@ -8,7 +8,7 @@ import logging from django.core.exceptions import ObjectDoesNotExist from opaque_keys.edx.keys import CourseKey -from lms.djangoapps.certificates.generation_handler import is_using_certificate_allowlist_and_is_on_allowlist +from lms.djangoapps.certificates.generation_handler import is_on_certificate_allowlist from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.utils import _get_key @@ -26,9 +26,8 @@ class CertificateService: course run. """ course_key = _get_key(course_key_or_id, CourseKey) - if is_using_certificate_allowlist_and_is_on_allowlist(user_id, course_key): - log.info('{course_key} is using allowlist certificates, and the user {user_id} is on its allowlist. The ' - 'certificate will not be invalidated.'.format(course_key=course_key, user_id=user_id)) + if is_on_certificate_allowlist(user_id, course_key): + log.info(f'User {user_id} is on the allowlist for {course_key}. The certificate will not be invalidated.') return False try: diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 5db6f14c15..e561d68571 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -15,7 +15,7 @@ from lms.djangoapps.certificates.generation_handler import ( can_generate_certificate_task, generate_allowlist_certificate_task, generate_certificate_task, - is_using_certificate_allowlist_and_is_on_allowlist + is_on_certificate_allowlist ) from lms.djangoapps.certificates.models import ( CertificateGenerationCourseSetting, @@ -60,9 +60,9 @@ def _listen_for_certificate_whitelist_append(sender, instance, **kwargs): # pyl if not auto_certificate_generation_enabled(): return - if is_using_certificate_allowlist_and_is_on_allowlist(instance.user, instance.course_id): - log.info(f'{instance.course_id} is using allowlist certificates, and the user {instance.user.id} is now on ' - f'its allowlist. Attempt will be made to generate an allowlist certificate.') + if is_on_certificate_allowlist(instance.user, instance.course_id): + log.info(f'User {instance.user.id} is now on the allowlist for course {instance.course_id}. Attempt will be ' + f'made to generate an allowlist certificate.') return generate_allowlist_certificate_task(instance.user, instance.course_id) if _fire_ungenerated_certificate_task(instance.user, instance.course_id): @@ -106,9 +106,9 @@ def _listen_for_failing_grade(sender, user, course_id, grade, **kwargs): # pyli If needed, mark the certificate as notpassing. """ - if is_using_certificate_allowlist_and_is_on_allowlist(user, course_id): - log.info('{course_id} is using allowlist certificates, and the user {user_id} is on its allowlist. The ' - 'failing grade will not affect the certificate.'.format(course_id=course_id, user_id=user.id)) + if is_on_certificate_allowlist(user, course_id): + log.info(f'User {user.id} is on the allowlist for {course_id}. The failing grade will not affect the ' + f'certificate.') return cert = GeneratedCertificate.certificate_for_student(user, course_id) diff --git a/lms/djangoapps/certificates/tests/test_generation_handler.py b/lms/djangoapps/certificates/tests/test_generation_handler.py index 40485771fd..c3c206d64b 100644 --- a/lms/djangoapps/certificates/tests/test_generation_handler.py +++ b/lms/djangoapps/certificates/tests/test_generation_handler.py @@ -17,8 +17,7 @@ from lms.djangoapps.certificates.generation_handler import ( generate_allowlist_certificate_task, generate_certificate_task, generate_regular_certificate_task, - is_using_certificate_allowlist, - is_using_certificate_allowlist_and_is_on_allowlist, + is_on_certificate_allowlist, is_using_v2_course_certificates, _set_allowlist_cert_status, _set_v2_cert_status @@ -66,21 +65,15 @@ class AllowlistTests(ModuleStoreTestCase): # Whitelist user CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user) - def test_is_using_allowlist_true(self): + def test_is_on_allowlist(self): """ - Test the allowlist flag + Test the presence of the user on the allowlist """ - assert is_using_certificate_allowlist(self.course_run_key) + assert is_on_certificate_allowlist(self.user, self.course_run_key) - def test_is_using_allowlist_and_is_on_list(self): + def test_is_on_allowlist_false(self): """ - Test the allowlist flag and the presence of the user on the list - """ - assert 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 + Test the absence of the user on the allowlist """ u = UserFactory() CourseEnrollmentFactory( @@ -90,7 +83,7 @@ class AllowlistTests(ModuleStoreTestCase): mode="verified", ) CertificateAllowlistFactory.create(course_id=self.course_run_key, user=u, whitelist=False) - assert not is_using_certificate_allowlist_and_is_on_allowlist(u, self.course_run_key) + assert not is_on_certificate_allowlist(u, self.course_run_key) @ddt.data( (CertificateStatuses.deleted, True),