Merge pull request #6453 from edx/adam/fix-delete-orphans-handler

only recusively delete modules if all their parents are marked for delet...

cms/djangoapps/contentstore/management/commands/delete_orphans.py

+"""Script for deleting orphans"""
+from django.core.management.base import BaseCommand, CommandError
+from contentstore.views.item import _delete_orphans
+from opaque_keys.edx.keys import CourseKey
+from opaque_keys import InvalidKeyError
+from xmodule.modulestore import ModuleStoreEnum
+
+
+class Command(BaseCommand):
+    """Command for deleting orphans"""
+    help = '''
+    Delete orphans from a MongoDB backed course. Takes two arguments:
+    <course_id>: the course id of the course whose orphans you want to delete
+    |commit|: optional argument. If not provided, will not run task.
+    '''
+
+    def handle(self, *args, **options):
+        if len(args) not in {1, 2}:
+            raise CommandError("delete_orphans requires one or more arguments: <course_id> |commit|")
+
+        try:
+            course_key = CourseKey.from_string(args[0])
+        except InvalidKeyError:
+            raise CommandError("Invalid course key.")
+
+        commit = False
+        if len(args) == 2:
+            commit = args[1] == 'commit'
+
+        if commit:
+            print 'Deleting orphans from the course:'
+            deleted_items = _delete_orphans(
+                course_key, ModuleStoreEnum.UserID.mgmt_command, commit
+            )
+            print "Success! Deleted the following orphans from the course:"
+            print "\n".join(deleted_items)
+        else:
+            print 'Dry run. The following orphans would have been deleted from the course:'
+            deleted_items = _delete_orphans(
+                course_key, ModuleStoreEnum.UserID.mgmt_command, commit
+            )
+            print "\n".join(deleted_items)

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

+"""Tests running the delete_orphan command"""
+
+from django.core.management import call_command
+from contentstore.tests.test_orphan import TestOrphanBase
+
+
+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):
+        """
+        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')))
+
+    def test_delete_orphans_commit(self):
+        """
+        Tests that running the command WITH the 'commit' argument
+        results in the orphans being deleted
+        """
+        call_command('delete_orphans', self.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')))
+
+        # 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')))

cms/djangoapps/contentstore/tests/test_orphan.py

@@ -8,46 +8,59 @@ from xmodule.modulestore.django import modulestore
 from contentstore.utils import reverse_course_url
 
 
-class TestOrphan(CourseTestCase):
+class TestOrphanBase(CourseTestCase):
     """
-    Test finding orphans via view and django config
+    Base class for Studio tests that require orphaned modules
     """
     def setUp(self):
-        super(TestOrphan, self).setUp()
+        super(TestOrphanBase, self).setUp()
 
-        runtime = self.course.runtime
+        # create chapters and add them to course tree
+        chapter1 = self.store.create_child(self.user.id, self.course.location, 'chapter', "Chapter1")
+        self.store.publish(chapter1.location, self.user.id)
 
-        self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', self.course.location.name, runtime)
-        self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', self.course.location.name, runtime)
-        self._create_item('chapter', 'OrphanChapter', {}, {'display_name': 'Orphan Chapter'}, None, None, runtime)
-        self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', runtime)
-        self._create_item('vertical', 'OrphanVert', {}, {'display_name': 'Orphan Vertical'}, None, None, runtime)
-        self._create_item('html', 'Html1', "<p>Goodbye</p>", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', runtime)
-        self._create_item('html', 'OrphanHtml', "<p>Hello</p>", {'display_name': 'Orphan html'}, None, None, runtime)
-        self._create_item('static_tab', 'staticuno', "<p>tab</p>", {'display_name': 'Tab uno'}, None, None, runtime)
-        self._create_item('about', 'overview', "<p>overview</p>", {}, None, None, runtime)
-        self._create_item('course_info', 'updates', "<ol><li><h2>Sep 22</h2><p>test</p></li></ol>", {}, None, None, runtime)
+        chapter2 = self.store.create_child(self.user.id, self.course.location, 'chapter', "Chapter2")
+        self.store.publish(chapter2.location, self.user.id)
 
-        self.orphan_url = reverse_course_url('orphan_handler', self.course.id)
+        # orphan chapter
+        orphan_chapter = self.store.create_item(self.user.id, self.course.id, 'chapter', "OrphanChapter")
+        self.store.publish(orphan_chapter.location, self.user.id)
 
-    def _create_item(self, category, name, data, metadata, parent_category, parent_name, runtime):
-        location = self.course.location.replace(category=category, name=name)
-        store = modulestore()
-        store.create_item(
-            self.user.id,
-            location.course_key,
-            location.block_type,
-            location.block_id,
-            definition_data=data,
-            metadata=metadata,
-            runtime=runtime
-        )
-        if parent_name:
-            # add child to parent in mongo
-            parent_location = self.course.location.replace(category=parent_category, name=parent_name)
-            parent = store.get_item(parent_location)
-            parent.children.append(location)
-            store.update_item(parent, self.user.id)
+        # create vertical and add it as child to chapter1
+        vertical1 = self.store.create_child(self.user.id, chapter1.location, 'vertical', "Vertical1")
+        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")
+        self.store.publish(orphan_vertical.location, self.user.id)
+
+        # create component and add it to vertical1
+        html1 = self.store.create_child(self.user.id, vertical1.location, 'html', "Html1")
+        self.store.publish(html1.location, self.user.id)
+
+        # create component and add it as a child to vertical1 and orphan_vertical
+        multi_parent_html = self.store.create_child(self.user.id, vertical1.location, 'html', "multi_parent_html")
+        self.store.publish(multi_parent_html.location, self.user.id)
+
+        orphan_vertical.children.append(multi_parent_html.location)
+        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")
+        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")
+
+
+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):
         """
