From 09e1197053121f809c1efab1254f7e26db27deec Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee Date: Sat, 4 Feb 2023 12:47:46 +0530 Subject: [PATCH] feat: remove CombinedSystem --- cms/djangoapps/contentstore/views/preview.py | 157 +++--------------- .../contentstore/views/tests/test_block.py | 3 +- lms/djangoapps/courseware/block_render.py | 79 ++------- xmodule/seq_block.py | 4 +- xmodule/services.py | 6 +- xmodule/x_module.py | 148 +++-------------- 6 files changed, 63 insertions(+), 334 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 937bda7a89..d627f64d60 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -24,7 +24,7 @@ from xmodule.services import SettingsService, TeamsConfigurationService from xmodule.studio_editable import has_author_view from xmodule.util.sandboxing import SandboxService from xmodule.util.xmodule_django import add_webpack_to_fragment -from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW, DescriptorSystem +from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW from cms.djangoapps.xblock_config.models import StudioConfig from cms.djangoapps.contentstore.toggles import individualize_anonymous_user_id, ENABLE_COPY_PASTE_FEATURE from cms.lib.xblock.field_data import CmsFieldData @@ -145,101 +145,13 @@ def preview_layout_asides(block, context, frag, view_name, aside_frag_fns, wrap_ return result -class PreviewModuleSystem(DescriptorSystem): # pylint: disable=abstract-method - """ - An XModule ModuleSystem for use in Studio previews - """ - # xblocks can check for this attribute during rendering to determine if - # they are being rendered for preview (i.e. in Studio) - ####################### - ####################### - ## Set directly to system below - ####################### - ####################### - # is_author_mode = True - - ####################### - ####################### - ## Implemented as handler_url above - ####################### - ####################### - # def handler_url(self, block, handler_name, suffix='', query='', thirdparty=False): - # return reverse('preview_handler', kwargs={ - # 'usage_key_string': str(block.scope_ids.usage_id), - # 'handler': handler_name, - # 'suffix': suffix, - # }) + '?' + query - - ####################### - ####################### - ## Being monkey patched in Descriptor system from cms.djangoapps.xblock_config.apps.py - ####################### - ####################### - # def local_resource_url(self, block, uri): - # return xblock_local_resource_url(block, uri) - - ####################### - ####################### - ## Implemented as preview_applicable_aside_types above - ####################### - ####################### - # def applicable_aside_types(self, block): - # """ - # Remove acid_aside and honor the config record - # """ - # if not StudioConfig.asides_enabled(block.scope_ids.block_type): - # return [] - - # # TODO: aside_type != 'acid_aside' check should be removed once AcidBlock is only installed during tests - # # (see https://openedx.atlassian.net/browse/TE-811) - # return [ - # aside_type - # for aside_type in super().applicable_aside_types(block) - # if aside_type != 'acid_aside' - # ] - - ####################### - ####################### - ## Implemented as render_child_placeholder above - ####################### - ####################### - # def render_child_placeholder(self, block, view_name, context): - # """ - # Renders a placeholder XBlock. - # """ - # return self.wrap_xblock(block, view_name, Fragment(), context) - - - ####################### - ####################### - ## Implemented as preview_layout_asides above - ####################### - ####################### - # def layout_asides(self, block, context, frag, view_name, aside_frag_fns): - # position_for_asides = '' - # result = Fragment() - # result.add_fragment_resources(frag) - - # for aside, aside_fn in aside_frag_fns: - # aside_frag = aside_fn(block, context) - # if aside_frag.content != '': - # aside_frag_wrapped = self.wrap_aside(block, aside, view_name, aside_frag, context) - # aside.save() - # result.add_fragment_resources(aside_frag_wrapped) - # replacement = position_for_asides + aside_frag_wrapped.content - # frag.content = frag.content.replace(position_for_asides, replacement) - - # result.add_content(frag.content) - # return result - - def _preview_module_system(request, descriptor, field_data): """ - Returns a ModuleSystem for the specified descriptor that is specialized for - rendering block previews. + Sets properties in the runtime of the specified descriptor that is + required for rendering block previews. request: The active django request - descriptor: An XModuleDescriptor + field_data: Wrapped field data for previews """ course_id = descriptor.location.course_key @@ -305,58 +217,28 @@ def _preview_module_system(request, descriptor, field_data): 'replace_urls': replace_url_service } - # system = PreviewModuleSystem( - # load_item=descriptor._runtime.load_item, - # resources_fs=descriptor._runtime.resources_fs, - # error_tracker=descriptor._runtime.error_tracker, - # # get_module=partial(_load_preview_module, request), - # # mixins=settings.XBLOCK_MIXINS, - - # # Set up functions to modify the fragment produced by student_view - # # wrappers=wrappers, - # # wrappers_asides=wrappers_asides, - # # Get the raw DescriptorSystem, not the CombinedSystem - # # descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access - # # services={ - # # "field-data": field_data, - # # "i18n": ModuleI18nService, - # # 'mako': mako_service, - # # "settings": SettingsService(), - # # "user": DjangoXBlockUserService( - # # request.user, - # # user_role=get_user_role(request.user, course_id), - # # anonymous_user_id=preview_anonymous_user_id, - # # ), - # # "partitions": StudioPartitionService(course_id=course_id), - # # "teams_configuration": TeamsConfigurationService(), - # # "sandbox": SandboxService(contentstore=contentstore, course_id=course_id), - # # "cache": CacheService(cache), - # # 'replace_urls': replace_url_service - # # }, - # ) - - descriptor._runtime.get_block = partial(_load_preview_block, request), - descriptor._runtime.mixins = settings.XBLOCK_MIXINS + descriptor.runtime.get_block_for_descriptor = partial(_load_preview_block, request), + descriptor.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._services.update(services) + descriptor.runtime.wrappers = wrappers + descriptor.runtime.wrappers_asides = wrappers_asides + descriptor.runtime._services.update(services) - 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( + # 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( render_child_placeholder, - wrap_xblock = descriptor._runtime.wrap_xblock + wrap_xblock = descriptor.runtime.wrap_xblock ) - descriptor._runtime.layout_asides_override = partial( + descriptor.runtime.layout_asides_override = partial( preview_layout_asides, - wrap_aside = descriptor._runtime.wrap_aside + wrap_aside = descriptor.runtime.wrap_aside ) - return descriptor._runtime - class StudioPartitionService(PartitionService): """ @@ -387,10 +269,9 @@ def _load_preview_block(request, descriptor): # wrap the _field_data upfront to pass to _preview_module_system wrapped_field_data = wrapper(descriptor._field_data) # pylint: disable=protected-access - preview_runtime = _preview_module_system(request, descriptor, wrapped_field_data) + _preview_module_system(request, descriptor, wrapped_field_data) descriptor.bind_for_student( - preview_runtime, request.user.id, [wrapper] ) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 7f5cbfe2be..863da2e02f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -2128,8 +2128,7 @@ class TestEditSplitModule(ItemTest): group_id_to_child = split_test.group_id_to_child.copy() self.assertEqual(2, len(group_id_to_child)) - # Test environment and Studio use different module systems - # (CachingDescriptorSystem is used in tests, PreviewModuleSystem in Studio). + # CachingDescriptorSystem is used in tests. # CachingDescriptorSystem doesn't have user service, that's needed for # SplitTestBlock. So, in this line of code we add this service manually. split_test.runtime._services['user'] = DjangoXBlockUserService(self.user) # pylint: disable=protected-access diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 05b8fec291..b77f4392a3 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -115,7 +115,7 @@ class LmsModuleRenderError(Exception): def make_track_function(request): ''' Make a tracking function that logs what happened. - For use in ModuleSystem. + For use in DescriptorSystem. ''' from common.djangoapps.track import views as track_views @@ -621,60 +621,18 @@ def get_module_system_for_user( 'publish': EventPublishingService(user, course_id, track_function), } + descriptor.runtime.get_block_for_descriptor = inner_get_block - # system = LmsModuleSystem( - # load_item=descriptor._runtime.load_item, - # resources_fs=descriptor._runtime.resources_fs, - # error_tracker=descriptor._runtime.error_tracker, - # # get_module=inner_get_module, - # # TODO: When we merge the descriptor and module systems, we can stop reaching into the mixologist (cpennington) - # # mixins=descriptor.runtime.mixologist._mixins, # pylint: disable=protected-access - # # wrappers=block_wrappers, - # # services={ - # # 'fs': FSService(), - # # 'field-data': field_data, - # # 'mako': mako_service, - # # 'user': user_service, - # # 'verification': XBlockVerificationService(), - # # 'proctoring': ProctoringService(), - # # 'milestones': milestones_helpers.get_service(), - # # 'credit': CreditService(), - # # 'bookmarks': BookmarksService(user=user), - # # 'gating': GatingService(), - # # 'grade_utils': GradesUtilService(course_id=course_id), - # # 'user_state': UserStateService(), - # # 'content_type_gating': ContentTypeGatingService(), - # # 'cache': CacheService(cache), - # # 'sandbox': SandboxService(contentstore=contentstore, course_id=course_id), - # # 'xqueue': xqueue_service, - # # 'replace_urls': replace_url_service, - # # 'rebind_user': rebind_user_service, - # # 'completion': CompletionService(user=user, context_key=course_id) - # # if user and user.is_authenticated - # # else None, - # # 'i18n': ModuleI18nService, - # # 'library_tools': LibraryToolsService(store, user_id=user.id if user else None), - # # 'partitions': PartitionService(course_id=course_id, cache=DEFAULT_REQUEST_CACHE.data), - # # 'settings': SettingsService(), - # # 'user_tags': UserTagsService(user=user, course_id=course_id), - # # 'badging': BadgingService(course_id=course_id, modulestore=store) if badges_enabled() else None, - # # 'teams': TeamsService(), - # # 'teams_configuration': TeamsConfigurationService(), - # # 'call_to_action': CallToActionService(), - # # 'publish': EventPublishingService(user, course_id, track_function), - # # }, - # # descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access - # # request_token=request_token, - # ) + # TODO: When we merge the descriptor and module systems, we can stop reaching into the mixologist (cpennington) + # mixins=descriptor.runtime.mixologist._mixins, # pylint: disable=protected-access + descriptor.runtime.mixins = descriptor.runtime.mixologist._mixins + + descriptor.runtime.wrappers = block_wrappers + descriptor.runtime._services.update(services) + descriptor.runtime.request_token = request_token - descriptor._runtime.get_block = inner_get_block - descriptor._runtime.mixins = descriptor.runtime.mixologist._mixins - descriptor._runtime.wrappers = block_wrappers - descriptor._runtime._services.update(services) - descriptor._runtime.request_token = request_token - - descriptor._runtime.wrap_asides_override = lms_wrappers_aside - descriptor._runtime.applicable_aside_types_override = lms_applicable_aside_types + descriptor.runtime.wrap_asides_override = lms_wrappers_aside + descriptor.runtime.applicable_aside_types_override = lms_applicable_aside_types # pass position specified in URL to module through ModuleSystem if position is not None: @@ -684,14 +642,14 @@ def get_module_system_for_user( log.exception('Non-integer %r passed as position.', position) position = None - descriptor._runtime.set('position', position) + descriptor.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) + 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) - return descriptor._runtime, field_data + return field_data # TODO: Find all the places that this method is called and figure out how to @@ -709,7 +667,7 @@ 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 """ - (system, student_data) = get_module_system_for_user( + student_data = get_module_system_for_user( user=user, student_data=student_data, # These have implicit user bindings, the rest of args are considered not to descriptor=descriptor, @@ -727,7 +685,6 @@ def get_block_for_descriptor_internal(user, descriptor, student_data, course_id, ) descriptor.bind_for_student( - system, user.id, [ partial(DateLookupFieldData, course_id=course_id, user=user), diff --git a/xmodule/seq_block.py b/xmodule/seq_block.py index fc7202bfe0..eab754ba06 100644 --- a/xmodule/seq_block.py +++ b/xmodule/seq_block.py @@ -300,11 +300,11 @@ class SequenceBlock( self.gated_sequence_paywall = None - def bind_for_student(self, xmodule_runtime, user_id, wrappers=None): + 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. - super().bind_for_student(xmodule_runtime, user_id, wrappers) + super().bind_for_student(user_id, wrappers) # If position is specified in system, then use that instead. position = getattr(self.runtime, 'position', None) if position is not None: diff --git a/xmodule/services.py b/xmodule/services.py index 5418ea4f19..d53d72fc32 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -202,7 +202,7 @@ class RebindUserService(Service): with modulestore().bulk_operations(self.course_id): course = modulestore().get_course(course_key=self.course_id) - (inner_system, inner_student_data) = self._ref["get_module_system_for_user"]( + inner_student_data = self._ref["get_module_system_for_user"]( user=real_user, student_data=student_data_real_user, # These have implicit user bindings, rest of args considered not to descriptor=block, @@ -212,7 +212,6 @@ class RebindUserService(Service): ) block.bind_for_student( - inner_system, real_user.id, [ partial(DateLookupFieldData, course_id=self.course_id, user=self.user), @@ -223,8 +222,7 @@ 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 = inner_system - inner_system.xmodule_instance = block + block.runtime.xmodule_instance = block class EventPublishingService(Service): diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 04d4d41970..8f04b284a2 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -313,19 +313,31 @@ class XModuleMixin(XModuleFields, XBlock): icon_class = 'other' def __init__(self, *args, **kwargs): - self.xmodule_runtime = None self._asides = [] super().__init__(*args, **kwargs) @property def runtime(self): - return CombinedSystem(self.xmodule_runtime, self._runtime) + return self._runtime @runtime.setter def runtime(self, value): self._runtime = value + @property + def xmodule_runtime(self): + """ + Shim to maintain backward compatibility. + + Deprecated in favor of the runtime property. + """ + warnings.warn( + 'xmodule_runtime property is deprecated. Please use the runtime property instead.', + DeprecationWarning, stacklevel=3, + ) + return self._runtime + @property def system(self): """ @@ -593,12 +605,11 @@ class XModuleMixin(XModuleFields, XBlock): """ return None - def bind_for_student(self, xmodule_runtime, user_id, wrappers=None): + def bind_for_student(self, user_id, wrappers=None): """ Set up this XBlock to act as an XModule instead of an XModuleDescriptor. Arguments: - xmodule_runtime (:class:`ModuleSystem'): the runtime to use when accessing student facing methods user_id: The user_id to set in scope_ids wrappers: These are a list functions that put a wrapper, such as LmsFieldData or OverrideFieldData, around the field_data. @@ -608,8 +619,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(xmodule_runtime, 'position', None): - self.position = xmodule_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. @@ -634,9 +645,6 @@ class XModuleMixin(XModuleFields, XBlock): if field in self._dirty_fields: del self._dirty_fields[field] - # Set the new xmodule_runtime and field_data (which are user-specific) - self.xmodule_runtime = xmodule_runtime - if wrappers is None: wrappers = [] @@ -1013,6 +1021,7 @@ class MetricsMixin: """ def render(self, block, view_name, context=None): # lint-amnesty, pylint: disable=missing-function-docstring + context = context or {} start_time = time.time() try: return super().render(block, view_name, context=context) @@ -1421,7 +1430,7 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemSh Base class for :class:`Runtime`s to be used with :class:`XModuleDescriptor`s """ def __init__( - self, load_item, resources_fs, error_tracker, get_policy=None, disabled_xblock_types=lambda: [], get_module=None, **kwargs + self, load_item, resources_fs, error_tracker, get_policy=None, disabled_xblock_types=lambda: [], **kwargs ): """ load_item: Takes a Location and returns an XModuleDescriptor @@ -1457,11 +1466,6 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemSh self.get_policy = lambda u: {} self.disabled_xblock_types = disabled_xblock_types - self.get_module = get_module - # self.handler_url_override = None - # self.applicable_aside_types_override = None - # self.wrap_asides_override = None - # self.layout_asides_override = None def get(self, attr): """ provide uniform access to attributes (like etree).""" @@ -1474,8 +1478,8 @@ 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) - if self.get_module: - return self.get_module(block) + if self.get_block_for_descriptor: + return self.get_block_for_descriptor(block) return block def load_block_type(self, block_type): @@ -1537,10 +1541,6 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemSh return self.applicable_aside_types_override(block, applicable_aside_types=super().applicable_aside_types) potential_set = set(super().applicable_aside_types(block)) - if getattr(block, 'xmodule_runtime', None) is not None: - if hasattr(block.xmodule_runtime, 'applicable_aside_types'): - application_set = set(block.xmodule_runtime.applicable_aside_types(block)) - return list(potential_set.intersection(application_set)) return list(potential_set) def resource_url(self, resource): @@ -1711,112 +1711,6 @@ class XMLParsingSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstr setattr(xblock, field.name, field_value) -class CombinedSystem: - """ - This class is a shim to allow both pure XBlocks and XModuleDescriptors - that have been bound as XModules to access both the attributes of ModuleSystem - and of DescriptorSystem as a single runtime. - """ - - __slots__ = ('_module_system', '_descriptor_system') - - # This system doesn't override a number of methods that are provided by ModuleSystem and DescriptorSystem, - # namely handler_url, local_resource_url, query, and resource_url. - # - # At runtime, the ModuleSystem and/or DescriptorSystem will define those methods - # - def __init__(self, module_system, descriptor_system): - # These attributes are set directly to __dict__ below to avoid a recursion in getattr/setattr. - self._module_system = module_system - self._descriptor_system = descriptor_system - - def render(self, block, view_name, context=None): - """ - Render a block by invoking its view. - - Finds the view named `view_name` on `block`. The default view will be - used if a specific view hasn't be registered. If there is no default - view, an exception will be raised. - - The view is invoked, passing it `context`. The value returned by the - view is returned, with possible modifications by the runtime to - integrate it into a larger whole. - - """ - context = context or {} - return self.__getattr__('render')(block, view_name, context) # pylint: disable=unnecessary-dunder-call - - def service(self, block, service_name): - """Return a service, or None. - - Services are objects implementing arbitrary other interfaces. They are - requested by agreed-upon names, see [XXX TODO] for a list of possible - services. The object returned depends on the service requested. - - XBlocks must announce their intention to request services with the - `XBlock.needs` or `XBlock.wants` decorators. Use `needs` if you assume - that the service is available, or `wants` if your code is flexible and - can accept a None from this method. - - Runtimes can override this method if they have different techniques for - finding and delivering services. - - Arguments: - block (an XBlock): this block's class will be examined for service - decorators. - service_name (string): the name of the service requested. - - Returns: - An object implementing the requested service, or None. - - """ - service = None - - if self._module_system: - service = self._module_system.service(block, service_name) - - if service is None: - service = self._descriptor_system.service(block, service_name) - - return service - - def __getattr__(self, name): - """ - If the ModuleSystem doesn't have an attribute, try returning the same attribute from the - DescriptorSystem, instead. This allows XModuleDescriptors that are bound as XModules - to still function as XModuleDescriptors. - """ - # First we try a lookup in the module system... - try: - return getattr(self._module_system, name) - except AttributeError: - return getattr(self._descriptor_system, name) - - def __setattr__(self, name, value): - """ - If the ModuleSystem is set, set the attr on it. - Always set the attr on the DescriptorSystem. - """ - if name in self.__slots__: - return super().__setattr__(name, value) - - if self._module_system: - setattr(self._module_system, name, value) - setattr(self._descriptor_system, name, value) - - def __delattr__(self, name): - """ - If the ModuleSystem is set, delete the attribute from it. - Always delete the attribute from the DescriptorSystem. - """ - if self._module_system: - delattr(self._module_system, name) - delattr(self._descriptor_system, name) - - def __repr__(self): - return f"CombinedSystem({self._module_system!r}, {self._descriptor_system!r})" - - class DoNothingCache: """A duck-compatible object to use in ModuleSystem when there's no cache.""" def get(self, _key):