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

ONLY_THESE_USERS is poorly named and not as useful as it should be #10817

Closed
andrewandante opened this issue Jun 14, 2023 · 7 comments
Closed

Comments

@andrewandante
Copy link
Contributor

andrewandante commented Jun 14, 2023

Affected Version

CMS all of them

Description

The scope of InheritedPermissions::ONLY_THESE_USERS doesn't do what it says on the tin; instead, this functionality is better described as "only these GROUPS". Equally, this means you can't assign a specific user access to anything in this way - you have to jump through hoops to add them to a group first, which is often unnecessary double-handling.

Proposal

Minor release: add a new permission InheritedPermissions::ONLY_THESE_INDIVIDUAL_USERS that allows mapping to many Users, instead of groups. Mark this permission and the existing permission as deprecated. This adds the new functionality without breaking the existing API.

Major release: rename the above as follows:

InheritedPermissions::ONLY_THESE_USERS => InheritedPermissions::ONLY_THESE_GROUPS

InheritedPermissions::ONLY_THESE_INDIVIDUAL_USERS => InheritedPermissions::ONLY_THESE_USERS

PRs

ListboxField React PRs

@sunnysideup
Copy link
Contributor

sunnysideup commented Jun 14, 2023

and the individual users would have a MANY MANY relationship to the Object?

@andrewandante
Copy link
Contributor Author

As per comment here: #10819 (comment) there's now a demo repo with all these PRs in one place: https://github.com/andrewandante/silverstripe-only-this-user-demo/blob/main/composer.json#L11-L23

@GuySartorelli GuySartorelli self-assigned this Jul 4, 2023
@GuySartorelli
Copy link
Member

@andrewandante FYI this has now progressed from todo to in progress, so I'll be reviewing the PRs more thoroughly today. I've given them only quick glances until now to check for anything obvious - so there may be a few more changes requested.

@GuySartorelli
Copy link
Member

Thank you for this @andrewandante, this is a great contribution! It'll be released with 5.1 - we're aiming for an October release at the moment.

I've linked a docs PR to the issue description which someone else in the team will review.

@andrewandante
Copy link
Contributor Author

Magic, thanks so much for your guidance and patience! Onward!

@emteknetnz
Copy link
Member

Have merged docs PR

@sunnysideup
Copy link
Contributor

THANK you SO MUCH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants