From e625f68d2a32b9a89a27d24bccc5fa60d5b11e17 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 1 Aug 2025 18:21:59 +0530 Subject: [PATCH 01/25] feat: allow editing html block imported from upstream The modified field is left untouched in future sync while storing the upstream values in hidden fields to allow authors to revert to upstream version at any point. --- .../tests/test_downstream_sync_integration.py | 107 +++++++++++++++ cms/djangoapps/contentstore/views/preview.py | 8 +- cms/envs/common.py | 2 +- cms/lib/xblock/upstream_sync.py | 128 +++++++++--------- cms/lib/xblock/upstream_sync_block.py | 19 +-- cms/templates/studio_xblock_wrapper.html | 6 + .../lib/xblock_serializer/block_serializer.py | 5 +- xmodule/html_block.py | 14 ++ 8 files changed, 215 insertions(+), 74 deletions(-) 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 b7890f821dcc..b088f4e2e39a 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 @@ -138,6 +138,7 @@ def test_problem_sync(self): 'version_declined': None, 'ready_to_sync': False, 'error_message': None, + 'is_modified': False, # 'upstream_link': 'http://course-authoring-mfe/library/lib:CL-TEST:testlib/components?usageKey=...' }) assert status["upstream_link"].startswith("http://course-authoring-mfe/library/") @@ -181,6 +182,7 @@ def test_problem_sync(self): upstream="{self.upstream_problem1['id']}" upstream_display_name="Problem 1 Display Name" upstream_version="2" + downstream_customized="["display_name"]" {self.standard_capa_attributes} >multiple choice... """) @@ -193,6 +195,7 @@ def test_problem_sync(self): 'version_declined': None, 'ready_to_sync': True, # <--- updated 'error_message': None, + 'is_modified': True, }) # 3️⃣ Now, sync and check the resulting OLX of the downstream @@ -212,6 +215,7 @@ def test_problem_sync(self): max_attempts="3" upstream="{self.upstream_problem1['id']}" upstream_display_name="Problem 1 NEW name" + downstream_customized="["display_name"]" upstream_version="3" {self.standard_capa_attributes} >multiple choice v2... @@ -238,6 +242,7 @@ def test_unit_sync(self): 'version_declined': None, 'ready_to_sync': False, 'error_message': None, + 'is_modified': False, # 'upstream_link': 'http://course-authoring-mfe/library/lib:CL-TEST:testlib/units/...' }) assert status["upstream_link"].startswith("http://course-authoring-mfe/library/") @@ -260,6 +265,7 @@ def test_unit_sync(self): editor="visual" upstream="{self.upstream_html1['id']}" upstream_version="2" + upstream_data="This is the HTML." >This is the HTML. This is the HTML. This is the HTML. This is the HTML. """) + + def test_html_sync(self): + """ + Test that we can sync a html from a library into a course. + """ + # 1️⃣ First, create the html in the course, using the upstream problem as a template: + downstream_html1 = self._create_block_from_upstream( + block_category="html", + parent_usage_key=str(self.course_subsection.usage_key), + upstream_key=self.upstream_html1["id"], + ) + status = self._get_sync_status(downstream_html1["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_html1["id"], # e.g. 'lb:CL-TEST:testlib:html:html1' + 'version_available': 2, + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': False, + 'error_message': None, + 'is_modified': False, + # 'upstream_link': 'http://course-authoring-mfe/library/lib:CL-TEST:testlib/components?usageKey=...' + }) + assert status["upstream_link"].startswith("http://course-authoring-mfe/library/") + assert status["upstream_link"].endswith(f"/components?usageKey={self.upstream_html1['id']}") + + # Check the OLX of the downstream block. Notice that: + # (1) fields display_name and data (content/body of the ) are synced. + self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" + This is the HTML. + """) + + # 2️⃣ Now, lets modify the upstream html AND the downstream html: + + self._update_course_block_fields(downstream_html1["locator"], { + "display_name": "Text Content", + "data": "The new downstream data.", # This change will be stay + }) + + self._set_library_block_olx( + self.upstream_html1["id"], + 'The new upstream data.' + ) + self._publish_library_block(self.upstream_html1["id"]) + + # Here's how the downstream OLX looks now, before we sync: + self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" + The new downstream data. + """) + + status = self._get_sync_status(downstream_html1["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_html1["id"], # e.g. 'lb:CL-TEST:testlib:html:html1' + 'version_available': 3, # <--- updated + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': True, # <--- updated + 'error_message': None, + 'is_modified': True, + }) + + # 3️⃣ Now, sync and check the resulting OLX of the downstream + + self._sync_downstream(downstream_html1["locator"]) + + # Here's how the downstream OLX looks now, after we synced it. + # Notice: + # (1) "display_name" field is synced as it was not customized in course. + # (2) the "data" is left alone (customized downstream), but + # (3) "upstream_data" is updated. + self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" + The new downstream data. + """) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index c98e3d764148..c96ed79133f1 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -292,6 +292,8 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): """ Wraps the results of rendering an XBlock view in a div which adds a header and Studio action buttons. """ + # Allow some imported components to be edited by authors in course. + editable_library_components = ["html"] # Only add the Studio wrapper when on the container page. The "Pages" page will remain as is for now. if not context.get('is_pages_view', None) and view in PREVIEW_VIEWS: root_xblock = context.get('root_xblock') @@ -305,7 +307,11 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): can_edit = context.get('can_edit', True) can_add = context.get('can_add', True) upstream_link = UpstreamLink.try_get_for_block(root_xblock, log_error=False) - if upstream_link.error_message is None and isinstance(upstream_link.upstream_key, LibraryContainerLocator): + if ( + upstream_link.error_message is None + and isinstance(upstream_link.upstream_key, LibraryContainerLocator) + and xblock.category not in editable_library_components + ): # If this unit is linked to a library unit, for now we make it completely read-only # because when it is synced, all local changes like added components will be lost. # (This is only on the frontend; the backend doesn't enforce it) diff --git a/cms/envs/common.py b/cms/envs/common.py index a6896160b630..58155c81830d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -843,8 +843,8 @@ InheritanceMixin, XModuleMixin, EditInfoMixin, + UpstreamSyncMixin, # Should be above AuthoringMixin for UpstreamSyncMixin.editor_saved to take effect AuthoringMixin, - UpstreamSyncMixin, ) # .. setting_name: XBLOCK_EXTRA_MIXINS diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 805019f5850d..88434fbad9c2 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -25,7 +25,7 @@ from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2 from xblock.exceptions import XBlockNotFoundError -from xblock.fields import Scope, String, Integer +from xblock.fields import Scope, String, Integer, List from xblock.core import XBlockMixin, XBlock if t.TYPE_CHECKING: @@ -81,6 +81,7 @@ class UpstreamLink: 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. + is_modified: bool | None # If modified in course, True. Otherwise, False. @property def ready_to_sync(self) -> bool: @@ -142,6 +143,7 @@ def try_get_for_block(cls, downstream: XBlock, log_error: bool = True) -> t.Self version_available=None, version_declined=None, error_message=str(exc), + is_modified=len(getattr(downstream, "downstream_customized", [])) > 0, ) @classmethod @@ -221,6 +223,7 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: version_available=version_available, version_declined=downstream.upstream_version_declined, error_message=None, + is_modified=len(getattr(downstream, "downstream_customized", [])) > 0, ) @@ -327,31 +330,6 @@ class UpstreamSyncMixin(XBlockMixin): default=None, scope=Scope.settings, hidden=True, enforce_type=True, ) - @classmethod - def get_customizable_fields(cls) -> dict[str, str | None]: - """ - Mapping from each customizable field to the field which can be used to restore its upstream value. - - If the customizable field is mapped to None, then it is considered "downstream only", and cannot be restored - from the 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", - "attempts_before_showanswer_button": None, - "due": None, - "force_save_button": None, - "graceperiod": None, - "grading_method": None, - "max_attempts": None, - "show_correctness": None, - "show_reset_button": None, - "showanswer": None, - "submission_wait_seconds": None, - "weight": None, - } - # 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 @@ -380,38 +358,66 @@ def get_customizable_fields(cls) -> dict[str, str | None]: # 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 there is no restore_field name, it's a downstream-only field - # if restore_field_name is None: - # 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) + 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, + ) + + @classmethod + def get_customizable_fields(cls) -> dict[str, str | None]: + """ + Mapping from each customizable field to the field which can be used to restore its upstream value. + + If the customizable field is mapped to None, then it is considered "downstream only", and cannot be restored + from the 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", + "attempts_before_showanswer_button": None, + "due": None, + "force_save_button": None, + "graceperiod": None, + "grading_method": None, + "max_attempts": None, + "show_correctness": None, + "show_reset_button": None, + "showanswer": None, + "submission_wait_seconds": None, + "weight": None, + } + + def editor_saved(self, user, old_metadata, old_content): + """ + Update `downstream_customized` when a customizable field is modified. + """ + super().editor_saved(user, old_metadata, old_content) + customizable_fields = self.get_customizable_fields() + new_data = ( + self.get_explicitly_set_fields_by_scope(Scope.settings) + | self.get_explicitly_set_fields_by_scope(Scope.content) + ) + old_data = old_metadata | old_content + + # Loop through all the fields that are potentially cutomizable. + for field_name, restore_field_name in customizable_fields.items(): + + # 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 there is no restore_field name, it's a downstream-only field + if restore_field_name is None: + 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 new_data.get(field_name) != old_data.get(restore_field_name): + self.downstream_customized.append(field_name) diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index 12c0095fe3f1..096327965ce0 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -107,7 +107,6 @@ def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fe 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) @@ -119,14 +118,13 @@ def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fe # 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 + 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. + # OLD BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. + # downstream_value = getattr(downstream, field_name) + # if old_upstream_value and downstream_value != old_upstream_value: + # continue # Field has been customized. Don't touch it. Move on. # Field isn't customized -- SYNC it! setattr(downstream, field_name, new_upstream_value) @@ -137,7 +135,10 @@ def _update_non_customizable_fields(*, upstream: XBlock, downstream: XBlock) -> 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()) + # Remove both field_name and its upstream_* counterpart from the list of fields to copy + customizable_fields = set(downstream.get_customizable_fields().keys()) | set( + downstream.get_customizable_fields().values() + ) # TODO: resolve this so there's no special-case happening for video block. # e.g. by some non_cloneable_fields property of the XBlock class? is_video_block = downstream.usage_key.block_type == "video" diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index f6022c6ac0e5..5ef7359a12e7 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -117,6 +117,12 @@ ${_("Sourced from a library - but the upstream link is broken/invalid.")} + % elif upstream_info.is_modified: + + + ${_("Sourced from a library - but has been modified locally.")} % else: Upstream content V3" # "unsafe" override is gone + assert downstream.data == "Downstream content override" # "safe" customization survives + # Verify hidden field has latest upstream value + assert downstream.upstream_data == "Upstream content V3" + assert downstream.upstream_display_name == "Upstream Title V3" # 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_block(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_block(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_block(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_block(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_block(downstream, self.user) - # assert downstream.upstream_version == 5 - # assert downstream.upstream_display_name == downstream.display_name == "Title V5" + + 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_block(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" + save_xblock_with_callback(downstream, self.user) + assert downstream.downstream_customized == ["display_name"] + + # Syncing should retain the customization. + sync_from_upstream_block(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() + libs.publish_changes(self.library.key, self.user.id) + + # ...which is reflected when we sync. + sync_from_upstream_block(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() + libs.publish_changes(self.library.key, self.user.id) + + # ...then the downstream title should remain put. + sync_from_upstream_block(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() + libs.publish_changes(self.library.key, self.user.id) + sync_from_upstream_block(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_update_customizable_fields(self, initial_upstream_display_name): From f61c80071a8a72c71ff4ef1b38a3eeae4715c6a5 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 6 Aug 2025 15:57:54 +0530 Subject: [PATCH 04/25] fix: lint issues --- cms/djangoapps/contentstore/helpers.py | 2 +- .../contentstore/rest_api/v2/views/tests/test_downstreams.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index b8a5588ce130..225906515d3b 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -488,7 +488,7 @@ def _fetch_and_set_upstream_link( # temp_xblock.data == temp_xblock.upstream_data # for html blocks # Even then we want to set `downstream_customized` value to avoid overriding user customaisations on sync downstream_customized = temp_xblock.xml_attributes.get("downstream_customized", '[]') - setattr(temp_xblock, "downstream_customized", json.loads(downstream_customized)) + temp_xblock.downstream_customized = json.loads(downstream_customized) def _import_xml_node_to_parent( 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 0b62b5ba1adb..536aac85a699 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 @@ -46,6 +46,7 @@ def _get_upstream_link_good_and_syncable(downstream): version_available=(downstream.upstream_version or 0) + 1, version_declined=downstream.upstream_version_declined, error_message=None, + is_modified=False, ) From 301ce21f471b3aa46b9b1b1b2cc548f59eab59d5 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 6 Aug 2025 18:58:08 +0530 Subject: [PATCH 05/25] test: copy paste --- .../tests/test_downstream_sync_integration.py | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) 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 b088f4e2e39a..346e35f97fd3 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 @@ -67,6 +67,29 @@ 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"] + def _copy_course_block(self, usage_key: str): + """ + Copy a course block to the clipboard + """ + data = self._api( + 'post', + "/api/content-staging/v1/clipboard/", + {"usage_key": usage_key}, + expect_response=200 + ) + return data + + def _paste_course_block(self, parent_usage_key: str): + """ + Paste a course block from the clipboard + """ + return self._api( + 'post', + '/xblock/', + {"parent_locator": parent_usage_key, "staged_content": "clipboard"}, + expect_response=200 + ) + # def _get_course_block_fields(self, usage_key: str): # return self._api('get', f'/xblock/{usage_key}', {}, expect_response=200) @@ -473,7 +496,7 @@ def test_unit_sync(self): """) - def test_html_sync(self): + def test_html_sync_and_copy_paste(self): """ Test that we can sync a html from a library into a course. """ @@ -567,3 +590,28 @@ def test_html_sync(self): downstream_customized="["data"]" >The new downstream data. """) + + # Copy this modified html block + self._copy_course_block(downstream_html1["locator"]) + # Paste it + pasted_block = self._paste_course_block(str(self.course_subsection.usage_key)) + + # The pasted block will have same fields as the above block except the + # upstream_* fields will now have the original blocks base value, so + # pasted_block.upstream_display_name == downstream_html1.display_name + # pasted_block.upstream_data == downstream_html1.data + # while still have `downstream_customized` same to avoid overriding during sync + # to allow authors to revert back to back to the original copied customized value + + # See `upstream_data` below is same as how downstream_html1.data is set. + self.assertXmlEqual(self._get_course_block_olx(pasted_block["locator"]), f""" + The new downstream data. + """) From d262f5e46a9cd1f84167f5b75236b06f1aea2108 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 7 Aug 2025 16:06:08 +0530 Subject: [PATCH 06/25] feat: skip sync if html data is modified --- .../tests/test_downstream_sync_integration.py | 83 +++++++++++++++---- .../xblock_storage_handlers/view_handlers.py | 5 +- cms/lib/xblock/upstream_sync_block.py | 23 ++++- 3 files changed, 92 insertions(+), 19 deletions(-) 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 346e35f97fd3..5233e16bfb65 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 @@ -533,11 +533,9 @@ def test_html_sync_and_copy_paste(self): >This is the HTML. """) - # 2️⃣ Now, lets modify the upstream html AND the downstream html: - + # 2️⃣ Now, lets modify the upstream html AND the downstream display_name: self._update_course_block_fields(downstream_html1["locator"], { - "display_name": "Text Content", - "data": "The new downstream data.", # This change will be stay + "display_name": "New Text Content", }) self._set_library_block_olx( @@ -549,14 +547,14 @@ def test_html_sync_and_copy_paste(self): # Here's how the downstream OLX looks now, before we sync: self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" The new downstream data. + >This is the HTML. """) status = self._get_sync_status(downstream_html1["locator"]) @@ -576,18 +574,71 @@ def test_html_sync_and_copy_paste(self): # Here's how the downstream OLX looks now, after we synced it. # Notice: - # (1) "display_name" field is synced as it was not customized in course. - # (2) the "data" is left alone (customized downstream), but - # (3) "upstream_data" is updated. + # (1) "display_name" field is left alone as it was customized + # (2) the "data" field is updated as only display_name was modified + # (3) "upstream_display_name" is updated. + # (4) "upstream_data" is updated. self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" The new upstream data. + """) + + # 2️⃣ Now, lets modify the upstream html AND the downstream html: + + self._update_course_block_fields(downstream_html1["locator"], { + "data": "The new downstream data.", # This change will be stay + }) + + self._set_library_block_olx( + self.upstream_html1["id"], + 'The new upstream data 2.' + ) + self._publish_library_block(self.upstream_html1["id"]) + + # Here's how the downstream OLX looks now, before we sync: + self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" + The new downstream data. + """) + + status = self._get_sync_status(downstream_html1["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_html1["id"], # e.g. 'lb:CL-TEST:testlib:html:html1' + 'version_available': 4, # <--- updated + 'version_synced': 3, + 'version_declined': None, + 'ready_to_sync': True, # <--- updated + 'error_message': None, + 'is_modified': True, + }) + + # 3️⃣ Now, sync and check the resulting OLX of the downstream + self._sync_downstream(downstream_html1["locator"]) + + # Notice that the update is completely skipped except version field is updated. + self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" + The new downstream data. """) @@ -606,12 +657,12 @@ def test_html_sync_and_copy_paste(self): # See `upstream_data` below is same as how downstream_html1.data is set. self.assertXmlEqual(self._get_course_block_olx(pasted_block["locator"]), f""" The new downstream data. """) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index a04d2f3217ff..7bddbd8253b0 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -535,7 +535,10 @@ def sync_library_content(downstream: XBlock, request, store) -> StaticFileNotice upstream_key = link.upstream_key if isinstance(upstream_key, LibraryUsageLocatorV2): lib_block = sync_from_upstream_block(downstream=downstream, user=request.user) - static_file_notices = import_static_assets_for_library_sync(downstream, lib_block, request) + if lib_block: + static_file_notices = import_static_assets_for_library_sync(downstream, lib_block, request) + else: + static_file_notices = StaticFileNotices() store.update_item(downstream, request.user.id) else: with store.bulk_operations(downstream.usage_key.context_key): diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index c7cdff62f47c..a15ddcdcff79 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -15,13 +15,13 @@ from xblock.fields import Scope from xblock.core import XBlock -from .upstream_sync import UpstreamLink, BadUpstream +from .upstream_sync import UpstreamLink, BadDownstream, BadUpstream if t.TYPE_CHECKING: from django.contrib.auth.models import User # pylint: disable=imported-auth-user -def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock: +def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock | None: """ Update `downstream` with content+settings from the latest available version of its linked upstream content. @@ -36,6 +36,12 @@ def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock: link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException if not isinstance(link.upstream_key, LibraryUsageLocatorV2): raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") + try: + _allow_modification_to_display_name_only(downstream) + except BadDownstream: + # Update only version to avoid showing this in updates available list. + downstream.upstream_version = link.version_available + return None # Upstream is a library block: upstream = _load_upstream_block(downstream, user) _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) @@ -57,6 +63,19 @@ def fetch_customizable_fields_from_block(*, downstream: XBlock, user: User, upst _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) +def _allow_modification_to_display_name_only(downstream: XBlock): + """ + Raise error if any field except for display_name is modified in course locally. + This is a temporary function to skip sync completely if other fields are modified. + """ + if len(downstream.downstream_customized) == 0: + return + if len(downstream.downstream_customized) > 1: + raise BadDownstream("Multiple fields modified, skip sync operation.") + if downstream.downstream_customized[0] != 'display_name': + raise BadDownstream("Only display_name modification is allowed, skip sync operation.") + + def _load_upstream_block(downstream: XBlock, user: User) -> XBlock: """ Load the upstream metadata and content for a downstream block. From d2ba2e76f448d3ba549a4297bc7b0b191f12cdf9 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 15 Aug 2025 12:25:43 +0530 Subject: [PATCH 07/25] feat: update upstream fields only when modified --- .../views/tests/test_downstream_sync_integration.py | 11 ++++++----- cms/lib/xblock/upstream_sync_block.py | 6 ++++-- .../core/lib/xblock_serializer/block_serializer.py | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) 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 b6986fd4b106..206d45cd0284 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 @@ -15,6 +15,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory +from xmodule.xml_block import serialize_field @ddt.ddt @@ -292,9 +293,9 @@ def test_unit_sync(self): parent_usage_key=str(self.course_subsection.usage_key), upstream_key=self.upstream_unit["id"], ) - downstream_unit_block_key = get_block_key_dict( + downstream_unit_block_key = serialize_field(get_block_key_dict( UsageKey.from_string(downstream_unit["locator"]), - ) + )).replace('"', '"') 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' @@ -982,15 +983,15 @@ def test_html_sync_and_copy_paste(self): # 3️⃣ Now, sync and check the resulting OLX of the downstream self._sync_downstream(downstream_html1["locator"]) - # Notice that the update is completely skipped except version field is updated. + # Notice that the update is skipped except version and upstream_* fields are updated. self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" The new downstream data. """) diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index a15ddcdcff79..6bf53886d155 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -36,14 +36,16 @@ def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock | None: link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException if not isinstance(link.upstream_key, LibraryUsageLocatorV2): raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") + upstream = _load_upstream_block(downstream, user) try: _allow_modification_to_display_name_only(downstream) except BadDownstream: - # Update only version to avoid showing this in updates available list. + # Update upstream_* fields only + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) + # Update version to avoid showing this in updates available list. downstream.upstream_version = link.version_available return None # Upstream is a library block: - upstream = _load_upstream_block(downstream, user) _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) _update_non_customizable_fields(upstream=upstream, downstream=downstream) _update_tags(upstream=upstream, downstream=downstream) diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 9495a343ad01..76611624cdc6 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -141,7 +141,7 @@ def _serialize_normal_block(self, block) -> etree.Element: if "top_level_downstream_parent_key" in block.fields \ and block.fields["top_level_downstream_parent_key"].is_set_on(block): - olx_node.attrib["top_level_downstream_parent_key"] = str(block.top_level_downstream_parent_key) + olx_node.attrib["top_level_downstream_parent_key"] = serialize_field(block.top_level_downstream_parent_key) return olx_node From 8b784fff2f49b43702c952e7f955bd4048e8cc69 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 18 Aug 2025 10:32:49 +0530 Subject: [PATCH 08/25] refactor: use version_synced field to skip sync --- cms/djangoapps/contentstore/models.py | 6 ++++++ cms/djangoapps/contentstore/utils.py | 31 +++++++++++++++++++++------ cms/lib/xblock/upstream_sync_block.py | 4 ++-- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index f641fbee1f7f..90ce5fc9afbb 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -188,6 +188,9 @@ def filter_links( ) & GreaterThan( Coalesce("upstream_block__publishable_entity__published__version__version_num", 0), Coalesce("version_declined", 0) + ) & GreaterThan( + Coalesce("version_synced", 0), + -1, ) ) ) @@ -410,6 +413,9 @@ def _annotate_query_with_ready_to_sync(cls, query_set: QuerySet["EntityLinkBase" ) & GreaterThan( Coalesce("upstream_container__publishable_entity__published__version__version_num", 0), Coalesce("version_declined", 0) + ) & GreaterThan( + Coalesce("version_synced", 0), + -1, ) ) ) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 6204f020d990..a365fdfc34dd 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -61,6 +61,8 @@ ) from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.djangoapps.models.settings.course_metadata import CourseMetadata +from cms.lib.xblock.upstream_sync import BadDownstream +from cms.lib.xblock.upstream_sync_block import check_downstream_customization from common.djangoapps.course_action_state.managers import CourseActionStateItemNotFoundError from common.djangoapps.course_action_state.models import CourseRerunState, CourseRerunUIStateManager from common.djangoapps.course_modes.models import CourseMode @@ -2385,7 +2387,12 @@ def get_xblock_render_context(request, block): return "" -def _create_or_update_component_link(course_key: CourseKey, created: datetime | None, xblock): +def _create_or_update_component_link( + course_key: CourseKey, + created: datetime | None, + xblock, + downstream_customized: bool = False +): """ Create or update upstream->downstream link for components in database for given xblock. """ @@ -2411,13 +2418,19 @@ def _create_or_update_component_link(course_key: CourseKey, created: datetime | downstream_context_key=course_key, downstream_usage_key=xblock.usage_key, top_level_parent_usage_key=top_level_parent_usage_key, - version_synced=xblock.upstream_version, + # Set version_synced to -1 if downstream was modified and further syncing doesn't make sense. + version_synced=xblock.upstream_version if not downstream_customized else -1, version_declined=xblock.upstream_version_declined, created=created, ) -def _create_or_update_container_link(course_key: CourseKey, created: datetime | None, xblock): +def _create_or_update_container_link( + course_key: CourseKey, + created: datetime | None, + xblock, + downstream_customized: bool = False +): """ Create or update upstream->downstream link for containers in database for given xblock. """ @@ -2442,7 +2455,8 @@ def _create_or_update_container_link(course_key: CourseKey, created: datetime | upstream_context_key=str(upstream_container_key.context_key), downstream_context_key=course_key, downstream_usage_key=xblock.usage_key, - version_synced=xblock.upstream_version, + # Set version_synced to -1 if downstream was modified and further syncing doesn't make sense. + version_synced=xblock.upstream_version if not downstream_customized else -1, top_level_parent_usage_key=top_level_parent_usage_key, version_declined=xblock.upstream_version_declined, created=created, @@ -2453,15 +2467,20 @@ def create_or_update_xblock_upstream_link(xblock, course_key: CourseKey, created """ Create or update upstream->downstream link in database for given xblock. """ + downstream_customized = False if not xblock.upstream: return None + try: + check_downstream_customization(xblock) + except BadDownstream: + downstream_customized = True try: # Try to create component link - _create_or_update_component_link(course_key, created, xblock) + _create_or_update_component_link(course_key, created, xblock, downstream_customized=downstream_customized) except InvalidKeyError: # It is possible that the upstream is a container and UsageKeyV2 parse failed # Create upstream container link and raise InvalidKeyError if xblock.upstream is a valid key. - _create_or_update_container_link(course_key, created, xblock) + _create_or_update_container_link(course_key, created, xblock, downstream_customized=downstream_customized) def get_previous_run_course_key(course_key): diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index 6bf53886d155..63d98a880971 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -38,7 +38,7 @@ def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock | None: raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") upstream = _load_upstream_block(downstream, user) try: - _allow_modification_to_display_name_only(downstream) + check_downstream_customization(downstream) except BadDownstream: # Update upstream_* fields only _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) @@ -65,7 +65,7 @@ def fetch_customizable_fields_from_block(*, downstream: XBlock, user: User, upst _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) -def _allow_modification_to_display_name_only(downstream: XBlock): +def check_downstream_customization(downstream: XBlock): """ Raise error if any field except for display_name is modified in course locally. This is a temporary function to skip sync completely if other fields are modified. From 026e248aa2a37ba26582ef74d111b41ed04d704e Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 21 Aug 2025 16:16:27 +0530 Subject: [PATCH 09/25] feat: edit title inplace for library source components --- cms/static/js/views/pages/container.js | 32 ++++++++++++++++++++++++ cms/templates/studio_xblock_wrapper.html | 5 ++++ 2 files changed, 37 insertions(+) diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 7360c79317de..367f452827bc 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -25,6 +25,7 @@ function($, _, Backbone, gettext, BasePage, events: { 'click .edit-button': 'editXBlock', + 'click .title-edit-button': 'clickTitleButton', 'click .access-button': 'editVisibilitySettings', 'click .duplicate-button': 'duplicateXBlock', 'click .copy-button': 'copyXBlock', @@ -1254,6 +1255,37 @@ function($, _, Backbone, gettext, BasePage, scrollToNewComponentButtons: function(event) { event.preventDefault(); $.scrollTo(this.$('.add-xblock-component'), {duration: 250}); + }, + + clickTitleButton: function(event) { + const xblockElement = this.findXBlockElement(event.target); + const xblockInfo = XBlockUtils.findXBlockInfo(xblockElement, this.model); + var self = this, + oldTitle = xblockInfo.get('display_name'), + titleElt = $(xblockElement).find('.xblock-display-name'), + buttonElt = $(xblockElement).find('.title-edit-button'), + $input = $(''), + changeFunc = function(evt) { + var newTitle = $(evt.target).val(); + if (oldTitle !== newTitle) { + xblockInfo.set('display_name', newTitle); + return XBlockUtils.updateXBlockField(xblockInfo, "display_name", newTitle).done(function() { + self.refreshXBlock(xblockElement, false); + }); + } else { + titleElt.html(newTitle); + $(buttonElt).show(); + } + return true; + }; + event.preventDefault(); + + $input.val(oldTitle); + $input.change(changeFunc).blur(changeFunc); + titleElt.html($input); // xss-lint: disable=javascript-jquery-html + $input.focus().select(); + $(buttonElt).hide(); + return true; } }); diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 5ef7359a12e7..56bb4d26a321 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -132,6 +132,11 @@ % endif % endif ${label} + % if not can_edit: + + % endif % endif % if selected_groups_label:

