Merge pull request #37430 from nextcloud/trash-cleanup-logging

add a bit more verbose option for trashbin cleanup

apps/files_trashbin/lib/Command/CleanUp.php

@@ -78,13 +78,14 @@ class CleanUp extends Command {
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$users = $input->getArgument('user_id');
+		$verbose = $input->getOption('verbose');
 		if ((!empty($users)) and ($input->getOption('all-users'))) {
 			throw new InvalidOptionException('Either specify a user_id or --all-users');
 		} elseif (!empty($users)) {
 			foreach ($users as $user) {
 				if ($this->userManager->userExists($user)) {
 					$output->writeln("Remove deleted files of   <info>$user</info>");
-					$this->removeDeletedFiles($user);
+					$this->removeDeletedFiles($user, $output, $verbose);
 				} else {
 					$output->writeln("<error>Unknown user $user</error>");
 					return 1;
@@ -104,7 +105,7 @@ class CleanUp extends Command {
 					$users = $backend->getUsers('', $limit, $offset);
 					foreach ($users as $user) {
 						$output->writeln("   <info>$user</info>");
-						$this->removeDeletedFiles($user);
+						$this->removeDeletedFiles($user, $output, $verbose);
 					}
 					$offset += $limit;
 				} while (count($users) >= $limit);
@@ -117,19 +118,31 @@ class CleanUp extends Command {
 
 	/**
 	 * remove deleted files for the given user
-	 *
-	 * @param string $uid
 	 */
-	protected function removeDeletedFiles($uid) {
+	protected function removeDeletedFiles(string $uid, OutputInterface $output, bool $verbose): void {
 		\OC_Util::tearDownFS();
 		\OC_Util::setupFS($uid);
-		if ($this->rootFolder->nodeExists('/' . $uid . '/files_trashbin')) {
-			$this->rootFolder->get('/' . $uid . '/files_trashbin')->delete();
+		$path = '/' . $uid . '/files_trashbin';
+		if ($this->rootFolder->nodeExists($path)) {
+			$node = $this->rootFolder->get($path);
+
+			if ($verbose) {
+				$output->writeln("Deleting <info>" . \OC_Helper::humanFileSize($node->getSize()) . "</info> in trash for <info>$uid</info>.");
+			}
+			$node->delete();
+			if ($this->rootFolder->nodeExists($path)) {
+				$output->writeln("<error>Trash folder sill exists after attempting to delete it</error>");
+				return;
+			}
 			$query = $this->dbConnection->getQueryBuilder();
 			$query->delete('files_trash')
 				->where($query->expr()->eq('user', $query->createParameter('uid')))
 				->setParameter('uid', $uid);
 			$query->execute();
+		} else {
+			if ($verbose) {
+				$output->writeln("No trash found for <info>$uid</info>");
+			}
 		}
 	}
 }

apps/files_trashbin/tests/Command/CleanUpTest.php

@@ -32,6 +32,7 @@ use OCP\Files\IRootFolder;
 use OCP\IDBConnection;
 use Symfony\Component\Console\Exception\InvalidOptionException;
 use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\NullOutput;
 use Symfony\Component\Console\Output\OutputInterface;
 use Test\TestCase;
 
@@ -101,27 +102,27 @@ class CleanUpTest extends TestCase {
 	 * @dataProvider dataTestRemoveDeletedFiles
 	 * @param boolean $nodeExists
 	 */
-	public function testRemoveDeletedFiles($nodeExists) {
+	public function testRemoveDeletedFiles(bool $nodeExists) {
 		$this->initTable();
-		$this->rootFolder->expects($this->once())
+		$this->rootFolder
 			->method('nodeExists')
 			->with('/' . $this->user0 . '/files_trashbin')
-			->willReturn($nodeExists);
+			->willReturnOnConsecutiveCalls($nodeExists, false);
 		if ($nodeExists) {
-			$this->rootFolder->expects($this->once())
+			$this->rootFolder
 				->method('get')
 				->with('/' . $this->user0 . '/files_trashbin')
 				->willReturn($this->rootFolder);
-			$this->rootFolder->expects($this->once())
+			$this->rootFolder
 				->method('delete');
 		} else {
 			$this->rootFolder->expects($this->never())->method('get');
 			$this->rootFolder->expects($this->never())->method('delete');
 		}
-		$this->invokePrivate($this->cleanup, 'removeDeletedFiles', [$this->user0]);
+		$this->invokePrivate($this->cleanup, 'removeDeletedFiles', [$this->user0, new NullOutput(), false]);
 
 		if ($nodeExists) {
-			// if the delete operation was execute only files from user1
+			// if the delete operation was executed only files from user1
 			// should be left.
 			$query = $this->dbConnection->getQueryBuilder();
 			$query->select('user')
@@ -136,7 +137,7 @@ class CleanUpTest extends TestCase {
 				$this->assertSame('user1', $r['user']);
 			}
 		} else {
-			// if no delete operation was execute we should still have all 10
+			// if no delete operation was executed we should still have all 10
 			// database entries
 			$getAllQuery = $this->dbConnection->getQueryBuilder();
 			$result = $getAllQuery->select('id')
@@ -171,9 +172,14 @@ class CleanUpTest extends TestCase {
 			->method('userExists')->willReturn(true);
 		$inputInterface = $this->getMockBuilder('\Symfony\Component\Console\Input\InputInterface')
 			->disableOriginalConstructor()->getMock();
-		$inputInterface->expects($this->once())->method('getArgument')
+		$inputInterface->method('getArgument')
 			->with('user_id')
 			->willReturn($userIds);
+		$inputInterface->method('getOption')
+			->willReturnMap([
+				['all-users', false],
+				['verbose', false],
+			]);
 		$outputInterface = $this->getMockBuilder('\Symfony\Component\Console\Output\OutputInterface')
 			->disableOriginalConstructor()->getMock();
 		$this->invokePrivate($instance, 'execute', [$inputInterface, $outputInterface]);
@@ -190,7 +196,7 @@ class CleanUpTest extends TestCase {
 			->setConstructorArgs([$this->rootFolder, $this->userManager, $this->dbConnection])
 			->getMock();
 		$backend = $this->createMock(\OCP\UserInterface::class);
-		$backend->expects($this->once())->method('getUsers')
+		$backend->method('getUsers')
 			->with('', 500, 0)
 			->willReturn($backendUsers);
 		$instance->expects($this->exactly(count($backendUsers)))
@@ -199,14 +205,16 @@ class CleanUpTest extends TestCase {
 				$this->assertTrue(in_array($user, $backendUsers));
 			});
 		$inputInterface = $this->createMock(InputInterface::class);
-		$inputInterface->expects($this->once())->method('getArgument')
+		$inputInterface->method('getArgument')
 			->with('user_id')
 			->willReturn($userIds);
 		$inputInterface->method('getOption')
-			->with('all-users')
-			->willReturn(true);
+			->willReturnMap([
+				['all-users', true],
+				['verbose', false],
+			]);
 		$outputInterface = $this->createMock(OutputInterface::class);
-		$this->userManager->expects($this->once())
+		$this->userManager
 			->method('getBackends')
 			->willReturn([$backend]);
 		$this->invokePrivate($instance, 'execute', [$inputInterface, $outputInterface]);
@@ -214,12 +222,14 @@ class CleanUpTest extends TestCase {
 
 	public function testExecuteNoUsersAndNoAllUsers() {
 		$inputInterface = $this->createMock(InputInterface::class);
-		$inputInterface->expects($this->once())->method('getArgument')
+		$inputInterface->method('getArgument')
 			->with('user_id')
 			->willReturn([]);
 		$inputInterface->method('getOption')
-			->with('all-users')
-			->willReturn(false);
+			->willReturnMap([
+				['all-users', false],
+				['verbose', false],
+			]);
 		$outputInterface = $this->createMock(OutputInterface::class);
 
 		$this->expectException(InvalidOptionException::class);
@@ -230,12 +240,14 @@ class CleanUpTest extends TestCase {
 
 	public function testExecuteUsersAndAllUsers() {
 		$inputInterface = $this->createMock(InputInterface::class);
-		$inputInterface->expects($this->once())->method('getArgument')
+		$inputInterface->method('getArgument')
 			->with('user_id')
 			->willReturn(['user1', 'user2']);
 		$inputInterface->method('getOption')
-			->with('all-users')
-			->willReturn(true);
+			->willReturnMap([
+				['all-users', true],
+				['verbose', false],
+			]);
 		$outputInterface = $this->createMock(OutputInterface::class);
 
 		$this->expectException(InvalidOptionException::class);