[D7] User point limit

Hi jaffaralia,

Thanks for submitting a project application for review.

The PAReview tool identified a few issues which need addressing:

• Remove "version" from the ./userpoints_limit.info file, it will be added by drupal.org packaging automatically.
• Remove "project" from the ./userpoints_limit.info file, it will be added by drupal.org packaging automatically.
• Remove all old CVS $Id tags, they are not needed anymore.
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

Please check/fix these issues before changing the status back to "needs review"

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hey just did a quick automated and manual review:

Automated review (trivial):

FILE: /mnt/www/html/pareviewsh/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 36 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /mnt/www/html/pareviewsh/pareview_temp/userpoints_limit.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 6 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /mnt/www/html/pareviewsh/pareview_temp/userpoints_limit.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 52 | ERROR | BREAK statements must be followed by a single blank line
 52 | ERROR | Break statement indented incorrectly; expected 6 spaces, found 4
--------------------------------------------------------------------------------

Manual Review

README.txt: Explains the project succinctly
Project Page: The project page for this project is fine.
Master Branch: It appears you are working in the "master" branch in git. You should really be working in a version specific branch. See: http://drupal.org/node/1127732
Code too short: No the code is large enough as clocks in at around 200 lines and is well documented.

Hello jaffaralia!

Thank you for your nice module!

To make your code longer (sylus proposed that), you can add a help hook (hook_help) and then put the content of or readme.txt file in.

For me the module works as described .... nice

Dear Josef Friedrich

Hi jaffaralia,

Nice module.
Your latest changes introduced some coding standard problems :

Automated review :

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

Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
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.

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
4 | WARNING | Line exceeds 80 characters; contains 203 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/userpoints_limit.module
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
14 | ERROR | There should be no white space after an opening "("
15 | ERROR | There should be no white space after an opening "("
19 | ERROR | Case breaking statements must be followed by a single blank line
20 | ERROR | Line indented incorrectly; expected 4 spaces, found 6
66 | ERROR | Case breaking statements must be followed by a single blank line
67 | ERROR | Line indented incorrectly; expected 4 spaces, found 6
--------------------------------------------------------------------------------

Manual Review

with my skill I have not found anything !

Tips !
You can use http://ventral.org/ for have a quick check of your module !

NoisetteProd

Passed all automated tests.

Manual review:
Code seems pretty clean and very well documented, just have a few small remarks.

1. When first enabling the module, the limit for a day is 0. It confused me for a while until I played with the settings a little and noticed the option.
2. When the "Your points exceed limit of a day" error is shown, it's shown as a success message (drupal_set_message("...", "success"), instead of drupal_set_message("...", "error")), should be changed to error, because it's a blocking transaction.

Manual review:

In line 75 of your module, you can remove one set of parenthesis, so your code is like this...
if (($duration = (variable_get('userpoints_limit_duration', 0) - 1)) && variable_get('userpoints_limit_points', -1) != -1)

Code comment at line 82: You should capitalize the sentence:
// Check time limit...

Also, for long comments, consider using this style...

/* Enforce consistent sort order.
 * Check time limit extends the duration
 * for eg: if user logged in 1'st day. till 2nd day he didnt logout but
 * the limit is one day.. so, we need to check and reset
 * userpoints_limits table.
 */

At line 174 you can eliminate one set of parenthesis, like this:
if (($interval = variable_get('userpoints_limit_duration', 0)) && variable_get('userpoints_limit_points', -1) != -1)

Line 58: You need a period at the end of the message. Also, the message always says "Your points exceed limit of a day". Instead this message should be generated dynamically depending on the settings for the limit. If there is a 40-point limit over a 3-day period, then the message should say something like "You may accrue a maximum of 40 points during each 3-day period."

Line 59: You don't have to capitalize "Limit".

I'm not sure you need an lid column in your table. If there will never be more than one row per user, you could use the uid column as the primary key.

Thanks for your contribution.

You also need a period at the end of the sentence at admin/help/userpoints_limit

Manual review:

Line 31: $intervals = array(...
Would it be better to use a variable here, so other developers could override array of intervals if needed.

Lines 131 - 144 and 150 - 162 look identical to me. I think this code can be separated to a function to make code DRY.

Lines 92 and 103 - consider getting last timestamap before if, as you seem to need it in all cases.
I suggest moving all code that is doing selects to functions as it takes a lot of space and repeats. This will also allow you to get rid of $last_timestamp->timestamp construction which also takes space.

Good luck with module.

Though I guess there is less duplication in the code, it is still hard to follow.

1. When I was suggesting a function for getting a timestamp I was referring to something like this:

function userpoints_limit_get_last_timestamp($params) {
  $result = db_select('userpoints_limits', 'p')
    ->fields('p')
    ->condition('p.uid', $params['uid'], '=')
    ->execute()
    ->fetchObject();
  if ($result) {
    return $result->timestamp;
  }
  return FALSE;
}

This will allow you to write:

  $last_timestamp = userpoints_limit_get_last_timestamp($params);
  if ($last_timestamp) {
    ...
  }

Instead of:

  $last_timestamp = userpoints_limit_generate_query($params);
  if (isset($last_timestamp) && !empty($last_timestamp->timestamp)) {
    ...
  }

2. Just noticed that you are writing to the database, and querying it straight after:

 $limit['uid'] = $params['uid'];
 $limit['timestamp'] = time();
 $ret = drupal_write_record('userpoints_limits', $limit, array('uid'));
 $last_timestamp = userpoints_limit_generate_query($params);

Could the last line read like this without changing the logic? :

  $last_timestamp = $limit['timestamp'];

Code seems fine, well documented and seems different enough from userpoints_cap.

----
Top Shelf Modules - Enterprise modules from the community for the community.

No further action is required, but the best thing you can do is get a Review Bonus by reviewing other applications. That will get you to the top of the list of projects to get reviewed (and hopefully approved).

----
Top Shelf Modules - Enterprise modules from the community for the community.

A also suggest adding tests.

It's been a while now, so

Thanks for your contribution, jaffaralia!

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.

----
Top Shelf Modules - Enterprise modules from the community for the community.