[FC-0009] Studio backend APIs to support copying and pasting of entire Units (#32812)

* feat: tests for copying units in content_staging app

* chore: convert old tests to pytest style

* feat: make pasting units work (in Studio backend API)

* refactor: make XmlMixin.parse_xml more like standard XBlock behavior

cms/djangoapps/contentstore/helpers.py

@@ -18,6 +18,7 @@ from xmodule.contentstore.content import StaticContent
 from xmodule.contentstore.django import contentstore
 from xmodule.exceptions import NotFoundError
 from xmodule.modulestore.django import modulestore
+from xmodule.xml_block import XmlMixin
 
 from cms.djangoapps.models.settings.course_grading import CourseGradingModel
 import openedx.core.djangoapps.content_staging.api as content_staging_api
@@ -238,7 +239,6 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) ->
     if not user_clipboard:
         # Clipboard is empty or expired/error/loading
         return None, StaticFileNotices()
-    block_type = user_clipboard.content.block_type
     olx_str = content_staging_api.get_staged_content_olx(user_clipboard.content.id)
     static_files = content_staging_api.get_staged_content_static_files(user_clipboard.content.id)
     node = etree.fromstring(olx_str)
@@ -246,35 +246,15 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) ->
     with store.bulk_operations(parent_key.course_key):
         parent_descriptor = store.get_item(parent_key)
         # Some blocks like drag-and-drop only work here with the full XBlock runtime loaded:
-
         parent_xblock = _load_preview_block(request, parent_descriptor)
-        runtime = parent_xblock.runtime
-        # Generate the new ID:
-        id_generator = ImportIdGenerator(parent_key.context_key)
-        def_id = id_generator.create_definition(block_type, user_clipboard.source_usage_key.block_id)
-        usage_id = id_generator.create_usage(def_id)
-        keys = ScopeIds(None, block_type, def_id, usage_id)
-        # parse_xml is a really messy API. We pass both 'keys' and 'id_generator' and, depending on the XBlock, either
-        # one may be used to determine the new XBlock's usage key, and the other will be ignored. e.g. video ignores
-        # 'keys' and uses 'id_generator', but the default XBlock parse_xml ignores 'id_generator' and uses 'keys'.
-        # For children of this block, obviously only id_generator is used.
-        xblock_class = runtime.load_block_type(block_type)
-        # Note: if we find a case where any XBlock needs access to the block-specific static files that were saved to
-        # export_fs during copying, we could make them available here via runtime.resources_fs before calling parse_xml.
-        # However, currently the only known case for that is video block's transcript files, and those will
-        # automatically be "carried over" to the new XBlock even in a different course because the video ID is the same,
-        # and VAL will thus make the transcript available.
-        temp_xblock = xblock_class.parse_xml(node, runtime, keys, id_generator)
-        if xblock_class.has_children and temp_xblock.children:
-            raise NotImplementedError("We don't yet support pasting XBlocks with children")
-        temp_xblock.parent = parent_key
-        # Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin)
-        temp_xblock.copied_from_block = str(user_clipboard.source_usage_key)
-        # Save the XBlock into modulestore. We need to save the block and its parent for this to work:
-        new_xblock = store.update_item(temp_xblock, request.user.id, allow_not_found=True)
-        parent_xblock.children.append(new_xblock.location)
-        store.update_item(parent_xblock, request.user.id)
-
+        new_xblock = _import_xml_node_to_parent(
+            node,
+            parent_xblock,
+            store,
+            user_id=request.user.id,
+            slug_hint=user_clipboard.source_usage_key.block_id,
+            copied_from_block=str(user_clipboard.source_usage_key),
+        )
     # Now handle static files that need to go into Files & Uploads:
     notices = _import_files_into_course(
         course_key=parent_key.context_key,
@@ -285,6 +265,80 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) ->
     return new_xblock, notices
 
 
+def _import_xml_node_to_parent(
+    node,
+    parent_xblock: XBlock,
+    # The modulestore we're using
+    store,
+    # The ID of the user who is performing this operation
+    user_id: int,
+    # Hint to use as usage ID (block_id) for the new XBlock
+    slug_hint: str | None = None,
+    # UsageKey of the XBlock that this one is a copy of
+    copied_from_block: str | None = None,
+) -> XBlock:
+    """
+    Given an XML node representing a serialized XBlock (OLX), import it into modulestore 'store' as a child of the
+    specified parent block. Recursively copy children as needed.
+    """
+    runtime = parent_xblock.runtime
+    parent_key = parent_xblock.scope_ids.usage_id
+    block_type = node.tag
+
+    # Generate the new ID:
+    id_generator = ImportIdGenerator(parent_key.context_key)
+    def_id = id_generator.create_definition(block_type, slug_hint)
+    usage_id = id_generator.create_usage(def_id)
+    keys = ScopeIds(None, block_type, def_id, usage_id)
+    # parse_xml is a really messy API. We pass both 'keys' and 'id_generator' and, depending on the XBlock, either
+    # one may be used to determine the new XBlock's usage key, and the other will be ignored. e.g. video ignores
+    # 'keys' and uses 'id_generator', but the default XBlock parse_xml ignores 'id_generator' and uses 'keys'.
+    # For children of this block, obviously only id_generator is used.
+    xblock_class = runtime.load_block_type(block_type)
+    # Note: if we find a case where any XBlock needs access to the block-specific static files that were saved to
+    # export_fs during copying, we could make them available here via runtime.resources_fs before calling parse_xml.
+    # However, currently the only known case for that is video block's transcript files, and those will
+    # automatically be "carried over" to the new XBlock even in a different course because the video ID is the same,
+    # and VAL will thus make the transcript available.
+
+    child_nodes = []
+    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)
+    else:
+        # We have to handle the children ourselves, because there are lots of complex interactions between
+        #    * the vanilla XBlock parse_xml() method, and its lack of API for "create and save a new XBlock"
+        #    * the XmlMixin version of parse_xml() which only works with ImportSystem, not modulestore or the v2 runtime
+        #    * the modulestore APIs for creating and saving a new XBlock, which work but don't support XML parsing.
+        # We can safely assume that if the XBLock class supports children, every child node will be the XML
+        # 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:
+        raise NotImplementedError("We don't yet support pasting XBlocks with children")
+    temp_xblock.parent = parent_key
+    if copied_from_block:
+        # Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin)
+        temp_xblock.copied_from_block = copied_from_block
+    # Save the XBlock into modulestore. We need to save the block and its parent for this to work:
+    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)
+    return new_xblock
+
+
 def _import_files_into_course(
     course_key: CourseKey,
     staged_content_id: int,

cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py

@@ -5,7 +5,7 @@ APIs.
 """
 from opaque_keys.edx.keys import UsageKey
 from rest_framework.test import APIClient
-from xmodule.modulestore.django import contentstore, modulestore
+from xmodule.modulestore.django import contentstore
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course
 from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory
 
@@ -34,7 +34,7 @@ class ClipboardPasteTestCase(ModuleStoreTestCase):
 
         # Check how many blocks are in the vertical currently
         parent_key = course_key.make_usage_key("vertical", "vertical_test")  # This is the vertical that holds the video
-        orig_vertical = modulestore().get_item(parent_key)
+        orig_vertical = self.store.get_item(parent_key)
         assert len(orig_vertical.children) == 4
 
         # Copy the video
@@ -51,16 +51,54 @@ class ClipboardPasteTestCase(ModuleStoreTestCase):
         new_block_key = UsageKey.from_string(paste_response.json()["locator"])
 
         # Now there should be an extra block in the vertical:
-        updated_vertical = modulestore().get_item(parent_key)
+        updated_vertical = self.store.get_item(parent_key)
         assert len(updated_vertical.children) == 5
         assert updated_vertical.children[-1] == new_block_key
         # And it should match the original:
-        orig_video = modulestore().get_item(video_key)
-        new_video = modulestore().get_item(new_block_key)
+        orig_video = self.store.get_item(video_key)
+        new_video = self.store.get_item(new_block_key)
         assert new_video.youtube_id_1_0 == orig_video.youtube_id_1_0
         # The new block should store a reference to where it was copied from
         assert new_video.copied_from_block == str(video_key)
 
+    def test_copy_and_paste_unit(self):
+        """
+        Test copying a unit (vertical) from one course into another
+        """
+        course_key, client = self._setup_course()
+        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 unit
+        unit_key = course_key.make_usage_key("vertical", "vertical_test")
+        copy_response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(unit_key)}, 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_unit_key = UsageKey.from_string(paste_response.json()["locator"])
+
+        # Now there should be a one unit/vertical as a child of the destination sequential/subsection:
+        updated_sequential = self.store.get_item(dest_sequential.location)
+        assert updated_sequential.children == [dest_unit_key]
+        # And it should match the original:
+        orig_unit = self.store.get_item(unit_key)
+        dest_unit = self.store.get_item(dest_unit_key)
+        assert len(orig_unit.children) == len(dest_unit.children)
+        # Check details of the fourth child (a poll)
+        orig_poll = self.store.get_item(orig_unit.children[3])
+        dest_poll = self.store.get_item(dest_unit.children[3])
+        assert dest_poll.display_name == orig_poll.display_name
+        assert dest_poll.question == orig_poll.question
+        # The new block should store a reference to where it was copied from
+        assert dest_unit.copied_from_block == str(unit_key)
+
     def test_paste_with_assets(self):
         """
         When pasting into a different course, any required static assets should

openedx/core/djangoapps/content_staging/tests/test_clipboard.py

@@ -42,20 +42,17 @@ class ClipboardTestCase(ModuleStoreTestCase):
         client.login(username=self.user.username, password=self.user_password)
         response = client.get(CLIPBOARD_ENDPOINT)
         # We don't consider this a 404 error, it's a 200 with an empty response
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.json(), {
+        assert response.status_code == 200
+        assert response.json() == {
             "content": None,
             "source_usage_key": "",
             "source_context_title": "",
             "source_edit_url": "",
-        })
+        }
         ## The Python method for getting the API response should be identical:
-        self.assertEqual(
-            response.json(),
-            python_api.get_user_clipboard_json(self.user.id, response.wsgi_request),
-        )
+        assert response.json() == python_api.get_user_clipboard_json(self.user.id, response.wsgi_request)
         # And the pure python API should return None
-        self.assertEqual(python_api.get_user_clipboard(self.user.id), None)
+        assert python_api.get_user_clipboard(self.user.id) is None
 
     def _setup_course(self):
         """ Set up the "Toy Course" and an APIClient for testing clipboard functionality. """
@@ -66,8 +63,8 @@ class ClipboardTestCase(ModuleStoreTestCase):
 
         # Initial conditions: clipboard is empty:
         response = client.get(CLIPBOARD_ENDPOINT)
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.json()["content"], None)
+        assert response.status_code == 200
+        assert response.json()["content"] is None
 
         return (course_key, client)
 
@@ -82,27 +79,27 @@ class ClipboardTestCase(ModuleStoreTestCase):
         response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(video_key)}, format="json")
 
         # Validate the response:
-        self.assertEqual(response.status_code, 200)
+        assert response.status_code == 200
         response_data = response.json()
-        self.assertEqual(response_data["source_usage_key"], str(video_key))
-        self.assertEqual(response_data["source_context_title"], "Toy Course")
-        self.assertEqual(response_data["content"], {**response_data["content"], **{
+        assert response_data["source_usage_key"] == str(video_key)
+        assert response_data["source_context_title"] == "Toy Course"
+        assert response_data["content"] == {**response_data["content"], **{
             "block_type": "video",
             "block_type_display": "Video",
             # To ensure API stability, we are hard-coding these expected values:
             "purpose": "clipboard",
             "status": "ready",
             "display_name": "default",  # Weird name but that's what defined in the toy course
-        }})
+        }}
         # Test the actual OLX in the clipboard:
         olx_url = response_data["content"]["olx_url"]
         olx_response = client.get(olx_url)
-        self.assertEqual(olx_response.status_code, 200)
-        self.assertEqual(olx_response.get("Content-Type"), "application/vnd.openedx.xblock.v1.video+xml")
+        assert olx_response.status_code == 200
+        assert olx_response.get("Content-Type") == "application/vnd.openedx.xblock.v1.video+xml"
         self.assertXmlEqual(olx_response.content.decode(), SAMPLE_VIDEO_OLX)
 
         # Now if we GET the clipboard again, the GET response should exactly equal the last POST response:
-        self.assertEqual(client.get(CLIPBOARD_ENDPOINT).json(), response_data)
+        assert client.get(CLIPBOARD_ENDPOINT).json() == response_data
 
     def test_copy_video_python_get(self):
         """
@@ -113,25 +110,25 @@ class ClipboardTestCase(ModuleStoreTestCase):
         # Copy the video
         video_key = course_key.make_usage_key("video", "sample_video")
         response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(video_key)}, format="json")
-        self.assertEqual(response.status_code, 200)
+        assert response.status_code == 200
 
         # Get the clipboard status using python:
         clipboard_data = python_api.get_user_clipboard(self.user.id)
-        self.assertIsNotNone(clipboard_data)
-        self.assertEqual(clipboard_data.source_usage_key, video_key)
+        assert clipboard_data is not None
+        assert clipboard_data.source_usage_key == video_key
         # source_context_title is not in the python API because it's easy to retrieve a course's name from python code.
-        self.assertEqual(clipboard_data.content.block_type, "video")
+        assert clipboard_data.content.block_type == "video"
         # To ensure API stability, we are hard-coding these expected values:
-        self.assertEqual(clipboard_data.content.purpose, "clipboard")
-        self.assertEqual(clipboard_data.content.status, "ready")
-        self.assertEqual(clipboard_data.content.display_name, "default")
+        assert clipboard_data.content.purpose == "clipboard"
+        assert clipboard_data.content.status == "ready"
+        assert clipboard_data.content.display_name == "default"
         # Test the actual OLX in the clipboard:
         olx_data = python_api.get_staged_content_olx(clipboard_data.content.id)
         self.assertXmlEqual(olx_data, SAMPLE_VIDEO_OLX)
 
     def test_copy_html(self):
         """
-        Test copying an HTML from the course
+        Test copying an HTML XBlock from the course
         """
         course_key, client = self._setup_course()
 
@@ -140,32 +137,104 @@ class ClipboardTestCase(ModuleStoreTestCase):
         response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_key)}, format="json")
 
         # Validate the response:
-        self.assertEqual(response.status_code, 200)
+        assert response.status_code == 200
         response_data = response.json()
-        self.assertEqual(response_data["source_usage_key"], str(html_key))
-        self.assertEqual(response_data["source_context_title"], "Toy Course")
-        self.assertEqual(response_data["content"], {**response_data["content"], **{
+        assert response_data["source_usage_key"] == str(html_key)
+        assert response_data["source_context_title"] == "Toy Course"
+        assert response_data["content"] == {**response_data["content"], **{
             "block_type": "html",
             # To ensure API stability, we are hard-coding these expected values:
             "purpose": "clipboard",
             "status": "ready",
             "display_name": "Text",  # Has no display_name set so we fallback to this default
-        }})
+        }}
         # Test the actual OLX in the clipboard:
         olx_url = response_data["content"]["olx_url"]
         olx_response = client.get(olx_url)
-        self.assertEqual(olx_response.status_code, 200)
-        self.assertEqual(olx_response.get("Content-Type"), "application/vnd.openedx.xblock.v1.html+xml")
+        assert olx_response.status_code == 200
+        assert olx_response.get("Content-Type") == "application/vnd.openedx.xblock.v1.html+xml"
         # For HTML, we really want to be sure that the OLX is serialized in this exact format (using CDATA), so we check
         # the actual string directly rather than using assertXmlEqual():
-        self.assertEqual(olx_response.content.decode(), dedent("""
+        assert olx_response.content.decode() == dedent("""
             <html url_name="toyhtml" display_name="Text"><![CDATA[
             <a href='/static/handouts/sample_handout.txt'>Sample</a>
             ]]></html>
-        """).lstrip())
+        """).lstrip()
 
         # Now if we GET the clipboard again, the GET response should exactly equal the last POST response:
-        self.assertEqual(client.get(CLIPBOARD_ENDPOINT).json(), response_data)
+        assert client.get(CLIPBOARD_ENDPOINT).json() == response_data
+
+    def test_copy_unit(self):
+        """
+        Test copying a unit (vertical block) from the course
+        """
+        course_key, client = self._setup_course()
+
+        # Copy the HTML
+        unit_key = course_key.make_usage_key("vertical", "vertical_test")
+        response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(unit_key)}, format="json")
+
+        # Validate the response:
+        assert response.status_code == 200
+        response_data = response.json()
+        assert response_data["source_usage_key"] == str(unit_key)
+        assert response_data["source_context_title"] == "Toy Course"
+        assert response_data["content"] == {**response_data["content"], **{
+            "block_type": "vertical",
+            # To ensure API stability, we are hard-coding these expected values:
+            "purpose": "clipboard",
+            "status": "ready",
+            "display_name": "vertical test",  # Has no display_name set so display_name_with_default falls back to this
+        }}
+        # Test the actual OLX in the clipboard:
+        olx_url = response_data["content"]["olx_url"]
+        olx_response = client.get(olx_url)
+        assert olx_response.status_code == 200
+        assert olx_response.get("Content-Type") == "application/vnd.openedx.xblock.v1.vertical+xml"
+        self.assertXmlEqual(olx_response.content.decode(), """
+            <vertical url_name="vertical_test">
+                <video
+                    url_name="sample_video"
+                    display_name="default"
+                    youtube="0.75:JMD_ifUUfsU,1.00:OEoXaMPEzfM,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY"
+                    youtube_id_0_75="JMD_ifUUfsU"
+                    youtube_id_1_0="OEoXaMPEzfM"
+                    youtube_id_1_25="AKqURZnYqpk"
+                    youtube_id_1_5="DYpADpL7jAY"
+                />
+                <video
+                    url_name="separate_file_video"
+                    display_name="default"
+                    youtube="0.75:JMD_ifUUfsU,1.00:OEoXaMPEzfM,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY"
+                    youtube_id_0_75="JMD_ifUUfsU"
+                    youtube_id_1_0="OEoXaMPEzfM"
+                    youtube_id_1_25="AKqURZnYqpk"
+                    youtube_id_1_5="DYpADpL7jAY"
+                />
+                <video
+                    url_name="video_with_end_time"
+                    display_name="default"
+                    youtube="0.75:JMD_ifUUfsU,1.00:OEoXaMPEzfM,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY"
+                    end_time="00:00:10"
+                    youtube_id_0_75="JMD_ifUUfsU"
+                    youtube_id_1_0="OEoXaMPEzfM"
+                    youtube_id_1_25="AKqURZnYqpk"
+                    youtube_id_1_5="DYpADpL7jAY"
+                />
+                <poll_question
+                    url_name="T1_changemind_poll_foo_2"
+                    display_name="Change your answer"
+                    reset="false"
+                >
+                    &lt;p&gt;Have you changed your mind?&lt;/p&gt;
+                    <answer id="yes">Yes</answer>
+                    <answer id="no">No</answer>
+                </poll_question>
+            </vertical>
+        """)
+
+        # Now if we GET the clipboard again, the GET response should exactly equal the last POST response:
+        assert client.get(CLIPBOARD_ENDPOINT).json() == response_data
 
     def test_copy_several_things(self):
         """
@@ -176,29 +245,29 @@ class ClipboardTestCase(ModuleStoreTestCase):
         # Copy the video and validate the response:
         video_key = course_key.make_usage_key("video", "sample_video")
         response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(video_key)}, format="json")
-        self.assertEqual(response.status_code, 200)
+        assert response.status_code == 200
         video_clip_data = response.json()
-        self.assertEqual(video_clip_data["source_usage_key"], str(video_key))
-        self.assertEqual(video_clip_data["content"]["block_type"], "video")
+        assert video_clip_data["source_usage_key"] == str(video_key)
+        assert video_clip_data["content"]["block_type"] == "video"
         old_olx_url = video_clip_data["content"]["olx_url"]
-        self.assertEqual(client.get(old_olx_url).status_code, 200)
+        assert client.get(old_olx_url).status_code == 200
 
         # Now copy some HTML:
         html_key = course_key.make_usage_key("html", "toyhtml")
         response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_key)}, format="json")
-        self.assertEqual(response.status_code, 200)
+        assert response.status_code == 200
 
         # Now check the clipboard:
         response = client.get(CLIPBOARD_ENDPOINT)
         html_clip_data = response.json()
-        self.assertEqual(html_clip_data["source_usage_key"], str(html_key))
-        self.assertEqual(html_clip_data["content"]["block_type"], "html")
-        self.assertEqual(html_clip_data["content"]["block_type_display"], "Text")
+        assert html_clip_data["source_usage_key"] == str(html_key)
+        assert html_clip_data["content"]["block_type"] == "html"
+        assert html_clip_data["content"]["block_type_display"] == "Text"
         ## The Python method for getting the API response should be identical:
-        self.assertEqual(html_clip_data, python_api.get_user_clipboard_json(self.user.id, response.wsgi_request))
+        assert html_clip_data == python_api.get_user_clipboard_json(self.user.id, response.wsgi_request)
 
         # The OLX link from the video will no longer work:
-        self.assertEqual(client.get(old_olx_url).status_code, 404)
+        assert client.get(old_olx_url).status_code == 404
 
     def test_copy_static_assets(self):
         """
@@ -218,7 +287,7 @@ class ClipboardTestCase(ModuleStoreTestCase):
         response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_key)}, format="json")
 
         # Validate the response:
-        self.assertEqual(response.status_code, 200)
+        assert response.status_code == 200
         response_data = response.json()
         staged_content_id = response_data["content"]["id"]
         olx_str = python_api.get_staged_content_olx(staged_content_id)
@@ -253,7 +322,7 @@ class ClipboardTestCase(ModuleStoreTestCase):
         response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_block.location)}, format="json")
 
         # Validate the response:
-        self.assertEqual(response.status_code, 200)
+        assert response.status_code == 200
         response_data = response.json()
         staged_content_id = response_data["content"]["id"]
         olx_str = python_api.get_staged_content_olx(staged_content_id)
@@ -274,9 +343,9 @@ class ClipboardTestCase(ModuleStoreTestCase):
         html_key = course_key.make_usage_key("html", "toyhtml")
         with self.allow_transaction_exception():
             response = nonstaff_client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_key)}, format="json")
-            self.assertEqual(response.status_code, 403)
+            assert response.status_code == 403
         response = nonstaff_client.get(CLIPBOARD_ENDPOINT)
-        self.assertEqual(response.json()["content"], None)
+        assert response.json()["content"] is None
 
     def test_no_stealing_clipboard_content(self):
         """
@@ -293,11 +362,10 @@ class ClipboardTestCase(ModuleStoreTestCase):
         # Then another user tries to get the OLX:
         olx_url = response.json()["content"]["olx_url"]
         response = nonstaff_client.get(olx_url)
-        self.assertEqual(response.status_code, 403)
+        assert response.status_code == 403
 
     def assertXmlEqual(self, xml_str_a: str, xml_str_b: str) -> bool:
         """ Assert that the given XML strings are equal, ignoring attribute order and some whitespace variations. """
-        self.assertEqual(
-            ElementTree.canonicalize(xml_str_a, strip_text=True),
-            ElementTree.canonicalize(xml_str_b, strip_text=True),
-        )
+        a = ElementTree.canonicalize(xml_str_a, strip_text=True)
+        b = ElementTree.canonicalize(xml_str_b, strip_text=True)
+        assert a == b

xmodule/xml_block.py

@@ -1,7 +1,6 @@
 # lint-amnesty, pylint: disable=missing-module-docstring
-import datetime
-
 import copy
+import datetime
 import json
 import logging
 import os
@@ -286,7 +285,7 @@ class XmlMixin:
                 metadata[attr] = value
 
     @classmethod
-    def parse_xml(cls, node, runtime, _keys, id_generator):
+    def parse_xml(cls, node, runtime, keys, id_generator):  # pylint: disable=too-many-statements
         """
         Use `node` to construct a new block.
 
@@ -295,8 +294,8 @@ class XmlMixin:
 
             runtime (:class:`.Runtime`): The runtime to use while parsing.
 
-            _keys (:class:`.ScopeIds`): The keys identifying where this block
-                will store its data. Not used by this implementation.
+            keys (:class:`.ScopeIds`): The keys identifying where this block
+                will store its data.
 
             id_generator (:class:`.IdGenerator`): An object that will allow the
                 runtime to generate correct definition and usage ids for
@@ -305,9 +304,13 @@ class XmlMixin:
         Returns (XBlock): The newly parsed XBlock
 
         """
-        url_name = node.get('url_name')
-        def_id = id_generator.create_definition(node.tag, url_name)
-        usage_id = id_generator.create_usage(def_id)
+        from xmodule.modulestore.xml import ImportSystem  # done here to avoid circular import
+        if id_generator is None:
+            id_generator = runtime.id_generator
+        if keys is None:
+            # Passing keys=None is against the XBlock API but some platform tests do it.
+            def_id = id_generator.create_definition(node.tag, node.get('url_name'))
+            keys = ScopeIds(None, node.tag, def_id, id_generator.create_usage(def_id))
         aside_children = []
 
         # VS[compat]
@@ -320,14 +323,14 @@ class XmlMixin:
         if is_pointer_tag(node):
             # new style:
             # read the actual definition file--named using url_name.replace(':','/')
-            definition_xml, filepath = cls.load_definition_xml(node, runtime, def_id)
-            aside_children = runtime.parse_asides(definition_xml, def_id, usage_id, id_generator)
+            definition_xml, filepath = cls.load_definition_xml(node, runtime, keys.def_id)
+            aside_children = runtime.parse_asides(definition_xml, keys.def_id, keys.usage_id, id_generator)
         else:
             filepath = None
             definition_xml = node
 
         # Note: removes metadata.
-        definition, children = cls.load_definition(definition_xml, runtime, def_id, id_generator)
+        definition, children = cls.load_definition(definition_xml, runtime, keys.def_id, id_generator)
 
         # VS[compat]
         # Make Ike's github preview links work in both old and new file layouts.
@@ -352,26 +355,31 @@ class XmlMixin:
             aside_children.extend(definition_aside_children)
 
         # Set/override any metadata specified by policy
-        cls.apply_policy(metadata, runtime.get_policy(usage_id))
-
-        field_data = {}
-        field_data.update(metadata)
-        field_data.update(definition)
-        field_data['children'] = children
+        cls.apply_policy(metadata, runtime.get_policy(keys.usage_id))
 
+        field_data = {**metadata, **definition, "children": children}
         field_data['xml_attributes']['filename'] = definition.get('filename', ['', None])  # for git link
-        # TODO: we shouldn't be instantiating our own field data instance here, but rather just call to
-        # runtime.construct_xblock_from_class() and then set fields on the returned block.
-        # See the base XBlock class (XmlSerializationMixin.parse_xml) for how it should be done.
-        kvs = InheritanceKeyValueStore(initial_values=field_data)
-        field_data = KvsFieldData(kvs)
+        if "filename" in field_data:
+            del field_data["filename"]  # filename should only be in xml_attributes.
 
-        xblock = runtime.construct_xblock_from_class(
-            cls,
-            # We're loading a block, so student_id is meaningless
-            ScopeIds(None, node.tag, def_id, usage_id),
-            field_data,
-        )
+        if isinstance(runtime, ImportSystem):
+            # we shouldn't be instantiating our own field data instance here, but there are complex inter-depenencies
+            # between this mixin and ImportSystem that currently seem to require it for proper metadata inheritance.
+            kvs = InheritanceKeyValueStore(initial_values=field_data)
+            field_data = KvsFieldData(kvs)
+            xblock = runtime.construct_xblock_from_class(cls, keys, field_data)
+        else:
+            # The "normal" / new way to set field data:
+            xblock = runtime.construct_xblock_from_class(cls, keys)
+            for (key, value_jsonish) in field_data.items():
+                if key in cls.fields:
+                    setattr(xblock, key, cls.fields[key].from_json(value_jsonish))
+                elif key == 'children':
+                    xblock.children = value_jsonish
+                else:
+                    log.warning(
+                        "Imported %s XBlock does not have field %s found in XML.", xblock.scope_ids.block_type, key,
+                    )
 
         if aside_children:
             asides_tags = [x.tag for x in aside_children]
@@ -447,7 +455,7 @@ class XmlMixin:
             if (attr not in self.metadata_to_strip
                     and attr not in self.metadata_to_export_to_policy
                     and attr not in not_to_clean_fields):
-                val = serialize_field(self._field_data.get(self, attr))
+                val = serialize_field(self.fields[attr].to_json(getattr(self, attr)))
                 try:
                     xml_object.set(attr, val)
                 except Exception:  # lint-amnesty, pylint: disable=broad-except