diff --git a/common/djangoapps/util/memcache.py b/common/djangoapps/util/memcache.py index 723ca87098d1..2f7e6dc623a0 100644 --- a/common/djangoapps/util/memcache.py +++ b/common/djangoapps/util/memcache.py @@ -9,7 +9,6 @@ from django.conf import settings from django.utils.encoding import smart_str -from edx_django_utils.monitoring.utils import increment def fasthash(string): @@ -49,18 +48,9 @@ def safe_key(key, key_prefix, version): # Attempt to combine the prefix, version, and key combined = ":".join([key_prefix, version, key]) - # Temporary: Add observability to large-key hashing to help us - # understand the safety of a cutover from md4 to blake2b hashing. - # See https://github.com/edx/edx-arch-experiments/issues/872 - increment('memcache.safe_key.called') - # If the total length is too long for memcache, hash it if len(combined) > 250: combined = fasthash(combined) - # Temporary: See - # https://github.com/edx/edx-arch-experiments/issues/872 and - # previous comment. - increment('memcache.safe_key.hash_large') # Return the result return combined 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. 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/constraints.txt b/requirements/constraints.txt index d7608873644e..541197d83242 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -78,7 +78,7 @@ django-storages<1.14.4 # The team that owns this package will manually bump this package rather than having it pulled in automatically. # This is to allow them to better control its deployment and to do it in a process that works better # for them. -edx-enterprise==5.4.2 +edx-enterprise==5.5.0 # Date: 2024-05-09 # This has to be constrained as well because newer versions of edx-i18n-tools need the diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index f95ae8adb2c9..dfcc8bcc5dd9 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -472,7 +472,7 @@ edx-drf-extensions==10.5.0 # edx-when # edxval # openedx-learning -edx-enterprise==5.4.2 +edx-enterprise==5.5.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in @@ -549,7 +549,7 @@ edx-when==2.5.0 # via # -r requirements/edx/kernel.in # edx-proctoring -edxval==2.6.1 +edxval==2.7.0 # via -r requirements/edx/kernel.in elasticsearch==7.9.1 # via @@ -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..15f08c1377c0 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -748,7 +748,7 @@ edx-drf-extensions==10.5.0 # edx-when # edxval # openedx-learning -edx-enterprise==5.4.2 +edx-enterprise==5.5.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt @@ -855,7 +855,7 @@ edx-when==2.5.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edx-proctoring -edxval==2.6.1 +edxval==2.7.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.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..d23eba58993d 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -555,7 +555,7 @@ edx-drf-extensions==10.5.0 # edx-when # edxval # openedx-learning -edx-enterprise==5.4.2 +edx-enterprise==5.5.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt @@ -635,7 +635,7 @@ edx-when==2.5.0 # via # -r requirements/edx/base.txt # edx-proctoring -edxval==2.6.1 +edxval==2.7.0 # via -r requirements/edx/base.txt elasticsearch==7.9.1 # via @@ -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..98178143005d 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -576,7 +576,7 @@ edx-drf-extensions==10.5.0 # edx-when # edxval # openedx-learning -edx-enterprise==5.4.2 +edx-enterprise==5.5.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt @@ -658,7 +658,7 @@ edx-when==2.5.0 # via # -r requirements/edx/base.txt # edx-proctoring -edxval==2.6.1 +edxval==2.7.0 # via -r requirements/edx/base.txt elasticsearch==7.9.1 # via @@ -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