XBlock Xml Serialization/Deserialization

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -1501,7 +1501,7 @@ def test_create_course_with_bad_organization(self):
         """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.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 @@ def definition_from_xml(cls, xml_object, system):
             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 from_xml(cls, xml_data, system, org=None, course=None):
             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 @@ def from_xml(cls, xml_data, system, org=None, course=None):
 
 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 @@
 
 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 @@ def parse_sources(xml_element, system, return_descriptor=False):
             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 @@ def definition_from_xml(cls, xml_object, system):
         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 @@ def read_grading_policy(cls, paths, system):
         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 @@ def definition_from_xml(cls, xml_object, system):
         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 @@ def get_html(self):
 
     @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 @@ def from_descriptor(cls, descriptor, error_msg='Error not available'):
         )
 
     @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 @@ def from_xml(cls, xml_data, system, org=None, course=None,
             # 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 @@
 _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 @@ def __new__(_cls, loc_or_tag=None, org=None, course=None, category=None,
         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 @@ def check_dict(dict_):
             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 @@ def course_id(self):
 
         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

@@ -1,26 +1,33 @@
+"""
+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):