Allow to specify allowed groups to share instead of excluded groups

[cdammanintopix]

In my use case, I use Nextcloud to share files with a lot of people that are members of separate groups. I do not want those people to be able to share files between them, nor to share files with me. However, I want to allow sharing from my team to those groups.

The feature I need is then to be able to specify allowed groups to share, instead of excluded groups. Hence, I can create a group with my team, allow it to share, and do not allow this feature to any other groups (without having to list all of them).

Suggested UI:

Relates to #3387

[cdammanintopix]

This is how it looks with the new UI: Let me know if I should change it.

Also, should I update the translation files?

[CarlSchwan]

No need to update the translation files, this is done automatically by scripts

[cdammanintopix]

Just added testing :slightly_smiling_face:

[jancborchardt]

Hm good question. Sounds like it should be 3 radio options and an input:

Limit sharing based on groups

• Allow sharing for everyone (default)
• Exclude some groups from sharing
• Limit sharing to some groups

And then an input field below the option which is chosen. @nimishavijay?

[nimishavijay]

Agreed with the UX, suggestion on the wording:

Allow sharing for

• everyone (default)
• only some groups
• everyone except some groups

And when an option is selected the input field appears below that option like @jancborchardt mentioned

[jancborchardt]

Sounds good @nimishavijay! Would just switch around option 2 and 3 in the order, so it's sorted by permissiveness.

[cdammanintopix]

New UI:

[cdammanintopix]

Hello, @CarlSchwan any updates on this?

[jancborchardt]

Actually thinking about it again accessibility-wise, I’m wondering if the sentence-like wording @nimishavijay suggested works well with a screen-reader. My previous suggestion reads a bit better when you only look at the radio buttons, and also when you use a screen reader to tab over the items – what do you think @cdammanintopix @nimishavijay?

[cdammanintopix]

@jancborchardt sure, could be clearer. @nimishavijay ok with that change?

[cdammanintopix]

I rebased on the latest master commit, and updated the UI as @jancborchardt suggested. The new UI looks like this:

[cdammanintopix]

Hello, do you think you could review this? @CarlSchwan This small pull request was opened about 10 months ago 🙂

[colin969]

Any day now I'm sure

[cdammanintopix]

Hello @CarlSchwan @icewind1991 any chance you could review this for next beta?

[cdammanintopix]

I updated the code to match the lastest release. The new UI looks like this

[CarlSchwan]

Code looks good but I lost a bit touch with Nextcloud codebase so I prefer if someone like @icewind1991 or @come-nc review this

[cdammanintopix]

So after your review @come-nc do you confirm you approve this pull request?

[cdammanintopix]

Checking why the cypress tests failed

[cdammanintopix]

Checking why the cypress tests failed

Seems like if I rename the branch "master", all tests pass. Do you know if this is a CI bug or not? Does it fail because this pull request comes from a fork?

[skjnldsv]

@cdammanintopix can you rebase and fix conflicts please?

Cypress will fail because it breaks on forks

[cdammanintopix]

@skjnldsv done 😉

[skjnldsv]

@cdammanintopix lint issue (see comment) Otherwise good for me.

Can you also reword your commit to follow conventional commits? I suggest something like feat(files_sharing): allow to specify allowed groups to share instead of excluded groups

[cdammanintopix]

Hello @skjnldsv I rebased and fixed the conflicts. Should I worry about this test No DB unit tests (PHP 8.1) failing?

[cdammanintopix]

Hello @skjnldsv I finally managed to reproduce the issue in the No DB unit tests (PHP 8.1) locally and fixed it. I rebased and fixed the conflicts. Could you start the tests? All should pass now 🙂 (except Cypress of course)

[cdammanintopix]

Seems like all tests passed (except Cypress that fails on forks), could you merge this then, so that it is included in the next beta? 🙂

[skjnldsv]

Aaaand we have conflicts again :see_no_evil: Sorry @cdammanintopix lots of merges lately, this is frustrating, we're aware of the issue :smiling_face_with_tear:

EDIT: rebased

[welcome[bot]]

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