#2982 - Bug in gift points used calculations

By PDStig
Added 13th Dec 2016, 6:29 PM
3 views

Identifier	#2982
Issue type	Minor issue (breaks specific functionality)
Title	Bug in gift points used calculations
Status	Closed (no changes needed)
Handling member	Chris Graham
Addon	points
Description	In line 208 of points.php within the get_gift_points_used function: return ($claimed_used < 0) ? $claimed_used : $actual_used; // Still allows $claimed_used to be fiddled to negative give members extra gift points The problem with this line is that if I had set a negative amount of gift points used, then the calculations completely ignore everything in the 'gifts' database table and instead base off of solely the value of that CPF. For example, a user has used 1,070 gift points according to the gifts database calculations. Their "gift points used" field is 0. I set the field to "-1000" to attempt to reward them 1,000 gift points. But because of the calculations neglecting the gifts table in this instance, technically the member is being awarded back all gift points they used in addition to 1,000 more gift points because of the "-1000" setting. In other words, they really earned (1,070 + 1,000 =) 2,070 gift points instead of 1,000 from that edit.
Steps to reproduce
Funded?	No

The system will post a comment when this issue is modified (e.g., status changes). To be notified of this, click "Enable comment notifications".

Rating

Unrated

Comments

By Guest posted 13th Dec 2016, 6:34 PM

To be very honest, I think gift points used should not be dynamically calculated and should be calculated similar to how points available is done instead. If, say, I give a user 30 days of donor usergroup membership and the donor user earns that member an additional 5 gift points per day, we run into a problem when the member's usergroup subscription expires; I assume they lose all those extra gift points.

By Guest posted 14th Dec 2016, 8:57 AM

The field is not designed as some kind of increment/decrement on top of another calculation, it was designed to actually hold the absolute value of the points used. So if you say they've used "-1000" then that's it, end of story.

We no longer rely on it for positive values for the reason you said, it is better to dynamically calculate it.

And setting a negative value in there has very questionable use, it's basically a hack in the code that users would never normally find, for a very rare use case.

There'd be an argument for removing the field entirely, but I suppose it's just there for consistency, as the various other fields exist and some of them are strictly needed while others help with performance.