Feature/cale/xblock init

CHANGELOG.rst

@@ -17,6 +17,10 @@ captions.
 
 CMS: Allow editors to delete uploaded files/assets
 
+XModules: `XModuleDescriptor.__init__` and `XModule.__init__` dropped the
+`location` parameter (and added it as a field), and renamed `system` to `runtime`,
+to accord more closely to `XBlock.__init__`
+
 LMS: Some errors handling Non-ASCII data in XML courses have been fixed.
 
 LMS: Add page-load tracking using segment-io (if SEGMENT_IO_LMS_KEY and

common/lib/xmodule/xmodule/capa_module.py

@@ -117,6 +117,8 @@ class CapaModule(CapaFields, XModule):
     '''
     An XModule implementing LonCapa format problems, implemented by way of
     capa.capa_problem.LoncapaProblem
+
+    CapaModule.__init__ takes the same arguments as xmodule.x_module:XModule.__init__
     '''
     icon_class = 'problem'
 
@@ -131,8 +133,9 @@ class CapaModule(CapaFields, XModule):
     js_module_name = "Problem"
     css = {'scss': [resource_string(__name__, 'css/capa/display.scss')]}
 
-    def __init__(self, system, location, descriptor, model_data):
-        XModule.__init__(self, system, location, descriptor, model_data)
+    def __init__(self, *args, **kwargs):
+        """ Accepts the same arguments as xmodule.x_module:XModule.__init__ """
+        XModule.__init__(self, *args, **kwargs)
 
         due_date = self.due
 

common/lib/xmodule/xmodule/combined_open_ended_module.py

@@ -116,6 +116,8 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule):
     incorporates multiple children (tasks):
         openendedmodule
         selfassessmentmodule
+
+    CombinedOpenEndedModule.__init__ takes the same arguments as xmodule.x_module:XModule.__init__
     """
     STATE_VERSION = 1
 
@@ -139,8 +141,7 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule):
 
     css = {'scss': [resource_string(__name__, 'css/combinedopenended/display.scss')]}
 
-    def __init__(self, system, location, descriptor, model_data):
-        XModule.__init__(self, system, location, descriptor, model_data)
+    def __init__(self, *args, **kwargs):
         """
         Definition file should have one or many task blocks, a rubric block, and a prompt block:
 
@@ -175,9 +176,9 @@ def __init__(self, system, location, descriptor, model_data):
         </combinedopenended>
 
         """
+        XModule.__init__(self, *args, **kwargs)
 
-        self.system = system
-        self.system.set('location', location)
+        self.system.set('location', self.location)
 
         if self.task_states is None:
             self.task_states = []
@@ -189,13 +190,11 @@ def __init__(self, system, location, descriptor, model_data):
 
         attributes = self.student_attributes + self.settings_attributes
 
-        static_data = {
-            'rewrite_content_links': self.rewrite_content_links,
-        }
+        static_data = {}
         instance_state = {k: getattr(self, k) for k in attributes}
         self.child_descriptor = version_tuple.descriptor(self.system)
         self.child_definition = version_tuple.descriptor.definition_from_xml(etree.fromstring(self.data), self.system)
-        self.child_module = version_tuple.module(self.system, location, self.child_definition, self.child_descriptor,
+        self.child_module = version_tuple.module(self.system, self.location, self.child_definition, self.child_descriptor,
                                                  instance_state=instance_state, static_data=static_data,
                                                  attributes=attributes)
         self.save_instance_data()

common/lib/xmodule/xmodule/error_module.py

@@ -94,11 +94,11 @@ def _construct(cls, system, contents, error_msg, location):
         model_data = {
             'error_msg': str(error_msg),
             'contents': contents,
-            'display_name': 'Error: ' + location.name
+            'display_name': 'Error: ' + location.name,
+            'location': location,
         }
         return cls(
             system,
-            location,
             model_data,
         )
 

common/lib/xmodule/xmodule/mako_module.py

@@ -18,14 +18,16 @@ class MakoModuleDescriptor(XModuleDescriptor):
     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__
     """
 
-    def __init__(self, system, location, model_data):
-        if getattr(system, 'render_template', None) is None:
-            raise TypeError('{system} must have a render_template function'
+    def __init__(self, *args, **kwargs):
+        super(MakoModuleDescriptor, self).__init__(*args, **kwargs)
+        if getattr(self.runtime, 'render_template', None) is None:
+            raise TypeError('{runtime} must have a render_template function'
                             ' in order to use a MakoDescriptor'.format(
-                    system=system))
-        super(MakoModuleDescriptor, self).__init__(system, location, model_data)
+                    runtime=self.runtime))
 
     def get_context(self):
         """

common/lib/xmodule/xmodule/modulestore/mongo.py

@@ -37,15 +37,23 @@ def get_course_id_no_run(location):
     return "/".join([location.org, location.course])
 
 
+class InvalidWriteError(Exception):
+    """
+    Raised to indicate that writing to a particular key
+    in the KeyValueStore is disabled
+    """
+
+
 class MongoKeyValueStore(KeyValueStore):
     """
     A KeyValueStore that maps keyed data access to one of the 3 data areas
     known to the MongoModuleStore (data, children, and metadata)
     """
-    def __init__(self, data, children, metadata):
+    def __init__(self, data, children, metadata, location):
         self._data = data
         self._children = children
         self._metadata = metadata
+        self._location = location
 
     def get(self, key):
         if key.scope == Scope.children:
@@ -55,7 +63,9 @@ def get(self, key):
         elif key.scope == Scope.settings:
             return self._metadata[key.field_name]
         elif key.scope == Scope.content:
-            if key.field_name == 'data' and not isinstance(self._data, dict):
+            if key.field_name == 'location':
+                return self._location
+            elif key.field_name == 'data' and not isinstance(self._data, dict):
                 return self._data
             else:
                 return self._data[key.field_name]
@@ -68,7 +78,9 @@ def set(self, key, value):
         elif key.scope == Scope.settings:
             self._metadata[key.field_name] = value
         elif key.scope == Scope.content:
-            if key.field_name == 'data' and not isinstance(self._data, dict):
+            if key.field_name == 'location':
+                self._location = value
+            elif key.field_name == 'data' and not isinstance(self._data, dict):
                 self._data = value
             else:
                 self._data[key.field_name] = value
@@ -82,7 +94,9 @@ def delete(self, key):
             if key.field_name in self._metadata:
                 del self._metadata[key.field_name]
         elif key.scope == Scope.content:
-            if key.field_name == 'data' and not isinstance(self._data, dict):
+            if key.field_name == 'location':
+                self._location = Location(None)
+            elif key.field_name == 'data' and not isinstance(self._data, dict):
                 self._data = None
             else:
                 del self._data[key.field_name]
@@ -95,7 +109,9 @@ def has(self, key):
         elif key.scope == Scope.settings:
             return key.field_name in self._metadata
         elif key.scope == Scope.content:
-            if key.field_name == 'data' and not isinstance(self._data, dict):
+            if key.field_name == 'location':
+                return True
+            elif key.field_name == 'data' and not isinstance(self._data, dict):
                 return True
             else:
                 return key.field_name in self._data
@@ -171,10 +187,11 @@ def load_item(self, location):
                     definition.get('data', {}),
                     definition.get('children', []),
                     metadata,
+                    location,
                 )
 
                 model_data = DbModel(kvs, class_, None, MongoUsage(self.course_id, location))
-                module = class_(self, location, model_data)
+                module = class_(self, model_data)
                 if self.cached_metadata is not None:
                     # parent container pointers don't differentiate between draft and non-draft
                     # so when we do the lookup, we should do so with a non-draft location

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

@@ -1,11 +1,14 @@
 import pymongo
 
 from mock import Mock
-from nose.tools import assert_equals, assert_raises, assert_not_equals, with_setup, assert_false
+from nose.tools import assert_equals, assert_raises, assert_not_equals, assert_false
 from pprint import pprint
 
+from xblock.core import Scope
+from xblock.runtime import KeyValueStore, InvalidScopeError
+
 from xmodule.modulestore import Location
-from xmodule.modulestore.mongo import MongoModuleStore
+from xmodule.modulestore.mongo import MongoModuleStore, MongoKeyValueStore
 from xmodule.modulestore.xml_importer import import_from_xml
 from xmodule.templates import update_templates
 
@@ -114,3 +117,75 @@ def test_get_courses_has_no_templates(self):
                 course.location.org == 'edx' and course.location.course == 'templates',
                 '{0} is a template course'.format(course)
             )
+
+class TestMongoKeyValueStore(object):
+
+    def setUp(self):
+        self.data = {'foo': 'foo_value'}
+        self.location = Location('i4x://org/course/category/name@version')
+        self.children = ['i4x://org/course/child/a', 'i4x://org/course/child/b']
+        self.metadata = {'meta': 'meta_val'}
+        self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata, self.location)
+
+    def _check_read(self, key, expected_value):
+        assert_equals(expected_value, self.kvs.get(key))
+        assert self.kvs.has(key)
+
+    def test_read(self):
+        assert_equals(self.data['foo'], self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'foo')))
+        assert_equals(self.location, self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'location')))
+        assert_equals(self.children, self.kvs.get(KeyValueStore.Key(Scope.children, None, None, 'children')))
+        assert_equals(self.metadata['meta'], self.kvs.get(KeyValueStore.Key(Scope.settings, None, None, 'meta')))
+        assert_equals(None, self.kvs.get(KeyValueStore.Key(Scope.parent, None, None, 'parent')))
+
+    def test_read_invalid_scope(self):
+        for scope in (Scope.preferences, Scope.user_info, Scope.user_state):
+            key = KeyValueStore.Key(scope, None, None, 'foo')
+            with assert_raises(InvalidScopeError):
+                self.kvs.get(key)
+            assert_false(self.kvs.has(key))
+
+    def test_read_non_dict_data(self):
+        self.kvs._data = 'xml_data'
+        assert_equals('xml_data', self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'data')))
+
+    def _check_write(self, key, value):
+        self.kvs.set(key, value)
+        assert_equals(value, self.kvs.get(key))
+
+    def test_write(self):
+        yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'foo'), 'new_data')
+        yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'location'), Location('i4x://org/course/category/name@new_version'))
+        yield (self._check_write, KeyValueStore.Key(Scope.children, None, None, 'children'), [])
+        yield (self._check_write, KeyValueStore.Key(Scope.settings, None, None, 'meta'), 'new_settings')
+
+    def test_write_non_dict_data(self):
+        self.kvs._data = 'xml_data'
+        self._check_write(KeyValueStore.Key(Scope.content, None, None, 'data'), 'new_data')
+
+    def test_write_invalid_scope(self):
+        for scope in (Scope.preferences, Scope.user_info, Scope.user_state, Scope.parent):
+            with assert_raises(InvalidScopeError):
+                self.kvs.set(KeyValueStore.Key(scope, None, None, 'foo'), 'new_value')
+
+    def _check_delete_default(self, key, default_value):
+        self.kvs.delete(key)
+        assert_equals(default_value, self.kvs.get(key))
+        assert self.kvs.has(key)
+
+    def _check_delete_key_error(self, key):
+        self.kvs.delete(key)
+        with assert_raises(KeyError):
+            self.kvs.get(key)
+        assert_false(self.kvs.has(key))
+
+    def test_delete(self):
+        yield (self._check_delete_key_error, KeyValueStore.Key(Scope.content, None, None, 'foo'))
+        yield (self._check_delete_default, KeyValueStore.Key(Scope.content, None, None, 'location'), Location(None))
+        yield (self._check_delete_default, KeyValueStore.Key(Scope.children, None, None, 'children'), [])
+        yield (self._check_delete_key_error, KeyValueStore.Key(Scope.settings, None, None, 'meta'))
+
+    def test_delete_invalid_scope(self):
+        for scope in (Scope.preferences, Scope.user_info, Scope.user_state, Scope.parent):
+            with assert_raises(InvalidScopeError):
+                self.kvs.delete(KeyValueStore.Key(scope, None, None, 'foo'))

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -463,7 +463,7 @@ def _load_extra_content(self, system, course_descriptor, category, path, course_
                     # tabs are referenced in policy.json through a 'slug' which is just the filename without the .html suffix
                     slug = os.path.splitext(os.path.basename(filepath))[0]
                     loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, category, slug)
-                    module = HtmlDescriptor(system, loc, {'data': html})
+                    module = HtmlDescriptor(system, {'data': html, 'location': loc})
                     # VS[compat]:
                     # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them)
                     # from the course policy

common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py

@@ -117,7 +117,6 @@ def __init__(self, system, location, definition, descriptor,
 
         self.instance_state = instance_state
         self.display_name = instance_state.get('display_name', "Open Ended")
-        self.rewrite_content_links = static_data.get('rewrite_content_links', "")
 
         #We need to set the location here so the child modules can use it
         system.set('location', location)
@@ -354,17 +353,7 @@ def get_html_base(self):
         Output: Child task HTML
         """
         self.update_task_states()
-        html = self.current_task.get_html(self.system)
-        return_html = html
-        try:
-            #Without try except block, get this error:
-            # File "/home/vik/mitx_all/mitx/common/lib/xmodule/xmodule/x_module.py", line 263, in rewrite_content_links
-            # if link.startswith(XASSET_SRCREF_PREFIX):
-            # Placing try except so that if the error is fixed, this code will start working again.
-            return_html = rewrite_links(html, self.rewrite_content_links)
-        except Exception:
-            pass
-        return return_html
+        return self.current_task.get_html(self.system)
 
     def get_current_attributes(self, task_number):
         """

common/lib/xmodule/xmodule/peer_grading_module.py

@@ -62,6 +62,9 @@ class PeerGradingFields(object):
 
 
 class PeerGradingModule(PeerGradingFields, XModule):
+    """
+    PeerGradingModule.__init__ takes the same arguments as xmodule.x_module:XModule.__init__
+    """
     _VERSION = 1
 
     js = {'coffee': [resource_string(__name__, 'js/src/peergrading/peer_grading.coffee'),
@@ -73,12 +76,11 @@ class PeerGradingModule(PeerGradingFields, XModule):
 
     css = {'scss': [resource_string(__name__, 'css/combinedopenended/display.scss')]}
 
-    def __init__(self, system, location, descriptor, model_data):
-        XModule.__init__(self, system, location, descriptor, model_data)
+    def __init__(self, *args, **kwargs):
+        super(PeerGradingModule, self).__init__(*args, **kwargs)
 
-        # We need to set the location here so the child modules can use it
-        system.set('location', location)
-        self.system = system
+        #We need to set the location here so the child modules can use it
+        self.runtime.set('location', self.location)
         if (self.system.open_ended_grading_interface):
             self.peer_gs = PeerGradingService(self.system.open_ended_grading_interface, self.system)
         else:

common/lib/xmodule/xmodule/tests/test_annotatable_module.py

@@ -29,10 +29,10 @@ class AnnotatableModuleTestCase(unittest.TestCase):
         </annotatable>
     '''
     descriptor = Mock()
-    module_data = {'data': sample_xml}
+    module_data = {'data': sample_xml, 'location': location}
 
     def setUp(self):
-        self.annotatable = AnnotatableModule(test_system(), self.location, self.descriptor, self.module_data)
+        self.annotatable = AnnotatableModule(test_system(), self.descriptor, self.module_data)
 
     def test_annotation_data_attr(self):
         el = etree.fromstring('<annotation title="bar" body="foo" problem="0">test</annotation>')

common/lib/xmodule/xmodule/tests/test_capa_module.py

@@ -86,7 +86,7 @@ def create(graceperiod=None,
         """
         location = Location(["i4x", "edX", "capa_test", "problem",
                              "SampleProblem{0}".format(CapaFactory.next_num())])
-        model_data = {'data': CapaFactory.sample_problem_xml}
+        model_data = {'data': CapaFactory.sample_problem_xml, 'location': location}
 
         if graceperiod is not None:
             model_data['graceperiod'] = graceperiod
@@ -113,7 +113,7 @@ def create(graceperiod=None,
 
         system = test_system()
         system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-        module = CapaModule(system, location, descriptor, model_data)
+        module = CapaModule(system, descriptor, model_data)
 
         if correct:
             # TODO: probably better to actually set the internal state properly, but...