From dc8fd749370b4db93bd98d54881bce719a44576b Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 18 Oct 2024 11:16:58 -0700
Subject: [PATCH 1/5] 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<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',
+    ),
+    # 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',
-            ),
-        ])),
+        # This pattern with the version must come first so '@' isn't included in usage_key_str below
+        path(r'xblocks/<str:usage_key_str>@<str:version>/', include(block_endpoints)),
+        path(r'xblocks/<str:usage_key_str>/', include(block_endpoints)),
     ])),
+    # Non-API views (these return HTML, not JSON):
     path('xblocks/v2/<str:usage_key_str>/', 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('/'):

From ea7b04995b2f7d69aaab43ddd7aa95b0c011f099 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 18 Oct 2024 12:50:26 -0700
Subject: [PATCH 2/5] refactor: simplify REST API handling of usage keys

---
 .../tests/test_content_libraries.py           |  4 --
 openedx/core/djangoapps/xblock/api.py         |  5 +-
 .../xblock/rest_api/url_converters.py         | 23 ++++++++
 .../core/djangoapps/xblock/rest_api/urls.py   | 16 +++---
 .../core/djangoapps/xblock/rest_api/views.py  | 56 +++++--------------
 5 files changed, 48 insertions(+), 56 deletions(-)
 create mode 100644 openedx/core/djangoapps/xblock/rest_api/url_converters.py

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 03b32e08ad36..04cf96abbf36 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 @@ def test_invalid_key(self, endpoint, endpoint_parameters):
             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/xblock/api.py b/openedx/core/djangoapps/xblock/api.py
index 919b4b63ec4b..773bf81edb66 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,10 +290,10 @@ 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,
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 000000000000..4b660fdc0bf4
--- /dev/null
+++ b/openedx/core/djangoapps/xblock/rest_api/url_converters.py
@@ -0,0 +1,23 @@
+"""
+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
+
+
+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):
+        try:
+            return UsageKeyV2.from_string(value)
+        except InvalidKeyError as exc:
+            raise ValueError from exc
+
+    def to_url(self, value):
+        return str(value)
diff --git a/openedx/core/djangoapps/xblock/rest_api/urls.py b/openedx/core/djangoapps/xblock/rest_api/urls.py
index 7bc61db144ba..b7504b7474a0 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 .url_converters import UsageKeyV2Converter
 from . import views
 
 # Note that the exact same API URLs are used in Studio and the LMS, but the API
@@ -10,15 +11,17 @@
 # urls_studio and urls_lms, and/or the views could be likewise duplicated.
 app_name = 'openedx.core.djangoapps.xblock.rest_api'
 
+register_converter(UsageKeyV2Converter, "usage_v2")
+
 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<view_name>[\w\-]+)/$', views.render_block_view),
+    path('view/<str:view_name>/', 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),
+    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>.+)?$',
@@ -30,12 +33,11 @@
 
 urlpatterns = [
     path('api/xblock/v2/', include([
-        # This pattern with the version must come first so '@' isn't included in usage_key_str below
-        path(r'xblocks/<str:usage_key_str>@<str:version>/', include(block_endpoints)),
-        path(r'xblocks/<str:usage_key_str>/', include(block_endpoints)),
+        path(r'xblocks/<usage_v2:usage_key>/', include(block_endpoints)),
+        path(r'xblocks/<usage_v2:usage_key>@<str:version>/', include(block_endpoints)),
     ])),
     # Non-API views (these return HTML, not JSON):
-    path('xblocks/v2/<str:usage_key_str>/', include([
+    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),
diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py
index 20fa926983ac..33066d97ab1b 100644
--- a/openedx/core/djangoapps/xblock/rest_api/views.py
+++ b/openedx/core/djangoapps/xblock/rest_api/views.py
@@ -8,7 +8,6 @@
 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
@@ -22,8 +21,6 @@
 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
@@ -65,7 +62,7 @@ def parse_version_request(version_str: str | None) -> LatestVersion | int:
 @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, version=None):
     """
     Get metadata about the specified block.
 
@@ -74,10 +71,8 @@ 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
+    if version:
+        raise NotImplementedError()
 
     block = load_block(usage_key, request.user)
     includes = request.GET.get("include", "").split(",")
@@ -92,14 +87,12 @@ 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, view_name, version=None):
     """
     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
+    if version:
+        raise NotImplementedError()
 
     try:
         block = load_block(usage_key, request.user)
@@ -116,17 +109,12 @@ 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, view_name):
     """
     Render the given XBlock in an <iframe>
 
     Unstable - may change after Sumac
     """
-    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"))
 
@@ -162,17 +150,15 @@ 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, handler_name, version=None):
     """
     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
