Merge pull request #750 from MITx/feature/cale/cms-save-validation

Feature/cale/cms save validation

cms/__init__.py

+

cms/djangoapps/contentstore/__init__.py

+from xmodule.templates import update_templates
+
+update_templates()

cms/djangoapps/contentstore/management/commands/import.py

@@ -26,4 +26,4 @@ class Command(BaseCommand):
         print "Importing.  Data_dir={data}, course_dirs={courses}".format(
             data=data_dir,
             courses=course_dirs)
-        import_from_xml(modulestore(), data_dir, course_dirs)
+        import_from_xml(modulestore(), data_dir, course_dirs, load_error_modules=False)

cms/djangoapps/contentstore/views.py

 from util.json_request import expect_json
 import json
 import logging
+import sys
 from collections import defaultdict
 
 from django.http import HttpResponse, Http404
@@ -12,6 +13,8 @@ from django.conf import settings
 
 from xmodule.modulestore import Location
 from xmodule.x_module import ModuleSystem
+from xmodule.error_module import ErrorDescriptor
+from xmodule.errortracker import exc_info_to_str
 from github_sync import export_to_github
 from static_replace import replace_urls
 
@@ -20,6 +23,8 @@ from xmodule.modulestore.django import modulestore
 from xmodule_modifiers import replace_static_urls, wrap_xmodule
 from xmodule.exceptions import NotFoundError
 from functools import partial
+from itertools import groupby
+from operator import attrgetter
 
 log = logging.getLogger(__name__)
 
@@ -128,6 +133,33 @@ def edit_item(request):
     })
 
 
+@login_required
+def new_item(request):
+    """
+    Display a page where the user can create a new item from a template
+
+    Expects a GET request with the parameter 'parent_location', which is the element to add
+    the newly created item to as a child.
+
+    parent_location: A Location URL
+    """
+
+    parent_location = request.GET['parent_location']
+    if not has_access(request.user, parent_location):
+        raise Http404
+
+    parent = modulestore().get_item(parent_location)
+    templates = modulestore().get_items(Location('i4x', 'edx', 'templates'))
+
+    templates.sort(key=attrgetter('location.category', 'display_name'))
+
+    return render_to_response('new_item.html', {
+        'parent_name': parent.display_name,
+        'parent_location': parent.location.url(),
+        'templates': groupby(templates, attrgetter('location.category')),
+    })
+
+
 def user_author_string(user):
     '''Get an author string for commits by this user.  Format:
     first last <email@email.com>.
@@ -259,10 +291,17 @@ def load_preview_module(request, preview_id, descriptor, instance_state, shared_
     shared_state: A shared state string
     """
     system = preview_module_system(request, preview_id, descriptor)
+    try:
         module = descriptor.xmodule_constructor(system)(instance_state, shared_state)
+    except:
+        module = ErrorDescriptor.from_descriptor(
+            descriptor,
+            error_msg=exc_info_to_str(sys.exc_info())
+        ).xmodule_constructor(system)(None, None)
+
     module.get_html = replace_static_urls(
         wrap_xmodule(module.get_html, module, "xmodule_display.html"),
-        module.metadata['data_dir']
+        module.metadata.get('data_dir', module.location.course)
     )
     save_preview_state(request, preview_id, descriptor.location.url(),
         module.get_instance_state(), module.get_shared_state())
@@ -326,3 +365,28 @@ def save_item(request):
     preview_html = get_module_previews(request, descriptor)
 
     return HttpResponse(json.dumps(preview_html))
+
+
+@login_required
+@expect_json
+def clone_item(request):
+    parent_location = Location(request.POST['parent_location'])
+    template = Location(request.POST['template'])
+    display_name = request.POST['name']
+
+    if not has_access(request.user, parent_location):
+        raise Http404  # TODO (vshnayder): better error
+
+    parent = modulestore().get_item(parent_location)
+    dest_location = parent_location._replace(category=template.category, name=Location.clean_for_url_name(display_name))
+
+    new_item = modulestore().clone_item(template, dest_location)
+    new_item.metadata['display_name'] = display_name
+
+    # TODO: This needs to be deleted when we have proper storage for static content
+    new_item.metadata['data_dir'] = parent.metadata['data_dir']
+
+    modulestore().update_metadata(new_item.location.url(), new_item.own_metadata)
+    modulestore().update_children(parent_location, parent.definition.get('children', []) + [new_item.location.url()])
+
+    return HttpResponse()

cms/envs/common.py

@@ -195,6 +195,7 @@ STATICFILES_STORAGE = 'pipeline.storage.PipelineCachedStorage'
 # prep it for use in pipeline js
 from xmodule.x_module import XModuleDescriptor
 from xmodule.raw_module import RawDescriptor
+from xmodule.error_module import ErrorDescriptor
 js_file_dir = PROJECT_ROOT / "static" / "coffee" / "module"
 css_file_dir = PROJECT_ROOT / "static" / "sass" / "module"
 module_styles_path = css_file_dir / "_module-styles.scss"
@@ -210,7 +211,7 @@ for dir_ in (js_file_dir, css_file_dir):
 
 js_fragments = set()
 css_fragments = defaultdict(set)
-for descriptor in XModuleDescriptor.load_classes() + [RawDescriptor]:
+for _, descriptor in XModuleDescriptor.load_classes() + [(None, RawDescriptor), (None, ErrorDescriptor)]:
     descriptor_js = descriptor.get_javascript()
     module_js = descriptor.module_class.get_javascript()
 

cms/envs/dev.py

@@ -14,7 +14,6 @@ LOGGING = get_logger_config(ENV_ROOT / "log",
                             tracking_filename="tracking.log",
                             debug=True)
 
-
 MODULESTORE = {
     'default': {
         'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',

cms/static/coffee/src/models/new_module.coffee

+class CMS.Models.NewModule extends Backbone.Model
+  url: '/clone_item'
+
+  newUrl: ->
+    "/new_item?#{$.param(parent_location: @get('parent_location'))}"

cms/static/coffee/src/views/module.coffee

@@ -11,3 +11,4 @@ class CMS.Views.Module extends Backbone.View
         id: @$el.data('id')
         type: if moduleType == 'None' then null else moduleType
         previewType: if previewType == 'None' then null else previewType
+

cms/static/coffee/src/views/module_add.coffee

+class CMS.Views.ModuleAdd extends Backbone.View
+  tagName: 'section'
+  className: 'add-pane'
+
+  events:
+    'click .cancel': 'cancel'
+    'click .save': 'save'
+
+  initialize: ->
+    @$el.load @model.newUrl()
+
+  save: (event) ->
+    event.preventDefault()
+    @model.save({
+      name: @$el.find('.name').val()
+      template: $(event.target).data('template-id')
+    }, {
+      success: -> CMS.popView()
+      error: -> alert('Create failed')
+    })
+
+  cancel: (event) ->
+    event.preventDefault()
+    CMS.popView()
+
+

cms/static/coffee/src/views/module_edit.coffee

@@ -39,7 +39,7 @@ class CMS.Views.ModuleEdit extends Backbone.View
       )
 
       XModule.loadModules('display')
-    ).fail(->
+    ).fail( ->
       alert("There was an error saving your changes. Please try again.")
     )
 

cms/static/coffee/src/views/week.coffee

 class CMS.Views.Week extends Backbone.View
   events:
     'click .week-edit': 'edit'
+    'click .new-module': 'new'
 
   initialize: ->
     CMS.on('content.show', @resetHeight)
@@ -23,3 +24,9 @@ class CMS.Views.Week extends Backbone.View
 
   resetHeight: =>
     @$el.height('')
+
+  new: (event) =>
+    event.preventDefault()
+    CMS.replaceView new CMS.Views.ModuleAdd
+      model: new CMS.Models.NewModule
+        parent_location: @$el.data('id')

cms/templates/new_item.html

+<section>
+  <div>${parent_name}</div>
+  <div>${parent_location}</div>
+  <input type="text" class="name"/>
+  <div>
+    % for module_type, module_templates in templates:
+    <div>
+      <div>${module_type}</div>
+      <div>
+        % for template in module_templates:
+        <a class="save" data-template-id="${template.location.url()}">${template.display_name}</a>
+        % endfor
+      </div>
+    </div>
+    % endfor
+  </div>
+  <a class='cancel'>Cancel</a>
+</section>
+

cms/templates/widgets/module-dropdown.html

@@ -7,7 +7,7 @@
     <form>
       <ul>
         <li>
-          <input type="text" name="" id="" placeholder="Moldule title" />
+          <input type="text" name="" id="" placeholder="Module title" />
         </li>
         <li>
           <select>

cms/templates/widgets/navigation.html

@@ -61,7 +61,6 @@
             data-preview-type="${module.module_class.js_module_name}">
 
             <a href="#" class="module-edit">${module.display_name}</a>
-            <a href="#" class="draggable">handle</a>
         </li>
         % endfor
         <%include file="module-dropdown.html"/>

cms/urls.py

@@ -9,8 +9,10 @@ import django.contrib.auth.views
 
 urlpatterns = ('',
     url(r'^$', 'contentstore.views.index', name='index'),
+    url(r'^new_item$', 'contentstore.views.new_item', name='new_item'),
     url(r'^edit_item$', 'contentstore.views.edit_item', name='edit_item'),
     url(r'^save_item$', 'contentstore.views.save_item', name='save_item'),
+    url(r'^clone_item$', 'contentstore.views.clone_item', name='clone_item'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)$',
         'contentstore.views.course_index', name='course_index'),
     url(r'^github_service_hook$', 'github_sync.views.github_post_receive'),

common/lib/xmodule/xmodule/abtest_module.py

 import json
 import random
+import logging
 from lxml import etree
 
 from xmodule.x_module import XModule
@@ -10,6 +11,9 @@ from xmodule.exceptions import InvalidDefinitionError
 DEFAULT = "_DEFAULT_GROUP"
 
 
+log = logging.getLogger(__name__)
+
+
 def group_from_value(groups, v):
     """
     Given group: (('a', 0.3), ('b', 0.4), ('c', 0.3)) and random value v
@@ -62,6 +66,8 @@ class ABTestModule(XModule):
 class ABTestDescriptor(RawDescriptor, XmlDescriptor):
     module_class = ABTestModule
 
+#    template_dir_name = "abtest"
+
     def __init__(self, system, definition=None, **kwargs):
         """
         definition is a dictionary with the following layout:
@@ -125,10 +131,13 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor):
                 name = group.get('name')
                 definition['data']['group_portions'][name] = float(group.get('portion', 0))
 
-            child_content_urls = [
-                system.process_xml(etree.tostring(child)).location.url()
-                for child in group
-            ]
+            child_content_urls = []
+            for child in group:
+                try:
+                    child_content_urls.append(system.process_xml(etree.tostring(child)).location.url())
+                except:
+                    log.exception("Unable to load child when parsing ABTest. Continuing...")
+                    continue
 
             definition['data']['group_content'][name] = child_content_urls
             definition['children'].extend(child_content_urls)

common/lib/xmodule/xmodule/capa_module.py

@@ -644,6 +644,8 @@ class CapaDescriptor(RawDescriptor):
     # actually use type and points?
     metadata_attributes = RawDescriptor.metadata_attributes + ('type', 'points')
 
+    template_dir_name = 'problem'
+
     # VS[compat]
     # TODO (cpennington): Delete this method once all fall 2012 course are being
     # edited in the cms

common/lib/xmodule/xmodule/editing_module.py

@@ -5,6 +5,7 @@ import logging
 
 log = logging.getLogger(__name__)
 
+
 class EditingDescriptor(MakoModuleDescriptor):
     """
     Module that provides a raw editing view of its data and children.  It does not
@@ -14,16 +15,31 @@ class EditingDescriptor(MakoModuleDescriptor):
     """
     mako_template = "widgets/raw-edit.html"
 
-    js = {'coffee': [resource_string(__name__, 'js/src/raw/edit.coffee')]}
-    js_module_name = "RawDescriptor"
-
-
     # cdodge: a little refactoring here, since we're basically doing the same thing
     # here as with our parent class, let's call into it to get the basic fields
     # set and then add our additional fields. Trying to keep it DRY.
     def get_context(self):
         _context = MakoModuleDescriptor.get_context(self)
         # Add our specific template information (the raw data body)
-        _context.update({ 'data' : self.definition.get('data','') })
+        _context.update({'data': self.definition.get('data', '')})
         return _context
 
+
+class XMLEditingDescriptor(EditingDescriptor):
+    """
+    Module that provides a raw editing view of its data as XML. It does not perform
+    any validation of its definition
+    """
+
+    js = {'coffee': [resource_string(__name__, 'js/src/raw/edit/xml.coffee')]}
+    js_module_name = "XMLEditingDescriptor"
+
+
+class JSONEditingDescriptor(EditingDescriptor):
+    """
+    Module that provides a raw editing view of its data as XML. It does not perform
+    any validation of its definition
+    """
+
+    js = {'coffee': [resource_string(__name__, 'js/src/raw/edit/json.coffee')]}
+    js_module_name = "JSONEditingDescriptor"

common/lib/xmodule/xmodule/error_module.py

 import hashlib
 import logging
-import random
-import string
+import json
 import sys
 
-from pkg_resources import resource_string
 from lxml import etree
 from xmodule.x_module import XModule
-from xmodule.mako_module import MakoModuleDescriptor
-from xmodule.xml_module import XmlDescriptor
-from xmodule.editing_module import EditingDescriptor
+from xmodule.editing_module import JSONEditingDescriptor
 from xmodule.errortracker import exc_info_to_str
+from xmodule.modulestore import Location
 
 
 log = logging.getLogger(__name__)
@@ -22,6 +19,7 @@ log = logging.getLogger(__name__)
 # what to show, and the logic for that belongs in the LMS (e.g. in get_module), so the error handler
 # decides whether to create a staff or not-staff module.
 
+
 class ErrorModule(XModule):
     def get_html(self):
         '''Show an error to staff.
