From a451688860fd0f809de1138b8562e430055fffcb Mon Sep 17 00:00:00 2001 From: Brian Mesick Date: Thu, 17 Oct 2024 10:10:50 -0400 Subject: [PATCH 1/6] build: Pin MyPY due to internal error Currently all PRs are failing on an INTERNAL ERROR in MyPY that seems to have been introduced in 1.12.0. This pins edx-platform to <1.12.0 until we can find out more information and hopefully get an upstream fix. --- requirements/constraints.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index b40f357ace80..370bf2f16d0c 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -123,6 +123,11 @@ markdown<3.4.0 # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35270 moto<5.0 +# Date: 2024-10-16 +# MyPY 1.12.0 fails on all PRs with the following error: +# openedx/core/djangoapps/content_libraries/api.py:732: error: INTERNAL ERROR +mypy<1.12.0 + # Date: 2024-07-16 # We need to upgrade the version of elasticsearch to atleast 7.15 before we can upgrade to Numpy 2.0.0 # Otherwise we see a failure while running the following command: From 82a0ded180c4759828cde91d905a9cab3d6dfa52 Mon Sep 17 00:00:00 2001 From: Brian Mesick Date: Thu, 17 Oct 2024 10:19:03 -0400 Subject: [PATCH 2/6] build: Propagate MyPY constraint The MyPY constraint is propagated to development.txt --- requirements/edx/development.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 75b966519d3d..5bbe07ade1de 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1279,8 +1279,9 @@ multidict==6.1.0 # -r requirements/edx/testing.txt # aiohttp # yarl -mypy==1.12.0 +mypy==1.11.2 # via + # -c requirements/edx/../constraints.txt # -r requirements/edx/development.in # django-stubs # djangorestframework-stubs From 3e14dd24c273f2a88d025c9ca9cc70cc33372215 Mon Sep 17 00:00:00 2001 From: Brian Mesick Date: Thu, 17 Oct 2024 10:48:09 -0400 Subject: [PATCH 3/6] docs: Add issue comment for unpinning MyPY Co-authored-by: Muhammad Farhan --- requirements/constraints.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 370bf2f16d0c..7bf90df3cb66 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -126,6 +126,7 @@ moto<5.0 # Date: 2024-10-16 # MyPY 1.12.0 fails on all PRs with the following error: # openedx/core/djangoapps/content_libraries/api.py:732: error: INTERNAL ERROR +# Issue for unpinning: https://github.com/openedx/edx-platform/issues/35667 mypy<1.12.0 # Date: 2024-07-16 From 795d03958116f34a535d1c65be7f24a3d9b6e0f9 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 17 Oct 2024 12:02:26 -0400 Subject: [PATCH 4/6] feat: Upstream Sync with Content Library Blocks (#34925) This introdues the idea of "upstream" and "downstream" content, where downstreams (like course components) can pull content updates from upstreams (like learning core-backed content library blocks). This supports the upcoming Content Libraries Relaunch Beta for Sumac. New features include: * A new XBlockMixin: UpstreamSyncMixin. * A new CMS Python API: cms.lib.xblock.upstream_sync * A new CMS JSON API: /api/contentstore/v2/downstreams * A temporary, very basic UI for syncing from Content Library blocks Implements: https://github.com/kdmccormick/edx-platform/blob/kdmccormick/upstream-proto/docs/decisions/0020-upstream-block.rst Co-authored-by: Braden MacDonald --- cms/djangoapps/contentstore/helpers.py | 47 +- .../rest_api/v1/serializers/vertical_block.py | 13 + .../v1/views/tests/test_vertical_block.py | 12 + .../rest_api/v1/views/vertical_block.py | 23 + .../contentstore/rest_api/v2/urls.py | 24 +- .../rest_api/v2/views/__init__.py | 3 - .../rest_api/v2/views/downstreams.py | 251 ++++++++++ .../v2/views/tests/test_downstreams.py | 266 +++++++++++ .../views/tests/test_clipboard_paste.py | 74 +++ .../xblock_storage_handlers/view_handlers.py | 3 +- cms/envs/common.py | 2 + cms/lib/xblock/test/test_upstream_sync.py | 337 +++++++++++++ cms/lib/xblock/upstream_sync.py | 451 ++++++++++++++++++ cms/templates/studio_xblock_wrapper.html | 32 +- docs/decisions/0020-upstream-downstream.rst | 5 + mypy.ini | 4 +- .../core/djangoapps/content_libraries/api.py | 9 +- .../core/djangoapps/content_staging/api.py | 4 +- .../core/djangoapps/content_staging/data.py | 1 + .../0005_stagedcontent_version_num.py | 18 + .../core/djangoapps/content_staging/models.py | 2 + .../core/djangoapps/content_staging/views.py | 4 +- 22 files changed, 1562 insertions(+), 23 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py create mode 100644 cms/lib/xblock/test/test_upstream_sync.py create mode 100644 cms/lib/xblock/upstream_sync.py create mode 100644 openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index a4ece6c85d59..e9f599772d3e 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -9,6 +9,7 @@ from attrs import frozen, Factory from django.conf import settings +from django.contrib.auth import get_user_model from django.utils.translation import gettext as _ from opaque_keys.edx.keys import AssetKey, CourseKey, UsageKey from opaque_keys.edx.locator import DefinitionLocator, LocalId @@ -22,6 +23,7 @@ from xmodule.xml_block import XmlMixin from cms.djangoapps.models.settings.course_grading import CourseGradingModel +from cms.lib.xblock.upstream_sync import UpstreamLink, BadUpstream, BadDownstream, fetch_customizable_fields from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers import openedx.core.djangoapps.content_staging.api as content_staging_api import openedx.core.djangoapps.content_tagging.api as content_tagging_api @@ -30,6 +32,10 @@ log = logging.getLogger(__name__) + +User = get_user_model() + + # Note: Grader types are used throughout the platform but most usages are simply in-line # strings. In addition, new grader types can be defined on the fly anytime one is needed # (because they're just strings). This dict is an attempt to constrain the sprawl in Studio. @@ -282,9 +288,10 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> node, parent_xblock, store, - user_id=request.user.id, + user=request.user, slug_hint=user_clipboard.source_usage_key.block_id, copied_from_block=str(user_clipboard.source_usage_key), + copied_from_version_num=user_clipboard.content.version_num, tags=user_clipboard.content.tags, ) # Now handle static files that need to go into Files & Uploads: @@ -293,7 +300,6 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> staged_content_id=user_clipboard.content.id, static_files=static_files, ) - return new_xblock, notices @@ -302,12 +308,15 @@ def _import_xml_node_to_parent( parent_xblock: XBlock, # The modulestore we're using store, - # The ID of the user who is performing this operation - user_id: int, + # The user who is performing this operation + user: User, # Hint to use as usage ID (block_id) for the new XBlock slug_hint: str | None = None, # UsageKey of the XBlock that this one is a copy of copied_from_block: str | None = None, + # Positive int version of source block, if applicable (e.g., library block). + # Zero if not applicable (e.g., course block). + copied_from_version_num: int = 0, # Content tags applied to the source XBlock(s) tags: dict[str, str] | None = None, ) -> XBlock: @@ -373,12 +382,32 @@ def _import_xml_node_to_parent( raise NotImplementedError("We don't yet support pasting XBlocks with children") temp_xblock.parent = parent_key if copied_from_block: - # Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin) - temp_xblock.copied_from_block = copied_from_block + # Try to link the pasted block (downstream) to the copied block (upstream). + temp_xblock.upstream = copied_from_block + try: + UpstreamLink.get_for_block(temp_xblock) + except (BadDownstream, BadUpstream): + # Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an + # upstream. That's fine! Instead, we store a reference to where this block was copied from, in the + # 'copied_from_block' field (from AuthoringMixin). + temp_xblock.upstream = None + temp_xblock.copied_from_block = copied_from_block + else: + # But if it doesn't fail, then populate the `upstream_version` field based on what was copied. Note that + # this could be the latest published version, or it could be an an even newer draft version. + temp_xblock.upstream_version = copied_from_version_num + # Also, fetch upstream values (`upstream_display_name`, etc.). + # Recall that the copied block could be a draft. So, rather than fetching from the published upstream (which + # could be older), fetch from the copied block itself. That way, if an author customizes a field, but then + # later wants to restore it, it will restore to the value that the field had when the block was pasted. Of + # course, if the author later syncs updates from a *future* published upstream version, then that will fetch + # new values from the published upstream content. + fetch_customizable_fields(upstream=temp_xblock, downstream=temp_xblock, user=user) + # Save the XBlock into modulestore. We need to save the block and its parent for this to work: - new_xblock = store.update_item(temp_xblock, user_id, allow_not_found=True) + new_xblock = store.update_item(temp_xblock, user.id, allow_not_found=True) parent_xblock.children.append(new_xblock.location) - store.update_item(parent_xblock, user_id) + store.update_item(parent_xblock, user.id) children_handled = False if hasattr(new_xblock, 'studio_post_paste'): @@ -394,7 +423,7 @@ def _import_xml_node_to_parent( child_node, new_xblock, store, - user_id=user_id, + user=user, copied_from_block=str(child_copied_from), tags=tags, ) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py index d72707ed7836..f2e8b6ef431b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/vertical_block.py @@ -103,6 +103,18 @@ def get_assets_url(self, obj): return None +class UpstreamLinkSerializer(serializers.Serializer): + """ + Serializer holding info for syncing a block with its upstream (eg, a library block). + """ + upstream_ref = serializers.CharField() + version_synced = serializers.IntegerField() + version_available = serializers.IntegerField(allow_null=True) + version_declined = serializers.IntegerField(allow_null=True) + error_message = serializers.CharField(allow_null=True) + ready_to_sync = serializers.BooleanField() + + class ChildVerticalContainerSerializer(serializers.Serializer): """ Serializer for representing a xblock child of vertical container. @@ -113,6 +125,7 @@ class ChildVerticalContainerSerializer(serializers.Serializer): block_type = serializers.CharField() user_partition_info = serializers.DictField() user_partitions = serializers.ListField() + upstream_link = UpstreamLinkSerializer(allow_null=True) actions = serializers.SerializerMethodField() validation_messages = MessageValidation(many=True) render_error = serializers.CharField() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py index d3fc37198213..7cac074a433f 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py @@ -70,6 +70,8 @@ def setup_xblock(self): parent=self.vertical.location, category="html", display_name="Html Content 2", + upstream="lb:FakeOrg:FakeLib:html:FakeBlock", + upstream_version=5, ) def create_block(self, parent, category, display_name, **kwargs): @@ -193,6 +195,7 @@ def test_children_content(self): "name": self.html_unit_first.display_name_with_default, "block_id": str(self.html_unit_first.location), "block_type": self.html_unit_first.location.block_type, + "upstream_link": None, "user_partition_info": expected_user_partition_info, "user_partitions": expected_user_partitions, "actions": { @@ -218,12 +221,21 @@ def test_children_content(self): "can_delete": True, "can_manage_tags": True, }, + "upstream_link": { + "upstream_ref": "lb:FakeOrg:FakeLib:html:FakeBlock", + "version_synced": 5, + "version_available": None, + "version_declined": None, + "error_message": "Linked library item was not found in the system", + "ready_to_sync": False, + }, "user_partition_info": expected_user_partition_info, "user_partitions": expected_user_partitions, "validation_messages": [], "render_error": "", }, ] + self.maxDiff = None self.assertEqual(response.data["children"], expected_response) def test_not_valid_usage_key_string(self): diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py index 670b94afbbe0..0798c341cc1c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py @@ -20,6 +20,7 @@ ContainerHandlerSerializer, VerticalContainerSerializer, ) +from cms.lib.xblock.upstream_sync import UpstreamLink from openedx.core.lib.api.view_utils import view_auth_classes from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order @@ -198,6 +199,7 @@ def get(self, request: Request, usage_key_string: str): "block_type": "drag-and-drop-v2", "user_partition_info": {}, "user_partitions": {} + "upstream_link": null, "actions": { "can_copy": true, "can_duplicate": true, @@ -215,6 +217,13 @@ def get(self, request: Request, usage_key_string: str): "block_type": "video", "user_partition_info": {}, "user_partitions": {} + "upstream_link": { + "upstream_ref": "lb:org:mylib:video:404", + "version_synced": 16 + "version_available": null, + "error_message": "Linked library item not found: lb:org:mylib:video:404", + "ready_to_sync": false, + }, "actions": { "can_copy": true, "can_duplicate": true, @@ -232,6 +241,13 @@ def get(self, request: Request, usage_key_string: str): "block_type": "html", "user_partition_info": {}, "user_partitions": {}, + "upstream_link": { + "upstream_ref": "lb:org:mylib:html:abcd", + "version_synced": 43, + "version_available": 49, + "error_message": null, + "ready_to_sync": true, + }, "actions": { "can_copy": true, "can_duplicate": true, @@ -267,6 +283,7 @@ def get(self, request: Request, usage_key_string: str): child_info = modulestore().get_item(child) user_partition_info = get_visibility_partition_info(child_info, course=course) user_partitions = get_user_partition_info(child_info, course=course) + upstream_link = UpstreamLink.try_get_for_block(child_info) validation_messages = get_xblock_validation_messages(child_info) render_error = get_xblock_render_error(request, child_info) @@ -277,6 +294,12 @@ def get(self, request: Request, usage_key_string: str): "block_type": child_info.location.block_type, "user_partition_info": user_partition_info, "user_partitions": user_partitions, + "upstream_link": ( + # If the block isn't linked to an upstream (which is by far the most common case) then just + # make this field null, which communicates the same info, but with less noise. + upstream_link.to_json() if upstream_link.upstream_ref + else None + ), "validation_messages": validation_messages, "render_error": render_error, }) diff --git a/cms/djangoapps/contentstore/rest_api/v2/urls.py b/cms/djangoapps/contentstore/rest_api/v2/urls.py index ad61cc937015..3e653d07fbcf 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v2/urls.py @@ -1,15 +1,31 @@ """Contenstore API v2 URLs.""" -from django.urls import path - -from cms.djangoapps.contentstore.rest_api.v2.views import HomePageCoursesViewV2 +from django.conf import settings +from django.urls import path, re_path +from cms.djangoapps.contentstore.rest_api.v2.views import home, downstreams app_name = "v2" urlpatterns = [ path( "home/courses", - HomePageCoursesViewV2.as_view(), + home.HomePageCoursesViewV2.as_view(), name="courses", ), + # TODO: Potential future path. + # re_path( + # fr'^downstreams/$', + # downstreams.DownstreamsListView.as_view(), + # name="downstreams_list", + # ), + re_path( + fr'^downstreams/{settings.USAGE_KEY_PATTERN}$', + downstreams.DownstreamView.as_view(), + name="downstream" + ), + re_path( + fr'^downstreams/{settings.USAGE_KEY_PATTERN}/sync$', + downstreams.SyncFromUpstreamView.as_view(), + name="sync_from_upstream" + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py index 73ddde98440c..e69de29bb2d1 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py @@ -1,3 +0,0 @@ -"""Module for v2 views.""" - -from cms.djangoapps.contentstore.rest_api.v2.views.home import HomePageCoursesViewV2 diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py new file mode 100644 index 000000000000..5079698082be --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -0,0 +1,251 @@ +""" +API Views for managing & syncing links between upstream & downstream content + +API paths (We will move these into proper api_doc_tools annotations soon +https://github.com/openedx/edx-platform/issues/35653): + + /api/contentstore/v2/downstreams/{usage_key_string} + + GET: Inspect a single downstream block's link to upstream content. + 200: Upstream link details successfully fetched. Returns UpstreamLink (may contain an error_message). + 404: Downstream block not found or user lacks permission to edit it. + + DELETE: Sever a single downstream block's link to upstream content. + 204: Block successfully unlinked (or it wasn't linked in the first place). No response body. + 404: Downstream block not found or user lacks permission to edit it. + + PUT: Establish or modify a single downstream block's link to upstream content. An authoring client could use this + endpoint to add library content in a two-step process, specifically: (1) add a blank block to a course, then + (2) link it to a content library with ?sync=True. + REQUEST BODY: { + "upstream_ref": str, // reference to upstream block (eg, library block usage key) + "sync": bool, // whether to sync in upstream content (False by default) + } + 200: Downstream block's upstream link successfully edited (and synced, if requested). Returns UpstreamLink. + 400: upstream_ref is malformed, missing, or inaccessible. + 400: Content at upstream_ref does not support syncing. + 404: Downstream block not found or user lacks permission to edit it. + + /api/contentstore/v2/downstreams/{usage_key_string}/sync + + POST: Sync a downstream block with upstream content. + 200: Downstream block successfully synced with upstream content. + 400: Downstream block is not linked to upstream content. + 400: Upstream is malformed, missing, or inaccessible. + 400: Upstream block does not support syncing. + 404: Downstream block not found or user lacks permission to edit it. + + DELETE: Decline an available sync for a downstream block. + 204: Sync successfuly dismissed. No response body. + 400: Downstream block is not linked to upstream content. + 404: Downstream block not found or user lacks permission to edit it. + + # NOT YET IMPLEMENTED -- Will be needed for full Libraries Relaunch in ~Teak. + /api/contentstore/v2/downstreams + /api/contentstore/v2/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true + GET: List downstream blocks that can be synced, filterable by course or sync-readiness. + 200: A paginated list of applicable & accessible downstream blocks. Entries are UpstreamLinks. + +UpstreamLink response schema: + { + "upstream_ref": string? + "version_synced": string?, + "version_available": string?, + "version_declined": string?, + "error_message": string?, + "ready_to_sync": Boolean + } +""" +import logging + +from django.contrib.auth.models import User # pylint: disable=imported-auth-user +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey +from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.request import Request +from rest_framework.response import Response +from rest_framework.views import APIView +from xblock.core import XBlock + +from cms.lib.xblock.upstream_sync import ( + UpstreamLink, UpstreamLinkException, NoUpstream, BadUpstream, BadDownstream, + fetch_customizable_fields, sync_from_upstream, decline_sync, sever_upstream_link +) +from common.djangoapps.student.auth import has_studio_write_access, has_studio_read_access +from openedx.core.lib.api.view_utils import ( + DeveloperErrorViewMixin, + view_auth_classes, +) +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError + + +logger = logging.getLogger(__name__) + + +class _AuthenticatedRequest(Request): + """ + Alias for the `Request` class which tells mypy to assume that `.user` is not an AnonymousUser. + + Using this does NOT ensure the request is actually authenticated-- + you will some other way to ensure that, such as `@view_auth_classes(is_authenticated=True)`. + """ + user: User + + +# TODO: Potential future view. +# @view_auth_classes(is_authenticated=True) +# class DownstreamListView(DeveloperErrorViewMixin, APIView): +# """ +# List all blocks which are linked to upstream content, with optional filtering. +# """ +# def get(self, request: _AuthenticatedRequest) -> Response: +# """ +# Handle the request. +# """ +# course_key_string = request.GET['course_id'] +# syncable = request.GET['ready_to_sync'] +# ... + + +@view_auth_classes(is_authenticated=True) +class DownstreamView(DeveloperErrorViewMixin, APIView): + """ + Inspect or manage an XBlock's link to upstream content. + """ + def get(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Inspect an XBlock's link to upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=False) + return Response(UpstreamLink.try_get_for_block(downstream).to_json()) + + def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Edit an XBlock's link to upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + new_upstream_ref = request.data.get("upstream_ref") + + # Set `downstream.upstream` so that we can try to sync and/or fetch. + # Note that, if this fails and we raise a 4XX, then we will not call modulstore().update_item, + # thus preserving the former value of `downstream.upstream`. + downstream.upstream = new_upstream_ref + sync_param = request.data.get("sync", "false").lower() + if sync_param not in ["true", "false"]: + raise ValidationError({"sync": "must be 'true' or 'false'"}) + try: + if sync_param == "true": + sync_from_upstream(downstream=downstream, user=request.user) + else: + # Even if we're not syncing (i.e., updating the downstream's values with the upstream's), we still need + # to fetch the upstream's customizable values and store them as hidden fields on the downstream. This + # ensures that downstream authors can restore defaults based on the upstream. + fetch_customizable_fields(downstream=downstream, user=request.user) + except BadDownstream as exc: + logger.exception( + "'%s' is an invalid downstream; refusing to set its upstream to '%s'", + usage_key_string, + new_upstream_ref, + ) + raise ValidationError(str(exc)) from exc + except BadUpstream as exc: + logger.exception( + "'%s' is an invalid upstream reference; refusing to set it as upstream of '%s'", + new_upstream_ref, + usage_key_string, + ) + raise ValidationError({"upstream_ref": str(exc)}) from exc + except NoUpstream as exc: + raise ValidationError({"upstream_ref": "value missing"}) from exc + modulestore().update_item(downstream, request.user.id) + # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the + # upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen. + return Response(UpstreamLink.get_for_block(downstream).to_json()) + + def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Sever an XBlock's link to upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + try: + sever_upstream_link(downstream) + except NoUpstream as exc: + logger.exception( + "Tried to DELETE upstream link of '%s', but it wasn't linked to anything in the first place. " + "Will do nothing. ", + usage_key_string, + ) + else: + modulestore().update_item(downstream, request.user.id) + return Response(status=204) + + +@view_auth_classes(is_authenticated=True) +class SyncFromUpstreamView(DeveloperErrorViewMixin, APIView): + """ + Accept or decline an opportunity to sync a downstream block from its upstream content. + """ + + def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Pull latest updates to the block at {usage_key_string} from its linked upstream content. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + try: + sync_from_upstream(downstream, request.user) + except UpstreamLinkException as exc: + logger.exception( + "Could not sync from upstream '%s' to downstream '%s'", + downstream.upstream, + usage_key_string, + ) + raise ValidationError(detail=str(exc)) from exc + modulestore().update_item(downstream, request.user.id) + # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the + # upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen. + return Response(UpstreamLink.get_for_block(downstream).to_json()) + + def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: + """ + Decline the latest updates to the block at {usage_key_string}. + """ + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + try: + decline_sync(downstream) + except (NoUpstream, BadUpstream, BadDownstream) as exc: + # This is somewhat unexpected. If the upstream link is missing or invalid, then the downstream author + # shouldn't have been prompted to accept/decline a sync in the first place. Of course, they could have just + # hit the HTTP API anyway, or they could be viewing a Studio page which hasn't been refreshed in a while. + # So, it's a 400, not a 500. + logger.exception( + "Tried to decline a sync to downstream '%s', but the upstream link '%s' is invalid.", + usage_key_string, + downstream.upstream, + ) + raise ValidationError(str(exc)) from exc + modulestore().update_item(downstream, request.user.id) + return Response(status=204) + + +def _load_accessible_block(user: User, usage_key_string: str, *, require_write_access: bool) -> XBlock: + """ + Given a logged in-user and a serialized usage key of an upstream-linked XBlock, load it from the ModuleStore, + raising a DRF-friendly exception if anything goes wrong. + + Raises NotFound if usage key is malformed, if the user lacks access, or if the block doesn't exist. + """ + not_found = NotFound(detail=f"Block not found or not accessible: {usage_key_string}") + try: + usage_key = UsageKey.from_string(usage_key_string) + except InvalidKeyError as exc: + raise ValidationError(detail=f"Malformed block usage key: {usage_key_string}") from exc + if require_write_access and not has_studio_write_access(user, usage_key.context_key): + raise not_found + if not has_studio_read_access(user, usage_key.context_key): + raise not_found + try: + block = modulestore().get_item(usage_key) + except ItemNotFoundError as exc: + raise not_found from exc + return block diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py new file mode 100644 index 000000000000..616035473e7e --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -0,0 +1,266 @@ +""" +Unit tests for /api/contentstore/v2/downstreams/* JSON APIs. +""" +from unittest.mock import patch + +from cms.lib.xblock.upstream_sync import UpstreamLink, BadUpstream +from common.djangoapps.student.tests.factories import UserFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory + +from .. import downstreams as downstreams_views + + +MOCK_UPSTREAM_REF = "mock-upstream-ref" +MOCK_UPSTREAM_ERROR = "your LibraryGPT subscription has expired" + + +def _get_upstream_link_good_and_syncable(downstream): + return UpstreamLink( + upstream_ref=downstream.upstream, + version_synced=downstream.upstream_version, + version_available=(downstream.upstream_version or 0) + 1, + version_declined=downstream.upstream_version_declined, + error_message=None, + ) + + +def _get_upstream_link_bad(_downstream): + raise BadUpstream(MOCK_UPSTREAM_ERROR) + + +class _DownstreamViewTestMixin: + """ + Shared data and error test cases. + """ + + def setUp(self): + """ + Create a simple course with one unit and two videos, one of which is linked to an "upstream". + """ + super().setUp() + self.course = CourseFactory.create() + chapter = BlockFactory.create(category='chapter', parent=self.course) + sequential = BlockFactory.create(category='sequential', parent=chapter) + unit = BlockFactory.create(category='vertical', parent=sequential) + self.regular_video_key = BlockFactory.create(category='video', parent=unit).usage_key + self.downstream_video_key = BlockFactory.create( + category='video', parent=unit, upstream=MOCK_UPSTREAM_REF, upstream_version=123, + ).usage_key + self.fake_video_key = self.course.id.make_usage_key("video", "NoSuchVideo") + self.superuser = UserFactory(username="superuser", password="password", is_staff=True, is_superuser=True) + self.learner = UserFactory(username="learner", password="password") + + def call_api(self, usage_key_string): + raise NotImplementedError + + def test_404_downstream_not_found(self): + """ + Do we raise 404 if the specified downstream block could not be loaded? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.fake_video_key) + assert response.status_code == 404 + assert "not found" in response.data["developer_message"] + + def test_404_downstream_not_accessible(self): + """ + Do we raise 404 if the user lacks read access on the specified downstream block? + """ + self.client.login(username="learner", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 404 + assert "not found" in response.data["developer_message"] + + +class GetDownstreamViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `GET /api/v2/contentstore/downstreams/...` inspects a downstream's link to an upstream. + """ + def call_api(self, usage_key_string): + return self.client.get(f"/api/contentstore/v2/downstreams/{usage_key_string}") + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_200_good_upstream(self): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 200 + assert response.data['upstream_ref'] == MOCK_UPSTREAM_REF + assert response.data['error_message'] is None + assert response.data['ready_to_sync'] is True + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_bad) + def test_200_bad_upstream(self): + """ + If the upstream link is broken, do we still return 200, but with an error message in body? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 200 + assert response.data['upstream_ref'] == MOCK_UPSTREAM_REF + assert response.data['error_message'] == MOCK_UPSTREAM_ERROR + assert response.data['ready_to_sync'] is False + + def test_200_no_upstream(self): + """ + If the upstream link is missing, do we still return 200, but with an error message in body? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key) + assert response.status_code == 200 + assert response.data['upstream_ref'] is None + assert "is not linked" in response.data['error_message'] + assert response.data['ready_to_sync'] is False + + +class PutDownstreamViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `PUT /api/v2/contentstore/downstreams/...` edits a downstream's link to an upstream. + """ + def call_api(self, usage_key_string, sync: str | None = None): + return self.client.put( + f"/api/contentstore/v2/downstreams/{usage_key_string}", + data={ + "upstream_ref": MOCK_UPSTREAM_REF, + **({"sync": sync} if sync else {}), + }, + content_type="application/json", + ) + + @patch.object(downstreams_views, "fetch_customizable_fields") + @patch.object(downstreams_views, "sync_from_upstream") + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_200_with_sync(self, mock_sync, mock_fetch): + """ + Does the happy path work (with sync=True)? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key, sync='true') + assert response.status_code == 200 + video_after = modulestore().get_item(self.regular_video_key) + assert mock_sync.call_count == 1 + assert mock_fetch.call_count == 0 + assert video_after.upstream == MOCK_UPSTREAM_REF + + @patch.object(downstreams_views, "fetch_customizable_fields") + @patch.object(downstreams_views, "sync_from_upstream") + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_200_no_sync(self, mock_sync, mock_fetch): + """ + Does the happy path work (with sync=False)? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key, sync='false') + assert response.status_code == 200 + video_after = modulestore().get_item(self.regular_video_key) + assert mock_sync.call_count == 0 + assert mock_fetch.call_count == 1 + assert video_after.upstream == MOCK_UPSTREAM_REF + + @patch.object(downstreams_views, "fetch_customizable_fields", side_effect=BadUpstream(MOCK_UPSTREAM_ERROR)) + def test_400(self, sync: str): + """ + Do we raise a 400 if the provided upstream reference is malformed or not accessible? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 400 + assert MOCK_UPSTREAM_ERROR in response.data['developer_message']['upstream_ref'] + video_after = modulestore().get_item(self.regular_video_key) + assert video_after.upstream is None + + +class DeleteDownstreamViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `DELETE /api/v2/contentstore/downstreams/...` severs a downstream's link to an upstream. + """ + def call_api(self, usage_key_string): + return self.client.delete(f"/api/contentstore/v2/downstreams/{usage_key_string}") + + @patch.object(downstreams_views, "sever_upstream_link") + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + def test_204(self, mock_sever): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 204 + assert mock_sever.call_count == 1 + + @patch.object(downstreams_views, "sever_upstream_link") + def test_204_no_upstream(self, mock_sever): + """ + If there's no upsream, do we still happily return 204? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key) + assert response.status_code == 204 + assert mock_sever.call_count == 1 + + +class _DownstreamSyncViewTestMixin(_DownstreamViewTestMixin): + """ + Shared tests between the /api/contentstore/v2/downstreams/.../sync endpoints. + """ + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_bad) + def test_400_bad_upstream(self): + """ + If the upstream link is bad, do we get a 400? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 400 + assert MOCK_UPSTREAM_ERROR in response.data["developer_message"][0] + + def test_400_no_upstream(self): + """ + If the upstream link is missing, do we get a 400? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.regular_video_key) + assert response.status_code == 400 + assert "is not linked" in response.data["developer_message"][0] + + +class PostDownstreamSyncViewTest(_DownstreamSyncViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `POST /api/v2/contentstore/downstreams/.../sync` initiates a sync from the linked upstream. + """ + def call_api(self, usage_key_string): + return self.client.post(f"/api/contentstore/v2/downstreams/{usage_key_string}/sync") + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + @patch.object(downstreams_views, "sync_from_upstream") + def test_200(self, mock_sync_from_upstream): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 200 + assert mock_sync_from_upstream.call_count == 1 + + +class DeleteDownstreamSyncViewtest(_DownstreamSyncViewTestMixin, SharedModuleStoreTestCase): + """ + Test that `DELETE /api/v2/contentstore/downstreams/.../sync` declines a sync from the linked upstream. + """ + def call_api(self, usage_key_string): + return self.client.delete(f"/api/contentstore/v2/downstreams/{usage_key_string}/sync") + + @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) + @patch.object(downstreams_views, "decline_sync") + def test_204(self, mock_decline_sync): + """ + Does the happy path work? + """ + self.client.login(username="superuser", password="password") + response = self.call_api(self.downstream_video_key) + assert response.status_code == 204 + assert mock_decline_sync.call_count == 1 diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 7979a422a331..5706b44e2cec 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -7,11 +7,13 @@ from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient from openedx_tagging.core.tagging.models import Tag +from organizations.models import Organization from xmodule.modulestore.django import contentstore, modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory, LibraryFactory from cms.djangoapps.contentstore.utils import reverse_usage_url +from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangoapps.content_tagging import api as tagging_api CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/" @@ -391,6 +393,78 @@ def test_paste_with_assets(self): assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged. +class ClipboardPasteFromV2LibraryTestCase(ModuleStoreTestCase): + """ + Test Clipboard Paste functionality with a "new" (as of Sumac) library + """ + + def setUp(self): + """ + Set up a v2 Content Library and a library content block + """ + super().setUp() + self.client = APIClient() + self.client.login(username=self.user.username, password=self.user_password) + self.store = modulestore() + + self.library = library_api.create_library( + library_type=library_api.COMPLEX, + org=Organization.objects.create(name="Test Org", short_name="CL-TEST"), + slug="lib", + title="Library", + ) + + self.lib_block_key = library_api.create_library_block(self.library.key, "problem", "p1").usage_key # v==1 + library_api.set_library_block_olx(self.lib_block_key, """ + + + + + Wrong + Right + + + + """) # v==2 + library_api.publish_changes(self.library.key) + library_api.set_library_block_olx(self.lib_block_key, """ + + + + + Wrong + Right + + + + """) # v==3 + lib_block_meta = library_api.get_library_block(self.lib_block_key) + assert lib_block_meta.published_version_num == 2 + assert lib_block_meta.draft_version_num == 3 + + self.course = CourseFactory.create(display_name='Course') + + def test_paste_from_library_creates_link(self): + """ + When we copy a v2 lib block into a course, the dest block should be linked up to the lib block. + """ + copy_response = self.client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(self.lib_block_key)}, format="json") + assert copy_response.status_code == 200 + + paste_response = self.client.post(XBLOCK_ENDPOINT, { + "parent_locator": str(self.course.usage_key), + "staged_content": "clipboard", + }, format="json") + assert paste_response.status_code == 200 + + new_block_key = UsageKey.from_string(paste_response.json()["locator"]) + new_block = modulestore().get_item(new_block_key) + assert new_block.upstream == str(self.lib_block_key) + assert new_block.upstream_version == 3 + assert new_block.upstream_display_name == "MCQ-draft" + assert new_block.upstream_max_attempts == 5 + + class ClipboardPasteFromV1LibraryTestCase(ModuleStoreTestCase): """ Test Clipboard Paste functionality with legacy (v1) library content diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 79137cfde11f..e4d37f942331 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -539,7 +539,8 @@ def _create_block(request): # Paste from the user's clipboard (content_staging app clipboard, not browser clipboard) into 'usage_key': try: created_xblock, notices = import_staged_content_from_user_clipboard( - parent_key=usage_key, request=request + parent_key=usage_key, + request=request, ) except Exception: # pylint: disable=broad-except log.exception( diff --git a/cms/envs/common.py b/cms/envs/common.py index 63221ee0b0a4..7297adae8354 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -127,6 +127,7 @@ from lms.djangoapps.lms_xblock.mixin import LmsBlockMixin from cms.lib.xblock.authoring_mixin import AuthoringMixin +from cms.lib.xblock.upstream_sync import UpstreamSyncMixin from xmodule.modulestore.edit_info import EditInfoMixin from openedx.core.djangoapps.theming.helpers_dirs import ( get_themes_unchecked, @@ -995,6 +996,7 @@ XModuleMixin, EditInfoMixin, AuthoringMixin, + UpstreamSyncMixin, ) # .. setting_name: XBLOCK_EXTRA_MIXINS diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py new file mode 100644 index 000000000000..5db020393eab --- /dev/null +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -0,0 +1,337 @@ +""" +Test CMS's upstream->downstream syncing system +""" +import ddt + +from organizations.api import ensure_organization +from organizations.models import Organization + +from cms.lib.xblock.upstream_sync import ( + UpstreamLink, + sync_from_upstream, decline_sync, fetch_customizable_fields, sever_upstream_link, + NoUpstream, BadUpstream, BadDownstream, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries import api as libs +from openedx.core.djangoapps.xblock import api as xblock +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory + + +@ddt.ddt +class UpstreamTestCase(ModuleStoreTestCase): + """ + Tests the upstream_sync mixin, data object, and Python APIs. + """ + + def setUp(self): + """ + Create a simple course with one unit, and simple V2 library with two blocks. + """ + super().setUp() + course = CourseFactory.create() + chapter = BlockFactory.create(category='chapter', parent=course) + sequential = BlockFactory.create(category='sequential', parent=chapter) + self.unit = BlockFactory.create(category='vertical', parent=sequential) + + ensure_organization("TestX") + self.library = libs.create_library( + org=Organization.objects.get(short_name="TestX"), + slug="TestLib", + title="Test Upstream Library", + ) + self.upstream_key = libs.create_library_block(self.library.key, "html", "test-upstream").usage_key + libs.create_library_block(self.library.key, "video", "video-upstream") + + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.display_name = "Upstream Title V2" + upstream.data = "Upstream content V2" + upstream.save() + + def test_sync_bad_downstream(self): + """ + Syncing into an unsupported downstream (such as a another Content Library block) raises BadDownstream, but + doesn't affect the block. + """ + downstream_lib_block_key = libs.create_library_block(self.library.key, "html", "bad-downstream").usage_key + downstream_lib_block = xblock.load_block(downstream_lib_block_key, self.user) + downstream_lib_block.display_name = "Another lib block" + downstream_lib_block.data = "another lib block" + downstream_lib_block.upstream = str(self.upstream_key) + downstream_lib_block.save() + + with self.assertRaises(BadDownstream): + sync_from_upstream(downstream_lib_block, self.user) + + assert downstream_lib_block.display_name == "Another lib block" + assert downstream_lib_block.data == "another lib block" + + def test_sync_no_upstream(self): + """ + Trivial case: Syncing a block with no upstream is a no-op + """ + block = BlockFactory.create(category='html', parent=self.unit) + block.display_name = "Block Title" + block.data = "Block content" + + with self.assertRaises(NoUpstream): + sync_from_upstream(block, self.user) + + assert block.display_name == "Block Title" + assert block.data == "Block content" + assert not block.upstream_display_name + + @ddt.data( + ("not-a-key-at-all", ".*is malformed.*"), + ("course-v1:Oops+ItsA+CourseKey", ".*is malformed.*"), + ("block-v1:The+Wrong+KindOfUsageKey+type@html+block@nope", ".*is malformed.*"), + ("lb:TestX:NoSuchLib:html:block-id", ".*not found in the system.*"), + ("lb:TestX:TestLib:video:should-be-html-but-is-a-video", ".*type mismatch.*"), + ("lb:TestX:TestLib:html:no-such-html", ".*not found in the system.*"), + ) + @ddt.unpack + def test_sync_bad_upstream(self, upstream, message_regex): + """ + Syncing with a bad upstream raises BadUpstream, but doesn't affect the block + """ + block = BlockFactory.create(category='html', parent=self.unit, upstream=upstream) + block.display_name = "Block Title" + block.data = "Block content" + + with self.assertRaisesRegex(BadUpstream, message_regex): + sync_from_upstream(block, self.user) + + assert block.display_name == "Block Title" + assert block.data == "Block content" + assert not block.upstream_display_name + + def test_sync_not_accessible(self): + """ + Syncing with an block that exists, but is inaccessible, raises BadUpstream + """ + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + user_who_cannot_read_upstream = UserFactory.create(username="rando", is_staff=False, is_superuser=False) + with self.assertRaisesRegex(BadUpstream, ".*could not be loaded.*") as exc: + sync_from_upstream(downstream, user_who_cannot_read_upstream) + + def test_sync_updates_happy_path(self): + """ + Can we sync updates from a content library block to a linked out-of-date course block? + """ + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + + # Initial sync + sync_from_upstream(downstream, self.user) + assert downstream.upstream_version == 2 # Library blocks start at version 2 (v1 is the empty new block) + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + + # Upstream updates + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.display_name = "Upstream Title V3" + upstream.data = "Upstream content V3" + upstream.save() + + # Follow-up sync. Assert that updates are pulled into downstream. + sync_from_upstream(downstream, self.user) + assert downstream.upstream_version == 3 + assert downstream.upstream_display_name == "Upstream Title V3" + assert downstream.display_name == "Upstream Title V3" + assert downstream.data == "Upstream content V3" + + def test_sync_updates_to_modified_content(self): + """ + If we sync to modified content, will it preserve customizable fields, but overwrite the rest? + """ + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + + # Initial sync + sync_from_upstream(downstream, self.user) + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + + # Upstream updates + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.display_name = "Upstream Title V3" + upstream.data = "Upstream content V3" + upstream.save() + + # Downstream modifications + downstream.display_name = "Downstream Title Override" # "safe" customization + downstream.data = "Downstream content override" # "unsafe" override + downstream.save() + + # Follow-up sync. Assert that updates are pulled into downstream, but customizations are saved. + sync_from_upstream(downstream, self.user) + assert downstream.upstream_display_name == "Upstream Title V3" + assert downstream.display_name == "Downstream Title Override" # "safe" customization survives + assert downstream.data == "Upstream content V3" # "unsafe" override is gone + + # For the Content Libraries Relaunch Beta, we do not yet need to support this edge case. + # See "PRESERVING DOWNSTREAM CUSTOMIZATIONS and RESTORING UPSTREAM DEFAULTS" in cms/lib/xblock/upstream_sync.py. + # + # def test_sync_to_downstream_with_subtle_customization(self): + # """ + # Edge case: If our downstream customizes a field, but then the upstream is changed to match the + # customization do we still remember that the downstream field is customized? That is, + # if the upstream later changes again, do we retain the downstream customization (rather than + # following the upstream update?) + # """ + # # Start with an uncustomized downstream block. + # downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + # sync_from_upstream(downstream, self.user) + # assert downstream.downstream_customized == [] + # assert downstream.display_name == downstream.upstream_display_name == "Upstream Title V2" + # + # # Then, customize our downstream title. + # downstream.display_name = "Title V3" + # downstream.save() + # assert downstream.downstream_customized == ["display_name"] + # + # # Syncing should retain the customization. + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 2 + # assert downstream.upstream_display_name == "Upstream Title V2" + # assert downstream.display_name == "Title V3" + # + # # Whoa, look at that, the upstream has updated itself to the exact same title... + # upstream = xblock.load_block(self.upstream_key, self.user) + # upstream.display_name = "Title V3" + # upstream.save() + # + # # ...which is reflected when we sync. + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 3 + # assert downstream.upstream_display_name == downstream.display_name == "Title V3" + # + # # But! Our downstream knows that its title is still customized. + # assert downstream.downstream_customized == ["display_name"] + # # So, if the upstream title changes again... + # upstream.display_name = "Title V4" + # upstream.save() + # + # # ...then the downstream title should remain put. + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 4 + # assert downstream.upstream_display_name == "Title V4" + # assert downstream.display_name == "Title V3" + # + # # Finally, if we "de-customize" the display_name field, then it should go back to syncing normally. + # downstream.downstream_customized = [] + # upstream.display_name = "Title V5" + # upstream.save() + # sync_from_upstream(downstream, self.user) + # assert downstream.upstream_version == 5 + # assert downstream.upstream_display_name == downstream.display_name == "Title V5" + + @ddt.data(None, "Title From Some Other Upstream Version") + def test_fetch_customizable_fields(self, initial_upstream_display_name): + """ + Can we fetch a block's upstream field values without syncing it? + + Test both with and without a pre-"fetched" upstrema values on the downstream. + """ + downstream = BlockFactory.create(category='html', parent=self.unit) + downstream.upstream_display_name = initial_upstream_display_name + downstream.display_name = "Some Title" + downstream.data = "Some content" + + # Note that we're not linked to any upstream. fetch_customizable_fields shouldn't care. + assert not downstream.upstream + assert not downstream.upstream_version + + # fetch! + upstream = xblock.load_block(self.upstream_key, self.user) + fetch_customizable_fields(upstream=upstream, downstream=downstream, user=self.user) + + # Ensure: fetching doesn't affect the upstream link (or lack thereof). + assert not downstream.upstream + assert not downstream.upstream_version + + # Ensure: fetching doesn't affect actual content or settings. + assert downstream.display_name == "Some Title" + assert downstream.data == "Some content" + + # Ensure: fetching DOES set the upstream_* fields. + assert downstream.upstream_display_name == "Upstream Title V2" + + def test_prompt_and_decline_sync(self): + """ + Is the user prompted for sync when it's available? Does declining remove the prompt until a new sync is ready? + """ + # Initial conditions (pre-sync) + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced is None + assert link.version_declined is None + assert link.version_available == 2 # Library block with content starts at version 2 + assert link.ready_to_sync is True + + # Initial sync to V2 + sync_from_upstream(downstream, self.user) + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined is None + assert link.version_available == 2 + assert link.ready_to_sync is False + + # Upstream updated to V3 + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.data = "Upstream content V3" + upstream.save() + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined is None + assert link.version_available == 3 + assert link.ready_to_sync is True + + # Decline to sync to V3 -- ready_to_sync becomes False. + decline_sync(downstream) + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined == 3 + assert link.version_available == 3 + assert link.ready_to_sync is False + + # Upstream updated to V4 -- ready_to_sync becomes True again. + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.data = "Upstream content V4" + upstream.save() + link = UpstreamLink.get_for_block(downstream) + assert link.version_synced == 2 + assert link.version_declined == 3 + assert link.version_available == 4 + assert link.ready_to_sync is True + + def test_sever_upstream_link(self): + """ + Does sever_upstream_link correctly disconnect a block from its upstream? + """ + # Start with a course block that is linked+synced to a content library block. + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + sync_from_upstream(downstream, self.user) + + # (sanity checks) + assert downstream.upstream == str(self.upstream_key) + assert downstream.upstream_version == 2 + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + assert downstream.copied_from_block is None + + # Now, disconnect the course block. + sever_upstream_link(downstream) + + # All upstream metadata has been wiped out. + assert downstream.upstream is None + assert downstream.upstream_version is None + assert downstream.upstream_display_name is None + + # BUT, the content which was synced into the upstream remains. + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + + # AND, we have recorded the old upstream as our copied_from_block. + assert downstream.copied_from_block == str(self.upstream_key) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py new file mode 100644 index 000000000000..785cc7dc7e36 --- /dev/null +++ b/cms/lib/xblock/upstream_sync.py @@ -0,0 +1,451 @@ +""" +Synchronize content and settings from upstream blocks to their downstream usages. + +At the time of writing, we assume that for any upstream-downstream linkage: +* The upstream is a Component from a Learning Core-backed Content Library. +* The downstream is a block of matching type in a SplitModuleStore-backed Course. +* They are both on the same Open edX instance. + +HOWEVER, those assumptions may loosen in the future. So, we consider these to be INTERNAL ASSUMPIONS that should not be +exposed through this module's public Python interface. +""" +from __future__ import annotations + +import logging +import typing as t +from dataclasses import dataclass, asdict + +from django.core.exceptions import PermissionDenied +from django.utils.translation import gettext_lazy as _ +from rest_framework.exceptions import NotFound +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from xblock.exceptions import XBlockNotFoundError +from xblock.fields import Scope, String, Integer +from xblock.core import XBlockMixin, XBlock + +if t.TYPE_CHECKING: + from django.contrib.auth.models import User # pylint: disable=imported-auth-user + + +logger = logging.getLogger(__name__) + + +class UpstreamLinkException(Exception): + """ + Raised whenever we try to inspect, sync-from, fetch-from, or delete a block's link to upstream content. + + There are three flavors (defined below): BadDownstream, BadUpstream, NoUpstream. + + Should be constructed with a human-friendly, localized, PII-free message, suitable for API responses and UI display. + For now, at least, the message can assume that upstreams are Content Library blocks and downstreams are Course + blocks, although that may need to change (see module docstring). + """ + + +class BadDownstream(UpstreamLinkException): + """ + Downstream content does not support sync. + """ + + +class BadUpstream(UpstreamLinkException): + """ + Reference to upstream content is malformed, invalid, and/or inaccessible. + """ + + +class NoUpstream(UpstreamLinkException): + """ + The downstream content does not have an upstream link at all (...as is the case for most XBlocks usages). + + (This isn't so much an "error" like the other two-- it's just a case that needs to be handled exceptionally, + usually by logging a message and then doing nothing.) + """ + def __init__(self): + super().__init__(_("Content is not linked to a Content Library.")) + + +@dataclass(frozen=True) +class UpstreamLink: + """ + Metadata about some downstream content's relationship with its linked upstream content. + """ + upstream_ref: str | None # Reference to the upstream content, e.g., a serialized library block usage key. + version_synced: int | None # Version of the upstream to which the downstream was last synced. + version_available: int | None # Latest version of the upstream that's available, or None if it couldn't be loaded. + version_declined: int | None # Latest version which the user has declined to sync with, if any. + error_message: str | None # If link is valid, None. Otherwise, a localized, human-friendly error message. + + @property + def ready_to_sync(self) -> bool: + """ + Should we invite the downstream's authors to sync the latest upstream updates? + """ + return bool( + self.upstream_ref and + self.version_available and + self.version_available > (self.version_synced or 0) and + self.version_available > (self.version_declined or 0) + ) + + def to_json(self) -> dict[str, t.Any]: + """ + Get an JSON-API-friendly representation of this upstream link. + """ + return { + **asdict(self), + "ready_to_sync": self.ready_to_sync, + } + + @classmethod + def try_get_for_block(cls, downstream: XBlock) -> t.Self: + """ + Same as `get_for_block`, but upon failure, sets `.error_message` instead of raising an exception. + """ + try: + return cls.get_for_block(downstream) + except UpstreamLinkException as exc: + logger.exception( + "Tried to inspect an unsupported, broken, or missing downstream->upstream link: '%s'->'%s'", + downstream.usage_key, + downstream.upstream, + ) + return cls( + upstream_ref=downstream.upstream, + version_synced=downstream.upstream_version, + version_available=None, + version_declined=None, + error_message=str(exc), + ) + + @classmethod + def get_for_block(cls, downstream: XBlock) -> t.Self: + """ + Get info on a block's relationship with its linked upstream content (without actually loading the content). + + Currently, the only supported upstreams are LC-backed Library Components. This may change in the future (see + module docstring). + + If link exists, is supported, and is followable, returns UpstreamLink. + Otherwise, raises an UpstreamLinkException. + """ + if not downstream.upstream: + raise NoUpstream() + if not isinstance(downstream.usage_key.context_key, CourseKey): + raise BadDownstream(_("Cannot update content because it does not belong to a course.")) + if downstream.has_children: + raise BadDownstream(_("Updating content with children is not yet supported.")) + try: + upstream_key = LibraryUsageLocatorV2.from_string(downstream.upstream) + except InvalidKeyError as exc: + raise BadUpstream(_("Reference to linked library item is malformed")) from exc + downstream_type = downstream.usage_key.block_type + if upstream_key.block_type != downstream_type: + # Note: Currently, we strictly enforce that the downstream and upstream block_types must exactly match. + # It could be reasonable to relax this requirement in the future if there's product need for it. + # For example, there's no reason that a StaticTabBlock couldn't take updates from an HtmlBlock. + raise BadUpstream( + _("Content type mismatch: {downstream_type} cannot be linked to {upstream_type}.").format( + downstream_type=downstream_type, upstream_type=upstream_key.block_type + ) + ) from TypeError( + f"downstream block '{downstream.usage_key}' is linked to " + f"upstream block of different type '{upstream_key}'" + ) + # We import this here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.content_libraries.api import ( + get_library_block # pylint: disable=wrong-import-order + ) + try: + lib_meta = get_library_block(upstream_key) + except XBlockNotFoundError as exc: + raise BadUpstream(_("Linked library item was not found in the system")) from exc + return cls( + upstream_ref=downstream.upstream, + version_synced=downstream.upstream_version, + version_available=(lib_meta.draft_version_num if lib_meta else None), + # TODO: Previous line is wrong. It should use the published version instead, but the + # LearningCoreXBlockRuntime APIs do not yet support published content yet. + # Will be fixed in a follow-up task: https://github.com/openedx/edx-platform/issues/35582 + # version_available=(lib_meta.published_version_num if lib_meta else None), + version_declined=downstream.upstream_version_declined, + error_message=None, + ) + + +def sync_from_upstream(downstream: XBlock, user: User) -> None: + """ + Update `downstream` with content+settings from the latest available version of its linked upstream content. + + Preserves overrides to customizable fields; overwrites overrides to other fields. + Does not save `downstream` to the store. That is left up to the caller. + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + link, upstream = _load_upstream_link_and_block(downstream, user) + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) + _update_non_customizable_fields(upstream=upstream, downstream=downstream) + downstream.upstream_version = link.version_available + + +def fetch_customizable_fields(*, downstream: XBlock, user: User, upstream: XBlock | None = None) -> None: + """ + Fetch upstream-defined value of customizable fields and save them on the downstream. + + If `upstream` is provided, use that block as the upstream. + Otherwise, load the block specified by `downstream.upstream`, which may raise an UpstreamLinkException. + """ + if not upstream: + _link, upstream = _load_upstream_link_and_block(downstream, user) + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) + + +def _load_upstream_link_and_block(downstream: XBlock, user: User) -> tuple[UpstreamLink, XBlock]: + """ + Load the upstream metadata and content for a downstream block. + + Assumes that the upstream content is an XBlock in an LC-backed content libraries. This assumption may need to be + relaxed in the future (see module docstring). + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException + # We import load_block here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.xblock.api import load_block # pylint: disable=wrong-import-order + try: + lib_block: XBlock = load_block(LibraryUsageLocatorV2.from_string(downstream.upstream), user) + except (NotFound, PermissionDenied) as exc: + raise BadUpstream(_("Linked library item could not be loaded: {}").format(downstream.upstream)) from exc + return link, lib_block + + +def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fetch: bool) -> None: + """ + For each customizable field: + * Save the upstream value to a hidden field on the downstream ("FETCH"). + * If `not only_fetch`, and if the field *isn't* customized on the downstream, then: + * Update it the downstream field's value from the upstream field ("SYNC"). + + Concrete example: Imagine `lib_problem` is our upstream and `course_problem` is our downstream. + + * Say that the customizable fields are [display_name, max_attempts]. + + * Set `course_problem.upstream_display_name = lib_problem.display_name` ("fetch"). + * If `not only_fetch`, and `course_problem.display_name` wasn't customized, then: + * Set `course_problem.display_name = lib_problem.display_name` ("sync"). + + * Set `course_problem.upstream_max_attempts = lib_problem.max_attempts` ("fetch"). + * If `not only_fetch`, and `course_problem.max_attempts` wasn't customized, then: + * Set `course_problem.max_attempts = lib_problem.max_attempts` ("sync"). + """ + syncable_field_names = _get_synchronizable_fields(upstream, downstream) + + for field_name, fetch_field_name in downstream.get_customizable_fields().items(): + + if field_name not in syncable_field_names: + continue + + # FETCH the upstream's value and save it on the downstream (ie, `downstream.upstream_$FIELD`). + old_upstream_value = getattr(downstream, fetch_field_name) + new_upstream_value = getattr(upstream, field_name) + setattr(downstream, fetch_field_name, new_upstream_value) + + if only_fetch: + continue + + # Okay, now for the nuanced part... + # We need to update the downstream field *iff it has not been customized**. + # Determining whether a field has been customized will differ in Beta vs Future release. + # (See "PRESERVING DOWNSTREAM CUSTOMIZATIONS" comment below for details.) + + ## FUTURE BEHAVIOR: field is "customized" iff we have noticed that the user edited it. + # if field_name in downstream.downstream_customized: + # continue + + ## BETA BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. + downstream_value = getattr(downstream, field_name) + if old_upstream_value and downstream_value != old_upstream_value: + continue # Field has been customized. Don't touch it. Move on. + + # Field isn't customized -- SYNC it! + setattr(downstream, field_name, new_upstream_value) + + +def _update_non_customizable_fields(*, upstream: XBlock, downstream: XBlock) -> None: + """ + For each field `downstream.blah` that isn't customizable: set it to `upstream.blah`. + """ + syncable_fields = _get_synchronizable_fields(upstream, downstream) + customizable_fields = set(downstream.get_customizable_fields().keys()) + for field_name in syncable_fields - customizable_fields: + new_upstream_value = getattr(upstream, field_name) + setattr(downstream, field_name, new_upstream_value) + + +def _get_synchronizable_fields(upstream: XBlock, downstream: XBlock) -> set[str]: + """ + The syncable fields are the ones which are content- or settings-scoped AND are defined on both (up,down)stream. + """ + return set.intersection(*[ + set( + field_name + for (field_name, field) in block.__class__.fields.items() + if field.scope in [Scope.settings, Scope.content] + ) + for block in [upstream, downstream] + ]) + + +def decline_sync(downstream: XBlock) -> None: + """ + Given an XBlock that is linked to upstream content, mark the latest available update as 'declined' so that its + authors are not prompted (until another upstream version becomes available). + + Does not save `downstream` to the store. That is left up to the caller. + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + upstream_link = UpstreamLink.get_for_block(downstream) # Can raise UpstreamLinkException + downstream.upstream_version_declined = upstream_link.version_available + + +def sever_upstream_link(downstream: XBlock) -> None: + """ + Given an XBlock that is linked to upstream content, disconnect the link, such that authors are never again prompted + to sync upstream updates. Erase all `.upstream*` fields from the downtream block. + + However, before nulling out the `.upstream` field, we copy its value over to `.copied_from_block`. This makes sense, + because once a downstream block has been de-linked from source (e.g., a Content Library block), it is no different + than if the block had just been copy-pasted in the first place. + + Does not save `downstream` to the store. That is left up to the caller. + + If `downstream` lacks a link, then this raises NoUpstream (though it is reasonable for callers to handle such + exception and ignore it, as the end result is the same: `downstream.upstream is None`). + """ + if not downstream.upstream: + raise NoUpstream() + downstream.copied_from_block = downstream.upstream + downstream.upstream = None + downstream.upstream_version = None + for _, fetched_upstream_field in downstream.get_customizable_fields().items(): + setattr(downstream, fetched_upstream_field, None) # Null out upstream_display_name, et al. + + +class UpstreamSyncMixin(XBlockMixin): + """ + Allows an XBlock in the CMS to be associated & synced with an upstream. + + Mixed into CMS's XBLOCK_MIXINS, but not LMS's. + """ + + # Upstream synchronization metadata fields + upstream = String( + help=( + "The usage key of a block (generally within a content library) which serves as a source of upstream " + "updates for this block, or None if there is no such upstream. Please note: It is valid for this " + "field to hold a usage key for an upstream block that does not exist (or does not *yet* exist) on " + "this instance, particularly if this downstream block was imported from a different instance." + ), + default=None, scope=Scope.settings, hidden=True, enforce_type=True + ) + upstream_version = Integer( + help=( + "Record of the upstream block's version number at the time this block was created from it. If this " + "upstream_version is smaller than the upstream block's latest published version, then the author will be " + "invited to sync updates into this downstream block, presuming that they have not already declined to sync " + "said version." + ), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + upstream_version_declined = Integer( + help=( + "Record of the latest upstream version for which the author declined to sync updates, or None if they have " + "never declined an update." + ), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + + # Store the fetched upstream values for customizable fields. + upstream_display_name = String( + help=("The value of display_name on the linked upstream block."), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + upstream_max_attempts = Integer( + help=("The value of max_attempts on the linked upstream block."), + default=None, scope=Scope.settings, hidden=True, enforce_type=True, + ) + + @classmethod + def get_customizable_fields(cls) -> dict[str, str]: + """ + Mapping from each customizable field to the field which can be used to restore its upstream value. + + XBlocks outside of edx-platform can override this in order to set up their own customizable fields. + """ + return { + "display_name": "upstream_display_name", + "max_attempts": "upstream_max_attempts", + } + + # PRESERVING DOWNSTREAM CUSTOMIZATIONS and RESTORING UPSTREAM VALUES + # + # For the full Content Libraries Relaunch, we would like to keep track of which customizable fields the user has + # actually customized. The idea is: once an author has customized a customizable field.... + # + # - future upstream syncs will NOT blow away the customization, + # - but future upstream syncs WILL fetch the upstream values and tuck them away in a hidden field, + # - and the author can can revert back to said fetched upstream value at any point. + # + # Now, whether field is "customized" (and thus "revertible") is dependent on whether they have ever edited it. + # To instrument this, we need to keep track of which customizable fields have been edited using a new XBlock field: + # `downstream_customized` + # + # Implementing `downstream_customized` has proven difficult, because there is no simple way to keep it up-to-date + # with the many different ways XBlock fields can change. The `.save()` and `.editor_saved()` methods are promising, + # but we need to do more due diligence to be sure that they cover all cases, including API edits, import/export, + # copy/paste, etc. We will figure this out in time for the full Content Libraries Relaunch (related ticket: + # https://github.com/openedx/frontend-app-authoring/issues/1317). But, for the Beta realease, we're going to + # implement something simpler: + # + # - We fetch upstream values for customizable fields and tuck them away in a hidden field (same as above). + # - If a customizable field DOES match the fetched upstream value, then future upstream syncs DO update it. + # - If a customizable field does NOT the fetched upstream value, then future upstream syncs DO NOT update it. + # - There is no UI option for explicitly reverting back to the fetched upstream value. + # + # For future reference, here is a partial implementation of what we are thinking for the full Content Libraries + # Relaunch:: + # + # downstream_customized = List( + # help=( + # "Names of the fields which have values set on the upstream block yet have been explicitly " + # "overridden on this downstream block. Unless explicitly cleared by the user, these customizations " + # "will persist even when updates are synced from the upstream." + # ), + # default=[], scope=Scope.settings, hidden=True, enforce_type=True, + # ) + # + # def save(self, *args, **kwargs): + # """ + # Update `downstream_customized` when a customizable field is modified. + # + # NOTE: This does not work, because save() isn't actually called in all the cases that we'd want it to be. + # """ + # super().save(*args, **kwargs) + # customizable_fields = self.get_customizable_fields() + # + # # Loop through all the fields that are potentially cutomizable. + # for field_name, restore_field_name in self.get_customizable_fields(): + # + # # If the field is already marked as customized, then move on so that we don't + # # unneccessarily query the block for its current value. + # if field_name in self.downstream_customized: + # continue + # + # # If this field's value doesn't match the synced upstream value, then mark the field + # # as customized so that we don't clobber it later when syncing. + # # NOTE: Need to consider the performance impact of all these field lookups. + # if getattr(self, field_name) != getattr(self, restore_field_name): + # self.downstream_customized.append(field_name) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 282629456633..0ee1099ca982 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -8,6 +8,7 @@ dump_js_escaped_json, js_escaped_string ) from cms.djangoapps.contentstore.toggles import use_new_text_editor, use_new_problem_editor, use_new_video_editor, use_video_gallery_flow +from cms.lib.xblock.upstream_sync import UpstreamLink from openedx.core.djangoapps.content_tagging.toggles import is_tagging_feature_disabled %> <% @@ -23,6 +24,8 @@ messages = xblock.validate().to_json() has_not_configured_message = messages.get('summary',{}).get('type', None) == 'not-configured' block_is_unit = is_unit(xblock) + +upstream_info = UpstreamLink.try_get_for_block(xblock) %> <%namespace name='static' file='static_content.html'/> @@ -103,7 +106,22 @@ ${label} % else: - ${label} + % if upstream_info.upstream_ref: + % if upstream_info.error_message: + + + ${_("Sourced from a library - but the upstream link is broken/invalid.")} + % else: + + + ${_("Sourced from a library.")} + % endif + % endif + ${label} % endif % if selected_groups_label:

