xblock fields persist w/o breaking by scope

Letting xblocks handle scope rather than separating fields into
different attrs. Although, split still shunts content fields to a
different collection than setting and children fields.

The big difference is that content fields will always be a dict and not
sometimes just a string and there's no special casing of 'data' attr.

The other mind change is no more 'metadata' dict.

cms/djangoapps/contentstore/tests/test_crud.py

@@ -75,7 +75,7 @@ class TemplateTests(unittest.TestCase):
             display_name='fun test course', user_id='testbot')
 
         test_chapter = XModuleDescriptor.load_from_json({'category': 'chapter',
-            'metadata': {'display_name': 'chapter n'}},
+            'fields': {'display_name': 'chapter n'}},
             test_course.system, parent_xblock=test_course)
         self.assertIsInstance(test_chapter, SequenceDescriptor)
         self.assertEqual(test_chapter.display_name, 'chapter n')
@@ -84,7 +84,7 @@ class TemplateTests(unittest.TestCase):
         # test w/ a definition (e.g., a problem)
         test_def_content = '<problem>boo</problem>'
         test_problem = XModuleDescriptor.load_from_json({'category': 'problem',
-            'definition': {'data': test_def_content}},
+            'fields': {'data': test_def_content}},
             test_course.system, parent_xblock=test_chapter)
         self.assertIsInstance(test_problem, CapaDescriptor)
         self.assertEqual(test_problem.data, test_def_content)
@@ -99,11 +99,12 @@ class TemplateTests(unittest.TestCase):
         test_course = persistent_factories.PersistentCourseFactory.create(org='testx', prettyid='tempcourse',
             display_name='fun test course', user_id='testbot')
         test_chapter = XModuleDescriptor.load_from_json({'category': 'chapter',
-            'metadata': {'display_name': 'chapter n'}},
+            'fields': {'display_name': 'chapter n'}},
             test_course.system, parent_xblock=test_course)
         test_def_content = '<problem>boo</problem>'
-        test_problem = XModuleDescriptor.load_from_json({'category': 'problem',
-            'definition': {'data': test_def_content}},
+        # create child
+        _ = XModuleDescriptor.load_from_json({'category': 'problem',
+            'fields': {'data': test_def_content}},
             test_course.system, parent_xblock=test_chapter)
         # better to pass in persisted parent over the subdag so
         # subdag gets the parent pointer (otherwise 2 ops, persist dag, update parent children,
@@ -152,15 +153,24 @@ class TemplateTests(unittest.TestCase):
             parent_location=test_course.location, user_id='testbot')
         sub = persistent_factories.ItemFactory.create(display_name='subsection 1',
             parent_location=chapter.location, user_id='testbot', category='vertical')
-        first_problem = persistent_factories.ItemFactory.create(display_name='problem 1',
-            parent_location=sub.location, user_id='testbot', category='problem', data="<problem></problem>")
+        first_problem = persistent_factories.ItemFactory.create(
+            display_name='problem 1', parent_location=sub.location, user_id='testbot', category='problem',
+            fields={'data':"<problem></problem>"}
+        )
         first_problem.max_attempts = 3
+        first_problem.save()  # decache the above into the kvs
         updated_problem = modulestore('split').update_item(first_problem, 'testbot')
-        updated_loc = modulestore('split').delete_item(updated_problem.location, 'testbot')
+        self.assertIsNotNone(updated_problem.previous_version)
+        self.assertEqual(updated_problem.previous_version, first_problem.update_version)
+        self.assertNotEqual(updated_problem.update_version, first_problem.update_version)
+        updated_loc = modulestore('split').delete_item(updated_problem.location, 'testbot', delete_children=True)
 
-        second_problem = persistent_factories.ItemFactory.create(display_name='problem 2',
+        second_problem = persistent_factories.ItemFactory.create(
+            display_name='problem 2',
             parent_location=BlockUsageLocator(updated_loc, usage_id=sub.location.usage_id),
-            user_id='testbot', category='problem', data="<problem></problem>")
+            user_id='testbot', category='problem',
+            fields={'data':"<problem></problem>"}
+        )
 
         # course root only updated 2x
         version_history = modulestore('split').get_block_generations(test_course.location)

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

@@ -11,18 +11,17 @@ from .split_mongo_kvs import SplitMongoKVS, SplitMongoKVSid
 
 log = logging.getLogger(__name__)
 
-# TODO should this be here or w/ x_module or ???
 class CachingDescriptorSystem(MakoDescriptorSystem):
     """
     A system that has a cache of a course version's json that it will use to load modules
     from, with a backup of calling to the underlying modulestore for more data.
 
-    Computes the metadata inheritance upon creation.
+    Computes the settings (nee 'metadata') inheritance upon creation.
     """
     def __init__(self, modulestore, course_entry, module_data, lazy,
         default_class, error_tracker, render_template):
         """
-        Computes the metadata inheritance and sets up the cache.
+        Computes the settings inheritance and sets up the cache.
 
         modulestore: the module store that can be used to retrieve additional
         modules
@@ -50,9 +49,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         self.default_class = default_class
         # TODO see if self.course_id is needed: is already in course_entry but could be > 1 value
         # Compute inheritance
-        modulestore.inherit_metadata(course_entry.get('blocks', {}),
-            course_entry.get('blocks', {})
-            .get(course_entry.get('root')))
+        modulestore.inherit_settings(
+            course_entry.get('blocks', {}),
+            course_entry.get('blocks', {}).get(course_entry.get('root'))
+        )
 
     def _load_item(self, usage_id, course_entry_override=None):
         # TODO ensure all callers of system.load_item pass just the id
@@ -73,9 +73,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     def xblock_from_json(self, class_, usage_id, json_data, course_entry_override=None):
         if course_entry_override is None:
             course_entry_override = self.course_entry
-        # most likely a lazy loader but not the id directly
+        # most likely a lazy loader or the id directly
         definition = json_data.get('definition', {})
-        metadata = json_data.get('metadata', {})
 
         block_locator = BlockUsageLocator(
             version_guid=course_entry_override['_id'],
@@ -86,9 +85,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
 
         kvs = SplitMongoKVS(
             definition,
-            json_data.get('children', []),
-            metadata,
-            json_data.get('_inherited_metadata'),
+            json_data.get('fields', {}),
+            json_data.get('_inherited_settings'),
             block_locator,
             json_data.get('category'))
         model_data = DbModel(kvs, class_, None,
@@ -111,10 +109,11 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 error_msg=exc_info_to_str(sys.exc_info())
             )
 
-        module.edited_by = json_data.get('edited_by')
-        module.edited_on = json_data.get('edited_on')
-        module.previous_version = json_data.get('previous_version')
-        module.update_version = json_data.get('update_version')
+        edit_info = json_data.get('edit_info', {})
+        module.edited_by = edit_info.get('edited_by')
+        module.edited_on = edit_info.get('edited_on')
+        module.previous_version = edit_info.get('previous_version')
+        module.update_version = edit_info.get('update_version')
         module.definition_locator = self.modulestore.definition_locator(definition)
         # decache any pending field settings
         module.save()

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

@@ -16,8 +16,7 @@ class PersistentCourseFactory(factory.Factory):
     * prettyid: defaults to 999
     * display_name
     * user_id
-    * data (optional) the data payload to save in the course item
-    * metadata (optional) the metadata payload. If display_name is in the metadata, that takes
+    * fields (optional) the settings and content payloads. If display_name is in the metadata, that takes
     precedence over any display_name provided directly.
     """
     FACTORY_FOR = CourseDescriptor
@@ -28,7 +27,7 @@ class PersistentCourseFactory(factory.Factory):
     user_id = "test_user"
     data = None
     metadata = None
-    master_version = 'draft'
+    master_branch = 'draft'
 
     # pylint: disable=W0613
     @classmethod
@@ -38,17 +37,14 @@ class PersistentCourseFactory(factory.Factory):
         prettyid = kwargs.get('prettyid')
         display_name = kwargs.get('display_name')
         user_id = kwargs.get('user_id')
-        data = kwargs.get('data')
-        metadata = kwargs.get('metadata', {})
-        if metadata is None:
-            metadata = {}
-        if 'display_name' not in metadata:
-            metadata['display_name'] = display_name
+        fields = kwargs.get('fields', {})
+        if display_name and 'display_name' not in fields:
+            fields['display_name'] = display_name
 
         # Write the data to the mongo datastore
         new_course = modulestore('split').create_course(
-            org, prettyid, user_id, metadata=metadata, course_data=data, id_root=prettyid,
-            master_version=kwargs.get('master_version'))
+            org, prettyid, user_id, fields=fields, id_root=prettyid,
+            master_branch=kwargs.get('master_branch'))
 
         return new_course
 
@@ -70,26 +66,23 @@ class ItemFactory(factory.Factory):
         """
         Uses *kwargs*:
 
-        *parent_location* (required): the location of the course & possibly parent
+        :param parent_location: (required) the location of the course & possibly parent
 
-        *category* (defaults to 'chapter')
+        :param category: (defaults to 'chapter')
 
-        *data* (optional): the data for the item
+        :param fields: (optional) the data for the item
 
-        definition_locator (optional): the DescriptorLocator for the definition this uses or branches
+        :param definition_locator (optional): the DescriptorLocator for the definition this uses or branches
 
-        *display_name* (optional): the display name of the item
-
-        *metadata* (optional): dictionary of metadata attributes (display_name here takes
-        precedence over the above attr)
+        :param display_name (optional): the display name of the item
         """