${selected_groups_label}

From 04aaef1fbce2810f8ad7191a429fcc0de3760d0d Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 21 Aug 2025 19:50:31 +0530 Subject: [PATCH 10/25] fixup! feat: edit title inplace for library source components --- cms/djangoapps/contentstore/views/preview.py | 16 +++++++++++----- cms/static/js/views/pages/container.js | 2 +- .../sass/course-unit-mfe-iframe-bundle.scss | 8 ++++++++ cms/templates/studio_xblock_wrapper.html | 12 ++++++------ 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index c96ed79133f1..4f8312fdb3f5 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -306,17 +306,22 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): can_edit = context.get('can_edit', True) can_add = context.get('can_add', True) - upstream_link = UpstreamLink.try_get_for_block(root_xblock, log_error=False) + can_move = context.get('can_move', True) + root_upstream_link = UpstreamLink.try_get_for_block(root_xblock, log_error=False) + upstream_link = UpstreamLink.try_get_for_block(xblock, log_error=False) if ( - upstream_link.error_message is None - and isinstance(upstream_link.upstream_key, LibraryContainerLocator) - and xblock.category not in editable_library_components + root_upstream_link.error_message is None + and isinstance(root_upstream_link.upstream_key, LibraryContainerLocator) ): # If this unit is linked to a library unit, for now we make it completely read-only # because when it is synced, all local changes like added components will be lost. # (This is only on the frontend; the backend doesn't enforce it) can_edit = False can_add = False + can_move = False + + if upstream_link.error_message is None and upstream_link.upstream_ref: + can_edit = xblock.category in editable_library_components # Is this a course or a library? is_course = xblock.context_key.is_course @@ -342,10 +347,11 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): # Generally speaking, "if you can add, you can delete". One exception is itembank (Problem Bank) # which has its own separate "add" workflow but uses the normal delete workflow for its child blocks. 'can_delete': can_add or (root_xblock and root_xblock.scope_ids.block_type == "itembank" and can_edit), - 'can_move': context.get('can_move', is_course), + 'can_move': can_move, 'language': getattr(course, 'language', None), 'is_course': is_course, 'tags_count': tags_count, + 'can_edit_title': True, # This is always true even for imported components } add_webpack_js_to_fragment(frag, "js/factories/xblock_validation") diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 367f452827bc..fa090eb459df 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -1264,7 +1264,7 @@ function($, _, Backbone, gettext, BasePage, oldTitle = xblockInfo.get('display_name'), titleElt = $(xblockElement).find('.xblock-display-name'), buttonElt = $(xblockElement).find('.title-edit-button'), - $input = $(''), + $input = $(''), changeFunc = function(evt) { var newTitle = $(evt.target).val(); if (oldTitle !== newTitle) { diff --git a/cms/static/sass/course-unit-mfe-iframe-bundle.scss b/cms/static/sass/course-unit-mfe-iframe-bundle.scss index 4100310f406b..3f46bf65ec24 100644 --- a/cms/static/sass/course-unit-mfe-iframe-bundle.scss +++ b/cms/static/sass/course-unit-mfe-iframe-bundle.scss @@ -567,6 +567,14 @@ body, background-color: #f8f7f6; } +input.xblock-inline-title-editor { + padding-top: 0px; + padding-bottom: 0px; + font-size: 18px; + height: 50px; + font-weight: 700; +} + .btn-default.action-edit.title-edit-button { @extend %button-styles; diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 56bb4d26a321..e50cd65325c3 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -132,11 +132,6 @@ % endif % endif ${label} - % if not can_edit: - - % endif % endif % if selected_groups_label:

