Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: expand support for versioned URLs in v2 XBlock runtime (#35676)

Browse Source 
* fix: problem block could not be used with versioned handler URls

* refactor: simplify REST API handling of usage keys

* feat: add more version awareness and update tests

* fix: make the preview changes modal bigger as requested

* refactor: parse version at the urlconf layer too
This commit is contained in:
Braden MacDonald
2024-10-21 09:14:17 -07:00
committed by GitHub
parent 9e14566f90
commit e2d6765637
11 changed files with 261 additions and 105 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
cms/static/js/views/modals/preview_v2_library_changes.js
View File

@@ -17,7 +17,7 @@ function($, _, gettext, BaseModal, ViewUtils, XBlockViewUtils) {
options: $.extend({}, BaseModal.prototype.options, {
modalName: 'preview-lib-changes',
modalSize: 'med',
modalSize: 'lg',
view: 'studio_view',
viewSpecificClasses: 'modal-lib-preview confirm',
// Translators: "title" is the name of the current component being edited.

2
cms/static/sass/views/_container.scss
View File

@@ -570,7 +570,7 @@
& > iframe {
width: 100%;
min-height: 350px;
min-height: 450px;
background: url('#{$static-path}/images/spinner.gif') center center no-repeat;
}
}

15
openedx/core/djangoapps/content_libraries/tests/base.py
View File

@@ -292,12 +292,14 @@ class ContentLibrariesRestApiTest(APITransactionTestCase):
data = {"block_id": block_id}
return self._api('post', url, data, expect_response)
def _render_block_view(self, block_key, view_name, expect_response=200):
def _render_block_view(self, block_key, view_name, version=None, expect_response=200):
"""
Render an XBlock's view in the active application's runtime.
Note that this endpoint has different behavior in Studio (draft mode)
vs. the LMS (published version only).
"""
if version is not None:
block_key += f"@{version}"
url = URL_BLOCK_RENDER_VIEW.format(block_key=block_key, view_name=view_name)
return self._api('get', url, None, expect_response)
@@ -328,8 +330,17 @@ class ContentLibrariesRestApiTest(APITransactionTestCase):
url = URL_BLOCK_GET_HANDLER_URL.format(block_key=block_key, handler_name=handler_name)
return self._api('get', url, None, expect_response=200)["handler_url"]
def _get_library_block_fields(self, block_key, expect_response=200):
def _get_basic_xblock_metadata(self, block_key, version=None, expect_response=200):
""" Get basic metadata about a specific block in the library. """
if version is not None:
block_key += f"@{version}"
result = self._api('get', URL_BLOCK_METADATA_URL.format(block_key=block_key), None, expect_response)
return result
def _get_library_block_fields(self, block_key, version=None, expect_response=200):
""" Get the fields of a specific block in the library. This API is only used by the MFE editors. """
if version is not None:
block_key += f"@{version}"
result = self._api('get', URL_BLOCK_FIELDS_URL.format(block_key=block_key), None, expect_response)
return result

4
openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py
View File

@@ -1101,10 +1101,6 @@ class ContentLibraryXBlockValidationTest(APITestCase):
endpoint.format(**endpoint_parameters),
)
self.assertEqual(response.status_code, 404)
msg = f"XBlock {endpoint_parameters.get('block_key')} does not exist, or you don't have permission to view it."
self.assertEqual(response.json(), {
'detail': msg,
})
def test_xblock_handler_invalid_key(self):
"""This endpoint is tested separately from the previous ones as it's not a DRF endpoint."""

104
openedx/core/djangoapps/content_libraries/tests/test_versioned_apis.py Normal file
View File

@@ -0,0 +1,104 @@
"""
Tests that several XBlock APIs support versioning
"""
from django.test.utils import override_settings
from openedx_events.tests.utils import OpenEdxEventsTestMixin
from xblock.core import XBlock
from openedx.core.djangoapps.content_libraries.tests.base import (
ContentLibrariesRestApiTest
)
from openedx.core.djangolib.testing.utils import skip_unless_cms
from .fields_test_block import FieldsTestBlock
@skip_unless_cms
@override_settings(CORS_ORIGIN_WHITELIST=[]) # For some reason, this setting isn't defined in our test environment?
class VersionedXBlockApisTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin):
"""
Tests for three APIs implemented by djangoapps.xblock, and used by content
libraries. These tests focus on versioning.
Note the metadata endpoint is different than the similar "metadata" endpoint
within the content libraries API, which returns a lot more information. This
endpoint pretty much only returns the display name of a block, but it does
allow retrieving past versions.
"""
@XBlock.register_temp_plugin(FieldsTestBlock, FieldsTestBlock.BLOCK_TYPE)
def test_versioned_metadata(self):
"""
Test that metadata endpoint can get different versions of the block's metadata
"""
# Create a library:
lib = self._create_library(slug="test-eb-1", title="Test Library", description="")
lib_id = lib["id"]
# Create an XBlock. This will be the empty version 1:
create_response = self._add_block_to_library(lib_id, FieldsTestBlock.BLOCK_TYPE, "block1")
block_id = create_response["id"]
# Create version 2 of the block by setting its OLX:
olx_response = self._set_library_block_olx(block_id, """
<fields-test
display_name="Field Test Block (Old, v2)"
setting_field="Old setting value 2."
content_field="Old content value 2."
/>
""")
assert olx_response["version_num"] == 2
# Create version 3 of the block by setting its OLX again:
olx_response = self._set_library_block_olx(block_id, """
<fields-test
display_name="Field Test Block (Published, v3)"
setting_field="Published setting value 3."
content_field="Published content value 3."
/>
""")
assert olx_response["version_num"] == 3
# Publish the library:
self._commit_library_changes(lib_id)
# Create the draft (version 4) of the block:
olx_response = self._set_library_block_olx(block_id, """
<fields-test
display_name="Field Test Block (Draft, v4)"
setting_field="Draft setting value 4."
content_field="Draft content value 4."
/>
""")
def check_results(version, display_name, settings_field, content_field):
meta = self._get_basic_xblock_metadata(block_id, version=version)
assert meta["block_id"] == block_id
assert meta["block_type"] == FieldsTestBlock.BLOCK_TYPE
assert meta["display_name"] == display_name
fields = self._get_library_block_fields(block_id, version=version)
assert fields["display_name"] == display_name
assert fields["metadata"]["setting_field"] == settings_field
rendered = self._render_block_view(block_id, "student_view", version=version)
assert rendered["block_id"] == block_id
assert f"SF: {settings_field}" in rendered["content"]
assert f"CF: {content_field}" in rendered["content"]
# Now get the metadata. If we don't specify a version, it should be the latest draft (in Studio):
check_results(
version=None,
display_name="Field Test Block (Draft, v4)",
settings_field="Draft setting value 4.",
content_field="Draft content value 4.",
)
# Get the published version:
check_results(
version="published",
display_name="Field Test Block (Published, v3)",
settings_field="Published setting value 3.",
content_field="Published content value 3.",
)
# Get a specific version:
check_results(
version="2",
display_name="Field Test Block (Old, v2)",
settings_field="Old setting value 2.",
content_field="Old content value 2.",
)

8
openedx/core/djangoapps/content_libraries/views.py
View File

@@ -617,7 +617,13 @@ class LibraryBlockView(APIView):
@convert_exceptions
def get(self, request, usage_key_str):
"""
Get metadata about an existing XBlock in the content library
Get metadata about an existing XBlock in the content library.
This API doesn't support versioning; most of the information it returns
is related to the latest draft version, or to all versions of the block.
If you need to get the display name of a previous version, use the
similar "metadata" API from djangoapps.xblock, which does support
versioning.
"""
key = LibraryUsageLocatorV2.from_string(usage_key_str)
api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY)

9
openedx/core/djangoapps/xblock/api.py
View File

@@ -279,7 +279,6 @@ def get_handler_url(
This view does not check/care if the XBlock actually exists.
"""
usage_key_str = str(usage_key)
site_root_url = get_xblock_app_config().get_site_root_url()
if not user:
raise TypeError("Cannot get handler URLs without specifying a specific user ID.")
@@ -291,17 +290,17 @@ def get_handler_url(
raise ValueError("Invalid user value")
# Now generate a token-secured URL for this handler, specific to this user
# and this XBlock:
secure_token = get_secure_token_for_xblock_handler(user_id, usage_key_str)
secure_token = get_secure_token_for_xblock_handler(user_id, str(usage_key))
# Now generate the URL to that handler:
kwargs = {
'usage_key_str': usage_key_str,
'usage_key': usage_key,
'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)
kwargs["version"] = version
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

57
openedx/core/djangoapps/xblock/rest_api/url_converters.py Normal file
View File

@@ -0,0 +1,57 @@
"""
URL pattern converters
https://docs.djangoproject.com/en/5.1/topics/http/urls/#registering-custom-path-converters
"""
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import UsageKeyV2
from ..api import LatestVersion
class UsageKeyV2Converter:
"""
Converter that matches V2 usage keys like:
lb:Org:LIB:drag-and-drop-v2:91c2b1d5
"""
regex = r'[\w-]+(:[\w\-.]+)+'
def to_python(self, value: str) -> UsageKeyV2:
try:
return UsageKeyV2.from_string(value)
except InvalidKeyError as exc:
raise ValueError from exc
def to_url(self, value: UsageKeyV2) -> str:
return str(value)
class VersionConverter:
"""
Converter that matches a version string like "draft", "published", or a
number, and converts it to either an 'int' or a LatestVersion enum value.
"""
regex = r'(draft|published|\d+)'
def to_python(self, value: str | None) -> LatestVersion | int:
""" Convert from string to LatestVersion or integer version spec """
if value is None:
return LatestVersion.AUTO # AUTO = published if we're in the LMS, draft if we're in Studio.
if value == "draft":
return LatestVersion.DRAFT
if value == "published":
return LatestVersion.PUBLISHED
return int(value) # May raise ValueError, which Django will convert to 404
def to_url(self, value: LatestVersion | int | None) -> str:
"""
Convert from LatestVersion or integer version spec to URL path string.
Note that if you provide any value at all, django won't be able to
match the paths that don't have a version in the URL, so if you want
LatestVersion.AUTO, don't pass any value for 'version' to reverse(...).
"""
if value is None or value == LatestVersion.AUTO:
raise ValueError # This default value does not need to be in the URL
if isinstance(value, int):
return str(value)
return value.value # LatestVersion.DRAFT or PUBLISHED

45
openedx/core/djangoapps/xblock/rest_api/urls.py
View File

@@ -1,7 +1,8 @@
"""
URL configuration for the new XBlock API
"""
from django.urls import include, path, re_path
from django.urls import include, path, re_path, register_converter
from . import url_converters
from . import views
# Note that the exact same API URLs are used in Studio and the LMS, but the API
@@ -10,26 +11,34 @@ from . import views
# urls_studio and urls_lms, and/or the views could be likewise duplicated.
app_name = 'openedx.core.djangoapps.xblock.rest_api'
register_converter(url_converters.UsageKeyV2Converter, "usage_v2")
register_converter(url_converters.VersionConverter, "block_version")
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)
path('view/<str:view_name>/', views.render_block_view),
# get the URL needed to call this XBlock's handlers
path('handler_url/<str:handler_name>/', views.get_handler_url),
# call one of this block's handlers
re_path(
r'^handler/(?P<user_id>\w+)-(?P<secure_token>\w+)/(?P<handler_name>[\w\-]+)/(?P<suffix>.+)?$',
views.xblock_handler,
name='xblock_handler',
),
# API endpoints related to a specific version of this XBlock:
]
urlpatterns = [
path('api/xblock/v2/', include([
path('xblocks/<str:usage_key_str>/', 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<view_name>[\w\-]+)/$', views.render_block_view),
# get the URL needed to call this XBlock's handlers
re_path(r'^handler_url/(?P<handler_name>[\w\-]+)/$', views.get_handler_url),
# call one of this block's handlers
re_path(
r'^handler/(?P<user_id>\w+)-(?P<secure_token>\w+)/(?P<handler_name>[\w\-]+)/(?P<suffix>.+)?$',
views.xblock_handler,
name='xblock_handler',
),
])),
path(r'xblocks/<usage_v2:usage_key>/', include(block_endpoints)),
path(r'xblocks/<usage_v2:usage_key>@<block_version:version>/', include(block_endpoints)),
])),
path('xblocks/v2/<str:usage_key_str>/', include([
# Non-API views (these return HTML, not JSON):
path('xblocks/v2/<usage_v2:usage_key>/', 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
path('embed/<str:view_name>/', views.embed_block_view),

111
openedx/core/djangoapps/xblock/rest_api/views.py
View File

@@ -8,11 +8,11 @@ from corsheaders.signals import check_request_enabled
from django.conf import settings
from django.contrib.auth import get_user_model
from django.db.transaction import atomic
from django.http import Http404
from django.shortcuts import render
from django.utils.translation import gettext as _
from django.views.decorators.clickjacking import xframe_options_exempt
from django.views.decorators.csrf import csrf_exempt
from opaque_keys.edx.keys import UsageKeyV2
from rest_framework import permissions, serializers
from rest_framework.decorators import api_view, permission_classes # lint-amnesty, pylint: disable=unused-import
from rest_framework.exceptions import PermissionDenied, AuthenticationFailed, NotFound
@@ -22,8 +22,6 @@ from xblock.django.request import DjangoWebobRequest, webob_to_django_response
from xblock.exceptions import NoSuchUsage
from xblock.fields import Scope
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import UsageKey
import openedx.core.djangoapps.site_configuration.helpers as configuration_helpers
from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl
from openedx.core.lib.api.view_utils import view_auth_classes
@@ -37,35 +35,15 @@ from ..api import (
render_block_view as _render_block_view,
)
from ..utils import validate_secure_token_for_xblock_handler
from .url_converters import VersionConverter
User = get_user_model()
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( # pylint: disable=raise-missing-from
"Invalid version specifier '{version_str}'. 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
def block_metadata(request, usage_key_str):
def block_metadata(request, usage_key: UsageKeyV2, version: LatestVersion | int = LatestVersion.AUTO):
"""
Get metadata about the specified block.
@@ -74,12 +52,7 @@ def block_metadata(request, usage_key_str):
* "include": a comma-separated list of keys to include.
Valid keys are "index_dictionary" and "student_view_data".
"""
try:
usage_key = UsageKey.from_string(usage_key_str)
except InvalidKeyError as e:
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
block = load_block(usage_key, request.user)
block = load_block(usage_key, request.user, version=version)
includes = request.GET.get("include", "").split(",")
metadata_dict = get_block_metadata(block, includes=includes)
if 'children' in metadata_dict:
@@ -92,17 +65,17 @@ def block_metadata(request, usage_key_str):
@api_view(['GET'])
@view_auth_classes(is_authenticated=False)
@permission_classes((permissions.AllowAny, )) # Permissions are handled at a lower level, by the learning context
def render_block_view(request, usage_key_str, view_name):
def render_block_view(
request,
usage_key: UsageKeyV2,
view_name: str,
version: LatestVersion | int = LatestVersion.AUTO,
):
"""
Get the HTML, JS, and CSS needed to render the given XBlock.
"""
try:
usage_key = UsageKey.from_string(usage_key_str)
except InvalidKeyError as e:
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
try:
block = load_block(usage_key, request.user)
block = load_block(usage_key, request.user, version=version)
except NoSuchUsage as exc:
raise NotFound(f"{usage_key} not found") from exc
@@ -116,19 +89,17 @@ def render_block_view(request, usage_key_str, view_name):
@view_auth_classes(is_authenticated=False)
@permission_classes((permissions.AllowAny, )) # Permissions are handled at a lower level, by the learning context
@xframe_options_exempt
def embed_block_view(request, usage_key_str, view_name):
def embed_block_view(request, usage_key: UsageKeyV2, view_name: str):
"""
Render the given XBlock in an <iframe>
Unstable - may change after Sumac
"""
# Check if a specific version has been requested. TODO: move this to a URL path param like the other views?
try:
usage_key = UsageKey.from_string(usage_key_str)
except InvalidKeyError as e:
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
# Check if a specific version has been requested
version = parse_version_request(request.GET.get("version"))
version = VersionConverter().to_python(request.GET.get("version"))
except ValueError as exc:
raise serializers.ValidationError("Invalid version specifier") from exc
try:
block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_LEARN, version=version)
@@ -162,19 +133,19 @@ def embed_block_view(request, usage_key_str, view_name):
@api_view(['GET'])
@view_auth_classes(is_authenticated=False)
def get_handler_url(request, usage_key_str, handler_name):
def get_handler_url(
request,
usage_key: UsageKeyV2,
handler_name: str,
version: LatestVersion | int = LatestVersion.AUTO,
):
"""
Get an absolute URL which can be used (without any authentication) to call
the given XBlock handler.
The URL will expire but is guaranteed to be valid for a minimum of 2 days.
"""
try:
usage_key = UsageKey.from_string(usage_key_str)
except InvalidKeyError as e:
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
handler_url = _get_handler_url(usage_key, handler_name, request.user)
handler_url = _get_handler_url(usage_key, handler_name, request.user, version=version)
return Response({"handler_url": handler_url})
@@ -184,7 +155,15 @@ 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: str,
usage_key: UsageKeyV2,
handler_name: str,
suffix: str | None = None,
version: LatestVersion | int = LatestVersion.AUTO,
):
"""
Run an XBlock's handler and return the result
@@ -192,16 +171,11 @@ def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name,
auth token included in the URL (see below). As a result it can be exempt
from CSRF, session auth, and JWT/OAuth.
"""
try:
usage_key = UsageKey.from_string(usage_key_str)
except InvalidKeyError as e:
raise Http404 from e
# To support sandboxed XBlocks, custom frontends, and other use cases, we
# authenticate requests using a secure token in the URL. see
# openedx.core.djangoapps.xblock.utils.get_secure_hash_for_xblock_handler
# for details and rationale.
if not validate_secure_token_for_xblock_handler(user_id, usage_key_str, secure_token):
if not validate_secure_token_for_xblock_handler(user_id, str(usage_key), secure_token):
raise PermissionDenied("Invalid/expired auth token.")
if request.user.is_authenticated:
# The user authenticated twice, e.g. with session auth and the token.
@@ -232,7 +206,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=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)
@@ -265,17 +239,13 @@ class BlockFieldsView(APIView):
"""
@atomic
def get(self, request, usage_key_str):
def get(self, request, usage_key: UsageKeyV2, version: LatestVersion | int = LatestVersion.AUTO):
"""
retrieves the xblock, returning display_name, data, and metadata
"""
try:
usage_key = UsageKey.from_string(usage_key_str)
except InvalidKeyError as e:
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
# The "fields" view requires "read as author" permissions because the fields can contain answers, etc.
block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_READ_AS_AUTHOR)
block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_READ_AS_AUTHOR, version=version)
# It would make more sense if this just had a "fields" dict with all the content+settings fields, but
# for backwards compatibility we call the settings metadata and split it up like this, ignoring all content
# fields except "data".
@@ -288,15 +258,12 @@ class BlockFieldsView(APIView):
return Response(block_dict)
@atomic
def post(self, request, usage_key_str):
def post(self, request, usage_key, version: LatestVersion | int = LatestVersion.AUTO):
"""
edits the xblock, saving changes to data and metadata only (display_name included in metadata)
"""
try:
usage_key = UsageKey.from_string(usage_key_str)
except InvalidKeyError as e:
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
if version != LatestVersion.AUTO:
raise serializers.ValidationError("Cannot specify a version when saving changes")
user = request.user
block = load_block(usage_key, user, check_permission=CheckPerm.CAN_EDIT)
data = request.data.get("data")

9
openedx/core/djangoapps/xblock/runtime/runtime.py
View File

@@ -156,7 +156,14 @@ class XBlockRuntime(RuntimeShim, Runtime):
# 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('/'):