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

Let occ trashbin:restore restore also from groupfolders and add filters #39818

Merged
merged 5 commits into from
Sep 18, 2023

Conversation

R0Wi
Copy link
Member

@R0Wi R0Wi commented Aug 11, 2023

Resolves: #39724

Summary

Let occ trashbin:restore restore also from groupfolders and add additional filters

  • Using the TrashManager allows access to all deleted files
  • Add 'scope' parameter to choose where to restore from (user or groupfolders)
  • Add 'restore-from' and 'restore-to' date parameters to filter files to be
    restored by their deletion date
  • Add 'dry-run' flag to be able to see which files would be restored and being
    able to adjust the filter parameters accordingly

Manual testcases

# Will still only restore user deleted files (backward compat)
php occ trashbin:restore --all-users

# Will only restore files from groupfolders visible to user "admin"
php occ trashbin:restore  --scope groupfolders admin

# Will only list files to be restored
php occ trashbin:restore --all-users --dry-run

# Will only restore files from groupfolders which have been deleted between 01.08.2023 10:00 and 02.08.2023 11:55:02
php occ trashbin:restore  --scope groupfolders --all-users --restore-from "01.08.2023 10:00" --restore-to "02.08.2023 11:55:02"

# Will restore all files from all users (user-files and groupfolders)
php occ trashbin:restore --all-users --scope all

Showcase

Assume we have two files in our trashbin. One is Userfile.txt which was deleted from a users private directory. The other one is GroupfoldersFile.jpg which was deleted from a groupfolder gf1 where user admin (which we use for restore) has access to.

# Lets get a list of all deleted files ...
devcontainer@b03e2fb2a292:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope all 2>/dev/null 
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Would restore 2 files...
Would restore Userfile.txt originally deleted at August 12, 2023, 1:04:52 PM UTC to /Userfile.txt
Would restore GroupfoldersFile.jpg originally deleted at August 12, 2023, 1:04:45 PM UTC to /gf1/GroupfoldersFile.jpg
# Only show me the local ones and activate verbose output...
devcontainer@b03e2fb2a292:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope user -v 2>/dev/null 
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Skipping GroupfoldersFile.jpg because it is not a user trash item
Would restore 1 files...
Would restore Userfile.txt originally deleted at August 12, 2023, 1:04:52 PM UTC to /Userfile.txt
# Ok, only groupfolder ones please ...
devcontainer@b03e2fb2a292:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope groupfolders -v 2>/dev/null 
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Skipping Userfile.txt because it is not a groupfolders trash item
Would restore 1 files...
Would restore GroupfoldersFile.jpg originally deleted at August 12, 2023, 1:04:45 PM UTC to /gf1/GroupfoldersFile.jpg
# Let's try some time filtering, again with "all" deleted files and please be verbose about the filters...
devcontainer@b03e2fb2a292:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope all --restore-from "12.08.2023 13:04:44" --restore-to "12.08.2023 13:04:46" -v 2>/dev/null 
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Skipping Userfile.txt because it was deleted after the restore-to timestamp
Would restore 1 files...
Would restore GroupfoldersFile.jpg originally deleted at August 12, 2023, 1:04:45 PM UTC to /gf1/GroupfoldersFile.jpg

Note: I only used 2>/dev/null to cleanup the output in my dev environment. Should not be needed in production environments.

TODO

Checklist

@szaimen szaimen added this to the Nextcloud 28 milestone Aug 11, 2023
@R0Wi R0Wi force-pushed the feature/trashbin-restore-improvements#39724 branch from 627a517 to d2581e5 Compare August 12, 2023 13:36
@R0Wi
Copy link
Member Author

R0Wi commented Aug 13, 2023

CI errors seems unrelated

@solracsf solracsf changed the title Let occ trashbin:restore restore also from groupfolders and add filters Let occ trashbin:restore restore also from groupfolders and add filters Aug 13, 2023
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Nice improvement of the command

apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
apps/files_trashbin/lib/Command/RestoreAllFiles.php Outdated Show resolved Hide resolved
build/psalm-baseline.xml Outdated Show resolved Hide resolved
@come-nc
Copy link
Contributor

come-nc commented Aug 14, 2023

Did you test the command with groupfolders application not installed?

@R0Wi
Copy link
Member Author

R0Wi commented Aug 14, 2023

Did you test the command with groupfolders application not installed?

First of all thanks for your thorough review. Will try to incorporate the changes ASAP. Indeed I did not yet test it without the Groupfolders app installed but since there's no direct reference to it, it should work. I can do that after I've improved the code 👍

EDIT

Code improved and also tested without Groupfolders app. Works fine

@solracsf
Copy link
Member

@R0Wi sorry but can you just insert in the 1st post the result of commands but with GF disabled, like:

php occ trashbin:restore --dry-run --all-users --scope groupfolders -v

Thanks 👍

@solracsf solracsf added pending documentation This pull request needs an associated documentation update and removed pending documentation This pull request needs an associated documentation update labels Aug 14, 2023
@R0Wi
Copy link
Member Author

R0Wi commented Aug 16, 2023

@solracsf please find the test-script attached. Note that no groupfolders app was installed.

devcontainer@a9176d865258:/var/www/html$ php occ app:list 2>/dev/null
Enabled:
  - cloud_federation_api: 1.11.0
  - comments: 1.18.0
  - contactsinteraction: 1.9.0
  - dashboard: 7.8.0
  - dav: 1.28.0
  - federatedfilesharing: 1.18.0
  - federation: 1.18.0
  - files: 1.23.0
  - files_reminders: 1.0.0
  - files_sharing: 1.20.0
  - files_trashbin: 1.18.0
  - files_versions: 1.21.0
  - lookup_server_connector: 1.16.0
  - oauth2: 1.16.2
  - provisioning_api: 1.18.0
  - settings: 1.10.0
  - sharebymail: 1.18.0
  - systemtags: 1.18.0
  - theming: 2.3.0
  - twofactor_backupcodes: 1.17.0
  - updatenotification: 1.18.0
  - user_status: 1.8.0
  - weather_status: 1.8.0
  - workflowengine: 2.10.0
Disabled:
  - admin_audit: 1.18.0
  - encryption: 2.16.0
  - files_external: 1.20.0
  - testing: 1.18.0
  - user_ldap: 1.19.0

devcontainer@a9176d865258:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope groupfolders -v 2>/dev/null
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Skipping welcome.txt because it is not a groupfolders trash item
User has no deleted files in the trashbin matching the given filters

devcontainer@a9176d865258:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope all 2>/dev/null 
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Would restore 1 files...
Would restore welcome.txt originally deleted at 14. August 2023, 10:43:41 UTC to /welcome.txt

devcontainer@a9176d865258:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope user -v 2>/dev/null 
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Would restore 1 files...
Would restore welcome.txt originally deleted at 14. August 2023, 10:43:41 UTC to /welcome.txt

devcontainer@a9176d865258:/var/www/html$ php occ trashbin:restore --dry-run --all-users --scope all --since "12.08.2023 13:04:44" --until "12.08.2023 13:04:46" -v 2>/dev/null 
Restoring deleted files for all users
Restoring deleted files for users on backend Database
admin
Skipping welcome.txt because it was deleted after the 'until' timestamp
User has no deleted files in the trashbin matching the given filters

Note that I adjusted the verbose output of the last command to match the new parameter names in 66c88cc

…ional filters

* Using the TrashManager allows access to all deleted files
* Add 'scope' parameter to choose where to restore from (user or groupfolders)
* Add 'restore-from' and 'restore-to' date parameters to filter files to be
  restored by their deletion date
* Add 'dry-run' flag to be able to see which files would be restored and being
  able to adjust the filter parameters accordingly

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
* Delete unnecessary function docs
* Rename parameters to 'since' and 'until'
* Style: use '&&' instead of 'and'
* Add types

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
@R0Wi R0Wi force-pushed the feature/trashbin-restore-improvements#39724 branch from 66c88cc to d7a73ff Compare August 18, 2023 05:25
@R0Wi
Copy link
Member Author

R0Wi commented Aug 18, 2023

@come-nc @solracsf anything else to add here?

@solracsf
Copy link
Member

Just waiting from reviews from core developpers :)

@susnux susnux requested a review from come-nc August 19, 2023 13:12
@susnux susnux added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 19, 2023
Copy link
Contributor

@artonge artonge 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 overall.
From what I read, it should work if groudfolders is not installed. Can you confirm that?

Comment on lines +201 to +195
// We use getTitle() here instead of getOriginalLocation() because
// for groupfolders this contains the groupfolder name itself as prefix
// which makes it more human readable
$location = $trashItem->getTitle();
Copy link
Contributor

Choose a reason for hiding this comment

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

The goal is to display the groupfolder name ? Could the following work?

			$userFolder = $this->rootFolder->getUserFolder($user->getUID());
			$userFolder->get($trashItem->getOriginalLocation())->getMountPoint()->getInternalPath();

@R0Wi
Copy link
Member Author

R0Wi commented Aug 21, 2023

Looks good overall. From what I read, it should work if groudfolders is not installed. Can you confirm that?

That's correct. I tested both situations where the groupfolders app was installed and also without it being installed. That's why I removed the reference to the groupfolders app in ee66518#diff-0ab8d076ba91033c02327893677784c2ba7b49a391221c0a8c974430c529388dR293 and instead used a string for class matching. Unfortunately I did not find an alternative way to distinguish between regular user trash items and items coming from the groupfolders trash implementation.

@R0Wi
Copy link
Member Author

R0Wi commented Sep 3, 2023

@artonge I don't want to put too much preasure on this but any additional feedback would be really appreciated. My customer would be highly interested in this new feature. Thanks 👍

@R0Wi
Copy link
Member Author

R0Wi commented Sep 16, 2023

@come-nc anything to add from your side?

@solracsf solracsf removed their request for review September 17, 2023 06:46
@come-nc
Copy link
Contributor

come-nc commented Sep 18, 2023

I’m still uneasy about this getTitle thing but I do not have a better idea.

@artonge
Copy link
Contributor

artonge commented Sep 18, 2023

@R0Wi I'll let you merge if everything is good on your side :)

@R0Wi R0Wi merged commit 7afcacd into master Sep 18, 2023
39 checks passed
@R0Wi R0Wi deleted the feature/trashbin-restore-improvements#39724 branch September 18, 2023 10:38
@R0Wi
Copy link
Member Author

R0Wi commented Sep 22, 2023

/backport to stable27

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.

[feature] occ trashbin:restore should support Groupfolders
6 participants