From 06e4c61e37c19ee5e44f2193c1a074df16c24b81 Mon Sep 17 00:00:00 2001 From: Agrendalath Date: Thu, 13 Jul 2023 13:40:40 +0200 Subject: [PATCH] Revert "refactor: reuse services and wrappers between XBlocks" This reverts commit 36cc415fc26d9506194d288ed181357472a1624e. --- cms/djangoapps/contentstore/views/preview.py | 2 +- lms/djangoapps/courseware/block_render.py | 282 +++++++++--------- .../courseware/tests/test_block_render.py | 29 +- .../tests/test_discussion_xblock.py | 26 +- lms/djangoapps/courseware/testutils.py | 18 +- lms/djangoapps/courseware/views/views.py | 10 +- .../tasks_helper/module_state.py | 24 +- .../instructor_task/tests/test_tasks.py | 12 +- .../lti_provider/tests/test_views.py | 12 - lms/djangoapps/lti_provider/views.py | 2 +- .../core/djangoapps/xblock/runtime/runtime.py | 2 + xmodule/services.py | 13 +- xmodule/tests/test_library_content.py | 2 +- xmodule/tests/test_lti20_unit.py | 2 +- xmodule/tests/test_lti_unit.py | 12 +- xmodule/tests/test_sequence.py | 12 +- xmodule/tests/test_split_test_block.py | 2 +- xmodule/tests/test_vertical.py | 8 +- xmodule/x_module.py | 36 ++- 19 files changed, 250 insertions(+), 256 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index cdfe08966e3f..37c788463ae9 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -220,7 +220,7 @@ def _prepare_runtime_for_preview(request, block): # Set up functions to modify the fragment produced by student_view block.runtime.wrappers = wrappers block.runtime.wrappers_asides = wrappers_asides - block.runtime._services.update(services) # pylint: disable=protected-access + 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) diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 6280f9253734..973f9b774247 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -2,7 +2,7 @@ Block rendering """ -from __future__ import annotations + import json import logging import textwrap @@ -12,7 +12,7 @@ from completion.services import CompletionService from django.conf import settings -from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.cache import cache from django.db import transaction from django.http import Http404, HttpResponse, HttpResponseForbidden @@ -33,7 +33,6 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from rest_framework.decorators import api_view from rest_framework.exceptions import APIException -from typing import Callable, TYPE_CHECKING from web_fragments.fragment import Fragment from xblock.django.request import django_to_webob_request, webob_to_django_response from xblock.exceptions import NoSuchHandlerError, NoSuchViewError @@ -98,13 +97,6 @@ from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from openedx.core.lib.cache_utils import CacheService -if TYPE_CHECKING: - from rest_framework.request import Request - from xblock.core import XBlock - from xblock.runtime import Runtime - - from xmodule.course_block import CourseBlock - log = logging.getLogger(__name__) # TODO: course_id and course_key are used interchangeably in this file, which is wrong. @@ -297,9 +289,8 @@ def get_block(user, request, usage_key, field_data_cache, position=None, log_if_ and such works based on user. - usage_key : A UsageKey object identifying the module to load - field_data_cache : a FieldDataCache - - position : Extra information from URL for user-specified position within module. - It is used to determine the active tab within the `SequenceBlock`/subsection. - Once the legacy course experience is removed, it should be safe to remove this, too. + - position : extra information from URL for user-specified + position within module - log_if_not_found : If this is True, we log a debug message if we cannot find the requested xmodule. - wrap_xblock_display : If this is True, wrap the output display in a single div to allow for the XModule javascript to be bound correctly @@ -373,24 +364,10 @@ def display_access_messages(user, block, view, frag, context): # pylint: disabl # pylint: disable=too-many-statements -def get_block_for_descriptor( - user: User | AnonymousUser, - request: Request | None, - block: XBlock, - field_data_cache: FieldDataCache | None, - course_key: CourseKey, - position: int | None = None, - wrap_xblock_display: bool = True, - grade_bucket_type: str | None = None, - static_asset_path: str = '', - disable_staff_debug_info: bool = False, - course: CourseBlock | None = None, - will_recheck_access: bool = False, - track_function: Callable[[str, dict], None] | None = None, - student_data: KvsFieldData | None = None, - request_token: str | None = None, - user_location: str | None = None, -) -> XBlock | None: +def get_block_for_descriptor(user, request, block, field_data_cache, course_key, + position=None, wrap_xblock_display=True, grade_bucket_type=None, + static_asset_path='', disable_staff_debug_info=False, + course=None, will_recheck_access=False): """ Implements get_block, extracting out the request-specific functionality. @@ -398,107 +375,49 @@ def get_block_for_descriptor( See get_block() docstring for further details. """ - if request: - track_function = track_function or make_track_function(request) - user_location = user_location or getattr(request, 'session', {}).get('country_code') - request_token = request_token or xblock_request_token(request) - - if not student_data: - student_kvs = DjangoKeyValueStore(field_data_cache) - if is_masquerading_as_specific_student(user, course_key): - student_kvs = MasqueradingKeyValueStore(student_kvs, request.session) - student_data = KvsFieldData(student_kvs) - - # The runtime is already shared between XBlocks. If there are no wrappers, it is the first initialization. - should_recreate_runtime = not block.runtime.wrappers - - # If the runtime was prepared for another user, it should be recreated. - # This part can be removed if we remove all user-specific handling from the runtime services and pull this - # information directly from XBlocks during the service initialization. - if not should_recreate_runtime: - # If the user service is absent (which should never happen), the runtime should be reinitialized. - # We retrieve this service directly to bypass service declaration checks. - if user_service := block.runtime._services.get('user'): # pylint: disable=protected-access - # Check the user ID bound to the runtime. This operation can run often for complex course structures, so we - # are accessing the protected attribute of the user service to reduce the number of queries. - should_recreate_runtime = user.id != user_service._django_user.id # pylint: disable=protected-access - else: - should_recreate_runtime = True - - if should_recreate_runtime: - prepare_runtime_for_user( - user=user, - student_data=student_data, # These have implicit user bindings, the rest of args are considered not to - runtime=block.runtime, - course_id=course_key, - track_function=track_function, - wrap_xblock_display=wrap_xblock_display, - grade_bucket_type=grade_bucket_type, - static_asset_path=static_asset_path, - user_location=user_location, - request_token=request_token, - disable_staff_debug_info=disable_staff_debug_info, - course=course, - will_recheck_access=will_recheck_access, - ) - - # Pass position specified in URL to runtime. - if position is not None: - try: - position = int(position) - except (ValueError, TypeError): - log.exception('Non-integer %r passed as position.', position) - position = None - - block.runtime.set('position', position) - - block.bind_for_student( - user.id, - [ - partial(DateLookupFieldData, course_id=course_key, user=user), - partial(OverrideFieldData.wrap, user, course), - partial(LmsFieldData, student_data=student_data), - ], + track_function = make_track_function(request) + + user_location = getattr(request, 'session', {}).get('country_code') + + student_kvs = DjangoKeyValueStore(field_data_cache) + if is_masquerading_as_specific_student(user, course_key): + student_kvs = MasqueradingKeyValueStore(student_kvs, request.session) + student_data = KvsFieldData(student_kvs) + + return get_block_for_descriptor_internal( + user=user, + block=block, + student_data=student_data, + course_id=course_key, + track_function=track_function, + position=position, + wrap_xblock_display=wrap_xblock_display, + grade_bucket_type=grade_bucket_type, + static_asset_path=static_asset_path, + user_location=user_location, + request_token=xblock_request_token(request), + disable_staff_debug_info=disable_staff_debug_info, + course=course, + will_recheck_access=will_recheck_access, ) - # Do not check access when it's a noauth request. - # Not that the access check needs to happen after the block is bound - # for the student, since there may be field override data for the student - # that affects xblock visibility. - user_needs_access_check = getattr(user, 'known', True) and not isinstance(user, SystemUser) - if user_needs_access_check: - access = has_access(user, 'load', block, course_key) - # A block should only be returned if either the user has access, or the user doesn't have access, but - # the failed access has a message for the user and the caller of this function specifies it will check access - # again. This allows blocks to show specific error message or upsells when access is denied. - caller_will_handle_access_error = ( - not access - and will_recheck_access - and (access.user_message or access.user_fragment) - ) - if access or caller_will_handle_access_error: - block.has_access_error = bool(caller_will_handle_access_error) - return block - return None - return block - def prepare_runtime_for_user( - user: User | AnonymousUser, - student_data: KvsFieldData, + user, + student_data, # TODO # Arguments preceding this comment have user binding, those following don't - runtime: Runtime, - course_id: CourseKey, - track_function: Callable[[str, dict], None], - request_token: str, - position: int | None = None, - wrap_xblock_display: bool = True, - grade_bucket_type: str | None = None, - static_asset_path: str = '', - user_location: str | None = None, - disable_staff_debug_info: bool = False, - course: CourseBlock | None = None, - will_recheck_access: bool = False, + block, + course_id, + track_function, + request_token, + position=None, + wrap_xblock_display=True, + grade_bucket_type=None, + static_asset_path='', + user_location=None, + disable_staff_debug_info=False, + course=None, + will_recheck_access=False, ): """ Helper function that binds the given xblock to a user and student_data to a user and the block. @@ -508,26 +427,25 @@ def prepare_runtime_for_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 used to instantiate the service required in LMS, which - are all the other arguments. + are all the other arguments. Ultimately, this isn't too different than how get_block_for_descriptor_internal + was before refactoring. Arguments: see arguments for get_block() request_token (str): A token unique to the request use by xblock initialization """ - def inner_get_block(block: XBlock) -> XBlock | None: + def inner_get_block(block): """ - Delegate to get_block_for_descriptor() with all values except `block` set. + Delegate to get_block_for_descriptor_internal() with all values except `block` set. Because it does an access check, it may return None. """ - return get_block_for_descriptor( + return get_block_for_descriptor_internal( user=user, - request=None, - field_data_cache=None, block=block, student_data=student_data, - course_key=course_id, + course_id=course_id, track_function=track_function, request_token=request_token, position=position, @@ -561,6 +479,16 @@ def inner_get_block(block: XBlock) -> XBlock | None: request_token=request_token, )) + # HACK: The following test fails when we do not access this attribute. + # lms/djangoapps/courseware/tests/test_views.py::TestRenderXBlock::test_success_enrolled_staff + # This happens because accessing `field_data` caches the XBlock's parents through the inheritance mixin. + # If we do this operation before assigning `inner_get_block` to the `get_block_for_descriptor` attribute, these + # parents will be initialized without binding the user data (that's how the `get_block` works in `x_module.py`). + # If we retrieve the parents after this operation (e.g., in the `enclosing_sequence_for_gating_checks` function), + # they will have their runtimes initialized, which can lead to an altered behavior of the XBlock-specific + # parts of other runtimes. We will keep this workaround until we remove all XBlock-specific code from here. + block.static_asset_path # pylint: disable=pointless-statement + replace_url_service = partial( ReplaceURLService, static_asset_path=static_asset_path, @@ -615,6 +543,7 @@ def inner_get_block(block: XBlock) -> XBlock | None: 'rebind_user': RebindUserService( user, course_id, + prepare_runtime_for_user, track_function=track_function, position=position, wrap_xblock_display=wrap_xblock_display, @@ -637,13 +566,86 @@ def inner_get_block(block: XBlock) -> XBlock | None: 'publish': EventPublishingService(user, course_id, track_function), } - runtime.get_block_for_descriptor = inner_get_block + block.runtime.get_block_for_descriptor = inner_get_block + + 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 runtime + if position is not None: + try: + position = int(position) + except (ValueError, TypeError): + log.exception('Non-integer %r passed as position.', position) + position = None + + block.runtime.set('position', position) + + +# TODO: Find all the places that this method is called and figure out how to +# get a loaded course passed into it +def get_block_for_descriptor_internal(user, block, student_data, course_id, track_function, request_token, + position=None, wrap_xblock_display=True, grade_bucket_type=None, + static_asset_path='', user_location=None, disable_staff_debug_info=False, + course=None, will_recheck_access=False): + """ + Actually implement get_block, without requiring a request. + + See get_block() docstring for further details. + + Arguments: + request_token (str): A unique token for this request, used to isolate xblock rendering + """ + + prepare_runtime_for_user( + user=user, + student_data=student_data, # These have implicit user bindings, the rest of args are considered not to + block=block, + course_id=course_id, + track_function=track_function, + position=position, + wrap_xblock_display=wrap_xblock_display, + grade_bucket_type=grade_bucket_type, + static_asset_path=static_asset_path, + user_location=user_location, + request_token=request_token, + disable_staff_debug_info=disable_staff_debug_info, + course=course, + will_recheck_access=will_recheck_access, + ) + + block.bind_for_student( + user.id, + [ + partial(DateLookupFieldData, course_id=course_id, user=user), + partial(OverrideFieldData.wrap, user, course), + partial(LmsFieldData, student_data=student_data), + ], + ) - runtime.wrappers = block_wrappers - runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access - runtime.request_token = request_token - runtime.wrap_asides_override = lms_wrappers_aside - runtime.applicable_aside_types_override = lms_applicable_aside_types + # Do not check access when it's a noauth request. + # Not that the access check needs to happen after the block is bound + # for the student, since there may be field override data for the student + # that affects xblock visibility. + user_needs_access_check = getattr(user, 'known', True) and not isinstance(user, SystemUser) + if user_needs_access_check: + access = has_access(user, 'load', block, course_id) + # A block should only be returned if either the user has access, or the user doesn't have access, but + # the failed access has a message for the user and the caller of this function specifies it will check access + # again. This allows blocks to show specific error message or upsells when access is denied. + caller_will_handle_access_error = ( + not access + and will_recheck_access + and (access.user_message or access.user_fragment) + ) + if access or caller_will_handle_access_error: + block.has_access_error = bool(caller_will_handle_access_error) + return block + return None + return block def load_single_xblock(request, user_id, course_id, usage_key_string, course=None, will_recheck_access=False): diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index 80827afa632c..1654196b47d2 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -949,7 +949,7 @@ def test_will_recheck_access_handler_attribute(self, handler, will_recheck_acces request.user = self.mock_user render.handle_xblock_callback(request, str(course.id), usage_id, handler) - assert mock_get_block.call_count == 2 + assert mock_get_block.call_count == 1 assert mock_get_block.call_args[1]['will_recheck_access'] == will_recheck_access @@ -1922,16 +1922,14 @@ def _get_anonymous_id(self, course_id, xblock_class, should_get_deprecated_id: b if hasattr(xblock_class, 'module_class'): block.module_class = xblock_class.module_class - rendered_block = render.get_block_for_descriptor( + rendered_block = render.get_block_for_descriptor_internal( user=self.user, block=block, student_data=Mock(spec=FieldData, name='student_data'), - course_key=course_id, + course_id=course_id, track_function=Mock(name='track_function'), # Track Function request_token='request_token', course=self.course, - request=None, - field_data_cache=None, ) current_user = rendered_block.runtime.service(rendered_block, 'user').get_current_user() @@ -2287,7 +2285,7 @@ def _prepare_runtime(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2441,6 +2439,7 @@ def test_no_service_exception_(self): Test: NoSuchServiceError should be raised if i18n service is none. """ i18nService = self.block.runtime._services['i18n'] # pylint: disable=protected-access + self.block.runtime._runtime_services['i18n'] = None # pylint: disable=protected-access self.block.runtime._services['i18n'] = None # pylint: disable=protected-access with pytest.raises(NoSuchServiceError): self.block.runtime.service(self.block, 'i18n') @@ -2657,7 +2656,7 @@ def setUp(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2685,7 +2684,7 @@ def test_user_is_staff(self, is_staff, expected_role): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2707,7 +2706,7 @@ def test_user_is_admin(self, is_global_staff): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2727,7 +2726,7 @@ def test_user_is_beta_tester(self, is_beta_tester): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2748,7 +2747,7 @@ def test_get_user_role(self, is_instructor, expected_role): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2777,7 +2776,7 @@ def test_anonymous_student_id_bug(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.problem_block.runtime, + self.problem_block, self.course.id, self.track_function, self.request_token, @@ -2791,7 +2790,7 @@ def test_anonymous_student_id_bug(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2811,7 +2810,7 @@ def test_user_service_with_anonymous_user(self): render.prepare_runtime_for_user( AnonymousUser(), self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, @@ -2840,7 +2839,7 @@ def test_get_real_user(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block.runtime, + self.block, self.course.id, self.track_function, self.request_token, diff --git a/lms/djangoapps/courseware/tests/test_discussion_xblock.py b/lms/djangoapps/courseware/tests/test_discussion_xblock.py index d6b793bfb9a2..ea265daab764 100644 --- a/lms/djangoapps/courseware/tests/test_discussion_xblock.py +++ b/lms/djangoapps/courseware/tests/test_discussion_xblock.py @@ -21,7 +21,7 @@ from xmodule.modulestore.tests.factories import BlockFactory, ToyCourseFactory from lms.djangoapps.course_api.blocks.tests.helpers import deserialize_usage_key -from lms.djangoapps.courseware.block_render import get_block_for_descriptor +from lms.djangoapps.courseware.block_render import get_block_for_descriptor_internal from lms.djangoapps.courseware.tests.helpers import XModuleRenderingTestBase from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory @@ -293,15 +293,13 @@ def test_html_with_user(self): """ Test rendered DiscussionXBlock permissions. """ - discussion_xblock = get_block_for_descriptor( + discussion_xblock = get_block_for_descriptor_internal( user=self.user, block=self.discussion, student_data=mock.Mock(name='student_data'), - course_key=self.course.id, + course_id=self.course.id, track_function=mock.Mock(name='track_function'), request_token='request_token', - request=None, - field_data_cache=None, ) fragment = discussion_xblock.render('student_view') @@ -338,15 +336,13 @@ def test_discussion_render_successfully_with_orphan_parent(self): assert orphan_sequential.location.block_id == root.location.block_id # Get xblock bound to a user and a block. - discussion_xblock = get_block_for_descriptor( + discussion_xblock = get_block_for_descriptor_internal( user=self.user, block=discussion, student_data=mock.Mock(name='student_data'), - course_key=self.course.id, + course_id=self.course.id, track_function=mock.Mock(name='track_function'), request_token='request_token', - request=None, - field_data_cache=None, ) fragment = discussion_xblock.render('student_view') @@ -390,15 +386,13 @@ def test_discussion_xblock_visibility(self): provider_type=Provider.OPEN_EDX, ) - discussion_xblock = get_block_for_descriptor( + discussion_xblock = get_block_for_descriptor_internal( user=self.user, block=self.discussion, student_data=mock.Mock(name='student_data'), - course_key=self.course.id, + course_id=self.course.id, track_function=mock.Mock(name='track_function'), request_token='request_token', - request=None, - field_data_cache=None, ) fragment = discussion_xblock.render('student_view') @@ -442,15 +436,13 @@ def test_permissions_query_load(self): num_queries = 6 for discussion in discussions: - discussion_xblock = get_block_for_descriptor( + discussion_xblock = get_block_for_descriptor_internal( user=user, block=discussion, student_data=mock.Mock(name='student_data'), - course_key=course.id, + course_id=course.id, track_function=mock.Mock(name='track_function'), request_token='request_token', - request=None, - field_data_cache=None, ) with self.assertNumQueries(num_queries): fragment = discussion_xblock.render('student_view') diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index 3c2b946e91df..d55495f4aa3f 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -44,8 +44,9 @@ class RenderXBlockTestMixin(MasqueradeMixin, metaclass=ABCMeta): '
""") self.course_id = CourseKey.from_string('org/course/run') - self.runtime = get_test_system(self.course_id) - self.runtime.publish = Mock() - self.runtime._services['rebind_user'] = Mock() # pylint: disable=protected-access + self.system = get_test_system(self.course_id) + self.system.publish = Mock() + self.system._runtime_services['rebind_user'] = Mock() # pylint: disable=protected-access self.xblock = LTIBlock( - self.runtime, + self.system, DictFieldData({}), ScopeIds(None, None, None, BlockUsageLocator(self.course_id, 'lti', 'name')) ) - current_user = self.runtime.service(self.xblock, 'user').get_current_user() + current_user = self.system.service(self.xblock, 'user').get_current_user() self.user_id = current_user.opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID) self.lti_id = self.xblock.lti_id @@ -178,7 +178,7 @@ def test_real_user_is_none(self): """ If we have no real user, we should send back failure response. """ - self.runtime._services['user'] = StubUserService(user=None) # pylint: disable=protected-access + self.system._runtime_services['user'] = StubUserService(user=None) # pylint: disable=protected-access self.xblock.verify_oauth_body_sign = Mock() self.xblock.has_score = True request = Request(self.environ) diff --git a/xmodule/tests/test_sequence.py b/xmodule/tests/test_sequence.py index c080be26516d..9a81ef32807a 100644 --- a/xmodule/tests/test_sequence.py +++ b/xmodule/tests/test_sequence.py @@ -94,8 +94,8 @@ def _set_up_block(self, parent, index_in_parent): self._set_up_module_system(block) - block.runtime._services['bookmarks'] = Mock() # pylint: disable=protected-access - block.runtime._services['user'] = StubUserService(user=Mock()) # pylint: disable=protected-access + block.runtime._runtime_services['bookmarks'] = Mock() # pylint: disable=protected-access + block.runtime._runtime_services['user'] = StubUserService(user=Mock()) # pylint: disable=protected-access block.parent = parent.location return block @@ -368,7 +368,7 @@ def test_gated_content(self): def test_xblock_handler_get_completion_success(self): """Test that the completion data is returned successfully on targeted vertical through ajax call""" - self.sequence_3_1.runtime._services['completion'] = Mock( # pylint: disable=protected-access + self.sequence_3_1.runtime._runtime_services['completion'] = Mock( # pylint: disable=protected-access return_value=Mock(vertical_is_complete=Mock(return_value=True)) ) for child in self.sequence_3_1.get_children(): @@ -380,7 +380,7 @@ def test_xblock_handler_get_completion_success(self): ) completion_return = self.sequence_3_1.handle('get_completion', request) assert completion_return.json == {'complete': True} - self.sequence_3_1.runtime._services['completion'] = None # pylint: disable=protected-access + self.sequence_3_1.runtime._runtime_services['completion'] = None # pylint: disable=protected-access def test_xblock_handler_get_completion_bad_key(self): """Test that the completion data is returned as False when usage key is None through ajax call""" @@ -394,14 +394,14 @@ def test_xblock_handler_get_completion_bad_key(self): def test_handle_ajax_get_completion_success(self): """Test that the old-style ajax handler for completion still works""" - self.sequence_3_1.runtime._services['completion'] = Mock( # pylint: disable=protected-access + self.sequence_3_1.runtime._runtime_services['completion'] = Mock( # pylint: disable=protected-access return_value=Mock(vertical_is_complete=Mock(return_value=True)) ) for child in self.sequence_3_1.get_children(): usage_key = str(child.location) completion_return = self.sequence_3_1.handle_ajax('get_completion', {'usage_key': usage_key}) assert json.loads(completion_return) == {'complete': True} - self.sequence_3_1.runtime._services['completion'] = None # pylint: disable=protected-access + self.sequence_3_1.runtime._runtime_services['completion'] = None # pylint: disable=protected-access def test_xblock_handler_goto_position_success(self): """Test that we can set position through ajax call""" diff --git a/xmodule/tests/test_split_test_block.py b/xmodule/tests/test_split_test_block.py index bfecce065631..d87e5ae38918 100644 --- a/xmodule/tests/test_split_test_block.py +++ b/xmodule/tests/test_split_test_block.py @@ -106,7 +106,7 @@ def setUp(self): self.course, course_id=self.course.id, ) - self.course.runtime._services['partitions'] = partitions_service # pylint: disable=protected-access + self.course.runtime._runtime_services['partitions'] = partitions_service # pylint: disable=protected-access # Mock user_service user user_service = Mock() diff --git a/xmodule/tests/test_vertical.py b/xmodule/tests/test_vertical.py index 1e8ababa912c..f4012125e259 100644 --- a/xmodule/tests/test_vertical.py +++ b/xmodule/tests/test_vertical.py @@ -212,17 +212,17 @@ def test_render_student_preview_view(self, context, view, completion_value, days """ Test the rendering of the student and public view. """ - self.course.runtime._services['bookmarks'] = Mock() + self.course.runtime._runtime_services['bookmarks'] = Mock() now = datetime.now(pytz.UTC) self.vertical.due = now + timedelta(days=days) if view == STUDENT_VIEW: - self.course.runtime._services['user'] = StubUserService(user=Mock(username=self.username)) - self.course.runtime._services['completion'] = StubCompletionService( + self.course.runtime._runtime_services['user'] = StubUserService(user=Mock(username=self.username)) + self.course.runtime._runtime_services['completion'] = StubCompletionService( enabled=True, completion_value=completion_value ) elif view == PUBLIC_VIEW: - self.course.runtime._services['user'] = StubUserService(user=AnonymousUser()) + self.course.runtime._runtime_services['user'] = StubUserService(user=AnonymousUser()) html = self.course.runtime.render( self.vertical, view, self.default_context if context is None else context diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 1c44b2cc0b8d..3d4a2801cd61 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1077,7 +1077,7 @@ def anonymous_student_id(self): 'runtime.anonymous_student_id is deprecated. Please use the user service instead.', DeprecationWarning, stacklevel=3, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return user_service.get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID) return None @@ -1107,7 +1107,7 @@ def user_id(self): 'runtime.user_id is deprecated. Use block.scope_ids.user_id or the user service instead.', DeprecationWarning, stacklevel=2, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_ID) return None @@ -1123,7 +1123,7 @@ def user_is_staff(self): 'runtime.user_is_staff is deprecated. Please use the user service instead.', DeprecationWarning, stacklevel=2, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) return None @@ -1139,7 +1139,7 @@ def user_location(self): 'runtime.user_location is deprecated. Please use the user service instead.', DeprecationWarning, stacklevel=2, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return user_service.get_current_user().opt_attrs.get(ATTR_KEY_REQUEST_COUNTRY_CODE) return None @@ -1159,7 +1159,7 @@ def get_real_user(self): 'runtime.get_real_user is deprecated. Please use the user service instead.', DeprecationWarning, stacklevel=2, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return user_service.get_user_by_anonymous_id return None @@ -1177,7 +1177,7 @@ def get_user_role(self): 'runtime.get_user_role is deprecated. Please use the user service instead.', DeprecationWarning, stacklevel=2, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return partial(user_service.get_current_user().opt_attrs.get, ATTR_KEY_USER_ROLE) @@ -1192,7 +1192,7 @@ def user_is_beta_tester(self): 'runtime.user_is_beta_tester is deprecated. Please use the user service instead.', DeprecationWarning, stacklevel=2, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_BETA_TESTER) @@ -1207,7 +1207,7 @@ def user_is_admin(self): 'runtime.user_is_admin is deprecated. Please use the user service instead.', DeprecationWarning, stacklevel=2, ) - user_service = self._services.get('user') + user_service = self._runtime_services.get('user') or self._services.get('user') if user_service: return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_GLOBAL_STAFF) @@ -1225,7 +1225,7 @@ def render_template(self): ) if hasattr(self, '_deprecated_render_template'): return self._deprecated_render_template - render_service = self._services.get('mako') + render_service = self._runtime_services.get('mako') or self._services.get('mako') if render_service: return render_service.render_template return None @@ -1251,7 +1251,7 @@ def can_execute_unsafe_code(self): 'runtime.can_execute_unsafe_code is deprecated. Please use the sandbox service instead.', DeprecationWarning, stacklevel=2, ) - sandbox_service = self._services.get('sandbox') + sandbox_service = self._runtime_services.get('sandbox') or self._services.get('sandbox') if sandbox_service: return sandbox_service.can_execute_unsafe_code # Default to saying "no unsafe code". @@ -1271,7 +1271,7 @@ def get_python_lib_zip(self): 'runtime.get_python_lib_zip is deprecated. Please use the sandbox service instead.', DeprecationWarning, stacklevel=2, ) - sandbox_service = self._services.get('sandbox') + sandbox_service = self._runtime_services.get('sandbox') or self._services.get('sandbox') if sandbox_service: return sandbox_service.get_python_lib_zip # Default to saying "no lib data" @@ -1290,7 +1290,7 @@ def cache(self): 'runtime.cache is deprecated. Please use the cache service instead.', DeprecationWarning, stacklevel=2, ) - return self._services.get('cache') or DoNothingCache() + return self._runtime_services.get('cache') or self._services.get('cache') or DoNothingCache() @property def filestore(self): @@ -1341,7 +1341,7 @@ def rebind_noauth_module_to_user(self): "rebind_noauth_module_to_user is deprecated. Please use the 'rebind_user' service instead.", DeprecationWarning, stacklevel=2, ) - rebind_user_service = self._services.get('rebind_user') + rebind_user_service = self._runtime_services.get('rebind_user') or self._services.get('rebind_user') if rebind_user_service: return partial(rebind_user_service.rebind_noauth_module_to_user) @@ -1421,6 +1421,7 @@ def __init__( self.get_policy = lambda u: {} self.disabled_xblock_types = disabled_xblock_types + self._runtime_services = {} def get(self, attr): """ provide uniform access to attributes (like etree).""" @@ -1518,7 +1519,7 @@ def publish(self, block, event_type, event): # lint-amnesty, pylint: disable=ar Publish events through the `EventPublishingService`. This ensures that the correct track method is used for Instructor tasks. """ - if publish_service := self._services.get('publish'): + if publish_service := self._runtime_services.get('publish') or self._services.get('publish'): publish_service.publish(block, event_type, event) def service(self, block, service_name): @@ -1535,8 +1536,11 @@ def service(self, block, service_name): Returns: An object implementing the requested service, or None. """ - # Getting the service from parent module. making sure of block service declarations. - service = super().service(block=block, service_name=service_name) + declaration = block.service_declaration(service_name) + service = self._runtime_services.get(service_name) + if declaration is None or service is None: + # getting the service from parent module. making sure of block service declarations. + service = super().service(block=block, service_name=service_name) # Passing the block to service if it is callable e.g. XBlockI18nService. It is the responsibility of calling # service to handle the passing argument. if callable(service):