Skip to content
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

Allow to specify allowed groups to share instead of excluded groups #34115

Merged
merged 1 commit into from Mar 15, 2024

Conversation

cdammanintopix
Copy link
Contributor

@cdammanintopix cdammanintopix commented Sep 16, 2022

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:
image

Relates to #3387

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cdammanintopix
Copy link
Contributor Author

cdammanintopix commented Sep 16, 2022

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

Also, should I update the translation files?

@CarlSchwan
Copy link
Member

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

@szaimen szaimen added the 3. to review Waiting for reviews label Sep 16, 2022
@szaimen szaimen added this to the Nextcloud 25 milestone Sep 16, 2022
@cdammanintopix
Copy link
Contributor Author

Just added testing 🙂

@cdammanintopix cdammanintopix force-pushed the allowed_grops_to_share branch 2 times, most recently from 0ff5e55 to 2b7bff6 Compare September 19, 2022 10:08
@CarlSchwan CarlSchwan requested review from jancborchardt, a team, ArtificialOwl and icewind1991 and removed request for a team September 19, 2022 10:17
@CarlSchwan CarlSchwan self-assigned this Sep 19, 2022
@jancborchardt
Copy link
Member

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?

@blizzz blizzz mentioned this pull request Sep 19, 2022
@nimishavijay
Copy link
Member

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
Copy link
Member

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

@cdammanintopix
Copy link
Contributor Author

New UI:
image

@cdammanintopix cdammanintopix force-pushed the allowed_grops_to_share branch 2 times, most recently from 0cea484 to 314a881 Compare September 19, 2022 16:20
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 27, 2024
@cdammanintopix
Copy link
Contributor Author

cdammanintopix commented Mar 5, 2024

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

@cdammanintopix
Copy link
Contributor Author

cdammanintopix commented Mar 12, 2024

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)

@Altahrim Altahrim mentioned this pull request Mar 12, 2024
@cdammanintopix
Copy link
Contributor Author

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? 🙂

@cdammanintopix cdammanintopix force-pushed the allowed_grops_to_share branch 2 times, most recently from bf34abe to d5527e9 Compare March 14, 2024 08:38
@Altahrim Altahrim mentioned this pull request Mar 14, 2024
@skjnldsv
Copy link
Member

skjnldsv commented Mar 15, 2024

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

EDIT: rebased

… of excluded groups

Relates to #3387

Signed-off-by: Corentin Damman <c.damman@intopix.com>
@skjnldsv skjnldsv merged commit d395715 into nextcloud:master Mar 15, 2024
54 of 55 checks passed
Copy link

welcome bot commented Mar 15, 2024

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

@cdammanintopix cdammanintopix deleted the allowed_grops_to_share branch March 15, 2024 18:13
@Altahrim Altahrim mentioned this pull request Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants