Skip to content

Commit fd3b0eb

Browse files
ttqureshittqureshi
committed
refactor: move the pointer tag parsing logic from core.py to utils/helpers.py 
1 parent dd627bc commit fd3b0eb

4 files changed

Lines changed: 143 additions & 138 deletions

File tree

‎xblock/core.py‎

Lines changed: 1 addition & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
from xblock.fields import Field, List, Reference, ReferenceList, Scope, String
2424
from xblock.internal import class_lazy
2525
from xblock.plugin import Plugin
26+
from xblock.utils.helpers import is_pointer_tag, load_definition_xml
2627
from xblock.validation import Validation
2728

2829
# OrderedDict is used so that namespace attributes are put in predictable order
@@ -32,85 +33,13 @@
3233
("block", "http://code.edx.org/xblock/block"),
3334
])
3435

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

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

4040
UNSET = object()
4141

4242

43-
def name_to_pathname(name):
44-
"""
45-
Convert a location name for use in a path: replace ':' with '/'.
46-
This allows users of the xml format to organize content into directories
47-
"""
48-
return name.replace(':', '/')
49-
50-
51-
def is_pointer_tag(xml_obj):
52-
"""
53-
Check if xml_obj is a pointer tag: <blah url_name="something" />.
54-
No children, one attribute named url_name, no text.
55-
56-
Special case for course roots: the pointer is
57-
<course url_name="something" org="myorg" course="course">
58-
59-
xml_obj: an etree Element
60-
61-
Returns a bool.
62-
"""
63-
if xml_obj.tag != "course":
64-
expected_attr = {'url_name'}
65-
else:
66-
expected_attr = {'url_name', 'course', 'org'}
67-
68-
actual_attr = set(xml_obj.attrib.keys())
69-
70-
has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0
71-
72-
return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text
73-
74-
75-
def load_definition_xml(node, runtime, def_id):
76-
"""
77-
Loads definition_xml stored in a dedicated file
78-
"""
79-
url_name = node.get('url_name')
80-
filepath = format_filepath(node.tag, name_to_pathname(url_name))
81-
definition_xml = load_file(filepath, runtime.resources_fs, def_id)
82-
return definition_xml, filepath
83-
84-
85-
def format_filepath(category, name):
86-
return f'{category}/{name}.xml'
87-
88-
89-
def load_file(filepath, fs, def_id): # pylint: disable=invalid-name
90-
"""
91-
Open the specified file in fs, and call cls.file_to_xml on it,
92-
returning the lxml object.
93-
94-
Add details and reraise on error.
95-
"""
96-
try:
97-
with fs.open(filepath) as xml_file:
98-
return file_to_xml(xml_file)
99-
except Exception as err: # lint-amnesty, pylint: disable=broad-except
100-
# Add info about where we are, but keep the traceback
101-
raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err
102-
103-
104-
def file_to_xml(file_object):
105-
"""
106-
Used when this module wants to parse a file object to xml
107-
that will be converted to the definition.
108-
109-
Returns an lxml Element
110-
"""
111-
return etree.parse(file_object, parser=XML_PARSER).getroot()
112-
113-
11443
class _AutoNamedFieldsMetaclass(type):
11544
"""
11645
Builds classes such that their Field attributes know their own names.

‎xblock/test/test_core.py‎

Lines changed: 0 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,10 @@
1313

1414
import ddt
1515
import pytest
16-
from lxml import etree
1716
from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryLocatorV2
1817
from webob import Response
1918

2019
from xblock.core import Blocklike, XBlock
21-
from xblock.core import (
22-
name_to_pathname,
23-
is_pointer_tag,
24-
load_definition_xml,
25-
format_filepath,
26-
file_to_xml,
27-
XML_PARSER,
28-
)
2920
from xblock.exceptions import (
3021
XBlockSaveError,
3122
KeyValueMultiSaveError,
@@ -1172,59 +1163,3 @@ def test_key_properties_when_usage_is_not_an_opaque_key(self):
11721163
block = XBlock(Mock(spec=Runtime), scope_ids=scope_ids)
11731164
self.assertEqual(block.usage_key, "myWeirdOldUsageId")
11741165
self.assertIsNone(block.context_key)
1175-
1176-
1177-
class TestCoreFunctions(unittest.TestCase):
1178-
"""
1179-
Tests for core functions in XBlock.
1180-
"""
1181-
def test_name_to_pathname(self):
1182-
self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse")
1183-
self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part")
1184-
self.assertEqual(name_to_pathname("no_colon"), "no_colon")
1185-
1186-
def test_is_pointer_tag(self):
1187-
# Case 1: Valid pointer tag
1188-
xml_obj = etree.Element("some_tag", url_name="test_url")
1189-
self.assertTrue(is_pointer_tag(xml_obj))
1190-
1191-
# Case 2: Valid course pointer tag
1192-
xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org")
1193-
self.assertTrue(is_pointer_tag(xml_obj))
1194-
1195-
# Case 3: Invalid case - extra attribute
1196-
xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid")
1197-
self.assertFalse(is_pointer_tag(xml_obj))
1198-
1199-
# Case 4: Invalid case - has text
1200-
xml_obj = etree.Element("some_tag", url_name="test_url")
1201-
xml_obj.text = "invalid_text"
1202-
self.assertFalse(is_pointer_tag(xml_obj))
1203-
1204-
# Case 5: Invalid case - has children
1205-
xml_obj = etree.Element("some_tag", url_name="test_url")
1206-
_ = etree.SubElement(xml_obj, "child")
1207-
self.assertFalse(is_pointer_tag(xml_obj))
1208-
1209-
@patch("xblock.core.load_file")
1210-
def test_load_definition_xml(self, mock_load_file):
1211-
mock_load_file.return_value = "<mock_xml />"
1212-
node = etree.Element("course", url_name="test_url")
1213-
runtime = Mock()
1214-
def_id = "mock_id"
1215-
1216-
definition_xml, filepath = load_definition_xml(node, runtime, def_id)
1217-
self.assertEqual(filepath, "course/test_url.xml")
1218-
self.assertEqual(definition_xml, "<mock_xml />")
1219-
mock_load_file.assert_called_once()
1220-
1221-
def test_format_filepath(self):
1222-
self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml")
1223-
1224-
@patch("xblock.core.etree.parse")
1225-
def test_file_to_xml(self, mock_etree_parse):
1226-
mock_etree_parse.return_value.getroot.return_value = "mock_xml_root"
1227-
file_object = Mock()
1228-
result = file_to_xml(file_object)
1229-
self.assertEqual(result, "mock_xml_root")
1230-
mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER)

‎xblock/test/utils/test_helpers.py‎

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,20 @@
33
"""
44

55
import unittest
6+
from unittest.mock import patch, Mock
7+
from lxml import etree
68

79
from xblock.core import XBlock
810
from xblock.test.toy_runtime import ToyRuntime
9-
from xblock.utils.helpers import child_isinstance
11+
from xblock.utils.helpers import (
12+
child_isinstance,
13+
name_to_pathname,
14+
is_pointer_tag,
15+
load_definition_xml,
16+
format_filepath,
17+
file_to_xml,
18+
XML_PARSER,
19+
)
1020

1121

1222
# pylint: disable=unnecessary-pass
@@ -78,3 +88,59 @@ def test_child_isinstance_descendants(self):
7888
self.assertTrue(child_isinstance(root, block.children[1], DogXBlock))
7989
self.assertTrue(child_isinstance(root, block.children[1], GoldenRetrieverXBlock))
8090
self.assertFalse(child_isinstance(root, block.children[1], CatXBlock))
91+
92+
93+
class TestPointerTagParsing(unittest.TestCase):
94+
"""
95+
Tests for core functions in XBlock.
96+
"""
97+
def test_name_to_pathname(self):
98+
self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse")
99+
self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part")
100+
self.assertEqual(name_to_pathname("no_colon"), "no_colon")
101+
102+
def test_is_pointer_tag(self):
103+
# Case 1: Valid pointer tag
104+
xml_obj = etree.Element("some_tag", url_name="test_url")
105+
self.assertTrue(is_pointer_tag(xml_obj))
106+
107+
# Case 2: Valid course pointer tag
108+
xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org")
109+
self.assertTrue(is_pointer_tag(xml_obj))
110+
111+
# Case 3: Invalid case - extra attribute
112+
xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid")
113+
self.assertFalse(is_pointer_tag(xml_obj))
114+
115+
# Case 4: Invalid case - has text
116+
xml_obj = etree.Element("some_tag", url_name="test_url")
117+
xml_obj.text = "invalid_text"
118+
self.assertFalse(is_pointer_tag(xml_obj))
119+
120+
# Case 5: Invalid case - has children
121+
xml_obj = etree.Element("some_tag", url_name="test_url")
122+
_ = etree.SubElement(xml_obj, "child")
123+
self.assertFalse(is_pointer_tag(xml_obj))
124+
125+
@patch("xblock.utils.helpers.load_file")
126+
def test_load_definition_xml(self, mock_load_file):
127+
mock_load_file.return_value = "<mock_xml />"
128+
node = etree.Element("course", url_name="test_url")
129+
runtime = Mock()
130+
def_id = "mock_id"
131+
132+
definition_xml, filepath = load_definition_xml(node, runtime, def_id)
133+
self.assertEqual(filepath, "course/test_url.xml")
134+
self.assertEqual(definition_xml, "<mock_xml />")
135+
mock_load_file.assert_called_once()
136+
137+
def test_format_filepath(self):
138+
self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml")
139+
140+
@patch("xblock.utils.helpers.etree.parse")
141+
def test_file_to_xml(self, mock_etree_parse):
142+
mock_etree_parse.return_value.getroot.return_value = "mock_xml_root"
143+
file_object = Mock()
144+
result = file_to_xml(file_object)
145+
self.assertEqual(result, "mock_xml_root")
146+
mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER)