@@ -29,9 +27,9 @@ class ErrorModule(XModule):
         '''
         # staff get to see all the details
         return self.system.render_template('module-error.html', {
-            'staff_access' : True,
-            'data' : self.definition['data']['contents'],
-            'error' : self.definition['data']['error_msg'],
+            'staff_access': True,
+            'data': self.definition['data']['contents'],
+            'error': self.definition['data']['error_msg'],
             })
 
 
@@ -42,19 +40,76 @@ class NonStaffErrorModule(XModule):
         '''
         # staff get to see all the details
         return self.system.render_template('module-error.html', {
-            'staff_access' : False,
-            'data' : "",
-            'error' : "",
+            'staff_access': False,
+            'data': "",
+            'error': "",
             })
 
 
-class ErrorDescriptor(EditingDescriptor):
+class ErrorDescriptor(JSONEditingDescriptor):
     """
     Module that provides a raw editing view of broken xml.
     """
     module_class = ErrorModule
 
     @classmethod
+    def _construct(self, system, contents, error_msg, location):
+
+        if location.name is None:
+            location = location._replace(
+                category='error',
+                # Pick a unique url_name -- the sha1 hash of the contents.
+                # NOTE: We could try to pull out the url_name of the errored descriptor,
+                # but url_names aren't guaranteed to be unique between descriptor types,
+                # and ErrorDescriptor can wrap any type.  When the wrapped module is fixed,
+                # it will be written out with the original url_name.
+                name=hashlib.sha1(contents).hexdigest()
+            )
+
+        definition = {
+            'data': {
+                'error_msg': str(error_msg),
+                'contents': contents,
+            }
+        }
+
+        # real metadata stays in the content, but add a display name
+        metadata = {'display_name': 'Error: ' + location.name}
+        return ErrorDescriptor(
+            system,
+            definition,
+            location=location,
+            metadata=metadata
+        )
+
+    def get_context(self):
+        return {
+            'module': self,
+            'data': self.definition['data']['contents'],
+        }
+
+    @classmethod
+    def from_json(cls, json_data, system, error_msg='Error not available'):
+        return cls._construct(
+            system,
+            json.dumps(json_data, indent=4),
+            error_msg,
+            location=Location(json_data['location']),
+        )
+
+    @classmethod
+    def from_descriptor(cls, descriptor, error_msg='Error not available'):
+        return cls._construct(
+            descriptor.system,
+            json.dumps({
+                'definition': descriptor.definition,
+                'metadata': descriptor.metadata,
+            }, indent=4),
+            error_msg,
+            location=descriptor.location,
+        )
+
+    @classmethod
     def from_xml(cls, xml_data, system, org=None, course=None,
                  error_msg='Error not available'):
         '''Create an instance of this descriptor from the supplied data.
@@ -65,17 +120,6 @@ class ErrorDescriptor(EditingDescriptor):
         Takes an extra, optional, parameter--the error that caused an
         issue.  (should be a string, or convert usefully into one).
         '''
-        # Use a nested inner dictionary because 'data' is hardcoded
-        inner = {}
-        definition = {'data': inner}
-        inner['error_msg'] = str(error_msg)
-
-        # Pick a unique url_name -- the sha1 hash of the xml_data.
-        # NOTE: We could try to pull out the url_name of the errored descriptor,
-        # but url_names aren't guaranteed to be unique between descriptor types,
-        # and ErrorDescriptor can wrap any type.  When the wrapped module is fixed,
-        # it will be written out with the original url_name.
-        url_name = hashlib.sha1(xml_data).hexdigest()
 
         try:
             # If this is already an error tag, don't want to re-wrap it.
@@ -84,22 +128,15 @@ class ErrorDescriptor(EditingDescriptor):
                 xml_data = xml_obj.text
                 error_node = xml_obj.find('error_msg')
                 if error_node is not None:
-                    inner['error_msg'] = error_node.text
+                    error_msg = error_node.text
                 else:
-                    inner['error_msg'] = 'Error not available'
+                    error_msg = 'Error not available'
 
         except etree.XMLSyntaxError:
             # Save the error to display later--overrides other problems
-            inner['error_msg'] = exc_info_to_str(sys.exc_info())
+            error_msg = exc_info_to_str(sys.exc_info())
 
