Aside rendered
" + + content = String(default="default_content", scope=Scope.content) + data_field = String(default="default_data", scope=Scope.settings) + + @XBlockAside.aside_for('student_view') + def student_view_aside(self, block, context): # pylint: disable=unused-argument + """Add to the student view""" + return Fragment(self.FRAG_CONTENT) + + +class TestAsidesXmlStore(TestCase): + """ + Test Asides sourced from xml store + """ + @patch('xmodule.x_module.descriptor_global_applicable_aside_types', lambda block: ['test_aside']) + @XBlockAside.register_temp_plugin(AsideTestType, 'test_aside') + def test_xml_aside(self): + """ + Check that the xml modulestore read in all the asides with their values + """ + with XmlModulestoreBuilder().build(course_ids=['edX/aside_test/2012_Fall']) as store: + def check_block(block): + """ + Check whether block has the expected aside w/ its fields and then recurse to the block's children + """ + asides = block.runtime.get_asides(block) + self.assertEqual(len(asides), 1, "Found {} asides but expected only test_aside".format(asides)) + self.assertIsInstance(asides[0], AsideTestType) + category = block.scope_ids.block_type + self.assertEqual(asides[0].data_field, "{} aside data".format(category)) + self.assertEqual(asides[0].content, "{} Aside".format(category.capitalize())) + + for child in block.get_children(): + check_block(child) + + check_block(store.get_course(store.make_course_key('edX', "aside_test", "2012_Fall"))) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py index 1447b02384..0e61e325be 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py @@ -203,6 +203,6 @@ class TestLibraries(MixedSplitTestCase): message = u"Hello world" hello_render = lambda _, context: Fragment(message) with patch('xmodule.html_module.HtmlDescriptor.author_view', hello_render, create=True): - with patch('xmodule.x_module.descriptor_global_get_asides', lambda block: []): + with patch('xmodule.x_module.descriptor_global_applicable_aside_types', lambda block: []): result = library.render(AUTHOR_VIEW, context) self.assertIn(message, result.content) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 0007d57574..a836597c00 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -22,7 +22,6 @@ from xmodule.x_module import XMLParsingSystem, policy_key, OpaqueKeyReader, Asid from xmodule.modulestore.xml_exporter import DEFAULT_CONTENT_FIELDS from xmodule.modulestore import ModuleStoreEnum, ModuleStoreReadBase from xmodule.tabs import CourseTabList -from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location from opaque_keys.edx.locator import CourseLocator @@ -33,7 +32,7 @@ from xblock.runtime import DictKeyValueStore from .exceptions import ItemNotFoundError from .inheritance import compute_inherited_metadata, inheriting_field_data -from xblock.fields import ScopeIds, Reference, ReferenceList, ReferenceValueDict +from xblock.fields import ScopeIds edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_comments=True, remove_blank_text=True) @@ -171,9 +170,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): make_name_unique(xml_data) - descriptor = create_block_from_xml( - etree.tostring(xml_data, encoding='unicode'), - self, + descriptor = self.xblock_from_node( + xml_data, + None, # parent_id id_manager, ) except Exception as err: # pylint: disable=broad-except @@ -279,71 +278,6 @@ class CourseLocationManager(OpaqueKeyReader, AsideKeyGenerator): return usage_id -def _make_usage_key(course_key, value): - """ - Makes value into a UsageKey inside the specified course. - If value is already a UsageKey, returns that. - """ - if isinstance(value, UsageKey): - return value - return course_key.make_usage_key_from_deprecated_string(value) - - -def _convert_reference_fields_to_keys(xblock): # pylint: disable=invalid-name - """ - Find all fields of type reference and convert the payload into UsageKeys - """ - course_key = xblock.scope_ids.usage_id.course_key - - for field in xblock.fields.itervalues(): - if field.is_set_on(xblock): - field_value = getattr(xblock, field.name) - if isinstance(field, Reference): - setattr(xblock, field.name, _make_usage_key(course_key, field_value)) - elif isinstance(field, ReferenceList): - setattr(xblock, field.name, [_make_usage_key(course_key, ele) for ele in field_value]) - elif isinstance(field, ReferenceValueDict): - for key, subvalue in field_value.iteritems(): - assert isinstance(subvalue, basestring) - field_value[key] = _make_usage_key(course_key, subvalue) - setattr(xblock, field.name, field_value) - - -def create_block_from_xml(xml_data, system, id_generator): - """ - Create an XBlock instance from XML data. - - Args: - xml_data (string): A string containing valid xml. - system (XMLParsingSystem): The :class:`.XMLParsingSystem` used to connect the block - to the outside world. - id_generator (IdGenerator): An :class:`~xblock.runtime.IdGenerator` that - will be used to construct the usage_id and definition_id for the block. - - Returns: - XBlock: The fully instantiated :class:`~xblock.core.XBlock`. - - """ - node = etree.fromstring(xml_data) - raw_class = system.load_block_type(node.tag) - xblock_class = system.mixologist.mix(raw_class) - - # leave next line commented out - useful for low-level debugging - # log.debug('[create_block_from_xml] tag=%s, class=%s' % (node.tag, xblock_class)) - - block_type = node.tag - url_name = node.get('url_name') - def_id = id_generator.create_definition(block_type, url_name) - usage_id = id_generator.create_usage(def_id) - - scope_ids = ScopeIds(None, block_type, def_id, usage_id) - xblock = xblock_class.parse_xml(node, system, scope_ids, id_generator) - - _convert_reference_fields_to_keys(xblock) - - return xblock - - class ParentTracker(object): """A simple class to factor out the logic for tracking location parent pointers.""" def __init__(self): diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 39a61bd342..4b946e4bb5 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -480,6 +480,8 @@ def _import_module_and_update_references( fields = {} for field_name, field in module.fields.iteritems(): if field.is_set_on(module): + if field.scope == Scope.parent: + continue if isinstance(field, Reference): fields[field_name] = _convert_reference_fields_to_new_namespace(field.read_from(module)) elif isinstance(field, ReferenceList): diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index 647a92f27c..8bc094f66d 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py @@ -262,7 +262,7 @@ class XBlockWrapperTestMixin(object): This is a mixin for building tests of the implementation of the XBlock api by wrapping XModule native functions. - You can creat an actual test case by inheriting from this class and UnitTest, + You can create an actual test case by inheriting from this class and UnitTest, and implement skip_if_invalid and check_property. """ @@ -289,6 +289,8 @@ class XBlockWrapperTestMixin(object): mocked_course = Mock() modulestore = Mock() modulestore.get_course.return_value = mocked_course + # pylint: disable=no-member + descriptor.runtime.id_reader.get_definition_id = Mock(return_value='a') descriptor.runtime.modulestore = modulestore self.check_property(descriptor) @@ -299,6 +301,8 @@ class XBlockWrapperTestMixin(object): descriptor_cls, fields = cls_and_fields self.skip_if_invalid(descriptor_cls) descriptor = ContainerModuleFactory(descriptor_cls=descriptor_cls, depth=2, **fields) + # pylint: disable=no-member + descriptor.runtime.id_reader.get_definition_id = Mock(return_value='a') self.check_property(descriptor) # Test that when an xmodule is generated from descriptor_cls @@ -347,7 +351,9 @@ class TestStudioView(XBlockWrapperTestMixin, TestCase): """ Assert that studio_view and get_html render the same. """ - self.assertEqual(descriptor.get_html(), descriptor.render(STUDIO_VIEW).content) + html = descriptor.get_html() + rendered_content = descriptor.render(STUDIO_VIEW).content + self.assertEqual(html, rendered_content) class TestXModuleHandler(TestCase): diff --git a/common/lib/xmodule/xmodule/tests/xml/__init__.py b/common/lib/xmodule/xmodule/tests/xml/__init__.py index 534d3d3d4c..c6ec34785c 100644 --- a/common/lib/xmodule/xmodule/tests/xml/__init__.py +++ b/common/lib/xmodule/xmodule/tests/xml/__init__.py @@ -2,12 +2,13 @@ Xml parsing tests for XModules """ import pprint +from lxml import etree from mock import Mock from unittest import TestCase from xmodule.x_module import XMLParsingSystem, policy_key from xmodule.mako_module import MakoDescriptorSystem -from xmodule.modulestore.xml import create_block_from_xml, CourseLocationManager +from xmodule.modulestore.xml import CourseLocationManager from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location from xblock.runtime import KvsFieldData, DictKeyValueStore @@ -40,9 +41,9 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable def process_xml(self, xml): # pylint: disable=method-hidden """Parse `xml` as an XBlock, and add it to `self._descriptors`""" - descriptor = create_block_from_xml( - xml, - self, + descriptor = self.xblock_from_node( + etree.fromstring(xml), + None, CourseLocationManager(self.course_id), ) self._descriptors[descriptor.location.to_deprecated_string()] = descriptor diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index a9b093590d..b7f7e10fe5 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -18,7 +18,6 @@ Examples of html5 videos for manual testing: import copy import json import logging -import os.path from collections import OrderedDict from operator import itemgetter @@ -30,14 +29,13 @@ from django.conf import settings from xblock.fields import ScopeIds from xblock.runtime import KvsFieldData -from xmodule.exceptions import NotFoundError from xmodule.modulestore.inheritance import InheritanceKeyValueStore, own_metadata from xmodule.x_module import XModule, module_attr from xmodule.editing_module import TabsEditingDescriptor from xmodule.raw_module import EmptyDataRawDescriptor from xmodule.xml_module import is_pointer_tag, name_to_pathname, deserialize_field -from .transcripts_utils import Transcript, VideoTranscriptsMixin +from .transcripts_utils import VideoTranscriptsMixin from .video_utils import create_youtube_string, get_video_from_cdn from .video_xfields import VideoFields from .video_handlers import VideoStudentViewHandlers, VideoStudioViewHandlers @@ -300,7 +298,7 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler super(VideoDescriptor, self).__init__(*args, **kwargs) # For backwards compatibility -- if we've got XML data, parse it out and set the metadata fields if self.data: - field_data = self._parse_video_xml(self.data) + field_data = self._parse_video_xml(etree.fromstring(self.data)) self._field_data.set_many(self, field_data) del self.data @@ -406,8 +404,9 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler usage_id = id_generator.create_usage(definition_id) if is_pointer_tag(xml_object): filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name)) - xml_data = etree.tostring(cls.load_file(filepath, system.resources_fs, usage_id)) - field_data = cls._parse_video_xml(xml_data) + xml_object = cls.load_file(filepath, system.resources_fs, usage_id) + system.parse_asides(xml_object, definition_id, usage_id, id_generator) + field_data = cls._parse_video_xml(xml_object) kvs = InheritanceKeyValueStore(initial_values=field_data) field_data = KvsFieldData(kvs) video = system.construct_xblock_from_class( @@ -543,12 +542,11 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler return ret @classmethod - def _parse_video_xml(cls, xml_data): + def _parse_video_xml(cls, xml): """ Parse video fields out of xml_data. The fields are set if they are present in the XML. """ - xml = etree.fromstring(xml_data) field_data = {} # Convert between key types for certain attributes -- diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 5b96721153..e660211fd5 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -15,8 +15,9 @@ from pkg_resources import ( from webob import Response from webob.multidict import MultiDict -from xblock.core import XBlock -from xblock.fields import Scope, Integer, Float, List, XBlockMixin, String, Dict +from xblock.core import XBlock, XBlockAside +from xblock.fields import Scope, Integer, Float, List, XBlockMixin, String, Dict, ScopeIds, Reference, \ + ReferenceList, ReferenceValueDict from xblock.fragment import Fragment from xblock.runtime import Runtime, IdReader, IdGenerator from xmodule.fields import RelativeTime @@ -852,6 +853,7 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): """ Interpret the parsed XML in `node`, creating an XModuleDescriptor. """ + # It'd be great to not reserialize and deserialize the xml xml = etree.tostring(node) block = cls.from_xml(xml, runtime, id_generator) return block @@ -1131,14 +1133,13 @@ def descriptor_global_local_resource_url(block, uri): # pylint: disable=invalid raise NotImplementedError("Applications must monkey-patch this function before using local_resource_url for studio_view") -# This function exists to give applications (LMS/CMS) a place to monkey-patch until -# we can refactor modulestore to split out the FieldData half of its interface from -# the Runtime part of its interface. This function matches the Runtime.get_asides interface -def descriptor_global_get_asides(block): # pylint: disable=unused-argument +# pylint: disable=invalid-name +def descriptor_global_applicable_aside_types(block): # pylint: disable=unused-argument """ - See :meth:`xblock.runtime.Runtime.get_asides`. + See :meth:`xblock.runtime.Runtime.applicable_aside_types`. """ - raise NotImplementedError("Applications must monkey-patch this function before using get_asides from a DescriptorSystem.") + raise NotImplementedError("Applications must monkey-patch this function before using applicable_aside_types" + " from a DescriptorSystem.") class MetricsMixin(object): @@ -1315,18 +1316,18 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # p # global function that the application can override. return descriptor_global_local_resource_url(block, uri) - def get_asides(self, block): + def applicable_aside_types(self, block): """ - See :meth:`xblock.runtime.Runtime:get_asides` for documentation. + See :meth:`xblock.runtime.Runtime:applicable_aside_types` for documentation. """ if getattr(block, 'xmodule_runtime', None) is not None: - return block.xmodule_runtime.get_asides(block) + return block.xmodule_runtime.applicable_aside_types(block) else: # Currently, Modulestore is responsible for instantiating DescriptorSystems # This means that LMS/CMS don't have a way to define a subclass of DescriptorSystem # that implements the correct get_asides. So, for now, instead, we will reference a # global function that the application can override. - return descriptor_global_get_asides(block) + return descriptor_global_applicable_aside_types(block) def resource_url(self, resource): """ @@ -1357,6 +1358,103 @@ class XMLParsingSystem(DescriptorSystem): super(XMLParsingSystem, self).__init__(**kwargs) self.process_xml = process_xml + def _usage_id_from_node(self, node, parent_id, id_generator=None): + """Create a new usage id from an XML dom node. + + Args: + node (lxml.etree.Element): The DOM node to interpret. + parent_id: The usage ID of the parent block + id_generator (IdGenerator): The :class:`.IdGenerator` to use + for creating ids + Returns: + UsageKey: the usage key for the new xblock + """ + return self.xblock_from_node(node, parent_id, id_generator).scope_ids.usage_id + + def xblock_from_node(self, node, parent_id, id_generator=None): + """ + Create an XBlock instance from XML data. + + Args: + xml_data (string): A string containing valid xml. + system (XMLParsingSystem): The :class:`.XMLParsingSystem` used to connect the block + to the outside world. + id_generator (IdGenerator): An :class:`~xblock.runtime.IdGenerator` that + will be used to construct the usage_id and definition_id for the block. + + Returns: + XBlock: The fully instantiated :class:`~xblock.core.XBlock`. + + """ + id_generator = id_generator or self.id_generator + # leave next line commented out - useful for low-level debugging + # log.debug('[_usage_id_from_node] tag=%s, class=%s' % (node.tag, xblock_class)) + + block_type = node.tag + # remove xblock-family from elements + node.attrib.pop('xblock-family', None) + + url_name = node.get('url_name') # difference from XBlock.runtime + def_id = id_generator.create_definition(block_type, url_name) + usage_id = id_generator.create_usage(def_id) + + keys = ScopeIds(None, block_type, def_id, usage_id) + block_class = self.mixologist.mix(self.load_block_type(block_type)) + + self.parse_asides(node, def_id, usage_id, id_generator) + + block = block_class.parse_xml(node, self, keys, id_generator) + self._convert_reference_fields_to_keys(block) # difference from XBlock.runtime + block.parent = parent_id + block.save() + + return block + + def parse_asides(self, node, def_id, usage_id, id_generator): + """pull the asides out of the xml payload and instantiate them""" + aside_children = [] + for child in node.iterchildren(): + # get xblock-family from node + xblock_family = child.attrib.pop('xblock-family', None) + if xblock_family: + xblock_family = self._family_id_to_superclass(xblock_family) + if issubclass(xblock_family, XBlockAside): + aside_children.append(child) + # now process them & remove them from the xml payload + for child in aside_children: + self._aside_from_xml(child, def_id, usage_id, id_generator) + node.remove(child) + + def _make_usage_key(self, course_key, value): + """ + Makes value into a UsageKey inside the specified course. + If value is already a UsageKey, returns that. + """ + if isinstance(value, UsageKey): + return value + return course_key.make_usage_key_from_deprecated_string(value) + + def _convert_reference_fields_to_keys(self, xblock): # pylint: disable=invalid-name + """ + Find all fields of type reference and convert the payload into UsageKeys + """ + course_key = xblock.scope_ids.usage_id.course_key + + for field in xblock.fields.itervalues(): + if field.is_set_on(xblock): + field_value = getattr(xblock, field.name) + if field_value is None: + continue + elif isinstance(field, Reference): + setattr(xblock, field.name, self._make_usage_key(course_key, field_value)) + elif isinstance(field, ReferenceList): + setattr(xblock, field.name, [self._make_usage_key(course_key, ele) for ele in field_value]) + elif isinstance(field, ReferenceValueDict): + for key, subvalue in field_value.iteritems(): + assert isinstance(subvalue, basestring) + field_value[key] = self._make_usage_key(course_key, subvalue) + setattr(xblock, field.name, field_value) + class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # pylint: disable=abstract-method """ diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index dc9a3931da..1f7973c188 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -181,10 +181,18 @@ class XmlDescriptor(XModuleDescriptor): raise Exception, msg, sys.exc_info()[2] @classmethod - def load_definition(cls, xml_object, system, def_id): - '''Load a descriptor definition from the specified xml_object. + 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)''' + cases (e.g. html module) + + Args: + xml_object: an lxml.etree._Element containing the definition to load + 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) @@ -208,6 +216,8 @@ class XmlDescriptor(XModuleDescriptor): break definition_xml = cls.load_file(filepath, system.resources_fs, def_id) + usage_id = id_generator.create_usage(def_id) + system.parse_asides(definition_xml, def_id, usage_id, id_generator) # Add the attributes from the pointer node definition_xml.attrib.update(xml_object.attrib) @@ -281,11 +291,12 @@ class XmlDescriptor(XModuleDescriptor): # 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) else: definition_xml = xml_object filepath = None - definition, children = cls.load_definition(definition_xml, system, def_id) # note this removes metadata + definition, children = cls.load_definition(definition_xml, system, def_id, id_generator) # note this removes metadata # VS[compat] -- make Ike's github preview links work in both old and # new file layouts diff --git a/common/test/data/aside/README.md b/common/test/data/aside/README.md new file mode 100644 index 0000000000..59ab392ed3 --- /dev/null +++ b/common/test/data/aside/README.md @@ -0,0 +1 @@ +This is a very very simple course, useful for initial debugging of processing code. diff --git a/common/test/data/aside/course.xml b/common/test/data/aside/course.xml new file mode 100644 index 0000000000..f04994e343 --- /dev/null +++ b/common/test/data/aside/course.xml @@ -0,0 +1 @@ +