Bug 34169 - Add validation for monetary input fields in acquisition module
Summary: Add validation for monetary input fields in acquisition module
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Katrin Fischer
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 35892
  Show dependency treegraph
 
Reported: 2023-06-30 09:27 UTC by Katrin Fischer
Modified: 2024-01-24 13:37 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
At the moment Koha can only calculate with amounts that are formatted with a decimal comma, but inputs were not always validated which could lead to errors and wrong amounts. Now entered amounts are validated before saving throughout the acquisitions module. **Sponsored by** *The Research University in the Helmholtz Association (KIT)*
Version(s) released in:
23.11.00,23.05.03


Attachments
Bug 34169: Use jQuery validator plugin to validate amounts (2.17 KB, patch)
2023-07-17 16:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module (83.56 KB, patch)
2023-07-17 16:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34169: Use jQuery validator plugin to validate amounts (2.22 KB, patch)
2023-07-17 20:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module (83.61 KB, patch)
2023-07-17 20:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 34169: (follow-up) Fix ordering from staged files by removing superfluous form (52.99 KB, patch)
2023-07-18 07:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34169: Use jQuery validator plugin to validate amounts (2.22 KB, patch)
2023-07-18 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module (83.61 KB, patch)
2023-07-18 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34169: (follow-up) Fix ordering from staged files by removing superfluous form (52.99 KB, patch)
2023-07-18 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34169: Use jQuery validator plugin to validate amounts (2.28 KB, patch)
2023-07-21 15:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module (83.68 KB, patch)
2023-07-21 15:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34169: (follow-up) Fix ordering from staged files by removing superfluous form (53.04 KB, patch)
2023-07-21 15:38 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-06-30 09:27:47 UTC
In the patron module we validate the input fields for monetary values using a pattern check 
(see https://wiki.koha-community.org/wiki/Coding_Guidelines#ACC2:_Input_type_.22number.22_should_be_avoided), but we don't do the same in acq.

Especially for countries that use decimal comma this can lead to problems. At the moment Koha only accepts decimal point and a wrongly entered value might get turned from 1,00 to 100 or similar easily. 

Until we can fix the input formats properly, we should extend the pattern check to fields in the acq module I think.

Also: We can use the pattern check later to identify any fields that needs work for different input formats.
Comment 1 Katrin Fischer 2023-07-07 16:41:13 UTC
This could be a quick fix while we work on 34231. 

Joubu suggested using a CSS class and adding the pattern check with jQuery in the global .js file.
Comment 2 Katrin Fischer 2023-07-11 12:38:49 UTC
I tried to write a POC using jQuery starting with the normal acq order form:

$("#acq_neworderempty #unitprice").attr({inputmode: "decimal", pattern: "^\d+(\.\d{2})?$" });

It didn't work there, because we use the validate JS plugin.

But we could maybe use that to our advantage. With Joubu's help:

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
index 42f537a5924..7f0be4c0af2 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
@@ -669,7 +669,7 @@
             </li>
             <li class="ordering_unitprice">
                 <label for="unitprice">Actual cost: </label>
-                <input type="text" id="unitprice" size="20" name="unitprice" value="[% unitprice | html %]" />
+                <input type="text" id="unitprice" size="20" name="unitprice" value="[% unitprice | html %]" class="price" />
                 [% IF (invoiceincgst == 1) %](tax inc.)[% ELSE %](tax exc.)[% END %]
             </li>
             <li>
diff --git a/koha-tmpl/intranet-tmpl/prog/js/staff-global.js b/koha-tmpl/intranet-tmpl/prog/js/staff-global.js
index 74af1ad89ae..aeab2be8667 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/staff-global.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/staff-global.js
@@ -110,6 +110,9 @@ $(document).ready(function() {
     $(".validated").each(function() {
         $(this).validate();
     });
+    jQuery.validator.addClassRules("price", {
+        number: true
+    });
 
     $("#logout").on("click",function(){
         logOut();

* It allows to add negative and positive amounts with decimal dot. This is OK as we do need the ability to add negatives in acquisitions. 
* It does allow entering thousand separators with a ., but Koha hates it and won't save the value:

check_budget_total.pl:
XML Parsing Error: syntax error
Location: http://localhost:8081/cgi-bin/koha/acqui/check_budget_total.pl?budget_id=2&total=21.60
Line Number 1, Column 1:
Comment 3 Katrin Fischer 2023-07-17 16:23:59 UTC
Created attachment 153568 [details] [review]
Bug 34169: Use jQuery validator plugin to validate amounts

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It allows us to mark all
input fields for monetary values, such as prices, replacement prices,
fees etc. with a class that is linked to a check for the 'number' format
in the jQuery Validator plugin.

This is the base patch that does nothing by itself, please see
test plan in second patch.
Comment 4 Katrin Fischer 2023-07-17 16:24:02 UTC
Created attachment 153569 [details] [review]
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It marks all input fields
for monetary values, such as prices, replacement prices etc. with a class
that is linked to a check for number format with the jQuery Validator plugin.

To test:

For any input field to test, try adding various false entries, like "abc" or "1,00".
It should only accept inputs with decimal dot, like: "1.00"

0) Apply patch, restart_all
1) Suggestion
  * Add a new suggestion in the staff interface
  * Test: price input field at the bottom of the form.
  * Accept the suggestion
2) Order form
  * Create a new basket
  * Create an order line from an existing record
  * Test: list price, replacement price, and actual price.
  * Check the checkbox for uncertain price before you save
3) Uncertain prices
  * Go to the uncertain prices page for this vendor
  * Test: price field
    Note: this form does its own validation, but the change should not change behaviour for now
  * Resolve the uncertain price
  * Close order
4) Receive shipment
  * Test: Shipping cost
5) Receive the order
  * Test: replacement price, actual price
  * Check checkbox for price in foreign currency
  * Test: price in foreign currency
  * Receive order line
6) Invoice summary
  * Finish receiving
  * Test: shipping cost
  * Test: invoice adjustments: amount in the form for the first entry, amount in the table after adding it
7) Merging invoices
  * Receive another shipment and create and invoice
  * Go to invoices and search all
  * Check the 2 entries for merging
  * Test: shipping cost
8) Adding orders from a staged/new file
  * Export some records using the cart or list
  * Create a new basket
  * Order from new file
  * Import your file, ignore item records
  * Test: price and replacement price
  + Bonus: also test with items, test plan and file from bug 22802 are really helpful here
Comment 5 PTFS Europe Sandboxes 2023-07-17 20:36:16 UTC
Created attachment 153577 [details] [review]
Bug 34169: Use jQuery validator plugin to validate amounts

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It allows us to mark all
input fields for monetary values, such as prices, replacement prices,
fees etc. with a class that is linked to a check for the 'number' format
in the jQuery Validator plugin.

This is the base patch that does nothing by itself, please see
test plan in second patch.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 6 PTFS Europe Sandboxes 2023-07-17 20:36:19 UTC
Created attachment 153578 [details] [review]
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It marks all input fields
for monetary values, such as prices, replacement prices etc. with a class
that is linked to a check for number format with the jQuery Validator plugin.

To test:

For any input field to test, try adding various false entries, like "abc" or "1,00".
It should only accept inputs with decimal dot, like: "1.00"

0) Apply patch, restart_all
1) Suggestion
  * Add a new suggestion in the staff interface
  * Test: price input field at the bottom of the form.
  * Accept the suggestion
2) Order form
  * Create a new basket
  * Create an order line from an existing record
  * Test: list price, replacement price, and actual price.
  * Check the checkbox for uncertain price before you save
3) Uncertain prices
  * Go to the uncertain prices page for this vendor
  * Test: price field
    Note: this form does its own validation, but the change should not change behaviour for now
  * Resolve the uncertain price
  * Close order
4) Receive shipment
  * Test: Shipping cost
5) Receive the order
  * Test: replacement price, actual price
  * Check checkbox for price in foreign currency
  * Test: price in foreign currency
  * Receive order line
