1122419 - Numeric values used in timetracking fields do not accept a comma: 5,0 instead of 5.0 doesn't work

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Jochen Wiedmann]

I've got a bug with non-zero values for "Orig Est." and "Hours Worked". When opening the bug, these are displayed as "5,0". (Use of the comma is most likely due to the servers german locale.)

When I try to save that bug, I get the error message "The value '5,0' in the Orig. Est. field is not a numeric value."

I can't exactly tell what's wrong, I assume either of the following possibilities:

1.) When displaying the decimal value, the server uses the german Locale to display it, which it shouldn't. Solution is to remove the conversion.
2.) When accepting the string value, the server should use the german Locale to convert it, but doesn't.

I suspect that it is the latter: I have created a new bug with all zero values for the amount of work fields and entered values like "3,0" manually with the same result: Obviously, the server doesn't use the german Locale for conversion.

Additional hint: In my log file, I see messages like the following:

[Fri Jan 16 09:17:51 2015] [error] [client 10.29.20.109] [Fri Jan 16 08:17:51 2015] show_bug.cgi: Argument "10,00" isn't numeric in sprintf at C:/bugzilla/Perl/site/lib/Template/Filters.pm line 495., referer: http://stujsr07.eur.ad.sag/bugzilla/buglist.cgi?cmdtype=runnamed&list_id=20&namedcmd=My%20Bugs

Comment 1

[Jochen Wiedmann]

Attachment: bug1122419.patch

Updated Patch. Changes with regard to the first version:

- Relative to the Master Branch, not to the 4.4.6 version, as suggested by dkl.
- Removed trailing whitespace, as pointed out by dkl.
- Respecting case, as suggested by LpSolit.
- Fixed bug regarding a trailing ">", as pointed out by LpSolit
- Including .htaccess in the top directory. (See bug 1124082.)

Comment 2

[:glob ✱]

wrong patch/bug?

Comment 3

[Jochen Wiedmann]

Thanks, Byron, you're absolutely right. Sorry!

Comment 4

[Jochen Wiedmann]

Sorry, resolved the wrong bug.
Reopening.

Comment 5

[Jochen Wiedmann]

I did a bit of research: We've got to deal with formatting, and parsing of numbers in a Locale specific fashion, like Java's NumberFormat does.
(See https://docs.oracle.com/javase/1.5.0/docs/api/java/text/NumberFormat.html)

I found two modules, that might be usable for the formatting part: Number::Format, and CLDR::Number.

The former

- Is basically unmaintained (the author has stated, in a private mail to me, that he "didn't look at
  the code in years")
- Isn't able to deal with Locales directly. Instead, you've got to configure it yourelf, based on the
  Locale. (In the same mail, the author has stated, that he "rarely has to deal with systems other
  than en_US".)
- Has no real parser. There is a method unformat_number, but that accepts "2Only-Nonsense" as a
  number, which wouldn't be acceptable to us.

The latter

- Doesn't have any parser and won't have in the foreseeable future
  (See https://github.com/perl-cldr/cldr-number-pm5/issues/19.)

Any alternatives?

Comment 6

[Jochen Wiedmann]

I won't be able to proceed without a bit of advice.

Comment 8

[Fabio]

We have the same problem, only way to save is to change the comma to a period each time.

Also located in Germany, although the entire machine is setup in english.

-Windows Server 2008 Enterprise 32-bit ENGLISH
-Bugzilla version 4.4.9 in english
-ActiveState Perl 5.20.1 32-bit
-MySQL Server 5.5.25

I wanted to try the German template for Bugzilla but it's not available for 4.4.9 yet.
I am surprised by the lack of comments on this, I thought this was a common software combination so lots of users should see this issue. Then again, it could have to do with our configuration so if anyone has any idea on what might be wrong I'd love to hear it.

Comment 9

[Ben]

this bug is really annoying! every time a bug is updated, commented or whatever, the user has to retype the timetracking fields, even if we don't use them. you'll usually see 0,0 in the fields by default. if you edit a bug or just comment and want to use the "save changes" button, there will be an arrow saying that "0,0" is not a numeric value. if you refill the field with e.g. "0", you may save your changes, but the fields will be filled again with "0,0", so next time you'll have to do it again.

we use:
- Windows Server 2012
- Bugzilla version 5.0 (engl.)

i haven't installed the german template now, but i guess it won't fix the bug.

does anybody know a workaround, e.g. disable the timetracking functionality?

Comment 10

[Alexander Gieg]

I've just installed Bugzilla 5.0.1 in my Windows 8.1 Professional 64-bit workstation and I see this happening too. Submitting anything results in an error about the comma.

Comment 11

[Alexander Gieg]

Attachment: Bugzilla Bug #1.png

Screenshot.

Comment 12

[Frédéric Buclin]

Just to be sure, could you please check which format is used to store decimal numbers in the DB? More exactly, in the bugs.estimated_time column. Either as 0.00 or 0,00?

This bug seems to affect Windows only.

Comment 13

[Alexander Gieg]

Attachment: "bugs" SQLite database in hex editor

Sure! Here are the "estimated_time" and "remaining_time" as they appear in the SQLite database when opened in an hex editor. As you can see, the in-file decimal separator is the dot.

Comment 14

[Alexander Gieg]

Attachment: "bugs" database in "DB Browser for SQLite"

Here's the same data as shown in "DB Browser for SQLite", as well as the table's structure, also shown with dots.

Comment 15

[Alexander Gieg]

Attachment: Dots by hand work

I managed to have non-integer values entered into the database by typing the dots myself. This works fine.

The problem then is that Bugzilla shows these fields with commas instead of with dots when it loads the page, and then tries do insert the comma'ified values into the database instead of sanitizing the numbers back into dot-separated ones. So, whenever one updates anything in a bug, one must remember to go into those two fields to manually replace these commas with dots, otherwise that "go back" error message appears.

Comment 16

[Fabio]

(In reply to Alexander Gieg from comment #15)
> Created attachment 8664194 [details]
> Dots by hand work
> 
> I managed to have non-integer values entered into the database by typing the
> dots myself. This works fine.

As has been said 5 months ago. See comment #8

Comment 17

[Frédéric Buclin]

I installed Bugzilla 5.0.1 on Windows 7 French with Strawberry Perl 5.22.0, MySQL 5.6.26 and IIS 7.5, and I cannot reproduce this issue. Decimal numbers are correctly displayed with a dot instead of a comma.

Anyway, a trivial fix would be to automatically convert , into . in the validator.

Comment 18

[Ben]

(In reply to Frédéric Buclin from comment #17)
> I installed Bugzilla 5.0.1 on Windows 7 French with Strawberry Perl 5.22.0,
> MySQL 5.6.26 and IIS 7.5, and I cannot reproduce this issue. Decimal numbers
> are correctly displayed with a dot instead of a comma.
> 
> Anyway, a trivial fix would be to automatically convert , into . in the
> validator.

please not. this is a locale issue. so with a different locale, you might not be able to reproduce this bug. it's due to a locale conversion to german float value representation with ',' (other locales might have different values). so in order for the fix to work, you need to make a locale-specific conversion! or much better and easier, just don't show locale specific numbers on pages. this is a common issue with all programs and we are used to type '.' instead of ','.

Comment 19

[Graziano]

Windows 7 32bit in Italy.

Same problem. 0,0 in Orig. Est..

Tryed to change localization of S.O..

'Format' tab: English
'Location' tab: United States
'Administrative' tab, 'Change System Locale': English

Copied settings for new account and welcome screen.

After restarting the S.O. problem disappeared.

PS: resetting Italian, Italy, Italian in indicated fields and restarting problem do not represent anymore.

Comment 20

[Jochen Wiedmann]

@Graziano: What is S.O., please?

Comment 21

[Graziano]

Sorry... O.S. (Operating System).

Comment 22

[Jochen Wiedmann]

Changing the Locale settings in the O.S. sounds like a reasonable workaround on machines, that don't provide other services, apart from Bugzilla. Which is not the case for me, though.

Question: Can we change the Locale for Apache HTTPD only (for example, by setting certain environment variables)?
Or, even better: Can we do that for Bugzilla only, for example like this in the configuration snippet regarding
Bugzilla:

    SetEnv LANG "C"

Comment 23

[Graziano]

If you read with attention my post you can discover that after changing the settings I rolled back them to Italian and the issue did not represent.

Comment 24

[marius.mittendorf]

I have the same Problem on Windows 8.1 64 bit in Germany.

I hope there is another solution than to Change the Settings in the O.S.?

Comment 25

[Frédéric Buclin]

Marc: can you reproduce this issue? Do you know what the root cause of the problem is?

Comment 26

[Marc Schumann [:Wurblzap]]

I can't see this, on Windows 7, neither with IE 11 nor with FF 42 beta, using Bugzilla 5.0. I have OS localization set to German, with comma as decimal separator.

The ones who do see the problem, can you please check the page source, and see what the "value" attribute of the input field with id "estimated_time" says? I would like to confirm that the server sends a value with a dot as decimal separator, and that the conversion to a comma happens client-side.

Comment 27

[Ben]

using Chrome on a windows 7 pc seeing a bugzilla 5.0 on a windows server 2012

      <td>
        <input name="estimated_time" id="estimated_time"
               value="0,0"
               size="6" maxlength="6">
      </td>
      <td>0,0
      </td>
      <td>0,0 +
        <input name="work_time" id="work_time"
               value="0" size="3" maxlength="6"
               onchange="adjustRemainingTime();">
      </td>
      <td>
        <input name="remaining_time" id="remaining_time"
               value="0,0"
               size="6" maxlength="6" onchange="updateRemainingTime();">
      </td>
      <td>0
      </td>
      <td>0,0
      </td>

Comment 28

[Marc Schumann [:Wurblzap]]

So it's a server-side thing. It appears I misread some earlier comments, too, already indicating this.

So we need to find out where this happens there. Maybe the database connection does localization? Do you use MySQL or MariaDB? If so, can you check whether there is a configuration setting for localization?

Comment 29

[Ben]

i use MySQL Server 5.6. The only locale parameter is for client error messages, time-zone and week format

Comment 30

[Marc Schumann [:Wurblzap]]

I see this effect all of a sudden. I need to find out now what has changed.

Comment 31

[Marc Schumann [:Wurblzap]]

I still don't know what caused the change, but I found out that I can make show_bug.cgi display time values using a decimal point if I put the following below the "use"-lines in show_bug.cgi:

 use POSIX qw(locale_h);
 setlocale(LC_ALL, 'C');

I'm fairly sure that this doesn't fix everything, though. And I'm not convinced yet that we should be fixing the issue this way.

Comment 32

[Jochen Wiedmann]

That is interesting news, nevertheless, Marc. Perhaps, we might introduce the "C" as a parameter?

Comment 33

[Philippe Richer]

Been following this issue for some weeks now, because this bug affects us at work since we migrated to Bugzilla 5.0. We run a 5.0 Bugzilla install on a Windows Server 2008 R2 (french version) and a MySQL 5.5.

I applied the modifications proposed by Marc, as a temporary workaround (to appease the crowd). 

It does fix the issue when you reach the bug detail page directly (via show_bug.cgi), which is great, but does not work when you save comment and then show_bug.cgi returns with the comment whether the save worked or not. I this situation, the hours are displayed with a coma, as before the temporary fix. You only need to reload the page and the time is again displayed correctly with a dot.

I know this might not the be the way to go to solve the issue, but just thought I would share my experience. Thanks for the update, Marc.

Comment 34

[Otto]

Isn't there any expert to go throug the source files of bugilla (version 4.4.11) and change all data types in "Orig.Est." "Current Est" "Hours worked" "Hours left" from floating point to integer. I do not need those time fields but when I did, why should I work with times smaller than one hour ??????

Comment 35

[Otto]

Maybe this interests no one (there was no response to my requests to Bugzilla experts who called themselves as developer. I said, I pay for the solution .... no interest), so I as a non-expert, don't know nothing about perl, found my solution.
The file
\\bugzilla\template\en\default\bug\time.html.tmpl

with the lines:

  [% time_unit = time_unit FILTER format('%.2f') %]
  [% IF time_unit.match('0\Z') %]
    [% time_unit FILTER format('%.1f') %]
  [% ELSE %]
    [% time_unit FILTER format('%.2f') %] 

were changed to:

  [% time_unit = time_unit FILTER format('%.0f') %]
  [% IF time_unit.match('0\Z') %]
    [% time_unit FILTER format('%.0f') %]
  [% ELSE %]
    [% time_unit FILTER format('%.0f') %]

with the result, that no decimal place is showed any more, no komma and no semicolon. Now you only can work with full hours. When you give in 1.2 hours, the software increases to 2 hours.

Comment 36

[Dylan Hardison [:dylan] (he/him)]

Hi! I'm the assistant project leader for Bugzilla -- since early this year -- I'm really sorry this bug hasn't been fixed.

I could write a whole analysis of why this bug is open for years, and how we can do better but that's not valuable
to you, so instead I'll just describe how to fix this -- and I'm also going to ask someone that may have time
to work on it. If they can't, I'll work on it as soon as I have a spare moment.

Perl is perfectly capable of handling German (or other european) numbers
if you tell it to. 

perl -E 'use locale; use POSIX qw(:locale_h); setlocale(LC_NUMERIC, "de_DE"); say "434,4" + 1'
435,4

If we want to take in European-style numbers and output the English ones:

LC_NUMERIC=de_DE perl -E 'use POSIX qw(:locale_h); setlocale(LC_NUMERIC, ""); my $i; { use locale; $i = "434,4" + 1; }; say $i'
435.4

1) we need to call setlocale(LC_NUMERIC, "") at bugzilla startup time.
2) We can convert numbers based on the locale, probably in Bugzilla/Bug.pm's validators

Comment 37

[Dylan Hardison [:dylan] (he/him)]

Rough outline of what to do:

Add unformat_number() to https://github.com/bugzilla/bugzilla/blob/master/Bugzilla/Util.pm


sub unformat_number {
    my ($number) = @_;
    return 0 + $number;
}

Add to https://github.com/bugzilla/bugzilla/blob/master/Bugzilla/Bug.pm#L162-L164
an elsif ($field->is_numeric) {
   $validator = \&_check_numeric_field;
}


Add also to that file a sub _check_numeric_field { } based
on the _check_integer_field(), but instead of checking if it is an integer, just use unformat_number() from Util.pm (defined above).

Comment 38

[olga-github]

Attachment: attempt1

Comment 39

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8795002 [details] [diff] [review]
attempt1

Review of attachment 8795002 [details] [diff] [review]:
-----------------------------------------------------------------

patch attachment success! Now just need to add the missing parts.

::: Bugzilla/Bug.pm
@@ +162,5 @@
>          elsif ($field->type == FIELD_TYPE_INTEGER) {
>              $validator = \&_check_integer_field;
>          }
> +        elsif ($field->is_numeric) {
> +   $validator = \&_check_numeric_field;

fix indentation

::: Bugzilla/Util.pm
@@ +864,5 @@
>      return $encoding;
>  }
>  
> +sub unformat_number {
> +    my ($number) = @_;

add use locale; here

Comment 40

[olga-github]

Attachment: Attempt2.patch

Comment 41

[olga-github]

Attachment: attempt3.patch

Comment 42

[olga-github]

Attachment: attempt4.patch

-Fixed typos;
-Added _check_numeric_field function;

Comment 43

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8795419 [details] [diff] [review]
attempt4.patch

Review of attachment 8795419 [details] [diff] [review]:
-----------------------------------------------------------------

Meanwhile I'll be adding a patch soon that makes the JS not freak out about the commas.

::: Bugzilla/Util.pm
@@ +864,3 @@
>      return $encoding;
>  }
>  

You're almost there. 
You need to 'export' this function by adding it to the @EXPORT
array at the top of this file

Comment 44

[Dylan Hardison [:dylan] (he/him)]

The reason for this is that

Bugzilla/Bug.pm needs to have the functions defined in Bugzilla/Util.pm,
the way of doing that is to add the names to the @EXPORT in Bugzilla/Util.pm,
and then when another file does "use Bugzilla::Util" it gets all the functions defined in @EXPORT.

Comment 45

[olga-github]

Attachment: attempt5.patch

FIXED: 
+ added unformat_number to @EXPORT in Util.pm

Comment 46

[David Lawrence [:dkl]]

Comment on attachment 8796231 [details] [diff] [review]
attempt5.patch

Review of attachment 8796231 [details] [diff] [review]:
-----------------------------------------------------------------

the timetracking fields are checked by Bugzilla::Object::check_time and subsequently Bugzilla::Object::_validate_time. _validate_time() checks with 
regex /^-?(?:\d+(?:\.\d*)?|\.\d+)$/ which disallows 5,0 for example. So we need to either use a different default validator for time tracking fields
or update the regex to something will work with the different numeric formats based on locale.

::: Bugzilla/Bug.pm
@@ +163,5 @@
>              $validator = \&_check_integer_field;
>          }
> +        elsif ($field->is_numeric) {
> +            $validator = \&_check_numeric_field;
> +        }

This will only execute for custom fields and not fields such as estimated_time and remaining_time which is what this bug was originally about.

::: Bugzilla/Util.pm
@@ +864,4 @@
>      return $encoding;
>  }
>  
> +sub unformat_number {

Seems like it would be better named format_number instead.

Comment 47

[olga-github]

Comment on attachment 8796231 [details] [diff] [review]
attempt5.patch

Review of attachment 8796231 [details] [diff] [review]:
-----------------------------------------------------------------

| the timetracking fields are checked by Bugzilla::Object::check_time and subsequently Bugzilla::Object::_validate_time.

What is the difference between them? Does "subsequently" mean I have to use both of them here?

|  _validate_time() checks with regex /^-?(?:\d+(?:\.\d*)?|\.\d+)$/ which disallows 5,0 for example.

OK, this looks gorgeous but I don't understand this regex yet - I am new to Perl. I'll read up on regular expressions. 

| So we need to either use a different default validator for time tracking fields or update the regex to something will work with the different numeric formats based on locale.

What is better in the case, in your opinion? Which one would be easier to employ here?

| Bugzilla/Bug.pm
|             $validator = \&_check_integer_field;
|        }
|        elsif ($field->is_numeric) {
|             $validator = \&_check_numeric_field;
|        }
|	
| This will only execute for custom fields and not fields such as estimated_time and remaining_time which is what this bug was originally about.
How could I apply it on estimated_time and remaining_time ? 

| Bugzilla/Util.pm
|    return $encoding;
| }
 
| sub unformat_number {
	
| Seems like it would be better named format_number instead.

OK, will be done!

Comment 48

[David Lawrence [:dkl]]

(In reply to olga-github from comment #47)
> Comment on attachment 8796231 [details] [diff] [review]
> attempt5.patch
> 
> Review of attachment 8796231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> | the timetracking fields are checked by Bugzilla::Object::check_time and
> subsequently Bugzilla::Object::_validate_time.
> 
> What is the difference between them? Does "subsequently" mean I have to use
> both of them here?

It is done this way since _validate_time can be used for other purposes unrelated to checking time tracking values.

> |  _validate_time() checks with regex /^-?(?:\d+(?:\.\d*)?|\.\d+)$/ which
> disallows 5,0 for example.
> 
> OK, this looks gorgeous but I don't understand this regex yet - I am new to
> Perl. I'll read up on regular expressions. 
> 
> | So we need to either use a different default validator for time tracking
> fields or update the regex to something will work with the different numeric
> formats based on locale.
> 
> What is better in the case, in your opinion? Which one would be easier to
> employ here?

Actually just updating the regexp to allow , and . would be the easiest fix for this.

Maybe:

if ($time !~ /^-?(?:\d+(?:(\.|,)\d*)?|(\.|,)\d+)$/) {

> | Bugzilla/Bug.pm
> |             $validator = \&_check_integer_field;
> |        }
> |        elsif ($field->is_numeric) {
> |             $validator = \&_check_numeric_field;
> |        }
> |	
> | This will only execute for custom fields and not fields such as
> estimated_time and remaining_time which is what this bug was originally
> about.
> How could I apply it on estimated_time and remaining_time ? 

remaining_time and estimated_time have their own validator already in Bugzilla/Bug.pm. 

Bugzilla::Bug::_check_time_field() => Bugzilla::Object::check_time() => Bugzilla::Object::_validate_time()

dkl

Comment 49

[D.Dierickx]

Hi guys, this is my approach. I'm new to perl, but not at all new to development. So please bare with me.
My solution/patch works fine at my end (so far... ;)  ) , but it's possible that there are issues/bugs lurking around in other places. (and it's probably also not a "clean" patch, no checks)

Can you please verify, thanks.

The only things to do are in the file "Bug.pm" , there are 2 lines to change.
I store the values in a local var, replace all "," by "." and return the value. As said, no checks, possible issues.

1) Line 2629
Change: "sub set_estimated_time { $_[0]->set('estimated_time', $_[1]); }"
Into  : "sub set_estimated_time { my $et = $_[1]; $et =~ s/,/./g; $_[0]->set('estimated_time', $et); }"

2) Line 2798
Change: "sub set_remaining_time { $_[0]->set('remaining_time', $_[1]); }"
Into  : "sub set_remaining_time { my $rt = $_[1]; $rt =~ s/,/./g; $_[0]->set('remaining_time', $rt); }"

Comment 50

[D.Dierickx]

(In reply to D.Dierickx from comment #49)
To be complete, i performed this change on version 5.0.3

Comment 51

[GitHub Autolander Bot]

Attachment: [bugzilla] DierickxD71:patch-1 > bugzilla:master

Comment 52

[David Lawrence [:dkl]]

Comment on attachment 8808602 [details] [review]
[bugzilla] DierickxD71:patch-1 > bugzilla:master

r=dkl

Comment 53

[David Lawrence [:dkl]]

To https://github.com/bugzilla/bugzilla.git
   3c60fba..b4a06dd  master -> master