[D6] Temporarily Block Users

You need to write the module first before appling for a cvs account. Once its done please upload it here. Also make sure there are no similar modules that already exist.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

yogesh1110,

Please check out the 'Migrating from CVS Applications to (Git) Full Project Applications' link that arianek posted, and specifically Step 5 on that page. If all that has been done, it's simply a waiting game until someone is able to pick up your issue in the project applications queue (http://drupal.org/project/issues/projectapplications).

Updating Issue title/queue/status as per the 'Migrating from CVS to Full Project Applications' instructions provided on the referenced page.

Updating priority according to the new project application priority guidelines. The application's priority should be set back to normal once a reviewer responds to your application and the application review process has continued.

Ok, this is just a quick pass to get things moving. I haven't tried using the module yet:

To tidy up your project file structure:
- Remove $Id at the top of the files as these are no longer needed (and ignore coder's warnings that these are missing if it gives them)
- Remove the LICENSE.TXT file as this will automatically be added.
- Go to admin/build/modules and select the (Code Review) link next to the names of your modules
- Open the “Selection form” fieldset and select “minor (most)” and tick “internationalization
- Submit the form
- Fix the errors ignoring any reports.

Hi Yogesh
I just want you to know that I'm not ignoring you. I've been extremely busy day and night and weekend over the last few weeks. I now have have a major issues with the VM that was providing my web services on this machine at present and can't currently run any code during the day. I'll pull down the latest version of your module just as soon as I can get things working again and catch up with what I should have been doing for the past few days.
MB

yogesh, go on IRC and go to the room #drupal-contribute and ask someone to look at your module in there. I found it can speed up the processes.

Hi Yoesh. Apologies for disappearing, my machine died and then I got very ill for a few weeks. Here's some more feedback for you.

1. When the module is enabled I get the following warning:

   Warning: Call-time pass-by-reference has been deprecated in temporarily_block_users/temporarily_block_users.module on line 140

2. Date Popup dependency: Although I was able to install your modules once I had the date picker installed I then got the following message when editing a user page:

   The Date Popup module now requires the jQuery UI module as a source for the datepicker. Please install it immediately.

   You may want to raise a ticket against Date Picker to add this as a dependency on their module. I'm not sure why they are doing it as a drupal message rather than a dependency, maybe they have a good reason for this?

3. The menu under “User management” is called “Temporarily user block”. In fitting with your module and better use of English this would be better renamed “Temporarily block users”. I will help you with the English and grammar later on if you need me to once we have everything else working.

4. I can see no mention of the “Temporarily user block” menu in your README.txt. It would be useful for user of the module to know about it in advance rather than just stumble across is.

5. This date format use to display user list. Set the date format from here.

   user/tbu/settings Add the missing “.” to the end of the description and correct the typos, you are missing a few words like "is" and "the".

6. user/tbu/settings I’m not quite sure what the following is supposed to mean?

   Block users by roles wise

   Should this be "Block users by role"?

7. user/tbu/settings - "Block users by roles wise" - This section seems unfinished. I can only see block all users. I would suggest that you either remove this or pupulate the list with all user roles. A brief description here explaining what this is for would also be helpful.

8. Please can you indicate where I can temporarily block users? I can see no option for this at /user/user/list or on the user edit pages.

9. There is a lack of comments in your code (functions) explaining what you are doing and why. For the benefit of yourself and others I recommend that you go through your functions and add comments to explain what you are doing and why. I have included an example of a commented function below:

   function honeytrap_get_trap_array() {
     // Get random trap path from list of disallowed paths<
     $honeytrap_settings = variable_get('honeytrap_settings', array());
   
     // Get a random disallowed path
     $rand_index = array_rand($honeytrap_settings['url']['disallowed_paths']);
     $disallowed_path  = rtrim($honeytrap_settings['url']['disallowed_paths'][$rand_index], '/');
   
     // Check if a random number should be added to the url
     $url = $disallowed_path;
     if ($honeytrap_settings['url']['rand_max']) {
       // Get a random number
       $rand_number = rand($honeytrap_settings['url']['rand_min'], $honeytrap_settings['url']['rand_max']);
       $url .= "/{$rand_number}";
     }
   
     $trap = array(
       'disallowed_path' => $disallowed_path,
       'url' => $url,
     );
   
     return $trap;
   }
   

10. Missing use of whitespace: You are missing some white space around the conditional items in if statements etc. Here's an example from check_user_block():

    if (time()<$blockdate || time()==$blockdate) {

    change this to:

    if (time() < $blockdate || time() == $blockdate) {
    

    You will need to check and correct his throughout your code. You will find more information on spacing and layout at http://drupal.org/coding-standards#comment.

11. temporarily_block_users.info description: "Temporarily Block User Wise." does not make sense. Please let me know what you are trying to say here?

12. Line length: I'm not sure that there is a definition for the general maximum line length in Drupal (someone please point this out if there is) but I can see lots of discussions on it. Comments should definitely not extend more than 80 characters per line, see http://drupal.org/node/1354. Many posts seem to recommend 80 characters for general code. I myself would say anthing up to 100 characters unless someone else can point out the relevant section in the Drupal standards?

    Here's an example of how to break your long lines up taken from theme_temporarily_block_users_settings():

    $output .= drupal_render($form['user_block']['block_user_role']['temporarily_block_role_' . $id]) .             drupal_render($form['user_block']['block_user_role']['temporarily_block_role_date_' . $id]);
    

    Indent the content by 2 spaces and align them as follows:

    $output .= drupal_render(
      $form['user_block']['block_user_role']['temporarily_block_role_' . $id]) . 
      drupal_render($form['user_block']['block_user_role']['temporarily_block_role_date_' . 
      $id
    );
    

    Also in temporarily_block_users_list()

    $result = db_query('SELECT tbu.uid AS uid, tbu.block_date AS timestamp_blocked, u.name AS name, tbu.updated AS timestamp_updated FROM {temporarily_block_users} tbu JOIN {users} u ON u.uid = tbu.uid
                                        WHERE tbu.block_date > %d', time());
    

    Would would be more readable as something like:

    $result = db_query('SELECT tbu.uid 
      AS uid, tbu.block_date AS timestamp_blocked, u.name AS name, tbu.updated AS timestamp_updated 
      FROM {temporarily_block_users} tbu 
      JOIN {users} u ON u.uid = tbu.uid
      WHERE tbu.block_date > %d', 
      time()
    );
    

    You will find more information on spacing and layout at http://drupal.org/coding-standards#comment and general coding standards via http://drupal.org/coding-standards.

13. Doxygen coding standards:
    It's great to see you adding header comments to your functions. Please take a look at http://drupal.org/node/1354 to see how you should add the addtional tags like @param, @return, @ingroup and @see. Also, treat your comments as English and start them with capital letters and end them with full stops.

14. check_role_block() missing function header comment.

15. SQL Query Standards:
    Reserved words in your queries should be in uppercase. See http://drupal.org/node/2497 for full details.

16. Typo in functiion "user_blocked_excluled" should be "user_blocked_excluded".

17. temporarily_block_users_init() - This function and the functions that it call are not clear to me. For example, I'm not sure that you should be checking for the admin user rather than looking at permissions. However, I could be wrong and this may make more sense once you have added comments in your functions to explain what and why you are doing things.

The applicant has been contacted to ask if the application is abandoned.

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256

It could not be marked as critical as it was just reopened.

Hi.
The module itself sounds really usefull (thought about creating something like this some time ago :)).

Quick review:
You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
(At least this has to be done before switching back to needs review)

For an in-depth review please look at the results on http://ventral.org/pareview/httpgitdrupalorgsandboxyogesh11101155096git.
There are several notices and errors you'll need to resolve.

Hi.
Actually, I just wanted to make a quick review, but somehow it has become a little more detailed :)
(This is no functional review, just code.)

First, set your default branch to 6.x-1.x and remove your master branch.
If you're doing so, you may delete the branch 6.x-1.0-dev as well as its no valid branch name (see Release Naming Conventions).

• README.txt
  • Remove the first line (// README.txt,v 1.5.2.6 ...) and the next blank line. This is a relict from old times (CVS) we do not use anymore.
• temporarily_block_users.info
  • Remove the "version" line as the packaging script will automatically include it.
  • "project" is missing: "project = temporarily_block_users"
  • Maybe you should not set a package here or find on your module fits in. See http://drupal.org/node/231036 section "package".
  • The first line is not really needed here.
  • Is the dependency to "Date Popup" really needed? It would be better to use this as an optional dependency.
• temporarily_block_users.install
  • Function comments for hook implementations in Drupal 6 are written as "Implementation of ..." (I know, this is very nitpicking ;) )
• temporarily_block_users.module
  • Function comments for hook implementations in Drupal 6 are written as "Implementation of ..."
  • function temporarily_block_users_user_not_admin
    • You're using the user name as a parameter. Why not use the uid? User names may change, the uid rather not.
    • Querying the database here is unnecessary because user_access() uses the current user if the second argument is empty.
      Additionally it is a major performance impact, as you are calling this on every init.
    • In my opinion its always better not to "negate" function names. If you would check if the user is not the admin user, name the function "user_is_admin" and check the return for FALSE.
    • Taking this into account you may drop this function and simply use <?php global $user; return $user->uid == 1;?>
  • function temporarily_block_users_user_blocked_excluled
    • The function name is mis-spelled. I think it should be "temporarily_block_users_user_blocked_excluded".
    • You're using the user name as a parameter. Why not use the uid? User names may change, the uid rather not.
    • $user_account = user_load($user_uid); is unnecessary because user_access() uses the current user if the second argument is empty.
    • You may drop this function and simply use user_access('exclude temporarily_block_users');
    • If you do no drop the function you may include the check for "is admin" here. Both functions are always called together so it would make sense to combine them:
      

      <?php
      function temporarily_block_users_user_excluded() {
        global $user;
        if ($user->uid == 1 || user_access('exclude temporarily_block_users')) {
          return TRUE;
        }
        return FALSE;
      }
      ?>

  • function temporarily_block_users_perm
    • Its just a personal feel but the permissions may be named more descriptive:
      * "access temporarily_block_users" is the permission to administrate the settings so it should be named "administer temporary_block_users"
      * "exclude temporarily_block_users" is the permission to ignore the block so it should be named "ignore temporary_block_users"
  • function temporarily_block_users_settings
    • Nitpicking: function comment should be "... will be displayed at ..."
    • Change description of date format dropdown: 'The date format used on the user list. You can change the format settings on the !date_format_settings.'
    • $base_url is not needed. Simply use <?php l(t('date format administration'), 'admin/settings/date-time/formats'); ?>
    • You are using a fixed set of date format options. What, if the user has defined custom formats?
    • btw., what is the date format selection good for?
  • function temporarily_block_users_form_alter
    • You may use menu_get_object() to get the current viewed user instead of arg().
    • $res_ex_user = ...: <?php $user_blocked_until = db_result(db_query("SELECT block_date FROM {temporarily_block_users} WHERE uid = %d ORDER BY block_date DESC LIMIT 1", $uid)); ?>
      This way you can remove the while-loop and the unused variable $count.
    • By using menu_get_object() before you can remove the call to user_load() as well.
    • Use TRUE and FALSE instead of numeric values for properties "#collapsible" and "#collapsed".
    • Suggestion: change checkbox title to "Enable temporary block".
    • Make date field a text field. If "Date Popup" is installed, use "date_popup" as type.
    • Suggestion: change field title to "Block user until".
    • Suggestion: change field description to "The user will be blocked until the given date. If no date is entered, the account is not blocked temporarily." (or something similar).
    • Do not write $form['#validate'][0], use <?php $form['#validate'][] = 'temporarily_block_users_login_validate'; ?> to add your validation function. If the function should run before all others, use <?php array_unshift($form['#validate'], 'temporarily_block_users_login_validate') ?>
  • function temporarily_block_users_login_validate
    • Remove the call to user_login_name_validate() (see above; its not needed anymore).
    • Do not use the user name, use the uid (as mentioned above for function "temporarily_block_users_user_blocked_excluded" and others).
    • The call to "temporarily_block_users_module_invoke" is not needed. Use module_invoke_all().
  • function temporarily_block_users_module_invoke
    • The function should be removed in favor of module_invoke_all().
    • bzw., the function is useless, as the return values from the called functions are not processed and neither $form nor $form_state is a reference.
  • function temporarily_block_users_list
    • $count is not needed, you can use <?php count($rows); ?> to get the number of blocked users.
  • function temporarily_block_users_list
    • $count is not needed, you can use <?php count($rows); ?> to get the number of blocked users.
  • General notes
    • The notes above are valid for most of the functions not listed here (e.g. descriptions, SQL-strings).
    • Some variables are named somehow strange (e.g. "$res_ex_user"). Consider to rename the variables.
    • Function comments should be more detailed (e.g. parameter descriptions). Using coder module will give you lots of hints.
• scripts/temporarily_block_users.js
  • Line 3: "var" is missing (var ab = ...;).
• temporarily_block_roles/temporarily_block_roles.info
  • "project" is missing: "project = temporarily_block_users" (this will fix the warnings of pareview).
  • see notes for temporarily_block_users.info
• temporarily_block_roles/temporarily_block_roles.module
  • Function comments for hook implementations in Drupal 6 are written as "Implementation of ..."
  • function temporarily_block_roles_form_alter
    • classes "user-role-hedder", "user-role-hedder-rolename", ...: did you mean "header"?
    • Make date field a text field. If "Date Popup" is installed, use "date_popup" as type.
    • Do not use $form['#validate'] = array('temporarily_block_users_settings_validate');. This removes all other validation functions. Use <?php $form['#validate'][] = '...'; ?> or array_unshift().
  • Function comments for hook implementations in Drupal 6 are written as "Implementation of ..."
  • function temporarily_block_roles_temporarilyblock
    • Function user_not_admin() is not defined.
    • Use uid instead of user name (see notes for temporarily_block_users_login_validate().
  • function theme_temporarily_block_users_settings
    • Initialize variable $output (or change ".=" to "=" in the first line).
  • General notes
    • Some variables are named somehow strange (e.g. "$callfrom"). Consider to rename the variables.
    • Function comments should be more detailed (e.g. parameter descriptions). Using coder module will give you lots of hints.

hi ... you need to check Drupal coding standard at http://ventral.org/pareview/httpgitdrupalorgsandboxyogesh11101155096git

1. temporarily_block_users.module: Try to write your code in Drupal way -

  $usermodulepath = drupal_get_path('module', 'user');
  include_once "$usermodulepath/user.pages.inc";
  global $user;

- Define variable at the top of your function.
- user module_load_include() instead include_once like

  global $user;
  module_load_include('inc', 'user', 'user.pages');

2.

array_unshift($form['#validate'], 'temporarily_block_users_login_validate');

i hope you can simple append it instead array_unshift().

3. temporarily_block_roles.module : Don't forget to delete variable you defined for your module in uninstall hook.

4. temporarily_block_roles.module : This doesn't make sense.

  $output = '';
  $output .= drupal_render($form['user_block']);

5. temporarily_block_users.js : Drupal will not recognize '$' for jQuery in many situations:
You can wrap your jQuery in this to 'fix' it to work like it should from the start:

( function($) {
.. your script here
  })( jQuery );

2. menu_get_object() function is not returning any thing as I am using this in form_alter hook.

Have a look at the api documentation of menu_get_object(). If you call it with "user" as first parameter drupal tries to extract the uid from the URL and returns the user object. It works.

uname instead of uid

This leads to lots of unnecessary code and function calls. Its much cleaner to rely on the uid here (you're saving the uid so you need extra function calls to query the user, etc.) ...

Hello,
Remove "project" from the info file, it will be added by drupal.org packaging automatically.
All functions should be prefixed with your module name.
And automated test find some errors. See http://ventral.org/pareview/httpgitdrupalorgsandboxyogesh11101155096git

Hello yogesh1110,

I have checked prefixes of your module and all is OK for me.
Also I reviewed your code a little and don’t found any problems.
Please participate in bonus review program and klausi will write to your soon.

I couldn't find any real issues outside of some code spacing or variable naming.

My one suggestion would be to drop the requirement for the Date Popup module (which would require you to enable multiple modules from the Date package) and simple provide an enhanced date input form item if the Date Popup module is installed.

You should be able to do a simple module_exists() around:

      $form['account']['temporarily_block']['status_block'] = array(
        '#type' => 'date_popup',
        '#title' => t('Block user until'),
        '#default_value' => (time() > $user_blocked_until)? NOW : date('Y-m-d H:i', $user_blocked_until),
        '#date_label_position' => 'within',
        '#date_increment' => 1,
        '#date_year_range' => '0 : +19',
        '#description' => t('The user will be blocked until the given date. If no date is entered, the account is not blocked temporarily.'),
      );

And if the Date Popup module is unavailable simply provide a more basic form item based on the core '#type' => 'date' form type.

One quick note:
in function temporarily_block_users_form_alter () you are using <?php '#default_value' => (time() > $user_blocked_until)? NOW : date('Y-m-d H:i', $user_blocked_until), ?>. Did you intend to use "NOW" as a constant (it is not defined) or as a string (quotes missing)?

Since we are currently quite busy with all the project applications it would be great if you can review some other project applications and get some review bonus.

Thanks for your contribution, Yogesh!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.