${selected_groups_label}

@@ -145,6 +140,11 @@
    + % if can_edit_title and not can_edit: + + % endif % if not is_root: % if can_edit: % if upstream_info.ready_to_sync: @@ -217,7 +217,7 @@
- % if is_reorderable: + % if is_reorderable and can_move:
    • From ae2ed50a1a135c389188b5295c6f7dddc4bf6efd Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 22 Aug 2025 11:18:27 +0530 Subject: [PATCH 11/25] fix: edit title button style --- cms/templates/studio_xblock_wrapper.html | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index e50cd65325c3..c7c8fab47ee6 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -141,9 +141,12 @@
      % if can_edit_title and not can_edit: - +
    • + +
      • % endif % if not is_root: % if can_edit: From 083df77145b26c96c53b4dec55a179268f4bd212 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 22 Aug 2025 11:32:19 +0530 Subject: [PATCH 12/25] fix: test case --- .../v2/views/tests/test_downstream_sync_integration.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 5045eac45278..5cd58779e48f 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 @@ -1051,9 +1051,9 @@ def test_unit_decline_sync(self): parent_usage_key=str(self.course_subsection.usage_key), upstream_key=self.upstream_unit["id"], ) - downstream_unit_block_key = get_block_key_dict( + downstream_unit_block_key = serialize_field(get_block_key_dict( UsageKey.from_string(downstream_unit["locator"]), - ) + )).replace('"', '"') children_downstream_keys = self._get_course_block_children(downstream_unit["locator"]) downstream_problem1 = children_downstream_keys[1] assert "type@problem" in downstream_problem1 @@ -1089,6 +1089,7 @@ def test_unit_decline_sync(self): upstream="{self.upstream_html1['id']}" upstream_version="2" top_level_downstream_parent_key="{downstream_unit_block_key}" + upstream_data="This is the HTML." >This is the HTML. This is the HTML. Date: Fri, 22 Aug 2025 12:33:57 +0530 Subject: [PATCH 13/25] fix: lint issue --- cms/static/js/views/pages/container.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index fa090eb459df..5253348c66c4 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -1273,7 +1273,7 @@ function($, _, Backbone, gettext, BasePage, self.refreshXBlock(xblockElement, false); }); } else { - titleElt.html(newTitle); + titleElt.html(newTitle); // xss-lint: disable=javascript-jquery-html $(buttonElt).show(); } return true; From c4a696198f92a2a34af1ff2c5f4022c42d827703 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 25 Aug 2025 11:43:15 +0530 Subject: [PATCH 14/25] refactor: don't show different icon for modified upstream blocks --- cms/templates/studio_xblock_wrapper.html | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index c7c8fab47ee6..ea363a3f7f62 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -117,18 +117,16 @@ ${_("Sourced from a library - but the upstream link is broken/invalid.")} - % elif upstream_info.is_modified: - - - ${_("Sourced from a library - but has been modified locally.")} % else: - ${_("Sourced from a library - but has been modified locally.")} + % else: ${_("Sourced from a library.")} + % endif % endif % endif ${label} From adde7216c2396c2f98b8cbfe92ee8fee997b1eb7 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 10 Sep 2025 12:28:15 +0530 Subject: [PATCH 15/25] Revert "refactor: use version_synced field to skip sync" This reverts commit 8b784fff2f49b43702c952e7f955bd4048e8cc69. --- cms/djangoapps/contentstore/models.py | 6 ------ cms/djangoapps/contentstore/utils.py | 19 ++++--------------- cms/lib/xblock/upstream_sync_block.py | 4 ++-- 3 files changed, 6 insertions(+), 23 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 41388ebd5750..fd98ec44fe09 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -215,9 +215,6 @@ def filter_links( ) & GreaterThan( Coalesce("upstream_block__publishable_entity__published__version__version_num", 0), Coalesce("version_declined", 0) - ) & GreaterThan( - Coalesce("version_synced", 0), - -1, ) ), # This is alwys False, the components doens't have children @@ -465,9 +462,6 @@ def _annotate_query_with_ready_to_sync(cls, query_set: QuerySet["EntityLinkBase" ) & GreaterThan( Coalesce("upstream_container__publishable_entity__published__version__version_num", 0), Coalesce("version_declined", 0) - ) & GreaterThan( - Coalesce("version_synced", 0), - -1, ) ), ready_to_sync_from_children=ExpressionWrapper( diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index d29a3d3be250..062c59647619 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -61,8 +61,6 @@ ) from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.djangoapps.models.settings.course_metadata import CourseMetadata -from cms.lib.xblock.upstream_sync import BadDownstream -from cms.lib.xblock.upstream_sync_block import check_downstream_customization from common.djangoapps.course_action_state.managers import CourseActionStateItemNotFoundError from common.djangoapps.course_action_state.models import CourseRerunState, CourseRerunUIStateManager from common.djangoapps.course_modes.models import CourseMode @@ -2390,7 +2388,6 @@ def get_xblock_render_context(request, block): def _create_or_update_component_link( created: datetime | None, xblock, - downstream_customized: bool = False ): """ Create or update upstream->downstream link for components in database for given xblock. @@ -2417,8 +2414,7 @@ def _create_or_update_component_link( downstream_context_key=xblock.usage_key.course_key, downstream_usage_key=xblock.usage_key, top_level_parent_usage_key=top_level_parent_usage_key, - # Set version_synced to -1 if downstream was modified and further syncing doesn't make sense. - version_synced=xblock.upstream_version if not downstream_customized else -1, + version_synced=xblock.upstream_version, version_declined=xblock.upstream_version_declined, created=created, ) @@ -2427,7 +2423,6 @@ def _create_or_update_component_link( def _create_or_update_container_link( created: datetime | None, xblock, - downstream_customized: bool = False ): """ Create or update upstream->downstream link for containers in database for given xblock. @@ -2453,8 +2448,7 @@ def _create_or_update_container_link( upstream_context_key=str(upstream_container_key.context_key), downstream_context_key=xblock.usage_key.course_key, downstream_usage_key=xblock.usage_key, - # Set version_synced to -1 if downstream was modified and further syncing doesn't make sense. - version_synced=xblock.upstream_version if not downstream_customized else -1, + version_synced=xblock.upstream_version, top_level_parent_usage_key=top_level_parent_usage_key, version_declined=xblock.upstream_version_declined, created=created, @@ -2465,20 +2459,15 @@ def create_or_update_xblock_upstream_link(xblock, created: datetime | None = Non """ Create or update upstream->downstream link in database for given xblock. """ - downstream_customized = False if not xblock.upstream: return None - try: - check_downstream_customization(xblock) - except BadDownstream: - downstream_customized = True try: # Try to create component link - _create_or_update_component_link(created, xblock, downstream_customized=downstream_customized) + _create_or_update_component_link(created, xblock) except InvalidKeyError: # It is possible that the upstream is a container and UsageKeyV2 parse failed # Create upstream container link and raise InvalidKeyError if xblock.upstream is a valid key. - _create_or_update_container_link(created, xblock, downstream_customized=downstream_customized) + _create_or_update_container_link(created, xblock) def get_previous_run_course_key(course_key): diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index 63d98a880971..6bf53886d155 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -38,7 +38,7 @@ def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock | None: raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") upstream = _load_upstream_block(downstream, user) try: - check_downstream_customization(downstream) + _allow_modification_to_display_name_only(downstream) except BadDownstream: # Update upstream_* fields only _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) @@ -65,7 +65,7 @@ def fetch_customizable_fields_from_block(*, downstream: XBlock, user: User, upst _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) -def check_downstream_customization(downstream: XBlock): +def _allow_modification_to_display_name_only(downstream: XBlock): """ Raise error if any field except for display_name is modified in course locally. This is a temporary function to skip sync completely if other fields are modified. From af030bea1943b72a206ab3aed00f64d00811d1ac Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 11 Sep 2025 16:11:48 +0530 Subject: [PATCH 16/25] feat: only skip sync for modified blocks if updated as part of container --- .../xblock_storage_handlers/view_handlers.py | 6 ++++- cms/lib/xblock/upstream_sync_block.py | 24 ++++++++++++------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 95ce753f1125..a65667b9571f 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -539,7 +539,11 @@ def sync_library_content( link = UpstreamLink.get_for_block(downstream) upstream_key = link.upstream_key if isinstance(upstream_key, LibraryUsageLocatorV2): - lib_block = sync_from_upstream_block(downstream=downstream, user=request.user) + lib_block = sync_from_upstream_block( + downstream=downstream, + user=request.user, + top_level_parent=top_level_parent, + ) if lib_block: static_file_notices = import_static_assets_for_library_sync(downstream, lib_block, request) else: diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index 6bf53886d155..ff59ac31de9c 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -21,7 +21,11 @@ from django.contrib.auth.models import User # pylint: disable=imported-auth-user -def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock | None: +def sync_from_upstream_block( + downstream: XBlock, + user: User, + top_level_parent: XBlock | None = None +) -> XBlock | None: """ Update `downstream` with content+settings from the latest available version of its linked upstream content. @@ -37,14 +41,16 @@ def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock | None: if not isinstance(link.upstream_key, LibraryUsageLocatorV2): raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") upstream = _load_upstream_block(downstream, user) - try: - _allow_modification_to_display_name_only(downstream) - except BadDownstream: - # Update upstream_* fields only - _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) - # Update version to avoid showing this in updates available list. - downstream.upstream_version = link.version_available - return None + # Skip sync if component is being updated as part of a parent container and the component content is modified + if top_level_parent: + try: + _allow_modification_to_display_name_only(downstream) + except BadDownstream: + # Update upstream_* fields only + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) + # Update version to avoid showing this in updates available list. + downstream.upstream_version = link.version_available + return None # Upstream is a library block: _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) _update_non_customizable_fields(upstream=upstream, downstream=downstream) From 5f68c78939beec62dfcac7270f86df448e7c3ba4 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 12 Sep 2025 10:32:52 +0530 Subject: [PATCH 17/25] refactor: update sync behaviour when synced individually and as part of parent --- cms/lib/xblock/upstream_sync.py | 10 +---- cms/lib/xblock/upstream_sync_block.py | 53 ++++++++++++++++----------- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index ae01ebf14d40..f3eee78614fb 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -127,16 +127,8 @@ def check_children_ready_to_sync(xblock_downstream): for child in downstream_children: if child.upstream: child_upstream_link = UpstreamLink.try_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: + if child_upstream_link._is_ready_to_sync_individually: return True if check_children_ready_to_sync(child): diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index ff59ac31de9c..a5fc116fefa6 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -6,21 +6,25 @@ """ from __future__ import annotations +import logging import typing as t from django.core.exceptions import PermissionDenied from django.utils.translation import gettext_lazy as _ -from rest_framework.exceptions import NotFound from opaque_keys.edx.locator import LibraryUsageLocatorV2 -from xblock.fields import Scope +from rest_framework.exceptions import NotFound from xblock.core import XBlock +from xblock.fields import Scope -from .upstream_sync import UpstreamLink, BadDownstream, BadUpstream +from .upstream_sync import BadDownstream, BadUpstream, UpstreamLink if t.TYPE_CHECKING: from django.contrib.auth.models import User # pylint: disable=imported-auth-user +logger = logging.getLogger(__name__) + + def sync_from_upstream_block( downstream: XBlock, user: User, @@ -41,18 +45,21 @@ def sync_from_upstream_block( if not isinstance(link.upstream_key, LibraryUsageLocatorV2): raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") upstream = _load_upstream_block(downstream, user) - # Skip sync if component is being updated as part of a parent container and the component content is modified + # Skip sync only if component is being updated as part of a parent container and the component is modified if top_level_parent: try: - _allow_modification_to_display_name_only(downstream) - except BadDownstream: + # Currently, we don't want to sync changes if any field is modified + _verify_modification_to(downstream, []) + except BadDownstream as e: + logger.warning(str(e), exc_info=True) # Update upstream_* fields only _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) # Update version to avoid showing this in updates available list. downstream.upstream_version = link.version_available return None # Upstream is a library block: - _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) + # Sync all fields from the upstream block and override customizations + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False, keep_customizations=False) _update_non_customizable_fields(upstream=upstream, downstream=downstream) _update_tags(upstream=upstream, downstream=downstream) downstream.upstream_version = link.version_available @@ -71,17 +78,15 @@ def fetch_customizable_fields_from_block(*, downstream: XBlock, user: User, upst _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) -def _allow_modification_to_display_name_only(downstream: XBlock): +def _verify_modification_to(downstream: XBlock, allowed_fields: list[str]): """ - Raise error if any field except for display_name is modified in course locally. - This is a temporary function to skip sync completely if other fields are modified. + Raise error if any field except for fields in allowed_fields is modified in course locally. """ - if len(downstream.downstream_customized) == 0: - return - if len(downstream.downstream_customized) > 1: - raise BadDownstream("Multiple fields modified, skip sync operation.") - if downstream.downstream_customized[0] != 'display_name': - raise BadDownstream("Only display_name modification is allowed, skip sync operation.") + if len(downstream.downstream_customized) > len(allowed_fields): + raise BadDownstream("Too many fields modified, skip sync operation") + not_allowed_modified = set(downstream.downstream_customized).difference(allowed_fields) + if len(not_allowed_modified) > 0: + raise BadDownstream(f"{not_allowed_modified} fields are modified locally") def _load_upstream_block(downstream: XBlock, user: User) -> XBlock: @@ -107,7 +112,13 @@ def _load_upstream_block(downstream: XBlock, user: User) -> XBlock: return lib_block -def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fetch: bool) -> None: +def _update_customizable_fields( + *, + upstream: XBlock, + downstream: XBlock, + only_fetch: bool, + keep_customizations: bool = False, +) -> None: """ For each customizable field: * Save the upstream value to a hidden field on the downstream ("FETCH"). @@ -123,6 +134,9 @@ def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fe * Set `course_problem.display_name = lib_problem.display_name` ("sync"). """ syncable_field_names = _get_synchronizable_fields(upstream, downstream) + if not keep_customizations and not only_fetch: + # Clear downstream_customized field on downstream to override all customizations + downstream.downstream_customized = [] for field_name, fetch_field_name in downstream.get_customizable_fields().items(): @@ -146,11 +160,6 @@ def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fe if field_name in downstream.downstream_customized: continue - # OLD BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. - # downstream_value = getattr(downstream, field_name) - # if old_upstream_value and downstream_value != old_upstream_value: - # continue # Field has been customized. Don't touch it. Move on. - # Field isn't customized -- SYNC it! setattr(downstream, field_name, new_upstream_value) From f4e43e065d9da0991c0cae47a5d0e9091d343bdb Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 12 Sep 2025 11:54:53 +0530 Subject: [PATCH 18/25] feat: include ready to sync children info in downstream link get api --- .../rest_api/v2/views/downstreams.py | 2 +- cms/lib/xblock/upstream_sync.py | 86 ++++++++++++------- 2 files changed, 58 insertions(+), 30 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index d41770702909..9cdfc9706eee 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -394,7 +394,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) - return Response(UpstreamLink.try_get_for_block(downstream).to_json()) + return Response(UpstreamLink.try_get_for_block(downstream).to_json(include_child_info=True)) def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index f3eee78614fb..a425e2d36a44 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -96,6 +96,43 @@ def _is_ready_to_sync_individually(self) -> bool: self.version_available > (self.version_declined or 0) ) + def _check_children_ready_to_sync(self, xblock_downstream: XBlock, return_fast: bool) -> list[dict[str, str]]: + """ + Check if all the children of the current XBlock are ready to be synced individually. + + Args: + xblock_downstream (XBlock): The XBlock mixin instance whose children need to be checked. + return_fast (bool): If True, return the first child that is ready to sync. + + Returns: + list[dict]: A list of children id and names that ready to sync. + """ + if not xblock_downstream.has_children: + return [] + + downstream_children = xblock_downstream.get_children() + child_info = [] + + for child in downstream_children: + if child.upstream: + child_upstream_link = UpstreamLink.try_get_for_block(child) + # If one child needs sync, it is not needed to check more children + if child_upstream_link._is_ready_to_sync_individually: + child_info.append({ + 'name': child.display_name, + 'id': str(child.usage_key), + }) + if return_fast: + return child_info + + grand_children_info = self._check_children_ready_to_sync(child, return_fast) + child_info.extend(grand_children_info) + if return_fast and len(grand_children_info) > 0: + # If one child needs sync, it is not needed to check more children + return child_info + + return child_info + @property def ready_to_sync(self) -> bool: """ @@ -112,34 +149,13 @@ def ready_to_sync(self) -> bool: 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.try_get_for_block(child) - # If one child needs sync, it is not needed to check more children - if child_upstream_link._is_ready_to_sync_individually: - 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 self._is_ready_to_sync_individually or ( + self.downstream_key is not None + and len(self._check_children_ready_to_sync( + modulestore().get_item(UsageKey.from_string(self.downstream_key)), + return_fast=True, + )) > 0 + ) return False @property @@ -155,7 +171,7 @@ def upstream_link(self) -> str | None: return _get_library_container_url(self.upstream_key) return None - def to_json(self) -> dict[str, t.Any]: + def to_json(self, include_child_info=False) -> dict[str, t.Any]: """ Get an JSON-API-friendly representation of this upstream link. """ @@ -164,6 +180,18 @@ def to_json(self) -> dict[str, t.Any]: "ready_to_sync": self.ready_to_sync, "upstream_link": self.upstream_link, } + if ( + include_child_info + and self.ready_to_sync + and isinstance(self.upstream_key, LibraryContainerLocator) + and self.downstream_key is not None + ): + from xmodule.modulestore.django import modulestore + + data["ready_to_sync_children"] = self._check_children_ready_to_sync( + modulestore().get_item(UsageKey.from_string(self.downstream_key)), + return_fast=False, + ) del data["upstream_key"] # As JSON (string), this would be redundant with upstream_ref return data From 8fdd55d08ec2489a1887d4977fa7341482b13f26 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 12 Sep 2025 15:41:25 +0530 Subject: [PATCH 19/25] test: fix failing tests --- cms/lib/xblock/test/test_upstream_sync.py | 26 +++++++++++------------ cms/lib/xblock/upstream_sync_block.py | 26 +++++++++++++++-------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index cdd6f9d1ab62..2e8d68de2288 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -316,9 +316,9 @@ def test_sync_updates_to_downstream_only_fields(self): assert downstream.rerandomize == '"per_student"' assert downstream.matlab_api_key == 'def' assert downstream.use_latex_compiler + assert downstream.display_name == "Upstream Problem Title V3" # but "safe" customizations survive - assert downstream.display_name == "Downstream Title Override" assert downstream.attempts_before_showanswer_button == 2 assert downstream.due == datetime.datetime(2025, 2, 2, tzinfo=utc) assert downstream.force_save_button @@ -350,15 +350,16 @@ def test_sync_updates_to_modified_content(self): upstream.save() libs.publish_changes(self.library.key, self.user.id) - # Downstream modifications - downstream.display_name = "Downstream Title Override" # "safe" customization - downstream.data = "Downstream content override" # "safe" customization + # Downstream modifications, currently all fields are overridden on individual sync + downstream.display_name = "Downstream Title Override" + downstream.data = "Downstream content override" save_xblock_with_callback(downstream, self.user) # Follow-up sync. Assert that updates are pulled into downstream, but customizations are saved. sync_from_upstream_block(downstream, self.user) - assert downstream.display_name == "Downstream Title Override" # "safe" customization survives - assert downstream.data == "Downstream content override" # "safe" customization survives + # All customizaions are lost + assert downstream.display_name == "Upstream Title V3" + assert downstream.data == "Upstream content V3" # Verify hidden field has latest upstream value assert downstream.upstream_data == "Upstream content V3" assert downstream.upstream_display_name == "Upstream Title V3" @@ -384,8 +385,8 @@ def test_sync_to_downstream_with_subtle_customization(self): save_xblock_with_callback(downstream, self.user) assert downstream.downstream_customized == ["display_name"] - # Syncing should retain the customization. - sync_from_upstream_block(downstream, self.user) + # Syncing should retain the customization if we allow display name customization. + sync_from_upstream_block(downstream, self.user, keep_custom_fields=["display_name"]) assert downstream.upstream_version == 2 assert downstream.upstream_display_name == "Upstream Title V2" assert downstream.display_name == "Title V3" @@ -397,7 +398,7 @@ def test_sync_to_downstream_with_subtle_customization(self): libs.publish_changes(self.library.key, self.user.id) # ...which is reflected when we sync. - sync_from_upstream_block(downstream, self.user) + sync_from_upstream_block(downstream, self.user, keep_custom_fields=["display_name"]) assert downstream.upstream_version == 3 assert downstream.upstream_display_name == downstream.display_name == "Title V3" @@ -409,17 +410,16 @@ def test_sync_to_downstream_with_subtle_customization(self): libs.publish_changes(self.library.key, self.user.id) # ...then the downstream title should remain put. - sync_from_upstream_block(downstream, self.user) + sync_from_upstream_block(downstream, self.user, keep_custom_fields=["display_name"]) 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 = [] + # Finally, if we don't allow keeping any customizations upstream.display_name = "Title V5" upstream.save() libs.publish_changes(self.library.key, self.user.id) - sync_from_upstream_block(downstream, self.user) + sync_from_upstream_block(downstream, self.user, keep_custom_fields=[]) # No customizations! assert downstream.upstream_version == 5 assert downstream.upstream_display_name == downstream.display_name == "Title V5" diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index a5fc116fefa6..74aabaf8dac2 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -28,7 +28,9 @@ def sync_from_upstream_block( downstream: XBlock, user: User, - top_level_parent: XBlock | None = None + *, + top_level_parent: XBlock | None = None, + keep_custom_fields: list[str] = [], ) -> XBlock | None: """ Update `downstream` with content+settings from the latest available version of its linked upstream content. @@ -49,7 +51,7 @@ def sync_from_upstream_block( if top_level_parent: try: # Currently, we don't want to sync changes if any field is modified - _verify_modification_to(downstream, []) + _verify_modification_to(downstream, keep_custom_fields) except BadDownstream as e: logger.warning(str(e), exc_info=True) # Update upstream_* fields only @@ -59,7 +61,12 @@ def sync_from_upstream_block( return None # Upstream is a library block: # Sync all fields from the upstream block and override customizations - _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False, keep_customizations=False) + _update_customizable_fields( + upstream=upstream, + downstream=downstream, + only_fetch=False, + keep_custom_fields=keep_custom_fields, + ) _update_non_customizable_fields(upstream=upstream, downstream=downstream) _update_tags(upstream=upstream, downstream=downstream) downstream.upstream_version = link.version_available @@ -117,7 +124,7 @@ def _update_customizable_fields( upstream: XBlock, downstream: XBlock, only_fetch: bool, - keep_customizations: bool = False, + keep_custom_fields: list[str] = [], ) -> None: """ For each customizable field: @@ -134,9 +141,6 @@ def _update_customizable_fields( * Set `course_problem.display_name = lib_problem.display_name` ("sync"). """ syncable_field_names = _get_synchronizable_fields(upstream, downstream) - if not keep_customizations and not only_fetch: - # Clear downstream_customized field on downstream to override all customizations - downstream.downstream_customized = [] for field_name, fetch_field_name in downstream.get_customizable_fields().items(): @@ -158,9 +162,13 @@ def _update_customizable_fields( # We need to update the downstream field *iff it has not been customized**. if field_name in downstream.downstream_customized: - continue + if field_name in keep_custom_fields: + continue + else: + # Remove the field from downstream_customized field as it can be overridden + downstream.downstream_customized.remove(field_name) - # Field isn't customized -- SYNC it! + # Field isn't customized or is can be overridden -- SYNC it! setattr(downstream, field_name, new_upstream_value) From ae6e56db8dea93de0c7ed90edafc6105d50807d7 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 12 Sep 2025 15:57:35 +0530 Subject: [PATCH 20/25] fix: lint issues --- cms/lib/xblock/upstream_sync.py | 8 ++++---- cms/lib/xblock/upstream_sync_block.py | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index a425e2d36a44..8d5a8253a7ed 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -88,7 +88,7 @@ class UpstreamLink: has_top_level_parent: bool # True if this Upstream link has a top-level parent @property - def _is_ready_to_sync_individually(self) -> bool: + def is_ready_to_sync_individually(self) -> bool: return bool( self.upstream_ref and self.version_available and @@ -117,7 +117,7 @@ def _check_children_ready_to_sync(self, xblock_downstream: XBlock, return_fast: if child.upstream: child_upstream_link = UpstreamLink.try_get_for_block(child) # If one child needs sync, it is not needed to check more children - if child_upstream_link._is_ready_to_sync_individually: + if child_upstream_link.is_ready_to_sync_individually: child_info.append({ 'name': child.display_name, 'id': str(child.usage_key), @@ -146,10 +146,10 @@ def ready_to_sync(self) -> bool: return False if isinstance(self.upstream_key, LibraryUsageLocatorV2): - return self._is_ready_to_sync_individually + 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 - return self._is_ready_to_sync_individually or ( + return self.is_ready_to_sync_individually or ( self.downstream_key is not None and len(self._check_children_ready_to_sync( modulestore().get_item(UsageKey.from_string(self.downstream_key)), diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index 74aabaf8dac2..0e021c39d625 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -30,7 +30,7 @@ def sync_from_upstream_block( user: User, *, top_level_parent: XBlock | None = None, - keep_custom_fields: list[str] = [], + keep_custom_fields: list[str] | None = None, ) -> XBlock | None: """ Update `downstream` with content+settings from the latest available version of its linked upstream content. @@ -51,7 +51,7 @@ def sync_from_upstream_block( if top_level_parent: try: # Currently, we don't want to sync changes if any field is modified - _verify_modification_to(downstream, keep_custom_fields) + _verify_modification_to(downstream, keep_custom_fields or []) except BadDownstream as e: logger.warning(str(e), exc_info=True) # Update upstream_* fields only @@ -124,7 +124,7 @@ def _update_customizable_fields( upstream: XBlock, downstream: XBlock, only_fetch: bool, - keep_custom_fields: list[str] = [], + keep_custom_fields: list[str] | None = None, ) -> None: """ For each customizable field: @@ -162,7 +162,7 @@ def _update_customizable_fields( # We need to update the downstream field *iff it has not been customized**. if field_name in downstream.downstream_customized: - if field_name in keep_custom_fields: + if keep_custom_fields and field_name in keep_custom_fields: continue else: # Remove the field from downstream_customized field as it can be overridden From 83d0e7aedccadf96f5a214cef880e104564e5028 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 12 Sep 2025 20:56:35 +0530 Subject: [PATCH 21/25] feat: new tests and update api to allow overriding modified fields in sync --- .../tests/test_downstream_sync_integration.py | 224 ++++++++++++++---- cms/djangoapps/contentstore/utils.py | 10 +- .../xblock_storage_handlers/view_handlers.py | 7 +- cms/lib/xblock/test/test_upstream_sync.py | 67 +++++- cms/lib/xblock/upstream_sync_block.py | 21 +- 5 files changed, 254 insertions(+), 75 deletions(-) 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 5cd58779e48f..8eba50c829a9 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 @@ -867,7 +867,182 @@ def test_unit_sync(self): self.assertEqual(data["count"], 4) self.assertListEqual(data["results"], expected_downstreams) - def test_html_sync_and_copy_paste(self): + def test_unit_sync_with_modified_downstream(self): + """ + Test that modified children component is not overridden when syncing parent container like unit + """ + # pylint: disable=too-many-statements + + # 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 = serialize_field(get_block_key_dict( + UsageKey.from_string(downstream_unit["locator"]), + )).replace('"', '"') + 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, + 'is_modified': False, + # '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... + + """) + + children_downstream_keys = self._get_course_block_children(downstream_unit["locator"]) + downstream_html1 = children_downstream_keys[0] + assert "type@html" in downstream_html1 + downstream_problem1 = children_downstream_keys[1] + assert "type@problem" in downstream_problem1 + downstream_problem2 = children_downstream_keys[2] + assert "type@problem" in downstream_problem2 + + # 2️⃣ Now, lets modify the upstream problem 1 and upstream html 1: + self._set_library_block_olx( + self.upstream_problem1["id"], + 'multiple choice v2...' + ) + self._set_library_block_olx( + self.upstream_html1["id"], + 'updated content upstream 1' + ) + 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, + 'is_modified': False, + }) + + # 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, + 'is_modified': False, + }) + + self.assertDictContainsEntries(self._get_sync_status(downstream_html1), { + 'upstream_ref': self.upstream_html1["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, + 'is_modified': False, + }) + + # Now let's modify course html block + self._update_course_block_fields(downstream_html1, { + "data": "The new downstream data.", + }) + + # Sync and check the resulting OLX of the downstream + self._sync_downstream(downstream_unit["locator"]) + + # Notice how the customization of html block i.e. modified child block is preserved by + # not updating any of the fields except for upstream_* fields + self.assertXmlEqual(self._get_course_block_olx(downstream_unit["locator"]), f""" + + The new downstream data. + + multiple choice v2... + multi select... + + """) + + def test_modified_html_copy_paste(self): """ Test that we can sync a html from a library into a course. """ @@ -944,11 +1119,7 @@ def test_html_sync_and_copy_paste(self): self._sync_downstream(downstream_html1["locator"]) # Here's how the downstream OLX looks now, after we synced it. - # Notice: - # (1) "display_name" field is left alone as it was customized - # (2) the "data" field is updated as only display_name was modified - # (3) "upstream_display_name" is updated. - # (4) "upstream_data" is updated. + # All customizations are preserved based on the post data self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" The new upstream data. """) - # 2️⃣ Now, lets modify the upstream html AND the downstream html: - self._update_course_block_fields(downstream_html1["locator"], { - "data": "The new downstream data.", # This change will be stay + "data": "The new downstream data.", }) - self._set_library_block_olx( - self.upstream_html1["id"], - 'The new upstream data 2.' - ) - self._publish_library_block(self.upstream_html1["id"]) - - # Here's how the downstream OLX looks now, before we sync: + # Here's how the downstream OLX looks now self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" The new downstream data. """) - status = self._get_sync_status(downstream_html1["locator"]) - self.assertDictContainsEntries(status, { - 'upstream_ref': self.upstream_html1["id"], # e.g. 'lb:CL-TEST:testlib:html:html1' - 'version_available': 4, # <--- updated - 'version_synced': 3, - 'version_declined': None, - 'ready_to_sync': True, # <--- updated - 'error_message': None, - 'is_modified': True, - }) - - # 3️⃣ Now, sync and check the resulting OLX of the downstream - self._sync_downstream(downstream_html1["locator"]) - - # Notice that the update is skipped except version and upstream_* fields are updated. - self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" - The new downstream data. - """) - # Copy this modified html block self._copy_course_block(downstream_html1["locator"]) # Paste it @@ -1032,7 +1168,7 @@ def test_html_sync_and_copy_paste(self): editor="visual" upstream="{self.upstream_html1['id']}" upstream_display_name="New Text Content" - upstream_version="4" + upstream_version="3" upstream_data="The new downstream data." downstream_customized="["display_name", "data"]" >The new downstream data. diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 062c59647619..775999ea60a9 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -2385,10 +2385,7 @@ def get_xblock_render_context(request, block): return "" -def _create_or_update_component_link( - created: datetime | None, - xblock, -): +def _create_or_update_component_link(created: datetime | None, xblock): """ Create or update upstream->downstream link for components in database for given xblock. """ @@ -2420,10 +2417,7 @@ def _create_or_update_component_link( ) -def _create_or_update_container_link( - created: datetime | None, - xblock, -): +def _create_or_update_container_link(created: datetime | None, xblock): """ Create or update upstream->downstream link for containers in database for given xblock. """ diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index a65667b9571f..ae7492ddbc91 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -530,7 +530,7 @@ def sync_library_content( downstream: XBlock, request, store, - top_level_parent: XBlock | None = None + top_level_parent: XBlock | None = None, ) -> StaticFileNotices: """ Handle syncing library content for given xblock depending on its upstream type. @@ -538,11 +538,16 @@ def sync_library_content( """ link = UpstreamLink.get_for_block(downstream) upstream_key = link.upstream_key + request_data = getattr(request, "json", getattr(request, "data", {})) + override_customizations = request_data.get("override_customizations", False) + keep_custom_fields = request_data.get("keep_custom_fields", []) if isinstance(upstream_key, LibraryUsageLocatorV2): lib_block = sync_from_upstream_block( downstream=downstream, user=request.user, top_level_parent=top_level_parent, + override_customizations=override_customizations, + keep_custom_fields=keep_custom_fields, ) if lib_block: static_file_notices = import_static_assets_for_library_sync(downstream, lib_block, request) diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index 2e8d68de2288..1c595d4f4f90 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -316,9 +316,9 @@ def test_sync_updates_to_downstream_only_fields(self): assert downstream.rerandomize == '"per_student"' assert downstream.matlab_api_key == 'def' assert downstream.use_latex_compiler - assert downstream.display_name == "Upstream Problem Title V3" # but "safe" customizations survive + assert downstream.display_name == "Downstream Title Override" assert downstream.attempts_before_showanswer_button == 2 assert downstream.due == datetime.datetime(2025, 2, 2, tzinfo=utc) assert downstream.force_save_button @@ -357,9 +357,8 @@ def test_sync_updates_to_modified_content(self): # Follow-up sync. Assert that updates are pulled into downstream, but customizations are saved. sync_from_upstream_block(downstream, self.user) - # All customizaions are lost - assert downstream.display_name == "Upstream Title V3" - assert downstream.data == "Upstream content V3" + assert downstream.display_name == "Downstream Title Override" # "safe" customization survives + assert downstream.data == "Downstream content override" # "safe" customization survives # Verify hidden field has latest upstream value assert downstream.upstream_data == "Upstream content V3" assert downstream.upstream_display_name == "Upstream Title V3" @@ -386,7 +385,12 @@ def test_sync_to_downstream_with_subtle_customization(self): assert downstream.downstream_customized == ["display_name"] # Syncing should retain the customization if we allow display name customization. - sync_from_upstream_block(downstream, self.user, keep_custom_fields=["display_name"]) + sync_from_upstream_block( + downstream, + self.user, + override_customizations=True, + keep_custom_fields=["display_name"] + ) assert downstream.upstream_version == 2 assert downstream.upstream_display_name == "Upstream Title V2" assert downstream.display_name == "Title V3" @@ -398,7 +402,12 @@ def test_sync_to_downstream_with_subtle_customization(self): libs.publish_changes(self.library.key, self.user.id) # ...which is reflected when we sync. - sync_from_upstream_block(downstream, self.user, keep_custom_fields=["display_name"]) + sync_from_upstream_block( + downstream, + self.user, + override_customizations=True, + keep_custom_fields=["display_name"] + ) assert downstream.upstream_version == 3 assert downstream.upstream_display_name == downstream.display_name == "Title V3" @@ -410,7 +419,12 @@ def test_sync_to_downstream_with_subtle_customization(self): libs.publish_changes(self.library.key, self.user.id) # ...then the downstream title should remain put. - sync_from_upstream_block(downstream, self.user, keep_custom_fields=["display_name"]) + sync_from_upstream_block( + downstream, + self.user, + override_customizations=True, + keep_custom_fields=["display_name"] + ) assert downstream.upstream_version == 4 assert downstream.upstream_display_name == "Title V4" assert downstream.display_name == "Title V3" @@ -419,9 +433,16 @@ def test_sync_to_downstream_with_subtle_customization(self): upstream.display_name = "Title V5" upstream.save() libs.publish_changes(self.library.key, self.user.id) - sync_from_upstream_block(downstream, self.user, keep_custom_fields=[]) # No customizations! + sync_from_upstream_block( + downstream, + self.user, + override_customizations=True, + keep_custom_fields=[] + ) # No customizations! assert downstream.upstream_version == 5 assert downstream.upstream_display_name == downstream.display_name == "Title V5" + # Clears downstream_customized field as well + assert downstream.downstream_customized == [] @ddt.data(None, "Title From Some Other Upstream Version") def test_update_customizable_fields(self, initial_upstream_display_name): @@ -593,3 +614,33 @@ def test_sync_video_block(self): # `edx_video_id` doesn't change assert downstream.edx_video_id == "test_video_id" + + def test_sync_keep_customizaton_option(self): + """ + Test that when an upstream block has a customized downstream block, we keep + the customized options when syncing based on keep_custom_fields option. + """ + # Start with an uncustomized downstream block. + downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) + sync_from_upstream_block(downstream, self.user) + assert downstream.downstream_customized == [] + assert downstream.display_name == downstream.upstream_display_name == "Upstream Title V2" + + # Then, customize our downstream title and content + downstream.display_name = "Title V3" + downstream.data = "Some content" + save_xblock_with_callback(downstream, self.user) + assert downstream.downstream_customized == ["display_name", "data"] + + # Now, sync the upstream block with `keep_custom_fields=["display_name"] only`. + # And let data be overridden + sync_from_upstream_block( + downstream, + self.user, + override_customizations=True, + keep_custom_fields=['display_name'] + ) + assert downstream.display_name == "Title V3" + # data is overridden + assert downstream.data == "Upstream content V2" + assert downstream.downstream_customized == ["display_name"] diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index 0e021c39d625..4ac7f11bd188 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -30,6 +30,7 @@ def sync_from_upstream_block( user: User, *, top_level_parent: XBlock | None = None, + override_customizations: bool = False, keep_custom_fields: list[str] | None = None, ) -> XBlock | None: """ @@ -47,24 +48,13 @@ def sync_from_upstream_block( if not isinstance(link.upstream_key, LibraryUsageLocatorV2): raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") upstream = _load_upstream_block(downstream, user) - # Skip sync only if component is being updated as part of a parent container and the component is modified - if top_level_parent: - try: - # Currently, we don't want to sync changes if any field is modified - _verify_modification_to(downstream, keep_custom_fields or []) - except BadDownstream as e: - logger.warning(str(e), exc_info=True) - # Update upstream_* fields only - _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) - # Update version to avoid showing this in updates available list. - downstream.upstream_version = link.version_available - return None # Upstream is a library block: # Sync all fields from the upstream block and override customizations _update_customizable_fields( upstream=upstream, downstream=downstream, only_fetch=False, + override_customizations=override_customizations, keep_custom_fields=keep_custom_fields, ) _update_non_customizable_fields(upstream=upstream, downstream=downstream) @@ -124,13 +114,16 @@ def _update_customizable_fields( upstream: XBlock, downstream: XBlock, only_fetch: bool, + override_customizations: bool = False, keep_custom_fields: list[str] | None = None, ) -> None: """ For each customizable field: * Save the upstream value to a hidden field on the downstream ("FETCH"). - * If `not only_fetch`, and if the field *isn't* customized on the downstream, then: + * If `not only_fetch`, and if the field *isn't* customized on the downstream + or if override_customizations=True and keep_custom_fields does not contain the field name, then: * Update it the downstream field's value from the upstream field ("SYNC"). + * Remove the field from downstream.downstream_customized field if exists. Concrete example: Imagine `lib_problem` is our upstream and `course_problem` is our downstream. @@ -162,7 +155,7 @@ def _update_customizable_fields( # We need to update the downstream field *iff it has not been customized**. if field_name in downstream.downstream_customized: - if keep_custom_fields and field_name in keep_custom_fields: + if not override_customizations or keep_custom_fields and field_name in keep_custom_fields: continue else: # Remove the field from downstream_customized field as it can be overridden From f20b0befcd88c3e9d830d9d872322112645439dc Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 15 Sep 2025 11:10:57 +0530 Subject: [PATCH 22/25] test: api changes --- .../v2/views/tests/test_downstreams.py | 23 +++++++++++++++++++ cms/lib/xblock/upstream_sync.py | 1 + 2 files changed, 24 insertions(+) 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 806fcb5751a7..bab601e9c092 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 @@ -626,6 +626,29 @@ def call_api( data["use_top_level_parents"] = str(use_top_level_parents) return self.client.get("/api/contentstore/v2/downstreams/", data=data) + def test_200_single_upstream_container(self): + """ + Test single upstream container link provides children info as well. + """ + self.client.login(username="superuser", password="password") + # Publish components + self._set_library_block_olx(self.html_lib_id_2, "Hello world!") + self._publish_library_block(self.html_lib_id_2) + + response = self.client.get(f"/api/contentstore/v2/downstreams/{self.top_level_downstream_unit.usage_key}") + assert response.status_code == 200 + data = response.json() + assert data['upstream_ref'] == self.top_level_unit_id + assert data['error_message'] is None + assert data['ready_to_sync'] is True + assert len(data['ready_to_sync_children']) == 1 + html_block = modulestore().get_item(self.top_level_downstream_html_key) + self.assertDictEqual(data['ready_to_sync_children'][0], { + 'name': html_block.display_name, + 'upstream': str(self.html_lib_id_2), + 'id': str(html_block.usage_key), + }) + def test_200_all_downstreams_for_a_course(self): """ Returns all links for given course diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 8d5a8253a7ed..9ceec37d2f1e 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -120,6 +120,7 @@ def _check_children_ready_to_sync(self, xblock_downstream: XBlock, return_fast: if child_upstream_link.is_ready_to_sync_individually: child_info.append({ 'name': child.display_name, + 'upstream': getattr(child, 'upstream', None), 'id': str(child.usage_key), }) if return_fast: From 81b08817cb87b68a63e82f0d9558d7e4f136fee1 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 15 Sep 2025 11:41:30 +0530 Subject: [PATCH 23/25] refactor: edit options should be visible for individual imports --- .../sass/course-unit-mfe-iframe-bundle.scss | 1 - cms/templates/studio_xblock_wrapper.html | 140 +++++++++--------- 2 files changed, 69 insertions(+), 72 deletions(-) diff --git a/cms/static/sass/course-unit-mfe-iframe-bundle.scss b/cms/static/sass/course-unit-mfe-iframe-bundle.scss index 3f46bf65ec24..b53210484a04 100644 --- a/cms/static/sass/course-unit-mfe-iframe-bundle.scss +++ b/cms/static/sass/course-unit-mfe-iframe-bundle.scss @@ -579,7 +579,6 @@ input.xblock-inline-title-editor { @extend %button-styles; position: relative; - top: -7px; .fa-pencil { display: none; diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index b4d69318b812..2c57872e10d3 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -148,87 +148,85 @@ % endif % if not is_root: - % if can_edit: - % if upstream_info.ready_to_sync: -
    • - -
      • - % endif - % if use_tagging: -
    • - % endif - % if not show_inline: -
    • - -
      • - % endif -
    • - -
      -
      -
      +
      +
      • + % if is_reorderable and can_move: +
    • +
      • - % if is_reorderable and can_move: -
    • - -
      • - % endif % endif % endif
    From c8dc6f85836c6e6e6acf2ed894d3c10cf8f1498f Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 15 Sep 2025 11:50:23 +0530 Subject: [PATCH 24/25] docs: update api docs --- .../contentstore/rest_api/v2/views/downstreams.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 9cdfc9706eee..7cfd095ae98e 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -499,6 +499,12 @@ class SyncFromUpstreamView(DeveloperErrorViewMixin, APIView): def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ Pull latest updates to the block at {usage_key_string} from its linked upstream content. + Optionally accepts json data in below format to control override of locally customized fields + { + "override_customizations": True or False (default: False), + "keep_custom_fields": [] (If override_customizations is True, this key can be used to preserve + some fields from override). + } """ downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) try: From 55e6a02da84d4cb51ad76487a057913c35591c85 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 17 Sep 2025 12:45:41 +0530 Subject: [PATCH 25/25] chore: remove old comments --- cms/djangoapps/contentstore/helpers.py | 4 ++-- cms/lib/xblock/test/test_upstream_sync.py | 3 --- cms/lib/xblock/upstream_sync.py | 16 ---------------- 3 files changed, 2 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index b80ddb0c7d0d..2bdbabc7df8c 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -495,11 +495,11 @@ def _fetch_and_set_upstream_link( if isinstance(upstream_link.upstream_key, UsageKey): # only if upstream is a block, not a container fetch_customizable_fields_from_block(downstream=temp_xblock, user=user, upstream=temp_xblock) # Although the above function will set all customisable fields to match its upstream_* counterpart - # We copy the downstream_customized list to the new block to avoid overriding user customaisations on sync + # We copy the downstream_customized list to the new block to avoid overriding user customisations on sync # So we will have: # temp_xblock.display_name == temp_xblock.upstream_display_name # temp_xblock.data == temp_xblock.upstream_data # for html blocks - # Even then we want to set `downstream_customized` value to avoid overriding user customaisations on sync + # Even then we want to set `downstream_customized` value to avoid overriding user customisations on sync downstream_customized = temp_xblock.xml_attributes.get("downstream_customized", '[]') temp_xblock.downstream_customized = json.loads(downstream_customized) diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index 1c595d4f4f90..c65c0fcb203c 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -363,9 +363,6 @@ def test_sync_updates_to_modified_content(self): assert downstream.upstream_data == "Upstream content V3" assert downstream.upstream_display_name == "Upstream Title V3" - # 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 diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 9ceec37d2f1e..5448a9b1cfbd 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -489,22 +489,6 @@ class UpstreamSyncMixin(XBlockMixin): # 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 "