Use XBlock xml serialization and deserialization

XModules continue to use their own interface for xml, but provide an
adaptor that makes the the XBlock interface available.

[LMS-179]

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."""

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)
@@ -223,12 +224,11 @@ class ConditionalDescriptor(ConditionalFields, SequenceDescriptor):
             if child.tag == 'show':
                 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):
+        location = Location(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

@@ -177,7 +177,7 @@ class Location(_LocationBase):
         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)

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

@@ -33,6 +33,7 @@ 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
@@ -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

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 KvsFieldData
+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

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
@@ -191,8 +199,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 descriptor = ErrorDescriptor.from_xml(
                     xml,
                     self,
-                    self.org,
-                    self.course,
+                    id_generator,
                     err_msg
                 )
 
@@ -210,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
+
+    def get_block_type(self, def_id):
+        location = def_id
+        return location.category
 
-    `xml_data' is a string containing valid xml.
 
-    `system` is an XMLParsingSystem.
+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
+
 
-    `org` and `course` are optional strings that will be used in the generated
-    block's url identifiers.
+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
 
 
@@ -270,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)
 
@@ -281,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):
@@ -293,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])
 
 
@@ -331,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
@@ -457,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,
@@ -464,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'))

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

@@ -393,10 +393,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)
 

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()
 
@@ -407,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

@@ -70,13 +70,13 @@ class InheritingFieldDataTest(unittest.TestCase):
     def setUp(self):
         self.system = get_test_descriptor_system()
         self.all_blocks = {}
-        self.system.load_item = self.all_blocks.get
+        self.system.get_block = self.all_blocks.get
         self.field_data = InheritingFieldData(
             inheritable_names=['inherited'],
             kvs=DictKeyValueStore({}),
         )
 
-    def get_block(self, usage_id=None):
+    def get_a_block(self, usage_id=None):
         """Construct an XBlock for testing with."""
         scope_ids = Mock()
         if usage_id is None:
@@ -92,13 +92,13 @@ class InheritingFieldDataTest(unittest.TestCase):
 
     def test_default_value(self):
         # Blocks with nothing set with return the fields' defaults.
-        block = self.get_block()
+        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_block()
+        block = self.get_a_block()
         block.inherited = "Changed!"
         block.not_inherited = "New Value!"
         self.assertEqual(block.inherited, "Changed!")
@@ -106,34 +106,34 @@ class InheritingFieldDataTest(unittest.TestCase):
 
     def test_inherited(self):
         # A child with get a value inherited from the parent.
-        parent = self.get_block(usage_id="parent")
+        parent = self.get_a_block(usage_id="parent")
         parent.inherited = "Changed!"
         self.assertEqual(parent.inherited, "Changed!")
 
-        child = self.get_block(usage_id="child")
+        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_block(usage_id="parent")
+        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_block(usage_id=usage_id)
+            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_block(usage_id="parent")
+        parent = self.get_a_block(usage_id="parent")
         parent.not_inherited = "Changed!"
         self.assertEqual(parent.not_inherited, "Changed!")
 
-        child = self.get_block(usage_id="child")
+        child = self.get_a_block(usage_id="child")
         child.parent = "parent"
         self.assertEqual(child.not_inherited, "nothing")
 

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

@@ -30,7 +30,7 @@ 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,14 +227,13 @@ 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 = KvsFieldData(kvs)
         video = system.construct_xblock_from_class(
@@ -242,7 +241,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
             # 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
-            for this module
+        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

@@ -176,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.
@@ -189,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)'''
@@ -219,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)
@@ -268,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
@@ -276,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
@@ -312,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)
@@ -326,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 = 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,
         )
 

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,
     }
 

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 KvsFieldData
+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

requirements/edx/github.txt

@@ -15,7 +15,7 @@
 -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