When owner edits comment, owner loses points and a different user is granted points

Can you respond with what other modules you are using on the site and with how you have userpoints configured (what modules are installed/enabled and their configuration). Several users are reporting similar issues but I can't duplicate the issues on 2.14 or the CVS version.

Have you tried upgrading to 2.14?

This is happening on my site as well. When a user edits his own comment, he not only loses the points he gained for creating the comment in the first place, but also another unrelated user gets the points added?! In the few instances I've seen it happen myself, the user account illegally getting points added have been some of the first user accounts setup.... could this be a clue?

I've just upgraded from drupal 4.7 to 5.3 (and naturally also switched to the 5.x-2.14 version of userpoints).

Could this be related to the "userpoints_txn" table? I'm also getting the same errors about the userpoints_txn table - same as this guy
http://drupal.org/node/141710

in response to the userpoints_txn table. In the later versions (>2) of this module that table is required. Make sure that you have ran update.php which should have created this table. If it still doesn't create the table you may want to try manually creating the table from the code in the .install file.

Running update.php is working though, so that should be all you need to do.

In regards to users getting odd points from posting comments can everyone exhibiting this problem respond back with more information.
Specifically
What OTHER modules are you running?
What exactly are you module settings and what other userpoint modules do you have enabled?
Is there any custom code interacting with userpoints?

Is there any correlation between the two users (i.e. the one losing points and the one gaining points). Are they within the same comment thread? Have they ever been within the same common thread?

The new(random/weird) users that are receiving points arbitrarily do they have anything in common? Same role? always with a UID < x, etc. etc.

I'll help squash this bug but I need a lot more information than what is posted as I can duplicate the bug myself.

Ok great thanks Jredding...

Alright, first off it looks like I'll need to add that userpoints_txn table manually, as I've run update.php but I don't think its been created for some reason.

Secondly, other modules I'm running alongside it... nothing I can see which stands out as a likely clash with Userpoints.

Audio
Image
Subscriptions
Pathauto
Taxonomy_Menu
Token
Views

Userpoints modules I have enabled currently are:

Userpoints
Userpoints Basic

No custom code interactiing with userpoints (or any other module for that matter). I simply have it set up to reward users with 1 point for a comment, or 5 points for uploading original audio, 2 points for creating forum thread etc.

Absolutely no correlatation between the users involved. The accounts I've witnessed getting the points illegally are some of the very first accounts created years ago - basically dead accounts with no user activity for years, or at all in some cases. My users are ALL in the same role, aside from Admins. So its all standard authenticated users that are involved that I've seen/had reported to me.

I really appreciate any help with this.

Regarding the userpoints_txn DB table - how can I manually add this? I keep getting SQL formating error warnings when I try to use the appropriate sql code snippet from the userpoints.install file?

Heres my post about this:
http://drupal.org/node/185711

thanks

In regards to your userpoints_txn table, answers are on the linked page.

To your problem. I've tested this on 2.14 and I can't find a problem. I've also stared at the code over and over again and I can't figure out what would be causing this. Thought I think that this problem could potentially be squashed with this addition
http://drupal.org/node/184896

However it won't help you in the short term. Because I can't duplicate this issue I, and others, are going to have a really difficult time trying to squash the bug. Please, please, please try your best to figure out if there are any consistencies between user accounts, comments, modules in use, etc. Try to find a way to consistently duplicate this error because until the error can be consistently duplicated squashing it is going to be extremely difficult.

I fully understand that this appears to be random but computers are quite precise so there has to be some cause to this.

To anyone else that is seeing this behavior PLEASE list all the modules that you are using on your site, the userpoints modules you are using as well as your userpoints settings. The more information we have the better.

Ok thanks for the manual sql instructions - I successfully added that userpoints_txn table.... but this issue is still happening despite that unfortunately.

I'll see if I can find any connection between the user accounts. But theres nothing I can see so far.

One thing is that Its always the same user that the points are getting illegally applied to.

So the points being added is consistently on the same user account?
Can you consistently duplicate this error on your site?

Yes what I mean is - if I use a test account "tester", and try and create a comment and then edit it - the points will always go to the same user, for example "bob".

But one of my actual users has told me that whenever he edits his own comments, he loses his points, and they are consistently given to "joe".

So it appears that some sort of pairing up is happening??

I can't be sure, but I think it must be a clue that despite my site having around 6000 users, both "bob" and "joe" account are something like user account 5 and 8.... created a couple of years ago, and not used since.

