From 5c7cf446ea568b87ed1b707c2cec85983936da3e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 7 Dec 2023 11:12:12 -0800 Subject: [PATCH] fix: error pasting a unit containing a completely blank problem --- cms/djangoapps/contentstore/helpers.py | 18 +++++---- .../views/tests/test_clipboard_paste.py | 37 +++++++++++++++++++ 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index c2cc876f19da..0d66070b1120 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -302,6 +302,16 @@ def _import_xml_node_to_parent( # and VAL will thus make the transcript available. child_nodes = [] + + if issubclass(xblock_class, XmlMixin): + # Hack: XBlocks that use "XmlMixin" have their own XML parsing behavior, and in particular if they encounter + # an XML node that has no children and has only a "url_name" attribute, they'll try to load the XML data + # from an XML file in runtime.resources_fs. But that file doesn't exist here. So we set at least one + # additional attribute here to make sure that url_name is not the only attribute; otherwise in some cases, + # XmlMixin.parse_xml will try to load an XML file that doesn't exist, giving an error. The name and value + # of this attribute don't matter and should be ignored. + node.attrib["x-is-pointer-node"] = "no" + if not xblock_class.has_children: # No children to worry about. The XML may contain child nodes, but they're not XBlocks. temp_xblock = xblock_class.parse_xml(node, runtime, keys, id_generator) @@ -314,14 +324,6 @@ def _import_xml_node_to_parent( # serialization of a child block, in order. For blocks that don't support children, their XML content/nodes # could be anything (e.g. HTML, capa) node_without_children = etree.Element(node.tag, **node.attrib) - if issubclass(xblock_class, XmlMixin): - # Hack: XBlocks that use "XmlMixin" have their own XML parsing behavior, and in particular if they encounter - # an XML node that has no children and has only a "url_name" attribute, they'll try to load the XML data - # from an XML file in runtime.resources_fs. But that file doesn't exist here. So we set at least one - # additional attribute here to make sure that url_name is not the only attribute; otherwise in some cases, - # XmlMixin.parse_xml will try to load an XML file that doesn't exist, giving an error. The name and value - # of this attribute don't matter and should be ignored. - node_without_children.attrib["x-is-pointer-node"] = "no" temp_xblock = xblock_class.parse_xml(node_without_children, runtime, keys, id_generator) child_nodes = list(node) if xblock_class.has_children and temp_xblock.children: diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 429630ac8d1b..a447799c2950 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -3,6 +3,7 @@ allow users to paste XBlocks that were copied using the staged_content/clipboard APIs. """ +import ddt from opaque_keys.edx.keys import UsageKey from rest_framework.test import APIClient from xmodule.modulestore.django import contentstore @@ -13,6 +14,7 @@ XBLOCK_ENDPOINT = "/xblock/" +@ddt.ddt class ClipboardPasteTestCase(ModuleStoreTestCase): """ Test Clipboard Paste functionality @@ -99,6 +101,41 @@ def test_copy_and_paste_unit(self): # The new block should store a reference to where it was copied from assert dest_unit.copied_from_block == str(unit_key) + @ddt.data( + # A problem with absolutely no fields set. A previous version of copy-paste had an error when pasting this. + {"category": "problem", "display_name": None, "data": ""}, + ) + 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_block = BlockFactory.create(parent_location=source_course.location, **block_args) + + dest_course = CourseFactory.create(display_name='Destination Course') + with self.store.bulk_operations(dest_course.id): + dest_chapter = BlockFactory.create(parent=dest_course, category='chapter', display_name='Section') + dest_sequential = BlockFactory.create(parent=dest_chapter, category='sequential', display_name='Subsection') + + # Copy the block + client = APIClient() + client.login(username=self.user.username, password=self.user_password) + copy_response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(source_block.location)}, format="json") + assert copy_response.status_code == 200 + + # Paste the unit + paste_response = client.post(XBLOCK_ENDPOINT, { + "parent_locator": str(dest_sequential.location), + "staged_content": "clipboard", + }, format="json") + assert paste_response.status_code == 200 + dest_block_key = UsageKey.from_string(paste_response.json()["locator"]) + + dest_block = self.store.get_item(dest_block_key) + assert dest_block.display_name == source_block.display_name + # The new block should store a reference to where it was copied from + assert dest_block.copied_from_block == str(source_block.location) + def test_paste_with_assets(self): """ When pasting into a different course, any required static assets should