Merge pull request #1100 from edx/ned/new-xml

New XML infrastructure for XBlocks

common/lib/xmodule/setup.py

 from setuptools import setup, find_packages
 
+XMODULES = [
+    "abtest = xmodule.abtest_module:ABTestDescriptor",
+    "book = xmodule.backcompat_module:TranslateCustomTagDescriptor",
+    "chapter = xmodule.seq_module:SequenceDescriptor",
+    "combinedopenended = xmodule.combined_open_ended_module:CombinedOpenEndedDescriptor",
+    "conditional = xmodule.conditional_module:ConditionalDescriptor",
+    "course = xmodule.course_module:CourseDescriptor",
+    "customtag = xmodule.template_module:CustomTagDescriptor",
+    "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor",
+    "html = xmodule.html_module:HtmlDescriptor",
+    "image = xmodule.backcompat_module:TranslateCustomTagDescriptor",
+    "error = xmodule.error_module:ErrorDescriptor",
+    "peergrading = xmodule.peer_grading_module:PeerGradingDescriptor",
+    "poll_question = xmodule.poll_module:PollDescriptor",
+    "problem = xmodule.capa_module:CapaDescriptor",
+    "problemset = xmodule.seq_module:SequenceDescriptor",
+    "randomize = xmodule.randomize_module:RandomizeDescriptor",
+    "section = xmodule.backcompat_module:SemanticSectionDescriptor",
+    "sequential = xmodule.seq_module:SequenceDescriptor",
+    "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor",
+    "timelimit = xmodule.timelimit_module:TimeLimitDescriptor",
+    "vertical = xmodule.vertical_module:VerticalDescriptor",
+    "video = xmodule.video_module:VideoDescriptor",
+    "videoalpha = xmodule.video_module:VideoDescriptor",
+    "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor",
+    "videosequence = xmodule.seq_module:SequenceDescriptor",
+    "discussion = xmodule.discussion_module:DiscussionDescriptor",
+    "course_info = xmodule.html_module:CourseInfoDescriptor",
+    "static_tab = xmodule.html_module:StaticTabDescriptor",
+    "custom_tag_template = xmodule.raw_module:RawDescriptor",
+    "about = xmodule.html_module:AboutDescriptor",
+    "wrapper = xmodule.wrapper_module:WrapperDescriptor",
+    "graphical_slider_tool = xmodule.gst_module:GraphicalSliderToolDescriptor",
+    "annotatable = xmodule.annotatable_module:AnnotatableDescriptor",
+    "foldit = xmodule.foldit_module:FolditDescriptor",
+    "word_cloud = xmodule.word_cloud_module:WordCloudDescriptor",
+    "hidden = xmodule.hidden_module:HiddenDescriptor",
+    "raw = xmodule.raw_module:RawDescriptor",
+    "crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor",
+    "lti = xmodule.lti_module:LTIModuleDescriptor",
+]
+
 setup(
     name="XModule",
     version="0.1",
@@ -11,55 +53,16 @@ setup(
         'path.py',
     ],
     package_data={
-        'xmodule': ['js/module/*']
+        'xmodule': ['js/module/*'],
     },
 
     # See http://guide.python-distribute.org/creation.html#entry-points
     # for a description of entry_points
     entry_points={
-        'xmodule.v1': [
-            "abtest = xmodule.abtest_module:ABTestDescriptor",
-            "book = xmodule.backcompat_module:TranslateCustomTagDescriptor",
-            "chapter = xmodule.seq_module:SequenceDescriptor",
-            "combinedopenended = xmodule.combined_open_ended_module:CombinedOpenEndedDescriptor",
-            "conditional = xmodule.conditional_module:ConditionalDescriptor",
-            "course = xmodule.course_module:CourseDescriptor",
-            "customtag = xmodule.template_module:CustomTagDescriptor",
-            "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor",
-            "html = xmodule.html_module:HtmlDescriptor",
-            "image = xmodule.backcompat_module:TranslateCustomTagDescriptor",
-            "error = xmodule.error_module:ErrorDescriptor",
-            "peergrading = xmodule.peer_grading_module:PeerGradingDescriptor",
-            "poll_question = xmodule.poll_module:PollDescriptor",
-            "problem = xmodule.capa_module:CapaDescriptor",
-            "problemset = xmodule.seq_module:SequenceDescriptor",
-            "randomize = xmodule.randomize_module:RandomizeDescriptor",
-            "section = xmodule.backcompat_module:SemanticSectionDescriptor",
-            "sequential = xmodule.seq_module:SequenceDescriptor",
-            "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor",
-            "timelimit = xmodule.timelimit_module:TimeLimitDescriptor",
-            "vertical = xmodule.vertical_module:VerticalDescriptor",
-            "video = xmodule.video_module:VideoDescriptor",
-            "videoalpha = xmodule.video_module:VideoDescriptor",
-            "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor",
-            "videosequence = xmodule.seq_module:SequenceDescriptor",
-            "discussion = xmodule.discussion_module:DiscussionDescriptor",
-            "course_info = xmodule.html_module:CourseInfoDescriptor",
-            "static_tab = xmodule.html_module:StaticTabDescriptor",
-            "custom_tag_template = xmodule.raw_module:RawDescriptor",
-            "about = xmodule.html_module:AboutDescriptor",
-            "wrapper = xmodule.wrapper_module:WrapperDescriptor",
-            "graphical_slider_tool = xmodule.gst_module:GraphicalSliderToolDescriptor",
-            "annotatable = xmodule.annotatable_module:AnnotatableDescriptor",
-            "foldit = xmodule.foldit_module:FolditDescriptor",
-            "word_cloud = xmodule.word_cloud_module:WordCloudDescriptor",
-            "hidden = xmodule.hidden_module:HiddenDescriptor",
-            "raw = xmodule.raw_module:RawDescriptor",
-            "crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor",
-            "lti = xmodule.lti_module:LTIModuleDescriptor"
-        ],
+        'xblock.v1': XMODULES,
+        'xmodule.v1': XMODULES,
         'console_scripts': [
             'xmodule_assets = xmodule.static_content:main',
-        ]
-    }
+        ],
+    },
 )

common/lib/xmodule/xmodule/error_module.py

@@ -105,10 +105,10 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
         })
         return system.construct_xblock_from_class(
             cls,
-            field_data,
             # The error module doesn't use scoped data, and thus doesn't need
             # real scope keys
-            ScopeIds('error', None, location, location)
+            ScopeIds('error', None, location, location),
+            field_data,
         )
 
     def get_context(self):

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -193,7 +193,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
 
                 field_data = DbModel(kvs)
                 scope_ids = ScopeIds(None, category, location, location)
-                module = self.construct_xblock_from_class(class_, field_data, scope_ids)
+                module = self.construct_xblock_from_class(class_, scope_ids, field_data)
                 if self.cached_metadata is not None:
                     # parent container pointers don't differentiate between draft and non-draft
                     # so when we do the lookup, we should do so with a non-draft location
@@ -621,12 +621,11 @@ class MongoModuleStore(ModuleStoreBase):
         dbmodel = self._create_new_field_data(location.category, location, definition_data, metadata)
         xmodule = system.construct_xblock_from_class(
             xblock_class,
-            dbmodel,
-
             # We're loading a descriptor, so student_id is meaningless
             # We also don't have separate notions of definition and usage ids yet,
             # so we use the location for both.
-            ScopeIds(None, location.category, location, location)
+            ScopeIds(None, location.category, location, location),
+            dbmodel,
         )
         # decache any pending field settings from init
         xmodule.save()

common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py

@@ -111,8 +111,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         try:
             module = self.construct_xblock_from_class(
                 class_,
+                ScopeIds(None, json_data.get('category'), definition_id, block_locator),
                 field_data,
-                ScopeIds(None, json_data.get('category'), definition_id, block_locator)
             )
         except Exception:
             log.warning("Failed to load descriptor", exc_info=True)

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -17,9 +17,10 @@ from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import make_error_tracker, exc_info_to_str
 from xmodule.course_module import CourseDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
-from xmodule.x_module import XModuleDescriptor, XMLParsingSystem
+from xmodule.x_module import XMLParsingSystem, XModuleDescriptor
 
 from xmodule.html_module import HtmlDescriptor
+from xblock.core import XBlock
 from xblock.fields import ScopeIds
 from xblock.field_data import DictFieldData
 
@@ -63,7 +64,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         self.load_error_modules = load_error_modules
 
         def process_xml(xml):