(Just on a side note - does anyone know why these userpoints issues don't appear in my Recent Posts tracker here at drupal.org? Issues I've posted for some of the other projects do, and I'd like to be able to see these ones there as well. Is this a setting which each project maintainer can choose to enable/disable?)

Ok we are getting closer to squashing this bug as there is definite consistency here. I used mediafrenzy's website to test out and zero-in on the bug.

I'm changing the title to be more reflective of the actual problem.

When the owner of a comment edits their OWN comment, points are subtracted from them and given to another user. That other user is consistently the same.

At this point I've only tested this bug on forum posts/comments.

Disabling the ability for a user to edit their own comments solves this issue.

Although I still don't know exactly who to fix the bug I have figured out the offending lines and why I couldn't duplicate it. Apparently when v2 was branched over to v3 I missed an update to version 2.14 which included some code to move points over from the original owner to a new owner upon editing of the node/comment.

I've since added the right code to version 3.x but, unfortunately the code is working correctly and moving points over to the rightful owner (if it exists).

As a quick fix you can comment out the lines that read.

        // Add to the new comment owner
        userpoints_userpointsapi('points', $points, $comment['uid']['#value'], 'post comment');
        // Subtract from the original comment owner
        $points = -$points;
        userpoints_userpointsapi('points', $points, $orig_uid, 'post comment');

Although I'm not seeing this error on my site when working on mediafrenzy's website I can only assume that the $orig_uid was being set to an incorrect value. Since this is a static variable I'm curious if this is being modified by a different module or if, potentially, $comment['uid']['#value'] is being modified (which would then modify $orig_uid).

Again this behavior is NOT consistent across sites and on fresh, clean installs things are working perfectly. So I still need more information from people. However we're getting closer to figuring out the issue.

If anyone that is exhibiting this behavior is willing to let me SSH into their box (hopefully on a dev site with the same error) I would appreciate it. I can't duplicate the error thus I can not fix it.

I have, however, modified the code (slightly) in version 3.0 to unset some variables after use and put in a few extra checks to make sure that it is being handled correctly. On my end things are working just fine though.

That was my code :(
The purpose was to update userpoints when a node or comment changes authorship (mostly happens when admin edits the author field of a node/comment, I wanted userpoints to reflect this change.)

I just spent 5 minutes on it and I am at work can't verify it now, my theories are:

1. As jredding suggested, I suspect there is conflict between modules. Other modules implementing hook_comment may have somehow changed the $comment['uid'][#value]. You can try forcing userpoints_comment to run before any other hook_comment kicks in by setting it's weight in system table:

Backup your database and try it on your dev site first:

UPDATE system SET weight = -100 WHERE name='userpoints';

Just a quick thought, but may worth a shot.

2. It won't fix the problem *if* we used the wrong 'op' in the first place. Maybe we should use 'validate' instead of 'form' op in hook_comment. Need to read api docs and comment.module to verify this.

3. As a temporary solution, maybe we could just get rid of this 'change-of-author' code for now. After all, its a rare case that node/comment authorship would change.

didn't even think of checking the $op.. ;) When I get a chance I'll look into it.

I definitely think we have some other modules at play here.

Just responding in a fresh comment, that I can NOT duplicate this bug on my configurations if YOU are exhibiting this problem and will grant ME access to your site to work on this bug I will do my best to fix it.

Thanks mediafrenzy and jredding for pointing me here when I had missed this thread.

I'm in the exact same situation as mediafrenzy, I've just upgraded from drupal 4.7 to 5.3 and have 5.x-2.14 version of user points.

In some cases, it will simply take the two points (my score for comments) away from the user doing the editing and adding them to anonymous instead (user 0). In other cases (see attached image) unrelated users are involved, user 1 edited a comment by user 306 (Andreas-Udd) and user 306 looses his points to user 2 (Claymore) who had nothing to do with it. What the heck..?

jredding , do you still need access to a site that has this kind of funky to see it in action? Mine has this quirk.

Yes I still need access to a site that is exhibiting this behavior but I need SSH access preferably to a non-production dev site. On a clean install of Drupal+Userpoints this doesn't occur. This bug might be brought on by a combination of modules, I'd like to dig into someone sites so that I can see what the comment (and node) forms look like before the edit, during the edit, and after the edit.

For some reason of other a variable ($orig_uid) is being set incorrectly thus causing these points to migrate from one user to the next. Mediafrenzy was kinda enough to let me create a user account on his sites and post various nodes/comment so that I can duplicate the error now I need the opportunity to really dig into the "behind the scenes" of a site.

Anyone with a dev site out there? Otherwise I will have to exactly duplicate someone's site modules and all. Unfortunately I just don't have the time for that.

Contact me directly if you have a site that I can get into the backend with.

@Dabitch - maybe you could post a list of your sites other modules, so that we can compare and see what similarity there is? If we can try and narrow it down perhaps that might help jredding and others pinpoint where the trouble lies?

Hello,
Having the same bug here.
We tried to correct this bug on our own, here's what we did:

/************************
 * Correct behaviour on comment edition users lost 1 point per modification
 ************************/
  case 'form':
      $orig_uid = $comment['uid']['#value'];

 // Add of:
      break;

  case 'update':

// Replacement of:
//    if ($orig_uid != $comment['uid']['#value']) {
//
// by:
      if ($orig_uid != $comment['uid']) {

        // Add to the new comment owner
        
// Replacement of:
//        userpoints_userpointsapi('points', $points, $comment['uid']['#value'], 'post comment');
//
// by:
        userpoints_userpointsapi('points', $points, $comment['uid'], 'post comment');

        // Subtract from the original comment owner
        $points = -$points;
        userpoints_userpointsapi('points', $points, $orig_uid, 'post comment');
      }
      break;
/*********************
 * End of modification
 *********************/

The case 'form' was lacking of a 'break' statement, and, in case 'form', $comment is not the same array as in case 'update'.
This works well with our Drupal installation. Could anyone give a feedback on this patch?

Thanks,
David

Drupal 5.3
Userpoints 2.14 and 5.x-3.x-dev (2007-Oct-24)

The following steps failed to reproduce the issue:
1) Install Drupal
2) Enable Userpoints and Userpoints Basic modules
3) Create a test user account
4) Create page, be sure to set Comments to: "Read/Write"
5) Login as test user
6) Go to node created in 4, and add comment
7) Edit comment

