[D6] Temporary Suspension

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxadam_thomason2158463git

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.

Hi,

There are two issues found on http://pareview.sh/pareview/httpgitdrupalorgsandboxadamthomason2158463git

FILE: ...var/www/drupal-7-pareview/pareview_temp/includes/temp_suspend.admin.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
98 | WARNING | Variable $converted is undefined.
105 | WARNING | Variable $converted is undefined.

Kindly fix them.

ALso @file comment is missing in hide_unused.js

Minor coding standard errors are not application blockers, please do a real manual review.

* The CSS file in your Git repository is empty. Can it be removed? If you do, also remember not to load it in your code.
* temp_suspend_page(): Links should be created with: ll()
*temp_suspend_suspension_handler(): Messages passed to drupal_set_message() not translated. Should be drupal_set_message(t('f00'))

temp_suspend_block_info() , temp_suspend_block_view():
hook_block_info() and hook_block_view() are not implemented in drupal6, kindly use hook_block() to create blocks.

Hi Adam

A few other minor issues i noticed:

In temp_suspend_settings_form_validate() and temp_suspend_suspension_handler() could you use the constants TEMP_SUSPEND_FULL, TEMP_SUSPEND_TEMP & TEMP_SUSPEND_REVOKE rather than 0, 1 & 2 like you have in temp_suspend_settings_form(). This would make it easier to follow the conditional logic.

Definitely not a bug, but the suspension of a user is likely to be something that other module may wish to pick up on (maybe to send an email or notify some 3rd party service for example). It would be nice if a hook be triggered (via module_implements or similar) to allow this to happen (You could even have temp_suspend_suspension_handler() as an impliementation of that hook rather then being called explicitly).

Good luck with the module

Andy

Hi Adam,

Great module mate...

Just did a manual review of your code and found some minor issues with it.
- LINE 50: $link = 'uid) . '">' . $user_name->user_name . '';
Wouldn't it be better if we use l($user_name->user_name, 'user/'.$user_name->uid); method instead of above?

- The module also requires CCK to work. Can you please include this in .info file.

- The module also allows you to suspend root user. Is this desirable?

Good luck...

Ash

In temp_suspend_page(), you should use l() to create links.
Also see http://drupal.org/node/447604

$date = date('d-m-y');
$current_time = strtotime($date);

With the above code as $date isn't used elsewhere in the function you could merge the two lines as:

$current_time = strtotime(date('d-m-y'));

Apart from that the module code inside looks good and the module works in Drupal 6. When's the Drupal 7 version due? :)

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

If you would like to further participate in the review bonus program just make sure to review a project in general; coding standards and branch naming are only one part which is covered pretty well by the automated review tools.

Does the project make sense? Is it a duplicate of an existing project? Are there suspicious parts in the code that could be security issues? Is it clear that the developer knows what she is doing and using Drupal as intended? Are the points of the review checklist ( https://drupal.org/node/1587704 ) covered? Then leave that outcome as feedback comment.

Hi Adam,

I have installed your module locally and can confirm that it works to your specification. From what I can see, there are no blocking errors in your code. Confirming RTBC.

Regards,
Fried Green Guatemalan

Review of the 6.x-1.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/includes/temp_suspend.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   109 | WARNING | Variable $converted is undefined.
   116 | WARNING | Variable $converted is undefined.
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. temp_suspend_date_validator(): why do you take $converted as parameter? It is overwritten immediately on the first line of the function? It should suffice to return $converted, right?
2. temp_suspend_schema(): why do you store user name and uid? Uid should be enough? Please add a comment.
3. temp_suspend_schema(): we usually store dates as unix timestamp integers in drupal, that way we can easily query the database for outdated suspensions in temp_suspend_cron(). Your current implementation will not scale if you have a large number of suspensions, since you have to iterate over the whole table in PHP on every cron run.
4. temp_suspend_page(): doc block is wrong, this is not a hook?
5. temp_suspend_block(): doc block is wrong. And does this function even work? I think that hook implementation must be in the module file in order to picked up, or you have to require that include file from the module file. Also, the function signature looks broken, see https://api.drupal.org/api/drupal/developer!hooks!core.php/function/hook...

So although this will not scale on large sites and the block implementation is completely broken, the rest looks good.

Thanks for your contribution, adam_thomason!

I updated your account so you can 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 stay 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.