Make XModuleDescriptor use __init__ from XBlock

Fixes JIRA LMS-203

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 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule):
         </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 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule):
 
         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 @@ class ErrorDescriptor(ErrorFields, JSONEditingDescriptor):
         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 @@ class MongoKeyValueStore(KeyValueStore):
         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 @@ class MongoKeyValueStore(KeyValueStore):
         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 @@ class MongoKeyValueStore(KeyValueStore):
             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 @@ class MongoKeyValueStore(KeyValueStore):
         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 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                     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

 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 @@ class TestMongoModuleStore(object):
                 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 @@ class XMLModuleStore(ModuleStoreBase):
                     # 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 @@ class CombinedOpenEndedV1Module():
 
         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 @@ class CombinedOpenEndedV1Module():
         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 @@ class CapaFactory(object):
         """
         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 @@ class CapaFactory(object):
 
         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...

common/lib/xmodule/xmodule/tests/test_combined_open_ended.py

@@ -175,7 +175,6 @@ class OpenEndedModuleTest(unittest.TestCase):
         'max_score': max_score,
         'display_name': 'Name',
         'accept_file_upload': False,
-        'rewrite_content_links': "",
         'close_date': None,
         's3_interface': test_util_open_ended.S3_INTERFACE,
         'open_ended_grading_interface': test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE,
@@ -332,7 +331,6 @@ class CombinedOpenEndedModuleTest(unittest.TestCase):
         'max_score': max_score,
         'display_name': 'Name',
         'accept_file_upload': False,
-        'rewrite_content_links': "",
         'close_date': "",
         's3_interface': test_util_open_ended.S3_INTERFACE,
         'open_ended_grading_interface': test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE,
@@ -370,10 +368,15 @@ class CombinedOpenEndedModuleTest(unittest.TestCase):
     full_definition = definition_template.format(prompt=prompt, rubric=rubric, task1=task_xml1, task2=task_xml2)
     descriptor = Mock(data=full_definition)
     test_system = test_system()
-    combinedoe_container = CombinedOpenEndedModule(test_system,
-                                                   location,
-                                                   descriptor,
-                                                   model_data={'data': full_definition, 'weight': '1'})
+    combinedoe_container = CombinedOpenEndedModule(
+        test_system,
+        descriptor,
+        model_data={
+            'data': full_definition,
+            'weight': '1',
+            'location': location
+        }
+    )
 
     def setUp(self):
         # TODO: this constructor call is definitely wrong, but neither branch

common/lib/xmodule/xmodule/tests/test_conditional.py

@@ -60,9 +60,9 @@ class ConditionalFactory(object):
         source_location = Location(["i4x", "edX", "conditional_test", "problem", "SampleProblem"])
         if source_is_error_module:
             # Make an error descriptor and module
-            source_descriptor = NonStaffErrorDescriptor.from_xml('some random xml data', 
+            source_descriptor = NonStaffErrorDescriptor.from_xml('some random xml data',
                                                                  system,
-                                                                 org=source_location.org, 
+                                                                 org=source_location.org,
                                                                  course=source_location.course,
                                                                  error_msg='random error message')
             source_module = source_descriptor.xmodule(system)
@@ -87,8 +87,8 @@ class ConditionalFactory(object):
 
         # construct conditional module:
         cond_location = Location(["i4x", "edX", "conditional_test", "conditional", "SampleConditional"])
-        model_data = {'data': '<conditional/>'}
-        cond_module = ConditionalModule(system, cond_location, cond_descriptor, model_data)
+        model_data = {'data': '<conditional/>', 'location': cond_location}
+        cond_module = ConditionalModule(system, cond_descriptor, model_data)
 
         # return dict:
         return {'cond_module': cond_module,
@@ -106,7 +106,7 @@ class ConditionalModuleBasicTest(unittest.TestCase):
         self.test_system = test_system()
 
     def test_icon_class(self):
-        '''verify that get_icon_class works independent of condition satisfaction''' 
+        '''verify that get_icon_class works independent of condition satisfaction'''
         modules = ConditionalFactory.create(self.test_system)
         for attempted in ["false", "true"]:
             for icon_class in [ 'other', 'problem', 'video']:
@@ -186,7 +186,6 @@ class ConditionalModuleXmlTest(unittest.TestCase):
             if isinstance(descriptor, Location):
                 location = descriptor
                 descriptor = self.modulestore.get_instance(course.id, location, depth=None)
-            location = descriptor.location
             return descriptor.xmodule(self.test_system)
 
         # edx - HarvardX

common/lib/xmodule/xmodule/tests/test_html_module.py

@@ -8,14 +8,13 @@ from xmodule.modulestore import Location
 from . import test_system
 
 class HtmlModuleSubstitutionTestCase(unittest.TestCase):
-    location = Location(["i4x", "edX", "toy", "html", "simple_html"])
     descriptor = Mock()
 
     def test_substitution_works(self):
         sample_xml = '''%%USER_ID%%'''
         module_data = {'data': sample_xml}
         module_system = test_system()
-        module = HtmlModule(module_system, self.location, self.descriptor, module_data)
+        module = HtmlModule(module_system, self.descriptor, module_data)
         self.assertEqual(module.get_html(), str(module_system.anonymous_student_id))
 
 
@@ -26,7 +25,7 @@ class HtmlModuleSubstitutionTestCase(unittest.TestCase):
             </html>
         '''
         module_data = {'data': sample_xml}
-        module = HtmlModule(test_system(), self.location, self.descriptor, module_data)
+        module = HtmlModule(test_system(), self.descriptor, module_data)
         self.assertEqual(module.get_html(), sample_xml)
 
 
@@ -35,6 +34,6 @@ class HtmlModuleSubstitutionTestCase(unittest.TestCase):
         module_data = {'data': sample_xml}
         module_system = test_system()
         module_system.anonymous_student_id = None
-        module = HtmlModule(module_system, self.location, self.descriptor, module_data)
+        module = HtmlModule(module_system, self.descriptor, module_data)
         self.assertEqual(module.get_html(), sample_xml)
 

common/lib/xmodule/xmodule/tests/test_logic.py

@@ -30,13 +30,13 @@ class LogicTest(unittest.TestCase):
             pass
 
         self.system = None
-        self.location = None
         self.descriptor = EmptyClass()
 
         self.xmodule_class = self.descriptor_class.module_class
         self.xmodule = self.xmodule_class(
-            self.system, self.location,
-            self.descriptor, self.raw_model_data
+            self.system,
+            self.descriptor,
+            self.raw_model_data
         )
 
     def ajax_request(self, dispatch, get):

common/lib/xmodule/xmodule/tests/test_mako_module.py

+""" Test mako_module.py """
+
+from unittest import TestCase
+from mock import Mock
+
+from xmodule.mako_module import MakoModuleDescriptor
+
+
+class MakoModuleTest(TestCase):
+    """ Test MakoModuleDescriptor """
+
+    def test_render_template_check(self):
+        mock_system = Mock()
+        mock_system.render_template = None
+
+        with self.assertRaises(TypeError):
+            MakoModuleDescriptor(mock_system, {})
+
+        del mock_system.render_template
+
+        with self.assertRaises(TypeError):
+            MakoModuleDescriptor(mock_system, {})

common/lib/xmodule/xmodule/tests/test_progress.py

@@ -134,6 +134,6 @@ class ModuleProgressTest(unittest.TestCase):
     '''
     def test_xmodule_default(self):
         '''Make sure default get_progress exists, returns None'''
-        xm = x_module.XModule(test_system(), 'a://b/c/d/e', None, {})
+        xm = x_module.XModule(test_system(), None, {'location': 'a://b/c/d/e'})
         p = xm.get_progress()
         self.assertEqual(p, None)

common/lib/xmodule/xmodule/tests/test_video_xml.py

@@ -47,13 +47,13 @@ class VideoFactory(object):
         """Method return Video Xmodule instance."""
         location = Location(["i4x", "edX", "video", "default",
                              "SampleProblem1"])
-        model_data = {'data': VideoFactory.sample_problem_xml_youtube}
+        model_data = {'data': VideoFactory.sample_problem_xml_youtube, 'location': location}
 
         descriptor = Mock(weight="1")
 
         system = test_system()
         system.render_template = lambda template, context: context
-        module = VideoModule(system, location, descriptor, model_data)
+        module = VideoModule(system, descriptor, model_data)
 
         return module
 

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

@@ -142,7 +142,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
     def get_xml_editable_fields(self, model_data):
         system = test_system()
         system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-        return XmlDescriptor(system=system, location=None, model_data=model_data).editable_metadata_fields
+        return XmlDescriptor(runtime=system, model_data=model_data).editable_metadata_fields
 
     def get_descriptor(self, model_data):
         class TestModuleDescriptor(TestFields, XmlDescriptor):
@@ -154,7 +154,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
 
         system = test_system()
         system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-        return TestModuleDescriptor(system=system, location=None, model_data=model_data)
+        return TestModuleDescriptor(runtime=system, model_data=model_data)
 
     def assert_field_values(self, editable_fields, name, field, explicitly_set, inheritable, value, default_value,
                             type='Generic', options=[]):

common/lib/xmodule/xmodule/x_module.py

@@ -10,7 +10,7 @@ from pkg_resources import resource_listdir, resource_string, resource_isdir
 from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
-from xblock.core import XBlock, Scope, String, Integer, Float
+from xblock.core import XBlock, Scope, String, Integer, Float, ModelType
 
 log = logging.getLogger(__name__)
 
@@ -19,6 +19,23 @@ def dummy_track(event_type, event):
     pass
 
 
+class LocationField(ModelType):
+    """
+    XBlock field for storing Location values
+    """
+    def from_json(self, value):
+        """
+        Parse the json value as a Location
+        """
+        return Location(value)
+
+    def to_json(self, value):
+        """
+        Store the Location as a url string in json
+        """
+        return value.url()
+
+
 class HTMLSnippet(object):
     """
     A base class defining an interface for an object that is able to present an
@@ -87,6 +104,16 @@ class XModuleFields(object):
         default=None
     )
 
+    # Please note that in order to be compatible with XBlocks more generally,
+    # the LMS and CMS shouldn't be using this field. It's only for internal
+    # consumption by the XModules themselves
+    location = LocationField(
+        display_name="Location",
+        help="This is the location id for the XModule.",
+        scope=Scope.content,
+        default=Location(None),
+    )
+
 
 class XModule(XModuleFields, HTMLSnippet, XBlock):
     ''' Implements a generic learning module.
@@ -106,24 +133,20 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
     icon_class = 'other'
 
 
-    def __init__(self, system, location, descriptor, model_data):
+    def __init__(self, runtime, descriptor, model_data):
         '''
         Construct a new xmodule
 
-        system: A ModuleSystem allowing access to external resources
-
-        location: Something Location-like that identifies this xmodule
+        runtime: An XBlock runtime allowing access to external resources
 
         descriptor: the XModuleDescriptor that this module is an instance of.
-            TODO (vshnayder): remove the definition parameter and location--they
-            can come from the descriptor.
 
         model_data: A dictionary-like object that maps field names to values
             for those fields.
         '''
+        super(XModule, self).__init__(runtime, model_data)
         self._model_data = model_data
-        self.system = system
-        self.location = Location(location)
+        self.system = runtime
         self.descriptor = descriptor
         self.url_name = self.location.name
         self.category = self.location.category
@@ -254,19 +277,6 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
             get is a dictionary-like object '''
         return ""
 
-    # cdodge: added to support dynamic substitutions of
-    # links for courseware assets (e.g. images). <link> is passed through from lxml.html parser
-    def rewrite_content_links(self, link):
-        # see if we start with our format, e.g. 'xasset:<filename>'
-        if link.startswith(XASSET_SRCREF_PREFIX):
-            # yes, then parse out the name
-            name = link[len(XASSET_SRCREF_PREFIX):]
-            loc = Location(self.location)
-            # resolve the reference to our internal 'filepath' which
-            link = StaticContent.compute_location_filename(loc.org, loc.course, name)
-
-        return link
-
 
 def policy_key(location):
     """
@@ -340,13 +350,12 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
     template_dir_name = "default"
 
     # Class level variable
+
+    # True if this descriptor always requires recalculation of grades, for
+    # example if the score can change via an extrnal service, not just when the
+    # student interacts with the module on the page.  A specific example is
+    # FoldIt, which posts grade-changing updates through a separate API.
     always_recalculate_grades = False
-    """
-    Return whether this descriptor always requires recalculation of grades, for
-    example if the score can change via an extrnal service, not just when the
-    student interacts with the module on the page.  A specific example is
-    FoldIt, which posts grade-changing updates through a separate API.
-    """
 
     # VS[compat].  Backwards compatibility code that can go away after
     # importing 2012 courses.
@@ -357,10 +366,7 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
     }
 
     # ============================= STRUCTURAL MANIPULATION ===================
-    def __init__(self,
-                 system,
-                 location,
-                 model_data):
+    def __init__(self, *args, **kwargs):
         """
         Construct a new XModuleDescriptor. The only required arguments are the
         system, used for interaction with external resources, and the
@@ -371,19 +377,17 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         This allows for maximal flexibility to add to the interface while
         preserving backwards compatibility.
 
-        system: A DescriptorSystem for interacting with external resources
-
-        location: Something Location-like that identifies this xmodule
+        runtime: A DescriptorSystem for interacting with external resources
 
         model_data: A dictionary-like object that maps field names to values
             for those fields.
+
+        XModuleDescriptor.__init__ takes the same arguments as xblock.core:XBlock.__init__
         """
-        self.system = system
-        self.location = Location(location)
+        super(XModuleDescriptor, self).__init__(*args, **kwargs)
+        self.system = self.runtime
         self.url_name = self.location.name
         self.category = self.location.category
-        self._model_data = model_data
-
         self._child_instances = None
 
     @property
@@ -441,7 +445,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         """
         return self.module_class(
             system,
-            self.location,
             self,
             system.xblock_model_data(self),
         )
@@ -510,7 +513,9 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
             else:
                 model_data['data'] = definition['data']
 
-        return cls(system=system, location=json_data['location'], model_data=model_data)
+        model_data['location'] = json_data['location']
+
+        return cls(system, model_data)
 
     @classmethod
     def _translate(cls, key):

common/lib/xmodule/xmodule/xml_module.py

@@ -254,7 +254,7 @@ class XmlDescriptor(XModuleDescriptor):
         definition, children = cls.definition_from_xml(definition_xml, system)
         if definition_metadata:
             definition['definition_metadata'] = definition_metadata
-        definition['filename'] = [ filepath, filename ]     
+        definition['filename'] = [ filepath, filename ]
 
         return definition, children
 
@@ -352,10 +352,10 @@ class XmlDescriptor(XModuleDescriptor):
         for key, value in metadata.items():
             if key not in set(f.name for f in cls.fields + cls.lms.fields):
                 model_data['xml_attributes'][key] = value
+        model_data['location'] = location
 
         return cls(
             system,
-            location,
             model_data,
         )
 

lms/djangoapps/courseware/tests/__init__.py

@@ -77,14 +77,15 @@ class BaseTestXmodule(ModuleStoreTestCase):
             data=self.DATA
         )
 
-        location = self.item_descriptor.location
         system = test_system()
         system.render_template = lambda template, context: context
+        model_data = {'location': self.item_descriptor.location}
+        model_data.update(self.MODEL_DATA)
 
         self.item_module = self.item_descriptor.module_class(
-            system, location, self.item_descriptor, self.MODEL_DATA
+            system, self.item_descriptor, model_data
         )
-        self.item_url = Location(location).url()
+        self.item_url = Location(self.item_module.location).url()
 
         # login all users for acces to Xmodule
         self.clients = {user.username: Client() for user in self.users}

lms/djangoapps/courseware/tests/test_video_mongo.py

@@ -15,8 +15,8 @@ class TestVideo(BaseTestXmodule):
             user.username: self.clients[user.username].post(
                 self.get_url('whatever'),
                 {},
-                HTTP_X_REQUESTED_WITH='XMLHttpRequest')
-            for user in self.users
+                HTTP_X_REQUESTED_WITH='XMLHttpRequest'
+            ) for user in self.users
         }
 
         self.assertEqual(

lms/djangoapps/open_ended_grading/tests.py

@@ -160,7 +160,7 @@ class TestPeerGradingService(LoginEnrollmentTestCase):
         self.course_id = "edX/toy/2012_Fall"
         self.toy = modulestore().get_course(self.course_id)
         location = "i4x://edX/toy/peergrading/init"
-        model_data = {'data': "<peergrading/>"}
+        model_data = {'data': "<peergrading/>", 'location': location}
         self.mock_service = peer_grading_service.MockPeerGradingService()
         self.system = ModuleSystem(
             ajax_url=location,
@@ -172,9 +172,9 @@ class TestPeerGradingService(LoginEnrollmentTestCase):
             s3_interface=test_util_open_ended.S3_INTERFACE,
             open_ended_grading_interface=test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE
         )
-        self.descriptor = peer_grading_module.PeerGradingDescriptor(self.system, location, model_data)
-        model_data = {}
-        self.peer_module = peer_grading_module.PeerGradingModule(self.system, location, self.descriptor, model_data)
+        self.descriptor = peer_grading_module.PeerGradingDescriptor(self.system, model_data)
+        model_data = {'location': location}
+        self.peer_module = peer_grading_module.PeerGradingModule(self.system, self.descriptor, model_data)
         self.peer_module.peer_gs = self.mock_service
         self.logout()