diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 103c34a7b30c..2bdbabc7df8c 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -2,6 +2,7 @@ Helper methods for Studio views. """ from __future__ import annotations +import json import logging import pathlib import urllib @@ -493,6 +494,14 @@ def _fetch_and_set_upstream_link( # new values from the published upstream content. 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 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 customisations on sync + downstream_customized = temp_xblock.xml_attributes.get("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/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index d41770702909..7cfd095ae98e 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: """ @@ -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: 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 c03f3f4e336f..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 @@ -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 @@ -78,6 +79,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) @@ -173,6 +197,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/") @@ -216,6 +241,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... """) @@ -228,6 +254,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 @@ -247,6 +274,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... @@ -268,9 +296,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' @@ -279,6 +307,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/") @@ -302,6 +331,7 @@ def test_unit_sync(self): editor="visual" upstream="{self.upstream_html1['id']}" upstream_version="2" + upstream_data="This is the HTML." top_level_downstream_parent_key="{downstream_unit_block_key}" >This is the HTML. This is the HTML. @@ -580,6 +613,7 @@ def test_unit_sync(self): 'version_declined': None, 'ready_to_sync': True, 'error_message': None, + 'is_modified': False, }) # Sync and check the resulting OLX of the downstream @@ -598,6 +632,7 @@ def test_unit_sync(self): editor="visual" upstream="{self.upstream_html1['id']}" upstream_version="2" + upstream_data="This is the HTML." top_level_downstream_parent_key="{downstream_unit_block_key}" >This is the HTML. This is the HTML. @@ -831,6 +867,313 @@ def test_unit_sync(self): self.assertEqual(data["count"], 4) self.assertListEqual(data["results"], expected_downstreams) + 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. + """ + # 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 display_name: + self._update_course_block_fields(downstream_html1["locator"], { + "display_name": "New Text Content", + }) + + 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""" + This is the HTML. + """) + + 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. + # All customizations are preserved based on the post data + self.assertXmlEqual(self._get_course_block_olx(downstream_html1["locator"]), f""" + The new upstream data. + """) + + self._update_course_block_fields(downstream_html1["locator"], { + "data": "The new downstream data.", + }) + + # Here's how the downstream OLX looks now + 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 + 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. + """) + def test_unit_decline_sync(self): """ Test that we can decline sync a unit from the library into the course @@ -844,9 +1187,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 @@ -882,6 +1225,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. 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/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index c98e3d764148..4f8312fdb3f5 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') @@ -304,13 +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) - if upstream_link.error_message is None and isinstance(upstream_link.upstream_key, LibraryContainerLocator): + 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 ( + 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 @@ -336,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/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index c0ab6e0e598f..ae7492ddbc91 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -301,14 +301,11 @@ def modify_xblock(usage_key, request): ) -def _update_with_callback(xblock, user, old_metadata=None, old_content=None): +def save_xblock_with_callback(xblock, user, old_metadata=None, old_content=None): """ Updates the xblock in the modulestore. But before doing so, it calls the xblock's editor_saved callback function, and after doing so, it calls the xblock's post_editor_saved callback function. - - TODO: Remove getattrs from this function. - See https://github.com/openedx/edx-platform/issues/33715 """ if old_metadata is None: old_metadata = own_metadata(xblock) @@ -377,7 +374,7 @@ def _save_xblock( if old_parent_location: old_parent = store.get_item(old_parent_location) old_parent.children.remove(new_child) - old_parent = _update_with_callback(old_parent, user) + old_parent = save_xblock_with_callback(old_parent, user) else: # the Studio UI currently doesn't present orphaned children, so assume this is an error return JsonResponse( @@ -447,7 +444,7 @@ def _save_xblock( validate_and_update_xblock_due_date(xblock) # update the xblock and call any xblock callbacks - xblock = _update_with_callback(xblock, user, old_metadata, old_content) + xblock = save_xblock_with_callback(xblock, user, old_metadata, old_content) # for static tabs, their containing course also records their display name course = store.get_course(xblock.location.course_key) @@ -533,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. @@ -541,9 +538,21 @@ 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) - static_file_notices = import_static_assets_for_library_sync(downstream, lib_block, request) + 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) + else: + static_file_notices = StaticFileNotices() store.update_item(downstream, request.user.id) else: with store.bulk_operations(downstream.usage_key.context_key): @@ -1678,7 +1687,7 @@ def _get_release_date(xblock, user=None): if reset_to_default and user: xblock.start = DEFAULT_START_DATE - xblock = _update_with_callback(xblock, user) + xblock = save_xblock_with_callback(xblock, user) # Treat DEFAULT_START_DATE as a magic number that means the release date has not been set return ( diff --git a/cms/envs/common.py b/cms/envs/common.py index 13811d2a1321..00a2ba63b1fc 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -844,8 +844,8 @@ ResourceTemplates, 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/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index 8902d67c8cb1..c65c0fcb203c 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -17,6 +17,7 @@ sever_upstream_link, ) from cms.lib.xblock.upstream_sync_block import sync_from_upstream_block, fetch_customizable_fields_from_block +from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import save_xblock_with_callback from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as libs from openedx.core.djangoapps.content_tagging import api as tagging_api @@ -305,7 +306,7 @@ def test_sync_updates_to_downstream_only_fields(self): # Modifying synchronized fields are "unsafe" customizations downstream.rerandomize = '"onreset"' downstream.matlab_api_key = 'hij' - downstream.save() + save_xblock_with_callback(downstream, self.user) # Follow-up sync. sync_from_upstream_block(downstream, self.user) @@ -349,73 +350,96 @@ 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" # "unsafe" override - downstream.save() + # 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.upstream_display_name == "Upstream Title V3" assert downstream.display_name == "Downstream Title Override" # "safe" customization survives - assert downstream.data == "Upstream content V3" # "unsafe" override is gone - - # For the Content Libraries Relaunch Beta, we do not yet need to support this edge case. - # See "PRESERVING DOWNSTREAM CUSTOMIZATIONS and RESTORING UPSTREAM DEFAULTS" in cms/lib/xblock/upstream_sync.py. - # - # def test_sync_to_downstream_with_subtle_customization(self): - # """ - # Edge case: If our downstream customizes a field, but then the upstream is changed to match the - # customization do we still remember that the downstream field is customized? That is, - # if the upstream later changes again, do we retain the downstream customization (rather than - # following the upstream update?) - # """ - # # Start with an uncustomized downstream block. - # downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) - # sync_from_upstream_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" + 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" + + 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 if we allow display name customization. + 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" + + # 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, + override_customizations=True, + keep_custom_fields=["display_name"] + ) + 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, + 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" + + # 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, + 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): @@ -587,3 +611,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.py b/cms/lib/xblock/upstream_sync.py index 1b78b6b3668e..5448a9b1cfbd 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -26,7 +26,7 @@ from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2 from opaque_keys.edx.keys import UsageKey from xblock.exceptions import XBlockNotFoundError -from xblock.fields import Scope, String, Integer, Dict +from xblock.fields import Scope, String, Integer, Dict, List from xblock.core import XBlockMixin, XBlock from xmodule.util.keys import BlockKey @@ -84,10 +84,11 @@ 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. 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 @@ -95,6 +96,44 @@ 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, + 'upstream': getattr(child, 'upstream', None), + '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: """ @@ -108,45 +147,16 @@ 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 - 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) - - child_ready_to_sync = bool( - child_upstream_link.upstream_ref and - child_upstream_link.version_available and - child_upstream_link.version_available > (child_upstream_link.version_synced or 0) and - child_upstream_link.version_available > (child_upstream_link.version_declined or 0) - ) - - # If one child needs sync, it is not needed to check more children - if child_ready_to_sync: - return True - - if check_children_ready_to_sync(child): - # If one child needs sync, it is not needed to check more children - return True - - return False - if self.downstream_key is not None: - return check_children_ready_to_sync( - modulestore().get_item(UsageKey.from_string(self.downstream_key)) - ) + return 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 @@ -162,7 +172,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. """ @@ -171,6 +181,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 @@ -198,6 +220,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, has_top_level_parent=getattr(downstream, "top_level_downstream_parent_key", None) is not None, ) @@ -280,6 +303,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, has_top_level_parent=downstream.top_level_downstream_parent_key is not None, ) @@ -453,6 +477,27 @@ class UpstreamSyncMixin(XBlockMixin): default=None, scope=Scope.settings, hidden=True, enforce_type=True, ) + # PRESERVING DOWNSTREAM CUSTOMIZATIONS and RESTORING UPSTREAM VALUES + # + # For the full Content Libraries Relaunch, we would like to keep track of which customizable fields the user has + # actually customized. The idea is: once an author has customized a customizable field.... + # + # - future upstream syncs will NOT blow away the customization, + # - but future upstream syncs WILL fetch the upstream values and tuck them away in a hidden field, + # - and the author can can revert back to said fetched upstream value at any point. + # + # Now, whether field is "customized" (and thus "revertible") is dependent on whether they have ever edited it. + # To instrument this, we need to keep track of which customizable fields have been edited using a new XBlock field: + # `downstream_customized` + 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]: """ @@ -478,66 +523,32 @@ def get_customizable_fields(cls) -> dict[str, str | 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 - # actually customized. The idea is: once an author has customized a customizable field.... - # - # - future upstream syncs will NOT blow away the customization, - # - but future upstream syncs WILL fetch the upstream values and tuck them away in a hidden field, - # - and the author can can revert back to said fetched upstream value at any point. - # - # Now, whether field is "customized" (and thus "revertible") is dependent on whether they have ever edited it. - # To instrument this, we need to keep track of which customizable fields have been edited using a new XBlock field: - # `downstream_customized` - # - # Implementing `downstream_customized` has proven difficult, because there is no simple way to keep it up-to-date - # with the many different ways XBlock fields can change. The `.save()` and `.editor_saved()` methods are promising, - # but we need to do more due diligence to be sure that they cover all cases, including API edits, import/export, - # copy/paste, etc. We will figure this out in time for the full Content Libraries Relaunch (related ticket: - # https://github.com/openedx/frontend-app-authoring/issues/1317). But, for the Beta realease, we're going to - # implement something simpler: - # - # - We fetch upstream values for customizable fields and tuck them away in a hidden field (same as above). - # - If a customizable field DOES match the fetched upstream value, then future upstream syncs DO update it. - # - If a customizable field does NOT the fetched upstream value, then future upstream syncs DO NOT update it. - # - There is no UI option for explicitly reverting back to the fetched upstream value. - # - # For future reference, here is a partial implementation of what we are thinking for the full Content Libraries - # Relaunch:: - # - # downstream_customized = List( - # help=( - # "Names of the fields which have values set on the upstream block yet have been explicitly " - # "overridden on this downstream block. Unless explicitly cleared by the user, these customizations " - # "will persist even when updates are synced from the upstream." - # ), - # default=[], scope=Scope.settings, hidden=True, enforce_type=True, - # ) - # - # def save(self, *args, **kwargs): - # """ - # Update `downstream_customized` when a customizable field is modified. - # - # NOTE: This does not work, because save() isn't actually called in all the cases that we'd want it to be. - # """ - # super().save(*args, **kwargs) - # customizable_fields = self.get_customizable_fields() - # - # # Loop through all the fields that are potentially cutomizable. - # for field_name, restore_field_name in self.get_customizable_fields(): - # - # # If the field is already marked as customized, then move on so that we don't - # # unneccessarily query the block for its current value. - # if field_name in self.downstream_customized: - # continue - # - # # If 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) + 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..4ac7f11bd188 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -6,22 +6,33 @@ """ 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, BadUpstream +from .upstream_sync import BadDownstream, BadUpstream, UpstreamLink 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: +logger = logging.getLogger(__name__) + + +def sync_from_upstream_block( + downstream: XBlock, + user: User, + *, + top_level_parent: XBlock | None = None, + override_customizations: bool = False, + keep_custom_fields: list[str] | None = None, +) -> XBlock | None: """ Update `downstream` with content+settings from the latest available version of its linked upstream content. @@ -36,9 +47,16 @@ 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") - # Upstream is a library block: upstream = _load_upstream_block(downstream, user) - _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) + # 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) _update_tags(upstream=upstream, downstream=downstream) downstream.upstream_version = link.version_available @@ -57,6 +75,17 @@ def fetch_customizable_fields_from_block(*, downstream: XBlock, user: User, upst _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) +def _verify_modification_to(downstream: XBlock, allowed_fields: list[str]): + """ + Raise error if any field except for fields in allowed_fields is modified in course locally. + """ + 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: """ Load the upstream metadata and content for a downstream block. @@ -80,12 +109,21 @@ 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, + 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. @@ -107,7 +145,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) @@ -116,19 +153,15 @@ def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fe # Okay, now for the nuanced part... # We need to update the downstream field *iff it has not been customized**. - # Determining whether a field has been customized will differ in Beta vs Future release. - # (See "PRESERVING DOWNSTREAM CUSTOMIZATIONS" comment below for details.) - - ## FUTURE BEHAVIOR: field is "customized" iff we have noticed that the user edited it. - # if field_name in downstream.downstream_customized: - # continue - ## BETA BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. - downstream_value = getattr(downstream, field_name) - if old_upstream_value and downstream_value != old_upstream_value: - continue # Field has been customized. Don't touch it. Move on. + if field_name in downstream.downstream_customized: + 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 + 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) @@ -137,7 +170,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/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index a7aabb1096e3..831f71747da1 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', @@ -1274,6 +1275,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); // xss-lint: disable=javascript-jquery-html + $(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/static/sass/course-unit-mfe-iframe-bundle.scss b/cms/static/sass/course-unit-mfe-iframe-bundle.scss index 4100310f406b..b53210484a04 100644 --- a/cms/static/sass/course-unit-mfe-iframe-bundle.scss +++ b/cms/static/sass/course-unit-mfe-iframe-bundle.scss @@ -567,11 +567,18 @@ 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; 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 89b62cc17c29..2c57872e10d3 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -120,10 +120,14 @@ ${_("Sourced from a library - but the upstream link is broken/invalid.")} % else: - ${_("Sourced from a library - but has been modified locally.")} + % else: ${_("Sourced from a library.")} + % endif % endif % endif ${label} @@ -135,88 +139,94 @@
    + % if can_edit_title and not can_edit: +
  • + +
    • + % 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: -
  • - -
    • - % endif % endif % endif
diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 4fb7301ced6b..76611624cdc6 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -9,6 +9,7 @@ from opaque_keys.edx.locator import LibraryLocatorV2 from openedx.core.djangoapps.content_tagging.api import get_all_object_tags, TagValuesByObjectIdDict +from xmodule.xml_block import serialize_field from .data import StaticFile from . import utils @@ -140,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 @@ -166,9 +167,10 @@ def _serialize_html_block(self, block) -> etree.Element: if block.use_latex_compiler: olx_node.attrib["use_latex_compiler"] = "true" for field_name in block.fields: - if (field_name.startswith("upstream") or field_name == "top_level_downstream_parent_key") \ - and block.fields[field_name].is_set_on(block): - olx_node.attrib[field_name] = str(getattr(block, field_name)) + if ( + field_name.startswith(("upstream", "downstream")) or field_name == "top_level_downstream_parent_key" + ) and block.fields[field_name].is_set_on(block): + olx_node.attrib[field_name] = serialize_field(getattr(block, field_name)) # Escape any CDATA special chars escaped_block_data = block.data.replace("]]>", "]]>") diff --git a/xmodule/html_block.py b/xmodule/html_block.py index 782020131987..3067a7cf9b95 100644 --- a/xmodule/html_block.py +++ b/xmodule/html_block.py @@ -61,6 +61,13 @@ class HtmlBlockMixin( # lint-amnesty, pylint: disable=abstract-method default=_("Text") ) data = String(help=_("Html contents to display for this block"), default="", scope=Scope.content) + upstream_data = String( + help=_("Upstream html contents to store upstream data field"), + default=None, + hidden=True, + enforce_type=True, + scope=Scope.content, + ) source_code = String( help=_("Source code for LaTeX documents. This feature is not well-supported."), scope=Scope.settings @@ -143,6 +150,13 @@ def studio_view(self, _context): shim_xmodule_js(fragment, 'HTMLEditingDescriptor') return fragment + @classmethod + def get_customizable_fields(cls) -> dict[str, str | None]: + return { + "display_name": "upstream_display_name", + "data": "upstream_data", + } + uses_xmodule_styles_setup = True mako_template = "widgets/html-edit.html"