-        inner['contents'] = xml_data
-        # TODO (vshnayder): Do we need a unique slug here?  Just pick a random
-        # 64-bit num?
-        location = ['i4x', org, course, 'error', url_name]
-        # real metadata stays in the xml_data, but add a display name
-        metadata = {'display_name': 'Error ' + url_name}
-
-        return cls(system, definition, location=location, metadata=metadata)
+        return cls._construct(system, xml_data, error_msg, location=Location('i4x', org, course, None, None))
 
     def export_to_xml(self, resource_fs):
         '''
@@ -121,6 +158,7 @@ class ErrorDescriptor(EditingDescriptor):
             err_node.text = self.definition['data']['error_msg']
             return etree.tostring(root)
 
+
 class NonStaffErrorDescriptor(ErrorDescriptor):
     """
     Module that provides non-staff error messages.

common/lib/xmodule/xmodule/html_module.py

@@ -6,7 +6,7 @@ import sys
 from lxml import etree
 from path import path
 
-from .x_module import XModule
+from .x_module import XModule, Template
 from .xml_module import XmlDescriptor, name_to_pathname
 from .editing_module import EditingDescriptor
 from .stringify import stringify_children

common/lib/xmodule/xmodule/js/src/raw/edit.coffee

-class @RawDescriptor
-    constructor: (@element) ->
-        @edit_box = $(".edit-box", @element)
-
-    save: -> @edit_box.val()

common/lib/xmodule/xmodule/js/src/raw/edit/json.coffee

+class @JSONEditingDescriptor
+  constructor: (@element) ->
+    @edit_box = CodeMirror.fromTextArea($(".edit-box", @element)[0], {
+      mode: { name: "javascript", json: true }
+    })
+
+  save: -> JSON.parse @edit_box.getValue()

common/lib/xmodule/xmodule/js/src/raw/edit/xml.coffee

+class @XMLEditingDescriptor
+  constructor: (@element) ->
+    @edit_box = CodeMirror.fromTextArea($(".edit-box", @element)[0], {
+      mode: "xml"
+    })
+
+  save: -> @edit_box.getValue()

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -101,8 +101,6 @@ class Location(_LocationBase):
                 raise InsufficientSpecificationError(location)
         return loc
 
-
-
     def __new__(_cls, loc_or_tag=None, org=None, course=None, category=None,
                 name=None, revision=None):
         """
@@ -297,8 +295,11 @@ class ModuleStore(object):
         """
         raise NotImplementedError
 
-    # TODO (cpennington): Replace with clone_item
-    def create_item(self, location, editor):
+    def clone_item(self, source, location):
+        """
+        Clone a new item that is a copy of the item at the location `source`
+        and writes it to `location`
+        """
         raise NotImplementedError
 
     def update_item(self, location, data):

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

 import pymongo
+import sys
 
 from bson.son import SON
 from fs.osfs import OSFS
@@ -6,13 +7,14 @@ from itertools import repeat
 from path import path
 
 from importlib import import_module
-from xmodule.errortracker import null_error_tracker
+from xmodule.errortracker import null_error_tracker, exc_info_to_str
 from xmodule.x_module import XModuleDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
+from xmodule.error_module import ErrorDescriptor
 
 from . import ModuleStoreBase, Location
 from .exceptions import (ItemNotFoundError,
-                         NoPathToItem, DuplicateItemError)
+                         DuplicateItemError)
 
 # TODO (cpennington): This code currently operates under the assumption that
 # there is only one revision for each item. Once we start versioning inside the CMS,
@@ -57,7 +59,14 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
             # TODO (vshnayder): metadata inheritance is somewhat broken because mongo, doesn't
             # always load an entire course.  We're punting on this until after launch, and then
             # will build a proper course policy framework.
+            try:
                 return XModuleDescriptor.load_from_json(json_data, self, self.default_class)
+            except:
+                return ErrorDescriptor.from_json(
+                    json_data,
+                    self,
+                    error_msg=exc_info_to_str(sys.exc_info())
+                )
 
 
 def location_to_query(location):
@@ -153,7 +162,12 @@ class MongoModuleStore(ModuleStoreBase):
         Load an XModuleDescriptor from item, using the children stored in data_cache
         """
         data_dir = item.get('metadata', {}).get('data_dir', item['location']['course'])
-        resource_fs = OSFS(self.fs_root / data_dir)
+        root = self.fs_root / data_dir
+
+        if not root.isdir():
+            root.mkdir()
+
+        resource_fs = OSFS(root)
 
         system = CachingDescriptorSystem(
             self,
@@ -232,23 +246,36 @@ class MongoModuleStore(ModuleStoreBase):
 
         return self._load_items(list(items), depth)
 
-    # TODO (cpennington): This needs to be replaced by clone_item as soon as we allow
-    # creation of items from the cms
-    def create_item(self, location):
+    def clone_item(self, source, location):
         """
-        Create an empty item at the specified location.
-
-        If that location already exists, raises a DuplicateItemError
-
-        location: Something that can be passed to Location
+        Clone a new item that is a copy of the item at the location `source`
+        and writes it to `location`
         """
         try:
-            self.collection.insert({
-                '_id': Location(location).dict(),
-            })
+            source_item = self.collection.find_one(location_to_query(source))
+            source_item['_id'] = Location(location).dict()
+            self.collection.insert(source_item)
+            return self._load_items([source_item])[0]
         except pymongo.errors.DuplicateKeyError:
             raise DuplicateItemError(location)
 
+    def _update_single_item(self, location, update):
+        """
+        Set update on the specified item, and raises ItemNotFoundError
+        if the location doesn't exist
+        """
+
+        # See http://www.mongodb.org/display/DOCS/Updating for
+        # atomic update syntax
+        result = self.collection.update(
+            {'_id': Location(location).dict()},
+            {'$set': update},
+            multi=False,
+            upsert=True,
+        )
+        if result['n'] == 0:
+            raise ItemNotFoundError(location)
+
     def update_item(self, location, data):
         """
         Set the data in the item specified by the location to
@@ -258,13 +285,7 @@ class MongoModuleStore(ModuleStoreBase):
         data: A nested dictionary of problem data
         """
 
-        # See http://www.mongodb.org/display/DOCS/Updating for
-        # atomic update syntax
-        self.collection.update(
-            {'_id': Location(location).dict()},
-            {'$set': {'definition.data': data}},
-
-        )
+        self._update_single_item(location, {'definition.data': data})
 
     def update_children(self, location, children):
         """
@@ -275,12 +296,7 @@ class MongoModuleStore(ModuleStoreBase):
         children: A list of child item identifiers
         """
 
-        # See http://www.mongodb.org/display/DOCS/Updating for
-        # atomic update syntax
-        self.collection.update(
-            {'_id': Location(location).dict()},
-            {'$set': {'definition.children': children}}
-        )
+        self._update_single_item(location, {'definition.children': children})
 
     def update_metadata(self, location, metadata):
         """
@@ -291,12 +307,7 @@ class MongoModuleStore(ModuleStoreBase):
         metadata: A nested dictionary of module metadata
         """
 
-        # See http://www.mongodb.org/display/DOCS/Updating for
-        # atomic update syntax
-        self.collection.update(
-            {'_id': Location(location).dict()},
-            {'$set': {'metadata': metadata}}
-        )
+        self._update_single_item(location, {'metadata': metadata})
 
     def get_parent_locations(self, location):
         '''Find all locations that are the parents of this location.  Needed
