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

refactor: deprecate ModuleSystem user attributes in favor of user service

Browse Source 
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 c41e7fb93a)
This commit is contained in:
Jillian Vogel
2021-08-31 16:18:43 +09:30
parent eabd6e8019
commit cf1064616c
9 changed files with 253 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
cms/djangoapps/contentstore/views/preview.py
View File

@@ -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(),
},

11
common/djangoapps/xblock_django/constants.py Normal file
View File

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

25
common/djangoapps/xblock_django/tests/test_user_service.py
View File

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

24
common/djangoapps/xblock_django/user_service.py
View File

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

95
common/lib/xmodule/xmodule/x_module.py
View File

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

37
lms/djangoapps/courseware/module_render.py
View File

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

92
lms/djangoapps/courseware/tests/test_module_render.py
View File

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

3
lms/djangoapps/lms_xblock/runtime.py
View File

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

9
openedx/core/djangoapps/xblock/runtime/runtime.py
View File

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