backdrop-issues icon indicating copy to clipboard operation
backdrop-issues copied to clipboard

[D9] Allow adding descriptions to user roles (and show them in the roles listing page)

Open klonos opened this issue 3 years ago • 2 comments

This is the same as https://www.drupal.org/project/drupal/issues/256287:

It would be useful to be able to document on the site why a certain role exists.

The feature is currently provided by the contrib module https://github.com/backdrop-contrib/role_help (that module also allows for a separate help text field that is shown to the user, but I don't think that this should be provided by core).


Advocate: @quicksketch

klonos avatar Aug 27 '22 21:08 klonos

I'd like to advocate for this (hopefully very simple) feature for 1.24.0.

quicksketch avatar Dec 06 '22 20:12 quicksketch

I think that this would be a very nice new feature in core!

stpaultim avatar Dec 08 '22 21:12 stpaultim

Initial PR for review: https://github.com/backdrop/backdrop/pull/4271

In particular, reviewing the default role descriptions assigned to anonymous, authenticated, and administrator would be helpful. Here's what I have in the PR:

  • anonymous: Special role assigned to any site visitor that has not signed into the site.
  • authenticated: Special role assigned to all users who have signed into the site. Permissions assigned to this role are given to all other roles except the anonymous role.
  • administrator: This role receives all permissions on the site.

The display of the description I modeled on the content type descriptions, with an additional column on the role listing page:

image

I also added the description to the title attribute of the Permissions page, which makes it show up on hover:

image

This PR still needs test coverage for the new description value.

I briefly read over https://github.com/backdrop-contrib/role_help and I'm not sure if it makes sense to provide an upgrade path from that contrib module. I'd be curious to hear what @bugfolder might say regarding adding this to core and what path he would recommend for the contrib module.

quicksketch avatar Dec 09 '22 07:12 quicksketch

This is a great idea! I've had a quick look at the default descriptions which look good to me.

I'd suggest to also add a description for the Editor role, something like: This role is by default configured to give sufficient permissions to content editors.

olafgrabienski avatar Dec 09 '22 09:12 olafgrabienski

I'd be curious to hear what @bugfolder might say regarding adding this to core

I'd support adding the functionality to core, but there should be a graceful path for users of the contrib module.

and what path he would recommend for the contrib module.

I will look into this deeper in several days when I'm back from travel; not in a position right now to look at the code. Note that role_help is a D7 module, so we'll need to gracefully accommodate users upgrading from D7, as well as users of the B module.

bugfolder avatar Dec 10 '22 02:12 bugfolder

All checks on all PHP version failed. I don't think we ever had that before. :wink:

All failing checks are user-related. So I'm guessing, this needs more work. Undefined index: #description

indigoxela avatar Dec 15 '22 14:12 indigoxela

I've done a little tinkering with and without the Role Help (role_help) module after applying this patch. The good news is that they more or less peacefully coexist (no smoke or flames). Role Help stores its extra info (summary and description) in a new db table (role_help), while this patch stores its new info (description) in config.

(In retrospect, since most role info moved into config, the module upgrade from D7 probably should have replaced the db table with config fields. Maybe a future version of the module could copy the description data from the table into config and then remove the table.)

Role Help actually has you enter two bits of new information: a description, which is longer and used for admin purposes (this is basically equivalent to the new description config field), and then a summary, which is shown on the user edit page. There is no equivalent of the summary in this new patch. I would like to suggest that we add this; if users can see their roles on their account edit page, it's informative to give them some clue about what those role names mean. See comparison screen shots below.

In what follows, anything prefixed by [RH] is a help field from the Role Help module. No-prefix description is what you get with this core change.

So first, let's compare the admin Roles listing. First, this module:

roles_patch

Next, with Role help enabled:

roles_rh

With RH enabled, it takes over the display of the roles, using its own information.

Now, the configuration page for a role. First, this patch (for the anonymous role):

configure_patch

Next, configure page with RH enabled.

configure_rh

We see both fields, those from this patch, and those from RH. Saving does save all of the information.

The last thing to compare is what one sees on the user edit page. Since RH has a summary field, we see that here with RH enabled:

user_rh

With RH disabled, we just see the role names, no summaries:

user_patch

I think the summary information is useful to provide to users, and so would like to suggest that a summary field be added to this PR.

