From c5439221cb22c2180bd463344d3b2cb47204b7b1 Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee Date: Wed, 15 Feb 2023 16:25:37 +0530 Subject: [PATCH] chore: address review comments --- .../contentstore/tests/test_i18n.py | 4 +- cms/djangoapps/contentstore/views/block.py | 2 +- cms/djangoapps/contentstore/views/preview.py | 38 ++++++------ .../contentstore/views/tests/test_preview.py | 16 ++--- lms/djangoapps/courseware/block_render.py | 61 +++++++++---------- lms/djangoapps/courseware/tests/helpers.py | 2 +- .../courseware/tests/test_block_render.py | 20 +++--- lms/djangoapps/courseware/views/views.py | 2 +- .../tasks_helper/module_state.py | 4 +- .../core/djangoapps/xblock/runtime/runtime.py | 4 +- xmodule/capa/capa_problem.py | 2 +- xmodule/mako_block.py | 7 +-- xmodule/seq_block.py | 4 +- xmodule/services.py | 25 ++++---- xmodule/x_module.py | 30 ++++++--- 15 files changed, 113 insertions(+), 108 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_i18n.py b/cms/djangoapps/contentstore/tests/test_i18n.py index 980490b7e4..e38094a2d3 100644 --- a/cms/djangoapps/contentstore/tests/test_i18n.py +++ b/cms/djangoapps/contentstore/tests/test_i18n.py @@ -15,7 +15,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory from xmodule.tests.test_export import PureXBlock from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient -from cms.djangoapps.contentstore.views.preview import _preview_module_system +from cms.djangoapps.contentstore.views.preview import _prepare_runtime_for_preview from common.djangoapps.student.tests.factories import UserFactory from openedx.core.lib.edx_six import get_gettext @@ -70,7 +70,7 @@ class TestXBlockI18nService(ModuleStoreTestCase): self.course = CourseFactory.create() self.field_data = mock.Mock() self.descriptor = BlockFactory(category="pure", parent=self.course) - _preview_module_system( + _prepare_runtime_for_preview( self.request, self.descriptor, self.field_data, diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index eb330da26a..7c28f00501 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -296,7 +296,7 @@ class StudioPermissionsService: def load_services_for_studio(runtime, user): """ Function to set some required services used for XBlock edits and studio_view. - (i.e. whenever we're not loading preview module system.) This is required to make information + (i.e. whenever we're not loading _prepare_runtime_for_preview.) This is required to make information about the current user (especially permissions) available via services as needed. """ services = { diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 29900af507..12b00d16e6 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -148,17 +148,17 @@ def preview_layout_asides(block, context, frag, view_name, aside_frag_fns, wrap_ return result -def _preview_module_system(request, descriptor, field_data): +def _prepare_runtime_for_preview(request, block, field_data): """ - Sets properties in the runtime of the specified descriptor that is + Sets properties in the runtime of the specified block that is required for rendering block previews. request: The active django request field_data: Wrapped field data for previews """ - course_id = descriptor.location.course_key - display_name_only = (descriptor.category == 'static_tab') + course_id = block.location.course_key + display_name_only = (block.category == 'static_tab') replace_url_service = ReplaceURLService(course_id=course_id) @@ -198,7 +198,7 @@ def _preview_module_system(request, descriptor, field_data): # the anonymous_user_id to specific courses. These are captured in the # block attribute 'requires_per_student_anonymous_id'. Please note, # the course_id field in AnynomousUserID model is blank if value is None. - if getattr(descriptor, 'requires_per_student_anonymous_id', False): + if getattr(block, 'requires_per_student_anonymous_id', False): preview_anonymous_user_id = anonymous_id_for_user(request.user, None) else: preview_anonymous_user_id = anonymous_id_for_user(request.user, course_id) @@ -220,26 +220,26 @@ def _preview_module_system(request, descriptor, field_data): 'replace_urls': replace_url_service } - descriptor.runtime.get_block_for_descriptor = partial(_load_preview_block, request) - descriptor.runtime.mixins = settings.XBLOCK_MIXINS + block.runtime.get_block_for_descriptor = partial(_load_preview_block, request) + block.runtime.mixins = settings.XBLOCK_MIXINS # Set up functions to modify the fragment produced by student_view - descriptor.runtime.wrappers = wrappers - descriptor.runtime.wrappers_asides = wrappers_asides - descriptor.runtime._runtime_services.update(services) # lint-amnesty, pylint: disable=protected-access + block.runtime.wrappers = wrappers + block.runtime.wrappers_asides = wrappers_asides + block.runtime._runtime_services.update(services) # lint-amnesty, pylint: disable=protected-access # xmodules can check for this attribute during rendering to determine if # they are being rendered for preview (i.e. in Studio) - descriptor.runtime.is_author_mode = True - descriptor.runtime.handler_url_override = handler_url - descriptor.runtime.applicable_aside_types_override = preview_applicable_aside_types - descriptor.runtime.render_child_placeholder = partial( + block.runtime.is_author_mode = True + block.runtime.handler_url_override = handler_url + block.runtime.applicable_aside_types_override = preview_applicable_aside_types + block.runtime.render_child_placeholder = partial( render_child_placeholder, - wrap_block=descriptor.runtime.wrap_xblock + wrap_block=block.runtime.wrap_xblock ) - descriptor.runtime.layout_asides_override = partial( + block.runtime.layout_asides_override = partial( preview_layout_asides, - wrap_aside=descriptor.runtime.wrap_aside + wrap_aside=block.runtime.wrap_aside ) @@ -270,9 +270,9 @@ def _load_preview_block(request, descriptor): else: wrapper = partial(LmsFieldData, student_data=student_data) - # wrap the _field_data upfront to pass to _preview_module_system + # wrap the _field_data upfront to pass to _prepare_runtime_for_preview wrapped_field_data = wrapper(descriptor._field_data) # pylint: disable=protected-access - _preview_module_system(request, descriptor, wrapped_field_data) + _prepare_runtime_for_preview(request, descriptor, wrapped_field_data) descriptor.bind_for_student( request.user.id, diff --git a/cms/djangoapps/contentstore/views/tests/test_preview.py b/cms/djangoapps/contentstore/views/tests/test_preview.py index 944cb4b9bc..c8d1266853 100644 --- a/cms/djangoapps/contentstore/views/tests/test_preview.py +++ b/cms/djangoapps/contentstore/views/tests/test_preview.py @@ -26,7 +26,7 @@ from cms.djangoapps.xblock_config.models import StudioConfig from common.djangoapps import static_replace from common.djangoapps.student.tests.factories import UserFactory -from ..preview import _preview_module_system, get_preview_fragment +from ..preview import _prepare_runtime_for_preview, get_preview_fragment @ddt.ddt @@ -214,7 +214,7 @@ class StudioXBlockServiceBindingTest(ModuleStoreTestCase): Tests that the 'user' and 'i18n' services are provided by the Studio runtime. """ descriptor = BlockFactory(category="pure", parent=self.course) - _preview_module_system( + _prepare_runtime_for_preview( self.request, descriptor, self.field_data, @@ -246,9 +246,9 @@ class CmsModuleSystemShimTest(ModuleStoreTestCase): self.field_data = mock.Mock() self.contentstore = contentstore() self.descriptor = BlockFactory(category="problem", parent=course) - _preview_module_system( + _prepare_runtime_for_preview( self.request, - descriptor=self.descriptor, + block=self.descriptor, field_data=mock.Mock(), ) self.course = self.store.get_item(course.location) @@ -305,9 +305,9 @@ class CmsModuleSystemShimTest(ModuleStoreTestCase): """Test anonymous_user_id on a block which uses per-student anonymous IDs""" # Create the runtime with the flag turned on. descriptor = BlockFactory(category="problem", parent=self.course) - _preview_module_system( + _prepare_runtime_for_preview( self.request, - descriptor=descriptor, + block=descriptor, field_data=mock.Mock(), ) assert descriptor.runtime.anonymous_student_id == '26262401c528d7c4a6bbeabe0455ec46' @@ -317,9 +317,9 @@ class CmsModuleSystemShimTest(ModuleStoreTestCase): """Test anonymous_user_id on a block which uses per-course anonymous IDs""" # Create the runtime with the flag turned on. descriptor = BlockFactory(category="lti", parent=self.course) - _preview_module_system( + _prepare_runtime_for_preview( self.request, - descriptor=descriptor, + block=descriptor, field_data=mock.Mock(), ) assert descriptor.runtime.anonymous_student_id == 'ad503f629b55c531fed2e45aa17a3368' diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index fb28034aaa..6cf435f5a0 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -403,11 +403,11 @@ def get_block_for_descriptor(user, request, descriptor, field_data_cache, course ) -def get_module_system_for_user( +def prepare_runtime_for_user( user, student_data, # TODO # Arguments preceding this comment have user binding, those following don't - descriptor, + block, course_id, track_function, request_token, @@ -421,14 +421,13 @@ def get_module_system_for_user( will_recheck_access=False, ): """ - Helper function that returns a module system and student_data bound to a user and a descriptor. + Helper function that binds the given xblock to a user and student_data to a user and the block. The purpose of this function is to factor out everywhere a user is implicitly bound when creating a module, - to allow an existing block to be re-bound to a user. Most of the user bindings happen when creating the - closures that feed the instantiation of ModuleSystem. + to allow an existing block to be re-bound to a user. The arguments fall into two categories: those that have explicit or implicit user binding, which are user - and student_data, and those don't and are just present so that ModuleSystem can be instantiated, which + and student_data, and those don't and are used to instantiate the service required in LMS, which are all the other arguments. Ultimately, this isn't too different than how get_block_for_descriptor_internal was before refactoring. @@ -437,7 +436,7 @@ def get_module_system_for_user( request_token (str): A token unique to the request use by xblock initialization Returns: - KvsFieldData: student_data bound to, primarily, the user and descriptor + KvsFieldData: student_data bound to, primarily, the user and block """ def make_xqueue_callback(dispatch='score_update'): @@ -449,7 +448,7 @@ def get_module_system_for_user( kwargs=dict( course_id=str(course_id), userid=str(user.id), - mod_id=str(descriptor.location), + mod_id=str(block.location), dispatch=dispatch ), ) @@ -459,7 +458,7 @@ def get_module_system_for_user( # Default queuename is course-specific and is derived from the course that # contains the current block. # TODO: Queuename should be derived from 'course_settings.json' of each course - xqueue_default_queuename = descriptor.location.org + '-' + descriptor.location.course + xqueue_default_queuename = block.location.org + '-' + block.location.course xqueue_service = XQueueService( construct_callback=make_xqueue_callback, @@ -500,12 +499,12 @@ def get_module_system_for_user( # 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): + if getattr(block, '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_is_staff = bool(has_access(user, 'staff', block.location, course_id)) user_service = DjangoXBlockUserService( user, user_is_staff=user_is_staff, @@ -518,7 +517,7 @@ def get_module_system_for_user( rebind_user_service = RebindUserService( user, course_id, - get_module_system_for_user, + prepare_runtime_for_user, track_function=track_function, position=position, wrap_xblock_display=wrap_xblock_display, @@ -552,9 +551,9 @@ def get_module_system_for_user( )) replace_url_service = ReplaceURLService( - data_directory=getattr(descriptor, 'data_dir', None), + data_directory=getattr(block, 'data_dir', None), course_id=course_id, - static_asset_path=static_asset_path or descriptor.static_asset_path, + static_asset_path=static_asset_path or block.static_asset_path, jump_to_id_base_url=reverse('jump_to_id', kwargs={'course_id': str(course_id), 'module_id': ''}) ) @@ -578,11 +577,11 @@ def get_module_system_for_user( del user.real_user.masquerade_settings user.real_user.masquerade_settings = masquerade_settings else: - staff_access = has_access(user, 'staff', descriptor, course_id) + staff_access = has_access(user, 'staff', block, course_id) if staff_access: block_wrappers.append(partial(add_staff_markup, user, disable_staff_debug_info)) - field_data = DateLookupFieldData(descriptor._field_data, course_id, user) # pylint: disable=protected-access + field_data = DateLookupFieldData(block._field_data, course_id, user) # pylint: disable=protected-access field_data = LmsFieldData(field_data, student_data) store = modulestore() @@ -621,17 +620,15 @@ def get_module_system_for_user( 'publish': EventPublishingService(user, course_id, track_function), } - descriptor.runtime.get_block_for_descriptor = inner_get_block + block.runtime.get_block_for_descriptor = inner_get_block - # TODO: When we merge the descriptor and module systems, we can stop reaching into the mixologist (cpennington) - descriptor.runtime.mixins = descriptor.runtime.mixologist._mixins # lint-amnesty, pylint: disable=protected-access - descriptor.runtime.wrappers = block_wrappers - descriptor.runtime._runtime_services.update(services) # lint-amnesty, pylint: disable=protected-access - descriptor.runtime.request_token = request_token - descriptor.runtime.wrap_asides_override = lms_wrappers_aside - descriptor.runtime.applicable_aside_types_override = lms_applicable_aside_types + block.runtime.wrappers = block_wrappers + block.runtime._runtime_services.update(services) # lint-amnesty, pylint: disable=protected-access + block.runtime.request_token = request_token + block.runtime.wrap_asides_override = lms_wrappers_aside + block.runtime.applicable_aside_types_override = lms_applicable_aside_types - # pass position specified in URL to module through ModuleSystem + # pass position specified in URL to runtime if position is not None: try: position = int(position) @@ -639,12 +636,12 @@ def get_module_system_for_user( log.exception('Non-integer %r passed as position.', position) position = None - descriptor.runtime.set('position', position) + block.runtime.set('position', position) - descriptor.runtime.set('user_is_staff', user_is_staff) - descriptor.runtime.set('user_is_admin', bool(has_access(user, 'staff', 'global'))) - descriptor.runtime.set('user_is_beta_tester', CourseBetaTesterRole(course_id).has_user(user)) - descriptor.runtime.set('days_early_for_beta', descriptor.days_early_for_beta) + block.runtime.set('user_is_staff', user_is_staff) + block.runtime.set('user_is_admin', bool(has_access(user, 'staff', 'global'))) + block.runtime.set('user_is_beta_tester', CourseBetaTesterRole(course_id).has_user(user)) + block.runtime.set('days_early_for_beta', block.days_early_for_beta) return field_data @@ -664,10 +661,10 @@ def get_block_for_descriptor_internal(user, descriptor, student_data, course_id, request_token (str): A unique token for this request, used to isolate xblock rendering """ - student_data = get_module_system_for_user( + student_data = prepare_runtime_for_user( user=user, student_data=student_data, # These have implicit user bindings, the rest of args are considered not to - descriptor=descriptor, + block=descriptor, course_id=course_id, track_function=track_function, position=position, diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index 5f8eadf8a3..99dbf75250 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -68,7 +68,7 @@ class BaseTestXmodule(ModuleStoreTestCase): def new_module_runtime(self, runtime=None, **kwargs): """ - Generate a new ModuleSystem that is minimally set up for testing + Generate a new DescriptorSystem that is minimally set up for testing """ if runtime: return prepare_block_runtime(runtime, course_id=self.course.id, **kwargs) diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index 36a674dff1..37c3848671 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -2270,7 +2270,7 @@ class LMSXBlockServiceMixin(SharedModuleStoreTestCase): """ Instantiate the runtem. """ - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.descriptor, @@ -2670,7 +2670,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.track_function = Mock() self.request_token = Mock() self.contentstore = contentstore() - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.descriptor, @@ -2694,7 +2694,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): @patch('lms.djangoapps.courseware.block_render.has_access', Mock(return_value=True, autospec=True)) def test_user_is_staff(self): - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.descriptor, @@ -2708,7 +2708,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): @patch('lms.djangoapps.courseware.block_render.get_user_role', Mock(return_value='instructor', autospec=True)) def test_get_user_role(self): - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.descriptor, @@ -2724,10 +2724,10 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): def test_anonymous_student_id_bug(self): """ - Verifies that subsequent calls to get_module_system_for_user have no effect on each block runtime's + Verifies that subsequent calls to prepare_runtime_for_user have no effect on each block runtime's anonymous_student_id value. """ - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.problem_descriptor, @@ -2739,7 +2739,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): # Ensure the problem block returns a per-user anonymous id assert self.problem_descriptor.runtime.anonymous_student_id == anonymous_id_for_user(self.user, None) - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.descriptor, @@ -2755,7 +2755,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): assert self.problem_descriptor.runtime.anonymous_student_id == anonymous_id_for_user(self.user, None) def test_user_service_with_anonymous_user(self): - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( AnonymousUser(), self.student_data, self.descriptor, @@ -2771,7 +2771,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): assert not self.descriptor.runtime.get_user_role() def test_get_real_user(self): - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.descriptor, @@ -2816,7 +2816,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): XQUEUE_WAITTIME_BETWEEN_REQUESTS=15, ) def test_xqueue_settings(self): - _ = render.get_module_system_for_user( + _ = render.prepare_runtime_for_user( self.user, self.student_data, self.descriptor, diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 79d2976d2c..bd549324fc 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -1487,7 +1487,7 @@ def enclosing_sequence_for_gating_checks(block): if ancestor: # get_parent() returns a parent block instance cached on the block which does not - # have the ModuleSystem bound to it so we need to get it again with get_block() which will set up everything. + # have user data bound to it so we need to get it again with get_block() which will set up everything. return block.runtime.get_block(ancestor.location) return None diff --git a/lms/djangoapps/instructor_task/tasks_helper/module_state.py b/lms/djangoapps/instructor_task/tasks_helper/module_state.py index ddec49cf8a..ec7d8b89d4 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/module_state.py +++ b/lms/djangoapps/instructor_task/tasks_helper/module_state.py @@ -351,7 +351,7 @@ def _get_module_instance_for_task(course_id, student, module_descriptor, xblock_ ''' Make a tracking function that logs what happened. - For insertion into ModuleSystem, and used by CapaModule, which will + For insertion into runtime, and used by CapaModule, which will provide the event_type (as string) and event (as dict) as arguments. The request_info and task_info (and page) are provided here. ''' @@ -375,7 +375,7 @@ def _get_track_function_for_task(student, xblock_instance_args=None, source_page """ Make a tracking function that logs what happened. - For insertion into ModuleSystem, and used by CapaModule, which will + For insertion into runtime, and used by CapaModule, which will provide the event_type (as string) and event (as dict) as arguments. The request_info and task_info (and page) are provided here. """ diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index cadc7f4e33..132e3b9728 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -256,13 +256,13 @@ class XBlockRuntime(RuntimeShim, Runtime): elif service_name == 'replace_urls': return ReplaceURLService(xblock=block, lookup_asset_url=self._lookup_asset_url) elif service_name == 'rebind_user': - # this service should ideally be initialized with all the arguments of get_module_system_for_user + # this service should ideally be initialized with all the arguments of prepare_runtime_for_user # but only the positional arguments are passed here as the other arguments are too # specific to the lms.block_render module return RebindUserService( self.user, context_key, - block_render.get_module_system_for_user, + block_render.prepare_runtime_for_user, track_function=make_track_function(), request_token=request_token(crum.get_current_request()), ) diff --git a/xmodule/capa/capa_problem.py b/xmodule/capa/capa_problem.py index 7a6592e095..9dd17cc253 100644 --- a/xmodule/capa/capa_problem.py +++ b/xmodule/capa/capa_problem.py @@ -93,7 +93,7 @@ class LoncapaSystem(object): i18n: an object implementing the `gettext.Translations` interface so that we can use `.ugettext` to localize strings. - See :class:`ModuleSystem` for documentation of other attributes. + See :class:`DescriptorSystem` for documentation of other attributes. """ def __init__( diff --git a/xmodule/mako_block.py b/xmodule/mako_block.py index 017b277e03..6e6a789104 100644 --- a/xmodule/mako_block.py +++ b/xmodule/mako_block.py @@ -19,12 +19,11 @@ class MakoDescriptorSystem(DescriptorSystem): # lint-amnesty, pylint: disable=a # Add the MakoService to the descriptor system. # - # This is not needed by most XBlocks, because they are initialized with a full runtime ModuleSystem that already - # has the MakoService. - # However, there are a few cases where the XBlock only has the descriptor system instead of the full module + # This is not needed by most XBlocks, because the MakoService is added to their runtimes. + # However, there are a few cases where the MakoService is not added to the XBlock's # runtime. Specifically: # * in the Instructor Dashboard bulk emails tab, when rendering the HtmlBlock for its WYSIWYG editor. - # * during testing, when using the ModuleSystemTestCase to fetch factory-created blocks. + # * during testing, when fetching factory-created blocks. from common.djangoapps.edxmako.services import MakoService self._services['mako'] = MakoService() diff --git a/xmodule/seq_block.py b/xmodule/seq_block.py index eab754ba06..0d8be411fd 100644 --- a/xmodule/seq_block.py +++ b/xmodule/seq_block.py @@ -302,8 +302,8 @@ class SequenceBlock( def bind_for_student(self, user_id, wrappers=None): # The position of the child XBlock to select can also be passed in via the URL. - # In such cases the value is set on the ModuleSystem in get_module_system_for_user() - # and needs to be read here after the ModuleSystem has been set on the XBlock. + # In such cases the value is set in the runtime inside prepare_runtime_for_user() + # and needs to be read here after the value has been set. super().bind_for_student(user_id, wrappers) # If position is specified in system, then use that instead. position = getattr(self.runtime, 'position', None) diff --git a/xmodule/services.py b/xmodule/services.py index d53d72fc32..c4d00a7390 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -147,9 +147,8 @@ class RebindUserService(Service): """ An XBlock Service that allows modules to get rebound to real users if it was previously bound to an AnonymousUser. - This used to be a local function inside the `lms.djangoapps.courseware.block_render.get_module_system_for_user` - method, and was passed as a constructor argument to x_module.ModuleSystem. This has been refactored out into a - service to simplify the ModuleSystem and lives in this module temporarily. + This used to be a local function inside the `lms.djangoapps.courseware.block_render.prepare_runtime_for_user` + method. This has been refactored out into a service and lives in this module temporarily. TODO: Only the old LTI XBlock uses it in 2 places for LTI 2.0 integration. As the LTI XBlock is deprecated in favour of the LTI Consumer XBlock, this should be removed when the LTI XBlock is removed. @@ -158,18 +157,18 @@ class RebindUserService(Service): user (User) - A Django User object course_id (str) - Course ID course (Course) - Course Object - get_module_system_for_user (function) - The helper function that will be called to create a module system + prepare_runtime_for_user (function) - The helper function that will be called to create a module system for a specfic user. This is the parent function from which this service was reactored out. - `lms.djangoapps.courseware.block_render.get_module_system_for_user` - kwargs (dict) - all the keyword arguments that need to be passed to the `get_module_system_for_user` + `lms.djangoapps.courseware.block_render.prepare_runtime_for_user` + kwargs (dict) - all the keyword arguments that need to be passed to the `prepare_runtime_for_user` function when it is called during rebinding """ - def __init__(self, user, course_id, get_module_system_for_user, **kwargs): + def __init__(self, user, course_id, prepare_runtime_for_user, **kwargs): super().__init__(**kwargs) self.user = user self.course_id = course_id self._ref = { - "get_module_system_for_user": get_module_system_for_user + "prepare_runtime_for_user": prepare_runtime_for_user } self._kwargs = kwargs @@ -202,10 +201,10 @@ class RebindUserService(Service): with modulestore().bulk_operations(self.course_id): course = modulestore().get_course(course_key=self.course_id) - inner_student_data = self._ref["get_module_system_for_user"]( + inner_student_data = self._ref["prepare_runtime_for_user"]( user=real_user, student_data=student_data_real_user, # These have implicit user bindings, rest of args considered not to - descriptor=block, + block=block, course_id=self.course_id, course=course, **self._kwargs @@ -221,16 +220,14 @@ class RebindUserService(Service): ) block.scope_ids = block.scope_ids._replace(user_id=real_user.id) - # now bind the module to the new ModuleSystem instance and vice-versa - block.runtime.xmodule_instance = block class EventPublishingService(Service): """ An XBlock Service that allows XModules to publish events (e.g. grading, completion). - We have separated it from the ModuleSystem to be able to alter its behavior when using a different context: - LMS, Studio, or Instructor tasks. + We have implemented it as a seperate service to be able to alter its behavior when using + a different context: LMS, Studio, or Instructor tasks. """ def __init__(self, user, course_id, track_function, **kwargs): super().__init__(**kwargs) diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 727a77840e..031a895b0f 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -336,13 +336,19 @@ class XModuleMixin(XModuleFields, XBlock): 'xmodule_runtime property is deprecated. Please use the runtime property instead.', DeprecationWarning, stacklevel=3, ) - return self._runtime + return self.runtime @property def system(self): """ Return the XBlock runtime (backwards compatibility alias provided for XModules). + + Deprecated in favor of the runtime property. """ + warnings.warn( + 'system property is deprecated. Please use the runtime property instead.', + DeprecationWarning, stacklevel=3, + ) return self.runtime @property @@ -619,8 +625,8 @@ class XModuleMixin(XModuleFields, XBlock): # Skip rebinding if we're already bound a user, and it's this user. if self.scope_ids.user_id is not None and user_id == self.scope_ids.user_id: - if getattr(self._runtime, 'position', None): - self.position = self._runtime.position # update the position of the tab + if getattr(self.runtime, 'position', None): + self.position = self.runtime.position # update the position of the tab return # If we are switching users mid-request, save the data from the old user. @@ -1017,7 +1023,7 @@ def descriptor_global_local_resource_url(block, uri): class MetricsMixin: """ - Mixin for adding metric logging for render and handle methods in the DescriptorSystem and ModuleSystem. + Mixin for adding metric logging for render and handle methods in the DescriptorSystem. """ def render(self, block, view_name, context=None): # lint-amnesty, pylint: disable=missing-function-docstring @@ -1478,6 +1484,9 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemSh def get_block(self, usage_id, for_parent=None): """See documentation for `xblock.runtime:Runtime.get_block`""" block = self.load_item(usage_id, for_parent=for_parent) + # get_block_for_descriptor property is used to bind additional data such as user data + # to the XBlock and to check if the user has access to the block as may be required for + # the LMS or Preview. if getattr(self, 'get_block_for_descriptor', None): return self.get_block_for_descriptor(block) return block @@ -1515,10 +1524,9 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemSh return result def handler_url(self, block, handler_name, suffix='', query='', thirdparty=False): - # Currently, Modulestore is responsible for instantiating DescriptorSystems - # This means that LMS/CMS don't have a way to define a subclass of DescriptorSystem - # that implements the correct handler url. So, for now, instead, we will reference a - # global function that the application can override. + # When the Modulestore instantiates DescriptorSystems, we will reference a + # global function that the application can override, unless a specific function is + # defined for LMS/CMS through the handler_url_override property. if getattr(self, 'handler_url_override', None): return self.handler_url_override(block, handler_name, suffix, query, thirdparty) return descriptor_global_handler_url(block, handler_name, suffix, query, thirdparty) @@ -1537,6 +1545,8 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemSh """ See :meth:`xblock.runtime.Runtime:applicable_aside_types` for documentation. """ + # applicable_aside_types_override property can be used by LMS/CMS to define specific filters + # and conditions as may be applicable. if getattr(self, 'applicable_aside_types_override', None): return self.applicable_aside_types_override(block, applicable_aside_types=super().applicable_aside_types) @@ -1588,11 +1598,13 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemSh return service def wrap_aside(self, block, aside, view, frag, context): + # LMS/CMS can define custom wrap aside using wrap_asides_override as required. if getattr(self, 'wrap_asides_override', None): return self.wrap_asides_override(block, aside, view, frag, context, request_token=self.request_token) return super().wrap_aside(block, aside, view, frag, context) def layout_asides(self, block, context, frag, view_name, aside_frag_fns): + # LMS/CMS can define custom layout aside using layout_asides_override as required. if getattr(self, 'layout_asides_override', None): return self.layout_asides_override(block, context, frag, view_name, aside_frag_fns) return super().layout_asides(block, context, frag, view_name, aside_frag_fns) @@ -1715,7 +1727,7 @@ class XMLParsingSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstr class DoNothingCache: - """A duck-compatible object to use in ModuleSystem when there's no cache.""" + """A duck-compatible object to use in ModuleSystemShim when there's no cache.""" def get(self, _key): return None