GHOP #64: Userpoints for titles

There's two things we have to do to make this work, but theoretically it's not that difficult, over all.

This module stores the total number of points separately so that it doesn't always have to count. This means that when an event happens that increases the number of points a user has, it has to reflect that in its counting. Is there a hook that I can respond to whenever a user gains points?

At that point, it becomes quite easy to have a checkbox to use userpoints, and if it's checked and userpoints is enabled, add those to the count as well.

This is the API from the readme... Will either of these work to hook into? I'd love to see this work with userpoints fo rmy site. :)

userpoints_userpointsapi('points', $points, $uid, $event, $description)
  Use this function to award points to a user.

  The arguments are:

  $op
    Must be 'points'.

  $points
    number of points to add (if positive) or subtract (if negative)

  $uid
    user ID to award points to.

  $event
    an identified of the event the points is being awarded for, this
    is a short word, and will be recorded in the transaction log.

  $description
    a description of the event. This is optional and is a more verbose
    version of the event identifier.

hook_userpoints($op, $points, $uid, $event) 

  Use this hook to act upon certain operations. When other modules award
  points to a user, your hook will be called, among others.

  The arguments are:

  $op: The operation to be acted upon.
    'setting'
      Pass a field set and fields that would be displayed in the userpoints
      settings page. For example, this can be used for your program to ask
      the admin to set a number of points for certain actions your module
      performs. The function should return an array object conforming to
      FormsAPI structure.

    'points before'
      Calls your module, and others, before the points are processed. You can
      prevent points from being awarded by returning FALSE.

    'points after'
      Calls your module, and others, after points are processed. You can take
      certain actions if you so wish. Return value is ignored.

   The rest of the arguments are the same as the userpoints_userpointsapi()
   function.
 
$points = userpoints_get_current_points($uid) 
   You can call this function to know how much points a use has.
   return value is the number of points.

Michelle

we'd probably want to rip out a) nodeapi and comment hooks, b) remove database tables, c) removing the node type settings, d) when checking for the # of points, use userpoints_get_current_points($uid).

dmitrig01: nonono. Just let userpoints add to the existing mechanism.

Set a flag "use userpoints". Then it's just another source. No ripping stuff out.

Changing settings. There is no patch and also bumping the version.

Michelle

This is now a GHOP task: http://code.google.com/p/google-highly-open-participation-drupal/issues/...

(rm duplicate post)

Assigning to self. Recon report coming soon.

I'm disagreeing merlinofchaos's assessment that we need to hook with userpoints. Indeed, userpoints stores post counts so it doesn't have to recalculate them (by summing up the number of nodes the user has created), but this maps cleanly with userpoints' user point count. In fact, changing this "post count" would be undesirable because it would corrupt the previous data and make it impossible to revert back without truncating the cache table.

Patch is actually very simple. Extra credit, I suppose, would be making these changes extensible, so any backend can be substituted for post count.

Updated patch with JavaScript usability enhancements

Revamped patch that implements an extensible interface using hooks. API.txt explains hook format.

Typo-fix API.txt.

Updated patch that fixes a bug that crept back in during one of the revisions (namely, node count settings destruction).

I haven't examined the code but, from a user's perspective, it works great. My only nit was it would be nice to change "posts" to "points" in the adding title section but ezyang said that would be difficult to do with translating. Otherwise, it works as advertised.

Michelle

I haven't tested the patch itself, but I've been told Michelle did, so for now I'm just doing a code review.

• I don't know anything about jQuery, so I can't comment on that stuff.

• +  if ($module != '' && module_exists($module)) return $module;
  +  return '';
  

  Would be a bit more readable as:

  if ($module != '' && module_exists($module)) {
    return $module;
  }
  else {
    return '';
  }
  

  ...and would also conform to coding standards.

• function user_titles_hook_module() is missing PHPDoc, and kind of needs it, since it's not quite obvious what's going on here at first glance. Maybe also rename it to something like user_titles_has_module_override() or something a bit more descriptive?

• +    $link_start = $link_end = '';
  +    if ($info['url']) {
  +      $link_start = '<a href="'. $info['url'] .'">';
  +      $link_end   = '</a>';
  +    }
  

  Let's do something like:

  if ($info['url']) {
    $info = l($info['name'], $info['url']);
  }
  else {
    $info = check_plain($info['name']);
  }
  

  Right now, it looks like if I had a malicious module that returned <script>alert('st33lin j00r c00ki3z!')</script> as its name, the admin would be in big trouble on this page.

• $form_values['types-disabled'] and the like... using a hyphen here looks odd to me. I believe everywhere in core these are underscores. I don't think there's anything particularly "wrong" about the hyphens, but there are API functions in 6 that anticipate this ID possibly being part of a function name, so I'd do underscores for consistency.
• user_titles_get_posts should probably now be called user_titles_get_points, since it's not just about post counts anymore. However, this might break other modules calling those functions directly, so maybe just mark it as a TODO for the 6.x port or something.
• I see you ran this through Coder module. :) Nice.

• +    case 'name':
  +      return 'User points';
  +    case 'description':
  +      return 'Different points values are assigned to user actions';
  

  Those look like interface strings, and if so they ought to be wrapped in t() so they can be translated.

Updated patch, addressing webchick's concerns.

Ok, one more *super* minor revision round, and then I'm marking this RTBC:

-$form['hook-module'] -> please change those to underscores rather than hyphens.

+function user_titles_hook_module() {
+  $module = variable_get('user_titles_hook_module', '');
+  if ($module != '' && module_exists($module)) {
+    return $module;
+  }
+  else {
+    return '';
+  }
+}

I changed my mind. ;) How about:

+function user_titles_hook_module() {
+  $module = variable_get('user_titles_hook_module', NULL);
+  if ($module != '' && module_exists($module)) {
+    return $module;
+  }
+}

... and just have it return NULL if there's nothing found? This is a bit more succinct.

Then I'd do..

+  $form['hook-module'] = array(
+    '#type' => 'radios',
+    '#title' => 'Point scheme',
+    '#default_value' => $hook_module ? $hook_module : 0, // changed this.
...
+  $form['hook-module']['#options'][] = array( // got rid of that weird '' index.

Finally,

+  $hook_module  = variable_get('user_titles_hook_module', '');
+  $hook_modules = module_invoke_all('user_titles', 'register');
+  if (!module_exists($hook_module)) $hook_module = '';

Can't that be written as:

$hook_module = user_titles_hook_module(); // I'm already checking to see if the module exists...no?
$hook_modules = module_invoke_all('user_titles', 'register');

?

Ok, I'm seriously done nit-picking now.

Final revision. Really. :-)

Ok, I redact my previous statement. Fix broken help link bug.

Two more fixes: units is now hookable, and userpoints needs to implement the hook on their own so that we can get rid of our implementation.

*Awesome* work, ezyang! Way to go above and beyond. :)

Can someone commit this please so that those of us who can't patch have access to it.

Thanks for the great patch.

This has been added to 5.x-1.0