Merge pull request #909 from cpennington/xmodule-mixin

Pull XModule attributes out into a mixin that can be applied to xblocks

cms/envs/common.py

@@ -31,6 +31,7 @@ from path import path
 from lms.xblock.mixin import LmsBlockMixin
 from cms.xmodule_namespace import CmsBlockMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
+from xmodule.x_module import XModuleMixin
 
 ############################ FEATURE CONFIGURATION #############################
 
@@ -168,7 +169,7 @@ MIDDLEWARE_CLASSES = (
 
 # This should be moved into an XBlock Runtime/Application object
 # once the responsibility of XBlock creation is moved out of modulestore - cpennington
-XBLOCK_MIXINS = (LmsBlockMixin, CmsBlockMixin, InheritanceMixin)
+XBLOCK_MIXINS = (LmsBlockMixin, CmsBlockMixin, InheritanceMixin, XModuleMixin)
 
 
 ############################ SIGNAL HANDLERS ################################

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -43,8 +43,6 @@ log = logging.getLogger(__name__)
 #==============================================================================
 
 
-
-
 class SplitMongoModuleStore(ModuleStoreBase):
     """
     A Mongodb backed ModuleStore supporting versions, inheritance,

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

@@ -15,6 +15,7 @@ from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemN
     DuplicateItemError
 from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DescriptionLocator
 from xmodule.modulestore.inheritance import InheritanceMixin
+from xmodule.x_module import XModuleMixin
 from pytz import UTC
 from path import path
 import re
@@ -34,7 +35,7 @@ class SplitModuleTest(unittest.TestCase):
         'db': 'test_xmodule',
         'collection': 'modulestore{0}'.format(uuid.uuid4().hex),
         'fs_root': '',
-        'xblock_mixins': (InheritanceMixin,)
+        'xblock_mixins': (InheritanceMixin, XModuleMixin)
     }
 
     MODULESTORE = {

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

@@ -11,15 +11,11 @@ import json
 import os
 import unittest
 
-import fs
-import fs.osfs
-import numpy
 from mock import Mock
 from path import path
 
-import calc
 from xblock.field_data import DictFieldData
-from xmodule.x_module import ModuleSystem, XModuleDescriptor, DescriptorSystem
+from xmodule.x_module import ModuleSystem, XModuleDescriptor, XModuleMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.mako_module import MakoDescriptorSystem
 
@@ -81,7 +77,7 @@ def get_test_descriptor_system():
         resources_fs=Mock(),
         error_tracker=Mock(),
         render_template=lambda template, context: repr(context),
-        mixins=(InheritanceMixin,),
+        mixins=(InheritanceMixin, XModuleMixin),
     )
 
 

common/lib/xmodule/xmodule/tests/test_import.py

@@ -13,6 +13,7 @@ from xmodule.xml_module import is_pointer_tag
 from xmodule.modulestore import Location
 from xmodule.modulestore.xml import ImportSystem, XMLModuleStore
 from xmodule.modulestore.inheritance import compute_inherited_metadata
+from xmodule.x_module import XModuleMixin
 from xmodule.fields import Date
 from xmodule.tests import DATA_DIR
 from xmodule.modulestore.inheritance import InheritanceMixin
@@ -42,7 +43,7 @@ class DummySystem(ImportSystem):
             error_tracker=error_tracker,
             parent_tracker=parent_tracker,
             load_error_modules=load_error_modules,
-            mixins=(InheritanceMixin,)
+            mixins=(InheritanceMixin, XModuleMixin)
         )
 
     def render_template(self, _template, _context):

common/lib/xmodule/xmodule/tests/test_xml_module.py

@@ -12,10 +12,10 @@ from xblock.runtime import DbModel
 
 from xmodule.fields import Date, Timedelta
 from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin
-from xmodule.x_module import XModuleFields
 from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field
 from xmodule.course_module import CourseDescriptor
 from xmodule.seq_module import SequenceDescriptor
+from xmodule.x_module import XModuleMixin
 
 from xmodule.tests import get_test_descriptor_system
 from xmodule.tests.xml import XModuleXmlImportTest
@@ -65,7 +65,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         # Also tests that xml_attributes is filtered out of XmlDescriptor.
         self.assertEqual(1, len(editable_fields), editable_fields)
         self.assert_field_values(
-            editable_fields, 'display_name', XModuleFields.display_name,
+            editable_fields, 'display_name', XModuleMixin.display_name,
             explicitly_set=False, value=None, default_value=None
         )
 
@@ -73,7 +73,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         # Tests that explicitly_set is correct when a value overrides the default (not inheritable).
         editable_fields = self.get_xml_editable_fields(DictFieldData({'display_name': 'foo'}))
         self.assert_field_values(
-            editable_fields, 'display_name', XModuleFields.display_name,
+            editable_fields, 'display_name', XModuleMixin.display_name,
             explicitly_set=True, value='foo', default_value=None
         )
 

common/lib/xmodule/xmodule/x_module.py

 import logging
-import copy
 import yaml
 import os
 
@@ -11,7 +10,7 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError
 
 from xblock.core import XBlock
-from xblock.fields import Scope, String, Integer, Float, List
+from xblock.fields import Scope, Integer, Float, List, XBlockMixin, String
 from xblock.fragment import Fragment
 from xblock.runtime import Runtime
 from xmodule.modulestore.locator import BlockUsageLocator
@@ -83,7 +82,13 @@ class HTMLSnippet(object):
             .format(self.__class__))
 
 
-class XModuleFields(object):
+class XModuleMixin(XBlockMixin):
+    """
+    Fields and methods used by XModules internally.
+
+    Adding this Mixin to an :class:`XBlock` allows it to cooperate with old-style :class:`XModules`
+    """
+
     display_name = String(
         display_name="Display Name",
         help="This name appears in the horizontal navigation at the top of the page.",
@@ -93,41 +98,6 @@ class XModuleFields(object):
         default=None
     )
 
-
-class XModule(XModuleFields, HTMLSnippet, XBlock):
-    ''' Implements a generic learning module.
-
-        Subclasses must at a minimum provide a definition for get_html in order
-        to be displayed to users.
-
-        See the HTML module for a simple example.
-    '''
-
-    # The default implementation of get_icon_class returns the icon_class
-    # attribute of the class
-    #
-    # This attribute can be overridden by subclasses, and
-    # the function can also be overridden if the icon class depends on the data
-    # in the module
-    icon_class = 'other'
-
-
-    def __init__(self, descriptor, *args, **kwargs):
-        '''
-        Construct a new xmodule
-
-        runtime: An XBlock runtime allowing access to external resources
-
-        descriptor: the XModuleDescriptor that this module is an instance of.
-
-        field_data: A dictionary-like object that maps field names to values
-            for those fields.
-        '''
-        super(XModule, self).__init__(*args, **kwargs)
-        self.system = self.runtime
-        self.descriptor = descriptor
-        self._loaded_children = None
-
     @property
     def id(self):
         return self.location.url()
@@ -155,31 +125,133 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
 
     @property
     def url_name(self):
-        if self.descriptor:
-            return self.descriptor.url_name
-        elif isinstance(self.location, Location):
+        if isinstance(self.location, Location):
             return self.location.name
         elif isinstance(self.location, BlockUsageLocator):
             return self.location.usage_id
         else:
             raise InsufficientSpecificationError()
 
-
     @property
     def display_name_with_default(self):
-        '''
+        """
         Return a display name for the module: use display_name if defined in
         metadata, otherwise convert the url name.
-        '''
+        """
         name = self.display_name
         if name is None:
             name = self.url_name.replace('_', ' ')
         return name
 
+    def get_explicitly_set_fields_by_scope(self, scope=Scope.content):
+        """
+        Get a dictionary of the fields for the given scope which are set explicitly on this xblock. (Including
+        any set to None.)
+        """
+        result = {}
+        for field in self.fields.values():
+            if (field.scope == scope and field.is_set_on(self)):
+                result[field.name] = field.read_json(self)
+        return result
+
+    @property
+    def xblock_kvs(self):
+        """
+        Use w/ caution. Really intended for use by the persistence layer.
+        """
+        # if caller wants kvs, caller's assuming it's up to date; so, decache it
+        self.save()
+        return self._field_data._kvs  # pylint: disable=protected-access
+
     def get_children(self):
-        '''
+        """Returns a list of XBlock instances for the children of
+        this module"""
+
+        if not self.has_children:
+            return []
+
+        if getattr(self, '_child_instances', None) is None:
+            self._child_instances = []  # pylint: disable=attribute-defined-outside-init
+            for child_loc in self.children:
+                try:
+                    child = self.runtime.get_block(child_loc)
+                except ItemNotFoundError:
+                    log.exception('Unable to load item {loc}, skipping'.format(loc=child_loc))
+                    continue
+                self._child_instances.append(child)
+
+        return self._child_instances
+
+    def get_required_module_descriptors(self):
+        """Returns a list of XModuleDescriptor instances upon which this module depends, but are
+        not children of this module"""
+        return []
+
+    def get_display_items(self):
+        """
+        Returns a list of descendent module instances that will display
+        immediately inside this module.
+        """
+        items = []
+        for child in self.get_children():
+            items.extend(child.displayable_items())
+
+        return items
+
+    def displayable_items(self):
+        """
+        Returns list of displayable modules contained by this module. If this
+        module is visible, should return [self].
+        """
+        return [self]
+
+    def get_child_by(self, selector):
+        """
+        Return a child XBlock that matches the specified selector
+        """
+        for child in self.get_children():
+            if selector(child):
+                return child
+        return None
+
+
+class XModule(XModuleMixin, HTMLSnippet, XBlock):  # pylint: disable=abstract-method
+    """ Implements a generic learning module.
+
+        Subclasses must at a minimum provide a definition for get_html in order
+        to be displayed to users.
+
+        See the HTML module for a simple example.
+    """
+
+    # The default implementation of get_icon_class returns the icon_class
+    # attribute of the class
+    #
+    # This attribute can be overridden by subclasses, and
+    # the function can also be overridden if the icon class depends on the data
+    # in the module
+    icon_class = 'other'
+
+    def __init__(self, descriptor, *args, **kwargs):
+        """
+        Construct a new xmodule
+
+        runtime: An XBlock runtime allowing access to external resources
+
+        descriptor: the XModuleDescriptor that this module is an instance of.
+
+        field_data: A dictionary-like object that maps field names to values
+            for those fields.
+        """
+        super(XModule, self).__init__(*args, **kwargs)
+        self.system = self.runtime
+        self.descriptor = descriptor
+        self._loaded_children = None
+
+    def get_children(self):
+        """
         Return module instances for all the children of this module.
-        '''
+        """
         if self._loaded_children is None:
             child_descriptors = self.get_child_descriptors()
 
@@ -200,7 +272,7 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
         return '<x_module(id={0})>'.format(self.id)
 
     def get_child_descriptors(self):
-        '''
+        """
         Returns the descriptors of the child modules
 
         Overriding this changes the behavior of get_children and
@@ -211,40 +283,13 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
 
         These children will be the same children returned by the
         descriptor unless descriptor.has_dynamic_children() is true.
-        '''
-        return self.descriptor.get_children()
-
-    def get_child_by(self, selector):
         """
-        Return a child XModuleDescriptor with the specified url_name, if it exists, and None otherwise.
-        """
-        for child in self.get_children():
-            if selector(child):
-                return child
-        return None
-
-    def get_display_items(self):
-        '''
-        Returns a list of descendent module instances that will display
-        immediately inside this module.
-        '''
-        items = []
-        for child in self.get_children():
-            items.extend(child.displayable_items())
-
-        return items
-
-    def displayable_items(self):
-        '''
-        Returns list of displayable modules contained by this module. If this
-        module is visible, should return [self].
-        '''
-        return [self]
+        return self.descriptor.get_children()
 
     def get_icon_class(self):
-        '''
+        """
         Return a css class identifying this module in the context of an icon
-        '''
+        """
         return self.icon_class
 
     # Functions used in the LMS
@@ -267,7 +312,7 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
         return None
 
     def max_score(self):
-        ''' Maximum score. Two notes:
+        """ Maximum score. Two notes:
 
             * This is generic; in abstract, a problem could be 3/5 points on one
               randomization, and 5/7 on another
@@ -275,22 +320,22 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
             * In practice, this is a Very Bad Idea, and (a) will break some code
               in place (although that code should get fixed), and (b) break some
               analytics we plan to put in place.
-        '''
+        """
         return None
 
     def get_progress(self):
-        ''' Return a progress.Progress object that represents how far the
+        """ Return a progress.Progress object that represents how far the
         student has gone in this module.  Must be implemented to get correct
         progress tracking behavior in nesting modules like sequence and
         vertical.
 
         If this module has no notion of progress, return None.
-        '''
+        """
         return None
 
     def handle_ajax(self, _dispatch, _data):
-        ''' dispatch is last part of the URL.
-            data is a dictionary-like object with the content of the request'''
+        """ dispatch is last part of the URL.
+            data is a dictionary-like object with the content of the request"""
         return ""
 
 
@@ -381,7 +426,7 @@ class ResourceTemplates(object):
             return None
 
 
-class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
+class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
     """
     An XModuleDescriptor is a specification for an element of a course. This
     could be a problem, an organizational element (a group of content), or a
@@ -446,86 +491,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         self.edited_by = self.edited_on = self.previous_version = self.update_version = self.definition_locator = None
         self._child_instances = None
 
-    @property
-    def id(self):
-        return self.location.url()
-
-    @property
-    def category(self):
-        return self.scope_ids.block_type
-
-    @property
-    def location(self):
-        try:
-            return Location(self.scope_ids.usage_id)
-        except InvalidLocationError:
-            if isinstance(self.scope_ids.usage_id, BlockUsageLocator):
-                return self.scope_ids.usage_id
-            else:
-                return BlockUsageLocator(self.scope_ids.usage_id)
-
-    @location.setter
-    def location(self, value):
-        self.scope_ids = self.scope_ids._replace(
-            def_id=value,
-            usage_id=value,
-        )
-
-    @property
-    def url_name(self):
-        if isinstance(self.location, Location):
-            return self.location.name
-        elif isinstance(self.location, BlockUsageLocator):
-            return self.location.usage_id
-        else:
-            raise InsufficientSpecificationError()
-
-
-    @property
-    def display_name_with_default(self):
-        '''
-        Return a display name for the module: use display_name if defined in
-        metadata, otherwise convert the url name.
-        '''
-        name = self.display_name
-        if name is None:
-            name = self.url_name.replace('_', ' ')
-        return name
-
-    def get_required_module_descriptors(self):
-        """Returns a list of XModuleDescritpor instances upon which this module depends, but are
-        not children of this module"""
-        return []
-
-    def get_children(self):
-        """Returns a list of XModuleDescriptor instances for the children of
-        this module"""
-        if not self.has_children:
-            return []
-
-        if self._child_instances is None:
-            self._child_instances = []
-            for child_loc in self.children:
-                if isinstance(child_loc, XModuleDescriptor):
-                    child = child_loc
-                else:
-                    try:
-                        child = self.runtime.get_block(child_loc)
-                    except ItemNotFoundError:
-                        log.exception('Unable to load item {loc}, skipping'.format(loc=child_loc))
-                        continue
-                self._child_instances.append(child)
-
-        return self._child_instances
-
-    def get_child_by(self, selector):
-        """
-        Return a child XModuleDescriptor with the specified url_name, if it exists, and None otherwise.
-        """
-        for child in self.get_children():
-            if selector(child):
-                return child
-        return None
 
     def xmodule(self, system):
         """
@@ -619,15 +584,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         raise NotImplementedError(
             'Modules must implement export_to_xml to enable xml export')
 
-    @property
-    def xblock_kvs(self):
-        """
-        Use w/ caution. Really intended for use by the persistence layer.
-        """
-        # if caller wants kvs, caller's assuming it's up to date; so, decache it
-        self.save()
-        return self._field_data._kvs
-
     # =============================== BUILTIN METHODS ==========================
     def __eq__(self, other):
         return (self.scope_ids == other.scope_ids and
@@ -655,17 +611,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         return [XBlock.tags, XBlock.name]
 
 
-    def get_explicitly_set_fields_by_scope(self, scope=Scope.content):
-        """
-        Get a dictionary of the fields for the given scope which are set explicitly on this xblock. (Including
-        any set to None.)
-        """
-        result = {}
-        for field in self.fields.values():
-            if (field.scope == scope and field.is_set_on(self)):
-                result[field.name] = field.read_json(self)
-        return result
-
     @property
     def editable_metadata_fields(self):
         """
@@ -825,7 +770,7 @@ class XMLParsingSystem(DescriptorSystem):
 
 
 class ModuleSystem(Runtime):
-    '''
+    """
     This is an abstraction such that x_modules can function independent
     of the courseware (e.g. import into other types of courseware, LMS,
     or if we want to have a sandbox server for user-contributed content)
@@ -835,7 +780,7 @@ class ModuleSystem(Runtime):
 
     Note that these functions can be closures over e.g. a django request
     and user, or other environment-specific info.
-    '''
+    """
     def __init__(
             self, ajax_url, track_function, get_module, render_template,
             replace_urls, xmodule_field_data, user=None, filestore=None,
@@ -844,7 +789,7 @@ class ModuleSystem(Runtime):
             open_ended_grading_interface=None, s3_interface=None,
             cache=None, can_execute_unsafe_code=None, replace_course_urls=None,
             replace_jump_to_id_urls=None, **kwargs):
-        '''
+        """
         Create a closure around the system environment.
 
         ajax_url - the url where ajax calls to the encapsulating module go.
@@ -893,7 +838,7 @@ class ModuleSystem(Runtime):
         can_execute_unsafe_code - A function returning a boolean, whether or
             not to allow the execution of unsafe, unsandboxed code.
 
-        '''
+        """
         super(ModuleSystem, self).__init__(**kwargs)
 
         self.ajax_url = ajax_url
@@ -926,11 +871,11 @@ class ModuleSystem(Runtime):
         self.replace_jump_to_id_urls = replace_jump_to_id_urls
 
     def get(self, attr):
-        '''	provide uniform access to attributes (like etree).'''
+        """	provide uniform access to attributes (like etree)."""
         return self.__dict__.get(attr)
 
     def set(self, attr, val):
-        '''provide uniform access to attributes (like etree)'''
+        """provide uniform access to attributes (like etree)"""
         self.__dict__[attr] = val
 
     def __repr__(self):

lms/envs/common.py

@@ -32,6 +32,7 @@ from .discussionsettings import *
 
 from lms.xblock.mixin import LmsBlockMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
+from xmodule.x_module import XModuleMixin
 
 ################################### FEATURES ###################################
 # The display name of the platform to be used in templates/emails/etc.
@@ -363,7 +364,7 @@ CONTENTSTORE = None
 
 # This should be moved into an XBlock Runtime/Application object
 # once the responsibility of XBlock creation is moved out of modulestore - cpennington
-XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin)
+XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin, XModuleMixin)
 
 #################### Python sandbox ############################################