Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 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
5 changes: 5 additions & 0 deletions xblock/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from xblock.fields import Field, List, Reference, ReferenceList, Scope, String
from xblock.internal import class_lazy
from xblock.plugin import Plugin
from xblock.utils.helpers import is_pointer_tag, load_definition_xml
from xblock.validation import Validation

# OrderedDict is used so that namespace attributes are put in predictable order
Expand All @@ -32,6 +33,7 @@
("block", "http://code.edx.org/xblock/block"),
])


# __all__ controls what classes end up in the docs.
__all__ = ['XBlock', 'XBlockAside']

Expand Down Expand Up @@ -746,6 +748,9 @@
keys (:class:`.ScopeIds`): The keys identifying where this block
will store its data.
"""
if is_pointer_tag(node):
node, _ = load_definition_xml(node, runtime, keys.def_id)

Check warning on line 752 in xblock/core.py

View check run for this annotation

Codecov / codecov/patch

xblock/core.py#L752

Added line #L752 was not covered by tests

block = runtime.construct_xblock_from_class(cls, keys)

# The base implementation: child nodes become child blocks.
Expand Down
73 changes: 72 additions & 1 deletion xblock/test/utils/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,20 @@
"""

import unittest
from io import BytesIO
from unittest.mock import patch, Mock
from lxml import etree

from xblock.core import XBlock
from xblock.test.toy_runtime import ToyRuntime
from xblock.utils.helpers import child_isinstance
from xblock.utils.helpers import (
child_isinstance,
name_to_pathname,
is_pointer_tag,
load_definition_xml,
format_filepath,
file_to_xml,
)


# pylint: disable=unnecessary-pass
Expand Down Expand Up @@ -78,3 +88,64 @@ def test_child_isinstance_descendants(self):
self.assertTrue(child_isinstance(root, block.children[1], DogXBlock))
self.assertTrue(child_isinstance(root, block.children[1], GoldenRetrieverXBlock))
self.assertFalse(child_isinstance(root, block.children[1], CatXBlock))


class TestPointerTagParsing(unittest.TestCase):
"""
Tests for core functions in XBlock.
"""
def test_name_to_pathname(self):
self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse")
self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part")
self.assertEqual(name_to_pathname("no_colon"), "no_colon")

def test_is_pointer_tag(self):
# Case 1: Valid pointer tag
xml_obj = etree.Element("some_tag", url_name="test_url")
self.assertTrue(is_pointer_tag(xml_obj))

# Case 2: Valid course pointer tag
xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org")
self.assertTrue(is_pointer_tag(xml_obj))

# Case 3: Invalid case - extra attribute
xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid")
self.assertFalse(is_pointer_tag(xml_obj))

# Case 4: Invalid case - has text
xml_obj = etree.Element("some_tag", url_name="test_url")
xml_obj.text = "invalid_text"
self.assertFalse(is_pointer_tag(xml_obj))

# Case 5: Invalid case - has children
xml_obj = etree.Element("some_tag", url_name="test_url")
_ = etree.SubElement(xml_obj, "child")
self.assertFalse(is_pointer_tag(xml_obj))

@patch("xblock.utils.helpers.load_file")
def test_load_definition_xml(self, mock_load_file):
mock_load_file.return_value = "<mock_xml />"
node = etree.Element("course", url_name="test_url")
runtime = Mock()
def_id = "mock_id"

definition_xml, filepath = load_definition_xml(node, runtime, def_id)
self.assertEqual(filepath, "course/test_url.xml")
self.assertEqual(definition_xml, "<mock_xml />")
mock_load_file.assert_called_once()

def test_format_filepath(self):
self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml")

def test_file_to_xml(self):
"""Test that `file_to_xml` correctly parses XML from a file object."""
# Create a BytesIO object
file_obj = BytesIO(b"<root><child>Value</child></root>")

# Parse the XML
result = file_to_xml(file_obj)

# Verify the result
self.assertEqual(result.tag, 'root')
self.assertEqual(result[0].tag, 'child')
self.assertEqual(result[0].text, 'Value')
88 changes: 88 additions & 0 deletions xblock/utils/helpers.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
"""
Useful helper methods
"""
from lxml import etree


XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8')


def child_isinstance(block, child_id, block_class_or_mixin):
Expand All @@ -23,3 +27,87 @@
type_name = block.runtime.id_reader.get_block_type(def_id)
child_class = block.runtime.load_block_type(type_name)
return issubclass(child_class, block_class_or_mixin)


def name_to_pathname(name):
"""
Convert a location name for use in a path: replace ':' with '/'.
This allows users of the xml format to organize content into directories
"""
return name.replace(':', '/')


def is_pointer_tag(xml_obj):
"""
Check if xml_obj is a pointer tag: <blah url_name="something" />.
No children, one attribute named url_name, no text.

Special case for course roots: the pointer is
<course url_name="something" org="myorg" course="course">

xml_obj: an etree Element

Returns a bool.
"""
if xml_obj.tag != "course":
expected_attr = {'url_name'}
else:
expected_attr = {'url_name', 'course', 'org'}

actual_attr = set(xml_obj.attrib.keys())

has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0

return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text


def load_definition_xml(node, runtime, def_id):
"""
Parses and loads an XML definition file based on a given node, runtime
environment, and definition ID.

Arguments:
node: XML element containing attributes for definition loading.
runtime: The runtime environment that provides resource access.
def_id: Unique identifier for the definition being loaded.

Returns:
tuple: A tuple containing the loaded XML definition and the
corresponding file path.
"""
url_name = node.get('url_name')
filepath = format_filepath(node.tag, name_to_pathname(url_name))
definition_xml = load_file(filepath, runtime.resources_fs, def_id)
return definition_xml, filepath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work in xblock-sdk as well, or is this logic specific to edx-platform?

If it is specific to edx-platform, then we want want to go through a Runtime method instead, so that edx-platform and xblock-sdk can implement it differently.

Copy link
Member Author

@ttqureshi ttqureshi May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work in xblock-sdk as well, or is this logic specific to edx-platform?

If it is specific to edx-platform, then we want want to go through a Runtime method instead, so that edx-platform and xblock-sdk can implement it differently.

@kdmccormick
I've added the load_definition_xml function to the Runtime class so that it can be optionally overridden by other Runtimes like xblock-sdk. I haven't made it abstract because, as of now, xblock-sdk does not have a course import mechanism that handles the 'pointer-tag' syntax in XML definitions. Making it abstract would unnecessarily enforce implementation where it's not currently needed.



def format_filepath(category, name):
"""
Construct a formatted filepath string based on the given category and name.
"""
return f'{category}/{name}.xml'


def load_file(filepath, fs, def_id): # pylint: disable=invalid-name
"""
Open the specified file in fs, and call cls.file_to_xml on it,
returning the lxml object.

Add details and reraise on error.
"""
try:
with fs.open(filepath) as xml_file:
return file_to_xml(xml_file)
except Exception as err: # lint-amnesty

Check warning on line 101 in xblock/utils/helpers.py

View check run for this annotation

Codecov / codecov/patch

xblock/utils/helpers.py#L98-L101

Added lines #L98 - L101 were not covered by tests
# Add info about where we are, but keep the traceback
raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err

Check warning on line 103 in xblock/utils/helpers.py

View check run for this annotation

Codecov / codecov/patch

xblock/utils/helpers.py#L103

Added line #L103 was not covered by tests


def file_to_xml(file_object):
"""
Used when this module wants to parse a file object to xml
that will be converted to the definition.

Returns an lxml Element
"""
return etree.parse(file_object, parser=XML_PARSER).getroot()