Also failed: Creating 3 test users and adding/editing comments to the node

I did a search though all my modules, and this userpoints code is the only code that refers to $comment['uid']

The modules that my site shared with mediafrenzy are: Pathauto, Token, and Views. I installed each and configured each, but still failed to reproduce the issue.

I'll keep trying to discover the steps to reproduce.

Just confirming what sto said above:

When $op=='form', the key is $comment['uid']['#value'].

When $op=='update', the key is $comment['uid'].

raspberryman
Thanks for your comments and taking the time to test things out.. Its appreciated.

Thanks sto, it looks good to me. However, since I can't duplicate the original problem on my install, I can't verify if it's a fix. Anybody can try and confirm it? I'd like to see this issue resolved and closed asap, especially the buggy code was from me :)

Right so where do I need to put that patch code? Do I just add that to the end of the userpoints.module?

I'll give it a test once I've got that confirmed,

I'd like to get this sorted as well.

Oh i see - its replacing those 2 commented lines and adding a couple of new things. Alright, I'll give it a shot when I have a spare moment

sto's patch worked perfectly on my site. Kudos! No side effects found. (yet..knock on wood spit three times)

sto, can you please read http://drupal.org/patch and create a patch in unidiff format?

Or at least say which module/version that you made the change to? userpoints.module or userpoints_basic.module?

Hello,
I will get you a unidiff patch as soon as I can access to my workstation, thus monday.

See you,
David

Hello,
Here is the patch.
It was made from 2.14 module version.
Thanks,
David

jredding,

Can we roll this in 3.x?

I have a different plan for fixing this bug in version 3.x. I'll post it up later tonight.

Desperately waiting for the patch for version 3.x. Though I'm having another issue which appeared coupled with this one. Some people are complaining about Private Messages (PM module) being sent to the wrong person. Still figuring out exactly what the behavior is.

Thanks.

version 3.x has been updated to include this feature --> http://drupal.org/node/184896
This means that version 3 now tracks which node or comment caused the points to be acquired. It also means, in my humble opinion, that we can correctly implement the ownership change feature. In turn it does squash this bug but only for new points granted by up v3

This is how I propose to squash this bug
In hook_update I'd like to query the DB and compare the uid with the result. This eliminates the need for a static variable and also shields against any form modifications by other modules.

example/pseudo code

userpoints_comment($comment, $op) {
  switch ($op) {
    case 'update' : 
     $txn = db_fetch_object(db_query("SELECT uid 
                                                          FROM {userpoints_txn} 
                                                          WHERE entity_id = %d AND entity_type= %s AND operation = %s", 
                                                          $comment['cid'], 'comment', 'insert'));
     if ( $txn->uid != $comment['uid'] ) {
         //subtract points from $txn->uid
        //add points to $comment['uid'] 
     }
     break;
  }
}

Essentially I want to rely upon the database to tell us who actually has points for this particular comment or node instead of simply assuming that the current owner/author has received points for it (they may not have).