@@ -310,7 +321,7 @@ class MongoModuleStore(ModuleStoreBase):
         '''
         location = Location.ensure_fully_specified(location)
         # Check that it's actually in this modulestore.
-        item = self._find_one(location)
+        self._find_one(location)
         # now get the parents
         items = self.collection.find({'definition.children': location.url()},
                                     {'_id': True})

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

@@ -3,16 +3,16 @@ import json
 import logging
 import os
 import re
+import sys
 
 from collections import defaultdict
 from cStringIO import StringIO
 from fs.osfs import OSFS
 from importlib import import_module
 from lxml import etree
-from lxml.html import HtmlComment
 from path import path
 
-from xmodule.errortracker import ErrorLog, make_error_tracker
+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
@@ -27,6 +27,7 @@ etree.set_default_parser(edx_xml_parser)
 
 log = logging.getLogger('mitx.' + __name__)
 
+
 # VS[compat]
 # TODO (cpennington): Remove this once all fall 2012 courses have been imported
 # into the cms from xml
@@ -35,9 +36,11 @@ def clean_out_mako_templating(xml_string):
     xml_string = re.sub("(?m)^\s*%.*$", '', xml_string)
     return xml_string
 
+
 class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
     def __init__(self, xmlstore, course_id, course_dir,
-                 policy, error_tracker, parent_tracker, **kwargs):
+                 policy, error_tracker, parent_tracker,
+                 load_error_modules=True, **kwargs):
         """
         A class that handles loading from xml.  Does some munging to ensure that
         all elements have unique slugs.
@@ -47,6 +50,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         self.unnamed = defaultdict(int)     # category -> num of new url_names for that category
         self.used_names = defaultdict(set)  # category -> set of used url_names
         self.org, self.course, self.url_name = course_id.split('/')
+        self.load_error_modules = load_error_modules
 
         def process_xml(xml):
             """Takes an xml string, and returns a XModuleDescriptor created from
@@ -93,7 +97,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                     because we want it to be lazy."""
                     if looks_like_fallback(orig_name):
                         # We're about to re-hash, in case something changed, so get rid of the tag_ and hash
-                        orig_name = orig_name[len(tag)+1:-12]
+                        orig_name = orig_name[len(tag) + 1:-12]
                     # append the hash of the content--the first 12 bytes should be plenty.
                     orig_name = "_" + orig_name if orig_name not in (None, "") else ""
                     return tag + orig_name + "_" + hashlib.sha1(xml).hexdigest()[:12]
@@ -144,16 +148,40 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 # have been imported into the cms from xml
                 xml = clean_out_mako_templating(xml)
                 xml_data = etree.fromstring(xml)
-            except Exception as err:
-                log.warning("Unable to parse xml: {err}, xml: {xml}".format(
-                    err=str(err), xml=xml))
-                raise
 
                 make_name_unique(xml_data)
 
                 descriptor = XModuleDescriptor.load_from_xml(
                     etree.tostring(xml_data), self, self.org,
                     self.course, xmlstore.default_class)
+            except Exception as err:
+                print err, self.load_error_modules
+                if not self.load_error_modules:
+                    raise
+
+                # Didn't load properly.  Fall back on loading as an error
+                # descriptor.  This should never error due to formatting.
+
+                # Put import here to avoid circular import errors
+                from xmodule.error_module import ErrorDescriptor
+
+                msg = "Error loading from xml. " + str(err)[:200]
+                log.warning(msg)
+                # Normally, we don't want lots of exception traces in our logs from common
+                # content problems.  But if you're debugging the xml loading code itself,
+                # uncomment the next line.
+                # log.exception(msg)
+
+                self.error_tracker(msg)
+                err_msg = msg + "\n" + exc_info_to_str(sys.exc_info())
+                descriptor = ErrorDescriptor.from_xml(
+                    xml,
+                    self,
+                    self.org,
+                    self.course,
+                    err_msg
+                )
+
             descriptor.metadata['data_dir'] = course_dir
 
             xmlstore.modules[course_id][descriptor.location] = descriptor
@@ -219,7 +247,7 @@ class XMLModuleStore(ModuleStoreBase):
     """
     An XML backed ModuleStore
     """
-    def __init__(self, data_dir, default_class=None, course_dirs=None):
+    def __init__(self, data_dir, default_class=None, course_dirs=None, load_error_modules=True):
         """
         Initialize an XMLModuleStore from data_dir
 
@@ -238,6 +266,8 @@ class XMLModuleStore(ModuleStoreBase):
         self.courses = {}  # course_dir -> XModuleDescriptor for the course
         self.errored_courses = {}  # course_dir -> errorlog, for dirs that failed to load
 
+        self.load_error_modules = load_error_modules
+
         if default_class is None:
             self.default_class = None
         else:
@@ -396,7 +426,15 @@ class XMLModuleStore(ModuleStoreBase):
 
 
             course_id = CourseDescriptor.make_id(org, course, url_name)
-            system = ImportSystem(self, course_id, course_dir, policy, tracker, self.parent_tracker)
+            system = ImportSystem(
+                self,
+                course_id,
+                course_dir,
+                policy,
+                tracker,
+                self.parent_tracker,
+                self.load_error_modules,
+            )
 
             course_descriptor = system.process_xml(etree.tostring(course_data))
 
@@ -471,10 +509,6 @@ class XMLModuleStore(ModuleStoreBase):
         """
         return dict( (k, self.errored_courses[k].errors) for k in self.errored_courses)
 
-    def create_item(self, location):
-        raise NotImplementedError("XMLModuleStores are read-only")
-
-
     def update_item(self, location, data):
         """
         Set the data in the item specified by the location to

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -7,7 +7,8 @@ log = logging.getLogger(__name__)
 
 
 def import_from_xml(store, data_dir, course_dirs=None, 
-                    default_class='xmodule.raw_module.RawDescriptor'):
+                    default_class='xmodule.raw_module.RawDescriptor',
+                    load_error_modules=True):
     """
     Import the specified xml data_dir into the "store" modulestore,
     using org and course as the location org and course.
@@ -19,18 +20,12 @@ def import_from_xml(store, data_dir, course_dirs=None,
     module_store = XMLModuleStore(
         data_dir,
         default_class=default_class,
-        course_dirs=course_dirs
+        course_dirs=course_dirs,
+        load_error_modules=load_error_modules,
     )
     for course_id in module_store.modules.keys():
         for module in module_store.modules[course_id].itervalues():
 
-            # TODO (cpennington): This forces import to overrite the same items.
-            # This should in the future create new revisions of the items on import
-            try:
-                store.create_item(module.location)
-            except DuplicateItemError:
-                log.exception('Item already exists at %s' % module.location.url())
-                pass
             if 'data' in module.definition:
                 store.update_item(module.location, module.definition['data'])
             if 'children' in module.definition:

common/lib/xmodule/xmodule/raw_module.py

 from lxml import etree
-from xmodule.editing_module import EditingDescriptor
+from xmodule.editing_module import XMLEditingDescriptor
 from xmodule.xml_module import XmlDescriptor
 import logging
 import sys
 
 log = logging.getLogger(__name__)
 
-class RawDescriptor(XmlDescriptor, EditingDescriptor):
+class RawDescriptor(XmlDescriptor, XMLEditingDescriptor):
     """
     Module that provides a raw editing view of its data and children.  It
     requires that the definition xml is valid.
     """
     @classmethod
     def definition_from_xml(cls, xml_object, system):
-        return {'data': etree.tostring(xml_object)}
+        return {'data': etree.tostring(xml_object, pretty_print=True)}
 
     def definition_to_xml(self, resource_fs):
         try:

common/lib/xmodule/xmodule/seq_module.py

@@ -118,12 +118,18 @@ class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor):
 
     stores_state = True # For remembering where in the sequence the student is
 
+    template_dir_name = 'sequential'
+
     @classmethod
     def definition_from_xml(cls, xml_object, system):
-        return {'children': [
-            system.process_xml(etree.tostring(child_module)).location.url()
-            for child_module in xml_object
-        ]}
+        children = []
+        for child in xml_object:
+            try:
+                children.append(system.process_xml(etree.tostring(child)).location.url())
+            except:
+                log.exception("Unable to load child when parsing Sequence. Continuing...")
+                continue
+        return {'children': children}
 
     def definition_to_xml(self, resource_fs):
         xml_object = etree.Element('sequential')

common/lib/xmodule/xmodule/templates.py

+"""
+This module handles loading xmodule templates from disk into the modulestore.
+These templates are used by the CMS to provide baseline content that
+can be cloned when adding new modules to a course.
+
+`Template`s are defined in x_module. They contain 3 attributes:
+    metadata: A dictionary with the template metadata
+    data: A JSON value that defines the template content
+    children: A list of Location urls that define the template children
+
+Templates are defined on XModuleDescriptor types, in the template attribute.
+"""
+
+
+import logging
+from fs.memoryfs import MemoryFS
+
+from collections import defaultdict
+from .x_module import XModuleDescriptor
+from .mako_module import MakoDescriptorSystem
+from .modulestore import Location
+from .modulestore.django import modulestore
+
+log = logging.getLogger(__name__)
+
+
+def all_templates():
+    """
+    Returns all templates for enabled modules, grouped by descriptor type
+    """
+
+    templates = defaultdict(list)
+    for category, descriptor in XModuleDescriptor.load_classes():
+        templates[category] = descriptor.templates()
+
+    return templates
+
+
+class TemplateTestSystem(MakoDescriptorSystem):
+    """
+    This system exists to help verify that XModuleDescriptors can be instantiated
+    from their defined templates before we load the templates into the modulestore.
+    """
+    def __init__(self):
+        super(TemplateTestSystem, self).__init__(
+            lambda *a, **k: None,
+            MemoryFS(),
+            lambda msg: None,
+            render_template=lambda *a, **k: None,
+        )
+
+
+def update_templates():
+    """
+    Updates the set of templates in the modulestore with all templates currently
+    available from the installed plugins
+    """
+
+    for category, templates in all_templates().items():
+        for template in templates:
+            if 'display_name' not in template.metadata:
+                log.warning('No display_name specified in template {0}, skipping'.format(template))
+                continue
+
+            template_location = Location('i4x', 'edx', 'templates', category, Location.clean_for_url_name(template.metadata['display_name']))
+
+            try:
+                json_data = template._asdict()
+                json_data['location'] = template_location.dict()
+                XModuleDescriptor.load_from_json(json_data, TemplateTestSystem())
+            except:
+                log.warning('Unable to instantiate {cat} from template {template}, skipping'.format(
+                    cat=category,
+                    template=template
+                ), exc_info=True)
+                continue
+
+            modulestore().update_item(template_location, template.data)
+            modulestore().update_children(template_location, template.children)
+            modulestore().update_metadata(template_location, template.metadata)

common/lib/xmodule/xmodule/templates/default/empty.yaml

+---
+metadata:
+    display_name: Empty
+data: ""
+children: []

common/lib/xmodule/xmodule/templates/problem/multiline.yaml

+---
+metadata:
+    display_name: Multiline XML
+data: |
+    <problem>
+    </problem>
+children: []

common/lib/xmodule/xmodule/templates/sequential/with_video.yaml

+---
+metadata:
+    display_name: Sequence with Video
+data: ''
+children:
+    - 'i4x://edx/templates/video/Empty'

common/lib/xmodule/xmodule/tests/test_import.py

@@ -3,12 +3,14 @@ import unittest
 from fs.memoryfs import MemoryFS
 
 from lxml import etree
+from mock import Mock, patch
+from collections import defaultdict
 
 from xmodule.x_module import XMLParsingSystem, XModuleDescriptor
 from xmodule.xml_module import is_pointer_tag
 from xmodule.errortracker import make_error_tracker
 from xmodule.modulestore import Location
-from xmodule.modulestore.xml import XMLModuleStore
+from xmodule.modulestore.xml import ImportSystem, XMLModuleStore
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
 from .test_export import DATA_DIR
@@ -16,35 +18,28 @@ from .test_export import DATA_DIR
 ORG = 'test_org'
 COURSE = 'test_course'
 
-class DummySystem(XMLParsingSystem):
-    def __init__(self):
 
-        self.modules = {}
-        self.resources_fs = MemoryFS()
-        self.errorlog = make_error_tracker()
+class DummySystem(ImportSystem):
 
-        def load_item(loc):
-            loc = Location(loc)
-            if loc in self.modules:
-                return self.modules[loc]
-
-            print "modules: "
-            print self.modules
-            raise ItemNotFoundError("Can't find item at loc: {0}".format(loc))
-
-        def process_xml(xml):
-            print "loading {0}".format(xml)
-            descriptor = XModuleDescriptor.load_from_xml(xml, self, ORG, COURSE, None)
-            # Need to save module so we can find it later
-            self.modules[descriptor.location] = descriptor
-
-            # always eager
-            descriptor.get_children()
-            return descriptor
+    @patch('xmodule.modulestore.xml.OSFS', lambda dir: MemoryFS())
+    def __init__(self, load_error_modules):
 
+        xmlstore = XMLModuleStore("data_dir", course_dirs=[], load_error_modules=load_error_modules)
+        course_id = "/".join([ORG, COURSE, 'test_run'])
+        course_dir = "test_dir"
         policy = {}
-        XMLParsingSystem.__init__(self, load_item, self.resources_fs,
-                                  self.errorlog.tracker, process_xml, policy)
+        error_tracker = Mock()
+        parent_tracker = Mock()
+
+        super(DummySystem, self).__init__(
+            xmlstore,
+            course_id,
+            course_dir,
+            policy,
+            error_tracker,
+            parent_tracker,
+            load_error_modules=load_error_modules,
+        )
 
     def render_template(self, template, context):
             raise Exception("Shouldn't be called")
@@ -53,9 +48,9 @@ class DummySystem(XMLParsingSystem):
 class ImportTestCase(unittest.TestCase):
     '''Make sure module imports work properly, including for malformed inputs'''
     @staticmethod
-    def get_system():
+    def get_system(load_error_modules=True):
         '''Get a dummy system'''
-        return DummySystem()
+        return DummySystem(load_error_modules)
 
     def test_fallback(self):
         '''Check that malformed xml loads as an ErrorDescriptor.'''
@@ -63,8 +58,7 @@ class ImportTestCase(unittest.TestCase):
         bad_xml = '''<sequential display_name="oops"><video url="hi"></sequential>'''
         system = self.get_system()
 
-        descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course',
-                                                     None)
+        descriptor = system.process_xml(bad_xml)
 
         self.assertEqual(descriptor.__class__.__name__,
                          'ErrorDescriptor')
@@ -76,11 +70,8 @@ class ImportTestCase(unittest.TestCase):
         bad_xml2 = '''<sequential url_name="oops"><video url="hi"></sequential>'''
         system = self.get_system()
 
-        descriptor1 = XModuleDescriptor.load_from_xml(bad_xml, system, 'org',
-                                                      'course', None)
-
-        descriptor2 = XModuleDescriptor.load_from_xml(bad_xml2, system, 'org',
-                                                      'course', None)
+        descriptor1 = system.process_xml(bad_xml)
+        descriptor2 = system.process_xml(bad_xml2)
 
         self.assertNotEqual(descriptor1.location, descriptor2.location)
 
@@ -91,13 +82,12 @@ class ImportTestCase(unittest.TestCase):
         self.maxDiff = None
         bad_xml = '''<sequential display_name="oops"><video url="hi"></sequential>'''
         system = self.get_system()
-        descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course',
-                                                     None)
+        descriptor = system.process_xml(bad_xml)
+
         resource_fs = None
         tag_xml = descriptor.export_to_xml(resource_fs)
-        re_import_descriptor = XModuleDescriptor.load_from_xml(tag_xml, system,
-                                                               'org', 'course',
-                                                               None)
+        re_import_descriptor = system.process_xml(tag_xml)
+
         self.assertEqual(re_import_descriptor.__class__.__name__,
                          'ErrorDescriptor')
 
@@ -116,8 +106,8 @@ class ImportTestCase(unittest.TestCase):
 
         # load it
         system = self.get_system()
-        descriptor = XModuleDescriptor.load_from_xml(xml_str_in, system, 'org', 'course',
-                                                     None)
+        descriptor = system.process_xml(xml_str_in)
+
         # export it
         resource_fs = None
         xml_str_out = descriptor.export_to_xml(resource_fs)
@@ -133,8 +123,6 @@ class ImportTestCase(unittest.TestCase):
         """
         system = self.get_system()
         v = '1 hour'
-        org = 'foo'
-        course = 'bbhh'
         url_name = 'test1'
         start_xml = '''
         <course org="{org}" course="{course}"
