2.x version added a new column 'max_points' to {userpoints} table, it breaks the current retroactive.module. It's sort of a easy fix, just adding it in the corresponding INSERT sql. But before submitting a patch, I'd like to get a few questions cleared/addresses:
1. What's the use of this 'max_points' field? I looked at the code to calculate but couldn't understand it. Can somebody help to explain?
2. When doing a retroactive through new userpoints_userpointsapi(), is it ok to use op='txn approval' instead of 'points' to bypass the moderation? This also leads to question 3
3. Does anyone really need moderation for points when a node/comment is posted or deleted? This is how the current 2.x version works. But it looks to me in most if not all cases, these points are better off handled automatically, admins just needs to moderate admin-manually-designated points. Otherwise, it's really a tedious work to moderate every node and comment creation.
4. If my argument above makes sense, in txn_table we probaly just need to store admin-designated points not points can be auto caculated from user posts.
Once these questions are answered, I will submit patches accordingly.
Thanks.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | userpoints_retroactive_0.patch | 1.98 KB | jenlampton |
Comments
Comment #1
kbahey commentedIt is for future use, for example, if someone redeems his current points for a gift, ...etc., we want to still know what was the maximum he got to before he redeemed them.
I don't think so. But go ahead and try it and see what happens to be sure.
Moderation was added to prevent abuse. There were cases where users would sit and comment/vote ..etc. to exceed a certain number.
You can turn moderation off completely, and that is the default.
If you want to make moderation more granular, i.e. for selected operations, then go ahead and submit a patch. However, remember that the event is free text, and not predefined, so you may not catch all cases.
No, we should store everything. The whole point is to be able to audit everything that happens. If you want to fast track some types of transactions (i.e. skip the moderation steps for them selectively), then go ahead and submit a patch. It has to be backwards compatible though.
Comment #2
jredding commentedI also don't understand the use of the max_points column. Couldn't the max points be calculated through the transactions? If someone redeems points its a transaction and similar to looking at a bank statement you can calculate the points up to the point of redemption.
I agree with kbahey in that ALL items should go into the transaction log. userpoints table should be a SUM of all transactions for auditing reasons. The userpoints table simply acts as a caching table.
The only time I can see killing the transaction table is for performance, but you'd have to be on a hefty site and be using points in such as a way as tension will never flair.
Comment #3
jenlamptonI was frustrated by this too, here is the fix I used.
My patch also includes an automatic 'reset' so that you don't have to run reset before retroactive anymore.
Jen
Comment #4
jredding commentedPatch in its current state is incorrect. It only takes into account the userpoints table and ignores the userpoints_txn table. If the is attempting to "reset" the userpoints it should ideally either
(A) use the API to insert a negating entry into userpoints_txn thus wiping out all points
or
(b) Truncate BOTH userpoints_txn table and userpoints.
This patch was not incorporated into v3 although I agree that both the retroactive module and the reset module could use some serious work.
Comment #5
jredding commentedBack to the original bug report. Max points is used to demonstrate what the highest points gained were. Although its correct that this could be calculated by a sum SQL statement, this wouldn't work IF older transaction had been deleted as might be the case on high traffic websites.
regardless, max point has been fixed and the API completely rearranged. in v3 the retroactive module was modified to NOT do a direct INSERT statement instead relying on the userpoints_userpointsapi() to do the work. All modules have been modified to do this.
NO MODULES should do direct INSERT or UPDATE statements to the tables instead they should us userpoints_userpointsapi to do the work this will eliminate bugs, such as this one, in the future.
Also.. moderation was modified in v3 which should address your concerns.
Comment #6
jredding commented