At that point, the PR would offer pretty much everything that RH offers. (RH also offers a separate admin page listing the roles, but that's not really needed since the description already appears on the admin role page under this PR.) So we could provide a more graceful path for users by copying the data from the role_help table into config via an update hook.

Alternatively, since there's only a handful of users of Role Help, we could also just add an update hook to that module that copies the data from the table into config and then disables the module (or suggests that the admin disable it), since it would no longer be needed. Then core would not need to include any Role Help-specific update code. But we (I) would probably need to have the updated version of Role Help module ready when 1.24.0 comes out so that both could be updated together.

bugfolder avatar Dec 16 '22 02:12 bugfolder

Thanks @bugfolder for this audit! Role Help's descriptions on the fields are a little confusing to me, since it says both fields are shown when editing a user's profile. Which one is actually used?

quicksketch avatar Dec 16 '22 22:12 quicksketch

Yeah, I noticed (with the benefit of fresh eyes, not having looked at it in a long time!) that the description wasn't clear.

Checking again, the summary is listed on BOTH the user edit page and on the Roles listing page. The longer description field (which is field help in the role_help table) shows up on the RH-specific listing of roles (a separate page, not the core Roles page) and on the RH-specific Roles tab of the user account.

So I was a bit confused above. (Hey, I only ported the module, didn't design it). Since RH is using the same summary information on both the user edit page and the admin Roles page, the longer description/help field is only used on the pages that are RH-specific (and I'm not advocating adding those new pages to core); so I guess we could go with a single field after all.

Although, I would just toss out for consideration whether it would be be useful to have separate description text on the user edit page and the admin page. The text shown to the user could be briefer and more function-oriented, while the admin information might be lengthier and more helpful to a site architect. For example, for the user:

Editor — this role allows you to create new stories on the site.

For the admin:

Editor — give this role to anyone who creates stories. It will allow them to post any HTML without further moderation.

But that's just a thought, not a strongly urging.

bugfolder avatar Dec 16 '22 22:12 bugfolder

I think that the admin page could have a different description, as that page is for users who assign roles to users. There should be information useful for them to give the correct role, for example information useful to understand the difference between two roles that could seems closer than they are.

avpaderno avatar Dec 17 '22 08:12 avpaderno

I'm also in favor of allowing for both a long description (for use in the roles listing page) and a short description (for when people are assigning roles to user accounts). We have a similar paradigm with blocks actually, where we provide an "Admin label" and an "Admin description":

I think that calling these "admin description" and "help text" in our case with roles would make sense.

If the UI seems overwhelming, then I'd suggest that we consider either a collapsed fieldset that holds these fields, or follow the checkbox paradigm that Views UI uses in admin/structure/views/add:

PS: we are using the same (but reverse) checkbox paradigm in admin/config/system/site-information for the "Use the logo supplied by the active theme" and "Use the shortcut icon supplied by the active theme" options too.

klonos avatar Dec 21 '22 15:12 klonos

Follow-up to today's Dev discussion of how to handle migration from role_help module. One of the things we discussed was potentially having Backdrop Core copy the information from the role_help table into the new core field(s). That could be problematic for those migrating from D7, because of the changes in role handling (no more rid, now use role). The role_help module takes care of that conversion durings its hook_update_1000() call. So it might be cleaner to leave the copying of role_help data into the (new) core fields to another role_help update hook that can be guaranteed to run after the conversion of the role_help table.

So, for D7 upgraders, they would install the (new) Role Help module into their Backdrop site, then run updates as usual, which would move all their Role Help descriptions into the new core Backdrop fields.

For existing Backdrop sites, they would update their Role Help module to the new version, whose update would copy the fields from the role_help table into the new core field(s).

And for those who updated their Backdrop site but not the Role Help module, they would still see extra fields on the role edit page (both core and Role Help), but it wouldn't create any harm (beyond possible confusion).

bugfolder avatar Dec 23 '22 00:12 bugfolder

I updated my PR at https://github.com/backdrop/backdrop/pull/4271

  • Fixed PHP notices
  • Added test coverage
  • Split the single description into admin_description (Admin description) and description (Help text). Which matches terminology in other places like Layout, Views, and Field module.
  • Added upgrade path if role_help module is installed. Copies the summary value into both description and admin_description to preserve the current behavior of Role Help.
  • The "help" property of Role Help module that previously showed a dedicated page is not included. If Role Help wants to continue providing that page in a new version, that might be a good use for the module, but I think the 90% use-case here is getting the shorter descriptions displayed (which core now does).

I wrote a draft of a changelog as well that we can link to from the update hook: https://docs.backdropcms.org/change-records/user-roles-now-have-descriptions-role-help-module-in-core (logged-in account required since it is unpublished).

quicksketch avatar Dec 29 '22 03:12 quicksketch

Thanks @quicksketch ...code looks good, but the linter is complaining about a few things still.

If the UI seems overwhelming, ...

That is not the case with the role create/edit forms, since they previously only held the role name field (and possibly the machine name field if people edited it manually). With the two description fields added, it still feels fine.

Adding the descriptions in the role names in the headers of the permission tables is a nice touch, although kinda "hidden feature" if you don't know it's there. We could style these with a dotted underline as a sort of "hover over me" hint perhaps, but can be a follow up.

klonos avatar Dec 29 '22 06:12 klonos

Added a follow-up issue in Role Help module: https://github.com/backdrop-contrib/role_help/issues/3.

bugfolder avatar Dec 29 '22 15:12 bugfolder

Discussed in dev meeting: We need to accomodate for non-upgraded Drupal 7 role names like 0 and 1 instead of anonymous and authenticated.

quicksketch avatar Dec 29 '22 21:12 quicksketch

PR updated to account for numeric role IDs "1" (anonymous) and "2" (authenticated) if present in the role_help database table.

I think all remaining PHPCS warnings are unrelated to this issue (unmodified lines by this PR).

I re-tested the upgrade path and it seems to work well as far as setting new descriptions but not overwriting existing descriptions if present. Testing this against a role_help installation would be appreciated.

quicksketch avatar Dec 30 '22 01:12 quicksketch

Applied the PR patch to a site that had Role Help installed with many roles defined.

  • Update proceeded fine, with the "Backdrop core now provides most of the functionality of Role Help module..." warning message shown at the end of update.
  • Roles listing at admin/config/people/roles still shows the Roles Help data and format (as expected).
  • Disabled Role Help module.
  • Roles listing at admin/config/people/roles now shows the new format, and is still showing the short descriptions that had been in the Role Help module.
  • Selected "Configure" for a role. Both the short and admin descriptions show the same information, which was the short description from the Role Help module.

Summary: it's doing what it's supposed to do.Testing-wise, WFM, LGTM.

bugfolder avatar Dec 31 '22 02:12 bugfolder

Also did an update on an ordinary site without Role Help. The four standard roles all got their descriptions on the Roles listing. On the user edit page user/N/edit, I don't see descriptions for the roles. Wasn't that part of the intended behavior?

bugfolder avatar Dec 31 '22 03:12 bugfolder

I don't see descriptions for the roles. Wasn't that part of the intended behavior?

The current implementation only adds admin-descriptions, not descriptions on the user account form. Only users moving from role_help maintain the current user account form descriptions. Both approaches keep things as similar as possible to the current implementation, though we definitely could add user account form descriptions for new sites; I just didn't know exactly how people would want to use these public descriptions.

quicksketch avatar Dec 31 '22 05:12 quicksketch

Yeah, while testing the PR sandbox and trying to see that from a new user's point of view, I kinda expected at the very least a short description for the "odd" there-but-locked authenticated role.

On freshly-installed sites, the editor and admin roles are kinda self-explanatory, but can we add a description for "Authenticated" at the very least? ...something along the lines of:

Automatically assigned to everyone that logs into the site.

I believe that the words "automatically" and "everyone" in the help text would do a good job explaining why the role is there, but cannot be removed.

Other than that, I'd be happy to RTBC this, especially after @bugfolder's testing 🙏🏼

klonos avatar Dec 31 '22 08:12 klonos

Here are the descriptions I came up with:

  • Authenticated: Automatically assigned role to all accounts.
  • Editor: Permission to create and edit content.
  • Administrator: All permissions, including the ability to assign roles.

image

This iteration I changed it to add the descriptions even on upgrades, so existing sites with these roles will now receive descriptions.

quicksketch avatar Jan 01 '23 00:01 quicksketch

Saying that the admin role receives "all permissions" makes me nervous, since that does not actually describe the admin role, it describes the "Administrator role" setting.

I do think it would make sense to append something similar to the description for whichever role has this setting.

But we shouldn't use it for a default value. As soon as I change the "Administrator role" setting so that my "developer" role gets all the permissions, this description becomes incorrect, and is likely to cause confusion and frustration down the road.

If we changed it just a tiny bit to something like Elevated permissions instead of All permissions that would make me feel a lot better. I'll post some alternatives for consideration.

Description / Long versions: (appears on roles page and permissions matrix)

  • Anonymous: Special role assigned to any visitor that is not logged into the site.
  • Authenticated: Special role assigned to all accounts. Permissions assigned to this role are given to all other roles except the anonymous role.
  • Editor: Role that can edit content on the site, but cannot modify website configuration.
  • Administrator: Role that receives elevated permissions for website structure and configuration, including the ability to assign roles.
Screen Shot 2023-01-02 at 5 25 49 PM

Hep text / Short versions: (appears on user/edit form)

  • Authenticated: Automatically assigned to all accounts.
  • Editor: Permission to create and edit content.
  • Administrator: Permission to manage configuration, including the ability to assign roles.
Screen Shot 2023-01-02 at 5 23 39 PM

jenlampton avatar Jan 03 '23 01:01 jenlampton

The new descriptions seem fine, but one thing that is unclear is whether an Administrator can edit content. From the descriptions, it might seem as though Administrators can only manage configuration and not modify content, and it might seem like both editor and administrator roles need to be assigned.

Saying that the admin role receives "all permissions" makes me nervous, since that does not actually describe the admin role, it describes the "Administrator role" setting.

I don't think the potential for discrepancy between the admin role setting and the Administrator account is much of a problem. Descriptions throughout Backdrop can become incorrect if you modify other configuration, like the descriptions on default content types and views.

For what it's worth, the update hook matches on the admin role setting, not on the role named "administrator". So if an existing site has a different setting (such as a "Developer" role), that role that will receive the current description of "all permissions".

quicksketch avatar Jan 03 '23 01:01 quicksketch

Counter suggestion for the short descriptions:

  • Editor: Can create and edit content.
  • Admin: Can configure the site, including managing permissions and assigning roles.

klonos avatar Jan 03 '23 01:01 klonos

I like those too :) @klonos

I don't think the potential for discrepancy between the admin role setting and the Administrator account is much of a problem. Descriptions throughout Backdrop can become incorrect if you modify other configuration, like the descriptions on default content types and views.

My primary concern was that there was no description for the role. (The only description provided was one for the setting.) And with no other context except a possibly incorrect description, that seemed like a problem.

Changing "all" to "elevated" makes the description match the role regardless of the setting, plus we've also aded some more wording around configuration and structure in the newer examples :)

one thing that is unclear is whether an Administrator can edit content.

I wondered about that too. We could make the short description a smidge longer: Permission to mange content and configuration, including the ability to assign roles.

jenlampton avatar Jan 03 '23 01:01 jenlampton

Admin: Can configure the site, including managing permissions and assigning roles.

On second thought, Admin's don't usually manage permissions, but they do almost always assign roles.

Adding "managing permissions" may be another example of not describing the role itself, but instead, describing the permissions that are assigned to that role at install time.

Keep in mind that people who are changing things on the permission matrix are not going to come back and edit the role descriptions. We should try to build something that's going to remain useful, and if we add things that are likely to be inaccurate, that could actually end up being a real pain point for people.

jenlampton avatar Jan 03 '23 02:01 jenlampton

On second thought

On third thought, we don't need to get the text exactly right tonight. Small string changes like this could get in after code freeze!

I've tested the PR and it is working nicely for me. I've also done a light code review, and I think the PR looks good. My only concern is changing the strings for the defaults, otherwise this is RTBC.

Sooooo.... if we want to wait on the strings until a later date, I am happy to RTBC at any time :)

jenlampton avatar Jan 03 '23 02:01 jenlampton

I incorporated combined suggestions for the role descriptions:

  • Authenticated: Automatically assigned to all accounts.
  • Editor: Can create and edit content.
  • Administrator: Can manage content and configuration, including the ability to assign roles.

image

I left admin descriptions as-is.

quicksketch avatar Jan 03 '23 02:01 quicksketch

Yeah, lets jus go with that as "good enough for now", and iterate (if needed) before final release as @jenlampton said 👍🏼

klonos avatar Jan 03 '23 02:01 klonos