Shortcut permission names are misleading

Actually, the "Edit own shortcut set" allows you to edit the set you are currently using. Here's the function (note that the machine name for this permisison is 'customize shortcut links'):

function shortcut_set_edit_access($shortcut_set = NULL) {
  // Sufficiently-privileged users can edit their currently displayed shortcut
  // set, but not other sets. Shortcut administrators can edit any set.
  if (user_access('administer shortcuts')) {
    return TRUE;
  }
  if (user_access('customize shortcut links')) {
    return !isset($shortcut_set) || $shortcut_set == shortcut_current_displayed_set();
  }
  return FALSE;
}

So... I'm OK with "Select any shortcut set" being the name for the "select" permission.

I think the edit permission needs to say "Edit selected shortcut set", to be consistent with the "select" permission's wording, and what the edit permission actually allows you to do. It doesn't have anything to do with whether you or someone else created the shortcut set. It only has to do with if it's your currently-selected shortcut set.

And those two changes should also make it clearer on the Permissions page that someone who is given both the permissions basically has permission to edit any shortcut set on the system (by first selecting then editing that set).

There's a typo in the patch on #2:
"... and each user with Select any shortcut set permission to select a shortcut site created by anyone at the site...."

to select => can select

I'm also not sure about "Administer all shortcut sets". That doesn't seem to match the other permissions in core, such as "Administer content" and "Administer content types". ??

ADDED LATER: To be explicit -- I think this should just be left as "Administer shortcuts" to make it match the other permissions in core.

I think it should be Administer shortcuts. Permissions describe the actions you can take, not the name of the module (which is kind of invisible for most actions). For instance for the Node module, the master-admin permission is called "Administer content", not "Administer Node" (anyway, if we were using module names it would have to be "Administer everything in Node module" I think, whereas "Administer content" is clearer and more concise).

I completely approve of the patch in #10.

I think we should get a 3rd opinon before marking it RTBC however, probably from someone who's been involved in recent other issues on shortcuts, such as Bojhan or anyone else who's been commenting on #647084: Missing shortcut edit and delete operations on shortcuts admin. Try #drupal-contrib on IRC to find them?

I think this is good, especially the "Select any shortcut set" part.

I'm not 100% sure about "Edit selected shortcut set" - how clear is it from that that the user might not have selected it themselves, but rather had it assigned to them by someone else? I agree that "own" is a bit confusing here (although nice in one way, in that it imitates the any/own distinction used elsewhere, e.g. in the node module permissions).

I dunno, maybe: "Edit displayed shortcut set"?

I also think if we need more clarity on this one, we might consider adding a permission description. The Shortcut module used to have some permission descriptions until many of the permission descriptions were removed from core en masse, but we could probably start fresh and come up with a better one instead.

I thought the word "selected" was pretty clear here, actually, especially since the other permission was called "Select any shortcut set".

The word "displayed" to indicate the shortcut set someone is currently using is in the code, but is not used in the UI anywhere, I think.

Yes, that can be done. But it should only be done if we are unable to make the permission strings themselves clear.

Hm, you mean like in #12 where I suggested we might add back a permission description here... or something else? :)

Anyway, agreed that it would be nice if we can make things clear without relying on a description.

I kinda want to RTBC this one, but my brain is still getting a bit caught on the "edit selected" wording.... Is it possible we are all overthinking this a bit, and that leaving that part at "edit own shortcut set" is fine after all? It's consistent with the rest of Drupal, and while it's true that there are subtleties here (e.g., it's not really your "own" because you might not have created it, or you might be sharing it with other users, etc), I'm not sure those are really any different than the subtleties associated with "edit own ___ content". In the case of content, just because you "own" it doesn't mean that you're the only one who can edit it (and certainly not the only one who can view it), nor does it even mean you created it; an administrator might have created it and entered your name in the author field. So are the shortcut permissions really that different?

Another option is 'current' or 'active' (instead of 'selected' or 'displayed')?

The problem with using the word "own" here for the shortcut set you've selected, in my opinion, is that elsewhere in permissions it means "that you created", whereas here it means "that you selected".

Hmmm...

OK, I think we do need to have descriptions. How about this?

- Select any shortcut set
From all available shortcut sets, select one to be own active set. Without this permission, an administrator selects shortcut sets for users.

- Edit currently selected shortcut set
May affect other users if shortcut sets are shared. If also granted Select any shortcut set permission, effectively grants permission to edit any available shortcut set.

OK Shai, the two of us agree - let's get a patch made. I can do it in a few hours probably (quite busy at the moment) unless you get there first. :)

Here's a patch with the changes from #18. Please, someone review it. :)

And for reference, here's a screen shot of the shortcut permissions, with this patch applied.

Incidentally, I tried to make the permission name EM there, but it didn't work (i.e. If also granted Select any shortcut set permission). The permissions page must be stripping out the HTML tags in the description field.

Doh!

Ummmm...

A user without Select permission can still be using a shared set of shortcuts, if an admin assigns the same shortcut set to more than one person. So the ability to affect other users is not limited to people with "Select any shortcut set" permission. That's why I wrote it that way. I thought the word "shared" conveyed that idea?

I'm OK with removing the word "available" from the second description.

Yes, better! Except now the second sentence seems stylistically disconnected from the first and kind of jarred me reading it... How about:

Editing the currently selected shortcut set will affect other users if that set has been assigned to or selected by other users. Granting "Select any shortcut set" permission along with this permission will grant permission to edit any shortcut set.

This might be personal taste, but I find 'Edit currently selected shortcut set' terribly long. It has one word too much.

I am completely happy with the latest patch. Since I participated in writing it, though, I guess I shouldn't mark it RTBC (but I would if I could).

#32: 685020-32-better-shortcut-perms.patch queued for re-testing.

I love the permission names now, but the descriptions feel a little long.... I think in general we want to keep permission descriptions as short as possible. A couple of those sentences don't seem like they're adding too much information. Would this work instead?

"Edit current shortcut set":
Editing the current shortcut set will affect any other users who have selected that set or been assigned to use it.

"Select any shortcut set":
Without this permission, users can only see the current shortcut set that was assigned to them.

I think the main idea is to put information in the descriptions that wouldn't be obvious at first glance, and only if it is important from a security perspective or adminstration perspective.

With this in mind, I'm in favor of keeping the "Granting "Select any shortcut set" permission along with this permission will grant permission to edit any shortcut set." warning on "Edit current shortcut set".

I don't like the second proposed change much either... "see" -- where? "current shortcut set" -- what does that mean?

I think the proposal in the patch is clearer:
"'From all shortcut sets, select one to be own active set. Without this permission, an administrator selects shortcut sets for users.'"

Hm, well if it isn't somewhat clear from the permission names alone that "Select any shortcut set" and "Edit current shortcut set" give you (effective) permission to edit any set, I'm not sure we've named them right? It's probably only a small number of people who will care about that anyway. I'm not totally opposed to leaving it in, but it feels long.

For the second one, I was trying to match the terminology between the two permissions, so that both refer to "current shortcut set". Otherwise, do you think it will be clear to people that "active set" (in the second description) refers to the same thing as "current set" (in the first one)?

Either work for me. Therefore, I'm delegating the decision on this to jhodgdon -- she can roll or approve the final patch, and I'll commit what she recommends me on this issue.

Ah, the power!

Committed to CVS HEAD.