-        metadata = kwargs.get('metadata', {})
-        if 'display_name' not in metadata and 'display_name' in kwargs:
-            metadata['display_name'] = kwargs['display_name']
+        fields = kwargs.get('fields', {})
+        if 'display_name' not in fields and 'display_name' in kwargs:
+            fields['display_name'] = kwargs['display_name']
 
         return modulestore('split').create_item(kwargs['parent_location'], kwargs['category'],
             kwargs['user_id'], definition_locator=kwargs.get('definition_locator'),
-            new_def_data=kwargs.get('data'), metadata=metadata)
+            fields=fields)
 
     @classmethod
     def _build(cls, target_class, *args, **kwargs):

common/lib/xmodule/xmodule/x_module.py

@@ -587,33 +587,20 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         Creates an instance of this descriptor from the supplied json_data.
         This may be overridden by subclasses
 
-        json_data: A json object with the keys 'definition' and 'metadata',
-            definition: A json object with the keys 'data' and 'children'
-                data: A json value
-                children: A list of edX Location urls
-            metadata: A json object with any keys
-
-        This json_data is transformed to model_data using the following rules:
-            1) The model data contains all of the fields from metadata
-            2) The model data contains the 'children' array
-            3) If 'definition.data' is a json object, model data contains all of its fields
-               Otherwise, it contains the single field 'data'
-            4) Any value later in this list overrides a value earlier in this list
-
         json_data:
         - 'category': the xmodule category (required)
-        - 'metadata': a dict of locally set metadata (not inherited)
-        - 'children': a list of children's usage_ids w/in this course
-        - 'definition':
+        - 'fields': a dict of locally set fields (not inherited)
+        - 'definition': (optional) the db id for the definition record (not the definition content) or a
+        definitionLazyLoader
         - '_id' (optional): the usage_id of this. Will generate one if not given one.
         """
         usage_id = json_data.get('_id', None)
-        if not '_inherited_metadata' in json_data and parent_xblock is not None:
-            json_data['_inherited_metadata'] = parent_xblock.xblock_kvs.get_inherited_metadata().copy()
-            json_metadata = json_data.get('metadata', {})
+        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_fields = json_data.get('fields', {})
             for field in inheritance.INHERITABLE_METADATA:
-                if field in json_metadata:
-                    json_data['_inherited_metadata'][field] = json_metadata[field]
+                if field in json_fields:
+                    json_data['_inherited_settings'][field] = json_fields[field]
 
         new_block = system.xblock_from_json(cls, usage_id, json_data)
         if parent_xblock is not None:
@@ -736,6 +723,27 @@ 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_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:
+                if (field.scope == scope and
+                        field.name in self._model_data and
+                        field.name not in inherited_metadata):
+                    result[field.name] = getattr(self, field.name)
+            return result
+        else:
+            result = {}
+            for field in self.fields:
+                if (field.scope == scope and field.name in self._model_data):
+                    result[field.name] = getattr(self, field.name)
+            return result
+
     @property
     def editable_metadata_fields(self):
         """

common/test/data/splitmongo_json/definitions.json

