Merge pull request #15881 from owncloud/share-moveversionsproperly

Fix version rename with files and folders

apps/files_versions/lib/storage.php

@@ -223,40 +223,60 @@ class Storage {
 	}
 
 	/**
-	 * rename or copy versions of a file
-	 * @param string $old_path
-	 * @param string $new_path
+	 * Rename or copy versions of a file of the given paths
+	 *
+	 * @param string $sourcePath source path of the file to move, relative to
+	 * the currently logged in user's "files" folder
+	 * @param string $targetPath target path of the file to move, relative to
+	 * the currently logged in user's "files" folder
 	 * @param string $operation can be 'copy' or 'rename'
 	 */
-	public static function renameOrCopy($old_path, $new_path, $operation) {
-		list($uid, $oldpath) = self::getSourcePathAndUser($old_path);
+	public static function renameOrCopy($sourcePath, $targetPath, $operation) {
+		list($sourceOwner, $sourcePath) = self::getSourcePathAndUser($sourcePath);
 
 		// it was a upload of a existing file if no old path exists
 		// in this case the pre-hook already called the store method and we can
 		// stop here
-		if ($oldpath === false) {
+		if ($sourcePath === false) {
 			return true;
 		}
 
-		list($uidn, $newpath) = self::getUidAndFilename($new_path);
-		$versions_view = new \OC\Files\View('/'.$uid .'/files_versions');
-		$files_view = new \OC\Files\View('/'.$uid .'/files');
+		list($targetOwner, $targetPath) = self::getUidAndFilename($targetPath);
+
+		$sourcePath = ltrim($sourcePath, '/');
+		$targetPath = ltrim($targetPath, '/');
 
+		$rootView = new \OC\Files\View('');
 
+		// did we move a directory ?
+		if ($rootView->is_dir('/' . $targetOwner . '/files/' . $targetPath)) {
+			// does the directory exists for versions too ?
+			if ($rootView->is_dir('/' . $sourceOwner . '/files_versions/' . $sourcePath)) {
+				// create missing dirs if necessary
+				self::createMissingDirectories($targetPath, new \OC\Files\View('/'. $targetOwner));
 
-		if ( $files_view->is_dir($oldpath) && $versions_view->is_dir($oldpath) ) {
-			$versions_view->$operation($oldpath, $newpath);
-		} else  if ( ($versions = Storage::getVersions($uid, $oldpath)) ) {
+				// move the directory containing the versions
+				$rootView->$operation(
+					'/' . $sourceOwner . '/files_versions/' . $sourcePath,
+					'/' . $targetOwner . '/files_versions/' . $targetPath
+				);
+			}
+		} else if ($versions = Storage::getVersions($sourceOwner, '/' . $sourcePath)) {
 			// create missing dirs if necessary
-			self::createMissingDirectories($newpath, new \OC\Files\View('/'. $uidn));
+			self::createMissingDirectories($targetPath, new \OC\Files\View('/'. $targetOwner));
 
 			foreach ($versions as $v) {
-				$versions_view->$operation($oldpath.'.v'.$v['version'], $newpath.'.v'.$v['version']);
+				// move each version one by one to the target directory
+				$rootView->$operation(
+					'/' . $sourceOwner . '/files_versions/' . $sourcePath.'.v' . $v['version'],
+					'/' . $targetOwner . '/files_versions/' . $targetPath.'.v'.$v['version']
+				);
 			}
 		}
 
-		if (!$files_view->is_dir($newpath)) {
-			self::scheduleExpire($newpath);
+		// if we moved versions directly for a file, schedule expiration check for that file
+		if (!$rootView->is_dir('/' . $targetOwner . '/files/' . $targetPath)) {
+			self::scheduleExpire($targetPath);
 		}
 
 	}
@@ -625,8 +645,11 @@ class Storage {
 	}
 
 	/**
-	 * create recursively missing directories
-	 * @param string $filename $path to a file
+	 * Create recursively missing directories inside of files_versions
+	 * that match the given path to a file.
+	 *
+	 * @param string $filename $path to a file, relative to the user's
+	 * "files" folder
 	 * @param \OC\Files\View $view view on data/user/
 	 */
 	private static function createMissingDirectories($filename, $view) {

apps/files_versions/tests/versions.php

@@ -77,7 +77,10 @@ class Test_Files_Versioning extends \Test\TestCase {
 	}
 
 	protected function tearDown() {
-		$this->rootView->deleteAll(self::USERS_VERSIONS_ROOT);
+		$this->rootView->deleteAll(self::TEST_VERSIONS_USER . '/files/');
+		$this->rootView->deleteAll(self::TEST_VERSIONS_USER2 . '/files/');
+		$this->rootView->deleteAll(self::TEST_VERSIONS_USER . '/files_versions/');
+		$this->rootView->deleteAll(self::TEST_VERSIONS_USER2 . '/files_versions/');
 
 		parent::tearDown();
 	}
@@ -257,9 +260,6 @@ class Test_Files_Versioning extends \Test\TestCase {
 
 		$this->assertTrue($this->rootView->file_exists($v1Renamed));
 		$this->assertTrue($this->rootView->file_exists($v2Renamed));
-
-		//cleanup
-		\OC\Files\Filesystem::unlink('test2.txt');
 	}
 
 	public function testRenameInSharedFolder() {
@@ -304,9 +304,136 @@ class Test_Files_Versioning extends \Test\TestCase {
 
 		$this->assertTrue($this->rootView->file_exists($v1Renamed));
 		$this->assertTrue($this->rootView->file_exists($v2Renamed));
+	}
 
-		//cleanup
-		\OC\Files\Filesystem::unlink('/folder1/folder2/test.txt');
+	public function testMoveFolder() {
+
+		\OC\Files\Filesystem::mkdir('folder1');
+		\OC\Files\Filesystem::mkdir('folder2');
+		\OC\Files\Filesystem::file_put_contents('folder1/test.txt', 'test file');
+
+		$t1 = time();
+		// second version is two weeks older, this way we make sure that no
+		// version will be expired
+		$t2 = $t1 - 60 * 60 * 24 * 14;
+
+		// create some versions
+		$this->rootView->mkdir(self::USERS_VERSIONS_ROOT . '/folder1');
+		$v1 = self::USERS_VERSIONS_ROOT . '/folder1/test.txt.v' . $t1;
+		$v2 = self::USERS_VERSIONS_ROOT . '/folder1/test.txt.v' . $t2;
+		$v1Renamed = self::USERS_VERSIONS_ROOT . '/folder2/folder1/test.txt.v' . $t1;
+		$v2Renamed = self::USERS_VERSIONS_ROOT . '/folder2/folder1/test.txt.v' . $t2;
+
+		$this->rootView->file_put_contents($v1, 'version1');
+		$this->rootView->file_put_contents($v2, 'version2');
+
+		// execute rename hook of versions app
+		\OC\Files\Filesystem::rename('folder1', 'folder2/folder1');
+
+		$this->runCommands();
+
+		$this->assertFalse($this->rootView->file_exists($v1));
+		$this->assertFalse($this->rootView->file_exists($v2));
+
+		$this->assertTrue($this->rootView->file_exists($v1Renamed));
+		$this->assertTrue($this->rootView->file_exists($v2Renamed));
+	}
+
+
+	public function testMoveFileIntoSharedFolderAsRecipient() {
+
+		\OC\Files\Filesystem::mkdir('folder1');
+		$fileInfo = \OC\Files\Filesystem::getFileInfo('folder1');
+
+		\OCP\Share::shareItem(
+			'folder',
+			$fileInfo['fileid'],
+			\OCP\Share::SHARE_TYPE_USER,
+			self::TEST_VERSIONS_USER2,
+			\OCP\Constants::PERMISSION_ALL
+		);
+
+		self::loginHelper(self::TEST_VERSIONS_USER2);
+		$versionsFolder2 = '/' . self::TEST_VERSIONS_USER2 . '/files_versions';
+		\OC\Files\Filesystem::file_put_contents('test.txt', 'test file');
+
+		$t1 = time();
+		// second version is two weeks older, this way we make sure that no
+		// version will be expired
+		$t2 = $t1 - 60 * 60 * 24 * 14;
+
+		$this->rootView->mkdir($versionsFolder2);
+		// create some versions
+		$v1 = $versionsFolder2 . '/test.txt.v' . $t1;
+		$v2 = $versionsFolder2 . '/test.txt.v' . $t2;
+
+		$this->rootView->file_put_contents($v1, 'version1');
+		$this->rootView->file_put_contents($v2, 'version2');
+
+		// move file into the shared folder as recipient
+		\OC\Files\Filesystem::rename('/test.txt', '/folder1/test.txt');
+
+		$this->assertFalse($this->rootView->file_exists($v1));
+		$this->assertFalse($this->rootView->file_exists($v2));
+
+		self::loginHelper(self::TEST_VERSIONS_USER);
+
+		$versionsFolder1 = '/' . self::TEST_VERSIONS_USER . '/files_versions';
+
+		$v1Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t1;
+		$v2Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t2;
+
+		$this->assertTrue($this->rootView->file_exists($v1Renamed));
+		$this->assertTrue($this->rootView->file_exists($v2Renamed));
+	}
+
+	public function testMoveFolderIntoSharedFolderAsRecipient() {
+
+		\OC\Files\Filesystem::mkdir('folder1');
+		$fileInfo = \OC\Files\Filesystem::getFileInfo('folder1');
+
+		\OCP\Share::shareItem(
+			'folder',
+			$fileInfo['fileid'],
+			\OCP\Share::SHARE_TYPE_USER,
+			self::TEST_VERSIONS_USER2,
+			\OCP\Constants::PERMISSION_ALL
+		);
+
+		self::loginHelper(self::TEST_VERSIONS_USER2);
+		$versionsFolder2 = '/' . self::TEST_VERSIONS_USER2 . '/files_versions';
+		\OC\Files\Filesystem::mkdir('folder2');
+		\OC\Files\Filesystem::file_put_contents('folder2/test.txt', 'test file');
+
+		$t1 = time();
+		// second version is two weeks older, this way we make sure that no
+		// version will be expired
+		$t2 = $t1 - 60 * 60 * 24 * 14;
+
+		$this->rootView->mkdir($versionsFolder2);
+		$this->rootView->mkdir($versionsFolder2 . '/folder2');
+		// create some versions
+		$v1 = $versionsFolder2 . '/folder2/test.txt.v' . $t1;
+		$v2 = $versionsFolder2 . '/folder2/test.txt.v' . $t2;
+
+		$this->rootView->file_put_contents($v1, 'version1');
+		$this->rootView->file_put_contents($v2, 'version2');
+
+		// move file into the shared folder as recipient
+		\OC\Files\Filesystem::rename('/folder2', '/folder1/folder2');
+
+		$this->assertFalse($this->rootView->file_exists($v1));
+		$this->assertFalse($this->rootView->file_exists($v2));
+
+		self::loginHelper(self::TEST_VERSIONS_USER);
+
+		$versionsFolder1 = '/' . self::TEST_VERSIONS_USER . '/files_versions';
+
+		$v1Renamed = $versionsFolder1 . '/folder1/folder2/test.txt.v' . $t1;
+		$v2Renamed = $versionsFolder1 . '/folder1/folder2/test.txt.v' . $t2;
+
+		$this->assertTrue($this->rootView->file_exists($v1Renamed));
+		$this->assertTrue($this->rootView->file_exists($v2Renamed));
 	}
 
 	public function testRenameSharedFile() {
@@ -349,9 +476,6 @@ class Test_Files_Versioning extends \Test\TestCase {
 
 		$this->assertFalse($this->rootView->file_exists($v1Renamed));
 		$this->assertFalse($this->rootView->file_exists($v2Renamed));
-
-		//cleanup
-		\OC\Files\Filesystem::unlink('/test.txt');
 	}
 
 	public function testCopy() {
@@ -382,10 +506,6 @@ class Test_Files_Versioning extends \Test\TestCase {
 
 		$this->assertTrue($this->rootView->file_exists($v1Copied));
 		$this->assertTrue($this->rootView->file_exists($v2Copied));
-
-		//cleanup
-		\OC\Files\Filesystem::unlink('test.txt');
-		\OC\Files\Filesystem::unlink('test2.txt');
 	}
 
 	/**