MICROBA-1038 | Don't check enrollment status when removing allowlist entries

[MICROBA-1038]
- Today, we check if a learner is actively enrolled in a course-run before we add or remove them from the Instructor Dashboard allow list. We ran into an issue where we couldn't remove an entry from the list because the learner is no longer actively enrolled in the course-run. Update instructor dashboard logic to only check enrollment status when _adding_ a learner to the allow list.

common/djangoapps/student/api.py

@@ -7,6 +7,7 @@ Python APIs exposed by the student app to other in-process apps.
 from django.contrib.auth import get_user_model
 from django.conf import settings
 
+from common.djangoapps.student.models import CourseEnrollment
 from common.djangoapps.student.models_api import create_manual_enrollment_audit as _create_manual_enrollment_audit
 from common.djangoapps.student.models_api import get_course_access_role
 from common.djangoapps.student.models_api import get_course_enrollment as _get_course_enrollment
@@ -101,3 +102,10 @@ def get_access_role_by_role_name(role_name):
         role_name: the name of the role
     """
     return _REGISTERED_ACCESS_ROLES.get(role_name, None)
+
+
+def is_user_enrolled_in_course(student, course_key):
+    """
+    Determines if a learner is enrolled in a given course-run.
+    """
+    return CourseEnrollment.is_enrolled(student, course_key)

common/djangoapps/student/tests/test_api.py

+"""
+Test Student api.py
+"""
+
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
+
+from common.djangoapps.student.api import is_user_enrolled_in_course
+from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory
+
+
+class TestStudentApi(SharedModuleStoreTestCase):
+    """
+    Tests for functionality in the api.py file of the Student django app.
+    """
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.course = CourseFactory.create()
+
+    def setUp(self):
+        super().setUp()
+        self.user = UserFactory.create()
+        self.course_run_key = self.course.id
+
+    def test_is_user_enrolled_in_course(self):
+        """
+        Verify the correct value is returned when a learner is actively enrolled in a course-run.
+        """
+        CourseEnrollmentFactory.create(
+            user_id=self.user.id,
+            course_id=self.course.id
+        )
+
+        result = is_user_enrolled_in_course(self.user, self.course_run_key)
+        assert result
+
+    def test_is_user_enrolled_in_course_not_active(self):
+        """
+        Verify the correct value is returned when a learner is not actively enrolled in a course-run.
+        """
+        CourseEnrollmentFactory.create(
+            user_id=self.user.id,
+            course_id=self.course.id,
+            is_active=False
+        )
+
+        result = is_user_enrolled_in_course(self.user, self.course_run_key)
+        assert not result
+
+    def test_is_user_enrolled_in_course_no_enrollment(self):
+        """
+        Verify the correct value is returned when a learner is not enrolled in a course-run.
+        """
+        result = is_user_enrolled_in_course(self.user, self.course_run_key)
+        assert not result

lms/djangoapps/instructor/tests/test_api.py

@@ -4383,7 +4383,7 @@ class TestInstructorCertificateExceptions(SharedModuleStoreTestCase):
         """
         Test ability to retrieve a learner record using their username and course id
         """
-        student = _get_student_from_request_data({"user": self.user.username}, self.course.id)
+        student = _get_student_from_request_data({"user": self.user.username})
 
         assert student.username == self.user.username
 
@@ -4392,7 +4392,7 @@ class TestInstructorCertificateExceptions(SharedModuleStoreTestCase):
         Test that we receive an expected error when no learner's username or email is entered
         """
         with pytest.raises(ValueError) as error:
-            _get_student_from_request_data({"user": ""}, self.course.id)
+            _get_student_from_request_data({"user": ""})
 
         assert str(error.value) == (
             'Student username/email field is required and can not be empty. Kindly fill in username/email and then '
@@ -4405,24 +4405,10 @@ class TestInstructorCertificateExceptions(SharedModuleStoreTestCase):
         in the LMS.
         """
         with pytest.raises(ValueError) as error:
-            _get_student_from_request_data({"user": "Neo"}, self.course.id)
+            _get_student_from_request_data({"user": "Neo"})
 
         assert str(error.value) == "Neo does not exist in the LMS. Please check your spelling and retry."
 
-    def test_get_student_from_request_data_user_not_enrolled(self):
-        """
-        Test to verify an expected error message is returned when attempting to retrieve a learner that is not enrolled
-        in a course-run.
-        """
-        new_course = CourseFactory.create()
-
-        with pytest.raises(ValueError) as error:
-            _get_student_from_request_data({"user": self.user.username}, new_course.id)
-
-        assert str(error.value) == (
-            f"{self.user.username} is not enrolled in this course. Please check your spelling and retry."
-        )
-
     def test_get_certificate_for_user(self):
         """
         Test that attempts to retrieve a Certificate for a learner in a course-run.

lms/djangoapps/instructor/tests/test_certificates.py

@@ -667,8 +667,9 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase):
         assert not res_json['success']
 
         # Assert Error Message
-        assert res_json['message'] == '{user} is not enrolled in this course. Please check your spelling and retry.'\
-            .format(user=self.certificate_exception['user_name'])
+        assert res_json['message'] == (
+            f"Student {self.user.username} is not enrolled in this course. Please check your spelling and retry."
+        )
 
     def test_certificate_exception_removed_successfully(self):
         """
@@ -1197,26 +1198,6 @@ class CertificateInvalidationViewTests(SharedModuleStoreTestCase):
         # Assert Error Message
         assert res_json['message'] == f'{invalid_user} does not exist in the LMS. Please check your spelling and retry.'
 
-    def test_user_not_enrolled_error(self):
-        """
-        Test error message if user is not enrolled in the course.
-        """
-        self.certificate_invalidation_data.update({"user": self.not_enrolled_student.username})
-
-        response = self.client.post(
-            self.url,
-            data=json.dumps(self.certificate_invalidation_data),
-            content_type='application/json',
-        )
-
-        # Assert 400 status code in response
-        assert response.status_code == 400
-        res_json = json.loads(response.content.decode('utf-8'))
-
-        # Assert Error Message
-        assert res_json['message'] == '{user} is not enrolled in this course. Please check your spelling and retry.'\
-            .format(user=self.not_enrolled_student.username)
-
     def test_no_generated_certificate_error(self):
         """
         Test error message if there is no generated certificate for the student.

lms/djangoapps/instructor/views/api.py

@@ -41,6 +41,7 @@ from submissions import api as sub_api  # installed from the edx-submissions rep
 
 from common.djangoapps.course_modes.models import CourseMode
 from common.djangoapps.student import auth
+from common.djangoapps.student.api import is_user_enrolled_in_course
 from common.djangoapps.student.models import (
     ALLOWEDTOENROLL_TO_ENROLLED,
     ALLOWEDTOENROLL_TO_UNENROLLED,
@@ -404,7 +405,7 @@ def register_and_enroll_students(request, course_id):  # pylint: disable=too-man
                         )
 
                     # enroll a user if it is not already enrolled.
-                    if not CourseEnrollment.is_enrolled(user, course_id):
+                    if not is_user_enrolled_in_course(user, course_id):
                         # Enroll user to the course and add manual enrollment audit trail
                         create_manual_course_enrollment(
                             user=user,
@@ -820,7 +821,7 @@ def bulk_beta_modify_access(request, course_id):
             # See if we should autoenroll the student
             if auto_enroll:
                 # Check if student is already enrolled
-                if not CourseEnrollment.is_enrolled(user, course_id):
+                if not is_user_enrolled_in_course(user, course_id):
                     CourseEnrollment.enroll(user, course_id)
 
         finally:
@@ -2609,7 +2610,7 @@ def certificate_exception_view(request, course_id):
     course_key = CourseKey.from_string(course_id)
     # Validate request data and return error response in case of invalid data
     try:
-        certificate_exception, student = parse_request_data_and_get_user(request, course_key)
+        certificate_exception, student = parse_request_data_and_get_user(request)
     except ValueError as error:
         return JsonResponse({'success': False, 'message': str(error)}, status=400)
 
@@ -2643,6 +2644,14 @@ def add_certificate_exception(course_key, student, certificate_exception):
     :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 actively enrolled in the course-run
+    if not is_user_enrolled_in_course(student, course_key):
+        raise ValueError(
+            _("Student {user} is not enrolled in this course. Please check your spelling and retry.")
+            .format(user=student.username)
+        )
+
     # Check if the learner is blocked from receiving certificates in this course run.
     if certs_api.is_certificate_invalidated(student, course_key):
         raise ValueError(
@@ -2706,7 +2715,7 @@ def remove_certificate_exception(course_key, student):
     allowlist_entry.delete()
 
 
-def parse_request_data_and_get_user(request, course_key):
+def parse_request_data_and_get_user(request):
     """
         Parse request data into Certificate Exception and User object.
         Certificate Exception is the dict object containing information about certificate exception.
@@ -2721,7 +2730,7 @@ def parse_request_data_and_get_user(request, course_key):
     if not user:
         raise ValueError(_('Student username/email field is required and can not be empty. '
                            'Kindly fill in username/email and then press "Add to Exception List" button.'))
-    db_user = get_student(user, course_key)
+    db_user = get_student(user)
 
     return certificate_exception, db_user
 
@@ -2741,7 +2750,7 @@ def parse_request_data(request):
     return data
 
 
-def get_student(username_or_email, course_key):
+def get_student(username_or_email):
     """
     Retrieve and return User object from db, raise ValueError
     if user is does not exists or is not enrolled in the given course.
@@ -2757,10 +2766,6 @@ def get_student(username_or_email, course_key):
             user=username_or_email
         ))
 
-    # Make Sure the given student is enrolled in the course
-    if not CourseEnrollment.is_enrolled(student, course_key):
-        raise ValueError(_("{user} is not enrolled in this course. Please check your spelling and retry.")
-                         .format(user=username_or_email))
     return student
 
 
@@ -2886,7 +2891,7 @@ def generate_bulk_certificate_exceptions(request, course_id):
                     build_row_errors('user_already_white_listed', user, row_num)
                     log.warning(f'Student {user.id} already on exception list in Course {course_key}.')
                 # make sure user is enrolled in course
-                elif not CourseEnrollment.is_enrolled(user, course_key):
+                elif not is_user_enrolled_in_course(user, course_key):
                     build_row_errors('user_not_enrolled', user, row_num)
                     log.warning(f'Student {user.id} is not enrolled in Course {course_key}')
                 else:
@@ -2925,7 +2930,7 @@ def certificate_invalidation_view(request, course_id):
     # Validate request data and return error response in case of invalid data
     try:
         certificate_invalidation_data = parse_request_data(request)
-        student = _get_student_from_request_data(certificate_invalidation_data, course_key)
+        student = _get_student_from_request_data(certificate_invalidation_data)
         certificate = _get_certificate_for_user(course_key, student)
     except ValueError as error:
         return JsonResponse({'message': str(error)}, status=400)
@@ -3045,7 +3050,7 @@ def _create_error_response(request, msg):
     return JsonResponse({"error": msg}, 400)
 
 
-def _get_student_from_request_data(request_data, course_key):
+def _get_student_from_request_data(request_data):
     """
     Attempts to retrieve the student information from the incoming request data.
 
@@ -3059,7 +3064,7 @@ def _get_student_from_request_data(request_data, course_key):
               'Kindly fill in username/email and then press "Invalidate Certificate" button.')
         )
 
-    return get_student(user, course_key)
+    return get_student(user)
 
 
 def _get_certificate_for_user(course_key, student):