Fixed issue #10211: Thousands separator - numeric input and multiple numeric input in the same group #412

GuillaumeSmaha · 2016-01-11T14:19:04Z

Fix issue when thousands_separator = '1'.

Shnoulle · 2016-01-11T14:28:44Z

Your sure it fix one Question with thousand and another one without thousand in same group ?

I don't find where you test the qid :)

PS : with other function we launch the function in HTML

script="doFunction({$ia[0]});"

Shnoulle · 2016-01-11T14:31:30Z

OK, never mind : bad look of code for me .... usage of class name :)

…numeric input in the same group Dev: Fix survey with numerical value and thousand separator Dev: Fix script numerical_input.js : Only use when thousand_separator = 1

GuillaumeSmaha · 2016-01-12T10:15:34Z

Yes, I tried with the .lss file provided on the bugtracker and I create an example with few numerical inputs. It works for this 2 cases.
Only the inputs with the thousand_separator use the functions priceFormat and custom_checkconditions.

Shnoulle · 2016-01-12T10:54:54Z

Yes, correct my comment in #412 (comment) :)

GuillaumeSmaha · 2016-01-12T11:01:16Z

Do you know what version will contain this patch ? (Client needs this information)

Shnoulle · 2016-01-12T11:02:36Z

application/helpers/expressions/em_manager_helper.php

+                                    if($aAttributes[$qid]['thousands_separator']=='1') {
+                                        $value=str_replace($invertRadix['separator'], '', $value);
+                                    }
+                                }


This part is to show "thousand seperator" without JS, right ? But thousand don't really work without JS. Really needed ?
-> js : update value after shown (.ready)
-> no js : get the value, and show the real value

I do a quick test with separator dot : user can not enter 1,000 without JS. Then : no js : no thousand_separator.

This part permit to remove the dots or commats from the value. (2,300,400.10 => 2300300.10 or 2.300.400,10 => 2300300,10).
Without this part, I had SQL crashes :

With dot as decimal mark : UPDATE lime_survey_499425 SET "lastpage"=0, "499425X8X40"='23,123,131,231.23', "499425X8X41"='1,231,232,131,231' WHERE ID=6 465,465,465,443 is not a decimal value

Or

With commat as decimal mark : UPDATE lime_survey_499425 SET "lastpage"=0, "499425X8X40"='1.231.231.23', "499425X8X41"='131.231.231.231.232' WHERE ID=5 This is the same with '131.231.231.231.232'

But you are right, with decimal mark as a dot and a numerical input with thousands_separator = 1 and interger_only = 0, the value saved is wrong : 100022 instead of 1000.22

In this case, the input value is 1.000.22
May be, should I move this part of code in the same place where the code transforms 1.000,22 into 1.000.22 ?

Ok, I found it ! :D
Line 8523 to 8527 :

if ($radixchange && isset($LEM->knownVars[$sq]['onlynum']) && $LEM->knownVars[$sq]['onlynum']=='1') { // convert from comma back to decimal $value = implode('.',explode(',',$value)); }

I just need to move this part after the "switch".

You have this one before right ?

Ok, I check it

@olleharstedt Ok for me, the data saved in database was fixed by your commit c11fe85
I test with a form with :

1 multiple numerical with thousand and int only

1 numerical with thousand

1 numerical with int only

1 numerical

I see 2 others errors with javascript :

1 numerical with int only : I can write a decimal part

1 numerical : I can't write a decimal part
I see two errors

@olleharstedt If you are agree with it, I can just update the file scripts/numerical_input.js to use the correct javascript part.

@GuillaumeSmaha This commit is the complete fix: 83beb12

To do this by the book, could you write a bug report and include your test survey? If you've already fixed the bugs, you can link to that PR which fixes it.

@olleharstedt Ok this is another issue, so I close this pull request.

…numeric input in the same group Dev: Move the part which convert from comma back to decimal

Shnoulle · 2016-01-12T13:20:37Z

application/helpers/expressions/em_manager_helper.php

-                        {
-                            // convert from comma back to decimal
-                            $value = implode('.',explode(',',$value));
-                        }


You remove it here ;) and remind "text" with ad attribute "only num". And Array number ;)

Shnoulle · 2016-01-12T13:21:31Z

For fix in core : like whole GPL : it's done when it's done ;).

I take a look this week to merge it

Shnoulle · 2016-01-13T17:02:42Z

@GuillaumeSmaha OK, seems to work. But i think we don't need some EM update .

Why you remove https://github.com/LimeSurvey/LimeSurvey/pull/412/files#diff-796aba4d06254b9caea9d461e4ea80c9L8521 ?
Then this was not needed : https://github.com/LimeSurvey/LimeSurvey/pull/412/files#diff-796aba4d06254b9caea9d461e4ea80c9R8548
You readd a second time here : https://github.com/LimeSurvey/LimeSurvey/pull/412/files#diff-796aba4d06254b9caea9d461e4ea80c9R8601

EM is really hard to fix and update , then if we can fix without update EM ...

Here : i think the base JS is broken : NO JS : accept only real number (just the radix)
JS : show the number with thousand separator but send to server the number without thousand

PS : in fact i really dislike the original system .... no option, no radix thinking, and right : seems we need update EM .... Better is to create a input to show the number and hide the original input.

GuillaumeSmaha · 2016-01-13T18:03:30Z

@Shnoulle 1. I just move this part on 3.
There are 2 steps for me :

(part 2) If thousands_separator = 1 for the current input then remove the thousand separator
(part 3) Change the commat into a dot.

JS : During my test, the number is sent to the server with the thousand separator and the custom decimal mark. So, I developed the fix in EM.
NO JS : I don't test it but normally, there are not bug because :

part 2 will replace thousand separator if they exist
part 3 will update the decimal mark

After all, I think it will be possible to reduce the code with this instead of the part 1 (removing part 2 & 3) :

if (isset($LEM->knownVars[$sq]['onlynum']) && $LEM->knownVars[$sq]['onlynum']=='1')
{
    //remove thousand separator
    $value=str_replace($invertRadix['separator'], '', $value);
    if($radixchange)
    {
        // convert from comma back to decimal
        $value = implode('.',explode(',',$value));
    }
}

OR as you say, it will be better to update the input value just before the form validation with an event on submit form.
If you prefer I can develop the second solution in another pull request ?

Shnoulle · 2016-01-13T18:22:00Z

Maybe you have right : we must :

Show input with thousand (without JS)
Fix input with thousand in php (then in EM like you done)

@SamMousa : you do the attribute : what did you think EM or just JS ?

PS : i think we must check how EM manage it : like date : always send YYYY-MM-DD value (for example for date comare etc) but show d/m/y value (or anything else).
PS2: todo : add a test with sum a "thousand" value and a "not thousand" value

GuillaumeSmaha · 2016-01-15T09:03:54Z

@Shnoulle
Yes, I agree with you :

EM checks data format
JS : Draw a custom value but send the real value
NoJS : Real value only

Shnoulle · 2016-01-15T09:05:09Z

I see with sam who made original modification (and other core dev)

Shnoulle · 2016-01-15T15:42:34Z

Thurday (if exist) dev meeting

SamMousa · 2016-01-19T13:46:25Z

I am not in favor of adding this type of validation to EM. Instead I recommend using it as a graphical helper only (and thus JS only).

In the database a number does not have a thousands separator.
It is supposed to help users not hurt them. (Having me submit only to tell me it is the wrong format is not helping me).

Shnoulle · 2016-01-19T13:52:23Z

For 2 : silently fix 1 000 to 1000 don't "hurt". But probelm is did we fix it in EM (like @GuillaumeSmaha do it in this fix). Or only in JS.

For only JS : we can use some $(document).on("submit","#limesurvey", function(){} ;
I don't remind if HTML5 validation is done before or after submit event .....

GuillaumeSmaha · 2016-01-19T14:33:20Z

For 2: Currently, conversion from comma mark to point mark is also done in EM. That why I added my fix at the same place.
See https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_manager_helper.php#L8524

GuillaumeSmaha · 2016-01-19T16:48:46Z

Another point : With the current implementation and JS, there are 4 kind of inputs :

Numercial input with int only & thousand separator : JS active with priceFormat (limit to 2 digits)
Numercial input with thousand separator : JS active with priceFormat (limit to 2 digits)
Numercial input with int only : JS active without priceFormat
Numercial input : JS NOT active
In the last case, there are no limit (ie: 0.00005 works)

Should I call priceFormat for this case too ?

Also, I think that priceFormat needs to be fix (in another pull request) :

When I enter a number, the cursor should not be placed at end
When I enter a number on an empty field, the cursor should be placed before the point

Shnoulle · 2016-01-19T18:04:34Z

Hi Guillaume : think for 2.06 fix : we just need a functionnal system. Maybe for another commit (2.5 or 2.06): need 3 options :

leave it
thousand seperator
price format (+ thousand seperator)

The 2 decimals is only for 3rd solution :) .

For . to , : yes ... old behaviour since 1.87 or maybe 1.70 ... BUT : here : i really don't know what user wants.

For dev : better to remove it from EM (PHP) and do it only in JS, but user don't see really difference (?) or see the difference ?
For template dev : without this option : we can easily move to HTML5 input :)

Shnoulle · 2016-01-23T10:44:03Z

I start a work to find a way to do it only in JS .... but have strange situation with priceFormat :

10000,9999 are updated to 100.099,99 with .priceFormat ( and , for separator). I make a new pull request with WIP monday .

GuillaumeSmaha · 2016-01-24T09:57:56Z

10000,9999 are updated to 100.099,99 with .priceFormat :
Yes because priceFormat do this :

Remove decimal mark: 10000,9999 => 100009999
Take the 2 last number as the decimal mark : 100009999 -> 1000099.99
Add thousand mark : 1000099.99 -> 1000,099.99
But here, there is a lost digit

Shnoulle · 2016-01-24T13:42:15Z

Yes, .... starting to do it in JS only are more difficult than i think ....
1st : due to some restriction in .priceFormat ?

        if($(this).val()!="" && centsSep == ',')
        {
            $(this).val($(this).val().split(',').join('.'));
        }

But after : need exactly 2 decimals ....

Thanks for the point . Maybe it's better to move to JS only at the same time than LEMradix ....

OK, got something :

    $('.thousandsseparator').not('.integeronly').each(function(){
        if($(this).val()!="")
        {
            var newVal=$(this).val();
            if(centsSep == ',')
                newVal=newVal.split(',').join('.');
            newVal=newVal*1;
            newVal=Math.round(newVal * 100); // What for 0 .....
            $(this).val(newVal);
        }
        $(this).unbind('keydown').removeAttr('onkeyup').priceFormat({
            'centsSeparator' : centsSep,
            'thousandsSeparator' : thousandsSep,
            'centsLimit' : 2,
            'prefix' : '',
            'allowNegative' : true
        });
    });

Must test with -1, -10, 1, 10, ....

10000.9999999 is set to 10001 ( 10.001,00) only with JS

GuillaumeSmaha · 2016-01-25T08:42:58Z

If you want to see, I have already done this work on this PR :
GuillaumeSmaha@3cee3fe#diff-7acdc538a29fbb1fac6b52b41d785b45R170
I use the separator, not the number of digits for the decimal part.

Shnoulle · 2016-01-25T09:05:26Z

Do a fix : without updating priceFormat and only in JS. But : maybe remove priceFormat and do own is a better solution ..... #423

…numeric input in the same group Dev: Avoid twice call of priceFormat Dev: Move the part which convert from comma back to decimal Dev: Fix survey with numerical value and thousand separator Dev: Fix script numerical_input.js : Only use when thousand_separator = 1

…numeric input in the same group Dev: Merge branch 'master' into fix_value_with_thousand_sep

…numeric input in the same group Revert chnage into application/helpers/expressions/em_manager_helper.php

Fixed issue #10211: Thousands separator - numeric input and multiple …

1212140

…numeric input in the same group Dev: Fix survey with numerical value and thousand separator Dev: Fix script numerical_input.js : Only use when thousand_separator = 1

GuillaumeSmaha force-pushed the fix_value_with_thousand_sep branch from 1c60550 to 1212140 Compare January 11, 2016 14:57

Shnoulle reviewed Jan 12, 2016
View reviewed changes

Fixed issue #10211: Thousands separator - numeric input and multiple …

0042d5f

