diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 4d09eb4549..90b73ef7f4 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -743,13 +743,15 @@ def get_library_block(usage_key, include_collections=False) -> LibraryXBlockMeta return xblock_metadata -def set_library_block_olx(usage_key, new_olx_str): +def set_library_block_olx(usage_key, new_olx_str) -> int: """ Replace the OLX source of the given XBlock. This is only meant for use by developers or API client applications, as very little validation is done and this can easily result in a broken XBlock that won't load. + + Returns the version number of the newly created ComponentVersion. """ # because this old pylint can't understand attr.ib() objects, pylint: disable=no-member assert isinstance(usage_key, LibraryUsageLocatorV2) @@ -786,7 +788,7 @@ def set_library_block_olx(usage_key, new_olx_str): text=new_olx_str, created=now, ) - authoring_api.create_next_version( + new_component_version = authoring_api.create_next_component_version( component.pk, title=new_title, content_to_replace={ @@ -802,6 +804,8 @@ def set_library_block_olx(usage_key, new_olx_str): ) ) + return new_component_version.version_num + def library_component_usage_key( library_key: LibraryLocatorV2, diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 8e9e5fc2a7..b19d27bed3 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -217,6 +217,7 @@ class LibraryXBlockOlxSerializer(serializers.Serializer): Serializer for representing an XBlock's OLX """ olx = serializers.CharField() + version_num = serializers.IntegerField(read_only=True, required=False) class LibraryXBlockStaticFileSerializer(serializers.Serializer): diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 0ed47f995f..a046206e30 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -36,6 +36,7 @@ URL_LIB_LTI_JWKS = URL_LIB_LTI_PREFIX + 'pub/jwks/' URL_LIB_LTI_LAUNCH = URL_LIB_LTI_PREFIX + 'launch/' URL_BLOCK_RENDER_VIEW = '/api/xblock/v2/xblocks/{block_key}/view/{view_name}/' +URL_BLOCK_EMBED_VIEW = '/xblocks/v2/{block_key}/embed/{view_name}/' # Returns HTML not JSON so its URL is different URL_BLOCK_GET_HANDLER_URL = '/api/xblock/v2/xblocks/{block_key}/handler_url/{handler_name}/' URL_BLOCK_METADATA_URL = '/api/xblock/v2/xblocks/{block_key}/' URL_BLOCK_FIELDS_URL = '/api/xblock/v2/xblocks/{block_key}/fields/' @@ -300,6 +301,24 @@ class ContentLibrariesRestApiTest(APITransactionTestCase): url = URL_BLOCK_RENDER_VIEW.format(block_key=block_key, view_name=view_name) return self._api('get', url, None, expect_response) + def _embed_block( + self, + block_key, + *, + view_name="student_view", + version: str | int | None = None, + expect_response=200, + ) -> str: + """ + Get an HTML response that displays the given XBlock. Returns HTML. + """ + url = URL_BLOCK_EMBED_VIEW.format(block_key=block_key, view_name=view_name) + if version is not None: + url += f"?version={version}" + response = self.client.get(url) + assert response.status_code == expect_response, 'Unexpected response code {}:'.format(response.status_code) + return response.content.decode() + def _get_block_handler_url(self, block_key, handler_name): """ Get the URL to call a specific XBlock's handler. diff --git a/openedx/core/djangoapps/content_libraries/tests/fields_test_block.py b/openedx/core/djangoapps/content_libraries/tests/fields_test_block.py new file mode 100644 index 0000000000..0db5ac28d7 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/tests/fields_test_block.py @@ -0,0 +1,60 @@ +""" +Block for testing variously scoped XBlock fields. +""" +import json + +from webob import Response +from web_fragments.fragment import Fragment +from xblock.core import XBlock, Scope +from xblock import fields + + +class FieldsTestBlock(XBlock): + """ + Block for testing variously scoped XBlock fields and XBlock handlers. + + This has only authored fields. See also UserStateTestBlock which has user fields. + """ + BLOCK_TYPE = "fields-test" + has_score = False + + display_name = fields.String(scope=Scope.settings, name='User State Test Block') + setting_field = fields.String(scope=Scope.settings, name='A setting') + content_field = fields.String(scope=Scope.content, name='A setting') + + @XBlock.json_handler + def update_fields(self, data, suffix=None): # pylint: disable=unused-argument + """ + Update the authored fields of this block + """ + self.display_name = data["display_name"] + self.setting_field = data["setting_field"] + self.content_field = data["content_field"] + return {} + + @XBlock.handler + def get_fields(self, request, suffix=None): # pylint: disable=unused-argument + """ + Get the various fields of this XBlock. + """ + return Response( + json.dumps({ + "display_name": self.display_name, + "setting_field": self.setting_field, + "content_field": self.content_field, + }), + content_type='application/json', + charset='UTF-8', + ) + + def student_view(self, _context): + """ + Return the student view. + """ + fragment = Fragment() + fragment.add_content(f'
SF: {self.setting_field}
\n') + fragment.add_content(f'CF: {self.content_field}
\n') + handler_url = self.runtime.handler_url(self, 'get_fields') + fragment.add_content(f'handler URL: {handler_url}
\n') + return fragment diff --git a/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py new file mode 100644 index 0000000000..712117e3d2 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py @@ -0,0 +1,184 @@ +""" +Tests for the XBlock v2 runtime's "embed" view, using Content Libraries + +This view is used in the MFE to preview XBlocks that are in the library. +""" +import re + +import ddt +from django.core.exceptions import ValidationError +from django.test.utils import override_settings +from openedx_events.tests.utils import OpenEdxEventsTestMixin +import pytest +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 +@ddt.ddt +@override_settings(CORS_ORIGIN_WHITELIST=[]) # For some reason, this setting isn't defined in our test environment? +class LibrariesEmbedViewTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin): + """ + Tests for embed_view and interacting with draft/published/past versions of + Learning-Core-based XBlocks (in Content Libraries). + + These tests use the REST API, which in turn relies on the Python API. + Some tests may use the python API directly if necessary to provide + coverage of any code paths not accessible via the REST API. + + In general, these tests should + (1) Use public APIs only - don't directly create data using other methods, + which results in a less realistic test and ties the test suite too + closely to specific implementation details. + (Exception: users can be provisioned using a user factory) + (2) Assert that fields are present in responses, but don't assert that the + entire response has some specific shape. That way, things like adding + new fields to an API response, which are backwards compatible, won't + break any tests, but backwards-incompatible API changes will. + + WARNING: every test should have a unique library slug, because even though + the django/mysql database gets reset for each test case, the lookup between + library slug and bundle UUID does not because it's assumed to be immutable + and cached forever. + """ + + @XBlock.register_temp_plugin(FieldsTestBlock, FieldsTestBlock.BLOCK_TYPE) + def test_embed_vew_versions(self): + """ + Test that the embed_view renders a block and can render different versions of it. + """ + # 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, """ +SF: {setting_value}
' in html + assert f'CF: {content_value}
' in html + handler_url = re.search(r'handler URL: ([^<]+)
', html).group(1) + assert handler_url.startswith('http') + handler_result = self.client.get(handler_url).json() + assert handler_result == { + "display_name": display_name, + "setting_field": setting_value, + "content_field": content_value, + } + check_fields('Field Test Block (Draft, v4)', 'Draft setting value 4.', 'Draft content value 4.') + + # But if we request the published version, we get that: + html = self._embed_block(block_id, version="published") + check_fields('Field Test Block (Published, v3)', 'Published setting value 3.', 'Published content value 3.') + + # And if we request a specific version, we get that: + html = self._embed_block(block_id, version=3) + check_fields('Field Test Block (Published, v3)', 'Published setting value 3.', 'Published content value 3.') + + # And if we request a specific version, we get that: + html = self._embed_block(block_id, version=2) + check_fields('Field Test Block (Old, v2)', 'Old setting value 2.', 'Old content value 2.') + + html = self._embed_block(block_id, version=4) + check_fields('Field Test Block (Draft, v4)', 'Draft setting value 4.', 'Draft content value 4.') + + @XBlock.register_temp_plugin(FieldsTestBlock, FieldsTestBlock.BLOCK_TYPE) + def test_handlers_modifying_published_data(self): + """ + Test that if we requested any version other than "draft", the handlers should not allow _writing_ to authored + field data (because you'd be overwriting the latest draft version with changes based on an old version). + + We may decide to relax this restriction in the future. Not sure how important it is. + + Writing to student state is OK. + """ + # Create a library: + lib = self._create_library(slug="test-eb-2", 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"] + + # Now render the "embed block" view. This test only runs in CMS so it should default to the draft: + html = self._embed_block(block_id) + + def call_update_handler(**kwargs): + handler_url = re.search(r'handler URL: ([^<]+)
', html).group(1) + assert handler_url.startswith('http') + handler_url = handler_url.replace('get_fields', 'update_fields') + response = self.client.post(handler_url, kwargs, format='json') + assert response.status_code == 200 + + def check_fields(display_name, setting_field, content_field): + assert f'SF: {setting_field}
' in html + assert f'CF: {content_field}
' in html + + # Call the update handler to change the fields on the draft: + call_update_handler(display_name="DN-01", setting_field="SV-01", content_field="CV-01") + + # Render the block again and check that the handler was able to update the fields: + html = self._embed_block(block_id) + check_fields(display_name="DN-01", setting_field="SV-01", content_field="CV-01") + + # Publish the library: + self._commit_library_changes(lib_id) + + # Now try changing the authored fields of the published version using a handler: + html = self._embed_block(block_id, version="published") + expected_msg = "Do not make changes to a component starting from the published or past versions." + with pytest.raises(ValidationError, match=expected_msg) as err: + call_update_handler(display_name="DN-X", setting_field="SV-X", content_field="CV-X") + + # Now try changing the authored fields of a specific past version using a handler: + html = self._embed_block(block_id, version=2) + with pytest.raises(ValidationError, match=expected_msg) as err: + call_update_handler(display_name="DN-X", setting_field="SV-X", content_field="CV-X") + + # Make sure the fields were not updated: + html = self._embed_block(block_id) + check_fields(display_name="DN-01", setting_field="SV-01", content_field="CV-01") + + # TODO: test that any static assets referenced in the student_view html are loaded as the correct version, and not + # always loaded as "latest draft". + + # TODO: if we are ever able to run these tests in the LMS, test that the LMS only allows accessing the published + # version. diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index afae04f403..4e48805ac5 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -731,10 +731,10 @@ class LibraryBlockOlxView(APIView): serializer.is_valid(raise_exception=True) new_olx_str = serializer.validated_data["olx"] try: - api.set_library_block_olx(key, new_olx_str) + version_num = api.set_library_block_olx(key, new_olx_str) except ValueError as err: raise ValidationError(detail=str(err)) # lint-amnesty, pylint: disable=raise-missing-from - return Response(LibraryXBlockOlxSerializer({"olx": new_olx_str}).data) + return Response(LibraryXBlockOlxSerializer({"olx": new_olx_str, "version_num": version_num}).data) @method_decorator(non_atomic_requests, name="dispatch") diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index c21d052cfc..95fa360d5e 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -5,7 +5,7 @@ we keep here some extra classes for usage within edx-platform. These classes cov import logging from edx_toggles.toggles import WaffleFlag -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, LearningContextKey log = logging.getLogger(__name__) @@ -99,7 +99,11 @@ class CourseWaffleFlag(WaffleFlag): def is_enabled(self, course_key=None): # pylint: disable=arguments-differ """ - Returns whether or not the flag is enabled within the context of a given course. + Returns whether or not the flag is enabled within the context of a given + course. + + Can also be given the key of any other learning context (like a content + library), but it will act like a regular waffle flag in that case. Arguments: course_key (Optional[CourseKey]): The course to check for override before @@ -107,12 +111,12 @@ class CourseWaffleFlag(WaffleFlag): outside the context of any course. """ if course_key: - assert isinstance( - course_key, CourseKey - ), "Provided course_key '{}' is not instance of CourseKey.".format( - course_key - ) - is_enabled_for_course = self._get_course_override_value(course_key) - if is_enabled_for_course is not None: - return is_enabled_for_course + if isinstance(course_key, CourseKey): + is_enabled_for_course = self._get_course_override_value(course_key) + if is_enabled_for_course is not None: + return is_enabled_for_course + else: + # In case this gets called with a content library key, that's fine - just ignore it and + # act like a normal waffle flag. We currently don't support library-specific overrides. + assert isinstance(course_key, LearningContextKey), "expected a course key or other learning context key" return super().is_enabled() diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index dbb7c82467..3cdafd4c90 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -17,7 +17,7 @@ from django.core.exceptions import PermissionDenied from django.urls import reverse from django.utils.translation import gettext as _ from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import Component +from openedx_learning.api.authoring_models import Component, ComponentVersion from opaque_keys.edx.keys import UsageKeyV2 from opaque_keys.edx.locator import BundleDefinitionLocator, LibraryUsageLocatorV2 from rest_framework.exceptions import NotFound @@ -32,6 +32,7 @@ from openedx.core.djangoapps.xblock.runtime.learning_core_runtime import ( LearningCoreFieldData, LearningCoreXBlockRuntime, ) +from .data import CheckPerm, LatestVersion from .utils import get_secure_token_for_xblock_handler, get_xblock_id_for_anonymous_user from .runtime.learning_core_runtime import LearningCoreXBlockRuntime @@ -44,16 +45,6 @@ from openedx.core.djangoapps.xblock.learning_context import LearningContext log = logging.getLogger(__name__) -class CheckPerm(Enum): - """ Options for the default permission check done by load_block() """ - # can view the published block and call handlers etc. but not necessarily view its OLX source nor field data - CAN_LEARN = 1 - # read-only studio view: can see the block (draft or published), see its OLX, see its field data, etc. - CAN_READ_AS_AUTHOR = 2 - # can view everything and make changes to the block - CAN_EDIT = 3 - - def get_runtime(user: UserType): """ Return a new XBlockRuntime. @@ -73,7 +64,13 @@ def get_runtime(user: UserType): return runtime -def load_block(usage_key, user, *, check_permission: CheckPerm | None = CheckPerm.CAN_LEARN): +def load_block( + usage_key: UsageKeyV2, + user: UserType, + *, + check_permission: CheckPerm | None = CheckPerm.CAN_LEARN, + version: int | LatestVersion = LatestVersion.AUTO, +): """ Load the specified XBlock for the given user. @@ -112,10 +109,13 @@ def load_block(usage_key, user, *, check_permission: CheckPerm | None = CheckPer runtime = get_runtime(user=user) try: - return runtime.get_block(usage_key) + return runtime.get_block(usage_key, version=version) except NoSuchUsage as exc: # Convert NoSuchUsage to NotFound so we do the right thing (404 not 500) by default. raise NotFound(f"The component '{usage_key}' does not exist.") from exc + except ComponentVersion.DoesNotExist as exc: + # Convert ComponentVersion.DoesNotExist to NotFound so we do the right thing (404 not 500) by default. + raise NotFound(f"The requested version of component '{usage_key}' does not exist.") from exc def get_block_metadata(block, includes=()): @@ -248,7 +248,13 @@ def render_block_view(block, view_name, user): # pylint: disable=unused-argumen return fragment -def get_handler_url(usage_key, handler_name, user): +def get_handler_url( + usage_key: UsageKeyV2, + handler_name: str, + user: UserType | None, + *, + version: int | LatestVersion = LatestVersion.AUTO, +): """ A method for getting the URL to any XBlock handler. The URL must be usable without any authentication (no cookie, no OAuth/JWT), and may expire. (So @@ -265,14 +271,19 @@ def get_handler_url(usage_key, handler_name, user): usage_key - Usage Key (Opaque Key object or string) handler_name - Name of the handler or a dummy name like 'any_handler' user - Django User (registered or anonymous) + version - Run the handler against a specific version of the + block (e.g. when viewing an old version of it in + Studio). Some blocks use handlers to load their data + so it's important the handler matches the student_view + etc. 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: # lint-amnesty, pylint: disable=no-else-raise + if not user: raise TypeError("Cannot get handler URLs without specifying a specific user ID.") - elif user.is_authenticated: + if user.is_authenticated: user_id = user.id elif user.is_anonymous: user_id = get_xblock_id_for_anonymous_user(user) @@ -282,12 +293,16 @@ def get_handler_url(usage_key, handler_name, user): # and this XBlock: secure_token = get_secure_token_for_xblock_handler(user_id, usage_key_str) # Now generate the URL to that handler: - path = reverse('xblock_api:xblock_handler', kwargs={ + kwargs = { 'usage_key_str': usage_key_str, '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) + # We must return an absolute URL. We can't just use # rest_framework.reverse.reverse to get the absolute URL because this method # can be called by the XBlock from python as well and in that case we don't diff --git a/openedx/core/djangoapps/xblock/apps.py b/openedx/core/djangoapps/xblock/apps.py index 848470a3a9..e4f07666c3 100644 --- a/openedx/core/djangoapps/xblock/apps.py +++ b/openedx/core/djangoapps/xblock/apps.py @@ -5,7 +5,7 @@ from django.apps import AppConfig, apps from django.conf import settings from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from .data import StudentDataMode +from .data import StudentDataMode, AuthoredDataMode class XBlockAppConfig(AppConfig): @@ -50,6 +50,7 @@ class LmsXBlockAppConfig(XBlockAppConfig): """ return dict( student_data_mode=StudentDataMode.Persisted, + authored_data_mode=AuthoredDataMode.STRICTLY_PUBLISHED, ) def get_site_root_url(self): @@ -72,6 +73,7 @@ class StudioXBlockAppConfig(XBlockAppConfig): """ return dict( student_data_mode=StudentDataMode.Ephemeral, + authored_data_mode=AuthoredDataMode.DEFAULT_DRAFT, ) def get_site_root_url(self): diff --git a/openedx/core/djangoapps/xblock/data.py b/openedx/core/djangoapps/xblock/data.py index 1da850caed..67cfda8dd4 100644 --- a/openedx/core/djangoapps/xblock/data.py +++ b/openedx/core/djangoapps/xblock/data.py @@ -11,3 +11,39 @@ class StudentDataMode(Enum): """ Ephemeral = 'ephemeral' Persisted = 'persisted' + + +class AuthoredDataMode(Enum): + """ + Runtime configuration which determines whether published or draft versions of content is used by default. + """ + # Published only: used by the LMS. ONLY the published version of an XBlock is ever loaded. Users/APIs cannot request + # the draft version nor a specific version. + STRICTLY_PUBLISHED = 'published' + # Default draft: used by Studio. By default the "lastest draft" version of an XBlock is used, but users/APIs can + # also request to see the published version or any specific (old) version. + DEFAULT_DRAFT = 'persisted' + + +class CheckPerm(Enum): + """ + Options for the default permission check done by load_block() + """ + # can view the published block and call handlers etc. but not necessarily view its OLX source nor field data + CAN_LEARN = 1 + # read-only studio view: can see the block (draft or published), see its OLX, see its field data, etc. + CAN_READ_AS_AUTHOR = 2 + # can view everything and make changes to the block + CAN_EDIT = 3 + + +class LatestVersion(Enum): + """ + Options for specifying which version of an XBlock you want to load, if not specifying a specific version. + """ + # Get the latest draft + DRAFT = "draft" + # Get the latest published version + PUBLISHED = "published" + # Get the default (based on AuthoredDataMode, i.e. published for LMS APIs, draft for Studio APIs) + AUTO = "auto" diff --git a/openedx/core/djangoapps/xblock/rest_api/urls.py b/openedx/core/djangoapps/xblock/rest_api/urls.py index dec2fc562f..6432291178 100644 --- a/openedx/core/djangoapps/xblock/rest_api/urls.py +++ b/openedx/core/djangoapps/xblock/rest_api/urls.py @@ -32,6 +32,6 @@ urlpatterns = [ path('xblocks/v2/