diff --git a/common/djangoapps/student/api.py b/common/djangoapps/student/api.py index 4ae91a43d1..dfcd35cdf6 100644 --- a/common/djangoapps/student/api.py +++ b/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) diff --git a/common/djangoapps/student/tests/test_api.py b/common/djangoapps/student/tests/test_api.py new file mode 100644 index 0000000000..0b313e24eb --- /dev/null +++ b/common/djangoapps/student/tests/test_api.py @@ -0,0 +1,57 @@ +""" +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 diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 6fde2b83a4..86ace48609 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/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. diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 78ee8d1360..64ff0d261d 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/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. diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 62c96ff05a..99bca9df64 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/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):