From cf1064616cfe251e05fc54ef7d4f0bed7d9006d8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 31 Aug 2021 16:18:43 +0930 Subject: [PATCH] refactor: deprecate ModuleSystem user attributes in favor of user service The following ModuleSystem attributes are deprecated by this change, and should be pulled directly from the user service instead: * anonymous_student_id * seed * user_id * user_is_staff Related changes: * Removes the `user` and `anonymous_student_id` parameters from the ModuleService constructor. * Stores anonymous_user_id in XBlockDjangoUserService's opt_attr * Pulls out constants used by DjangoXBlockUserService opt_attr so they can be used in the platform code. * LmsModuleSystem uses the user service created in wrapper function for runtime.publish to avoid requiring the user service to be "needed" by all XBlocks. * LmsModuleSystem no longer checks for instances of XModuleDescriptor when deciding what kind of anonymous_user_id to provide: all XModules are XBlocks, so this check is unnecessary. * XBlockRuntime returns a user service when requested * Adds tests for deprecated ModuleSystem attributes and changes to XBlockDjangoUserService. (cherry picked from commit c41e7fb93a8204aba3015776a063054b01fd0546) --- cms/djangoapps/contentstore/views/preview.py | 4 +- common/djangoapps/xblock_django/constants.py | 11 +++ .../xblock_django/tests/test_user_service.py | 25 ++++- .../djangoapps/xblock_django/user_service.py | 24 ++++- common/lib/xmodule/xmodule/x_module.py | 95 ++++++++++++++++--- lms/djangoapps/courseware/module_render.py | 37 ++++---- .../courseware/tests/test_module_render.py | 92 ++++++++++++++++++ lms/djangoapps/lms_xblock/runtime.py | 3 +- .../core/djangoapps/xblock/runtime/runtime.py | 9 ++ 9 files changed, 253 insertions(+), 47 deletions(-) create mode 100644 common/djangoapps/xblock_django/constants.py diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index be7453ced5..1abafe5fb9 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -198,12 +198,10 @@ def _preview_module_system(request, descriptor, field_data): render_template=render_from_lms, debug=True, replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_id=course_id), - user=request.user, can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)), get_python_lib_zip=(lambda: get_python_lib_zip(contentstore, course_id)), mixins=settings.XBLOCK_MIXINS, course_id=course_id, - anonymous_student_id='student', # Set up functions to modify the fragment produced by student_view wrappers=wrappers, @@ -216,7 +214,7 @@ def _preview_module_system(request, descriptor, field_data): "field-data": field_data, "i18n": ModuleI18nService, "settings": SettingsService(), - "user": DjangoXBlockUserService(request.user), + "user": DjangoXBlockUserService(request.user, anonymous_user_id='student'), "partitions": StudioPartitionService(course_id=course_id), "teams_configuration": TeamsConfigurationService(), }, diff --git a/common/djangoapps/xblock_django/constants.py b/common/djangoapps/xblock_django/constants.py new file mode 100644 index 0000000000..3d44db7877 --- /dev/null +++ b/common/djangoapps/xblock_django/constants.py @@ -0,0 +1,11 @@ +""" +Constants used by DjangoXBlockUserService +""" + +# Optional attributes stored on the XBlockUser +ATTR_KEY_ANONYMOUS_USER_ID = 'edx-platform.anonymous_user_id' +ATTR_KEY_IS_AUTHENTICATED = 'edx-platform.is_authenticated' +ATTR_KEY_USER_ID = 'edx-platform.user_id' +ATTR_KEY_USERNAME = 'edx-platform.username' +ATTR_KEY_USER_IS_STAFF = 'edx-platform.user_is_staff' +ATTR_KEY_USER_PREFERENCES = 'edx-platform.user_preferences' diff --git a/common/djangoapps/xblock_django/tests/test_user_service.py b/common/djangoapps/xblock_django/tests/test_user_service.py index 6d090417c5..7954a09dc1 100644 --- a/common/djangoapps/xblock_django/tests/test_user_service.py +++ b/common/djangoapps/xblock_django/tests/test_user_service.py @@ -2,6 +2,7 @@ Tests for the DjangoXBlockUserService. """ +import ddt import pytest from django.test import TestCase from opaque_keys.edx.keys import CourseKey @@ -12,6 +13,7 @@ from common.djangoapps.student.models import anonymous_id_for_user from common.djangoapps.student.tests.factories import AnonymousUserFactory, UserFactory from common.djangoapps.xblock_django.user_service import ( ATTR_KEY_IS_AUTHENTICATED, + ATTR_KEY_ANONYMOUS_USER_ID, ATTR_KEY_USER_ID, ATTR_KEY_USER_IS_STAFF, ATTR_KEY_USER_PREFERENCES, @@ -21,6 +23,7 @@ from common.djangoapps.xblock_django.user_service import ( ) +@ddt.ddt class UserServiceTestCase(TestCase): """ Tests for the DjangoXBlockUserService. @@ -42,7 +45,7 @@ class UserServiceTestCase(TestCase): assert xb_user.full_name is None self.assertListEqual(xb_user.emails, []) - def assert_xblock_user_matches_django(self, xb_user, dj_user): + def assert_xblock_user_matches_django(self, xb_user, dj_user, user_is_staff=False, anonymous_user_id=None): """ A set of assertions for comparing a XBlockUser to a django User """ @@ -51,7 +54,8 @@ class UserServiceTestCase(TestCase): assert xb_user.full_name == dj_user.profile.name assert xb_user.opt_attrs[ATTR_KEY_USERNAME] == dj_user.username assert xb_user.opt_attrs[ATTR_KEY_USER_ID] == dj_user.id - assert not xb_user.opt_attrs[ATTR_KEY_USER_IS_STAFF] + assert xb_user.opt_attrs[ATTR_KEY_USER_IS_STAFF] == user_is_staff + assert xb_user.opt_attrs[ATTR_KEY_ANONYMOUS_USER_ID] == anonymous_user_id assert all((pref in USER_PREFERENCES_WHITE_LIST) for pref in xb_user.opt_attrs[ATTR_KEY_USER_PREFERENCES]) def test_convert_anon_user(self): @@ -63,14 +67,25 @@ class UserServiceTestCase(TestCase): assert xb_user.is_current_user self.assert_is_anon_xb_user(xb_user) - def test_convert_authenticate_user(self): + @ddt.data( + (False, None), + (True, None), + (False, 'abcdef0123'), + (True, 'abcdef0123'), + ) + @ddt.unpack + def test_convert_authenticate_user(self, user_is_staff, anonymous_user_id): """ Tests for convert_django_user_to_xblock_user behavior when django user is User. """ - django_user_service = DjangoXBlockUserService(self.user) + django_user_service = DjangoXBlockUserService( + self.user, + user_is_staff=user_is_staff, + anonymous_user_id=anonymous_user_id, + ) xb_user = django_user_service.get_current_user() assert xb_user.is_current_user - self.assert_xblock_user_matches_django(xb_user, self.user) + self.assert_xblock_user_matches_django(xb_user, self.user, user_is_staff, anonymous_user_id) def test_get_anonymous_user_id_returns_none_for_non_staff_users(self): """ diff --git a/common/djangoapps/xblock_django/user_service.py b/common/djangoapps/xblock_django/user_service.py index 23af5ca273..b11cb02d77 100644 --- a/common/djangoapps/xblock_django/user_service.py +++ b/common/djangoapps/xblock_django/user_service.py @@ -11,11 +11,16 @@ from openedx.core.djangoapps.external_user_ids.models import ExternalId from openedx.core.djangoapps.user_api.preferences.api import get_user_preferences from common.djangoapps.student.models import anonymous_id_for_user, get_user_by_username_or_email -ATTR_KEY_IS_AUTHENTICATED = 'edx-platform.is_authenticated' -ATTR_KEY_USER_ID = 'edx-platform.user_id' -ATTR_KEY_USERNAME = 'edx-platform.username' -ATTR_KEY_USER_IS_STAFF = 'edx-platform.user_is_staff' -ATTR_KEY_USER_PREFERENCES = 'edx-platform.user_preferences' +from .constants import ( + ATTR_KEY_ANONYMOUS_USER_ID, + ATTR_KEY_IS_AUTHENTICATED, + ATTR_KEY_USER_ID, + ATTR_KEY_USERNAME, + ATTR_KEY_USER_IS_STAFF, + ATTR_KEY_USER_PREFERENCES, +) + + USER_PREFERENCES_WHITE_LIST = ['pref-lang', 'time_zone'] @@ -24,10 +29,18 @@ class DjangoXBlockUserService(UserService): A user service that converts Django users to XBlockUser """ def __init__(self, django_user, **kwargs): + """ + Constructs a DjangoXBlockUserService object. + + Args: + user_is_staff(bool): optional - whether the user is staff in the course + anonymous_user_id(str): optional - anonymous_user_id for the user in the course + """ super().__init__(**kwargs) self._django_user = django_user if self._django_user: self._django_user.user_is_staff = kwargs.get('user_is_staff', False) + self._django_user.anonymous_user_id = kwargs.get('anonymous_user_id', None) def get_current_user(self): """ @@ -82,6 +95,7 @@ class DjangoXBlockUserService(UserService): full_name = None xblock_user.full_name = full_name xblock_user.emails = [django_user.email] + xblock_user.opt_attrs[ATTR_KEY_ANONYMOUS_USER_ID] = django_user.anonymous_user_id xblock_user.opt_attrs[ATTR_KEY_IS_AUTHENTICATED] = True xblock_user.opt_attrs[ATTR_KEY_USER_ID] = django_user.id xblock_user.opt_attrs[ATTR_KEY_USERNAME] = django_user.username diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 9b8f898d1a..593d8f1176 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -4,6 +4,7 @@ import logging import os import sys import time +import warnings from collections import namedtuple from functools import partial @@ -41,6 +42,13 @@ from xmodule.fields import RelativeTime from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.util.xmodule_django import add_webpack_to_fragment +from common.djangoapps.xblock_django.constants import ( + ATTR_KEY_ANONYMOUS_USER_ID, + ATTR_KEY_USER_ID, + ATTR_KEY_USER_IS_STAFF, +) + + log = logging.getLogger(__name__) XMODULE_METRIC_NAME = 'edxapp.xmodule' @@ -1732,7 +1740,77 @@ class XMLParsingSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstr setattr(xblock, field.name, field_value) -class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): +class ModuleSystemShim: + """ + This shim provides the properties formerly available from ModuleSystem which are now being provided by services. + + This shim will be removed, so all properties raise a deprecation warning. + """ + + @property + def anonymous_student_id(self): + """ + Returns the anonymous user ID for the current user and course. + + Deprecated in favor of the user service. + """ + warnings.warn( + 'runtime.anonymous_student_id is deprecated. Please use the user service instead.', + DeprecationWarning, stacklevel=3, + ) + user_service = self._services.get('user') + if user_service: + return user_service.get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID) + return None + + @property + def seed(self): + """ + Returns the numeric current user id, for use as a random seed. + Returns 0 if there is no current user. + + Deprecated in favor of the user service. + """ + warnings.warn( + 'runtime.seed is deprecated. Please use the user service `user_id` instead.', + DeprecationWarning, stacklevel=3, + ) + return self.user_id or 0 + + @property + def user_id(self): + """ + Returns the current user id, or None if there is no current user. + + Deprecated in favor of the user service. + """ + warnings.warn( + 'runtime.user_id is deprecated. Please use the user service instead.', + DeprecationWarning, stacklevel=3, + ) + user_service = self._services.get('user') + if user_service: + return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_ID) + return None + + @property + def user_is_staff(self): + """ + Returns whether the current user has staff access to the course. + + Deprecated in favor of the user service. + """ + warnings.warn( + 'runtime.user_is_staff is deprecated. Please use the user service instead.', + DeprecationWarning, stacklevel=3, + ) + user_service = self._services.get('user') + if user_service: + return self._services['user'].get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) + return None + + +class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, Runtime): """ This is an abstraction such that x_modules can function independent of the courseware (e.g. import into other types of courseware, LMS, @@ -1747,9 +1825,9 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): def __init__( self, static_url, track_function, get_module, render_template, - replace_urls, descriptor_runtime, user=None, filestore=None, + replace_urls, descriptor_runtime, filestore=None, debug=False, hostname="", xqueue=None, publish=None, node_path="", - anonymous_student_id='', course_id=None, + course_id=None, cache=None, can_execute_unsafe_code=None, replace_course_urls=None, replace_jump_to_id_urls=None, error_descriptor_class=None, get_real_user=None, field_data=None, get_user_role=None, rebind_noauth_module_to_user=None, @@ -1771,9 +1849,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): render_template - a function that takes (template_file, context), and returns rendered html. - user - The user to base the random number generator seed off of for this - request - filestore - A filestore ojbect. Defaults to an instance of OSFS based at settings.DATA_DIR. @@ -1789,8 +1864,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): descriptor_runtime - A `DescriptorSystem` to use for loading xblocks by id - anonymous_student_id - Used for tracking modules with student id - course_id - the course_id containing this module publish(event) - A function that allows XModules to publish events (such as grade changes) @@ -1834,12 +1907,9 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): self.render_template = render_template self.DEBUG = self.debug = debug self.HOSTNAME = self.hostname = hostname - self.seed = user.id if user is not None else 0 self.replace_urls = replace_urls self.node_path = node_path - self.anonymous_student_id = anonymous_student_id self.course_id = course_id - self.user_is_staff = user is not None and user.is_staff if publish: self.publish = publish @@ -1859,9 +1929,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): self.descriptor_runtime = descriptor_runtime self.rebind_noauth_module_to_user = rebind_noauth_module_to_user - if user: - self.user_id = user.id - def get(self, attr): """ provide uniform access to attributes (like etree).""" return self.__dict__.get(attr) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index bee106a483..d0c1639bc2 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -35,7 +35,6 @@ from requests.auth import HTTPBasicAuth from rest_framework.decorators import api_view from rest_framework.exceptions import APIException from web_fragments.fragment import Fragment -from xblock.core import XBlock from xblock.django.request import django_to_webob_request, webob_to_django_response from xblock.exceptions import NoSuchHandlerError, NoSuchViewError from xblock.reference.plugins import FSService @@ -97,7 +96,6 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.util.sandboxing import can_execute_unsafe_code, get_python_lib_zip -from xmodule.x_module import XModuleDescriptor log = logging.getLogger(__name__) @@ -539,6 +537,24 @@ def get_module_system_for_user( }) return handlers.get(event_type) + # These modules store data using the anonymous_student_id as a key. + # To prevent loss of data, we will continue to provide old modules with + # the per-student anonymized id (as we have in the past), + # while giving selected modules a per-course anonymized id. + # As we have the time to manually test more modules, we can add to the list + # of modules that get the per-course anonymized id. + if getattr(descriptor, 'requires_per_student_anonymous_id', False): + anonymous_student_id = anonymous_id_for_user(user, None) + else: + anonymous_student_id = anonymous_id_for_user(user, course_id) + + user_is_staff = bool(has_access(user, 'staff', descriptor.location, course_id)) + user_service = DjangoXBlockUserService( + user, + user_is_staff=user_is_staff, + anonymous_user_id=anonymous_student_id, + ) + def publish(block, event_type, event): """ A function that allows XModules to publish events. @@ -746,23 +762,9 @@ def get_module_system_for_user( if staff_access: block_wrappers.append(partial(add_staff_markup, user, disable_staff_debug_info)) - # These modules store data using the anonymous_student_id as a key. - # To prevent loss of data, we will continue to provide old modules with - # the per-student anonymized id (as we have in the past), - # while giving selected modules a per-course anonymized id. - # As we have the time to manually test more modules, we can add to the list - # of modules that get the per-course anonymized id. - is_pure_xblock = isinstance(descriptor, XBlock) and not isinstance(descriptor, XModuleDescriptor) - if (is_pure_xblock and not getattr(descriptor, 'requires_per_student_anonymous_id', False)): - anonymous_student_id = anonymous_id_for_user(user, course_id) - else: - anonymous_student_id = anonymous_id_for_user(user, None) - field_data = DateLookupFieldData(descriptor._field_data, course_id, user) # pylint: disable=protected-access field_data = LmsFieldData(field_data, student_data) - user_is_staff = bool(has_access(user, 'staff', descriptor.location, course_id)) - system = LmsModuleSystem( track_function=track_function, render_template=render_to_string, @@ -794,7 +796,6 @@ def get_module_system_for_user( ), node_path=settings.NODE_PATH, publish=publish, - anonymous_student_id=anonymous_student_id, course_id=course_id, cache=cache, can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)), @@ -806,7 +807,7 @@ def get_module_system_for_user( services={ 'fs': FSService(), 'field-data': field_data, - 'user': DjangoXBlockUserService(user, user_is_staff=user_is_staff), + 'user': user_service, 'verification': XBlockVerificationService(), 'proctoring': ProctoringService(), 'milestones': milestones_helpers.get_service(), diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index e6acde7d22..45afaff3f1 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -2557,3 +2557,95 @@ class TestDisabledXBlockTypes(ModuleStoreTestCase): item = self.store.get_item(item_id) assert item.__class__.__name__ == descriptor return item_id + + +@ddt.ddt +class LmsModuleSystemShimTest(SharedModuleStoreTestCase): + """ + Tests that the deprecated attributes in the LMS Module System (XBlock Runtime) return the expected values. + """ + + @classmethod + def setUpClass(cls): + """ + Set up the course and descriptor used to instantiate the runtime. + """ + super().setUpClass() + cls.course = CourseFactory.create() + cls.descriptor = ItemFactory(category="vertical", parent=cls.course) + + def setUp(self): + """ + Set up the user and other fields that will be used to instantiate the runtime. + """ + super().setUp() + self.user = UserFactory(id=232) + self.student_data = Mock() + self.track_function = Mock() + self.xqueue_callback_url_prefix = Mock() + self.request_token = Mock() + + @ddt.data( + ('seed', 232), + ('user_id', 232), + ('user_is_staff', False), + ) + @ddt.unpack + def test_user_service_attributes(self, attribute, expected_value): + """ + Tests that the deprecated attributes provided by the user service match expected values. + """ + runtime, _ = render.get_module_system_for_user( + self.user, + self.student_data, + self.descriptor, + self.course.id, + self.track_function, + self.xqueue_callback_url_prefix, + self.request_token, + course=self.course, + ) + assert getattr(runtime, attribute) == expected_value + + @patch('lms.djangoapps.courseware.module_render.has_access', Mock(return_value=True, autospec=True)) + def test_user_is_staff(self): + runtime, _ = render.get_module_system_for_user( + self.user, + self.student_data, + self.descriptor, + self.course.id, + self.track_function, + self.xqueue_callback_url_prefix, + self.request_token, + course=self.course, + ) + assert runtime.user_is_staff + + def test_anonymous_student_id(self): + runtime, _ = render.get_module_system_for_user( + self.user, + self.student_data, + self.descriptor, + self.course.id, + self.track_function, + self.xqueue_callback_url_prefix, + self.request_token, + course=self.course, + ) + assert runtime.anonymous_student_id == anonymous_id_for_user(self.user, self.course.id) + + def test_user_service_with_anonymous_user(self): + runtime, _ = render.get_module_system_for_user( + AnonymousUser(), + self.student_data, + self.descriptor, + self.course.id, + self.track_function, + self.xqueue_callback_url_prefix, + self.request_token, + course=self.course, + ) + assert runtime.anonymous_student_id is None + assert runtime.seed == 0 + assert runtime.user_id is None + assert not runtime.user_is_staff diff --git a/lms/djangoapps/lms_xblock/runtime.py b/lms/djangoapps/lms_xblock/runtime.py index 57e048aca0..7a74c4fa4c 100644 --- a/lms/djangoapps/lms_xblock/runtime.py +++ b/lms/djangoapps/lms_xblock/runtime.py @@ -142,12 +142,11 @@ class LmsModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ ModuleSystem specialized to the LMS """ - def __init__(self, **kwargs): + def __init__(self, user=None, **kwargs): request_cache_dict = DEFAULT_REQUEST_CACHE.data store = modulestore() services = kwargs.setdefault('services', {}) - user = kwargs.get('user') if user and user.is_authenticated: services['completion'] = CompletionService(user=user, context_key=kwargs.get('course_id')) services['fs'] = xblock.reference.plugins.FSService() diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 4bf9f2f0f6..558259d998 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -21,6 +21,7 @@ from xblock.runtime import KvsFieldData, MemoryIdManager, Runtime from common.djangoapps.track import contexts as track_contexts from common.djangoapps.track import views as track_views +from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache from lms.djangoapps.grades.api import signals as grades_signals from openedx.core.djangoapps.xblock.apps import get_xblock_app_config @@ -226,6 +227,14 @@ class XBlockRuntime(RuntimeShim, Runtime): elif service_name == "completion": context_key = block.scope_ids.usage_id.context_key return CompletionService(user=self.user, context_key=context_key) + elif service_name == "user": + return DjangoXBlockUserService( + self.user, + # The value should be updated to whether the user is staff in the context when Blockstore runtime adds + # support for courses. + user_is_staff=self.user.is_staff, + anonymous_user_id=self.anonymous_student_id, + ) elif service_name == "i18n": return ModuleI18nService(block=block) # Check if the XBlockRuntimeSystem wants to handle this: