add ability to delete published orphans from courses (PLAT-832)

fix quality violations

create child parent mapping to avoid potential performance hit when deleting items

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

@@ -3,6 +3,8 @@
 import ddt
 from django.core.management import call_command
 from contentstore.tests.test_orphan import TestOrphanBase
+
+from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore import ModuleStoreEnum
 
 
@@ -42,3 +44,68 @@ class TestDeleteOrphan(TestOrphanBase):
         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')))
+
+    def test_delete_orphans_published_branch_split(self):
+        """
+        Tests that if there are orphans only on the published branch,
+        running delete orphans with a course key that specifies
+        the published branch will delete the published orphan
+        """
+        course, orphan = self.create_split_course_with_published_orphan()
+        published_branch = course.id.for_branch(ModuleStoreEnum.BranchName.published)
+
+        items_in_published = self.store.get_items(published_branch)
+        items_in_draft_preferred = self.store.get_items(course.id)
+
+        # call delete orphans, specifying the published branch
+        # of the course
+        call_command('delete_orphans', unicode(published_branch), 'commit')
+
+        # now all orphans should be deleted
+        self.assertOrphanCount(course.id, 0)
+        self.assertOrphanCount(published_branch, 0)
+        self.assertNotIn(orphan, self.store.get_items(published_branch))
+        # we should have one fewer item in the published branch of the course
+        self.assertEqual(
+            len(items_in_published) - 1,
+            len(self.store.get_items(published_branch)),
+        )
+        # and the same amount of items in the draft branch of the course
+        self.assertEqual(
+            len(items_in_draft_preferred),
+            len(self.store.get_items(course.id)),
+        )
+
+    def create_split_course_with_published_orphan(self):
+        """
+        Helper to create a split course with a published orphan
+        """
+        course = CourseFactory.create(default_store=ModuleStoreEnum.Type.split)
+        # create an orphan
+        orphan = self.store.create_item(
+            self.user.id, course.id, 'html', "PublishedOnlyOrphan"
+        )
+        self.store.publish(orphan.location, self.user.id)
+
+        # grab the published branch of the course
+        published_branch = course.id.for_branch(
+            ModuleStoreEnum.BranchName.published
+        )
+
+        # assert that this orphan is present in both branches
+        self.assertOrphanCount(course.id, 1)
+        self.assertOrphanCount(published_branch, 1)
+
+        # delete this orphan from the draft branch without
+        # auto-publishing this change to the published branch
+        self.store.delete_item(
+            orphan.location, self.user.id, skip_auto_publish=True
+        )
+
+        # now there should be no orphans in the draft branch, but
+        # there should be one in published
+        self.assertOrphanCount(course.id, 0)
+        self.assertOrphanCount(published_branch, 1)
+        self.assertIn(orphan, self.store.get_items(published_branch))
+
+        return course, orphan

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

@@ -16,7 +16,7 @@ class TestFixNotFound(ModuleStoreTestCase):
         """
         The management command doesn't work on non split courses
         """
-        course = CourseFactory(default_store=ModuleStoreEnum.Type.mongo)
+        course = CourseFactory.create(default_store=ModuleStoreEnum.Type.mongo)
         with self.assertRaises(SystemExit):
             call_command("fix_not_found", unicode(course.id))
 

cms/djangoapps/contentstore/tests/test_orphan.py

@@ -60,6 +60,13 @@ class TestOrphanBase(CourseTestCase):
 
         return course
 
+    def assertOrphanCount(self, course_key, number):
+        """
+        Asserts that we have the expected count of orphans
+        for a given course_key
+        """
+        self.assertEqual(len(self.store.get_orphans(course_key)), number)
+
 
 @ddt.ddt
 class TestOrphan(TestOrphanBase):

cms/djangoapps/contentstore/views/item.py

@@ -702,10 +702,14 @@ def _delete_orphans(course_usage_key, user_id, commit=False):
     """
     store = modulestore()
     items = store.get_orphans(course_usage_key)
+    branch = course_usage_key.branch
     if commit:
         for itemloc in items:
-            # need to delete all versions
-            store.delete_item(itemloc, user_id, revision=ModuleStoreEnum.RevisionOption.all)
+            revision = ModuleStoreEnum.RevisionOption.all
+            # specify branches when deleting orphans
+            if branch == ModuleStoreEnum.BranchName.published:
+                revision = ModuleStoreEnum.RevisionOption.published_only
+            store.delete_item(itemloc, user_id, revision=revision)
     return [unicode(item) for item in items]
 
 

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_structure)
+            self._remove_subtree(BlockKey.from_usage_key(usage_locator), new_blocks)
 
             # update index if appropriate and structures
             self.update_structure(usage_locator.course_key, new_structure)
@@ -2416,30 +2416,36 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
             return result
 
-    @contract(block_key=BlockKey, structure='dict')
-    def _remove_subtree(self, block_key, structure):
+    @contract(root_block_key=BlockKey, blocks='dict(BlockKey: BlockData)')
+    def _remove_subtree(self, root_block_key, blocks):
         """
-        Remove the subtree rooted at block_key
+        Remove the subtree rooted at root_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}
+        # create mapping from each child's key to its parents' keys
+        child_parent_map = defaultdict(set)
+        for block_key, block_data in blocks.iteritems():
+            for child in block_data.fields.get('children', []):
+                child_parent_map[BlockKey(*child)].add(block_key)
+
+        to_delete = {root_block_key}
+        tier = {root_block_key}
         while tier:
-            new_tier = set()
+            next_tier = set()
             for block_key in tier:
-                for child in structure['blocks'][block_key].fields.get('children', []):
+                for child in blocks[block_key].fields.get('children', []):
                     child_block_key = BlockKey(*child)
-                    parents = self._get_parents_from_structure(child_block_key, structure)
+                    parents = child_parent_map[child_block_key]
                     # 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
+                    if parents.issubset(to_delete):
+                        next_tier.add(child_block_key)
+            tier = next_tier
             to_delete.update(tier)
 
         for block_key in to_delete:
-            del structure['blocks'][block_key]
+            del blocks[block_key]
 
     def delete_course(self, course_key, user_id):
         """

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

@@ -175,7 +175,7 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
             self._auto_publish_no_children(parent_usage_key, item.location.category, user_id, **kwargs)
             return item
 
-    def delete_item(self, location, user_id, revision=None, **kwargs):
+    def delete_item(self, location, user_id, revision=None, skip_auto_publish=False, **kwargs):
         """
         Delete the given item from persistence. kwargs allow modulestore specific parameters.
 
@@ -217,7 +217,8 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
                 if (
                         branch == ModuleStoreEnum.BranchName.draft and
                         branched_location.block_type in (DIRECT_ONLY_CATEGORIES + ['vertical']) and
-                        parent_loc
+                        parent_loc and
+                        not skip_auto_publish
                 ):
                     # will publish if its not an orphan
                     self.publish(parent_loc.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
@@ -410,7 +411,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)
+            self._remove_subtree(BlockKey.from_usage_key(location), new_structure['blocks'])
 
             # copy over the block and its descendants from the published branch
             def copy_from_published(root_block_id):