The audit trail is not kept if $op is "txn approve" in userpoints_userpointsapi PLUS the module needs a searchable index

Hi,

OK, I wrote a patch that fixes this problem.
This patch is a pretty strong rewrite of userpoints_userpointsapi().
Basically, now $op can be:

'points' - Points are awarded instantly if the module is configured without moderation, or they might just end up in the trail as "unapproved" points. NOTE: even if the module is not set to moderate, entries are added to the trail

'txn approve' - Points are approved, and the trail is ignored. This is used by the module to confirm points. It could also be used by other modules to add "un-trailed" points

'points approved' - Points are awarded instantly, and the the right information is added to the trail. This is very useful for modules like "simple_karma", which might end up awarding a lot of points (every time somebody votes)

This patch ALSO adds an indexed field to the txn table, and all of the relevant code in the userpoint module to deal with it. So, it implements http://drupal.org/node/142834 completely. Since this patch also fixes that issue, here is the text for that issue:

---------------------------------------------------------------------------
At the moment, the trail table userpoint_txn doesn't have any searchable field a part from the txn_id.
I am a module developer myself, and need such a field.
Find attached two patches to:

* Change the .install file, so that the database has a new field called "reference" which is 128 characters long and it's indexable

* Change userpoints.module so that it allows people to define a "reference".

Note that the API is compatible, since $reference is added right at the end in the API function:

function userpoints_userpointsapi($op, $points = 0, $uid = 0, $event = NULL, $description = NULL, $reference=NULL){
-----------------------------------------------------------------------

The patched module is working right now in Free Software Magazine. However, I was hoping you could check this patch and make sure everything is OK.

Thanks,

Merc.

Hi,

Any news about this patch?

Merc.

Hi,

Before putting my hands on the code (which BTW works great, as it's being used on Free Software Magazine), I would like to see what the module maintainer thinks.

In general, I thing the audit trail is _always_ needed. The "txn approve" option is only there to give the module a clear way of bypassing any moderation/trail, in order to actually award points that were in the moderation queue.

So, the real three operations here are:

'points' - moderation depends on the configuration. TRAILED.
'points approved' - never moderated. TRAILED

These are the only options any module should ever use, really. The fourth option you are talking about would be "forcing moderation", which I don't think is something the module should really allow through its API (the queue would build up even though the admin DOESN'T want an "approval queue"...)

Just my 2c

Bye,

Merc.

Hi,

I *think* it would make a lot of sense not to change the API, but simply add a configuration option to the module itself so that the trail can be globally switched on or off. So, modules themselves don't have to specify whether they want the trail or not - the user does, in a site-wide fashion.

MInd you, I am only doing my best to minimize the possible confusion, and I think a general option would e good because 1) It won't break the API, nor require major modifications 2) It won't add possible confusion/complication 3) It would be easy to add it to my patch

Waiting for kbahey's opinion!

Merc.

Hi,

It is almost midnight here, and hence brain on autopilot.

OK.

I want the audit trail to always be on, and not being bypassed. I see some value in what dami said about making everything configurable, but having the trail on allows for things like recalculating ...etc.

I agree with that.

Also, for Tony, it is better to split patches that are functionally different, unless they are trivial one liners, or cosmetic.

OK. However, the two patches are inter-dependent, and modify the same section of the module. Therefore, I think they need to live as one patch.

By the way, several features/fixes went in, we are at 2.9 now. Try to sync with that and roll another patch.

The patch is substantial.
Rolling another patch basically means:

* Running a diff with the module I modified
* Getting the new version of the module
* Figuring out what should be changed, line by line
* Debugging it all over again

I *can* find the time to do this (although it means taking it from my own family), although I am very slow because I am renown to be crap at merging/etc..

I really think that it would be easier for you to apply the patch to the module's HEAD, since it would mean only doing it once, and since you know the module better than anybody else.

Did you check the contrib modules?

userpoints_cap
userpoints_nodelimit
userpoints_retroactive
referral_points
userpoints_ecommerce
userpoints_reserve
userpoints_transaction_tools
userpoints_autoapprove
userpoints_invite
userpoints_reset

Yes, I have installed quite a few of them

One of them is userpoints_autoapprove. It provides automatic approval of points after some period (requires cron). Not the most elegant way (manipulates the database), but shows an alternate way of doing things. Maybe it is the one needing refactoring after this patch goes in?

I honestly don't know. I don't *think* so, since my patch doesn't actually change anything at all in the API. However, I have never installed this particular module.

Rename "points approved" to "points immediate". Too long, but more descriptive?

I frankly prefer "points approved", because it tells us what it does: it awards points, and they are approved. "immediate" doesn't work as well for my brain, but it could just be me.

Merc.

Hi,

In my version of CVS, the right command was:

cvs diff -upR userpoints > upc.patch

(Capital R)

I am not sure why the patch failed. However, I have checked out the module's HEAD, and re-applied the changes manually.
I also retested it, and it works OK here.

I really hope this is OK. Feel free to apply it, and change whatever you like in it. If you change the interface, please let me know so that I can upgrade the karma module.

I looked at the autoapprove module. Actually, it should work as it is. When $op is "txn approve", userpointsapi only needs the uid. So, all the parameters passed are actually useless.

ONE thing my patch doesn't deal with is the hooks.
I suspect this:

$rc = module_invoke_all('userpoints', 'points before', $points, $uid, $event);

Should be:

$rc = module_invoke_all('userpoints', 'points before', $points, $uid, $event, $reference);

However, I have never dealt with hooks before, and while I think it should be fine, I leave it up to you.

Bye,

Merc.

Hi,

I don't mean to be pushy or anything. However, HEAD - I assume - changes all the time, and this patch is now 3 days old. I suspect that it enough time passes, it will need to re-ported to the current HEAD.

Any news on this one?

Merc.

OK.
I will be very happy to apply the patch a third time to DRUPAL-5. I know it by heart.
However, I noticed that you made some dangerous changes to the database structure. You changed the order of the fields, and yet I can see this is userpoints.module:

db_query("INSERT INTO {userpoints_txn}
VALUES (0, %d, 0, %d, %d, '%s', '%s', %d)",
$uid,
$points,
time(),
$event,
$description,
$moderation);

I checked, and this *seems* to be the only spot where the order of the fields actually mattered while dealing with userpoints_txn).
I changed the order of the fields in this patch, which right now fixes this probably newly introduced bug (I don't think the module works fine once this patch is applied).

However, please note that yyoou will have to change the patch if you decide to set the order of thefields back to the way they were originally.

Another issue you might want to check is that these queries in your update_4 functions:

$ret[] = update_sql("ALTER TABLE {userpoints_txn} CHANGE event VARCHAR(32)");
$ret[] = update_sql("ALTER TABLE {userpoints_txn} CHANGE status BEFORE event");

Don't actually work in MySql 5.x.

The first one should be:

ALTER TABLE master_userpoints_txn change column event event VARCHAR(32);

The second one should be:

ALTER TABLE master_userpoints_txn CHANGE status status int(1) AFTER time_stamp;

I have NOT changed your .install file just in case you have some good reason to have those queries there.

So, I cheerfully present this patch, applied against DRUPAL-5, hoping that it will be accepted.

Thank you.

Merc.

Hi,

I am not entirely sure this patch could be used to create buckets - sorry.

It would indeed be useful, I must say. I talked to the maintainer about this very topic before. This missing feature triggered the development of the Karma module.

I really think this patch's scope is more restricted - extending the API so that it manages multiple buckets is a different kettle of fish.

However, the module's maintainer will definitely know more than I do!

Merc.

My patch is currently used so that I can have a searchable field in the database. I need it because the Karma module needs to be able to "revoke" points.
In order to do this, I also needed to fix a bug in the module (the fact that the trail wasn't kept unless moderation). I don't understand why nobody else before complained about it, but well, the point if having an audit trail is that... it's always there, moderation or not!

While I was there, I made the core part of userpoints.module (in my humble opinion) a lot clearer. I have a low IQ, and now even I can actually read through the code and figure out exactly what is going on.

I will show you what I do with an example:

+--------+-------+--------------+--------+------------+--------+-----------------+--------------+-------------------------------------+
| txn_id | uid | approver_uid | points | time_stamp | status | event | description | reference |
+--------+-------+--------------+--------+------------+--------+-----------------+--------------+-------------------------------------+
| 1 | 12981 | 1 | 1 | 1176415572 | 0 | post comment | | NULL |
| 2 | 4919 | 1 | 1 | 1176416087 | 0 | post comment | | NULL |
| 3 | 9598 | 1 | 1 | 1176417680 | 0 | post comment | | NULL |
| 4 | 25804 | 1 | 1 | 1176422383 | 0 | post comment | | NULL |
| 5 | 32234 | 1 | 1 | 1176428628 | 0 | post comment | | NULL |
| 6 | 5 | 1 | -400 | 1176431684 | 0 | admin | Just testing | NULL |
| 7 | 5 | 1 | 150 | 1176434970 | 0 | admin | Testing | NULL |
| 8 | 5 | 1 | -300 | 1176434991 | 0 | admin | | NULL |
| 9 | 5 | 1 | -150 | 1176435019 | 0 | admin | | NULL |
| 10 | 39453 | 1 | 2 | 1176440589 | 0 | invite invite | | NULL |
| 329 | 31677 | 1 | 1 | 1178803163 | 0 | post comment | | |
| 328 | 8833 | 1 | 1 | 1178801888 | 0 | post comment | | |
| 342 | 5541 | 0 | 12 | 1178858019 | 0 | karma | | karma; oid: 60992, otype: c, uid: 1 |
| 343 | 5541 | 0 | -12 | 1178858033 | 0 | karma | | karma; oid: 60992, otype: c, uid: 1 |
| 344 | 5541 | 0 | 28 | 1178858033 | 0 | karma | | karma; oid: 60992, otype: c, uid: 1 |
| 345 | 5541 | 0 | -28 | 1178858038 | 0 | karma | | karma; oid: 60992, otype: c, uid: 1 |
| 346 | 5541 | 0 | 4 | 1178858038 | 0 | karma | | karma; oid: 60992, otype: c, uid: 1 |

In this case, a user was first given 12 karma points. Then, a little while later, for the SAME comment he was given 28 karma points (the person voting changed his/her mind). So, the original 12 points were taken away, and 28 points where assigned instead. THEN, the person voting changed his/her mind again, and only gave the comment 4 points. As a result, the 28 points were taken, and 4 points were awarded instead.

I could have done this probably more easily if "description" had been searchable (which in my opinion should be, but the module's maintainer has a different opinion). So yes, you could effectively take my patch, change it a bit so that it actually creates buckets, make description searchable, and then we are all happy. I am fine as long as:

* The trail is always kept
* I have a searchable field

I mean, there is the issue of me having to change my existing database, etc., but that's OK.

I submitted my patch. You know what I did it for, and what bug I need fixed. I think it would be a shame not to make the core of userpoints more readable. Other than that... I recommend you talk to the module's maintainer and see what he thinks. I will be happy to review your patch to make sure it still fits my purposes and fixes the important trail (or lack thereof) bug.

Bye,

Merc.

Hi,

OK for all.
Now what we really do is wait for Khalid Baheyeldin's approval/comments. I (gladly) re-applied the patch 3 times. I am getting a little concerned that the DRUPAL-5 version of the module might get old, since it's already been a few days.

Khalid?

Merc.

Hi,

My patch effectively solves a current bug which affects userpoints.
Although the implementation is somewhat similar, I don't think it's a good idea to wait for the next major version of userpoint to fix it.
My patch is there, is ready, and it's working right now on a major production site (http://www.freesoftwaremagazine.com). The lack of an official version of userpoints which corrects the bug *now* is stopping people from using the karma module (I had to disable the userpoints interface, because this patch took a lot longer than expected).

I _really_ think the sanest thing to do is to roll this patch out, since it fixes a critical bug (lack of trail) and continue the discussion about the new API/etc. as well.

Bye,

Merc.

Hi,

Thanks mate.
I will audit the code by hand, and test it on Free Software Magazine. Please keep your fingers crossed for me! :-D

It it works on FSM, it works full stop - FSM uses pretty much all of the API, and it has a _lot_ of traffic.

Bye,

Merc.

Hi,

Alright, I can confirm that the new userpoints works.
Unfortunately, I didn't manage to test the upgrading process.
My production server was using a "fork" of userpoints, my development machine had a data structure that was a little messed up...

In the end, I had to ALTER TABLE my production server, and actuallytest it on the live data. Luckily, it's nighttime in the US, so nobody boted on anything while I was working on it.

My apologies for not being able to test... however, well, it works on a production server with tons of userpoints etc.

I will let you know tomorrow, once the site's run for 24 hours, if everything is 10000000000% OK.

Thank you!

Merc,