@@ -142,9 +130,8 @@ class ImportTestCase(unittest.TestCase):
             <chapter url="hi" url_name="ch" display_name="CH">
                 <html url_name="h" display_name="H">Two houses, ...</html>
             </chapter>
-        </course>'''.format(grace=v, org=org, course=course, url_name=url_name)
-        descriptor = XModuleDescriptor.load_from_xml(start_xml, system,
-                                                     org, course)
+        </course>'''.format(grace=v, org=ORG, course=COURSE, url_name=url_name)
+        descriptor = system.process_xml(start_xml)
 
         print descriptor, descriptor.metadata
         self.assertEqual(descriptor.metadata['graceperiod'], v)
@@ -166,8 +153,8 @@ class ImportTestCase(unittest.TestCase):
         pointer = etree.fromstring(exported_xml)
         self.assertTrue(is_pointer_tag(pointer))
         # but it's a special case course pointer
-        self.assertEqual(pointer.attrib['course'], course)
-        self.assertEqual(pointer.attrib['org'], org)
+        self.assertEqual(pointer.attrib['course'], COURSE)
+        self.assertEqual(pointer.attrib['org'], ORG)
 
         # Does the course still have unicorns?
         with resource_fs.open('course/{url_name}.xml'.format(url_name=url_name)) as f:
