Bug 33028 - Wrongly formatted monetary amounts in circulation rules break scripts and calculations
Summary: Wrongly formatted monetary amounts in circulation rules break scripts and cal...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Thibaud Guillot
QA Contact: Kyle M Hall
URL:
Keywords:
: 30672 33240 33359 (view as bug list)
Depends on:
Blocks: 32271
  Show dependency treegraph
 
Reported: 2023-02-21 16:02 UTC by Thibaud Guillot
Modified: 2023-10-18 08:57 UTC (History)
11 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:
23.11.00,23.05.03,22.11.09


Attachments
Bug 33028: Fix calculations around cronjob fines.pl (3.96 KB, patch)
2023-02-21 16:08 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 33028: Fix calculations around cronjob fines.pl (4.02 KB, patch)
2023-03-03 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Move monetary definition into hash (2.33 KB, patch)
2023-03-03 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Fix trailing 0 decimals (1.28 KB, patch)
2023-03-03 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Fix calculations around cronjob fines.pl (4.02 KB, patch)
2023-06-16 17:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Move monetary definition into hash (2.33 KB, patch)
2023-06-16 17:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Fix trailing 0 decimals (1.28 KB, patch)
2023-06-16 17:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee (1.10 KB, patch)
2023-06-16 17:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Add TT filters for Price and pattern checks to input fields (5.14 KB, patch)
2023-06-16 17:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Rewrite database update (4.10 KB, patch)
2023-06-16 17:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Fix calculations around cronjob fines.pl (4.02 KB, patch)
2023-06-21 10:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Move monetary definition into hash (2.33 KB, patch)
2023-06-21 10:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Fix trailing 0 decimals (1.28 KB, patch)
2023-06-21 10:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee (1.10 KB, patch)
2023-06-21 10:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Add TT filters for Price and pattern checks to input fields (5.14 KB, patch)
2023-06-21 10:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Rewrite database update (4.16 KB, patch)
2023-06-21 10:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Remove Price formatting as we are missing JS unformat (3.54 KB, patch)
2023-06-21 10:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Add unformat_price js function (1.65 KB, patch)
2023-06-27 15:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Revert "Bug 33028: Remove Price formatting as we are missing JS unformat" (3.19 KB, patch)
2023-06-27 15:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Apply unformat_price to decimal fields (1.47 KB, patch)
2023-06-27 15:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Fix calculations around cronjob fines.pl (4.02 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Move monetary definition into hash (2.33 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Fix trailing 0 decimals (1.28 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee (1.16 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Add TT filters for Price and pattern checks to input fields (5.20 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Rewrite database update (4.22 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Remove Price formatting as we are missing JS unformat (3.60 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Add unformat_price js function (1.72 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Revert "Bug 33028: Remove Price formatting as we are missing JS unformat" (3.25 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Apply unformat_price to decimal fields (1.53 KB, patch)
2023-06-27 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Add unformat_price js function (1.74 KB, patch)
2023-06-28 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Revert "Bug 33028: Remove Price formatting as we are missing JS unformat" (3.25 KB, patch)
2023-06-28 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Apply unformat_price to decimal fields (1.66 KB, patch)
2023-06-28 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Remove inputmode and pattern (4.27 KB, patch)
2023-06-28 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Fix calculations around cronjob fines.pl (4.02 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Move monetary definition into hash (2.33 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Fix trailing 0 decimals (1.28 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee (1.16 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Add TT filters for Price and pattern checks to input fields (5.20 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Rewrite database update (4.22 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Add unformat_price js function (1.74 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Apply unformat_price to decimal fields (1.66 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Throw exception if not passed a decimal number (1.15 KB, patch)
2023-07-03 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Fix calculations around cronjob fines.pl (4.02 KB, patch)
2023-07-11 13:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Fix calculations around cronjob fines.pl (4.08 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Move monetary definition into hash (2.39 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Fix trailing 0 decimals (1.33 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee (1.22 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Add TT filters for Price and pattern checks to input fields (5.26 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Rewrite database update (4.28 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Add unformat_price js function (1.79 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (follow-up) Apply unformat_price to decimal fields (1.72 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Throw exception if not passed a decimal number (1.21 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: Perltidy database update script (3.16 KB, patch)
2023-07-11 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33028: (QA follow-up) Tidy introduced code (1.05 KB, patch)
2023-07-18 18:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33028: Make exception less generic (2.98 KB, patch)
2023-07-18 18:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33028: Unit tests (2.21 KB, patch)
2023-07-19 10:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Unit tests (2.21 KB, patch)
2023-07-19 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Add POD to new Exception (1.01 KB, patch)
2023-07-19 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: Unit tests (2.32 KB, patch)
2023-07-19 10:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Add POD to new Exception (1.01 KB, patch)
2023-07-19 10:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33028: (follow-up) Lower the two digits requirement (1.69 KB, patch)
2023-07-19 15:09 UTC, Tomás Cohen Arazi
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-02-21 16:02:42 UTC
Hello @ll,

I hesitate between 'fines and fee' section or 'cron' so don't move it if you think that's better in cron section..

When currency format is set on FR commas are decimals separators but when cron like fines.pl try to calculate fines it's fails due to this format.

I know there is a lot of discussion around this behavior and decimal separator by 'countries'.. like bug 12310 and bug 21507 but I would like to propose this patch to solve at least the behavior on the fine calculations related to cronjob 'fines.pl' which will use some circulation rules and therefore their formatting is important if you want to make calculations with
Comment 1 Thibaud Guillot 2023-02-21 16:08:41 UTC
Created attachment 147058 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)
Comment 2 Martin Renvoize 2023-03-03 11:12:33 UTC
Comment on attachment 147058 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

Review of attachment 147058 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/fix_calc_fines_fr-currency_21-11-MT39815.pl
@@ +40,5 @@
> +                                $rule_id, $rule_value
> +                            )
> +                        );
> +                    }
> +                }

We need some additional handling here for the .00/,00 case.. in particular 0.00/0,00

Perl will treat 0.00 as a string for truthyness and thus we need to reduce to just the integer 0 in this case.
Comment 3 Martin Renvoize 2023-03-03 11:19:16 UTC
*** Bug 32271 has been marked as a duplicate of this bug. ***
Comment 4 Martin Renvoize 2023-03-03 11:23:55 UTC
Comment on attachment 147058 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

Review of attachment 147058 [details] [review]:
-----------------------------------------------------------------

::: Koha/CirculationRules.pm
@@ +392,5 @@
> +    if ( $rule
> +    && ( $rule->rule_name eq 'overduefinescap' || $rule->rule_name eq 'fine' ) )
> +    {
> +        $rule_value = Koha::Number::Price->new($rule_value)->unformat;
> +    }

I think we should generalise this and add it as a parameter in the hash above..

i.e.

. . . 

     overduefinescap => {                                                                                                                                                                                                                                                                                                  
         scope => [ 'branchcode', 'categorycode', 'itemtype' ],                                                                                                                                                                                                                                                            
         is_monetary => 1,
     }, 

. . .

my $is_monetary = defined $kind_info->{is_monetary} ? $kind_info->{is_monetary} : 0;
$rule_value = Koha::Number::Price->new($rule_value)->unformat if $is_monetary;

. . .
Comment 5 Martin Renvoize 2023-03-03 11:35:41 UTC
Created attachment 147686 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2023-03-03 11:35:43 UTC
Created attachment 147687 [details] [review]
Bug 33028: (follow-up) Move monetary definition into hash

This patch moves the defintion of monetary rule type into the rule kinds
hash.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2023-03-03 11:35:46 UTC
Created attachment 147688 [details] [review]
Bug 33028: (follow-up) Fix trailing 0 decimals

We want to recognise the truthyness of a number vs string so we drop
trailing decimals if they're just 0.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-03-03 11:36:37 UTC
I've added follow-ups for my comments.. signing off.
Comment 9 Thibaud Guillot 2023-03-03 11:53:46 UTC
wow thanks Martin, that's great ^^
Comment 10 Martin Renvoize 2023-03-03 12:13:42 UTC
I've asked Jonathan D to review it for QA too as I think he's pretty close to how the circ rules stuff all fits together these days.

Be nice to have this fixed. :)
Comment 11 Jonathan Druart 2023-03-07 10:15:41 UTC
> $rule_value !~ /,.*?,/

.*?
What are you trying to do here? .* should be enough.
Comment 12 Jonathan Druart 2023-03-07 10:16:59 UTC
> 'Many commas in rule id %s ("%s") - fix it before restart this update',

Do you mean "More than one commas"?
Comment 13 Thibaud Guillot 2023-03-07 10:23:41 UTC
(In reply to Jonathan Druart from comment #11)
> > $rule_value !~ /,.*?,/
> 
> .*?
> What are you trying to do here? .* should be enough.

Hello Jonathan,

Here I just want to avoid several commas and check if there are numbers after the comma and before another one.. that's useless maybe but I think that user must change values himself if there are not simply like xx,xx no ?
Comment 14 Thibaud Guillot 2023-03-07 10:25:09 UTC
(In reply to Jonathan Druart from comment #12)
> > 'Many commas in rule id %s ("%s") - fix it before restart this update',
> 
> Do you mean "More than one commas"?

Yes, in circ_rules tables declaration it's only a varchar type if I remember, so user can have bad values and must fix it
Comment 15 Thibaud Guillot 2023-03-07 10:27:48 UTC
I take advantage of this patch to ask you a wider opinion on the management of the decimal separator, there is the BZ 21507 but I do not know in which direction to direct it to fix the problem once and for all

Cause this problem (have a comma in rule values, occurs another problems around supplier update for example - when we want to change discount input with comma separator..)
Comment 16 Jonathan Druart 2023-03-07 10:29:46 UTC
I think this is wrong.

PriceFormat=US, edit a rule with "360,000" (meaning 360000), and "360.00" is saved.

In my opinion we should keep things simple to avoid bugs, and don't try anything else than warn the user that they are doing something wrong.

I would:
Run a dbrev to warn about incorrect values
Add a JS alert when the user is trying to save a rule that is not %.2f formatted
Raise an exception in Koha::Circulation->store in case incorrect values arrived there.
Comment 17 Katrin Fischer 2023-03-07 11:34:16 UTC
I think I'd also throw in a database update just replacing , with . If that's not enough, it won't work before or after... but we might fix a ton of problematic values.
Comment 18 Martin Renvoize 2023-03-13 17:18:20 UTC
(In reply to Jonathan Druart from comment #16)
> I think this is wrong.
> 
> PriceFormat=US, edit a rule with "360,000" (meaning 360000), and "360.00" is
> saved.
> 
> In my opinion we should keep things simple to avoid bugs, and don't try
> anything else than warn the user that they are doing something wrong.
> 
> I would:
> Run a dbrev to warn about incorrect values
> Add a JS alert when the user is trying to save a rule that is not %.2f
> formatted
> Raise an exception in Koha::Circulation->store in case incorrect values
> arrived there.

So, are you saying the simple 'unformat' call isn't working as we would expect..

That's a shame :(
Comment 19 Katrin Fischer 2023-04-04 09:01:50 UTC
*** Bug 33359 has been marked as a duplicate of this bug. ***
Comment 20 Kyle M Hall 2023-04-28 13:57:21 UTC
Circ rules are not enforcing currency formats either. This might fix things once, but does not keep the issue from coming back.
Comment 21 Katrin Fischer 2023-04-28 13:59:29 UTC
(In reply to Kyle M Hall from comment #20)
> Circ rules are not enforcing currency formats either. This might fix things
> once, but does not keep the issue from coming back.

Hi Kyle, what do you suggest? A format check on the inputs in the circ rules? We could use the pattern check we use in other forms. Or did you think of something else?
Comment 22 Kyle M Hall 2023-05-01 11:12:04 UTC
(In reply to Katrin Fischer from comment #21)
> (In reply to Kyle M Hall from comment #20)
> > Circ rules are not enforcing currency formats either. This might fix things
> > once, but does not keep the issue from coming back.
> 
> Hi Kyle, what do you suggest? A format check on the inputs in the circ
> rules? We could use the pattern check we use in other forms. Or did you
> think of something else?

Yes. Ideally we'd enforce the chosen format using javascript on the frontend, and in perl on the backend. Since the js should enforce it on the frontend, the backend could just die or throw an exception if it sees a violation.
Comment 23 Katrin Fischer 2023-05-01 11:47:54 UTC
We had multiple reports of fines not working in 22.11 on the mailing lists and on bugzilla - I'd like to move this forward in some way since I am pretty sure that this one is to blame from the logs we saw complaining about invalid values.

I am upping severity for now. This is definitely not an enhancement.

The problem is, that when you edit an existing rule, you might create problematic values without noticing.
Comment 24 Katrin Fischer 2023-05-01 12:19:44 UTC
*** Bug 33240 has been marked as a duplicate of this bug. ***
Comment 25 Katrin Fischer 2023-06-16 14:18:16 UTC
To test:

* Set CurrencyFormat to FR
* Enable UseRecalls
* Administration > circulation and fine rules
* Add a new rule:
  * Fine amount: 0.10
  * Fine charging interval: 1  
  * Overdue fines cap: 1.50
  * Recall overdue fine amount: 1.10
* Save rule
* Verify that both Overdue fines cap and recall overdue fine amount show 
  formatted with comma as: 1,50, 1,10
* Edit rule
  * Fine amount: 0.10 - OK!
  * Fine charging interval: 1  
  * Overdue fines cap: 1,50 - NOT OK!
  * Recall overdue fine amount: 1,10 - NOT OK!
* Save rule
* With SQL:

select * from circulation_rules where rule_name in ("fine","overduefinescap","recall_overdue_fine");

+----+------------+--------------+----------+---------------------+------------+
| id | branchcode | categorycode | itemtype | rule_name           | rule_value |
+----+------------+--------------+----------+---------------------+------------+
|  9 | NULL       | NULL         | NULL     | fine                | 0.10       |
| 11 | NULL       | NULL         | NULL     | overduefinescap     | 1,50       |
| 32 | NULL       | NULL         | NULL     | recall_overdue_fine | 1,10       |
+----+------------+--------------+----------+---------------------+------------+

So the problem is, that you will save false values unnoticed that will cause errors.

You cannot tell wrong and right values apart in the table, because all values show formatted, if correct in the database or not.
Comment 26 Katrin Fischer 2023-06-16 14:47:07 UTC
I am working on a few follow-ups here.
Comment 27 Katrin Fischer 2023-06-16 17:05:26 UTC
Created attachment 152441 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Katrin Fischer 2023-06-16 17:05:29 UTC
Created attachment 152442 [details] [review]
Bug 33028: (follow-up) Move monetary definition into hash

This patch moves the defintion of monetary rule type into the rule kinds
hash.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Katrin Fischer 2023-06-16 17:05:32 UTC
Created attachment 152443 [details] [review]
Bug 33028: (follow-up) Fix trailing 0 decimals

We want to recognise the truthyness of a number vs string so we drop
trailing decimals if they're just 0.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Katrin Fischer 2023-06-16 17:05:35 UTC
Created attachment 152444 [details] [review]
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee

This patch marks the 2 missing monetary values for recal over due fines
and article request fees as monetary.
Comment 31 Katrin Fischer 2023-06-16 17:05:38 UTC
Created attachment 152445 [details] [review]
Bug 33028: Add TT filters for Price and pattern checks to input fields

With this patch, all monetary values in the table will be displayed
formatted.

Also, the input will be checked against our agreed pattern to make
sure no false values can be entered.

Missing: When editing a rule, we need to unformat the value, so that
instead of the display format we have the input format available
for editing.
Comment 32 Katrin Fischer 2023-06-16 17:05:41 UTC
Created attachment 152446 [details] [review]
Bug 33028: (follow-up) Rewrite database update

This rewrite the database update with some things in mind:

* We now use a positive value list of allowed characters to check
  This makes sure that all of those are recognized:
  1,00
  1.00€
  abc
* Instead of dying after finding one wrong value, we loop through
  all values first, building up an error string
* When we have errors... we die and print the full list of things
  that need fixing.
Comment 33 Katrin Fischer 2023-06-16 17:15:06 UTC
Updated test plan:

* Set CurrencyFormat to FR
* Enable UseRecalls
* Enable ArticleRequests
* Administration > circulation and fine rules
* Add a new rule:
  * Fine amount: 0.10
  * Fine charging interval: 1  
  * Overdue fines cap: 1.50
  * Recall overdue fine amount: 1.10
* Save rule
* Verify that both Overdue fines cap and recall overdue fine amount show 
  formatted with comma as: 1,50, 1,10
* Edit rule
  * Fine amount: 0.10 - OK!
  * Fine charging interval: 1  
  * Overdue fines cap: 1,50 - NOT OK!
  * Recall overdue fine amount: 1,10 - NOT OK!
* Save rule
* With SQL:

select * from circulation_rules where rule_name in ("fine","overduefinescap","recall_overdue_fine","article_request_fee");

+----+------------+--------------+----------+---------------------+------------+
| id | branchcode | categorycode | itemtype | rule_name           | rule_value |
+----+------------+--------------+----------+---------------------+------------+
|  9 | NULL       | NULL         | NULL     | fine                | 0.10       |
| 11 | NULL       | NULL         | NULL     | overduefinescap     | 1,50       |
| 32 | NULL       | NULL         | NULL     | recall_overdue_fine | 1,10       |
+----+------------+--------------+----------+---------------------+------------+

* Edit rule for Article request fee 
  * Fee: 1,0
* Verify it's also wrong in the database with above SQL.
--> This is already fixed by the patch set now, as we don't allow to edit the rules and the pattern check works.
  
Missing bits and pieces:

Have to:
* When a rule is edited, the input field will be set to the display value (1,00). I didn't find an "unformat" equivalent for JS. This now triggers the pattern check, so you need to edit all values before being able to save them. We could use the inputmode="decimal" on the input fields to select for unformatting.
* Should we be able to deactivate overduefinescap by leaving it empty? (Unlimited?)

Would be nice:
* It would be nice to provide more help in fixing wrong values or automatically fixing obvious ones like xx,xx.
Comment 34 Katrin Fischer 2023-06-17 21:47:52 UTC
*** Bug 30672 has been marked as a duplicate of this bug. ***
Comment 35 Katrin Fischer 2023-06-21 10:48:45 UTC
Created attachment 152509 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Katrin Fischer 2023-06-21 10:48:47 UTC
Created attachment 152510 [details] [review]
Bug 33028: (follow-up) Move monetary definition into hash

This patch moves the defintion of monetary rule type into the rule kinds
hash.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Katrin Fischer 2023-06-21 10:48:50 UTC
Created attachment 152511 [details] [review]
Bug 33028: (follow-up) Fix trailing 0 decimals

We want to recognise the truthyness of a number vs string so we drop
trailing decimals if they're just 0.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Katrin Fischer 2023-06-21 10:48:53 UTC
Created attachment 152512 [details] [review]
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee

This patch marks the 2 missing monetary values for recal over due fines
and article request fees as monetary.
Comment 39 Katrin Fischer 2023-06-21 10:48:56 UTC
Created attachment 152513 [details] [review]
Bug 33028: Add TT filters for Price and pattern checks to input fields

With this patch, all monetary values in the table will be displayed
formatted.

Also, the input will be checked against our agreed pattern to make
sure no false values can be entered.

Missing: When editing a rule, we need to unformat the value, so that
instead of the display format we have the input format available
for editing.
Comment 40 Katrin Fischer 2023-06-21 10:48:59 UTC
Created attachment 152514 [details] [review]
Bug 33028: (follow-up) Rewrite database update

This rewrite the database update with some things in mind:

* We now use a positive value list of allowed characters to check
  This makes sure that all of those are recognized:
  1,00
  1.00€
  abc
* Instead of dying after finding one wrong value, we loop through
  all values first, building up an error string
* When we have errors... we die and print the full list of things
  that need fixing.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 41 Katrin Fischer 2023-06-21 10:49:02 UTC
Created attachment 152515 [details] [review]
Bug 33028: Remove Price formatting as we are missing JS unformat

Taking a step back with the formatting as we don't have an implementation
of unformat in JS yet. It could be readded later on a separate
report.

With this we will avoid the problem that editing a rule copies
falsely formatted values into the input fields that trigger
the pattern check on saving and require manual changing (or
without the check would be falsely saved to the db)

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 42 Katrin Fischer 2023-06-21 11:23:00 UTC
Adjusted test plan:

* Set CurrencyFormat to FR
* Enable UseRecalls
* Enable ArticleRequests
* Administration > circulation and fine rules
* Add a new rule:
* Fine amount: 0.10
* Fine charging interval: 1  
* Overdue fines cap: 1.50
* Recall overdue fine amount: 1.10
* Save rule
* Verify that both Overdue fines cap and recall overdue fine amount show 
formatted with comma as: 1,50, 1,10
* Edit rule
* Fine amount: 0.10 - OK!
* Fine charging interval: 1  
* Overdue fines cap: 1,50 - NOT OK!
* Recall overdue fine amount: 1,10 - NOT OK!
* Save rule
* With SQL:

select * from circulation_rules where rule_name in
("fine","overduefinescap","recall_overdue_fine","article_request_fee");

You should see the false values in the database:
+----+------------+--------------+----------+---------------------+----------
--+
| id | branchcode | categorycode | itemtype | rule_name           |
rule_value |
+----+------------+--------------+----------+---------------------+----------
--+
|  9 | NULL       | NULL         | NULL     | fine                | 0.10    
|
| 11 | NULL       | NULL         | NULL     | overduefinescap     | 1,50    
|
| 32 | NULL       | NULL         | NULL     | recall_overdue_fine | 1,10    
|
+----+------------+--------------+----------+---------------------+----------
--+

* Edit rule for Article request fee 
* Fee: 1,0
* Verify it's also wrong in the database with above SQL.

* Apply patch
* Run database update
* Verify your wrong values cause the update to stop
* Fix them (one or all at once)
* Verify the update now continues
* Repeat editing and adding rules actions from before, a pattern check will prevent you now from saving false values.


------------------

Missing bits and pieces:

>Have to:
>* When a rule is edited, the input field will be set to the display value
>(1,00). I didn't find an "unformat" equivalent for JS. This now triggers the
>pattern check, so you need to edit all values before being able to save
>them. We could use the inputmode="decimal" on the input fields to select for
>unformatting.

I have decided that this could be a separate bug. The JS used for editing/cloning the rules is quite complex and it's easier to make a step back first to get this moving and into old versions. 

--> I have removed the Price formatting from the circulation rules table in the last follow-up.
 
> * Edit rule for Article request fee 
>   * Fee: 1,0
> * Verify it's also wrong in the database with above SQL.
> --> This is already fixed by the patch set now, as we don't allow to edit
> the rules and the pattern check works.
>   
> Missing bits and pieces:
> 
> Have to:
> * When a rule is edited, the input field will be set to the display value
> (1,00). I didn't find an "unformat" equivalent for JS. This now triggers the
> pattern check, so you need to edit all values before being able to save
> them. We could use the inputmode="decimal" on the input fields to select for
> unformatting.

I have now fixed this by removing the Price formatting from all fields. I think
this can be filed separately but is the quickest fix now that can also be 
backported.


> * Should we be able to deactivate overduefinescap by leaving it empty?
> (Unlimited?)

Yes, definitely. But there is already a separate bug (and a duplicate) for that:
Bug 32271 - Overdue fines cap (amount) set to 0.00 when editing rule

So I decided to split this issue out in hope to be able to move this forward and
get help with the other issue.

> Would be nice:
> * It would be nice to provide more help in fixing wrong values or
> automatically fixing obvious ones like xx,xx.

I think we don't need that to move on here.
Comment 43 Martin Renvoize 2023-06-27 15:27:46 UTC
Created attachment 152752 [details] [review]
Bug 33028: (follow-up) Add unformat_price js function
Comment 44 Martin Renvoize 2023-06-27 15:27:49 UTC
Created attachment 152753 [details] [review]
Revert "Bug 33028: Remove Price formatting as we are missing JS unformat"

This reverts commit c10aa96e4f1fac0f34e02caade3aa259d7233c71.
Comment 45 Martin Renvoize 2023-06-27 15:31:14 UTC
I ran out of tuits here I'm afraid.. passing back to Katrin.. I created an 'unformat_price' function for String and exported it for you.. I'll come back and try to apply it to the right editing fields here when I can.. 

Adding the patches as is for now in case you have a moment before me.
Comment 46 Martin Renvoize 2023-06-27 15:54:32 UTC
Created attachment 152754 [details] [review]
Bug 33028: (follow-up) Apply unformat_price to decimal fields
Comment 47 Martin Renvoize 2023-06-27 15:55:02 UTC
OK.. I found the 5 minutes!

Please test
Comment 48 Martin Renvoize 2023-06-27 15:58:25 UTC
Created attachment 152755 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2023-06-27 15:58:29 UTC
Created attachment 152756 [details] [review]
Bug 33028: (follow-up) Move monetary definition into hash

This patch moves the defintion of monetary rule type into the rule kinds
hash.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2023-06-27 15:58:32 UTC
Created attachment 152757 [details] [review]
Bug 33028: (follow-up) Fix trailing 0 decimals

We want to recognise the truthyness of a number vs string so we drop
trailing decimals if they're just 0.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2023-06-27 15:58:35 UTC
Created attachment 152758 [details] [review]
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee

This patch marks the 2 missing monetary values for recal over due fines
and article request fees as monetary.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2023-06-27 15:58:39 UTC
Created attachment 152759 [details] [review]
Bug 33028: Add TT filters for Price and pattern checks to input fields

With this patch, all monetary values in the table will be displayed
formatted.

Also, the input will be checked against our agreed pattern to make
sure no false values can be entered.

Missing: When editing a rule, we need to unformat the value, so that
instead of the display format we have the input format available
for editing.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize 2023-06-27 15:58:42 UTC
Created attachment 152760 [details] [review]
Bug 33028: (follow-up) Rewrite database update

This rewrite the database update with some things in mind:

* We now use a positive value list of allowed characters to check
  This makes sure that all of those are recognized:
  1,00
  1.00€
  abc
* Instead of dying after finding one wrong value, we loop through
  all values first, building up an error string
* When we have errors... we die and print the full list of things
  that need fixing.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2023-06-27 15:58:46 UTC
Created attachment 152761 [details] [review]
Bug 33028: Remove Price formatting as we are missing JS unformat

Taking a step back with the formatting as we don't have an implementation
of unformat in JS yet. It could be readded later on a separate
report.

With this we will avoid the problem that editing a rule copies
falsely formatted values into the input fields that trigger
the pattern check on saving and require manual changing (or
without the check would be falsely saved to the db)

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2023-06-27 15:58:49 UTC
Created attachment 152762 [details] [review]
Bug 33028: (follow-up) Add unformat_price js function

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2023-06-27 15:58:53 UTC
Created attachment 152763 [details] [review]
Revert "Bug 33028: Remove Price formatting as we are missing JS unformat"

This reverts commit c10aa96e4f1fac0f34e02caade3aa259d7233c71.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2023-06-27 15:58:56 UTC
Created attachment 152764 [details] [review]
Bug 33028: (follow-up) Apply unformat_price to decimal fields

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2023-06-27 16:00:07 UTC
Added my signoff line to all patches.. I'm pretty happy with is solid now.. but given all the challenges we've had with it I'd love a final QA run from an independance party.
Comment 59 Nick Clemens 2023-06-28 11:59:22 UTC
Editing a rule with 1.50 shows 1.5 - and tells me to 'please match the requested format' - but doesn't say what that format is
Entering 1.50 and saving, i see: 150,00 in the matrix and 150 in the DB
Comment 60 Martin Renvoize 2023-06-28 13:50:37 UTC
Created attachment 152803 [details] [review]
Bug 33028: (follow-up) Add unformat_price js function

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 61 Martin Renvoize 2023-06-28 13:50:40 UTC
Created attachment 152804 [details] [review]
Revert "Bug 33028: Remove Price formatting as we are missing JS unformat"

This reverts commit c10aa96e4f1fac0f34e02caade3aa259d7233c71.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2023-06-28 13:50:44 UTC
Created attachment 152805 [details] [review]
Bug 33028: (follow-up) Apply unformat_price to decimal fields

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2023-06-28 13:56:38 UTC
I resolved this: Editing a rule with 1.50 shows 1.5
But I can't replicate this: Entering 1.50 and saving, i see: 150,00 in the matrix and 150 in the DB
Comment 64 Martin Renvoize 2023-06-28 15:57:45 UTC
OK.. I should have read Katrins patches in more depth before continuing here.. 

Bug 33028: Add TT filters for Price and pattern checks to input fields

This actually takes us backwards and breaks input more.. We are expecting currency format to be correctly passed to us in system currency format.. NOT always US decimal.. as such by adding the pattern and setting the input types to decimal only we are forcing US decimal input which means the unformat call on the serverside is breaking the inputs.

I think we should go all the way back to `Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee` and we had a working system at that point following Katrins test plan.. at least it was working for me for all cases at that point.
Comment 65 Martin Renvoize 2023-06-28 16:08:52 UTC
Created attachment 152829 [details] [review]
Bug 33028: (follow-up) Remove inputmode and pattern
Comment 66 Martin Renvoize 2023-06-28 16:11:55 UTC
OK... with that last patch I think we should be working correctly in all cases..

Please follow the test plan in comment 25 to confirm this.

We can drop/squash half the patches here.. the bug has become a real mess with things getting added then reverted, then added and finally reverted again.

I'm not sure we need the js unformat function I created at all at this point.. it's no longer used here.. but might be helpful migrated to another bug in case anyone needs it elsewhere.  I'm also not sure about the location of the unformat call deep down in Circulation rules.. it feels like we should perhaps be calling this higher up in the controller rather than the module expecting different input types based on configuration.

Anyway.. I'd love to see this move forward.
Comment 67 Katrin Fischer 2023-06-30 18:29:25 UTC
I feel like we moved into contrary directions:

I tried to restore the former behaviour. Koha expects us to enter monetary amounts with decimal dot everywhere, so I was trying to get back to this here.
I'd like to see input formats fixed, but I feel that a bugfix that needs to go back into stables is the wrong place. I used the same pattern that we agreed on and use in patron accounting and removed the Price TT filters to make sure we get back to decimal dot everywhere, giving up the display in favor of having the input fixed.

I believe Martin tried to allow entering with decimal comma with his patches, which appears more risky to me. Also: how to communicate that this page expects different input than anywhere else?

I might be mistaken - I need to take a closer look for sure.
Comment 68 Martin Renvoize 2023-06-30 18:34:25 UTC
OK, well that may be the case.. but you'll need to also undo the first patch here as that make it expect bon decimal dot
Comment 69 Martin Renvoize 2023-07-03 12:47:12 UTC
Created attachment 152928 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 70 Martin Renvoize 2023-07-03 12:47:15 UTC
Created attachment 152929 [details] [review]
Bug 33028: (follow-up) Move monetary definition into hash

This patch moves the defintion of monetary rule type into the rule kinds
hash.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2023-07-03 12:47:19 UTC
Created attachment 152930 [details] [review]
Bug 33028: (follow-up) Fix trailing 0 decimals

We want to recognise the truthyness of a number vs string so we drop
trailing decimals if they're just 0.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 72 Martin Renvoize 2023-07-03 12:47:22 UTC
Created attachment 152931 [details] [review]
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee

This patch marks the 2 missing monetary values for recal over due fines
and article request fees as monetary.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 73 Martin Renvoize 2023-07-03 12:47:25 UTC
Created attachment 152932 [details] [review]
Bug 33028: Add TT filters for Price and pattern checks to input fields

With this patch, all monetary values in the table will be displayed
formatted.

Also, the input will be checked against our agreed pattern to make
sure no false values can be entered.

Missing: When editing a rule, we need to unformat the value, so that
instead of the display format we have the input format available
for editing.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2023-07-03 12:47:29 UTC
Created attachment 152933 [details] [review]
Bug 33028: (follow-up) Rewrite database update

This rewrite the database update with some things in mind:

* We now use a positive value list of allowed characters to check
  This makes sure that all of those are recognized:
  1,00
  1.00€
  abc
* Instead of dying after finding one wrong value, we loop through
  all values first, building up an error string
* When we have errors... we die and print the full list of things
  that need fixing.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Martin Renvoize 2023-07-03 12:47:33 UTC
Created attachment 152934 [details] [review]
Bug 33028: (follow-up) Add unformat_price js function

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2023-07-03 12:47:37 UTC
Created attachment 152935 [details] [review]
Bug 33028: (follow-up) Apply unformat_price to decimal fields

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2023-07-03 12:47:40 UTC
Created attachment 152936 [details] [review]
Bug 33028: Throw exception if not passed a decimal number
Comment 78 Martin Renvoize 2023-07-03 12:52:04 UTC
OK.. I believe this now works as Katrin intended as a whole...

* Displays are in localised format
* Inputs are in US Decimal Dot with decimal input and a pattern match

I prefer the patchset prior to this one, where both display and inputs followed currency format (though I do prefer the exception this raises if passed a bad format and would like to have moved the unformat into the controller level instead of where it was)

This is, however, more consistent with the rest of the UI where we appear to expect non-localised decimal inputs regardless of how we then display them.
Comment 79 Katrin Fischer 2023-07-11 13:13:05 UTC
Testing here again... back and forth we go!
Comment 80 Katrin Fischer 2023-07-11 13:26:46 UTC
Created attachment 153313 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Katrin Fischer 2023-07-11 13:27:26 UTC
Created attachment 153314 [details] [review]
Bug 33028: Fix calculations around cronjob fines.pl

When currency format is set on FR commas are decimals separators
but when cron like fines.pl try to calculate fines it's fails due to
this format.

I changed this behavior by targetted 'fine' and 'overduefinescap' in
circulation_rules.rule_name to unformat them when we save them.

This also fix the display in smart_rules table (before with commas price
was not good displayed - without decimals)

Test Plan :
1) Set your currency format on 'FR' and 'fine' OR/AND 'overduefinescap'
with commas
2) Be sure to have some patron overdues
3) Run ~/misc/cronjobs/fines.pl with args to find overdues
4) See an error like 'isn't numeric in substraction[..] or gt > [...]'
5) Run updatedatabase script (it will replace commas in your rules
changed in step 1) )
6) Repeat step 3 and see that everything was going "fine" (:tada:)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 82 Katrin Fischer 2023-07-11 13:27:30 UTC
Created attachment 153315 [details] [review]
Bug 33028: (follow-up) Move monetary definition into hash

This patch moves the defintion of monetary rule type into the rule kinds
hash.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 83 Katrin Fischer 2023-07-11 13:27:32 UTC
Created attachment 153316 [details] [review]
Bug 33028: (follow-up) Fix trailing 0 decimals

We want to recognise the truthyness of a number vs string so we drop
trailing decimals if they're just 0.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 84 Katrin Fischer 2023-07-11 13:27:35 UTC
Created attachment 153317 [details] [review]
Bug 33028: Add is_monetary to recall_overdue_fine and article_request_fee

This patch marks the 2 missing monetary values for recal over due fines
and article request fees as monetary.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 85 Katrin Fischer 2023-07-11 13:27:38 UTC
Created attachment 153318 [details] [review]
Bug 33028: Add TT filters for Price and pattern checks to input fields

With this patch, all monetary values in the table will be displayed
formatted.

Also, the input will be checked against our agreed pattern to make
sure no false values can be entered.

Missing: When editing a rule, we need to unformat the value, so that
instead of the display format we have the input format available
for editing.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 86 Katrin Fischer 2023-07-11 13:27:41 UTC
Created attachment 153319 [details] [review]
Bug 33028: (follow-up) Rewrite database update

This rewrite the database update with some things in mind:

* We now use a positive value list of allowed characters to check
  This makes sure that all of those are recognized:
  1,00
  1.00€
  abc
* Instead of dying after finding one wrong value, we loop through
  all values first, building up an error string
* When we have errors... we die and print the full list of things
  that need fixing.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 87 Katrin Fischer 2023-07-11 13:27:44 UTC
Created attachment 153320 [details] [review]
Bug 33028: (follow-up) Add unformat_price js function

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 88 Katrin Fischer 2023-07-11 13:27:47 UTC
Created attachment 153321 [details] [review]
Bug 33028: (follow-up) Apply unformat_price to decimal fields

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 89 Katrin Fischer 2023-07-11 13:27:50 UTC
Created attachment 153322 [details] [review]
Bug 33028: Throw exception if not passed a decimal number

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 90 Katrin Fischer 2023-07-11 13:27:53 UTC
Created attachment 153323 [details] [review]
Bug 33028: Perltidy database update script

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 91 Nick Clemens 2023-07-11 13:33:12 UTC
(In reply to Katrin Fischer from comment #79)
> Testing here again... back and forth we go!

Heh, tested as well, Katrin beat me by about 5 minutes,  but adding my support too :-)
Comment 92 Katrin Fischer 2023-07-11 13:47:30 UTC
(In reply to Nick Clemens from comment #91)
> (In reply to Katrin Fischer from comment #79)
> > Testing here again... back and forth we go!
> 
> Heh, tested as well, Katrin beat me by about 5 minutes,  but adding my
> support too :-)

I think another set of eyes definitely won't hurt for sure! 

I'd love to see a fix for bug 32271 as next step, so we hopefully get in a state again where editing rules doesn't create unwanted side effects.
Comment 93 Tomás Cohen Arazi 2023-07-18 18:37:23 UTC
Created attachment 153619 [details] [review]
Bug 33028: (QA follow-up) Tidy introduced code

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 94 Tomás Cohen Arazi 2023-07-18 18:38:04 UTC
Created attachment 153620 [details] [review]
Bug 33028: Make exception less generic

While testing this bug I found Circulation.t was failing, but the
exception doesn't actually display anything useful in terms of helping
debug what's going on.

This patch makes it add the rule_name and rule_value to the message.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 95 Tomás Cohen Arazi 2023-07-18 19:05:49 UTC
This is almost done, and already on my branch. But...

prove t/db_dependent/Koha/CirculationRules.t
t/db_dependent/Koha/CirculationRules.t .. 1/7
        #   Failed test 'setting fine with branch/category/itemtype succeeds'
        #   at t/db_dependent/Koha/CirculationRules.t line 211.
        # died: Koha::Exceptions::CirculationRule::NotDecimal (Exception 'Koha::Exceptions::CirculationRule::NotDecimal' thrown 'The circulation rule expected a decimal value' with name => fine, value =>
        # )
        # Looks like you failed 1 test of 5.


Please fix ASAP so I can push tomorrow morning.
Comment 96 Katrin Fischer 2023-07-18 22:00:46 UTC
(In reply to Tomás Cohen Arazi from comment #95)
> This is almost done, and already on my branch. But...
> 
> prove t/db_dependent/Koha/CirculationRules.t
> t/db_dependent/Koha/CirculationRules.t .. 1/7
>         #   Failed test 'setting fine with branch/category/itemtype succeeds'
>         #   at t/db_dependent/Koha/CirculationRules.t line 211.
>         # died: Koha::Exceptions::CirculationRule::NotDecimal (Exception
> 'Koha::Exceptions::CirculationRule::NotDecimal' thrown 'The circulation rule
> expected a decimal value' with name => fine, value =>
>         # )
>         # Looks like you failed 1 test of 5.
> 
> 
> Please fix ASAP so I can push tomorrow morning.

Martin, do you have a moment to have a look?
Comment 97 Martin Renvoize 2023-07-19 10:22:43 UTC Comment hidden (obsolete)
Comment 98 Martin Renvoize 2023-07-19 10:31:18 UTC Comment hidden (obsolete)
Comment 99 Martin Renvoize 2023-07-19 10:31:21 UTC Comment hidden (obsolete)
Comment 100 Martin Renvoize 2023-07-19 10:33:24 UTC
Short of tidying the whole CirculationRule module I couldn't get the perltidy QA complaint to go away (I'd love to tidy the whole file.. there's isn't all that many changes to be honest so I don't think it would really hurt for future conflicts much)

Unit tests corrected and additional one's added.
Comment 101 Martin Renvoize 2023-07-19 10:48:41 UTC
Created attachment 153635 [details] [review]
Bug 33028: Unit tests

Add unit tests for is_monetary functionality introduced in the
CirculationRules module.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 102 Martin Renvoize 2023-07-19 10:48:44 UTC
Created attachment 153636 [details] [review]
Bug 33028: (follow-up) Add POD to new Exception

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 103 Tomás Cohen Arazi 2023-07-19 15:08:33 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 104 Tomás Cohen Arazi 2023-07-19 15:09:15 UTC
Created attachment 153677 [details] [review]
Bug 33028: (follow-up) Lower the two digits requirement

This patch makes the code not require two decimal digits, as the main
intention here is to forbid (locale) formatted strings to reach the DB.

The number of digits we support needs to be discussed on its own bug,
and a centralized check implemented.

This patch fixes tests:

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 1/67 Exception 'Koha::Exceptions::CirculationRule::NotDecimal' thrown 'The circulation rule expected a decimal value' with name => fine, value => 0.1
t/db_dependent/Circulation.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 53/67 subtests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 105 Katrin Fischer 2023-07-26 18:40:14 UTC
This requires backporting down to 22.11 please.
Comment 106 Fridolin Somers 2023-08-04 01:34:30 UTC
Pushed to 23.05.x for 23.05.03
Comment 107 Pedro Amorim 2023-08-17 16:05:44 UTC
Nice work everyone!

Pushed to 22.11.x for next release
Comment 108 Katrin Fischer 2023-08-23 16:56:48 UTC
Thank you, everyone and Pedro :)
Comment 109 Lucas Gass 2023-09-13 20:53:19 UTC
If this is needed in 22.05.x the patchset will need a rebase for that version.