From 5179a8fe79565b327e7dc0213321b3fecf6797c2 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Fri, 26 Jul 2024 17:39:38 -0400 Subject: [PATCH] feat: Upstream Sync with Content Library Blocks Implements: https://github.com/kdmccormick/edx-platform/blob/kdmccormick/upstream-proto/docs/decisions/0020-upstream-block.rst --- cms/djangoapps/contentstore/helpers.py | 43 +- .../rest_api/v1/serializers/vertical_block.py | 13 + .../v1/views/tests/test_vertical_block.py | 12 + .../rest_api/v1/views/vertical_block.py | 18 + .../contentstore/rest_api/v2/urls.py | 24 +- .../rest_api/v2/views/__init__.py | 3 - .../rest_api/v2/views/downstreams.py | 228 ++++++++++ .../v2/views/tests/test_downstreams.py | 228 ++++++++++ .../xblock_storage_handlers/view_handlers.py | 3 +- cms/envs/common.py | 1 + cms/lib/xblock/test/test_upstream_sync.py | 285 +++++++++++++ cms/lib/xblock/upstream_sync.py | 395 ++++++++++++++++++ docs/decisions/0020-upstream-downstream.rst | 5 + mypy.ini | 4 +- .../core/djangoapps/content_libraries/api.py | 5 + .../core/djangoapps/content_staging/api.py | 1 + .../core/djangoapps/content_staging/data.py | 1 + .../0005_stagedcontent_version_num.py | 18 + .../core/djangoapps/content_staging/models.py | 2 + 19 files changed, 1273 insertions(+), 16 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..da355c6cb4c4 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: @@ -375,10 +384,30 @@ def _import_xml_node_to_parent( 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; just don't link. + temp_xblock.upstream = None + 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 form 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..85e14de15f81 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,7 @@ 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": upstream_link.to_json() if upstream_link 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..a3a2c404329e --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -0,0 +1,228 @@ +""" +API Views for managing & syncing links between upstream & downstream content + +Only [BETA] endpoints are implemented currently. +The [FULL] endpoints should be implemented for the Full Libraries Relaunch, or removed from this doc. + +[FULL] List downstream blocks that can be synced, filterable by course or sync-readiness. + GET /api/v2/contentstore/downstreams + GET /api/v2/contentstore/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true + 200: A paginated list of applicable & accessible downstream blocks. + +[BETA] Inspect a single downstream block's link to upstream content. + GET /api/v2/contentstore/downstreams/{usage_key_string} + 200: Upstream link details successfully fetched. + 404: Block not found, OR user lacks permission to read block. + 400: Block it not linked to upstream content. + +[FULL] Sever a single downstream block's link to upstream content. + DELETE /api/v2/contentstore/downstreams/{usage_key_string} + 204: Block successfully unlinked. No response body. + 404: Block not found, OR user lacks permission to edit block + 400: Block it not linked to upstream content. + +[BETA] Establish or modify a single downstream block's link to upstream content. +(This makes sense REST-wise, but I'm not sure we'll actually need it.) + PUT /api/v2/contentstore/downstreams/{usage_key_string} + 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: Block's upstream link successfully edited (and synced, if requested). + 404: Block not found, OR user lacks permission to edit block + 400: upstream_ref is malformed, missing, or inaccessible. + 400: Upstream block does not support syncing. + +[BETA] Sync a downstream block with upstream content. + POST /api/v2/contentstore/downstreams/{usage_key_string}/sync + 200: Block successfully synced with upstream content. + 404: Block is not linked to upstream, OR block not found, OR user lacks permission to edit block. + 400: Block it not linked to upstream content. + 400: Upstream is malformed, missing, or inaccessible. + 400: Upstream block does not support syncing. + +[BETA] Decline an available sync for a downstream block. + DELETE /api/v2/contentstore/downstreams/{usage_key_string}/sync + 204: Sync successfuly dismissed. No response body. + 404: Block not found, OR user lacks permission to edit block. + 400: Block it not linked to upstream content. + +Schema for 200 responses, except where noted: + { + "upstream_ref": string? + "version_synced": string?, + "version_available": string?, + "version_declined": string?, + "error_message": string?, + "ready_to_sync": Boolean + } + +Schema for 4XX responses: + { + "developer_message": string? + } +""" +from django.contrib.auth.models import AbstractUser +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, sync_from_upstream, decline_sync, BadUpstream, BadDownstream, fetch_customizable_fields +) +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 + + +# 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: Request) -> 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: Request, usage_key_string: str) -> Response: + """ + Inspect an XBlock's link to upstream content. + """ + assert isinstance(request.user, AbstractUser) + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=False) + _ensure_upstream_ref(downstream) + if link := UpstreamLink.try_get_for_block(downstream): + return Response(link.to_json()) + raise ValidationError(detail=f"Block '{usage_key_string}' is not linked to an upstream") + + def put(self, request: Request, usage_key_string: str) -> Response: + """ + Edit an XBlock's link to upstream content. + """ + assert isinstance(request.user, AbstractUser) + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + new_upstream_ref = request.data.get("upstream_ref") + if not isinstance(new_upstream_ref, str): + raise ValidationError({"upstream_ref": "value missing"}) + + # 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: + fetch_customizable_fields(downstream=downstream, user=request.user) + except BadDownstream as exc: + raise ValidationError(str(exc)) from exc + except BadUpstream as exc: + raise ValidationError({"upstream_ref": str(exc)}) from exc + modulestore().update_item(downstream, request.user.id) + link = UpstreamLink.get_for_block(downstream) + assert link + return Response(link.to_json()) + + # def delete(self, request: Request, usage_key_string: str) -> Response: + # """ + # Sever an XBlock's link to upstream content. + # """ + # assert isinstance(request.user, AbstractUser) + # downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + # _ensure_upstream_ref(downstream) + # .... + + +@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: Request, usage_key_string: str) -> Response: + """ + Pull latest updates to the block at {usage_key_string} from its linked upstream content. + """ + assert isinstance(request.user, AbstractUser) + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + _ensure_upstream_ref(downstream) + if not downstream.upstream: + raise NotFound(detail=f"Block '{usage_key_string}' is not linked to a library block") + old_version = downstream.upstream_version + try: + sync_from_upstream(downstream, request.user) + except (BadUpstream, BadDownstream) as exc: + raise ValidationError(detail=str(exc)) from exc + modulestore().update_item(downstream, request.user.id) + upstream_link = UpstreamLink.get_for_block(downstream) + assert upstream_link + return Response(upstream_link.to_json(), status=200) + + def delete(self, request: Request, usage_key_string: str) -> Response: + """ + Decline the latest updates to the block at {usage_key_string}. + """ + assert isinstance(request.user, AbstractUser) + downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + _ensure_upstream_ref(downstream) + try: + decline_sync(downstream) + except (BadUpstream, BadDownstream) as exc: + raise ValidationError(str(exc)) from exc + modulestore().update_item(downstream, request.user.id) + return Response(status=204) + + +def _load_accessible_block(user: AbstractUser, 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. + Raises NotFound if user lacks access. + Raises NotFound if block does not 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 + + +def _ensure_upstream_ref(block: XBlock) -> None: + """ + Raises ValidationError if block is not a downstream block. + """ + if not block.upstream: + raise ValidationError(detail=f"Block '{block.usage_key}' is not linked to a library 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..b73d01e914f5 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -0,0 +1,228 @@ +""" +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_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 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 PostDownstreamSyncViewTest(_DownstreamViewTestMixin, 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): + """ + @@TODO + """ + 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 + + 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 DeleteDownstreamSyncViewtest(_DownstreamViewTestMixin, 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_200(self, mock_decline_sync): + """ + @@TODO + """ + 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 + + 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] 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 7521cc21fa11..fbd296020fb6 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1008,6 +1008,7 @@ XModuleMixin, EditInfoMixin, AuthoringMixin, + 'cms.lib.xblock.upstream_sync.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..edc2b58b357f --- /dev/null +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -0,0 +1,285 @@ +""" +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 ( + sync_from_upstream, BadUpstream, UpstreamLink, decline_sync, fetch_customizable_fields +) +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 UpstreamSyncMixin + """ + + def setUp(self): + """ + Create a simple course with one unit. + """ + 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") + library = libs.create_library( + org=Organization.objects.get(short_name="TestX"), + slug="TestLib", + title="Test Upstream Library", + ) + self.upstream_key = libs.create_library_block(library.key, "html", "test-upstream").usage_key + libs.create_library_block(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_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" + + 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 diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py new file mode 100644 index 000000000000..3cdc5edcf5d3 --- /dev/null +++ b/cms/lib/xblock/upstream_sync.py @@ -0,0 +1,395 @@ +""" +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. +""" +import typing as t +from dataclasses import dataclass, asdict + +from django.contrib.auth.models import AbstractUser +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 UsageKey, 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 + +import openedx.core.djangoapps.xblock.api as xblock_api +from openedx.core.djangoapps.content_libraries.api import get_library_block + + +class BadUpstream(Exception): + """ + Reference to upstream content is malformed, invalid, and/or inaccessible. + + Should be constructed with a human-friendly, localized, PII-free message, suitable for API responses and UI display. + """ + + +class BadDownstream(Exception): + """ + Downstream content does not support sync. + + Should be constructed with a human-friendly, localized, PII-free message, suitable for API responses and UI display. + """ + + +@dataclass(frozen=True) +class UpstreamLink: + """ + Metadata about some downstream content's relationship with its linked upstream content. + """ + upstream_ref: str # Reference to the upstream content, e.g., a serialized library block usage key. + version_synced: int # 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 | None: + """ + Same as `get_for_block`, but upon failure, sets `.error_message` instead of raising an exception. + """ + try: + return cls.get_for_block(downstream) + except (BadDownstream, BadUpstream) as exc: + 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 | None: + """ + 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 is valid, returns an UpstreamLink object. + If the syncing to `downstream` is not supported, raises BadDownstream. + If the upstream link is invalid or broken, raises BadUpstream. + If `downstream` is not linked at all, returns None. + """ + if not downstream.upstream: + return None + 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}'" + ) + 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: AbstractUser) -> 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 the syncing to `downstream` is not supported, raises BadDownstream. + If the upstream link is invalid or broken, raises BadUpstream. + If `downstream` is not linked at all, does nothing. + """ + if not (link_and_upstream := _load_upstream_link_and_block(downstream, user)): + return + link, upstream = link_and_upstream + _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: AbstractUser, 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 BadUpstream/BadDownstream. + """ + if not upstream: + if not (link_and_upstream := _load_upstream_link_and_block(downstream, user)): + return # `downstream.upstream is None` -> nothing to do + _link, upstream = link_and_upstream + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) + + +def _load_upstream_link_and_block(downstream: XBlock, user: AbstractUser) -> tuple[UpstreamLink, XBlock] | None: + """ + Load the upstream metadata and content for a downstream block (or None if it is not linked). + + Can raise BadUpstream and BadDownstream. + + 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 not (link := UpstreamLink.get_for_block(downstream)): # can raise BadUpstream, BadDownstream + return None + try: + lib_block: XBlock = xblock_api.load_block(UsageKey.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 `downstream.blah`: + + 1. Fetch `upstream.blah` and save it in `downstream.upstream_blah`. + 2. If not only_fetch, and if `downstream.blah` isn't customized, update it from `upstream.blah`. + """ + 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 -- update 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 the syncing to `downstream` is not supported, raises BadDownstream. + If the upstream link is invalid or broken, raises BadUpstream. + If `downstream` is not linked at all, does nothing. + """ + # Try to load the upstream. + upstream_link = UpstreamLink.get_for_block(downstream) # Can raise BadUpstream or BadUpstream + if not upstream_link: + return # No upstream -> nothing to sync. + downstream.upstream_version_declined = upstream_link.version_available + + +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/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 b9f3779af539..7dc8f2e798ff 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -212,6 +212,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) @@ -232,6 +234,7 @@ def from_component(cls, library_key, component): 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 @@ -243,6 +246,8 @@ def from_component(cls, library_key, component): display_name=component.versioning.draft.title, created=component.created, modified=component.versioning.draft.created, + draft_version_num=component.versioning.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..206740efe19a 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -205,6 +205,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"))