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

[UX] Add help text to the roles listing that explains how the order is used. #4504

Closed
ghost opened this issue Jul 29, 2020 · 37 comments · Fixed by backdrop/backdrop#3888
Closed

Comments

@ghost
Copy link

ghost commented Jul 29, 2020

On /admin/config/people/roles you have the ability to re-order the roles, but there's no explanation what this is for. We should add some documentation explaining this.

For the record, Drupal 7 has this help text:

Roles allow you to fine tune the security and administration of Drupal. A role defines a group of users that have certain privileges as defined on the permissions page. Examples of roles include: anonymous user, authenticated user, moderator, administrator and so on. In this area you will define the names and order of the roles on your site. It is recommended to order your roles from least permissive (anonymous user) to most permissive (administrator). To delete a role choose "edit role".

By default, Drupal comes with two user roles:

  • Anonymous user: this role is used for users that don't have a user account or that are not authenticated.
  • Authenticated user: this role is automatically granted to all logged in users.

It explains that you can, and suggests how, but doesn't explain why... Surely we can do better.


Translation update:

New strings:

  • 'Roles provide a way to organize people into groups that have similar duties. Each user account can be given one or more roles, and <a href="@permissions">permissions</a> can be granted to those roles. To make the <a href="@permissions">permissions</a> page easier to navigate, it is recommended that roles be ordered from the least permissive (Anonymous) to the most permissive (usually Administrator).

PR: backdrop/backdrop#3203
PR: backdrop/backdrop#3888

@ghost ghost added the type - task label Jul 29, 2020
@ghost
Copy link
Author

ghost commented Jul 29, 2020

Also, creating a new role adds it to the end of the list, after 'Administrator'. Assuming Drupal's recommendations about ordering apply to Backdrop too, maybe new roles should be placed between Authenticated and Administrator...?

@klonos
Copy link
Member

klonos commented Jul 29, 2020

Thanks for raising this @BWPanda 👍

To delete a role choose "edit role".

^^ we don't need that 🙂

By default, Drupal comes with two user roles

^^ that should be removed, and perhaps those bullet points can be added in the form of description text bellow each respective role.

@klonos klonos changed the title Describe ability to re-order roles [UX] Describe ability to re-order roles Jul 29, 2020
@klonos
Copy link
Member

klonos commented Jul 29, 2020

Initial stab at this: backdrop/backdrop#3203

image

The text is a bit wordy, so please feel free to suggest tweaks. cc @jenlampton @stpaultim @BWPanda and others.

Does this portion provide any value, or are we stating the obvious?:

In this section, you can add or delete roles, define the role names, and change their order.

Also, should the help text be at the top of the page, or come right after the roles table, and before the fieldsets?

@jenlampton
Copy link
Member

jenlampton commented Jul 29, 2020

maybe new roles should be placed between Authenticated and Administrator...?

Brilliant.

The text is a bit wordy

Yeah, nobody is going to read this much text. I think separating it into two sections is a good idea @klonos. Let's see if we can get each one down to 1 or 2 sentences a most. How about:

