Merge pull request #1903 from MITx/fix/cdodge/dangling-verticals-on-delete

Fix/cdodge/dangling verticals on delete

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -264,13 +264,13 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
     def test_delete(self):
         import_from_xml(modulestore(), 'common/test/data/', ['full'])
 
-        module_store = modulestore('direct')
+        direct_store = modulestore('direct')
 
-        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)
 
         self.client.post(
@@ -281,18 +281,19 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         found = False
         try:
-            module_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]))
+            direct_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 = 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
         self.assertFalse(sequential.location.url() in chapter.children)
 
+
     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

@@ -615,25 +615,14 @@ def delete_item(request):
     delete_children = request.POST.get('delete_children', False)
     delete_all_versions = request.POST.get('delete_all_versions', False)
 
-    item = modulestore().get_item(item_location)
+    store = modulestore()
 
-    store = get_modulestore(item_loc)
-
-    # @TODO: this probably leaves draft items dangling. My preferance would be for the semantic to be
-    # if item.location.revision=None, then delete both draft and published version
-    # if caller wants to only delete the draft than the caller should put item.location.revision='draft'
+    item = store.get_item(item_location)
 
     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

@@ -13,6 +13,12 @@ def as_draft(location):
     """
     return Location(location)._replace(revision=DRAFT)
 
+def as_published(location):
+    """
+    Returns the Location that is the published version for `location`
+    """
+    return Location(location)._replace(revision=None)    
+
 
 def wrap_draft(item):
     """
@@ -159,13 +165,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(as_published(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,11 +694,12 @@ 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.
         """
         # 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)