Switch Users should limit the users list to those who are allowed to use it

Ok, I'll buy that about reproducing problems in a specific user account.

I've only read the description of masquerade, and it seems to be an improvement over having to log out and log in among various accounts, but Switch Users is light-years ahead by showing a menu of users at all times and being able to switch with a single click, without even loosing your place in the site. I haven't actually used that yet, but I guess if you have System users with different themes, you should be able to switch between themes with a single click and without losing your place (as long as the new user has access to the page (1)).

I'm only just getting started with Drupal and haven't had much use for the other devel features yet, but Switch Users is absolutely fantastic, and if you trim the user list, it won't lose any of its appeal as the list of users grows. Maybe you can trim the list and add a menu item "other users" that expands to show all other users? Then you don't need to introduce a configuration option, and the added value of seeing who has the privilege will more than make up for the inconvenience of an additional click.

(1) One thing bothers me a bit: Whenever I change an administrative setting and switch to a normal System user to check the effect, I get an access denied error (because the normal user doesn't have access to the administrative page). Of course that's correct behavior, but it needlessly pollutes the log file. Would it be possible to check whether the page is accessible to the new user and redirect to /node if it isn't?

I just added my twelveth user and found out that the Switch User menu seems to be limited to 10 items. Is that true?

That makes it all the more desirable to filter out the users that don't have the Switch User privilege and keep those in the list that do. Can you switch to a user that is not on the list? Makes a submenu with the full user list look attractive. And sort the items, too. It's not always the same user that gets clipped, but often it's the administrator, and that obviously kills the feature!

Is there any chance that you might do some work on this in the next few days? I'm new to PHP, but I must have this functionality, and if I can't convince you to do it, I'll have to try to do it myself...

Ok, here's the first tiny iteration: change the sort order in the Change User list such that you get the ten most recently used users instead of the ten that haven't been active the longest.

At this point I don't know how to proceed. One possibility is this:

(add the user_roles() call, the if and the then branch)

if (user_access('switch users')) {
  $roles = user_roles(1, 'switch users');
  if (count($roles))
    $users = db_query_range('SELECT u.uid, u.name FROM {users_roles} ur INNER JOIN {users} u' .
                                    ' ON ur.uid = u.uid WHERE ur.rid IN (' . 
                                    implode(',', array_keys($roles)) .
                                    ') ORDER BY u.access DESC', 0, 10);
  else
    $users = db_query_range('SELECT uid, name FROM {users} WHERE uid > 0 ORDER BY access DESC', 0, 10);
  while ($user = db_fetch_object($users)) {
    $dest = drupal_get_destination();
    $links[] = l(check_plain($user->name), 'devel/switch/'. $user->uid, array(), $dest);
  }
}

This preserves the old functionality in the case where no roles have been given the "switch users" permission, i.e. the administrator gets the Switch User block with the 10 most recent (after the patch in #5) users. No one else gets the Switch User block.

If one or more roles have the "switch users" privilege, then it shows only those users. This might not include the administrator, if he doesn't have one of those roles. In the past (before the patch in #5) he also scrolled off the list when there were more than 10 users, but if the list was short, he was included.

If we may use subqueries, we can include the administrator with:

    $users = db_query_range('SELECT uid, name FROM {users}' .
                                    ' WHERE uid = 1 OR uid IN (' .
                                        ' SELECT uid FROM {users_roles} WHERE rid IN (' . 
                                        implode(',', array_keys($roles)) . '))' .
                                        ' ORDER BY access DESC', 0, 10);

If we may not use a subquery, we can do the subquery ourselves, adding one database access. Alternatively, we can check at the end whether we have the administrator and only if we don't, we can add it (but then it won't be in the proper sequence, and the code won't be pretty...).

Let me know which path you want me to pursue...

I have 5 System users, and when my Change User menu dropped down from 10 to 5, I found that I'd started to enjoyed seeing the 5 other most recently logged in users. So, where are we now:

1. With your current code, the active users scrolled off the list because the inactive ones were sorted to the top. This is a bug IMHO, and #5 fixes it.

2. #6 was my attempt to limit the list to only those users that have the "change users" privilege, with a nested query (or a kludge) to ensure that the administrator was part of the list.

3. After seeing the result, I'm backing off a little, and this is what this patch does:
a) take all uid's that have a role with "change users"
b) add uid 1 if not already there
c) fill any remaining spots up to 10 with other most recently logged-in uid's
d) populate the menu, ordered by access

This might result in more than 10 items, if more than 10 users have the "change user" privilege, but that's fine. All those should be in there IMO. It requires one or two additional database queries, but they're all straight one-table queries, and they add value.

This patch depends on the one in #5.

Thanks for reviewing and considering inclusion.

Here's the last iteration I can provide: It bothers me a bit to have two kinds of users in the list, those that can "switch users" and those that can't. This patch builds upon #7 and appends an asterisk to the user names that have the privilege, i.e. to those that'll let you switch back.

There's one thing that I don't understand in your code: what is the purpose of the WHERE uid > 0 clause? I haven't included it in my SQL -- if it's required, please add it.

I would have liked to check the accessability of the destination url, but I haven't been able to find out how to get the $nid from the url...

I would also have liked to add an expandable "all users" menu item, but I don't know how to do it, so this is how far I can take it. IAC, each iteration adds value, and you don't lose anything compared to what you have now: if there are 10 users or less, you'll always see all of them, if there are more than 10, you'll see the important ones (those that have the "switch users" privilege) plus the most recent ones up to 10.

Thanks for the encouragement, dww. I've just upgraded to 5.1 and ported the sum of the patches up to #8. Only slight changes were needed. Your review might help to get this into the module.

I'd still like to check the accessibility of the destination url and avoid going to the 403 page (or maybe just prepend a '!' to those user names that won't have access), but I don't know how to do it...

Thanks for getting the ball rolling!

#12: - always puts the users that can switch (if any) at the top of the list

