Allow to specify allowed groups to share instead of excluded groups #34115
Conversation
There was a problem hiding this comment.
Code looks good, I'm just unsure if the UI is good enough.
@jancborchardt This is the new UX
- Exclude groups from sharing / allow only groups to share
[ list-of-groups ]- Allow only instead of exclude
I wondering if the title of the group selector should be updated when the checkbox is updated
|
This is how it looks with the new UI: Also, should I update the translation files? |
|
No need to update the translation files, this is done automatically by scripts |
|
Just added testing 🙂 |
CarlSchwan
requested review from
jancborchardt,
a team,
ArtificialOwl and
icewind1991
and removed request for
a team
|
Hm good question. Sounds like it should be 3 radio options and an input:
And then an input field below the option which is chosen. @nimishavijay? |
|
Agreed with the UX, suggestion on the wording:
And when an option is selected the input field appears below that option like @jancborchardt mentioned |
|
Sounds good @nimishavijay! Would just switch around option 2 and 3 in the order, so it's sorted by permissiveness. |
|
New UI: |
cdammanintopix
requested review from
CarlSchwan
and removed request for
jancborchardt,
ArtificialOwl and
icewind1991
|
Hello, @CarlSchwan any updates on this? |
|
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? |
|
@jancborchardt sure, could be clearer. @nimishavijay ok with that change? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -86,7 +86,7 @@ | |||
| 'defaultExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_default_expire_date'), | |||
| 'expireAfterNDays' => $this->config->getAppValue('core', 'shareapi_expire_after_n_days', '7'), | |||
| 'enforceExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_enforce_expire_date'), | |||
| 'excludeGroups' => $this->getHumanBooleanConfig('core', 'shareapi_exclude_groups'), | |||
| 'excludeGroups' => $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no'), | |||
Check notice
Code scanning / Psalm
DeprecatedMethod Note
|
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? |
|
@cdammanintopix can you rebase and fix conflicts please? Cypress will fail because it breaks on forks |
skjnldsv
added
enhancement
2. developing
|
@skjnldsv done 😉 |
skjnldsv
added
3. to review
|
@cdammanintopix lint issue (see comment) Can you also reword your commit to follow conventional commits? |
skjnldsv
added
4. to release
|
Hello @skjnldsv I rebased and fixed the conflicts. |
|
Hello @skjnldsv I finally managed to reproduce the issue in the No DB unit tests (PHP 8.1) locally and fixed it. |
|
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? 🙂 |
|
Aaaand we have conflicts again 🙈 EDIT: rebased |
… of excluded groups Relates to #3387 Signed-off-by: Corentin Damman <c.damman@intopix.com>
|
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 |
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