Download & Extend

Move permissions to a tab at admin/people/permissions

Project:Drupal core
Version:7.x-dev
Component:user.module
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (fixed)
Issue tags:D7UX, Usability

Issue Summary

User permissions are currently off to the side at admin/config/people/permissions, webchick suggested making them a tab of admin/people at http://drupal.org/node/672252#comment-2428022

Makes sense to me, should be an easy enough patch. webchick also mentioned that this breaks the 'settings in config' paradigm - I actually disagree that permissions are settings, at least in Drupal they're as important as anything under /structure (not that we should put them there though, oh no).

Comments

#1

Title:Move permissions» Move permissions to a tab at admin/people/permissions

#2

#3

Status:active» needs review

Patch & screenshot attached.

AttachmentSizeStatusTest resultOperations
people-permissions.patch23.81 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch people-permissions.patch.View details
screenshot-people-permissions.png17.26 KBIgnored: Check issue status.NoneNone

#4

sub

#5

I support this. Subscribe.

#6

So, I talked about this with Dries and actually discussed this in length before many times. The reason it was put up there, because People is sort of an entity for other objects to be attached to. For example, events - showing as a tab on People.

This paradigm would go up, if we treat entities in their own right in the IA. But sadly we don't, given that the events as a tab is also kind of an edge case.

So if we move Permissions, there are couple of next steps to take "Roles" should be a secondary navigation item to Permissions.

And on the Configuration page, we need to move out "Permissions "Roles", and rename the category to People.

#7

Rerolled to take Bojhan's comments into account.

AttachmentSizeStatusTest resultOperations
people-permissions.patch27.36 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch people-permissions_0.patch.View details

#8

Could we get screenshots of the latest patch?

And +1,000,000 for this change. :P From the "old" new IA ;), the modules page and the permissions page were the two top ones that no one could ever find. We moved modules to top-level, and since then I've seen no complaints about finding it. If we move permissions up to "People" I think that will eliminate 90% of the grief people give the new IA.

(As a side note, I think it's awesome that of all the re-jiggering of pages we did this release, there were really only two that a wide swath of the population had problems with. Bravo!)

#9

Screenshots attached, one of the trimmed and renamed "People" section on the configuration page, and one of the extended people section.

AttachmentSizeStatusTest resultOperations
screenshot-people-permissions.png17.15 KBIgnored: Check issue status.NoneNone
screenshot-configuration-people.png25.13 KBIgnored: Check issue status.NoneNone

#10

That looks fine to me. Let's get final approval from yoroy/Bojhan.

#11

I support this. Having People/Permissions next to each other as tabs is a handy thing. Roles as a sub of permissions is great too, it's a very likely next destination.

Only thing that worries me (just a) bit is that we now use 'People' as a label twice in different contexts. I guess for now that's ok, better to keep a clear relation than forced renaming of the Category ("More people"? ;-) "Accounts"?)

#12

Status:needs review» reviewed & tested by the community

Ok, RTBC

We can open a new issue to discuss the label, I am unsure as we obviously named this People keeping in mind that a lot of modules would add their module under People and thats still appropriate.

#13

Status:reviewed & tested by the community» fixed

Awesome, thanks folks!

Committed to HEAD!

#14

I support this too. I definitely think its better, given that permissions is buried without this patch.

However, one major nit I have is that we need a more clear definition of tab use. For example, under Content, tabs are used to splice different kinds of content. Under Appearance, tabs are not splicing different kinds of themes. The same is true for Modules. And now, the same is true for People. Seems the most common use of tabs is to splice methods of managing (List view/manage stuff in the list). I'm not sure there's time to flush out a comprehensive solution for this, but it's definitely an itch of mine for d8.

#15

Agreed. We split out the main actions ('Add') from the tabs and made them links, aligned to the left. Remaining tabs are still a mixed bag. I think we're looking at D8 for that indeed.

#16

Status:fixed» needs review

One broken test less!

While the path has been changed, the test tried to assert that the title of the page is Permissions, which isn't the case anymore. Changed to the first permission on that patch, I don't think that shows up anywhere else. I'm open for other ideas though.

AttachmentSizeStatusTest resultOperations
fix_usertests.patch827 bytesIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_usertests.patch.View details

#17

Status:needs review» needs work

Well, the other test was more granular, since it was testing for that string in the title. This one checks for it anywhere.

It's a pretty stupid test though. :P

#18

Status:needs work» needs review

The new patch looks for an active link to admin/people/permissions, that is displayed in form of the 2nd level local task.

Chx also suggested to remove the assert completely as it doesn't test much.

AttachmentSizeStatusTest resultOperations
fix_usertest.patch857 bytesIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_usertest.patch.View details

#19

Status:needs review» fixed

That works! Committed to HEAD.

#20

Status:fixed» needs review

Ok, the above patch as crap and only works when drupal is not installed inside a subdirectory.

This is better, thanks DamZ!

AttachmentSizeStatusTest resultOperations
fix_usertest.patch873 bytesIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_usertest_0.patch.View details

#21

Status:needs review» reviewed & tested by the community

Thanks Berdir. Let's fix the test bot :)

#22

Status:reviewed & tested by the community» fixed

Committed to HEAD!

#23

Status:fixed» needs review

Sorry, I'm not sure what I was thinking, DrupalWebTestCase::getAbsoluteUrl() cannot be used in this context, as it is just an internal function to the internal browser (to handle relative paths in redirects and stuff). url() is the correct way to do this, and the only way for this to be safe for both clean and normal URLs.

Here is a patch.

AttachmentSizeStatusTest resultOperations
699842-fix-at-last.patch886 bytesIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 699842-fix-at-last.patch.View details

#24

Status:needs review» fixed

haha. third time's a charm? :)

Committed to HEAD!

#25

#26

Status:fixed» needs work

This is not really completed.

The menu item descriptions of admin/people and admin/config/people are now wrong. admin/config/people still mentions permissions, and admin/people does not

You can see the latter menu item description in Toolbar (if you have toolbar.module enabled) by hovering over the People menu item. The former can be seen on path "admin".

#27

Assigned to:Anonymous» jhodgdon

There were also several URLs on various pages that are still using the old values.

I'm patching...

#28

Assigned to:jhodgdon» Anonymous
Status:needs work» needs review

Here's a patch. I've verified that all the links on the user module's help page at least now go to the right spot, and I don't think any of the old URLs are in core any more. Descriptions of a few menu items also updated to reflect what's actually in those menu trees.

AttachmentSizeStatusTest resultOperations
699842cleanup.patch5.46 KBIdlePASSED: [[SimpleTest]]: [MySQL] 17,852 pass(es).View details

#29

Status:needs review» fixed

Committed to CVS HEAD. Thanks.

#30

Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.