add a delete-all-versions flag to the delete_item method in the modulestores.…

add a delete-all-versions flag to the delete_item method in the modulestores. Extend tests to verify. Still wip

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -264,15 +264,27 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
     def test_delete(self):
         import_from_xml(modulestore(), 'common/test/data/', ['full'])
 
-        module_store = modulestore('direct')
+        direct_store = modulestore('direct')
+        draft_store = modulestore('draft')
 
-        sequential = module_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]))
+        sequential = direct_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]))
 
-        chapter = module_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None]))
+        chapter = direct_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None]))
 
-        # make sure the parent no longer points to the child object which was deleted
+        # make sure the parent points to the child object which is to be deleted
         self.assertTrue(sequential.location.url() in chapter.children)
 
+        # get a vertical (and components in it) to put into 'draft'
+        vertical = direct_store.get_item(Location(['i4x', 'edX', 'full',
+                                         'vertical', 'vertical_66', None]))
+
+        private_item_location = vertical.location._replace(name='private_vertical')
+
+        draft_store.clone_item(vertical.location, private_item_location)
+
+        direct_store.update_children(sequential.location, sequential.children +
+                                     [private_item_location.url()])
+
         self.client.post(
             reverse('delete_item'),
             json.dumps({'id': sequential.location.url(), 'delete_children': 'true', 'delete_all_versions': 'true'}),
@@ -281,18 +293,28 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         found = False
         try:
-            module_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]))
+            draft_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]))
             found = True
         except ItemNotFoundError:
             pass
 
         self.assertFalse(found)
 
-        chapter = module_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None]))
+        chapter = draft_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None]))
 
         # make sure the parent no longer points to the child object which was deleted
         self.assertFalse(sequential.location.url() in chapter.children)
 
+        # make sure we can't look up the private vertical
+        found_private = False
+        try:
+            draft_store.get_item(private_item_location)
+            found_private = True
+        except ItemNotFoundError:
+            pass
+
+        self.assertFalse(found_private)
+
     def test_about_overrides(self):
         '''
         This test case verifies that a course can use specialized override for about data, e.g. /about/Fall_2012/effort.html

cms/djangoapps/contentstore/views.py

@@ -624,16 +624,9 @@ def delete_item(request):
     # if caller wants to only delete the draft than the caller should put item.location.revision='draft'
 
     if delete_children:
-        _xmodule_recurse(item, lambda i: store.delete_item(i.location))
+        _xmodule_recurse(item, lambda i: store.delete_item(i.location, delete_all_versions))
     else:
-        store.delete_item(item.location)
-
-    # cdodge: this is a bit of a hack until I can talk with Cale about the
-    # semantics of delete_item whereby the store is draft aware. Right now calling
-    # delete_item on a vertical tries to delete the draft version leaving the
-    # requested delete to never occur
-    if item.location.revision is None and item.location.category == 'vertical' and delete_all_versions:
-        modulestore('direct').delete_item(item.location)
+        store.delete_item(item.location, delete_all_versions)
 
     # cdodge: we need to remove our parent's pointer to us so that it is no longer dangling
     if delete_all_versions:

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

@@ -4,6 +4,8 @@ from . import ModuleStoreBase, Location, namedtuple_to_son
 from .exceptions import ItemNotFoundError
 from .inheritance import own_metadata
 
+import logging
+
 DRAFT = 'draft'
 
 
@@ -159,13 +161,17 @@ class DraftModuleStore(ModuleStoreBase):
 
         return super(DraftModuleStore, self).update_metadata(draft_loc, metadata)
 
-    def delete_item(self, location):
+    def delete_item(self, location, delete_all_versions=False):
         """
         Delete an item from this modulestore
 
         location: Something that can be passed to Location
         """
-        return super(DraftModuleStore, self).delete_item(as_draft(location))
+        super(DraftModuleStore, self).delete_item(as_draft(location))
+        if delete_all_versions:
+            super(DraftModuleStore, self).delete_item(location)
+
+        return
 
     def get_parent_locations(self, location, course_id):
         '''Find all locations that are the parents of this location.  Needed

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

@@ -694,12 +694,14 @@ class MongoModuleStore(ModuleStoreBase):
         self.refresh_cached_metadata_inheritance_tree(loc)
         self.fire_updated_modulestore_signal(get_course_id_no_run(Location(location)), Location(location))
 
-    def delete_item(self, location):
+    def delete_item(self, location, delete_all_versions=False):
         """
         Delete an item from this modulestore
 
         location: Something that can be passed to Location
+        delete_all_versions: is here because the DraftMongoModuleStore needs it and we need to keep the interface the same. It is unused.
         """
+        logging.debug('delete_all_versions = {0}'.format(delete_all_versions))
         # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
         # if we add one then we need to also add it to the policy information (i.e. metadata)
         # we should remove this once we can break this reference from the course to static tabs