Merge pull request #2129 from cpennington/xblocks-xml-import-export

XBlock Xml Serialization/Deserialization

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -1501,7 +1501,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         """Test new course creation - error path for bad organization name"""
         self.course_data['org'] = 'University of California, Berkeley'
         self.assert_course_creation_failed(
-            "Unable to create course 'Robot Super Course'.\n\nInvalid characters in 'University of California, Berkeley'.")
+            "Unable to create course 'Robot Super Course'.\n\nInvalid characters in u'University of California, Berkeley'.")
 
     def test_create_course_with_course_creation_disabled_staff(self):
         """Test new course creation -- course creation disabled, but staff access."""

cms/djangoapps/contentstore/views/preview.py

@@ -12,7 +12,7 @@ from xmodule.error_module import ErrorDescriptor
 from xmodule.exceptions import NotFoundError, ProcessingError
 from xmodule.modulestore.django import modulestore
 from xmodule.x_module import ModuleSystem
-from xblock.runtime import DbModel
+from xblock.runtime import KvsFieldData
 from xblock.django.request import webob_to_django_response, django_to_webob_request
 from xblock.exceptions import NoSuchHandlerError
 
@@ -142,7 +142,7 @@ def _load_preview_module(request, descriptor):
     request: The active django request
     descriptor: An XModuleDescriptor
     """
-    student_data = DbModel(SessionKeyValueStore(request))
+    student_data = KvsFieldData(SessionKeyValueStore(request))
     descriptor.bind_for_student(
         _preview_module_system(request, descriptor),
         LmsFieldData(descriptor._field_data, student_data),  # pylint: disable=protected-access

common/lib/xmodule/xmodule/abtest_module.py

@@ -111,7 +111,8 @@ class ABTestDescriptor(ABTestFields, RawDescriptor, XmlDescriptor):
             child_content_urls = []
             for child in group:
                 try:
-                    child_content_urls.append(system.process_xml(etree.tostring(child)).location.url())
+                    child_block = system.process_xml(etree.tostring(child))
+                    child_content_urls.append(child_block.scope_ids.usage_id)
                 except:
                     log.exception("Unable to load child when parsing ABTest. Continuing...")
                     continue

common/lib/xmodule/xmodule/backcompat_module.py

@@ -17,7 +17,7 @@ def process_includes(fn):
     are supposed to include
     """
     @wraps(fn)
-    def from_xml(cls, xml_data, system, org=None, course=None):
+    def from_xml(cls, xml_data, system, id_generator):
         xml_object = etree.fromstring(xml_data)
         next_include = xml_object.find('include')
         while next_include is not None:
@@ -55,14 +55,14 @@ def process_includes(fn):
             parent.remove(next_include)
 
             next_include = xml_object.find('include')
-        return fn(cls, etree.tostring(xml_object), system, org, course)
+        return fn(cls, etree.tostring(xml_object), system, id_generator)
     return from_xml
 
 
 class SemanticSectionDescriptor(XModuleDescriptor):
     @classmethod
     @process_includes
-    def from_xml(cls, xml_data, system, org=None, course=None):
+    def from_xml(cls, xml_data, system, id_generator):
         """
         Removes sections with single child elements in favor of just embedding
         the child element
@@ -83,7 +83,7 @@ class SemanticSectionDescriptor(XModuleDescriptor):
 
 class TranslateCustomTagDescriptor(XModuleDescriptor):
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None):
+    def from_xml(cls, xml_data, system, id_generator):
         """
         Transforms the xml_data from <$custom_tag attr="" attr=""/> to
         <customtag attr="" attr="" impl="$custom_tag"/>

common/lib/xmodule/xmodule/conditional_module.py

@@ -20,7 +20,7 @@ log = logging.getLogger('edx.' + __name__)
 
 class ConditionalFields(object):
     has_children = True
-    show_tag_list = List(help="Poll answers", scope=Scope.content)
+    show_tag_list = List(help="List of urls of children that are references to external modules", scope=Scope.content)
 
 
 class ConditionalModule(ConditionalFields, XModule):
@@ -196,6 +196,7 @@ class ConditionalDescriptor(ConditionalFields, SequenceDescriptor):
             locations = [location.strip() for location in sources.split(';')]
             for location in locations:
                 if Location.is_valid(location):  # Check valid location url.
+                    location = Location(location)
                     try:
                         if return_descriptor:
                             descriptor = system.load_item(location)
@@ -221,15 +222,13 @@ class ConditionalDescriptor(ConditionalFields, SequenceDescriptor):
         show_tag_list = []
         for child in xml_object:
             if child.tag == 'show':
-                location = ConditionalDescriptor.parse_sources(
-                    child, system)
+                location = ConditionalDescriptor.parse_sources(child, system)
                 children.extend(location)
-                show_tag_list.extend(location)
+                show_tag_list.extend(location.url())
             else:
                 try:
                     descriptor = system.process_xml(etree.tostring(child))
-                    module_url = descriptor.location.url()
-                    children.append(module_url)
+                    children.append(descriptor.scope_ids.usage_id)
                 except:
                     msg = "Unable to load child when parsing Conditional."
                     log.exception(msg)

common/lib/xmodule/xmodule/course_module.py

@@ -497,8 +497,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
         return policy_str
 
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None):
-        instance = super(CourseDescriptor, cls).from_xml(xml_data, system, org, course)
+    def from_xml(cls, xml_data, system, id_generator):
+        instance = super(CourseDescriptor, cls).from_xml(xml_data, system, id_generator)
 
         # bleh, have to parse the XML here to just pull out the url_name attribute
         # I don't think it's stored anywhere in the instance.

common/lib/xmodule/xmodule/crowdsource_hinter.py

@@ -388,7 +388,8 @@ class CrowdsourceHinterDescriptor(CrowdsourceHinterFields, RawDescriptor):
         children = []
         for child in xml_object:
             try:
-                children.append(system.process_xml(etree.tostring(child, encoding='unicode')).location.url())
+                child_block = system.process_xml(etree.tostring(child, encoding='unicode'))
+                children.append(child_block.scope_ids.usage_id)
             except Exception as e:
                 log.exception("Unable to load child when parsing CrowdsourceHinter. Continuing...")
                 if system.error_tracker is not None:

common/lib/xmodule/xmodule/error_module.py

@@ -81,12 +81,10 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
 
     @classmethod
     def _construct(cls, system, contents, error_msg, location):
-
-        if isinstance(location, dict) and 'course' in location:
         location = Location(location)
-        if isinstance(location, Location) and location.name is None:
+
+        if location.category == 'error':
             location = location.replace(
-                category='error',
                 # Pick a unique url_name -- the sha1 hash of the contents.
                 # NOTE: We could try to pull out the url_name of the errored descriptor,
                 # but url_names aren't guaranteed to be unique between descriptor types,
@@ -136,7 +134,7 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
         )
 
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None,
+    def from_xml(cls, xml_data, system, id_generator,
                  error_msg='Error not available'):
         '''Create an instance of this descriptor from the supplied data.
 
@@ -162,7 +160,7 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
             # Save the error to display later--overrides other problems
             error_msg = exc_info_to_str(sys.exc_info())
 
-        return cls._construct(system, xml_data, error_msg, location=Location('i4x', org, course, None, None))
+        return cls._construct(system, xml_data, error_msg, location=id_generator.create_definition('error'))
 
     def export_to_xml(self, resource_fs):
         '''

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -40,6 +40,21 @@ INVALID_HTML_CHARS = re.compile(r"[^\w-]")
 _LocationBase = namedtuple('LocationBase', 'tag org course category name revision')
 
 
+def _check_location_part(val, regexp):
+    """
+    Check that `regexp` doesn't match inside `val`. If it does, raise an exception
+
+    Args:
+        val (string): The value to check
+        regexp (re.RegexObject): The regular expression specifying invalid characters
+
+    Raises:
+        InvalidLocationError: Raised if any invalid character is found in `val`
+    """
+    if val is not None and regexp.search(val) is not None:
+        raise InvalidLocationError("Invalid characters in {!r}.".format(val))
+
+
 class Location(_LocationBase):
     '''
     Encodes a location.
@@ -145,7 +160,6 @@ class Location(_LocationBase):
         Components may be set to None, which may be interpreted in some contexts
         to mean wildcard selection.
         """
-
         if (org is None and course is None and category is None and name is None and revision is None):
             location = loc_or_tag
         else:
@@ -161,23 +175,18 @@ class Location(_LocationBase):
             check_list(list_)
 
         def check_list(list_):
-            def check(val, regexp):
-                if val is not None and regexp.search(val) is not None:
-                    log.debug('invalid characters val="%s", list_="%s"' % (val, list_))
-                    raise InvalidLocationError("Invalid characters in '%s'." % (val))
-
             list_ = list(list_)
             for val in list_[:4] + [list_[5]]:
-                check(val, INVALID_CHARS)
+                _check_location_part(val, INVALID_CHARS)
             # names allow colons
-            check(list_[4], INVALID_CHARS_NAME)
+            _check_location_part(list_[4], INVALID_CHARS_NAME)
 
         if isinstance(location, Location):
             return location
         elif isinstance(location, basestring):
             match = URL_RE.match(location)
             if match is None:
-                log.debug('location is instance of %s but no URL match' % basestring)
+                log.debug("location %r doesn't match URL", location)
                 raise InvalidLocationError(location)
             groups = match.groupdict()
             check_dict(groups)
@@ -249,6 +258,18 @@ class Location(_LocationBase):
 
         return "/".join([self.org, self.course, self.name])
 
+    def _replace(self, **kwargs):
+        """
+        Return a new :class:`Location` with values replaced
+        by the values specified in `**kwargs`
+        """
+        for name, value in kwargs.iteritems():
+            if name == 'name':
+                _check_location_part(value, INVALID_CHARS_NAME)
+            else:
+                _check_location_part(value, INVALID_CHARS)
+        return super(Location, self)._replace(**kwargs)
+
     def replace(self, **kwargs):
         '''
         Expose a public method for replacing location elements

common/lib/xmodule/xmodule/modulestore/inheritance.py

+"""
+Support for inheritance of fields down an XBlock hierarchy.
+"""
+
 from datetime import datetime
 from pytz import UTC
 
 from xblock.fields import Scope, Boolean, String, Float, XBlockMixin, Dict
+from xblock.runtime import KeyValueStore, KvsFieldData
+
 from xmodule.fields import Date, Timedelta
-from xblock.runtime import KeyValueStore
 
 
 class InheritanceMixin(XBlockMixin):
-    """Field definitions for inheritable fields"""
+    """Field definitions for inheritable fields."""
 
     graded = Boolean(
         help="Whether this module contributes to the final course grade",
+        scope=Scope.settings,
         default=False,
-        scope=Scope.settings
     )
-
     start = Date(
         help="Start time when this module is visible",
         default=datetime(2030, 1, 1, tzinfo=UTC),
         scope=Scope.settings
     )
-    due = Date(help="Date that this problem is due by", scope=Scope.settings)
+    due = Date(
+        help="Date that this problem is due by",
+        scope=Scope.settings,
+    )
     extended_due = Date(
         help="Date that this problem is due by for a particular student. This "
              "can be set by an instructor, and will override the global due "
@@ -29,31 +36,38 @@ class InheritanceMixin(XBlockMixin):
         default=None,
         scope=Scope.user_state,
     )
-    giturl = String(help="url root for course data git repository", scope=Scope.settings)
+    giturl = String(
+        help="url root for course data git repository",
+        scope=Scope.settings,
+    )
     xqa_key = String(help="DO NOT USE", scope=Scope.settings)
     graceperiod = Timedelta(
         help="Amount of time after the due date that submissions will be accepted",
-        scope=Scope.settings
+        scope=Scope.settings,
     )
     showanswer = String(
         help="When to show the problem answer to the student",
         scope=Scope.settings,
-        default="finished"
+        default="finished",
     )
     rerandomize = String(
         help="When to rerandomize the problem",
+        scope=Scope.settings,
         default="never",
-        scope=Scope.settings
     )
     days_early_for_beta = Float(
         help="Number of days early to show content to beta users",
+        scope=Scope.settings,
         default=None,
-        scope=Scope.settings
     )