6) Invoice summary
  * Finish receiving
  * Test: shipping cost
  * Test: invoice adjustments: amount in the form for the first entry, amount in the table after adding it
7) Merging invoices
  * Receive another shipment and create and invoice
  * Go to invoices and search all
  * Check the 2 entries for merging
  * Test: shipping cost
8) Adding orders from a staged/new file
  * Export some records using the cart or list
  * Create a new basket
  * Order from new file
  * Import your file, ignore item records
  * Test: price and replacement price
  + Bonus: also test with items, test plan and file from bug 22802 are really helpful here

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 7 Michaela Sieber 2023-07-17 20:43:14 UTC
Could please someone check

8) Adding orders from a staged/new file


I tested it in 
http://staff-pricevalid.sandboxes.ptfs-europe.co.uk/cgi-bin/koha/acqui/basket.pl?basketno=4

but the prices are not stored, the price is always 0.00

I'm not sure if I did everything correctly.

Perhaps this is a separate bug and has nothing to do with the validation.

1) - 7) Signed Off
Comment 8 Katrin Fischer 2023-07-18 07:12:27 UTC
(In reply to Michaela Sieber from comment #7)
> Could please someone check
> 
> 8) Adding orders from a staged/new file
> 
> 
> I tested it in 
> http://staff-pricevalid.sandboxes.ptfs-europe.co.uk/cgi-bin/koha/acqui/
> basket.pl?basketno=4
> 
> but the prices are not stored, the price is always 0.00
> 
> I'm not sure if I did everything correctly.
> 
> Perhaps this is a separate bug and has nothing to do with the validation.
> 
> 1) - 7) Signed Off

Hi Michaela,

could it have been:
Bug 18914 - 'Add order' links from staged file order information isn't carried over

It happens when you use the order link to add a single order line insted of using the checkboxes and the button at the bottom.
Comment 9 Katrin Fischer 2023-07-18 07:22:12 UTC
I think there is something wrong still with the multi-select. If I pick more than one, only the price of the first is carried over into the order. I'll have a look.
Comment 10 Katrin Fischer 2023-07-18 07:32:18 UTC
Created attachment 153580 [details] [review]
Bug 34169: (follow-up) Fix ordering from staged files by removing superfluous form

Removes the unneded new form element as we have one big form for the whole page.

This should fix the situation where only the prices and information
of the first selected record carreid over into the order.
Comment 11 Katrin Fischer 2023-07-18 07:34:05 UTC
Created attachment 153581 [details] [review]
Bug 34169: Use jQuery validator plugin to validate amounts

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It allows us to mark all
input fields for monetary values, such as prices, replacement prices,
fees etc. with a class that is linked to a check for the 'number' format
in the jQuery Validator plugin.

This is the base patch that does nothing by itself, please see
test plan in second patch.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 12 Katrin Fischer 2023-07-18 07:34:07 UTC
Created attachment 153582 [details] [review]
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It marks all input fields
for monetary values, such as prices, replacement prices etc. with a class
that is linked to a check for number format with the jQuery Validator plugin.

To test:

For any input field to test, try adding various false entries, like "abc" or "1,00".
It should only accept inputs with decimal dot, like: "1.00"

0) Apply patch, restart_all
1) Suggestion
  * Add a new suggestion in the staff interface
  * Test: price input field at the bottom of the form.
  * Accept the suggestion
2) Order form
  * Create a new basket
  * Create an order line from an existing record
  * Test: list price, replacement price, and actual price.
  * Check the checkbox for uncertain price before you save
3) Uncertain prices
  * Go to the uncertain prices page for this vendor
  * Test: price field
    Note: this form does its own validation, but the change should not change behaviour for now
  * Resolve the uncertain price
  * Close order
4) Receive shipment
  * Test: Shipping cost
5) Receive the order
  * Test: replacement price, actual price
  * Check checkbox for price in foreign currency
  * Test: price in foreign currency
  * Receive order line
6) Invoice summary
  * Finish receiving
  * Test: shipping cost
  * Test: invoice adjustments: amount in the form for the first entry, amount in the table after adding it
