As we discussed, I needed float support. It seems to be working fine.

The patch basically changes every reference to points or max_points from an int to a float, and updates the .install file to alter the database so the point columns are floats instead of ints.

CommentFileSizeAuthor
#137 userpoints.patch25.49 KBDrakeRemory
#135 userpoints.zip77.2 KBDrakeRemory
#134 userpoints.zip77.16 KBDrakeRemory
#132 userpoints.zip77.15 KBDrakeRemory
#131 userpoints.zip77.8 KBDrakeRemory
#122 userpoints.zip77.78 KBDrakeRemory
#117 userpoints.zip75.25 KBDrakeRemory
#101 userpoints.patch22.4 KBMasterChief
#99 float.patch22.27 KBberdir
#97 userpoints.patch21.41 KBMasterChief
#96 userpoints.patch21.87 KBMasterChief
#93 userpoints.patch21.92 KBMasterChief
#90 userpoints.patch21.4 KBMasterChief
#85 userpoints.patch18.7 KBMasterChief
#83 userpoints.patch18.71 KBMasterChief
#79 userpoints.patch18.68 KBMasterChief
#75 userpoints.patch18.64 KBMasterChief
#73 userpoints.patch18.33 KBMasterChief
#72 userpoints.patch18.44 KBMasterChief
#65 userpoints.patch17.66 KBMasterChief
#58 userpoints.patch17.74 KBMasterChief
#56 userpoints.patch17.32 KBMasterChief
#46 userpoints.patch14.83 KBMasterChief
#44 userpoints.patch15.15 KBMasterChief
#42 userpoints.patch4.97 KBMasterChief
#32 masterchief_userpoints-install.patch1.52 KBMasterChief
#32 masterchief_userpoints-module.patch5.48 KBMasterChief
#32 masterchief_userpoints_badges-install.patch507 bytesMasterChief
#32 masterchief_userpoints_badges-module.patch1.23 KBMasterChief
#32 masterchief_userpoints_role-module.patch521 bytesMasterChief
#29 masterchief_userpoints-install.patch1.56 KBMasterChief
#29 masterchief_userpoints-module.patch5.59 KBMasterChief
#29 masterchief_userpoints_badges-install.patch518 bytesMasterChief
#29 masterchief_userpoints_badges-module.patch1.25 KBMasterChief
#29 masterchief_userpoints_role-module.patch532 bytesMasterChief
#27 masterchief_userpoints_badges-install.patch518 bytesMasterChief
#27 masterchief_userpoints_badges-module.patch1.25 KBMasterChief
#27 masterchief_userpoints_role-module.patch532 bytesMasterChief
#26 masterchief_userpoints-install.patch1.56 KBMasterChief
#25 masterchief_userpoints-module-v2.patch5.59 KBMasterChief
#24 masterchief_userpoints-module.patch4.38 KBMasterChief
#8 417830-float-with-precision-2.patch7.52 KBjrust
#7 417830-float-with-precision.patch8.49 KBjrust
#3 convert-to-float.patch5.03 KBkbahey
#2 addFloat2.patch6.44 KBgdoteof
addFloat.patch9.8 KBgdoteof

Comments

jredding’s picture

The patch is welcomed but it has problems.

each .install update is marred with this typo change

- db_change_field($ret, 'userpoints_txn', 'tid', 'tid', array('type' => 'int', 'not null' => true, 'default' => 0, 'length' => 11));
- db_change_field($ret, 'userpoints', 'tid', 'tid', array('type' => 'int', 'not null' => true, 'default' => 0, 'length' => 11));
+ db_change_field($ret, 'userpoints_txn', 'tid', 'tid', array('type' => 'int', 'not null' => true, 'default' => 0, 'lenght' => 11));
+ db_change_field($ret, 'userpoints', 'tid', 'tid', array('type' => 'int', 'not null' => true, 'default' => 0, 'lenght' => 11));

note the 'lenght' misspelling for the last db_change_field. each update has this error.

Can you please fix this error and reroll the patch.

Thanks

gdoteof’s picture

StatusFileSize
new6.44 KB

oops. i even brought up the typo in the first place :). with typo fixed

kbahey’s picture

StatusFileSize
new5.03 KB

Reviewed visually and removed some stray files and hunks.

Here is the reformatted version.

jredding’s picture

visual once-over looks good to me, I'm excited for it.

jrust’s picture

Huge +1 for this one -- makes purchasing with point via Ubercart possible without having to round the order total. Applied the patch and it worked fine, except for one problem that arises due to using floats:

Once you add or subtract float points i.e. (50.54), then you start getting lots of digits for values like "Approved points balance" on the /myuserpoints page. This is because when MySQL does SUM() on a FLOAT column it pulls numbers with lots of numbers after the decimal point because floating point numbers are only approximate. So, two options: use DECIMAL instead or round the values in the summary.

kbahey’s picture

Status: Needs review » Needs work
jrust’s picture

Status: Needs work » Needs review
StatusFileSize
new8.49 KB

Attached is a patch that adds a setting specifying what precision to display and then formats the points when they are displayed with that level of precision.

jrust’s picture

StatusFileSize
new7.52 KB