-    static_asset_path = String(help="Path to use for static assets - overrides Studio c4x://", scope=Scope.settings, default='')
+    static_asset_path = String(
+        help="Path to use for static assets - overrides Studio c4x://",
+        scope=Scope.settings,
+        default='',
+    )
     text_customization = Dict(
         help="String customization substitutions for particular locations",
-        scope=Scope.settings
+        scope=Scope.settings,
     )
     use_latex_compiler = Boolean(
         help="Enable LaTeX templates?",
@@ -104,6 +118,37 @@ def own_metadata(module):
     return module.get_explicitly_set_fields_by_scope(Scope.settings)
 
 
+class InheritingFieldData(KvsFieldData):
+    """A `FieldData` implementation that can inherit value from parents to children."""
+
+    def __init__(self, inheritable_names, **kwargs):
+        """
+        `inheritable_names` is a list of names that can be inherited from
+        parents.
+
+        """
+        super(InheritingFieldData, self).__init__(**kwargs)
+        self.inheritable_names = set(inheritable_names)
+
+    def default(self, block, name):
+        """
+        The default for an inheritable name is found on a parent.
+        """
+        if name in self.inheritable_names and block.parent is not None:
+            parent = block.get_parent()
+            if parent:
+                return getattr(parent, name)
+        super(InheritingFieldData, self).default(block, name)
+
+
+def inheriting_field_data(kvs):
+    """Create an InheritanceFieldData that inherits the names in InheritanceMixin."""
+    return InheritingFieldData(
+        inheritable_names=InheritanceMixin.fields.keys(),
+        kvs=kvs,
+    )
+
+
 class InheritanceKeyValueStore(KeyValueStore):
     """
     Common superclass for kvs's which know about inheritance of settings. Offers simple

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -26,7 +26,8 @@ class LocalId(object):
 
     Should be hashable and distinguishable, but nothing else
     """
-    pass
+    def __str__(self):
+        return "localid_{}".format(id(self))
 
 
 class Locator(object):

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

@@ -26,13 +26,14 @@ from importlib import import_module
 from xmodule.errortracker import null_error_tracker, exc_info_to_str
 from xmodule.mako_module import MakoDescriptorSystem
 from xmodule.error_module import ErrorDescriptor
-from xblock.runtime import DbModel
+from xblock.runtime import KvsFieldData
 from xblock.exceptions import InvalidScopeError
 from xblock.fields import Scope, ScopeIds
 
 from xmodule.modulestore import ModuleStoreWriteBase, Location, MONGO_MODULESTORE_TYPE
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore
+from xmodule.modulestore.xml import LocationReader
 
 log = logging.getLogger(__name__)
 
@@ -146,7 +147,12 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         render_template: a function for rendering templates, as per
             MakoDescriptorSystem
         """
-        super(CachingDescriptorSystem, self).__init__(load_item=self.load_item, **kwargs)
+        super(CachingDescriptorSystem, self).__init__(
+            id_reader=LocationReader(),
+            field_data=None,
+            load_item=self.load_item,
+            **kwargs
+        )
 
         self.modulestore = modulestore
         self.module_data = module_data
@@ -187,7 +193,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                     metadata,
                 )
 
-                field_data = DbModel(kvs)
+                field_data = KvsFieldData(kvs)
                 scope_ids = ScopeIds(None, category, location, location)
                 module = self.construct_xblock_from_class(class_, scope_ids, field_data)
                 if self.cached_metadata is not None:
@@ -480,7 +486,8 @@ class MongoModuleStore(ModuleStoreWriteBase):
         """
         Load an XModuleDescriptor from item, using the children stored in data_cache
         """
-        data_dir = getattr(item, 'data_dir', item['location']['course'])
+        location = Location(item['location'])
+        data_dir = getattr(item, 'data_dir', location.course)
         root = self.fs_root / data_dir
 
         if not root.isdir():
@@ -490,7 +497,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
 
         cached_metadata = {}
         if apply_cached_metadata:
-            cached_metadata = self.get_cached_metadata_inheritance_tree(Location(item['location']))
+            cached_metadata = self.get_cached_metadata_inheritance_tree(location)
 
         # TODO (cdodge): When the 'split module store' work has been completed, we should remove
         # the 'metadata_inheritance_tree' parameter
@@ -505,7 +512,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
             mixins=self.xblock_mixins,
             select=self.xblock_select,
         )
-        return system.load_item(item['location'])
+        return system.load_item(location)
 
     def _load_items(self, items, depth=0):
         """
@@ -779,6 +786,8 @@ class MongoModuleStore(ModuleStoreWriteBase):
         location: Something that can be passed to Location
         children: A list of child item identifiers
         """
+        # Normalize the children to urls
+        children = [Location(child).url() for child in children]
 
         self._update_single_item(location, {'definition.children': children})
         # recompute (and update) the metadata inheritance tree which is cached
@@ -888,5 +897,5 @@ class MongoModuleStore(ModuleStoreWriteBase):
             metadata,
         )
 
-        field_data = DbModel(kvs)
+        field_data = KvsFieldData(kvs)
         return field_data

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

@@ -4,7 +4,7 @@ from xmodule.mako_module import MakoDescriptorSystem
 from xmodule.modulestore.locator import BlockUsageLocator, LocalId
 from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import exc_info_to_str
-from xblock.runtime import DbModel
+from xblock.runtime import KvsFieldData, IdReader
 from ..exceptions import ItemNotFoundError
 from .split_mongo_kvs import SplitMongoKVS
 from xblock.fields import ScopeIds
@@ -12,6 +12,23 @@ from xblock.fields import ScopeIds
 log = logging.getLogger(__name__)
 
 
+class SplitMongoIdReader(IdReader):
+    """
+    An :class:`~xblock.runtime.IdReader` associated with a particular
+    :class:`.CachingDescriptorSystem`.
+    """
+    def __init__(self, system):
+        self.system = system
+
+    def get_definition_id(self, usage_id):
+        usage = self.system.load_item(usage_id)
+        return usage.definition_locator
+
+    def get_block_type(self, def_id):
+        definition = self.system.modulestore.db_connection.get_definition(def_id)
+        return definition['category']
+
+
 class CachingDescriptorSystem(MakoDescriptorSystem):
     """
     A system that has a cache of a course version's json that it will use to load modules
@@ -33,7 +50,12 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         module_data: a dict mapping Location -> json that was cached from the
             underlying modulestore
         """
-        super(CachingDescriptorSystem, self).__init__(load_item=self._load_item, **kwargs)
+        super(CachingDescriptorSystem, self).__init__(
+            id_reader=SplitMongoIdReader(self),
+            field_data=None,
+            load_item=self._load_item,
+            **kwargs
+        )
         self.modulestore = modulestore
         self.course_entry = course_entry
         self.lazy = lazy
@@ -102,7 +124,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
             json_data.get('fields', {}),
             json_data.get('_inherited_settings'),
         )
-        field_data = DbModel(kvs)
+        field_data = KvsFieldData(kvs)
 
         try:
             module = self.construct_xblock_from_class(

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

@@ -485,7 +485,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         }
         """
         course = self._lookup_course(course_locator)['structure']
-        return {'original_version': course['original_version'],
+        return {
+            'original_version': course['original_version'],
             'previous_version': course['previous_version'],
             'edited_by': course['edited_by'],
             'edited_on': course['edited_on']
@@ -1328,8 +1329,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         if depth is None or depth > 0:
             depth = depth - 1 if depth is not None else None
             for child in block_map[block_id]['fields'].get('children', []):
-                descendent_map = self.descendants(block_map, child, depth,
-                    descendent_map)
+                descendent_map = self.descendants(block_map, child, depth, descendent_map)
 
         return descendent_map
 

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

-from nose.tools import assert_equals, assert_raises, assert_not_equals  # pylint: disable=E0611
+import ddt
+
+from unittest import TestCase
 from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import InvalidLocationError
 
+# Pairs for testing the clean* functions.
+# The first item in the tuple is the input string.
+# The second item in the tuple is what the result of
+# replacement should be.
+GENERAL_PAIRS = [
+    ('', ''),
+    (' ', '_'),
+    ('abc,', 'abc_'),
+    ('ab    fg!@//\\aj', 'ab_fg_aj'),
+    (u"ab\xA9", "ab_"),  # no unicode allowed for now
+]
 
-def check_string_roundtrip(url):
-    assert_equals(url, Location(url).url())
-    assert_equals(url, str(Location(url)))
-
-
-def test_string_roundtrip():
-    check_string_roundtrip("tag://org/course/category/name")
-    check_string_roundtrip("tag://org/course/category/name@revision")
 
+@ddt.ddt
+class TestLocations(TestCase):
+    @ddt.data(
+        "tag://org/course/category/name",
+        "tag://org/course/category/name@revision"
+    )
+    def test_string_roundtrip(self, url):
+        self.assertEquals(url, Location(url).url())
+        self.assertEquals(url, str(Location(url)))
 
-input_dict = {
+    @ddt.data(
+        {
             'tag': 'tag',
             'course': 'course',
             'category': 'category',
             'name': 'name',
             'org': 'org'
-}
-
-
-also_valid_dict = {
+        },
+        {
             'tag': 'tag',
             'course': 'course',
             'category': 'category',
             'name': 'name:more_name',
             'org': 'org'
-}
-
-
-input_list = ['tag', 'org', 'course', 'category', 'name']
-
-input_str = "tag://org/course/category/name"
-input_str_rev = "tag://org/course/category/name@revision"
-
-valid = (input_list, input_dict, input_str, input_str_rev, also_valid_dict)
+        },
+        ['tag', 'org', 'course', 'category', 'name'],
+        "tag://org/course/category/name",
+        "tag://org/course/category/name@revision",
+        u"tag://org/course/category/name",
+        u"tag://org/course/category/name@revision",
+    )
+    def test_is_valid(self, loc):
+        self.assertTrue(Location.is_valid(loc))
 
-invalid_dict = {
+    @ddt.data(
+        {
             'tag': 'tag',
             'course': 'course',
             'category': 'category',
             'name': 'name@more_name',
             'org': 'org'
-}
-
-invalid_dict2 = {
+        },
+        {
             'tag': 'tag',
             'course': 'course',
             'category': 'category',
             'name': 'name ',   # extra space
             'org': 'org'
-}
-
-invalid = ("foo", ["foo"], ["foo", "bar"],
+        },
+        "foo",
+        ["foo"],
+        ["foo", "bar"],
         ["foo", "bar", "baz", "blat:blat", "foo:bar"],  # ':' ok in name, not in category
         "tag://org/course/category/name with spaces@revision",
         "tag://org/course/category/name/with/slashes@revision",
-           invalid_dict,
-           invalid_dict2)
-
-
-def test_is_valid():
-    for v in valid:
-        assert_equals(Location.is_valid(v), True)
-
-    for v in invalid:
-        assert_equals(Location.is_valid(v), False)
+        u"tag://org/course/category/name\xae",  # No non-ascii characters for now
+        u"tag://org/course/category\xae/name",  # No non-ascii characters for now
+    )
+    def test_is_invalid(self, loc):
+        self.assertFalse(Location.is_valid(loc))
 
+    def test_dict(self):
+        input_dict = {
+            'tag': 'tag',
+            'course': 'course',
+            'category': 'category',
+            'name': 'name',
+            'org': 'org'
+        }
 
-def test_dict():
-    assert_equals("tag://org/course/category/name", Location(input_dict).url())
-    assert_equals(dict(revision=None, **input_dict), Location(input_dict).dict())
+        self.assertEquals("tag://org/course/category/name", Location(input_dict).url())
+        self.assertEquals(dict(revision=None, **input_dict), Location(input_dict).dict())
 
         input_dict['revision'] = 'revision'
-    assert_equals("tag://org/course/category/name@revision", Location(input_dict).url())
-    assert_equals(input_dict, Location(input_dict).dict())
+        self.assertEquals("tag://org/course/category/name@revision", Location(input_dict).url())
+        self.assertEquals(input_dict, Location(input_dict).dict())
 
-
-def test_list():
-    assert_equals("tag://org/course/category/name", Location(input_list).url())
-    assert_equals(input_list + [None], Location(input_list).list())
+    def test_list(self):
+        input_list = ['tag', 'org', 'course', 'category', 'name']
+        self.assertEquals("tag://org/course/category/name", Location(input_list).url())
+        self.assertEquals(input_list + [None], Location(input_list).list())
 
         input_list.append('revision')
-    assert_equals("tag://org/course/category/name@revision", Location(input_list).url())
-    assert_equals(input_list, Location(input_list).list())
+        self.assertEquals("tag://org/course/category/name@revision", Location(input_list).url())
+        self.assertEquals(input_list, Location(input_list).list())
 
-
-def test_location():
+    def test_location(self):
         input_list = ['tag', 'org', 'course', 'category', 'name']
-    assert_equals("tag://org/course/category/name", Location(Location(input_list)).url())
-
+        self.assertEquals("tag://org/course/category/name", Location(Location(input_list)).url())
 
-def test_none():
-    assert_equals([None] * 6, Location(None).list())
-
-
-def test_invalid_locations():
-    assert_raises(InvalidLocationError, Location, "foo")
-    assert_raises(InvalidLocationError, Location, ["foo", "bar"])
-    assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat/blat", "foo"])
-    assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat", "foo/bar"])
-    assert_raises(InvalidLocationError, Location, "tag://org/course/category/name with spaces@revision")
-    assert_raises(InvalidLocationError, Location, "tag://org/course/category/name/revision")
+    def test_none(self):
+        self.assertEquals([None] * 6, Location(None).list())
 
+    @ddt.data(
+        "foo",
+        ["foo", "bar"],
+        ["foo", "bar", "baz", "blat/blat", "foo"],
+        ["foo", "bar", "baz", "blat", "foo/bar"],
+        "tag://org/course/category/name with spaces@revision",
+        "tag://org/course/category/name/revision",
+    )
+    def test_invalid_locations(self, loc):
+        with self.assertRaises(InvalidLocationError):
+            Location(loc)
 
-def test_equality():
-    assert_equals(
+    def test_equality(self):
+        self.assertEquals(
             Location('tag', 'org', 'course', 'category', 'name'),
             Location('tag', 'org', 'course', 'category', 'name')
         )
 
-    assert_not_equals(
+        self.assertNotEquals(
             Location('tag', 'org', 'course', 'category', 'name1'),
             Location('tag', 'org', 'course', 'category', 'name')
         )
 
-# All the cleaning functions should do the same thing with these
-general_pairs = [('', ''),
-                 (' ', '_'),
-                 ('abc,', 'abc_'),
-                 ('ab    fg!@//\\aj', 'ab_fg_aj'),
-                 (u"ab\xA9", "ab_"),  # no unicode allowed for now
-                 ]
-
-
-def test_clean():
-    pairs = general_pairs + [
+    @ddt.data(
         ('a:b', 'a_b'),  # no colons in non-name components
         ('a-b', 'a-b'),  # dashes ok
         ('a.b', 'a.b'),  # dot ok
-    ]
-    for input, output in pairs:
-        assert_equals(Location.clean(input), output)
-
+        *GENERAL_PAIRS
+    )
+    def test_clean(self, pair):
+        self.assertEquals(Location.clean(pair[0]), pair[1])
 
-def test_clean_for_url_name():
-    pairs = general_pairs + [
+    @ddt.data(
         ('a:b', 'a:b'),  # colons ok in names
         ('a-b', 'a-b'),  # dashes ok in names
         ('a.b', 'a.b'),  # dot ok in names
-    ]
-    for input, output in pairs:
-        assert_equals(Location.clean_for_url_name(input), output)
-
+        *GENERAL_PAIRS
+    )
+    def test_clean_for_url_name(self, pair):
+        self.assertEquals(Location.clean_for_url_name(pair[0]), pair[1])
 
-def test_clean_for_html():
-    pairs = general_pairs + [
+    @ddt.data(
         ("a:b", "a_b"),   # no colons for html use
         ("a-b", "a-b"),   # dashes ok (though need to be replaced in various use locations. ugh.)
         ('a.b', 'a_b'),   # no dots.
-    ]
-    for input, output in pairs:
-        assert_equals(Location.clean_for_html(input), output)
-
+        *GENERAL_PAIRS
+    )
+    def test_clean_for_html(self, pair):
+        self.assertEquals(Location.clean_for_html(pair[0]), pair[1])
 
-def test_html_id():
+    def test_html_id(self):
         loc = Location("tag://org/course/cat/name:more_name@rev")
-    assert_equals(loc.html_id(), "tag-org-course-cat-name_more_name-rev")
-
+        self.assertEquals(loc.html_id(), "tag-org-course-cat-name_more_name-rev")
 
-def test_course_id():
+    def test_course_id(self):
         loc = Location('i4x', 'mitX', '103', 'course', 'test2')
-    assert_equals('mitX/103/test2', loc.course_id)
+        self.assertEquals('mitX/103/test2', loc.course_id)
 
         loc = Location('i4x', 'mitX', '103', '_not_a_course', 'test2')
-    with assert_raises(InvalidLocationError):
-        loc.course_id
+        with self.assertRaises(InvalidLocationError):
+            loc.course_id  # pylint: disable=pointless-statement
+
+    def test_replacement(self):
+        self.assertEquals(
+            Location('t://o/c/c/n@r')._replace(name='new_name'),
+            Location('t://o/c/c/new_name@r'),
+        )
+
+        with self.assertRaises(InvalidLocationError):
+            Location('t://o/c/c/n@r')._replace(name=u'name\xae')
+
+    @ddt.data('org', 'course', 'category', 'name', 'revision')
+    def test_immutable(self, attr):
+        loc = Location('t://o/c/c/n@r')
+        with self.assertRaises(AttributeError):
+            setattr(loc, attr, attr)

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

@@ -285,13 +285,6 @@ class TestMongoKeyValueStore(object):
         self.metadata = {'meta': 'meta_val'}
         self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata)
 
-    def _check_read(self, key, expected_value):
-        """
-        Asserts the get and has methods.
-        """
-        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.children, self.kvs.get(KeyValueStore.Key(Scope.children, None, None, 'children')))

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

 import hashlib
+import itertools
 import json
 import logging
 import os
@@ -17,17 +18,18 @@ from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import make_error_tracker, exc_info_to_str
 from xmodule.course_module import CourseDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
-from xmodule.x_module import XMLParsingSystem, prefer_xmodules
+from xmodule.x_module import XMLParsingSystem, prefer_xmodules, policy_key
 
 from xmodule.html_module import HtmlDescriptor
 from xblock.core import XBlock
 from xblock.fields import ScopeIds
 from xblock.field_data import DictFieldData
+from xblock.runtime import DictKeyValueStore, IdReader, IdGenerator
 
 from . import ModuleStoreReadBase, Location, XML_MODULESTORE_TYPE
 
 from .exceptions import ItemNotFoundError
-from .inheritance import compute_inherited_metadata
+from .inheritance import compute_inherited_metadata, inheriting_field_data
 
 edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False,
                                  remove_comments=True, remove_blank_text=True)
@@ -49,7 +51,7 @@ def clean_out_mako_templating(xml_string):
 class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
     def __init__(self, xmlstore, course_id, course_dir,
                  error_tracker, parent_tracker,
-                 load_error_modules=True, **kwargs):
+                 load_error_modules=True, id_reader=None, **kwargs):
         """
         A class that handles loading from xml.  Does some munging to ensure that
         all elements have unique slugs.
@@ -59,6 +61,10 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         self.unnamed = defaultdict(int)  # category -> num of new url_names for that category
         self.used_names = defaultdict(set)  # category -> set of used url_names
         self.org, self.course, self.url_name = course_id.split('/')
+        if id_reader is None:
+            id_reader = LocationReader()
+        id_generator = CourseLocationGenerator(self.org, self.course)
+
         # cdodge: adding the course_id as passed in for later reference rather than having to recomine the org/course/url_name
         self.course_id = course_id
         self.load_error_modules = load_error_modules
@@ -165,8 +171,10 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 make_name_unique(xml_data)
 
                 descriptor = create_block_from_xml(
-                    etree.tostring(xml_data, encoding='unicode'), self, self.org,
-                    self.course, xmlstore.default_class)
+                    etree.tostring(xml_data, encoding='unicode'),
+                    self,
+                    id_generator,
+                )
             except Exception as err:
                 if not self.load_error_modules:
                     raise
@@ -174,30 +182,34 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 # Didn't load properly.  Fall back on loading as an error
                 # descriptor.  This should never error due to formatting.
 
-                msg = "Error loading from xml. " + unicode(err)[:200]
-                log.warning(msg)
+                msg = "Error loading from xml. %s"
+                log.warning(
+                    msg,
+                    unicode(err)[:200],
                     # Normally, we don't want lots of exception traces in our logs from common
                     # content problems.  But if you're debugging the xml loading code itself,
                     # uncomment the next line.
-                #   log.exception(msg)
+                    # exc_info=True
+                )
+
+                msg = msg % (unicode(err)[:200])
 
                 self.error_tracker(msg)
                 err_msg = msg + "\n" + exc_info_to_str(sys.exc_info())
                 descriptor = ErrorDescriptor.from_xml(
                     xml,
                     self,
-                    self.org,
-                    self.course,
+                    id_generator,
                     err_msg
                 )
 
             descriptor.data_dir = course_dir
 
-            xmlstore.modules[course_id][descriptor.location] = descriptor
+            xmlstore.modules[course_id][descriptor.scope_ids.usage_id] = descriptor
 
-            if hasattr(descriptor, 'children'):
+            if descriptor.has_children:
                 for child in descriptor.get_children():
-                    parent_tracker.add_parent(child.location, descriptor.location)
+                    parent_tracker.add_parent(child.scope_ids.usage_id, descriptor.scope_ids.usage_id)
 
             # After setting up the descriptor, save any changes that we have
             # made to attributes on the descriptor to the underlying KeyValueStore.
@@ -205,50 +217,94 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
             return descriptor
 
         render_template = lambda template, context: u''
+
         # TODO (vshnayder): we are somewhat architecturally confused in the loading code:
         # load_item should actually be get_instance, because it expects the course-specific
         # policy to be loaded.  For now, just add the course_id here...
-        load_item = lambda location: xmlstore.get_instance(course_id, location)
+        def load_item(location):
+            return xmlstore.get_instance(course_id, Location(location))
+
         resources_fs = OSFS(xmlstore.data_dir / course_dir)
+
         super(ImportSystem, self).__init__(
             load_item=load_item,
             resources_fs=resources_fs,
             render_template=render_template,
             error_tracker=error_tracker,
             process_xml=process_xml,
+            id_reader=id_reader,
             **kwargs
         )
 
+    # id_generator is ignored, because each ImportSystem is already local to
+    # a course, and has it's own id_generator already in place
+    def add_node_as_child(self, block, node, id_generator):
+        child_block = self.process_xml(etree.tostring(node))
+        block.children.append(child_block.scope_ids.usage_id)
 
-def create_block_from_xml(xml_data, system, org=None, course=None, default_class=None):
+
+class LocationReader(IdReader):
     """
-    Create an XBlock instance from XML data.
+    IdReader for definition and usage ids that are Locations
+    """
+    def get_definition_id(self, usage_id):
+        return usage_id
 
-    `xml_data' is a string containing valid xml.
+    def get_block_type(self, def_id):
+        location = def_id
+        return location.category
 
-    `system` is an XMLParsingSystem.
 
-    `org` and `course` are optional strings that will be used in the generated
-    block's url identifiers.
+class CourseLocationGenerator(IdGenerator):
+    """
+    IdGenerator for Location-based definition ids and usage ids
+    based within a course
+    """
+    def __init__(self, org, course):
+        self.org = org
+        self.course = course
+        self.autogen_ids = itertools.count(0)
+
+    def create_usage(self, def_id):
+        return Location(def_id)
+
+    def create_definition(self, block_type, slug=None):
+        assert block_type is not None
+        if slug is None:
+            slug = 'autogen_{}_{}'.format(block_type, self.autogen_ids.next())
+        location = Location('i4x', self.org, self.course, block_type, slug)
+        return location
+
+
+def create_block_from_xml(xml_data, system, id_generator):
+    """
+    Create an XBlock instance from XML data.
 
-    `default_class` is the class to instantiate of the XML indicates a class
-    that can't be loaded.
+    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 the fully instantiated XBlock.
+    Returns:
+        XBlock: The fully instantiated :class:`~xblock.core.XBlock`.
 
     """
     node = etree.fromstring(xml_data)
-    raw_class = XBlock.load_class(node.tag, default_class, select=prefer_xmodules)
+    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))
 
-    url_name = node.get('url_name', node.get('slug'))
-    location = Location('i4x', org, course, node.tag, url_name)
+    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, location.category, location, location)
-    xblock = xblock_class.parse_xml(node, system, scope_ids)
+    scope_ids = ScopeIds(None, block_type, def_id, usage_id)
+    xblock = xblock_class.parse_xml(node, system, scope_ids, id_generator)
     return xblock
 
 
@@ -265,10 +321,8 @@ class ParentTracker(object):
         """
         Add a parent of child location to the set of parents.  Duplicate calls have no effect.
 
-        child and parent must be something that can be passed to Location.
+        child and parent must be :class:`.Location` instances.
         """
-        child = Location(child)
-        parent = Location(parent)
         s = self._parents.setdefault(child, set())
         s.add(parent)
 
@@ -276,7 +330,6 @@ class ParentTracker(object):
         """
         returns True iff child has some parents.
         """
-        child = Location(child)
         return child in self._parents
 
     def make_known(self, location):
@@ -288,7 +341,6 @@ class ParentTracker(object):
         """
         Return a list of the parents of this child.  If not is_known(child), will throw a KeyError
         """
-        child = Location(child)
         return list(self._parents[child])
 
 
@@ -326,6 +378,9 @@ class XMLModuleStore(ModuleStoreReadBase):
 
         self.parent_trackers = defaultdict(ParentTracker)
 
+        # All field data will be stored in an inheriting field data.
+        self.field_data = inheriting_field_data(kvs=DictKeyValueStore())
+
         # If we are specifically asked for missing courses, that should
         # be an error.  If we are asked for "all" courses, find the ones
         # that have a course.xml. We sort the dirs in alpha order so we always
@@ -357,8 +412,8 @@ class XMLModuleStore(ModuleStoreReadBase):
 
         if course_descriptor is not None and not isinstance(course_descriptor, ErrorDescriptor):
             self.courses[course_dir] = course_descriptor
-            self._location_errors[course_descriptor.location] = errorlog
-            self.parent_trackers[course_descriptor.id].make_known(course_descriptor.location)
+            self._location_errors[course_descriptor.scope_ids.usage_id] = errorlog
+            self.parent_trackers[course_descriptor.id].make_known(course_descriptor.scope_ids.usage_id)
         else:
             # Didn't load course.  Instead, save the errors elsewhere.
             self.errored_courses[course_dir] = errorlog
@@ -368,7 +423,8 @@ class XMLModuleStore(ModuleStoreReadBase):
         String representation - for debugging
         '''
         return '<XMLModuleStore data_dir=%r, %d courses, %d modules>' % (
-            self.data_dir, len(self.courses), len(self.modules))
+            self.data_dir, len(self.courses), len(self.modules)
+        )
 
     def load_policy(self, policy_path, tracker):
         """
@@ -451,6 +507,10 @@ class XMLModuleStore(ModuleStoreReadBase):
                                      "(or 'name') set.  Set url_name.")
 
             course_id = CourseDescriptor.make_id(org, course, url_name)
+
+            def get_policy(usage_id):
+                return policy.get(policy_key(usage_id), {})
+
             system = ImportSystem(
                 xmlstore=self,
                 course_id=course_id,
@@ -458,9 +518,11 @@ class XMLModuleStore(ModuleStoreReadBase):
                 error_tracker=tracker,
                 parent_tracker=self.parent_trackers[course_id],
                 load_error_modules=self.load_error_modules,
-                policy=policy,
+                get_policy=get_policy,
                 mixins=self.xblock_mixins,
+                default_class=self.default_class,
                 select=self.xblock_select,
+                field_data=self.field_data,
             )
 
             course_descriptor = system.process_xml(etree.tostring(course_data, encoding='unicode'))
@@ -508,7 +570,7 @@ class XMLModuleStore(ModuleStoreReadBase):
                     html = f.read().decode('utf-8')
                     # 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)
+                    loc = course_descriptor.scope_ids.usage_id._replace(category=category, name=slug)
                     module = system.construct_xblock_from_class(
                         HtmlDescriptor,
                         # We're loading a descriptor, so student_id is meaningless
@@ -526,7 +588,7 @@ class XMLModuleStore(ModuleStoreReadBase):
                                 module.display_name = tab['name']
                     module.data_dir = course_dir
                     module.save()
-                    self.modules[course_descriptor.id][module.location] = module
+                    self.modules[course_descriptor.id][module.scope_ids.usage_id] = module
                 except Exception, e:
                     logging.exception("Failed to load %s. Skipping... \
                             Exception: %s", filepath, unicode(e))

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -310,9 +310,7 @@ def import_module(
         source_course_location, dest_course_location, allow_not_found=False,
         do_import_static=True):
 
-    logging.debug('processing import of module {url}...'.format(
-        url=module.location.url()
-    ))
+    logging.debug('processing import of module {}...'.format(module.location.url()))
 
     content = {}
     for field in module.fields.values():
@@ -393,10 +391,10 @@ def import_course_draft(
         xmlstore=xml_module_store,
         course_id=target_location_namespace.course_id,
         course_dir=draft_course_dir,
-        policy={},
         error_tracker=errorlog.tracker,
         parent_tracker=ParentTracker(),
         load_error_modules=False,
+        field_data=None,
     )
 
     # now walk the /vertical directory where each file in there

common/lib/xmodule/xmodule/peer_grading_module.py

 import json
 import logging
 
-from lxml import etree
-
 from datetime import datetime
+from lxml import etree
 from pkg_resources import resource_string
-from .capa_module import ComplexEncoder
-from .x_module import XModule, module_attr
-from .raw_module import RawDescriptor
-from .modulestore.exceptions import ItemNotFoundError, NoPathToItem
-from .timeinfo import TimeInfo
-from .util.duedate import get_extended_due_date
-from xblock.fields import Dict, String, Scope, Boolean, Float
-from xmodule.fields import Date, Timedelta
+from xblock.fields import Dict, String, Scope, Boolean, Float, Reference
 
+from xmodule.capa_module import ComplexEncoder
+from xmodule.fields import Date, Timedelta
+from xmodule.modulestore import Location
+from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem
+from xmodule.raw_module import RawDescriptor
+from xmodule.timeinfo import TimeInfo
+from xmodule.util.duedate import get_extended_due_date
+from xmodule.x_module import XModule, module_attr
 from xmodule.open_ended_grading_classes.peer_grading_service import PeerGradingService, GradingServiceError, MockPeerGradingService
+
 from open_ended_grading_classes import combined_open_ended_rubric
 from django.utils.timezone import UTC
 
@@ -32,7 +33,7 @@ class PeerGradingFields(object):
         default=False,
         scope=Scope.settings
     )
-    link_to_location = String(
+    link_to_location = Reference(
         display_name="Link to Problem Location",
         help='The location of the problem being graded. Only used when "Show Single Problem" is True.',
         default="",
@@ -560,7 +561,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
 
         good_problem_list = []
         for problem in problem_list:
-            problem_location = problem['location']
+            problem_location = Location(problem['location'])
             try:
                 descriptor = self._find_corresponding_module_for_location(problem_location)
             except (NoPathToItem, ItemNotFoundError):
@@ -608,10 +609,10 @@ class PeerGradingModule(PeerGradingFields, XModule):
                 log.error(
                     "Peer grading problem in peer_grading_module called with no get parameters, but use_for_single_location is False.")
                 return {'html': "", 'success': False}
-            problem_location = self.link_to_location
+            problem_location = Location(self.link_to_location)
 
         elif data.get('location') is not None:
-            problem_location = data.get('location')
+            problem_location = Location(data.get('location'))
 
         module = self._find_corresponding_module_for_location(problem_location)
 
@@ -660,7 +661,7 @@ class PeerGradingDescriptor(PeerGradingFields, RawDescriptor):
     metadata_translations = {
         'is_graded': 'graded',
         'attempts': 'max_attempts',
-        'due_data' : 'due'
+        'due_data': 'due'
     }
 
     @property

common/lib/xmodule/xmodule/seq_module.py

@@ -138,7 +138,8 @@ class SequenceDescriptor(SequenceFields, MakoModuleDescriptor, XmlDescriptor):
         children = []
         for child in xml_object:
             try:
-                children.append(system.process_xml(etree.tostring(child, encoding='unicode')).location.url())
+                child_block = system.process_xml(etree.tostring(child, encoding='unicode'))
+                children.append(child_block.scope_ids.usage_id)
             except Exception as e:
                 log.exception("Unable to load child when parsing Sequence. Continuing...")
                 if system.error_tracker is not None:

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

@@ -16,10 +16,12 @@ from mock import Mock
 from path import path
 
 from xblock.field_data import DictFieldData
+
 from xmodule.x_module import ModuleSystem, XModuleDescriptor, XModuleMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.mako_module import MakoDescriptorSystem
 from xmodule.error_module import ErrorDescriptor
+from xmodule.modulestore.xml import LocationReader
 
 
 # Location of common test DATA directory
@@ -88,6 +90,8 @@ def get_test_descriptor_system():
         error_tracker=Mock(),
         render_template=mock_render_template,
         mixins=(InheritanceMixin, XModuleMixin),
+        field_data=DictFieldData({}),
+        id_reader=LocationReader(),
     )
 
 

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

@@ -9,7 +9,7 @@ from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
 from xmodule.error_module import NonStaffErrorDescriptor
 from xmodule.modulestore import Location
-from xmodule.modulestore.xml import ImportSystem, XMLModuleStore
+from xmodule.modulestore.xml import ImportSystem, XMLModuleStore, CourseLocationGenerator
 from xmodule.conditional_module import ConditionalDescriptor
 from xmodule.tests import DATA_DIR, get_test_system, get_test_descriptor_system
 
@@ -32,7 +32,6 @@ class DummySystem(ImportSystem):
             error_tracker=Mock(),
             parent_tracker=Mock(),
             load_error_modules=load_error_modules,
-            policy={},
         )
 
     def render_template(self, template, context):
@@ -61,8 +60,7 @@ class ConditionalFactory(object):
             source_descriptor = NonStaffErrorDescriptor.from_xml(
                 'some random xml data',
                 system,
-                org=source_location.org,
-                course=source_location.course,
+                id_generator=CourseLocationGenerator(source_location.org, source_location.course),
                 error_msg='random error message'
             )
         else:

common/lib/xmodule/xmodule/tests/test_course_module.py

@@ -5,8 +5,10 @@ from fs.memoryfs import MemoryFS
 
 from mock import Mock, patch
 
-from xmodule.modulestore.xml import ImportSystem, XMLModuleStore
+from xblock.runtime import KvsFieldData, DictKeyValueStore
+
 import xmodule.course_module
+from xmodule.modulestore.xml import ImportSystem, XMLModuleStore, LocationReader
 from django.utils.timezone import UTC
 
 
@@ -32,7 +34,6 @@ class DummySystem(ImportSystem):
                                   load_error_modules=load_error_modules)
         course_id = "/".join([ORG, COURSE, 'test_run'])
         course_dir = "test_dir"
-        policy = {}
         error_tracker = Mock()
         parent_tracker = Mock()
 
@@ -40,10 +41,11 @@ class DummySystem(ImportSystem):
             xmlstore=xmlstore,
             course_id=course_id,
             course_dir=course_dir,
-            policy=policy,
             error_tracker=error_tracker,
             parent_tracker=parent_tracker,
             load_error_modules=load_error_modules,
+            field_data=KvsFieldData(DictKeyValueStore()),
+            id_reader=LocationReader(),
         )
 
 

common/lib/xmodule/xmodule/tests/test_error_module.py

@@ -5,9 +5,10 @@ import unittest
 from xmodule.tests import get_test_system
 from xmodule.error_module import ErrorDescriptor, ErrorModule, NonStaffErrorDescriptor
 from xmodule.modulestore import Location
+from xmodule.modulestore.xml import CourseLocationGenerator
 from xmodule.x_module import XModuleDescriptor, XModule
 from mock import MagicMock, Mock, patch
-from xblock.runtime import Runtime, UsageStore
+from xblock.runtime import Runtime, IdReader
 from xblock.field_data import FieldData
 from xblock.fields import ScopeIds
 from xblock.test.tools import unabc
@@ -32,7 +33,11 @@ class TestErrorModule(unittest.TestCase, SetupTestErrorModules):
 
     def test_error_module_xml_rendering(self):
         descriptor = ErrorDescriptor.from_xml(
-            self.valid_xml, self.system, self.org, self.course, self.error_msg)
+            self.valid_xml,
+            self.system,
+            CourseLocationGenerator(self.org, self.course),
+            self.error_msg
+        )
         self.assertIsInstance(descriptor, ErrorDescriptor)
         descriptor.xmodule_runtime = self.system
         context_repr = self.system.render(descriptor, 'student_view').content
@@ -63,12 +68,18 @@ class TestNonStaffErrorModule(unittest.TestCase, SetupTestErrorModules):
 
     def test_non_staff_error_module_create(self):
         descriptor = NonStaffErrorDescriptor.from_xml(
-            self.valid_xml, self.system, self.org, self.course)
+            self.valid_xml,
+            self.system,
+            CourseLocationGenerator(self.org, self.course)
+        )
         self.assertIsInstance(descriptor, NonStaffErrorDescriptor)
 
     def test_from_xml_render(self):
         descriptor = NonStaffErrorDescriptor.from_xml(
-            self.valid_xml, self.system, self.org, self.course)
+            self.valid_xml,
+            self.system,
+            CourseLocationGenerator(self.org, self.course)
+        )
         descriptor.xmodule_runtime = self.system
         context_repr = self.system.render(descriptor, 'student_view').content
         self.assertNotIn(self.error_msg, context_repr)
@@ -117,11 +128,11 @@ class TestErrorModuleConstruction(unittest.TestCase):
     def setUp(self):
         field_data = Mock(spec=FieldData)
         self.descriptor = BrokenDescriptor(
-            TestRuntime(Mock(spec=UsageStore), field_data),
+            TestRuntime(Mock(spec=IdReader), field_data),
             field_data,
             ScopeIds(None, None, None, 'i4x://org/course/broken/name')
         )
-        self.descriptor.xmodule_runtime = TestRuntime(Mock(spec=UsageStore), field_data)
+        self.descriptor.xmodule_runtime = TestRuntime(Mock(spec=IdReader), field_data)
         self.descriptor.xmodule_runtime.error_descriptor_class = ErrorDescriptor
         self.descriptor.xmodule_runtime.xmodule_instance = None
 

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

 # -*- coding: utf-8 -*-
 
 import datetime
+import ddt
 import unittest
 
 from fs.memoryfs import MemoryFS
@@ -11,13 +12,17 @@ from django.utils.timezone import UTC
 
 from xmodule.xml_module import is_pointer_tag
 from xmodule.modulestore import Location
-from xmodule.modulestore.xml import ImportSystem, XMLModuleStore
+from xmodule.modulestore.xml import ImportSystem, XMLModuleStore, LocationReader
 from xmodule.modulestore.inheritance import compute_inherited_metadata
 from xmodule.x_module import XModuleMixin, only_xmodules
 from xmodule.fields import Date
 from xmodule.tests import DATA_DIR
 from xmodule.modulestore.inheritance import InheritanceMixin
 
