Give roles a description value

Patch still applied against CVS HEAD.

Subscribing. This seems like a really good idea.

Applying usability tag.

"autheticated" is a typo.

I like this patch. Descriptions for roles would be handy. I'd note that we have a couple of varying ways of showing examples in core; this one uses the 'Example: "authenticated user"' rather than the 'Example: authenticated user' approach. In another patch, one of these days, we should really conform these example strings to a single standard.

Needs screenshot

@webchick: if theres no complaint im going to roll this new functionality into the UI redesign of the roles page #393406: "Edit" links on roles admin page are confusing to users: "Edit permissions" should be the primary link.I dont link multiissue patches but they are very interrelated. Im going to mimic the content types page for the design and operation which leaves the ease of extension for contrib modules on the "edit link" and still lets us mitigate the permissions link on this page while adding your new functionality. ill have a patch for review on the other issue today or tomorrow morning.

@webchick: rolled your changes into the most recent patch on the issue above. also enabled editing of the "protected roles". your review and thoughts would be appreciated. the migration really simplified form validation and submission logic as well as creating a common UI that users already know from the content types page. im marking this issue as a duplicate but please revert if this isnt the best way to move forward.

Let's reopen this since the other issue wound up considering those changes to be out of scope.

There is some code in the other issue too though (in addition to the patch above). I assume it's pretty far out of date at this point though :)

The linked issue is one way to do it (although probably not for Drupal 8).

