Add InheritanceKVS and standardize inherited attr patterns

cms/djangoapps/contentstore/tests/test_crud.py

@@ -218,7 +218,7 @@ class TemplateTests(unittest.TestCase):
         )
         usage_id = json_data.get('_id', None)
         if not '_inherited_settings' in json_data and parent_xblock is not None:
-            json_data['_inherited_settings'] = parent_xblock.xblock_kvs.get_inherited_settings().copy()
+            json_data['_inherited_settings'] = parent_xblock.xblock_kvs.inherited_settings.copy()
             json_fields = json_data.get('fields', {})
             for field_name in inheritance.InheritanceMixin.fields:
                 if field_name in json_fields:

cms/static/coffee/spec/views/metadata_edit_spec.coffee

@@ -26,7 +26,6 @@ describe "Test Metadata Editor", ->
         explicitly_set: true,
         field_name: "display_name",
         help: "Specifies the name for this component.",
-        inheritable: false,
         options: [],
         type: CMS.Models.Metadata.GENERIC_TYPE,
         value: "Word cloud"
@@ -38,7 +37,6 @@ describe "Test Metadata Editor", ->
         explicitly_set: false,
         field_name: "show_answer",
         help: "When should you show the answer",
-        inheritable: true,
         options: [
             {"display_name": "Always", "value": "always"},
             {"display_name": "Answered", "value": "answered"},
@@ -54,7 +52,6 @@ describe "Test Metadata Editor", ->
         explicitly_set: false,
         field_name: "num_inputs",
         help: "Number of text boxes for student to input words/sentences.",
-        inheritable: false,
         options: {min: 1},
         type: CMS.Models.Metadata.INTEGER_TYPE,
         value: 5
@@ -66,7 +63,6 @@ describe "Test Metadata Editor", ->
         explicitly_set: true,
         field_name: "weight",
         help: "Weight for this problem",
-        inheritable: true,
         options: {min: 1.3, max:100.2, step:0.1},
         type: CMS.Models.Metadata.FLOAT_TYPE,
         value: 10.2
@@ -78,7 +74,6 @@ describe "Test Metadata Editor", ->
         explicitly_set: false,
         field_name: "list",
         help: "A list of things.",
-        inheritable: false,
         options: [],
         type: CMS.Models.Metadata.LIST_TYPE,
         value: ["the first display value", "the second"]
@@ -99,7 +94,6 @@ describe "Test Metadata Editor", ->
                         explicitly_set: true,
                         field_name: "unknown_type",
                         help: "Mystery property.",
-                        inheritable: false,
                         options: [
                             {"display_name": "Always", "value": "always"},
                             {"display_name": "Answered", "value": "answered"},
@@ -145,7 +139,6 @@ describe "Test Metadata Editor", ->
                     explicitly_set: false,
                     field_name: "display_name",
                     help: "",
-                    inheritable: false,
                     options: [],
                     type: CMS.Models.Metadata.GENERIC_TYPE,
                     value: null

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

@@ -3,6 +3,7 @@ from pytz import UTC
 
 from xblock.fields import Scope, Boolean, String, Float, XBlockMixin
 from xmodule.fields import Date, Timedelta
+from xblock.runtime import KeyValueStore
 
 
 class InheritanceMixin(XBlockMixin):
@@ -51,16 +52,18 @@ def compute_inherited_metadata(descriptor):
 
     NOTE: This means that there is no such thing as lazy loading at the
     moment--this accesses all the children."""
-    for child in descriptor.get_children():
-        inherit_metadata(
-            child,
-            {
-                name: field.read_from(descriptor)
-                for name, field in InheritanceMixin.fields.items()
-                if field.is_set_on(descriptor)
-            }
-        )
-        compute_inherited_metadata(child)
+    if descriptor.has_children:
+        parent_metadata = descriptor.xblock_kvs.inherited_settings.copy()
+        # add any of descriptor's explicitly set fields to the inheriting list
+        for field in InheritanceMixin.fields.values():
+            # pylint: disable = W0212
+            if descriptor._field_data.has(descriptor, field.name):
+                # inherited_settings values are json repr
+                parent_metadata[field.name] = field.read_json(descriptor)
+
+        for child in descriptor.get_children():
+            inherit_metadata(child, parent_metadata)
+            compute_inherited_metadata(child)
 
 
 def inherit_metadata(descriptor, inherited_data):
@@ -72,53 +75,46 @@ def inherit_metadata(descriptor, inherited_data):
     `inherited_data`: A dictionary mapping field names to the values that
         they should inherit
     """
-    # The inherited values that are actually being used.
-    if not hasattr(descriptor, '_inherited_metadata'):
-        setattr(descriptor, '_inherited_metadata', {})
-
-    # All inheritable metadata values (for which a value exists in field_data).
-    if not hasattr(descriptor, '_inheritable_metadata'):
-        setattr(descriptor, '_inheritable_metadata', {})
-
-    # Set all inheritable metadata from kwargs that are
-    # in self.inheritable_metadata and aren't already set in metadata
-    for name, field in InheritanceMixin.fields.items():
-        if name not in inherited_data:
-            continue
-        inherited_value = inherited_data[name]
-
-        descriptor._inheritable_metadata[name] = inherited_value
-        if not field.is_set_on(descriptor):
-            descriptor._inherited_metadata[name] = inherited_value
-            field.write_to(descriptor, inherited_value)
-            # We've updated the fields on the descriptor, so we need to save it
-            descriptor.save()
+    try:
+        descriptor.xblock_kvs.inherited_settings = inherited_data
+    except AttributeError:  # the kvs doesn't have inherited_settings probably b/c it's an error module
+        pass
 
 
 def own_metadata(module):
-    # IN SPLIT MONGO this is just ['metadata'] as it keeps ['_inherited_metadata'] separate!
-    # FIXME move into kvs? will that work for xml mongo?
     """
     Return a dictionary that contains only non-inherited field keys,
     mapped to their serialized values
     """
-    inherited_metadata = getattr(module, '_inherited_metadata', {})
-    metadata = {}
-    for name, field in module.fields.items():
-        # Only save metadata that wasn't inherited
-        if field.scope != Scope.settings:
-            continue
-
-        if not field.is_set_on(module):
-            continue
-
-        if name in inherited_metadata and field.read_from(module) == inherited_metadata.get(name):
-            continue
-
-        try:
-            metadata[name] = field.read_json(module)
-        except KeyError:
-            # Ignore any missing keys in _field_data
-            pass
-
-    return metadata
+    return module.get_explicitly_set_fields_by_scope(Scope.settings)
+
+class InheritanceKeyValueStore(KeyValueStore):
+    """
+    Common superclass for kvs's which know about inheritance of settings. Offers simple
+    dict-based storage of fields and lookup of inherited values.
+
+    Note: inherited_settings is a dict of key to json values (internal xblock field repr)
+    """
+    def __init__(self, initial_values=None, inherited_settings=None):
+        super(InheritanceKeyValueStore, self).__init__()
+        self.inherited_settings = inherited_settings or {}
+        self._fields = initial_values or {}
+
+    def get(self, key):
+        return self._fields[key.field_name]
+
+    def set(self, key, value):
+        # xml backed courses are read-only, but they do have some computed fields
+        self._fields[key.field_name] = value
+
+    def delete(self, key):
+        del self._fields[key.field_name]
+
+    def has(self, key):
+        return key.field_name in self._fields
+
+    def default(self, key):
+        """
+        Check to see if the default should be from inheritance rather than from the field's global default
+        """
+        return self.inherited_settings[key.field_name]

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

@@ -17,25 +17,23 @@ import sys
 import logging
 import copy
 
-from collections import namedtuple
 from fs.osfs import OSFS
 from itertools import repeat
 from path import path
 from operator import attrgetter
-from uuid import uuid4
 
 from importlib import import_module
 from xmodule.errortracker import null_error_tracker, exc_info_to_str
 from xmodule.mako_module import MakoDescriptorSystem
 from xmodule.x_module import XModuleDescriptor
 from xmodule.error_module import ErrorDescriptor
-from xblock.runtime import DbModel, KeyValueStore
+from xblock.runtime import DbModel
 from xblock.exceptions import InvalidScopeError
 from xblock.fields import Scope, ScopeIds
 
 from xmodule.modulestore import ModuleStoreBase, Location, namedtuple_to_son, MONGO_MODULESTORE_TYPE
 from xmodule.modulestore.exceptions import ItemNotFoundError
-from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata
+from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore
 
 log = logging.getLogger(__name__)
 
@@ -58,12 +56,13 @@ class InvalidWriteError(Exception):
     """
 
 
-class MongoKeyValueStore(KeyValueStore):
+class MongoKeyValueStore(InheritanceKeyValueStore):
     """
     A KeyValueStore that maps keyed data access to one of the 3 data areas
     known to the MongoModuleStore (data, children, and metadata)
     """
     def __init__(self, data, children, metadata):
+        super(MongoKeyValueStore, self).__init__()
         self._data = data
         self._children = children
         self._metadata = metadata
@@ -201,10 +200,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
 
                     # Convert the serialized fields values in self.cached_metadata
                     # to python values
-                    metadata_to_inherit = {
-                        key: module.fields[key].from_json(value)
-                        for key, value in self.cached_metadata.get(non_draft_loc.url(), {}).items()
-                    }
+                    metadata_to_inherit = self.cached_metadata.get(non_draft_loc.url(), {})
                     inherit_metadata(module, metadata_to_inherit)
                 # decache any computed pending field settings
                 module.save()

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

@@ -4,37 +4,31 @@ from collections import namedtuple
 from xblock.runtime import KeyValueStore
 from xblock.exceptions import InvalidScopeError
 from .definition_lazy_loader import DefinitionLazyLoader
+from xmodule.modulestore.inheritance import InheritanceKeyValueStore
 
 # id is a BlockUsageLocator, def_id is the definition's guid
 SplitMongoKVSid = namedtuple('SplitMongoKVSid', 'id, def_id')
 
 
-PROVENANCE_LOCAL = 'local'
-PROVENANCE_DEFAULT = 'default'
-PROVENANCE_INHERITED = 'inherited'
-
-class SplitMongoKVS(KeyValueStore):
+class SplitMongoKVS(InheritanceKeyValueStore):
     """
     A KeyValueStore that maps keyed data access to one of the 3 data areas
     known to the MongoModuleStore (data, children, and metadata)
     """
 
-    def __init__(self, definition, fields, _inherited_settings):
+    def __init__(self, definition, fields, inherited_settings):
         """
 
         :param definition: either a lazyloader or definition id for the definition
         :param fields: a dictionary of the locally set fields
-        :param _inherited_settings: the value of each inheritable field from above this.
+        :param inherited_settings: the json value of each inheritable field from above this.
             Note, local fields may override and disagree w/ this b/c this says what the value
             should be if the field is undefined.
         """
-        # ensure kvs's don't share objects w/ others so that changes can't appear in separate ones
-        # the particular use case was that changes to kvs's were polluting caches. My thinking was
-        # that kvs's should be independent thus responsible for the isolation.
+        super(SplitMongoKVS, self).__init__(copy.copy(fields), inherited_settings)
         self._definition = definition  # either a DefinitionLazyLoader or the db id of the definition.
         # if the db id, then the definition is presumed to be loaded into _fields
-        self._fields = copy.copy(fields)
-        self._inherited_settings = _inherited_settings
+
 
     def get(self, key):
         # simplest case, field is directly set
@@ -49,12 +43,8 @@ class SplitMongoKVS(KeyValueStore):
             # didn't find children in _fields; so, see if there's a default
             raise KeyError()
         elif key.scope == Scope.settings:
-            # didn't find in _fields; so, get from inheritance since not locally set
-            if key.field_name in self._inherited_settings:
-                return self._inherited_settings[key.field_name]
-            else:
-                # or get default
-                raise KeyError()
+            # get default which may be the inherited value
+            raise KeyError()
         elif key.scope == Scope.content:
             if isinstance(self._definition, DefinitionLazyLoader):
                 self._load_definition()
@@ -113,40 +103,6 @@ class SplitMongoKVS(KeyValueStore):
         # if someone changes it so that they do, then change any tests of field.name in xx._field_data
         return key.field_name in self._fields
 
-    # would like to just take a key, but there's a bunch of magic in DbModel for constructing the key via
-    # a private method
-    def field_value_provenance(self, key_scope, key_name):
-        """
-        Where the field value comes from: one of [PROVENANCE_LOCAL, PROVENANCE_DEFAULT, PROVENANCE_INHERITED].
-        """
-        # handle any special cases
-        if key_scope == Scope.content:
-            if key_name == 'location':
-                return PROVENANCE_LOCAL
-            elif key_name == 'category':
-                return PROVENANCE_LOCAL
-            else:
-                self._load_definition()
-                if key_name in self._fields:
-                    return PROVENANCE_LOCAL
-                else:
-                    return PROVENANCE_DEFAULT
-        elif key_scope == Scope.parent:
-            return PROVENANCE_DEFAULT
-        # catch the locally set state
-        elif key_name in self._fields:
-            return PROVENANCE_LOCAL
-        elif key_scope == Scope.settings and key_name in self._inherited_settings:
-            return PROVENANCE_INHERITED
-        else:
-            return PROVENANCE_DEFAULT
-
-    def get_inherited_settings(self):
-        """
-        Get the settings set by the ancestors (which locally set fields may override or not)
-        """
-        return self._inherited_settings
-
     def _load_definition(self):
         """
         Update fields w/ the lazily loaded definitions

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

@@ -19,7 +19,9 @@ from path import path
 
 import calc
 from xblock.field_data import DictFieldData
-from xmodule.x_module import ModuleSystem, XModuleDescriptor
+from xmodule.x_module import ModuleSystem, XModuleDescriptor, DescriptorSystem
+from xmodule.modulestore.inheritance import InheritanceMixin
+from xmodule.mako_module import MakoDescriptorSystem
 
 
 # Location of common test DATA directory
@@ -64,7 +66,20 @@ def get_test_system():
         node_path=os.environ.get("NODE_PATH", "/usr/local/lib/node_modules"),
         xblock_field_data=lambda descriptor: descriptor._field_data,
         anonymous_student_id='student',
-        open_ended_grading_interface= open_ended_grading_interface
+        open_ended_grading_interface=open_ended_grading_interface
+    )
+
+
+def get_test_descriptor_system():
+    """
+    Construct a test DescriptorSystem instance.
+    """
+    return MakoDescriptorSystem(
+        load_item=Mock(),
+        resources_fs=Mock(),
+        error_tracker=Mock(),
+        render_template=lambda template, context: repr(context),
+        mixins=(InheritanceMixin,),
     )
 
 

common/lib/xmodule/xmodule/tests/test_editing_module.py

@@ -9,7 +9,7 @@ from xmodule.editing_module import TabsEditingDescriptor
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
 
-from .import get_test_system
+from xmodule.tests import get_test_descriptor_system
 
 log = logging.getLogger(__name__)
 
@@ -19,7 +19,7 @@ class TabsEditingDescriptorTestCase(unittest.TestCase):
 
     def setUp(self):
         super(TabsEditingDescriptorTestCase, self).setUp()
-        system = get_test_system()
+        system = get_test_descriptor_system()
         system.render_template = Mock(return_value="<div>Test Template HTML</div>")
         self.tabs = [
             {
@@ -44,8 +44,8 @@ class TabsEditingDescriptorTestCase(unittest.TestCase):
         ]
 
         TabsEditingDescriptor.tabs = self.tabs
-        self.descriptor = TabsEditingDescriptor(
-            runtime=system,
+        self.descriptor = system.construct_xblock_from_class(
+            TabsEditingDescriptor,
             field_data=DictFieldData({}),
             scope_ids=ScopeIds(None, None, None, None),
         )

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

@@ -158,11 +158,10 @@ class ImportTestCase(BaseCourseTestCase):
         # Check that the child inherits due correctly
         child = descriptor.get_children()[0]
         self.assertEqual(child.due, ImportTestCase.date.from_json(v))
-        self.assertEqual(child._inheritable_metadata, child._inherited_metadata)
-        self.assertEqual(1, len(child._inherited_metadata))
+        # need to convert v to canonical json b4 comparing
         self.assertEqual(
-            datetime.datetime(2013, 3, 20, 17, 0, tzinfo=UTC()),
-            child._inherited_metadata['due']
+            ImportTestCase.date.to_json(ImportTestCase.date.from_json(v)),
+            child.xblock_kvs.inherited_settings['due']
         )
 
         # Now export and check things
@@ -218,8 +217,6 @@ class ImportTestCase(BaseCourseTestCase):
         # Check that the child does not inherit a value for due
         child = descriptor.get_children()[0]
         self.assertEqual(child.due, None)
-        # pylint: disable=W0212
-        self.assertEqual(child._inheritable_metadata, child._inherited_metadata)
         self.assertLessEqual(
             child.start,
             datetime.datetime.now(UTC())
@@ -249,10 +246,9 @@ class ImportTestCase(BaseCourseTestCase):
         self.assertEqual(descriptor.due, ImportTestCase.date.from_json(course_due))
         self.assertEqual(child.due, ImportTestCase.date.from_json(child_due))
         # Test inherited metadata. Due does not appear here (because explicitly set on child).
-        self.assertEqual(1, len(child._inheritable_metadata))
         self.assertEqual(
-            datetime.datetime(2013, 3, 20, 17, 0, tzinfo=UTC()),
-            child._inheritable_metadata['due']
+            ImportTestCase.date.to_json(ImportTestCase.date.from_json(course_due)),
+            child.xblock_kvs.inherited_settings['due']
         )
 
     def test_is_pointer_tag(self):
@@ -288,14 +284,14 @@ class ImportTestCase(BaseCourseTestCase):
         print("Starting import")
         course = self.get_course('toy')
 
-        def check_for_key(key, node):
+        def check_for_key(key, node, value):
             "recursive check for presence of key"
             print("Checking {0}".format(node.location.url()))
-            self.assertTrue(node._field_data.has(node, key))
+            self.assertEqual(getattr(node, key), value)
             for c in node.get_children():
-                check_for_key(key, c)
+                check_for_key(key, c, value)
 
-        check_for_key('graceperiod', course)
+        check_for_key('graceperiod', course, course.graceperiod)
 
     def test_policy_loading(self):
         """Make sure that when two courses share content with the same

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

@@ -18,7 +18,6 @@ from mock import Mock
 
 from . import LogicTest
 from lxml import etree
-from .import get_test_system
 from xmodule.modulestore import Location
 from xmodule.video_module import VideoDescriptor, _create_youtube_string
 from .test_import import DummySystem
@@ -26,6 +25,7 @@ from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
 
 from textwrap import dedent
+from xmodule.tests import get_test_descriptor_system
 
 
 class VideoModuleTest(LogicTest):
@@ -124,9 +124,9 @@ class VideoDescriptorTest(unittest.TestCase):
     """Test for VideoDescriptor"""
 
     def setUp(self):
-        system = get_test_system()
-        self.descriptor = VideoDescriptor(
-            runtime=system,
+        system = get_test_descriptor_system()
+        self.descriptor = system.construct_xblock_from_class(
+            VideoDescriptor,
             field_data=DictFieldData({}),
             scope_ids=ScopeIds(None, None, None, None),
         )
@@ -304,7 +304,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
         a few weeks).
         """
         module_system = DummySystem(load_error_modules=True)
-        xml_data ='''
+        xml_data = '''
             <video display_name="&quot;display_name&quot;"
                 html5_sources="[&quot;source_1&quot;, &quot;source_2&quot;]"
                 show_captions="false"
@@ -418,6 +418,11 @@ class VideoExportTestCase(unittest.TestCase):
     Make sure that VideoDescriptor can export itself to XML
     correctly.
     """
+    def assertXmlEqual(self, expected, xml):
+        for attr in ['tag', 'attrib', 'text', 'tail']:
+            self.assertEqual(getattr(expected, attr), getattr(xml, attr))
+        for left, right in zip(expected, xml):
+            self.assertXmlEqual(left, right)
 
     def test_export_to_xml(self):
         """Test that we write the correct XML on export."""
@@ -436,7 +441,7 @@ class VideoExportTestCase(unittest.TestCase):
         desc.html5_sources = ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg']
 
         xml = desc.definition_to_xml(None)  # We don't use the `resource_fs` parameter
-        expected = dedent('''\
+        expected = etree.fromstring('''\
          <video url_name="SampleProblem1" start_time="0:00:01" youtube="0.75:izygArpw-Qo,1.00:p2Q6BrNhdh8,1.25:1EeWXzPdhSA,1.50:rABDYkeK0x8" show_captions="false" end_time="0:01:00">
            <source src="http://www.example.com/source.mp4"/>
            <source src="http://www.example.com/source.ogg"/>
@@ -444,7 +449,7 @@ class VideoExportTestCase(unittest.TestCase):
          </video>
         ''')
 
-        self.assertEquals(expected, etree.tostring(xml, pretty_print=True))
+        self.assertXmlEqual(expected, xml)
 
     def test_export_to_xml_empty_parameters(self):
         """Test XML export with defaults."""

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

@@ -29,6 +29,7 @@ from xmodule.conditional_module import ConditionalDescriptor
 from xmodule.randomize_module import RandomizeDescriptor
 from xmodule.vertical_module import VerticalDescriptor
 from xmodule.wrapper_module import WrapperDescriptor
+from xmodule.tests import get_test_descriptor_system
 
 LEAF_XMODULES = (
     AnnotatableDescriptor,
@@ -80,20 +81,12 @@ class TestXBlockWrapper(object):
         )
         return runtime
 
-    @property
-    def leaf_descriptor_runtime(self):
-        runtime = MakoDescriptorSystem(
-            load_item=Mock(),
-            resources_fs=Mock(),
-            error_tracker=Mock(),
-            render_template=(lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs)),
-        )
-        return runtime
-
     def leaf_descriptor(self, descriptor_cls):
         location = 'i4x://org/course/category/name'
-        return descriptor_cls(
-            self.leaf_descriptor_runtime,
+        runtime = get_test_descriptor_system()
+        runtime.render_template = lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs)
+        return runtime.construct_xblock_from_class(
+            descriptor_cls,
             DictFieldData({}),
             ScopeIds(None, descriptor_cls.__name__, location, location)
         )
@@ -110,16 +103,12 @@ class TestXBlockWrapper(object):
         runtime.position = 2
         return runtime
 
-    @property
-    def container_descriptor_runtime(self):
-        runtime = Mock()
-        runtime.render_template = lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs)
-        return runtime
-
     def container_descriptor(self, descriptor_cls):
         location = 'i4x://org/course/category/name'
-        return descriptor_cls(
-            self.container_descriptor_runtime,
+        runtime = get_test_descriptor_system()
+        runtime.render_template = lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs)
+        return runtime.construct_xblock_from_class(
+            descriptor_cls,
             DictFieldData({
                 'children': range(3)
             }),

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

@@ -7,9 +7,11 @@ from xblock.field_data import DictFieldData
 from xmodule.fields import Date, Timedelta
 from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field
 import unittest
-from .import get_test_system
 from nose.tools import assert_equals  # pylint: disable=E0611
 from mock import Mock
+from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin
+from xblock.runtime import DbModel
+from xmodule.tests import get_test_descriptor_system
 
 
 class CrazyJsonString(String):
@@ -34,6 +36,11 @@ class TestFields(object):
         values=[{'display_name': 'first', 'value': 'value a'},
                 {'display_name': 'second', 'value': 'value b'}]
     )
+    showanswer = String(
+        help="When to show the problem answer to the student",
+        scope=Scope.settings,
+        default="finished"
+    )
     # Used for testing select type
     float_select = Float(scope=Scope.settings, default=.999, values=[1.23, 0.98])
     # Used for testing float type
@@ -48,10 +55,10 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         editable_fields = self.get_xml_editable_fields(DictFieldData({}))
         # Tests that the xblock fields (currently tags and name) get filtered out.
         # Also tests that xml_attributes is filtered out of XmlDescriptor.
-        self.assertEqual(1, len(editable_fields), "Expected only 1 editable field for xml descriptor.")
+        self.assertEqual(1, len(editable_fields), editable_fields)
         self.assert_field_values(
             editable_fields, 'display_name', XModuleFields.display_name,
-            explicitly_set=False, inheritable=False, value=None, default_value=None
+            explicitly_set=False, value=None, default_value=None
         )
 
     def test_override_default(self):
@@ -59,50 +66,51 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         editable_fields = self.get_xml_editable_fields(DictFieldData({'display_name': 'foo'}))
         self.assert_field_values(
             editable_fields, 'display_name', XModuleFields.display_name,
-            explicitly_set=True, inheritable=False, value='foo', default_value=None
+            explicitly_set=True, value='foo', default_value=None
         )
 
     def test_integer_field(self):
         descriptor = self.get_descriptor(DictFieldData({'max_attempts': '7'}))
         editable_fields = descriptor.editable_metadata_fields
-        self.assertEqual(7, len(editable_fields))
+        self.assertEqual(8, len(editable_fields))
         self.assert_field_values(
             editable_fields, 'max_attempts', TestFields.max_attempts,
-            explicitly_set=True, inheritable=False, value=7, default_value=1000, type='Integer',
+            explicitly_set=True, value=7, default_value=1000, type='Integer',
             options=TestFields.max_attempts.values
         )
         self.assert_field_values(
             editable_fields, 'display_name', TestFields.display_name,
-            explicitly_set=False, inheritable=False, value='local default', default_value='local default'
+            explicitly_set=False, value='local default', default_value='local default'
         )
 
         editable_fields = self.get_descriptor(DictFieldData({})).editable_metadata_fields
         self.assert_field_values(
             editable_fields, 'max_attempts', TestFields.max_attempts,
-            explicitly_set=False, inheritable=False, value=1000, default_value=1000, type='Integer',
+            explicitly_set=False, value=1000, default_value=1000, type='Integer',
             options=TestFields.max_attempts.values
         )
 
     def test_inherited_field(self):
-        model_val = {'display_name': 'inherited'}
-        descriptor = self.get_descriptor(DictFieldData(model_val))
-        # Mimic an inherited value for display_name (inherited and inheritable are the same in this case).
-        descriptor._inherited_metadata = model_val
-        descriptor._inheritable_metadata = model_val
+        kvs = InheritanceKeyValueStore(initial_values={}, inherited_settings={'showanswer': 'inherited'})
+        model_data = DbModel(kvs)
+        descriptor = self.get_descriptor(model_data)
         editable_fields = descriptor.editable_metadata_fields
         self.assert_field_values(
-            editable_fields, 'display_name', TestFields.display_name,
-            explicitly_set=False, inheritable=True, value='inherited', default_value='inherited'
+            editable_fields, 'showanswer', InheritanceMixin.showanswer,
+            explicitly_set=False, value='inherited', default_value='inherited'
         )
 
-        descriptor = self.get_descriptor(DictFieldData({'display_name': 'explicit'}))
         # Mimic the case where display_name WOULD have been inherited, except we explicitly set it.
-        descriptor._inheritable_metadata = {'display_name': 'inheritable value'}
-        descriptor._inherited_metadata = {}
+        kvs = InheritanceKeyValueStore(
+            initial_values={'showanswer': 'explicit'},
+            inherited_settings={'showanswer': 'inheritable value'}
+        )
+        model_data = DbModel(kvs)
+        descriptor = self.get_descriptor(model_data)
         editable_fields = descriptor.editable_metadata_fields
         self.assert_field_values(
-            editable_fields, 'display_name', TestFields.display_name,
-            explicitly_set=True, inheritable=True, value='explicit', default_value='inheritable value'
+            editable_fields, 'showanswer', InheritanceMixin.showanswer,
+            explicitly_set=True, value='explicit', default_value='inheritable value'
         )
 
     def test_type_and_options(self):
@@ -115,41 +123,44 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         # Tests for select
         self.assert_field_values(
             editable_fields, 'string_select', TestFields.string_select,
-            explicitly_set=False, inheritable=False, value='default value', default_value='default value',
+            explicitly_set=False, value='default value', default_value='default value',
             type='Select', options=[{'display_name': 'first', 'value': 'value a JSON'},
                                     {'display_name': 'second', 'value': 'value b JSON'}]
         )
 
         self.assert_field_values(
             editable_fields, 'float_select', TestFields.float_select,
-            explicitly_set=False, inheritable=False, value=.999, default_value=.999,
+            explicitly_set=False, value=.999, default_value=.999,
             type='Select', options=[1.23, 0.98]
         )
 
         self.assert_field_values(
             editable_fields, 'boolean_select', TestFields.boolean_select,
-            explicitly_set=False, inheritable=False, value=None, default_value=None,
+            explicitly_set=False, value=None, default_value=None,
             type='Select', options=[{'display_name': "True", "value": True}, {'display_name': "False", "value": False}]
         )
 
         # Test for float
         self.assert_field_values(
             editable_fields, 'float_non_select', TestFields.float_non_select,
-            explicitly_set=False, inheritable=False, value=.999, default_value=.999,
+            explicitly_set=False, value=.999, default_value=.999,
             type='Float', options={'min': 0, 'step': .3}
         )
 
         self.assert_field_values(
             editable_fields, 'list_field', TestFields.list_field,
-            explicitly_set=False, inheritable=False, value=[], default_value=[],
+            explicitly_set=False, value=[], default_value=[],
             type='List'
         )
 
     # Start of helper methods
     def get_xml_editable_fields(self, field_data):
-        system = get_test_system()
-        system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-        return XmlDescriptor(runtime=system, field_data=field_data, scope_ids=Mock()).editable_metadata_fields
+        runtime = get_test_descriptor_system()
+        return runtime.construct_xblock_from_class(
+            XmlDescriptor,
+            field_data=field_data,
+            scope_ids=Mock()
+        ).editable_metadata_fields
 
     def get_descriptor(self, field_data):
         class TestModuleDescriptor(TestFields, XmlDescriptor):
@@ -159,11 +170,11 @@ class EditableMetadataFieldsTest(unittest.TestCase):
                 non_editable_fields.append(TestModuleDescriptor.due)
                 return non_editable_fields
 
-        system = get_test_system()
+        system = get_test_descriptor_system()
         system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-        return TestModuleDescriptor(runtime=system, field_data=field_data, scope_ids=Mock())
+        return system.construct_xblock_from_class(TestModuleDescriptor, field_data=field_data, scope_ids=Mock())
 
-    def assert_field_values(self, editable_fields, name, field, explicitly_set, inheritable, value, default_value,
+    def assert_field_values(self, editable_fields, name, field, explicitly_set, value, default_value,
                             type='Generic', options=[]):
         test_field = editable_fields[name]
 
@@ -178,7 +189,6 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         self.assertEqual(type, test_field['type'])
 
         self.assertEqual(explicitly_set, test_field['explicitly_set'])
-        self.assertEqual(inheritable, test_field['inheritable'])
 
 
 class TestSerialize(unittest.TestCase):

common/lib/xmodule/xmodule/video_module.py

@@ -15,6 +15,8 @@ import logging
 
 from lxml import etree
 from pkg_resources import resource_string
+import datetime
+import time
 
 from django.http import Http404
 from django.conf import settings
@@ -28,8 +30,8 @@ from xblock.fields import Scope, String, Boolean, Float, List, Integer, ScopeIds
 
 from xblock.field_data import DictFieldData
 
-import datetime
-import time
+from xmodule.modulestore.inheritance import InheritanceKeyValueStore
+from xblock.runtime import DbModel
 
 log = logging.getLogger(__name__)
 
@@ -240,9 +242,11 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
             xml_data = etree.tostring(cls.load_file(filepath, system.resources_fs, location))
         field_data = VideoDescriptor._parse_video_xml(xml_data)
         field_data['location'] = location
+        kvs = InheritanceKeyValueStore(initial_values=field_data)
+        field_data = DbModel(kvs)
         video = system.construct_xblock_from_class(
             cls,
-            DictFieldData(field_data),
+            field_data,
 
             # We're loading a descriptor, so student_id is meaningless
             # We also don't have separate notions of definition and usage ids yet,
@@ -259,25 +263,22 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
         youtube_string = _create_youtube_string(self)
         # Mild workaround to ensure that tests pass -- if a field
         # is set to its default value, we don't need to write it out.
-        if youtube_string == '1.00:OEoXaMPEzfM':
-            youtube_string = ''
+        if youtube_string and youtube_string != '1.00:OEoXaMPEzfM':
+            xml.set('youtube', unicode(youtube_string))
+        xml.set('url_name', self.url_name)
         attrs = {
             'display_name': self.display_name,
             'show_captions': json.dumps(self.show_captions),
-            'youtube': youtube_string,
             'start_time': datetime.timedelta(seconds=self.start_time),
             'end_time': datetime.timedelta(seconds=self.end_time),
             'sub': self.sub,
-            'url_name': self.url_name
         }
-        fields = {field.name: field for field in self.fields.values()}
         for key, value in attrs.items():
             # Mild workaround to ensure that tests pass -- if a field
-            # is set to its default value, we don't need to write it out.
-            if key in fields and fields[key].default == getattr(self, key):
-                continue
+            # is set to its default value, we don't write it out.
             if value:
-                xml.set(key, unicode(value))
+                if key in self.fields and self.fields[key].is_set_on(self):
+                    xml.set(key, unicode(value))
 
         for source in self.html5_sources:
             ele = etree.Element('source')

common/lib/xmodule/xmodule/x_module.py

@@ -648,26 +648,17 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         # We are not allowing editing of xblock tag and name fields at this time (for any component).
         return [XBlock.tags, XBlock.name]
 
+
     def get_explicitly_set_fields_by_scope(self, scope=Scope.content):
         """
         Get a dictionary of the fields for the given scope which are set explicitly on this xblock. (Including
         any set to None.)
         """
-        if scope == Scope.settings and hasattr(self, '_inherited_metadata'):
-            inherited_metadata = getattr(self, '_inherited_metadata')
-            result = {}
-            for field in self.fields.values():
-                if (field.scope == scope and
-                        self._field_data.has(self, field.name) and
-                        field.name not in inherited_metadata):
-                    result[field.name] = self._field_data.get(self, field.name)
-            return result
-        else:
-            result = {}
-            for field in self.fields.values():
-                if (field.scope == scope and self._field_data.has(self, field.name)):
-                    result[field.name] = self._field_data.get(self, field.name)
-            return result
+        result = {}
+        for field in self.fields.values():
+            if (field.scope == scope and self._field_data.has(self, field.name)):
+                result[field.name] = self._field_data.get(self, field.name)
+        return result
 
     @property
     def editable_metadata_fields(self):
@@ -676,8 +667,14 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
 
         Can be limited by extending `non_editable_metadata_fields`.
         """
-        inherited_metadata = getattr(self, '_inherited_metadata', {})
-        inheritable_metadata = getattr(self, '_inheritable_metadata', {})
+        def jsonify_value(field, json_choice):
+            if isinstance(json_choice, dict) and 'value' in json_choice:
+                json_choice = dict(json_choice)  # make a copy so below doesn't change the original
+                json_choice['value'] = field.to_json(json_choice['value'])
+            else:
+                json_choice = field.to_json(json_choice)
+            return json_choice
+
         metadata_fields = {}
 
         # Only use the fields from this class, not mixins
@@ -688,56 +685,35 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
             if field.scope != Scope.settings or field in self.non_editable_metadata_fields:
                 continue
 
-            inheritable = False
-            value = getattr(self, field.name)
-            default_value = field.default
-            explicitly_set = self._field_data.has(self, field.name)
-            if field.name in inheritable_metadata:
-                inheritable = True
-                default_value = field.from_json(inheritable_metadata.get(field.name))
-                if field.name in inherited_metadata:
-                    explicitly_set = False
+            # gets the 'default_value' and 'explicitly_set' attrs
+            metadata_fields[field.name] = self.runtime.get_field_provenance(self, field)
+            metadata_fields[field.name]['field_name'] = field.name
+            metadata_fields[field.name]['display_name'] = field.display_name
+            metadata_fields[field.name]['help'] = field.help
+            metadata_fields[field.name]['value'] = field.read_json(self)
 
             # We support the following editors:
             # 1. A select editor for fields with a list of possible values (includes Booleans).
             # 2. Number editors for integers and floats.
             # 3. A generic string editor for anything else (editing JSON representation of the value).
             editor_type = "Generic"
-            values = copy.deepcopy(field.values)
-            if isinstance(values, tuple):
-                values = list(values)
-            if isinstance(values, list):
-                if len(values) > 0:
-                    editor_type = "Select"
-                for index, choice in enumerate(values):
-                    json_choice = copy.deepcopy(choice)
-                    if isinstance(json_choice, dict) and 'value' in json_choice:
-                        json_choice['value'] = field.to_json(json_choice['value'])
-                    else:
-                        json_choice = field.to_json(json_choice)
-                    values[index] = json_choice
+            values = field.values
+            if isinstance(values, (tuple, list)) and len(values) > 0:
+                editor_type = "Select"
+                values = [jsonify_value(field, json_choice) for json_choice in values]
             elif isinstance(field, Integer):
                 editor_type = "Integer"
             elif isinstance(field, Float):
                 editor_type = "Float"
             elif isinstance(field, List):
                 editor_type = "List"
-            metadata_fields[field.name] = {
-               'field_name': field.name,
-               'type': editor_type,
-               'display_name': field.display_name,
-               'value': field.to_json(value),
-               'options': [] if values is None else values,
-               'default_value': field.to_json(default_value),
-               'inheritable': inheritable,
-               'explicitly_set': explicitly_set,
-               'help': field.help,
-            }
+            metadata_fields[field.name]['type'] = editor_type
+            metadata_fields[field.name]['options'] = [] if values is None else values
 
         return metadata_fields
 
     # ~~~~~~~~~~~~~~~ XBlock API Wrappers ~~~~~~~~~~~~~~~~
-    def studio_view(self, context):
+    def studio_view(self, _context):
         """
         Return a fragment with the html from this XModuleDescriptor's editing view
 
@@ -750,6 +726,7 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
 
 
 class DescriptorSystem(Runtime):
+
     def __init__(self, load_item, resources_fs, error_tracker, **kwargs):
         """
         load_item: Takes a Location and returns an XModuleDescriptor
@@ -797,6 +774,33 @@ class DescriptorSystem(Runtime):
         """See documentation for `xblock.runtime:Runtime.get_block`"""
         return self.load_item(block_id)
 
+    def get_field_provenance(self, xblock, field):
+        """
+        For the given xblock, return a dict for the field's current state:
+        {
+            'default_value': what json'd value will take effect if field is unset: either the field default or
+            inherited value,
+            'explicitly_set': boolean for whether the current value is set v default/inherited,
+        }
+        :param xblock:
+        :param field:
+        """
+        # in runtime b/c runtime contains app-specific xblock behavior. Studio's the only app
+        # which needs this level of introspection right now. runtime also is 'allowed' to know
+        # about the kvs, dbmodel, etc.
+
+        result = {}
+        result['explicitly_set'] = xblock._field_data.has(xblock, field.name)
+        try:
+            block_inherited = xblock.xblock_kvs.inherited_settings
+        except AttributeError:  # if inherited_settings doesn't exist on kvs
+            block_inherited = {}
+        if field.name in block_inherited:
+            result['default_value'] = block_inherited[field.name]
+        else:
+            result['default_value'] = field.to_json(field.default)
+        return result
+
 
 class XMLParsingSystem(DescriptorSystem):
     def __init__(self, process_xml, policy, **kwargs):

common/lib/xmodule/xmodule/xml_module.py

@@ -9,10 +9,9 @@ from lxml import etree
 from xblock.fields import Dict, Scope, ScopeIds
 from xmodule.x_module import (XModuleDescriptor, policy_key)
 from xmodule.modulestore import Location
-from xmodule.modulestore.inheritance import own_metadata
+from xmodule.modulestore.inheritance import own_metadata, InheritanceKeyValueStore
 from xmodule.modulestore.xml_exporter import EdxJSONEncoder
-
-from xblock.field_data import DictFieldData
+from xblock.runtime import DbModel
 
 log = logging.getLogger(__name__)
 
@@ -365,10 +364,12 @@ class XmlDescriptor(XModuleDescriptor):
                 field_data['xml_attributes'][key] = value
         field_data['location'] = location
         field_data['category'] = xml_object.tag
+        kvs = InheritanceKeyValueStore(initial_values=field_data)
+        field_data = DbModel(kvs)
 
         return system.construct_xblock_from_class(
             cls,
-            DictFieldData(field_data),
+            field_data,
 
             # We're loading a descriptor, so student_id is meaningless
             # We also don't have separate notions of definition and usage ids yet,