@@ -2,7 +2,7 @@
     {
         "_id":"head12345_12",
         "category":"course",
-        "data":{
+        "fields":{
             "textbooks":[
                 
             ],
@@ -43,15 +43,17 @@
             },
             "wiki_slug":null
         },
-        "edited_by":"testassist@edx.org",
-        "edited_on":{"$date" : 1364481713238},
-        "previous_version":"head12345_11",
-        "original_version":"head12345_10"
+        "edit_info": {
+            "edited_by":"testassist@edx.org",
+            "edited_on":{"$date" : 1364481713238},
+            "previous_version":"head12345_11",
+            "original_version":"head12345_10"
+        }
     },
     {
         "_id":"head12345_11",
         "category":"course",
-        "data":{
+        "fields":{
             "textbooks":[
                 
             ],
@@ -92,15 +94,17 @@
             },
             "wiki_slug":null
         },
-        "edited_by":"testassist@edx.org",
-        "edited_on":{"$date" : 1364481713238},
-        "previous_version":"head12345_10",
-        "original_version":"head12345_10"
+        "edit_info": {
+            "edited_by":"testassist@edx.org",
+            "edited_on":{"$date" : 1364481713238},
+            "previous_version":"head12345_10",
+            "original_version":"head12345_10"
+        }
     },
     {
         "_id":"head12345_10",
         "category":"course",
-        "data":{
+        "fields":{
             "textbooks":[
                 
             ],
@@ -141,15 +145,17 @@
             },
             "wiki_slug":null
         },
-        "edited_by":"test@edx.org",
-        "edited_on":{"$date": 1364473713238},
-        "previous_version":null,
-        "original_version":"head12345_10"
+        "edit_info": {
+            "edited_by":"test@edx.org",
+            "edited_on":{"$date": 1364473713238},
+            "previous_version":null,
+            "original_version":"head12345_10"
+        }
     },
     {
         "_id":"head23456_1",
         "category":"course",
-        "data":{
+        "fields":{
             "textbooks":[
                 
             ],
@@ -190,15 +196,17 @@
             },
             "wiki_slug":null
         },
-        "edited_by":"test@edx.org",
-        "edited_on":{"$date": 1364481313238},
-        "previous_version":"head23456_0",
-        "original_version":"head23456_0"
+        "edit_info": {
+            "edited_by":"test@edx.org",
+            "edited_on":{"$date": 1364481313238},
+            "previous_version":"head23456_0",
+            "original_version":"head23456_0"
+        }
     },
     {
         "_id":"head23456_0",
         "category":"course",
-        "data":{
+        "fields":{
             "textbooks":[
                 
             ],
@@ -239,15 +247,17 @@
             },
             "wiki_slug":null
         },
-        "edited_by":"test@edx.org",
-        "edited_on":{"$date" : 1364481313238},
-        "previous_version":null,
-        "original_version":"head23456_0"
+        "edit_info": {
+            "edited_by":"test@edx.org",
+            "edited_on":{"$date" : 1364481313238},
+            "previous_version":null,
+            "original_version":"head23456_0"
+        }
     },
     {
         "_id":"head345679_1",
         "category":"course",
-        "data":{
+        "fields":{
             "textbooks":[
                 
             ],
@@ -281,54 +291,66 @@
             },
             "wiki_slug":null
         },
-        "edited_by":"test@edx.org",
-        "edited_on":{"$date" : 1364481313238},
-        "previous_version":null,
-        "original_version":"head23456_0"
+        "edit_info": {
+            "edited_by":"test@edx.org",
+            "edited_on":{"$date" : 1364481313238},
+            "previous_version":null,
+            "original_version":"head23456_0"
+        }
     },
     {
         "_id":"chapter12345_1",
         "category":"chapter",
-        "data":null,
-        "edited_by":"testassist@edx.org",
-        "edited_on":{"$date" : 1364483713238},
-        "previous_version":null,
-        "original_version":"chapter12345_1"
+        "fields":{},
+        "edit_info": {
+            "edited_by":"testassist@edx.org",
+            "edited_on":{"$date" : 1364483713238},
+            "previous_version":null,
+            "original_version":"chapter12345_1"
+        }
     },
     {
         "_id":"chapter12345_2",
         "category":"chapter",
-        "data":null,
-        "edited_by":"testassist@edx.org",
-        "edited_on":{"$date" : 1364483713238},
-        "previous_version":null,
-        "original_version":"chapter12345_2"
+        "fields":{},
+        "edit_info": {
+            "edited_by":"testassist@edx.org",
+            "edited_on":{"$date" : 1364483713238},
+            "previous_version":null,
+            "original_version":"chapter12345_2"
+        }
     },
     {
         "_id":"chapter12345_3",
         "category":"chapter",
-        "data":null,
-        "edited_by":"testassist@edx.org",
-        "edited_on":{"$date" : 1364483713238},
-        "previous_version":null,
-        "original_version":"chapter12345_3"
+        "fields":{},
+        "edit_info": {
+            "edited_by":"testassist@edx.org",
+            "edited_on":{"$date" : 1364483713238},
+            "previous_version":null,
+            "original_version":"chapter12345_3"
+        }
     },
     {
         "_id":"problem12345_3_1",
         "category":"problem",
-        "data":"",
-        "edited_by":"testassist@edx.org",
-        "edited_on":{"$date" : 1364483713238},
-        "previous_version":null,
-        "original_version":"problem12345_3_1"
+        "fields": {"data": ""},
+        "edit_info": {
+            "edited_by":"testassist@edx.org",
+            "edited_on":{"$date" : 1364483713238},
+            "previous_version":null,
+            "original_version":"problem12345_3_1"
+        }
     },
     {
         "_id":"problem12345_3_2",
         "category":"problem",
-        "data":"",
-        "edited_by":"testassist@edx.org",
-        "edited_on":{"$date" : 1364483713238},
-        "previous_version":null,
-        "original_version":"problem12345_3_2"
+        "fields": {"data": ""},
+        "edit_info": {
+            "edited_by":"testassist@edx.org",
+            "edited_on":{"$date" : 1364483713238},
+            "previous_version":null,
+            "original_version":"problem12345_3_2"
+        }
     }
 ]
\ No newline at end of file