+    if version:
+        raise NotImplementedError()
 
     handler_url = _get_handler_url(usage_key, handler_name, request.user)
     return Response({"handler_url": handler_url})
@@ -184,7 +170,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, version=None):
+def xblock_handler(request, user_id, secure_token, usage_key, handler_name, suffix=None, version=None):
     """
     Run an XBlock's handler and return the result
 
@@ -192,16 +178,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.
@@ -265,14 +246,10 @@ class BlockFieldsView(APIView):
     """
 
     @atomic
-    def get(self, request, usage_key_str):
+    def get(self, request, usage_key):
         """
         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)
@@ -288,15 +265,10 @@ def get(self, request, usage_key_str):
         return Response(block_dict)
 
     @atomic
-    def post(self, request, usage_key_str):
+    def post(self, request, usage_key):
         """
         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
-
         user = request.user
         block = load_block(usage_key, user, check_permission=CheckPerm.CAN_EDIT)
         data = request.data.get("data")

From ec63d4a62bfc771aa13b9b20712404f821055310 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 18 Oct 2024 13:56:41 -0700
Subject: [PATCH 3/5] feat: add more version awareness and update tests

---
 .../content_libraries/tests/base.py           |  15 ++-
 .../tests/test_versioned_apis.py              | 104 ++++++++++++++++++
 .../djangoapps/content_libraries/views.py     |   8 +-
 .../core/djangoapps/xblock/rest_api/views.py  |  28 +++--
 4 files changed, 137 insertions(+), 18 deletions(-)
 create mode 100644 openedx/core/djangoapps/content_libraries/tests/test_versioned_apis.py

diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py
index a046206e302f..cad6d37f5126 100644
--- a/openedx/core/djangoapps/content_libraries/tests/base.py
+++ b/openedx/core/djangoapps/content_libraries/tests/base.py
@@ -292,12 +292,14 @@ def _paste_clipboard_content_in_library(self, lib_key, block_id, expect_response
         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 @@ def _get_block_handler_url(self, block_key, handler_name):
         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_versioned_apis.py b/openedx/core/djangoapps/content_libraries/tests/test_versioned_apis.py
new file mode 100644
index 000000000000..ad7ea54d8dfb
--- /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, """
+            <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.",
+        )
diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py
index 4e48805ac5ff..75c75e70b822 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/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py
index 33066d97ab1b..38939ccaa67e 100644
--- a/openedx/core/djangoapps/xblock/rest_api/views.py
+++ b/openedx/core/djangoapps/xblock/rest_api/views.py
@@ -71,10 +71,7 @@ def block_metadata(request, usage_key, version=None):
     * "include": a comma-separated list of keys to include.
       Valid keys are "index_dictionary" and "student_view_data".
     """
-    if version:
-        raise NotImplementedError()
-
-    block = load_block(usage_key, request.user)
+    block = load_block(usage_key, request.user, version=parse_version_request(version))
     includes = request.GET.get("include", "").split(",")
     metadata_dict = get_block_metadata(block, includes=includes)
     if 'children' in metadata_dict:
@@ -91,11 +88,8 @@ def render_block_view(request, usage_key, view_name, version=None):
     """
     Get the HTML, JS, and CSS needed to render the given XBlock.
     """
-    if version:
-        raise NotImplementedError()
-
     try:
-        block = load_block(usage_key, request.user)
+        block = load_block(usage_key, request.user, version=parse_version_request(version))
     except NoSuchUsage as exc:
         raise NotFound(f"{usage_key} not found") from exc
 
@@ -157,10 +151,7 @@ def get_handler_url(request, usage_key, handler_name, version=None):
 
     The URL will expire but is guaranteed to be valid for a minimum of 2 days.
     """
-    if version:
-        raise NotImplementedError()
-
-    handler_url = _get_handler_url(usage_key, handler_name, request.user)
+    handler_url = _get_handler_url(usage_key, handler_name, request.user, version=parse_version_request(version))
     return Response({"handler_url": handler_url})
 
 
@@ -246,13 +237,18 @@ class BlockFieldsView(APIView):
     """
 
     @atomic
-    def get(self, request, usage_key):
+    def get(self, request, usage_key, version=None):
         """
         retrieves the xblock, returning display_name, data, and metadata
         """
 
         # 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=parse_version_request(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".
@@ -265,10 +261,12 @@ def get(self, request, usage_key):
         return Response(block_dict)
 
     @atomic
-    def post(self, request, usage_key):
+    def post(self, request, usage_key, version=None):
         """
         edits the xblock, saving changes to data and metadata only (display_name included in metadata)
         """
+        if version:
+            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")

From 72950a9a68535206808c002a6da75b4b9a01dbd0 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 18 Oct 2024 14:36:45 -0700
Subject: [PATCH 4/5] fix: make the preview changes modal bigger as requested

---
 cms/static/js/views/modals/preview_v2_library_changes.js | 2 +-
 cms/static/sass/views/_container.scss                    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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 e80c40e3ea3e..282132898895 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 da87873283a8..8a0e5cd5062d 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;
     }
   }

From a8508213cfd0c6c178e67db416832a684b5db2ab Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sat, 19 Oct 2024 09:02:09 -0700
Subject: [PATCH 5/5] refactor: parse version at the urlconf layer too

---
 openedx/core/djangoapps/xblock/api.py         |  2 +-
 .../xblock/rest_api/url_converters.py         | 38 ++++++++-
 .../core/djangoapps/xblock/rest_api/urls.py   |  7 +-
 .../core/djangoapps/xblock/rest_api/views.py  | 79 +++++++++----------
 4 files changed, 79 insertions(+), 47 deletions(-)

diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py
index 773bf81edb66..00bb8bc356a6 100644
--- a/openedx/core/djangoapps/xblock/api.py
+++ b/openedx/core/djangoapps/xblock/api.py
@@ -299,7 +299,7 @@ def get_handler_url(
         'handler_name': handler_name,
     }
     if version != LatestVersion.AUTO:
-        kwargs["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
diff --git a/openedx/core/djangoapps/xblock/rest_api/url_converters.py b/openedx/core/djangoapps/xblock/rest_api/url_converters.py
index 4b660fdc0bf4..baa4a5d5e2f0 100644
--- a/openedx/core/djangoapps/xblock/rest_api/url_converters.py
+++ b/openedx/core/djangoapps/xblock/rest_api/url_converters.py
@@ -5,6 +5,8 @@
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import UsageKeyV2
 
+from ..api import LatestVersion
+
 
 class UsageKeyV2Converter:
     """
@@ -13,11 +15,43 @@ class UsageKeyV2Converter:
     """
     regex = r'[\w-]+(:[\w\-.]+)+'
 
-    def to_python(self, value):
+    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):
+    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 b7504b7474a0..ee41b43f5b39 100644
--- a/openedx/core/djangoapps/xblock/rest_api/urls.py
+++ b/openedx/core/djangoapps/xblock/rest_api/urls.py
@@ -2,7 +2,7 @@
 URL configuration for the new XBlock API
 """
 from django.urls import include, path, re_path, register_converter
-from .url_converters import UsageKeyV2Converter
+from . import url_converters
 from . import views
 
 # Note that the exact same API URLs are used in Studio and the LMS, but the API
@@ -11,7 +11,8 @@
 # urls_studio and urls_lms, and/or the views could be likewise duplicated.
 app_name = 'openedx.core.djangoapps.xblock.rest_api'
 
-register_converter(UsageKeyV2Converter, "usage_v2")
+register_converter(url_converters.UsageKeyV2Converter, "usage_v2")
+register_converter(url_converters.VersionConverter, "block_version")
 
 block_endpoints = [
     # get metadata about an XBlock:
@@ -34,7 +35,7 @@
 urlpatterns = [
     path('api/xblock/v2/', include([
         path(r'xblocks/<usage_v2:usage_key>/', include(block_endpoints)),
-        path(r'xblocks/<usage_v2:usage_key>@<str:version>/', include(block_endpoints)),
+        path(r'xblocks/<usage_v2:usage_key>@<block_version:version>/', include(block_endpoints)),
     ])),
     # Non-API views (these return HTML, not JSON):
     path('xblocks/v2/<usage_v2:usage_key>/', include([
diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py
index 38939ccaa67e..d69edcbfd51c 100644
--- a/openedx/core/djangoapps/xblock/rest_api/views.py
+++ b/openedx/core/djangoapps/xblock/rest_api/views.py
@@ -12,6 +12,7 @@
 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
@@ -34,35 +35,15 @@
     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
-            f"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, version=None):
+def block_metadata(request, usage_key: UsageKeyV2, version: LatestVersion | int = LatestVersion.AUTO):
     """
     Get metadata about the specified block.
 
@@ -71,7 +52,7 @@ def block_metadata(request, usage_key, version=None):
     * "include": a comma-separated list of keys to include.
       Valid keys are "index_dictionary" and "student_view_data".
     """
-    block = load_block(usage_key, request.user, version=parse_version_request(version))
+    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:
@@ -84,12 +65,17 @@ def block_metadata(request, usage_key, version=None):
 @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, view_name, version=None):
+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:
-        block = load_block(usage_key, request.user, version=parse_version_request(version))
+        block = load_block(usage_key, request.user, version=version)
     except NoSuchUsage as exc:
         raise NotFound(f"{usage_key} not found") from exc
 
@@ -103,14 +89,17 @@ def render_block_view(request, usage_key, view_name, version=None):
 @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, 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
-    version = parse_version_request(request.GET.get("version"))
+    # Check if a specific version has been requested. TODO: move this to a URL path param like the other views?
+    try:
+        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)
@@ -144,14 +133,19 @@ def embed_block_view(request, usage_key, view_name):
 
 @api_view(['GET'])
 @view_auth_classes(is_authenticated=False)
-def get_handler_url(request, usage_key, handler_name, version=None):
+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.
     """
-    handler_url = _get_handler_url(usage_key, handler_name, request.user, version=parse_version_request(version))
+    handler_url = _get_handler_url(usage_key, handler_name, request.user, version=version)
     return Response({"handler_url": handler_url})
 
 
@@ -161,7 +155,15 @@ def get_handler_url(request, usage_key, handler_name, version=None):
 # 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, handler_name, suffix=None, version=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
 
@@ -204,7 +206,7 @@ def xblock_handler(request, user_id, secure_token, usage_key, handler_name, suff
 
     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(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)
@@ -237,18 +239,13 @@ class BlockFieldsView(APIView):
     """
 
     @atomic
-    def get(self, request, usage_key, version=None):
+    def get(self, request, usage_key: UsageKeyV2, version: LatestVersion | int = LatestVersion.AUTO):
         """
         retrieves the xblock, returning display_name, data, and metadata
         """
 
         # 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,
-            version=parse_version_request(version),
-        )
+        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".
@@ -261,11 +258,11 @@ def get(self, request, usage_key, version=None):
         return Response(block_dict)
 
     @atomic
-    def post(self, request, usage_key, version=None):
+    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)
         """
-        if version:
+        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)