@@ -77,6 +90,10 @@ class TestOrphan(CourseTestCase):
         )
         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")))
+
     def test_not_permitted(self):
         """
         Test that auth restricts get and delete appropriately

cms/djangoapps/contentstore/views/item.py

@@ -604,16 +604,27 @@ def orphan_handler(request, course_key_string):
             raise PermissionDenied()
     if request.method == 'DELETE':
         if request.user.is_staff:
-            store = modulestore()
-            items = store.get_orphans(course_usage_key)
-            for itemloc in items:
-                # need to delete all versions
-                store.delete_item(itemloc, request.user.id, revision=ModuleStoreEnum.RevisionOption.all)
-            return JsonResponse({'deleted': [unicode(item) for item in items]})
+            deleted_items = _delete_orphans(course_usage_key, request.user.id, commit=True)
+            return JsonResponse({'deleted': deleted_items})
         else:
             raise PermissionDenied()
 
 
+def _delete_orphans(course_usage_key, user_id, commit=False):
+    """
+    Helper function to delete orphans for a given course.
+    If `commit` is False, this function does not actually remove
+    the orphans.
+    """
+    store = modulestore()
+    items = store.get_orphans(course_usage_key)
+    if commit:
+        for itemloc in items:
+            # need to delete all versions
+            store.delete_item(itemloc, user_id, revision=ModuleStoreEnum.RevisionOption.all)
+    return [unicode(item) for item in items]
+
+
 def _get_xblock(usage_key, user):
     """
     Returns the xblock for the specified usage key. Note: if failing to find a key with a category

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

@@ -541,7 +541,7 @@ class DraftModuleStore(MongoModuleStore):
             )
         self._delete_subtree(location, as_functions)
 
-    def _delete_subtree(self, location, as_functions):
+    def _delete_subtree(self, location, as_functions, draft_only=False):
         """
         Internal method for deleting all of the subtree whose revisions match the as_functions
         """
@@ -555,10 +555,23 @@ class DraftModuleStore(MongoModuleStore):
             next_tier = []
             for child_loc in current_entry.get('definition', {}).get('children', []):
                 child_loc = course_key.make_usage_key_from_deprecated_string(child_loc)
-                for rev_func in as_functions:
-                    current_loc = rev_func(child_loc)
-                    current_son = current_loc.to_deprecated_son()
-                    next_tier.append(current_son)
+
+                # single parent can have 2 versions: draft and published
+                # get draft parents only while deleting draft module
+                if draft_only:
+                    revision = MongoRevisionKey.draft
+                else:
+                    revision = ModuleStoreEnum.RevisionOption.all
+
+                parents = self._get_raw_parent_locations(child_loc, revision)
+                # Don't delete modules if one of its parents shouldn't be deleted
+                # This should only be an issue for courses have ended up in
+                # a state where modules have multiple parents
+                if all(parent.to_deprecated_son() in to_be_deleted for parent in parents):
+                    for rev_func in as_functions:
+                        current_loc = rev_func(child_loc)
+                        current_son = current_loc.to_deprecated_son()
+                        next_tier.append(current_son)
 
             return next_tier
 
@@ -738,7 +751,7 @@ class DraftModuleStore(MongoModuleStore):
             # If 2 versions versions exist, we can assume one is a published version. Go ahead and do the delete
             # of the draft version.
             if versions_found.count() > 1:
-                self._delete_subtree(root_location, [as_draft])
+                self._delete_subtree(root_location, [as_draft], draft_only=True)
             elif versions_found.count() == 1:
                 # Since this method cannot be called on something in DIRECT_ONLY_CATEGORIES and we call
                 # delete_subtree as soon as we find an item with a draft version, if there is only 1 version

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

@@ -723,7 +723,7 @@ class TestMixedModuleStore(CourseComparisonTest):
     # Split:
     #    queries: active_versions, draft and published structures, definition (unnecessary)
     #    sends: update published (why?), draft, and active_versions
-    @ddt.data(('draft', 8, 2), ('split', 2, 2))
+    @ddt.data(('draft', 9, 2), ('split', 2, 2))
     @ddt.unpack
     def test_delete_private_vertical(self, default_ms, max_find, max_send):
         """