#168: Batch actions in admin users page

[franzliedke]

Ticket: http://fluxbb.org/development/core/tickets/168/

Things that need review:

• These are pretty big and complex features, I might have missed something.
• The JavaScript code
• Should I add code to auto-focus some fields in the new forms I created?
• Somebody glossing over language strings (especially the keys) and form field names (and IDs etc.) would be cool, too.

I think I have tested everything, but there are always situations...

[Quy]

Good job! Here are some changes:

Missing language string for 'No ban admins message'.

It is currently possible for moderators to ban moderators when mass banning.

Ban/Delete buttons should only be displayed if there are records found.
find:
[code]$can_action = $can_delete || $can_ban;[/code]

change to:
[code]$can_action = ($can_delete || $can_ban) && $num_users > 0;[/code]

To keep code consistent with other files:
change COUNT(*) to SELECT 1
change if ($db->result($result) > 0) to if ($db->num_rows($result))
in the following code:

find:
[code] // Are we trying to ban any admins?
$result = $db->query('SELECT COUNT() FROM '.$db->prefix.'users WHERE id IN ('.implode(',', $user_ids).') AND group_id='.PUN_ADMIN) or error('Unable to fetch user info', FILE, LINE, $db->error());
if ($db->result($result) > 0)
message($lang_admin_users['No ban admins message']);
[/code]
change to:
[code] // Are we trying to ban any admins?
$result = $db->query('SELECT 1 FROM '.$db->prefix.'users WHERE id IN ('.implode(',', $user_ids).') AND group_id='.PUN_ADMIN) or error('Unable to fetch user info', FILE, LINE, $db->error());
if ($db->num_rows($result))
message($lang_admin_users['No ban admins message']);
[/code]
find:
[code] // Are we trying to delete any admins?
$result = $db->query('SELECT COUNT() FROM '.$db->prefix.'users WHERE id IN ('.implode(',', $user_ids).') AND group_id='.PUN_ADMIN) or error('Unable to fetch user info', FILE, LINE, $db->error());
if ($db->result($result) > 0)
message($lang_admin_users['No delete admins message']);
[/code]
change to:
[code] // Are we trying to delete any admins?
$result = $db->query('SELECT 1 FROM '.$db->prefix.'users WHERE id IN ('.implode(',', $user_ids).') AND group_id='.PUN_ADMIN) or error('Unable to fetch user info', FILE, LINE, $db->error());
if ($db->num_rows($result))
message($lang_admin_users['No delete admins message']);
[/code]

[franzliedke]

Thanks for the review. Gosh, how do you do this all the time? ;)

Darn - so I did, in the end, forget to add the moderator check to this code. I will fix the bugs tomorrow.

But I would actually vote against consistency this one time, as the COUNT way is (in theory) the faster way. Let the database do what it's an expert for...

[franzliedke]

It looks like it did not merge all of my code correctly. I am having some branch problems, it seems. Looking into them right now.

[franzliedke]

Much better now.

This means I had actually solved any problems you mentioned except for the empty list issue. I will do that later on. Anything else (now that we can move users to different groups)?

[Quy]

File: C:\xampp\htdocs\fluxbb145master\admin_users.php
Line: 535

FluxBB reported: Unable to fetch moderator group info

Database reported: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '*) FROM users AS u INNER JOIN groups AS g ON u.group_id = g.' at line 1 (Errno: 1064)

Failed query: SELECT COUNT(u.*) FROM users AS u INNER JOIN groups AS g ON u.group_id = g.g_id WHERE g.moderator=1 AND u.id IN (9)

line 535:
g.moderator=1 should be g.g_moderator=1

Remove the space before and after = in:
u.group_id = g.g_id should be u.group_id=g.g_id

Since buttons are hidden if not applicable, this can be removed:

[franzliedke]

Thanks. Every known bug should now be fixed.

[Quy]

File: C:\xampp\htdocs\fluxbb145master\admin_users.php
Line: 535

FluxBB reported: Unable to fetch moderator group info

Database reported: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '*) FROM users AS u INNER JOIN groups AS g ON u.group_id=g.g_' at line 1 (Errno: 1064)

Failed query: SELECT COUNT(u.*) FROM users AS u INNER JOIN groups AS g ON u.group_id=g.g_id WHERE g.g_moderator=1 AND u.id IN (2211)

Is not liking this COUNT(u.*) ?

Also when no users selected:
Notice: Undefined index: users in C:\xampp\htdocs\fluxbb145master\admin_users.php on line 264
Notice: Undefined index: users in C:\xampp\htdocs\fluxbb145master\admin_users.php on line 380
Notice: Undefined index: users in C:\xampp\htdocs\fluxbb145master\admin_users.php on line 518

[franzliedke]

The query should now definitely be fixed. Sorry for not testing it properly (both times).

I really hope that's enough of me messing up. Thanks for your thorough testing.

[Quy]

This is still happening when no users selected:
Notice: Undefined index: users in C:\xampp\htdocs\fluxbb145master\admin_users.php on line 264
Notice: Undefined index: users in C:\xampp\htdocs\fluxbb145master\admin_users.php on line 380
Notice: Undefined index: users in C:\xampp\htdocs\fluxbb145master\admin_users.php on line 518

You're welcome.

[franzliedke]

Gosh, I suck at debugging (today).

I think the same thing will happen in many other places in the core (e.g. moderate.php), but that's another issue (and we already have a ticket: http://fluxbb.org/development/core/tickets/14/).

Thanks again.

Do you think the JavaScript is good? As in, the behaviour is ok?

[Quy]

Perfect. The behaviour of the JavaScript is fine.

[franzliedke]

Cool. Do you think this is ready to be merged into core?

[Quy]

I think so.

[franzliedke]

Squash-merged in 6bfcd8d