group shares should respect to users auto accept preference

[karakayasemi]

Description

If auto accept enabled by admin and if it is a group share, create sub-share for auto accept disabled users in the pending state to respect users auto accept preference.

Related Issue

Fixes #34705

Motivation and Context

Removing bug.

How Has This Been Tested?

• Enable auto-accept share in the admin sharing panel
• Create testuser and testgroup.
• add testuser to testgroup
• login with test-user and disable auto-accept share from the personal panel
• share a file to testgroup with admin user
• the share should be created in pending status for testuser

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

Open tasks:

• Backport (if applicable set "backport-request" label and remove when the backport was done)

[codecov]

Codecov Report

Merging #34850 into master will increase coverage by <.01%.
The diff coverage is 92.85%.

@@             Coverage Diff              @@
##             master   #34850      +/-   ##
============================================
+ Coverage     65.35%   65.35%   +<.01%     
- Complexity    18484    18488       +4     
============================================
  Files          1208     1208              
  Lines         69969    69977       +8     
  Branches       1280     1280              
============================================
+ Hits          45727    45734       +7     
- Misses        23870    23871       +1     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	53.04% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	66.77% <92.85%> (ø)	18488 <0> (+4)	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...es_sharing/lib/Controller/Share20OcsController.php	85.42% <92.85%> (+0.03%)	198 <0> (+4)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a3e711...5a37f4e. Read the comment docs.

[jvillafanez]

Code looks fine. I have some questions:

If the admin has set autoaccept and the group has one member with explicit autoaccept disabled, the user will have his share in a pending state. However, what happens the other way around? What if the admin doesn't have set the autoaccept, but the user has it enabled? I expect the user to autoaccept the share (or maybe this situation isn't possible, I don't remember)

For the code, I think we should start considering splitting things. The createShare method has more than 200 lines of code. Just having a quick look:

• Recommended maximum number of lines of code is 50. Even if we consider just logic lines or sentencies, I'm pretty sure it will be above 100 lines. This method is becoming very big.
• Recommended maximum complexity for a method is 10. I might be counting wrong but the complexity is around 40-50. The method is becoming more complex.

As long as the logic works fine, for me it's ok to merge this PR (double-check the above case first) and figure out a refactor plan at some point.

[karakayasemi]

What if the admin doesn't have set the autoaccept, but the user has it enabled?

@jvillafanez like you said if it is disabled by admin, user can not even see this option. User only can configure when it is enabled. We have also acceptance tests for this case.

For the code, I think we should start considering splitting things. The createShare method has more than 200 lines of code. Just having a quick look:

Actually, most of the sharing code base are complex and some parts of it are including legacy things. We should definitely consider to refactor it. However, this PR is only a bug fix. I prefer to make it out of this PR's scope.

[jvillafanez]

@jvillafanez like you said if it is disabled by admin, user can not even see this option. User only can configure when it is enabled. We have also acceptance tests for this case.

So if it's disabled, we ignore whatever option the user could have been set (?).
I mean, the admin might have enabled the autoaccept, and the user might explicitly set the autoaccept (the preference is written for that user). Then the admin decided to disable the autoaccept. So technically, this scenario is still possible.

If that's what we want and there are test covering that scenario, then it's ok. I think we should somehow document this in order to make the behaviour as clear as possible, although I'm not sure where.

[karakayasemi]

So if it's disabled, we ignore whatever option the user could have been set (?).

Yes. It is decided in that way.

I guess you are describing the following scenario:
https://github.com/owncloud/core/blob/master/tests/acceptance/features/webUISharingAcceptShares/acceptShares.feature#L281
When admin disables auto-accept, user's auto-accept preference is not valid. A user can only disable auto-accept share for himself when it is enabled by admin. Also, he/she can re-activate it.

[PVince81]

@karakayasemi please backport