Merge pull request #4989 from edx/split/revert-to-published

LMS-11185 Split implementation of revert_to_published

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.

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.

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
                         )
                     )

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):
         """

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.