diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index 2a99156d9c..83590847a7 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 9027f7290c..cad0e2a918 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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( diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 9685a5eb3e..569f606fe6 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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