[D7] Delete Block Users

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

It seems that the default branch is 7.x but if we check the project with Ventral (http://pareview.sh/pareview/httpgitdrupalorgsandboxlgrtm2074157git) :
"There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732 "

FILE: del_block_users.module
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AND 2 WARNING(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
57 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines
60 | ERROR | Comment indentation error, expected only 1 spaces
64 | WARNING | There must be no blank line following an inline comment
65 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines
68 | WARNING | There must be no blank line following an inline comment
68 | ERROR | No space before comment text; expected "// =
| | array_diff_assoc($users, $usersexclude);" but found "//=
| | array_diff_assoc($users, $usersexclude);"
68 | ERROR | Comments may not appear after statements.
78 | ERROR | No space before comment text; expected "// dpm($users);" but
| | found "//dpm($users);"
79 | ERROR | No space before comment text; expected "// dpm($usersexclude);"
| | but found "//dpm($usersexclude);"
80 | ERROR | No space before comment text; expected "// dpm($users);" but
| | found "//dpm($users);"
96 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

this module looks like it could work very nicely in combination with mine: https://drupal.org/node/2083831 :-)

Please, add instructions on how to clone your project: git clone http://git.drupal.org/sandbox/lgrtm/2074157.git del_block_users

Is the user in the README pointing to the wrong Drupal User? http://drupal.org/user/5546

The module installs correctly and works fine. Tested with a blocked user, selecting the user to avoid deletion and without selecting it works properly. The code is also well written and documented, nothing else to comment in that.

Help us with some reviews to accelerate your review process. You can help me too :-): https://drupal.org/node/2083831

Once done this, I think it could be ready :-).

Manual Review

1. The Erase Confirmation should be in separate form (Should work like confirm form)

2. You should explode and trim the uids in

$usersexclude = explode(',', $form['del_block_users']['selectusers']['#value']);

replace with

$usersexclude = array_map('trim', explode(",", $form['del_block_users']['selectusers']['#value']));

3. To exclude users from delete list you have used

foreach ($usersexclude as $user) {
    if (in_array($user, $users)) {
      $key_users = array_search($user, $users);
      unset($users[$key_users]);
    }
  }

This is very time consuming. You can use below

$users = array_diff($users, $usersexclude);

4. To delete user you user foreach

foreach ($users as $user) {
      db_delete('users')
      ->condition('uid', $user)
      ->execute();
    }

This is again very time consuming. You can use below

db_delete('users')
      ->condition('uid', $users, 'IN')
      ->execute();

All blocked users will be deleted in a single query.

5. I am feeling the exclude uid fiels title should be easy to understand in

'#title' => t('Select id of block user to no delete.'),

it can be something like below

'#title' => t('Select user ids from blocked users to exclude from deletion.'),

Hey lgrtm,

I'll just do a quick review with your FAPI implementations.

In del_block_users.module line 10, function del_block_users_form_alter;

1. Why are you passing the form_state by reference?
2. Don't return $form.

Have a look at hook_form_FORM_ID_alter as your only targeting a single form.

So something like;

/**
 * Implements hook_form_alter().
 */
function del_block_users_form_user_filter_form_alter(&$form, &$form_state) {

}

Lastly your CSS can be attached to your form like so.

$form['#attached']['css'][] = $path . '/del_block_users.css';

Just a suggestion but consider adding a validation function.

Cheers,

Updated status.

Module working fine for me.
Here is one monor comment:
Please add dt function for 'description' string inside hook_drush_command

Cancel similar project, only D6.

Module seems to be working fine on my tests also. Just curious though, what is that "checkbox," file in your module? I'm thinking it was included there by accident?

Understandable. I too am new to git also. Anyway, you don't seem to have any problems here also: http://pareview.sh/pareview/httpgitdrupalorgsandboxlgrtm2074157

So other than that rogue file, this module seems to be good to go.

Hi,
manual review:
1 - In file "del_block_users.module", line 40

current_path() . '/del_block_users.css';

is returning "admin/people/del_block_users.css"

I think this is what you are looking for:

drupal_get_path('module', 'del_block_users') . '/del_block_users.css';

2 - As iamrasec says there is a file calle checkbox.

3 - In your submit function You could considering to add a "where clause" to the query instead of retrieving unuseful elements and then make an "array_diff" which probably will become in a poorer perfomance.

4 - Doctumentation says:
Note: in 90% of select query use cases you will have a static query. If in a critical performance path, you should use db_query() and friends instead of db_select() for performance reasons. Only use dynamic queries if the query parts vary (example: adding WHERE conditions depending on the context) or if they should be alterable
https://drupal.org/node/310075

5 - When I execute some drush command with an invalid argument ("drush cc asdasdad" or "drush vget asgdjagdjagsj") I have the next message:

"Delete all block users in site, caution for manual block user. You can select uid for no delete."

Hope this help.

Regards.

This looks like a nice, straightforward module for removing all those pesky auto-blocked users. However, there are several confusing grammar errors in this module, most seriously in the title itself.

From what I can tell, this module should be called "Delete Blocked Users". Without that "ed", the module's name sounds like it's related to Drupal's Block system (the admin/structure/block page), rather than to blocked user accounts. This is a common conjugation mistake for ESL users, so don't feel bad. :)

I've also included a patch that fixes up the grammar of all the user-visible strings in the module. My patch doesn't change the name of the module, though, because the name is so closely tied to the file and function names. That would be a really significant change that I figured should be done by lgrtm, if he decides to do so.

Additionally, my patch tweaks the del_block_users_validate() to be more robust. It now supports whitespace between the comma-separated numbers, and has a slightly more readable regex.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.