Adding a revised patch that fixes some bugs that showed up when users had more than 1000 points.

kbahey’s picture

I want to commit this, but due to the fact that most contrib will not work properly until they deal with floats, I think that we should branch t 6.x-2.x-dev (DRUPAL-6--2) then commit this.

That is unless some kind soul(s) volunteer their time to changing all userpoints_contrib that are in 6.x-1.x-dev to the new API and make it ready for commit when I am ready.

What does everybody think?

joyltd’s picture

Just thought I'd add my support to this functionality - would love to help but my PHP knowledge is not strong enough!

I have tried to apply these patches to 6.x-1.x-dev, 6.x-1.0 & 6.x-1.1 and i get failures on them all.

I'm more than happy to help out as a tester so any ideas of what release to apply the patches to?

kbahey’s picture

Status: Needs review » Needs work

I like this feature but because it impacts all contrib modules using userpoints' API, I'd rather delay that to D7, where a change in the API will be more palatable.

killah89’s picture

Patch does not work for me, i have test all versions of 6.x and no one work. Please help me.

killah89’s picture

The patch does not work for me. Please help to solve this problem. My Version is 6.x-1.x-dev. and my contrib 6.x-1.0-beta1. Please please help me ...

Thanks a lot.

kbahey’s picture

This patch will not go to the 6.x branch.

It should go to the 7.x branch, because it changes the data structure drastically and will cause all sorts of issues.

killah89’s picture

Give no decimal support for 6.x? I need decimal! Please help.

Thanks

kbahey’s picture

Read the comments above.

It requires too many changes in too many modules.

Not feasible unless people are willing to work on all modules and have them ready.

Will not happen ...

YK85’s picture

If the float support is added to 7.x, may there be a 6.x-2.x version that is in sync with the D7 version to support floats? It seems other modules are also having a new branch of the D6 version in sync with D7 version where new features are implemented and supported until D7 becomes standard and D8 comes into the horizon.

Thanks!

kbahey’s picture

This is possible in theory.

In practice, it means more branches to maintain, and hence more work for the maintainers.

If someone is capable and willing, and has a history of contribution can step in as a co-maintainer, it will not happen.

johnnygg’s picture

any workaround yet for using decimals with 'userpoints' available? i really need it for my site. thanks

bmagistro’s picture

I am curious, what does it break in the contrib modules? If they return an int instead of a float the db should still be able to handle it like it does now wouldn't it?

berdir’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev

Every module that somehow displays points need to be changed for example.

I'll see if I can port the patch to 7.x there's quite something missing. If someone else wants to try:

- updated hook_schema() so that it works for new installs too
- write a helper function (a theme function maybe?) to display points so we don't have to repeatedly use numer_format() with all these arguments everywhere.
- Update to D7, everything that is related to queries (most of the patch is), needs work.

BenK’s picture

Subscribing

MasterChief’s picture

A lot of modules Drupal 7 will not be really working before 1 year, so it's important to take in consideration 6.x-1.x-dev.

Actually 353,080 are using drupal 6.x and just 23,913 drupal 7.x.

So please integrate this feature too in 6.x-1.x-dev.

5,478 are using drupal 6 version of this module and just 131 people are using drupal 7.

PS:

I will happily test patches provide here.

Patch in #8 is it working without contrib modules ?

MasterChief’s picture

StatusFileSize
new4.38 KB

Hi kbahey and Berdir!

I could try to make patches to a 6.x-2.x-dev. branch with float.

If i do it, do you will create a new branch for userpoints and userpoints_contrib ?

I am attaching a first patch for userpoints.module file like #8 patch was applied to userpoints.install .

Let met know your point of view on this patch, it's the first time i am trying to contribute to a module.

The patch applies on the latest 6.x-1.x-dev.

MasterChief’s picture

StatusFileSize
new5.59 KB

I made a v2 of the patch.

MasterChief’s picture

StatusFileSize
new1.56 KB

Now the patch of the installation.

I would like an answer, thanks.

MasterChief’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Assigned: gdoteof » MasterChief
Status: Needs work » Needs review
StatusFileSize
new532 bytes
new1.25 KB
new518 bytes

Now the patches for userpoints_contrib !

I think i didn't miss anything let me know your review.

Status: Needs review » Needs work

The last submitted patch, masterchief_userpoints_role-module.patch, failed testing.

MasterChief’s picture

MasterChief’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, masterchief_userpoints_role-module.patch, failed testing.

MasterChief’s picture

Ok that would be good this time :)

Status: Needs review » Needs work

The last submitted patch, masterchief_userpoints_role-module.patch, failed testing.

berdir’s picture

- Maintaining an additional feature branch is a lot of work. It's not finished by commiting a few patches, bugs need to be fixed, supported, and so on.

- I personally have no plans right now to start 6.x-2.x, I am focussing on 7.x. That doesn't mean that we can't talk about it if you are willing to help maintaining it. One way to prove that would IMHO be to get a number of issues (features, D7 backports of usability and other improvements, ..) into a commit-worthy state.

- You are right, the 6.x version has currently much more installations but I it is only a matter of time until that will change.