‎xblock/utils/helpers.py‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
"""
22
Useful helper methods
33
"""
4+
from lxml import etree
5+
6+
7+
XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8')
48

59

610
def child_isinstance(block, child_id, block_class_or_mixin):
@@ -23,3 +27,74 @@ def child_isinstance(block, child_id, block_class_or_mixin):
2327
type_name = block.runtime.id_reader.get_block_type(def_id)
2428
child_class = block.runtime.load_block_type(type_name)
2529
return issubclass(child_class, block_class_or_mixin)
30+
31+
32+
def name_to_pathname(name):
33+
"""
34+
Convert a location name for use in a path: replace ':' with '/'.
35+
This allows users of the xml format to organize content into directories
36+
"""
37+
return name.replace(':', '/')
38+
39+
40+
def is_pointer_tag(xml_obj):
41+
"""
42+
Check if xml_obj is a pointer tag: <blah url_name="something" />.
43+
No children, one attribute named url_name, no text.
44+
45+
Special case for course roots: the pointer is
46+
<course url_name="something" org="myorg" course="course">
47+
48+
xml_obj: an etree Element
49+
50+
Returns a bool.
51+
"""
52+
if xml_obj.tag != "course":
53+
expected_attr = {'url_name'}
54+
else:
55+
expected_attr = {'url_name', 'course', 'org'}
56+
57+
actual_attr = set(xml_obj.attrib.keys())
58+
59+
has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0
60+
61+
return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text
62+
63+
64+
def load_definition_xml(node, runtime, def_id):
65+
"""
66+
Loads definition_xml stored in a dedicated file
67+
"""
68+
url_name = node.get('url_name')
69+
filepath = format_filepath(node.tag, name_to_pathname(url_name))
70+
definition_xml = load_file(filepath, runtime.resources_fs, def_id)
71+
return definition_xml, filepath
72+
73+
74+
def format_filepath(category, name):
75+
return f'{category}/{name}.xml'
76+
77+
78+
def load_file(filepath, fs, def_id): # pylint: disable=invalid-name
79+
"""
80+
Open the specified file in fs, and call cls.file_to_xml on it,
81+
returning the lxml object.
82+
83+
Add details and reraise on error.
84+
"""
85+
try:
86+
with fs.open(filepath) as xml_file:
87+
return file_to_xml(xml_file)
88+
except Exception as err: # lint-amnesty
89+
# Add info about where we are, but keep the traceback
90+
raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err
91+
92+
93+
def file_to_xml(file_object):
94+
"""
95+
Used when this module wants to parse a file object to xml
96+
that will be converted to the definition.
97+
98+
Returns an lxml Element
99+
"""
100+
return etree.parse(file_object, parser=XML_PARSER).getroot()

0 commit comments

Comments
 (0)