Again if we take this route (which is cleaner, in my opinion) existing points (pre v3) WILL NOT change ownership thus leaving a strong potential of having an odd site wherein some points change ownership correctly and others do not.

We can take the current code (with the fix provided above) and move them out to a contrib module so that it does continue working (bugs n' all ;) )

The part I don't like about this is pre-v3 points vs. post-v3 points. This can lead to a lot of nasty situations (e.g. editing a comment made pre-v3 will behave differently from a comment made after v3 was installed).

We can make it a fall back strategy: if we don't find the info we need, we use the code above.

This could be a config option, or even something automated, yet optional, like this.

if (module_exists('old_comments_compatibility')) { 
  // do the fallback strategy
}

So if the module is installed, we do that backward compatibility stuff, otherwise, for a new site, or a site that never used pre-v3, this is skipped.

dami, what do you think?

Thanks jredding, it's a much more reliable solution than my buggy one. However I agree with kbarhey, we need to find a way to take care of pre-v3 points. Can retroactive.module take care of this by recalculating and populating txn table with proper records for pre-v3 points?

Before we delve more into it, I suggest commit the v2 patch to fix the existing problem first. Then we can either continue discussion here or move to a new thread.

hhhm.. using the retroactive module is a decent idea. However people would need to wipe out all the original points and recalculate. Its actually quite an excellent idea!

This is what I propose

#1) Update the retroactive module to recalculate points for all users nodes/comments/etc using the new API calls so that nids/cids/etc. are grabbed. This would mean that all points granted by the userpoints_basic.module would be accounted for with nids/cids/etc.

#2) Place my proposed code (DB comparison) in the userpoints.module to properly subtract/add points for author changes.

#3) Take Dami's code (with the patch above) and place it in a contrib module for those people that absolutely can not (for whatever reason) use the retroactive module.

#4) within the userpoints.module do a quick if (module_exists("damiscode")) to run that code

--------------------------

I'd also suggest that the "Official" stance is to retroactively recalculate ALL points granted by the userpoints_basic.module so that they are better handled. This would make the DB much much cleaner as well as provide a ton of information to users and administrators.

No offense to Dami's code (sorry Dami) but its nature is just a bit hackish and is incredibly prone to fluctuations in the form. Obviously it was the only thing that could've been done b/c of the DB structure but thats fixed now so we should also fix that bit of code.

Thoughts? Comments?

No offense to Dami's code (sorry Dami) but its nature is just a bit hackish and is incredibly prone to fluctuations in the form. Obviously it was the only thing that could've been done b/c of the DB structure but thats fixed now so we should also fix that bit of code.

Not at all! My code was indeed a hack. It's supposed to take care of an edge case, but proved to be buggy. I am perfectly fine with your proposal and appreciate your taking time to fix it in v3. Let me know when the code is in, I'd be glad to test it out.

aaah.. trouble in paradise. I forgot that retroactive points just SUMS the possible points and then adds a single point entry. I thought it added new points for every node/comment/etc available but I was wrong.

hhm.. I'll look into how much work it would be to upgrade it to provide this functionality. If anyone has some free coding time feel free to ping me and I'll point you in the right direction.

subscribing to this issue.

patched my 2.14 with comment #29's patch. seems to have fixed the problem. D5.3

Thanks,
kpaul

Subscribing for a solution in the 5.x-3.x version

Just an update on my part as I was planning on fixing this bug. I just moved to a new city (and country) and I'm busy with finding a place to live. I probably won't get to this bug for another two weeks.

If anyone wants to help squash this bug I can tell you exactly how to do it. I just don't have the time to do it myself.

Hello,
I spotted another bug concerning the same piece of code.

case 'prepare':
    $up_orig_uid = $node->uid;
    break;
case 'update':
    //!!!!!THE CODE BELOW MIGHT BE CAUSING A BUG
    //On clean installs of Drupal it works fine
    //on certain installs it takes away points
    //from the rightfull owner (when ownership hasn't changed)
    //http://drupal.org/node/183520

Actually, the update case assumes the node was prepared before (and $up_orig_uid was set). Which is not the case when modules save a node with node_save without preparing it before (like a module that would trigger an boolean field basing on some indicators on a daily basis, without using node_prepare or even node_load).

I don't have any suggestion on how to correct this bug in a pretty manner, but I think I spotted the erroneous code...
I hope this will help.
jredding> maybe you could tell me a little more of your solution, and I will try to do something... :)

Regards,
David

Edit: after thinking of it, there seems to be two completely different problems: one concerning comments submission/edition, corrected by the patch I committed in #29, and the one I am describing just above...

Sto: Sorry I've been swamped lately with moving and other things so I haven't had the time to fix this but its a really simple fix.

