diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 2615f259becc..e9f599772d3e 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -382,16 +382,16 @@ 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; just don't link. + # 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. 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 85e14de15f81..0798c341cc1c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py @@ -294,7 +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": upstream_link.to_json() if upstream_link else None, + "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/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 5dc3990c4496..cf76c8b206d4 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -1,66 +1,63 @@ """ 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: Blocks is 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: Blocks is not linked to upstream content. - -[BETA] Establish or modify a single downstream block's link to upstream content. - PUT /api/v2/contentstore/downstreams/{usage_key_string} +API paths (We will move these into proper api_doc_tools annotations soon +https://github.com/openedx/edx-platform/issues/35653): + + /api/v2/contentstore/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: Block's upstream link successfully edited (and synced, if requested). - 404: Block not found, OR user lacks permission to edit block + 200: Downstream block's upstream link successfully edited (and synced, if requested). Returns UpstreamLink. 400: upstream_ref is malformed, missing, or inaccessible. - 400: Upstream block does not support syncing. + 400: Content at upstream_ref does not support syncing. + 404: Downstream block not found or user lacks permission to edit it. + + /api/v2/contentstore/downstreams/{usage_key_string}/sync -[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: Blocks is not linked to upstream content. + 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. -[BETA] Decline an available sync for a downstream block. - DELETE /api/v2/contentstore/downstreams/{usage_key_string}/sync + DELETE: Decline an available sync for a downstream block. 204: Sync successfuly dismissed. No response body. - 404: Block not found, OR user lacks permission to edit block. - 400: Blocks is not linked to upstream content. + 400: Downstream block is not linked to upstream content. + 404: Downstream block not found or user lacks permission to edit it. -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 - } + # NOT YET IMPLEMENTED -- Will be needed for full Libraries Relaunch in ~Teak. + /api/v2/contentstore/downstreams + /api/v2/contentstore/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. -Schema for 4XX responses: +UpstreamLink response schema: { - "developer_message": string? + "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 @@ -71,7 +68,8 @@ from xblock.core import XBlock from cms.lib.xblock.upstream_sync import ( - UpstreamLink, sync_from_upstream, decline_sync, BadUpstream, BadDownstream, fetch_customizable_fields + 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 ( @@ -82,6 +80,9 @@ 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. @@ -117,10 +118,7 @@ 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) - _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") + return Response(UpstreamLink.try_get_for_block(downstream).to_json()) def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ @@ -128,8 +126,6 @@ def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response """ 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, @@ -142,22 +138,47 @@ def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response 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) - link: UpstreamLink = UpstreamLink.get_for_block(downstream) # type: ignore[assignment] - return Response(link.to_json()) + # 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) - # _ensure_upstream_ref(downstream) - # .... + 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) @@ -171,27 +192,37 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons 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) - _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: + 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) - link: UpstreamLink = UpstreamLink.get_for_block(downstream) # type: ignore[assignment] - return Response(link.to_json(), status=200) + # 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) - _ensure_upstream_ref(downstream) try: decline_sync(downstream) - except (BadUpstream, BadDownstream) as exc: + 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) @@ -202,9 +233,7 @@ def _load_accessible_block(user: User, usage_key_string: str, *, require_write_a 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. + 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: @@ -220,11 +249,3 @@ def _load_accessible_block(user: User, usage_key_string: str, *, require_write_a 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 index 2890bd7c672e..616035473e7e 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -105,14 +105,16 @@ def test_200_bad_upstream(self): assert response.data['error_message'] == MOCK_UPSTREAM_ERROR assert response.data['ready_to_sync'] is False - def test_400_no_upstream(self): + def test_200_no_upstream(self): """ - If the upstream link is missing, do we get a 400? + 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 == 400 - assert "is not linked" in response.data["developer_message"][0] + 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): @@ -172,23 +174,49 @@ def test_400(self, sync: str): assert video_after.upstream is None -class PostDownstreamSyncViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): +class DeleteDownstreamViewTest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): """ - Test that `POST /api/v2/contentstore/downstreams/.../sync` initiates a sync from the linked upstream. + 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.post(f"/api/contentstore/v2/downstreams/{usage_key_string}/sync") + 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) - @patch.object(downstreams_views, "sync_from_upstream") - def test_200(self, mock_sync_from_upstream): + 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 == 200 - assert mock_sync_from_upstream.call_count == 1 + 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): """ @@ -200,7 +228,26 @@ def test_400_no_upstream(self): assert "is not linked" in response.data["developer_message"][0] -class DeleteDownstreamSyncViewtest(_DownstreamViewTestMixin, SharedModuleStoreTestCase): +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. """ @@ -209,7 +256,7 @@ def call_api(self, usage_key_string): @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): + def test_204(self, mock_decline_sync): """ Does the happy path work? """ @@ -217,12 +264,3 @@ def test_200(self, mock_decline_sync): 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/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index edc2b58b357f..5db020393eab 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -7,7 +7,9 @@ from organizations.models import Organization from cms.lib.xblock.upstream_sync import ( - sync_from_upstream, BadUpstream, UpstreamLink, decline_sync, fetch_customizable_fields + 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 @@ -19,12 +21,12 @@ @ddt.ddt class UpstreamTestCase(ModuleStoreTestCase): """ - Tests the UpstreamSyncMixin + Tests the upstream_sync mixin, data object, and Python APIs. """ def setUp(self): """ - Create a simple course with one unit. + Create a simple course with one unit, and simple V2 library with two blocks. """ super().setUp() course = CourseFactory.create() @@ -33,19 +35,37 @@ def setUp(self): self.unit = BlockFactory.create(category='vertical', parent=sequential) ensure_organization("TestX") - library = libs.create_library( + 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(library.key, "html", "test-upstream").usage_key - libs.create_library_block(library.key, "video", "video-upstream") + 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 @@ -54,7 +74,8 @@ def test_sync_no_upstream(self): block.display_name = "Block Title" block.data = "Block content" - sync_from_upstream(block, self.user) + with self.assertRaises(NoUpstream): + sync_from_upstream(block, self.user) assert block.display_name == "Block Title" assert block.data == "Block content" @@ -283,3 +304,34 @@ def test_prompt_and_decline_sync(self): 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 index d529879a6fb3..8dbe672a5d10 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -11,6 +11,7 @@ """ from __future__ import annotations +import logging import typing as t from dataclasses import dataclass, asdict @@ -28,20 +29,42 @@ from django.contrib.auth.models import AbstractUser -class BadUpstream(Exception): +logger = logging.getLogger(__name__) + + +class UpstreamLinkException(Exception): """ - Reference to upstream content is malformed, invalid, and/or inaccessible. + 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(Exception): +class BadDownstream(UpstreamLinkException): """ Downstream content does not support sync. + """ - Should be constructed with a human-friendly, localized, PII-free message, suitable for API responses and UI display. + +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) @@ -49,8 +72,8 @@ 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. + 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. @@ -77,13 +100,18 @@ def to_json(self) -> dict[str, t.Any]: } @classmethod - def try_get_for_block(cls, downstream: XBlock) -> t.Self | None: + 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 (BadDownstream, BadUpstream) as exc: + 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, @@ -93,20 +121,18 @@ def try_get_for_block(cls, downstream: XBlock) -> t.Self | None: ) @classmethod - def get_for_block(cls, downstream: XBlock) -> t.Self | None: + 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 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 link exists, is supported, and is followable, returns UpstreamLink. + Otherwise, raises an UpstreamLinkException. """ if not downstream.upstream: - return None + 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: @@ -156,13 +182,9 @@ def sync_from_upstream(downstream: XBlock, user: AbstractUser) -> None: 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 `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. """ - if not (link_and_upstream := _load_upstream_link_and_block(downstream, user)): - return - link, upstream = link_and_upstream + 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 @@ -173,26 +195,23 @@ def fetch_customizable_fields(*, downstream: XBlock, user: AbstractUser, upstrea 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. + Otherwise, load the block specified by `downstream.upstream`, which may raise an UpstreamLinkException. """ 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 + _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: AbstractUser) -> tuple[UpstreamLink, XBlock] | None: +def _load_upstream_link_and_block(downstream: XBlock, user: AbstractUser) -> tuple[UpstreamLink, XBlock]: """ - Load the upstream metadata and content for a downstream block (or None if it is not linked). - - Can raise BadUpstream and BadDownstream. + 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. """ - if not (link := UpstreamLink.get_for_block(downstream)): # can raise BadUpstream, BadDownstream - return None + 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: @@ -274,17 +293,38 @@ def decline_sync(downstream: XBlock) -> None: 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 `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. """ # Try to load the upstream. - upstream_link = UpstreamLink.get_for_block(downstream) # Can raise BadUpstream or BadUpstream + upstream_link = UpstreamLink.get_for_block(downstream) # Can raise UpstreamLinkException if not upstream_link: return # No upstream -> nothing to sync. 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. diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 7442efac28e0..0ee1099ca982 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -25,9 +25,7 @@ has_not_configured_message = messages.get('summary',{}).get('type', None) == 'not-configured' block_is_unit = is_unit(xblock) -upstream_info = None -if xblock.upstream: - upstream_info = UpstreamLink.try_get_for_block(xblock) +upstream_info = UpstreamLink.try_get_for_block(xblock) %> <%namespace name='static' file='static_content.html'/> @@ -108,7 +106,7 @@ ${label} % else: - % if upstream_info: + % if upstream_info.upstream_ref: % if upstream_info.error_message: