-
Notifications
You must be signed in to change notification settings - Fork 6
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
adds admin role #217
base: 2.x
Are you sure you want to change the base?
adds admin role #217
Conversation
Thanks @markconroy Admin roles should get all permissions by default, so it's more for completeness, though there might be other needs for devs to refer to the admin role. |
With this added, would we then want to disable user 1? |
Thanks @andybroomfield. I have the constant added now. I don't think we should disable user/1, at least not initially. Let's leave it there for now, and let this settle in, then remove it later in a follow-up if we feel we should. === |
Installs and works as an admin role. |
@andybroomfield I'm not sure we want to add an update hook to add this to existing sites. I have a feeling that other sites may already have created their own role called 'admin' or 'administrator' or whatever and then set this, which we don't want to override. I think it might be safer to keep it as just for new sites. === |
My reasoning for suggesting the update hook is that it would be helpful for consitency that there is a reliably identifiable admin role, as we may need to add other functionality that checks if a user is an admin user if not for permissions in the future. I also suspect that a lot of sites out there are not using admin role as it wasn't part of the default install, so nudging them slightly in the right direction. |
I've an update hook added now to create the admin role, and set it as the admin. === |
Discussing in tech drop-in: not sure that we want to update existing sites with the new role. Happy with the role for new sites, maybe with docs / release notes on how to add to existing sites. |
Aha - sorry @andybroomfield - only just read your comments. Hmmm... let's discuss! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to add this, it works and adds the new role during an update.
I'm still uncomfortable with the update hook to all sites. @markconroy suggests a small module to define the role. @ekes - this is a good case for a recipe. @willguv the name 'Admin' seems fine. Also agree on avoiding the forced update. So we'll convert this to draft and make some changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the update hook.
Then either a module or a recipe to deploy consistent changes.
FWIW, +1 to the concept of small module to enable it on older sites. Let people do it with open eyes if they want it. I wouldn't go relying on a consistently named admin role for checking if a user has a role or not anyway... checking permissions is going to be much more reliable. Checking roles is a bit sketchier. |
Here's a PR for the module: #234 |
Since we now have PR #234 I wonder is this PR redundant? We can just set #234 to be enabled by default in the profile instead. That will ensure all future sites have it installed (handy if we want to apply updates to it) and all existing sites will have it if they desire it. Thanks to Big Blue Door for sponsoring my time to work on this. |
Closes #216
===
Thanks to Big Blue Door for sponsoring my time to work on this.