@@ -317,3 +304,10 @@ class ImportTestCase(unittest.TestCase):
 
             self.assertEqual(len(video.url_name), len('video_') + 12)
 
+    def test_error_on_import(self):
+        '''Check that when load_error_module is false, an exception is raised, rather than returning an ErrorModule'''
+
+        bad_xml = '''<sequential display_name="oops"><video url="hi"></sequential>'''
+        system = self.get_system(False)
+
+        self.assertRaises(etree.XMLSyntaxError, system.process_xml, bad_xml)

common/lib/xmodule/xmodule/x_module.py

 import logging
 import pkg_resources
-import sys
+import yaml
+import os
 
-from fs.errors import ResourceNotFoundError
 from functools import partial
 from lxml import etree
-from lxml.etree import XMLSyntaxError
 from pprint import pprint
+from collections import namedtuple
+from pkg_resources import resource_listdir, resource_string, resource_isdir
 
-from xmodule.errortracker import exc_info_to_str
 from xmodule.modulestore import Location
 from xmodule.timeparse import parse_time
 
@@ -71,7 +71,11 @@ class Plugin(object):
 
     @classmethod
     def load_classes(cls):
-        return [class_.load()
+        """
+        Returns a list of containing the identifiers and their corresponding classes for all
+        of the available instances of this plugin
+        """
+        return [(class_.name, class_.load())
                 for class_
                 in pkg_resources.iter_entry_points(cls.entry_point)]
 
@@ -211,6 +215,7 @@ class XModule(HTMLSnippet):
         '''
         return self.metadata.get('display_name',
                                  self.url_name.replace('_', ' '))
+
     def __unicode__(self):
         return '<x_module(id={0})>'.format(self.id)
 
@@ -321,7 +326,39 @@ def policy_key(location):
     return '{cat}/{name}'.format(cat=location.category, name=location.name)
 
 
-class XModuleDescriptor(Plugin, HTMLSnippet):
+Template = namedtuple("Template", "metadata data children")
+
+
+class ResourceTemplates(object):
+    @classmethod
+    def templates(cls):
+        """
+        Returns a list of Template objects that describe possible templates that can be used
+        to create a module of this type.
+        If no templates are provided, there will be no way to create a module of
+        this type
+
+        Expects a class attribute template_dir_name that defines the directory
+        inside the 'templates' resource directory to pull templates from
+        """
+        templates = []
+        dirname = os.path.join('templates', cls.template_dir_name)
+        if not resource_isdir(__name__, dirname):
+            log.warning("No resource directory {dir} found when loading {cls_name} templates".format(
+                dir=dirname,
+                cls_name=cls.__name__,
+            ))
+            return []
+
+        for template_file in resource_listdir(__name__, dirname):
+            template_content = resource_string(__name__, os.path.join(dirname, template_file))
+            template = yaml.load(template_content)
+            templates.append(Template(**template))
+
+        return templates
+
+
+class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates):
     """
     An XModuleDescriptor is a specification for an element of a course. This
     could be a problem, an organizational element (a group of content), or a
@@ -361,6 +398,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
     equality_attributes = ('definition', 'metadata', 'location',
                            'shared_state_key', '_inherited_metadata')
 
+    # Name of resource directory to load templates from
+    template_dir_name = "default"
+
     # ============================= STRUCTURAL MANIPULATION ===================
     def __init__(self,
                  system,
@@ -440,10 +480,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         """
         Return the metadata that is not inherited, but was defined on this module.
         """
-        return dict((k,v) for k,v in self.metadata.items()
+        return dict((k, v) for k, v in self.metadata.items()
                     if k not in self._inherited_metadata)
 
-
     @staticmethod
     def compute_inherited_metadata(node):
         """Given a descriptor, traverse all of its descendants and do metadata
@@ -484,7 +523,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
 
         return self._child_instances
 
-
     def get_child_by_url_name(self, url_name):
         """
         Return a child XModuleDescriptor with the specified url_name, if it exists, and None otherwise.
@@ -568,7 +606,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         org and course are optional strings that will be used in the generated
             module's url identifiers
         """
-        try:
         class_ = XModuleDescriptor.load_class(
             etree.fromstring(xml_data).tag,
             default_class
@@ -577,27 +614,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         # log.debug('[XModuleDescriptor.load_from_xml] tag=%s, class_=%s' % (
         #        etree.fromstring(xml_data).tag,class_))
 
-            descriptor = class_.from_xml(xml_data, system, org, course)
-        except Exception as err:
-            # Didn't load properly.  Fall back on loading as an error
-            # descriptor.  This should never error due to formatting.
-
-            # Put import here to avoid circular import errors
-            from xmodule.error_module import ErrorDescriptor
-            msg = "Error loading from xml."
-            log.warning(msg + " " + str(err)[:200])
-
-            # Normally, we don't want lots of exception traces in our logs from common
-            # content problems.  But if you're debugging the xml loading code itself,
-            # uncomment the next line.
-            # log.exception(msg)
-
-            system.error_tracker(msg)
-            err_msg = msg + "\n" + exc_info_to_str(sys.exc_info())
-            descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course,
-                                                  err_msg)
-
-        return descriptor
+        return class_.from_xml(xml_data, system, org, course)
 
     @classmethod
     def from_xml(cls, xml_data, system, org=None, course=None):
@@ -682,7 +699,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         return None
 
 
-
 class DescriptorSystem(object):
     def __init__(self, load_item, resources_fs, error_tracker, **kwargs):
         """

lms/envs/common.py

@@ -468,7 +468,7 @@ for dir_ in (js_file_dir, css_file_dir):
 
 js_fragments = set()
 css_fragments = defaultdict(set)
-for descriptor in XModuleDescriptor.load_classes() + [HiddenDescriptor]:
+for _, descriptor in XModuleDescriptor.load_classes() + [(None, HiddenDescriptor)]:
     module_js = descriptor.module_class.get_javascript()
     for filetype in ('coffee', 'js'):
         for idx, fragment in enumerate(module_js.get(filetype, [])):