Convert VerticalModule/VerticalDescriptor to a pure XBlock: VerticalBlock

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
@@ -193,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
@@ -204,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/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/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/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

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/tests/test_module_render.py

@@ -800,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/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;
     }