From 4ec6dce683135262fbd388bf46623491c18609d6 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 21 Feb 2024 16:51:02 -0800 Subject: [PATCH 1/7] fix: handle paste of library content blocks correctly --- cms/djangoapps/contentstore/helpers.py | 11 +- .../views/tests/test_clipboard_paste.py | 105 +++++++++++++++++- .../core/djangoapps/content_libraries/api.py | 2 + 3 files changed, 114 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 0d66070b1120..9fd09193b59f 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -17,6 +17,7 @@ from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore from xmodule.exceptions import NotFoundError +from xmodule.library_content_block import LibraryContentBlock from xmodule.modulestore.django import modulestore from xmodule.xml_block import XmlMixin @@ -336,8 +337,14 @@ def _import_xml_node_to_parent( new_xblock = store.update_item(temp_xblock, user_id, allow_not_found=True) parent_xblock.children.append(new_xblock.location) store.update_item(parent_xblock, user_id) - for child_node in child_nodes: - _import_xml_node_to_parent(child_node, new_xblock, store, user_id=user_id) + if isinstance(new_xblock, LibraryContentBlock): + # Special case handling for library content. If we need this for other blocks in the future, it can be made into + # an API, and we'd call new_block.studio_post_paste() instead of this code. + # In this case, we want to pull the children from the library and let library_tools assign their IDs. + new_xblock.sync_from_library(upgrade_to_latest=False) + else: + for child_node in child_nodes: + _import_xml_node_to_parent(child_node, new_xblock, store, user_id=user_id) return new_xblock diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index a447799c2950..4158625ddf15 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -4,12 +4,19 @@ APIs. """ import ddt +from django.test import LiveServerTestCase from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient -from xmodule.modulestore.django import contentstore +from organizations.models import Organization +from xmodule.modulestore.django import contentstore, modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory +from cms.djangoapps.contentstore.utils import reverse_usage_url +from openedx.core.lib.blockstore_api.tests.base import BlockstoreAppTestMixin +from openedx.core.djangoapps.content_libraries import api as library_api +from blockstore.apps import api as blockstore_api + CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/" XBLOCK_ENDPOINT = "/xblock/" @@ -109,7 +116,7 @@ def test_copy_and_paste_component(self, block_args): """ Test copying a component (XBlock) from one course into another """ - source_course = CourseFactory.create(display_name='Destination Course') + source_course = CourseFactory.create(display_name='Source Course') source_block = BlockFactory.create(parent_location=source_course.location, **block_args) dest_course = CourseFactory.create(display_name='Destination Course') @@ -204,3 +211,97 @@ def test_paste_with_assets(self): source_pic2_hash = contentstore().find(source_course.id.make_asset_key("asset", "picture2.jpg")).content_digest dest_pic2_hash = contentstore().find(dest_course_key.make_asset_key("asset", "picture2.jpg")).content_digest assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged. + + +class ClipboardLibraryContentPasteTestCase(BlockstoreAppTestMixin, LiveServerTestCase, ModuleStoreTestCase): + """ + Test Clipboard Paste functionality with library content + """ + + def setUp(self): + """ + Set up a v2 Content Library and a library content block + """ + super().setUp() + self.client = APIClient() + self.client.login(username=self.user.username, password=self.user_password) + self.store = modulestore() + # Create a content library: + library = library_api.create_library( + collection_uuid=blockstore_api.create_collection("Collection").uuid, + library_type=library_api.COMPLEX, + org=Organization.objects.create(name="Test Org", short_name="CL-TEST"), + slug="lib", + title="Library", + ) + # Populate it with a problem: + problem_key = library_api.create_library_block(library.key, "problem", "p1").usage_key + library_api.set_library_block_olx(problem_key, """ + + + + + Wrong + Right + + + + """) + library_api.publish_changes(library.key) + + # Create a library content block (lc), point it out our library, and sync it. + self.course = CourseFactory.create(display_name='Course') + self.orig_lc_block = BlockFactory.create( + parent=self.course, + category="library_content", + source_library_id=str(library.key), + display_name="LC Block", + publish_item=False, + ) + self.dest_lc_block = None + + self._sync_lc_block_from_library('orig_lc_block') + orig_child = self.store.get_item(self.orig_lc_block.children[0]) + assert orig_child.display_name == "MCQ" + + def test_paste_library_content_block(self): + """ + Test the special handling of copying and pasting library content + """ + # Copy a library content block that has children: + copy_response = self.client.post(CLIPBOARD_ENDPOINT, { + "usage_key": str(self.orig_lc_block.location) + }, format="json") + assert copy_response.status_code == 200 + + # Paste the Library content block: + paste_response = self.client.post(XBLOCK_ENDPOINT, { + "parent_locator": str(self.course.location), + "staged_content": "clipboard", + }, format="json") + assert paste_response.status_code == 200 + dest_lc_block_key = UsageKey.from_string(paste_response.json()["locator"]) + + # Get the ID of the new child: + self.dest_lc_block = self.store.get_item(dest_lc_block_key) + dest_child = self.store.get_item(self.dest_lc_block.children[0]) + assert dest_child.display_name == "MCQ" + + # Importantly, the ID of the child must not changed when the library content is synced. + # Otherwise, user state saved against this child will be lost when it syncs. + self._sync_lc_block_from_library('dest_lc_block') + updated_dest_child = self.store.get_item(self.dest_lc_block.children[0]) + assert dest_child.location == updated_dest_child.location + + def _sync_lc_block_from_library(self, attr_name): + """ + Helper method to "sync" a Library Content Block by [re-]fetching its + children from the library. + """ + usage_key = getattr(self, attr_name).location + # It's easiest to do this via the REST API: + handler_url = reverse_usage_url('preview_handler', usage_key, kwargs={'handler': 'upgrade_and_sync'}) + response = self.client.post(handler_url) + assert response.status_code == 200 + # Now reload the block and make sure the child is in place + setattr(self, attr_name, self.store.get_item(usage_key)) # we must reload after upgrade_and_sync diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 737673b32e3f..696c4042f004 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -425,6 +425,8 @@ def create_library( allow_public_read: Allow anyone to view blocks (including source) in Studio? + library_type: Deprecated parameter, not really used. Set to COMPLEX. + Returns a ContentLibraryMetadata instance. """ assert isinstance(collection_uuid, UUID) From 79d10deba992f1a6434b0c14fbf5c481dcb7fca6 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 21 Feb 2024 17:08:13 -0800 Subject: [PATCH 2/7] fix: make test work with the old API format. --- .../contentstore/views/tests/test_clipboard_paste.py | 4 ++++ openedx/core/djangoapps/content_libraries/api.py | 10 +++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 4158625ddf15..b0d3f19abdbe 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -233,6 +233,10 @@ def setUp(self): org=Organization.objects.create(name="Test Org", short_name="CL-TEST"), slug="lib", title="Library", + description="", + allow_public_learning=False, + allow_public_read=False, + library_license=library_api.ALL_RIGHTS_RESERVED, ) # Populate it with a problem: problem_key = library_api.create_library_block(library.key, "problem", "p1").usage_key diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 696c4042f004..d020a7635b97 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -91,7 +91,15 @@ from xblock.exceptions import XBlockNotFoundError from edx_rest_api_client.client import OAuthAPIClient from openedx.core.djangoapps.content_libraries import permissions -from openedx.core.djangoapps.content_libraries.constants import DRAFT_NAME, COMPLEX +# pylint: disable=unused-import +from openedx.core.djangoapps.content_libraries.constants import ( + ALL_RIGHTS_RESERVED, + CC_4_BY, + COMPLEX, + DRAFT_NAME, + PROBLEM, + VIDEO, +) from openedx.core.djangoapps.content_libraries.library_bundle import LibraryBundle from openedx.core.djangoapps.content_libraries.libraries_index import ContentLibraryIndexer, LibraryBlockIndexer from openedx.core.djangoapps.content_libraries.models import ( From ea4a66334031e29abc0f03cc3327c92c6c6218d2 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 23 Feb 2024 11:23:55 -0800 Subject: [PATCH 3/7] fix: actually skip the test; it won't work at all --- cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index b0d3f19abdbe..5a481cb985fa 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -4,6 +4,7 @@ APIs. """ import ddt +import unittest from django.test import LiveServerTestCase from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient @@ -268,6 +269,7 @@ def setUp(self): orig_child = self.store.get_item(self.orig_lc_block.children[0]) assert orig_child.display_name == "MCQ" + @unittest.skip("Quince doesn't support using V2 libraries via library content block; this test was backported") def test_paste_library_content_block(self): """ Test the special handling of copying and pasting library content From e0909d5c0962a2d7e5055ce1ea6a6ed5e164d563 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 27 Feb 2024 10:45:01 -0800 Subject: [PATCH 4/7] fix: compat with v1 libraries --- cms/djangoapps/contentstore/helpers.py | 2 +- .../views/tests/test_clipboard_paste.py | 63 ++++++++++++++++--- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 9fd09193b59f..2bfdd6574aad 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -341,7 +341,7 @@ def _import_xml_node_to_parent( # Special case handling for library content. If we need this for other blocks in the future, it can be made into # an API, and we'd call new_block.studio_post_paste() instead of this code. # In this case, we want to pull the children from the library and let library_tools assign their IDs. - new_xblock.sync_from_library(upgrade_to_latest=False) + new_xblock.tools.update_children(new_xblock, version=new_xblock.source_library_version) else: for child_node in child_nodes: _import_xml_node_to_parent(child_node, new_xblock, store, user_id=user_id) diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 5a481cb985fa..3fb0815574e5 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -6,12 +6,13 @@ import ddt import unittest from django.test import LiveServerTestCase +from django.urls import reverse from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient from organizations.models import Organization from xmodule.modulestore.django import contentstore, modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course -from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, LibraryFactory, ToyCourseFactory from cms.djangoapps.contentstore.utils import reverse_usage_url from openedx.core.lib.blockstore_api.tests.base import BlockstoreAppTestMixin @@ -227,6 +228,12 @@ def setUp(self): self.client = APIClient() self.client.login(username=self.user.username, password=self.user_password) self.store = modulestore() + + @unittest.skip("Quince doesn't support using V2 libraries via library content block; this test was backported") + def test_paste_library_content_block(self): + """ + Test the special handling of copying and pasting library content + """ # Create a content library: library = library_api.create_library( collection_uuid=blockstore_api.create_collection("Collection").uuid, @@ -268,12 +275,6 @@ def setUp(self): self._sync_lc_block_from_library('orig_lc_block') orig_child = self.store.get_item(self.orig_lc_block.children[0]) assert orig_child.display_name == "MCQ" - - @unittest.skip("Quince doesn't support using V2 libraries via library content block; this test was backported") - def test_paste_library_content_block(self): - """ - Test the special handling of copying and pasting library content - """ # Copy a library content block that has children: copy_response = self.client.post(CLIPBOARD_ENDPOINT, { "usage_key": str(self.orig_lc_block.location) @@ -299,6 +300,54 @@ def test_paste_library_content_block(self): updated_dest_child = self.store.get_item(self.dest_lc_block.children[0]) assert dest_child.location == updated_dest_child.location + def test_paste_library_content_block_v1(self): + """ + Same as the above test, but uses modulestore (v1) content library + """ + library = LibraryFactory.create() + data = { + 'parent_locator': str(library.location), + 'category': 'html', + 'display_name': 'HTML Content', + } + response = self.client.ajax_post(reverse('xblock_handler'), data) + self.assertEqual(response.status_code, 200) + course = CourseFactory.create(display_name='Course') + orig_lc_block = BlockFactory.create( + parent=course, + category="library_content", + source_library_id=str(library.location.library_key), + display_name="LC Block", + publish_item=False, + ) + orig_lc_block.refresh_children() + orig_child = self.store.get_item(orig_lc_block.children[0]) + assert orig_child.display_name == "HTML Content" + # Copy a library content block that has children: + copy_response = self.client.post(CLIPBOARD_ENDPOINT, { + "usage_key": str(orig_lc_block.location) + }, format="json") + assert copy_response.status_code == 200 + + # Paste the Library content block: + paste_response = self.client.post(XBLOCK_ENDPOINT, { + "parent_locator": str(course.location), + "staged_content": "clipboard", + }, format="json") + assert paste_response.status_code == 200 + dest_lc_block_key = UsageKey.from_string(paste_response.json()["locator"]) + + # Get the ID of the new child: + dest_lc_block = self.store.get_item(dest_lc_block_key) + dest_child = self.store.get_item(dest_lc_block.children[0]) + assert dest_child.display_name == "HTML Content" + + # Importantly, the ID of the child must not changed when the library content is synced. + # Otherwise, user state saved against this child will be lost when it syncs. + dest_lc_block.refresh_children() + updated_dest_child = self.store.get_item(dest_lc_block.children[0]) + assert dest_child.location == updated_dest_child.location + def _sync_lc_block_from_library(self, attr_name): """ Helper method to "sync" a Library Content Block by [re-]fetching its From 41afb48850fcaec83ff81253c2d9d40496fd2f9e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 27 Feb 2024 10:59:22 -0800 Subject: [PATCH 5/7] chore: remove unused code --- .../views/tests/test_clipboard_paste.py | 75 ------------------- 1 file changed, 75 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 3fb0815574e5..dfa5d6dfea97 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -4,20 +4,16 @@ APIs. """ import ddt -import unittest from django.test import LiveServerTestCase from django.urls import reverse from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient -from organizations.models import Organization from xmodule.modulestore.django import contentstore, modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, LibraryFactory, ToyCourseFactory from cms.djangoapps.contentstore.utils import reverse_usage_url from openedx.core.lib.blockstore_api.tests.base import BlockstoreAppTestMixin -from openedx.core.djangoapps.content_libraries import api as library_api -from blockstore.apps import api as blockstore_api CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/" XBLOCK_ENDPOINT = "/xblock/" @@ -229,77 +225,6 @@ def setUp(self): self.client.login(username=self.user.username, password=self.user_password) self.store = modulestore() - @unittest.skip("Quince doesn't support using V2 libraries via library content block; this test was backported") - def test_paste_library_content_block(self): - """ - Test the special handling of copying and pasting library content - """ - # Create a content library: - library = library_api.create_library( - collection_uuid=blockstore_api.create_collection("Collection").uuid, - library_type=library_api.COMPLEX, - org=Organization.objects.create(name="Test Org", short_name="CL-TEST"), - slug="lib", - title="Library", - description="", - allow_public_learning=False, - allow_public_read=False, - library_license=library_api.ALL_RIGHTS_RESERVED, - ) - # Populate it with a problem: - problem_key = library_api.create_library_block(library.key, "problem", "p1").usage_key - library_api.set_library_block_olx(problem_key, """ - - - - - Wrong - Right - - - - """) - library_api.publish_changes(library.key) - - # Create a library content block (lc), point it out our library, and sync it. - self.course = CourseFactory.create(display_name='Course') - self.orig_lc_block = BlockFactory.create( - parent=self.course, - category="library_content", - source_library_id=str(library.key), - display_name="LC Block", - publish_item=False, - ) - self.dest_lc_block = None - - self._sync_lc_block_from_library('orig_lc_block') - orig_child = self.store.get_item(self.orig_lc_block.children[0]) - assert orig_child.display_name == "MCQ" - # Copy a library content block that has children: - copy_response = self.client.post(CLIPBOARD_ENDPOINT, { - "usage_key": str(self.orig_lc_block.location) - }, format="json") - assert copy_response.status_code == 200 - - # Paste the Library content block: - paste_response = self.client.post(XBLOCK_ENDPOINT, { - "parent_locator": str(self.course.location), - "staged_content": "clipboard", - }, format="json") - assert paste_response.status_code == 200 - dest_lc_block_key = UsageKey.from_string(paste_response.json()["locator"]) - - # Get the ID of the new child: - self.dest_lc_block = self.store.get_item(dest_lc_block_key) - dest_child = self.store.get_item(self.dest_lc_block.children[0]) - assert dest_child.display_name == "MCQ" - - # Importantly, the ID of the child must not changed when the library content is synced. - # Otherwise, user state saved against this child will be lost when it syncs. - self._sync_lc_block_from_library('dest_lc_block') - updated_dest_child = self.store.get_item(self.dest_lc_block.children[0]) - assert dest_child.location == updated_dest_child.location - def test_paste_library_content_block_v1(self): """ Same as the above test, but uses modulestore (v1) content library From 89c07e83f22a798166647409f4d4d8fa28e56c93 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 27 Feb 2024 11:13:22 -0800 Subject: [PATCH 6/7] test: fix test --- .../contentstore/views/tests/test_clipboard_paste.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index dfa5d6dfea97..e9d7d5bba372 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -13,7 +13,7 @@ from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, LibraryFactory, ToyCourseFactory from cms.djangoapps.contentstore.utils import reverse_usage_url -from openedx.core.lib.blockstore_api.tests.base import BlockstoreAppTestMixin +from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/" XBLOCK_ENDPOINT = "/xblock/" @@ -211,7 +211,7 @@ def test_paste_with_assets(self): assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged. -class ClipboardLibraryContentPasteTestCase(BlockstoreAppTestMixin, LiveServerTestCase, ModuleStoreTestCase): +class ClipboardLibraryContentPasteTestCase(LiveServerTestCase, ModuleStoreTestCase): """ Test Clipboard Paste functionality with library content """ @@ -221,7 +221,7 @@ def setUp(self): Set up a v2 Content Library and a library content block """ super().setUp() - self.client = APIClient() + self.client = AjaxEnabledTestClient() self.client.login(username=self.user.username, password=self.user_password) self.store = modulestore() From e6bc97cc04442fe92f63a367f3edeceb8a43cf3b Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 27 Feb 2024 11:54:13 -0800 Subject: [PATCH 7/7] test: fix test? --- .../contentstore/views/tests/test_clipboard_paste.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index e9d7d5bba372..bdf22532fd5f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -5,7 +5,6 @@ """ import ddt from django.test import LiveServerTestCase -from django.urls import reverse from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient from xmodule.modulestore.django import contentstore, modulestore @@ -235,7 +234,7 @@ def test_paste_library_content_block_v1(self): 'category': 'html', 'display_name': 'HTML Content', } - response = self.client.ajax_post(reverse('xblock_handler'), data) + response = self.client.ajax_post(XBLOCK_ENDPOINT, data) self.assertEqual(response.status_code, 200) course = CourseFactory.create(display_name='Course') orig_lc_block = BlockFactory.create( @@ -255,10 +254,10 @@ def test_paste_library_content_block_v1(self): assert copy_response.status_code == 200 # Paste the Library content block: - paste_response = self.client.post(XBLOCK_ENDPOINT, { + paste_response = self.client.ajax_post(XBLOCK_ENDPOINT, { "parent_locator": str(course.location), "staged_content": "clipboard", - }, format="json") + }) assert paste_response.status_code == 200 dest_lc_block_key = UsageKey.from_string(paste_response.json()["locator"])