Drupal 7 port (userpoints_cap)

Looks good, just some minor issues...

+++ b/userpoints_cap/userpoints_cap.moduleundefined
@@ -6,50 +6,75 @@
+    break;
     case 'points before':

The break should be intended two spaces, on the same level as the code inside the switch, I think.

+++ b/userpoints_cap/userpoints_cap.moduleundefined
@@ -6,50 +6,75 @@
+      if (variable_get('userpoints_cap_enabled', TRUE) == TRUE) {

The == TRUE part is unecessary. What happens is that the return value of the variable_get() call is casted to a boolean value, compared with TRUE which again results in a boolean value which is then checked by the if(). So that part is unecessary.

+++ b/userpoints_cap/userpoints_cap.moduleundefined
@@ -6,50 +6,75 @@
+          $message = t(variable_get('userpoints_cap_message_' . $params['tid'], '@Category !points are limited to a maximum number of @cap. You must spend !points to earn more.'),
+            array_merge(userpoints_translation(), array(
+              '@Category' => ucfirst($category),
+              '@category' => lcfirst($category),
+              '@cap' => $max_points
+            ))

The problem with ucfirst/lcfirst is that it might not work so well in non-english languages. Not sure how to do it better, though. And they can always directly use the category name directly.

Also, the text should probably not be translated at all. Instead, multi-lang variables should be used with help of i18n.module.

Powered by Dreditor.

You do not need to add any dependency. I18n can be configured to make any variable translatable and will automatically display the string depending on the user language. All you need to do is not calling t() (t() should never be called with dynamic strings anyway, it is just not designed for that).

If users only have a single language, they can enter the text in whatever language they want. And for a multisite, they can configure i18n.

Hm, yeah, I forgot about the parameters. One solution would be to use strtr() for the replacement, just like t() does if locale.module is not enabled. See http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/t/7

Something like this (translatable, configurable strings, especially with placeholders) is currently a real issue, there is no nice solution for it that I know of.

Hey Bastlynn,

The functionality of the module works well. I didn't notice any obvious bugs when testing.

I'd just suggest the following string changes:

a) For the field labels in the settings, I think it may be better to put the category first and keep them shorter (since there's a description for each setting anyway). I'd suggest:

"General category cap"
"General category message"

b) Here's a suggestion for a more direct default message for each category:

"You may not have more than !cap !points in the !Category category. You must use !points to earn more."

c) The description for each message settings seems kind of long (3 lines in Seven theme), which doesn't look great. To shorten, I'd suggest:

"The message a user will see if they exceed the cap. Available replacement values include !Points, !points, !Point, !point !Category, !category, and !cap."

Thanks for your work on this,
Ben

+++ b/userpoints_cap/userpoints_cap.moduleundefined
@@ -6,49 +6,74 @@
+          /* Not using t() here is deliberate. See conversation at: http://drupal.org/node/1133828 */
+          drupal_set_message($message);
+          return FALSE; // DO NOT add points
+        }
+        else {
+          return TRUE; // OK to add points

Minor: Please move comments on a separate line, always start with // and then an uppercase character and end with a .

Also, another idea that I just had. Say the cap is at 5000 points, someone has 4997 points, is granted 5 points. Instead of simply denying the points transaction, we could change it to 3 points and only grant that so that he exactly reaches the cap. $points can be per reference now, so that should be possible to do.

And yet another idea from #985948-6: Create a "Flood control" sub-module to limit points that can be added/deducted over a configurable amount of time, we might want to make the action configurable, so that instead of just blocking, you could also insert the points, but with status pending or declined.

Powered by Dreditor.

Fixed the comments, tested it and commited.

My ideas in the last comment could be done as follow-up issues.