-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix: promote re-shares when deleting the parent share #47425
base: master
Are you sure you want to change the base?
Conversation
88e1618
to
941934a
Compare
The failing tests are related. The test failing is testing this scenario:
It fails at the step in bold. If I reorder the transferring code and use The more I think about it the more it feels like it’s not the job of transfer-ownership to delete broken shares. |
After internal discussion, reshares should not be deleted when share is deleted but turned into direct shares instead. We should still be careful about what that means for transfer-ownership when incomming shares are not transfered. |
56e6eff
to
1527836
Compare
To make sure I understand this correct:
I think there was a discussion recently with @jancborchardt if this is expected behavior. |
Yes that is correct
Before deleting the share to B, A sees shares both to B and C in the UI. There is no visible distinction between the share and the reshare. |
7c3f054
to
f389c6f
Compare
9968018
to
8aeedbc
Compare
8aeedbc
to
8974542
Compare
Rebased and fixed code style. |
lib/private/Share20/Manager.php
Outdated
} | ||
|
||
if ($node instanceof Folder) { | ||
$sharesInFolder = $this->getSharesInFolder($userId, $node, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about reshares in subfolders?
$sharesInFolder = $this->getSharesInFolder($userId, $node, false); | |
$sharesInFolder = $this->getSharesInFolder($userId, $node, false, false); |
But I think to remember that the shalow
argument is somehow deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shallow was removed in #44460
So, indeed, no idea how to get all shares in a folder subtree apart from recursing which would kill performance.
@icewind1991 Is there a trick we can use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List all shares for a user and then filter them by path.
It's far from great but should be better than recursing into folders
Note: Removed part about fix command from original PR Signed-off-by: Luka Trovic <luka@nextcloud.com> Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Canceling the previous add of deletion of invalid shares in transferownership because in some cases it deletes valid reshares, if incoming shares are not transfered on purpose. Inverting the order of transfer between incoming and outgoing so that reshare can be migrated when incoming shares are transfered. Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de> Signed-off-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
… but promoted Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
ab967de
to
e584e9b
Compare
Summary
Same as #43025 but without the new command.
Catch share exception in transfer ownership and delete invalid sharesChecklist