+from xblock.core import XBlock
+from xblock.fields import Scope, String, Integer
+from xblock.runtime import KvsFieldData, DictKeyValueStore
+
 
 ORG = 'test_org'
 COURSE = 'test_course'
@@ -31,7 +36,6 @@ class DummySystem(ImportSystem):
         xmlstore = XMLModuleStore("data_dir", course_dirs=[], load_error_modules=load_error_modules)
         course_id = "/".join([ORG, COURSE, 'test_run'])
         course_dir = "test_dir"
-        policy = {}
         error_tracker = Mock()
         parent_tracker = Mock()
 
@@ -39,11 +43,12 @@ class DummySystem(ImportSystem):
             xmlstore=xmlstore,
             course_id=course_id,
             course_dir=course_dir,
-            policy=policy,
             error_tracker=error_tracker,
             parent_tracker=parent_tracker,
             load_error_modules=load_error_modules,
-            mixins=(InheritanceMixin, XModuleMixin)
+            mixins=(InheritanceMixin, XModuleMixin),
+            field_data=KvsFieldData(DictKeyValueStore()),
+            id_reader=LocationReader(),
         )
 
     def render_template(self, _template, _context):
@@ -72,6 +77,40 @@ class BaseCourseTestCase(unittest.TestCase):
         return courses[0]
 
 
+class GenericXBlock(XBlock):
+    has_children = True
+    field1 = String(default="something", scope=Scope.user_state)
+    field2 = Integer(scope=Scope.user_state)
+
+
+@ddt.ddt
+class PureXBlockImportTest(BaseCourseTestCase):
+
+    def assert_xblocks_are_good(self, block):
+        """Assert a number of conditions that must be true for `block` to be good."""
+        scope_ids = block.scope_ids
+        self.assertIsNotNone(scope_ids.usage_id)
+        self.assertIsNotNone(scope_ids.def_id)
+
+        for child_id in block.children:
+            child = block.runtime.get_block(child_id)
+            self.assert_xblocks_are_good(child)
+
+    @XBlock.register_temp_plugin(GenericXBlock)
+    @ddt.data(
+        "<genericxblock/>",
+        "<genericxblock field1='abc' field2='23' />",
+        "<genericxblock field1='abc' field2='23'><genericxblock/></genericxblock>",
+    )
+    @patch('xmodule.x_module.XModuleMixin.location')
+    def test_parsing_pure_xblock(self, xml, mock_location):
+        system = self.get_system(load_error_modules=False)
+        descriptor = system.process_xml(xml)
+        self.assertIsInstance(descriptor, GenericXBlock)
+        self.assert_xblocks_are_good(descriptor)
+        self.assertFalse(mock_location.called)
+
+
 class ImportTestCase(BaseCourseTestCase):
     date = Date()
 
@@ -119,13 +158,10 @@ class ImportTestCase(BaseCourseTestCase):
         tag_xml = descriptor.export_to_xml(resource_fs)
         re_import_descriptor = system.process_xml(tag_xml)
 
-        self.assertEqual(re_import_descriptor.__class__.__name__,
-                         'ErrorDescriptorWithMixins')
+        self.assertEqual(re_import_descriptor.__class__.__name__, 'ErrorDescriptorWithMixins')
 
-        self.assertEqual(descriptor.contents,
-                         re_import_descriptor.contents)
-        self.assertEqual(descriptor.error_msg,
-                         re_import_descriptor.error_msg)
+        self.assertEqual(descriptor.contents, re_import_descriptor.contents)
+        self.assertEqual(descriptor.error_msg, re_import_descriptor.error_msg)
 
     def test_fixed_xml_tag(self):
         """Make sure a tag that's been fixed exports as the original tag type"""
@@ -410,7 +446,7 @@ class ImportTestCase(BaseCourseTestCase):
         self.assertTrue(any(expect in msg or expect in err
             for msg, err in errors))
         chapters = course.get_children()
-        self.assertEqual(len(chapters), 3)
+        self.assertEqual(len(chapters), 4)
 
     def test_url_name_mangling(self):
         """

common/lib/xmodule/xmodule/tests/test_peer_grading.py

@@ -414,6 +414,6 @@ class PeerGradingModuleTrackChangesTest(unittest.TestCase, DummyModulestore):
         @return:
         """
         self.peer_grading._find_corresponding_module_for_location = self.mock_track_changes_problem
-        response = self.peer_grading.peer_grading_problem({'location': 'mocked'})
+        response = self.peer_grading.peer_grading_problem({'location': 'i4x://mock_org/mock_course/mock_cat/mock_name'})
         self.assertTrue(response['success'])
         self.assertIn("'track_changes': True", response['html'])

common/lib/xmodule/xmodule/tests/test_video.py

@@ -114,9 +114,10 @@ class VideoDescriptorTest(unittest.TestCase):
 
     def setUp(self):
         system = get_test_descriptor_system()
+        location = Location('i4x://org/course/video/name')
         self.descriptor = system.construct_xblock_from_class(
             VideoDescriptor,
-            scope_ids=ScopeIds(None, None, None, None),
+            scope_ids=ScopeIds(None, None, location, location),
             field_data=DictFieldData({}),
         )
 
@@ -226,7 +227,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
               <track src="http://www.example.com/track"/>
             </video>
         '''
-        output = VideoDescriptor.from_xml(xml_data, module_system)
+        output = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(output, {
             'youtube_id_0_75': 'izygArpw-Qo',
             'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -255,7 +256,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
               <track src="http://www.example.com/track"/>
             </video>
         '''
-        output = VideoDescriptor.from_xml(xml_data, module_system)
+        output = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(output, {
             'youtube_id_0_75': '',
             'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -276,7 +277,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
         """
         module_system = DummySystem(load_error_modules=True)
         xml_data = '<video></video>'
-        output = VideoDescriptor.from_xml(xml_data, module_system)
+        output = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(output, {
             'youtube_id_0_75': '',
             'youtube_id_1_0': 'OEoXaMPEzfM',
@@ -310,7 +311,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
                 youtube_id_1_0="&quot;OEoXaMPEzf10&quot;"
                 />
         '''
-        output = VideoDescriptor.from_xml(xml_data, module_system)
+        output = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(output, {
             'youtube_id_0_75': 'OEoXaMPEzf65',
             'youtube_id_1_0': 'OEoXaMPEzf10',
@@ -332,7 +333,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
                    youtube="1.0:&quot;p2Q6BrNhdh8&quot;,1.25:&quot;1EeWXzPdhSA&quot;">
             </video>
         '''
-        output = VideoDescriptor.from_xml(xml_data, module_system)
+        output = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(output, {
             'youtube_id_0_75': '',
             'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -362,7 +363,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
               <track src="http://www.example.com/track"/>
             </video>
         """
-        output = VideoDescriptor.from_xml(xml_data, module_system)
+        output = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(output, {
             'youtube_id_0_75': 'izygArpw-Qo',
             'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -391,7 +392,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
               <track src="http://www.example.com/track"/>
             </video>
         """
-        video = VideoDescriptor.from_xml(xml_data, module_system)
+        video = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(video, {
             'youtube_id_0_75': 'izygArpw-Qo',
             'youtube_id_1_0': 'p2Q6BrNhdh8',
@@ -420,7 +421,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
               <track src="http://www.example.com/track"/>
             </video>
         """
-        video = VideoDescriptor.from_xml(xml_data, module_system)
+        video = VideoDescriptor.from_xml(xml_data, module_system, Mock())
         self.assert_attributes_equal(video, {
             'youtube_id_0_75': 'izygArpw-Qo',
             'youtube_id_1_0': 'p2Q6BrNhdh8',

common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py

@@ -13,6 +13,8 @@ from mock import Mock
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
 
+from xmodule.modulestore import Location
+
 from xmodule.x_module import ModuleSystem, XModule, XModuleDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
 from xmodule.annotatable_module import AnnotatableDescriptor
@@ -75,7 +77,7 @@ class TestXBlockWrapper(object):
         return get_test_system()
 
     def leaf_descriptor(self, descriptor_cls):
-        location = 'i4x://org/course/category/name'
+        location = Location('i4x://org/course/category/name')
         runtime = get_test_descriptor_system()
         return runtime.construct_xblock_from_class(
             descriptor_cls,
@@ -100,7 +102,7 @@ class TestXBlockWrapper(object):
 
     def container_descriptor(self, descriptor_cls, depth):
         """Return an instance of `descriptor_cls` with `depth` levels of children"""
-        location = 'i4x://org/course/category/name'
+        location = Location('i4x://org/course/category/name')
         runtime = get_test_descriptor_system()
 
         if depth == 0:

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

@@ -8,10 +8,10 @@ from nose.tools import assert_equals, assert_not_equals, assert_true, assert_fal
 
 from xblock.field_data import DictFieldData
 from xblock.fields import Scope, String, Dict, Boolean, Integer, Float, Any, List
-from xblock.runtime import DbModel
+from xblock.runtime import KvsFieldData, DictKeyValueStore
 
 from xmodule.fields import Date, Timedelta, RelativeTime
-from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin
+from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin, InheritingFieldData
 from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field
 from xmodule.course_module import CourseDescriptor
 from xmodule.seq_module import SequenceDescriptor
@@ -58,6 +58,87 @@ class TestFields(object):
     # Used for testing Lists
     list_field = List(scope=Scope.settings, default=[])
 
+
+class InheritingFieldDataTest(unittest.TestCase):
+    """Tests of InheritingFieldData."""
+
+    class TestableInheritingXBlock(XmlDescriptor):
+        """An XBlock we can use in these tests."""
+        inherited = String(scope=Scope.settings, default="the default")
+        not_inherited = String(scope=Scope.settings, default="nothing")
+
+    def setUp(self):
+        self.system = get_test_descriptor_system()
+        self.all_blocks = {}
+        self.system.get_block = self.all_blocks.get
+        self.field_data = InheritingFieldData(
+            inheritable_names=['inherited'],
+            kvs=DictKeyValueStore({}),
+        )
+
+    def get_a_block(self, usage_id=None):
+        """Construct an XBlock for testing with."""
+        scope_ids = Mock()
+        if usage_id is None:
+            usage_id = "_auto%d" % len(self.all_blocks)
+        scope_ids.usage_id = usage_id
+        block = self.system.construct_xblock_from_class(
+            self.TestableInheritingXBlock,
+            field_data=self.field_data,
+            scope_ids=scope_ids,
+        )
+        self.all_blocks[usage_id] = block
+        return block
+
+    def test_default_value(self):
+        # Blocks with nothing set with return the fields' defaults.
+        block = self.get_a_block()
+        self.assertEqual(block.inherited, "the default")
+        self.assertEqual(block.not_inherited, "nothing")
+
+    def test_set_value(self):
+        # If you set a value, that's what you get back.
+        block = self.get_a_block()
+        block.inherited = "Changed!"
+        block.not_inherited = "New Value!"
+        self.assertEqual(block.inherited, "Changed!")
+        self.assertEqual(block.not_inherited, "New Value!")
+
+    def test_inherited(self):
+        # A child with get a value inherited from the parent.
+        parent = self.get_a_block(usage_id="parent")
+        parent.inherited = "Changed!"
+        self.assertEqual(parent.inherited, "Changed!")
+
+        child = self.get_a_block(usage_id="child")
+        child.parent = "parent"
+        self.assertEqual(child.inherited, "Changed!")
+
+    def test_inherited_across_generations(self):
+        # A child with get a value inherited from a great-grandparent.
+        parent = self.get_a_block(usage_id="parent")
+        parent.inherited = "Changed!"
+        self.assertEqual(parent.inherited, "Changed!")
+        parent_id = "parent"
+        for child_num in range(10):
+            usage_id = "child_{}".format(child_num)
+            child = self.get_a_block(usage_id=usage_id)
+            child.parent = "parent"
+            self.assertEqual(child.inherited, "Changed!")
+            parent_id = usage_id
+
+    def test_not_inherited(self):
+        # Fields not in the inherited_names list won't be inherited.
+        parent = self.get_a_block(usage_id="parent")
+        parent.not_inherited = "Changed!"
+        self.assertEqual(parent.not_inherited, "Changed!")
+
+        child = self.get_a_block(usage_id="child")
+        child.parent = "parent"
+        self.assertEqual(child.not_inherited, "nothing")
+
+
+
 class EditableMetadataFieldsTest(unittest.TestCase):
     def test_display_name_field(self):
         editable_fields = self.get_xml_editable_fields(DictFieldData({}))
@@ -100,7 +181,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
 
     def test_inherited_field(self):
         kvs = InheritanceKeyValueStore(initial_values={}, inherited_settings={'showanswer': 'inherited'})
-        model_data = DbModel(kvs)
+        model_data = KvsFieldData(kvs)
         descriptor = self.get_descriptor(model_data)
         editable_fields = descriptor.editable_metadata_fields
         self.assert_field_values(
@@ -113,7 +194,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
             initial_values={'showanswer': 'explicit'},
             inherited_settings={'showanswer': 'inheritable value'}
         )
-        model_data = DbModel(kvs)
+        model_data = KvsFieldData(kvs)
         descriptor = self.get_descriptor(model_data)
         editable_fields = descriptor.editable_metadata_fields
         self.assert_field_values(

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

@@ -4,9 +4,12 @@ Xml parsing tests for XModules
 import pprint
 from mock import Mock
 
-from xmodule.x_module import XMLParsingSystem
+from xmodule.x_module import XMLParsingSystem, policy_key
 from xmodule.mako_module import MakoDescriptorSystem
-from xmodule.modulestore.xml import create_block_from_xml
+from xmodule.modulestore.xml import create_block_from_xml, LocationReader, CourseLocationGenerator
+from xmodule.modulestore import Location
+
+from xblock.runtime import KvsFieldData, DictKeyValueStore
 
 
 class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem):  # pylint: disable=abstract-method
@@ -18,26 +21,37 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem):  # pylint: disable
         self.course = xml_import_data.course
         self.default_class = xml_import_data.default_class
         self._descriptors = {}
+
+        def get_policy(usage_id):
+            """Return the policy data for the specified usage"""
+            return xml_import_data.policy.get(policy_key(usage_id), {})
+
         super(InMemorySystem, self).__init__(
-            policy=xml_import_data.policy,
+            get_policy=get_policy,
             process_xml=self.process_xml,
             load_item=self.load_item,
             error_tracker=Mock(),
             resources_fs=xml_import_data.filesystem,
             mixins=xml_import_data.xblock_mixins,
             select=xml_import_data.xblock_select,
-            render_template=lambda template, context: pprint.pformat((template, context))
+            render_template=lambda template, context: pprint.pformat((template, context)),
+            field_data=KvsFieldData(DictKeyValueStore()),
+            id_reader=LocationReader(),
         )
 
     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, self.org, self.course, self.default_class)
+        descriptor = create_block_from_xml(
+            xml,
+            self,
+            CourseLocationGenerator(self.org, self.course),
+        )
         self._descriptors[descriptor.location.url()] = descriptor
         return descriptor
 
     def load_item(self, location):  # pylint: disable=method-hidden
         """Return the descriptor loaded for `location`"""
-        return self._descriptors[location]
+        return self._descriptors[Location(location).url()]
 
 
 class XModuleXmlImportTest(object):

common/lib/xmodule/xmodule/video_module.py

@@ -16,7 +16,6 @@ import logging
 from lxml import etree
 from pkg_resources import resource_string
 import datetime
-import time
 import copy
 
 from django.http import Http404
@@ -31,7 +30,8 @@ from xblock.fields import Scope, String, Boolean, List, Integer, ScopeIds
 from xmodule.fields import RelativeTime
 
 from xmodule.modulestore.inheritance import InheritanceKeyValueStore
-from xblock.runtime import DbModel
+from xblock.runtime import KvsFieldData
+
 log = logging.getLogger(__name__)
 
 
@@ -214,7 +214,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
             del self.data
 
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None):
+    def from_xml(cls, xml_data, system, id_generator):
         """
         Creates an instance of this descriptor from the supplied xml_data.
         This may be overridden by subclasses
@@ -227,22 +227,21 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
         """
         xml_object = etree.fromstring(xml_data)
         url_name = xml_object.get('url_name', xml_object.get('slug'))
-        location = Location(
-            'i4x', org, course, 'video', url_name
-        )
+        block_type = 'video'
+        definition_id = id_generator.create_definition(block_type, url_name)
+        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, location))
+            xml_data = etree.tostring(cls.load_file(filepath, system.resources_fs, usage_id))
         field_data = cls._parse_video_xml(xml_data)
-        field_data['location'] = location
         kvs = InheritanceKeyValueStore(initial_values=field_data)
-        field_data = DbModel(kvs)
+        field_data = KvsFieldData(kvs)
         video = system.construct_xblock_from_class(
             cls,
             # We're loading a descriptor, so student_id is meaningless
             # We also don't have separate notions of definition and usage ids yet,
             # so we use the location for both
-            ScopeIds(None, location.category, location, location),
+            ScopeIds(None, block_type, definition_id, usage_id),
             field_data,
         )
         return video

common/lib/xmodule/xmodule/x_module.py

@@ -10,18 +10,19 @@ from pkg_resources import resource_listdir, resource_string, resource_isdir
 from webob import Response
 from webob.multidict import MultiDict
 
-from xmodule.modulestore import Location
-from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError
-
 from xblock.core import XBlock
 from xblock.fields import Scope, Integer, Float, List, XBlockMixin, String
 from xblock.fragment import Fragment
 from xblock.plugin import default_select
-from xblock.runtime import Runtime
+from xblock.runtime import Runtime, MemoryIdManager
 from xmodule.fields import RelativeTime
+
 from xmodule.errortracker import exc_info_to_str
+from xmodule.modulestore import Location
+from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError
 from xmodule.modulestore.locator import BlockUsageLocator
 
+
 log = logging.getLogger(__name__)
 
 
@@ -650,28 +651,30 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
 
     # ================================= XML PARSING ============================
     @classmethod
-    def parse_xml(cls, node, runtime, keys):
+    def parse_xml(cls, node, runtime, keys, id_generator):
         """
         Interpret the parsed XML in `node`, creating an XModuleDescriptor.
         """
         xml = etree.tostring(node)
         # TODO: change from_xml to not take org and course, it can use self.system.
-        block = cls.from_xml(xml, runtime, runtime.org, runtime.course)
+        block = cls.from_xml(xml, runtime, id_generator)
         return block
 
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None):
+    def from_xml(cls, xml_data, system, id_generator):
         """
         Creates an instance of this descriptor from the supplied xml_data.
         This may be overridden by subclasses.
 
-        xml_data: A string of xml that will be translated into data and children
+        Args:
+            xml_data (str): A string of xml that will be translated into data and children
                 for this module
 
-        system is an XMLParsingSystem
+            system (:class:`.XMLParsingSystem):
+
+            id_generator (:class:`xblock.runtime.IdGenerator`): Used to generate the
+                usage_ids and definition_ids when loading this xml
 
-        org and course are optional strings that will be used in the generated
-            module's url identifiers
         """
         raise NotImplementedError('Modules must implement from_xml to be parsable from xml')
 
@@ -872,7 +875,9 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable
     Base class for :class:`Runtime`s to be used with :class:`XModuleDescriptor`s
     """
 
-    def __init__(self, load_item, resources_fs, error_tracker, **kwargs):
+    def __init__(
+        self, load_item, resources_fs, error_tracker, get_policy=None, **kwargs
+    ):
         """
         load_item: Takes a Location and returns an XModuleDescriptor
 
@@ -907,15 +912,20 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable
 
                NOTE: To avoid duplication, do not call the tracker on errors
                that you're about to re-raise---let the caller track them.
-        """
 
-        # Right now, usage_store is unused, and field_data is always supplanted
-        # with an explicit field_data during construct_xblock, so None's suffice.
-        super(DescriptorSystem, self).__init__(usage_store=None, field_data=None, **kwargs)
+        get_policy: a function that takes a usage id and returns a dict of
+            policy to apply.
+
+        """
+        super(DescriptorSystem, self).__init__(**kwargs)
 
         self.load_item = load_item
         self.resources_fs = resources_fs
         self.error_tracker = error_tracker
+        if get_policy:
+            self.get_policy = get_policy
+        else:
+            self.get_policy = lambda u: {}
 
     def get_block(self, usage_id):
         """See documentation for `xblock.runtime:Runtime.get_block`"""
@@ -978,17 +988,14 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable
 
 
 class XMLParsingSystem(DescriptorSystem):
-    def __init__(self, process_xml, policy, **kwargs):
+    def __init__(self, process_xml, **kwargs):
         """
-        policy: a policy dictionary for overriding xml metadata
-
         process_xml: Takes an xml string, and returns a XModuleDescriptor
             created from that xml
         """
 
         super(XMLParsingSystem, self).__init__(**kwargs)
         self.process_xml = process_xml
-        self.policy = policy
 
 
 class ModuleSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abstract-method
@@ -1010,7 +1017,9 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abs
             anonymous_student_id='', course_id=None,
             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, error_descriptor_class=None, get_real_user=None, **kwargs):
+            replace_jump_to_id_urls=None, error_descriptor_class=None, get_real_user=None,
+            field_data=None,
+            **kwargs):
         """
         Create a closure around the system environment.
 
@@ -1062,11 +1071,12 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abs
         get_real_user - function that takes `anonymous_student_id` and returns real user_id,
         associated with `anonymous_student_id`.
 
+        field_data - the `FieldData` to use for backing XBlock storage.
         """
 
-        # Right now, usage_store is unused, and field_data is always supplanted
-        # with an explicit field_data during construct_xblock, so None's suffice.
-        super(ModuleSystem, self).__init__(usage_store=None, field_data=None, **kwargs)
+        # Usage_store is unused, and field_data is often supplanted with an
+        # explicit field_data during construct_xblock.
+        super(ModuleSystem, self).__init__(id_reader=None, field_data=field_data, **kwargs)
 
         self.STATIC_URL = static_url
         self.xqueue = xqueue

common/lib/xmodule/xmodule/xml_module.py

@@ -6,11 +6,11 @@ import sys
 from lxml import etree
 
 from xblock.fields import Dict, Scope, ScopeIds
-from xmodule.x_module import (XModuleDescriptor, policy_key)
+from xmodule.x_module import XModuleDescriptor, policy_key
 from xmodule.modulestore import Location
 from xmodule.modulestore.inheritance import own_metadata, InheritanceKeyValueStore
 from xmodule.modulestore.xml_exporter import EdxJSONEncoder
-from xblock.runtime import DbModel
+from xblock.runtime import KvsFieldData
 
 log = logging.getLogger(__name__)
 
@@ -153,8 +153,7 @@ class XmlDescriptor(XModuleDescriptor):
 
         xml_object: An etree Element
         """
-        raise NotImplementedError(
-            "%s does not implement definition_from_xml" % cls.__name__)
+        raise NotImplementedError("%s does not implement definition_from_xml" % cls.__name__)
 
     @classmethod
     def clean_metadata_from_xml(cls, xml_object):
@@ -177,7 +176,7 @@ class XmlDescriptor(XModuleDescriptor):
         return etree.parse(file_object, parser=edx_xml_parser).getroot()
 
     @classmethod
-    def load_file(cls, filepath, fs, location):
+    def load_file(cls, filepath, fs, def_id):
         '''
         Open the specified file in fs, and call cls.file_to_xml on it,
         returning the lxml object.
@@ -190,11 +189,11 @@ class XmlDescriptor(XModuleDescriptor):
         except Exception as err:
             # Add info about where we are, but keep the traceback
             msg = 'Unable to load file contents at path %s for item %s: %s ' % (
-                filepath, location.url(), str(err))
+                filepath, def_id, err)
             raise Exception, msg, sys.exc_info()[2]
 
     @classmethod
-    def load_definition(cls, xml_object, system, location):
+    def load_definition(cls, xml_object, system, def_id):
         '''Load a descriptor definition from the specified xml_object.
         Subclasses should not need to override this except in special
         cases (e.g. html module)'''
@@ -220,7 +219,7 @@ class XmlDescriptor(XModuleDescriptor):
                         filepath = candidate
                         break
 
-            definition_xml = cls.load_file(filepath, system.resources_fs, location)
+            definition_xml = cls.load_file(filepath, system.resources_fs, def_id)
 
         definition_metadata = get_metadata_from_xml(definition_xml)
         cls.clean_metadata_from_xml(definition_xml)
@@ -269,7 +268,7 @@ class XmlDescriptor(XModuleDescriptor):
                 metadata[attr] = value
 
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None):
+    def from_xml(cls, xml_data, system, id_generator):
         """
         Creates an instance of this descriptor from the supplied xml_data.
         This may be overridden by subclasses
@@ -277,26 +276,25 @@ class XmlDescriptor(XModuleDescriptor):
         xml_data: A string of xml that will be translated into data and children for
             this module
         system: A DescriptorSystem for interacting with external resources
-        org and course are optional strings that will be used in the generated modules
-            url identifiers
         """
 
         xml_object = etree.fromstring(xml_data)
         # VS[compat] -- just have the url_name lookup, once translation is done
         url_name = xml_object.get('url_name', xml_object.get('slug'))
-        location = Location('i4x', org, course, xml_object.tag, url_name)
+        def_id = id_generator.create_definition(xml_object.tag, url_name)
+        usage_id = id_generator.create_usage(def_id)
 
         # VS[compat] -- detect new-style each-in-a-file mode
         if is_pointer_tag(xml_object):
             # new style:
             # read the actual definition file--named using url_name.replace(':','/')
             filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name))
-            definition_xml = cls.load_file(filepath, system.resources_fs, location)
+            definition_xml = cls.load_file(filepath, system.resources_fs, def_id)
         else:
             definition_xml = xml_object
             filepath = None
 
-        definition, children = cls.load_definition(definition_xml, system, location)  # note this removes metadata
+        definition, children = cls.load_definition(definition_xml, system, def_id)  # note this removes metadata
 
         # VS[compat] -- make Ike's github preview links work in both old and
         # new file layouts
@@ -313,13 +311,11 @@ class XmlDescriptor(XModuleDescriptor):
             try:
                 metadata.update(json.loads(dmdata))
             except Exception as err:
-                log.debug('Error %s in loading metadata %s' % (err, dmdata))
+                log.debug('Error in loading metadata %r', dmdata, exc_info=True)
                 metadata['definition_metadata_err'] = str(err)
 
         # Set/override any metadata specified by policy
-        k = policy_key(location)
-        if k in system.policy:
-            cls.apply_policy(metadata, system.policy[k])
+        cls.apply_policy(metadata, system.get_policy(usage_id))
 
         field_data = {}
         field_data.update(metadata)
@@ -327,17 +323,13 @@ class XmlDescriptor(XModuleDescriptor):
         field_data['children'] = children
 
         field_data['xml_attributes']['filename'] = definition.get('filename', ['', None])  # for git link
-        field_data['location'] = location
-        field_data['category'] = xml_object.tag
         kvs = InheritanceKeyValueStore(initial_values=field_data)
-        field_data = DbModel(kvs)
+        field_data = KvsFieldData(kvs)
 
         return system.construct_xblock_from_class(
             cls,
             # We're loading a descriptor, so student_id is meaningless
-            # We also don't have separate notions of definition and usage ids yet,
-            # so we use the location for both
-            ScopeIds(None, location.category, location, location),
+            ScopeIds(None, xml_object.tag, def_id, usage_id),
             field_data,
         )
 

common/test/data/simple/course.xml

@@ -27,5 +27,4 @@
       </vertical>
     </sequential>
   </chapter>
-
 </course>

lms/djangoapps/courseware/management/commands/dump_course_structure.py

@@ -95,7 +95,7 @@ def dump_module(module, destination=None, inherited=False, defaults=False):
 
     destination[module.location.url()] = {
         'category': module.location.category,
-        'children': module.children if hasattr(module, 'children') else [],
+        'children': [str(child) for child in getattr(module, 'children', [])],
         'metadata': filtered_metadata,
     }
 
@@ -110,7 +110,7 @@ def dump_module(module, destination=None, inherited=False, defaults=False):
                 return False
             elif field.scope != Scope.settings:
                 return False
-            elif defaults == True:
+            elif defaults:
                 return True
             else:
                 return field.values != field.default

lms/djangoapps/courseware/model_data.py

@@ -317,7 +317,7 @@ class DjangoKeyValueStore(KeyValueStore):
         Provide a bulk save mechanism.
 
         `kv_dict`: A dictionary of dirty fields that maps
-          xblock.DbModel._key : value
+          xblock.KvsFieldData._key : value
 
         """
         saved_fields = []

lms/djangoapps/courseware/module_render.py

@@ -29,7 +29,7 @@ from util.json_request import JsonResponse
 from util.sandboxing import can_execute_unsafe_code
 from xblock.core import XBlock
 from xblock.fields import Scope
-from xblock.runtime import DbModel, KeyValueStore
+from xblock.runtime import KvsFieldData, KeyValueStore
 from xblock.exceptions import NoSuchHandlerError
 from xblock.django.request import django_to_webob_request, webob_to_django_response
 from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor
@@ -222,7 +222,7 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours
     if not has_access(user, descriptor, 'load', course_id):
         return None
 
-    student_data = DbModel(DjangoKeyValueStore(field_data_cache))
+    student_data = KvsFieldData(DjangoKeyValueStore(field_data_cache))
     descriptor._field_data = LmsFieldData(descriptor._field_data, student_data)
 
 

requirements/edx/base.txt

@@ -93,7 +93,6 @@ transifex-client==0.9.1
 
 # Used for testing
 coverage==3.7
-ddt==0.4.0
 factory_boy==2.2.1
 mock==1.0.1
 nosexcover==1.0.7

requirements/edx/github.txt

@@ -15,10 +15,13 @@
 -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk
 
 # Our libraries:
--e git+https://github.com/edx/XBlock.git@cd77808aadd3ea1c2027ca8c0aa5624d8ccccc52#egg=XBlock
+-e git+https://github.com/edx/XBlock.git@a1a3e76b269d15b7bbd11976d8aef63e1db6c4c2#egg=XBlock
 -e git+https://github.com/edx/codejail.git@e3d98f9455#egg=codejail
 -e git+https://github.com/edx/diff-cover.git@v0.2.6#egg=diff_cover
 -e git+https://github.com/edx/js-test-tool.git@v0.1.5#egg=js_test_tool
 -e git+https://github.com/edx/django-waffle.git@823a102e48#egg=django-waffle
 -e git+https://github.com/edx/event-tracking.git@f0211d702d#egg=event-tracking
 -e git+https://github.com/edx/bok-choy.git@bc6f1adbe439618162079f1004b2b3db3b6f8916#egg=bok_choy
+
+# Move back to upstream release once https://github.com/txels/ddt/pull/13 is merged
+-e git+https://github.com/edx/ddt.git@9e8010b8777aa40b848fdb76de6e60081616325a#egg=ddt
\ No newline at end of file