Merge pull request #4805 from edx/zub/bugfix/std2007-jumptoid

update get_parent_location method to select non-orphan parents from mult...

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

@@ -1228,6 +1228,38 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                     jsonfields[field_name] = field.read_json(xblock)
         return jsonfields
 
+    def _get_non_orphan_parents(self, location, parents, revision):
+        """
+        Extract non orphan parents by traversing the list of possible parents and remove current location
+        from orphan parents to avoid parents calculation overhead next time.
+        """
+        non_orphan_parents = []
+        for parent in parents:
+            parent_loc = Location._from_deprecated_son(parent['_id'], location.course_key.run)
+
+            # travel up the tree for orphan validation
+            ancestor_loc = parent_loc
+            while ancestor_loc is not None:
+                current_loc = ancestor_loc
+                ancestor_loc = self._get_raw_parent_location(current_loc, revision)
+                if ancestor_loc is None:
+                    # The parent is an orphan, so remove all the children including
+                    # the location whose parent we are looking for from orphan parent
+                    self.collection.update(
+                        {'_id': parent_loc.to_deprecated_son()},
+                        {'$set': {'definition.children': []}},
+                        multi=False,
+                        upsert=True,
+                        safe=self.collection.safe
+                    )
+                elif ancestor_loc.category == 'course':
+                    # once we reach the top location of the tree and if the location is not an orphan then the
+                    # parent is not an orphan either
+                    non_orphan_parents.append(parent_loc)
+                    break
+
+        return non_orphan_parents
+
     def _get_raw_parent_location(self, location, revision=ModuleStoreEnum.RevisionOption.published_only):
         '''
         Helper for get_parent_location that finds the location that is the parent of this location in this course,
@@ -1254,10 +1286,18 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
 
         if revision == ModuleStoreEnum.RevisionOption.published_only:
             if parents.count() > 1:
-                # should never have multiple PUBLISHED parents
-                raise ReferentialIntegrityError(
-                    u"{} parents claim {}".format(parents.count(), location)
-                )
+                non_orphan_parents = self._get_non_orphan_parents(location, parents, revision)
+                if len(non_orphan_parents) == 0:
+                    # no actual parent found
+                    return None
+
+                if len(non_orphan_parents) > 1:
+                    # should never have multiple PUBLISHED parents
+                    raise ReferentialIntegrityError(
+                        u"{} parents claim {}".format(parents.count(), location)
+                    )
+                else:
+                    return non_orphan_parents[0]
             else:
                 # return the single PUBLISHED parent
                 return Location._from_deprecated_son(parents[0]['_id'], location.course_key.run)
@@ -1265,9 +1305,20 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             # there could be 2 different parents if
             #   (1) the draft item was moved or
             #   (2) the parent itself has 2 versions: DRAFT and PUBLISHED
+            #  if there are multiple parents with version PUBLISHED then choose from non-orphan parents
+            all_parents = []
+            published_parents = 0
+            for parent in parents:
+                if parent['_id']['revision'] is None:
+                    published_parents += 1
+                all_parents.append(parent)
 
             # since we sorted by SORT_REVISION_FAVOR_DRAFT, the 0'th parent is the one we want
-            found_id = parents[0]['_id']
+            if published_parents > 1:
+                non_orphan_parents = self._get_non_orphan_parents(location, all_parents, revision)
+                return non_orphan_parents[0]
+
+            found_id = all_parents[0]['_id']
             # don't disclose revision outside modulestore
             return Location._from_deprecated_son(found_id, location.course_key.run)
 

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

@@ -1,34 +1,32 @@
-import pymongo
-from uuid import uuid4
+import datetime
 import ddt
 import itertools
-from importlib import import_module
-from collections import namedtuple
+import pymongo
 import unittest
-import datetime
+
+from collections import namedtuple
+from importlib import import_module
 from pytz import UTC
-
-from xmodule.tests import DATA_DIR
-from xmodule.modulestore import ModuleStoreEnum
-from xmodule.modulestore.exceptions import ItemNotFoundError
-from xmodule.exceptions import InvalidVersionError
-
-from opaque_keys.edx.locations import SlashSeparatedCourseKey
-from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
+from uuid import uuid4
 
 # Mixed modulestore depends on django, so we'll manually configure some django settings
 # before importing the module
 # TODO remove this import and the configuration -- xmodule should not depend on django!
 from django.conf import settings
-from xmodule.modulestore.tests.factories import check_mongo_calls
-from xmodule.modulestore.search import path_to_location
-from xmodule.modulestore.exceptions import DuplicateCourseError, NoPathToItem
-
 if not settings.configured:
     settings.configure()
-from xmodule.modulestore.mixed import MixedModuleStore
+
+from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
+from xmodule.exceptions import InvalidVersionError
+from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.draft_and_published import UnsupportedRevisionError
+from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError, NoPathToItem
+from xmodule.modulestore.mixed import MixedModuleStore
+from xmodule.modulestore.search import path_to_location
+from xmodule.modulestore.tests.factories import check_mongo_calls
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
+from xmodule.tests import DATA_DIR
 
 
 @ddt.ddt
@@ -909,6 +907,68 @@ class TestMixedModuleStore(unittest.TestCase):
             found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key)
         self.assertItemsEqual(found_orphans, orphan_locations)
 
+    @ddt.data('draft')
+    def test_get_non_orphan_parents(self, default_ms):
+        """
+        Test finding non orphan parents from many possible parents.
+        """
+        self.initdb(default_ms)
+        course_id = self.course_locations[self.MONGO_COURSEID].course_key
+
+        # create parented children
+        self._create_block_hierarchy()
+        self.store.publish(self.course.location, self.user_id)
+
+        # test that problem "problem_x1a_1" has only one published parent
+        mongo_store = self.store._get_modulestore_for_courseid(course_id)
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
+            parent = mongo_store.get_parent_location(self.problem_x1a_1)
+            self.assertEqual(parent, self.vertical_x1a)
+
+        # add some published orphans
+        orphan_sequential = course_id.make_usage_key('sequential', 'OrphanSequential')
+        orphan_vertical = course_id.make_usage_key('vertical', 'OrphanVertical')
+        orphan_locations = [orphan_sequential, orphan_vertical]
+        for location in orphan_locations:
+            self.store.create_item(
+                self.user_id,
+                location.course_key,
+                location.block_type,
+                block_id=location.block_id
+            )
+            self.store.publish(location, self.user_id)
+
+        found_orphans = mongo_store.get_orphans(course_id)
+        self.assertEqual(set(found_orphans), set(orphan_locations))
+        self.assertEqual(len(set(found_orphans)), 2)
+
+        # add orphan vertical and sequential as another parents of problem "problem_x1a_1"
+        mongo_store.collection.update(
+            orphan_sequential.to_deprecated_son('_id.'),
+            {'$push': {'definition.children': unicode(self.problem_x1a_1)}}
+        )
+        mongo_store.collection.update(
+            orphan_vertical.to_deprecated_son('_id.'),
+            {'$push': {'definition.children': unicode(self.problem_x1a_1)}}
+        )
+        # test that "get_parent_location" method of published branch still returns the correct non-orphan parent for
+        # problem "problem_x1a_1" since the two other parents are orphans
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
+            parent = mongo_store.get_parent_location(self.problem_x1a_1)
+            self.assertEqual(parent, self.vertical_x1a)
+
+        # now add valid published vertical as another parent of problem
+        mongo_store.collection.update(
+            self.sequential_x1.to_deprecated_son('_id.'),
+            {'$push': {'definition.children': unicode(self.problem_x1a_1)}}
+        )
+        # now check that "get_parent_location" method of published branch raises "ReferentialIntegrityError" for
+        # problem "problem_x1a_1" since it has now 2 valid published parents
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
+            self.assertTrue(self.store.has_item(self.problem_x1a_1))
+            with self.assertRaises(ReferentialIntegrityError):
+                self.store.get_parent_location(self.problem_x1a_1)
+
     @ddt.data('draft')
     def test_create_item_from_parent_location(self, default_ms):
         """