From 1f17538d423b4cad6915273d810db872dc1d4d80 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 13:58:28 -0400 Subject: [PATCH 01/10] Clean up tests in preparation for switching Vertical from an XModule to an XBlock --- cms/djangoapps/contentstore/features/component.py | 2 +- cms/djangoapps/contentstore/features/pages.py | 2 +- .../contentstore/views/tests/test_item.py | 4 +++- common/lib/safe_lxml/safe_lxml/etree.py | 1 + .../modulestore/tests/test_mixed_modulestore.py | 3 ++- .../xmodule/modulestore/tests/test_mongo.py | 4 +++- .../modulestore/tests/test_split_w_old_mongo.py | 3 ++- .../xmodule/xmodule/modulestore/tests/test_xml.py | 13 +++++++++++-- .../xmodule/xmodule/modulestore/tests/utils.py | 3 ++- .../xmodule/xmodule/modulestore/xml_importer.py | 9 ++++++--- common/lib/xmodule/xmodule/tests/__init__.py | 15 +++++++++++++++ common/lib/xmodule/xmodule/tests/xml/factories.py | 3 ++- .../management/commands/tests/test_dump_course.py | 7 +++++-- .../courseware/tests/test_module_render.py | 6 ++++-- requirements/edx/base.txt | 1 + 15 files changed, 59 insertions(+), 17 deletions(-) diff --git a/cms/djangoapps/contentstore/features/component.py b/cms/djangoapps/contentstore/features/component.py index 820af47d59..029dc01f87 100644 --- a/cms/djangoapps/contentstore/features/component.py +++ b/cms/djangoapps/contentstore/features/component.py @@ -61,7 +61,7 @@ def see_a_multi_step_component(step, category): 'Raw HTML': '

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) diff --git a/cms/djangoapps/contentstore/features/pages.py b/cms/djangoapps/contentstore/features/pages.py index 5558f20390..ccae95b3fd 100644 --- a/cms/djangoapps/contentstore/features/pages.py +++ b/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$') diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 908fd784d2..3bc3982f99 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/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): diff --git a/common/lib/safe_lxml/safe_lxml/etree.py b/common/lib/safe_lxml/safe_lxml/etree.py index 40b4665ff8..83052b22b6 100644 --- a/common/lib/safe_lxml/safe_lxml/etree.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index cbd6c6762e..a69fd5e8aa 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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, diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index e9876dd1db..c0bac5094e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/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( diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py index 939d2d09b8..31d1dba5af 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py index c842408b90..3bac20f363 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py +++ b/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( diff --git a/common/lib/xmodule/xmodule/modulestore/tests/utils.py b/common/lib/xmodule/xmodule/modulestore/tests/utils.py index ad71fd5f85..ebf8efd767 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/utils.py +++ b/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, diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 68d8634c5d..4e54dd589f 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index e8771915b4..6b6edda4c4 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/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')): """ diff --git a/common/lib/xmodule/xmodule/tests/xml/factories.py b/common/lib/xmodule/xmodule/tests/xml/factories.py index 478c0d1f59..ab844735be 100644 --- a/common/lib/xmodule/xmodule/tests/xml/factories.py +++ b/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 = {} diff --git a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py index 44fb69415e..2f2e986c8a 100644 --- a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 3ec1ffcddb..d4d4ccc2b5 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/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('

Date: Fri, 27 Mar 2015 14:03:46 -0400 Subject: [PATCH 02/10] Make XBlockToXModuleShim use json-init-args to record the XModule constructor to shim to --- common/djangoapps/xmodule_modifiers.py | 8 ++++++-- common/lib/xmodule/xmodule/js/src/poll/poll_main.js | 2 +- common/lib/xmodule/xmodule/js/src/xmodule.js | 12 +++++++++--- common/lib/xmodule/xmodule/x_module.py | 3 ++- common/static/js/xblock/core.js | 2 +- common/templates/xblock_wrapper.html | 4 ++-- .../courseware/tests/test_module_render.py | 2 +- 7 files changed, 22 insertions(+), 11 deletions(-) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index f82a194e93..160e3e5fb3 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -77,7 +77,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 +94,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 diff --git a/common/lib/xmodule/xmodule/js/src/poll/poll_main.js b/common/lib/xmodule/xmodule/js/src/poll/poll_main.js index 3b61f5744f..13542899b4 100644 --- a/common/lib/xmodule/xmodule/js/src/poll/poll_main.js +++ b/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; diff --git a/common/lib/xmodule/xmodule/js/src/xmodule.js b/common/lib/xmodule/xmodule/js/src/xmodule.js index 7676f33da5..be3f911e4d 100644 --- a/common/lib/xmodule/xmodule/js/src/xmodule.js +++ b/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; diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 3d01be6a71..6bcc147c89 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -237,12 +237,13 @@ 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): diff --git a/common/static/js/xblock/core.js b/common/static/js/xblock/core.js index 99b2ae0489..1c6632c71d 100644 --- a/common/static/js/xblock/core.js +++ b/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) : {}; } diff --git a/common/templates/xblock_wrapper.html b/common/templates/xblock_wrapper.html index 41951dc61c..998d00a90a 100644 --- a/common/templates/xblock_wrapper.html +++ b/common/templates/xblock_wrapper.html @@ -1,8 +1,8 @@
% if js_init_parameters: - % endif - ${content} + ${content}
diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index d4d4ccc2b5..c803611e88 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -432,7 +432,7 @@ class TestHandleXBlockCallback(ModuleStoreTestCase, LoginEnrollmentTestCase): for section in expected: self.assertIn(section, content) doc = PyQuery(content['html']) - self.assertEquals(len(doc('div.xblock.xblock-student_view')), 1) + self.assertEquals(len(doc('div.xblock-student_view-videosequence')), 1) @ddt.ddt From bf62c4b70e1541579da69d5c94ac8ab410149c49 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:05:44 -0400 Subject: [PATCH 03/10] Extract a pure-XBlock version of MakoModuleDescriptor --- common/lib/xmodule/xmodule/mako_module.py | 35 +++++++++++++++++------ 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index 7718cb341e..da9da9a31d 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -1,4 +1,9 @@ -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 From ee401f15f4f892263ed523d92febccd7c0591bc4 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:10:18 -0400 Subject: [PATCH 04/10] Make mongo/draft.py pass the active descriptor system through item loading --- .../lib/xmodule/xmodule/modulestore/mongo/draft.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 5c30f5e379..cff1d2bf98 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/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: From 62a90db1a73a5419e63d10845efed9a24f2f27b5 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:10:46 -0400 Subject: [PATCH 05/10] Extract a pure-XBlock version of StudioEditingModule --- common/lib/xmodule/xmodule/studio_editable.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/studio_editable.py b/common/lib/xmodule/xmodule/studio_editable.py index 5b173ecbab..28e44d4bcc 100644 --- a/common/lib/xmodule/xmodule/studio_editable.py +++ b/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. From da2964421e37eb7da51f85375be1cee52d566974 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:19:01 -0400 Subject: [PATCH 06/10] Extract a pure-XBlock version of XmlDescriptor --- common/lib/xmodule/xmodule/xml_module.py | 250 ++++++++++++++++------- 1 file changed, 173 insertions(+), 77 deletions(-) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 6e18effd83..b53d0550af 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/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. + + 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_data: A string of xml that will be translated into data and children for - this module - system: A DescriptorSystem for interacting with external resources """ - - 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) From 391db6a8480b52d575adba3906fbabc9ae9202e9 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:21:17 -0400 Subject: [PATCH 07/10] Linting cleanups --- common/lib/xmodule/xmodule/tests/__init__.py | 11 ++++++++++- common/test/data/aside/policies/2012_Fall.json | 4 ++-- common/test/data/xml_dag/policies/2012_Fall.json | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 6b6edda4c4..e8dc7ab1d8 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -143,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", @@ -378,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: @@ -436,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)) diff --git a/common/test/data/aside/policies/2012_Fall.json b/common/test/data/aside/policies/2012_Fall.json index c3ba2f869e..4a59afde57 100644 --- a/common/test/data/aside/policies/2012_Fall.json +++ b/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" - }, + } } diff --git a/common/test/data/xml_dag/policies/2012_Fall.json b/common/test/data/xml_dag/policies/2012_Fall.json index 82bfa20787..a135872075 100644 --- a/common/test/data/xml_dag/policies/2012_Fall.json +++ b/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" } } From 85fce7e3b76e816db692794760ed3e0af00c9190 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:21:38 -0400 Subject: [PATCH 08/10] Move more common operations (which work in both XBlocks and XModules) into XModuleMixin --- common/lib/xmodule/xmodule/x_module.py | 200 +++++++++++++------------ 1 file changed, 104 insertions(+), 96 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 6bcc147c89..6f103640fa 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -246,7 +246,21 @@ def shim_xmodule_js(block, fragment): 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. @@ -277,15 +291,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 @@ -570,6 +575,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): """ @@ -964,92 +1058,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): From 8341f1b7c8267b15cfad27d6142d577aa0c5341d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:23:13 -0400 Subject: [PATCH 09/10] Move type-checking of position earlier in courseware.views.index --- lms/djangoapps/courseware/views.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 7b19c24c51..a7b3ea6530 100644 --- a/lms/djangoapps/courseware/views.py +++ b/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, From a0bae0c794cc7951cabdc15dd28d5c77a1e3282d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 27 Mar 2015 14:20:54 -0400 Subject: [PATCH 10/10] Convert VerticalModule/VerticalDescriptor to a pure XBlock: VerticalBlock --- common/djangoapps/xmodule_modifiers.py | 8 +- common/lib/xmodule/setup.py | 4 +- .../xmodule/xmodule/css/wrapper/display.scss | 0 .../xmodule/tests/test_crowdsource_hinter.py | 6 +- .../xmodule/tests/test_studio_editable.py | 7 +- .../xmodule/xmodule/tests/test_vertical.py | 12 +- .../xmodule/tests/test_xblock_wrappers.py | 31 +++-- common/lib/xmodule/xmodule/vertical_block.py | 130 ++++++++++++++++++ common/lib/xmodule/xmodule/vertical_module.py | 93 ------------- common/lib/xmodule/xmodule/wrapper_module.py | 18 +-- lms/djangoapps/courseware/features/common.py | 10 +- .../courseware/tests/test_module_render.py | 4 +- .../sass/course/courseware/_courseware.scss | 2 +- 13 files changed, 192 insertions(+), 133 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/css/wrapper/display.scss create mode 100644 common/lib/xmodule/xmodule/vertical_block.py delete mode 100644 common/lib/xmodule/xmodule/vertical_module.py 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; }