Merge pull request #9862 from edx/aj/plat833-orphan-problem-with-preview-button

Orphans in split breaks preview button in Studio

cms/djangoapps/contentstore/tests/test_orphan.py

@@ -3,10 +3,13 @@ Test finding orphans via the view and django config
 """
 import json
 import ddt
+
 from contentstore.tests.utils import CourseTestCase
-from student.models import CourseEnrollment
 from contentstore.utils import reverse_course_url
+from opaque_keys.edx.locator import BlockUsageLocator
+from student.models import CourseEnrollment
 from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.search import path_to_location
 from xmodule.modulestore.tests.factories import CourseFactory
 
 
@@ -128,3 +131,113 @@ class TestOrphan(TestOrphanBase):
         self.assertEqual(response.status_code, 403)
         response = test_user_client.delete(orphan_url)
         self.assertEqual(response.status_code, 403)
+
+    @ddt.data(ModuleStoreEnum.Type.split)
+    def test_path_to_location_for_orphan_vertical(self, module_store):
+        r"""
+        Make sure that path_to_location works with a component having multiple vertical parents,
+        from which one of them is orphan.
+
+         course
+            |
+         chapter
+           |
+         vertical vertical
+            \     /
+              html
+        """
+        # Get a course with orphan modules
+        course = self.create_course_with_orphans(module_store)
+
+        # Fetch the required course components.
+        vertical1 = self.store.get_item(BlockUsageLocator(course.id, 'vertical', 'Vertical1'))
+        chapter1 = self.store.get_item(BlockUsageLocator(course.id, 'chapter', 'Chapter1'))
+        orphan_vertical = self.store.get_item(BlockUsageLocator(course.id, 'vertical', 'OrphanVert'))
+        multi_parent_html = self.store.get_item(BlockUsageLocator(course.id, 'html', 'multi_parent_html'))
+
+        # Verify `OrphanVert` is an orphan
+        self.assertIn(orphan_vertical.location, self.store.get_orphans(course.id))
+
+        # Verify `multi_parent_html` is child of both `Vertical1` and `OrphanVert`
+        self.assertIn(multi_parent_html.location, orphan_vertical.children)
+        self.assertIn(multi_parent_html.location, vertical1.children)
+
+        # HTML component has `vertical1` as its parent.
+        html_parent = self.store.get_parent_location(multi_parent_html.location)
+        self.assertNotEqual(unicode(html_parent), unicode(orphan_vertical.location))
+        self.assertEqual(unicode(html_parent), unicode(vertical1.location))
+
+        # Get path of the `multi_parent_html` & verify path_to_location returns a expected path
+        path = path_to_location(self.store, multi_parent_html.location)
+        expected_path = (
+            course.id,
+            chapter1.location.block_id,
+            vertical1.location.block_id,
+            multi_parent_html.location.block_id,
+            "",
+            path[-1]
+        )
+        self.assertIsNotNone(path)
+        self.assertEqual(len(path), 6)
+        self.assertEqual(path, expected_path)
+
+    @ddt.data(ModuleStoreEnum.Type.split)
+    def test_path_to_location_for_orphan_chapter(self, module_store):
+        r"""
+        Make sure that path_to_location works with a component having multiple chapter parents,
+        from which one of them is orphan
+
+         course
+            |
+        chapter   chapter
+           |         |
+        vertical  vertical
+              \    /
+               html
+
+        """
+        # Get a course with orphan modules
+        course = self.create_course_with_orphans(module_store)
+        orphan_chapter = self.store.get_item(BlockUsageLocator(course.id, 'chapter', 'OrphanChapter'))
+        chapter1 = self.store.get_item(BlockUsageLocator(course.id, 'chapter', 'Chapter1'))
+        vertical1 = self.store.get_item(BlockUsageLocator(course.id, 'vertical', 'Vertical1'))
+
+        # Verify `OrhanChapter` is an orphan
+        self.assertIn(orphan_chapter.location, self.store.get_orphans(course.id))
+
+        # Create a vertical (`Vertical0`) in orphan chapter (`OrphanChapter`).
+        # OrphanChapter -> Vertical0
+        vertical0 = self.store.create_child(self.user.id, orphan_chapter.location, 'vertical', "Vertical0")
+        self.store.publish(vertical0.location, self.user.id)
+
+        # Create a component in `Vertical0`
+        # OrphanChapter -> Vertical0 -> Html
+        html = self.store.create_child(self.user.id, vertical0.location, 'html', "HTML0")
+        self.store.publish(html.location, self.user.id)
+
+        # Verify chapter1 is parent of vertical1.
+        vertical1_parent = self.store.get_parent_location(vertical1.location)
+        self.assertEqual(unicode(vertical1_parent), unicode(chapter1.location))
+
+        # Make `Vertical1` the parent of `HTML0`. So `HTML0` will have to parents (`Vertical0` & `Vertical1`)
+        vertical1.children.append(html.location)
+        self.store.update_item(vertical1, self.user.id)
+
+        # Get parent location & verify its either of the two verticals. As both parents are non-orphan,
+        # alphabetically least is returned
+        html_parent = self.store.get_parent_location(html.location)
+        self.assertEquals(unicode(html_parent), unicode(vertical1.location))
+
+        # verify path_to_location returns a expected path
+        path = path_to_location(self.store, html.location)
+        expected_path = (
+            course.id,
+            chapter1.location.block_id,
+            vertical1.location.block_id,
+            html.location.block_id,
+            "",
+            path[-1]
+        )
+        self.assertIsNotNone(path)
+        self.assertEqual(len(path), 6)
+        self.assertEqual(path, expected_path)

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

@@ -1121,6 +1121,23 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         else:
             return []
 
+    def has_path_to_root(self, block_key, course):
+        """
+        Check recursively if an xblock has a path to the course root
+
+        :param block_key: BlockKey of the component whose path is to be checked
+        :param course: actual db json of course from structures
+
+        :return Bool: whether or not component has path to the root
+        """
+
+        xblock_parents = self._get_parents_from_structure(block_key, course.structure)
+        if len(xblock_parents) == 0 and block_key.type in ["course", "library"]:
+            # Found, xblock has the path to the root
+            return True
+
+        return any(self.has_path_to_root(xblock_parent, course) for xblock_parent in xblock_parents)
+
     def get_parent_location(self, locator, **kwargs):
         """
         Return the location (Locators w/ block_ids) for the parent of this location in this
