Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 29, 2018

Conversation

liamdennehy
Copy link
Contributor

Signed-off-by: Liam Dennehy [email protected]

* Add option --all-users to explicitly clean all trashbins
* Reject no users on commandline and no --all-users
* Warn when --all-users and userids are specified

Signed-off-by: Liam Dennehy <[email protected]>
} else {
$output->writeln('Remove all deleted files');
} elseif ($input->getOption('all-users')) {
$output->writeln('Remove deleted files for all users');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add following else block to inform about the needed parameters:

} else {
	throw new InvalidOptionException('Either specify a user_id or --all-users');
}

And don't forget to import the exception then as well:

use Symfony\Component\Console\Exception\InvalidOptionException;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good beside my little nitpick 👍

* throw on no parameters provided
* throw on --all-users and userid provided

Signed-off-by: Liam Dennehy <[email protected]>
@codecov
Copy link

codecov bot commented Jun 28, 2018

Codecov Report

Merging #10041 into master will decrease coverage by 20.27%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master   #10041       +/-   ##
=============================================
- Coverage     51.99%   31.71%   -20.28%     
- Complexity    26007    26010        +3     
=============================================
  Files          1661     1661               
  Lines         96132    96142       +10     
  Branches       1290     1290               
=============================================
- Hits          49984    30494    -19490     
- Misses        46148    65648    +19500
Impacted Files Coverage Δ Complexity Δ
apps/files_trashbin/lib/Command/CleanUp.php 0% <0%> (-89.59%) 15 <0> (+3)
lib/private/SystemTag/ManagerFactory.php 0% <0%> (-100%) 3% <0%> (ø)
lib/private/DB/SQLiteSessionInit.php 0% <0%> (-100%) 4% <0%> (ø)
apps/files_sharing/lib/External/MountProvider.php 0% <0%> (-100%) 4% <0%> (ø)
...rivate/Authentication/Token/DefaultTokenMapper.php 0% <0%> (-100%) 11% <0%> (ø)
lib/private/Share20/Hooks.php 0% <0%> (-100%) 3% <0%> (ø)
lib/private/Route/CachingRouter.php 0% <0%> (-100%) 3% <0%> (ø)
apps/dav/lib/Connector/Sabre/Server.php 0% <0%> (-100%) 1% <0%> (ø)
apps/user_ldap/lib/BackendUtility.php 0% <0%> (-100%) 1% <0%> (ø)
lib/public/Files/ForbiddenException.php 0% <0%> (-100%) 2% <0%> (ø)
... and 381 more

@blizzz
Copy link
Member

blizzz commented Jun 28, 2018

unit tests need to be adjusted

@MorrisJobke
Copy link
Member

unit tests need to be adjusted

Ref:

2) OCA\Files_Trashbin\Tests\Command\CleanUpTest::testExecuteAllUsers
Symfony\Component\Console\Exception\InvalidOptionException: Either specify a user_id or --all-users

/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Command/CleanUp.php:110
/drone/src/github.com/nextcloud/server/tests/lib/TestCase.php:210
/drone/src/github.com/nextcloud/server/apps/files_trashbin/tests/Command/CleanUpTest.php:206

@liamdennehy If you need help, then reach out to us. Would be good to get this in today, as we have feature freeze today.

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Copy link
Member

rullzer commented Jun 29, 2018

I took the liberty of fixing the tests so we can merge it :)

@rullzer
Copy link
Member

rullzer commented Jun 29, 2018

Failing tests are unrelated.

@rullzer rullzer merged commit 9b6c0ac into nextcloud:master Jun 29, 2018
@rullzer
Copy link
Member

rullzer commented Jun 29, 2018

Cool stuff @liamdennehy

Feel free to keep joining IRC.
And also feel free to tackle other issues 🚀 🎸

@liamdennehy
Copy link
Contributor Author

Thanks @rullzer, I would have taken ages to figure out how to create those tests!

@liamdennehy liamdennehy deleted the 10001/trashbin-defaults branch June 30, 2018 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants