diff --git a/cms/djangoapps/contentstore/course_info_model.py b/cms/djangoapps/contentstore/course_info_model.py
index 1013c3939e51..5ecfcb193757 100644
--- a/cms/djangoapps/contentstore/course_info_model.py
+++ b/cms/djangoapps/contentstore/course_info_model.py
@@ -24,7 +24,7 @@
from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order
-# # This should be in a class which inherits from XmlDescriptor
+# # This should be in a class which inherits from XModuleDescriptor
log = logging.getLogger(__name__)
diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py
index bb4feb852d9e..ede041dc5e85 100644
--- a/lms/djangoapps/courseware/tests/test_video_mongo.py
+++ b/lms/djangoapps/courseware/tests/test_video_mongo.py
@@ -1839,9 +1839,10 @@ def test_import_val_data_internal(self):
val_transcript_language_code=val_transcript_language_code,
val_transcript_provider=val_transcript_provider
)
+ xml_object = etree.fromstring(xml_data)
id_generator = Mock()
id_generator.target_course_id = "test_course_id"
- video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+ video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
assert video.edx_video_id == 'test_edx_video_id'
video_data = get_video_info(video.edx_video_id)
@@ -1887,13 +1888,14 @@ def test_import_no_video_id(self):
Test that importing a video with no video id, creates a new external video.
"""
xml_data = """"""
+ xml_object = etree.fromstring(xml_data)
module_system = DummySystem(load_error_modules=True)
id_generator = Mock()
# Verify edx_video_id is empty before.
assert self.descriptor.edx_video_id == ''
- video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+ video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
# Verify edx_video_id is populated after the import.
assert video.edx_video_id != ''
@@ -1923,6 +1925,7 @@ def test_import_val_transcript(self):
val_transcript_language_code=val_transcript_language_code,
val_transcript_provider=val_transcript_provider
)
+ xml_object = etree.fromstring(xml_data)
module_system = DummySystem(load_error_modules=True)
id_generator = Mock()
@@ -1940,7 +1943,7 @@ def test_import_val_transcript(self):
# Verify edx_video_id is empty before.
assert self.descriptor.edx_video_id == ''
- video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+ video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
# Verify edx_video_id is populated after the import.
assert video.edx_video_id != ''
@@ -2077,11 +2080,12 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_
xml_data += val_transcripts
xml_data += ''
+ xml_object = etree.fromstring(xml_data)
# Verify edx_video_id is empty before import.
assert self.descriptor.edx_video_id == ''
- video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+ video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
# Verify edx_video_id is not empty after import.
assert video.edx_video_id != ''
@@ -2107,8 +2111,9 @@ def test_import_val_data_invalid(self):
"""
+ xml_object = etree.fromstring(xml_data)
with pytest.raises(ValCannotCreateError):
- VideoBlock.from_xml(xml_data, module_system, id_generator=Mock())
+ VideoBlock.parse_xml(xml_object, module_system, None, id_generator=Mock())
with pytest.raises(ValVideoNotFoundError):
get_video_info("test_edx_video_id")
diff --git a/openedx/core/djangoapps/olx_rest_api/adapters.py b/openedx/core/djangoapps/olx_rest_api/adapters.py
index a9943fd3ac42..4db3cb03aa34 100644
--- a/openedx/core/djangoapps/olx_rest_api/adapters.py
+++ b/openedx/core/djangoapps/olx_rest_api/adapters.py
@@ -1,22 +1,22 @@
"""
Helpers required to adapt to differing APIs
"""
-from contextlib import contextmanager
import logging
import re
+from contextlib import contextmanager
-from opaque_keys import InvalidKeyError
-from opaque_keys.edx.keys import AssetKey, CourseKey
from fs.memoryfs import MemoryFS
from fs.wrapfs import WrapFS
+from opaque_keys import InvalidKeyError
+from opaque_keys.edx.keys import AssetKey, CourseKey
+from xmodule.assetstore.assetmgr import AssetManager
+from xmodule.contentstore.content import StaticContent
+from xmodule.exceptions import NotFoundError
+from xmodule.modulestore.django import modulestore as store
+from xmodule.modulestore.exceptions import ItemNotFoundError
+from xmodule.xml_module import XmlMixin
from common.djangoapps.static_replace import replace_static_urls
-from xmodule.contentstore.content import StaticContent # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.assetstore.assetmgr import AssetManager # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.modulestore.django import modulestore as store # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.exceptions import NotFoundError # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.xml_module import XmlParserMixin # lint-amnesty, pylint: disable=wrong-import-order
log = logging.getLogger(__name__)
@@ -105,7 +105,7 @@ def override_export_fs(block):
XmlSerializationMixin.add_xml_to_node() method.
This method temporarily replaces a block's runtime's
'export_fs' system with an in-memory filesystem.
- This method also abuses the XmlParserMixin.export_to_file()
+ This method also abuses the XmlMixin.export_to_file()
API to prevent the XModule export code from exporting each
block as two files (one .olx pointing to one .xml file).
The export_to_file was meant to be used only by the
@@ -120,10 +120,10 @@ def override_export_fs(block):
if hasattr(block, 'export_to_file'):
old_export_to_file = block.export_to_file
block.export_to_file = lambda: False
- old_global_export_to_file = XmlParserMixin.export_to_file
- XmlParserMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export
+ old_global_export_to_file = XmlMixin.export_to_file
+ XmlMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export
yield fs
block.runtime.export_fs = old_export_fs
if hasattr(block, 'export_to_file'):
block.export_to_file = old_export_to_file
- XmlParserMixin.export_to_file = old_global_export_to_file
+ XmlMixin.export_to_file = old_global_export_to_file
diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py
index f48ae7d0b887..25f3a78aa75c 100644
--- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py
+++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py
@@ -67,7 +67,7 @@ def get_block(self, usage_id, for_parent=None, block_type_overrides=None): # py
# This is a (former) XModule with messy XML parsing code; let its parse_xml() method continue to work
# as it currently does in the old runtime, but let this parse_xml_new_runtime() method parse the XML in
# a simpler way that's free of tech debt, if defined.
- # In particular, XmlParserMixin doesn't play well with this new runtime, so this is mostly about
+ # In particular, XmlMixin doesn't play well with this new runtime, so this is mostly about
# bypassing that mixin's code.
# When a former XModule no longer needs to support the old runtime, its parse_xml_new_runtime method
# should be removed and its parse_xml() method should be simplified to just call the super().parse_xml()
diff --git a/openedx/core/djangoapps/xblock/runtime/serializer.py b/openedx/core/djangoapps/xblock/runtime/serializer.py
index 9694f6a6e58c..3cf803b4db3a 100644
--- a/openedx/core/djangoapps/xblock/runtime/serializer.py
+++ b/openedx/core/djangoapps/xblock/runtime/serializer.py
@@ -12,7 +12,7 @@
from lxml.etree import Element
from lxml.etree import tostring as etree_tostring
-from xmodule.xml_module import XmlParserMixin
+from xmodule.xml_module import XmlMixin
log = logging.getLogger(__name__)
@@ -104,7 +104,7 @@ def override_export_fs(block):
This method temporarily replaces a block's runtime's 'export_fs' system with
an in-memory filesystem. This method also abuses the
- XmlParserMixin.export_to_file()
+ XmlMixin.export_to_file()
API to prevent the XModule export code from exporting each block as two
files (one .olx pointing to one .xml file). The export_to_file was meant to
be used only by the customtag XModule but it makes our lives here much
@@ -119,8 +119,8 @@ def override_export_fs(block):
if hasattr(block, 'export_to_file'):
old_export_to_file = block.export_to_file
block.export_to_file = lambda: False
- old_global_export_to_file = XmlParserMixin.export_to_file
- XmlParserMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export
+ old_global_export_to_file = XmlMixin.export_to_file
+ XmlMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export
try:
yield fs
except: # lint-amnesty, pylint: disable=try-except-raise
@@ -129,4 +129,4 @@ def override_export_fs(block):
block.runtime.export_fs = old_export_fs
if hasattr(block, 'export_to_file'):
block.export_to_file = old_export_to_file
- XmlParserMixin.export_to_file = old_global_export_to_file
+ XmlMixin.export_to_file = old_global_export_to_file
diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py
index 5f9909c95ddd..e9b57f4058cf 100644
--- a/openedx/core/djangoapps/xblock/runtime/shims.py
+++ b/openedx/core/djangoapps/xblock/runtime/shims.py
@@ -154,7 +154,7 @@ def render_template(self, template_name, dictionary, namespace='main'):
def process_xml(self, xml):
"""
- Code to handle parsing of child XML for old blocks that use XmlParserMixin.
+ Code to handle parsing of child XML for old blocks that use XmlMixin.
"""
# We can't parse XML in a vacuum - we need to know the parent block and/or the
# OLX file that holds this XML in order to generate useful definition keys etc.
diff --git a/xmodule/course_module.py b/xmodule/course_module.py
index d78e871e345d..7d186c60b29c 100644
--- a/xmodule/course_module.py
+++ b/xmodule/course_module.py
@@ -6,7 +6,6 @@
import json
import logging
from datetime import datetime, timedelta
-from io import BytesIO
import dateutil.parser
import requests
@@ -1128,18 +1127,11 @@ def read_grading_policy(cls, paths, system):
return policy_str
@classmethod
- def from_xml(cls, xml_data, system, id_generator):
- instance = super().from_xml(xml_data, system, id_generator)
-
- # bleh, have to parse the XML here to just pull out the url_name attribute
- # I don't think it's stored anywhere in the instance.
- if isinstance(xml_data, str):
- xml_data = xml_data.encode('ascii', 'ignore')
- course_file = BytesIO(xml_data)
- xml_obj = etree.parse(course_file, parser=edx_xml_parser).getroot()
+ def parse_xml(cls, node, runtime, keys, id_generator):
+ instance = super().parse_xml(node, runtime, keys, id_generator)
policy_dir = None
- url_name = xml_obj.get('url_name', xml_obj.get('slug'))
+ url_name = node.get('url_name')
if url_name:
policy_dir = 'policies/' + url_name
@@ -1149,9 +1141,9 @@ def from_xml(cls, xml_data, system, id_generator):
paths = [policy_dir + '/grading_policy.json'] + paths
try:
- policy = json.loads(cls.read_grading_policy(paths, system))
+ policy = json.loads(cls.read_grading_policy(paths, runtime))
except ValueError:
- system.error_tracker("Unable to decode grading policy as json")
+ runtime.error_tracker("Unable to decode grading policy as json")
policy = {}
# now set the current instance. set_grading_policy() will apply some inheritance rules
diff --git a/xmodule/discussion_block.py b/xmodule/discussion_block.py
index 3e0d4ed254f0..63106fa9f258 100644
--- a/xmodule/discussion_block.py
+++ b/xmodule/discussion_block.py
@@ -18,7 +18,7 @@
from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider
from openedx.core.djangolib.markup import HTML, Text
from openedx.core.lib.xblock_utils import get_css_dependencies, get_js_dependencies
-from xmodule.xml_module import XmlParserMixin
+from xmodule.xml_module import XmlMixin
log = logging.getLogger(__name__)
loader = ResourceLoader(__name__) # pylint: disable=invalid-name
@@ -34,7 +34,7 @@ def _(text):
@XBlock.needs('user') # pylint: disable=abstract-method
@XBlock.needs('i18n')
@XBlock.needs('mako')
-class DiscussionXBlock(XBlock, StudioEditableXBlockMixin, XmlParserMixin): # lint-amnesty, pylint: disable=abstract-method
+class DiscussionXBlock(XBlock, StudioEditableXBlockMixin, XmlMixin): # lint-amnesty, pylint: disable=abstract-method
"""
Provides a discussion forum that is inline with other content in the courseware.
"""
diff --git a/xmodule/raw_module.py b/xmodule/raw_module.py
index 8201f0b63b7f..06668e3d460e 100644
--- a/xmodule/raw_module.py
+++ b/xmodule/raw_module.py
@@ -4,7 +4,6 @@
from lxml import etree
from xblock.fields import Scope, String
-from xmodule.xml_module import XmlDescriptor # pylint: disable=unused-import
from .exceptions import SerializationError
diff --git a/xmodule/template_module.py b/xmodule/template_module.py
index 86aadcf16927..86dbe883f316 100644
--- a/xmodule/template_module.py
+++ b/xmodule/template_module.py
@@ -147,19 +147,18 @@ class TranslateCustomTagBlock( # pylint: disable=abstract-method
resources_dir = None
@classmethod
- def from_xml(cls, xml_data, system, id_generator):
+ def parse_xml(cls, node, runtime, _keys, _id_generator):
"""
Transforms the xml_data from <$custom_tag attr="" attr=""/> to
"""
- xml_object = etree.fromstring(xml_data)
- system.error_tracker(Text('WARNING: the <{tag}> tag is deprecated. '
- 'Instead, use . ')
- .format(tag=xml_object.tag))
+ runtime.error_tracker(Text('WARNING: the <{tag}> tag is deprecated. '
+ 'Instead, use . ')
+ .format(tag=node.tag))
- tag = xml_object.tag
- xml_object.tag = 'customtag'
- xml_object.attrib['impl'] = tag
+ tag = node.tag
+ node.tag = 'customtag'
+ node.attrib['impl'] = tag
- return system.process_xml(etree.tostring(xml_object))
+ return runtime.process_xml(etree.tostring(node))
diff --git a/xmodule/tests/test_poll.py b/xmodule/tests/test_poll.py
index ea0f6049db62..a879de121495 100644
--- a/xmodule/tests/test_poll.py
+++ b/xmodule/tests/test_poll.py
@@ -2,14 +2,14 @@
import json
import unittest
-
from unittest.mock import Mock
from opaque_keys.edx.keys import CourseKey
from xblock.field_data import DictFieldData
from xblock.fields import ScopeIds
-from xmodule.poll_module import PollBlock
+from openedx.core.lib.safe_lxml import etree
+from xmodule.poll_module import PollBlock
from . import get_test_system
from .test_import import DummySystem
@@ -29,9 +29,9 @@ def setUp(self):
self.system = get_test_system(course_key)
usage_key = course_key.make_usage_key(PollBlock.category, 'test_loc')
# ScopeIds has 4 fields: user_id, block_type, def_id, usage_id
- scope_ids = ScopeIds(1, PollBlock.category, usage_key, usage_key)
+ self.scope_ids = ScopeIds(1, PollBlock.category, usage_key, usage_key)
self.xmodule = PollBlock(
- self.system, DictFieldData(self.raw_field_data), scope_ids
+ self.system, DictFieldData(self.raw_field_data), self.scope_ids
)
def ajax_request(self, dispatch, data):
@@ -70,8 +70,9 @@ def test_poll_export_with_unescaped_characters_xml(self):
18
'''
+ node = etree.fromstring(sample_poll_xml)
- output = PollBlock.from_xml(sample_poll_xml, module_system, id_generator)
+ output = PollBlock.parse_xml(node, module_system, self.scope_ids, id_generator)
# Update the answer with invalid character.
invalid_characters_poll_answer = output.answers[0]
# Invalid less-than character.
diff --git a/xmodule/tests/test_video.py b/xmodule/tests/test_video.py
index 79e2522ce230..6fcb118130d8 100644
--- a/xmodule/tests/test_video.py
+++ b/xmodule/tests/test_video.py
@@ -282,7 +282,7 @@ def test_constructor(self):
'transcripts': {'ua': 'ukrainian_translation.srt', 'ge': 'german_translation.srt'}
})
- def test_from_xml(self):
+ def test_parse_xml(self):
module_system = DummySystem(load_error_modules=True)
xml_data = '''
'''
- output = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(output, {
'youtube_id_0_75': 'izygArpw-Qo',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -323,7 +324,7 @@ def test_from_xml(self):
('test_org/test_course/test_run', '/c4x/test_org/test_course/asset/test.png')
)
@ddt.unpack
- def test_from_xml_when_handout_is_course_asset(self, course_id_string, expected_handout_link):
+ def test_parse_xml_when_handout_is_course_asset(self, course_id_string, expected_handout_link):
"""
Test that if handout link is course_asset then it will contain targeted course_id in handout link.
"""
@@ -344,10 +345,11 @@ def test_from_xml_when_handout_is_course_asset(self, course_id_string, expected_
'''
+ xml_object = etree.fromstring(xml_data)
id_generator = Mock()
id_generator.target_course_id = course_id
- output = VideoBlock.from_xml(xml_data, module_system, id_generator)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, id_generator)
self.assert_attributes_equal(output, {
'youtube_id_0_75': 'izygArpw-Qo',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -365,7 +367,7 @@ def test_from_xml_when_handout_is_course_asset(self, course_id_string, expected_
'transcripts': {'uk': 'ukrainian_translation.srt', 'de': 'german_translation.srt'},
})
- def test_from_xml_missing_attributes(self):
+ def test_parse_xml_missing_attributes(self):
"""
Ensure that attributes have the right values if they aren't
explicitly set in XML.
@@ -378,7 +380,8 @@ def test_from_xml_missing_attributes(self):
'''
- output = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(output, {
'youtube_id_0_75': '',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -395,7 +398,7 @@ def test_from_xml_missing_attributes(self):
'data': ''
})
- def test_from_xml_missing_download_track(self):
+ def test_parse_xml_missing_download_track(self):
"""
Ensure that attributes have the right values if they aren't
explicitly set in XML.
@@ -409,7 +412,8 @@ def test_from_xml_missing_download_track(self):
'''
- output = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(output, {
'youtube_id_0_75': '',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -426,13 +430,14 @@ def test_from_xml_missing_download_track(self):
'transcripts': {},
})
- def test_from_xml_no_attributes(self):
+ def test_parse_xml_no_attributes(self):
"""
Make sure settings are correct if none are explicitly set in XML.
"""
module_system = DummySystem(load_error_modules=True)
xml_data = ''
- output = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(output, {
'youtube_id_0_75': '',
'youtube_id_1_0': '3_yD_cEKoCk',
@@ -450,7 +455,7 @@ def test_from_xml_no_attributes(self):
'transcripts': {},
})
- def test_from_xml_double_quotes(self):
+ def test_parse_xml_double_quotes(self):
"""
Make sure we can handle the double-quoted string format (which was used for exporting for
a few weeks).
@@ -471,7 +476,8 @@ def test_from_xml_double_quotes(self):
youtube_id_1_0=""OEoXaMPEzf10""
/>
'''
- output = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(output, {
'youtube_id_0_75': 'OEoXaMPEzf65',
'youtube_id_1_0': 'OEoXaMPEzf10',
@@ -488,14 +494,15 @@ def test_from_xml_double_quotes(self):
'data': ''
})
- def test_from_xml_double_quote_concatenated_youtube(self):
+ def test_parse_xml_double_quote_concatenated_youtube(self):
module_system = DummySystem(load_error_modules=True)
xml_data = '''
'''
- output = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(output, {
'youtube_id_0_75': '',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -528,7 +535,8 @@ def test_old_video_format(self):
"""
- output = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ output = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(output, {
'youtube_id_0_75': 'izygArpw-Qo',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -558,7 +566,8 @@ def test_old_video_data(self):
"""
- video = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ video = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(video, {
'youtube_id_0_75': 'izygArpw-Qo',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -588,7 +597,8 @@ def test_import_with_float_times(self):
"""
- video = VideoBlock.from_xml(xml_data, module_system, Mock())
+ xml_object = etree.fromstring(xml_data)
+ video = VideoBlock.parse_xml(xml_object, module_system, None, Mock())
self.assert_attributes_equal(video, {
'youtube_id_0_75': 'izygArpw-Qo',
'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -606,10 +616,10 @@ def test_import_with_float_times(self):
@patch('xmodule.video_module.video_module.edxval_api')
def test_import_val_data(self, mock_val_api):
"""
- Test that `from_xml` works method works as expected.
+ Test that `parse_xml` works method works as expected.
"""
def mock_val_import(xml, edx_video_id, resource_fs, static_dir, external_transcripts, course_id):
- """Mock edxval.api.import_from_xml"""
+ """Mock edxval.api.import_parse_xml"""
assert xml.tag == 'video_asset'
assert dict(list(xml.items())) == {'mock_attr': ''}
assert edx_video_id == 'test_edx_video_id'
@@ -634,9 +644,10 @@ def mock_val_import(xml, edx_video_id, resource_fs, static_dir, external_transcr
""".format(
edx_video_id=edx_video_id
)
+ xml_object = etree.fromstring(xml_data)
id_generator = Mock()
id_generator.target_course_id = 'test_course_id'
- video = VideoBlock.from_xml(xml_data, module_system, id_generator)
+ video = VideoBlock.parse_xml(xml_object, module_system, None, id_generator)
self.assert_attributes_equal(video, {'edx_video_id': edx_video_id})
mock_val_api.import_from_xml.assert_called_once_with(
@@ -660,8 +671,9 @@ def test_import_val_data_invalid(self, mock_val_api):
"""
+ xml_object = etree.fromstring(xml_data)
with pytest.raises(mock_val_api.ValCannotCreateError):
- VideoBlock.from_xml(xml_data, module_system, id_generator=Mock())
+ VideoBlock.parse_xml(xml_object, module_system, None, Mock())
class VideoExportTestCase(VideoBlockTestBase):
diff --git a/xmodule/tests/test_xml_module.py b/xmodule/tests/test_xml_module.py
index 9b4d4fc9103c..09fd7714215d 100644
--- a/xmodule/tests/test_xml_module.py
+++ b/xmodule/tests/test_xml_module.py
@@ -20,7 +20,7 @@
from xmodule.tests.xml import XModuleXmlImportTest
from xmodule.tests.xml.factories import CourseFactory, ProblemFactory, SequenceFactory
from xmodule.x_module import XModuleMixin
-from xmodule.xml_module import XmlDescriptor, deserialize_field, serialize_field
+from xmodule.xml_module import XmlMixin, deserialize_field, serialize_field
class CrazyJsonString(String):
@@ -65,7 +65,7 @@ class InheritingFieldDataTest(unittest.TestCase):
Tests of InheritingFieldData.
"""
- class TestableInheritingXBlock(XmlDescriptor): # lint-amnesty, pylint: disable=abstract-method
+ class TestableInheritingXBlock(XmlMixin): # lint-amnesty, pylint: disable=abstract-method
"""
An XBlock we can use in these tests.
"""
@@ -227,11 +227,15 @@ def test_defaults_not_inherited_across_lib(self):
class EditableMetadataFieldsTest(unittest.TestCase):
+ class TestableXmlXBlock(XmlMixin, XModuleMixin): # lint-amnesty, pylint: disable=abstract-method
+ """
+ This is subclassing `XModuleMixin` to use metadata fields in the unmixed class.
+ """
def test_display_name_field(self):
editable_fields = self.get_xml_editable_fields(DictFieldData({}))
# Tests that the xblock fields (currently tags and name) get filtered out.
- # Also tests that xml_attributes is filtered out of XmlDescriptor.
+ # Also tests that xml_attributes is filtered out of XmlMixin.
assert 1 == len(editable_fields), editable_fields
self.assert_field_values(
editable_fields, 'display_name', XModuleMixin.display_name,
@@ -334,13 +338,13 @@ def test_type_and_options(self):
def get_xml_editable_fields(self, field_data):
runtime = get_test_descriptor_system()
return runtime.construct_xblock_from_class(
- XmlDescriptor,
+ self.TestableXmlXBlock,
scope_ids=Mock(),
field_data=field_data,
).editable_metadata_fields
def get_descriptor(self, field_data):
- class TestModuleDescriptor(TestFields, XmlDescriptor): # lint-amnesty, pylint: disable=abstract-method
+ class TestModuleDescriptor(TestFields, self.TestableXmlXBlock): # lint-amnesty, pylint: disable=abstract-method
@property
def non_editable_metadata_fields(self):
non_editable_fields = super().non_editable_metadata_fields
diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py
index 3efc1fdc1554..cdd2fce32b57 100644
--- a/xmodule/vertical_block.py
+++ b/xmodule/vertical_block.py
@@ -21,7 +21,7 @@
from xmodule.util.misc import is_xblock_an_assignment
from xmodule.util.xmodule_django import add_webpack_to_fragment
from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW, XModuleFields
-from xmodule.xml_module import XmlParserMixin
+from xmodule.xml_module import XmlMixin
log = logging.getLogger(__name__)
@@ -55,7 +55,7 @@ class VerticalBlock(
VerticalFields,
XModuleFields,
StudioEditableBlock,
- XmlParserMixin,
+ XmlMixin,
MakoTemplateBlockBase,
XBlock
):
diff --git a/xmodule/video_module/video_module.py b/xmodule/video_module/video_module.py
index 7ee3b0c0f884..df27c2d7f840 100644
--- a/xmodule/video_module/video_module.py
+++ b/xmodule/video_module/video_module.py
@@ -624,8 +624,7 @@ def editable_metadata_fields(self):
def parse_xml_new_runtime(cls, node, runtime, keys):
"""
Implement the video block's special XML parsing requirements for the
- new runtime only. For all other runtimes, use the existing XModule-style
- methods like .from_xml().
+ new runtime only. For all other runtimes, use .parse_xml().
"""
video_block = runtime.construct_xblock_from_class(cls, keys)
field_data = cls.parse_video_xml(node)
@@ -638,28 +637,24 @@ def parse_xml_new_runtime(cls, node, runtime, keys):
return video_block
@classmethod
- def from_xml(cls, xml_data, system, id_generator):
+ def parse_xml(cls, node, runtime, _keys, id_generator):
"""
- Creates an instance of this descriptor from the supplied xml_data.
- This may be overridden by subclasses
- xml_data: A string of xml that will be translated into data and children for
- this module
- system: A DescriptorSystem for interacting with external resources
- id_generator is used to generate course-specific urls and identifiers
+ Use `node` to construct a new block.
+
+ See XmlMixin.parse_xml for the detailed description.
"""
- xml_object = etree.fromstring(xml_data)
- url_name = xml_object.get('url_name', xml_object.get('slug'))
+ url_name = node.get('url_name')
block_type = 'video'
definition_id = id_generator.create_definition(block_type, url_name)
usage_id = id_generator.create_usage(definition_id)
- if is_pointer_tag(xml_object):
- filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name))
- xml_object = cls.load_file(filepath, system.resources_fs, usage_id)
- system.parse_asides(xml_object, definition_id, usage_id, id_generator)
- field_data = cls.parse_video_xml(xml_object, id_generator)
+ if is_pointer_tag(node):
+ filepath = cls._format_filepath(node.tag, name_to_pathname(url_name))
+ node = cls.load_file(filepath, runtime.resources_fs, usage_id)
+ runtime.parse_asides(node, definition_id, usage_id, id_generator)
+ field_data = cls.parse_video_xml(node, id_generator)
kvs = InheritanceKeyValueStore(initial_values=field_data)
field_data = KvsFieldData(kvs)
- video = system.construct_xblock_from_class(
+ video = runtime.construct_xblock_from_class(
cls,
# We're loading a descriptor, so student_id is meaningless
# We also don't have separate notions of definition and usage ids yet,
@@ -668,10 +663,10 @@ def from_xml(cls, xml_data, system, id_generator):
field_data,
)
- # Update VAL with info extracted from `xml_object`
+ # Update VAL with info extracted from `node`
video.edx_video_id = video.import_video_info_into_val(
- xml_object,
- system.resources_fs,
+ node,
+ runtime.resources_fs,
getattr(id_generator, 'target_course_id', None)
)
diff --git a/xmodule/x_module.py b/xmodule/x_module.py
index 0ae31ddd381a..cd21878409c4 100644
--- a/xmodule/x_module.py
+++ b/xmodule/x_module.py
@@ -279,6 +279,7 @@ class XModuleFields:
)
+@XBlock.needs("i18n")
class XModuleMixin(XModuleFields, XBlock):
"""
Fields and methods used by XModules internally.
@@ -1169,7 +1170,7 @@ 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):
- child = etree.SubElement(node, "unknown")
+ child = etree.SubElement(node, block.category)
child.set('url_name', block.url_name)
block.add_xml_to_node(child)
diff --git a/xmodule/xml_module.py b/xmodule/xml_module.py
index 488d6b1919b6..94b90d89c463 100644
--- a/xmodule/xml_module.py
+++ b/xmodule/xml_module.py
@@ -7,15 +7,13 @@
import os
from lxml import etree
-from lxml.etree import Element, ElementTree, XMLParser
-from xblock.core import XBlock, XML_NAMESPACES
+from lxml.etree import ElementTree, XMLParser
+from xblock.core import XML_NAMESPACES
from xblock.fields import Dict, Scope, ScopeIds
from xblock.runtime import KvsFieldData
from xmodule.modulestore import EdxJSONEncoder
from xmodule.modulestore.inheritance import InheritanceKeyValueStore, own_metadata
-from .x_module import XModuleMixin
-
log = logging.getLogger(__name__)
# assume all XML files are persisted as utf-8.
@@ -103,10 +101,12 @@ def deserialize_field(field, value):
return value
-class XmlParserMixin:
+class XmlMixin:
"""
Class containing XML parsing functionality shared between XBlock and XModuleDescriptor.
"""
+ resources_dir = None
+
# Extension to append to filename paths
filename_extension = 'xml'
@@ -287,7 +287,7 @@ def apply_policy(cls, metadata, policy):
metadata[attr] = value
@classmethod
- def parse_xml(cls, node, runtime, keys, id_generator): # pylint: disable=unused-argument
+ def parse_xml(cls, node, runtime, _keys, id_generator):
"""
Use `node` to construct a new block.
@@ -296,8 +296,8 @@ def parse_xml(cls, node, runtime, keys, id_generator): # pylint: disable=unused
runtime (:class:`.Runtime`): The runtime to use while parsing.
- keys (:class:`.ScopeIds`): The keys identifying where this block
- will store its data.
+ _keys (:class:`.ScopeIds`): The keys identifying where this block
+ will store its data. Not used by this implementation.
id_generator (:class:`.IdGenerator`): An object that will allow the
runtime to generate correct definition and usage ids for
@@ -306,8 +306,7 @@ def parse_xml(cls, node, runtime, keys, id_generator): # pylint: disable=unused
Returns (XBlock): The newly parsed XBlock
"""
- # VS[compat] -- just have the url_name lookup, once translation is done
- url_name = cls._get_url_name(node)
+ url_name = node.get('url_name')
def_id = id_generator.create_definition(node.tag, url_name)
usage_id = id_generator.create_usage(def_id)
aside_children = []
@@ -386,19 +385,12 @@ def parse_xml_new_runtime(cls, node, runtime, keys):
except AttributeError:
return super().parse_xml(node, runtime, keys, id_generator=None)
- @classmethod
- def _get_url_name(cls, node):
- """
- Reads url_name attribute from the node
- """
- return node.get('url_name', node.get('slug'))
-
@classmethod
def load_definition_xml(cls, node, runtime, def_id):
"""
Loads definition_xml stored in a dedicated file
"""
- url_name = cls._get_url_name(node)
+ url_name = node.get('url_name')
filepath = cls._format_filepath(node.tag, name_to_pathname(url_name))
definition_xml = cls.load_file(filepath, runtime.resources_fs, def_id)
return definition_xml, filepath
@@ -480,7 +472,9 @@ def add_xml_to_node(self, node):
node.attrib.update(xml_object.attrib)
node.extend(xml_object)
- node.set('url_name', self.url_name)
+ # Do not override an existing value for the course.
+ if not node.get('url_name'):
+ node.set('url_name', self.url_name)
# Special case for course pointers:
if self.category == 'course':
@@ -501,110 +495,5 @@ def non_editable_metadata_fields(self):
Return a list of all metadata fields that cannot be edited.
"""
non_editable_fields = super().non_editable_metadata_fields
- non_editable_fields.append(XmlParserMixin.xml_attributes)
+ non_editable_fields.append(XmlMixin.xml_attributes)
return non_editable_fields
-
-
-class XmlMixin(XmlParserMixin): # lint-amnesty, pylint: disable=abstract-method
- """
- Mixin class for standardized parsing of XModule xml.
- """
- resources_dir = None
-
- @classmethod
- def from_xml(cls, xml_data, system, id_generator):
- """
- Creates an instance of this descriptor from the supplied xml_data.
- This may be overridden by subclasses.
-
- Args:
- xml_data (str): A string of xml that will be translated into data and children
- for this module
-
- system (:class:`.XMLParsingSystem):
-
- id_generator (:class:`xblock.runtime.IdGenerator`): Used to generate the
- usage_ids and definition_ids when loading this xml
-
- """
- # Shim from from_xml to the parse_xml defined in XmlParserMixin.
- # This only exists to satisfy subclasses that both:
- # a) define from_xml themselves
- # b) call super(..).from_xml(..)
- return super().parse_xml(
- etree.fromstring(xml_data),
- system,
- None, # This is ignored by XmlParserMixin
- id_generator,
- )
-
- @classmethod
- def parse_xml(cls, node, runtime, keys, id_generator):
- """
- Interpret the parsed XML in `node`, creating an XModuleDescriptor.
- """
- if cls.from_xml != XmlMixin.from_xml:
- xml = etree.tostring(node).decode('utf-8')
- block = cls.from_xml(xml, runtime, id_generator)
- return block
- else:
- return super().parse_xml(node, runtime, keys, id_generator)
-
- @classmethod
- def parse_xml_new_runtime(cls, node, runtime, keys):
- """
- This XML lives within Blockstore and the new runtime doesn't need this
- legacy XModule code. Use the "normal" XBlock parsing code.
- """
- try:
- return super().parse_xml_new_runtime(node, runtime, keys)
- except AttributeError:
- return super().parse_xml(node, runtime, keys, id_generator=None)
-
- def export_to_xml(self, resource_fs): # lint-amnesty, pylint: disable=unused-argument
- """
- Returns an xml string representing this module, and all modules
- underneath it. May also write required resources out to resource_fs.
-
- Assumes that modules have single parentage (that no module appears twice
- in the same course), and that it is thus safe to nest modules as xml
- children as appropriate.
-
- The returned XML should be able to be parsed back into an identical
- XModuleDescriptor using the from_xml method with the same system, org,
- and course
- """
- # Shim from export_to_xml to the add_xml_to_node defined in XmlParserMixin.
- # This only exists to satisfy subclasses that both:
- # a) define export_to_xml themselves
- # b) call super(..).export_to_xml(..)
- node = Element(self.category)
- super().add_xml_to_node(node)
- return etree.tostring(node)
-
- def add_xml_to_node(self, node):
- """
- Export this :class:`XModuleDescriptor` as XML, by setting attributes on the provided
- `node`.
- """
- if self.export_to_xml != XmlMixin.export_to_xml: # lint-amnesty, pylint: disable=comparison-with-callable
- xml_string = self.export_to_xml(self.runtime.export_fs)
- exported_node = etree.fromstring(xml_string)
- node.tag = exported_node.tag
- node.text = exported_node.text
- node.tail = exported_node.tail
-
- for key, value in exported_node.items():
- if key == 'url_name' and value == 'course' and key in node.attrib:
- # if url_name is set in ExportManager then do not override it here.
- continue
- node.set(key, value)
-
- node.extend(list(exported_node))
- else:
- super().add_xml_to_node(node)
-
-
-@XBlock.needs("i18n")
-class XmlDescriptor(XmlMixin, XModuleMixin): # lint-amnesty, pylint: disable=abstract-method
- pass