Don't switch to blocked users [#974658]

The 'switch user' functionality is really useful, and I have made an enhancement which I think makes it even more helpful. The problem is that switching to a blocked user causes a forced log-out which is not particularly helpful, especially if you don't know the user is blocked and you spend a while trying to work out why the logout is happening, (he says from experiencing this ;-)

The following patch to devel_switch_user_list() makes the first selection from only active users. Then the top-up list selects active users in preference to blocked users. If any blocked users have to be used to make up the required list size then instead of a link their name is shown in plain text with '(blocked)' appended to it, to explain why.

Also, I added the date last accessed into the hover, as this info was being extracted anyway and is useful to show.

Hope you find this useful. I'll attach the patch on the following comment when I know the issue number.

Jonathan

Comment	File	Size	Author
#15	_974658.devel_.15.dont_switch_to_blocked_user-D6.patch	3.47 KB	jonathan1055
#15
#13	_974658.devel_.dont_switch_to_blocked_user-D7.patch	2.4 KB	jonathan1055
#13
#10	switch user last accessed date.jpg	54.8 KB	jonathan1055
#1	_974658.devel_.dont_switch_to_blocked_user-D6.patch	4.11 KB	jonathan1055
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jonathan1055 CreditAttribution: jonathan1055 commented 17 November 2010 at 18:20

File	Size
_974658.devel_.dont_switch_to_blocked_user-D6.patch	4.11 KB

Here's the patch against 6.x 1.22

Comment #2

jonathan1055 CreditAttribution: jonathan1055 commented 17 November 2010 at 18:27

Status:

Active

» Needs review

... and set the status.

The patch was against 1.22 but I've just checked and it applies OK on the latest dev (13th Nov) with all three hunks having an offset of 34 lines.

Comment #3

moshe weitzman CreditAttribution: moshe weitzman commented 18 November 2010 at 12:55

Assigned:

Unassigned

» salvis

salvis typically reviews and commits patches for this block.

Comment #4

jonathan1055 CreditAttribution: jonathan1055 commented 18 November 2010 at 13:32

At first viewing, the patch may seem more complicated than you were expecting, but really it is a very simple change. It appears that more lines have been altered because I have moved a section of (nearly) duplicated code into a sub-function called devel_switch_user_list_link() which is called twice from the main devel_switch_user_list() function.

Comment #5

salvis

he/his

CreditAttribution: salvis commented 19 November 2010 at 00:34

Status:

Needs review

» Needs work

I've never seen sub-functions in Drupal. Do we allow them? The function name should probably start with an underscore.

You're using tabs and have some trailing spaces. Please fix that.

I was kind of expecting moshe to say that this was going too far, but since he didn't wontfix it, your chances are intact. But we'll need a D7 patch first...

Are the blocked users still listed as links? Why would you click on one? Why list them at all?

Comment #6

jonathan1055 CreditAttribution: jonathan1055 commented 19 November 2010 at 10:24

Status:

Needs work

» Needs review

Thanks for the feedback. Sorry about tabs and trailing spaces, I usually check for these but forgot.

I am OK with changing the sub-function to an ordinary one, if that is preferred, it was just that this functionality would only be used from within in the main function. Yes I'll change it to start with an underscore, but also given that devel_switch_user_list() is not a hook function then strictly speaking so should that one also. I was just going for consistency ;-)

To answer your question, currently you don't know they are blocked users so you click on the link and it immediately logs you out. In my patch the blocked user names are shown but not made into active links, and they have '(blocked)' after the name. These are all shown at the end of the list and only get to be seen if the number of active users is not enough to fill the specified list size. I find it to be useful information, as otherwise you would be wondering why a user does not appear in the list.

I am very happy to make a D7 version first if Moshe says it is a goer.

Jonathan

Comment #7

salvis

he/his

CreditAttribution: salvis commented 19 November 2010 at 22:25

Status:

Needs review

» Needs work

I find it to be useful information, as otherwise you would be wondering why a user does not appear in the list.

No, that's not enough justification for adding that overhead and complexity. I agree that removing items that go into a black hole is a good thing, but if you're confused about the disappearance of blocked users, then you shouldn't be using Devel. Instead you should be worried if blocked users are still visible.

What's more, devel_switch_user_list() is a public function — that's why it MUST NOT have an underscore at the beginning. It's called by the admin_menu module, and possibly others, and we don't want to return items to admin_menu that aren't menu items.

Start with D7, please.

Comment #8

jonathan1055 CreditAttribution: jonathan1055 commented 20 November 2010 at 09:34

Version:	6.x-1.x-dev	» 7.x-1.x-dev
Category:	bug	» feature

OK, I take your point, blocked users do not need to in the list at all.

Sorry about the underscore confusion, I was not suggesting at all that the name of devel_switch_user_list() should be changed! I was just of the understanding (which seems wrong now) that functions which are hooks for other modules to call have a name starting with the module name and all other functions should be differentiated from these by adding an underscore at the start.

Yes, I'll make a patch for D7 first.

Jonathan

Comment #9

salvis

he/his

CreditAttribution: salvis commented 20 November 2010 at 10:49

Assigned:

salvis

» Unassigned

I'm not sure that is defined somewhere, but my understanding is that _functions are internal ones that cannot possibly be useful for any other modules and that may change at any time, while functions without a leading underscore are either hooks (which do need to start with the module name, as you say) or other functions that might be useful for some other module. They should be documented and they should not change within the same major version.

Please assign yourself if you intend to provide a patch. TIA!