- About your patches, please provide a single patch per project and use either cvs or git to create them, see http://drupal.org/patch/create. The idea is that the patches need to apply cleanly on the testbot what they only do if the patch directory is relative from the top-level directory of that project.

- Create a separate issue in the userpoints_contrib project, link it from here and attach a single patch for all files in there.

- Once that is done, I can start to review your patches and give you feedback. But be warned, my experience with core development has converted me into a nitpicking reviewer ;) Plus side is, you are going to learn more about coding style, drupal and maintaing a project.

- Some initial thoughts: a) we need a way to consistently display these points, as said above, maybe a theme function. For example, so that points are _always_ displayed with the same amount of decimal places. b) As mentioned for example in comment #5, it might actually be a better idea to use a decimal data type with a fixed number of decimal places. Because doubt that we will have to store very large numbers, which is the main advantage of float imho.

MasterChief’s picture

Thank you for your answer Berdir.

1) i know it's a lot of work :)

2) i will provide the most patches i can.

3) But when in 1 or 2 years ? it's a lot of time.

4) I will read your link to create a proper patch to be able to pass the test (^_^)

5) I will do it.

6) It's noted and learn it's great !

7) What do you think for the code above, it will be a good start ? let me know your point of view !

'points' => array(
             'description' => 'Current Points',
             'type' => 'numeric',
             'precision' => 15,
             'scale' => 2,
             'not null' => TRUE,
             'default' => 0,
       ),
MasterChief’s picture

The separate issue in userpoints_contrib :

Userpoints_contrib new branch with float support

MasterChief’s picture

Just a question, i am using winmerge to make patches, a solution exists to make patches with this software to pass test ?

If no, how can i download the userpoints directory wit Git ? (no CVS)

I read the handbook but no success.

berdir’s picture

git clone git://git.drupal.org/project/userpoints.git

And to create a patch that works with the testbot, it's import to use the "--no-prefix" option.

git diff --no-prefix > something.patch

MasterChief’s picture

Thank you Berdir, now i know how to make clean patches!

Did you agree to make points numeric like in #35 ?

Tell me what you think about it :)

berdir’s picture

Yes, numeric sounds good, scale 2 should be enough I guess, not sure if we need precision 15 but it shouldn't hurt.

MasterChief’s picture

I have a question, i saw in the API drupal this :

db_query($query) => Valid %-modifiers are: %s, %d, %f, %b (binary data, do not enclose in '') and %%.

So what i must use ? i don't see %n .

MasterChief’s picture

Version: 6.x-1.x-dev » 6.x-1.2
Status: Needs work » Needs review
StatusFileSize
new4.97 KB

Here the patch for userpoints module.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new15.15 KB

Now the same patch + patch userpoints_api.test.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new14.83 KB

New patch.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review

Ok the problem is with userpoints_api.test, the patch in #44 was the better, could you check it and tell me what is wrong.

I made a patch too for userpoints_contrib which has no problems.

berdir’s picture

Status: Needs review » Needs work
+++ userpoints.install
@@ -22,13 +22,17 @@ function userpoints_schema() {
-        'type' => 'int',
+        'type' => 'numeric',
+		'precision' => 9,
+		'scale' => 2,
         'not null' => TRUE,
         'default' => 0,
       ),
       'max_points' => array(
         'description' => 'Out of a maximum points',
-        'type' => 'int',
+        'type' => 'numeric',
+		'precision' => 9,
+		'scale' => 2,
         'not null' => TRUE,
         'default' => 0,
       ),
@@ -74,7 +78,9 @@ function userpoints_schema() {

@@ -74,7 +78,9 @@ function userpoints_schema() {
       ),
       'points' => array(
         'description' => 'Points',
-        'type' => 'int',
+        'type' => 'numeric',
+		'precision' => 9,
+		'scale' => 2,

Use 2 spaces instead of tabs.

+++ userpoints.module
@@ -481,9 +481,9 @@ function userpoints_get_current_points($uid = NULL, $tid = NULL) {
   elseif ($tid == 'all') {
-    return (int)db_result(db_query('SELECT SUM(points) FROM {userpoints} WHERE uid = %d', $uid));
+    return (numeric)db_result(db_query('SELECT SUM(points) FROM {userpoints} WHERE uid = %d', $uid));
   }
-  return (int)db_result(db_query('SELECT points FROM {userpoints} WHERE uid = %d AND tid = %d', $uid, $tid));
+  return (numeric)db_result(db_query('SELECT points FROM {userpoints} WHERE uid = %d AND tid = %d', $uid, $tid));

There is no numeric data type in PHP. Not sure how to properly use numeric, actually. Try removing the cast or try (float) and then we'll see what happens. Maybe we'll have to go back to float, although I don't like that.

+++ userpoints.module
@@ -776,19 +776,19 @@ function _userpoints_update_cache(&$params) {
-              SET points = %d, max_points = %d, last_update = %d
+              SET points = %n, max_points = %n, last_update = %d
               WHERE uid = %d AND tid = %d",
               $current_points,
               $max_points,
@@ -800,7 +800,7 @@ function _userpoints_update_cache(&$params) {

@@ -800,7 +800,7 @@ function _userpoints_update_cache(&$params) {
   else {
     $result = db_query("INSERT INTO {userpoints}
       (uid, points, max_points, last_update, tid)
-      VALUES (%d, %d, %d, %d, %d )",
+      VALUES (%d, %n, %n, %d, %d )",

I see that you found %n. Interestingly, that seems to be something very new and nobody is using it, except twitter.module, need to ask eaton about this.

Powered by Dreditor.

MasterChief’s picture

When you say use 2 spaces it's this ?

+++ userpoints.install
@@ -22,13 +22,17 @@ function userpoints_schema() {
-        'type' => 'int',
+          'type' => 'numeric',
+          'precision' => 9,
+          'scale' => 2,
         'not null' => TRUE,
         'default' => 0,
       ),

or this ?

+++ userpoints.install
@@ -22,13 +22,17 @@ function userpoints_schema() {
-        'type' => 'int',


+         'type' => 'numeric',
+         'precision' => 9,
+         'scale' => 2,
         'not null' => TRUE,
         'default' => 0,
       ),

Not sure to have understand.

Otherwise, tell me the eaton's answer, i know decimal it's the same than numeric.

berdir’s picture

Both of them. You should see the difference if you look at it, tabs have a size of 4 spaces, the old and the new lines aren't on the same line.

Try installing http://drupal.org/project/dreditor if you are using Firefox, that will make it easier to see things like that.

MasterChief’s picture

Hi Berdir!

Did you have a contact with Eaton ?

MasterChief’s picture

I found the links below about numeric and drupal 6:

http://drupal.org/node/904880

http://drupal.org/node/609060

berdir’s picture

I am not sure.

From what I can see, numeric's are treated as string when passed and returned from the database. So I'm not convinced that is a good idea, because the value will be converted to a int or float as soon as you do something like if $points + 1. For example, when $points is '100.1' and you add 1 to it, it will be 101.1 (float), but when you have '100' and then add 1, you have 101 (int).

So we can just as well store it as a float value directly. Then it is at least consistent everywhere.

Whatever we do, will absolutely need something like a theme_userpoints_points() function and use that *everywhere* where points are displayed to ensure that you will never see something like 4.9999999, which you can end up easily when using floats. By default, that theme function can be a simple wrapper of number_format(), which is passed with the correct arguments according to the settings. You could then override that and for example do something similar to stackoverflow and display large amounts shortened, say 21100 as 21.1k.

There is another option that would avoid changing the database and the internal code. Because we need a theme function to display points anyway, we could do something similar like most systems that work with currencies. There is a setting that allows you to configure the amount of decimal places. Say, it is set to 2. Then, when you have 100.00 points, it is actually stored as 10000 (int). There are however two problems with that:

- When you would change the number of decimal places from 2 to 3, all points would basically be divided by 10, with the above example, you're left with only 10.000 points. We can deal with that with a bold warning below that setting I think.

- The other thing could be a bigger problem. How to deal with it when inserting values, for example when adding points or when configured all those contrib modules how many points you get for a specific action. I can see two possibilities right now:
+ Do nothing. Site admins will have to take care of it itself in a way that we tell them, if they have 2 decimal places, they will actually have to put 100 in any points textfield when they want to grant 1.00 points.
+ provide an api function that will automatically convert the values and hope that modules will use it. For example, it would basically do something like "$actual_value = (int)($inserted * $number_of_decimal_places)".

What do you think?

MasterChief’s picture

I think option 1 is the best solution.

So with this solution we need float and a function theme_userpoints_points() with number_format().

I never check the drupal 7 version, you used float no ?

Anyway with option 1 we need another branch right ?

When all patches will be good for userpoints and userpoints_contrib, I will contact some maintainers like the guy of ubercart userpoints and ask to make another branch compatible with this branch.

MasterChief’s picture

Version: 6.x-1.2 » 6.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new17.32 KB

Ok here the patch with the solution 1 :)

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new17.74 KB

Another patch.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Could you check the patch Berdir ?

Seems to be always the same problem in the 3 fails.

berdir’s picture

Status: Needs work » Needs review

Well, that is exactly the problem you're getting into when using float's and the reason it might not be as trivial as you initially thought :)

When you have floats, something like this might not actually give the desired result:

$value = 2.0 + 3.0;
if ($value == 5.0) {
  echo 'equal';
}

This might or might not work, it is possible that $value is now actually 4.9999999999. And the chance that this happens gets higher the bigger the values get. This is because the way floating point numbers work, you only ever get an approximate value that is more or less close to what you actually wanted.

This is why currency systems either work with numeric data types or integers and then internally always use the smallest currency format available, for example cents instead of dollars. That is the reason I was proposing the solution with the int above.

berdir’s picture

About the test fails, the test report tells you on which line they exactly happen. Those are likely lines which are doing comparisons of stored and expected values. One way to fix it is to use a rounded value.

berdir’s picture

Some feedback about your patch.

+++ tests/userpoints_api.test
@@ -38,7 +38,7 @@ class UserpointsTestCase extends DrupalWebTestCase {
-    $points = (int) rand(1, 500);
+    $points = (float) rand(1, 500);

You don't need to replace all (int) with floats. We still want to allow passing in integers into our API, if at all, it should only be converted inside that function. Additionally, rand() already returns an int, so the initial cast isn't even necessary.

+++ userpoints.install
@@ -281,3 +281,16 @@ function userpoints_update_6012() {
+/**
+ * int becomes decimal
+ */
+ ¶
+function userpoints_update_6013() {

This needs a better description, there is also a unecessary empty line.

+++ userpoints.module
@@ -463,6 +465,25 @@ function userpoints_admin_settings() {
+    '#description' => t(''),

No need to have an empty description. Also not sure if this really needs yet another category for a single setting.

+++ userpoints.module
@@ -1510,6 +1531,7 @@ function userpoints_block($op = 'list', $delta = 0, $edit = array()) {
+          $points = theme_userpoints_points($points,variable_get(USERPOINTS_PRECISION_NUM));

First, you need to add that theme function to the hook_theme() implementation and then, call it like this: theme('userpoints_points', $points). The point is that it can be overriden by a theme, if you call it directly, you are making that impossible.

Additionally, the variable_get() should be inside the theme function, makes it much simpler.

+++ userpoints.module
@@ -1874,3 +1896,12 @@ function userpoints_views_api() {
+function theme_userpoints_points($points, $number_decimal = 2, $decimal_separator = ',', $thousands_separator = ' ') {
+ $decimal_points = number_format($points, $number_decimal, $decimal_separator, $thousands_separator);
+ return $decimal_points;
+}

As I said, we only want to pass in $points here, making it a dumb wrapper of number_format() is not useful, then we could just as well use number_format directly. That's exactly what we want to avoid here :)

Also, note that there are many more places where points are displayed, for example the user profile, all administration screens, drupal_set_message() calls and so on.

Powered by Dreditor.

berdir’s picture

Status: Needs review » Needs work

Somehow managed to set the wrong status..

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new17.66 KB

Ok i made changes, here the new patch.

I am not sure to have really understand the part :

"First, you need to add that theme function to the hook_theme() implementation and then, call it like this: theme('userpoints_points', $points). The point is that it can be overriden by a theme, if you call it directly, you are making that impossible."

Let me know what you think about the new patch and maybe explain a little more about hook_theme.

berdir’s picture

Have a look at the function userpoints_theme(). Similar to the already existing definitions, you need to add one for 'userpoints_points'. See also http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo....

+++ userpoints.module
@@ -1874,3 +1895,12 @@ function userpoints_views_api() {
+function theme_userpoints_points($points, $number_decimal = 2, $decimal_separator = ',', $thousands_separator = ' ') {

You can remove all arguments except $points. I don't think we need the ability to be able to override for specific calls.

Powered by Dreditor.

MasterChief’s picture

function userpoints_theme() {
  return array(
    'userpoints_points' => array(
      'arguments' => array()
     ), 
  ); //return array
}

Is this right ?

berdir’s picture

You just need to append the inner three lines to the existing function. And inside arguments, add "points => NULL".

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review

You don't write quotes in #68, so what's is good between this :

function userpoints_theme() {
  return array(
    'userpoints_list_users' => array(
      'arguments' => array(
        'header' => NULL,
        'rows' => NULL,
        'tid' => NULL,
        'pager_limit' => NULL,
        )
      ),
    'userpoints_list_users_header' => array(
      'arguments' => array()
     ),
    'userpoints_list_users_row' => array(
      'arguments' => array(
        'row' => NULL,
         )
    ),
    'userpoints_list_my_userpoints' => array(
      'arguments' => array(
        'args' => NULL,
        'header' => NULL,
        'rows' => NULL,
        ),
    ),
    'userpoints_points' => array(
      'arguments' => array(
	    'points' => NULL
	  ),
     ),
  ); //return array
}

and this :

function userpoints_theme() {
  return array(
    'userpoints_list_users' => array(
      'arguments' => array(
        'header' => NULL,
        'rows' => NULL,
        'tid' => NULL,
        'pager_limit' => NULL,
        )
      ),
    'userpoints_list_users_header' => array(
      'arguments' => array()
     ),
    'userpoints_list_users_row' => array(
      'arguments' => array(
        'row' => NULL,
         )
    ),
    'userpoints_list_my_userpoints' => array(
      'arguments' => array(
        'args' => NULL,
        'header' => NULL,
        'rows' => NULL,
        ),
    ),
    'userpoints_points' => array(
      'arguments' => array(
	    points => NULL
	  ),
     ),
  ); //return array
}
berdir’s picture

The first one, sorry. Oh, and please add a comma at the end :)

MasterChief’s picture

StatusFileSize
new18.44 KB

lol :)

New patch.

MasterChief’s picture

StatusFileSize
new18.33 KB

Minor changes.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new18.64 KB

New patch with round for the 5 fails.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Ok so round() doesn't work to pass test :(

berdir’s picture

Well, at least not like that.

You will have to round both values the same way. Or cast them to an int, that should work too. But again, both values.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new18.68 KB

First try with round for both values.

berdir’s picture

You would probably be faster if you install http://drupal.org/project/simpletest on a local test site and then run the tests there.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

I made a new install with simpletest just to test.

I run the test and no fails.

I think the last patch doesn't pass because you make some commits.

I will wait the new dev version to be available to download.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new18.71 KB

Here the patch which pass simpletest with no fails and apply on the latest dev version :)

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new18.7 KB

Change round() to cast (int) => line 133 and 166.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

I don't know why in line 133 and 166 it doesn't work, because it's working in simpletest.

MasterChief’s picture

Hi Berdir!

Could you check the last patch and give your opinion about it ?

It is working in simpletest.

berdir’s picture

I do have the same fails locally, here is why:

+++ tests/userpoints_api.test
@@ -49,15 +49,15 @@ class UserpointsTestCase extends DrupalWebTestCase {
+    $sql = "SELECT points from {userpoints_txn} WHERE uid = %d AND points = %f";
+    $db_points = (float) db_result(db_query($sql, $user->uid, $points));

We are looking for a row with an exact amount of points. That doesn't work just like it doesn't work to compare them. Instead, rewrite that query by just looking for the uid and order by timestamp, if thee are really multiple rows, probably not.

+++ userpoints.module
@@ -216,6 +218,11 @@ function userpoints_theme() {
+	'userpoints_points' => array(
+      'arguments' => array(

Tabs here, replace them with two spaces each.

+++ userpoints.module
@@ -460,6 +467,24 @@ function userpoints_admin_settings() {
+  ¶
+  // Number of decimals to display

Trailing spaces.

Also, many things from #63 haven't been fixed yet, for example the lonely setting, places where you are not using the theme function and

Powered by Dreditor.

MasterChief’s picture

StatusFileSize
new21.4 KB

Hi berdir,

you didn't finish your last sentence in the last post.

I am providing a new patch with a lot of changes, i integrate the lonely settings in the best place to my opinion, add a call of the theme function in a lot of places.

The patch has 2 fails in simpletest, line 71 and 107 but it seems i can't skip them with round or (int).

Let me know your point of view.

MasterChief’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new21.92 KB

New patch.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

MasterChief’s picture

I let you check the last patch, i think i will go crazy with test lol :)

I tested manually add points, retire points, check my points, users by points, points in admin and try the option with number of decimals, all seems to work fine with the right display.

The only thing i didn't check is the part with moderate.

MasterChief’s picture

Status: Needs work » Needs review
StatusFileSize
new21.87 KB

Minor changes.

MasterChief’s picture

StatusFileSize
new21.41 KB

Delete all not necessary spaces.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new22.27 KB

I never said that this is going to be easy :)

You are changing one of the most basic aspects of this module. Additionally, the tests are rather crappy :)

Anyway, I looked at here patch, here's the things I've changed:

- You went a bit overboard with your theme('userpoints_points') usage :) Only use that function when displaying points, not when doing internal calculations or inside userpoints_get_current|max_points() api functions.

- Always include a space after a comma in a function call (theme('userpoints_points', $points) instead of theme('userpoints_points',$points).

- Some adjustments to the tests in general

- Changed the default value of precision to 0, so that it looks the same visually as without the patch.

Tests now all pass locally for me.

Also, it would be really good to actually have tests for this new stuff instead of just hacking around to make the existing tests pass.

- First, we have to verify that float calcuations work properly, currently we just use integers.

- Second, we should verify that the display with the theme functions works as expected. Could work like this: add some weird points like "1.24667", verify several places (myuserpoints, total, profile, ..) that it shows up correctly for a precision setting of 0, 2 and 4 or something like that.

MasterChief’s picture

Hi berdir!

I am attaching a new patch with the patch in #99 with some adds (lack a call of theme function in 1 place)?

I tested with 0-2-3-4 number of decimals.

0-2-3 works and display well.

4 works but round the value at the 3rd decimal but do it well :)

I hope to have not forgotten anything in #99 patch.

MasterChief’s picture

StatusFileSize
new22.4 KB

Oops i forgot the patch lol :)

MasterChief’s picture

Just a message to have some news about the last patch.

MasterChief’s picture

Do you think it's ready to commit?

Thank you for your answer :)

MasterChief’s picture

Just a message to have some news :)

quotesbro’s picture

Status: Needs review » Needs work

Thank you for your work on this problem.
I'm testing patch #101 and I noticed that userpoints info (category 'User !points') in user profiles is gone after patching.

Also while patching 6.x-1.x-dev (February 25 version) there was 1 fail: Hunk #13 FAILED at 1710.
Reject:

--- userpoints.module
+++ userpoints.module
@@ -1710,11 +1725,11 @@
     //maintain a grand total
     $grand_total += $result['total'];
   }
-  $args['approved_total'] = $grand_total;
+  $args['approved_total'] = theme('userpoints_points', $grand_total);
 
   //Grab the unmoderated point total
-  $args['unapproved_total'] = (int)db_result(db_query("SELECT SUM(points) FROM {userpoints_txn} WHERE uid = %d AND status = 1", $uid));
-  $args['overall_total'] = ($args['approved_total'] + $args['unapproved_total']);
+  $args['unapproved_total'] = theme('userpoints_points', (float)db_result(db_query("SELECT SUM(points) FROM {userpoints_txn} WHERE uid = %d AND status = 1", $uid)));
+  $args['overall_total'] = theme('userpoints_points', ($args['approved_total'] + $args['unapproved_total']));
 
   $header = array(
     array('data' => t('!Points', userpoints_translation()), 'field' => 'points'),

I simply replaced these strings by myself.

berdir’s picture

I realy don't know yet what to do with this. This patch can have huge implications (for example on the performance on large sites, dealing with float *is* slower than int). It will at least need a lot of feedback from people, performance tests and so on.

One possible way for now might be to create your own sandbox project ("Userpoints Float" for example) and then maintain your changes there until we figure out how to proceed here.

If you want to do that, then use what Git provides us, don't start from scratch but simply branch off:

- Clone the userpoints repository
- Create a new branch, name it "6.x-2.x", "float" or whatever you want
- Create a sandbox project, push your new branch up there

This allows you to easily fetch updates from the original project and integrate them with your changes and it allows other users to download your changes (with git or by providing a manual tarball).

If you need more detailed instructions for git, the "Git instructions" tab on every project is very helpful, you can also check the documentation or ask in IRC (#drupal-gitsupport)

MasterChief’s picture

Hi berdir, hi dberror !

I made a new sandbox module Userpoints Float, you can access it here :

http://drupal.org/sandbox/MasterChief/1091888

Maybe someone could change the description of userpoints module to tell about this sandbox module, it will help to have feedbacks.

The initial commit is 6.x-1-x.dev of userpoints module.

The 2nd commit is #101 patch with some changes.

Let me know if you have already the issue dberror.

PS:

Don't hesitate to create issues in the sandbox :)

quotesbro’s picture

About userpoints info in user profiles - I found out that this is issue of version 6.x-1.x-dev (not patched), and the patched version has the same issue as result.

MasterChief’s picture

Did you open an issue about this in the userpoints issues queue ?

I will apply the patch when it will be commit in userpoints module.

Did you have already the 1 fail you said in #105 ? (A link is prodived in the description to download the module).

berdir’s picture

MasterChief, can you join the irc channel #drupal-games? (In a few hours, I don't have time right now anyway) See http://drupal.org/irc

MasterChief’s picture

Yes i can but when you say few hours is it 2 or 3 hours ? (or more?)

berdir’s picture

~3 hours

MasterChief’s picture

Ok i will be there :)

that0n3guy’s picture

subscribing

grandmaster’s picture

Component: Code: userpoints API » Code: userpoints

can't the user just enter a value in cents, if you want to use this for a currency? so if you want to add points to an account, or $100.14, can't you just enter 10014. then format how the points are displayed on the site. this seems simple. could that work?

berdir’s picture

That is my idea as well and will eventually happen in a 7.x-2.x branch. Not trivial either, however. See #1165256: Allow points with decimal places for more information of what that would require.

DrakeRemory’s picture

StatusFileSize
new75.25 KB

I took Berdirs idea from http://drupal.org/node/1165256#comment-4712606 and implemented a separate column for the decimal places. Works fine for me and shouldn't break any contributed modules. You can choose between 0 and 8 decimal places.

DrakeRemory’s picture

Sorry, I forgot about views. Will do that probably tomorrow.

MasterChief’s picture

Just a question is it for drupal 6 ?

If it is i will test it tomorrow :)

DrakeRemory’s picture

Yes it is.

MasterChief’s picture

Great i am waiting the views support to test it :)

DrakeRemory’s picture

StatusFileSize
new77.78 KB

Here is my first implementation of views handlers. Filters are tricky and not working properly. I'll have to look into that again in next few days. Otherwise you can only filter by full points, which should be fine.

matdab’s picture

subscribing, thanks for that work!

matdab’s picture

DrakeRemory, I've installed your version, but I'm afraid points weren't counted at all. Maybe I've missed sth - am I correct to think, that this is full "replacement" version for userpoints.module for drupal 6?

thanks,
mat

MasterChief’s picture

I am lacking of time to test it, i think you understood well matdab.

matdab’s picture

Can I help somehow? I am not a programmer, but as far as user/admin tests are considered I will be eager to participate. Just please tell me what kind of information I should gather:)
MasterChief, is your version somehow working, or we should focus on DrakeRemjory's?

MasterChief’s picture

My version works fine but it will be more easy, because he keeps integers, with contrib modules with Drake version maybe even best for performances.

But it seems you have difficulties to make it work with drake version ?

DrakeRemory’s picture

I haven't tested the update paths, so that might be your problem. Hav you tested it in a clean installation?

matdab’s picture

Issue tags: +userpoints decimals

Thanks guys, yes - I tested it as an update on production site.
I've tested it now on clean installation and yes!!!:) it worked! Only the green messages about earning points wern't shown, though configured.

The problem with update, i can see, is that the update is not recognised during update.php and thus in userpoints settings, I don't get decimal settings fieldset. Is there any way to update it, cause uninstalling would mean for me loosing all points-data...?

Now I've tested it on the older version non-production as an update and it worked halfly, but still I didn't get decimal points settings, so:
- 10.5 points for adding node were counted as 10 points (contrib module - points for posting nodes), I haven't checked other transactions
- there were no green system messages about earning points
- sum of user points weren't counted - it was still the same figure
- though the table: /myuserpoints/% was showing new points and transactions...

Guys, can I do anything to help with implementing this work? I need that feature very much:)

Edit: Wait, now I discovered that in the newest update version I've got this fieldset, I'll let know in a few minutes if it works:)

matdab’s picture

So after update, I've got fieldset, but:
- views with user points dissapeared
- table /myuserpoints/% dissapeared
- I've got custom module showing points table - there also nothing changes after there should points be added to user
- points are not earned, not counted - nothing happens

What can I do now to test what happens?

cheers
mat

DrakeRemory’s picture

StatusFileSize
new77.8 KB

There was a typo in the update path. Update this new version. Go to update.php and select update 6013 for userpoints. Couldn't test it but it should work. Otherwise you'll have to add the field manually with phpmyadmin or something similar. Just use the same settings as the points field and name it points_dec.

DrakeRemory’s picture

StatusFileSize
new77.15 KB

I had to rewrite the views integration, because the other one didn't work with views calc. This one is even simpler and should work better. I also fixed an issue with the category field in views. It didn't show any results because it relied on the node table.

matdab’s picture

Thanks Drake! Now it works with that 6013 update:)

DrakeRemory’s picture

StatusFileSize
new77.16 KB

Fixed another bug with subtractions.

DrakeRemory’s picture

StatusFileSize
new77.2 KB

Fixed even more issues.

berdir’s picture

Can you please provide your changes as a patch?

That would allow me to review your implementation and give feedback...

DrakeRemory’s picture

StatusFileSize
new25.49 KB

Ok, I fixed another issue, hopefully the last one. I also tried to make a patch which is always a pain in the ass on my windows machine. I hope it works.

tejaspmehta’s picture

I have used patch provided in #101 by MasterChief. It was applied properly but only 2 places it did not replaced it, so i have manually replaced it. After that i found that theme() function is used to display userpoints. It was adding userpoints in proper format like 1.2 or 1.4 or 1.7 but display was either 1 or 2. So i have removed theme() wherever it is used to display points. And it simply works.

Thank you
Tejas Mehta

MasterChief’s picture

Hi tejaspmehta,

i am not using anymore the #101 patch, i made a new version, i will post the patch here when i will have time.

it's a custom version of DrakeRemory patch, i fixed some problems, i didn't test the lastest version in #137.

What did you change drake in #137, it's a fix with views support or another fix ?

DrakeRemory’s picture

I completely rewrote the views imlementation of my changes and fixed some other stuff, too. So you should use the newest version. There were no changes to the database though so just apply the path to the newest userpoints 6.x-1.2 and you're good to go.

MasterChief’s picture

Why you didn't apply your patch again 6.x-1.x-dev version ?

I think it's better to apply the patch with the new stuff, because you can't apply it directly!

berdir’s picture

Status: Needs work » Needs review

@DrakeRemory

You might be interested in reading my review over at #1165256-28: Allow points with decimal places, which contains a patch from MasterChief which is based on yours.

Many of my remarks (although not all, some are only relevant to MasterChief's changes or Drupal 7) also affect your patch and should be fixed here too.

If there are any questions, please ask.

Also note that you should set an issue to "needs review" after uploading a patch, this will automatically try to apply the patch and run the existing tests.

Status: Needs review » Needs work

The last submitted patch, userpoints.patch, failed testing.

matdab’s picture

Hi guys,

I've tested #137 patch on production site and everything was OK until decimals exceeded 21000000000 in userpoints table, points_dec column, which always ends with "2147483647" value and u can't change it to higher, only lower, tried also with different users... - tried to change in mySQL via phpMyAdmin with no result. Also on site if I try to gain more decimal points, they're not added to userpoints table, points_dec column, however in userpoints_txn table they're added correctly and the last update value in userpoints table, points_dec corresponds to the real last txn value. Any idea what's happening?

cheers,
mat

berdir’s picture

That's the highest value that can be stored as an integer, you will need to adjust the table scheme to allow larger values and e.g. use big integer.

matdab’s picture

Thanks Berdir, would be grateful for a hint how to do that...:)

cheers
mat

berdir’s picture

Google is your friend, you should be able to find he information your looking for in this article: http://www.homeandlearn.co.uk/php/php12p3.html

matdab’s picture

I appreciate your help, esspecialy one sometimes doesn't know enough to search good phrase, so thanks for a link:)!

mat

indiayogi’s picture

Thanks a lot @DrakeRemory for this modified module, I downloaded userpoints_6 and it worked for me exactly what I want,
My setup is to allow customer to purchase product using user points, and they can buy these user points also.
But previously user points module was not deducting product prices in decimals, but it is now.
Once again thanks a lot for this....

manuel.adan’s picture

Assigned: MasterChief » Unassigned
Issue summary: View changes
Status: Needs work » Closed (outdated)

Closing this as outdated, 6.x version is no longer maintained. Floats are supported in the 8.x version.