Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Merge pull request #26857 from edx/jhynes/microba-1038_fix-removal-bug

Browse Source 
MICROBA-1038 | Don't check enrollment status when removing allowlist entries
This commit is contained in:
Justin Hynes
2021-03-05 08:02:03 -05:00
committed by GitHub
parent 875a06d2fc a3a05bc440
commit 8155190511
5 changed files with 90 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
common/djangoapps/student/api.py
View File

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

57
common/djangoapps/student/tests/test_api.py Normal file
View File

@@ -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

20
lms/djangoapps/instructor/tests/test_api.py
View File

@@ -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.

25
lms/djangoapps/instructor/tests/test_certificates.py
View File

@@ -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.

33
lms/djangoapps/instructor/views/api.py
View File

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