…numeric input in the same group Dev: Move the part which convert from comma back to decimal

Shnoulle reviewed Jan 12, 2016
View reviewed changes

Shnoulle self-assigned this Jan 12, 2016

GuillaumeSmaha mentioned this pull request Jan 20, 2016

Update feature: Improvement priceFormat #421

Merged

Shnoulle mentioned this pull request Jan 25, 2016

Fixed issue #09876: Different bogus with thousand seperator #423

Closed

Shnoulle assigned olleharstedt and unassigned Shnoulle Feb 19, 2016

olleharstedt closed this Feb 22, 2016

olleharstedt reopened this Feb 22, 2016

GuillaumeSmaha added 2 commits February 23, 2016 10:56

Fixed issue #10211: Thousands separator - numeric input and multiple …

dd25979

…numeric input in the same group Dev: Merge branch 'master' into fix_value_with_thousand_sep

Fixed issue #10211: Thousands separator - numeric input and multiple …

ee23f1e

…numeric input in the same group Revert chnage into application/helpers/expressions/em_manager_helper.php

GuillaumeSmaha closed this Feb 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue #10211: Thousands separator - numeric input and multiple numeric input in the same group #412

Fixed issue #10211: Thousands separator - numeric input and multiple numeric input in the same group #412

GuillaumeSmaha commented Jan 11, 2016

Shnoulle commented Jan 11, 2016

Shnoulle commented Jan 11, 2016

GuillaumeSmaha commented Jan 12, 2016

Shnoulle commented Jan 12, 2016

GuillaumeSmaha commented Jan 12, 2016

Shnoulle Jan 12, 2016

GuillaumeSmaha Jan 12, 2016

GuillaumeSmaha Jan 12, 2016

GuillaumeSmaha Jan 12, 2016

Shnoulle Jan 12, 2016

GuillaumeSmaha Feb 24, 2016

GuillaumeSmaha Feb 24, 2016

GuillaumeSmaha Feb 24, 2016

olleharstedt Feb 24, 2016

GuillaumeSmaha Feb 24, 2016

Shnoulle Jan 12, 2016

Shnoulle commented Jan 12, 2016

Shnoulle commented Jan 13, 2016

GuillaumeSmaha commented Jan 13, 2016

Shnoulle commented Jan 13, 2016

GuillaumeSmaha commented Jan 15, 2016

Shnoulle commented Jan 15, 2016

Shnoulle commented Jan 15, 2016

SamMousa commented Jan 19, 2016

Shnoulle commented Jan 19, 2016

GuillaumeSmaha commented Jan 19, 2016

GuillaumeSmaha commented Jan 19, 2016

Shnoulle commented Jan 19, 2016

Shnoulle commented Jan 23, 2016

GuillaumeSmaha commented Jan 24, 2016

Shnoulle commented Jan 24, 2016

GuillaumeSmaha commented Jan 25, 2016

Shnoulle commented Jan 25, 2016

Fixed issue #10211: Thousands separator - numeric input and multiple numeric input in the same group #412

Fixed issue #10211: Thousands separator - numeric input and multiple numeric input in the same group #412

Conversation

GuillaumeSmaha commented Jan 11, 2016

Shnoulle commented Jan 11, 2016

Shnoulle commented Jan 11, 2016

GuillaumeSmaha commented Jan 12, 2016

Shnoulle commented Jan 12, 2016

GuillaumeSmaha commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shnoulle commented Jan 12, 2016

Shnoulle commented Jan 13, 2016

GuillaumeSmaha commented Jan 13, 2016

Shnoulle commented Jan 13, 2016

GuillaumeSmaha commented Jan 15, 2016

Shnoulle commented Jan 15, 2016

Shnoulle commented Jan 15, 2016

SamMousa commented Jan 19, 2016

Shnoulle commented Jan 19, 2016

GuillaumeSmaha commented Jan 19, 2016

GuillaumeSmaha commented Jan 19, 2016

Shnoulle commented Jan 19, 2016

Shnoulle commented Jan 23, 2016

GuillaumeSmaha commented Jan 24, 2016

Shnoulle commented Jan 24, 2016

GuillaumeSmaha commented Jan 25, 2016

Shnoulle commented Jan 25, 2016