refactor: move the pointer-tag parsing logic from core.py to utils/helpers.py

Author: ttqureshi

xblock/core.py

@@ -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
@@ -32,85 +33,13 @@
     ("block", "http://code.edx.org/xblock/block"),
 ])
 
-XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8')
 
 # __all__ controls what classes end up in the docs.
 __all__ = ['XBlock', 'XBlockAside']
 
 UNSET = object()
 
 
-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):
-    """
-    Loads definition_xml stored in a dedicated file
-    """
-    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
-
-
-def format_filepath(category, 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, pylint: disable=broad-except
-        # 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
-
-
-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()
-
-
 class _AutoNamedFieldsMetaclass(type):
     """
     Builds classes such that their Field attributes know their own names.

xblock/test/test_core.py

@@ -13,19 +13,10 @@
 
 import ddt
 import pytest
-from lxml import etree
 from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryLocatorV2
 from webob import Response
 
 from xblock.core import Blocklike, XBlock
-from xblock.core import (
-    name_to_pathname,
-    is_pointer_tag,
-    load_definition_xml,
-    format_filepath,
-    file_to_xml,
-    XML_PARSER,
-)
 from xblock.exceptions import (
     XBlockSaveError,
     KeyValueMultiSaveError,
@@ -1172,59 +1163,3 @@ def test_key_properties_when_usage_is_not_an_opaque_key(self):
         block = XBlock(Mock(spec=Runtime), scope_ids=scope_ids)
         self.assertEqual(block.usage_key, "myWeirdOldUsageId")
         self.assertIsNone(block.context_key)
-
-
-class TestCoreFunctions(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.core.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")
-
-    @patch("xblock.core.etree.parse")
-    def test_file_to_xml(self, mock_etree_parse):
-        mock_etree_parse.return_value.getroot.return_value = "mock_xml_root"
-        file_object = Mock()
-        result = file_to_xml(file_object)
-        self.assertEqual(result, "mock_xml_root")
-        mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER)

xblock/test/utils/test_helpers.py

@@ -3,10 +3,20 @@
 """
 
 import unittest
+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,
+    XML_PARSER,
+)
 
 
 # pylint: disable=unnecessary-pass
@@ -78,3 +88,59 @@ 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")
+
+    @patch("xblock.utils.helpers.etree.parse")
+    def test_file_to_xml(self, mock_etree_parse):
+        mock_etree_parse.return_value.getroot.return_value = "mock_xml_root"
+        file_object = Mock()
+        result = file_to_xml(file_object)
+        self.assertEqual(result, "mock_xml_root")
+        mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER)

xblock/utils/helpers.py

@@ -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):
@@ -23,3 +27,74 @@ def child_isinstance(block, child_id, block_class_or_mixin):
     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):
+    """
+    Loads definition_xml stored in a dedicated file
+    """
+    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
+
+
+def format_filepath(category, 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
+        # 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
+
+
+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()