Merge pull request #6035 from edx/jeskew/xmodule_to_xblock_vertical

Convert Vertical XModule to pure Vertical XBlock.

cms/djangoapps/contentstore/features/component.py

@@ -61,7 +61,7 @@ def see_a_multi_step_component(step, category):
                 'Raw HTML': '<p>This template is similar to the Text template. The only difference is',
             }
             actual_html = world.css_html(selector, index=idx)
-            assert_in(html_matcher[step_hash['Component']], actual_html)
+            assert_in(html_matcher[step_hash['Component']].strip(), actual_html.strip())
         else:
             actual_text = world.css_text(selector, index=idx)
             assert_in(step_hash['Component'].upper(), actual_text)

cms/djangoapps/contentstore/features/pages.py

@@ -27,7 +27,7 @@ def add_page(step):
 def see_a_static_page_named_foo(step, name):
     pages_css = 'div.xmodule_StaticTabModule'
     page_name_html = world.css_html(pages_css)
-    assert_equal(page_name_html, '\n    {name}\n'.format(name=name))
+    assert_equal(page_name_html.strip(), name)
 
 
 @step(u'I should not see any static pages$')

cms/djangoapps/contentstore/views/tests/test_item.py

@@ -5,6 +5,7 @@ import ddt
 
 from mock import patch, Mock, PropertyMock
 from pytz import UTC
+from pyquery import PyQuery
 from webob import Response
 
 from django.http import Http404
@@ -1026,7 +1027,8 @@ class TestEditItemSplitMongo(TestEditItemSetup):
         for __ in xrange(3):
             resp = self.client.get(view_url, HTTP_ACCEPT='application/json')
             self.assertEqual(resp.status_code, 200)
-            self.assertEqual(resp.content.count('xblock-{}'.format(STUDIO_VIEW)), 1)
+            content = json.loads(resp.content)
+            self.assertEqual(len(PyQuery(content['html'])('.xblock-{}'.format(STUDIO_VIEW))), 1)
 
 
 class TestEditSplitModule(ItemTest):

common/djangoapps/xmodule_modifiers.py

@@ -9,16 +9,19 @@ import static_replace
 import uuid
 import markupsafe
 from lxml import html, etree
+from contracts import contract
 
 from django.conf import settings
 from django.utils.timezone import UTC
 from django.utils.html import escape
+from django.contrib.auth.models import User
 from edxmako.shortcuts import render_to_string
+from xblock.core import XBlock
 from xblock.exceptions import InvalidScopeError
 from xblock.fragment import Fragment
 
 from xmodule.seq_module import SequenceModule
-from xmodule.vertical_module import VerticalModule
+from xmodule.vertical_block import VerticalBlock
 from xmodule.x_module import shim_xmodule_js, XModuleDescriptor, XModule, PREVIEW_VIEWS, STUDIO_VIEW
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
@@ -77,7 +80,11 @@ def wrap_xblock(runtime_class, block, view, frag, context, usage_id_serializer,
 
     css_classes = [
         'xblock',
-        'xblock-{}'.format(markupsafe.escape(view))
+        'xblock-{}'.format(markupsafe.escape(view)),
+        'xblock-{}-{}'.format(
+            markupsafe.escape(view),
+            markupsafe.escape(block.scope_ids.block_type),
+        )
     ]
 
     if isinstance(block, (XModule, XModuleDescriptor)):
@@ -90,7 +97,7 @@ def wrap_xblock(runtime_class, block, view, frag, context, usage_id_serializer,
 
         css_classes.append('xmodule_' + markupsafe.escape(class_name))
         data['type'] = block.js_module_name
-        shim_xmodule_js(frag)
+        shim_xmodule_js(block, frag)
 
     if frag.js_init_fn:
         data['init'] = frag.js_init_fn
@@ -189,6 +196,7 @@ def grade_histogram(module_id):
     return grades
 
 
+@contract(user=User, has_instructor_access=bool, block=XBlock, view=basestring, frag=Fragment, context=dict)
 def add_staff_markup(user, has_instructor_access, block, view, frag, context):  # pylint: disable=unused-argument
     """
     Updates the supplied module with a new get_html function that wraps
@@ -200,7 +208,7 @@ def add_staff_markup(user, has_instructor_access, block, view, frag, context):
     Does nothing if module is a SequenceModule.
     """
     # TODO: make this more general, eg use an XModule attribute instead
-    if isinstance(block, VerticalModule) and (not context or not context.get('child_of_vertical', False)):
+    if isinstance(block, VerticalBlock) and (not context or not context.get('child_of_vertical', False)):
         # check that the course is a mongo backed Studio course before doing work
         is_mongo_course = modulestore().get_modulestore_type(block.location.course_key) != ModuleStoreEnum.Type.xml
         is_studio_course = block.course_edit_method == "Studio"

common/lib/safe_lxml/safe_lxml/etree.py

@@ -9,6 +9,7 @@ For processing xml always prefer this over using lxml.etree directly.
 
 from lxml.etree import *  # pylint: disable=wildcard-import, unused-wildcard-import
 from lxml.etree import XMLParser as _XMLParser
+from lxml.etree import _ElementTree  # pylint: disable=unused-import
 
 # This should be imported after lxml.etree so that it overrides the following attributes.
 from defusedxml.lxml import parse, fromstring, XML

common/lib/xmodule/setup.py

@@ -22,7 +22,6 @@ XMODULES = [
     "section = xmodule.backcompat_module:SemanticSectionDescriptor",
     "sequential = xmodule.seq_module:SequenceDescriptor",
     "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor",
-    "vertical = xmodule.vertical_module:VerticalDescriptor",
     "video = xmodule.video_module:VideoDescriptor",
     "videoalpha = xmodule.video_module:VideoDescriptor",
     "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor",
@@ -32,7 +31,6 @@ XMODULES = [
     "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",
     "textannotation = xmodule.textannotation_module:TextAnnotationDescriptor",
@@ -47,6 +45,8 @@ XMODULES = [
 ]
 XBLOCKS = [
     "library = xmodule.library_root_xblock:LibraryRoot",
+    "vertical = xmodule.vertical_block:VerticalBlock",
+    "wrapper = xmodule.wrapper_module:WrapperBlock",
 ]
 
 setup(

common/lib/xmodule/xmodule/js/src/poll/poll_main.js

@@ -273,7 +273,7 @@ function PollMain(el) {
 
             if (
                 (tempEl.tagName.toLowerCase() === 'div') &&
-                ($(tempEl).hasClass('xmodule_WrapperModule') === true)
+                ($(tempEl).data('block-type') === 'wrapper')
             ) {
                 _this.wrapperSectionEl = tempEl;
 

common/lib/xmodule/xmodule/js/src/xmodule.js

@@ -58,14 +58,20 @@
         return Descriptor;
     }());
 
-    this.XBlockToXModuleShim = function (runtime, element) {
+    this.XBlockToXModuleShim = function (runtime, element, initArgs) {
         /*
          * Load a single module (either an edit module or a display module)
          * from the supplied element, which should have a data-type attribute
          * specifying the class to load
          */
-        var moduleType = $(element).data('type'),
-            module;
+        var moduleType, module;
+
+        if (initArgs) {
+            moduleType = initArgs['xmodule-type'];
+        }
+        if (!moduleType) {
+            moduleType = $(element).data('type');
+        }
 
         if (moduleType === 'None') {
             return;

common/lib/xmodule/xmodule/mako_module.py

-from .x_module import XModuleDescriptor, DescriptorSystem
+"""
+Code to handle mako templating for XModules and XBlocks.
+"""
+from xblock.fragment import Fragment
+
+from .x_module import XModuleDescriptor, DescriptorSystem, shim_xmodule_js
 
 
 class MakoDescriptorSystem(DescriptorSystem):
@@ -8,20 +13,19 @@ class MakoDescriptorSystem(DescriptorSystem):
         self.render_template = render_template
 
 
-class MakoModuleDescriptor(XModuleDescriptor):
+class MakoTemplateBlockBase(object):
     """
-    Module descriptor intended as a mixin that uses a mako template
+    XBlock intended as a mixin that uses a mako template
     to specify the module html.
 
     Expects the descriptor to have the `mako_template` attribute set
     with the name of the template to render, and it will pass
     the descriptor as the `module` parameter to that template
-
-    MakoModuleDescriptor.__init__ takes the same arguments as xmodule.x_module:XModuleDescriptor.__init__
     """
+    # pylint: disable=no-member
 
     def __init__(self, *args, **kwargs):
-        super(MakoModuleDescriptor, self).__init__(*args, **kwargs)
+        super(MakoTemplateBlockBase, self).__init__(*args, **kwargs)
         if getattr(self.runtime, 'render_template', None) is None:
             raise TypeError(
                 '{runtime} must have a render_template function'
@@ -39,6 +43,21 @@ class MakoModuleDescriptor(XModuleDescriptor):
             'editable_metadata_fields': self.editable_metadata_fields
         }
 
+    def studio_view(self, context):  # pylint: disable=unused-argument
+        """
+        View used in Studio.
+        """
+        # pylint: disable=no-member
+        fragment = Fragment(
+            self.system.render_template(self.mako_template, self.get_context())
+        )
+        shim_xmodule_js(self, fragment)
+        return fragment
+
+
+class MakoModuleDescriptor(MakoTemplateBlockBase, XModuleDescriptor):  # pylint: disable=abstract-method
+    """
+    Mixin to use for XModule descriptors.
+    """
     def get_html(self):
-        return self.system.render_template(
-            self.mako_template, self.get_context())
+        return self.studio_view(None).content

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

@@ -47,7 +47,7 @@ class DraftModuleStore(MongoModuleStore):
     This module also includes functionality to promote DRAFT modules (and their children)
     to published modules.
     """
-    def get_item(self, usage_key, depth=0, revision=None, **kwargs):
+    def get_item(self, usage_key, depth=0, revision=None, using_descriptor_system=None, **kwargs):
         """
         Returns an XModuleDescriptor instance for the item at usage_key.
 
@@ -70,6 +70,9 @@ class DraftModuleStore(MongoModuleStore):
                 Note: If the item is in DIRECT_ONLY_CATEGORIES, then returns only the PUBLISHED
                 version regardless of the revision.
 
+            using_descriptor_system (CachingDescriptorSystem): The existing CachingDescriptorSystem
+                to add data to, and to load the XBlocks from.
+
         Raises:
             xmodule.modulestore.exceptions.InsufficientSpecificationError
             if any segment of the usage_key is None except revision
@@ -78,10 +81,14 @@ class DraftModuleStore(MongoModuleStore):
             is found at that usage_key
         """
         def get_published():
-            return wrap_draft(super(DraftModuleStore, self).get_item(usage_key, depth=depth))
+            return wrap_draft(super(DraftModuleStore, self).get_item(
+                usage_key, depth=depth, using_descriptor_system=using_descriptor_system
+            ))
 
         def get_draft():
-            return wrap_draft(super(DraftModuleStore, self).get_item(as_draft(usage_key), depth=depth))
+            return wrap_draft(super(DraftModuleStore, self).get_item(
+                as_draft(usage_key), depth=depth, using_descriptor_system=using_descriptor_system
+            ))
 
         # return the published version if ModuleStoreEnum.RevisionOption.published_only is requested
         if revision == ModuleStoreEnum.RevisionOption.published_only:

common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py

@@ -20,6 +20,7 @@ from nose.plugins.attrib import attr
 import pymongo
 from pytz import UTC
 
+from xmodule.x_module import XModuleMixin
 from xmodule.modulestore.edit_info import EditInfoMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.modulestore.tests.test_cross_modulestore_import_export import MongoContentstoreBuilder
@@ -73,7 +74,7 @@ class TestMixedModuleStore(CourseComparisonTest):
         'default_class': DEFAULT_CLASS,
         'fs_root': DATA_DIR,
         'render_template': RENDER_TEMPLATE,
-        'xblock_mixins': (EditInfoMixin, InheritanceMixin, LocationMixin),
+        'xblock_mixins': (EditInfoMixin, InheritanceMixin, LocationMixin, XModuleMixin),
     }
     DOC_STORE_CONFIG = {
         'host': HOST,

common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py

@@ -41,8 +41,10 @@ from git.test.lib.asserts import assert_not_none
 from xmodule.x_module import XModuleMixin
 from xmodule.modulestore.mongo.base import as_draft
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
+from xmodule.modulestore.tests.utils import LocationMixin
 from xmodule.modulestore.edit_info import EditInfoMixin
 from xmodule.modulestore.exceptions import ItemNotFoundError
+from xmodule.modulestore.inheritance import InheritanceMixin
 
 
 log = logging.getLogger(__name__)
@@ -124,7 +126,7 @@ class TestMongoModuleStoreBase(unittest.TestCase):
             doc_store_config, FS_ROOT, RENDER_TEMPLATE,
             default_class=DEFAULT_CLASS,
             branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred,
-            xblock_mixins=(EditInfoMixin,)
+            xblock_mixins=(EditInfoMixin, InheritanceMixin, LocationMixin, XModuleMixin)
 
         )
         import_course_from_xml(

common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py

@@ -8,6 +8,7 @@ import mock
 
 from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator
 from xmodule.modulestore import ModuleStoreEnum
+from xmodule.x_module import XModuleMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.modulestore.mongo import DraftMongoModuleStore
 from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore
@@ -41,7 +42,7 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase):
         'default_class': 'xmodule.raw_module.RawDescriptor',
         'fs_root': '',
         'render_template': mock.Mock(return_value=""),
-        'xblock_mixins': (InheritanceMixin,)
+        'xblock_mixins': (InheritanceMixin, XModuleMixin)
     }
 
     split_course_key = CourseLocator('test_org', 'test_course', 'runid', branch=ModuleStoreEnum.BranchName.draft)

common/lib/xmodule/xmodule/modulestore/tests/test_xml.py

@@ -9,6 +9,7 @@ from mock import patch
 
 from xmodule.modulestore.xml import XMLModuleStore
 from xmodule.modulestore import ModuleStoreEnum
+from xmodule.x_module import XModuleMixin
 
 from xmodule.tests import DATA_DIR
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -46,7 +47,11 @@ class TestXMLModuleStore(unittest.TestCase):
 
         # Load the course, but don't make error modules.  This will succeed,
         # but will record the errors.
-        modulestore = XMLModuleStore(DATA_DIR, source_dirs=['toy'], load_error_modules=False)
+        modulestore = XMLModuleStore(
+            DATA_DIR,
+            source_dirs=['toy'],
+            xblock_mixins=(XModuleMixin,),
+            load_error_modules=False)
 
         # Look up the errors during load. There should be none.
         errors = modulestore.get_course_errors(SlashSeparatedCourseKey("edX", "toy", "2012_Fall"))
@@ -119,7 +124,11 @@ class TestXMLModuleStore(unittest.TestCase):
         """
         Test a course whose structure is not a tree.
         """
-        store = XMLModuleStore(DATA_DIR, source_dirs=['xml_dag'])
+        store = XMLModuleStore(
+            DATA_DIR,
+            source_dirs=['xml_dag'],
+            xblock_mixins=(XModuleMixin,),
+        )
         course_key = store.get_courses()[0].id
 
         mock_logging.warning.assert_called_with(

common/lib/xmodule/xmodule/modulestore/tests/utils.py

@@ -5,6 +5,7 @@ from importlib import import_module
 from opaque_keys.edx.keys import UsageKey
 from unittest import TestCase
 from xblock.fields import XBlockMixin
+from xmodule.x_module import XModuleMixin
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished
 from xmodule.modulestore.edit_info import EditInfoMixin
@@ -84,7 +85,7 @@ class MixedSplitTestCase(TestCase):
         'default_class': 'xmodule.raw_module.RawDescriptor',
         'fs_root': DATA_DIR,
         'render_template': RENDER_TEMPLATE,
-        'xblock_mixins': (EditInfoMixin, InheritanceMixin, LocationMixin),
+        'xblock_mixins': (EditInfoMixin, InheritanceMixin, LocationMixin, XModuleMixin),
     }
     DOC_STORE_CONFIG = {
         'host': MONGO_HOST,

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

@@ -32,7 +32,7 @@ from lxml import etree
 
 from xmodule.modulestore.xml import XMLModuleStore, LibraryXMLModuleStore, ImportSystem
 from xblock.runtime import KvsFieldData, DictKeyValueStore
-from xmodule.x_module import XModuleDescriptor
+from xmodule.x_module import XModuleDescriptor, XModuleMixin
 from opaque_keys.edx.keys import UsageKey
 from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict
 from xmodule.contentstore.content import StaticContent
@@ -47,6 +47,7 @@ from xmodule.modulestore.exceptions import DuplicateCourseError
 from xmodule.modulestore.mongo.base import MongoRevisionKey
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots
+from xmodule.modulestore.tests.utils import LocationMixin
 
 
 log = logging.getLogger(__name__)
@@ -1031,7 +1032,8 @@ def validate_course_policy(module_store, course_id):
 def perform_xlint(
         data_dir, source_dirs,
         default_class='xmodule.raw_module.RawDescriptor',
-        load_error_modules=True):
+        load_error_modules=True,
+        xblock_mixins=(LocationMixin, XModuleMixin)):
     err_cnt = 0
     warn_cnt = 0
 
@@ -1039,7 +1041,8 @@ def perform_xlint(
         data_dir,
         default_class=default_class,
         source_dirs=source_dirs,
-        load_error_modules=load_error_modules
+        load_error_modules=load_error_modules,
+        xblock_mixins=xblock_mixins
     )
 
     # check all data source path information

common/lib/xmodule/xmodule/studio_editable.py

@@ -4,13 +4,13 @@ Mixin to support editing in Studio.
 from xmodule.x_module import module_attr, STUDENT_VIEW, AUTHOR_VIEW
 
 
-class StudioEditableModule(object):
+class StudioEditableBlock(object):
     """
-    Helper methods for supporting Studio editing of xmodules.
+    Helper methods for supporting Studio editing of XBlocks.
 
-    This class is only intended to be used with an XModule, as it assumes the existence of
-    self.descriptor and self.system.
+    This class is only intended to be used with an XBlock!
     """
+    has_author_view = True
 
     def render_children(self, context, fragment, can_reorder=False, can_add=False):
         """
@@ -19,15 +19,14 @@ class StudioEditableModule(object):
         """
         contents = []
 
-        for child in self.descriptor.get_children():  # pylint: disable=no-member
+        for child in self.get_children():  # pylint: disable=no-member
             if can_reorder:
                 context['reorderable_items'].add(child.location)
-            child_module = self.system.get_module(child)  # pylint: disable=no-member
-            rendered_child = child_module.render(StudioEditableModule.get_preview_view_name(child_module), context)
+            rendered_child = child.render(StudioEditableModule.get_preview_view_name(child), context)
             fragment.add_frag_resources(rendered_child)
 
             contents.append({
-                'id': child.location.to_deprecated_string(),
+                'id': unicode(child.location),
                 'content': rendered_child.content
             })
 
@@ -46,6 +45,9 @@ class StudioEditableModule(object):
         return AUTHOR_VIEW if hasattr(block, AUTHOR_VIEW) else STUDENT_VIEW
 
 
+StudioEditableModule = StudioEditableBlock
+
+
 class StudioEditableDescriptor(object):
     """
     Helper mixin for supporting Studio editing of xmodules.

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

@@ -86,6 +86,21 @@ class TestModuleSystem(ModuleSystem):  # pylint: disable=abstract-method
     def get_asides(self, block):
         return []
 
+    def __repr__(self):
+        """
+        Custom hacky repr.
+        XBlock.Runtime.render() replaces the _view_name attribute while rendering, which
+        causes rendered comparisons of blocks to fail as unequal. So make the _view_name
+        attribute None during the base repr - and set it back to original value afterward.
+        """
+        orig_view_name = None
+        if hasattr(self, '_view_name'):
+            orig_view_name = self._view_name
+        self._view_name = None
+        rt_repr = super(TestModuleSystem, self).__repr__()
+        self._view_name = orig_view_name
+        return rt_repr
+
 
 def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')):
     """
@@ -128,7 +143,7 @@ def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')):
         render_template=mock_render_template,
         replace_urls=str,
         user=user,
-        get_real_user=lambda(__): user,
+        get_real_user=lambda __: user,
         filestore=Mock(name='get_test_system.filestore'),
         debug=True,
         hostname="edx.org",
@@ -363,11 +378,17 @@ class CourseComparisonTest(BulkAssertionTest):
         )
 
     def assertBlocksEqualByFields(self, expected_block, actual_block):
+        """
+        Compare block fields to check for equivalence.
+        """
         self.assertEqual(expected_block.fields, actual_block.fields)
         for field in expected_block.fields.values():
             self.assertFieldEqual(field, expected_block, actual_block)
 
     def assertFieldEqual(self, field, expected_block, actual_block):
+        """
+        Compare a single block field for equivalence.
+        """
         if isinstance(field, (Reference, ReferenceList, ReferenceValueDict)):
             self.assertReferenceRelativelyEqual(field, expected_block, actual_block)
         else:
@@ -421,6 +442,9 @@ class CourseComparisonTest(BulkAssertionTest):
                     self._assertCoursesEqual(expected_items, actual_items, actual_course_key, expect_drafts=True)
 
     def _assertCoursesEqual(self, expected_items, actual_items, actual_course_key, expect_drafts=False):
+        """
+        Actual algorithm to compare courses.
+        """
         with self.bulk_assertions():
             self.assertEqual(len(expected_items), len(actual_items))
 

common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py

@@ -7,7 +7,7 @@ import unittest
 import copy
 
 from xmodule.crowdsource_hinter import CrowdsourceHinterModule
-from xmodule.vertical_module import VerticalModule, VerticalDescriptor
+from xmodule.vertical_block import VerticalBlock
 from xmodule.x_module import STUDENT_VIEW
 from xblock.field_data import DictFieldData
 from xblock.fragment import Fragment
@@ -203,8 +203,8 @@ class VerticalWithModulesFactory(object):
         """Make a vertical."""
         field_data = {'data': VerticalWithModulesFactory.sample_problem_xml}
         system = get_test_system()
-        descriptor = VerticalDescriptor.from_xml(VerticalWithModulesFactory.sample_problem_xml, system)
-        module = VerticalModule(system, descriptor, field_data)
+        descriptor = VerticalBlock.parse_xml(VerticalWithModulesFactory.sample_problem_xml, system)
+        module = VerticalBlock(system, descriptor, field_data)
 
         return module
 

common/lib/xmodule/xmodule/tests/test_studio_editable.py

@@ -2,11 +2,14 @@
 Tests for StudioEditableModule.
 """
 
-from xmodule.tests.test_vertical import BaseVerticalModuleTest
+from xmodule.tests.test_vertical import BaseVerticalBlockTest
 from xmodule.x_module import AUTHOR_VIEW
 
 
-class StudioEditableModuleTestCase(BaseVerticalModuleTest):
+class StudioEditableModuleTestCase(BaseVerticalBlockTest):
+    """
+    Class containing StudioEditableModule tests.
+    """
     def test_render_reorderable_children(self):
         """
         Test the behavior of render_reorderable_children.

common/lib/xmodule/xmodule/tests/test_vertical.py

@@ -9,12 +9,15 @@ from xmodule.tests.xml import factories as xml
 from xmodule.x_module import STUDENT_VIEW, AUTHOR_VIEW
 
 
-class BaseVerticalModuleTest(XModuleXmlImportTest):
+class BaseVerticalBlockTest(XModuleXmlImportTest):
+    """
+    Tests for the BaseVerticalBlock.
+    """
     test_html_1 = 'Test HTML 1'
     test_html_2 = 'Test HTML 2'
 
     def setUp(self):
-        super(BaseVerticalModuleTest, self).setUp()
+        super(BaseVerticalBlockTest, self).setUp()
         # construct module
         course = xml.CourseFactory.build()
         sequence = xml.SequenceFactory.build(parent=course)
@@ -35,7 +38,10 @@ class BaseVerticalModuleTest(XModuleXmlImportTest):
         self.vertical.xmodule_runtime = self.module_system
 
 
-class VerticalModuleTestCase(BaseVerticalModuleTest):
+class VerticalBlockTestCase(BaseVerticalBlockTest):
+    """
+    Tests for the VerticalBlock.
+    """
     def test_render_student_view(self):
         """
         Test the rendering of the student view.

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

@@ -23,6 +23,7 @@ from unittest.case import SkipTest, TestCase
 
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
+from xblock.core import XBlock
 
 from opaque_keys.edx.locations import Location
 
@@ -42,8 +43,8 @@ from xmodule.crowdsource_hinter import CrowdsourceHinterDescriptor
 from xmodule.seq_module import SequenceDescriptor
 from xmodule.conditional_module import ConditionalDescriptor
 from xmodule.randomize_module import RandomizeDescriptor
-from xmodule.vertical_module import VerticalDescriptor
-from xmodule.wrapper_module import WrapperDescriptor
+from xmodule.vertical_block import VerticalBlock
+from xmodule.wrapper_module import WrapperBlock
 from xmodule.tests import get_test_descriptor_system, get_test_system
 
 
@@ -74,8 +75,8 @@ CONTAINER_XMODULES = {
     CrowdsourceHinterDescriptor: [{}],
     RandomizeDescriptor: [{}],
     SequenceDescriptor: [{}],
-    VerticalDescriptor: [{}],
-    WrapperDescriptor: [{}],
+    VerticalBlock: [{}],
+    WrapperBlock: [{}],
 }
 
 # These modules are editable in studio yet
@@ -141,7 +142,10 @@ class ContainerModuleRuntimeFactory(ModuleSystemFactory):
         if depth == 0:
             self.get_module.side_effect = lambda x: LeafModuleFactory(descriptor_cls=HtmlDescriptor)
         else:
-            self.get_module.side_effect = lambda x: ContainerModuleFactory(descriptor_cls=VerticalDescriptor, depth=depth - 1)
+            self.get_module.side_effect = lambda x: ContainerModuleFactory(
+                descriptor_cls=VerticalBlock,
+                depth=depth - 1
+            )
 
     @post_generation
     def position(self, create, position=2, **kwargs):  # pylint: disable=unused-argument, method-hidden
@@ -166,7 +170,10 @@ class ContainerDescriptorRuntimeFactory(DescriptorSystemFactory):
         if depth == 0:
             self.load_item.side_effect = lambda x: LeafModuleFactory(descriptor_cls=HtmlDescriptor)
         else:
-            self.load_item.side_effect = lambda x: ContainerModuleFactory(descriptor_cls=VerticalDescriptor, depth=depth - 1)
+            self.load_item.side_effect = lambda x: ContainerModuleFactory(
+                descriptor_cls=VerticalBlock,
+                depth=depth - 1
+            )
 
     @post_generation
     def position(self, create, position=2, **kwargs):  # pylint: disable=unused-argument, method-hidden
@@ -323,7 +330,12 @@ class TestStudentView(XBlockWrapperTestMixin, TestCase):
     This tests that student_view and XModule.get_html produce the same results.
     """
     def skip_if_invalid(self, descriptor_cls):
-        if descriptor_cls.module_class.student_view != XModule.student_view:
+        pure_xblock_class = issubclass(descriptor_cls, XBlock) and not issubclass(descriptor_cls, XModuleDescriptor)
+        if pure_xblock_class:
+            student_view = descriptor_cls.student_view
+        else:
+            student_view = descriptor_cls.module_class.student_view
+        if student_view != XModule.student_view:
             raise SkipTest(descriptor_cls.__name__ + " implements student_view")
 
     def check_property(self, descriptor):
@@ -344,7 +356,10 @@ class TestStudioView(XBlockWrapperTestMixin, TestCase):
         if descriptor_cls in NOT_STUDIO_EDITABLE:
             raise SkipTest(descriptor_cls.__name__ + " is not editable in studio")
 
-        if descriptor_cls.studio_view != XModuleDescriptor.studio_view:
+        pure_xblock_class = issubclass(descriptor_cls, XBlock) and not issubclass(descriptor_cls, XModuleDescriptor)
+        if pure_xblock_class:
+            raise SkipTest(descriptor_cls.__name__ + " is a pure XBlock and implements studio_view")
+        elif descriptor_cls.studio_view != XModuleDescriptor.studio_view:
             raise SkipTest(descriptor_cls.__name__ + " implements studio_view")
 
     def check_property(self, descriptor):

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

@@ -9,6 +9,7 @@ from factory import Factory, lazy_attribute, post_generation, Sequence
 from lxml import etree
 
 from xmodule.modulestore.inheritance import InheritanceMixin
+from xmodule.x_module import XModuleMixin
 from xmodule.modulestore import only_xmodules
 
 
@@ -66,7 +67,7 @@ class XmlImportFactory(Factory):
     FACTORY_FOR = XmlImportData
 
     filesystem = MemoryFS()
-    xblock_mixins = (InheritanceMixin,)
+    xblock_mixins = (InheritanceMixin, XModuleMixin)
     xblock_select = only_xmodules
     url_name = Sequence(str)
     attribs = {}

common/lib/xmodule/xmodule/vertical_module.py → common/lib/xmodule/xmodule/vertical_block.py

+"""
+VerticalBlock - an XBlock which renders its children in a column.
+"""
+import logging
+from copy import copy
+from lxml import etree
+from xblock.core import XBlock
 from xblock.fragment import Fragment
-from xmodule.x_module import XModule, STUDENT_VIEW
-from xmodule.seq_module import SequenceDescriptor
+from xmodule.mako_module import MakoTemplateBlockBase
 from xmodule.progress import Progress
-from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor
-from pkg_resources import resource_string
-from copy import copy
+from xmodule.seq_module import SequenceFields
+from xmodule.studio_editable import StudioEditableBlock
+from xmodule.x_module import STUDENT_VIEW, XModuleFields
+from xmodule.xml_module import XmlParserMixin
 
+log = logging.getLogger(__name__)
 
 # HACK: This shouldn't be hard-coded to two types
 # OBSOLETE: This obsoletes 'type'
-class_priority = ['video', 'problem']
-
+CLASS_PRIORITY = ['video', 'problem']
 
-class VerticalFields(object):
-    has_children = True
 
+class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParserMixin, MakoTemplateBlockBase, XBlock):
+    """
+    Layout XBlock for rendering subblocks vertically.
+    """
+    mako_template = 'widgets/sequence-edit.html'
+    js_module_name = "VerticalBlock"
 
-class VerticalModule(VerticalFields, XModule, StudioEditableModule):
-    ''' Layout module for laying out submodules vertically.'''
+    has_children = True
 
     def student_view(self, context):
+        """
+        Renders the student view of the block in the LMS.
+        """
         fragment = Fragment()
         contents = []
 
         child_context = {} if not context else copy(context)
         child_context['child_of_vertical'] = True
 
+        # pylint: disable=no-member
         for child in self.get_display_items():
             rendered_child = child.render(STUDENT_VIEW, child_context)
             fragment.add_frag_resources(rendered_child)
@@ -47,7 +61,7 @@ class VerticalModule(VerticalFields, XModule, StudioEditableModule):
         """
         fragment = Fragment()
         root_xblock = context.get('root_xblock')
-        is_root = root_xblock and root_xblock.location == self.location
+        is_root = root_xblock and root_xblock.location == self.location  # pylint: disable=no-member
 
         # For the container page we want the full drag-and-drop, but for unit pages we want
         # a more concise version that appears alongside the "View =>" link-- unless it is
@@ -57,37 +71,60 @@ class VerticalModule(VerticalFields, XModule, StudioEditableModule):
         return fragment
 
     def get_progress(self):
+        """
+        Returns the progress on this block and all children.
+        """
         # TODO: Cache progress or children array?
-        children = self.get_children()
+        children = self.get_children()  # pylint: disable=no-member
         progresses = [child.get_progress() for child in children]
         progress = reduce(Progress.add_counts, progresses, None)
         return progress
 
     def get_icon_class(self):
-        child_classes = set(child.get_icon_class() for child in self.get_children())
+        """
+        Returns the highest priority icon class.
+        """
+        child_classes = set(child.get_icon_class() for child in self.get_children())  # pylint: disable=no-member
         new_class = 'other'
-        for c in class_priority:
-            if c in child_classes:
-                new_class = c
+        for higher_class in CLASS_PRIORITY:
+            if higher_class in child_classes:
+                new_class = higher_class
         return new_class
 
-
-class VerticalDescriptor(VerticalFields, SequenceDescriptor, StudioEditableDescriptor):
-    """
-    Descriptor class for editing verticals.
-    """
-    module_class = VerticalModule
-
-    js = {'coffee': [resource_string(__name__, 'js/src/vertical/edit.coffee')]}
-    js_module_name = "VerticalDescriptor"
-
-    # TODO (victor): Does this need its own definition_to_xml method?  Otherwise it looks
-    # like verticals will get exported as sequentials...
+    @classmethod
+    def definition_from_xml(cls, xml_object, system):
+        children = []
+        for child in xml_object:
+            try:
+                child_block = system.process_xml(etree.tostring(child, encoding='unicode'))  # pylint: disable=no-member
+                children.append(child_block.scope_ids.usage_id)
+            except Exception as exc:  # pylint: disable=broad-except
+                log.exception("Unable to load child when parsing Vertical. Continuing...")
+                if system.error_tracker is not None:
+                    system.error_tracker(u"ERROR: {0}".format(exc))
+                continue
+        return {}, children
+
+    def definition_to_xml(self, resource_fs):
+        xml_object = etree.Element('vertical')  # pylint: disable=no-member
+        for child in self.get_children():  # pylint: disable=no-member
+            self.runtime.add_block_as_child_node(child, xml_object)
+        return xml_object
 
     @property
     def non_editable_metadata_fields(self):
-        non_editable_fields = super(VerticalDescriptor, self).non_editable_metadata_fields
+        """
+        Gather all fields which can't be edited.
+        """
+        non_editable_fields = super(VerticalBlock, self).non_editable_metadata_fields
         non_editable_fields.extend([
-            VerticalDescriptor.due,
+            self.fields['due'],
         ])
         return non_editable_fields
+
+    def studio_view(self, context):
+        fragment = super(VerticalBlock, self).studio_view(context)
+        # This continues to use the old XModuleDescriptor javascript code to enabled studio editing.
+        # TODO: Remove this when studio better supports editing of pure XBlocks.
+        fragment.add_javascript('VerticalBlock = XModule.Descriptor;')
+        return fragment

common/lib/xmodule/xmodule/wrapper_module.py

 # Same as vertical,
 # But w/o css delimiters between children
 
-from xmodule.vertical_module import VerticalModule, VerticalDescriptor
+from xmodule.vertical_block import VerticalBlock
 from pkg_resources import resource_string
 
 # HACK: This shouldn't be hard-coded to two types
@@ -9,14 +9,8 @@ from pkg_resources import resource_string
 class_priority = ['video', 'problem']
 
 
-class WrapperModule(VerticalModule):
-    ''' Layout module for laying out submodules vertically w/o css delimiters'''
-
-    has_children = True
-    css = {'scss': [resource_string(__name__, 'css/wrapper/display.scss')]}
-
-
-class WrapperDescriptor(VerticalDescriptor):
-    module_class = WrapperModule
-
-    has_children = True
+class WrapperBlock(VerticalBlock):
+    '''
+    Layout block for laying out sub-blocks vertically *w/o* css delimiters.
+    '''
+    pass

common/lib/xmodule/xmodule/x_module.py

@@ -239,15 +239,30 @@ class HTMLSnippet(object):
             .format(self.__class__))
 
 
-def shim_xmodule_js(fragment):
+def shim_xmodule_js(block, fragment):
     """
     Set up the XBlock -> XModule shim on the supplied :class:`xblock.fragment.Fragment`
     """
     if not fragment.js_init_fn:
         fragment.initialize_js('XBlockToXModuleShim')
+        fragment.json_init_args = {'xmodule-type': block.js_module_name}
 
 
-class XModuleMixin(XBlockMixin):
+class XModuleFields(object):
+    """
+    Common fields for XModules.
+    """
+    display_name = String(
+        display_name="Display Name",
+        help="This name appears in the horizontal navigation at the top of the page.",
+        scope=Scope.settings,
+        # it'd be nice to have a useful default but it screws up other things; so,
+        # use display_name_with_default for those
+        default=None
+    )
+
+
+class XModuleMixin(XModuleFields, XBlockMixin):
     """
     Fields and methods used by XModules internally.
 
@@ -278,15 +293,6 @@ class XModuleMixin(XBlockMixin):
     # in the module
     icon_class = 'other'
 
-    display_name = String(
-        display_name="Display Name",
-        help="This name appears in the horizontal navigation at the top of the page.",
-        scope=Scope.settings,
-        # it'd be nice to have a useful default but it screws up other things; so,
-        # use display_name_with_default for those
-        default=None
-    )
-
     def __init__(self, *args, **kwargs):
         self.xmodule_runtime = None
         self._child_instances = None
@@ -571,6 +577,95 @@ class XModuleMixin(XBlockMixin):
         self.xmodule_runtime = xmodule_runtime
         self._field_data = field_data
 
+    @property
+    def non_editable_metadata_fields(self):
+        """
+        Return the list of fields that should not be editable in Studio.
+
+        When overriding, be sure to append to the superclasses' list.
+        """
+        # We are not allowing editing of xblock tag and name fields at this time (for any component).
+        return [XBlock.tags, XBlock.name]
+
+    @property
+    def editable_metadata_fields(self):
+        """
+        Returns the metadata fields to be edited in Studio. These are fields with scope `Scope.settings`.
+
+        Can be limited by extending `non_editable_metadata_fields`.
+        """
+        metadata_fields = {}
+
+        # Only use the fields from this class, not mixins
+        fields = getattr(self, 'unmixed_class', self.__class__).fields
+
+        for field in fields.values():
+
+            if field.scope != Scope.settings or field in self.non_editable_metadata_fields:
+                continue
+
+            metadata_fields[field.name] = self._create_metadata_editor_info(field)
+
+        return metadata_fields
+
+    def _create_metadata_editor_info(self, field):
+        """
+        Creates the information needed by the metadata editor for a specific field.
+        """
+        def jsonify_value(field, json_choice):
+            """
+            Convert field value to JSON, if needed.
+            """
+            if isinstance(json_choice, dict):
+                new_json_choice = dict(json_choice)  # make a copy so below doesn't change the original
+                if 'display_name' in json_choice:
+                    new_json_choice['display_name'] = get_text(json_choice['display_name'])
+                if 'value' in json_choice:
+                    new_json_choice['value'] = field.to_json(json_choice['value'])
+            else:
+                new_json_choice = field.to_json(json_choice)
+            return new_json_choice
+
+        def get_text(value):
+            """Localize a text value that might be None."""
+            if value is None:
+                return None
+            else:
+                return self.runtime.service(self, "i18n").ugettext(value)
+
+        # gets the 'default_value' and 'explicitly_set' attrs
+        metadata_field_editor_info = self.runtime.get_field_provenance(self, field)
+        metadata_field_editor_info['field_name'] = field.name
+        metadata_field_editor_info['display_name'] = get_text(field.display_name)
+        metadata_field_editor_info['help'] = get_text(field.help)
+        metadata_field_editor_info['value'] = field.read_json(self)
+
+        # We support the following editors:
+        # 1. A select editor for fields with a list of possible values (includes Booleans).
+        # 2. Number editors for integers and floats.
+        # 3. A generic string editor for anything else (editing JSON representation of the value).
+        editor_type = "Generic"
+        values = field.values
+        if "values_provider" in field.runtime_options:
+            values = field.runtime_options['values_provider'](self)
+        if isinstance(values, (tuple, list)) and len(values) > 0:
+            editor_type = "Select"
+            values = [jsonify_value(field, json_choice) for json_choice in values]
+        elif isinstance(field, Integer):
+            editor_type = "Integer"
+        elif isinstance(field, Float):
+            editor_type = "Float"
+        elif isinstance(field, List):
+            editor_type = "List"
+        elif isinstance(field, Dict):
+            editor_type = "Dict"
+        elif isinstance(field, RelativeTime):
+            editor_type = "RelativeTime"
+        metadata_field_editor_info['type'] = editor_type
+        metadata_field_editor_info['options'] = [] if values is None else values
+
+        return metadata_field_editor_info
+
 
 class ProxyAttribute(object):
     """
@@ -965,92 +1060,6 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
             ")".format(self)
         )
 
-    @property
-    def non_editable_metadata_fields(self):
-        """
-        Return the list of fields that should not be editable in Studio.
-
-        When overriding, be sure to append to the superclasses' list.
-        """
-        # We are not allowing editing of xblock tag and name fields at this time (for any component).
-        return [XBlock.tags, XBlock.name]
-
-    @property
-    def editable_metadata_fields(self):
-        """
-        Returns the metadata fields to be edited in Studio. These are fields with scope `Scope.settings`.
-
-        Can be limited by extending `non_editable_metadata_fields`.
-        """
-        metadata_fields = {}
-
-        # Only use the fields from this class, not mixins
-        fields = getattr(self, 'unmixed_class', self.__class__).fields
-
-        for field in fields.values():
-
-            if field.scope != Scope.settings or field in self.non_editable_metadata_fields:
-                continue
-
-            metadata_fields[field.name] = self._create_metadata_editor_info(field)
-
-        return metadata_fields
-
-    def _create_metadata_editor_info(self, field):
-        """
-        Creates the information needed by the metadata editor for a specific field.
-        """
-        def jsonify_value(field, json_choice):
-            if isinstance(json_choice, dict):
-                json_choice = dict(json_choice)  # make a copy so below doesn't change the original
-                if 'display_name' in json_choice:
-                    json_choice['display_name'] = get_text(json_choice['display_name'])
-                if 'value' in json_choice:
-                    json_choice['value'] = field.to_json(json_choice['value'])
-            else:
-                json_choice = field.to_json(json_choice)
-            return json_choice
-
-        def get_text(value):
-            """Localize a text value that might be None."""
-            if value is None:
-                return None
-            else:
-                return self.runtime.service(self, "i18n").ugettext(value)
-
-        # gets the 'default_value' and 'explicitly_set' attrs
-        metadata_field_editor_info = self.runtime.get_field_provenance(self, field)
-        metadata_field_editor_info['field_name'] = field.name
-        metadata_field_editor_info['display_name'] = get_text(field.display_name)
-        metadata_field_editor_info['help'] = get_text(field.help)
-        metadata_field_editor_info['value'] = field.read_json(self)
-
-        # We support the following editors:
-        # 1. A select editor for fields with a list of possible values (includes Booleans).
-        # 2. Number editors for integers and floats.
-        # 3. A generic string editor for anything else (editing JSON representation of the value).
-        editor_type = "Generic"
-        values = field.values
-        if "values_provider" in field.runtime_options:
-            values = field.runtime_options['values_provider'](self)
-        if isinstance(values, (tuple, list)) and len(values) > 0:
-            editor_type = "Select"
-            values = [jsonify_value(field, json_choice) for json_choice in values]
-        elif isinstance(field, Integer):
-            editor_type = "Integer"
-        elif isinstance(field, Float):
-            editor_type = "Float"
-        elif isinstance(field, List):
-            editor_type = "List"
-        elif isinstance(field, Dict):
-            editor_type = "Dict"
-        elif isinstance(field, RelativeTime):
-            editor_type = "RelativeTime"
-        metadata_field_editor_info['type'] = editor_type
-        metadata_field_editor_info['options'] = [] if values is None else values
-
-        return metadata_field_editor_info
-
     # ~~~~~~~~~~~~~~~ XModule Indirection ~~~~~~~~~~~~~~~~
     @property
     def _xmodule(self):

common/lib/xmodule/xmodule/xml_module.py

@@ -13,12 +13,16 @@ from xmodule.modulestore import EdxJSONEncoder
 
 import dogstats_wrapper as dog_stats_api
 
+from lxml.etree import (  # pylint: disable=no-name-in-module
+    Element, ElementTree, XMLParser,
+)
+
 log = logging.getLogger(__name__)
 
 # assume all XML files are persisted as utf-8.
-edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False,
-                                 remove_comments=True, remove_blank_text=True,
-                                 encoding='utf-8')
+EDX_XML_PARSER = XMLParser(dtd_validation=False, load_dtd=False,
+                           remove_comments=True, remove_blank_text=True,
+                           encoding='utf-8')
 
 
 def name_to_pathname(name):
@@ -53,16 +57,6 @@ def is_pointer_tag(xml_obj):
     return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text
 
 
-def get_metadata_from_xml(xml_object, remove=True):
-    meta = xml_object.find('meta')
-    if meta is None:
-        return ''
-    dmdata = meta.text
-    if remove:
-        xml_object.remove(meta)
-    return dmdata
-
-
 def serialize_field(value):
     """
     Return a string version of the value (where value is the JSON-formatted, internally stored value).
@@ -108,16 +102,30 @@ def deserialize_field(field, value):
         return value
 
 
-class XmlDescriptor(XModuleDescriptor):
+class XmlParserMixin(object):
     """
-    Mixin class for standardized parsing of from xml
+    Class containing XML parsing functionality shared between XBlock and XModuleDescriptor.
     """
+    # Extension to append to filename paths
+    filename_extension = 'xml'
 
     xml_attributes = Dict(help="Map of unhandled xml attributes, used only for storage between import and export",
                           default={}, scope=Scope.settings)
 
-    # Extension to append to filename paths
-    filename_extension = 'xml'
+    # VS[compat].  Backwards compatibility code that can go away after
+    # importing 2012 courses.
+    # A set of metadata key conversions that we want to make
+    metadata_translations = {
+        'slug': 'url_name',
+        'name': 'display_name',
+    }
+
+    @classmethod
+    def _translate(cls, key):
+        """
+        VS[compat]
+        """
+        return cls.metadata_translations.get(key, key)
 
     # The attributes will be removed from the definition xml passed
     # to definition_from_xml, and from the xml returned by definition_to_xml
@@ -135,6 +143,19 @@ class XmlDescriptor(XModuleDescriptor):
 
     metadata_to_export_to_policy = ('discussion_topics', 'checklists')
 
+    @staticmethod
+    def _get_metadata_from_xml(xml_object, remove=True):
+        """
+        Extract the metadata from the XML.
+        """
+        meta = xml_object.find('meta')
+        if meta is None:
+            return ''
+        dmdata = meta.text
+        if remove:
+            xml_object.remove(meta)
+        return dmdata
+
     @classmethod
     def definition_from_xml(cls, xml_object, system):
         """
@@ -163,16 +184,16 @@ class XmlDescriptor(XModuleDescriptor):
 
         Returns an lxml Element
         """
-        return etree.parse(file_object, parser=edx_xml_parser).getroot()
+        return etree.parse(file_object, parser=EDX_XML_PARSER).getroot()  # pylint: disable=no-member
 
     @classmethod
     def load_file(cls, filepath, fs, def_id):  # pylint: disable=invalid-name
-        '''
+        """
         Open the specified file in fs, and call cls.file_to_xml on it,
         returning the lxml object.
 
         Add details and reraise on error.
-        '''
+        """
         try:
             with fs.open(filepath) as xml_file:
                 return cls.file_to_xml(xml_file)
@@ -184,7 +205,7 @@ class XmlDescriptor(XModuleDescriptor):
 
     @classmethod
     def load_definition(cls, xml_object, system, def_id, id_generator):
-        '''
+        """
         Load a descriptor definition from the specified xml_object.
         Subclasses should not need to override this except in special
         cases (e.g. html module)
@@ -194,7 +215,7 @@ class XmlDescriptor(XModuleDescriptor):
             system: the modulestore system (aka, runtime) which accesses data and provides access to services
             def_id: the definition id for the block--used to compute the usage id and asides ids
             id_generator: used to generate the usage_id
-        '''
+        """
 
         # VS[compat] -- the filename attr should go away once everything is
         # converted.  (note: make sure html files still work once this goes away)
@@ -234,7 +255,7 @@ class XmlDescriptor(XModuleDescriptor):
             # Add the attributes from the pointer node
             definition_xml.attrib.update(xml_object.attrib)
 
-        definition_metadata = get_metadata_from_xml(definition_xml)
+        definition_metadata = cls._get_metadata_from_xml(definition_xml)
         cls.clean_metadata_from_xml(definition_xml)
         definition, children = cls.definition_from_xml(definition_xml, system)
         if definition_metadata:
@@ -289,42 +310,51 @@ class XmlDescriptor(XModuleDescriptor):
                 metadata[attr] = value
 
     @classmethod
-    def from_xml(cls, xml_data, system, id_generator):
+    def parse_xml(cls, node, runtime, keys, id_generator):  # pylint: disable=unused-argument
         """
-        Creates an instance of this descriptor from the supplied xml_data.
-        This may be overridden by subclasses
+        Use `node` to construct a new block.
 
-        xml_data: A string of xml that will be translated into data and children for
-            this module
-        system: A DescriptorSystem for interacting with external resources
-        """
+        Arguments:
+            node (etree.Element): The xml node to parse into an xblock.
+
+            runtime (:class:`.Runtime`): The runtime to use while parsing.
+
+            keys (:class:`.ScopeIds`): The keys identifying where this block
+                will store its data.
+
+            id_generator (:class:`.IdGenerator`): An object that will allow the
+                runtime to generate correct definition and usage ids for
+                children of this block.
+
+        Returns (XBlock): The newly parsed XBlock
 
-        xml_object = etree.fromstring(xml_data)
+        """
         # VS[compat] -- just have the url_name lookup, once translation is done
-        url_name = xml_object.get('url_name', xml_object.get('slug'))
-        def_id = id_generator.create_definition(xml_object.tag, url_name)
+        url_name = node.get('url_name', node.get('slug'))
+        def_id = id_generator.create_definition(node.tag, url_name)
         usage_id = id_generator.create_usage(def_id)
 
         # VS[compat] -- detect new-style each-in-a-file mode
-        if is_pointer_tag(xml_object):
+        if is_pointer_tag(node):
             # new style:
             # read the actual definition file--named using url_name.replace(':','/')
-            filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name))
-            definition_xml = cls.load_file(filepath, system.resources_fs, def_id)
-            system.parse_asides(definition_xml, def_id, usage_id, id_generator)
+            filepath = cls._format_filepath(node.tag, name_to_pathname(url_name))
+            definition_xml = cls.load_file(filepath, runtime.resources_fs, def_id)
+            runtime.parse_asides(definition_xml, def_id, usage_id, id_generator)
         else:
             filepath = None
-            definition_xml = xml_object
+            definition_xml = node
             dog_stats_api.increment(
                 DEPRECATION_VSCOMPAT_EVENT,
                 tags=["location:xmlparser_util_mixin_parse_xml"]
             )
 
-        definition, children = cls.load_definition(definition_xml, system, def_id, id_generator)  # note this removes metadata
+        # Note: removes metadata.
+        definition, children = cls.load_definition(definition_xml, runtime, def_id, id_generator)
 
         # VS[compat] -- make Ike's github preview links work in both old and
         # new file layouts
-        if is_pointer_tag(xml_object):
+        if is_pointer_tag(node):
             # new style -- contents actually at filepath
             definition['filename'] = [filepath, filepath]
 
@@ -341,7 +371,7 @@ class XmlDescriptor(XModuleDescriptor):
                 metadata['definition_metadata_err'] = str(err)
 
         # Set/override any metadata specified by policy
-        cls.apply_policy(metadata, system.get_policy(usage_id))
+        cls.apply_policy(metadata, runtime.get_policy(usage_id))
 
         field_data = {}
         field_data.update(metadata)
@@ -352,10 +382,10 @@ class XmlDescriptor(XModuleDescriptor):
         kvs = InheritanceKeyValueStore(initial_values=field_data)
         field_data = KvsFieldData(kvs)
 
-        return system.construct_xblock_from_class(
+        return runtime.construct_xblock_from_class(
             cls,
             # We're loading a descriptor, so student_id is meaningless
-            ScopeIds(None, xml_object.tag, def_id, usage_id),
+            ScopeIds(None, node.tag, def_id, usage_id),
             field_data,
         )
 
@@ -374,32 +404,17 @@ class XmlDescriptor(XModuleDescriptor):
         """
         return True
 
-    def export_to_xml(self, resource_fs):
+    def add_xml_to_node(self, node):
         """
-        Returns an xml string representing this module, and all modules
-        underneath it.  May also write required resources out to resource_fs
-
-        Assumes that modules have single parentage (that no module appears twice
-        in the same course), and that it is thus safe to nest modules as xml
-        children as appropriate.
-
-        The returned XML should be able to be parsed back into an identical
-        XModuleDescriptor using the from_xml method with the same system, org,
-        and course
-
-        resource_fs is a pyfilesystem object (from the fs package)
+        For exporting, set data on `node` from ourselves.
         """
-
-        # 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)
+        xml_object = self.definition_to_xml(self.runtime.export_fs)
         self.clean_metadata_from_xml(xml_object)
 
-        # Set the tag so we get the file path right
+        # Set the tag on both nodes so we get the file path right.
         xml_object.tag = self.category
+        node.tag = self.category
 
         # Add the non-inherited metadata
         for attr in sorted(own_metadata(self)):
@@ -422,24 +437,25 @@ class XmlDescriptor(XModuleDescriptor):
             # Write the definition to a file
             url_path = name_to_pathname(self.url_name)
             filepath = self._format_filepath(self.category, url_path)
-            resource_fs.makedir(os.path.dirname(filepath), recursive=True, allow_recreate=True)
-            with resource_fs.open(filepath, 'w') as fileobj:
-                fileobj.write(etree.tostring(xml_object, pretty_print=True, encoding='utf-8'))
-
-            # And return just a pointer with the category and filename.
-            record_object = etree.Element(self.category)
+            self.runtime.export_fs.makedir(os.path.dirname(filepath), recursive=True, allow_recreate=True)
+            with self.runtime.export_fs.open(filepath, 'w') as fileobj:
+                ElementTree(xml_object).write(fileobj, pretty_print=True, encoding='utf-8')
         else:
-            record_object = xml_object
+            # Write all attributes from xml_object onto node
+            node.clear()
+            node.tag = xml_object.tag
+            node.text = xml_object.text
+            node.tail = xml_object.tail
+            node.attrib = xml_object.attrib
+            node.extend(xml_object)
 
-        record_object.set('url_name', self.url_name)
+        node.set('url_name', self.url_name)
 
         # Special case for course pointers:
         if self.category == 'course':
             # add org and course attributes on the pointer tag
-            record_object.set('org', self.location.org)
-            record_object.set('course', self.location.course)
-
-        return etree.tostring(record_object, pretty_print=True, encoding='utf-8')
+            node.set('org', self.location.org)
+            node.set('course', self.location.course)
 
     def definition_to_xml(self, resource_fs):
         """
@@ -450,6 +466,86 @@ class XmlDescriptor(XModuleDescriptor):
 
     @property
     def non_editable_metadata_fields(self):
-        non_editable_fields = super(XmlDescriptor, self).non_editable_metadata_fields
-        non_editable_fields.append(XmlDescriptor.xml_attributes)
+        """
+        Return a list of all metadata fields that cannot be edited.
+        """
+        non_editable_fields = super(XmlParserMixin, self).non_editable_metadata_fields
+        non_editable_fields.append(XmlParserMixin.xml_attributes)
         return non_editable_fields
+
+
+class XmlDescriptor(XmlParserMixin, XModuleDescriptor):  # pylint: disable=abstract-method
+    """
+    Mixin class for standardized parsing of XModule xml.
+    """
+    @classmethod
+    def from_xml(cls, xml_data, system, id_generator):
+        """
+        Creates an instance of this descriptor from the supplied xml_data.
+        This may be overridden by subclasses.
+
+        Args:
+            xml_data (str): A string of xml that will be translated into data and children
+                for this module
+
+            system (:class:`.XMLParsingSystem):
+
+            id_generator (:class:`xblock.runtime.IdGenerator`): Used to generate the
+                usage_ids and definition_ids when loading this xml
+
+        """
+        # Shim from from_xml to the parse_xml defined in XmlParserMixin.
+        # This only exists to satisfy subclasses that both:
+        #    a) define from_xml themselves
+        #    b) call super(..).from_xml(..)
+        return super(XmlDescriptor, cls).parse_xml(
+            etree.fromstring(xml_data),  # pylint: disable=no-member
+            system,
+            None,  # This is ignored by XmlParserMixin
+            id_generator,
+        )
+
+    @classmethod
+    def parse_xml(cls, node, runtime, keys, id_generator):
+        """
+        Interpret the parsed XML in `node`, creating an XModuleDescriptor.
+        """
+        if cls.from_xml != XmlDescriptor.from_xml:
+            # Skip the parse_xml from XmlParserMixin to get the shim parse_xml
+            # from XModuleDescriptor, which actually calls `from_xml`.
+            return super(XmlParserMixin, cls).parse_xml(node, runtime, keys, id_generator)  # pylint: disable=bad-super-call
+        else:
+            return super(XmlDescriptor, cls).parse_xml(node, runtime, keys, id_generator)
+
+    def export_to_xml(self, resource_fs):  # pylint: disable=unused-argument
+        """
+        Returns an xml string representing this module, and all modules
+        underneath it.  May also write required resources out to resource_fs.
+
+        Assumes that modules have single parentage (that no module appears twice
+        in the same course), and that it is thus safe to nest modules as xml
+        children as appropriate.
+
+        The returned XML should be able to be parsed back into an identical
+        XModuleDescriptor using the from_xml method with the same system, org,
+        and course
+        """
+        # Shim from export_to_xml to the add_xml_to_node defined in XmlParserMixin.
+        # This only exists to satisfy subclasses that both:
+        #    a) define export_to_xml themselves
+        #    b) call super(..).export_to_xml(..)
+        node = Element(self.category)
+        super(XmlDescriptor, self).add_xml_to_node(node)
+        return etree.tostring(node)  # pylint: disable=no-member
+
+    def add_xml_to_node(self, node):
+        """
+        Export this :class:`XModuleDescriptor` as XML, by setting attributes on the provided
+        `node`.
+        """
+        if self.export_to_xml != XmlDescriptor.export_to_xml:
+            # Skip the add_xml_to_node from XmlParserMixin to get the shim add_xml_to_node
+            # from XModuleDescriptor, which actually calls `export_to_xml`.
+            super(XmlParserMixin, self).add_xml_to_node(node)  # pylint: disable=bad-super-call
+        else:
+            super(XmlDescriptor, self).add_xml_to_node(node)

common/static/js/xblock/core.js

@@ -32,7 +32,7 @@
     }
 
     function initArgs(element) {
-        var initargs = $('.xblock_json_init_args', element).text();
+        var initargs = $(element).children('.xblock-json-init-args').remove().text();
         return initargs ? JSON.parse(initargs) : {};
     }
 

common/templates/xblock_wrapper.html

 <div class="${' '.join(classes) | n}" ${data_attributes}>
 % if js_init_parameters:
-  <script type="json/xblock-args" class="xblock_json_init_args">
+  <script type="json/xblock-args" class="xblock-json-init-args">
     ${js_init_parameters}
   </script>
 % endif
-    ${content}
+  ${content}
 </div>

common/test/data/aside/policies/2012_Fall.json

@@ -3,7 +3,7 @@
         "graceperiod": "2 days 5 hours 59 minutes 59 seconds",
         "start": "2015-07-17T12:00",
         "display_name": "Toy Course",
-        "graded": "true",
+        "graded": "true"
     },
     "tabs": [
           {"type": "courseware"},
@@ -19,5 +19,5 @@
     },
     "html/secret:toylab": {
         "display_name": "Toy lab"
-    },
+    }
 }

common/test/data/xml_dag/policies/2012_Fall.json

@@ -3,6 +3,6 @@
         "graceperiod": "2 days 5 hours 59 minutes 59 seconds",
         "start": "2015-07-17T12:00",
         "display_name": "Toy Course",
-        "graded": "true",
+        "graded": "true"
     }
 }

lms/djangoapps/courseware/features/common.py

@@ -13,7 +13,7 @@ from student.models import CourseEnrollment
 from xmodule.modulestore.django import modulestore
 from xmodule.course_module import CourseDescriptor
 from courseware.courses import get_course_by_id
-from xmodule import seq_module, vertical_module
+from xmodule import seq_module, vertical_block
 from logging import getLogger
 logger = getLogger(__name__)
 
@@ -181,7 +181,7 @@ def get_courseware_with_tabs(course_id):
         }, {
             'clickable_tab_count': 1,
             'section_name': 'System Usage Sequence',
-            'tab_classes': ['VerticalDescriptor']
+            'tab_classes': ['VerticalBlock']
         }, {
             'clickable_tab_count': 0,
             'section_name': 'Lab0: Using the tools',
@@ -196,7 +196,7 @@ def get_courseware_with_tabs(course_id):
         'sections': [{
             'clickable_tab_count': 4,
             'section_name': 'Administrivia and Circuit Elements',
-            'tab_classes': ['VerticalDescriptor', 'VerticalDescriptor', 'VerticalDescriptor', 'VerticalDescriptor']
+            'tab_classes': ['VerticalBlock', 'VerticalBlock', 'VerticalBlock', 'VerticalBlock']
         }, {
             'clickable_tab_count': 0,
             'section_name': 'Basic Circuit Analysis',
@@ -215,7 +215,7 @@ def get_courseware_with_tabs(course_id):
         'sections': [{
             'clickable_tab_count': 2,
             'section_name': 'Midterm Exam',
-            'tab_classes': ['VerticalDescriptor', 'VerticalDescriptor']
+            'tab_classes': ['VerticalBlock', 'VerticalBlock']
         }]
     }]
     """
@@ -228,7 +228,7 @@ def get_courseware_with_tabs(course_id):
             'section_name': s.display_name_with_default,
             'clickable_tab_count': len(s.get_children()) if (type(s) == seq_module.SequenceDescriptor) else 0,
             'tabs': [{
-                'children_count': len(t.get_children()) if (type(t) == vertical_module.VerticalDescriptor) else 0,
+                'children_count': len(t.get_children()) if (type(t) == vertical_block.VerticalBlock) else 0,
                 'class': t.__class__.__name__} for t in s.get_children()
             ]
         } for s in c.get_children() if not s.hide_from_toc]

lms/djangoapps/courseware/management/commands/tests/test_dump_course.py

@@ -126,9 +126,12 @@ class CommandsTestBase(ModuleStoreTestCase):
         self.assertEqual(dump[child_id]['category'], 'videosequence')
         self.assertEqual(len(dump[child_id]['children']), 2)
 
-        video_id = test_course_key.make_usage_key('video', 'Welcome').to_deprecated_string()
+        video_id = unicode(test_course_key.make_usage_key('video', 'Welcome'))
         self.assertEqual(dump[video_id]['category'], 'video')
-        self.assertEqual(len(dump[video_id]['metadata']), 5)
+        self.assertItemsEqual(
+            dump[video_id]['metadata'].keys(),
+            ['download_video', 'youtube_id_0_75', 'youtube_id_1_0', 'youtube_id_1_25', 'youtube_id_1_5']
+        )
         self.assertIn('youtube_id_1_0', dump[video_id]['metadata'])
 
         # Check if there are the right number of elements

lms/djangoapps/courseware/tests/test_module_render.py

@@ -16,6 +16,7 @@ from django.contrib.auth.models import AnonymousUser
 from mock import MagicMock, patch, Mock
 from opaque_keys.edx.keys import UsageKey, CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from pyquery import PyQuery
 from courseware.module_render import hash_resource
 from xblock.field_data import FieldData
 from xblock.runtime import Runtime
@@ -430,7 +431,8 @@ class TestHandleXBlockCallback(ModuleStoreTestCase, LoginEnrollmentTestCase):
         content = json.loads(response.content)
         for section in expected:
             self.assertIn(section, content)
-        self.assertIn('<div class="xblock xblock-student_view xmodule_display', content['html'])
+        doc = PyQuery(content['html'])
+        self.assertEquals(len(doc('div.xblock-student_view-videosequence')), 1)
 
 
 @ddt.ddt
@@ -567,7 +569,7 @@ class TestHtmlModifiers(ModuleStoreTestCase):
         )
         result_fragment = module.render(STUDENT_VIEW)
 
-        self.assertIn('div class="xblock xblock-student_view xmodule_display xmodule_HtmlModule"', result_fragment.content)
+        self.assertEquals(len(PyQuery(result_fragment.content)('div.xblock.xblock-student_view.xmodule_HtmlModule')), 1)
 
     def test_xmodule_display_wrapper_disabled(self):
         module = render.get_module(
@@ -798,8 +800,8 @@ class MongoViewInStudioTest(ViewInStudioTest):
         # Render the parent vertical, then check that there is only a single "View Unit in Studio" link.
         result_fragment = self.module.render(STUDENT_VIEW)
         # The single "View Unit in Studio" link should appear before the first xmodule vertical definition.
-        parts = result_fragment.content.split('xmodule_VerticalModule')
-        self.assertEqual(3, len(parts), "Did not find two vertical modules")
+        parts = result_fragment.content.split('data-block-type="vertical"')
+        self.assertEqual(3, len(parts), "Did not find two vertical blocks")
         self.assertIn('View Unit in Studio', parts[0])
         self.assertNotIn('View Unit in Studio', parts[1])
         self.assertNotIn('View Unit in Studio', parts[2])

lms/djangoapps/courseware/views.py

@@ -345,6 +345,13 @@ def _index_bulk_op(request, course_key, chapter, section, position):
     """
     Render the index page for the specified course.
     """
+    # Verify that position a string is in fact an int
+    if position is not None:
+        try:
+            int(position)
+        except ValueError:
+            raise Http404("Position {} is not an integer!".format(position))
+
     user = request.user
     course = get_course_with_access(user, 'load', course_key, depth=2)
 
@@ -493,13 +500,6 @@ def _index_bulk_op(request, course_key, chapter, section, position):
                 section_descriptor, depth=None
             )
 
-            # Verify that position a string is in fact an int
-            if position is not None:
-                try:
-                    int(position)
-                except ValueError:
-                    raise Http404("Position {} is not an integer!".format(position))
-
             section_module = get_module_for_descriptor(
                 request.user,
                 request,

lms/static/sass/course/courseware/_courseware.scss

@@ -162,7 +162,7 @@ div.course-wrapper {
       }
     }
 
-    section.xmodule_WrapperModule div.vert-mod > div {
+    section.xblock-student_view-wrapper div.vert-mod > div {
       border-bottom: none;
     }
 

requirements/edx/base.txt

@@ -144,6 +144,7 @@ pep8==1.5.7
 PyContracts==1.7.1
 pylint==1.4.2
 python-subunit==0.0.16
+pyquery==1.2.9
 radon==1.2
 rednose==0.3
 selenium==2.42.1