From 05c82810e0fb95098c02daa505c425b9de3889ce Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Wed, 3 Mar 2021 13:14:01 -0500 Subject: [PATCH] MB-1023 | Fix defect when removing allowlist entry on instructor dashboard [MB-1023] - Fix issue from recent refactor. Verify certificate is not none before attempting to invalidate the certificate. - Add more logging --- lms/djangoapps/certificates/api.py | 21 +++++++++- lms/djangoapps/certificates/tests/test_api.py | 42 +++++++++++++++++++ lms/djangoapps/instructor/views/api.py | 33 +++++++-------- 3 files changed, 77 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 5a97ecd22b..7ec9cdc627 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -11,6 +11,7 @@ certificates models or any other certificates modules. import logging from django.contrib.auth import get_user_model +from django.core.exceptions import ObjectDoesNotExist from django.db.models import Q from eventtracking import tracker from opaque_keys.edx.django.models import CourseKeyField @@ -578,6 +579,7 @@ def create_certificate_allowlist_entry(user, course_key, notes): """ Creates a certificate exception for a given learner 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( user=user, course_id=course_key, @@ -594,6 +596,7 @@ def create_certificate_invalidation_entry(certificate, user_requesting_invalidat """ Invalidates a certificate with the given certificate id. """ + log.info(f"Creating a certificate invalidation entry linked to certificate with id {certificate.id}.") certificate_invalidation, __ = CertificateInvalidation.objects.update_or_create( generated_certificate=certificate, defaults={ @@ -610,14 +613,28 @@ def get_allowlist_entry(user, course_key): """ Retrieves and returns an allowlist entry for a given learner and course-run. """ - return CertificateWhitelist.objects.get(user=user, course_id=course_key) + 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. """ - return CertificateInvalidation.objects.get(generated_certificate=certificate) + log.info(f"Attempting to retrieve certificate invalidation entry for certificate with id {certificate.id}.") + try: + certificate_invalidation_entry = CertificateInvalidation.objects.get(generated_certificate=certificate) + except ObjectDoesNotExist: + log.warning(f"No certificate invalidation found linked to certificate with id {certificate.id}.") + return None + + return certificate_invalidation_entry def is_on_allowlist(user, course_key): diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index 693de69a7f..49be77761f 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -19,6 +19,7 @@ from freezegun import freeze_time from mock import patch from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import CourseLocator +from testfixtures import LogCapture from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -944,6 +945,23 @@ class InstructorDashboardFunctionalityTests(ModuleStoreTestCase): 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. @@ -967,6 +985,30 @@ class InstructorDashboardFunctionalityTests(ModuleStoreTestCase): assert retrieved_invalidation.generated_certificate == certificate assert retrieved_invalidation.active == invalidation.active + def test_get_certificate_invalidation_entry_dne(self): + """ + Test to verify behavior when a certificate invalidation entry does not exist. + """ + certificate = GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course_run_key, + status=CertificateStatuses.unavailable, + mode='verified' + ) + + expected_messages = [ + f"Attempting to retrieve certificate invalidation entry for certificate with id {certificate.id}.", + f"No certificate invalidation found linked to certificate with id {certificate.id}.", + ] + + with LogCapture() as log: + retrieved_invalidation = get_certificate_invalidation_entry(certificate) + + assert retrieved_invalidation 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. diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 1f66a4bed0..62c96ff05a 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -2642,6 +2642,7 @@ def add_certificate_exception(course_key, student, certificate_exception): :param certificate_exception: A dict object containing certificate exception info. :return: CertificateWhitelist item in dict format containing certificate exception info. """ + log.info(f"Request received to add an allowlist entry for student {student.id} in course {course_key}") # Check if the learner is blocked from receiving certificates in this course run. if certs_api.is_certificate_invalidated(student, course_key): raise ValueError( @@ -2686,27 +2687,23 @@ def remove_certificate_exception(course_key, student): :param student: User object whose certificate exception needs to be removed. :return: """ - try: - certificate_exception = certs_api.get_allowlist_entry(student, course_key) - except ObjectDoesNotExist: + 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) ) - try: - certificate = certs_api.get_certificate_for_user(student.username, course_key, False) - log.info( - f"Invalidating certificate for student {student.id} in course {course_key} before removing them from the " - "allowlist." - ) + # 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() - except ObjectDoesNotExist: - # Certificate has not been generated yet, so just remove the certificate exception from white list - pass log.info(f"Removing student {student.id} from the allowlist in course {course_key}.") - certificate_exception.delete() + allowlist_entry.delete() def parse_request_data_and_get_user(request, course_key): @@ -3018,12 +3015,14 @@ def re_validate_certificate(request, course_key, generated_certificate, student) :param course_key: CourseKey object identifying the current course. :param generated_certificate: GeneratedCertificate object of the student for the given course """ - try: - certificate_invalidation = certs_api.get_certificate_invalidation_entry(generated_certificate) - certificate_invalidation.deactivate() - except ObjectDoesNotExist: + log.info(f"Attempting to revalidate certificate for student {student.id} in course {course_key}") + + certificate_invalidation = certs_api.get_certificate_invalidation_entry(generated_certificate) + if not certificate_invalidation: raise ValueError(_("Certificate Invalidation does not exist, Please refresh the page and try again.")) # lint-amnesty, pylint: disable=raise-missing-from + certificate_invalidation.deactivate() + task_api.generate_certificates_for_students( request, course_key, student_set="specific_student", specific_student_id=student.id )