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.

For instance, 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 assumed max_points for all categories would equal the user's highest lifetime point total. But this isn't actually the case.

As Berdir pointed out in IRC, to fix this we may need to store the total max_points. Berdir mentioned that maybe we should do this for the normal points too for performance reasons (and views integrations reasons).

--Ben

CommentFileSizeAuthor
#8 0001-Issue-1070068-by-Berdir-Added-userpoints_total-to-tr.patch19.69 KBberdir
#1 1070068.patch17.09 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Component: Code: userpoints API » Code: userpoints
Category: feature » task
Status: Active » Needs review
StatusFileSize
new 1070068.patch17.09 KB

The attached patch adds a new userpoints_total table, which contains the total points and max points for each users. This patch also contains some test updates, update functions and changes the API functions which return the total/all points. I also added views integration for the new table. This makes it very easy to create a view of the total points

The downside is that there is an additional query every time a new transaction is added.

Please verify that both the update path is correct (correct as in it shows the same values as before), the view works (maybe we want to improve some of the labels there, as there are now 3 tables that you can query against and it might not be clear for what each of them is). Also, obviously, make sure that the max_points logic now works correct.

BenK’s picture

BenK commented

Hey Berdir,

I'm currently testing this, but I can't quite remember how to do the query for max_points... can you remind me?

--Ben

BenK’s picture

BenK commented

Wait, I think I remember....

debug(userpoints_get_max_points($uid, $category_id));

... and execute this from the Devel's PHP block.

--Ben

BenK’s picture

BenK commented

And just making a note that I should use "0" for the general category and "all" for all categories combined.

BenK’s picture

BenK commented
Status: Needs review » Needs work

Hey Berdir,

I've tested your patch and everything is working very well! :-)

I can confirm that the new logic is working (max points calculations are now correct), views integration is working, and the update function is working (showing the same userpoints values as before).

Here is my summary of what's left to do:

a) When you add points using the "Add points transaction" local task, there's a debug message that still displays. I'm assuming you knew that (it was helpful in my testing), but I thought I'd mention it since we need to take it out.

b) Max points are now calculated properly, but if a prior user (prior to your patch) has not exceeded their old max point total, then the old (incorrect) max point total is displayed. I'm assuming there's no way to address this (since we have no way of knowing their correct max points amount), but I just wanted to confirm that all the update function is doing is taking the old, incorrect max_points number and adding it to the new table. All new max_points calculations, of course, are working great.

c) Some suggestions for the views field labels. I'm trying to standardize on the same syntax so it's easier to read. How about this:

* Max points in category
* Current points in category
* Last update in category
* Max total points
* Current total points
* Last update of total points

If you think those are too long, one other option:

* Max points (category)
* Current points (category)
* Last update (category)
* Max points (total)
* Current points (total)
* Last update (total)

What do you think?

d) Now that max_points is working great, I think it would be nice to include in the UI directly. How about adding a setting in the "Categorization" fieldset on the settings page to show the max_points in the totals at /myuserpoints and user/[uid]/points? We could list it underneath "Approved points" and "Pending points". Perhaps we could call it "Lifetime max points" or something like that?

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

a) Removed debug statements.

b) Correct, we can't do anything about that.

c) I like your suggestions, but I'm not sure how to apply them. For example, most fields have a title and a help/description text, I'm not sure what to add where. Also, the tables have help texts too. And the userpoints table also has a category field. The following is a list of all the strings that we currently have for those two tables (without userpoints_txn):

 $data['userpoints']['table']['group'] = t('Userpoints');
      'title' => t('Userpoints'),
      'help' => t('!Points accumulated by users on your site.', userpoints_translation()),
      'title' => t('!Points', userpoints_translation()),
      'help' => t("A User's current !points.", userpoints_translation()), // The help that appears on the UI,
      'title' => t('Category'),
      'help' => t('The categories (terms) of userpoints used'), // The help that appears on the UI,
      'title' => t('Max !points', userpoints_translation()),
      'help' => t("A User's max !points.", userpoints_translation()), // The help that appears on the UI,
      'title' => t('Last update'),
      'help' => t("The last update timestamp for a User's current !points.", userpoints_translation()),
      'title' => t('User'),
      'help' => t('Relate the userpoints table to the user table.'),
          'label' => t('Users'),
  $data['userpoints_total']['table']['group'] = t('Userpoints total');
      'title' => t('Userpoints Total'),
      'help' => t('Total !points over all categories accumulated by users on your site.', userpoints_translation()),
      'title' => t('Total !points', userpoints_translation()),
      'help' => t("A User's current total !points.", userpoints_translation()), // The help that appears on the UI,
      'title' => t('Total max !points', userpoints_translation()),
      'help' => t("A user's total max !points.", userpoints_translation()), // The help that appears on the UI,
      'title' => t('Last update'),
      'help' => t("The last update timestamp for a user's total !points.", userpoints_translation()),
      'title' => t('User'),
      'help' => t('Relate the userpoints total table to the user table.'),
          'label' => t('Users'),

The help key usually belongs to the title above. I'm also perfectly fine with removing the help if it's not necessary. What do you think?

d) Hm, not too sure about adding that right now. I'd like to keep changes as small as possible at this point for 7.x-1.x. But feel free to open a follow-up issue. I also have some ideas on making the whole max/current thing a more generic aggregation storage.

BenK’s picture

BenK commented

Hmmm.... My suggestions were for the 'title' in each case. I think that's the most important because I end up rarely looking at the help/description text.

And I think the help text looks pretty good in most cases. I'd keep it. The one exception is that when were talking about the help text for total max points, I'd use the descriptive phrase "A user's maximum lifetime !points across all categories." For the category-specific help, we could use "A user's maximum lifetime !points in a single category."

Actually, any time we use talk about totals, I'd probably use the help phrase "across all categories" just to be clear. And I'd use "in a single category" as a phrase for the other case. This way, things would be consistent.

One more thing: Maybe change the 'Category' title to '!Points category'

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 0001-Issue-1070068-by-Berdir-Added-userpoints_total-to-tr.patch19.69 KB

Ok, changed.

BenK’s picture

BenK commented
Status: Needs review » Reviewed & tested by the community

Hey Berdir,

I tested the latest patch and everything looks good... so I'm marking this RTBC.

When you commit the patch, I did notice one small typo to correct (which is really my fault). Currently, the points category label reads "!Points category", but it doesn't look like any of the the labels are being branded by using "!Points". So just like we do for all of the other fields, this should probably just be "Points category".

So what's next? ;-)

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Commited.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.