First things first. The code that presently exists in the module (please take no offense Dami) is a hack. It works but its a hack. Thus the fix in comment 29 isn't a fix in my opinion (its a hack on top of a hack). The real fix came in the most recent changes to userpoints with the addition of the entity_type and entity_id fields.

Now when points are accumulated entity_type is filled with either comment or node and entity_id is filled with the comment ID or the node ID (respectively). Thus when a comment is updated ($op == update via hook_nodeapi) you can do a quick SQL statement (shown above in comment #33) to pull the points associated with this node. You can then compare the ID of the NODE author with the ID of the POINTS granted to see if they match. If they match you don't do anything if they don't match then the node author has changed thus you should subtract points from the original author and grant the points to the new author. This should be done by two new point entries.
The first entry negates the points granted to the original owner
The second grants points to the new owner.

Now the problem is that this doesn't work for points granted BEFORE version 3 because those points won't have node ids (because the table structure was different in version 2).

So we have two options

1) We include the code above (comment #29 and your suggestion) IF the SQL statement fails

2) Update the retroactive module so that it recalculates on a per node and per comment basis ALL points for all users. This is a major change to the module as it currently only grants a lump-sum total of points. This change, however, would make #1 unnecessary.

These two options are simply for backward compatibility (which is very important).

So where things sit right now is that I haven't implement my suggestion(s) because I don't have the time (but directions are above). If you have the time to work on it I'm volunteering the little time I have to guide you and critique code. I want this done as much as anyone but I have bigger issues on my plate (like a place to live... no home = no place to code ;) )

CVS commit today into version 3. The bug is squashed for all NEW points accrued by version 3. Backward compatibility is not supported yet. I'll put that in tomorrow.

CVS commit today into version 3. The bug is squashed for all NEW points accrued by version 3. Backward compatibility is not supported yet. I'll put that in tomorrow.

CVS commit today into version 3. The bug is squashed for all NEW points accrued by version 3. Backward compatibility is not supported yet. I'll put that in tomorrow.

the problem that i face is this...

+ say i post a node / comment. i get the appropriate user points for posting this.
+ subsequently i edit it.
+ as a result of the edit, i lose the original points i gained... instead, either user #1 or some other (apparently) random user gains the points.

i would like to know if this is fixed.

This has only been fixed for new nodes/points added via version 3 of the module. If you have points added via version 2 then it has not been fixed yet.

The question for you would be if you are only using v3-dev and have you ever used v2.x to assign points. (i.e. have you at any point used version 2)

i am currently using version 2.14 of userpoints.

what are the known issues using the version 3.x-dev to override the nodes/points added via version 2?

Please read the issue queue before posting.

The bug has been fixed for version 3 only and hasn't been resolved for version 2.14. Some people have submitted patches for version 2.14 that fixes the issue but these haven't been fully tested and there is question as to their effectiveness. Version 3 is still in development.

I don't understand your second question but there are issues with version 3 as its in development but they are being squashed as quickly as time permits. There is no current development on v2.14 (that I'm aware of)

jredding,

i read the issue queue and i understand that the fix isn't done for version 2. however, my question is: if i update to version 3, will there be any adverse effects to the nodes/points created using version 2?

you also mentioned something on backward compatibility... is this in place?

i read the issue queue and i understand that the fix isn't done for version 2. however, my question is: if i update to version 3, will there be any adverse effects to the nodes/points created using version 2?

there could be. Version 3 is in development so only use it only a development site.

you also mentioned something on backward compatibility... is this in place?

no. You are more than welcome to get involved in the development to help put this in place. All of the pieces have already been talked about in the issues queue. I'm not 100% sure how we are going to implement backwards compatibility given the fact that this particular feature was a hack to begin with and hasn't been stable code. Again the answers to this are discussed in this issues queue.

OK final commit was made today. The fix has been put in place for both all new points granted by version 3 as well as points granted by version 2 of the module.

I really have to apologize to dami for this but I feel pretty strongly that the original code that caused this bug was a hack and is subject to all kinds of weird issues. Specifically because its a static variable that is only checked during a nodeapi call its going to have issues with custom module that do odd things with the node or comment form, modules that do node_saves directly and various other items.

The new userpoints_basic currently checked into CVS does implement dami's code as well as the patch submitted by sto in #29 (http://drupal.org/node/183520#comment-621087) however you MUST ENABLE IT via the settings page.

Ideally I would much rather this be squashed by an update to the userpoints_retroactive module so that all points are recalculated so that we can capture the node ID and Comment ID in order to do a more clean ownership check/move operation.

In either case. the fix is in.