Hi, this is my first post on drupal.org, please redirect me if I am in the wrong area.
We're using Ubercart Userpoints, and I have found a slight problem with the way points are deducted from the user after making a payment.
The problem is that Userpoints can be set to expire, and Ubercart Userpoints' payment function does not respect this feature of userpoints.
Say I give users of the site 100 points to spend in a week. A week later, that 100 points is subtracted from their total. What makes sense to me is this: If in that time a user spends for instance 60 points in the store, upon expiry only 40 points should be subtracted. This does not happen. Instead, the full 100 points are subtracted, regardless of any points payments the user makes.
After some investigation I narrowed the problem down to Userpoints Ubercart; the payment function simply enters a transaction equivalent to the negative of the total order points value with no respect to any awarded points which may be set to expire.
I've made a fix for this that incrementally consumes points that are set to expire before subtracting the remainder from the points total. This means that when points do expire, people don't end up losing non-expiring points they may have been awarded in other ways (such as making blog posts etc).
This fix may not be perfect, and it may be that perhaps Userpoints itself needs some kind of "incremental_points_subtract" function so modules such as this one can more easily and safely deduct user points - however I think it's important that this issue is raised and if I am mistaken, at least I can be corrected :)
Here's the code I came up with to fix the problem in the uc_ubercart_payments.module file if it's of any use:
function uc_userpoints_payment_payment($order) {
global $user;
$points = ceil($order->order_total * variable_get(USERPOINTS_UC_PAYMENT, 1));
$total_points = $points;
$description = t('Order #!order_id paid with !points', array_merge(uc_userpoints_translation(), array('!order_id' => $order->order_id)));
if ($order === FALSE || uc_order_status_data($order->order_status, 'state') != 'in_checkout') {
print t('An error has occurred during payment. Please contact us to ensure your order has submitted.');
exit();
}
if($order->payment_method == 'points'){
//Algorithm to apply to oldest unexpired points (with expiry dates)
//Store remaining cost to subtract
//We can just use $points
//Fetch all unexpired transactions with expiry dates
$query = "SELECT *
FROM
users
INNER JOIN userpoints_txn
ON (users.uid = userpoints_txn.uid)
WHERE expired = 0
AND NOT expirydate = 0
AND users.uid = '%s';";
$db_result = db_query($query,$order->uid);
//Are there any results?
while ($row = db_fetch_array($db_result)){
//Yes, use a semaphore
$continue = true;
//Define variables
$new_points = 0;
$expired = 0;
//Get current row points
$this_points = $row['points'];
//Make sure we update the right transaction
$txn_id = $row['txn_id'];
//Is the points value of the row greater than or equal to the remaining cost to subtract?
if ($this_points >= $points){
//Subtract remaining points from that row
$new_points = $this_points - $points;
//No more points left to subtract
$points = 0;
//If the row is now equal to zero set it to expired
$expired = 0;
if ($new_points == 0) $expired = 1;
//We're finished.
$continue = false;
} else {
//Is the points value of the row less than the remaining cost?
//Set row to expired
$expired = 1;
//Subtract row points from remaining cost to subtract
$points = $points - $this_points;
//Zero the row points
$new_points = 0;
//Continue to next row
}
//Update row
$params = array (
'txn_id' => $txn_id,
'expired' => $expired,
'points' => $new_points,
'operation' => 'purchase',
'description' => $description,
'entity_id' => $order->order_id,
'entity_type' => 'uc_order',
'moderate' => variable_get(USERPOINTS_PAY_MODERATE, 0),
'display' => variable_get(USERPOINTS_PAY_DISPLAY, 1),
);
userpoints_userpointsapi($params);
//Do we continue?
if ($continue == false) break;
}
//Are there remaining points to subtract and there are no unexpired transactions with expiry dates?
if ($points > 0)
{
//Add new transaction with negative remaining points value
$params = array (
'tid' => variable_get(USERPOINTS_PAY_CATEGORY, NULL),
'uid' => $order->uid,
'points' => -$points,
'operation' => 'purchase',
'description' => $description,
'entity_id' => $order->order_id,
'entity_type' => 'uc_order',
'moderate' => variable_get(USERPOINTS_PAY_MODERATE, 0),
'display' => variable_get(USERPOINTS_PAY_DISPLAY, 1),
);
}
//Enter the new order in Ubercart
uc_payment_enter($order->order_id, 'points', $order->order_total, $user->uid, NULL, $description);
db_query("insert into {uc_up_payment_log} (uid, oid, points) values(%d, %d, %d)", $order->uid, $order->order_id, $total_points);
//Change the user's points tally
db_query("UPDATE userpoints SET points = points - %d WHERE uid = '%s'",$total_points,$order->uid);
}
}
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | userpoints-expiring_unspent_points_only-885174-6.patch | 6.69 KB | attisan |
Comments
Comment #1
dhmalex commentedMy first patch... Hope it works...
Comment #2
bmagistro commentedThanks for taking the time to put this together. I am hesitant to apply something like this to the module simply because I think this means I am circumventing a portion of the API they (userpoints) provide. The api is the interface I use to add/subtract points. The way I view it is they provide an API for me to see how many points a user has (userpoints_get_current_points) and an API for me to add/subtract points (userpoints_userpointsapi). I would like to think the API knows how to enter points so that when it tells me there are 100 points available, I make a transaction that uses 60 of them, as you stated only 40 are left to be expired. They provide the cron job (or whatever method) to subtract expired points, which would make me lean towards a bug on their side. I do agree this should be addressed though.
Comment #3
bmagistro commentedClosing issue, Won't fix as I believe this should be handled via the userpoints API interface.
Comment #4
mr.j commentedI just discovered the same bug, so I am assuming it was never fixed anywhere. I will re-assign it to User Points as per bmagistro in #2.
Quick summary:
When a user consumes User Points that may be expired - eg. buying something in an Ubercart shop, or any other way you can consume them - those points needed to be eliminated from possibly being expired in the future as they have been used. Unfortunately userpoints_expire_transactions() just expires every transaction that has an expiry date that is overdue.
You could do this a few ways - update the original transactions and mark them expired without creating the reversing transaction that the expiry process normally creates (as in patch above). Or you could update them and set expirydate to 0 so they never expire. I guess a FIFO basis would be best so that the oldest non-expired transactions are the ones updated (patch above does not take order into account).
A problem I see is handling refunds or positive consumption transactions. As Ubercart_userpoints creates a single negative transaction for the purchase price but you need to alter the original transactions too, how do you keep track of which transactions were updated in case you need to reverse them for a refund and allow them to be expired again in the future?
Comment #5
Wtower commentedI came across this accidentally. I had exactly the same issue with a slightly different case so I decided to share my findings in case it helps someone.
This particular site runs on Drupal 7 and Drupal Commerce, so I don't know how applicable this is to the particular issue with D6 and Ubercart. My solution is based on Rules and Views Rules.
The idea is based on mr.j's comment, to let userpoints handle expiration as it would. Then collect all negative userpoints and create a special transaction that will add the exact consolidated number of userpoints that have been used after the userpoints where obtained, up to the expiry date (FIFO). If the algorithm comes across another pre-existing special transaction (I specify userpoints operation field as 'special') like this one, it will reset the number of userpoints so that they don't get added twice. This way we deal with cases where there are more than one overlapping transactions which add points and expire.
I have created a view A which displays all userpoint transactions, not expired, not declined, with timestamp before now.
A second view display B with the same fields and different filters (I have used: not declined, op not admin), as well as with contextual filters that provide uid and tid of userpoints category.
Then I create a rule which loops through view A, obtains start date, expiry date, uid and tid. In it, I add a variable with 0, then loop through view B.
In the second loop, I call two components where I provide the start date, end date, and tx details. The first component checks if the tx date is within start date and exp date, and if the op is 'special' and it returns 0. The second component checks the dates and if the op is not special then returns the previous value + points. In the end I create a rule action that grants this variable to the user.
Comment #6
attisanwith this applied only the unspent points will be expired. e.g.
add 200 points
spend 50 points
auto-expire 150 points (as 50 points have been spent)
the patch "should" work for positive / negative expiries (though not tested) and "should" handel complex credit / debit situations.
hope this helps someone ;-)
attisan
Comment #7
manomintis commentedPatch "userpoints-expiring_unspent_points_only-885174-6.patch" worked for me with module stable version 7.x-1.1. Why not to commit it?
Comment #8
Bhuvana_Indukuri commentedThanks Attisan for the patch. It works in most scenarios except when there are multiple active transactions with positive points.
Following scenario fails:
Txn Id Description Points
1 Added 10 Points(Credit 1) 10
2 Point used -1
3 Point used -1
4 Added 5 points(Credit 2) 5
5 Point used -1
6 Point used -1
7 Reminder points for txn 1 expired -6
8 Point used -1
9 Point used -1
If the points added in Txn 4 expire after Txn 9, balance 3 points should get expired and new transaction with id 10 should get added with -3 points. However the balance 3 points are not getting expired but the expired flag for Txn Id 4 is being set to 1. The balance remains at 3 while the transaction is expired.
The issue was because Txn 7 was also being considered as consumption for Txn 5 which is incorrect. The issue got fixed when I added the additional condition(->condition('expirydate', 0, '!=')) in the query to extract consumptions in the method userpoints_expire_transactions. I am not sure if that is the right fix though. So please take a look.
Comment #9
socialnicheguru commented@Bhuvana_Indukuri could you reroll the pach with your changes so that others might be able to test?
Comment #10
shi99 commentedI used the patch in #6 and it works well.
I haven't been able to raise the edge case mentioned in #8. But for now #6 works well.
Thanks,
Comment #11
Anonymous (not verified) commentedJust wanted to mention that we've been using this patch for the past year and a half to handle points expiring and spending from the oldest points first, and it has been working without issue.
Comment #12
roxart commentedis this working now in version 7.x - 1.1?
Im thinking of activating expiration on my site but I want to know if this works as intended also with a lot of transactions or not (people get credits on events and can spend them in my store)