Roles provide a way to organize people into groups that have roughly the same duties. Each user account can be given one or more roles, and [permissions](http://3203.backdrop.backdrop.qa.backdropcms.org/admin/config/people/permissions) can be granted to those roles (instead of directly to user accounts). 

In this section, you can add or delete roles, define the role names, and change the order. To make the permissions page easier to navigate, we recommended ordering roles from the least permissive (Anonymous) to the most permissive (usually Administrator).

Highlights:

  • two paragraphs (maybe we move the 2nd one?)
  • users becomes user accounts if we mean accounts, or people if we don't.
  • removed you and make actions passive.
  • removed the sentence This allows updating permissions for entire groups of users, without having to define permissions individually for each user. since it duplicated what was said in the previous sentence.

Is the bold in the PR just to show what changed? Since changing the order is the least important thing on this page, it shouldn't be emphasized any more than all the other things.

@klonos
Copy link
Member

klonos commented Jul 29, 2020

maybe new roles should be placed between Authenticated and Administrator

I think I like this suggestion 👍

To provide some context for some of the suggestions I'm making bellow, here's a screenshot of what this page looked like in D7:

image

Here are some other things that I thought we could improve on this page, or that I could use some feedback for:

  1. In D7, there was no way to directly delete a role from this page - the help text stated that To delete a role choose "edit role" 👎 ...we've improved the UX in this by providing a "Delete role" option in the operations drop-button 👍

    Now, as a curious user, if I click the drop-button on the first role in the list, I see no delete action available. If I try the next role, it's the same ...so do I keep going, or assume that none of the roles can be deleted? We may know what/why, but we shouldn't be assuming that it is common knowledge that the anonymous and authenticated roles cannot be deleted, whereas the rest can. To that effect, I have changed the vague "required" note next to these special-cased roles with "system role - cannot be deleted", which I think helps this situation (the respective D7 text for this was "locked"). Let me know what you think.

  2. I have made the mention to the Permissions page a link, but I think that the UX in D7 was better, with the secondary tabs that allowed to easily switch between the "Roles" and "Permissions" page. I understand that in Backdrop we have moved these pages under "Configuration" instead of directly under "People", but should we consider making those (and the other sections under Configuration -> User accounts) tabs? (this will most likely need to be a follow-up) ...or should we alternatively just add some help text at the top of the "Permissions" page, linking back to the roles page?

  3. Should we allow descriptions to be added to user roles?

  4. There was an entire section of CSS that was trying to put the "Add role" button next to the respective text field. In D7, both the text field and the button were rendered in the same table cell, with colspan="4", but we've moved the button in the "Operations" column in Backdrop. Do we prefer it that way (visually, it makes sense to have it horizontally aligned with the rest of the operations drop-buttons), or should we move it back next to the text field (less distance for the mouse to travel)? I personally don't have a specific preference one way or the other, but as it is, the CSS that was achieving that was not necessary; so I've removed it.

@jenlampton
Copy link
Member

jenlampton commented Jul 29, 2020

@klonos these changes are getting way out of scope :)

I have made the mention to the Permissions page a link

I did that in my text changes too! (to decrease verbosity)

or should we alternatively just add some help text at the top of the "Permissions" page, linking back to the roles page?

I like this idea, but yes definitely should be a follow-up issue.

There was an entire section of CSS...

Unrelated, follow-up issue please :)

@ghost
Copy link
Author

ghost commented Jul 29, 2020

I bolded some text in my quote of Drupal's text to show the part I thought was relevent. But happy with whatever text you guys agree to add, as long as it explains the reordering ability.

@klonos
Copy link
Member

klonos commented Jul 29, 2020

@klonos these changes are getting way out of scope :)

Of course ...in case you haven't noticed all these years, once I start working on issues, I tend to turn them into "overhaul the UX in this entire admin page". Sorry, and thanks for putting up with my over-excitement 😅 ...will definitely split those to separate follow-ups 👍

Is the bold in the PR just to show what changed? Since changing the order is the least important thing on this page, it shouldn't be emphasized any more than all the other things.

I bolded some text in my quote of Drupal's text to show the part I thought was relevent.

Oh, I thought it was to emphasize the security best practice implied.

Thanks for the very prompt feedback 🙏

Working on implementing changes/suggestions...

@klonos
Copy link
Member

klonos commented Jul 29, 2020

...I still think that this bit of text has value, because it is explaining the "why" of the previous sentence:

This allows updating permissions for entire groups of users, without having to define permissions individually for each user.

...but I agree that we should keep things short, so I'll remove it 👍

On the other hand, as I said earlier, I find this bit to be stating the obvious, and of very little value:

In this section, you can add or delete roles, define the role names, and change their order.

...the reordering ability is already implied by this:

It is recommended to order your roles from the least permissive (Anonymous) to the most permissive (Administrator)

@jenlampton
Copy link
Member

jenlampton commented Jul 29, 2020

I still think that this bit of text has value, because it is explaining the "why" of the previous sentence:

What do you see in here that isn't in the previous sentence?

This allows updating permissions for entire groups of users, without having to define permissions individually for each user.
vs
permissions can be granted to those roles (instead of directly to user accounts).

Maybe we replace directly with individually?

permissions can be granted to those roles (instead of individually to user accounts).

