From 92b684004e3813df0ece6fdc5acaebb5297b5fe1 Mon Sep 17 00:00:00 2001 From: Agrendalath Date: Thu, 13 Jul 2023 19:22:06 +0200 Subject: [PATCH] refactor: reuse services and wrappers between XBlocks (fixed) This re-applies commit 36cc415 with handling an invalid context_key in the `PartitionService`. It can happen when rendering a `LibraryContentBlock` in Studio because this service is initialized by the modulestore when validating an XBlock to gather its error messages in the `studio_xblock_wrapper`. --- 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/partitions/partitions_service.py | 4 +- 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 +-- 20 files changed, 259 insertions(+), 251 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 37c788463ae9..cdfe08966e3f 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._runtime_services.update(services) # lint-amnesty, pylint: disable=protected-access + block.runtime._services.update(services) # 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 973f9b774247..6280f9253734 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 User # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth.models import AnonymousUser, 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,6 +33,7 @@ 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 @@ -97,6 +98,13 @@ 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. @@ -289,8 +297,9 @@ 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 + - 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. - 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 @@ -364,10 +373,24 @@ def display_access_messages(user, block, view, frag, context): # pylint: disabl # pylint: disable=too-many-statements -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): +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: """ Implements get_block, extracting out the request-specific functionality. @@ -375,49 +398,107 @@ def get_block_for_descriptor(user, request, block, field_data_cache, course_key, See get_block() docstring for further details. """ - 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, + 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), + ], ) + # 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, - student_data, # TODO + user: User | AnonymousUser, + student_data: KvsFieldData, # Arguments preceding this comment have user binding, those following don't - 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, + 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, ): """ Helper function that binds the given xblock to a user and student_data to a user and the block. @@ -427,25 +508,26 @@ 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. Ultimately, this isn't too different than how get_block_for_descriptor_internal - was before refactoring. + are all the other arguments. Arguments: see arguments for get_block() request_token (str): A token unique to the request use by xblock initialization """ - def inner_get_block(block): + def inner_get_block(block: XBlock) -> XBlock | None: """ - Delegate to get_block_for_descriptor_internal() with all values except `block` set. + Delegate to get_block_for_descriptor() with all values except `block` set. Because it does an access check, it may return None. """ - return get_block_for_descriptor_internal( + return get_block_for_descriptor( user=user, + request=None, + field_data_cache=None, block=block, student_data=student_data, - course_id=course_id, + course_key=course_id, track_function=track_function, request_token=request_token, position=position, @@ -479,16 +561,6 @@ def inner_get_block(block): 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, @@ -543,7 +615,6 @@ def inner_get_block(block): 'rebind_user': RebindUserService( user, course_id, - prepare_runtime_for_user, track_function=track_function, position=position, wrap_xblock_display=wrap_xblock_display, @@ -566,86 +637,13 @@ def inner_get_block(block): 'publish': EventPublishingService(user, course_id, track_function), } - 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.get_block_for_descriptor = inner_get_block - # 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 + 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 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 1654196b47d2..80827afa632c 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 == 1 + assert mock_get_block.call_count == 2 assert mock_get_block.call_args[1]['will_recheck_access'] == will_recheck_access @@ -1922,14 +1922,16 @@ 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_internal( + rendered_block = render.get_block_for_descriptor( user=self.user, block=block, student_data=Mock(spec=FieldData, name='student_data'), - course_id=course_id, + course_key=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() @@ -2285,7 +2287,7 @@ def _prepare_runtime(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2439,7 +2441,6 @@ 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') @@ -2656,7 +2657,7 @@ def setUp(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2684,7 +2685,7 @@ def test_user_is_staff(self, is_staff, expected_role): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2706,7 +2707,7 @@ def test_user_is_admin(self, is_global_staff): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2726,7 +2727,7 @@ def test_user_is_beta_tester(self, is_beta_tester): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2747,7 +2748,7 @@ def test_get_user_role(self, is_instructor, expected_role): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2776,7 +2777,7 @@ def test_anonymous_student_id_bug(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.problem_block, + self.problem_block.runtime, self.course.id, self.track_function, self.request_token, @@ -2790,7 +2791,7 @@ def test_anonymous_student_id_bug(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2810,7 +2811,7 @@ def test_user_service_with_anonymous_user(self): render.prepare_runtime_for_user( AnonymousUser(), self.student_data, - self.block, + self.block.runtime, self.course.id, self.track_function, self.request_token, @@ -2839,7 +2840,7 @@ def test_get_real_user(self): render.prepare_runtime_for_user( self.user, self.student_data, - self.block, + self.block.runtime, 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 ea265daab764..d6b793bfb9a2 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_internal +from lms.djangoapps.courseware.block_render import get_block_for_descriptor 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,13 +293,15 @@ def test_html_with_user(self): """ Test rendered DiscussionXBlock permissions. """ - discussion_xblock = get_block_for_descriptor_internal( + discussion_xblock = get_block_for_descriptor( user=self.user, block=self.discussion, student_data=mock.Mock(name='student_data'), - course_id=self.course.id, + course_key=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') @@ -336,13 +338,15 @@ 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_internal( + discussion_xblock = get_block_for_descriptor( user=self.user, block=discussion, student_data=mock.Mock(name='student_data'), - course_id=self.course.id, + course_key=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') @@ -386,13 +390,15 @@ def test_discussion_xblock_visibility(self): provider_type=Provider.OPEN_EDX, ) - discussion_xblock = get_block_for_descriptor_internal( + discussion_xblock = get_block_for_descriptor( user=self.user, block=self.discussion, student_data=mock.Mock(name='student_data'), - course_id=self.course.id, + course_key=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') @@ -436,13 +442,15 @@ def test_permissions_query_load(self): num_queries = 6 for discussion in discussions: - discussion_xblock = get_block_for_descriptor_internal( + discussion_xblock = get_block_for_descriptor( user=user, block=discussion, student_data=mock.Mock(name='student_data'), - course_id=course.id, + course_key=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 d55495f4aa3f..3c2b946e91df 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -44,9 +44,8 @@ class RenderXBlockTestMixin(MasqueradeMixin, metaclass=ABCMeta): '
""") self.course_id = CourseKey.from_string('org/course/run') - self.system = get_test_system(self.course_id) - self.system.publish = Mock() - self.system._runtime_services['rebind_user'] = Mock() # pylint: disable=protected-access + self.runtime = get_test_system(self.course_id) + self.runtime.publish = Mock() + self.runtime._services['rebind_user'] = Mock() # pylint: disable=protected-access self.xblock = LTIBlock( - self.system, + self.runtime, DictFieldData({}), ScopeIds(None, None, None, BlockUsageLocator(self.course_id, 'lti', 'name')) ) - current_user = self.system.service(self.xblock, 'user').get_current_user() + current_user = self.runtime.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.system._runtime_services['user'] = StubUserService(user=None) # pylint: disable=protected-access + self.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 9a81ef32807a..c080be26516d 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._runtime_services['bookmarks'] = Mock() # pylint: disable=protected-access - block.runtime._runtime_services['user'] = StubUserService(user=Mock()) # pylint: disable=protected-access + block.runtime._services['bookmarks'] = Mock() # pylint: disable=protected-access + block.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._runtime_services['completion'] = Mock( # pylint: disable=protected-access + self.sequence_3_1.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._runtime_services['completion'] = None # pylint: disable=protected-access + self.sequence_3_1.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._runtime_services['completion'] = Mock( # pylint: disable=protected-access + self.sequence_3_1.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._runtime_services['completion'] = None # pylint: disable=protected-access + self.sequence_3_1.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 d87e5ae38918..bfecce065631 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._runtime_services['partitions'] = partitions_service # pylint: disable=protected-access + self.course.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 f4012125e259..1e8ababa912c 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._runtime_services['bookmarks'] = Mock() + self.course.runtime._services['bookmarks'] = Mock() now = datetime.now(pytz.UTC) self.vertical.due = now + timedelta(days=days) if view == STUDENT_VIEW: - self.course.runtime._runtime_services['user'] = StubUserService(user=Mock(username=self.username)) - self.course.runtime._runtime_services['completion'] = StubCompletionService( + self.course.runtime._services['user'] = StubUserService(user=Mock(username=self.username)) + self.course.runtime._services['completion'] = StubCompletionService( enabled=True, completion_value=completion_value ) elif view == PUBLIC_VIEW: - self.course.runtime._runtime_services['user'] = StubUserService(user=AnonymousUser()) + self.course.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 3d4a2801cd61..1c44b2cc0b8d 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._runtime_services.get('user') or self._services.get('user') + user_service = self._services.get('user') if user_service: return user_service.get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID) return None @@ -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._runtime_services.get('user') or self._services.get('user') + user_service = 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._runtime_services.get('user') or self._services.get('user') + user_service = 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._runtime_services.get('user') or self._services.get('user') + user_service = 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._runtime_services.get('user') or self._services.get('user') + user_service = 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._runtime_services.get('user') or self._services.get('user') + user_service = 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._runtime_services.get('user') or self._services.get('user') + user_service = 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._runtime_services.get('user') or self._services.get('user') + user_service = 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._runtime_services.get('mako') or self._services.get('mako') + render_service = 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._runtime_services.get('sandbox') or self._services.get('sandbox') + sandbox_service = 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._runtime_services.get('sandbox') or self._services.get('sandbox') + sandbox_service = 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._runtime_services.get('cache') or self._services.get('cache') or DoNothingCache() + return 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._runtime_services.get('rebind_user') or self._services.get('rebind_user') + rebind_user_service = self._services.get('rebind_user') if rebind_user_service: return partial(rebind_user_service.rebind_noauth_module_to_user) @@ -1421,7 +1421,6 @@ 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).""" @@ -1519,7 +1518,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._runtime_services.get('publish') or self._services.get('publish'): + if publish_service := self._services.get('publish'): publish_service.publish(block, event_type, event) def service(self, block, service_name): @@ -1536,11 +1535,8 @@ def service(self, block, service_name): Returns: An object implementing the requested service, or None. """ - 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) + # 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):