@@ -1134,9 +1151,19 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             raise ItemNotFoundError(locator)
 
         course = self._lookup_course(locator.course_key)
-        parent_ids = self._get_parents_from_structure(BlockKey.from_usage_key(locator), course.structure)
+        all_parent_ids = self._get_parents_from_structure(BlockKey.from_usage_key(locator), course.structure)
+
+        # Check and verify the found parent_ids are not orphans; Remove parent which has no valid path
+        # to the course root
+        parent_ids = [
+            valid_parent
+            for valid_parent in all_parent_ids
+            if self.has_path_to_root(valid_parent, course)
+        ]
+
         if len(parent_ids) == 0:
             return None
+
         # find alphabetically least
         parent_ids.sort(key=lambda parent: (parent.type, parent.id))
         return BlockUsageLocator.make_relative(

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

@@ -643,7 +643,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
         test_course = self.store.create_course('test_org', 'test_course', 'test_run', self.user_id)
 
         # create sequential and vertical to test against
-        sequential = self.store.create_item(self.user_id, test_course.id, 'sequential', 'test_sequential')
+        sequential = self.store.create_child(self.user_id, test_course.location, 'sequential', 'test_sequential')
         vertical = self.store.create_child(self.user_id, sequential.location, 'vertical', 'test_vertical')
 
         # publish sequential changes