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
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 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.
*** Bug 32271 has been marked as a duplicate of this bug. ***
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; . . .
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>
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>
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>
I've added follow-ups for my comments.. signing off.
wow thanks Martin, that's great ^^
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. :)
> $rule_value !~ /,.*?,/ .*? What are you trying to do here? .* should be enough.
> 'Many commas in rule id %s ("%s") - fix it before restart this update', Do you mean "More than one commas"?
(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 ?
(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
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..)
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.
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.
(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 :(
*** Bug 33359 has been marked as a duplicate of this bug. ***
Circ rules are not enforcing currency formats either. This might fix things once, but does not keep the issue from coming back.
(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?
(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.
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.
*** Bug 33240 has been marked as a duplicate of this bug. ***
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.
I am working on a few follow-ups here.
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>
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>
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>
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.
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.
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.
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.
*** Bug 30672 has been marked as a duplicate of this bug. ***
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>
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>
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>
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.
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.
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>
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>
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.
Created attachment 152752 [details] [review] Bug 33028: (follow-up) Add unformat_price js function
Created attachment 152753 [details] [review] Revert "Bug 33028: Remove Price formatting as we are missing JS unformat" This reverts commit c10aa96e4f1fac0f34e02caade3aa259d7233c71.
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.
Created attachment 152754 [details] [review] Bug 33028: (follow-up) Apply unformat_price to decimal fields
OK.. I found the 5 minutes! Please test
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>
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>
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>
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>
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>
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>
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>
Created attachment 152762 [details] [review] Bug 33028: (follow-up) Add unformat_price js function Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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.
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
Created attachment 152803 [details] [review] Bug 33028: (follow-up) Add unformat_price js function Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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
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.
Created attachment 152829 [details] [review] Bug 33028: (follow-up) Remove inputmode and pattern
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.
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.
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
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>
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>
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>
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>
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>
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>
Created attachment 152934 [details] [review] Bug 33028: (follow-up) Add unformat_price js function Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 152936 [details] [review] Bug 33028: Throw exception if not passed a decimal number
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.
Testing here again... back and forth we go!
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 153323 [details] [review] Bug 33028: Perltidy database update script Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
(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 :-)
(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.
Created attachment 153619 [details] [review] Bug 33028: (QA follow-up) Tidy introduced code Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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.
(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?
Created attachment 153632 [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>
Created attachment 153633 [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>
Created attachment 153634 [details] [review] Bug 33028: (follow-up) Add POD to new Exception Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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.
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>
Created attachment 153636 [details] [review] Bug 33028: (follow-up) Add POD to new Exception Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 23.11. Nice work everyone, thanks!
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>
This requires backporting down to 22.11 please.
Pushed to 23.05.x for 23.05.03
Nice work everyone! Pushed to 22.11.x for next release
Thank you, everyone and Pedro :)
If this is needed in 22.05.x the patchset will need a rebase for that version.