diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 160e3e5fb3..72adfa97a7 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/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" diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index f2b548efe9..fedb0e4447 100644 --- a/common/lib/xmodule/setup.py +++ b/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( diff --git a/common/lib/xmodule/xmodule/css/wrapper/display.scss b/common/lib/xmodule/xmodule/css/wrapper/display.scss deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index f32b9c1a83..ade4597a78 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/tests/test_studio_editable.py b/common/lib/xmodule/xmodule/tests/test_studio_editable.py index 44ed0ca022..849890b136 100644 --- a/common/lib/xmodule/xmodule/tests/test_studio_editable.py +++ b/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. diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index 0261143863..5997b2324c 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/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. diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index e23995ce6a..bd1fdddb8f 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/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): diff --git a/common/lib/xmodule/xmodule/vertical_block.py b/common/lib/xmodule/xmodule/vertical_block.py new file mode 100644 index 0000000000..293c865790 --- /dev/null +++ b/common/lib/xmodule/xmodule/vertical_block.py @@ -0,0 +1,130 @@ +""" +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.mako_module import MakoTemplateBlockBase +from xmodule.progress import Progress +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 VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParserMixin, MakoTemplateBlockBase, XBlock): + """ + Layout XBlock for rendering subblocks vertically. + """ + mako_template = 'widgets/sequence-edit.html' + js_module_name = "VerticalBlock" + + 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) + + contents.append({ + 'id': child.location.to_deprecated_string(), + 'content': rendered_child.content + }) + + fragment.add_content(self.system.render_template('vert_module.html', { + 'items': contents, + 'xblock_context': context, + })) + return fragment + + def author_view(self, context): + """ + Renders the Studio preview view, which supports drag and drop. + """ + fragment = Fragment() + root_xblock = context.get('root_xblock') + 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 + # the unit page and the vertical being rendered is itself the unit vertical (is_root == True). + if is_root or not context.get('is_unit_page'): + self.render_children(context, fragment, can_reorder=True, can_add=True) + 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() # 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): + """ + 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 higher_class in CLASS_PRIORITY: + if higher_class in child_classes: + new_class = higher_class + return new_class + + @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): + """ + Gather all fields which can't be edited. + """ + non_editable_fields = super(VerticalBlock, self).non_editable_metadata_fields + non_editable_fields.extend([ + 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 diff --git a/common/lib/xmodule/xmodule/vertical_module.py b/common/lib/xmodule/xmodule/vertical_module.py deleted file mode 100644 index f8cfaa4256..0000000000 --- a/common/lib/xmodule/xmodule/vertical_module.py +++ /dev/null @@ -1,93 +0,0 @@ -from xblock.fragment import Fragment -from xmodule.x_module import XModule, STUDENT_VIEW -from xmodule.seq_module import SequenceDescriptor -from xmodule.progress import Progress -from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor -from pkg_resources import resource_string -from copy import copy - - -# HACK: This shouldn't be hard-coded to two types -# OBSOLETE: This obsoletes 'type' -class_priority = ['video', 'problem'] - - -class VerticalFields(object): - has_children = True - - -class VerticalModule(VerticalFields, XModule, StudioEditableModule): - ''' Layout module for laying out submodules vertically.''' - - def student_view(self, context): - fragment = Fragment() - contents = [] - - child_context = {} if not context else copy(context) - child_context['child_of_vertical'] = True - - for child in self.get_display_items(): - rendered_child = child.render(STUDENT_VIEW, child_context) - fragment.add_frag_resources(rendered_child) - - contents.append({ - 'id': child.location.to_deprecated_string(), - 'content': rendered_child.content - }) - - fragment.add_content(self.system.render_template('vert_module.html', { - 'items': contents, - 'xblock_context': context, - })) - return fragment - - def author_view(self, context): - """ - Renders the Studio preview view, which supports drag and drop. - """ - fragment = Fragment() - root_xblock = context.get('root_xblock') - is_root = root_xblock and root_xblock.location == self.location - - # 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 - # the unit page and the vertical being rendered is itself the unit vertical (is_root == True). - if is_root or not context.get('is_unit_page'): - self.render_children(context, fragment, can_reorder=True, can_add=True) - return fragment - - def get_progress(self): - # TODO: Cache progress or children array? - children = self.get_children() - 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()) - new_class = 'other' - for c in class_priority: - if c in child_classes: - new_class = c - 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... - - @property - def non_editable_metadata_fields(self): - non_editable_fields = super(VerticalDescriptor, self).non_editable_metadata_fields - non_editable_fields.extend([ - VerticalDescriptor.due, - ]) - return non_editable_fields diff --git a/common/lib/xmodule/xmodule/wrapper_module.py b/common/lib/xmodule/xmodule/wrapper_module.py index 96546c3628..622fbd28e7 100644 --- a/common/lib/xmodule/xmodule/wrapper_module.py +++ b/common/lib/xmodule/xmodule/wrapper_module.py @@ -1,7 +1,7 @@ # 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 diff --git a/lms/djangoapps/courseware/features/common.py b/lms/djangoapps/courseware/features/common.py index b6eebe7b9e..f00e3cca0b 100644 --- a/lms/djangoapps/courseware/features/common.py +++ b/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] diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index c803611e88..1ef7fc94ae 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/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]) diff --git a/lms/static/sass/course/courseware/_courseware.scss b/lms/static/sass/course/courseware/_courseware.scss index ddaad0875c..e653b38c5d 100644 --- a/lms/static/sass/course/courseware/_courseware.scss +++ b/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; }