diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 4d1d4037f8..4092306e9e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -472,7 +472,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): def revert_to_published(self, location, user_id): """ Reverts an item to its last published version (recursively traversing all of its descendants). - If no published version exists, a VersionConflictError is thrown. + If no published version exists, an InvalidVersionError is thrown. If a published version exists but there is no draft version of this item or any of its descendants, this method is a no-op. diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 08f3aa30ad..95882cdb3b 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -693,7 +693,7 @@ class DraftModuleStore(MongoModuleStore): def revert_to_published(self, location, user_id=None): """ Reverts an item to its last published version (recursively traversing all of its descendants). - If no published version exists, a VersionConflictError is thrown. + If no published version exists, an InvalidVersionError is thrown. If a published version exists but there is no draft version of this item or any of its descendants, this method is a no-op. It is also a no-op if the root item is in DIRECT_ONLY_CATEGORIES. diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 4a822ddc4e..6e63c927bd 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1421,7 +1421,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # brand new course raise ItemNotFoundError(destination_course) if destination_course.branch not in index_entry['versions']: - # must be publishing the dag root if there's no current dag + # must be copying the dag root if there's no current dag root_block_id = source_structure['root'] if not any(root_block_id == subtree.block_id for subtree in subtree_list): raise ItemNotFoundError(u'Must publish course root {}'.format(root_block_id)) @@ -1457,7 +1457,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): ) # update/create the subtree and its children in destination (skipping blacklist) orphans.update( - self._publish_subdag( + self._copy_subdag( user_id, destination_structure['_id'], subtree_root.block_id, source_structure['blocks'], destination_blocks, blacklist ) @@ -1886,7 +1886,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): destination_parent['fields']['children'] = destination_reordered return orphans - def _publish_subdag(self, user_id, destination_version, block_id, source_blocks, destination_blocks, blacklist): + def _copy_subdag(self, user_id, destination_version, block_id, source_blocks, destination_blocks, blacklist): """ Update destination_blocks for the sub-dag rooted at block_id to be like the one in source_blocks excluding blacklist. @@ -1900,7 +1900,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if destination_block: # reorder children to correspond to whatever order holds for source. # remove any which source no longer claims (put into orphans) - # add any which are being published + # add any which are being copied source_children = new_block['fields'].get('children', []) existing_children = destination_block['fields'].get('children', []) destination_reordered = SparseList() @@ -1939,7 +1939,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): for child in destination_block['fields'].get('children', []): if child not in blacklist: orphans.update( - self._publish_subdag( + self._copy_subdag( user_id, destination_version, child, source_blocks, destination_blocks, blacklist ) ) 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 698769e2ca..3c29b23b78 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -3,6 +3,7 @@ Module for the dual-branch fall-back Draft->Published Versioning ModuleStore """ from split import SplitMongoModuleStore, EXCLUDE_ALL +from xmodule.exceptions import InvalidVersionError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import InsufficientSpecificationError from xmodule.modulestore.draft_and_published import ( @@ -252,6 +253,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS user_id, # Directly using the replace function rather than the for_branch function # because for_branch obliterates the version_guid and will lead to missed version conflicts. + # TODO Instead, the for_branch implementation should be fixed in the Opaque Keys library. location.course_key.replace(branch=ModuleStoreEnum.BranchName.draft), location.course_key.for_branch(ModuleStoreEnum.BranchName.published), [location], @@ -277,7 +279,22 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS :raises InvalidVersionError: if no published version exists for the location specified """ - raise NotImplementedError() + if location.category in DIRECT_ONLY_CATEGORIES: + return + + if not self.has_item(location, revision=ModuleStoreEnum.RevisionOption.published_only): + raise InvalidVersionError(location) + + SplitMongoModuleStore.copy( + self, + user_id, + # Directly using the replace function rather than the for_branch function + # because for_branch obliterates the version_guid and will lead to missed version conflicts. + # TODO Instead, the for_branch implementation should be fixed in the Opaque Keys library. + location.course_key.replace(branch=ModuleStoreEnum.BranchName.published), + location.course_key.for_branch(ModuleStoreEnum.BranchName.draft), + [location] + ) def get_course_history_info(self, course_locator): """ 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 394b55652e..02140b62fb 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -762,7 +762,7 @@ class TestMixedModuleStore(unittest.TestCase): with self.assertRaises(ItemNotFoundError): path_to_location(self.store, location) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_revert_to_published_root_draft(self, default_ms): """ Test calling revert_to_published on draft vertical. @@ -791,7 +791,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertEqual(vertical_children_num, len(published_parent.children)) self.assertEqual(reverted_parent, published_parent) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_revert_to_published_root_published(self, default_ms): """ Test calling revert_to_published on a published vertical with a draft child. @@ -811,7 +811,7 @@ class TestMixedModuleStore(unittest.TestCase): reverted_problem = self.store.get_item(self.problem_x1a_1) self.assertEqual(orig_display_name, reverted_problem.display_name) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_revert_to_published_no_draft(self, default_ms): """ Test calling revert_to_published on vertical with no draft content does nothing. @@ -825,7 +825,7 @@ class TestMixedModuleStore(unittest.TestCase): reverted_vertical = self.store.get_item(self.vertical_x1a) self.assertEqual(orig_vertical, reverted_vertical) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_revert_to_published_no_published(self, default_ms): """ Test calling revert_to_published on vertical with no published version errors. @@ -835,7 +835,7 @@ class TestMixedModuleStore(unittest.TestCase): with self.assertRaises(InvalidVersionError): self.store.revert_to_published(self.vertical_x1a, self.user_id) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_revert_to_published_direct_only(self, default_ms): """ Test calling revert_to_published on a direct-only item is a no-op.