Merge pull request #26836 from edx/jhynes/mb-1023_bugfix

MB-1023 | Fix defect when removing allowlist entry on instructor dashboard

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):

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.

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
     )