make block.get_parent() work.

Co-Authored-By: Christina Roberts <christina@edx.org>
Co-Authored-By: Daniel Friedman <dfriedman@edx.org>
Co-Authored-By: Don Mitchell <dmitchell@edx.org>

cms/djangoapps/contentstore/views/item.py

@@ -143,7 +143,8 @@ def xblock_handler(request, usage_key_string):
                     # right now can't combine output of this w/ output of _get_module_info, but worthy goal
                     return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key))
                 # TODO: pass fields to _get_module_info and only return those
-                rsp = _get_module_info(_get_xblock(usage_key, request.user))
+                with modulestore().bulk_operations(usage_key.course_key):
+                    rsp = _get_module_info(_get_xblock(usage_key, request.user))
                 return JsonResponse(rsp)
             else:
                 return HttpResponse(status=406)

cms/djangoapps/contentstore/views/tests/test_item.py

@@ -116,9 +116,20 @@ class GetItemTest(ItemTest):
         return resp
 
     @ddt.data(
-        (1, 21, 23, 35, 37),
-        (2, 22, 24, 38, 39),
-        (3, 23, 25, 41, 41),
+        # chapter explanation:
+        # 1-3. get course, chapter, chapter's children,
+        # 4-7. chapter's published grandchildren, chapter's draft grandchildren, published & then draft greatgrand
+        # 8 compute chapter's parent
+        # 9 get chapter's parent
+        # 10-16. run queries 2-8 again
+        # 17-19. compute seq, vert, and problem's parents (odd since it's going down; so, it knows)
+        # 20-22. get course 3 times
+        # 23. get chapter
+        # 24. compute chapter's parent (course)
+        # 25. compute course's parent (None)
+        (1, 20, 20, 26, 26),
+        (2, 21, 21, 29, 28),
+        (3, 22, 22, 32, 30),
     )
     @ddt.unpack
     def test_get_query_count(self, branching_factor, chapter_queries, section_queries, unit_queries, problem_queries):
@@ -411,21 +422,46 @@ class TestDuplicateItem(ItemTest):
         except for location and display name.
         """
         def duplicate_and_verify(source_usage_key, parent_usage_key):
+            """ Duplicates the source, parenting to supplied parent. Then does equality check. """
             usage_key = self._duplicate_item(parent_usage_key, source_usage_key)
-            self.assertTrue(check_equality(source_usage_key, usage_key), "Duplicated item differs from original")
+            self.assertTrue(
+                check_equality(source_usage_key, usage_key, parent_usage_key),
+                "Duplicated item differs from original"
+            )
 
-        def check_equality(source_usage_key, duplicate_usage_key):
+        def check_equality(source_usage_key, duplicate_usage_key, parent_usage_key=None):
+            """
+            Gets source and duplicated items from the modulestore using supplied usage keys.
+            Then verifies that they represent equivalent items (modulo parents and other
+            known things that may differ).
+            """
             original_item = self.get_item_from_modulestore(source_usage_key)
             duplicated_item = self.get_item_from_modulestore(duplicate_usage_key)
 
             self.assertNotEqual(
-                original_item.location,
-                duplicated_item.location,
+                unicode(original_item.location),
+                unicode(duplicated_item.location),
                 "Location of duplicate should be different from original"
             )
-            # Set the location and display name to be the same so we can make sure the rest of the duplicate is equal.
+
+            # Parent will only be equal for root of duplicated structure, in the case
+            # where an item is duplicated in-place.
+            if parent_usage_key and unicode(original_item.parent) == unicode(parent_usage_key):
+                self.assertEqual(
+                    unicode(parent_usage_key), unicode(duplicated_item.parent),
+                    "Parent of duplicate should equal parent of source for root xblock when duplicated in-place"
+                )
+            else:
+                self.assertNotEqual(
+                    unicode(original_item.parent), unicode(duplicated_item.parent),
+                    "Parent duplicate should be different from source"
+                )
+
+            # Set the location, display name, and parent to be the same so we can make sure the rest of the
+            # duplicate is equal.
             duplicated_item.location = original_item.location
             duplicated_item.display_name = original_item.display_name
+            duplicated_item.parent = original_item.parent
 
             # Children will also be duplicated, so for the purposes of testing equality, we will set
             # the children to the original after recursively checking the children.

common/lib/xmodule/xmodule/modulestore/modulestore_settings.py

@@ -98,6 +98,7 @@ def update_module_store_settings(
         module_store_options=None,
         xml_store_options=None,
         default_store=None,
+        mappings=None,
 ):
     """
     Updates the settings for each store defined in the given module_store_setting settings
@@ -123,6 +124,9 @@ def update_module_store_settings(
                 return
         raise Exception("Could not find setting for requested default store: {}".format(default_store))
 
+    if mappings and 'mappings' in module_store_setting['default']['OPTIONS']:
+        module_store_setting['default']['OPTIONS']['mappings'] = mappings
+
 
 def get_mixed_stores(mixed_setting):
     """

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

@@ -29,7 +29,7 @@ from contracts import contract, new_contract
 
 from importlib import import_module
 from opaque_keys.edx.keys import UsageKey, CourseKey, AssetKey
-from opaque_keys.edx.locations import Location
+from opaque_keys.edx.locations import Location, BlockUsageLocator
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from opaque_keys.edx.locator import CourseLocator, LibraryLocator
 
@@ -56,6 +56,7 @@ new_contract('CourseKey', CourseKey)
 new_contract('AssetKey', AssetKey)
 new_contract('AssetMetadata', AssetMetadata)
 new_contract('long', long)
+new_contract('BlockUsageLocator', BlockUsageLocator)
 
 # sort order that returns DRAFT items first
 SORT_REVISION_FAVOR_DRAFT = ('_id.revision', pymongo.DESCENDING)
@@ -93,12 +94,13 @@ 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):
+    def __init__(self, data, parent, children, metadata):
         super(MongoKeyValueStore, self).__init__()
         if not isinstance(data, dict):
             self._data = {'data': data}
         else:
             self._data = data
+        self._parent = parent
         self._children = children
         self._metadata = metadata
 
@@ -106,7 +108,7 @@ class MongoKeyValueStore(InheritanceKeyValueStore):
         if key.scope == Scope.children:
             return self._children
         elif key.scope == Scope.parent:
-            return None
+            return self._parent
         elif key.scope == Scope.settings:
             return self._metadata[key.field_name]
         elif key.scope == Scope.content:
@@ -219,15 +221,35 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
                     self._convert_reference_to_key(childloc)
                     for childloc in definition.get('children', [])
                 ]
+
+                parent = None
+                if self.cached_metadata is not None:
+                    # fish the parent out of here if it's available
+                    parent_url = self.cached_metadata.get(unicode(location), {}).get('parent', {}).get(
+                        ModuleStoreEnum.Branch.published_only if location.revision is None
+                        else ModuleStoreEnum.Branch.draft_preferred
+                    )
+                    if parent_url:
+                        parent = BlockUsageLocator.from_string(parent_url)
+                if not parent and category != 'course':
+                    # try looking it up just-in-time (but not if we're working with a root node (course).
+                    parent = self.modulestore.get_parent_location(
+                        as_published(location),
+                        ModuleStoreEnum.RevisionOption.published_only if location.revision is None
+                        else ModuleStoreEnum.RevisionOption.draft_preferred
+                    )
+
                 data = definition.get('data', {})
                 if isinstance(data, basestring):
                     data = {'data': data}
+
                 mixed_class = self.mixologist.mix(class_)
                 if data:  # empty or None means no work
                     data = self._convert_reference_fields_to_keys(mixed_class, location.course_key, data)
                 metadata = self._convert_reference_fields_to_keys(mixed_class, location.course_key, metadata)
                 kvs = MongoKeyValueStore(
                     data,
+                    parent,
                     children,
                     metadata,
                 )
@@ -439,6 +461,32 @@ class MongoBulkOpsMixin(BulkOperationsMixin):
         )
 
 
+class ParentLocationCache(dict):
+    """
+    Dict-based object augmented with a more cache-like interface, for internal use.
+    """
+    # pylint: disable=missing-docstring
+
+    @contract(key=unicode)
+    def has(self, key):
+        return key in self
+
+    @contract(key=unicode, value="BlockUsageLocator | None")
+    def set(self, key, value):
+        self[key] = value
+
+    @contract(key=unicode)
+    def delete(self, key):
+        if key in self:
+            del self[key]
+
+    @contract(value="BlockUsageLocator")
+    def delete_by_value(self, value):
+        keys_to_delete = [k for k, v in self.iteritems() if v == value]
+        for key in keys_to_delete:
+            del self[key]
+
+
 class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, MongoBulkOpsMixin):
     """
     A Mongodb backed ModuleStore
@@ -572,6 +620,16 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             return location.replace(revision=MongoRevisionKey.draft)
         return location.replace(revision=MongoRevisionKey.published)
 
+    def _get_parent_cache(self, branch):
+        """
+        Provides a reference to one of the two branch-specific
+        ParentLocationCaches associated with the current request (if any).
+        """
+        if self.request_cache is not None:
+            return self.request_cache.data.setdefault('parent-location-{}'.format(branch), ParentLocationCache())
+        else:
+            return ParentLocationCache()
+
     def _compute_metadata_inheritance_tree(self, course_id):
         '''
         TODO (cdodge) This method can be deleted when the 'split module store' work has been completed
@@ -640,7 +698,13 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                     _compute_inherited_metadata(child)
                 else:
                     # this is likely a leaf node, so let's record what metadata we need to inherit
-                    metadata_to_inherit[child] = my_metadata
+                    metadata_to_inherit[child] = my_metadata.copy()
+                # WARNING: 'parent' is not part of inherited metadata, but
+                # we're piggybacking on this recursive traversal to grab
+                # and cache the child's parent, as a performance optimization.
+                # The 'parent' key will be popped out of the dictionary during
+                # CachingDescriptorSystem.load_item
+                metadata_to_inherit[child].setdefault('parent', {})[self.get_branch_setting()] = url
 
         if root is not None:
             _compute_inherited_metadata(root)
@@ -735,12 +799,18 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         data = {}
         to_process = list(items)
         course_key = self.fill_in_run(course_key)
+        parent_cache = self._get_parent_cache(self.get_branch_setting())
+
         while to_process and depth is None or depth >= 0:
             children = []
             for item in to_process:
                 self._clean_item_data(item)
-                children.extend(item.get('definition', {}).get('children', []))
-                data[Location._from_deprecated_son(item['location'], course_key.run)] = item
+                item_location = Location._from_deprecated_son(item['location'], course_key.run)
+                item_children = item.get('definition', {}).get('children', [])
+                children.extend(item_children)
+                for item_child in item_children:
+                    parent_cache.set(item_child, item_location)
+                data[item_location] = item
 
             if depth == 0:
                 break
@@ -1245,6 +1315,13 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             if xblock.has_children:
                 children = self._serialize_scope(xblock, Scope.children)
                 payload.update({'definition.children': children['children']})
+
+                # Remove all old pointers to me, then add my current children back
+                parent_cache = self._get_parent_cache(self.get_branch_setting())
+                parent_cache.delete_by_value(xblock.location)
+                for child in xblock.children:
+                    parent_cache.set(unicode(child), xblock.location)
+
             self._update_single_item(xblock.scope_ids.usage_id, payload, allow_not_found=allow_not_found)
 
             # update subtree edited info for ancestors
@@ -1339,6 +1416,10 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         assert revision == ModuleStoreEnum.RevisionOption.published_only \
             or revision == ModuleStoreEnum.RevisionOption.draft_preferred
 
+        parent_cache = self._get_parent_cache(self.get_branch_setting())
+        if parent_cache.has(unicode(location)):
+            return parent_cache.get(unicode(location))
+
         # create a query with tag, org, course, and the children field set to the given location
         query = self._course_key_to_son(location.course_key)
         query['definition.children'] = unicode(location)
@@ -1347,30 +1428,35 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         if revision == ModuleStoreEnum.RevisionOption.published_only:
             query['_id.revision'] = MongoRevisionKey.published
 
-        # query the collection, sorting by DRAFT first
-        parents = self.collection.find(query, {'_id': True}, sort=[SORT_REVISION_FAVOR_DRAFT])
+        def cache_and_return(parent_loc):  # pylint:disable=missing-docstring
+            parent_cache.set(unicode(location), parent_loc)
+            return parent_loc
 
-        if parents.count() == 0:
+        # query the collection, sorting by DRAFT first
+        parents = list(
+            self.collection.find(query, {'_id': True}, sort=[SORT_REVISION_FAVOR_DRAFT])
+        )
+        if len(parents) == 0:
             # no parents were found
-            return None
+            return cache_and_return(None)
 
         if revision == ModuleStoreEnum.RevisionOption.published_only:
-            if parents.count() > 1:
+            if len(parents) > 1:
                 non_orphan_parents = self._get_non_orphan_parents(location, parents, revision)
                 if len(non_orphan_parents) == 0:
                     # no actual parent found
-                    return None
+                    return cache_and_return(None)
 
                 if len(non_orphan_parents) > 1:
                     # should never have multiple PUBLISHED parents
                     raise ReferentialIntegrityError(
-                        u"{} parents claim {}".format(parents.count(), location)
+                        u"{} parents claim {}".format(len(parents), location)
                     )
                 else:
-                    return non_orphan_parents[0]
+                    return cache_and_return(non_orphan_parents[0].replace(run=location.course_key.run))
             else:
                 # return the single PUBLISHED parent
-                return Location._from_deprecated_son(parents[0]['_id'], location.course_key.run)
+                return cache_and_return(Location._from_deprecated_son(parents[0]['_id'], location.course_key.run))
         else:
             # there could be 2 different parents if
             #   (1) the draft item was moved or
@@ -1386,11 +1472,11 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             # since we sorted by SORT_REVISION_FAVOR_DRAFT, the 0'th parent is the one we want
             if published_parents > 1:
                 non_orphan_parents = self._get_non_orphan_parents(location, all_parents, revision)
-                return non_orphan_parents[0]
+                return cache_and_return(non_orphan_parents[0].replace(run=location.course_key.run))
 
             found_id = all_parents[0]['_id']
             # don't disclose revision outside modulestore
-            return Location._from_deprecated_son(found_id, location.course_key.run)
+            return cache_and_return(Location._from_deprecated_son(found_id, location.course_key.run))
 
     def get_parent_location(self, location, revision=ModuleStoreEnum.RevisionOption.published_only, **kwargs):
         '''
@@ -1409,7 +1495,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         '''
         parent = self._get_raw_parent_location(location, revision)
         if parent:
-            return as_published(parent)
+            return parent
         return None
 
     def get_modulestore_type(self, course_key=None):
@@ -1463,6 +1549,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         """
         kvs = MongoKeyValueStore(
             definition_data,
+            None,
             [],
             metadata,
         )

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

@@ -643,6 +643,9 @@ class DraftModuleStore(MongoModuleStore):
 
         Raises:
             ItemNotFoundError: if any of the draft subtree nodes aren't found
+
+        Returns:
+            The newly published xblock
         """
         # NOTE: cannot easily use self._breadth_first b/c need to get pub'd and draft as pairs
         # (could do it by having 2 breadth first scans, the first to just get all published children

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

@@ -210,26 +210,18 @@ class ItemFactory(XModuleFactory):
             # replace the display name with an optional parameter passed in from the caller
             if display_name is not None:
                 metadata['display_name'] = display_name
-            runtime = parent.runtime if parent else None
-            store.create_item(
+
+            module = store.create_child(
                 user_id,
-                location.course_key,
+                parent.location,
                 location.block_type,
                 block_id=location.block_id,
                 metadata=metadata,
                 definition_data=data,
-                runtime=runtime
+                runtime=parent.runtime,
+                fields=kwargs,
             )
 
-            module = store.get_item(location)
-
-            for attr, val in kwargs.items():
-                setattr(module, attr, val)
-            # Save the attributes we just set
-            module.save()
-
-            store.update_item(module, user_id)
-
             # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
             # if we add one then we need to also add it to the policy information (i.e. metadata)
             # we should remove this once we can break this reference from the course to static tabs
@@ -248,12 +240,15 @@ class ItemFactory(XModuleFactory):
                 parent.children.append(location)
                 store.update_item(parent, user_id)
                 if publish_item:
-                    store.publish(parent.location, user_id)
+                    published_parent = store.publish(parent.location, user_id)
+                    # module is last child of parent
+                    return published_parent.get_children()[-1]
+                else:
+                    return store.get_item(location)
             elif publish_item:
-                store.publish(location, user_id)
-
-        # return the published item
-        return store.get_item(location)
+                return store.publish(location, user_id)
+            else:
+                return module
 
 
 @contextmanager

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

@@ -358,12 +358,12 @@ class TestMixedModuleStore(CourseComparisonTest):
             self.store.has_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred)
 
     # draft queries:
-    #   problem: find draft item, find all items pertinent to inheritance computation
+    #   problem: find draft item, find all items pertinent to inheritance computation, find parent
     #   non-existent problem: find draft, find published
     # split:
     #   problem: active_versions, structure
     #   non-existent problem: ditto
-    @ddt.data(('draft', [2, 2], 0), ('split', [2, 2], 0))
+    @ddt.data(('draft', [3, 2], 0), ('split', [2, 2], 0))
     @ddt.unpack
     def test_get_item(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
@@ -388,10 +388,10 @@ class TestMixedModuleStore(CourseComparisonTest):
             self.store.get_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred)
 
     # Draft:
-    #    wildcard query, 6! load pertinent items for inheritance calls, course root fetch (why)
+    #    wildcard query, 6! load pertinent items for inheritance calls, load parents, course root fetch (why)
     # Split:
     #    active_versions (with regex), structure, and spurious active_versions refetch
-    @ddt.data(('draft', 8, 0), ('split', 3, 0))
+    @ddt.data(('draft', 14, 0), ('split', 3, 0))
     @ddt.unpack
     def test_get_items(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
@@ -405,7 +405,6 @@ class TestMixedModuleStore(CourseComparisonTest):
 
         course_locn = self.course_locations[self.MONGO_COURSEID]
         with check_mongo_calls(max_find, max_send):
-            # NOTE: use get_course if you just want the course. get_items is expensive
             modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'})
         self.assertEqual(len(modules), 6)
 
@@ -416,12 +415,11 @@ class TestMixedModuleStore(CourseComparisonTest):
                 revision=ModuleStoreEnum.RevisionOption.draft_preferred
             )
 
-    # draft: get draft, count parents, get parents, count & get grandparents, count & get greatgrand,
-    #        count & get next ancestor (chapter's parent), count non-existent next ancestor, get inheritance
+    # draft: get draft, get ancestors up to course (2-6), compute inheritance
     #    sends: update problem and then each ancestor up to course (edit info)
     # split: active_versions, definitions (calculator field), structures
     #  2 sends to update index & structure (note, it would also be definition if a content field changed)
-    @ddt.data(('draft', 11, 5), ('split', 3, 2))
+    @ddt.data(('draft', 7, 5), ('split', 3, 2))
     @ddt.unpack
     def test_update_item(self, default_ms, max_find, max_send):
         """
@@ -886,9 +884,9 @@ class TestMixedModuleStore(CourseComparisonTest):
 
     # notice this doesn't test getting a public item via draft_preferred which draft would have 2 hits (split
     # still only 2)
-    # Draft: count via definition.children query, then fetch via that query
+    # Draft: get_parent
     # Split: active_versions, structure
-    @ddt.data(('draft', 2, 0), ('split', 2, 0))
+    @ddt.data(('draft', 1, 0), ('split', 2, 0))
     @ddt.unpack
     def test_get_parent_locations(self, default_ms, max_find, max_send):
         """
@@ -1022,20 +1020,12 @@ class TestMixedModuleStore(CourseComparisonTest):
     # Draft:
     #   Problem path:
     #    1. Get problem
-    #    2-3. count matches definition.children called 2x?
-    #    4. get parent via definition.children query
-    #    5-7. 2 counts and 1 get grandparent via definition.children
-    #    8-10. ditto for great-grandparent
-    #    11-13. ditto for next ancestor
-    #    14. fail count query looking for parent of course (unnecessary)
-    #    15. get course record direct query (not via definition.children) (already fetched in 13)
-    #    16. get items for inheritance computation
-    #    17. get vertical (parent of problem)
-    #    18. get items for inheritance computation (why? caching should handle)
-    #    19-20. get vertical_x1b (? why? this is the only ref in trace) & items for inheritance computation
-    #   Chapter path: get chapter, count parents 2x, get parents, count non-existent grandparents
+    #    2-6. get parent and rest of ancestors up to course
+    #    7-8. get sequential, compute inheritance
+    #    8-9. get vertical, compute inheritance
+    #    10-11. get other vertical_x1b (why?) and compute inheritance
     # Split: active_versions & structure
-    @ddt.data(('draft', [20, 5], 0), ('split', [2, 2], 0))
+    @ddt.data(('draft', [12, 3], 0), ('split', [2, 2], 0))
     @ddt.unpack
     def test_path_to_location(self, default_ms, num_finds, num_sends):
         """

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

@@ -717,15 +717,16 @@ class TestMongoKeyValueStore(object):
     def setUp(self):
         self.data = {'foo': 'foo_value'}
         self.course_id = SlashSeparatedCourseKey('org', 'course', 'run')
+        self.parent = self.course_id.make_usage_key('parent', 'p')
         self.children = [self.course_id.make_usage_key('child', 'a'), self.course_id.make_usage_key('child', 'b')]
         self.metadata = {'meta': 'meta_val'}
-        self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata)
+        self.kvs = MongoKeyValueStore(self.data, self.parent, self.children, self.metadata)
 
     def test_read(self):
         assert_equals(self.data['foo'], self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'foo')))
+        assert_equals(self.parent, self.kvs.get(KeyValueStore.Key(Scope.parent, None, None, 'parent')))
         assert_equals(self.children, self.kvs.get(KeyValueStore.Key(Scope.children, None, None, 'children')))
         assert_equals(self.metadata['meta'], self.kvs.get(KeyValueStore.Key(Scope.settings, None, None, 'meta')))
-        assert_equals(None, self.kvs.get(KeyValueStore.Key(Scope.parent, None, None, 'parent')))
 
     def test_read_invalid_scope(self):
         for scope in (Scope.preferences, Scope.user_info, Scope.user_state):
@@ -735,7 +736,7 @@ class TestMongoKeyValueStore(object):
             assert_false(self.kvs.has(key))
 
     def test_read_non_dict_data(self):
-        self.kvs = MongoKeyValueStore('xml_data', self.children, self.metadata)
+        self.kvs = MongoKeyValueStore('xml_data', self.parent, self.children, self.metadata)
         assert_equals('xml_data', self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'data')))
 
     def _check_write(self, key, value):
@@ -746,9 +747,10 @@ class TestMongoKeyValueStore(object):
         yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'foo'), 'new_data')
         yield (self._check_write, KeyValueStore.Key(Scope.children, None, None, 'children'), [])
         yield (self._check_write, KeyValueStore.Key(Scope.settings, None, None, 'meta'), 'new_settings')
+        # write Scope.parent raises InvalidScope, which is covered in test_write_invalid_scope
 
     def test_write_non_dict_data(self):
-        self.kvs = MongoKeyValueStore('xml_data', self.children, self.metadata)
+        self.kvs = MongoKeyValueStore('xml_data', self.parent, self.children, self.metadata)
         self._check_write(KeyValueStore.Key(Scope.content, None, None, 'data'), 'new_data')
 
     def test_write_invalid_scope(self):

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

@@ -47,14 +47,10 @@ class TestPublish(SplitWMongoCourseBoostrapper):
             # For each (4) item created
             #   - try to find draft
             #   - try to find non-draft
-            #   - retrieve draft of new parent
-            #   - get last error
-            #   - load parent
-            #   - load inheritable data
-            #   - load parent
-            #   - load ancestors
+            #   - compute what is parent
+            #   - load draft parent again & compute its parent chain up to course
             # count for updates increased to 16 b/c of edit_info updating
-            with check_mongo_calls(40, 16):
+            with check_mongo_calls(36, 16):
                 self._create_item('html', 'Html1', "<p>Goodbye</p>", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', split=False)
                 self._create_item(
                     'discussion', 'Discussion1',
@@ -96,22 +92,22 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         item = self.draft_mongo.get_item(vert_location, 2)
         # Finds:
         #   1 get draft vert,
-        #   2-10 for each child: (3 children x 3 queries each)
-        #      get draft and then published child
+        #   2 compute parent
+        #   3-14 for each child: (3 children x 4 queries each)
+        #      get draft, compute parent, and then published child
         #      compute inheritance
-        #   11 get published vert
-        #   12-15 get each ancestor (count then get): (2 x 2),
-        #   16 then fail count of course parent (1)
-        #   17 compute inheritance
-        #   18-19 get draft and published vert
+        #   15 get published vert
+        #   16-18 get ancestor chain
+        #   19 compute inheritance
+        #   20-22 get draft and published vert, compute parent
         # Sends:
         #   delete the subtree of drafts (1 call),
         #   update the published version of each node in subtree (4 calls),
         #   update the ancestors up to course (2 calls)
         if mongo_uses_error_check(self.draft_mongo):
-            max_find = 20
+            max_find = 23
         else:
-            max_find = 19
+            max_find = 22
         with check_mongo_calls(max_find, 7):
             self.draft_mongo.publish(item.location, self.user_id)
 

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

@@ -31,7 +31,7 @@ class TestXMLModuleStore(unittest.TestCase):
     Test around the XML modulestore
     """
     def test_xml_modulestore_type(self):
-        store = XMLModuleStore(DATA_DIR, course_dirs=['toy', 'simple'])
+        store = XMLModuleStore(DATA_DIR, course_dirs=[])
         self.assertEqual(store.get_modulestore_type(), ModuleStoreEnum.Type.xml)
 
     def test_unicode_chars_in_xml_content(self):

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

@@ -53,7 +53,7 @@ def clean_out_mako_templating(xml_string):
 
 class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
     def __init__(self, xmlstore, course_id, course_dir,
-                 error_tracker, parent_tracker,
+                 error_tracker,
                  load_error_modules=True, **kwargs):
         """
         A class that handles loading from xml.  Does some munging to ensure that
@@ -209,7 +209,8 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
 
             if descriptor.has_children:
                 for child in descriptor.get_children():
-                    parent_tracker.add_parent(child.scope_ids.usage_id, descriptor.scope_ids.usage_id)
+                    child.parent = descriptor.location
+                    child.save()
 
             # After setting up the descriptor, save any changes that we have
             # made to attributes on the descriptor to the underlying KeyValueStore.
@@ -278,41 +279,6 @@ class CourseLocationManager(OpaqueKeyReader, AsideKeyGenerator):
         return usage_id
 
 
-class ParentTracker(object):
-    """A simple class to factor out the logic for tracking location parent pointers."""
-    def __init__(self):
-        """
-        Init
-        """
-        # location -> parent.  Not using defaultdict because we care about the empty case.
-        self._parents = dict()
-
-    def add_parent(self, child, parent):
-        """
-        Add a parent of child location to the set of parents.  Duplicate calls have no effect.
-
-        child and parent must be :class:`.Location` instances.
-        """
-        self._parents[child] = parent
-
-    def is_known(self, child):
-        """
-        returns True iff child has some parents.
-        """
-        return child in self._parents
-
-    def make_known(self, location):
-        """Tell the parent tracker about an object, without registering any
-        parents for it.  Used for the top level course descriptor locations."""
-        self._parents.setdefault(location, None)
-
-    def parent(self, child):
-        """
-        Return the parent of this child.  If not is_known(child), will throw a KeyError
-        """
-        return self._parents[child]
-
-
 class XMLModuleStore(ModuleStoreReadBase):
     """
     An XML backed ModuleStore
@@ -352,8 +318,6 @@ class XMLModuleStore(ModuleStoreReadBase):
             class_ = getattr(import_module(module_path), class_name)
             self.default_class = class_
 
-        self.parent_trackers = defaultdict(ParentTracker)
-
         # All field data will be stored in an inheriting field data.
         self.field_data = inheriting_field_data(kvs=DictKeyValueStore())
 
@@ -400,7 +364,7 @@ class XMLModuleStore(ModuleStoreReadBase):
         else:
             self.courses[course_dir] = course_descriptor
             self._course_errors[course_descriptor.id] = errorlog
-            self.parent_trackers[course_descriptor.id].make_known(course_descriptor.scope_ids.usage_id)
+            course_descriptor.parent = None
 
     def __unicode__(self):
         '''
@@ -512,7 +476,6 @@ class XMLModuleStore(ModuleStoreReadBase):
                 course_id=course_id,
                 course_dir=course_dir,
                 error_tracker=tracker,
-                parent_tracker=self.parent_trackers[course_id],
                 load_error_modules=self.load_error_modules,
                 get_policy=get_policy,
                 mixins=self.xblock_mixins,
@@ -756,10 +719,8 @@ class XMLModuleStore(ModuleStoreReadBase):
         '''Find the location that is the parent of this location in this
         course.  Needed for path_to_location().
         '''
-        if not self.parent_trackers[location.course_key].is_known(location):
-            raise ItemNotFoundError("{0} not in {1}".format(location, location.course_key))
-
-        return self.parent_trackers[location.course_key].parent(location)
+        block = self.get_item(location, 0)
+        return block.parent
 
     def get_modulestore_type(self, course_key=None):
         """

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

@@ -28,7 +28,7 @@ import json
 import re
 from lxml import etree
 
-from .xml import XMLModuleStore, ImportSystem, ParentTracker
+from .xml import XMLModuleStore, ImportSystem
 from xblock.runtime import KvsFieldData, DictKeyValueStore
 from xmodule.x_module import XModuleDescriptor
 from opaque_keys.edx.keys import UsageKey
@@ -479,11 +479,13 @@ def _import_module_and_update_references(
 
     fields = {}
     for field_name, field in module.fields.iteritems():
-        if field.is_set_on(module):
-            if field.scope == Scope.parent:
-                continue
+        if field.scope != Scope.parent and field.is_set_on(module):
             if isinstance(field, Reference):
-                fields[field_name] = _convert_reference_fields_to_new_namespace(field.read_from(module))
+                value = field.read_from(module)
+                if value is None:
+                    fields[field_name] = None
+                else:
+                    fields[field_name] = _convert_reference_fields_to_new_namespace(field.read_from(module))
             elif isinstance(field, ReferenceList):
                 references = field.read_from(module)
                 fields[field_name] = [_convert_reference_fields_to_new_namespace(reference) for reference in references]
@@ -548,7 +550,6 @@ def _import_course_draft(
         course_id=source_course_id,
         course_dir=draft_course_dir,
         error_tracker=errorlog.tracker,
-        parent_tracker=ParentTracker(),
         load_error_modules=False,
         mixins=xml_module_store.xblock_mixins,
         field_data=KvsFieldData(kvs=DictKeyValueStore()),

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

@@ -30,7 +30,6 @@ class DummySystem(ImportSystem):
             course_id=SlashSeparatedCourseKey(ORG, COURSE, 'test_run'),
             course_dir='test_dir',
             error_tracker=Mock(),
-            parent_tracker=Mock(),
             load_error_modules=load_error_modules,
         )
 

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

@@ -36,14 +36,12 @@ class DummySystem(ImportSystem):
         course_id = SlashSeparatedCourseKey(ORG, COURSE, 'test_run')
         course_dir = "test_dir"
         error_tracker = Mock()
-        parent_tracker = Mock()
 
         super(DummySystem, self).__init__(
             xmlstore=xmlstore,
             course_id=course_id,
             course_dir=course_dir,
             error_tracker=error_tracker,
-            parent_tracker=parent_tracker,
             load_error_modules=load_error_modules,
             field_data=KvsFieldData(DictKeyValueStore()),
         )

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

@@ -39,14 +39,12 @@ class DummySystem(ImportSystem):
         course_id = SlashSeparatedCourseKey(ORG, COURSE, 'test_run')
         course_dir = "test_dir"
         error_tracker = Mock()
-        parent_tracker = Mock()
 
         super(DummySystem, self).__init__(
             xmlstore=xmlstore,
             course_id=course_id,
             course_dir=course_dir,
             error_tracker=error_tracker,
-            parent_tracker=parent_tracker,
             load_error_modules=load_error_modules,
             mixins=(InheritanceMixin, XModuleMixin),
             field_data=KvsFieldData(DictKeyValueStore()),

common/lib/xmodule/xmodule/x_module.py

@@ -918,7 +918,8 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
 
     # =============================== BUILTIN METHODS ==========================
     def __eq__(self, other):
-        return (self.scope_ids == other.scope_ids and
+        return (hasattr(other, 'scope_ids') and
+                self.scope_ids == other.scope_ids and
                 self.fields.keys() == other.fields.keys() and
                 all(getattr(self, field.name) == getattr(other, field.name)
                     for field in self.fields.values()))

lms/djangoapps/courseware/tests/test_lti_integration.py

@@ -163,7 +163,7 @@ class TestLTIModuleListing(ModuleStoreTestCase):
             parent_location=self.section2.location,
             display_name="lti draft",
             category="lti",
-            location=self.course.id.make_usage_key('lti', 'lti_published'),
+            location=self.course.id.make_usage_key('lti', 'lti_draft'),
             publish_item=False,
         )
 
@@ -199,7 +199,7 @@ class TestLTIModuleListing(ModuleStoreTestCase):
             "lti_1_1_result_service_xml_endpoint": self.expected_handler_url('grade_handler'),
             "lti_2_0_result_service_json_endpoint":
             self.expected_handler_url('lti_2_0_result_rest_handler') + "/user/{anon_user_id}",
-            "display_name": self.lti_draft.display_name
+            "display_name": self.lti_published.display_name,
         }
         self.assertEqual([expected], json.loads(response.content))
 

lms/djangoapps/courseware/tests/test_submitting_problems.py

@@ -1159,11 +1159,11 @@ class TestConditionalContent(TestSubmittingProblems):
         vertical_0, vertical_1 = self.split_setup(user_partition_group)
 
         # Group 0 will have 2 problems in the section, worth a total of 4 points.
-        self.add_dropdown_to_section(vertical_0.location, 'H2P1', 1).location.html_id()
-        self.add_dropdown_to_section(vertical_0.location, 'H2P2', 3).location.html_id()
+        self.add_dropdown_to_section(vertical_0.location, 'H2P1_GROUP0', 1).location.html_id()
+        self.add_dropdown_to_section(vertical_0.location, 'H2P2_GROUP0', 3).location.html_id()
 
         # Group 1 will have 1 problem in the section, worth a total of 1 point.
-        self.add_dropdown_to_section(vertical_1.location, 'H2P1', 1).location.html_id()
+        self.add_dropdown_to_section(vertical_1.location, 'H2P1_GROUP1', 1).location.html_id()
 
         # Submit answers for problem in Section 1, which is visible to all students.
         self.submit_question_answer('H1P1', {'2_1': 'Correct', '2_2': 'Incorrect'})
@@ -1175,8 +1175,8 @@ class TestConditionalContent(TestSubmittingProblems):
         """
         self.split_different_problems_setup(self.user_partition_group_0)
 
-        self.submit_question_answer('H2P1', {'2_1': 'Correct'})
-        self.submit_question_answer('H2P2', {'2_1': 'Correct', '2_2': 'Incorrect', '2_3': 'Correct'})
+        self.submit_question_answer('H2P1_GROUP0', {'2_1': 'Correct'})
+        self.submit_question_answer('H2P2_GROUP0', {'2_1': 'Correct', '2_2': 'Incorrect', '2_3': 'Correct'})
 
         self.assertEqual(self.score_for_hw('homework1'), [1.0])
         self.assertEqual(self.score_for_hw('homework2'), [1.0, 2.0])
@@ -1194,7 +1194,7 @@ class TestConditionalContent(TestSubmittingProblems):
         """
         self.split_different_problems_setup(self.user_partition_group_1)
 
-        self.submit_question_answer('H2P1', {'2_1': 'Correct'})
+        self.submit_question_answer('H2P1_GROUP1', {'2_1': 'Correct'})
 
         self.assertEqual(self.score_for_hw('homework1'), [1.0])
         self.assertEqual(self.score_for_hw('homework2'), [1.0])
@@ -1219,7 +1219,7 @@ class TestConditionalContent(TestSubmittingProblems):
         [_, vertical_1] = self.split_setup(user_partition_group)
 
         # Group 1 will have 1 problem in the section, worth a total of 1 point.
-        self.add_dropdown_to_section(vertical_1.location, 'H2P1', 1).location.html_id()
+        self.add_dropdown_to_section(vertical_1.location, 'H2P1_GROUP1', 1).location.html_id()
 
         self.submit_question_answer('H1P1', {'2_1': 'Correct'})
 
@@ -1244,7 +1244,7 @@ class TestConditionalContent(TestSubmittingProblems):
         """
         self.split_one_group_no_problems_setup(self.user_partition_group_1)
 
-        self.submit_question_answer('H2P1', {'2_1': 'Correct'})
+        self.submit_question_answer('H2P1_GROUP1', {'2_1': 'Correct'})
 
         self.assertEqual(self.score_for_hw('homework1'), [1.0])
         self.assertEqual(self.score_for_hw('homework2'), [1.0])

lms/djangoapps/instructor/tests/test_tools.py

@@ -119,7 +119,8 @@ class TestFindUnit(ModuleStoreTestCase):
         Test finding a nested unit.
         """
         url = self.homework.location.to_deprecated_string()
-        self.assertEqual(tools.find_unit(self.course, url), self.homework)
+        found_unit = tools.find_unit(self.course, url)
+        self.assertEqual(found_unit.location, self.homework.location)
 
     def test_find_unit_notfound(self):
         """

lms/lib/xblock/test/test_mixin.py

 """
 Tests of the LMS XBlock Mixin
 """
+import ddt
+from django.conf import settings
 
 from xblock.validation import ValidationMessage
+from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.modulestore_settings import update_module_store_settings
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.partitions.partitions import Group, UserPartition
@@ -13,9 +17,11 @@ class LmsXBlockMixinTestCase(ModuleStoreTestCase):
     Base class for XBlock mixin tests cases. A simple course with a single user partition is created
     in setUp for all subclasses to use.
     """
-
-    def setUp(self):
-        super(LmsXBlockMixinTestCase, self).setUp()
+    def build_course(self):
+        """
+        Build up a course tree with a UserPartition.
+        """
+        # pylint: disable=attribute-defined-outside-init
         self.user_partition = UserPartition(
             0,
             'first_partition',
@@ -31,13 +37,16 @@ class LmsXBlockMixinTestCase(ModuleStoreTestCase):
         self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section')
         self.subsection = ItemFactory.create(parent=self.section, category='sequential', display_name='Test Subsection')
         self.vertical = ItemFactory.create(parent=self.subsection, category='vertical', display_name='Test Unit')
-        self.video = ItemFactory.create(parent=self.subsection, category='video', display_name='Test Video')
+        self.video = ItemFactory.create(parent=self.vertical, category='video', display_name='Test Video 1')
 
 
 class XBlockValidationTest(LmsXBlockMixinTestCase):
     """
     Unit tests for XBlock validation
     """
+    def setUp(self):
+        super(XBlockValidationTest, self).setUp()
+        self.build_course()
 
     def verify_validation_message(self, message, expected_message, expected_message_type):
         """
@@ -92,6 +101,9 @@ class XBlockGroupAccessTest(LmsXBlockMixinTestCase):
     """
     Unit tests for XBlock group access.
     """
+    def setUp(self):
+        super(XBlockGroupAccessTest, self).setUp()
+        self.build_course()
 
     def test_is_visible_to_group(self):
         """
@@ -143,3 +155,90 @@ class OpenAssessmentBlockMixinTestCase(ModuleStoreTestCase):
         Test has_score is true for ora2 problems.
         """
         self.assertTrue(self.open_assessment.has_score)
+
+
+@ddt.ddt
+class XBlockGetParentTest(LmsXBlockMixinTestCase):
+    """
+    Test that XBlock.get_parent returns correct results with each modulestore
+    backend.
+    """
+    def _pre_setup(self):
+        # load the one xml course into the xml store
+        update_module_store_settings(
+            settings.MODULESTORE,
+            mappings={'edX/toy/2012_Fall': ModuleStoreEnum.Type.xml},
+            xml_store_options={
+                'data_dir': settings.COMMON_TEST_DATA_ROOT  # where toy course lives
+            },
+        )
+        super(XBlockGetParentTest, self)._pre_setup()
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.xml)
+    def test_parents(self, modulestore_type):
+        with self.store.default_store(modulestore_type):
+
+            # setting up our own local course tree here, since it needs to be
+            # created with the correct modulestore type.
+
+            if modulestore_type == 'xml':
+                course_key = self.store.make_course_key('edX', 'toy', '2012_Fall')
+            else:
+                course_key = self.create_toy_course('edX', 'toy', '2012_Fall_copy')
+            course = self.store.get_course(course_key)
+
+            self.assertIsNone(course.get_parent())
+
+            def recurse(parent):
+                """
+                Descend the course tree and ensure the result of get_parent()
+                is the expected one.
+                """
+                visited = []
+                for child in parent.get_children():
+                    self.assertEqual(parent.location, child.get_parent().location)
+                    visited.append(child)
+                    visited += recurse(child)
+                return visited
+
+            visited = recurse(course)
+            self.assertEqual(len(visited), 28)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_parents_draft_content(self, modulestore_type):
+        # move the video to the new vertical
+        with self.store.default_store(modulestore_type):
+            self.build_course()
+            new_vertical = ItemFactory.create(parent=self.subsection, category='vertical', display_name='New Test Unit')
+            child_to_move_location = self.video.location.for_branch(None)
+            new_parent_location = new_vertical.location.for_branch(None)
+            old_parent_location = self.vertical.location.for_branch(None)
+
+            with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
+                self.assertIsNone(self.course.get_parent())
+
+                with self.store.bulk_operations(self.course.id):
+                    user_id = ModuleStoreEnum.UserID.test
+
+                    old_parent = self.store.get_item(old_parent_location)
+                    old_parent.children.remove(child_to_move_location)
+                    self.store.update_item(old_parent, user_id)
+
+                    new_parent = self.store.get_item(new_parent_location)
+                    new_parent.children.append(child_to_move_location)
+                    self.store.update_item(new_parent, user_id)
+
+                    # re-fetch video from draft store
+                    video = self.store.get_item(child_to_move_location)
+
+                    self.assertEqual(
+                        new_parent_location,
+                        video.get_parent().location
+                    )
+            with self.store.branch_setting(ModuleStoreEnum.Branch.published_only):
+                # re-fetch video from published store
+                video = self.store.get_item(child_to_move_location)
+                self.assertEqual(
+                    old_parent_location,
+                    video.get_parent().location.for_branch(None)
+                )