Merge pull request #4462 from edx/bug/delete_draft

Piecemeal conversion to draft needs to allow

cms/djangoapps/contentstore/views/item.py

@@ -305,7 +305,6 @@ def _save_item(user, usage_key, data=None, children=None, metadata=None, nullout
                 pass
         elif publish == 'create_draft':
             try:
-                # This recursively clones the item subtree and marks the copies as draft
                 store.convert_to_draft(existing_item.location, user.id)
             except DuplicateItemError:
                 pass

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

@@ -369,10 +369,11 @@ class DraftModuleStore(MongoModuleStore):
         Raises:
             InvalidVersionError: if the source can not be made into a draft
             ItemNotFoundError: if the source does not exist
-            DuplicateItemError: if the source or any of its descendants already has a draft copy
         """
+        # TODO (dhm) I don't think this needs to recurse anymore but can convert each unit on demand.
+        #     See if that's true.
         # delegating to internal b/c we don't want any public user to use the kwargs on the internal
-        self._convert_to_draft(location, user_id)
+        self._convert_to_draft(location, user_id, ignore_if_draft=True)
 
         # return the new draft item (does another fetch)
         # get_item will wrap_draft so don't call it here (otherwise, it would override the is_draft attribute)
@@ -391,7 +392,8 @@ class DraftModuleStore(MongoModuleStore):
         Raises:
             InvalidVersionError: if the source can not be made into a draft
             ItemNotFoundError: if the source does not exist
-            DuplicateItemError: if the source or any of its descendants already has a draft copy
+            DuplicateItemError: if the source or any of its descendants already has a draft copy. Only
+                useful for unpublish b/c we don't want unpublish to overwrite any existing drafts.
         """
         # verify input conditions
         self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)

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

@@ -314,7 +314,7 @@ class TestMixedModuleStore(unittest.TestCase):
         Delete should reject on r/o db and work on r/w one
         """
         self.initdb(default_ms)
-        # r/o try deleting the course (is here to ensure it can't be deleted)
+        # r/o try deleting the chapter (is here to ensure it can't be deleted)
         with self.assertRaises(NotImplementedError):
             self.store.delete_item(self.xml_chapter_location, self.user_id)
         self.store.delete_item(self.writable_chapter_location, self.user_id)
@@ -344,6 +344,7 @@ class TestMixedModuleStore(unittest.TestCase):
         course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0)
         self.assertIn(vert_loc, course.children)
 
+        # update the component to force it to draft w/o forcing the unit to draft
         # delete the vertical and ensure the course no longer points to it
         self.store.delete_item(vert_loc, self.user_id)
         course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0)
@@ -358,6 +359,26 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertFalse(self.store.has_item(leaf_loc))
         self.assertNotIn(vert_loc, course.children)
 
+        # NAATODO enable for split after your converge merge
+        if default_ms == 'split':
+            return
+
+        # reproduce bug STUD-1965
+        # create and delete a private vertical with private children
+        private_vert = self.store.create_item(
+            # don't use course_location as it may not be the repr
+            self.course_locations[self.MONGO_COURSEID], 'vertical', user_id=self.user_id, block_id='publish'
+        )
+        private_leaf = self.store.create_item(
+            private_vert.location, 'html', user_id=self.user_id, block_id='bug_leaf'
+        )
+
+        self.store.publish(private_vert.location, self.user_id)
+        private_leaf.display_name = 'change me'
+        private_leaf = self.store.update_item(private_leaf, self.user_id)
+        # test succeeds if delete succeeds w/o error
+        self.store.delete_item(private_leaf.location, self.user_id)
+
     @ddt.data('draft', 'split')
     def test_get_courses(self, default_ms):
         self.initdb(default_ms)