Merge pull request #16247 from owncloud/fixrmdirtodeletefolderstotrash

Added rmdir to trashbin storage wrapper

2 files changed, 199 insertions, 15 deletions

diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php
index 418d7d2f1fd..006971fb242 100644
--- a/apps/files_trashbin/lib/storage.php
+++ b/apps/files_trashbin/lib/storage.php
@@ -81,14 +81,39 @@ class Storage extends Wrapper {
 	/**
 	 * Deletes the given file by moving it into the trashbin.
 	 *
-	 * @param string $path
+	 * @param string $path path of file or folder to delete
+	 *
+	 * @return bool true if the operation succeeded, false otherwise
 	 */
 	public function unlink($path) {
+		return $this->doDelete($path, 'unlink');
+	}
+
+	/**
+	 * Deletes the given folder by moving it into the trashbin.
+	 *
+	 * @param string $path path of folder to delete
+	 *
+	 * @return bool true if the operation succeeded, false otherwise
+	 */
+	public function rmdir($path) {
+		return $this->doDelete($path, 'rmdir');
+	}
+
+	/**
+	 * Run the delete operation with the given method
+	 *
+	 * @param string $path path of file or folder to delete
+	 * @param string $method either "unlink" or "rmdir"
+	 *
+	 * @return bool true if the operation succeeded, false otherwise
+	 */
+	private function doDelete($path, $method) {
 		if (self::$disableTrash
 			|| !\OC_App::isEnabled('files_trashbin')
 			|| (pathinfo($path, PATHINFO_EXTENSION) === 'part')
 		) {
-			return $this->storage->unlink($path);
+			return call_user_func_array([$this->storage, $method], [$path]);
 		}
 		$normalized = Filesystem::normalizePath($this->mountPoint . '/' . $path);
 		$result = true;
@@ -101,14 +126,14 @@ class Storage extends Wrapper {
 				// in cross-storage cases the file will be copied
 				// but not deleted, so we delete it here
 				if ($result) {
-					$this->storage->unlink($path);
+					call_user_func_array([$this->storage, $method], [$path]);
 				}
 			} else {
-				$result = $this->storage->unlink($path);
+				$result = call_user_func_array([$this->storage, $method], [$path]);
 			}
 			unset($this->deletedFiles[$normalized]);
 		} else if ($this->storage->file_exists($path)) {
-			$result = $this->storage->unlink($path);
+			$result = call_user_func_array([$this->storage, $method], [$path]);
 		}
 
 		return $result;
diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php
index d1468522dc2..7415aba09e9 100644
--- a/apps/files_trashbin/tests/storage.php
+++ b/apps/files_trashbin/tests/storage.php
@@ -62,6 +62,8 @@ class Storage extends \Test\TestCase {
 		$this->userView = new \OC\Files\View('/' . $this->user . '/files/');
 		$this->userView->file_put_contents('test.txt', 'foo');
 
+		$this->userView->mkdir('folder');
+		$this->userView->file_put_contents('folder/inside.txt', 'bar');
 	}
 
 	protected function tearDown() {
@@ -75,7 +77,7 @@ class Storage extends \Test\TestCase {
 	/**
 	 * Test that deleting a file puts it into the trashbin.
 	 */
-	public function testSingleStorageDelete() {
+	public function testSingleStorageDeleteFile() {
 		$this->assertTrue($this->userView->file_exists('test.txt'));
 		$this->userView->unlink('test.txt');
 		list($storage,) = $this->userView->resolvePath('test.txt');
@@ -90,12 +92,34 @@ class Storage extends \Test\TestCase {
 	}
 
 	/**
+	 * Test that deleting a folder puts it into the trashbin.
+	 */
+	public function testSingleStorageDeleteFolder() {
+		$this->assertTrue($this->userView->file_exists('folder/inside.txt'));
+		$this->userView->rmdir('folder');
+		list($storage,) = $this->userView->resolvePath('folder/inside.txt');
+		$storage->getScanner()->scan(''); // make sure we check the storage
+		$this->assertFalse($this->userView->getFileInfo('folder'));
+
+		// check if folder is in trashbin
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
+		$this->assertEquals(1, count($results));
+		$name = $results[0]->getName();
+		$this->assertEquals('folder', substr($name, 0, strrpos($name, '.')));
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/' . $name . '/');
+		$this->assertEquals(1, count($results));
+		$name = $results[0]->getName();
+		$this->assertEquals('inside.txt', $name);
+	}
+
+	/**
 	 * Test that deleting a file from another mounted storage properly
 	 * lands in the trashbin. This is a cross-storage situation because
 	 * the trashbin folder is in the root storage while the mounted one
 	 * isn't.
 	 */
-	public function testCrossStorageDelete() {
+	public function testCrossStorageDeleteFile() {
 		$storage2 = new Temporary(array());
 		\OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage');
 
@@ -116,9 +140,41 @@ class Storage extends \Test\TestCase {
 	}
 
 	/**
+	 * Test that deleting a folder from another mounted storage properly
+	 * lands in the trashbin. This is a cross-storage situation because
+	 * the trashbin folder is in the root storage while the mounted one
+	 * isn't.
+	 */
+	public function testCrossStorageDeleteFolder() {
+		$storage2 = new Temporary(array());
+		\OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage');
+
+		$this->userView->mkdir('substorage/folder');
+		$this->userView->file_put_contents('substorage/folder/subfile.txt', 'bar');
+		$storage2->getScanner()->scan('');
+		$this->assertTrue($storage2->file_exists('folder/subfile.txt'));
+		$this->userView->rmdir('substorage/folder');
+
+		$storage2->getScanner()->scan('');
+		$this->assertFalse($this->userView->getFileInfo('substorage/folder'));
+		$this->assertFalse($storage2->file_exists('folder/subfile.txt'));
+
+		// check if folder is in trashbin
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files');
+		$this->assertEquals(1, count($results));
+		$name = $results[0]->getName();
+		$this->assertEquals('folder', substr($name, 0, strrpos($name, '.')));
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/' . $name . '/');
+		$this->assertEquals(1, count($results));
+		$name = $results[0]->getName();
+		$this->assertEquals('subfile.txt', $name);
+	}
+
+	/**
 	 * Test that deleted versions properly land in the trashbin.
 	 */
-	public function testDeleteVersions() {
+	public function testDeleteVersionsOfFile() {
 		\OCA\Files_Versions\Hooks::connectHooks();
 
 		// trigger a version (multiple would not work because of the expire logic)
@@ -137,7 +193,38 @@ class Storage extends \Test\TestCase {
 		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions');
 		$this->assertEquals(1, count($results));
 		$name = $results[0]->getName();
-		$this->assertEquals('test.txt', substr($name, 0, strlen('test.txt')));
+		$this->assertEquals('test.txt.v', substr($name, 0, strlen('test.txt.v')));
+	}
+
+	/**
+	 * Test that deleted versions properly land in the trashbin.
+	 */
+	public function testDeleteVersionsOfFolder() {
+		\OCA\Files_Versions\Hooks::connectHooks();
+
+		// trigger a version (multiple would not work because of the expire logic)
+		$this->userView->file_put_contents('folder/inside.txt', 'v1');
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_versions/folder/');
+		$this->assertEquals(1, count($results));
+
+		$this->userView->rmdir('folder');
+
+		// rescan trash storage
+		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		$rootStorage->getScanner()->scan('');
+
+		// check if versions are in trashbin
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions');
+		$this->assertEquals(1, count($results));
+		$name = $results[0]->getName();
+		$this->assertEquals('folder.d', substr($name, 0, strlen('folder.d')));
+
+		// check if versions are in trashbin
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/' . $name . '/');
+		$this->assertEquals(1, count($results));
+		$name = $results[0]->getName();
+		$this->assertEquals('inside.txt.v', substr($name, 0, strlen('inside.txt.v')));
 	}
 
 	/**
@@ -145,7 +232,7 @@ class Storage extends \Test\TestCase {
 	 * storages. This is because rename() between storages would call
 	 * unlink() which should NOT trigger the version deletion logic.
 	 */
-	public function testKeepFileAndVersionsWhenMovingBetweenStorages() {
+	public function testKeepFileAndVersionsWhenMovingFileBetweenStorages() {
 		\OCA\Files_Versions\Hooks::connectHooks();
 
 		$storage2 = new Temporary(array());
@@ -162,7 +249,7 @@ class Storage extends \Test\TestCase {
 
 		// move to another storage
 		$this->userView->rename('test.txt', 'substorage/test.txt');
-		$this->userView->file_exists('substorage/test.txt');
+		$this->assertTrue($this->userView->file_exists('substorage/test.txt'));
 
 		// rescan trash storage
 		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
@@ -182,9 +269,50 @@ class Storage extends \Test\TestCase {
 	}
 
 	/**
+	 * Test that versions are not auto-trashed when moving a file between
+	 * storages. This is because rename() between storages would call
+	 * unlink() which should NOT trigger the version deletion logic.
+	 */
+	public function testKeepFileAndVersionsWhenMovingFolderBetweenStorages() {
+		\OCA\Files_Versions\Hooks::connectHooks();
+
+		$storage2 = new Temporary(array());
+		\OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage');
+
+		// trigger a version (multiple would not work because of the expire logic)
+		$this->userView->file_put_contents('folder/inside.txt', 'v1');
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files');
+		$this->assertEquals(0, count($results));
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_versions/folder/');
+		$this->assertEquals(1, count($results));
+
+		// move to another storage
+		$this->userView->rename('folder', 'substorage/folder');
+		$this->assertTrue($this->userView->file_exists('substorage/folder/inside.txt'));
+
+		// rescan trash storage
+		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		$rootStorage->getScanner()->scan('');
+
+		// versions were moved too
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_versions/substorage/folder/');
+		$this->assertEquals(1, count($results));
+
+		// check that nothing got trashed by the rename's unlink() call
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files');
+		$this->assertEquals(0, count($results));
+
+		// check that versions were moved and not trashed
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/');
+		$this->assertEquals(0, count($results));
+	}
+
+	/**
 	 * Delete should fail is the source file cant be deleted
 	 */
-	public function testSingleStorageDeleteFail() {
+	public function testSingleStorageDeleteFileFail() {
 		/**
 		 * @var \OC\Files\Storage\Temporary | \PHPUnit_Framework_MockObject_MockObject $storage
 		 */
@@ -194,9 +322,6 @@ class Storage extends \Test\TestCase {
 			->getMock();
 
 		$storage->expects($this->any())
-			->method('rename')
-			->will($this->returnValue(false));
-		$storage->expects($this->any())
 			->method('unlink')
 			->will($this->returnValue(false));
 
@@ -214,4 +339,38 @@ class Storage extends \Test\TestCase {
 		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
 		$this->assertEquals(0, count($results));
 	}
+
+	/**
+	 * Delete should fail is the source folder cant be deleted
+	 */
+	public function testSingleStorageDeleteFolderFail() {
+		/**
+		 * @var \OC\Files\Storage\Temporary | \PHPUnit_Framework_MockObject_MockObject $storage
+		 */
+		$storage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
+			->setConstructorArgs([[]])
+			->setMethods(['rename', 'unlink', 'rmdir'])
+			->getMock();
+
+		$storage->expects($this->any())
+			->method('rmdir')
+			->will($this->returnValue(false));
+
+		$cache = $storage->getCache();
+
+		Filesystem::mount($storage, [], '/' . $this->user);
+		$storage->mkdir('files');
+		$this->userView->mkdir('folder');
+		$this->userView->file_put_contents('folder/test.txt', 'foo');
+		$this->assertTrue($storage->file_exists('files/folder/test.txt'));
+		$this->assertFalse($this->userView->rmdir('files/folder'));
+		$this->assertTrue($storage->file_exists('files/folder'));
+		$this->assertTrue($storage->file_exists('files/folder/test.txt'));
+		$this->assertTrue($cache->inCache('files/folder'));
+		$this->assertTrue($cache->inCache('files/folder/test.txt'));
+
+		// file should not be in the trashbin
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
+		$this->assertEquals(0, count($results));
+	}
 }