The duplicate issue (#1835668: Add description field to roles) pointed out that there's a contrib module which does this: https://www.drupal.org/project/role_help

But does it really make sense to require a contrib module for something essential like that?
Every field has a description field for a very good reason > inline documentation. So if roles don't become fieldable entity in the future, there should be clearly a help text, I think!

description field not present for roles

Reviving this issue since the referenced module role_help has not been ported and roles aren't fieldable.

Here is a patch to bring role descriptions into the interface. Patch is against 8.9.2, will update if necessary.

1. +++ b/core/modules/user/config/install/user.role.anonymous.yml
   @@ -3,6 +3,7 @@ status: true
   +description: ''
   
   +++ b/core/modules/user/config/install/user.role.authenticated.yml
   @@ -3,6 +3,7 @@ status: true
   +description: ''
   

   We could probably provide these with descriptions, for completeness.

2. +++ b/core/modules/user/src/RoleForm.php
   @@ -12,6 +12,13 @@
   +  /**
   +   * Role.
   +   *
   +   * @var \Drupal\user\RoleInterface
   +   */
   +  protected $entity;
   

   Poor naming choice / comment description.

   Edit: Not needed at all. This is declared in the parent class.

3. +++ b/core/modules/user/src/RoleListBuilder.php
   @@ -64,6 +64,7 @@ public function getFormId() {
   +    $header['description'] = t('Description');
   

   Utilise $this->t within classes.

Tests need to be added for this issue.

Addressed all issues I outlined in #26.

Added tests for the description functions as well as the RoleListBuilder.

Just wondering if this new description field would be useable or somehow extendable to attaching specific images (media) to roles?

For example, if I wanted a different icon to represent different roles and to show the correct icon next to user names in a view (my specific use case).

@W01F I think you are looking for #775734: Role as fieldable entity, this will only allow adding a single text field to roles.

Patch needs work as the tests failed entirely, also I don't think we can add methods to RoleInterface except in a major version jump as this is a backward compatibility break for anything that is currently implementing it.

I'm unsure why the tests failed like that, I'll give it another look.

This only adds new features and most cases where someone has overridden the roles entity they would be using this as a base already. But you are correct, this could be pushed to 10.0 in that case.

Test groups arent generated due to missing @group annotation in the new test added. Fixing that.

+++ b/core/modules/user/src/RoleListBuilder.php
@@ -72,6 +73,9 @@ public function buildHeader() {
+    $row['description'] = [
+      '#markup' => $entity->getDescription(),
+    ];

This is the only thing I don't like. I am not sure why it does not work without the markup.

Thank you for catching the group tag. : )

Fixing test cases!

core/tests/fixtures/config_install/multilingual.tar.gz needs to be updated. This is the cause for some of the test fails.

Still looking into how to fix the other errors caused by the configuration difference.

Trying to fix more tests!

Fixing the rest of the test cases!

Moving to "Needs Review" as all the tests are now passing.

Thanks for working on this!

1. +++ b/core/modules/jsonapi/tests/src/Functional/RoleTest.php
   @@ -88,6 +88,7 @@ protected function getExpectedDocument() {
   +          'description' => null,
   

   NULL should be uppercase.

2. +++ b/core/modules/user/src/RoleForm.php
   @@ -16,7 +16,10 @@ class RoleForm extends EntityForm {
   +
   

   Extra blank line at start of method.

3. +++ b/core/modules/user/src/RoleForm.php
   @@ -49,10 +58,13 @@ public function form(array $form, FormStateInterface $form_state) {
   +
   

   Same

4. +++ b/core/modules/user/src/RoleForm.php
   @@ -49,10 +58,13 @@ public function form(array $form, FormStateInterface $form_state) {
        // Prevent leading and trailing spaces in role names.
        $entity->set('label', trim($entity->label()));
   +    $entity->setDescription(trim($entity->getDescription()));
   

   Do we trim this for other descriptions? Do we need to update the comment to match?

5. +++ b/core/modules/user/src/RoleInterface.php
   @@ -97,4 +97,22 @@ public function getWeight();
   +  /**
   +   * Returns the description.
   +   *
   +   * @return string
   +   *   The description.
   +   */
   +  public function getDescription();
   +
   +  /**
   +   * Sets the description to the given value.
   +   *
   +   * @param string $description
   +   *   The desired description.
   +   *
   +   * @return $this
   +   */
   +  public function setDescription($description);
   

   I had BC concerns but I think looking at the policy we are OK: https://www.drupal.org/core/d8-bc-policy#interfaces

   "Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.

   However, we reserve the ability to add methods to these interfaces in minor releases to support new features."

6. +++ b/core/modules/user/src/RoleListBuilder.php
   @@ -72,6 +73,9 @@ public function buildHeader() {
        $row['label'] = $entity->label();
   +    $row['description'] = [
   +      '#markup' => $entity->getDescription(),
   +    ];
   

   Why #markup for the description but not for the label?

7. +++ b/core/modules/user/tests/src/Functional/Rest/RoleResourceTestBase.php
   @@ -52,6 +52,7 @@ protected function getExpectedNormalizedEntity() {
   +      'description' => null,
   

   NULL should be capitalised.

8. +++ b/core/profiles/demo_umami/config/install/user.role.author.yml
   @@ -40,3 +40,4 @@ permissions:
   +description: null
   
   +++ b/core/profiles/demo_umami/config/install/user.role.editor.yml
   @@ -44,3 +44,4 @@ permissions:
   +description: null
   

   We should set some good descriptions for the Umami roles.

9. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerExistingConfigTestBase.php
   @@ -107,7 +107,11 @@ public function testConfigSync() {
   +        'user.role.anonymous',
   +        'user.role.authenticated',
   

   Is this only to make the tests pass? This feels like working around a problem that should be solved elsewhere.

+++ b/core/modules/user/src/RoleInterface.php
@@ -97,4 +97,22 @@ public function getWeight();
+  public function getDescription();
...
+  public function setDescription($description);

A further thought: should we drop NULLs, use empty string as the default, and add typehints on the methods?

#3154762: [policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible is not yet policy but is starting to be followed in other new features I think.

I like the idea of using an empty string by default as opposed to NULL.

Good find on the interface documentation.

As for #40.6, I was not sure why the markup was needed but without it the rows would not render properly. Now I'm thinking it may have been the NULLs. If not, no idea.

#40.9, it's definitely skirting the issue. We'll need to find out how to have it come up as not changed.

Implemented suggestions from #40 except the 9th point. Still looking into the last point.

Do we also need to consider an upgrade path here? The descriptions should be set for anonymous and authenticated user roles on existing sites, I think.

Fixing code standard and set description = '' instead of NULL RoleResourceTestBase.php

It still needs work.

Fixing few test cases!

In the failed test, the minimal profile gets installed and it picks the config from core/modules/user/config but it doesn't get the "description" field in it. Hence the test cases fail. I have checked this issue, the config in the user module is correct but in the tests, the new config doesn't get updated. Can someone suggest a step forward.
Thank you

I think we need to update the configuration tarballs to fix the remaining tests

tests/fixtures/config_install/multilingual.tar.gz
tests/fixtures/config_install/testing_config_install.tar.gz

Triggering the tests

This is looking good, but I still think we need an upgrade path to add the description to anonymous and authenticated user roles, and tests for this.

Set to need work because of comment #50.

Adding post update to add description to anonymous and authenticated user roles and a test for it

Now patch looks good.
Set to RTBC.

Nice work, this looks really good now. RTBC+1

Great work, thank you all! This is a very helpful little improvement missing since Drupal 6 :)
RTBC+1

Setting to NW for #59

I've tried to reroll the patch in #53 for the latest 9.3.x (and 9.4.x). This went mostly okay, except for the changes to tests/fixtures/config_install/multilingual.tar.gz and tests/fixtures/config_install/testing_config_install.tar.gz, from which I'm not sure yet how to update these. So I'm leaving this issue to "Needs work" because of that.

@raman.b
Can you re-add the changes needed for the tarballs?

I've also updated the issue summary and added screenshots.

I'll try to work on this.

Sorry, I tried to work on this one but it's beyond my actual knowlege...

Changing the status to unassigned for someone with more experience take a look.

Hi! I'll try to help on this one more time...

Hi guys! Sorry... I really tried to help but didn't make it... So, I'm leaving for someone with more knowledge to work on...

Fixing failed tests of patch #63.

I'll work on this

I was able to fix the problems on the "Installer" tests. Could not make any progress on the "Config" tests, though.

Leaving unassigned and as "Needs work".

I'll fix the #71 patch and the interdiff because i removed some tests that i should not have removed.

Patch adjusted. Leaving unassigned.

2 years later I just saw this helpful one got stuck again as of #63.

So perhaps this should first be reviewed at a usability team meeting as of Needs usability review so we can do the right things here afterwards and finally fix this?

It's a pity these nice little usabillitytweaks get stalled. I would say this is fine to continue with. Content types, vocabularies, media types, menus all have their own description field, it's perfectly reasonable and useful to add similar for roles.

Edit: screenshots look fine, with one remark: the description text for the description text field in the edit-role screenshot essentially duplicates the label, without adding any more helpful context. It could probably be left.

Adding a condition on configs to not add a description on a role when it does not exist

Usability review

We've discussed this issue several times over the course of the last few months.

The first time we've discussed this issue at #3364446: Drupal Usability Meeting 2023-06-09. The link to the recording of the meeting is https://youtu.be/V_jT7Y9wWL8 . For the record the attendees at the meeting were @aaronmchale, @benjifisher, @emma-horrell, @rkoller, @simohell, and @worldlinemine.

We've revisited this issue at #3365687: Drupal Usability Meeting 2023-06-16. The link to the recording of the meeting is https://youtu.be/iriHrANnMKk . For the record the attendees at the meeting were @benjifisher, @blackbamboo, @rkoller, @shaal, and @worldlinemine.

The third and last time we've discussed this issue was in today's meeting at #3404028: Drupal Usability Meeting 2023-12-01. That issue will have a link to a recording of the meeting. For the record, the attendees at the usability meeting were @AaronMcHale, @anushrikumari, @benjifisher, @ckrina, @rkoller, @simohell, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

At first it has to be pointed out that there was a clear consensus right from the first meeting on that adding a description field to a role is a helpful and desirable addition.

The problem with the currently suggested descriptions is for one some sort of redundancy in the beginning of the sentences with "The role given..." or "The basic role give..." while the end of the sentences mostly mirrors the role name and doesn't add any extra value/information except for Anonymous users to a certain degree where it is stated that it is a role given to logged in users.

Therefore we've tried to come up with suggestions for the Anonymous user, Authenticated user, Content editor, and Administrator role during the three meetings as well as asynchronous discussions. It was sort of a rabbit hole if you are trying to get to a consensus for clear, concise, none redundant descriptions in plain language minding edge cases and scenarios. Three meetings plus asynchronous discussion in a Google Doc in between are testament to that. In today's call we came to the conclusion that this issue might "never" or at least not get in the near future. The missing consensus on the micro copy is just blocking a helpful and useful feature.

The group would suggest the following approach to proceed:

1. Make this issue only about providing/adding a description field to a role without adding a default description for any of the roles in the standard profile ( Anonymous user, Authenticated user, Content editor, and Administrator) and the additional roles for demo_umami (Author, Editor).

2. Move the work on the default role descriptions to a to be created follow-up issue.

3. In regards of micro copy the only thing left to change in this issue is the description for the description field on the role edit form. At the moment the description The description for this role. doesn't provide any value/information. It would be good to be inline with the proposed changes in #3365222: Make Description Field Labels Consistent:

For the field label use:
Description

For the field description use:
Displays on [the place where the description will be shown]

For a content type the description for the description is for example Displays on the Content types page. so analogous for the Roles page it would be Displays on the Roles page. there. That way the user knows where that information will be displayed and used.

one addition to #88.2 after some post meeting followup conversation on slack yesterday. Ideally there should be three followup issues:

1. Default role descriptions for the Standard profile
2. Default role descriptions for the demo_umami profile ( compared to standard it is missing the content editor role but has Author and Editor instead)
3. Discuss the implications if the user is changing the Administrator role on the role settings page (admin/people/role-settings). How should that change be handled in regards of the descriptions of the former Administrator role and the new Administrator role? Because if you change the Administrator role from the Standard or the demo_umami profile that Administrator role gets set back to the initial list of permissions which is actually none: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/profiles/sta... . Therefore its description would have to be updated and reflect those changes otherwise it would be misleading and confusing to the user. That brings up also the question if you update the description for the former Administrator role to something reflecting that this role isn't an administration role anymore still having the name Administrator could also be potentially puzzling. There is also an existing issue that deals with the potential consequences of changing the Administrator role and the possibility of locking yourself out: #3293874: Prevent accidental lockouts on the Role settings page and clarify the corresponding description for its select box.
   Plus how to handle the description of the new Administrator role permission. Was that new role previously one of the Standard roles or a role newly created by the user? Is the old description for a role becoming the new Administrator role be stored in case the Administrator role is changed again? All details that would have to be discussed and minded in the followup.

Thanks @rkoller! Let's finish this. I think we should turn this into a MR for the last steps. I'll see what I can do in the next days. This will be a tiny, but very helpful addition!

I think that the most useful of having a role description is show this description in the roles checkboxes as description in the user form and be available for others contrib modules like role delegation.

\Drupal\user\AccountForm::form

$form['account']['roles']

@jansete yes! Any plans to proceed here as written in #89 + #90?

It looks like that the list of remaining tasks is outdated. I tried to update it based on the information from #88.

I also looked in the code and I see the following:

1. If we are going to remove default descriptions for now, we no longer need the test 'AddUserRoleDescriptionTest'.
2. In RoleListBuilderTest::testListBuilder() I see the following:
   

   $this->assertSession()->pageTextContains($role->label());
   $this->assertSession()->pageTextContains($role->getDescription());
   

   I think that should be replaced with what is actually expected to be displayed on the screen:

   $this->assertSession()->pageTextContains('My Role');
   $this->assertSession()->pageTextContains('Lorem ipsum');
   

@anybody, @rkoller
Can you verify if the list of remaining tasks is complete? Did I forget something?

I'll open a followup issue for #88.2 now. will update the issue summary here accordingly and link the issue as soon as the issue is created.

I've created the follow up for #88.2, and updated the issue summary accordingly linking to #3470972: Create default role descriptions for the standard profile .

UPDATE: argh while now cross checking the question from @megachriz i've realized i'Ve forgot about my comment in #89. Need to update the issue and create a second one for umami as well. after that is done i will take another look if the rest of the remaining tasks is complete. apologies.

Based on #89 i've also created #3470976: Create additional default role descriptions unique to the demo_umami profile and updated the issue summary accordingly.

In regards of #93, i've made some additional slight adjustments to the issue summary. for the linked comments i've added a reference to the points made within the comment and i've added the point about #89.3 to the list of remaining tasks, that was missing so far. That way everything is covered that was raised in the ux meetings. @anybody might need to take another look if the rest of the remaining tasks looks also fine and complete.

This issue looks very nice, although we have to unify efforts if anybody need a temporary function avoiding conflicts with the final solution of this issue, I've implemented a little module: https://www.drupal.org/project/role_description

Since the latest patch no longer applied on Drupal 10.4.0, I've "rerolled" the patch into a MR. And with the following changes:

• Default descriptions are removed. @rkoller created follow-ups #3470972: Create default role descriptions for the standard profile and #3470976: Create additional default role descriptions unique to the demo_umami profile for this earlier.
• And because of that also config updates and their tests are removed.
• Made the $description property in the Role class protected instead of public.
• Added more strict typing for getDescription() and setDescription().
• Updated RoleListBuilderTest as per #93.
• Changed the description of the description field to "Displays on the Roles page", following the suggestion from #88.
• Moved tests for setting/getting description to a new Unit test called RoleTest.

Also attached a patch for Drupal 10.4.0, with the changes to tests removed. The MR is based on Drupal 11.1.x.

Hopefully, it passes tests.

Thanks

So MR should point to 11.x vs 11.1.x

Also with the change we will need an upgrade hook (plus tests) that sets the description to '' for all roles

@smustgrave
Setting default descriptions was postponed to a follow-up, to reduce the complexity of this issue and and to increase the likelyhood of getting this done (less chance for merge conflicts). Does it then still make sense to set them to empty strings here? The code ensures that getDescription() returns an empty string when the property is not set. Or should each property of a config entity always be set?
Also, the update tests made things more complicated when rerolling patches.