I disagree with this. Having the entire list sorted by last access date is valuable information, and it's part of the original design. If you put the ones with the asterisks at the top you don't add any information (after all they're easy to spot in a list of ten), but you unnecessarily destroy information. I use some of the switchable users more often than others and having the entire list sorted will instantly give me an idea of how recently the non-switchable most recently logged-in users were on.

In my implementation, it can also happen that all switchable users are at the top: If I've just used all of them, that's the normal state, but if the lesser used ones don't tend to sink to the bottom of the list, then I know that my testers haven't been on-line for a while, and that's very useful information! Also, the very moment one of my testers comes online, I see his name rise to the top -- that too, is useful information — I might even give him a quick phone call, seeing that he's working for me and I'm not interrupting him in something entirely different.

There are other ways to get that information, but having it in the Switch Users block is free (code-wise, processing-wise, screen real estate-wise, and user attention-wise). Please revert the separation in two disjoint lists!

- never includes the current user as a valid target to switch to

I disagree with this, too. I've been using my patch for almost half a year, so I may be a little bit ahead of you :-). You save one spot out of ten, but you lose a lot of usability:

1. In the most common themes and setups the navigation menu shows the user name as the block title, but this is not necessarily so. If you have several browser tabs each may present the view of a different user (after all that's the motivation of this thread), and if you don't see the current user, you'll get confused pretty quickly.
2. Even if you see the name of the current user somewhere else on the screen, I find that I look at the Switch Users block very often (because I often click there, too), and having that information right there in the proper context is worth one item out of ten.
3. I don't just look for the current user name in the Switch Users block, I regularly click on it, too! Sorry about being so long-winded; when this Switch Users is in your daily routine then it's obvious, but if it isn't, I'll have to describe it in detail: let's say you have two switchable users, Webmaster and Member, and you're working on some feature for members that creates watchdog entries that you want to check. You exercise this feature as Member in browser tab 1. To check the log you open browser tab 2, switch to Webmaster and navigate to the Log. Then you flip back to tab 1, where you see Member's view, but your browser has only one session, and this session has been switched to Webmaster, and if you click a link or button in the Member view in tab 1, that action will be processed as Webmaster!

   You could flip back to tab 2, switch users to Member, but then the Log is replaced with your 403 page and you generate a 403 entry in the log because Member is not allowed to view the Log, and flip back-back to tab 1 to continue your Member session, but this is certainly not ideal. In my version of the patch, a "Member" link is at the top of the list in the Member view on tab 1, and I can just click on it to switch the session back to Member.

   After some interaction as Member, you want to check the Log again and you flip to tab 2. With my patch you'll see the Log and you click on "Webmaster" to switch the session to Webmaster and refresh the view. That's how it's supposed to work! If you take away the "Webmaster" entry (to save one out of ten) you're stuck!

   So, here's the routine: whenever you flip from one browser tab to another that has a different user context, you just click on the user name, which is conveniently located at the top of the Switch Users list, to restore the correct user context for that tab.

If it hurts you to "waste" one of ten, then why not just raise the count to eleven, but by all means don't remove the current user!

(In fact it might make sense to make the number of entries configurable, so that those who have many switchable users could raise the count if they want, but that's another issue.)

#16: * means 'required'

This is true for * in connection with entry fields. Here we don't have entry fields, so it's obvious, that we're in a different context where 'required' doesn't apply.

The * is a useful hint for those who know what it means, but it's not a problem if you don't know. If we take away the *, most people wouldn't recognize the difference in the behavior of the Switch Users block (assuming that the changes from #12 will be reverted as suggested in #19) — the way you use it stays the same, whether

• there are no *,
• there are * but you don't know what they mean, or
• there are * and you know what they mean

It's very similar to the ellipses on button captions and menu entries. If you know that [Print...] will lead you to a dialog with a [Cancel] button and [Print] will print right away, then you appreciate having the ellipsis, but if you don't know the difference, then it doesn't matter.

In the default installation (without a group that has the switch users permission), only user 1 will have an *, and I don't believe that the concept of user 1 being different will confuse or disturb anyone.

Moreover, after switching to a user that won't let them switch back, people will quickly recognize that there are two different kinds of users, so I'd say that this is self-explanatory.

A mouse-over hint doesn't hurt much, but IMO its usefulness is extremely short and slight, nothing compared to the annoyance of flashing up unnecessary tooltips for the rest of your life.

(Oops, sorry about resetting the status — that wasn't intentional. My form had the old status, you changed it, and when I submitted my form, it was inadvertently changed back.)

#20: we have [...] 10 that are in a special role that can switch. we just want the switch block to show those users that can switch

My code did exactly that. Now I understand why you wrote "was a little bit complicated" in #12. You simplified it and inadvertently removed an essential property that you're trying to bring back now at the expense of another valuable property.

My code did this:

1. Take user 1,
2. add all other switchable users,
3. if we have less than 10, then add most recently used ones (that aren't already in there) up to 10 — don't limit the query to 10-count, because some of the first 10-count ones may already be in the list and we need others to fill the list up to 10,
4. now sort the list by access and clip to 10.

   This never clips switchable ones while keeping non-switchable ones, because if there are 10 or more switchable ones, step 3 does nothing and no non-switchable ones ever get added.

   It might make sense to not clip to 10 at this point to avoid dropping switchable users, because if we have more than 10 users in the switchable role, we probably want to see all of them.

So, if you have exactly 10 switchable users (including user 1), then my code did exactly what you wanted, and I don't understand why you would need to break the list into two, because you only see those that would be in the first half anyway. It doesn't matter to you whether the others would appear below, or in access order.

#20: in your case, you want to overload the "Switch users" block as your "Recently logged in" block and/or your "Who's online" block.

I want to keep/preserve the "Recently logged in" and "Who's online" information that is already in the current Switch Users block. It's not that I want to add something new, I'd like to keep you from removing something that's there.

Actually, I don't know the "Recently logged in" block, that must be an separate module? But adding those two blocks would be overkill and far away from the convenience of the "Switch Users" block which can provide exactly the right mix of "recently logged in" and "currently online" information in a spot that I look at regularly anyway.

So far you haven't provided a reason for why you want to break the over-all sort by access, on the contrary, you've explained that it doesn't matter to you, so why unnecessarily destroy a feature that the old Switch Users list has?

#22: however, until somone posts a working patch to split this up into the 2 blocks i outlined in #20

I see no need for two blocks as explained above. If you take my patch

• and you have exactly 10 switchable users, you get exactly behavior 20.2,
• I get exactly what I want,
• and those who don't care get (almost) exactly the behavior before the patch.

Rather than bloating this to two blocks (I don't think anyone would want to see both at the same time or even at different times...) a configurable number of entries and/or a flag to limit the selection to switchable users only (and possibly use simpler queries), might be a better idea.

Thanks for bearing with me...
Hans

#22: your "if you don't know what it is, it doesn't hurt you" attitude isn't really the right approach in general.

Yeah, not in general, but don't you see the analogy to the ellipses? And your OTOH is pretty convincing, too...   :-)

Oh, and in a sense the * does stand for required — if you hope to come back without having to log out and in again, you're "required" to chose an entry with a *.   ;-)

OTOH we could also argue that we should really use ellipses, because the changeable users allow you to "cancel" the switch and come back, while the others switch you permanently.   8-0

I'm simply looking for the most unintrusive way to distinguish the switchable ones — the * seemed to fit nicely, but I'm happy with anything else, as long as it's visible without mousing over, and preferably without any annoying dynamic behavior at all. Would menu.module support making them bold or italic? Not that I'd prefer that over a tiny *, just exploring the possibilities...

Sorry about opening a third subthread, but things have moved so quickly when I was away yesterday, that I'd like to bring this up as long as there's still time, and it's related to the discussion about the * indicator.

As mentioned before (#10) I'd like to check the accessibility of the destination url. If you've used the patch for any length of time you've doubtlessly found yourself switching to a user with lesser permissions and ending up with "Access Denied." I could live with that, but what really bothers me is that a watchdog record is created.

Is there a way to check whether the current page is accessible for each user? Would this add undue overhead? Could it be made optional?

If we can detect accessibility there are two possible strategies:

1. We could just redirect to the home page; this would (usually) avoid generating a watchdog entry, but without a prior visual indication it would be surprising behavior, and it would cause us to lose drupal_get_destination().

   If you get the 403, the URL is not changed, and you can switch back to a user that has the required permission and see your page again, without losing your place. This is a valuable property and I'd hate losing it.

2. I'd prefer to add a visual indicator to the menu entries in the Switch Users block, but seeing how we argue over the *, it isn't going to be easy...

   Nonetheless, I believe a visual indicator would add considerable value to the Switch Users block: you'd be able to see at a glance which of your users (role models) are able to view the current page!

   If we had a visual indicator, there'd be no need to break the link target, because the user would know ahead of time that he can't go there without risking a 403 and a watchdog entry.

   So, the indicator: Being a programmer, I thought of the exclamation mark (!), the tilde (~), and the minus (-), all meaning some sort of 'not' or 'negation', but they all carry more weight than the *. The * is easily used in colloquial writing as a footnote reference, and I feel that most people should be able to pass over it without second thoughts. The other three, however, are less common and beg for questions.

   As with the *, once you've realized what it means, you won't need constant reminding, because the division into "can go" and "cannot go" users is just as fundamental as the one into "can come back" and "cannot come back" ones, but I'm hard-pressed for another symbol that is both unobtrusive and sufficiently intuitively distinct from *.

   If menu.module supports it, I'd suggest using strike-through text as an intuitive indicator for "Don't go there!" Is there a way to do this?

   • The <strike> tag is deprecated in HTML4 and removed in XHTML1-Strict.
   • The alternative <del> tag is new in HTML4 and may not be universally supported.
   • The cleanest alternative would be to use the CSS text-decoration property, but does menu.module support it?

Is there any chance for rolling this into this patch?

@All: Thank you for considering my input.

@dww: I'm sorry, I didn't want to escalate this into a fight where point-by-point refutes are needed.

It seems that your latest (and committed) version doesn't ensure anymore that user 1 is among the selected list.

You mentioned that my patch didn't do what I described, and you questioned the necessity of the ineffecient and convoluted queries. I don't want to argue, but I'd sincerely like to recognize and learn from my mistakes. I have two or three straight-forward single-table SELECTs. You have one less in your last iteration, but you may have to re-add it for user 1, and one of yours is a two-table JOIN.

I cannot see why your solution is more efficient, nor can I find a mismatch in my specs vs. code. Would you enlighten me, please? (I'd be glad to expose the code and add some comments and numbering, to make it as easy as possible for you to comment.)

@moshe: I'm sorry to test your patience and I won't beat this dead horse anymore, but I'd really appreciate

• getting a hint on how to check accessibility for my own private patch (if anyone can help) and
• getting some insight from dww, so that I can improve my coding skills.

The Drupal Dojo is probably the right place for this type of discussions, but we've gone this far here — maybe you'll let us hash it out to the end?