From 374906ccee2b9095e91eeceb49d31c5624934cee Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Fri, 3 Oct 2025 15:45:03 -0400 Subject: [PATCH] Revert "fix: support parsing the pointer-tag OLX for external XBlocks (#37133)" This reverts commit d382721cf854fe02e030c2825ba29144ea95bc45. --- xmodule/conditional_block.py | 16 ++-------------- xmodule/item_bank_block.py | 18 ++---------------- xmodule/modulestore/xml.py | 20 +++----------------- xmodule/tests/xml/__init__.py | 7 +++---- xmodule/vertical_block.py | 18 ++---------------- xmodule/x_module.py | 15 ++++----------- 6 files changed, 16 insertions(+), 78 deletions(-) diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 308d38bc181f..78a5b8c1c1c9 100644 --- a/xmodule/conditional_block.py +++ b/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, is_pointer_tag +from xmodule.xml_block import XmlMixin from xmodule.x_module import ( ResourceTemplates, shim_xmodule_js, @@ -335,19 +335,7 @@ def definition_from_xml(cls, xml_object, system): show_tag_list.append(location) else: try: - 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, - ) + block = system.process_xml(etree.tostring(child, encoding='unicode')) children.append(block.scope_ids.usage_id) except: # lint-amnesty, pylint: disable=bare-except msg = "Unable to load child when parsing Conditional." diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index 3866aac985d4..b53617e2c8e4 100644 --- a/xmodule/item_bank_block.py +++ b/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, is_pointer_tag +from xmodule.xml_block import XmlMixin from xmodule.x_module import ( ResourceTemplates, XModuleMixin, @@ -406,21 +406,7 @@ def definition_from_xml(cls, xml_object, system): for child in xml_object.getchildren(): try: - 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) + children.append(system.process_xml(etree.tostring(child)).scope_ids.usage_id) except (XMLSyntaxError, AttributeError): msg = ( "Unable to load child when parsing {self.usage_key.block_type} Block. " diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py index af28f21ff430..738035b19438 100644 --- a/xmodule/modulestore/xml.py +++ b/xmodule/modulestore/xml.py @@ -65,21 +65,9 @@ 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, def_id=None, def_loaded=False): # lint-amnesty, pylint: disable=too-many-statements - """ - Takes an xml string, and returns an XBlock created from + def process_xml(xml): # lint-amnesty, pylint: disable=too-many-statements + """Takes an xml string, and returns a 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): @@ -171,13 +159,11 @@ def fallback_name(orig_name=None): try: xml_data = etree.fromstring(xml) - if not def_loaded: - make_name_unique(xml_data) + 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: diff --git a/xmodule/tests/xml/__init__.py b/xmodule/tests/xml/__init__.py index 835bc605ddcd..90545f1b88bb 100644 --- a/xmodule/tests/xml/__init__.py +++ b/xmodule/tests/xml/__init__.py @@ -42,14 +42,13 @@ def get_policy(usage_id): ) self.id_generator = Mock() - def process_xml(self, xml, def_id=None, def_loaded=False): # pylint: disable=method-hidden + def process_xml(self, xml): # 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 @@ -62,7 +61,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_id=None, def_loaded=False): + def process_xml(cls, xml_import_data): """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, def_id) + return system.process_xml(xml_import_data.xml_string) diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index 1f078ad8ee21..150e6310853e 100644 --- a/xmodule/vertical_block.py +++ b/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, is_pointer_tag +from xmodule.xml_block import XmlMixin log = logging.getLogger(__name__) @@ -250,21 +250,7 @@ def definition_from_xml(cls, xml_object, system): children = [] for child in xml_object: try: - 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, - ) + child_block = system.process_xml(etree.tostring(child, encoding='unicode')) 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...") diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 22fdddd8ae92..d1d23342b7a9 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1543,20 +1543,16 @@ 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_id=None): + def xblock_from_node(self, node, parent_id, id_generator=None): """ Create an XBlock instance from XML data. Args: - node (RestrictedElement): The :class:`openedx.core.lib.safe_lxml.xmlparser.RestrictedElement` - containing the XML data to parse. + xml_data (string): A string containing valid xml. 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 - 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. + will be used to construct the usage_id and definition_id for the block. Returns: XBlock: The fully instantiated :class:`~xblock.core.XBlock`. @@ -1571,10 +1567,7 @@ def xblock_from_node(self, node, parent_id, id_generator=None, def_id=None): node.attrib.pop('xblock-family', None) url_name = node.get('url_name') # difference from XBlock.runtime - - if not def_id: - def_id = id_generator.create_definition(block_type, url_name) - + 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)