diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 8fe9c67554..325d98cc5f 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -149,7 +149,7 @@ class ABTestDescriptor(ABTestFields, RawDescriptor, XmlDescriptor): for child_loc in group: child = self.system.load_item(child_loc) - group_elem.append(etree.fromstring(child.export_to_xml(resource_fs))) + self.runtime.add_block_as_child_node(child, group_elem) return xml_object diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index e296af5aa5..58f7dc5bce 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/common/lib/xmodule/xmodule/conditional_module.py @@ -222,9 +222,9 @@ class ConditionalDescriptor(ConditionalFields, SequenceDescriptor): show_tag_list = [] for child in xml_object: if child.tag == 'show': - location = ConditionalDescriptor.parse_sources(child, system) - children.extend(location) - show_tag_list.extend(location.url()) # pylint: disable=no-member + locations = ConditionalDescriptor.parse_sources(child, system) + children.extend(locations) + show_tag_list.extend(location.url() for location in locations) # pylint: disable=no-member else: try: descriptor = system.process_xml(etree.tostring(child)) @@ -244,6 +244,5 @@ class ConditionalDescriptor(ConditionalFields, SequenceDescriptor): tag_name='show', sources=location) xml_object.append(etree.fromstring(show_str)) else: - xml_object.append( - etree.fromstring(child.export_to_xml(resource_fs))) + self.runtime.add_block_as_child_node(child, xml_object) return xml_object diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index d2c6224112..0ccd7d6613 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -400,6 +400,5 @@ class CrowdsourceHinterDescriptor(CrowdsourceHinterFields, RawDescriptor): def definition_to_xml(self, resource_fs): xml_object = etree.Element('crowdsource_hinter') for child in self.get_children(): - xml_object.append( - etree.fromstring(child.export_to_xml(resource_fs))) + self.runtime.add_block_as_child_node(child, xml_object) return xml_object diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index e9360a53de..c78b8d240f 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -3,6 +3,7 @@ Methods for exporting course data to XML """ import logging +import lxml.etree from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata from fs.osfs import OSFS @@ -57,11 +58,13 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d course = modulestore.get_course(course_id) fs = OSFS(root_dir) - export_fs = fs.makeopendir(course_dir) + export_fs = course.runtime.export_fs = fs.makeopendir(course_dir) + + root = lxml.etree.Element('unknown') + course.add_xml_to_node(root) - xml = course.export_to_xml(export_fs) with export_fs.open('course.xml', 'w') as course_xml: - course_xml.write(xml) + lxml.etree.ElementTree(root).write(course_xml) # export the static assets policies_dir = export_fs.makeopendir('policies') @@ -112,7 +115,9 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d sequential = modulestore.get_item(Location(parent_locs[0])) index = sequential.children.index(draft_vertical.location.url()) draft_vertical.xml_attributes['index_in_children_list'] = str(index) - draft_vertical.export_to_xml(draft_course_dir) + draft_vertical.runtime.export_fs = draft_course_dir + node = lxml.etree.Element('unknown') + draft_vertical.add_xml_to_node(node) def export_extra_content(export_fs, modulestore, course_id, course_location, category_type, dirname, file_suffix=''): diff --git a/common/lib/xmodule/xmodule/randomize_module.py b/common/lib/xmodule/xmodule/randomize_module.py index 56b7c079aa..30febac9b2 100644 --- a/common/lib/xmodule/xmodule/randomize_module.py +++ b/common/lib/xmodule/xmodule/randomize_module.py @@ -100,8 +100,7 @@ class RandomizeDescriptor(RandomizeFields, SequenceDescriptor): xml_object = etree.Element('randomize') for child in self.get_children(): - xml_object.append( - etree.fromstring(child.export_to_xml(resource_fs))) + self.runtime.add_block_as_child_node(child, xml_object) return xml_object def has_dynamic_children(self): diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 4387d6286b..51aa2d4261 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -150,6 +150,5 @@ class SequenceDescriptor(SequenceFields, MakoModuleDescriptor, XmlDescriptor): def definition_to_xml(self, resource_fs): xml_object = etree.Element('sequential') for child in self.get_children(): - xml_object.append( - etree.fromstring(child.export_to_xml(resource_fs))) + self.runtime.add_block_as_child_node(child, xml_object) return xml_object diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index a1fe66af39..dacc433bee 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -2,19 +2,25 @@ Tests of XML export """ -from datetime import datetime, timedelta, tzinfo -from tempfile import mkdtemp -import unittest -import shutil -from textwrap import dedent +import ddt +import lxml.etree import mock - +import os import pytz +import shutil +import tarfile +import unittest +import uuid + +from datetime import datetime, timedelta, tzinfo from fs.osfs import OSFS from path import path -import uuid -import tarfile -import os +from tempfile import mkdtemp +from textwrap import dedent + +from xblock.core import XBlock +from xblock.fields import String, Scope, Integer +from xblock.test.tools import blocks_are_equivalent from xmodule.modulestore import Location from xmodule.modulestore.xml import XMLModuleStore @@ -23,6 +29,7 @@ from xmodule.modulestore.xml_exporter import ( ) from xmodule.tests import DATA_DIR from xmodule.tests.helpers import directories_equal +from xmodule.x_module import XModuleMixin def strip_filenames(descriptor): @@ -43,6 +50,16 @@ def strip_filenames(descriptor): descriptor.save() +class PureXBlock(XBlock): + + """Class for testing pure XBlocks.""" + + has_children = True + field1 = String(default="something", scope=Scope.user_state) + field2 = Integer(scope=Scope.user_state) + + +@ddt.ddt class RoundTripTestCase(unittest.TestCase): """ Check that our test courses roundtrip properly. @@ -51,8 +68,25 @@ class RoundTripTestCase(unittest.TestCase): Thus we make sure that export and import work properly. """ + + def setUp(self): + self.maxDiff = None + self.temp_dir = mkdtemp() + self.addCleanup(shutil.rmtree, self.temp_dir) + @mock.patch('xmodule.course_module.requests.get') - def check_export_roundtrip(self, data_dir, course_dir, mock_get): + @ddt.data( + "toy", + "simple", + "conditional_and_poll", + "self_assessment", + "graphic_slider_tool", + "test_exam_registration", + "word_cloud", + "pure_xblock", + ) + @XBlock.register_temp_plugin(PureXBlock, 'pure') + def test_export_roundtrip(self, course_dir, mock_get): # Patch network calls to retrieve the textbook TOC mock_get.return_value.text = dedent(""" @@ -64,11 +98,11 @@ class RoundTripTestCase(unittest.TestCase): root_dir = path(self.temp_dir) print("Copying test course to temp dir {0}".format(root_dir)) - data_dir = path(data_dir) + data_dir = path(DATA_DIR) shutil.copytree(data_dir / course_dir, root_dir / course_dir) print("Starting import") - initial_import = XMLModuleStore(root_dir, course_dirs=[course_dir]) + initial_import = XMLModuleStore(root_dir, course_dirs=[course_dir], xblock_mixins=(XModuleMixin,)) courses = initial_import.get_courses() self.assertEquals(len(courses), 1) @@ -78,14 +112,15 @@ class RoundTripTestCase(unittest.TestCase): # will still be there. print("Starting export") fs = OSFS(root_dir) - export_fs = fs.makeopendir(course_dir) + initial_course.runtime.export_fs = fs.makeopendir(course_dir) + root = lxml.etree.Element('root') - xml = initial_course.export_to_xml(export_fs) - with export_fs.open('course.xml', 'w') as course_xml: - course_xml.write(xml) + initial_course.add_xml_to_node(root) + with initial_course.runtime.export_fs.open('course.xml', 'w') as course_xml: + lxml.etree.ElementTree(root).write(course_xml) print("Starting second import") - second_import = XMLModuleStore(root_dir, course_dirs=[course_dir]) + second_import = XMLModuleStore(root_dir, course_dirs=[course_dir], xblock_mixins=(XModuleMixin,)) courses2 = second_import.get_courses() self.assertEquals(len(courses2), 1) @@ -98,48 +133,24 @@ class RoundTripTestCase(unittest.TestCase): strip_filenames(initial_course) strip_filenames(exported_course) - self.assertEquals(initial_course, exported_course) + self.assertTrue(blocks_are_equivalent(initial_course, exported_course)) self.assertEquals(initial_course.id, exported_course.id) course_id = initial_course.id print("Checking key equality") - self.assertEquals(sorted(initial_import.modules[course_id].keys()), - sorted(second_import.modules[course_id].keys())) + self.assertItemsEqual( + initial_import.modules[course_id].keys(), + second_import.modules[course_id].keys() + ) print("Checking module equality") for location in initial_import.modules[course_id].keys(): print("Checking", location) - self.assertEquals(initial_import.modules[course_id][location], - second_import.modules[course_id][location]) + self.assertTrue(blocks_are_equivalent( + initial_import.modules[course_id][location], + second_import.modules[course_id][location] + )) - def setUp(self): - self.maxDiff = None - self.temp_dir = mkdtemp() - self.addCleanup(shutil.rmtree, self.temp_dir) - - def test_toy_roundtrip(self): - self.check_export_roundtrip(DATA_DIR, "toy") - - def test_simple_roundtrip(self): - self.check_export_roundtrip(DATA_DIR, "simple") - - def test_conditional_and_poll_roundtrip(self): - self.check_export_roundtrip(DATA_DIR, "conditional_and_poll") - - def test_selfassessment_roundtrip(self): - #Test selfassessment xmodule to see if it exports correctly - self.check_export_roundtrip(DATA_DIR, "self_assessment") - - def test_graphicslidertool_roundtrip(self): - #Test graphicslidertool xmodule to see if it exports correctly - self.check_export_roundtrip(DATA_DIR, "graphic_slider_tool") - - def test_exam_registration_roundtrip(self): - # Test exam_registration xmodule to see if it exports correctly - self.check_export_roundtrip(DATA_DIR, "test_exam_registration") - - def test_word_cloud_roundtrip(self): - self.check_export_roundtrip(DATA_DIR, "word_cloud") class TestEdxJsonEncoder(unittest.TestCase): diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 12f889d897..0f0aa80145 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -158,9 +158,9 @@ class ImportTestCase(BaseCourseTestCase): system = self.get_system() descriptor = system.process_xml(bad_xml) - resource_fs = None - tag_xml = descriptor.export_to_xml(resource_fs) - re_import_descriptor = system.process_xml(tag_xml) + node = etree.Element('unknown') + descriptor.add_xml_to_node(node) + re_import_descriptor = system.process_xml(etree.tostring(node)) self.assertEqual(re_import_descriptor.__class__.__name__, 'ErrorDescriptorWithMixins') @@ -182,12 +182,11 @@ class ImportTestCase(BaseCourseTestCase): descriptor = system.process_xml(xml_str_in) # export it - resource_fs = None - xml_str_out = descriptor.export_to_xml(resource_fs) + node = etree.Element('unknown') + descriptor.add_xml_to_node(node) # Now make sure the exported xml is a sequential - xml_out = etree.fromstring(xml_str_out) - self.assertEqual(xml_out.tag, 'sequential') + self.assertEqual(node.tag, 'sequential') def test_metadata_import_export(self): """Two checks: @@ -221,19 +220,19 @@ class ImportTestCase(BaseCourseTestCase): ) # Now export and check things - resource_fs = MemoryFS() - exported_xml = descriptor.export_to_xml(resource_fs) + descriptor.runtime.export_fs = MemoryFS() + node = etree.Element('unknown') + descriptor.add_xml_to_node(node) # Check that the exported xml is just a pointer - print("Exported xml:", exported_xml) - pointer = etree.fromstring(exported_xml) - self.assertTrue(is_pointer_tag(pointer)) + print("Exported xml:", etree.tostring(node)) + self.assertTrue(is_pointer_tag(node)) # but it's a special case course pointer - self.assertEqual(pointer.attrib['course'], COURSE) - self.assertEqual(pointer.attrib['org'], ORG) + self.assertEqual(node.attrib['course'], COURSE) + self.assertEqual(node.attrib['org'], ORG) # Does the course still have unicorns? - with resource_fs.open('course/{url_name}.xml'.format(url_name=url_name)) as f: + with descriptor.runtime.export_fs.open('course/{url_name}.xml'.format(url_name=url_name)) as f: course_xml = etree.fromstring(f.read()) self.assertEqual(course_xml.attrib['unicorn'], 'purple') @@ -247,7 +246,7 @@ class ImportTestCase(BaseCourseTestCase): # Does the chapter tag now have a due attribute? # hardcoded path to child - with resource_fs.open('chapter/ch.xml') as f: + with descriptor.runtime.export_fs.open('chapter/ch.xml') as f: chapter_xml = etree.fromstring(f.read()) self.assertEqual(chapter_xml.tag, 'chapter') self.assertFalse('due' in chapter_xml.attrib) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 9fe208fd04..9acc686208 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -222,6 +222,7 @@ class XModuleMixin(XBlockMixin): for child_loc in self.children: try: child = self.runtime.get_block(child_loc) + child.runtime.export_fs = self.runtime.export_fs except ItemNotFoundError: log.exception(u'Unable to load item {loc}, skipping'.format(loc=child_loc)) continue @@ -685,6 +686,21 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): """ raise NotImplementedError('Modules must implement from_xml to be parsable from xml') + def add_xml_to_node(self, node): + """ + Export this :class:`XModuleDescriptor` as XML, by setting attributes on the provided + `node`. + """ + 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(): + node.set(key, value) + + node.extend(list(exported_node)) + def export_to_xml(self, resource_fs): """ Returns an xml string representing this module, and all modules @@ -926,6 +942,9 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable """ super(DescriptorSystem, self).__init__(**kwargs) + # This is used by XModules to write out separate files during xml export + self.export_fs = None + self.load_item = load_item self.resources_fs = resources_fs self.error_tracker = error_tracker @@ -996,6 +1015,11 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable def publish(self, block, event): raise NotImplementedError("edX Platform doesn't currently implement XBlock publish") + def add_block_as_child_node(self, block, node): + child = etree.SubElement(node, "unknown") + child.set('url_name', block.url_name) + block.add_xml_to_node(child) + class XMLParsingSystem(DescriptorSystem): def __init__(self, process_xml, **kwargs): diff --git a/common/test/data/pure_xblock/course.xml b/common/test/data/pure_xblock/course.xml new file mode 100644 index 0000000000..b3d829ad02 --- /dev/null +++ b/common/test/data/pure_xblock/course.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/lms/djangoapps/courseware/management/commands/clean_xml.py b/lms/djangoapps/courseware/management/commands/clean_xml.py index 45674f66e0..dd61ff36a2 100644 --- a/lms/djangoapps/courseware/management/commands/clean_xml.py +++ b/lms/djangoapps/courseware/management/commands/clean_xml.py @@ -1,3 +1,4 @@ +import lxml.etree import os import sys import traceback @@ -30,9 +31,11 @@ def export(course, export_dir): ' May clobber/confuse things'.format(dir=export_dir)) try: - xml = course.export_to_xml(fs) + course.runtime.export_fs = fs + root = lxml.etree.Element('root') + course.add_xml_to_node(root) with fs.open('course.xml', mode='w') as f: - f.write(xml) + root.write(f) return True except: diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 06358f427e..61ef871263 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -15,7 +15,7 @@ -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@de92d3bf798699a6bbd06b54012ef15934c41ac0#egg=XBlock +-e git+https://github.com/edx/XBlock.git@3830ee50015b460fad63ff3b71f77bf1a2684195#egg=XBlock -e git+https://github.com/edx/codejail.git@e3d98f9455#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.2.9#egg=diff_cover -e git+https://github.com/edx/js-test-tool.git@v0.1.5#egg=js_test_tool