On the other hand, as I said earlier, I find this bit to be stating the obvious, and of very little value:

Okay, let's kill it :)

@klonos
Copy link
Member

klonos commented Jul 29, 2020

Maybe we replace directly with individually?

Yup, that would do it 👍

PR updated:

maybe new roles should be placed between Authenticated and Administrator...?

Brilliant.

Done. I've implemented this in a simple way, so that new roles are added directly after the authenticated role. We could make that more elaborate, but it'd have to be a separate issue I think.

@jenlampton
Copy link
Member

Done. I've implemented this in a simple way, so that new roles are added directly after the authenticated role. We could make that more elaborate, but it'd have to be a separate issue I think.

Is that in a separate PR, or this one for the text change?

@klonos
Copy link
Member

klonos commented Jul 29, 2020

Same 😅

Moving it....

@klonos
Copy link
Member

klonos commented Jul 29, 2020

...unless now that we've added this bit of help text it makes it part of this PR:

...we recommended ordering roles from the least permissive (Anonymous) to the most permissive (usually Administrator).

This is the commit with this specific change (if amount of change matters to make a decision): a188b7c (#3203)

@jenlampton
Copy link
Member

jenlampton commented Jul 29, 2020

I've done a quick code review and spotted a few minor issues, so marking NW. Testing now :)

@jenlampton
Copy link
Member

jenlampton commented Jul 29, 2020

Done. I've implemented this in a simple way, so that new roles are added directly after the authenticated role. We could make that more elaborate, but it'd have to be a separate issue I think.

This doesn't behave reliably the way it's currently implemented. The first time you add new roles it's fine, because all the weights for the new roles are the same, so it places them alphabetically. But if you then re-order the roles, then try to add another new role, it behaves very strangely.

I think rather than trying to put it one position after the Authenticated role, we should try to place it one position before the Administrator role. Not only does this more closely match what people will be doing initially (since people would usually add more than one role in increasing order of permissions) but it will also work nicely when people return at a later date and add another new role.

From an implementation standpoint, we'll need to pull all the current roles by weight, insert the new one at the position where we'd like it, then save all the roles that have new weights. If we are assuming most people leave the Administrator role as the most permissive, in most cases this would mean only saving the new role, and the Administrator role. Alternatively, to place a role after the Authenticated role, we'd likely need to save them all.

Since this is no longer a simple change, I recommend we do this in another issue and keep this one to the text change. :)

@klonos
Copy link
Member

klonos commented Jul 29, 2020

Since this is no longer a simple change, I recommend we do this in another issue and keep this one to the text change. :)

Agreed: #4507

@quicksketch quicksketch added this to the 1.20.1 milestone Sep 15, 2021
@quicksketch quicksketch modified the milestones: 1.20.1, 1.20.2 Oct 11, 2021
@jenlampton jenlampton modified the milestones: 1.20.2, 1.20.3 Nov 18, 2021
@quicksketch quicksketch modified the milestones: 1.20.3, 1.20.4 Dec 3, 2021
docwilmot added a commit to docwilmot/backdrop that referenced this issue Jan 2, 2022
@docwilmot
Copy link
Contributor

New PR removes everything but the original issue as in the title: adds help text describing the point of re-ordering.

@herbdool
Copy link

herbdool commented Jan 2, 2022

I agree about @docwilmot approach to keep it simple and focused on the title. I've RTBCed that PR.

I think everything in addition to that should be in a new issue since it needs more discussion.

@stpaultim
Copy link
Member

I'm in agreement. I was about to mark this "Works for Me."

Thanks @docwilmot and @herbdool for work today.

@klonos klonos removed their assignment Jan 2, 2022
@klonos
Copy link
Member

klonos commented Jan 2, 2022

Thanks everyone 🙏🏼 ...I'll go through this issue and file follow-up issues as required.

@quicksketch
Copy link
Member

I merged backdrop/backdrop#3888 and closed out backdrop/backdrop#3203.

Thanks everyone for your work and collaboration in working this out!

@jenlampton jenlampton changed the title [UX] Describe ability to re-order roles [UX] Add help text to the roles listing that explains how the order is used. Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants