diff --git a/cms/static/js/views/modals/preview_v2_library_changes.js b/cms/static/js/views/modals/preview_v2_library_changes.js index e80c40e3ea..2821328988 100644 --- a/cms/static/js/views/modals/preview_v2_library_changes.js +++ b/cms/static/js/views/modals/preview_v2_library_changes.js @@ -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. diff --git a/cms/static/sass/views/_container.scss b/cms/static/sass/views/_container.scss index da87873283..8a0e5cd506 100644 --- a/cms/static/sass/views/_container.scss +++ b/cms/static/sass/views/_container.scss @@ -570,7 +570,7 @@ & > iframe { width: 100%; - min-height: 350px; + min-height: 450px; background: url('#{$static-path}/images/spinner.gif') center center no-repeat; } } diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index a046206e30..cad6d37f51 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -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 diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 03b32e08ad..04cf96abbf 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -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.""" diff --git a/openedx/core/djangoapps/content_libraries/tests/test_versioned_apis.py b/openedx/core/djangoapps/content_libraries/tests/test_versioned_apis.py new file mode 100644 index 0000000000..ad7ea54d8d --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/tests/test_versioned_apis.py @@ -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, """ + + """) + 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, """ + + """) + 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, """ + + """) + + 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.", + ) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 4e48805ac5..75c75e70b8 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -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) diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 3cdafd4c90..00bb8bc356 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -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 diff --git a/openedx/core/djangoapps/xblock/rest_api/url_converters.py b/openedx/core/djangoapps/xblock/rest_api/url_converters.py new file mode 100644 index 0000000000..baa4a5d5e2 --- /dev/null +++ b/openedx/core/djangoapps/xblock/rest_api/url_converters.py @@ -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 diff --git a/openedx/core/djangoapps/xblock/rest_api/urls.py b/openedx/core/djangoapps/xblock/rest_api/urls.py index 6432291178..ee41b43f5b 100644 --- a/openedx/core/djangoapps/xblock/rest_api/urls.py +++ b/openedx/core/djangoapps/xblock/rest_api/urls.py @@ -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//', views.render_block_view), + # get the URL needed to call this XBlock's handlers + path('handler_url//', 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', - ), - ])), + path(r'xblocks//', include(block_endpoints)), + path(r'xblocks/@/', include(block_endpoints)), ])), - path('xblocks/v2//', include([ + # 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 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 32e19ebad8..d69edcbfd5 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -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