delete_item should not delete any child that has a parent we do not intend to delete

cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py

 """Tests running the delete_orphan command"""
 
+import ddt
 from django.core.management import call_command
 from contentstore.tests.test_orphan import TestOrphanBase
+from xmodule.modulestore import ModuleStoreEnum
 
 
+@ddt.ddt
 class TestDeleteOrphan(TestOrphanBase):
     """
     Tests for running the delete_orphan management command.
     Inherits from TestOrphan in order to use its setUp method.
     """
-    def setUp(self):
-        super(TestDeleteOrphan, self).setUp()
-        self.course_id = self.course.id.to_deprecated_string()
-
-    def test_delete_orphans_no_commit(self):
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_delete_orphans_no_commit(self, default_store):
         """
         Tests that running the command without a 'commit' argument
         results in no orphans being deleted
         """
-        call_command('delete_orphans', self.course_id)
-        self.assertTrue(self.store.has_item(self.course.id.make_usage_key('html', 'multi_parent_html')))
-        self.assertTrue(self.store.has_item(self.course.id.make_usage_key('vertical', 'OrphanVert')))
-        self.assertTrue(self.store.has_item(self.course.id.make_usage_key('chapter', 'OrphanChapter')))
-        self.assertTrue(self.store.has_item(self.course.id.make_usage_key('html', 'OrphanHtml')))
+        course = self.create_course_with_orphans(default_store)
+        call_command('delete_orphans', unicode(course.id))
+        self.assertTrue(self.store.has_item(course.id.make_usage_key('html', 'multi_parent_html')))
+        self.assertTrue(self.store.has_item(course.id.make_usage_key('vertical', 'OrphanVert')))
+        self.assertTrue(self.store.has_item(course.id.make_usage_key('chapter', 'OrphanChapter')))
+        self.assertTrue(self.store.has_item(course.id.make_usage_key('html', 'OrphanHtml')))
 
-    def test_delete_orphans_commit(self):
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_delete_orphans_commit(self, default_store):
         """
         Tests that running the command WITH the 'commit' argument
         results in the orphans being deleted
         """
-        call_command('delete_orphans', self.course_id, 'commit')
+        course = self.create_course_with_orphans(default_store)
+
+        call_command('delete_orphans', unicode(course.id), 'commit')
 
         # make sure this module wasn't deleted
-        self.assertTrue(self.store.has_item(self.course.id.make_usage_key('html', 'multi_parent_html')))
+        self.assertTrue(self.store.has_item(course.id.make_usage_key('html', 'multi_parent_html')))
 
         # and make sure that these were
-        self.assertFalse(self.store.has_item(self.course.id.make_usage_key('vertical', 'OrphanVert')))
-        self.assertFalse(self.store.has_item(self.course.id.make_usage_key('chapter', 'OrphanChapter')))
-        self.assertFalse(self.store.has_item(self.course.id.make_usage_key('html', 'OrphanHtml')))
+        self.assertFalse(self.store.has_item(course.id.make_usage_key('vertical', 'OrphanVert')))
+        self.assertFalse(self.store.has_item(course.id.make_usage_key('chapter', 'OrphanChapter')))
+        self.assertFalse(self.store.has_item(course.id.make_usage_key('html', 'OrphanHtml')))

cms/djangoapps/contentstore/tests/test_orphan.py

@@ -2,27 +2,34 @@
 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 xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.tests.factories import CourseFactory
 
 
 class TestOrphanBase(CourseTestCase):
     """
     Base class for Studio tests that require orphaned modules
     """
-    def setUp(self):
-        super(TestOrphanBase, self).setUp()
+    def create_course_with_orphans(self, default_store):
+        """
+        Creates a course with 3 orphan modules, one of which
+        has a child that's also in the course tree.
+        """
+        course = CourseFactory.create(default_store=default_store)
 
         # create chapters and add them to course tree
-        chapter1 = self.store.create_child(self.user.id, self.course.location, 'chapter', "Chapter1")
+        chapter1 = self.store.create_child(self.user.id, course.location, 'chapter', "Chapter1")
         self.store.publish(chapter1.location, self.user.id)
 
-        chapter2 = self.store.create_child(self.user.id, self.course.location, 'chapter', "Chapter2")
+        chapter2 = self.store.create_child(self.user.id, course.location, 'chapter', "Chapter2")
         self.store.publish(chapter2.location, self.user.id)
 
         # orphan chapter
-        orphan_chapter = self.store.create_item(self.user.id, self.course.id, 'chapter', "OrphanChapter")
+        orphan_chapter = self.store.create_item(self.user.id, course.id, 'chapter', "OrphanChapter")
         self.store.publish(orphan_chapter.location, self.user.id)
 
         # create vertical and add it as child to chapter1
@@ -30,7 +37,7 @@ class TestOrphanBase(CourseTestCase):
         self.store.publish(vertical1.location, self.user.id)
 
         # create orphan vertical
-        orphan_vertical = self.store.create_item(self.user.id, self.course.id, 'vertical', "OrphanVert")
+        orphan_vertical = self.store.create_item(self.user.id, course.id, 'vertical', "OrphanVert")
         self.store.publish(orphan_vertical.location, self.user.id)
 
         # create component and add it to vertical1
@@ -45,61 +52,72 @@ class TestOrphanBase(CourseTestCase):
         self.store.update_item(orphan_vertical, self.user.id)
 
         # create an orphaned html module
-        orphan_html = self.store.create_item(self.user.id, self.course.id, 'html', "OrphanHtml")
+        orphan_html = self.store.create_item(self.user.id, course.id, 'html', "OrphanHtml")
         self.store.publish(orphan_html.location, self.user.id)
 
-        self.store.create_child(self.user.id, self.course.location, 'static_tab', "staticuno")
-        self.store.create_child(self.user.id, self.course.location, 'about', "overview")
-        self.store.create_child(self.user.id, self.course.location, 'course_info', "updates")
+        self.store.create_child(self.user.id, course.location, 'static_tab', "staticuno")
+        self.store.create_child(self.user.id, course.location, 'course_info', "updates")
+
+        return course
 
 
+@ddt.ddt
 class TestOrphan(TestOrphanBase):
     """
     Test finding orphans via view and django config
     """
-    def setUp(self):
-        super(TestOrphan, self).setUp()
-        self.orphan_url = reverse_course_url('orphan_handler', self.course.id)
 
-    def test_mongo_orphan(self):
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_get_orphans(self, default_store):
         """
-        Test that old mongo finds the orphans
+        Test that the orphan handler finds the orphans
         """
+        course = self.create_course_with_orphans(default_store)
+        orphan_url = reverse_course_url('orphan_handler', course.id)
+
         orphans = json.loads(
             self.client.get(
-                self.orphan_url,
+                orphan_url,
                 HTTP_ACCEPT='application/json'
             ).content
         )
         self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans))
-        location = self.course.location.replace(category='chapter', name='OrphanChapter')
+        location = course.location.replace(category='chapter', name='OrphanChapter')
         self.assertIn(location.to_deprecated_string(), orphans)
-        location = self.course.location.replace(category='vertical', name='OrphanVert')
+        location = course.location.replace(category='vertical', name='OrphanVert')
         self.assertIn(location.to_deprecated_string(), orphans)
-        location = self.course.location.replace(category='html', name='OrphanHtml')
+        location = course.location.replace(category='html', name='OrphanHtml')
         self.assertIn(location.to_deprecated_string(), orphans)
 
-    def test_mongo_orphan_delete(self):
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_delete_orphans(self, default_store):
         """
-        Test that old mongo deletes the orphans
+        Test that the orphan handler deletes the orphans
         """
-        self.client.delete(self.orphan_url)
+        course = self.create_course_with_orphans(default_store)
+        orphan_url = reverse_course_url('orphan_handler', course.id)
+
+        self.client.delete(orphan_url)
         orphans = json.loads(
-            self.client.get(self.orphan_url, HTTP_ACCEPT='application/json').content
+            self.client.get(orphan_url, HTTP_ACCEPT='application/json').content
         )
         self.assertEqual(len(orphans), 0, "Orphans not deleted {}".format(orphans))
 
         # make sure that any children with one orphan parent and one non-orphan
         # parent are not deleted
-        self.assertTrue(self.store.has_item(self.course.id.make_usage_key('html', "multi_parent_html")))
+        self.assertTrue(self.store.has_item(course.id.make_usage_key('html', "multi_parent_html")))
 
-    def test_not_permitted(self):
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_not_permitted(self, default_store):
         """
         Test that auth restricts get and delete appropriately
         """
+        course = self.create_course_with_orphans(default_store)
+        orphan_url = reverse_course_url('orphan_handler', course.id)
+
         test_user_client, test_user = self.create_non_staff_authed_user_client()
-        CourseEnrollment.enroll(test_user, self.course.id)
-        response = test_user_client.get(self.orphan_url)
+        CourseEnrollment.enroll(test_user, course.id)
+        response = test_user_client.get(orphan_url)
         self.assertEqual(response.status_code, 403)
-        response = test_user_client.delete(self.orphan_url)
+        response = test_user_client.delete(orphan_url)
         self.assertEqual(response.status_code, 403)

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

@@ -2399,7 +2399,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 parent_block.edit_info.source_version = None
                 self.decache_block(usage_locator.course_key, new_id, parent_block_key)
 
-            self._remove_subtree(BlockKey.from_usage_key(usage_locator), new_blocks)
+            self._remove_subtree(BlockKey.from_usage_key(usage_locator), new_structure)
 
             # update index if appropriate and structures
             self.update_structure(usage_locator.course_key, new_structure)
@@ -2416,14 +2416,30 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
             return result
 
-    @contract(block_key=BlockKey, blocks='dict(BlockKey: BlockData)')
-    def _remove_subtree(self, block_key, blocks):
+    @contract(block_key=BlockKey, structure='dict')
+    def _remove_subtree(self, block_key, structure):
         """
         Remove the subtree rooted at block_key
-        """
-        for child in blocks[block_key].fields.get('children', []):
-            self._remove_subtree(BlockKey(*child), blocks)
-        del blocks[block_key]
+        We do this breadth-first to make sure that we don't remove
+        any children that may have parents that we don't want to delete.
+        """
+        to_delete = {block_key}
+        tier = {block_key}
+        while tier:
+            new_tier = set()
+            for block_key in tier:
+                for child in structure['blocks'][block_key].fields.get('children', []):
+                    child_block_key = BlockKey(*child)
+                    parents = self._get_parents_from_structure(child_block_key, structure)
+                    # Make sure we want to delete all of the child's parents
+                    # before slating it for deletion
+                    if all(parent in to_delete for parent in parents):
+                        new_tier.add(child_block_key)
+            tier = new_tier
+            to_delete.update(tier)
+
+        for block_key in to_delete:
+            del structure['blocks'][block_key]
 
     def delete_course(self, course_key, user_id):
         """

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

@@ -410,7 +410,7 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
             new_structure = self.version_structure(draft_course_key, draft_course_structure, user_id)
 
             # remove the block and its descendants from the new structure
-            self._remove_subtree(BlockKey.from_usage_key(location), new_structure['blocks'])
+            self._remove_subtree(BlockKey.from_usage_key(location), new_structure)
 
             # copy over the block and its descendants from the published branch
             def copy_from_published(root_block_id):