From 32685a7999f6f93adf00783cd13ecc6f27c8ea19 Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Fri, 5 Mar 2021 10:09:45 -0500 Subject: [PATCH] MICROBA-1025 | Update cert_whitelist.py management command [MICROBA-1025] - Update management command to use the same logic that the Instructor Dashboard uses - Fix bug in management command where processing stopped when encountering a user that did not exist - Add more logging - Add and update tests where needed --- common/djangoapps/student/api.py | 5 + lms/djangoapps/certificates/api.py | 111 +++++--- .../management/commands/cert_whitelist.py | 55 ++-- .../commands/tests/test_cert_whitelist.py | 86 +++++- lms/djangoapps/certificates/tests/test_api.py | 249 +++++++++++++----- .../instructor/tests/test_certificates.py | 7 +- lms/djangoapps/instructor/views/api.py | 31 +-- 7 files changed, 386 insertions(+), 158 deletions(-) diff --git a/common/djangoapps/student/api.py b/common/djangoapps/student/api.py index dfcd35cdf6..d1effcae89 100644 --- a/common/djangoapps/student/api.py +++ b/common/djangoapps/student/api.py @@ -4,6 +4,8 @@ Python APIs exposed by the student app to other in-process apps. """ +import logging + from django.contrib.auth import get_user_model from django.conf import settings @@ -48,6 +50,8 @@ TRANSITION_STATES = ( COURSE_DASHBOARD_PLUGIN_VIEW_NAME = "course_dashboard" +log = logging.getLogger() + def create_manual_enrollment_audit( enrolled_by, @@ -108,4 +112,5 @@ def is_user_enrolled_in_course(student, course_key): """ Determines if a learner is enrolled in a given course-run. """ + log.info(f"Checking if {student.id} is enrolled in course {course_key}") return CourseEnrollment.is_enrolled(student, course_key) diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 7ec9cdc627..312c3fb654 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -17,17 +17,16 @@ from eventtracking import tracker from opaque_keys.edx.django.models import CourseKeyField from organizations.api import get_course_organization_id +from common.djangoapps.student.api import is_user_enrolled_in_course from lms.djangoapps.branding import api as branding_api from lms.djangoapps.certificates.generation_handler import \ - generate_user_certificates as _generate_user_certificates + generate_allowlist_certificate_task as _generate_allowlist_certificate_task +from lms.djangoapps.certificates.generation_handler import generate_user_certificates as _generate_user_certificates from lms.djangoapps.certificates.generation_handler import \ is_using_certificate_allowlist as _is_using_certificate_allowlist from lms.djangoapps.certificates.generation_handler import \ is_using_certificate_allowlist_and_is_on_allowlist as _is_using_certificate_allowlist_and_is_on_allowlist -from lms.djangoapps.certificates.generation_handler import \ - generate_allowlist_certificate_task as _generate_allowlist_certificate_task -from lms.djangoapps.certificates.generation_handler import \ - regenerate_user_certificates as _regenerate_user_certificates +from lms.djangoapps.certificates.generation_handler import regenerate_user_certificates as _regenerate_user_certificates from lms.djangoapps.certificates.models import ( CertificateGenerationConfiguration, CertificateGenerationCourseSetting, @@ -300,6 +299,8 @@ def is_certificate_invalidated(student, course_key): Returns: Boolean denoting whether the certificate has been invalidated for this learner. """ + log.info(f"Checking if student {student.id} has an invalidated certificate in course {course_key}.") + certificate = GeneratedCertificate.certificate_for_student(student, course_key) if certificate: return CertificateInvalidation.has_certificate_invalidation(student, course_key) @@ -575,21 +576,88 @@ def get_allowlisted_users(course_key): return User.objects.filter(certificatewhitelist__course_id=course_key, certificatewhitelist__whitelist=True) -def create_certificate_allowlist_entry(user, course_key, notes): +def create_or_update_certificate_allowlist_entry(user, course_key, notes, enabled=True): """ - Creates a certificate exception for a given learner in a given course-run. + Update-or-create an allowlist entry for a student in a given course-run. """ - log.info(f"Creating an allowlist entry for student {user.id} in course {course_key}") - certificate_allowlist, __ = CertificateWhitelist.objects.get_or_create( + certificate_allowlist, created = CertificateWhitelist.objects.update_or_create( user=user, course_id=course_key, defaults={ - 'whitelist': True, + 'whitelist': enabled, 'notes': notes, } ) - return certificate_allowlist + log.info(f"Updated the allowlist of course {course_key} with student {user.id} and enabled={enabled}") + + return certificate_allowlist, created + + +def remove_allowlist_entry(user, course_key): + """ + Removes an allowlist entry for a user in a given course-run. If a certificate exists for the student in the + course-run we will also attempt to invalidate the it before removing them from the allowlist. + + Returns the result of the removal operation as a bool. + """ + log.info(f"Removing student {user.id} from the allowlist in course {course_key}") + + allowlist_entry = get_allowlist_entry(user, course_key) + if allowlist_entry: + certificate = get_certificate_for_user(user.username, course_key, False) + if certificate: + log.info(f"Invalidating certificate for student {user.id} in course {course_key} before allowlist removal.") + certificate.invalidate() + + log.info(f"Removing student {user.id} from the allowlist in course {course_key}.") + allowlist_entry.delete() + return True + + return False + + +def get_allowlist_entry(user, course_key): + """ + Retrieves and returns an allowlist entry for a given learner and course-run. + """ + log.info(f"Attempting to retrieve an allowlist entry for student {user.id} in course {course_key}.") + try: + allowlist_entry = CertificateWhitelist.objects.get(user=user, course_id=course_key) + except ObjectDoesNotExist: + log.warning(f"No allowlist entry found for student {user.id} in course {course_key}.") + return None + + return allowlist_entry + + +def is_on_allowlist(user, course_key): + """ + Determines if a learner has an active allowlist entry for a given course-run. + """ + log.info(f"Checking if student {user.id} is on the allowlist in course {course_key}") + return CertificateWhitelist.objects.filter(user=user, course_id=course_key, whitelist=True).exists() + + +def can_be_added_to_allowlist(user, course_key): + """ + Determines if a student is able to be added to the allowlist in a given course-run. + """ + log.info(f"Checking if student {user.id} in course {course_key} can be added to the allowlist.") + + if not is_user_enrolled_in_course(user, course_key): + log.info(f"Student {user.id} is not enrolled in course {course_key}") + return False + + if is_certificate_invalidated(user, course_key): + log.info(f"Student {user.id} is on the certificate invalidation list for course {course_key}") + return False + + if is_on_allowlist(user, course_key): + log.info(f"Student {user.id} already appears on allowlist in course {course_key}") + return False + + return True def create_certificate_invalidation_entry(certificate, user_requesting_invalidation, notes): @@ -609,20 +677,6 @@ def create_certificate_invalidation_entry(certificate, user_requesting_invalidat return certificate_invalidation -def get_allowlist_entry(user, course_key): - """ - Retrieves and returns an allowlist entry for a given learner and course-run. - """ - log.info(f"Attempting to retrieve an allowlist entry for student {user.id} in course {course_key}.") - try: - allowlist_entry = CertificateWhitelist.objects.get(user=user, course_id=course_key) - except ObjectDoesNotExist: - log.warning(f"No allowlist entry found for student {user.id} in course {course_key}.") - return None - - return allowlist_entry - - def get_certificate_invalidation_entry(certificate): """ Retrieves and returns an certificate invalidation entry for a given certificate id. @@ -635,10 +689,3 @@ def get_certificate_invalidation_entry(certificate): return None return certificate_invalidation_entry - - -def is_on_allowlist(user, course_key): - """ - Determines if a learner appears on the allowlist for a given course-run. - """ - return CertificateWhitelist.objects.filter(user=user, course_id=course_key, whitelist=True).exists() diff --git a/lms/djangoapps/certificates/management/commands/cert_whitelist.py b/lms/djangoapps/certificates/management/commands/cert_whitelist.py index c019048c9f..0876166767 100644 --- a/lms/djangoapps/certificates/management/commands/cert_whitelist.py +++ b/lms/djangoapps/certificates/management/commands/cert_whitelist.py @@ -1,5 +1,5 @@ """ -Management command which sets or gets the certificate whitelist for a given +Management command which sets or gets the certificate allowlist for a given user/course """ @@ -9,14 +9,19 @@ from django.core.management.base import BaseCommand, CommandError from django.db.models import Q from opaque_keys.edx.keys import CourseKey +from lms.djangoapps.certificates.api import ( + can_be_added_to_allowlist, + create_or_update_certificate_allowlist_entry, remove_allowlist_entry, +) from lms.djangoapps.certificates.models import CertificateWhitelist + User = get_user_model() def get_user_from_identifier(identifier): """ - This function takes the string identifier and fetch relevant user object from database + This function takes the string identifier and fetch relevant user object from database """ user = User.objects.filter(Q(username=identifier) | Q(email=identifier)).first() if not user: @@ -24,9 +29,26 @@ def get_user_from_identifier(identifier): return user +def update_allowlist(user, course, enable): + """ + Update the status of a user on the allowlist. + """ + if enable and can_be_added_to_allowlist(user, course): + create_or_update_certificate_allowlist_entry( + user, + course, + "Updated by mngmt cmd", + enable + ) + elif not enable: + remove_allowlist_entry(user, course) + else: + print(f"Failed to process allowlist request for student {user.id} in course {course} and enable={enable}.") + + class Command(BaseCommand): """ - Management command to set or get the certificate whitelist + Management command to set or get the certificate allowlist for a given user(s)/course """ @@ -58,14 +80,14 @@ class Command(BaseCommand): metavar='USER', dest='add', default=False, - help='user or list of users to add to the certificate whitelist' + help='user or list of users to add to the certificate allowlist' ) parser.add_argument( '-d', '--del', metavar='USER', dest='del', default=False, - help='user or list of users to remove from the certificate whitelist' + help='user or list of users to remove from the certificate allowlist' ) parser.add_argument( '-c', '--course-id', @@ -80,17 +102,6 @@ class Command(BaseCommand): if not course_id: raise CommandError("You must specify a course-id") - def update_user_whitelist(username, add=True): - """ - Update the status of whitelist user(s) - """ - user = get_user_from_identifier(username) - cert_whitelist, _created = CertificateWhitelist.objects.get_or_create( - user=user, course_id=course - ) - cert_whitelist.whitelist = add - cert_whitelist.save() - # try to parse the serialized course key into a CourseKey course = CourseKey.from_string(course_id) @@ -99,16 +110,22 @@ class Command(BaseCommand): if options['add'] or options['del']: user_str = options['add'] or options['del'] - add_to_whitelist = True if options['add'] else False # pylint: disable=simplifiable-if-expression + enable = True if options['add'] else False # pylint: disable=simplifiable-if-expression + users_list = user_str.split(",") for username in users_list: username = username.strip() if username: - update_user_whitelist(username, add=add_to_whitelist) + try: + user = get_user_from_identifier(username) + except CommandError as error: + print(f"Error occurred retrieving user {username}: {error}") + else: + update_allowlist(user, course, enable) whitelist = CertificateWhitelist.objects.filter(course_id=course) wl_users = '\n'.join( "{u.user.username} {u.user.email} {u.whitelist}".format(u=u) for u in whitelist ) - print(f"User whitelist for course {course_id}:\n{wl_users}") + print(f"Allowlist for course {course_id}:\n{wl_users}") diff --git a/lms/djangoapps/certificates/management/commands/tests/test_cert_whitelist.py b/lms/djangoapps/certificates/management/commands/tests/test_cert_whitelist.py index bc76c45d38..0dac3d1101 100644 --- a/lms/djangoapps/certificates/management/commands/tests/test_cert_whitelist.py +++ b/lms/djangoapps/certificates/management/commands/tests/test_cert_whitelist.py @@ -1,10 +1,15 @@ """ Extremely basic tests for the cert_whitelist command """ -import ddt import pytest -from django.core.management import call_command, CommandError +from django.core.exceptions import ObjectDoesNotExist +from django.core.management import call_command + +from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory +from lms.djangoapps.certificates.models import CertificateWhitelist +from lms.djangoapps.certificates.tests.factories import CertificateWhitelistFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory def test_cert_whitelist_help(capsys): @@ -18,20 +23,73 @@ def test_cert_whitelist_help(capsys): assert "COURSE_ID" in out -@ddt.ddt -class CertWhitelistGenerationTests(ModuleStoreTestCase): +class CertAllowlistManagementCommandTests(ModuleStoreTestCase): """ Tests for the cert_whitelist management command. """ - @ddt.data( - 'jeo', - 'jeo@edx.org', - 'robo_user, jenny, tom, jerry' - ) - def test_user_not_found(self, user_identifier): + def setUp(self): + super().setUp() + + self.user = UserFactory() + self.user2 = UserFactory() + self.course_run = CourseFactory() + self.course_run_key = self.course_run.id # pylint: disable=no-member + + CourseEnrollmentFactory( + user=self.user, + course_id=self.course_run_key, + is_active=True, + mode="verified", + ) + CourseEnrollmentFactory( + user=self.user2, + course_id=self.course_run_key, + is_active=True, + mode="verified", + ) + + def test_allowlist_entry_created(self): """ - Basic test to see if the command will raise user not found error. + Verify an allowlist entry can be made using the management command. """ - error_identifier = user_identifier.split(',')[0].strip() - with pytest.raises(CommandError, match=f"User {error_identifier} does not exist"): - call_command('cert_whitelist', '--add', user_identifier, '-c', 'MITx/6.002x/2012_Fall') + call_command( + "cert_whitelist", + "--add", + f"{self.user.username},{self.user2.username}", + "-c", + f"{self.course_run_key}") + + allowlist_entry_1 = CertificateWhitelist.objects.get(user_id=self.user.id, course_id=self.course_run_key) + assert allowlist_entry_1.user_id == self.user.id + assert allowlist_entry_1.course_id == self.course_run_key + + allowlist_entry_2 = CertificateWhitelist.objects.get(user_id=self.user2.id, course_id=self.course_run_key) + assert allowlist_entry_2.user_id == self.user2.id + assert allowlist_entry_2.course_id == self.course_run_key + + def test_allowlist_removal(self): + """ + Verify an allowlist entry can be removed using the management command. + """ + CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) + + call_command( + "cert_whitelist", + "--del", + f"{self.user.username}", + "-c", + f"{self.course_run_key}") + + with pytest.raises(ObjectDoesNotExist) as error: + CertificateWhitelist.objects.get(user=self.user, course_id=self.course_run_key) + assert str(error.value) == "CertificateWhitelist matching query does not exist." + + def test_bad_user_account(self): + """ + Verify that the management command will continue processing when running into a user account problem. + """ + call_command("cert_whitelist", "--add", f"gumby,{self.user.username}", "-c", f"{self.course_run_key}") + + allowlist_entry_1 = CertificateWhitelist.objects.get(user_id=self.user.id, course_id=self.course_run_key) + assert allowlist_entry_1.user_id == self.user.id + assert allowlist_entry_1.course_id == self.course_run_key diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index 49be77761f..8cb50a82d7 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -4,12 +4,14 @@ import uuid from contextlib import contextmanager from datetime import datetime, timedelta +import pytest import ddt import pytz import six from config_models.models import cache from django.conf import settings +from django.core.exceptions import ObjectDoesNotExist from django.test import RequestFactory, TestCase from django.test.utils import override_settings from django.urls import reverse @@ -32,10 +34,11 @@ from common.djangoapps.student.tests.factories import ( ) from common.djangoapps.util.testing import EventTestMixin from lms.djangoapps.certificates.api import ( + can_be_added_to_allowlist, cert_generation_enabled, certificate_downloadable_status, - create_certificate_allowlist_entry, create_certificate_invalidation_entry, + create_or_update_certificate_allowlist_entry, example_certificates_status, generate_example_certificates, generate_user_certificates, @@ -50,12 +53,14 @@ from lms.djangoapps.certificates.api import ( get_certificate_url, is_certificate_invalidated, is_on_allowlist, + remove_allowlist_entry, set_cert_generation_enabled ) from lms.djangoapps.certificates.generation_handler import CERTIFICATES_USE_ALLOWLIST from lms.djangoapps.certificates.models import ( CertificateGenerationConfiguration, CertificateStatuses, + CertificateWhitelist, ExampleCertificate, GeneratedCertificate, certificate_status_for_student @@ -884,11 +889,184 @@ class AllowlistTests(ModuleStoreTestCase): assert 0 == users.count() -class InstructorDashboardFunctionalityTests(ModuleStoreTestCase): +class CertificateAllowlistTests(ModuleStoreTestCase): """ - Tests for some functionality that the Instructor Dashboard django app relies on. + Tests for allowlist functionality. """ + def setUp(self): + super().setUp() + self.user = UserFactory() + self.global_staff = GlobalStaffFactory() + self.course_run = CourseFactory() + self.course_run_key = self.course_run.id # pylint: disable=no-member + + CourseEnrollmentFactory( + user=self.user, + course_id=self.course_run_key, + is_active=True, + mode="verified", + ) + + def test_create_certificate_allowlist_entry(self): + """ + Test for creating and updating allowlist entries. + """ + result, __ = create_or_update_certificate_allowlist_entry(self.user, self.course_run_key, "Testing!") + + assert result.course_id == self.course_run_key + assert result.user == self.user + assert result.notes == "Testing!" + + result, __ = create_or_update_certificate_allowlist_entry(self.user, self.course_run_key, "New test", False) + + assert result.notes == "New test" + assert not result.whitelist + + def test_remove_allowlist_entry(self): + """ + Test for removing an allowlist entry for a user in a given course-run. + """ + CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) + + result = remove_allowlist_entry(self.user, self.course_run_key) + assert result + + with pytest.raises(ObjectDoesNotExist) as error: + CertificateWhitelist.objects.get(user=self.user, course_id=self.course_run_key) + assert str(error.value) == "CertificateWhitelist matching query does not exist." + + def test_remove_allowlist_entry_with_certificate(self): + """ + Test for removing an allowlist entry. Verify that we also invalidate the certificate for the student. + """ + CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) + GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course_run_key, + status=CertificateStatuses.downloadable, + mode='verified' + ) + + result = remove_allowlist_entry(self.user, self.course_run_key) + assert result + + certificate = GeneratedCertificate.objects.get(user=self.user, course_id=self.course_run_key) + assert certificate.status == CertificateStatuses.unavailable + + with pytest.raises(ObjectDoesNotExist) as error: + CertificateWhitelist.objects.get(user=self.user, course_id=self.course_run_key) + assert str(error.value) == "CertificateWhitelist matching query does not exist." + + def test_remove_allowlist_entry_entry_dne(self): + """ + Test for removing an allowlist entry that does not exist + """ + result = remove_allowlist_entry(self.user, self.course_run_key) + assert not result + + def test_get_allowlist_entry(self): + """ + Test to verify that we can retrieve an allowlist entry for a learner. + """ + allowlist_entry = CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) + + retrieved_entry = get_allowlist_entry(self.user, self.course_run_key) + + assert retrieved_entry.id == allowlist_entry.id + assert retrieved_entry.course_id == allowlist_entry.course_id + assert retrieved_entry.user == allowlist_entry.user + + def test_get_allowlist_entry_dne(self): + """ + Test to verify behavior when an allowlist entry for a user does not exist + """ + expected_messages = [ + f"Attempting to retrieve an allowlist entry for student {self.user.id} in course {self.course_run_key}.", + f"No allowlist entry found for student {self.user.id} in course {self.course_run_key}." + ] + + with LogCapture() as log: + retrieved_entry = get_allowlist_entry(self.user, self.course_run_key) + + assert retrieved_entry is None + + for index, message in enumerate(expected_messages): + assert message in log.records[index].getMessage() + + def test_is_on_allowlist(self): + """ + Test to verify that we return True when an allowlist entry exists. + """ + CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) + + result = is_on_allowlist(self.user, self.course_run_key) + assert result + + def test_is_on_allowlist_expect_false(self): + """ + Test to verify that we will not return False when no allowlist entry exists. + """ + result = is_on_allowlist(self.user, self.course_run_key) + assert not result + + def test_is_on_allowlist_entry_disabled(self): + """ + Test to verify that we will return False when the allowlist entry if it is disabled. + """ + CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user, whitelist=False) + + result = is_on_allowlist(self.user, self.course_run_key) + assert not result + + def test_can_be_added_to_allowlist(self): + """ + Test to verify that a learner can be added to the allowlist that fits all needed criteria. + """ + assert can_be_added_to_allowlist(self.user, self.course_run_key) + + def test_can_be_added_to_allowlist_not_enrolled(self): + """ + Test to verify that a learner will be rejected from the allowlist without an active enrollmeint in a + course-run. + """ + new_course_run = CourseFactory() + + assert not can_be_added_to_allowlist(self.user, new_course_run.id) # pylint: disable=no-member + + def test_can_be_added_to_allowlist_certificate_invalidated(self): + """ + Test to verify that a learner will be rejected from the allowlist if they currently appear on the certificate + invalidation list. + """ + certificate = GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course_run_key, + status=CertificateStatuses.unavailable, + mode='verified' + ) + CertificateInvalidationFactory.create( + generated_certificate=certificate, + invalidated_by=self.global_staff, + active=True + ) + + assert not can_be_added_to_allowlist(self.user, self.course_run_key) + + def test_can_be_added_to_allowlist_is_already_on_allowlist(self): + """ + Test to verify that a learner will be rejected from the allowlist if they currently already appear on the + allowlist. + """ + CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) + + assert not can_be_added_to_allowlist(self.user, self.course_run_key) + + +class CertificateInvalidationTests(ModuleStoreTestCase): + """ + Tests for the certificate invalidation functionality. + """ def setUp(self): super().setUp() @@ -922,46 +1100,6 @@ class InstructorDashboardFunctionalityTests(ModuleStoreTestCase): assert result.active is True assert result.notes == "Test!" - def test_create_certificate_allowlist_entry(self): - """ - Test to verify that we can use the functionality defined in the Certificates api.py to create allowlist - entries. This is functionality the Instructor Dashboard django app relies on. - """ - result = create_certificate_allowlist_entry(self.user, self.course_run_key, "Testing!") - - assert result.course_id == self.course_run_key - assert result.user == self.user - assert result.notes == "Testing!" - - def test_get_allowlist_entry(self): - """ - Test to verify that we can retrieve an allowlist entry for a learner. - """ - allowlist_entry = CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) - - retrieved_entry = get_allowlist_entry(self.user, self.course_run_key) - - assert retrieved_entry.id == allowlist_entry.id - assert retrieved_entry.course_id == allowlist_entry.course_id - assert retrieved_entry.user == allowlist_entry.user - - def test_get_allowlist_entry_dne(self): - """ - Test to verify behavior when an allowlist entry for a user does not exist - """ - expected_messages = [ - f"Attempting to retrieve an allowlist entry for student {self.user.id} in course {self.course_run_key}.", - f"No allowlist entry found for student {self.user.id} in course {self.course_run_key}." - ] - - with LogCapture() as log: - retrieved_entry = get_allowlist_entry(self.user, self.course_run_key) - - assert retrieved_entry is None - - for index, message in enumerate(expected_messages): - assert message in log.records[index].getMessage() - def test_get_certificate_invalidation_entry(self): """ Test to verify that we can retrieve a certificate invalidation entry for a learner. @@ -1008,28 +1146,3 @@ class InstructorDashboardFunctionalityTests(ModuleStoreTestCase): for index, message in enumerate(expected_messages): assert message in log.records[index].getMessage() - - def test_is_on_allowlist(self): - """ - Test to verify that we return True when an allowlist entry exists. - """ - CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user) - - result = is_on_allowlist(self.user, self.course_run_key) - assert result - - def test_is_on_allowlist_expect_false(self): - """ - Test to verify that we will not return False when no allowlist entry exists. - """ - result = is_on_allowlist(self.user, self.course_run_key) - assert not result - - def test_is_on_allowlist_entry_disabled(self): - """ - Test to verify that we will return False when the allowlist entry if it is disabled. - """ - CertificateWhitelistFactory.create(course_id=self.course_run_key, user=self.user, whitelist=False) - - result = is_on_allowlist(self.user, self.course_run_key) - assert not result diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 64ff0d261d..4a2eb412a8 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -740,9 +740,10 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase): # Assert Request not successful assert not res_json['success'] # Assert Error Message - assert res_json['message'] ==\ - 'Certificate exception (user={user}) does not exist in certificate white list.' \ - ' Please refresh the page and try again.'.format(user=self.certificate_exception['user_name']) + assert res_json['message'] == ( + f"Error occurred removing the allowlist entry for student {self.user.username}. Please refresh the page " + "and try again" + ) def test_certificate_invalidation_already_exists(self): """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 99bca9df64..4948ec2695 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -2664,14 +2664,12 @@ def add_certificate_exception(course_key, student, certificate_exception): _("Student (username/email={user}) already in certificate exception list.").format(user=student.username) ) - certificate_allowlist_entry = certs_api.create_certificate_allowlist_entry( + certificate_allowlist_entry, __ = certs_api.create_or_update_certificate_allowlist_entry( student, course_key, certificate_exception.get("notes", "") ) - log.info(f"Student {student.id} has been added to the whitelist in course {course_key}") - generated_certificate = certs_api.get_certificate_for_user(student.username, course_key, False) exception = dict({ @@ -2688,32 +2686,21 @@ def add_certificate_exception(course_key, student, certificate_exception): def remove_certificate_exception(course_key, student): """ - Remove certificate exception for given course and student from CertificateWhitelist table and - invalidate its GeneratedCertificate if present. - Raises ValueError in case no exception exists for the student in the given course. + Remove certificate exception for given course and student from the certificate allowlist. + + Raises ValueError if an error occurs during removal of the allowlist entry. :param course_key: identifier of the course whose certificate exception needs to be removed. :param student: User object whose certificate exception needs to be removed. :return: """ - log.info(f"Request received to remove an allowlist entry for student {student.id} in course {course_key}.") - - allowlist_entry = certs_api.get_allowlist_entry(student, course_key) - if not allowlist_entry: - raise ValueError( # lint-amnesty, pylint: disable=raise-missing-from - _('Certificate exception (user={user}) does not exist in certificate white list. ' - 'Please refresh the page and try again.').format(user=student.username) + result = certs_api.remove_allowlist_entry(student, course_key) + if not result: + raise ValueError( + _("Error occurred removing the allowlist entry for student {student}. Please refresh the page " + "and try again").format(student=student.username) ) - # If a certificate was generated then we should invalidate it before removing the learner from the allowlist - certificate = certs_api.get_certificate_for_user(student.username, course_key, False) - if certificate: - log.info(f"Invalidating certificate for student {student.id} in course {course_key} before allowlist removal.") - certificate.invalidate() - - log.info(f"Removing student {student.id} from the allowlist in course {course_key}.") - allowlist_entry.delete() - def parse_request_data_and_get_user(request): """