${selected_groups_label}

@@ -114,6 +132,18 @@
    % if not is_root: % if can_edit: + % if upstream_info.ready_to_sync: +
  • + +
    • + % endif % if use_tagging:
  • % endif diff --git a/docs/decisions/0020-upstream-downstream.rst b/docs/decisions/0020-upstream-downstream.rst index 8ceb9e775274..3f6bbacfb839 100644 --- a/docs/decisions/0020-upstream-downstream.rst +++ b/docs/decisions/0020-upstream-downstream.rst @@ -257,6 +257,11 @@ To support the Libraries Relaunch in Sumac: For reference, here are some excerpts of a potential implementation. This may change through development and code review. +(UPDATE: When implementing, we ended up factoring this code differently. +Particularly, we opted to use regular functions rather than add new +XBlock Runtime methods, allowing us to avoid mucking with the complicated +inheritance hierarchy of CachingDescriptorSystem and SplitModuleStoreRuntime.) + .. code-block:: python ########################################################################### diff --git a/mypy.ini b/mypy.ini index 5027c3bb5595..4e69ef616391 100644 --- a/mypy.ini +++ b/mypy.ini @@ -6,7 +6,9 @@ plugins = mypy_django_plugin.main, mypy_drf_plugin.main files = - openedx/core/djangoapps/content/learning_sequences/, + cms/lib/xblock/upstream_sync.py, + cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py, + openedx/core/djangoapps/content/learning_sequences, openedx/core/djangoapps/content_staging, openedx/core/djangoapps/content_libraries, openedx/core/djangoapps/xblock, diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 90b73ef7f474..7d4da2a998c5 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -225,6 +225,8 @@ class LibraryXBlockMetadata: usage_key = attr.ib(type=LibraryUsageLocatorV2) created = attr.ib(type=datetime) modified = attr.ib(type=datetime) + draft_version_num = attr.ib(type=int) + published_version_num = attr.ib(default=None, type=int) display_name = attr.ib("") last_published = attr.ib(default=None, type=datetime) last_draft_created = attr.ib(default=None, type=datetime) @@ -246,6 +248,7 @@ def from_component(cls, library_key, component, associated_collections=None): published_by = last_publish_log.published_by.username draft = component.versioning.draft + published = component.versioning.published last_draft_created = draft.created if draft else None last_draft_created_by = draft.publishable_entity_version.created_by if draft else None @@ -254,9 +257,11 @@ def from_component(cls, library_key, component, associated_collections=None): library_key, component, ), - display_name=component.versioning.draft.title, + display_name=draft.title, created=component.created, - modified=component.versioning.draft.created, + modified=draft.created, + draft_version_num=draft.version_num, + published_version_num=published.version_num if published else None, last_published=None if last_publish_log is None else last_publish_log.published_at, published_by=published_by, last_draft_created=last_draft_created, diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 8632d0cb0740..5efaa28d670c 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -34,7 +34,7 @@ log = logging.getLogger(__name__) -def save_xblock_to_user_clipboard(block: XBlock, user_id: int) -> UserClipboardData: +def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int | None = None) -> UserClipboardData: """ Copy an XBlock's OLX to the user's clipboard. """ @@ -64,6 +64,7 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int) -> UserClipboardD display_name=block_metadata_utils.display_name_with_default(block), suggested_url_name=usage_key.block_id, tags=block_data.tags, + version_num=(version_num or 0), ) (clipboard, _created) = _UserClipboard.objects.update_or_create(user_id=user_id, defaults={ "content": staged_content, @@ -205,6 +206,7 @@ def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardDat block_type=content.block_type, display_name=content.display_name, tags=content.tags, + version_num=content.version_num, ), source_usage_key=clipboard.source_usage_key, source_context_title=clipboard.get_source_context_title(), diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index e952357f4cf9..d077d05a0aa4 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -43,6 +43,7 @@ class StagedContentData: block_type: str = field(validator=validators.instance_of(str)) display_name: str = field(validator=validators.instance_of(str)) tags: dict = field(validator=validators.optional(validators.instance_of(dict))) + version_num: int = field(validator=validators.instance_of(int)) @frozen diff --git a/openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py b/openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py new file mode 100644 index 000000000000..c3438ecb813b --- /dev/null +++ b/openedx/core/djangoapps/content_staging/migrations/0005_stagedcontent_version_num.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.16 on 2024-10-09 16:07 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_staging', '0004_stagedcontent_tags'), + ] + + operations = [ + migrations.AddField( + model_name='stagedcontent', + name='version_num', + field=models.PositiveIntegerField(default=0), + ), + ] diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 334fc1c6b5d3..2eab7954e826 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -63,6 +63,8 @@ class Meta: # A _suggested_ URL name to use for this content. Since this suggestion may already be in use, it's fine to generate # a new url_name instead. suggested_url_name = models.CharField(max_length=1024) + # If applicable, an int >=1 indicating the version of copied content. If not applicable, zero (default). + version_num = models.PositiveIntegerField(default=0) # Tags applied to the original source block(s) will be copied to the new block(s) on paste. tags = models.JSONField(null=True, help_text=_("Content tags applied to these blocks")) diff --git a/openedx/core/djangoapps/content_staging/views.py b/openedx/core/djangoapps/content_staging/views.py index 1b9790cfbeee..2a08ccfd3873 100644 --- a/openedx/core/djangoapps/content_staging/views.py +++ b/openedx/core/djangoapps/content_staging/views.py @@ -101,6 +101,7 @@ def post(self, request): "You must be a member of the course team in Studio to export OLX using this API." ) block = modulestore().get_item(usage_key) + version_num = None elif isinstance(course_key, LibraryLocatorV2): lib_api.require_permission_for_library_key( @@ -109,6 +110,7 @@ def post(self, request): lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) block = xblock_api.load_block(usage_key, user=None) + version_num = lib_api.get_library_block(usage_key).draft_version_num else: raise ValidationError("Invalid usage_key for the content.") @@ -116,7 +118,7 @@ def post(self, request): except ItemNotFoundError as exc: raise NotFound("The requested usage key does not exist.") from exc - clipboard = api.save_xblock_to_user_clipboard(block=block, user_id=request.user.id) + clipboard = api.save_xblock_to_user_clipboard(block=block, version_num=version_num, user_id=request.user.id) # Return the current clipboard exactly as if GET was called: serializer = UserClipboardSerializer(clipboard, context={"request": request}) From 451012460de15fef4798c218da04219266a1c183 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 15 Oct 2024 13:01:08 -0400 Subject: [PATCH 5/6] feat: versioned asset support for Learning Core XBlock runtime Add support for displaying static assets in the Learing Core XBlock runtime via "/static/asset-name" style substitutions in the OLX. This is currently used for new Content Library components. Static asset display is version-aware, so viewing older versions of the XBlock content via the embed view will show the appropriate assets for that version. --- .../tests/test_embed_block.py | 53 ++++++- .../xblock/runtime/learning_core_runtime.py | 143 +++++++++++++++++- .../lib/xblock_serializer/block_serializer.py | 39 +++-- 3 files changed, 213 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py index 712117e3d245..a554e6157e8e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py @@ -48,7 +48,7 @@ class LibrariesEmbedViewTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestM """ @XBlock.register_temp_plugin(FieldsTestBlock, FieldsTestBlock.BLOCK_TYPE) - def test_embed_vew_versions(self): + def test_embed_view_versions(self): """ Test that the embed_view renders a block and can render different versions of it. """ @@ -177,8 +177,55 @@ def check_fields(display_name, setting_field, content_field): 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". + def test_embed_view_versions_static_assets(self): + """ + Test asset substitution and version-awareness. + """ + # Create a library: + lib = self._create_library( + slug="test-eb-asset-1", title="Asset Test Library", description="", + ) + lib_id = lib["id"] + + # Create an HTMLBlock. This will be the empty version 1: + create_response = self._add_block_to_library(lib_id, "html", "asset_block") + block_id = create_response["id"] + + # Create version 2 of the block by setting its OLX. This has a reference + # to an image, but not the image itself–so it won't get auto-replaced. + olx_response = self._set_library_block_olx(block_id, """ + This is the enemy of our garden:

    +

    + ]]> + """) + assert olx_response["version_num"] == 2 + + # Create version 3 with some bogus file data + self._set_library_block_asset(block_id, "static/deer.jpg", b"This is not a valid JPEG file") + + # Publish the library (making version 3 the published state): + self._commit_library_changes(lib_id) + + # Create version 4 by deleting the asset + self._delete_library_block_asset(block_id, "static/deer.jpg") + + # Grab version 2, which has the asset reference but not the asset. No + # substitution should happen. + html = self._embed_block(block_id, version=2) + assert 'src="/static/deer.jpg"' in html + + # Grab the published version 3. This has the asset, so the link should + # show up. + html = self._embed_block(block_id, version='published') + # This is the pattern we're looking for: + # + assert re.search(r'/library_assets/[0-9a-f-]*/static/deer.jpg', html) + + # Now grab the draft version (4), which is going to once again not have + # the asset (because we deleted it). + html = self._embed_block(block_id, version='draft') + assert 'src="/static/deer.jpg"' in html # 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/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index 8d22626e081d..0942a5a8b3c7 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -9,6 +9,7 @@ from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.transaction import atomic +from django.urls import reverse from openedx_learning.api import authoring as authoring_api @@ -19,7 +20,9 @@ from xblock.fields import Field, Scope, ScopeIds from xblock.field_data import FieldData +from openedx.core.djangoapps.xblock.api import get_xblock_app_config from openedx.core.lib.xblock_serializer.api import serialize_modulestore_block_for_learning_core +from openedx.core.lib.xblock_serializer.data import StaticFile from ..data import AuthoredDataMode, LatestVersion from ..learning_context.manager import get_learning_context_impl from .runtime import XBlockRuntime @@ -230,6 +233,33 @@ def get_block(self, usage_key, for_parent=None, *, version: int | LatestVersion return block + def get_block_assets(self, block): + """ + Return a list of StaticFile entries. + + TODO: When we want to copy a whole Section at a time, doing these + lookups one by one is going to get slow. At some point we're going to + want something to look up a bunch of blocks at once. + """ + component_version = self._get_component_version_from_block(block) + + # cvc = the ComponentVersionContent through-table + cvc_list = ( + component_version + .componentversioncontent_set + .filter(content__has_file=True) + .order_by('key') + ) + + return [ + StaticFile( + name=cvc.key, + url=self._absolute_url_for_asset(component_version, cvc.key), + data=None, + ) + for cvc in cvc_list + ] + def save_block(self, block): """ Save any pending field data values to Learning Core data models. @@ -300,19 +330,120 @@ def _get_component_from_usage_key(self, usage_key): return component - def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: # pylint: disable=unused-argument + def _get_component_version_from_block(self, block): + """ + Given an XBlock instance, return the Learning Core ComponentVersion. + + This relies on our runtime setting the _runtime_requested_version + attribute on blocks that it fetches. + """ + usage_key = block.usage_key + component = self._get_component_from_usage_key(usage_key) + + block_version = block._runtime_requested_version # pylint: disable=protected-access + if block_version == LatestVersion.DRAFT: + component_version = component.versioning.draft + elif block_version == LatestVersion.PUBLISHED: + component_version = component.versioning.published + else: + component_version = component.versioning.version_num(block_version) + + return component_version + + def _absolute_url_for_asset(self, component_version, asset_path): + """ + The full URL for a specific library asset in a ComponentVersion. + + This does not check for whether the path actually exists–it just returns + where it would be if it did exist. + """ + # This function should return absolute URLs, so we need the site root. + site_root_url = get_xblock_app_config().get_site_root_url() + + return site_root_url + reverse( + 'content_libraries:library-assets', + kwargs={ + 'component_version_uuid': component_version.uuid, + 'asset_path': f"static/{asset_path}", + } + ) + + def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: """ Return an absolute URL for the specified static asset file that may belong to this XBlock. - e.g. if the XBlock settings have a field value like "/static/foo.png" - then this method will be called with asset_path="foo.png" and should - return a URL like https://cdn.none/xblock/f843u89789/static/foo.png + e.g. if the XBlock settings have a field value like "/static/test.png" + then this method will be called with asset_path="test.png" and should + return a URL like: + + http://studio.local.openedx.io:8001/library_assets/cd31871e-a342-4c3f-ba2f-a661bf630996/static/test.png If the asset file is not recognized, return None This is called by the XBlockRuntime superclass in the .runtime module. - TODO: Implement as part of larger static asset effort. + Implementation Details + ---------------------- + The standard XBlock "static/{asset_path}" substitution strips out the + leading "static/" part because it assumes that all files will exist in a + shared, flat namespace (i.e. a course's Files and Uploads). + + Learning Core treats assets differently. Each Component has its own, + isolated namespace for asset storage. Furthermore, that namespace + contains content that are not meant to be downloadable, like the + block.xml (the OLX of the Component). There may one day be other files + that are not meant to be externally downloadable there as well, like + Markdown or LaTeX source files or grader code. + + By convention, the static assets that we store in Learning Core and are + meant for download sit inside a static/ directory that is local to each + Component (and actually separate for each Component Version). + + So the transformation looks like this: + + 1. The Learning Core ComponentVersion has an asset stored as + ``static/test.png`` in the database. + 2. The original OLX content we store references ``/static/test.png``, + per OLX convention. Note the leading "/". + 3. The ReplaceURLService XBlock runtime service invokes + ``static_replace`` and strips out ``/static/``. + 4. The method we're in now is invoked with a ``static_path`` of just + ``test.png``, because that's the transformation that works for + ModuleStore-based courses, where everything is stored in the root of + a shared Files and Uploads space. + 5. This method then builds a URL that re-adds the "static/" prefix, and + then points to the ComponentVersion-specific location for that asset. + + Note: This means that the URL for a static asset will change with every + new version of the Component that is created, i.e. with every edit + that's saved–even if the asset itself doesn't change. This was the + tradeoff we made in order to put each version's assets in an isolated + space, and to make sure that we don't break things like relative links. + On the backend, we only store the asset once. + + Performance Note + ---------------- + This can theoretically get very expensive if there are many, many static + asset references in a Component. It's also very cacheable–we could put + it in a RequestCache with a key of (usage_key, version_num), and have + the value be the component_version.uuid and the full list of assets. I'm + not doing this yet in order to keep the code simpler, but I'm leaving + this note here in case someone needs to optimize this later. """ - return None + component_version = self._get_component_version_from_block(block) + + try: + content = ( + component_version + .componentversioncontent_set + .filter(content__has_file=True) + .get(key=f"static/{asset_path}") + ) + except ObjectDoesNotExist: + # This means we see a path that _looks_ like it should be a static + # asset for this Component, but that static asset doesn't really + # exist. + return None + + return self._absolute_url_for_asset(component_version, asset_path) diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index f12bf5336af5..4f94b1acb11b 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -37,19 +37,32 @@ def __init__(self, block): # Search the OLX for references to files stored in the course's # "Files & Uploads" (contentstore): self.olx_str = utils.rewrite_absolute_static_urls(self.olx_str, course_key) - for asset in utils.collect_assets_from_text(self.olx_str, course_key): - path = asset['path'] - if path not in [sf.name for sf in self.static_files]: - self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) - - if block.scope_ids.usage_id.block_type in ['problem', 'vertical']: - py_lib_zip_file = utils.get_python_lib_zip_if_using(self.olx_str, course_key) - if py_lib_zip_file: - self.static_files.append(py_lib_zip_file) - - js_input_files = utils.get_js_input_files_if_using(self.olx_str, course_key) - for js_input_file in js_input_files: - self.static_files.append(js_input_file) + + runtime_supports_explicit_assets = hasattr(block.runtime, 'get_block_assets') + if runtime_supports_explicit_assets: + # If a block supports explicitly tracked assets, things are simple. + # Learning Core backed content supports this, which currently means + # v2 Content Libraries. + self.static_files.extend( + block.runtime.get_block_assets(block) + ) + else: + # Otherwise, we have to scan the content to extract associated asset + # by inference. This is what we have to do for Modulestore-backed + # courses, which store files a course-global "Files and Uploads". + for asset in utils.collect_assets_from_text(self.olx_str, course_key): + path = asset['path'] + if path not in [sf.name for sf in self.static_files]: + self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) + + if block.scope_ids.usage_id.block_type in ['problem', 'vertical']: + py_lib_zip_file = utils.get_python_lib_zip_if_using(self.olx_str, course_key) + if py_lib_zip_file: + self.static_files.append(py_lib_zip_file) + + js_input_files = utils.get_js_input_files_if_using(self.olx_str, course_key) + for js_input_file in js_input_files: + self.static_files.append(js_input_file) def _serialize_block(self, block) -> etree.Element: """ Serialize an XBlock to OLX/XML. """ From 23c4276ec62fab02795245000046cd8be7d3b133 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 17 Oct 2024 12:08:01 -0400 Subject: [PATCH 6/6] test: add data.py to acceptable isolated app imports Per OEP-49, both api.py and data.py are allowed to be imported into other apps: https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0049-django-app-patterns.html#api-py --- setup.cfg | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index f36e0f8b0203..987f4474531f 100644 --- a/setup.cfg +++ b/setup.cfg @@ -183,9 +183,10 @@ isolated_apps = openedx.core.djangoapps.xblock openedx.core.lib.xblock_serializer allowed_modules = - # Only imports from api.py are allowed elsewhere in the code + # Only imports from api.py and data.py are allowed elsewhere in the code # See https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0049-django-app-patterns.html#api-py api + data [importlinter:contract:3] name = Do not import apps from openedx-learning (only import from openedx_learning.api.* and openedx_learning.lib.*).