Deleting a private unit should remove it from the course

STUD-1962

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

@@ -494,15 +494,13 @@ class DraftModuleStore(MongoModuleStore):
         #   Case 1: the draft item moved from one parent to another
         #   Case 2: revision==ModuleStoreEnum.RevisionOption.all and the single parent has 2 versions: draft and published
         for parent_location in parent_locations:
-            # don't remove from direct_only parent if other versions of this still exists
+            # don't remove from direct_only parent if other versions of this still exists (this code
+            # assumes that there's only one parent_location in this case)
             if not is_item_direct_only and parent_location.category in DIRECT_ONLY_CATEGORIES:
-                # see if other version of root exists
-                alt_location = location.replace(
-                    revision=MongoRevisionKey.published
-                    if location.revision == MongoRevisionKey.draft
-                    else MongoRevisionKey.draft
-                )
-                if super(DraftModuleStore, self).has_item(alt_location):
+                # see if other version of to-be-deleted root exists
+                query = location.to_deprecated_son(prefix='_id.')
+                del query['_id.revision']
+                if self.collection.find(query).count() > 1:
                     continue
 
             parent_block = super(DraftModuleStore, self).get_item(parent_location)

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

@@ -332,30 +332,40 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             self.store.get_item(self.writable_chapter_location)
 
         # create and delete a private vertical with private children
-        private_vert_loc = self.course_locations[self.MONGO_COURSEID].course_key.make_usage_key('vertical', 'private')
-        private_vert = self.store.create_and_save_xmodule(private_vert_loc, self.user_id, runtime=self.course.runtime)
-        self.course.children.append(private_vert_loc)
-        self.store.update_item(self.course, self.user_id)
-        private_leaf_loc = self.course_locations[self.MONGO_COURSEID].course_key.make_usage_key('html', 'private_leaf')
-        self.store.create_and_save_xmodule(private_leaf_loc, self.user_id, runtime=self.course.runtime)
-        private_vert.children.append(private_leaf_loc)
-        self.store.update_item(private_vert, self.user_id)
+        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='private'
+        )
+        private_leaf = self.store.create_item(
+            # don't use course_location as it may not be the repr
+            private_vert.location, 'html', user_id=self.user_id, block_id='private_leaf'
+        )
 
         # verify pre delete state (just to verify that the test is valid)
-        self.assertTrue(hasattr(private_vert, 'is_draft') or private_vert.location.branch == MongoRevisionKey.draft)
-        self.assertIsNotNone(self.store.get_item(private_vert_loc))
-        self.assertIsNotNone(self.store.get_item(private_leaf_loc))
+        self.assertTrue(hasattr(private_vert, 'is_draft') or private_vert.location.branch == ModuleStoreEnum.BranchName.draft)
+        if hasattr(private_vert.location, 'version_guid'):
+            # change to the HEAD version
+            vert_loc = private_vert.location.for_version(private_leaf.location.version_guid)
+        else:
+            vert_loc = private_vert.location
+        self.assertTrue(self.store.has_item(vert_loc))
+        self.assertTrue(self.store.has_item(private_leaf.location))
         course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0)
-        self.assertIn(private_vert_loc, course.children)
+        self.assertIn(vert_loc, course.children)
 
         # delete the vertical and ensure the course no longer points to it
-        self.store.delete_item(private_vert_loc, self.user_id)
-        with self.assertRaises(ItemNotFoundError):
-            self.store.get_item(private_vert_loc)
-        with self.assertRaises(ItemNotFoundError):
-            self.store.get_item(private_leaf_loc)
+        self.store.delete_item(vert_loc, self.user_id)
         course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0)
-        self.assertNotIn(private_vert_loc, course.children)
+        if hasattr(private_vert.location, 'version_guid'):
+            # change to the HEAD version
+            vert_loc = private_vert.location.for_version(course.location.version_guid)
+            leaf_loc = private_leaf.location.for_version(course.location.version_guid)
+        else:
+            vert_loc = private_vert.location
+            leaf_loc = private_leaf.location
+        self.assertFalse(self.store.has_item(vert_loc))
+        self.assertFalse(self.store.has_item(leaf_loc))
+        self.assertNotIn(vert_loc, course.children)
 
     @ddt.data('draft', 'split')
     def test_get_courses(self, default_ms):

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

@@ -10,7 +10,6 @@ from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore
 from xmodule.modulestore.mongo import MongoModuleStore, DraftMongoModuleStore
 from xmodule.modulestore.mongo.draft import DIRECT_ONLY_CATEGORIES
 from xmodule.modulestore import ModuleStoreEnum
-from mock import Mock
 
 
 class SplitWMongoCourseBoostrapper(unittest.TestCase):