-            """Takes an xml string, and returns a XModuleDescriptor created from
+            """Takes an xml string, and returns a XBlock created from
             that xml.
             """
 
@@ -163,7 +164,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
 
                 make_name_unique(xml_data)
 
-                descriptor = XModuleDescriptor.load_from_xml(
+                descriptor = create_block_from_xml(
                     etree.tostring(xml_data, encoding='unicode'), self, self.org,
                     self.course, xmlstore.default_class)
             except Exception as err:
@@ -219,6 +220,38 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         )
 
 
+def create_block_from_xml(xml_data, system, org=None, course=None, default_class=None):
+    """
+    Create an XBlock instance from XML data.
+
+    `xml_data' is a string containing valid xml.
+
+    `system` is an XMLParsingSystem.
+
+    `org` and `course` are optional strings that will be used in the generated
+    block's url identifiers.
+
+    `default_class` is the class to instantiate of the XML indicates a class
+    that can't be loaded.
+
+    Returns the fully instantiated XBlock.
+
+    """
+    node = etree.fromstring(xml_data)
+    raw_class = XModuleDescriptor.load_class(node.tag, default_class)
+    xblock_class = system.mixologist.mix(raw_class)
+
+    # leave next line commented out - useful for low-level debugging
+    # log.debug('[create_block_from_xml] tag=%s, class=%s' % (node.tag, xblock_class))
+
+    url_name = node.get('url_name', node.get('slug'))
+    location = Location('i4x', org, course, node.tag, url_name)
+
+    scope_ids = ScopeIds(None, location.category, location, location)
+    xblock = xblock_class.parse_xml(node, system, scope_ids)
+    return xblock
+
+
 class ParentTracker(object):
     """A simple class to factor out the logic for tracking location parent pointers."""
     def __init__(self):
@@ -278,8 +311,8 @@ class XMLModuleStore(ModuleStoreBase):
         super(XMLModuleStore, self).__init__(**kwargs)
 
         self.data_dir = path(data_dir)
-        self.modules = defaultdict(dict)  # course_id -> dict(location -> XModuleDescriptor)
-        self.courses = {}  # course_dir -> XModuleDescriptor for the course
+        self.modules = defaultdict(dict)  # course_id -> dict(location -> XBlock)
+        self.courses = {}  # course_dir -> XBlock for the course
         self.errored_courses = {}  # course_dir -> errorlog, for dirs that failed to load
 
         self.load_error_modules = load_error_modules
@@ -477,11 +510,11 @@ class XMLModuleStore(ModuleStoreBase):
                     loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, category, slug)
                     module = system.construct_xblock_from_class(
                         HtmlDescriptor,
-                        DictFieldData({'data': html, 'location': loc, 'category': category}),
                         # We're loading a descriptor, so student_id is meaningless
                         # We also don't have separate notions of definition and usage ids yet,
                         # so we use the location for both
                         ScopeIds(None, category, loc, loc),
+                        DictFieldData({'data': html, 'location': loc, 'category': category}),
                     )
                     # VS[compat]:
                     # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them)
@@ -500,7 +533,7 @@ class XMLModuleStore(ModuleStoreBase):
 
     def get_instance(self, course_id, location, depth=0):
         """
-        Returns an XModuleDescriptor instance for the item at
+        Returns an XBlock instance for the item at
         location, with the policy for course_id.  (In case two xml
         dirs have different content at the same location, return the
         one for this course_id.)
@@ -528,7 +561,7 @@ class XMLModuleStore(ModuleStoreBase):
 
     def get_item(self, location, depth=0):
         """
-        Returns an XModuleDescriptor instance for the item at location.
+        Returns an XBlock instance for the item at location.
 
         If any segment of the location is None except revision, raises
             xmodule.modulestore.exceptions.InsufficientSpecificationError

common/lib/xmodule/xmodule/tests/test_editing_module.py

@@ -46,8 +46,8 @@ class TabsEditingDescriptorTestCase(unittest.TestCase):
         TabsEditingDescriptor.tabs = self.tabs
         self.descriptor = system.construct_xblock_from_class(
             TabsEditingDescriptor,
-            field_data=DictFieldData({}),
             scope_ids=ScopeIds(None, None, None, None),
+            field_data=DictFieldData({}),
         )
 
     def test_get_css(self):

common/lib/xmodule/xmodule/tests/test_video.py

@@ -133,8 +133,8 @@ class VideoDescriptorTest(unittest.TestCase):
         system = get_test_descriptor_system()
         self.descriptor = system.construct_xblock_from_class(
             VideoDescriptor,
-            field_data=DictFieldData({}),
             scope_ids=ScopeIds(None, None, None, None),
+            field_data=DictFieldData({}),
         )
 
     def test_get_context(self):

common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py

@@ -87,8 +87,8 @@ class TestXBlockWrapper(object):
         runtime.render_template = lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs)
         return runtime.construct_xblock_from_class(
             descriptor_cls,
+            ScopeIds(None, descriptor_cls.__name__, location, location),
             DictFieldData({}),
-            ScopeIds(None, descriptor_cls.__name__, location, location)
         )
 
     def leaf_module(self, descriptor_cls):
@@ -109,10 +109,10 @@ class TestXBlockWrapper(object):
         runtime.render_template = lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs)
         return runtime.construct_xblock_from_class(
             descriptor_cls,
+            ScopeIds(None, descriptor_cls.__name__, location, location),
             DictFieldData({
                 'children': range(3)
             }),
-            ScopeIds(None, descriptor_cls.__name__, location, location)
         )
 
     def container_module(self, descriptor_cls, depth):

common/lib/xmodule/xmodule/tests/test_xml_module.py

@@ -166,8 +166,8 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         runtime = get_test_descriptor_system()
         return runtime.construct_xblock_from_class(
             XmlDescriptor,
+            scope_ids=Mock(),
             field_data=field_data,
-            scope_ids=Mock()
         ).editable_metadata_fields
 
     def get_descriptor(self, field_data):

common/lib/xmodule/xmodule/tests/xml/__init__.py

@@ -4,8 +4,9 @@ Xml parsing tests for XModules
 import pprint
 from mock import Mock
 
-from xmodule.x_module import XMLParsingSystem, XModuleDescriptor
+from xmodule.x_module import XMLParsingSystem
 from xmodule.mako_module import MakoDescriptorSystem
+from xmodule.modulestore.xml import create_block_from_xml
 
 
 class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem):  # pylint: disable=abstract-method
@@ -28,8 +29,8 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem):  # pylint: disable
         )
 
     def process_xml(self, xml):  # pylint: disable=method-hidden
-        """Parse `xml` as an XModuleDescriptor, and add it to `self._descriptors`"""
-        descriptor = XModuleDescriptor.load_from_xml(xml, self, self.org, self.course, self.default_class)
+        """Parse `xml` as an XBlock, and add it to `self._descriptors`"""
+        descriptor = create_block_from_xml(xml, self, self.org, self.course, self.default_class)
         self._descriptors[descriptor.location.url()] = descriptor
         return descriptor
 
@@ -39,8 +40,8 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem):  # pylint: disable
 
 
 class XModuleXmlImportTest(object):
-    """Base class for tests that use basic `XModuleDescriptor.load_from_xml` xml parsing"""
+    """Base class for tests that use basic XML parsing"""
     def process_xml(self, xml_import_data):
-        """Use the `xml_import_data` to import an :class:`XModuleDescriptor` from xml"""
+        """Use the `xml_import_data` to import an :class:`XBlock` from XML."""
         system = InMemorySystem(xml_import_data)
         return system.process_xml(xml_import_data.xml_string)

common/lib/xmodule/xmodule/video_module.py

@@ -247,12 +247,11 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
         field_data = DbModel(kvs)
         video = system.construct_xblock_from_class(
             cls,
-            field_data,
-
             # We're loading a descriptor, so student_id is meaningless
             # We also don't have separate notions of definition and usage ids yet,
             # so we use the location for both
-            ScopeIds(None, location.category, location, location)
+            ScopeIds(None, location.category, location, location),
+            field_data,
         )
         return video
 

common/lib/xmodule/xmodule/x_module.py

@@ -502,8 +502,8 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
         module = system.construct_xblock_from_class(
             self.module_class,
             descriptor=self,
-            field_data=system.xmodule_field_data(self),
             scope_ids=self.scope_ids,
+            field_data=system.xmodule_field_data(self),
         )
         module.save()
         return module
@@ -524,38 +524,21 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
         return cls.metadata_translations.get(key, key)
 
     # ================================= XML PARSING ============================
-    @staticmethod
-    def load_from_xml(xml_data,
-                      system,
-                      org=None,
-                      course=None,
-                      default_class=None):
+    @classmethod
+    def parse_xml(cls, node, runtime, keys):
         """
-        This method instantiates the correct subclass of XModuleDescriptor based
-        on the contents of xml_data.
-
-        xml_data must be a string containing valid xml
-
-        system is an XMLParsingSystem
-
-        org and course are optional strings that will be used in the generated
-            module's url identifiers
+        Interpret the parsed XML in `node`, creating an XModuleDescriptor.
         """
-        class_ = system.mixologist.mix(XModuleDescriptor.load_class(
-            etree.fromstring(xml_data).tag,
-            default_class
-        ))
-        # leave next line, commented out - useful for low-level debugging
-        # log.debug('[XModuleDescriptor.load_from_xml] tag=%s, class_=%s' % (
-        #        etree.fromstring(xml_data).tag,class_))
-
-        return class_.from_xml(xml_data, system, org, course)
+        xml = etree.tostring(node)
+        # TODO: change from_xml to not take org and course, it can use self.system.
+        block = cls.from_xml(xml, runtime, runtime.org, runtime.course)
+        return block
 
     @classmethod
     def from_xml(cls, xml_data, system, org=None, course=None):
         """
         Creates an instance of this descriptor from the supplied xml_data.
-        This may be overridden by subclasses
+        This may be overridden by subclasses.
 
         xml_data: A string of xml that will be translated into data and children
             for this module
@@ -565,13 +548,12 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
         org and course are optional strings that will be used in the generated
             module's url identifiers
         """
-        raise NotImplementedError(
-            'Modules must implement from_xml to be parsable from xml')
+        raise NotImplementedError('Modules must implement from_xml to be parsable from xml')
 
     def export_to_xml(self, resource_fs):
         """
         Returns an xml string representing this module, and all modules
-        underneath it.  May also write required resources out to resource_fs
+        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
@@ -581,8 +563,7 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
         XModuleDescriptor using the from_xml method with the same system, org,
         and course
         """
-        raise NotImplementedError(
-            'Modules must implement export_to_xml to enable xml export')
+        raise NotImplementedError('Modules must implement export_to_xml to enable xml export')
 
     # =============================== BUILTIN METHODS ==========================
     def __eq__(self, other):
@@ -715,7 +696,9 @@ class DescriptorSystem(Runtime):
                that you're about to re-raise---let the caller track them.
         """
 
-        super(DescriptorSystem, self).__init__(**kwargs)
+        # Right now, usage_store is unused, and field_data is always supplanted
+        # with an explicit field_data during construct_xblock, so None's suffice.
+        super(DescriptorSystem, self).__init__(usage_store=None, field_data=None, **kwargs)
 
         self.load_item = load_item
         self.resources_fs = resources_fs
@@ -756,8 +739,6 @@ class DescriptorSystem(Runtime):
 class XMLParsingSystem(DescriptorSystem):
     def __init__(self, process_xml, policy, **kwargs):
         """
-        load_item, resources_fs, error_tracker: see DescriptorSystem
-
         policy: a policy dictionary for overriding xml metadata
 
         process_xml: Takes an xml string, and returns a XModuleDescriptor
@@ -839,7 +820,10 @@ class ModuleSystem(Runtime):
             not to allow the execution of unsafe, unsandboxed code.
 
         """
-        super(ModuleSystem, self).__init__(**kwargs)
+
+        # Right now, usage_store is unused, and field_data is always supplanted
+        # with an explicit field_data during construct_xblock, so None's suffice.
+        super(ModuleSystem, self).__init__(usage_store=None, field_data=None, **kwargs)
 
         self.ajax_url = ajax_url
         self.xqueue = xqueue

common/lib/xmodule/xmodule/xml_module.py

@@ -217,8 +217,7 @@ class XmlDescriptor(XModuleDescriptor):
             # give the class a chance to fix it up. The file will be written out
             # again in the correct format.  This should go away once the CMS is
             # online and has imported all current (fall 2012) courses from xml
-            if not system.resources_fs.exists(filepath) and hasattr(
-                    cls, 'backcompat_paths'):
+            if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'):
                 candidates = cls.backcompat_paths(filepath)
                 for candidate in candidates:
                     if system.resources_fs.exists(candidate):
@@ -339,12 +338,11 @@ class XmlDescriptor(XModuleDescriptor):
 
         return system.construct_xblock_from_class(
             cls,
-            field_data,
-
             # We're loading a descriptor, so student_id is meaningless
             # We also don't have separate notions of definition and usage ids yet,
             # so we use the location for both
-            ScopeIds(None, location.category, location, location)
+            ScopeIds(None, location.category, location, location),
+            field_data,
         )
 
     @classmethod

requirements/edx/github.txt

@@ -14,7 +14,7 @@
 -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk
 
 # Our libraries:
--e git+https://github.com/edx/XBlock.git@a8de02c0#egg=XBlock
+-e git+https://github.com/edx/XBlock.git@8a66ca3#egg=XBlock
 -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail
 -e git+https://github.com/edx/diff-cover.git@v0.2.4#egg=diff_cover
 -e git+https://github.com/edx/js-test-tool.git@v0.0.7#egg=js_test_tool