Merge pull request #2221 from cpennington/xblock-xml-serialization

Enable (and test) xml export of pure XBlocks

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
 

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

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

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=''):

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):

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

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])
-
-    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")
+            self.assertTrue(blocks_are_equivalent(
+                initial_import.modules[course_id][location],
+                second_import.modules[course_id][location]
+            ))
 
-    def test_word_cloud_roundtrip(self):
-        self.check_export_roundtrip(DATA_DIR, "word_cloud")
 
 
 class TestEdxJsonEncoder(unittest.TestCase):

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)

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):

common/lib/xmodule/xmodule/xml_module.py

@@ -363,6 +363,10 @@ class XmlDescriptor(XModuleDescriptor):
         resource_fs is a pyfilesystem object (from the fs package)
         """
 
+        # Set up runtime.export_fs so that it's available through future
+        # uses of the pure xblock add_xml_to_node api
+        self.runtime.export_fs = resource_fs
+
         # Get the definition
         xml_object = self.definition_to_xml(resource_fs)
         self.clean_metadata_from_xml(xml_object)
@@ -377,12 +381,11 @@ class XmlDescriptor(XModuleDescriptor):
                 val = serialize_field(self._field_data.get(self, attr))
                 try:
                     xml_object.set(attr, val)
-                except Exception, e:
+                except Exception:
                     logging.exception(
-                        u'Failed to serialize metadata attribute %s with value %s in module %s. This could mean data loss!!! Exception: %s',
-                        attr, val, self.url_name, e
+                        u'Failed to serialize metadata attribute %s with value %s in module %s. This could mean data loss!!!',
+                        attr, val, self.url_name
                     )
-                    pass
 
         for key, value in self.xml_attributes.items():
             if key not in self.metadata_to_strip:

common/test/data/pure_xblock/course.xml

+<course org="edX" course="pure_xblock" url_name="2012_Fall">
+    <pure field1="a.field1" url_name="pure0">
+        <vertical url_name="vert0">
+            <pure field2="10" url_name="pure1"/>
+            <pure field2="20" url_name="pure2"/>
+        </vertical>
+    </pure>
+</course>
\ No newline at end of file

lms/djangoapps/courseware/management/commands/clean_xml.py

+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:

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