Bug 35087 - Discount rate should only allow valid input formats
Summary: Discount rate should only allow valid input formats
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial
Assignee: Thibaud Guillot (thibaud_g)
QA Contact: Marcel de Rooy
URL:
Keywords: Sandbox
Depends on:
Blocks:
 
Reported: 2023-10-18 08:56 UTC by Thibaud Guillot (thibaud_g)
Modified: 2024-12-16 03:58 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the vendor discount field so that it now only accepts values that will save correctly: - a decimal number in the format 0.0 (if a comma is entered, you are now prompted to enter a decimal point) - numbers with up to two digits before the decimal and up to three digits after the decimal, for example: 9, 99, -99, 99.9, 0.99, 99.99, 99.999
Version(s) released in:
24.11.00,24.05.06,23.11.11
Circulation function:


Attachments
Bug 35087: Discount rate only accepts a point as decimal separator (2.40 KB, patch)
2023-10-18 09:02 UTC, Thibaud Guillot (thibaud_g)
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 (thibaud_g)
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 (thibaud_g)
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 (ashimema)
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 (thibaud_g)
Details | Diff | Splinter Review
Bug 35087: alternative patch to display validation (1.55 KB, patch)
2024-01-29 14:54 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 35087: alternative patch to display validation (960 bytes, patch)
2024-01-29 14:57 UTC, Thibaud Guillot (thibaud_g)
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 (thibaud_g)
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 (thibaud_g)
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.56 KB, patch)
2024-06-14 15:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35087: (follow-up) Accept only values which will be saved correctly (4.25 KB, patch)
2024-06-14 15:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.61 KB, patch)
2024-09-09 13:45 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 35087: (follow-up) Accept only values which will be saved correctly (4.31 KB, patch)
2024-09-09 13:45 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 35087: Discount rate only accepts a point as decimal separator (3.67 KB, patch)
2024-09-16 18:47 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35087: (follow-up) Accept only values which will be saved correctly (4.37 KB, patch)
2024-09-16 18:47 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thibaud Guillot (thibaud_g) 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 (thibaud_g) 2023-10-18 09:02:47 UTC Comment hidden (obsolete)
[+] Comment 2 David Nind 2023-10-23 02:41:32 UTC Comment hidden (obsolete)
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 (thibaud_g) 2023-11-14 14:01:34 UTC Comment hidden (obsolete)
Comment 5 Thibaud Guillot (thibaud_g) 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 Comment hidden (obsolete)
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 (thibaud_g) 2024-01-18 08:30:27 UTC Comment hidden (obsolete)
Comment 11 Thibaud Guillot (thibaud_g) 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 Comment hidden (obsolete)
[+] Comment 13 Martin Renvoize (ashimema) 2024-01-22 12:46:35 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize (ashimema) 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 (thibaud_g) 2024-01-29 14:09:37 UTC Comment hidden (obsolete)
Comment 17 Thibaud Guillot (thibaud_g) 2024-01-29 14:10:36 UTC
Hello Katrin,

Thanks ;) now normally it's done
[+] Comment 18 Thibaud Guillot (thibaud_g) 2024-01-29 14:54:25 UTC Comment hidden (obsolete)
[+] Comment 19 Thibaud Guillot (thibaud_g) 2024-01-29 14:57:03 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2024-01-29 20:31:08 UTC
Are all 3 patches needed?
Comment 21 Thibaud Guillot (thibaud_g) 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 (thibaud_g) 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 Comment hidden (obsolete)
[+] Comment 25 Owen Leonard 2024-01-31 17:27:22 UTC Comment hidden (obsolete)
[+] Comment 26 Owen Leonard 2024-01-31 17:27:25 UTC Comment hidden (obsolete)
[+] Comment 27 PTFS Europe Sandboxes 2024-02-12 17:49:20 UTC Comment hidden (obsolete)
[+] Comment 28 PTFS Europe Sandboxes 2024-02-12 17:49:23 UTC Comment hidden (obsolete)
[+] Comment 29 PTFS Europe Sandboxes 2024-02-12 17:49:25 UTC Comment hidden (obsolete)
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 (thibaud_g) 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 (thibaud_g) 2024-04-11 14:27:47 UTC Comment hidden (obsolete)
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 (thibaud_g) 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).
Comment 37 Owen Leonard 2024-06-14 15:57:47 UTC
Created attachment 167746 [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 38 Owen Leonard 2024-06-14 15:57:49 UTC
Created attachment 167747 [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 39 Owen Leonard 2024-06-14 16:01:47 UTC
Rebased on current main. It looks like it should be "Signed off" but there seems to be some question about whether it's working?
Comment 40 David Nind 2024-06-15 07:23:33 UTC
It looks like it is working to me.

However, these values are rounded when displayed after saving (when edited again it is the value that was entered):

- 0.99 => 1.0%
- 99.9, 99.99, 99.999 => 100.0%
Comment 41 Sylvain Def. 2024-06-19 13:14:47 UTC
Hi, 

It still doesn't work for me with trying to save value with a comma in it as a separator. 
Only numbers separated with a dot are saved
When I use a comma, I get a "Please enter a decimal number in the format: 0.0" message.
I'm using a french Firefox version, btw. 
Looks like it's a french problem with commas used as a dot (as common in France).
Comment 42 Katrin Fischer 2024-06-19 13:16:44 UTC
(In reply to Sylvain Def. from comment #41)
> Hi, 
> 
> It still doesn't work for me with trying to save value with a comma in it as
> a separator. 
> Only numbers separated with a dot are saved
> When I use a comma, I get a "Please enter a decimal number in the format:
> 0.0" message.
> I'm using a french Firefox version, btw. 
> Looks like it's a french problem with commas used as a dot (as common in
> France).

I think it should not work with a comma, that is the point. Koha doesn't support other input formats yet.
Comment 43 Katrin Fischer 2024-06-19 13:18:08 UTC
(In reply to Katrin Fischer from comment #42)
> (In reply to Sylvain Def. from comment #41)
> > Hi, 
> > 
> > It still doesn't work for me with trying to save value with a comma in it as
> > a separator. 
> > Only numbers separated with a dot are saved
> > When I use a comma, I get a "Please enter a decimal number in the format:
> > 0.0" message.
> > I'm using a french Firefox version, btw. 
> > Looks like it's a french problem with commas used as a dot (as common in
> > France).
> 
> I think it should not work with a comma, that is the point. Koha doesn't
> support other input formats yet.

To explain: the patch makes it impossible to input data that would be invalid. The problem with different input formats needs to be solved separatly.
Comment 44 Olivier Vezina 2024-09-09 13:45:44 UTC
Created attachment 171197 [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>
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Comment 45 Olivier Vezina 2024-09-09 13:45:47 UTC
Created attachment 171198 [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>
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Comment 46 Nick Clemens (kidclamp) 2024-09-16 18:47:20 UTC
Created attachment 171594 [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>
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 47 Nick Clemens (kidclamp) 2024-09-16 18:47:23 UTC
Created attachment 171595 [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>
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 48 Martin Renvoize (ashimema) 2024-10-02 09:33:52 UTC
Wow.. Loads of signoffs.. this is a popular one!
Comment 49 Martin Renvoize (ashimema) 2024-10-02 09:56:02 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 50 Lucas Gass (lukeg) 2024-11-18 22:33:32 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 51 Fridolin Somers 2024-12-11 13:52:25 UTC
Pushed to 23.11.x for 23.11.11