fix: support parsing the pointer-tag OLX for external XBlocks (#37133)

Previously, the built-in XBlocks (problem, video, etc) could be parsed
from pointer tag syntax, but externally-defined XBlocks (drag-and-drop,
ORA, etc.) could only be parsed from inline syntax. This is because the
built-in blocks have special parsing logic, defined in XmlMixin,
which is not available to external blocks.

This PR shifts the pointer tag parsing "up a level" such that the parent
blocks parse the pointer tag, regardless of whether the child is built-in
or external:
* vertical (aka unit)
* split_test (aka content experiment)
* itembank (aka problem bank)
* library_content (aka randomized legacy library)

The following parent blocks still lack support for external pointer-tag children;
we will fix this in a follow-up PR:
* randomize
* all externally defined container blocks

Part of: openedx/XBlock#823

Author: ttqureshi

xmodule/conditional_block.py

@@ -20,7 +20,7 @@
 from xmodule.studio_editable import StudioEditableBlock
 from xmodule.util.builtin_assets import add_webpack_js_to_fragment
 from xmodule.validation import StudioValidation, StudioValidationMessage
-from xmodule.xml_block import XmlMixin
+from xmodule.xml_block import XmlMixin, is_pointer_tag
 from xmodule.x_module import (
     ResourceTemplates,
     shim_xmodule_js,
@@ -335,7 +335,19 @@ def definition_from_xml(cls, xml_object, system):
                     show_tag_list.append(location)
             else:
                 try:
-                    block = system.process_xml(etree.tostring(child, encoding='unicode'))
+                    def_id = None
+                    def_loaded = False
+
+                    if is_pointer_tag(child):
+                        def_id = system.id_generator.create_definition(child.tag, child.get('url_name'))
+                        child, _ = cls.load_definition_xml(child, system, def_id)
+                        def_loaded = True
+
+                    block = system.process_xml(
+                        etree.tostring(child, encoding='unicode'),
+                        def_id,
+                        def_loaded=def_loaded,
+                    )
                     children.append(block.scope_ids.usage_id)
                 except:  # lint-amnesty, pylint: disable=bare-except
                     msg = "Unable to load child when parsing Conditional."

xmodule/item_bank_block.py

@@ -24,7 +24,7 @@
 from xmodule.studio_editable import StudioEditableBlock
 from xmodule.util.builtin_assets import add_webpack_js_to_fragment
 from xmodule.validation import StudioValidation, StudioValidationMessage
-from xmodule.xml_block import XmlMixin
+from xmodule.xml_block import XmlMixin, is_pointer_tag
 from xmodule.x_module import (
     ResourceTemplates,
     XModuleMixin,
@@ -406,7 +406,21 @@ def definition_from_xml(cls, xml_object, system):
 
         for child in xml_object.getchildren():
             try:
-                children.append(system.process_xml(etree.tostring(child)).scope_ids.usage_id)
+                def_id = None
+                def_loaded = False
+
+                if is_pointer_tag(child):
+                    def_id = system.id_generator.create_definition(child.tag, child.get('url_name'))
+                    child, _ = cls.load_definition_xml(child, system, def_id)
+                    def_loaded = True
+
+                child_block = system.process_xml(
+                    etree.tostring(child, encoding='unicode'),
+                    def_id,
+                    def_loaded=def_loaded,
+                )
+
+                children.append(child_block.scope_ids.usage_id)
             except (XMLSyntaxError, AttributeError):
                 msg = (
                     "Unable to load child when parsing {self.usage_key.block_type} Block. "

xmodule/modulestore/xml.py

@@ -65,9 +65,21 @@ def __init__(self, xmlstore, course_id, course_dir,  # lint-amnesty, pylint: dis
         self.load_error_blocks = load_error_blocks
         self.modulestore = xmlstore
 
-        def process_xml(xml):  # lint-amnesty, pylint: disable=too-many-statements
-            """Takes an xml string, and returns a XBlock created from
+        def process_xml(xml, def_id=None, def_loaded=False):  # lint-amnesty, pylint: disable=too-many-statements
+            """
+            Takes an xml string, and returns an XBlock created from
             that xml.
+
+            Args:
+                xml (string): A string containing xml.
+                def_id (BlockUsageLocator): The :class:`BlockUsageLocator` to use as the
+                    definition_id for the block.
+                def_loaded (bool): Indicates if the pointer tag definition is loaded from
+                    from the pointed-to file.
+
+            Returns:
+                XBlock: The fully instantiated :class:`~xblock.core.XBlock`.
+
             """
 
             def make_name_unique(xml_data):
@@ -159,11 +171,13 @@ def fallback_name(orig_name=None):
 
             try:
                 xml_data = etree.fromstring(xml)
-                make_name_unique(xml_data)
+                if not def_loaded:
+                    make_name_unique(xml_data)
                 block = self.xblock_from_node(
                     xml_data,
                     None,  # parent_id
                     id_manager,
+                    def_id,
                 )
             except Exception as err:  # pylint: disable=broad-except
                 if not self.load_error_blocks:

xmodule/tests/xml/__init__.py

@@ -42,13 +42,14 @@ def get_policy(usage_id):
         )
         self.id_generator = Mock()
 
-    def process_xml(self, xml):  # pylint: disable=method-hidden
+    def process_xml(self, xml, def_id=None, def_loaded=False):  # pylint: disable=method-hidden
         """Parse `xml` as an XBlock, and add it to `self._blocks`"""
         self.get_asides = Mock(return_value=[])
         block = self.xblock_from_node(
             etree.fromstring(xml),
             None,
             CourseLocationManager(self.course_id),
+            def_id,
         )
         self._blocks[str(block.location)] = block
         return block
@@ -61,7 +62,7 @@ def load_item(self, location, for_parent=None):  # pylint: disable=method-hidden
 class XModuleXmlImportTest(TestCase):
     """Base class for tests that use basic XML parsing"""
     @classmethod
-    def process_xml(cls, xml_import_data):
+    def process_xml(cls, xml_import_data, def_id=None, def_loaded=False):
         """Use the `xml_import_data` to import an :class:`XBlock` from XML."""
         system = InMemorySystem(xml_import_data)
-        return system.process_xml(xml_import_data.xml_string)
+        return system.process_xml(xml_import_data.xml_string, def_id)

xmodule/vertical_block.py

@@ -23,7 +23,7 @@
 from xmodule.util.builtin_assets import add_webpack_js_to_fragment
 from xmodule.util.misc import is_xblock_an_assignment
 from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW, XModuleFields
-from xmodule.xml_block import XmlMixin
+from xmodule.xml_block import XmlMixin, is_pointer_tag
 
 log = logging.getLogger(__name__)
 
@@ -250,7 +250,21 @@ def definition_from_xml(cls, xml_object, system):
         children = []
         for child in xml_object:
             try:
-                child_block = system.process_xml(etree.tostring(child, encoding='unicode'))
+                def_id = None
+                def_loaded = False
+
+                if is_pointer_tag(child):
+                    id_generator = system.id_generator
+                    def_id = id_generator.create_definition(child.tag, child.get('url_name'))
+
+                    child, _ = cls.load_definition_xml(child, system, def_id)
+                    def_loaded = True
+
+                child_block = system.process_xml(
+                    etree.tostring(child, encoding='unicode'),
+                    def_id,
+                    def_loaded=def_loaded,
+                )
                 children.append(child_block.scope_ids.usage_id)
             except Exception as exc:  # pylint: disable=broad-except
                 log.exception("Unable to load child when parsing Vertical. Continuing...")

xmodule/x_module.py

@@ -1543,16 +1543,20 @@ def _usage_id_from_node(self, node, parent_id):
         """
         return self.xblock_from_node(node, parent_id, self.id_generator).scope_ids.usage_id
 
-    def xblock_from_node(self, node, parent_id, id_generator=None):
+    def xblock_from_node(self, node, parent_id, id_generator=None, def_id=None):
         """
         Create an XBlock instance from XML data.
 
         Args:
-            xml_data (string): A string containing valid xml.
+            node (RestrictedElement): The :class:`openedx.core.lib.safe_lxml.xmlparser.RestrictedElement`
+                containing the XML data to parse.
             system (XMLParsingSystem): The :class:`.XMLParsingSystem` used to connect the block
                 to the outside world.
             id_generator (IdGenerator): An :class:`~xblock.runtime.IdGenerator` that
-                will be used to construct the usage_id and definition_id for the block.
+                will be used to construct the usage_id and definition_id for the block
+                if `def_id` is None.
+            def_id (BlockUsageLocator): The :class:`BlockUsageLocator` to use as the
+                definition_id for the block. If None, a new definition_id will be generated.
 
         Returns:
             XBlock: The fully instantiated :class:`~xblock.core.XBlock`.
@@ -1567,7 +1571,10 @@ def xblock_from_node(self, node, parent_id, id_generator=None):
         node.attrib.pop('xblock-family', None)
 
         url_name = node.get('url_name')  # difference from XBlock.runtime
-        def_id = id_generator.create_definition(block_type, url_name)
+
+        if not def_id:
+            def_id = id_generator.create_definition(block_type, url_name)
+
         usage_id = id_generator.create_usage(def_id)
 
         keys = ScopeIds(None, block_type, def_id, usage_id)