From df5daa20dd72c2dc4653a396c291dd22bbf7bb1b Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Sat, 16 Aug 2025 12:56:29 -0500 Subject: [PATCH] feat: Add the top-level parent logic to the ready_to_sync value in the UpstreamLink refactor: decline_sync to decline children recursively test: Add tests fro new ready_to_sync and new decline chore: Delete Deprecated code of downstreams style: Fix broken lint style: fix broken tests refactor: restore ready_to_sync as property style: Fix broken lint refactor: add _is_ready_to_sync_individually refactor: remove store as parameter in decline_sync() --- .../contentstore/rest_api/v2/urls.py | 15 +- .../rest_api/v2/views/downstreams.py | 90 +-------- .../tests/test_downstream_sync_integration.py | 175 +++++++++++++++++- .../v2/views/tests/test_downstreams.py | 4 +- cms/lib/xblock/upstream_sync.py | 98 ++++++++-- 5 files changed, 263 insertions(+), 119 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/urls.py b/cms/djangoapps/contentstore/rest_api/v2/urls.py index ce2a78c0e2a5..5fa954a4ef82 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v2/urls.py @@ -13,24 +13,11 @@ home.HomePageCoursesViewV2.as_view(), name="courses", ), - # TODO: Rename this to `downstreams/` after full deprecate `DownstreamComponentsListView` - re_path( - r'^downstreams-all/$', - downstreams.DownstreamListView.as_view(), - name="downstreams_list_all", - ), - # [DEPRECATED], use `downstreams-all/` instead. re_path( r'^downstreams/$', - downstreams.DownstreamComponentsListView.as_view(), + downstreams.DownstreamListView.as_view(), name="downstreams_list", ), - # [DEPRECATED], use `downstreams-all/` instead. - re_path( - r'^downstream-containers/$', - downstreams.DownstreamContainerListView.as_view(), - name="container_downstreams_list", - ), re_path( fr'^downstreams/{settings.USAGE_KEY_PATTERN}$', downstreams.DownstreamView.as_view(), diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index b08a222ff351..4e168bcaf8af 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -81,7 +81,6 @@ """ import logging -import warnings from attrs import asdict as attrs_asdict from django.db.models import QuerySet @@ -101,8 +100,6 @@ from cms.djangoapps.contentstore.models import ComponentLink, ContainerLink, EntityLinkBase from cms.djangoapps.contentstore.rest_api.v2.serializers import ( PublishableEntityLinkSerializer, - ComponentLinksSerializer, - ContainerLinksSerializer, PublishableEntityLinksSummarySerializer, ) from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import sync_library_content @@ -276,47 +273,6 @@ def _remove_duplicates(self, links: list[EntityLinkBase]) -> list[EntityLinkBase return unique_links -@view_auth_classes() -class DownstreamComponentsListView(DeveloperErrorViewMixin, APIView): - """ - [DEPRECATED], use DownstreamListView instead. - - List all components which are linked to an upstream context, with optional filtering. - """ - - def get(self, request: _AuthenticatedRequest): - """ - [DEPRECATED], use DownstreamListView.get instead, with `item_type='components'` - - Fetches publishable entity links for given course key - """ - warnings.warn( - '`downstreams/` API is deprecated. Please use `downstreams-all/?item_type=components` instead.', - DeprecationWarning, stacklevel=3, - ) - course_key_string = request.GET.get('course_id') - ready_to_sync = request.GET.get('ready_to_sync') - upstream_usage_key = request.GET.get('upstream_usage_key') - link_filter: dict[str, CourseKey | UsageKey | bool] = {} - paginator = DownstreamListPaginator() - if course_key_string: - try: - link_filter["downstream_context_key"] = CourseKey.from_string(course_key_string) - except InvalidKeyError as exc: - raise ValidationError(detail=f"Malformed course key: {course_key_string}") from exc - if ready_to_sync is not None: - link_filter["ready_to_sync"] = BooleanField().to_internal_value(ready_to_sync) - if upstream_usage_key: - try: - link_filter["upstream_usage_key"] = UsageKey.from_string(upstream_usage_key) - except InvalidKeyError as exc: - raise ValidationError(detail=f"Malformed usage key: {upstream_usage_key}") from exc - links = ComponentLink.filter_links(**link_filter) - paginated_links = paginator.paginate_queryset(links, self.request, view=self) - serializer = ComponentLinksSerializer(paginated_links, many=True) - return paginator.get_paginated_response(serializer.data, self.request) - - @view_auth_classes() class DownstreamSummaryView(DeveloperErrorViewMixin, APIView): """ @@ -551,8 +507,10 @@ def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respo Decline the latest updates to the block at {usage_key_string}. """ downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + if downstream.upstream is None: + raise ValidationError(str(NoUpstream())) try: - decline_sync(downstream) + decline_sync(downstream, request.user.id) 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 @@ -564,51 +522,9 @@ def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respo downstream.upstream, ) raise ValidationError(str(exc)) from exc - modulestore().update_item(downstream, request.user.id) return Response(status=204) -@view_auth_classes() -class DownstreamContainerListView(DeveloperErrorViewMixin, APIView): - """ - [DEPRECATED], use DownstreamListView instead. - - List all container blocks which are linked to an upstream context, with optional filtering. - """ - - def get(self, request: _AuthenticatedRequest): - """ - [DEPRECATED], use DownstreamListView.get instead, with `item_type='containers'` - - Fetches publishable container entity links for given course key - """ - warnings.warn( - '`downstreams/` API is deprecated. Please use `downstreams-all/?item_type=components` instead.', - DeprecationWarning, stacklevel=3, - ) - course_key_string = request.GET.get('course_id') - ready_to_sync = request.GET.get('ready_to_sync') - upstream_container_key = request.GET.get('upstream_container_key') - link_filter: dict[str, CourseKey | LibraryContainerLocator | bool] = {} - paginator = DownstreamListPaginator() - if course_key_string: - try: - link_filter["downstream_context_key"] = CourseKey.from_string(course_key_string) - except InvalidKeyError as exc: - raise ValidationError(detail=f"Malformed course key: {course_key_string}") from exc - if ready_to_sync is not None: - link_filter["ready_to_sync"] = BooleanField().to_internal_value(ready_to_sync) - if upstream_container_key: - try: - link_filter["upstream_container_key"] = LibraryContainerLocator.from_string(upstream_container_key) - except InvalidKeyError as exc: - raise ValidationError(detail=f"Malformed usage key: {upstream_container_key}") from exc - links = ContainerLink.filter_links(**link_filter) - paginated_links = paginator.paginate_queryset(links, self.request, view=self) - serializer = ContainerLinksSerializer(paginated_links, many=True) - return paginator.get_paginated_response(serializer.data, self.request) - - def _load_accessible_block(user: User, usage_key_string: str, *, require_write_access: bool) -> XBlock: """ Given a logged in-user and a serialized usage key of an upstream-linked XBlock, load it from the ModuleStore, diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py index 1d7e6b1277de..c03f3f4e336f 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py @@ -71,6 +71,9 @@ def _get_sync_status(self, usage_key: str): def _sync_downstream(self, usage_key: str): return self._api('post', f"/api/contentstore/v2/downstreams/{usage_key}/sync", {}, expect_response=200) + def _decline_sync_downstream(self, usage_key: str): + return self._api('delete', f"/api/contentstore/v2/downstreams/{usage_key}/sync", {}, expect_response=204) + def _get_course_block_olx(self, usage_key: str): data = self._api('get', f'/api/olx-export/v1/xblock/{usage_key}/', {}, expect_response=200) return data["blocks"][data["root_block_id"]]["olx"] @@ -130,7 +133,7 @@ def _get_downstream_links( data["item_type"] = str(item_type) if use_top_level_parents is not None: data["use_top_level_parents"] = str(use_top_level_parents) - return self.client.get("/api/contentstore/v2/downstreams-all/", data=data) + return self.client.get("/api/contentstore/v2/downstreams/", data=data) def assertXmlEqual(self, xml_str_a: str, xml_str_b: str) -> bool: """ Assert that the given XML strings are equal, ignoring attribute order and some whitespace variations. """ @@ -421,9 +424,7 @@ def test_unit_sync(self): 'version_available': 2, # <--- not updated since we didn't directly modify the unit 'version_synced': 2, 'version_declined': None, - # FIXME: ready_to_sync should be true, since a child block needs syncing. - # This may need to be fixed post-Teak, as syncing the children directly is still possible. - 'ready_to_sync': False, + 'ready_to_sync': True, # <--- It's the top-level parent of the block 'error_message': None, }) @@ -434,7 +435,7 @@ def test_unit_sync(self): 'version_available': 3, # <--- updated since we modified the problem 'version_synced': 2, 'version_declined': None, - 'ready_to_sync': True, # <--- updated + 'ready_to_sync': False, # <-- It has top-level parent, the parent is the one who must synchronize 'error_message': None, }) @@ -829,3 +830,167 @@ def test_unit_sync(self): data = downstreams.json() self.assertEqual(data["count"], 4) self.assertListEqual(data["results"], expected_downstreams) + + def test_unit_decline_sync(self): + """ + Test that we can decline sync a unit from the library into the course + """ + # 1️⃣ Create a "vertical" block in the course based on a "unit" container: + downstream_unit = self._create_block_from_upstream( + # The API consumer needs to specify "vertical" here, even though upstream is "unit". + # In the future we could create a nicer REST API endpoint for this that's not part of + # the messy '/xblock/' API and which auto-detects the types based on the upstream_key. + block_category="vertical", + parent_usage_key=str(self.course_subsection.usage_key), + upstream_key=self.upstream_unit["id"], + ) + downstream_unit_block_key = get_block_key_dict( + UsageKey.from_string(downstream_unit["locator"]), + ) + children_downstream_keys = self._get_course_block_children(downstream_unit["locator"]) + downstream_problem1 = children_downstream_keys[1] + assert "type@problem" in downstream_problem1 + status = self._get_sync_status(downstream_unit["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_unit["id"], # e.g. 'lct:CL-TEST:testlib:unit:u1' + 'version_available': 2, + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': False, + 'error_message': None, + # 'upstream_link': 'http://course-authoring-mfe/library/lib:CL-TEST:testlib/units/...' + }) + assert status["upstream_link"].startswith("http://course-authoring-mfe/library/") + assert status["upstream_link"].endswith(f"/units/{self.upstream_unit['id']}") + + # Check that the downstream container matches our expectations. + # Note that: + # (1) Every XBlock has an "upstream" field + # (2) some "downstream only" fields like weight and max_attempts are omitted. + # (3) The "top_level_downstream_parent" is the container created + self.assertXmlEqual(self._get_course_block_olx(downstream_unit["locator"]), f""" + + This is the HTML. + multiple choice... + multi select... + + """) + + # 2️⃣ Now, lets modify the upstream problem 1: + self._set_library_block_olx( + self.upstream_problem1["id"], + 'multiple choice v2...' + ) + self._publish_container(self.upstream_unit["id"]) + + status = self._get_sync_status(downstream_unit["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_unit["id"], # e.g. 'lct:CL-TEST:testlib:unit:u1' + 'version_available': 2, # <--- not updated since we didn't directly modify the unit + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': True, # <--- It's the top-level parent of the block + 'error_message': None, + }) + + # Check the upstream/downstream status of [one of] the children + self.assertDictContainsEntries(self._get_sync_status(downstream_problem1), { + 'upstream_ref': self.upstream_problem1["id"], + 'version_available': 3, # <--- updated since we modified the problem + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': False, # <-- It has top-level parent, the parent is the one who must synchronize + 'error_message': None, + }) + + # Now, decline the sync + self._decline_sync_downstream(downstream_unit["locator"]) + + status = self._get_sync_status(downstream_unit["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_unit["id"], + 'version_available': 2, + 'version_synced': 2, + 'version_declined': 2, + 'ready_to_sync': False, + 'error_message': None, + }) + + # Check the upstream/downstream status of [one of] the children + self.assertDictContainsEntries(self._get_sync_status(downstream_problem1), { + 'upstream_ref': self.upstream_problem1["id"], + 'version_available': 3, + 'version_synced': 2, + 'version_declined': 3, + 'ready_to_sync': False, + 'error_message': None, + }) + + # Check that the downstream container has not had any changes + self.assertXmlEqual(self._get_course_block_olx(downstream_unit["locator"]), f""" + + This is the HTML. + multiple choice... + multi select... + + """) 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 cad103c81185..00db886240ef 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 @@ -43,10 +43,12 @@ def _get_upstream_link_good_and_syncable(downstream): return UpstreamLink( upstream_ref=downstream.upstream, upstream_key=UsageKey.from_string(downstream.upstream), + downstream_key=str(downstream.usage_key), version_synced=downstream.upstream_version, version_available=(downstream.upstream_version or 0) + 1, version_declined=downstream.upstream_version_declined, error_message=None, + has_top_level_parent=False, ) @@ -582,7 +584,7 @@ def call_api( data["item_type"] = str(item_type) if use_top_level_parents is not None: data["use_top_level_parents"] = str(use_top_level_parents) - return self.client.get("/api/contentstore/v2/downstreams-all/", data=data) + return self.client.get("/api/contentstore/v2/downstreams/", data=data) def test_200_all_downstreams_for_a_course(self): """ diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index bc1148b2cebe..076afef505a7 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -24,6 +24,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2 +from opaque_keys.edx.keys import UsageKey from xblock.exceptions import XBlockNotFoundError from xblock.fields import Scope, String, Integer, Dict from xblock.core import XBlockMixin, XBlock @@ -77,16 +78,15 @@ class UpstreamLink: """ upstream_ref: str | None # Reference to the upstream content, e.g., a serialized library block usage key. upstream_key: LibraryUsageLocatorV2 | LibraryContainerLocator | None # parsed opaque key version of upstream_ref + downstream_key: str | None # Key of the downstream object. 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. + has_top_level_parent: bool # True if this Upstream link has a top-level parent @property - def ready_to_sync(self) -> bool: - """ - Should we invite the downstream's authors to sync the latest upstream updates? - """ + def _is_ready_to_sync_individually(self) -> bool: return bool( self.upstream_ref and self.version_available and @@ -94,6 +94,60 @@ def ready_to_sync(self) -> bool: self.version_available > (self.version_declined or 0) ) + @property + def ready_to_sync(self) -> bool: + """ + Calculates the ready to sync value using the version available. + If is a container, also verifies if the children needs sync. + """ + from xmodule.modulestore.django import modulestore + + # If this component/container has top-level parent, so we need to sync the parent + if self.has_top_level_parent: + return False + + if isinstance(self.upstream_key, LibraryUsageLocatorV2): + return self._is_ready_to_sync_individually + elif isinstance(self.upstream_key, LibraryContainerLocator): + # The container itself has changes to update, it is not necessary to review its children + if self._is_ready_to_sync_individually: + return True + + def check_children_ready_to_sync(xblock_downstream): + """ + Checks if one of the children of `xblock_downstream` is ready to sync + """ + if not xblock_downstream.has_children: + return False + + downstream_children = xblock_downstream.get_children() + + for child in downstream_children: + if child.upstream: + child_upstream_link = UpstreamLink.get_for_block(child) + + child_ready_to_sync = bool( + child_upstream_link.upstream_ref and + child_upstream_link.version_available and + child_upstream_link.version_available > (child_upstream_link.version_synced or 0) and + child_upstream_link.version_available > (child_upstream_link.version_declined or 0) + ) + + # If one child needs sync, it is not needed to check more children + if child_ready_to_sync: + return True + + if check_children_ready_to_sync(child): + # If one child needs sync, it is not needed to check more children + return True + + return False + if self.downstream_key is not None: + return check_children_ready_to_sync( + modulestore().get_item(UsageKey.from_string(self.downstream_key)) + ) + return False + @property def upstream_link(self) -> str | None: """ @@ -138,10 +192,12 @@ def try_get_for_block(cls, downstream: XBlock, log_error: bool = True) -> t.Self return cls( upstream_ref=getattr(downstream, "upstream", ""), upstream_key=None, + downstream_key=str(getattr(downstream, "usage_key", "")), version_synced=getattr(downstream, "upstream_version", None), version_available=None, version_declined=None, error_message=str(exc), + has_top_level_parent=False, ) @classmethod @@ -189,15 +245,16 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: except XBlockNotFoundError as exc: raise BadUpstream(_("Linked upstream library block was not found in the system")) from exc version_available = block_meta.published_version_num - else: + elif isinstance(upstream_key, LibraryContainerLocator): # The upstream is a Container: - assert isinstance(upstream_key, LibraryContainerLocator) try: container_meta = lib_api.get_container(upstream_key) except lib_api.ContentLibraryContainerNotFound as exc: raise BadUpstream(_("Linked upstream library container was not found in the system")) from exc expected_downstream_block_type = container_meta.container_type.olx_tag version_available = container_meta.published_version_num + else: + raise BadUpstream(_("Linked `upstream_key` is not a valid key")) if downstream_type != expected_downstream_block_type: # Note: generally the upstream and downstream types must match, except that upstream containers @@ -214,27 +271,44 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: ) ) - return cls( + result = cls( upstream_ref=downstream.upstream, upstream_key=upstream_key, + downstream_key=str(downstream.usage_key), version_synced=downstream.upstream_version, version_available=version_available, version_declined=downstream.upstream_version_declined, error_message=None, + has_top_level_parent=downstream.top_level_downstream_parent_key is not None, ) + return result -def decline_sync(downstream: XBlock) -> None: + +def decline_sync(downstream: XBlock, user_id=None) -> 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. + The function is called recursively to perform the same operation on the children of the `downstream`. If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. """ - upstream_link = UpstreamLink.get_for_block(downstream) # Can raise UpstreamLinkException - downstream.upstream_version_declined = upstream_link.version_available + if downstream.upstream: + from xmodule.modulestore.django import modulestore + + store = modulestore() + upstream_link = UpstreamLink.get_for_block(downstream) # Can raise UpstreamLinkException + upstream_key = upstream_link.upstream_key + + downstream.upstream_version_declined = upstream_link.version_available + + if isinstance(upstream_key, LibraryContainerLocator) and downstream.has_children: + with store.bulk_operations(downstream.usage_key.context_key): + children = downstream.get_children() + for child in children: + decline_sync(child, user_id) + + store.update_item(downstream, user_id) def sever_upstream_link(downstream: XBlock) -> None: