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

Added support for transferring incoming file shares. #28118

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

ipasanec
Copy link
Contributor

@ipasanec ipasanec commented Jul 22, 2021

Fix #21856

@PVince81

  • new option --transfer-incoming-shares=1 | 0
  • new config.php option 'transfer-incoming-shares' => true | false 'transferIncomingShares' => true | false

The command line option overrules the config.php option.

@szaimen szaimen added the 3. to review Waiting for reviews label Jul 22, 2021
@szaimen szaimen added this to the Nextcloud 23 milestone Jul 22, 2021
@szaimen szaimen added pending documentation This pull request needs an associated documentation update enhancement feature: occ labels Jul 22, 2021
apps/files/lib/Command/TransferOwnership.php Outdated Show resolved Hide resolved
apps/files/lib/Command/TransferOwnership.php Outdated Show resolved Hide resolved
apps/files/lib/Service/OwnershipTransferService.php Outdated Show resolved Hide resolved
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Had a deeper look at the logic and added some comments

apps/files/lib/Service/OwnershipTransferService.php Outdated Show resolved Hide resolved
apps/files/lib/Service/OwnershipTransferService.php Outdated Show resolved Hide resolved
apps/files/lib/Service/OwnershipTransferService.php Outdated Show resolved Hide resolved
apps/files/lib/Service/OwnershipTransferService.php Outdated Show resolved Hide resolved
apps/files/lib/Service/OwnershipTransferService.php Outdated Show resolved Hide resolved
@PVince81
Copy link
Member

PVince81 commented Aug 18, 2021

  • BUG: must not transfer incoming share if not in the requested path:

    • user2 shares "test" with user1
    • user1 creates dir "sub"
    • occ files:transfer-ownership user1 user2 --path=sub -vvv --transfer-incoming-shares=1
    • expected: "test" stays
    • actual: "test" was transferred even though it was not in "sub"
  • BUG: transferred incoming share doesn't stay in folder

    • user2 shares "test" with user1
    • user1 creates dir "sub"
    • user1 moves "test" into "sub"
    • occ files:transfer-ownership user1 user2 --path=sub -vvv --transfer-incoming-shares=1
    • expected: "test" is inside "sub" for user2
    • actual: "test" is outside of "sub" and even outside the transferred folder

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes so far.
I had a few more comments after doing some tests.

See test results above.
I also took the liberty to push an integration test to your branch for the reported cases 😄

@ipasanec
Copy link
Contributor Author

Originally the customer only required the share transferring to the users root directory,
I now added the behaviour to take in account the original share path and path restriction.

@PVince81
Copy link
Member

@ipasanec I have rebased the PR. You might need to delete your local branch and check it out again.

I've also pushed a fix for the path matching, but for some reason the new integration test fails and I'm trying to debug it.

@PVince81
Copy link
Member

@ipasanec seems we had a collision and pushed something at the same time

I've resolved the conflicts.

I think for now it looks good, I'll check why the integration test isn't passing despite representing the scenarios I manually tested.

@PVince81
Copy link
Member

I've fixed the tests, there was a regexp mismatch and I used the wrong wording.
They should pass now.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 I hereby approve the code from @ipasanec

second reviewer should also check my commits

ipasanec and others added 2 commits August 27, 2021 17:55
- new option --transfer-incoming-shares=1 | 0
- new config.php option 'transfer-incoming-shares' => true | false

The command line option overrules the config.php option.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member

I've rebased and squashed to reduce the number of commits and remove the merge commits.

@PVince81 PVince81 self-assigned this Aug 27, 2021
@skjnldsv skjnldsv merged commit 995aa65 into master Sep 15, 2021
@skjnldsv skjnldsv deleted the transfer-incoming-shares branch September 15, 2021 08:26
@welcome
Copy link

welcome bot commented Sep 15, 2021

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@PVince81
Copy link
Member

/backport to stable22

@solracsf
Copy link
Member

Shouldn't be this documented too? 🤔

@PVince81
Copy link
Member

PVince81 commented Sep 15, 2021

I'll take care:

@PVince81 PVince81 removed the pending documentation This pull request needs an associated documentation update label Sep 15, 2021
@PVince81
Copy link
Member

docs done!

@Vilasamuni
Copy link

I've been looking for documentation about how to use this flag and it's not clear to me what the values mean. I've seen --transfer-incoming-shares=1 and --transfer-incoming-shares=2 but I haven't found any explanation of what 1 and 2 actually do.

I've got backups, but I'm wary about running commands on real-world data with flags that I don't understand.

Short term request: please explain to me what they do.
Medium term suggestion: update the documentation to make this clearer.

@ChristophWurst
Copy link
Member

ChristophWurst commented Nov 10, 2021

Short term request: please explain to me what they do.

https://github.com/nextcloud/server/pull/28118/files#diff-e299680f9e139f82c630e100e9a4750851e51598a493f185ce8fc7baf2544ea3R127-R147

Medium term suggestion: update the documentation to make this clearer.

Please open a ticket at https://github.com/nextcloud/documentation/issues/new

For any further questions please use https://help.nextcloud.com/c/support/7. Github is not a support forum.

@ipasanec
Copy link
Contributor Author

The basic usage is:
--transfer-incoming-shares=1 for transferring incoming shares explicitly and 0 for not transferring them explicitly.
This command line option overwrites the value of 'transferIncomingShares' in the config.php if set.
If nothing is set, neither in command line nor in config.php, incoming shares are not transferred.

The default value of '2' for an unset option is necessary to detect a missing value in the command line and give an error message to notify the user.
The only valid values for --transfer-incoming-shares are 0 or 1.

btw. there is no --include-incoming-shares option. (It was changed in the original project, hence some variables use 'include' in the name)

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.

Transfer "incoming shares" in occ files:transfer-ownership command
8 participants