From e800ae7622eb6a802b555416c393634d36dfaf6c Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 20 Nov 2023 10:58:10 -0500 Subject: [PATCH] feat: provisionally support V2 libraries in LibraryContentBlock (randomized only) (#33263) Refactors and reworks the LibraryContentBlock so that its sync-from-library operations are asynchronous and work with V2 content libraries. This also required us to make library_content block duplication asynchronous, as that involves syncing from the source library. For the sake of clarity, this PR includes two major method renames: * update_children(...) -> sync_from_library(...) * refresh_library(...) -> sync_from_library(upgrade_to_latest=True, ...) an an XBlock HTTP handler rename: /refresh_children -> /upgrade_and_sync There are still a couple issues with import or duplication of library_content blocks referencing V2 libraries other than latest. These will be resolved in an upcoming PR. Part of: https://openedx.atlassian.net/wiki/spaces/COMM/pages/3820617729/Spec+Memo+Content+Library+Authoring+Experience+V2 Follow-up work: https://github.com/openedx/edx-platform/issues/33640 Co-authored-by: Connor Haugh Co-authored-by: Eugene Dyudyunov --- .../contentstore/tests/test_libraries.py | 147 ++++---- .../contentstore/views/component.py | 5 + cms/djangoapps/contentstore/views/preview.py | 6 +- .../contentstore/views/tests/test_block.py | 133 ++++++- .../views/tests/test_container_page.py | 2 +- .../views/tests/test_import_export.py | 2 +- .../xblock_storage_handlers/view_handlers.py | 81 ++--- cms/envs/common.py | 2 - cms/lib/xblock/tagging/test.py | 7 +- cms/static/js/views/pages/container.js | 142 +++++++- cms/static/sass/elements/_layout.scss | 4 + cms/static/sass/elements/_xblocks.scss | 25 +- cms/templates/container.html | 11 +- cms/templates/studio_xblock_wrapper.html | 19 +- docs/docs_settings.py | 1 - lms/envs/common.py | 3 + .../core/djangoapps/content_libraries/api.py | 94 ++++- .../commands/content_libraries_import.py | 2 +- .../djangoapps/content_libraries/tasks.py | 344 +++++++++++++++++- openedx/core/djangolib/testing/utils.py | 19 +- openedx/core/lib/__init__.py | 40 ++ openedx/core/lib/blockstore_api/methods.py | 9 +- .../completion_integration/test_services.py | 11 +- webpack.common.config.js | 12 - .../public/js/library_content_edit.js | 20 +- .../public/js/library_content_edit_helpers.js | 54 +++ xmodule/library_content_block.py | 211 +++++++---- xmodule/library_tools.py | 321 ++++++---------- .../split_mongo/caching_descriptor_system.py | 7 +- xmodule/modulestore/xml_importer.py | 18 +- xmodule/studio_editable.py | 30 ++ xmodule/tests/test_library_content.py | 236 +++++++----- xmodule/tests/test_library_tools.py | 150 +++++++- 33 files changed, 1596 insertions(+), 572 deletions(-) create mode 100644 xmodule/assets/library_content/public/js/library_content_edit_helpers.js diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index bb692c016033..e7dcc3647886 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -96,9 +96,9 @@ def _add_simple_content_block(self): user_id=self.user.id, publish_item=False ) - def _refresh_children(self, lib_content_block, status_code_expected=200): + def _upgrade_and_sync(self, lib_content_block, status_code_expected=200): """ - Helper method: Uses the REST API to call the 'refresh_children' handler + Helper method: Uses the REST API to call the 'upgrade_and_sync' handler of a LibraryContent block """ if 'user' not in lib_content_block.runtime._services: # pylint: disable=protected-access @@ -106,9 +106,9 @@ def _refresh_children(self, lib_content_block, status_code_expected=200): lib_content_block.runtime._services['user'] = user_service # pylint: disable=protected-access handler_url = reverse_usage_url( - 'component_handler', + 'preview_handler', lib_content_block.location, - kwargs={'handler': 'refresh_children'} + kwargs={'handler': 'upgrade_and_sync'} ) response = self.client.ajax_post(handler_url) self.assertEqual(response.status_code, status_code_expected) @@ -171,7 +171,7 @@ def test_max_items(self, num_to_create, num_to_select, num_expected): lc_block = self._add_library_content_block(course, self.lib_key, other_settings={'max_count': num_to_select}) self.assertEqual(len(lc_block.children), 0) - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) # Now, we want to make sure that .children has the total # of potential # children, and that get_child_blocks() returns the actual children @@ -198,7 +198,7 @@ def test_consistent_children(self): lc_block = self._add_library_content_block(course, self.lib_key, {'max_count': 1}) lc_block_key = lc_block.location - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) def get_child_of_lc_block(block): """ @@ -231,7 +231,7 @@ def check(): check() # Refresh the children: - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) # Now re-load the block and try yet again, in case refreshing the children changed anything: check() @@ -251,7 +251,7 @@ def test_definition_shared_with_library(self): # Add a LibraryContent block to the course: lc_block = self._add_library_content_block(course, self.lib_key) - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) for child_key in lc_block.children: child = modulestore().get_item(child_key) def_id = child.definition_locator.definition_id @@ -281,7 +281,7 @@ def test_fields(self): # Add a LibraryContent block to the course: lc_block = self._add_library_content_block(course, self.lib_key) - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) course_block = modulestore().get_item(lc_block.children[0]) self.assertEqual(course_block.data, data_value) @@ -317,7 +317,7 @@ def test_block_with_children(self): # Add a LibraryContent block to the course: lc_block = self._add_library_content_block(course, self.lib_key) - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) self.assertEqual(len(lc_block.children), 1) course_vert_block = modulestore().get_item(lc_block.children[0]) self.assertEqual(len(course_vert_block.children), 1) @@ -326,10 +326,11 @@ def test_block_with_children(self): self.assertEqual(course_child_block.data, data_value) self.assertEqual(course_child_block.display_name, name_value) - def test_change_after_first_sync(self): + def test_switch_to_unknown_source_library_preserves_settings(self): """ - Check that nothing goes wrong if we (A) Set up a LibraryContent block - and use it successfully, then (B) Give it an invalid configuration. + Check that nothing goes wrong if we (A) set up a LibraryContent block + and use it successfully, then (B) give it an invalid source lib, and then + (C) try to upgrade the source library version. No children should be deleted until the configuration is fixed. """ # Add a block to the library: @@ -348,31 +349,58 @@ def test_change_after_first_sync(self): # Add a LibraryContent block to the course: lc_block = self._add_library_content_block(course, self.lib_key) - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) + good_library_id = lc_block.source_library_id + good_library_version = lc_block.source_library_version + assert good_library_id + assert good_library_version self.assertEqual(len(lc_block.children), 1) + self.assertEqual(modulestore().get_item(lc_block.children[0]).data, data_value) # Now, change the block settings to have an invalid library key: + bad_library_id = "library-v1:NOT+FOUND" resp = self._update_block( lc_block.location, - {"source_library_id": "library-v1:NOT+FOUND"}, + {"source_library_id": bad_library_id}, ) self.assertEqual(resp.status_code, 200) + lc_block = modulestore().get_item(lc_block.location) - self.assertEqual(len(lc_block.children), 1) # Children should not be deleted due to a bad setting. - html_block = modulestore().get_item(lc_block.children[0]) - self.assertEqual(html_block.data, data_value) + # Source library id should be set to the new bad one... + assert lc_block.source_library_id == bad_library_id + # ...but old source library version should be preserved... + assert lc_block.source_library_version == good_library_version + # ...and children should not be deleted due to a bad setting. + self.assertEqual(len(lc_block.children), 1) + self.assertEqual(modulestore().get_item(lc_block.children[0]).data, data_value) - def test_refreshes_children_if_libraries_change(self): - """ Tests that children are automatically refreshed if libraries list changes """ + # Attempting to force an upgrade (the user would have to do this through the API, as + # the UI wouldn't give them the option) returns a 400 and preserves the LC block's state. + self._upgrade_and_sync(lc_block, status_code_expected=400) + + # (Repeat the exact same checks) + lc_block = modulestore().get_item(lc_block.location) + # Source library id should be set to the new bad one... + assert lc_block.source_library_id == bad_library_id + # ...but old source library version should be preserved... + assert lc_block.source_library_version == good_library_version + # ...and children should not be deleted due to a bad setting. + self.assertEqual(len(lc_block.children), 1) + self.assertEqual(modulestore().get_item(lc_block.children[0]).data, data_value) + + def test_sync_if_source_library_changed(self): + """ + Tests that children are automatically synced with new lib if source library id is changed. + """ library2key = self._create_library("org2", "lib2", "Library2") library2 = modulestore().get_library(library2key) - data1, data2 = "Hello world!", "Hello other world!" + data1, data2 = "Hello world from lib 1!", "Hello other world from lib 2" BlockFactory.create( category="html", parent_location=self.library.location, user_id=self.user.id, publish_item=False, - display_name="Lib1: HTML BLock", + display_name="Lib 1: HTML BLock", data=data1, ) @@ -389,25 +417,34 @@ def test_refreshes_children_if_libraries_change(self): with modulestore().default_store(ModuleStoreEnum.Type.split): course = CourseFactory.create() - # Add a LibraryContent block to the course: + # Add a LibraryContent block to the course. lc_block = self._add_library_content_block(course, self.lib_key) - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) + + # Sanity check the initial condition. self.assertEqual(len(lc_block.children), 1) + html_block_1 = modulestore().get_item(lc_block.children[0]) + self.assertEqual(html_block_1.data, data1) - # Now, change the block settings to have an invalid library key: + # Now, switch over to new library. Don't call upgrade_and_sync, because we are + # testing that it happens automatically. resp = self._update_block( lc_block.location, {"source_library_id": str(library2key)}, ) self.assertEqual(resp.status_code, 200) - lc_block = modulestore().get_item(lc_block.location) + # Check that the course now has the new lib's new block. + lc_block = modulestore().get_item(lc_block.location) self.assertEqual(len(lc_block.children), 1) - html_block = modulestore().get_item(lc_block.children[0]) - self.assertEqual(html_block.data, data2) + html_block_2 = modulestore().get_item(lc_block.children[0]) + self.assertEqual(html_block_2.data, data2) - @patch("xmodule.library_tools.SearchEngine.get_search_engine", Mock(return_value=None, autospec=True)) - def test_refreshes_children_if_capa_type_change(self): + @patch( + 'openedx.core.djangoapps.content_libraries.tasks.SearchEngine.get_search_engine', + Mock(return_value=None, autospec=True), + ) + def test_sync_if_capa_type_changed(self): """ Tests that children are automatically refreshed if capa type field changes """ name1, name2 = "Option Problem", "Multiple Choice Problem" BlockFactory.create( @@ -433,7 +470,7 @@ def test_refreshes_children_if_capa_type_change(self): # Add a LibraryContent block to the course: lc_block = self._add_library_content_block(course, self.lib_key) - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) self.assertEqual(len(lc_block.children), 2) resp = self._update_block( @@ -458,26 +495,6 @@ def test_refreshes_children_if_capa_type_change(self): html_block = modulestore().get_item(lc_block.children[0]) self.assertEqual(html_block.display_name, name2) - def test_refresh_fails_for_unknown_library(self): - """ Tests that refresh children fails if unknown library is configured """ - # Create a course: - with modulestore().default_store(ModuleStoreEnum.Type.split): - course = CourseFactory.create() - - # Add a LibraryContent block to the course: - lc_block = self._add_library_content_block(course, self.lib_key) - lc_block = self._refresh_children(lc_block) - self.assertEqual(len(lc_block.children), 0) - - # Now, change the block settings to have an invalid library key: - resp = self._update_block( - lc_block.location, - {"source_library_id": "library-v1:NOT+FOUND"}, - ) - self.assertEqual(resp.status_code, 200) - with self.assertRaises(ValueError): - self._refresh_children(lc_block, status_code_expected=400) - def test_library_filters(self): """ Test the filters in the list libraries API @@ -742,9 +759,9 @@ def test_duplicate_across_courses(self, library_role, course_role, expected_resu (LibraryUserRole, None, False), ) @ddt.unpack - def test_refresh_library_content_permissions(self, library_role, course_role, expected_result): + def test_upgrade_and_sync_handler_content_permissions(self, library_role, course_role, expected_result): """ - Test that the LibraryContent block's 'refresh_children' handler will correctly + Test that the LibraryContent block's 'upgrade_library' handler will correctly handle permissions and allow/refuse when updating its content with the latest version of a library. We try updating from a library with (write, read, or no) access to a course with (write or no) access. @@ -767,7 +784,7 @@ def test_refresh_library_content_permissions(self, library_role, course_role, ex lc_block = self._add_library_content_block(course, self.lib_key) # We must use the CMS's module system in order to get permissions checks. self._bind_block(lc_block, user=self.non_staff_user) - lc_block = self._refresh_children(lc_block, status_code_expected=200 if expected_result else 403) + lc_block = self._upgrade_and_sync(lc_block, status_code_expected=200 if expected_result else 403) self.assertEqual(len(lc_block.children), 1 if expected_result else 0) def test_studio_user_permissions(self): @@ -860,7 +877,7 @@ def setUp(self): # Add a LibraryContent block to the course: self.lc_block = self._add_library_content_block(self.course, self.lib_key) - self.lc_block = self._refresh_children(self.lc_block) + self.lc_block = self._upgrade_and_sync(self.lc_block) self.problem_in_course = modulestore().get_item(self.lc_block.children[0]) def test_overrides(self): @@ -875,7 +892,7 @@ def test_overrides(self): # Add a second LibraryContent block to the course, with no override: lc_block2 = self._add_library_content_block(self.course, self.lib_key) - lc_block2 = self._refresh_children(lc_block2) + lc_block2 = self._upgrade_and_sync(lc_block2) # Re-load the two problem blocks - one with and one without an override: self.problem_in_course = modulestore().get_item(self.lc_block.children[0]) problem2_in_course = modulestore().get_item(lc_block2.children[0]) @@ -925,7 +942,7 @@ def test_consistent_definitions(self): self.problem.weight = 20 self.problem.display_name = "NEW" modulestore().update_item(self.problem, self.user.id) - self.lc_block = self._refresh_children(self.lc_block) + self.lc_block = self._upgrade_and_sync(self.lc_block) self.problem_in_course = modulestore().get_item(self.problem_in_course.location) self.assertEqual(self.problem.definition_locator.definition_id, definition_id) @@ -962,7 +979,7 @@ def test_persistent_overrides(self, duplicate): self.problem.data = new_data_value modulestore().update_item(self.problem, self.user.id) - self.lc_block = self._refresh_children(self.lc_block) + self.lc_block = self._upgrade_and_sync(self.lc_block) self.problem_in_course = modulestore().get_item(self.problem_in_course.location) self.assertEqual(self.problem_in_course.display_name, new_display_name) @@ -992,14 +1009,11 @@ def test_duplicated_version(self): # Refresh our reference to the library self.library = store.get_library(self.lib_key) - # Refresh our reference to the block - self.lc_block = store.get_item(self.lc_block.location) - self.problem_in_course = store.get_item(self.problem_in_course.location) - # The library has changed... self.assertEqual(len(self.library.children), 2) # But the block hasn't. + self.lc_block = store.get_item(self.lc_block.location) self.assertEqual(len(self.lc_block.children), 1) self.assertEqual(self.problem_in_course.location, self.lc_block.children[0]) self.assertEqual(self.problem_in_course.display_name, self.original_display_name) @@ -1009,8 +1023,15 @@ def test_duplicated_version(self): duplicate_block(self.course.location, self.lc_block.location, self.user) ) # The duplicate should have identical children to the original: - self.assertEqual(len(duplicate.children), 1) self.assertTrue(self.lc_block.source_library_version) self.assertEqual(self.lc_block.source_library_version, duplicate.source_library_version) + self.assertEqual(len(duplicate.children), 1) problem2_in_course = store.get_item(duplicate.children[0]) self.assertEqual(problem2_in_course.display_name, self.original_display_name) + + # Refresh our reference to the block + self.lc_block = self._upgrade_and_sync(self.lc_block) + self.problem_in_course = store.get_item(self.problem_in_course.location) + + # and the block has changed too. + self.assertEqual(len(self.lc_block.children), 2) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 3c31637d4c78..b815a14ed171 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -195,6 +195,9 @@ def container_handler(request, usage_key_string): # Get the status of the user's clipboard so they can paste components if they have something to paste user_clipboard = content_staging_api.get_user_clipboard_json(request.user.id, request) + library_block_types = [problem_type['component'] for problem_type in LIBRARY_BLOCK_TYPES] + is_library_xblock = xblock.location.block_type in library_block_types + return render_to_response('container.html', { 'language_code': request.LANGUAGE_CODE, 'context_course': course, # Needed only for display of menus at top of page. @@ -203,6 +206,7 @@ def container_handler(request, usage_key_string): 'xblock_locator': xblock.location, 'unit': unit, 'is_unit_page': is_unit_page, + 'is_collapsible': is_library_xblock, 'subsection': subsection, 'section': section, 'position': index, @@ -218,6 +222,7 @@ def container_handler(request, usage_key_string): 'templates': CONTAINER_TEMPLATES, # Status of the user's clipboard, exactly as would be returned from the "GET clipboard" REST API. 'user_clipboard': user_clipboard, + 'is_fullwidth_content': is_library_xblock, }) else: return HttpResponseBadRequest("Only supports HTML requests") diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 48c323c1aa88..acab35471813 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -45,7 +45,7 @@ wrap_xblock_aside ) -from ..utils import get_visibility_partition_info +from ..utils import get_visibility_partition_info, StudioPermissionsService from .access import get_user_role from .session_kv_store import SessionKeyValueStore @@ -198,6 +198,7 @@ def _prepare_runtime_for_preview(request, block): deprecated_anonymous_user_id = anonymous_id_for_user(request.user, None) services = { + "studio_user_permissions": StudioPermissionsService(request.user), "i18n": XBlockI18nService, 'mako': mako_service, "settings": SettingsService(), @@ -310,6 +311,9 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'is_reorderable': is_reorderable, 'can_edit': can_edit, 'can_edit_visibility': context.get('can_edit_visibility', is_course), + 'is_loading': context.get('is_loading', False), + 'is_selected': context.get('is_selected', False), + 'selectable': context.get('selectable', False), 'selected_groups_label': selected_groups_label, 'can_add': context.get('can_add', True), 'can_move': context.get('can_move', is_course), diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 26b3f91a0bd7..a90187ef605d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -211,7 +211,7 @@ def test_get_empty_container_fragment(self): self.assertNotRegex(html, r"wrapper-xblock[^-]+") # Verify that the header and article tags are still added - self.assertIn('
', html) + self.assertIn('
', html) self.assertIn('
', html) def test_get_container_fragment(self): @@ -233,7 +233,7 @@ def test_get_container_fragment(self): # Verify that the Studio nesting wrapper has been added self.assertIn("level-nesting", html) - self.assertIn('
', html) + self.assertIn('
', html) self.assertIn('
', html) # Verify that the Studio element wrapper has been added @@ -950,6 +950,135 @@ def test_shallow_duplicate(self): self.assertEqual(len(destination_chapter.get_children()), 0) self.assertEqual(destination_chapter.display_name, "Source Chapter") + def test_duplicate_library_content_block(self): # pylint: disable=too-many-statements + """ + Test the LibraryContentBlock's special duplication process. + """ + store = modulestore() + + # Create a library with two blocks (HTML 1 and HTML 2). + # These are the "Original" version of the blocks. + lib = LibraryFactory() + BlockFactory( + parent=lib, + category="html", + display_name="HTML 1 Title (Original)", + data="HTML 1 Content (Original)", + publish_item=False, + ) + BlockFactory( + parent=lib, + category="html", + display_name="HTML 2 Title (Original)", + data="HTML 2 Content (Original)", + publish_item=False, + ) + original_lib_version = store.get_library( + lib.location.library_key, remove_version=False, remove_branch=False, + ).location.library_key.version_guid + assert original_lib_version is not None + + # Create a library content block (lc), point it out our library, and sync it. + unit = BlockFactory( + parent_location=self.seq_usage_key, + category="vertical", + display_name="Parent Unit of LC and its Dupe", + publish_item=False, + ) + lc = BlockFactory( + parent=unit, + category="library_content", + source_library_id=str(lib.location.library_key), + display_name="LC Block", + max_count=1, + publish_item=False, + ) + lc.sync_from_library() + lc = store.get_item(lc.location) # we must reload because sync_from_library happens out-of-thread + assert lc.source_library_version == str(original_lib_version) + lc_html_1 = store.get_item(lc.children[0]) + lc_html_2 = store.get_item(lc.children[1]) + assert lc_html_1.display_name == "HTML 1 Title (Original)" + assert lc_html_2.display_name == "HTML 2 Title (Original)" + assert lc_html_1.data == "HTML 1 Content (Original)" + assert lc_html_2.data == "HTML 2 Content (Original)" + + # Override the title and data of HTML 1 under lc ("Course Override"). + # Note that title is settings-scoped and data is content-scoped. + lc_html_1.display_name = "HTML 1 Title (Course Override)" + lc_html_1.data = "HTML 1 Content (Course Override)" + store.update_item(lc_html_1, self.user.id) + + # Now, update the titles and contents of both HTML 1 and HTML 2 ("Lib Update"). + # This will yield a new version of the library (updated_lib_version). + lib_html_1 = store.get_item(lib.children[0]) + lib_html_2 = store.get_item(lib.children[1]) + assert lib_html_1.display_name == "HTML 1 Title (Original)" + assert lib_html_2.display_name == "HTML 2 Title (Original)" + lib_html_1.display_name = "HTML 1 Title (Lib Update)" + lib_html_2.display_name = "HTML 2 Title (Lib Update)" + lib_html_1.data = "HTML 1 Content (Lib Update)" + lib_html_2.data = "HTML 2 Content (Lib Update)" + store.update_item(lib_html_1, self.user.id) + store.update_item(lib_html_2, self.user.id) + updated_lib_version = store.get_library( + lib.location.library_key, remove_version=False, remove_branch=False, + ).location.library_key.version_guid + assert updated_lib_version is not None + assert updated_lib_version != original_lib_version + + # DUPLICATE lc. + # Unit should now contain both lc and dupe. + # All settings should match between lc and dupe. + dupe = store.get_item( + self._duplicate_item( + parent_usage_key=unit.location, + source_usage_key=lc.location, + display_name="Dupe LC Block", + ) + ) + lc = store.get_item(lc.location) + unit = store.get_item(unit.location) + assert unit.children == [lc.location, dupe.location] + assert len(lc.children) == len(dupe.children) == 2 + assert lc.max_count == dupe.max_count == 1 + assert lc.source_library_id == dupe.source_library_id == str(lib.location.library_key) + assert lc.source_library_version == dupe.source_library_version == str(original_lib_version) + + # The lc block's children should remain unchanged. + # That is: HTML 1 has overrides, HTML 2 has originals. + lc_html_1 = store.get_item(lc.children[0]) + assert lc_html_1.display_name == "HTML 1 Title (Course Override)" + assert lc_html_1.data == "HTML 1 Content (Course Override)" + lc_html_2 = store.get_item(lc.children[1]) + assert lc_html_2.display_name == "HTML 2 Title (Original)" + assert lc_html_2.data == "HTML 2 Content (Original)" + + # Now, the dupe's children should copy *settings* overrides over from the lc block, + # but we don't actually expect it to copy *content* overrides over from the lc block. + # (Yes, this is weird. One would expect it to copy all fields from the lc block, whether settings or content. + # But that's the existing behavior, so we're going to test for it, for now at least. + # We may change this in the future: https://github.com/openedx/edx-platform/issues/33739) + dupe_html_1 = store.get_item(dupe.children[0]) + assert dupe_html_1.display_name == "HTML 1 Title (Course Override)" # <- as you'd expect + assert dupe_html_1.data == "HTML 1 Content (Original)" # <- weird! + dupe_html_2 = store.get_item(dupe.children[1]) + assert dupe_html_2.display_name == "HTML 2 Title (Original)" # <- as you'd expect + assert dupe_html_2.data == "HTML 2 Content (Original)" # <- as you'd expect + + # Finally, upgrade the dupe's library version, and make sure it pulls in updated library block *content*, + # whilst preserving *settings overrides* (specifically, HTML 1's title override). + dupe.sync_from_library(upgrade_to_latest=True) + dupe = store.get_item(dupe.location) + assert dupe.source_library_version == str(updated_lib_version) + assert len(dupe.children) == 2 + dupe_html_1 = store.get_item(dupe.children[0]) + dupe_html_2 = store.get_item(dupe.children[1]) + assert dupe_html_1.display_name == "HTML 1 Title (Course Override)" + assert dupe_html_1.data == "HTML 1 Content (Lib Update)" + assert dupe_html_2.display_name == "HTML 2 Title (Lib Update)" + assert dupe_html_2.data == "HTML 2 Content (Lib Update)" + @ddt.ddt class TestMoveItem(ItemTest): diff --git a/cms/djangoapps/contentstore/views/tests/test_container_page.py b/cms/djangoapps/contentstore/views/tests/test_container_page.py index b78c0cce8347..1d5b52905357 100644 --- a/cms/djangoapps/contentstore/views/tests/test_container_page.py +++ b/cms/djangoapps/contentstore/views/tests/test_container_page.py @@ -167,7 +167,7 @@ def test_library_content_preview_html(self): self.assertEqual(len(lc_block.children), 0) # Refresh children to be reflected in lc_block - lc_block = self._refresh_children(lc_block) + lc_block = self._upgrade_and_sync(lc_block) self.assertEqual(len(lc_block.children), 1) self.validate_preview_html( diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index d01f6800c105..0744c436cf99 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -1112,7 +1112,7 @@ def _setup_source_course_with_library_content(self, publish=False, version=None) lc_block = self._add_library_content_block( vertical, self.lib_key, publish_item=publish, other_settings=dict(source_library_version=version) ) - self._refresh_children(lc_block) + lc_block.sync_from_library(upgrade_to_latest=True) def get_lib_content_block_children(self, block_location): """ diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index dca25695a29d..37dbee4507a9 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -8,7 +8,6 @@ contentstore/views/block.py to this file, because the logic is reused in another view now. Along with it, we moved the business logic of the other views in that file, since that is related. """ - import logging from datetime import datetime from uuid import uuid4 @@ -16,7 +15,7 @@ from attrs import asdict from django.conf import settings from django.contrib.auth.decorators import login_required -from django.contrib.auth.models import (User) # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth.models import User # pylint: disable=imported-auth-user from django.core.exceptions import PermissionDenied from django.http import HttpResponse, HttpResponseBadRequest from django.utils.timezone import timezone @@ -57,37 +56,15 @@ from openedx.core.lib.gating import api as gating_api from openedx.core.lib.cache_utils import request_cached from openedx.core.toggles import ENTRANCE_EXAMS -from xmodule.course_block import ( - DEFAULT_START_DATE, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.library_tools import ( - LibraryToolsService, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore import ( - EdxJSONEncoder, - ModuleStoreEnum, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.django import ( - modulestore, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.draft_and_published import ( - DIRECT_ONLY_CATEGORIES, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.exceptions import ( - InvalidLocationError, - ItemNotFoundError, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.inheritance import ( - own_metadata, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.services import ( - ConfigurationService, - SettingsService, - TeamsConfigurationService, -) # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.tabs import ( - CourseTabList, -) # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.course_block import DEFAULT_START_DATE +from xmodule.library_tools import LibraryToolsService +from xmodule.modulestore import EdxJSONEncoder, ModuleStoreEnum +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES +from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError +from xmodule.modulestore.inheritance import own_metadata +from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService +from xmodule.tabs import CourseTabList from ..utils import ( ancestor_has_staff_lock, @@ -180,6 +157,7 @@ def handle_xblock(request, usage_key_string=None): the public CMS API. """ if usage_key_string: + usage_key = usage_key_with_run(usage_key_string) access_check = ( @@ -220,7 +198,8 @@ def handle_xblock(request, usage_key_string=None): _delete_item(usage_key, request.user) return JsonResponse() else: # Since we have a usage_key, we are updating an existing xblock. - return modify_xblock(usage_key, request) + modified_xblock = modify_xblock(usage_key, request) + return modified_xblock elif request.method in ("PUT", "POST"): if "duplicate_source_locator" in request.json: @@ -228,7 +207,6 @@ def handle_xblock(request, usage_key_string=None): duplicate_source_usage_key = usage_key_with_run( request.json["duplicate_source_locator"] ) - source_course = duplicate_source_usage_key.course_key dest_course = parent_usage_key.course_key if not has_studio_write_access( @@ -255,6 +233,7 @@ def handle_xblock(request, usage_key_string=None): request.user, request.json.get("display_name"), ) + return JsonResponse( { "locator": str(dest_usage_key), @@ -298,7 +277,6 @@ def handle_xblock(request, usage_key_string=None): def modify_xblock(usage_key, request): request_data = request.json - print(f'In modify_xblock with data = {request_data.get("data")}, fields = {request_data.get("fields")}') return _save_xblock( request.user, get_xblock(usage_key, request.user), @@ -360,21 +338,27 @@ def load_services_for_studio(runtime, user): def _update_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. + 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 callable(getattr(xblock, "editor_saved", None)): - if old_metadata is None: - old_metadata = own_metadata(xblock) - if old_content is None: - old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) + if old_metadata is None: + old_metadata = own_metadata(xblock) + if old_content is None: + old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) + if hasattr(xblock, "editor_saved"): load_services_for_studio(xblock.runtime, user) xblock.editor_saved(user, old_metadata, old_content) - - # Update after the callback so any changes made in the callback will get persisted. - return modulestore().update_item(xblock, user.id) + xblock_updated = modulestore().update_item(xblock, user.id) + if hasattr(xblock_updated, "post_editor_saved"): + load_services_for_studio(xblock_updated.runtime, user) + xblock_updated.post_editor_saved(user, old_metadata, old_content) + return xblock_updated -def _save_xblock( # lint-amnesty, pylint: disable=too-many-statements +def _save_xblock( user, xblock, data=None, @@ -389,12 +373,11 @@ def _save_xblock( # lint-amnesty, pylint: disable=too-many-statements publish=None, fields=None, summary_configuration_enabled=None, -): +): # lint-amnesty, pylint: disable=too-many-statements """ Saves xblock w/ its fields. Has special processing for grader_type, publish, and nullout and Nones in metadata. nullout means to truly set the field to None whereas nones in metadata mean to unset them (so they revert to default). - """ store = modulestore() # Perform all xblock changes within a (single-versioned) transaction @@ -881,6 +864,8 @@ def _duplicate_block( # Allow an XBlock to do anything fancy it may need to when duplicated from another block. # These blocks may handle their own children or parenting if needed. Let them return booleans to # let us know if we need to handle these or not. + # TODO: Make this a proper method in the base class so we don't need getattr. + # See https://github.com/openedx/edx-platform/issues/33715 load_services_for_studio(dest_block.runtime, user) children_handled = dest_block.studio_post_duplicate(store, source_item) diff --git a/cms/envs/common.py b/cms/envs/common.py index a007424cf18f..11afd870a016 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1994,8 +1994,6 @@ ] LIBRARY_BLOCK_TYPES = [ - # Per https://github.com/openedx/build-test-release-wg/issues/231 - # we removed the library source content block from defaults until complete. { 'component': 'library_content', 'boilerplate_name': None diff --git a/cms/lib/xblock/tagging/test.py b/cms/lib/xblock/tagging/test.py index f00d1a53c89b..173523452d54 100644 --- a/cms/lib/xblock/tagging/test.py +++ b/cms/lib/xblock/tagging/test.py @@ -148,9 +148,12 @@ def test_preview_html(self): tree = etree.parse(StringIO(problem_html), parser) main_div_nodes = tree.xpath('/html/body/div/section/div') - self.assertEqual(len(main_div_nodes), 1) + self.assertEqual(len(main_div_nodes), 2) - div_node = main_div_nodes[0] + loader_div_node = main_div_nodes[0] + self.assertIn('ui-loading', loader_div_node.get('class')) + + div_node = main_div_nodes[1] self.assertEqual(div_node.get('data-init'), 'StructuredTagsInit') self.assertEqual(div_node.get('data-runtime-class'), 'PreviewRuntime') self.assertEqual(div_node.get('data-block-type'), 'tagging_aside') diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 8f296ebb6b58..8495bfd27180 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -7,11 +7,14 @@ define(['jquery', 'underscore', 'backbone', 'gettext', 'js/views/pages/base_page 'js/views/components/add_xblock', 'js/views/modals/edit_xblock', 'js/views/modals/move_xblock_modal', 'js/models/xblock_info', 'js/views/xblock_string_field_editor', 'js/views/xblock_access_editor', 'js/views/pages/container_subviews', 'js/views/unit_outline', 'js/views/utils/xblock_utils', - 'common/js/components/views/feedback_notification', 'common/js/components/views/feedback_prompt', + 'common/js/components/views/feedback_notification', 'common/js/components/views/feedback_prompt', 'js/utils/module', ], -function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView, AddXBlockComponent, - EditXBlockModal, MoveXBlockModal, XBlockInfo, XBlockStringFieldEditor, XBlockAccessEditor, - ContainerSubviews, UnitOutlineView, XBlockUtils, NotificationView, PromptView) { +function($, _, Backbone, gettext, BasePage, + ViewUtils, ContainerView, XBlockView, + AddXBlockComponent, EditXBlockModal, MoveXBlockModal, + XBlockInfo, XBlockStringFieldEditor, XBlockAccessEditor, + ContainerSubviews, UnitOutlineView, XBlockUtils, + NotificationView, PromptView, ModuleUtils) { 'use strict'; var XBlockContainerPage = BasePage.extend({ @@ -26,7 +29,10 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView 'click .delete-button': 'deleteXBlock', 'click .show-actions-menu-button': 'showXBlockActionsMenu', 'click .new-component-button': 'scrollToNewComponentButtons', + 'click .save-button': 'saveSelectedLibraryComponents', 'click .paste-component-button': 'pasteComponent', + 'change .header-library-checkbox': 'toggleLibraryComponent', + 'click .collapse-button': 'collapseXBlock', }, options: { @@ -102,6 +108,7 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView model: this.model }); this.unitOutlineView.render(); + } this.listenTo(Backbone, 'move:onXBlockMoved', this.onXBlockMoved); @@ -521,6 +528,78 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView }); }, + duplicateXBlock: function(event) { + event.preventDefault(); + this.duplicateComponent(this.findXBlockElement(event.target)); + }, + + showMoveXBlockModal: function(event) { + var xblockElement = this.findXBlockElement(event.target), + parentXBlockElement = xblockElement.parents('.studio-xblock-wrapper'), + modal = new MoveXBlockModal({ + sourceXBlockInfo: XBlockUtils.findXBlockInfo(xblockElement, this.model), + sourceParentXBlockInfo: XBlockUtils.findXBlockInfo(parentXBlockElement, this.model), + XBlockURLRoot: this.getURLRoot(), + outlineURL: this.options.outlineURL + }); + + event.preventDefault(); + modal.show(); + }, + + deleteXBlock: function(event) { + event.preventDefault(); + this.deleteComponent(this.findXBlockElement(event.target)); + }, + + createPlaceholderElement: function() { + return $('
', {class: 'studio-xblock-wrapper'}); + }, + + createComponent: function(template, target) { + // A placeholder element is created in the correct location for the new xblock + // and then onNewXBlock will replace it with a rendering of the xblock. Note that + // for xblocks that can't be replaced inline, the entire parent will be refreshed. + var parentElement = this.findXBlockElement(target), + parentLocator = parentElement.data('locator'), + buttonPanel = target.closest('.add-xblock-component'), + listPanel = buttonPanel.prev(), + scrollOffset = ViewUtils.getScrollOffset(buttonPanel), + $placeholderEl = $(this.createPlaceholderElement()), + requestData = _.extend(template, { + parent_locator: parentLocator + }), + placeholderElement; + placeholderElement = $placeholderEl.appendTo(listPanel); + return $.postJSON(this.getURLRoot() + '/', requestData, + _.bind(this.onNewXBlock, this, placeholderElement, scrollOffset, false)) + .fail(function() { + // Remove the placeholder if the update failed + placeholderElement.remove(); + }); + }, + + duplicateComponent: function(xblockElement) { + // A placeholder element is created in the correct location for the duplicate xblock + // and then onNewXBlock will replace it with a rendering of the xblock. Note that + // for xblocks that can't be replaced inline, the entire parent will be refreshed. + var self = this, + parentElement = self.findXBlockElement(xblockElement.parent()), + scrollOffset = ViewUtils.getScrollOffset(xblockElement), + $placeholderEl = $(self.createPlaceholderElement()), + placeholderElement; + + placeholderElement = $placeholderEl.insertAfter(xblockElement); + XBlockUtils.duplicateXBlock(xblockElement, parentElement) + .done(function(data) { + self.onNewXBlock(placeholderElement, scrollOffset, true, data); + }) + .fail(function() { + // Remove the placeholder if the update failed + placeholderElement.remove(); + }); + }, + deleteComponent: function(xblockElement) { var self = this, xblockInfo = new XBlockInfo({ @@ -531,6 +610,61 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView }); }, + getSelectedLibraryComponents: function() { + var self = this; + var locator = this.$el.find('.studio-xblock-wrapper').data('locator'); + console.log(ModuleUtils); + $.getJSON( + ModuleUtils.getUpdateUrl(locator) + '/handler/get_block_ids', + function(data) { + self.selectedLibraryComponents = Array.from(data.source_block_ids); + self.storedSelectedLibraryComponents = Array.from(data.source_block_ids); + } + ); + }, + + saveSelectedLibraryComponents: function(e) { + var self = this; + var locator = this.$el.find('.studio-xblock-wrapper').data('locator'); + e.preventDefault(); + $.postJSON( + ModuleUtils.getUpdateUrl(locator) + '/handler/submit_studio_edits', + {values: {source_block_ids: self.storedSelectedLibraryComponents}}, + function() { + self.selectedLibraryComponents = Array.from(self.storedSelectedLibraryComponents); + self.toggleSaveButton(); + } + ); + }, + + toggleLibraryComponent: function(event) { + var componentId = $(event.target).closest('.studio-xblock-wrapper').data('locator'); + var storeIndex = this.storedSelectedLibraryComponents.indexOf(componentId); + if (storeIndex > -1) { + this.storedSelectedLibraryComponents.splice(storeIndex, 1); + this.toggleSaveButton(); + } else { + this.storedSelectedLibraryComponents.push(componentId); + this.toggleSaveButton(); + } + }, + + toggleSaveButton: function() { + var $saveButton = $('.nav-actions .save-button'); + if (JSON.stringify(this.selectedLibraryComponents.sort()) === JSON.stringify(this.storedSelectedLibraryComponents.sort())) { + $saveButton.addClass('is-hidden'); + window.removeEventListener('beforeunload', this.onBeforePageUnloadCallback); + } else { + $saveButton.removeClass('is-hidden'); + window.addEventListener('beforeunload', this.onBeforePageUnloadCallback); + } + }, + + onBeforePageUnloadCallback: function (event) { + event.preventDefault(); + event.returnValue = ''; + }, + onDelete: function(xblockElement) { // get the parent so we can remove this component from its parent. var xblockView = this.xblockView, diff --git a/cms/static/sass/elements/_layout.scss b/cms/static/sass/elements/_layout.scss index d4240b450f47..ae1090fe4623 100644 --- a/cms/static/sass/elements/_layout.scss +++ b/cms/static/sass/elements/_layout.scss @@ -223,6 +223,10 @@ box-shadow: none; border: 0; background-color: $white; + + &-fullwidth { + width: flex-grid(12, 12); + } } .content-supplementary { diff --git a/cms/static/sass/elements/_xblocks.scss b/cms/static/sass/elements/_xblocks.scss index 87dcb8c7d7c6..119a14826c63 100644 --- a/cms/static/sass/elements/_xblocks.scss +++ b/cms/static/sass/elements/_xblocks.scss @@ -43,6 +43,19 @@ display: flex; align-items: center; + .header-library-checkbox { + margin-right: 10px; + width: 17px; + height: 17px; + cursor: pointer; + vertical-align: middle; + } + + .header-library-checkbox-label { + vertical-align: middle; + cursor: pointer; + } + .header-details { @extend %cont-truncated; @@ -433,7 +446,17 @@ border-color: $blue; } - .xblock-header { + &.is-collapsed { + .xblock-render { + display: none; + } + + .collapse-button .fa { + transform: scale(1, -1); + } + } + + .xblock-header:not(.is-hidden) { display: block; } diff --git a/cms/templates/container.html b/cms/templates/container.html index 2f83c0c6d5e1..6650d66a7f7d 100644 --- a/cms/templates/container.html +++ b/cms/templates/container.html @@ -152,6 +152,14 @@

${_("Page Actions")}

${_("Edit")} + % if is_collapsible: +
  • + + + ${_("Collapse All")} + +
    • + % endif % endif @@ -164,8 +172,7 @@

    ${_("Page Actions")}

    - -
    +
    <% assets_url = reverse('assets_handler', kwargs={'course_key_string': str(xblock_locator.course_key)}) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index a027cc764bd7..2741879b999c 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -17,7 +17,6 @@ xblock_url = xblock_studio_url(xblock) show_inline = xblock.has_children and not xblock_url section_class = "level-nesting" if show_inline else "level-element" -collapsible_class = "is-collapsible" if xblock.has_children else "" label = determine_label(xblock.display_name_with_default, xblock.scope_ids.block_type) messages = xblock.validate().to_json() block_is_unit = is_unit(xblock) @@ -48,14 +47,17 @@
    % endif -
    +
    +

    ${_("Importing components")}

    +
    % endif -
    +