Wrong calculation of userpoints in case of points updates
scoorch - August 27, 2009 - 11:18
| Project: | User Points |
| Version: | 6.x-1.x-dev |
| Component: | Code: userpoints API |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Jump to:
Description
I use the userpoints_userpointsapi to UPDATE entries in the userpoint table.
I know, it is recommended not to update existing entries but rather to create new ones to have a better audit track, but in my case UPDATING is necessary (A user gets points based on averagefivestar votings made on his/her content; when the average fivestar voting changes because of new votes the userpoints have to be updated)
When updating a userpoint entry the function _userpoints_update_cache() (called by _userpoints_transaction()) does not delivery accurate results, because it always adds the points stored in params[points]. This if fine when a new entry to the userpoint table was made but it causes wrong results if an entry is updated:
Example:
userpoints for given user was 20 and is changed to 5. If we just add the 5 then we have 25. We have to substract the old value before!!!
I have a dirty bugfix, which seems to work for me. Inserted lines are commentes with "bugfix" so you just need to search the words "bugfix" to find my slight changes.
<?php
/*
* Adds the points to the txn table
* PRIVATE FUNCTION use userpoints_userpointsapi!
*/
function _userpoints_transaction(&$params) {
//Check, again, for a properly formed array
if (!is_array($params)) {
return false;
}
if (!isset($params['txn_id'])) {
//If a txn_id is preset we UPDATE the record instead of adding one
//the standard checks don't apply
if (!is_numeric($params['points'])) {
return false;
}
if (!isset($params['uid'])) {
global $user;
$params['uid'] = $user->uid;
//there must be a UID, anonymous does not receive points
if (!$params['uid'] > 0) {
return false;
}
}
if (!isset($params['moderate'])) {
//if not passed then site default is used
$params['status'] = variable_get(USERPOINTS_POINTS_MODERATION, 0);
}
else {
if ($params['moderate'] == true) {
$params['status'] = 1;
}
else {
$params['status'] = 0;
}
}
if (isset($params['expirydate']) && !is_numeric($params['expirydate'])) {
return false;
}
// check if parameters are set
$params_null_check = array('operation', 'description', 'reference', 'expired', 'parent_txn_id', 'entity_id', 'entity_type');
foreach($params_null_check as $param_null_check) {
if (!isset($params[$param_null_check])) {
$params[$param_null_check] = NULL;
}
}
if (!isset($params['tid']) || !is_numeric($params['tid'])) {
$params['tid'] = variable_get(USERPOINTS_CATEGORY_DEFAULT_TID, NULL);
}
elseif ($params['tid'] == 0) {
//tid with 0 are uncategorized and are set to NULL
//this is a backwards compatibilty issue
$params['tid'] = NULL;
}
if (!isset($params['expirydate'])) {
$params['expirydate'] = userpoints_get_default_expiry_date();
}
} // if txn_id
if (is_numeric($params['txn_id'])) {
//A transaction ID was passed in so we'll update the transaction
$result = db_query("SELECT txn_id, uid, approver_uid, points,
time_stamp, status, operation, description, reference, expirydate, expired,
parent_txn_id, tid, entity_id, entity_type
FROM {userpoints_txn}
WHERE txn_id = %d",
$params['txn_id']);
$txn = db_fetch_array($result);
foreach ($txn as $key => $value) {
if (isset($params[$key])) {
$arr[] = $key .' = \''. $params[$key] .'\'';
}
else {
$params[$key] = $value;
}
}
$substract = (int)db_result(db_query("SELECT points FROM {userpoints_txn} WHERE txn_id = %d", $params['txn_id'])); # bugfix: remember the old points
db_query('UPDATE {userpoints_txn} SET '. implode(', ', $arr) .'
WHERE txn_id = %d',
$params['txn_id']
);
_userpoints_update_cache($params, $substract); # bugfix: pass old points
}
else {
$ret = db_query("INSERT INTO {userpoints_txn}
(uid, points, time_stamp, status, operation, description,
reference, expirydate, expired, parent_txn_id, tid, entity_id, entity_type)
VALUES (%d, %d, %d, %d, '%s', '%s', '%s', %d, %d, %d, %d, %d, '%s')",
$params['uid'],
$params['points'],
time(),
$params['status'],
$params['operation'],
$params['description'],
$params['reference'],
$params['expirydate'],
$params['expired'],
$params['parent_txn_id'],
$params['tid'],
$params['entity_id'],
$params['entity_type']
);
if ($params['status'] != true && $ret != false) {
_userpoints_update_cache($params);
}
}
return TRUE;
} //function userpoints_transaction
/*
* Update the caching table
*/
function _userpoints_update_cache(&$params, $substract = 0) { # bugfix, add second param $substract
if ( $params['status'] != 0 || $params['expired'] == 1) {
//Only update the cache for fully approved non-expired points
return false;
}
if (!isset($params['tid'])) {
$params['tid'] = NULL;
}
// Calculate the current points based upon the tid
$current_points = (int)$params['points'] - $substract + userpoints_get_current_points($params['uid'], $params['tid']); # bugfix: insert "- $substract"
//Grab the user's maximum points to preserve it
$max_points = db_result(db_query('SELECT max_points FROM {userpoints} WHERE uid = %d AND tid = %d',
$params['uid'], $params['tid']));
if ($params['points'] > 0) {
//points are greater than zero, update their max_points
$max_points = (int)$params['points'] + (int)$max_points;
}
// insert or update the userpoints caching table with the user's current points
if (_userpoints_user_exists($params['uid'], $params['tid'])) {
db_query("UPDATE {userpoints}
SET points = %d, max_points = %d, last_update = %d
WHERE uid = %d AND tid = %d",
$current_points,
$max_points,
time(),
$params['uid'],
$params['tid']
);
}
else {
$result = db_query("INSERT INTO {userpoints}
(uid, points, max_points, last_update, tid)
VALUES (%d, %d, %d, %d, %d )",
$params['uid'],
$current_points,
$max_points,
time(),
$params['tid']
);
}
unset($params);
}
?>
#1
Can anyone please confirm if this is both working and an appropriate fix?
#2
#3
subscribing