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] 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)