Bug 35087 - Discount rate does not accept comma as decimal separator
Summary: Discount rate does not accept comma as decimal separator
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Thibaud Guillot
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-18 08:56 UTC by Thibaud Guillot
Modified: 2024-05-10 21:32 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35087: Discount rate only accepts a point as decimal separator (2.40 KB, patch)
2023-10-18 09:02 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (2.45 KB, patch)
2023-10-23 02:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.66 KB, patch)
2023-11-14 14:01 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.56 KB, patch)
2023-12-14 11:09 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.34 KB, patch)
2024-01-18 08:30 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.39 KB, patch)
2024-01-21 20:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.46 KB, patch)
2024-01-22 12:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35087: (follow-up) string validation is now translatable (1.41 KB, patch)
2024-01-29 14:09 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35087: alternative patch to display validation (1.55 KB, patch)
2024-01-29 14:54 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35087: alternative patch to display validation (960 bytes, patch)
2024-01-29 14:57 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.51 KB, patch)
2024-01-31 17:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35087: alternative patch to display validation (1017 bytes, patch)
2024-01-31 17:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35087: (follow-up) Accept only values which will be saved correctly (4.32 KB, patch)
2024-01-31 17:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.56 KB, patch)
2024-02-12 17:49 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35087: alternative patch to display validation (1.04 KB, patch)
2024-02-12 17:49 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35087: (follow-up) Accept only values which will be saved correctly (4.37 KB, patch)
2024-02-12 17:49 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.56 KB, patch)
2024-04-11 14:27 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35087: (follow-up) Accept only values which will be saved correctly (4.26 KB, patch)
2024-04-11 14:27 UTC, Thibaud Guillot
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thibaud Guillot 2023-10-18 08:56:43 UTC
Hello all, 

When a comma is used to define the discount rate of a vendor, the data is not recorded (as NULL), it is however recorded if the separator is a point. 

In the same way that the BZ rectifying the problems linked to the payment of fines (bug 33028) obliges the input with a point I will propose a corrective in this same direction.
Comment 1 Thibaud Guillot 2023-10-18 09:02:47 UTC
Created attachment 157302 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
6) Repeat step 2 and see the warning to inform you that you must use a point
Comment 2 David Nind 2023-10-23 02:41:32 UTC
Created attachment 157626 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
6) Repeat step 2 and see the warning to inform you that you must use a point

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Katrin Fischer 2023-10-23 21:09:26 UTC
You removed the "validated" class from the form, this disables all standard validation that already was applied here. 

An examples: the mandatory field "Name" now uses the standard HTML5 message instead of our own. Email and website no longer validate at all. 

You need to keep the Validator active and implement your check additionally.
Comment 4 Thibaud Guillot 2023-11-14 14:01:34 UTC
Created attachment 158921 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
6) Repeat step 2 and see the warning to inform you that you must use a point
Comment 5 Thibaud Guillot 2023-11-14 14:04:22 UTC
(In reply to Katrin Fischer from comment #3)
> You removed the "validated" class from the form, this disables all standard
> validation that already was applied here. 
> 
> An examples: the mandatory field "Name" now uses the standard HTML5 message
> instead of our own. Email and website no longer validate at all. 
> 
> You need to keep the Validator active and implement your check additionally.

Hello Katrin,

Thanks for your comment, sorry for the error. I tried to fix it, but I kept the deleted class, and added a validation for the 'discount' input, I think it's correct for now, but if you see a problem, I'd be happy to hear from you.
Comment 6 Katrin Fischer 2023-11-14 22:03:06 UTC
I confirm that it does on longer break the other validation, but I am not sure if it's the right way to do it. Adding Owen in hope of some guidance.

Also: Your hint displays as:  % - Example: 7.5 % - that doesn't look quite right yet.

I wonder if using a standard class of Validator would be better here? We could use number like we do on Bug 34169 - Add validation for monetary input fields in acquisition module, but maybe with its own class and hint?

That would also make it reusable - the discount is also set on the order form and there might be other use cases like the search field weighting.
Comment 7 Biblibre Sandboxes 2023-12-14 11:09:42 UTC
Created attachment 159831 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
6) Repeat step 2 and see the warning to inform you that you must use a point

Signed-off-by: Sabrina <kiehl@mpis.mpg.de>
Comment 8 Sabrina Kiehl 2023-12-14 11:12:21 UTC
Tested following the mentioned steps, warning was shown correctly.
Comment 9 Owen Leonard 2023-12-19 16:56:32 UTC
(In reply to Katrin Fischer from comment #6)
> We
> could use number like we do on Bug 34169

I agree... There doesn't seem to be a reason not to. In similar cases we don't add any kind of hint (e.g. a <div class="hint"> under the input field), so I don't think we have to include that in this patch.
Comment 10 Thibaud Guillot 2024-01-18 08:30:27 UTC
Created attachment 161128 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
6) Repeat step 2 and see the warning to inform you that you must use a point
Comment 11 Thibaud Guillot 2024-01-18 08:33:35 UTC
Thanks for your feedback, I've modified the code to create a specific class for reuse it
Comment 12 David Nind 2024-01-21 20:20:46 UTC
Created attachment 161225 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
6) Repeat step 2 and see the warning to inform you that you must use a point

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Martin Renvoize 2024-01-22 12:46:35 UTC
Created attachment 161237 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
6) Repeat step 2 and see the warning to inform you that you must use a point

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2024-01-22 12:47:54 UTC
I thought we were attempting to go the other way generally.. i.e. allowing people to input in their local format and translating at our end.

