From 8aa23de55a7f363ab010509c7f0be5f98ccf603b Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Fri, 11 Sep 2015 12:54:58 -0400 Subject: [PATCH] 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 --- .../commands/tests/test_delete_orphans.py | 67 +++++++++++++++++++ .../commands/tests/test_fix_not_found.py | 2 +- .../contentstore/tests/test_orphan.py | 7 ++ cms/djangoapps/contentstore/views/item.py | 8 ++- .../xmodule/modulestore/split_mongo/split.py | 32 +++++---- .../modulestore/split_mongo/split_draft.py | 7 +- 6 files changed, 104 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py b/cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py index 7bd4ed4a6f..9f38a85438 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_delete_orphans.py +++ b/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 diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_fix_not_found.py b/cms/djangoapps/contentstore/management/commands/tests/test_fix_not_found.py index bd519a4167..ceff802d46 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_fix_not_found.py +++ b/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)) diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index 69d7be38b7..2a99156d9c 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/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): diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index debc327ab5..d1283ece1b 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/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] diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 0393049555..9fed0c0f9e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 669eb2b0f6..4dcb0f4cff 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/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):