7) Merging invoices
  * Receive another shipment and create and invoice
  * Go to invoices and search all
  * Check the 2 entries for merging
  * Test: shipping cost
8) Adding orders from a staged/new file
  * Export some records using the cart or list
  * Create a new basket
  * Order from new file
  * Import your file, ignore item records
  * Test: price and replacement price
  + Bonus: also test with items, test plan and file from bug 22802 are really helpful here

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 13 Katrin Fischer 2023-07-18 07:34:10 UTC
Created attachment 153583 [details] [review]
Bug 34169: (follow-up) Fix ordering from staged files by removing superfluous form

Removes the unneded new form element as we have one big form for the whole page.

This should fix the situation where only the prices and information
of the first selected record carreid over into the order.
Comment 14 Katrin Fischer 2023-07-18 07:39:21 UTC
I think I found an fixed it for the checkboxes/multi-select.

Bug 18914 is not fixed by it.
Comment 15 Nick Clemens 2023-07-21 15:38:31 UTC
Created attachment 153801 [details] [review]
Bug 34169: Use jQuery validator plugin to validate amounts

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It allows us to mark all
input fields for monetary values, such as prices, replacement prices,
fees etc. with a class that is linked to a check for the 'number' format
in the jQuery Validator plugin.

This is the base patch that does nothing by itself, please see
test plan in second patch.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2023-07-21 15:38:34 UTC
Created attachment 153802 [details] [review]
Bug 34169: Add decimal class to all relevant input fields in the acquisitions module

This is a first step towards more consistency and possibly supporting
multiple input formats as well in the future. It marks all input fields
for monetary values, such as prices, replacement prices etc. with a class
that is linked to a check for number format with the jQuery Validator plugin.

To test:

For any input field to test, try adding various false entries, like "abc" or "1,00".
It should only accept inputs with decimal dot, like: "1.00"

0) Apply patch, restart_all
1) Suggestion
  * Add a new suggestion in the staff interface
  * Test: price input field at the bottom of the form.
  * Accept the suggestion
2) Order form
  * Create a new basket
  * Create an order line from an existing record
  * Test: list price, replacement price, and actual price.
  * Check the checkbox for uncertain price before you save
3) Uncertain prices
  * Go to the uncertain prices page for this vendor
  * Test: price field
    Note: this form does its own validation, but the change should not change behaviour for now
  * Resolve the uncertain price
  * Close order
4) Receive shipment
  * Test: Shipping cost
5) Receive the order
  * Test: replacement price, actual price
  * Check checkbox for price in foreign currency
  * Test: price in foreign currency
  * Receive order line
6) Invoice summary
  * Finish receiving
  * Test: shipping cost
  * Test: invoice adjustments: amount in the form for the first entry, amount in the table after adding it
7) Merging invoices
  * Receive another shipment and create and invoice
  * Go to invoices and search all
  * Check the 2 entries for merging
  * Test: shipping cost
8) Adding orders from a staged/new file
  * Export some records using the cart or list
  * Create a new basket
  * Order from new file
  * Import your file, ignore item records
  * Test: price and replacement price
  + Bonus: also test with items, test plan and file from bug 22802 are really helpful here

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2023-07-21 15:38:36 UTC
Created attachment 153803 [details] [review]
Bug 34169: (follow-up) Fix ordering from staged files by removing superfluous form

Removes the unneded new form element as we have one big form for the whole page.

This should fix the situation where only the prices and information
of the first selected record carreid over into the order.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2023-07-21 15:39:29 UTC
There are places where you end up with NaN when you enter invalid format, but you cannot submit and this seems a large improvement, passing QA
Comment 19 Tomás Cohen Arazi 2023-07-24 17:56:01 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Michaela Sieber 2023-07-28 20:09:38 UTC
Thank you!  Is backporting for 23.05.x  possible ?
Comment 21 Fridolin Somers 2023-08-04 02:22:36 UTC
Long lasting bug I gladly backport.

Pushed to 23.05.x for 23.05.03
Comment 22 Pedro Amorim 2023-08-18 11:49:56 UTC
Missing dependencies for 22.11.x. Not pushing.