This patch works though technically and resolves a problem so I'm happy to PQA.. at some point I'd really like to see a concerted effort to fix these formatting numerics issues globally.
Comment 15 Katrin Fischer 2024-01-29 09:54:41 UTC
Hi Thibaud, thanks for your perseverance!
Can you please change these lines a bit so that the string is translatable? Maybe assigning it to a variable first?

+            decimal_rate: $.validator.addMethod("decimal_rate", $.validator.methods.number, "Please add rate in valid format: 0.00"),
Comment 16 Thibaud Guillot 2024-01-29 14:09:37 UTC
Created attachment 161596 [details] [review]
Bug 35087: (follow-up) string validation is now translatable
Comment 17 Thibaud Guillot 2024-01-29 14:10:36 UTC
Hello Katrin,

Thanks ;) now normally it's done
Comment 18 Thibaud Guillot 2024-01-29 14:54:25 UTC
Created attachment 161597 [details] [review]
Bug 35087: alternative patch to display validation

Same test plan as for the other patch, but here validation impacts the js asset
Comment 19 Thibaud Guillot 2024-01-29 14:57:03 UTC
Created attachment 161598 [details] [review]
Bug 35087: alternative patch to display validation

Same test plan as for the other patch, but here validation impacts the js asset
Comment 20 Katrin Fischer 2024-01-29 20:31:08 UTC
Are all 3 patches needed?
Comment 21 Thibaud Guillot 2024-01-30 08:02:54 UTC
(In reply to Katrin Fischer from comment #20)
> Are all 3 patches needed?

Hello Katrin,

No, the third is only an alternative way to fix it without touch include (all is done in staff-global.js).
Comment 22 Katrin Fischer 2024-01-31 11:25:36 UTC
Hi Thibaud, what do we need here, additional feedback or are you still working on the patches?
Comment 23 Thibaud Guillot 2024-01-31 11:35:56 UTC
Sorry I should have been more exhaustive, the alternative patch doesn't modify the 'validator-string' include so I thought that in terms of integration it would be simpler to have just one modified file. Nevertheless, I'll let you "choose" the best solution for validating the patch and I'll change the solution not chosen to "obsolete".
Comment 24 Owen Leonard 2024-01-31 17:27:20 UTC
Created attachment 161704 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the
separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
 6) Repeat step 2 and see the warning to inform you that you must use a
    point

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 25 Owen Leonard 2024-01-31 17:27:22 UTC
Created attachment 161705 [details] [review]
Bug 35087: alternative patch to display validation

Same test plan as for the other patch, but here validation impacts the
js asset

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 26 Owen Leonard 2024-01-31 17:27:25 UTC
Created attachment 161706 [details] [review]
Bug 35087: (follow-up) Accept only values which will be saved correctly

This patch adds a regex (thanks to Katrin for the help) specific to the
range of values which will be accepted by the discount column in the
database which has the type "float(6,4)".

The patch also removes the format() filter from the template so that
values returned from the database won't be truncated:

Using "format ('%.1f')", a value in the db of '9.009' would appear in
the edit form as '9.0', a change which could be missed if the user was
editing some other field in the vendor edit form.

To test, apply the patch and clear your browser cache if necessary.

- Go to Acquisitions -> Vendors -> Edit vendor.
- Under "Ordering information," test entering various values in the
  "Discount" field. It should accept numbers with up to two digits
   before the decimal and up to three digits after the decimal:

   9, 99, -99, 99.9, 0.99, 99.99, 99.999

- In each of these cases, confirm that a value accepted by the
  client-side validation is also accepted when you submit the form and
  is correctly displayed when you edit that vendor again.

- It should not accept:

   100, 100.001, 100.00001

- The error message has been updated to read, "Please enter a decimal
  number in the format: 0.0"

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 27 PTFS Europe Sandboxes 2024-02-12 17:49:20 UTC
Created attachment 162065 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the
separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
 6) Repeat step 2 and see the warning to inform you that you must use a
    point

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Sonia <sonia.bouis@univ-lyon3.fr>
Comment 28 PTFS Europe Sandboxes 2024-02-12 17:49:23 UTC
Created attachment 162066 [details] [review]
Bug 35087: alternative patch to display validation

Same test plan as for the other patch, but here validation impacts the
js asset

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Sonia <sonia.bouis@univ-lyon3.fr>
Comment 29 PTFS Europe Sandboxes 2024-02-12 17:49:25 UTC
Created attachment 162067 [details] [review]
Bug 35087: (follow-up) Accept only values which will be saved correctly

