From dc8fd749370b4db93bd98d54881bce719a44576b Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 18 Oct 2024 11:16:58 -0700 Subject: [PATCH] fix: problem block could not be used with versioned handler URls --- openedx/core/djangoapps/xblock/api.py | 4 +- .../core/djangoapps/xblock/rest_api/urls.py | 38 +++++++++++-------- .../core/djangoapps/xblock/rest_api/views.py | 6 +-- .../core/djangoapps/xblock/runtime/runtime.py | 9 ++++- 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 3cdafd4c909d..919b4b63ec4b 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -299,9 +299,9 @@ def get_handler_url( '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) + kwargs["version"] = str(version) if isinstance(version, int) else version.value + path = reverse('xblock_api:xblock_handler', kwargs=kwargs) # We must return an absolute URL. We can't just use # rest_framework.reverse.reverse to get the absolute URL because this method diff --git a/openedx/core/djangoapps/xblock/rest_api/urls.py b/openedx/core/djangoapps/xblock/rest_api/urls.py index 6432291178ec..7bc61db144ba 100644 --- a/openedx/core/djangoapps/xblock/rest_api/urls.py +++ b/openedx/core/djangoapps/xblock/rest_api/urls.py @@ -10,25 +10,31 @@ # urls_studio and urls_lms, and/or the views could be likewise duplicated. app_name = 'openedx.core.djangoapps.xblock.rest_api' +block_endpoints = [ + # get metadata about an XBlock: + path('', views.block_metadata), + # get/post full json fields of an XBlock: + path('fields/', views.BlockFieldsView.as_view()), + # render one of this XBlock's views (e.g. student_view) + re_path(r'^view/(?P[\w\-]+)/$', views.render_block_view), + # get the URL needed to call this XBlock's handlers + re_path(r'^handler_url/(?P[\w\-]+)/$', views.get_handler_url), + # call one of this block's handlers + re_path( + r'^handler/(?P\w+)-(?P\w+)/(?P[\w\-]+)/(?P.+)?$', + views.xblock_handler, + name='xblock_handler', + ), + # API endpoints related to a specific version of this XBlock: +] + urlpatterns = [ path('api/xblock/v2/', include([ - path('xblocks//', include([ - # get metadata about an XBlock: - path('', views.block_metadata), - # get/post full json fields of an XBlock: - path('fields/', views.BlockFieldsView.as_view()), - # render one of this XBlock's views (e.g. student_view) - re_path(r'^view/(?P[\w\-]+)/$', views.render_block_view), - # get the URL needed to call this XBlock's handlers - re_path(r'^handler_url/(?P[\w\-]+)/$', views.get_handler_url), - # call one of this block's handlers - re_path( - r'^handler/(?P\w+)-(?P\w+)/(?P[\w\-]+)/(?P.+)?$', - views.xblock_handler, - name='xblock_handler', - ), - ])), + # This pattern with the version must come first so '@' isn't included in usage_key_str below + path(r'xblocks/@/', include(block_endpoints)), + path(r'xblocks//', include(block_endpoints)), ])), + # Non-API views (these return HTML, not JSON): 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 diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 32e19ebad882..20fa926983ac 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -58,7 +58,7 @@ def parse_version_request(version_str: str | None) -> LatestVersion | int: return int(version_str) except ValueError: raise serializers.ValidationError( # pylint: disable=raise-missing-from - "Invalid version specifier '{version_str}'. Expected 'draft', 'published', or an integer." + f"Invalid version specifier '{version_str}'. Expected 'draft', 'published', or an integer." ) @@ -184,7 +184,7 @@ def get_handler_url(request, usage_key_str, handler_name): # and https://github.com/openedx/XBlock/pull/383 for context. @csrf_exempt @xframe_options_exempt -def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name, suffix=None): +def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name, suffix=None, version=None): """ Run an XBlock's handler and return the result @@ -232,7 +232,7 @@ def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name, request_webob = DjangoWebobRequest(request) # Convert from django request to the webob format that XBlocks expect - block = load_block(usage_key, user, version=parse_version_request(request.GET.get("version"))) + block = load_block(usage_key, user, version=parse_version_request(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/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index c1fb6870c007..2bf84e995417 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -156,7 +156,14 @@ def handler_url(self, block, handler_name: str, suffix='', query='', thirdparty= # 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 {} # pylint: disable=protected-access + kwargs = {} + if hasattr(block, "_runtime_requested_version"): # pylint: disable=protected-access + if self.authored_data_mode == AuthoredDataMode.DEFAULT_DRAFT: + default_version = LatestVersion.DRAFT + else: + default_version = LatestVersion.PUBLISHED + if block._runtime_requested_version != default_version: # pylint: disable=protected-access + kwargs["version"] = block._runtime_requested_version # pylint: disable=protected-access url = self.handler_url_fn(block.usage_key, handler_name, self.user, **kwargs) if suffix: if not url.endswith('/'):