Merge pull request #4369 from edx/bug/stud-1962

Bug/stud 1962

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -138,8 +138,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
     def get_item(self, usage_key, depth=0, **kwargs):
         """
-        This method is explicitly not implemented as we need a course_id to disambiguate
-        We should be able to fix this when the data-model rearchitecting is done
+        see parent doc
         """
         store = self._get_modulestore_for_courseid(usage_key.course_key)
         return store.get_item(usage_key, depth, **kwargs)

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

@@ -506,15 +506,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

@@ -17,6 +17,7 @@ from xmodule.modulestore.tests.test_location_mapper import LocMapperSetupSansDja
 # before importing the module
 from django.conf import settings
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from xmodule.modulestore.mongo.base import MongoRevisionKey
 if not settings.configured:
     settings.configure()
 from xmodule.modulestore.mixed import MixedModuleStore
@@ -326,6 +327,42 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         with self.assertRaises(ItemNotFoundError):
             self.store.get_item(self.writable_chapter_location)
 
+        # 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='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 == 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(vert_loc, course.children)
+
+        # 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)
+        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):
         self.initdb(default_ms)