This patch adds a regex (thanks to Katrin for the help) specific to the
range of values which will be accepted by the discount column in the
database which has the type "float(6,4)".

The patch also removes the format() filter from the template so that
values returned from the database won't be truncated:

Using "format ('%.1f')", a value in the db of '9.009' would appear in
the edit form as '9.0', a change which could be missed if the user was
editing some other field in the vendor edit form.

To test, apply the patch and clear your browser cache if necessary.

- Go to Acquisitions -> Vendors -> Edit vendor.
- Under "Ordering information," test entering various values in the
  "Discount" field. It should accept numbers with up to two digits
   before the decimal and up to three digits after the decimal:

   9, 99, -99, 99.9, 0.99, 99.99, 99.999

- In each of these cases, confirm that a value accepted by the
  client-side validation is also accepted when you submit the form and
  is correctly displayed when you edit that vendor again.

- It should not accept:

   100, 100.001, 100.00001

- The error message has been updated to read, "Please enter a decimal
  number in the format: 0.0"

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Sonia <sonia.bouis@univ-lyon3.fr>
Comment 30 Koha Team University Lyon 3 2024-02-12 17:56:49 UTC
Hi,
tested on a sandbox.
When we edit a vendor with a discount with no decimal (ex : 10), nothing is added when we saved it.
If we try to enter a number with a comma in the discount, a red message appears "Please enter a decimal number in the format: 0.0"

It's no more possible to make the discount not recorded because of a wrong input.
Comment 31 Marcel de Rooy 2024-03-15 10:31:07 UTC
             number: _("Please add amount in valid format: 0.00"),
+            decimal_rate: $.validator.addMethod("decimal_rate", $.validator.methods.number, "Please add rate in valid format: 0.00"),

Confusing

+                <input class="rate" type="text" size="6" id="discount" name="discount" value="[% discount | format ('%.1f') %]" />%</li>

.1f linked to 0.00 ?
Comment 32 Marcel de Rooy 2024-03-15 10:32:08 UTC
We have 3 patches. And later patches revert stuff from earlier ones.
Please squash, explain, fix ?
Comment 33 Thibaud Guillot 2024-04-11 14:27:45 UTC
Created attachment 164739 [details] [review]
Bug 35087: Discount rate only accepts a point as decimal separator

When a comma is used to define the discount rate of a vendor,
the data is not recorded (as NULL), it is however recorded if the
separator is a point.

Test plan:

1) Go to acqui module and edit a vendor
2) Set a discount rate with a comma as decimal separator
3) Edit again or request your database to see that there is no data
   saved
4) Do it again with a point as decimal and see it works
5) Apply this patch and reloard the build for the templates
 6) Repeat step 2 and see the warning to inform you that you must use a
    point

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Sonia <sonia.bouis@univ-lyon3.fr>
Comment 34 Thibaud Guillot 2024-04-11 14:27:47 UTC
Created attachment 164740 [details] [review]
Bug 35087: (follow-up) Accept only values which will be saved correctly

This patch adds a regex (thanks to Katrin for the help) specific to the
range of values which will be accepted by the discount column in the
database which has the type "float(6,4)".

The patch also removes the format() filter from the template so that
values returned from the database won't be truncated:

Using "format ('%.1f')", a value in the db of '9.009' would appear in
the edit form as '9.0', a change which could be missed if the user was
editing some other field in the vendor edit form.

To test, apply the patch and clear your browser cache if necessary.

- Go to Acquisitions -> Vendors -> Edit vendor.
- Under "Ordering information," test entering various values in the
  "Discount" field. It should accept numbers with up to two digits
   before the decimal and up to three digits after the decimal:

   9, 99, -99, 99.9, 0.99, 99.99, 99.999

- In each of these cases, confirm that a value accepted by the
  client-side validation is also accepted when you submit the form and
  is correctly displayed when you edit that vendor again.

- It should not accept:

   100, 100.001, 100.00001

- The error message has been updated to read, "Please enter a decimal
  number in the format: 0.0"

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Sonia <sonia.bouis@univ-lyon3.fr>
Comment 35 Koha Team University Lyon 3 2024-04-12 09:59:22 UTC
I just tested it again and it doesn't work.
If I put "4,5" in the discount rate of a vendor, then save and edit it again the discount rate hasn't been saved, the discount rate is empty.

Sonia
Comment 36 Thibaud Guillot 2024-04-15 08:03:43 UTC
(In reply to Koha Team University Lyon 3 from comment #35)
> I just tested it again and it doesn't work.
> If I put "4,5" in the discount rate of a vendor, then save and edit it again
> the discount rate hasn't been saved, the discount rate is empty.
> 
> Sonia

Normally this should not be possible. I just tested again, the validation of the form imposes a comma, did you rebuild the css ? (This is an option on the sandbox management interface).