adds admin role

[markconroy]

Closes #216

===

Thanks to Big Blue Door for sponsoring my time to work on this.

[andybroomfield]

Thanks @markconroy Remember to add a constant to https://github.com/localgovdrupal/localgov_core/blob/04bb3a54d3b7e9ff05db3b1fb0863538f155db99/modules/localgov_roles/src/RolesHelper.php also.

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.

[andybroomfield]

With this added, would we then want to disable user 1?

[markconroy]

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.

=== Thanks to Big Blue Door for sponsoring my time to work on this.

[andybroomfield]

Installs and works as an admin role. We should also add an update hook to install this, then I can approve.

[markconroy]

@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.

=== Thanks to Big Blue Door for sponsoring my time to work on this.

[andybroomfield]

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.

[markconroy]

Hi @andybroomfield

I've an update hook added now to create the admin role, and set it as the admin.

=== Thanks to Big Blue Door for sponsoring my time to work on this.

[finnlewis]

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.

[finnlewis]

Aha - sorry @andybroomfield - only just read your comments.

Hmmm... let's discuss!

[finnlewis]

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.

[anthonylindsay]

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.

[markconroy]

Here's a PR for the module: https://github.com/localgovdrupal/localgov_core/pull/234

[markconroy]

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.

[stephen-cox]

We can just set https://github.com/localgovdrupal/localgov_core/pull/234 to be enabled by default in the profile instead.

Agreed - makes more sense just to enable the module given it's included.

[markconroy]

I'm closing this in favour of #234

---

Thanks to Big Blue Door for sponsoring my time to work on this.