Fix edit and max points bugs and improve tests

Subscribing and will test! :-)

Berdir,

Wow... what a great catch and what a great patch. The work you did seems rock solid. I tried to trick the system with changes from positive to negative points, category changes, and status changes (approved, pending, etc.)... sometimes all at the same time. And I couldn't fool the system. It came out right every time.

I also tested out the max_points implementation by using your suggestion of debug(userpoints_get_max_points($uid, $category_id)); in the devel php window. I added and subtracted lots of points amounts. From what I can tell, max_points is working terrifically now. I wasn't quite sure how to test max_points for all categories (since "General" isn't actually a category and using "0" doesn't seem to to work). But if you let me know how, I can test that, too.

So the only "bug" I found was something you already knew: Problems with point totals involving expired transactions. More specifically, I took a points transaction that was declined and expired... and I changed it to approved (it was still expired). I found that the user was awarded points, but he shouldn't have been because the approved points were already expired.

So you want to tackle expiration in this patch or postpone to a follow up patch? If we're not going to do expiration now, then this patch is a huge improvement and RTBC. Really great work on this!

--Ben

I just tested the actual 7.x-1.x branch and everything is working great... except for one thing that could be addressed in a follow up patch.

It appears there is something wrong with the max_points logic for 'all' categories. To test, I had 1,098 points in all categories before I began my testing. Initially, the max_points were at 1,100. So I added 5,000 points to set a new max_points in all categories. But instead of showing 6,098 as my max points (which was also the total that showed on the transactions page), I showed 6,150 as my max_points.

So somehow max_points is overstating the actual max_points when computing it for all categories. (Max_points for individual categories are working great.)

--Ben

P.S. To test max_points, I've now been executing the following commands from the devel module PHP execution window:

General category max_points (for user #2): debug(userpoints_get_max_points(2, 0)); or debug(userpoints_get_max_points(2));

Taxonomy term 2 max_points (for user #2): debug(userpoints_get_max_points(2, 2));

All categories max_points (for user #2): debug(userpoints_get_max_points(2, 'all'));

These commands seem to work fine. Thanks for the explanation.

Also, did you already solve the expiration bug in the committed patch? From what I've just tested, it seems like the bug related to "getting points for changes to expired transactions" has already been fixed. :-)

--Ben

Hey Berdir,

I've finally isolated why the max_points seems wrong when computing it for all categories. Your suggestion of looking at the data in phpmyadmin was very helpful.

Basically, I think I might disagree with the logic of how max_points is calculated for all categories (it is fine for individual categories). Here's the problem: max_points for all categories appears to be a simple sum of the max_points for each individual category. But because the max_points for each individual category did not occur at the same moment in time, it tends to overstate how many points a user actually had at any one time.

So here's why this was confusing. I was adding 5,000 points to a user (an absurdly large number of points just to make sure it was the highest point total they ever had) and then was expecting the current "all" categories point total to equal the max_points for all categories (since I had just added enough points to make it their highest point total ever). But the current point total was always smaller than the max_points. This is because the user had slightly higher totals previously in the other categories (even though they never had anywhere this number of points before).

So I thought max_points for all categories would equal the user's highest lifetime point total. But this isn't actually the case.

So the problem is how do we fix this? We want to know the highest point total (across all categories) that the user has ever had. Is there any way to compute this? Or is the only way to know this to actually save a new "highest lifetime points" field for each user?

Don't want to create more work for us than is really needed...

Hope this makes sense,
Ben

I also determined that the expiration issue I previously noticed still occurs. But it also stems from a mistaken understanding of the system's current logic. The question is whether we should change the logic.

Basically, when adding an expiration date in the past to a points transaction, I expected the user to immediately lose those points because the expiration date has already happened. But the system is only designed to consider that expiration date once cron had been run. (I wasn't running cron on this -dev server.) Once I manually hit the cron button, then a new expiration transaction occurred and the points were deducted.

So the question is whether we want to continue to do these "expiry" transactions on cron or if we instead want to create an "expired" status on the original transaction. If we do the second option, then we would also need a way for setting the expiration date to immediately trigger a status change (and thus a reduction in points) if the expiration date occurred in the past.

Not quite sure what to do with this... thoughts?

--Ben

I loaded userpoints 7.x-1.0-beta1.
I get the following notice info.

Notice: Undefined index: points in userpoints_userpointsapi() (line 551 of /path/sites/all/modules/userpoints/userpoints.module).

I am not able to load dev version and any patches. I get service from Web Sevice Provider, so they do not allow me to run drush.
Drupal update does not allow to upgrade to dev version.
Could you release beta2.
Thank you.