From 7837607ca902a05cd0bd5f6a4f072c5d7263b15a Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Thu, 11 Dec 2014 12:48:35 -0500 Subject: [PATCH] only recusively delete modules if all their parents are marked for deletion (PLAT-333) update '_delete_item' method for draft only modules and update delete_orphans management command fix test_courseware which fails due to invalid future date --- .../management/commands/delete_orphans.py | 42 +++++++++ .../commands/tests/test_delete_orphans.py | 40 +++++++++ .../contentstore/tests/test_orphan.py | 85 +++++++++++-------- cms/djangoapps/contentstore/views/item.py | 23 +++-- .../xmodule/modulestore/mongo/draft.py | 25 ++++-- .../tests/test_mixed_modulestore.py | 2 +- 6 files changed, 170 insertions(+), 47 deletions(-) create mode 100644 cms/djangoapps/contentstore/management/commands/delete_orphans.py create mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py diff --git a/cms/djangoapps/contentstore/management/commands/delete_orphans.py b/cms/djangoapps/contentstore/management/commands/delete_orphans.py new file mode 100644 index 0000000000..202f8c2057 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/delete_orphans.py @@ -0,0 +1,42 @@ +"""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: + : 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: |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) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py b/cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py new file mode 100644 index 0000000000..649c4e7fb0 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py @@ -0,0 +1,40 @@ +"""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'))) diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index 1b0c2f3281..4dede3f71c 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -8,47 +8,60 @@ from xmodule.modulestore.django import modulestore from contentstore.utils import reverse_course_url -class TestOrphan(CourseTestCase): +class TestOrphanBase(CourseTestCase): + """ + Base class for Studio tests that require orphaned modules + """ + def setUp(self): + super(TestOrphanBase, self).setUp() + + # 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) + + chapter2 = self.store.create_child(self.user.id, self.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") + self.store.publish(orphan_chapter.location, 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() - - runtime = self.course.runtime - - 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', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', runtime) - self._create_item('html', 'OrphanHtml', "

Hello

", {'display_name': 'Orphan html'}, None, None, runtime) - self._create_item('static_tab', 'staticuno', "

tab

", {'display_name': 'Tab uno'}, None, None, runtime) - self._create_item('about', 'overview', "

overview

", {}, None, None, runtime) - self._create_item('course_info', 'updates', "

  1. Sep 22

    test

", {}, None, None, runtime) - self.orphan_url = reverse_course_url('orphan_handler', self.course.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) - def test_mongo_orphan(self): """ Test that old mongo finds the orphans @@ -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 diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 28c9a5e480..2e6528ed0d 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 4f2c1360df..76ce0a6db4 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/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 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 0bdef107b5..2b70cb7584 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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): """