Merge pull request #3108 from kirit93/fixes2760

takluyver · web-flow · commit ae011a179742 · 2017-12-05T14:27:14.000Z
Allowing non empty dirs to be deleted
diff --git a/notebook/services/contents/filemanager.py b/notebook/services/contents/filemanager.py
@@ -489,17 +489,8 @@ def delete_file(self, path):
         path = path.strip('/')
         os_path = self._get_os_path(path)
         rm = os.unlink
-        if os.path.isdir(os_path):
-            listing = os.listdir(os_path)
-            # Don't delete non-empty directories.
-            # A directory containing only leftover checkpoints is
-            # considered empty.
-            cp_dir = getattr(self.checkpoints, 'checkpoint_dir', None)
-            for entry in listing:
-                if entry != cp_dir:
-                    raise web.HTTPError(400, u'Directory %s not empty' % os_path)
-        elif not os.path.isfile(os_path):
-            raise web.HTTPError(404, u'File does not exist: %s' % os_path)
+        if not os.path.exists(os_path):
+            raise web.HTTPError(404, u'File or directory does not exist: %s' % os_path)
 
         if self.delete_to_trash:
             self.log.debug("Sending %s to trash", os_path)
@@ -510,6 +501,14 @@ def delete_file(self, path):
             return
 
         if os.path.isdir(os_path):
+            listing = os.listdir(os_path)
+            # Don't permanently delete non-empty directories.
+            # A directory containing only leftover checkpoints is
+            # considered empty.
+            cp_dir = getattr(self.checkpoints, 'checkpoint_dir', None)
+            for entry in listing:
+                if entry != cp_dir:
+                    raise web.HTTPError(400, u'Directory %s not empty' % os_path)
             self.log.debug("Removing directory %s", os_path)
             with self.perm_to_403():
                 shutil.rmtree(os_path)
diff --git a/notebook/services/contents/tests/test_contents_api.py b/notebook/services/contents/tests/test_contents_api.py
@@ -523,9 +523,11 @@ def test_delete_dirs(self):
         self.assertEqual(listing, [])
 
     def test_delete_non_empty_dir(self):
-        """delete non-empty dir raises 400"""
-        with assert_http_error(400):
-            self.api.delete(u'å b')
+        # Test that non empty directory can be deleted
+        self.api.delete(u'å b')
+        # Check if directory has actually been deleted
+        with assert_http_error(404):
+            self.api.list(u'å b')
 
     def test_rename(self):
         resp = self.api.rename('foo/a.ipynb', 'foo/z.ipynb')
