From 89f369cf0b621c5221162c970217e5ef0b23e3d9 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 10 Oct 2024 23:31:11 -0700 Subject: [PATCH] feat: call XBlock handlers using the same version of the block --- openedx/core/djangoapps/xblock/api.py | 21 ++++++++-- .../core/djangoapps/xblock/rest_api/urls.py | 2 +- .../core/djangoapps/xblock/rest_api/views.py | 42 ++++++++++++------- .../xblock/runtime/learning_core_runtime.py | 7 ++++ .../core/djangoapps/xblock/runtime/runtime.py | 25 ++++++++--- 5 files changed, 72 insertions(+), 25 deletions(-) diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 6125343aa3c6..aef8314ff3bd 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -248,7 +248,13 @@ def render_block_view(block, view_name, user): # pylint: disable=unused-argumen return fragment -def get_handler_url(usage_key, handler_name, user): +def get_handler_url( + usage_key: UsageKeyV2, + handler_name: str, + user: UserType, + *, + version: int | LatestVersion = LatestVersion.AUTO, +): """ A method for getting the URL to any XBlock handler. The URL must be usable without any authentication (no cookie, no OAuth/JWT), and may expire. (So @@ -265,6 +271,11 @@ def get_handler_url(usage_key, handler_name, user): usage_key - Usage Key (Opaque Key object or string) handler_name - Name of the handler or a dummy name like 'any_handler' user - Django User (registered or anonymous) + version - Run the handler against a specific version of the + block (e.g. when viewing an old version of it in + Studio). Some blocks use handlers to load their data + so it's important the handler matches the student_view + etc. This view does not check/care if the XBlock actually exists. """ @@ -282,12 +293,16 @@ def get_handler_url(usage_key, handler_name, user): # and this XBlock: secure_token = get_secure_token_for_xblock_handler(user_id, usage_key_str) # Now generate the URL to that handler: - path = reverse('xblock_api:xblock_handler', kwargs={ + kwargs = { 'usage_key_str': usage_key_str, 'user_id': user_id, 'secure_token': secure_token, 'handler_name': handler_name, - }) + } + path = reverse('xblock_api:xblock_handler', kwargs=kwargs) + if version != LatestVersion.AUTO: + path += "?version=" + (str(version) if isinstance(version, int) else version.value) + # We must return an absolute URL. We can't just use # rest_framework.reverse.reverse to get the absolute URL because this method # can be called by the XBlock from python as well and in that case we don't diff --git a/openedx/core/djangoapps/xblock/rest_api/urls.py b/openedx/core/djangoapps/xblock/rest_api/urls.py index dec2fc562fee..6432291178ec 100644 --- a/openedx/core/djangoapps/xblock/rest_api/urls.py +++ b/openedx/core/djangoapps/xblock/rest_api/urls.py @@ -32,6 +32,6 @@ path('xblocks/v2//', include([ # render one of this XBlock's views (e.g. student_view) for embedding in an iframe # NOTE: this endpoint is **unstable** and subject to changes after Sumac - re_path(r'^embed/(?P[\w\-]+)/$', views.embed_block_view), + path('embed//', views.embed_block_view), ])), ] diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 3edd2bb7a594..147b1d21dae7 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -44,6 +44,23 @@ invalid_not_found_fmt = "XBlock {usage_key} does not exist, or you don't have permission to view it." +def parse_version_request(version_str: str | None) -> LatestVersion | int: + """ + Given a version parameter from a query string (?version=14, ?version=draft, + ?version=published), get the LatestVersion parameter to use with the API. + """ + if version_str is None: + return LatestVersion.AUTO # AUTO = published if we're in the LMS, draft if we're in Studio. + if version_str == "draft": + return LatestVersion.DRAFT + if version_str == "published": + return LatestVersion.PUBLISHED + try: + return int(version_str) + except ValueError: + raise serializers.ValidationError("Invalid version specifier. Expected 'draft', 'published', or an integer.") + + @api_view(['GET']) @view_auth_classes(is_authenticated=False) @permission_classes((permissions.AllowAny, )) # Permissions are handled at a lower level, by the learning context @@ -110,19 +127,7 @@ def embed_block_view(request, usage_key_str, view_name): raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e # Check if a specific version has been requested - version = LatestVersion.AUTO # AUTO = published if we're in the LMS, draft if we're in Studio. - if version_request := request.GET.get("version"): - if version_request == "draft": - version = LatestVersion.DRAFT - elif version_request == "published": - version = LatestVersion.PUBLISHED - else: - try: - version = int(version_request) - except ValueError: - raise serializers.ValidationError( - "Invalid version specifier. Expected 'draft', 'published', or an integer." - ) + version = parse_version_request(request.GET.get("version")) try: block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_LEARN, version=version) @@ -131,9 +136,13 @@ def embed_block_view(request, usage_key_str, view_name): fragment = _render_block_view(block, view_name, request.user) handler_urls = { - str(key): _get_handler_url(key, 'handler_name', request.user) - for key in itertools.chain([block.scope_ids.usage_id], getattr(block, 'children', [])) + str(block.usage_key): _get_handler_url(block.usage_key, 'handler_name', request.user, version=version) } + # Currently we don't support child blocks so we don't need this pre-loading of child handler URLs: + # handler_urls = { + # str(key): _get_handler_url(key, 'handler_name', request.user) + # for key in itertools.chain([block.scope_ids.usage_id], getattr(block, 'children', [])) + # } lms_root_url = configuration_helpers.get_value('LMS_ROOT_URL', settings.LMS_ROOT_URL) context = { 'fragment': fragment, @@ -221,7 +230,8 @@ def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name, raise AuthenticationFailed("Invalid user ID format.") request_webob = DjangoWebobRequest(request) # Convert from django request to the webob format that XBlocks expect - block = load_block(usage_key, user) + + block = load_block(usage_key, user, version=parse_version_request(request.GET.get("version"))) # Run the handler, and save any resulting XBlock field value changes: response_webob = block.handle(handler_name, request_webob, suffix) response = webob_to_django_response(response_webob) diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index 61db3d520e29..2cc0aa1500b5 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -239,6 +239,13 @@ def save_block(self, block): if not self.authored_data_store.has_changes(block): return # No changes, so no action needed. + if block._runtime_requested_version != LatestVersion.DRAFT: + # Not sure if this is an important restriction but it seems like overwriting the latest version based on + # an old version is likely an accident, so for now we're not going to allow it. + raise ValidationError( + "Do not make changes to a component starting from the published or past versions. Use the latest draft." + ) + # Verify that the user has permission to write to authored data in this # learning context: if self.user is not None: diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index c927aca53f1a..8e8c59db2191 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -3,7 +3,7 @@ """ from __future__ import annotations import logging -from typing import Callable, Optional +from typing import Callable, Optional, Protocol from urllib.parse import urljoin # pylint: disable=import-error import crum @@ -15,7 +15,7 @@ from django.core.cache import cache from django.core.exceptions import PermissionDenied from eventtracking import tracker -from opaque_keys.edx.keys import UsageKey, LearningContextKey +from opaque_keys.edx.keys import UsageKeyV2, LearningContextKey from web_fragments.fragment import Fragment from xblock.core import XBlock from xblock.exceptions import NoSuchServiceError @@ -38,7 +38,7 @@ from openedx.core.types import User as UserType from openedx.core.djangoapps.enrollments.services import EnrollmentsService from openedx.core.djangoapps.xblock.apps import get_xblock_app_config -from openedx.core.djangoapps.xblock.data import AuthoredDataMode, StudentDataMode +from openedx.core.djangoapps.xblock.data import AuthoredDataMode, StudentDataMode, LatestVersion from openedx.core.djangoapps.xblock.runtime.ephemeral_field_data import EphemeralKeyValueStore from openedx.core.djangoapps.xblock.runtime.mixin import LmsBlockMixin from openedx.core.djangoapps.xblock.utils import get_xblock_id_for_anonymous_user @@ -63,6 +63,18 @@ def function(event_type, event): return function +class GetHandlerFunction(Protocol): + def __call__( + self, + usage_key: UsageKeyV2, + handler_name: str, + user: UserType, + *, + version: int | LatestVersion = LatestVersion.AUTO, + ) -> str: + ... + + class XBlockRuntime(RuntimeShim, Runtime): """ This class manages one or more instantiated XBlocks for a particular user, @@ -99,7 +111,7 @@ def __init__( self, user: UserType | None, *, - handler_url: Callable[[UsageKey, str, UserType | None], str], + handler_url: GetHandlerFunction, student_data_mode: StudentDataMode, authored_data_mode: AuthoredDataMode, id_reader: Optional[IdReader] = None, @@ -140,7 +152,10 @@ def handler_url(self, block, handler_name: str, suffix='', query='', thirdparty= if thirdparty: log.warning("thirdparty handlers are not supported by this runtime for XBlock %s.", type(block)) - url = self.handler_url_fn(block.scope_ids.usage_id, handler_name, self.user) + # Note: it's important that we call handlers based on the same version of the block + # (draft block -> draft data available to handler; published block -> published data available to handler) + kwargs = {"version": block._runtime_requested_version} if hasattr(block, "_runtime_requested_version") else {} + url = self.handler_url_fn(block.usage_key, handler_name, self.user, **kwargs) if suffix: if not url.endswith('/'): url += '/'