From 011e5b4c0a78273fbcf092afe0d7515b49a72dfd Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 7 Oct 2024 17:47:30 +0500 Subject: [PATCH 01/11] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 162 ++++++++++-------- lms/djangoapps/instructor/views/api_urls.py | 2 +- lms/djangoapps/instructor/views/serializer.py | 54 ++++++ 3 files changed, 143 insertions(+), 75 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 94184f6d9373..b30187f3743b 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -108,7 +108,9 @@ from lms.djangoapps.instructor_task.models import ReportStore from lms.djangoapps.instructor.views.serializer import ( AccessSerializer, BlockDueDateSerializer, RoleNameSerializer, ShowStudentExtensionSerializer, UserSerializer, - SendEmailSerializer, StudentAttemptsSerializer, ListInstructorTaskInputSerializer, UniqueStudentIdentifierSerializer + SendEmailSerializer, StudentAttemptsSerializer, ListInstructorTaskInputSerializer, + UniqueStudentIdentifierSerializer, + ModifyAccessSerializer ) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted @@ -908,88 +910,100 @@ def students_update_enrollment(request, course_id): # lint-amnesty, pylint: dis return JsonResponse(response_payload) -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.CAN_BETATEST) -@common_exceptions_400 -@require_post_params( - identifiers="stringified list of emails and/or usernames", - action="add or remove", -) -def bulk_beta_modify_access(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class BulkBetaModifyAccess(DeveloperErrorViewMixin, APIView): """ Enroll or unenroll users in beta testing program. - - Query parameters: - - identifiers is string containing a list of emails and/or usernames separated by - anything split_input_list can handle. - - action is one of ['add', 'remove'] """ - course_id = CourseKey.from_string(course_id) - action = request.POST.get('action') - identifiers_raw = request.POST.get('identifiers') - identifiers = _split_input_list(identifiers_raw) - email_students = _get_boolean_param(request, 'email_students') - auto_enroll = _get_boolean_param(request, 'auto_enroll') - results = [] - rolename = 'beta' - course = get_course_by_id(course_id) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.CAN_BETATEST + serializer_class = ModifyAccessSerializer - email_params = {} - if email_students: - secure = request.is_secure() - email_params = get_email_params(course, auto_enroll=auto_enroll, secure=secure) + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """ + Enroll or unenroll users in beta testing program. - for identifier in identifiers: - try: - error = False - user_does_not_exist = False - user = get_student_from_identifier(identifier) - user_active = user.is_active + Query parameters: + - identifiers is string containing a list of emails and/or usernames separated by + anything split_input_list can handle. + - action is one of ['add', 'remove'] + """ + course_id = CourseKey.from_string(course_id) + + serializer = self.serializer_class(data=request.data) + import pdb; + pdb.set_trace() + + if not serializer.is_valid(): + return JsonResponse( + {'message': _('Data is not valid.')}, + status=400 + ) - if action == 'add': - allow_access(course, user, rolename) - elif action == 'remove': - revoke_access(course, user, rolename) + action = serializer.validated_data['action'] + identifiers = serializer.validated_data['identifiers'] + email_students = serializer.validated_data['email_students'] + auto_enroll = serializer.validated_data['auto_enroll'] + + results = [] + rolename = 'beta' + course = get_course_by_id(course_id) + + email_params = {} + if email_students: + secure = request.is_secure() + email_params = get_email_params(course, auto_enroll=auto_enroll, secure=secure) + + for identifier in identifiers: + try: + error = False + user_does_not_exist = False + user = get_student_from_identifier(identifier) + user_active = user.is_active + + if action == 'add': + allow_access(course, user, rolename) + elif action == 'remove': + revoke_access(course, user, rolename) + else: + return HttpResponseBadRequest(strip_tags( + f"Unrecognized action '{action}'" + )) + except User.DoesNotExist: + error = True + user_does_not_exist = True + user_active = None + # catch and log any unexpected exceptions + # so that one error doesn't cause a 500. + except Exception as exc: # pylint: disable=broad-except + log.exception("Error while #{}ing student") + log.exception(exc) + error = True else: - return HttpResponseBadRequest(strip_tags( - f"Unrecognized action '{action}'" - )) - except User.DoesNotExist: - error = True - user_does_not_exist = True - user_active = None - # catch and log any unexpected exceptions - # so that one error doesn't cause a 500. - except Exception as exc: # pylint: disable=broad-except - log.exception("Error while #{}ing student") - log.exception(exc) - error = True - else: - # If no exception thrown, see if we should send an email - if email_students: - send_beta_role_email(action, user, email_params) - # See if we should autoenroll the student - if auto_enroll: - # Check if student is already enrolled - if not is_user_enrolled_in_course(user, course_id): - CourseEnrollment.enroll(user, course_id) + # If no exception thrown, see if we should send an email + if email_students: + send_beta_role_email(action, user, email_params) + # See if we should autoenroll the student + if auto_enroll: + # Check if student is already enrolled + if not is_user_enrolled_in_course(user, course_id): + CourseEnrollment.enroll(user, course_id) - finally: - # Tabulate the action result of this email address - results.append({ - 'identifier': identifier, - 'error': error, # pylint: disable=used-before-assignment - 'userDoesNotExist': user_does_not_exist, # pylint: disable=used-before-assignment - 'is_active': user_active # pylint: disable=used-before-assignment - }) + finally: + # Tabulate the action result of this email address + results.append({ + 'identifier': identifier, + 'error': error, # pylint: disable=used-before-assignment + 'userDoesNotExist': user_does_not_exist, # pylint: disable=used-before-assignment + 'is_active': user_active # pylint: disable=used-before-assignment + }) - response_payload = { - 'action': action, - 'results': results, - } - return JsonResponse(response_payload) + response_payload = { + 'action': action, + 'results': results, + } + return JsonResponse(response_payload) @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 5976411a9756..1e8a757c033e 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -25,7 +25,7 @@ path('register_and_enroll_students', api.RegisterAndEnrollStudents.as_view(), name='register_and_enroll_students'), path('list_course_role_members', api.ListCourseRoleMembersView.as_view(), name='list_course_role_members'), path('modify_access', api.ModifyAccess.as_view(), name='modify_access'), - path('bulk_beta_modify_access', api.bulk_beta_modify_access, name='bulk_beta_modify_access'), + path('bulk_beta_modify_access', api.BulkBetaModifyAccess.as_view(), name='bulk_beta_modify_access'), path('get_problem_responses', api.get_problem_responses, name='get_problem_responses'), path('get_grading_config', api.GetGradingConfig.as_view(), name='get_grading_config'), re_path(r'^get_students_features(?P/csv)?$', api.get_students_features, name='get_students_features'), diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 59ac66ab838b..5b4a4c3b4380 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -228,3 +228,57 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if disable_due_datetime: self.fields['due_datetime'].required = False + + +class ModifyAccessSerializer(serializers.Serializer): + identifiers = serializers.ListField( + child=serializers.CharField(), + allow_empty=False, + help_text="A list of stringified emails or usernames." + ) + action = serializers.ChoiceField( + choices=["add", "remove"], + help_text="Action to perform: add or remove." + ) + + email_students = serializers.BooleanField( + default=False, + help_text="Boolean flag to indicate if students should be emailed." + ) + + auto_enroll = serializers.BooleanField( + default=False, + help_text="Boolean flag to indicate if the user should be auto-enrolled." + ) + + def validate_identifiers(self, value): + """ + Validate the 'identifiers' field which is now a list of strings. + """ + # Iterate over the list of identifiers and validate each one + validated_list = [] + for identifier in value: + identifier = identifier.strip() # Strip leading/trailing whitespace + if identifier: # Skip empty identifiers + validated_list.append(identifier) + + if not validated_list: + raise serializers.ValidationError("The identifiers list cannot be empty.") + + return validated_list + + def validate_email_students(self, value): + """ + Override this method to handle string values like 'true' or 'false'. + """ + if isinstance(value, str): + return value.lower() == 'true' + return bool(value) + + def validate_auto_enroll(self, value): + """ + Validate the 'auto_enroll' field to handle string values like 'true' or 'false'. + """ + if isinstance(value, str): + return value.lower() == 'true' + return bool(value) From 5ce2f34ba743dde420110b73f6a8f0ac01769151 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 7 Oct 2024 18:03:18 +0500 Subject: [PATCH 02/11] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index b30187f3743b..6b42d1dac1ca 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -922,8 +922,6 @@ class BulkBetaModifyAccess(DeveloperErrorViewMixin, APIView): @method_decorator(ensure_csrf_cookie) def post(self, request, course_id): """ - Enroll or unenroll users in beta testing program. - Query parameters: - identifiers is string containing a list of emails and/or usernames separated by anything split_input_list can handle. From b806d52c47bb8710bdbccf47779931736115cf6f Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 7 Oct 2024 20:30:26 +0500 Subject: [PATCH 03/11] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 9 +-------- lms/djangoapps/instructor/views/serializer.py | 6 ++++-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 6b42d1dac1ca..6d4f14a2cef0 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -928,16 +928,9 @@ def post(self, request, course_id): - action is one of ['add', 'remove'] """ course_id = CourseKey.from_string(course_id) - serializer = self.serializer_class(data=request.data) - import pdb; - pdb.set_trace() - if not serializer.is_valid(): - return JsonResponse( - {'message': _('Data is not valid.')}, - status=400 - ) + return JsonResponse({'message': serializer.errors}, status=400) action = serializer.validated_data['action'] identifiers = serializer.validated_data['identifiers'] diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 5b4a4c3b4380..23fa7664c0a1 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -234,11 +234,13 @@ class ModifyAccessSerializer(serializers.Serializer): identifiers = serializers.ListField( child=serializers.CharField(), allow_empty=False, - help_text="A list of stringified emails or usernames." + help_text="A list of stringified emails or usernames.", + required=True ) action = serializers.ChoiceField( choices=["add", "remove"], - help_text="Action to perform: add or remove." + help_text="Action to perform: add or remove.", + required=True ) email_students = serializers.BooleanField( From 97476a49c7b6c3008d7c2d06f3d6ce399af53c59 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 7 Oct 2024 20:34:20 +0500 Subject: [PATCH 04/11] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/serializer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 23fa7664c0a1..de86a515933a 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -271,7 +271,7 @@ def validate_identifiers(self, value): def validate_email_students(self, value): """ - Override this method to handle string values like 'true' or 'false'. + handle string values like 'true' or 'false'. """ if isinstance(value, str): return value.lower() == 'true' @@ -279,7 +279,7 @@ def validate_email_students(self, value): def validate_auto_enroll(self, value): """ - Validate the 'auto_enroll' field to handle string values like 'true' or 'false'. + handle string values like 'true' or 'false'. """ if isinstance(value, str): return value.lower() == 'true' From f1399c5e0721d50ae3ac47361c7f73fc479b22e3 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 7 Oct 2024 20:41:05 +0500 Subject: [PATCH 05/11] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/serializer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index de86a515933a..b0b3c78993c3 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -234,7 +234,7 @@ class ModifyAccessSerializer(serializers.Serializer): identifiers = serializers.ListField( child=serializers.CharField(), allow_empty=False, - help_text="A list of stringified emails or usernames.", + help_text="A comma separated list of emails or usernames.", required=True ) action = serializers.ChoiceField( From ed3dd8509ea148b67745a471086695d63c29df3f Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 7 Oct 2024 21:35:52 +0500 Subject: [PATCH 06/11] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/serializer.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index b0b3c78993c3..f46ac78b9bc7 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -231,6 +231,9 @@ def __init__(self, *args, **kwargs): class ModifyAccessSerializer(serializers.Serializer): + """ + serializers for enroll or un-enroll users in beta testing program. + """ identifiers = serializers.ListField( child=serializers.CharField(), allow_empty=False, From 6719f150b772e02a37dd8f1aeb2c78e5af4ffd4d Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 16 Dec 2024 15:07:32 +0500 Subject: [PATCH 07/11] chore: Separate out individual student email from the comma, or space separated string --- lms/djangoapps/instructor/tests/test_api.py | 6 ++++ lms/djangoapps/instructor/views/api.py | 1 - lms/djangoapps/instructor/views/serializer.py | 29 +++++++++++++------ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 4d3e413f5a2a..a0f844486000 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1990,6 +1990,12 @@ def test_add_notenrolled_username_autoenroll(self): self.add_notenrolled(response, self.notenrolled_student.username) assert CourseEnrollment.is_enrolled(self.notenrolled_student, self.course.id) + def test_add_notenrolled_username_autoenroll_with_multiple_users(self): + url = reverse('bulk_beta_modify_access', kwargs={'course_id': str(self.course.id)}) + identifiers = f"Lorem@ipsum.dolor, sit@amet.consectetur\nadipiscing@elit.Aenean\r convallis@at.lacus\r, ut@lacinia.Sed, {self.notenrolled_student.username}" + response = self.client.post(url, {'identifiers': identifiers, 'action': 'add', 'email_students': False, 'auto_enroll': True}) # lint-amnesty, pylint: disable=line-too-long + assert 6, len(json.loads(response.content.decode())['results']) + @ddt.data('http', 'https') def test_add_notenrolled_with_email(self, protocol): url = reverse('bulk_beta_modify_access', kwargs={'course_id': str(self.course.id)}) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 6d4f14a2cef0..5bb5a0e2614a 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1024,7 +1024,6 @@ def post(self, request, course_id): course = get_course_with_access( request.user, 'instructor', course_id, depth=None ) - serializer_data = AccessSerializer(data=request.data) if not serializer_data.is_valid(): return HttpResponseBadRequest(reason=serializer_data.errors) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index f46ac78b9bc7..c2a7c2522933 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -234,9 +234,7 @@ class ModifyAccessSerializer(serializers.Serializer): """ serializers for enroll or un-enroll users in beta testing program. """ - identifiers = serializers.ListField( - child=serializers.CharField(), - allow_empty=False, + identifiers = serializers.CharField( help_text="A comma separated list of emails or usernames.", required=True ) @@ -261,12 +259,7 @@ def validate_identifiers(self, value): Validate the 'identifiers' field which is now a list of strings. """ # Iterate over the list of identifiers and validate each one - validated_list = [] - for identifier in value: - identifier = identifier.strip() # Strip leading/trailing whitespace - if identifier: # Skip empty identifiers - validated_list.append(identifier) - + validated_list = _split_input_list(value) if not validated_list: raise serializers.ValidationError("The identifiers list cannot be empty.") @@ -287,3 +280,21 @@ def validate_auto_enroll(self, value): if isinstance(value, str): return value.lower() == 'true' return bool(value) + +def _split_input_list(str_list): + """ + Separate out individual student email from the comma, or space separated string. + + e.g. + in: "Lorem@ipsum.dolor, sit@amet.consectetur\nadipiscing@elit.Aenean\r convallis@at.lacus\r, ut@lacinia.Sed" + out: ['Lorem@ipsum.dolor', 'sit@amet.consectetur', 'adipiscing@elit.Aenean', 'convallis@at.lacus', 'ut@lacinia.Sed'] + + `str_list` is a string coming from an input text area + returns a list of separated values + """ + import re + new_list = re.split(r'[,\s\n\r]+', str_list) + new_list = [s.strip() for s in new_list] + new_list = [s for s in new_list if s != ''] + + return new_list From a2baeed644165e0361c44ccd06209fdf2bf89287 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Mon, 16 Dec 2024 15:20:35 +0500 Subject: [PATCH 08/11] chore: Update api.py --- lms/djangoapps/instructor/views/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 04774728c3ba..43c9c1947c71 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -109,7 +109,7 @@ CertificateSerializer, CertificateStatusesSerializer, ListInstructorTaskInputSerializer, - ModifyAccessSerializer + ModifyAccessSerializer, RoleNameSerializer, SendEmailSerializer, ShowStudentExtensionSerializer, From 9d937ba7f934ccb4c6b7ecd8add7a2c2892084f6 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Mon, 16 Dec 2024 16:13:23 +0500 Subject: [PATCH 09/11] chore: Update serializer.py --- lms/djangoapps/instructor/views/serializer.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index a6453d80f6cb..5a662d7decfc 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -284,6 +284,7 @@ def validate_auto_enroll(self, value): return value.lower() == 'true' return bool(value) + def _split_input_list(str_list): """ Separate out individual student email from the comma, or space separated string. @@ -301,6 +302,7 @@ def _split_input_list(str_list): return new_list + class CertificateStatusesSerializer(serializers.Serializer): """ Serializer for validating and serializing certificate status inputs. From 061569934219a2e880f6c645e90c06de9f959493 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Mon, 16 Dec 2024 16:16:46 +0500 Subject: [PATCH 10/11] chore: Update serializer.py --- lms/djangoapps/instructor/views/serializer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 5a662d7decfc..7b20eed32f60 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -303,7 +303,7 @@ def _split_input_list(str_list): return new_list - class CertificateStatusesSerializer(serializers.Serializer): +class CertificateStatusesSerializer(serializers.Serializer): """ Serializer for validating and serializing certificate status inputs. From af6edc88625f2db65e6b33fa394e06edb00ad391 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 16 Dec 2024 17:16:17 +0500 Subject: [PATCH 11/11] chore: minor fix. --- lms/djangoapps/instructor/tests/test_api.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index a0f844486000..f1e7322abc6b 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1992,7 +1992,10 @@ def test_add_notenrolled_username_autoenroll(self): def test_add_notenrolled_username_autoenroll_with_multiple_users(self): url = reverse('bulk_beta_modify_access', kwargs={'course_id': str(self.course.id)}) - identifiers = f"Lorem@ipsum.dolor, sit@amet.consectetur\nadipiscing@elit.Aenean\r convallis@at.lacus\r, ut@lacinia.Sed, {self.notenrolled_student.username}" + identifiers = (f"Lorem@ipsum.dolor, " + f"sit@amet.consectetur\nadipiscing@elit.Aenean\r convallis@at.lacus\r, ut@lacinia.Sed, " + f"{self.notenrolled_student.username}" + ) response = self.client.post(url, {'identifiers': identifiers, 'action': 'add', 'email_students': False, 'auto_enroll': True}) # lint-amnesty, pylint: disable=line-too-long assert 6, len(json.loads(response.content.decode())['results'])