Merge pull request #10041 from liamdennehy/10001/trashbin-defaults

Safe default behaviour when no users are specified on trashbin:cleanup

2 files changed, 57 insertions, 10 deletions

diff --git a/apps/files_trashbin/lib/Command/CleanUp.php b/apps/files_trashbin/lib/Command/CleanUp.php
index b727dd9e133..d2f8f72be8b 100644
--- a/apps/files_trashbin/lib/Command/CleanUp.php
+++ b/apps/files_trashbin/lib/Command/CleanUp.php
@@ -29,8 +29,10 @@ use OCP\IUserBackend;
 use OCP\IUserManager;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputArgument;
+use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Console\Exception\InvalidOptionException;
 
 class CleanUp extends Command {
 
@@ -62,13 +64,21 @@ class CleanUp extends Command {
 			->addArgument(
 				'user_id',
 				InputArgument::OPTIONAL | InputArgument::IS_ARRAY,
-				'remove deleted files of the given user(s), if no user is given all deleted files will be removed'
+				'remove deleted files of the given user(s)'
+			)
+			->addOption(
+				'all-users',
+				null,
+				InputOption::VALUE_NONE,
+				'run action on all users'
 			);
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output) {
 		$users = $input->getArgument('user_id');
-		if (!empty($users)) {
+		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>");
@@ -77,8 +87,8 @@ class CleanUp extends Command {
 					$output->writeln("<error>Unknown user $user</error>");
 				}
 			}
-		} else {
-			$output->writeln('Remove all deleted files');
+		} elseif ($input->getOption('all-users')) {
+			$output->writeln('Remove deleted files for all users');
 			foreach ($this->userManager->getBackends() as $backend) {
 				$name = get_class($backend);
 				if ($backend instanceof IUserBackend) {
@@ -96,6 +106,8 @@ class CleanUp extends Command {
 					$offset += $limit;
 				} while (count($users) >= $limit);
 			}
+		} else {
+			throw new InvalidOptionException('Either specify a user_id or --all-users');
 		}
 	}
 
diff --git a/apps/files_trashbin/tests/Command/CleanUpTest.php b/apps/files_trashbin/tests/Command/CleanUpTest.php
index 36b1ff10727..1ea3fc19c05 100644
--- a/apps/files_trashbin/tests/Command/CleanUpTest.php
+++ b/apps/files_trashbin/tests/Command/CleanUpTest.php
@@ -28,6 +28,9 @@ namespace OCA\Files_Trashbin\Tests\Command;
 
 
 use OCA\Files_Trashbin\Command\CleanUp;
+use Symfony\Component\Console\Exception\InvalidOptionException;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
 use Test\TestCase;
 use OC\User\Manager;
 use OCP\Files\IRootFolder;
@@ -183,8 +186,7 @@ class CleanUpTest extends TestCase {
 			->setMethods(['removeDeletedFiles'])
 			->setConstructorArgs([$this->rootFolder, $this->userManager, $this->dbConnection])
 			->getMock();
-		$backend = $this->getMockBuilder(\OCP\UserInterface::class)
-			->disableOriginalConstructor()->getMock();
+		$backend = $this->createMock(\OCP\UserInterface::class);
 		$backend->expects($this->once())->method('getUsers')
 			->with('', 500, 0)
 			->willReturn($backendUsers);
@@ -193,17 +195,50 @@ class CleanUpTest extends TestCase {
 			->willReturnCallback(function ($user) use ($backendUsers) {
 				$this->assertTrue(in_array($user, $backendUsers));
 			});
-		$inputInterface = $this->getMockBuilder('\Symfony\Component\Console\Input\InputInterface')
-			->disableOriginalConstructor()->getMock();
+		$inputInterface = $this->createMock(InputInterface::class);
 		$inputInterface->expects($this->once())->method('getArgument')
 			->with('user_id')
 			->willReturn($userIds);
-		$outputInterface = $this->getMockBuilder('\Symfony\Component\Console\Output\OutputInterface')
-			->disableOriginalConstructor()->getMock();
+		$inputInterface->method('getOption')
+			->with('all-users')
+			->willReturn(true);
+		$outputInterface = $this->createMock(OutputInterface::class);
 		$this->userManager->expects($this->once())
 			->method('getBackends')
 			->willReturn([$backend]);
 		$this->invokePrivate($instance, 'execute', [$inputInterface, $outputInterface]);
 	}
 
+	public function testExecuteNoUsersAndNoAllUsers() {
+		$inputInterface = $this->createMock(InputInterface::class);
+		$inputInterface->expects($this->once())->method('getArgument')
+			->with('user_id')
+			->willReturn([]);
+		$inputInterface->method('getOption')
+			->with('all-users')
+			->willReturn(false);
+		$outputInterface = $this->createMock(OutputInterface::class);
+
+		$this->expectException(InvalidOptionException::class);
+		$this->expectExceptionMessage('Either specify a user_id or --all-users');
+
+		$this->invokePrivate($this->cleanup, 'execute', [$inputInterface, $outputInterface]);
+	}
+
+	public function testExecuteUsersAndAllUsers() {
+		$inputInterface = $this->createMock(InputInterface::class);
+		$inputInterface->expects($this->once())->method('getArgument')
+			->with('user_id')
+			->willReturn(['user1', 'user2']);
+		$inputInterface->method('getOption')
+			->with('all-users')
+			->willReturn(true);
+		$outputInterface = $this->createMock(OutputInterface::class);
+
+		$this->expectException(InvalidOptionException::class);
+		$this->expectExceptionMessage('Either specify a user_id or --all-users');
+
+		$this->invokePrivate($this->cleanup, 'execute', [$inputInterface, $outputInterface]);
+	}
+
 }