From 53de4065377272f5a8cb5482aee8846b9d36d42e Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Tue, 17 Dec 2024 16:35:22 +0500 Subject: [PATCH 1/2] feat!: upgrade bulk_beta_modify_access to drf ( 30 ) (#35604) * feat!: upgrading api to DRF. --- lms/djangoapps/instructor/tests/test_api.py | 9 ++ lms/djangoapps/instructor/views/api.py | 151 +++++++++--------- lms/djangoapps/instructor/views/api_urls.py | 2 +- lms/djangoapps/instructor/views/serializer.py | 71 ++++++++ 4 files changed, 158 insertions(+), 75 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 4d3e413f5a2a..f1e7322abc6b 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1990,6 +1990,15 @@ 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, " + 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']) + @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 631500fe3246..43c9c1947c71 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -109,6 +109,7 @@ CertificateSerializer, CertificateStatusesSerializer, ListInstructorTaskInputSerializer, + ModifyAccessSerializer, RoleNameSerializer, SendEmailSerializer, ShowStudentExtensionSerializer, @@ -914,88 +915,91 @@ 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): + """ + 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) + if not serializer.is_valid(): + return JsonResponse({'message': serializer.errors}, status=400) - for identifier in identifiers: - try: - error = False - user_does_not_exist = False - user = get_student_from_identifier(identifier) - user_active = user.is_active + action = serializer.validated_data['action'] + identifiers = serializer.validated_data['identifiers'] + email_students = serializer.validated_data['email_students'] + auto_enroll = serializer.validated_data['auto_enroll'] - if action == 'add': - allow_access(course, user, rolename) - elif action == 'remove': - revoke_access(course, user, rolename) + 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') @@ -1025,7 +1029,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/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index ea27034d0942..6f67a1a6863c 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_issued_certificates/', api.GetIssuedCertificates.as_view(), name='get_issued_certificates'), re_path(r'^get_students_features(?P/csv)?$', api.GetStudentsFeatures.as_view(), name='get_students_features'), diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index f7fc685f658c..7b20eed32f60 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -1,4 +1,5 @@ """ Instructor apis serializers. """ +import re from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import ValidationError @@ -232,6 +233,76 @@ def __init__(self, *args, **kwargs): self.fields['due_datetime'].required = False +class ModifyAccessSerializer(serializers.Serializer): + """ + serializers for enroll or un-enroll users in beta testing program. + """ + identifiers = serializers.CharField( + help_text="A comma separated list of emails or usernames.", + required=True + ) + action = serializers.ChoiceField( + choices=["add", "remove"], + help_text="Action to perform: add or remove.", + required=True + ) + + 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 = _split_input_list(value) + if not validated_list: + raise serializers.ValidationError("The identifiers list cannot be empty.") + + return validated_list + + def validate_email_students(self, value): + """ + handle string values like 'true' or 'false'. + """ + if isinstance(value, str): + return value.lower() == 'true' + return bool(value) + + def validate_auto_enroll(self, value): + """ + handle string values like 'true' or 'false'. + """ + 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 + """ + 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 + + class CertificateStatusesSerializer(serializers.Serializer): """ Serializer for validating and serializing certificate status inputs. From 5bf0b2704f782904be0b1fcfa7355ae486e2ea46 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama <64033729+BryanttV@users.noreply.github.com> Date: Tue, 17 Dec 2024 09:52:56 -0500 Subject: [PATCH 2/2] feat: add schedule queryset request filter integration (#35982) * feat: add schedule queryset request filter integration * chore: remove try-except when running filter * chore: upgrade openedx-filters to v1.12.0 * test: add filter unit tests * test: inherit of ModuleStoreTestCase * fix: add missing attribute in resolver instance --- .../core/djangoapps/schedules/resolvers.py | 5 ++ .../schedules/tests/test_filters.py | 71 +++++++++++++++++++ requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 6 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 openedx/core/djangoapps/schedules/tests/test_filters.py diff --git a/openedx/core/djangoapps/schedules/resolvers.py b/openedx/core/djangoapps/schedules/resolvers.py index 5f769e6af299..a07f9c5de7f5 100644 --- a/openedx/core/djangoapps/schedules/resolvers.py +++ b/openedx/core/djangoapps/schedules/resolvers.py @@ -14,6 +14,7 @@ from edx_ace.recipient import Recipient from edx_ace.recipient_resolver import RecipientResolver from edx_django_utils.monitoring import function_trace, set_custom_attribute +from openedx_filters.learning.filters import ScheduleQuerySetRequested from lms.djangoapps.courseware.utils import verified_upgrade_deadline_link, can_show_verified_upgrade from lms.djangoapps.discussion.notification_prefs.views import UsernameCipher @@ -154,6 +155,10 @@ def get_schedules_with_target_date_by_bin_and_orgs( schedules = self.filter_by_org(schedules) + # .. filter_implemented_name: ScheduleQuerySetRequested + # .. filter_type: org.openedx.learning.schedule.queryset.requested.v1 + schedules = ScheduleQuerySetRequested.run_filter(schedules) + if "read_replica" in settings.DATABASES: schedules = schedules.using("read_replica") diff --git a/openedx/core/djangoapps/schedules/tests/test_filters.py b/openedx/core/djangoapps/schedules/tests/test_filters.py new file mode 100644 index 000000000000..9f7efa050447 --- /dev/null +++ b/openedx/core/djangoapps/schedules/tests/test_filters.py @@ -0,0 +1,71 @@ +""" +Test cases for the Open edX Filters associated with the schedule app. +""" + +import datetime +from unittest.mock import Mock + +from django.db.models.query import QuerySet +from django.test import override_settings +from openedx_filters import PipelineStep + +from openedx.core.djangoapps.schedules.resolvers import BinnedSchedulesBaseResolver +from openedx.core.djangoapps.schedules.tests.test_resolvers import SchedulesResolverTestMixin +from openedx.core.djangolib.testing.utils import skip_unless_lms +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + + +class TestScheduleQuerySetRequestedPipelineStep(PipelineStep): + """Pipeline step class to test a configured pipeline step""" + + filtered_schedules = Mock(spec=QuerySet, __len__=Mock(return_value=0)) + + def run_filter(self, schedules: QuerySet): # pylint: disable=arguments-differ + """Pipeline step to filter the schedules""" + return { + "schedules": self.filtered_schedules, + } + + +@skip_unless_lms +class ScheduleQuerySetRequestedFiltersTest(SchedulesResolverTestMixin, ModuleStoreTestCase): + """ + Tests for the Open edX Filters associated with the schedule queryset requested. + + The following filters are tested: + - ScheduleQuerySetRequested + """ + + def setUp(self): + super().setUp() + self.resolver = BinnedSchedulesBaseResolver( + async_send_task=Mock(name="async_send_task"), + site=self.site, + target_datetime=datetime.datetime.now(), + day_offset=3, + bin_num=2, + ) + self.resolver.schedule_date_field = "created" + + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.schedule.queryset.requested.v1": { + "pipeline": [ + "openedx.core.djangoapps.schedules.tests.test_filters.TestScheduleQuerySetRequestedPipelineStep", + ], + "fail_silently": False, + }, + }, + ) + def test_schedule_with_queryset_requested_filter_enabled(self) -> None: + """Test to verify the schedule queryset was modified by the pipeline step.""" + schedules = self.resolver.get_schedules_with_target_date_by_bin_and_orgs() + + self.assertEqual(TestScheduleQuerySetRequestedPipelineStep.filtered_schedules, schedules) + + @override_settings(OPEN_EDX_FILTERS_CONFIG={}) + def test_schedule_with_queryset_requested_filter_disabled(self) -> None: + """Test to verify the schedule queryset was not modified when the pipeline step is not configured.""" + schedules = self.resolver.get_schedules_with_target_date_by_bin_and_orgs() + + self.assertNotEqual(TestScheduleQuerySetRequestedPipelineStep.filtered_schedules, schedules) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index f95ae8adb2c9..ccedfe5a0227 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -836,7 +836,7 @@ openedx-events==9.15.0 # edx-name-affirmation # event-tracking # ora2 -openedx-filters==1.11.0 +openedx-filters==1.12.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 75c533947407..0191c779d363 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1390,7 +1390,7 @@ openedx-events==9.15.0 # edx-name-affirmation # event-tracking # ora2 -openedx-filters==1.11.0 +openedx-filters==1.12.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index f7031d349784..dacd1efa6143 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1005,7 +1005,7 @@ openedx-events==9.15.0 # edx-name-affirmation # event-tracking # ora2 -openedx-filters==1.11.0 +openedx-filters==1.12.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 57a0dc6341ad..db1ed7889667 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1050,7 +1050,7 @@ openedx-events==9.15.0 # edx-name-affirmation # event-tracking # ora2 -openedx-filters==1.11.0 +openedx-filters==1.12.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock