Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions xmodule/modulestore/tests/test_xml.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,71 @@ def test_tilde_files_ignored(self, _fake_glob): # noqa: PT019
about_block = modulestore.get_item(about_location)
assert 'GREEN' in about_block.data
assert 'RED' not in about_block.data


class TestLoadPointerNodeHelper(TestCase):
"""
Unit tests for ``XMLParsingModuleStoreRuntime._load_pointer_node``.

Regression for bug #36390: external XBlocks (which do not inherit
``XmlMixin``) must be able to import pointer-tag OLX like built-in
blocks. The runtime-level helper is the new chokepoint.
"""

def _make_runtime_with_fs(self, fs):
"""Instantiate just the helper without building a full modulestore."""
from xmodule.modulestore.xml import XMLParsingModuleStoreRuntime
runtime = XMLParsingModuleStoreRuntime.__new__(XMLParsingModuleStoreRuntime)
runtime.resources_fs = fs
return runtime

def test_unit_load_pointer_node_reads_referenced_file(self):
"""Pointer node resolves by reading ``<tag>/<url_name>.xml``."""
from fs.memoryfs import MemoryFS
from lxml import etree as _etree

fs = MemoryFS()
fs.makedirs("ext_block")
with fs.open("ext_block/hello.xml", "w") as fp:
fp.write('<ext_block url_name="hello">PAYLOAD</ext_block>')

runtime = self._make_runtime_with_fs(fs)
pointer = _etree.fromstring('<ext_block url_name="hello"/>')
resolved = runtime._load_pointer_node(pointer) # pylint: disable=protected-access
assert resolved.tag == "ext_block"
assert (resolved.text or "").strip() == "PAYLOAD"
assert resolved.get("url_name") == "hello"

def test_unit_load_pointer_node_preserves_url_name_if_missing(self):
"""When the inner file omits url_name, we copy it from the pointer."""
from fs.memoryfs import MemoryFS
from lxml import etree as _etree

fs = MemoryFS()
fs.makedirs("ext_block")
with fs.open("ext_block/foo.xml", "w") as fp:
fp.write('<ext_block>BODY</ext_block>')

runtime = self._make_runtime_with_fs(fs)
pointer = _etree.fromstring('<ext_block url_name="foo"/>')
resolved = runtime._load_pointer_node(pointer) # pylint: disable=protected-access
assert resolved.get("url_name") == "foo"

def test_bug_36390_regression_load_pointer_node_honors_url_name(self):
"""
Regression for bug #36390: an inner file that already declares
``url_name`` must not be clobbered by the pointer's url_name.
"""
from fs.memoryfs import MemoryFS
from lxml import etree as _etree

fs = MemoryFS()
fs.makedirs("ext_block")
with fs.open("ext_block/bar.xml", "w") as fp:
fp.write('<ext_block url_name="inner-name">BODY</ext_block>')

runtime = self._make_runtime_with_fs(fs)
pointer = _etree.fromstring('<ext_block url_name="bar"/>')
resolved = runtime._load_pointer_node(pointer) # pylint: disable=protected-access
# The inner file's url_name wins (it was explicitly set).
assert resolved.get("url_name") == "inner-name"
30 changes: 30 additions & 0 deletions xmodule/modulestore/xml.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,15 @@ def xblock_from_node(self, node, parent_id, id_generator=None):
keys = ScopeIds(None, block_type, def_id, usage_id)
block_class = self.mixologist.mix(self.load_block_type(block_type))

# Resolve pointer-tag syntax at the runtime level so external XBlocks
# (which do not inherit XmlMixin) can be loaded from a separate file
# just like built-in blocks. Classes that already inherit XmlMixin
# handle pointer tags in their own parse_xml, so we skip to avoid a
# double file read. See bug #36390.
from xmodule.xml_block import XmlMixin, is_pointer_tag # noqa: PLC0415
if is_pointer_tag(node) and not issubclass(block_class, XmlMixin):
node = self._load_pointer_node(node)

aside_children = self.parse_asides(node, def_id, usage_id, id_generator)
asides_tags = [x.tag for x in aside_children]

Expand All @@ -107,6 +116,27 @@ def xblock_from_node(self, node, parent_id, id_generator=None):

return block

def _load_pointer_node(self, node):
"""
Resolve a pointer-tag XML node (e.g. ``<drag-and-drop-v2 url_name="foo"/>``)
by loading the referenced file from ``resources_fs``. Returns the
resolved ``lxml.etree`` element.

Used so external XBlocks — which do not inherit ``XmlMixin`` — can be
imported from pointer-tag OLX at the runtime level. See bug #36390.
"""
from lxml import etree as _etree # noqa: PLC0415
from xmodule.xml_block import EDX_XML_PARSER, name_to_pathname # noqa: PLC0415

url_name = node.get("url_name")
filepath = f"{node.tag}/{name_to_pathname(url_name)}.xml"
with self.resources_fs.open(filepath) as xml_file:
resolved = _etree.parse(xml_file, parser=EDX_XML_PARSER).getroot()
# Preserve the url_name from the pointer tag — inner files may omit it.
if "url_name" not in resolved.attrib:
resolved.set("url_name", url_name)
return resolved

def parse_asides(self, node, def_id, usage_id, id_generator):
"""pull the asides out of the xml payload and instantiate them"""
aside_children = []
Expand Down
27 changes: 25 additions & 2 deletions xmodule/x_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -1550,10 +1550,33 @@ def resource_url(self, resource):
raise NotImplementedError("edX Platform doesn't currently implement XBlock resource urls")

def add_block_as_child_node(self, block, node):
"""Append the block’s XML to the given parent XML node."""
"""Append the block's XML to the given parent XML node.

For blocks that do not inherit ``XmlMixin`` (i.e. external XBlocks),
also write the block's definition to a separate file and leave a
pointer tag in the parent, matching the export format used by
built-in blocks. See bug #36390.
"""
from xmodule.xml_block import XmlMixin, name_to_pathname # noqa: PLC0415
child = etree.SubElement(node, block.category)
child.set("url_name", block.url_name)
block.add_xml_to_node(child)
if not isinstance(block, XmlMixin) and hasattr(self, "export_fs"):
# External block: dispatch into a scratch element, then serialize
# that element into its own file, leaving only a pointer tag here.
scratch = etree.Element(block.category)
block.add_xml_to_node(scratch)
url_path = name_to_pathname(block.url_name)
filepath = f"{block.category}/{url_path}.xml"
dirname = os.path.dirname(filepath)
if dirname:
self.export_fs.makedirs(dirname, recreate=True)
with self.export_fs.open(filepath, "wb") as fileobj:
etree.ElementTree(scratch).write(
fileobj, pretty_print=True, encoding="utf-8",
)
# Leave the child node as a pure pointer; url_name is already set.
else:
block.add_xml_to_node(child)

def publish(self, block, event_type, event_data):
"""
Expand Down