This does not work YET. However I have it hammered out most of the way, but when I actually try to send the points, it does not go through. This is accompanied by a very non-descript error:

preg_match() expects parameter 2 to be string, array given in /var/wwwtest/includes/bootstrap.inc on line 723.

A warning, I have never done much PHP, and I just really was using the 'coder' module to do this. Still, I have a hunch it will work if this one error is resolved.

I have never had much luck creating patches either, so I just made a tar.gz archive for you in case the patches don't work. I hope that's ok.

Comments

neomenlo’s picture

Status: Active » Needs work

Sorry, I didn't classify the status properly.

kenorb’s picture

subscribing

OneTwoTait’s picture

subscribing. I could really use this. :)

johnbarclay’s picture

Version: 5.x-3.x-dev » 6.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new51.67 KB
new3.26 KB

I worked from neomenlo start and got what I think is a working D6 version. Its attached as a zip. Since I haven't used it in drupal 5, when you test it keep an eye out for what you would expect in D5. Also note there is no update code to execute to go from d5 to d6 for user2userpoints.

There are still some to do's that I think would be useful, such as being able to use the form in a block, making it an enpoint for REST calls, and something more theming functionality.

I added some configuration inputs into the points settings page (/admin/settings/userpoints) using hook_form_alter. See attached screenshot.

I also added a category select input to pick the correct term. I'd never used the drupal 5 version of the module, so I'm not sure how a points category was assigned to the transferred points.

It handles urls of form:

/user2userpoints/[to]/[amount]/[tid]
where:
[to] is uid or username of user points are given to
[amount] (optional) is points quantity. defaults to 1 if empty.
[tid] (optional) taxonomy id, 'choose' to have user choose, 'all' to pass all param to userpoints api

eg:
/user2userpoints/jbarclay/5/51 (apply 5 points to jbarclay in in term 51)
/user2userpoints/11/5/51 (apply 5 points to uid #7 in in term 51. User may select another term)
/user2userpoints/jbarclay/5 (apply 5 points to jandoe and let them select category)
/user2userpoints/jbarclay (apply to jandoe, user selects amount and category)
/user2userpoints/ (blank form)

these urls will also take get and post parameters (to , amount, and tid). Same parameters in url override get or post values.

kbahey’s picture

Status: Needs review » Needs work

1. Please use the coder module to check the code style for this module. It needs a lot of attention to confirm to Drupal's code style.

2. Why do we need an explicit user2userpoints_admin_submit() function?

3. Remove the debugging code, print/print_r().

4. The module accepts parameters from the URL. I have not digged deep enough to see if this is before the form, or after the form. But if it is after the form without a confirm form, then it opens the door to Cross Site Request Forgeries (CSRF). If someone tricks a user into viewing an image that links to a URL for giving points, they will give points unwittingly. Please read this http://drupal.org/node/178896 and this http://drupal.org/writing-secure-code. You need to add a confirm form to giving forms.

johnbarclay’s picture

Thanks. This is very helpful. #1,#3, and #4 I'll take care of.

With #2, I wasn't sure how to add configuration options for user2userpoints. So I did the hook_form_alter to the form at:
/admin/settings/userpoints

What's a better way to do this?

John

kbahey’s picture

Please read the API section in the README.txt file in Userpoints, and look at how other modules (e.g. userpoints basic) do this. It is simpler than that.

johnbarclay’s picture

Status: Needs work » Needs review
StatusFileSize
new2.96 KB

Got it. Thanks. I like that approach. The attached module fixes the 4 problems.

kbahey’s picture

1. I don't see anything done for code formatting using the coder module. See http://drupal.org/project/coder and apply it to your code. I tried doing the formatting manually but it got tedious.

4. I don't see a confirm_form() call to prevent CSRF. But I have not run the module. So here is what to look for: If someone visits /user2userpoints/uid/points, do the points get given immediately, or does the user have to click "submit" to give the points? If there is a submit (i.e. a POST operation), then we are safe. If the points are given immediately, then we have a problem since this causes CSRF.

5. The function user2userpoints_giveform() is very hard to read because of the inline Forms API stuff. You must change it to have the logic separate from the FAPI elements. Do the logic first, then have normally formatted FAPI elements instead of appending to elements and such.

6. Also, why not junk the usernames in URLs altogether? Make things simpler: just the uid, or uid + points, or uid + points + tid.

kbahey’s picture

Status: Needs review » Needs work
johnbarclay’s picture

Status: Needs work » Needs review
StatusFileSize
new2.95 KB

1. I tested for minor problems with the coder module and everything came out fine this time.
4. Yes. You do have to confirm with the submit button.
5. I cleaned this up as you suggested. Reads much better now.
6. I left the username in for backward compatibility and because sometimes people want a readable url. Also how handy the uid or username are depends on what context you are generating the url in.

kbahey’s picture

Status: Needs review » Needs work
StatusFileSize
new2.55 KB

Much better.

I did some reformatting and simplified the code in some places.

The part I don't like is this:

'page arguments'   => array('user2userpoints_giveform', NULL, NULL),

And this:

function user2userpoints_giveform(&$form_state, $op = NULL) {
  $to_id = (arg(1)) ? arg(1) : $_GET['to'] . $_POST['to'];
  if (is_numeric($to_id)) {
    $to = user_load(array('uid' => $to_id));
    $to_id =  $to->name;
  }

  $amount = (arg(2)) ? arg(2) : $_GET['amount'] . $_POST['amount'];

  $default_tid = variable_get(USERPOINTS_CATEGORY_DEFAULT_TID, NULL);
  if ($default_tid) {
    if (is_numeric(arg(3))) {
      $tid  = arg(3) ;
    }
    elseif ($_GET['tid'] . $_POST['tid']) {
      $tid = $_GET['tid'] . $_POST['tid'];
    }
    else {
      $tid = $default_tid;
    }
  }

You should use this:

'page arguments'   => array('user2userpoints_giveform', 1, 2, 3),

And

function user2userpoints_giveform(&$form_state, $account = NULL, $amount = 0, $tid = 0) {

And they get mapped into variables automatically by FAPI, without you having to call arg() all the time. Also don't use $_GET and $_POST directly because of security issues.

I am attaching my version, which you should use as a starting point, then fix the above issue and it will be ready for committing it.

johnbarclay’s picture

Status: Needs work » Needs review
StatusFileSize
new2.78 KB

Thanks. I made these changes. Code keeps getting shorter :) I appreciate your mentoring. Are there any other userpoints modules you'd like me to port to 6?

kbahey’s picture

Status: Needs review » Fixed

Thanks. This should be available in the -dev tarball tonight.

As for other modules, Download the latest 5.x -dev tarball and see what is not in 6.x and port them as you find them interesting and/or find time for.

jordanmagnuson’s picture

Subscribing.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

mikou’s picture

Hello, sorry for reopening this issue.
I wonder if there is a way to get a fivestar vote on a user by combining user points and user2user ?
The second thing would be to give a points to another user without decreasing your.