Group shares with same source and target

[rullzer]

Fixes #24575

Note that this is a very limited solution and eventually we want smarter
merging!

TODO:

• Add intergration tests
• Check if permissions are properly accumulated
• Check if javascript needs to be fixed

CC: @PVince81 @jonasheinisch

[mention-bot]

By analyzing the blame information on this pull request, we identified @icewind1991, @schiessle and @bartv2 to be potential reviewers

[rullzer]

Mmmm seems the js side needs more fixing...

[PVince81]

Wasn't there such logic already in the past ? Or did it get discarded through refactorings ?

[PVince81]

@icewind1991 do you remember anything ?

[rullzer]

Yes there was such logic. But it had to be discarded for various reasons.

• We can't group shares in the providers as there is no (future) guarantee that the current provider division is true for all installations
• We can't group shares in the providers since we support pagination. So if you only request the first N shares what to do.

At the same time I think this logic is now in the place where it should be. In the sharedMount/sharedStorage. Since that is the actual place where we group stuff.

[PVince81]

Doesn't help with #25186 which will need a different fix.

[PVince81]

I don't know what to do with this. It feels a bit dangerous to merge that late.
Also, unit tests are missing.

[PVince81]

don't add extra commas in JS, this used to break in IE

[PVince81]

Rebased

[PVince81]

Ouch, this breaks sharing: when opening the share panel it always says "Resharing not allowed" even for non-shared folders where I'm the owner.

[PVince81]

Some test cases for later, where "user1" is in "group1" and "group2"

• TEST: share folder "test" as outsider to "group1" and "user1" with same permissions
• TEST: share folder "test" as outsider to "group1" and "user1" with different permissions
• TEST: share folder "test" as outsider to "group1" and "group2" with same permissions
• TEST: share folder "test" as outsider to "group1" and "group2" with different permissions
• TEST: share folder "test" as outsider to "group1" and "group2" and "user1" (why not!) with different permissions
• TEST: share folder "test" as "user1" to "group1"
• TEST: share folder "test" as "user1" to "group1" and "group2" with same permissions
• TEST: share folder "test" as "user1" to "group1" and "group2" with different permissions

Most of these gave me two folders on v9.0.3 so let's hope this PR covers them all 😄

[PVince81]

After reverting the JS commit locally I'm able to share again, so something to look into.

I did a quick test with the different grouping cases and it seems to work nicely ! Great job !

The hardest part: write integration tests for them.

[PVince81]

@rullzer any reason for the JS fix ? Without the JS there is no duplication in the "Shared with others" and "Shared with you" entries. Unless I missed a case ?

[PVince81]

ah, reshares.. forgot those 😦

[PVince81]

• TODO: need repair step to recombine the wrongly duplicated folders (even if they were manually excluded as a workaround)

Sounds like fun.

[PVince81]

• TEST: unshare from self for a grouped entry, which one is affected ? => both
• TEST: reshare grouped entry => creates a new independent entry

[PVince81]

Rebased for CI

[PVince81]

Rebased and adjusted version for 9.2

[PVince81]

how many timeswill I need to rebase this ?

[PVince81]

Close enough for CI => merge

[PVince81]

stable9.1: #25534
stable9: #25543

[PVince81]

Looks like stable9 backport will be tricky, the old code is using old APIs and an old way. Or might require backporting more stuff.

[PVince81]

From what I see a lot of the code would require this commit 6123bad from #23919.

Because on stable9 the MountProvider and SharedStorage still use the old array-style shares instead of the ShareManager style. I'm not sure I want to risk backporting this, so will see if there is a way to make it work with the old code.

[PVince81]

stable9 PR #25543

[PVince81]

Looks like this fix is not enough, there's another scenario that is not covered: #25564
This is getting tiring...

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.