From 1aed4e66afba087abfc40e1234a672725d40555e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 2 Aug 2023 12:24:10 -0700 Subject: [PATCH] [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 | 112 ++++++++--- .../views/tests/test_clipboard_paste.py | 48 ++++- .../content_staging/tests/test_clipboard.py | 178 ++++++++++++------ xmodule/xml_block.py | 66 ++++--- 4 files changed, 286 insertions(+), 118 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 706a202737..c7a39be8f7 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/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, diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index d17745654a..429630ac8d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/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 diff --git a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py index 17d6c5f9af..bc351b6b95 100644 --- a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py +++ b/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(""" Sample ]]> - """).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(), """ + + + """) + + # 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 diff --git a/xmodule/xml_block.py b/xmodule/xml_block.py index 45b7589357..00e396e4da 100644 --- a/xmodule/xml_block.py +++ 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