Bug 28482

Summary: Floating point math prevents items from being returned
Product: Koha Reporter: Nick Clemens <nick>
Component: Fines and feesAssignee: Nick Clemens <nick>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrew, fridolin.somers, jonathan.druart+koha, kyle, lisetteslatah, martin.renvoize, victor, wainuiwitikapark
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24855
Change sponsored?: --- Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01,20.11.07,20.05.13,19.11.20
Attachments: Bug 28482: Unit test
Bug 28482: Refresh line from DB to get stored value
Bug 28482: Unit test
Bug 28482: Refresh line from DB to get stored value
Bug 28482: Unit test
Bug 28482: Refresh line from DB to get stored value
Bug 28482: [19.11.x] Unit test
Bug 28482: [19.11.x] Refresh line from DB to get stored value
Bug 28482: [19.11.x] Unit test
Bug 28482: [19.11.x] Refresh line from DB to get stored value

Description Nick Clemens 2021-05-28 12:01:53 UTC
We have seen this before, cropping up again.

To recreate:
1 - Add a processing fee of 15 to an itemtype
2 - Add an item of that type, set the replacement fee to 12.63
3 - Set MarkLostItemsAsReturned to 'On payment' only
4 - Set  WhenLostChargeReplacementFee to 'Charge'
5 - Checkout the item to a patron
6 - Mark the item lost
7 - Reload patron and confirm they are charged 27.63
8 - Go to accounting, pay amount, pay 27.63
9 - Item is still lost and not returned
Comment 1 Nick Clemens 2021-05-28 12:05:02 UTC
Created attachment 121476 [details] [review]
Bug 28482: Unit test
Comment 2 Nick Clemens 2021-05-28 12:05:05 UTC
Created attachment 121477 [details] [review]
Bug 28482: Refresh line from DB to get stored value

Before checking if the amount is 0 we get the stored value from the DB. This
ensures any amounts beyond the 6 digit precision we store will be removed.

To test:
1 - Add a processing fee of 15 to an itemtype
2 - Add an item of that type, set the replacement fee to 12.63
3 - Set MarkLostItemsAsReturned to 'On payment' only
4 - Set  WhenLostChargeReplacementFee to 'Charge'
5 - Checkout the item to a patron
6 - Mark the item lost
7 - Reload patron and confirm they are charged 27.63
8 - Go to accounting, pay amount, pay 27.63
9 - Item is still lost and not returned
10 - Apply patch
11 - Checkin the item
12 - Checkout to another patron
13 - Mark lost
14 - Patron charged 27.63
15 - Pay amount, 27.63
16 - Item returned!
Comment 3 Martin Renvoize 2021-05-28 12:45:57 UTC
Created attachment 121479 [details] [review]
Bug 28482: Unit test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2021-05-28 12:46:00 UTC
Created attachment 121480 [details] [review]
Bug 28482: Refresh line from DB to get stored value

Before checking if the amount is 0 we get the stored value from the DB. This
ensures any amounts beyond the 6 digit precision we store will be removed.

To test:
1 - Add a processing fee of 15 to an itemtype
2 - Add an item of that type, set the replacement fee to 12.63
3 - Set MarkLostItemsAsReturned to 'On payment' only
4 - Set  WhenLostChargeReplacementFee to 'Charge'
5 - Checkout the item to a patron
6 - Mark the item lost
7 - Reload patron and confirm they are charged 27.63
8 - Go to accounting, pay amount, pay 27.63
9 - Item is still lost and not returned
10 - Apply patch
11 - Checkin the item
12 - Checkout to another patron
13 - Mark lost
14 - Patron charged 27.63
15 - Pay amount, 27.63
16 - Item returned!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-05-28 12:48:04 UTC
We could have used the compare technique as used in bug 27079 here too, but I think we're OK doing a fetch from the DB for now.

I'd really like to treat floating point more widely with the Koha::Money or whatever class I hoped to introduce in bug 17138 at some point but I've not managed to find the time to work on that bug again of late :(

In short.. signing off.
Comment 6 Victor Grousset/tuxayo 2021-05-31 17:48:34 UTC
> 16 - Item returned!

Still marked as lost, is that expected?
Comment 7 Katrin Fischer 2021-06-03 11:23:38 UTC
Created attachment 121586 [details] [review]
Bug 28482: Unit test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2021-06-03 11:23:43 UTC
Created attachment 121587 [details] [review]
Bug 28482: Refresh line from DB to get stored value

Before checking if the amount is 0 we get the stored value from the DB. This
ensures any amounts beyond the 6 digit precision we store will be removed.

To test:
1 - Add a processing fee of 15 to an itemtype
2 - Add an item of that type, set the replacement fee to 12.63
3 - Set MarkLostItemsAsReturned to 'On payment' only
4 - Set  WhenLostChargeReplacementFee to 'Charge'
5 - Checkout the item to a patron
6 - Mark the item lost
7 - Reload patron and confirm they are charged 27.63
8 - Go to accounting, pay amount, pay 27.63
9 - Item is still lost and not returned
10 - Apply patch
11 - Checkin the item
12 - Checkout to another patron
13 - Mark lost
14 - Patron charged 27.63
15 - Pay amount, 27.63
16 - Item returned!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Andrew Fuerste-Henry 2021-06-04 12:06:26 UTC
*** Bug 24855 has been marked as a duplicate of this bug. ***
Comment 10 Jonathan Druart 2021-06-14 14:22:17 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2021-06-14 14:23:11 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 12 Kyle M Hall 2021-06-18 10:40:01 UTC
Pushed to 21.05.x for 21.05.01
Comment 13 Fridolin Somers 2021-06-21 12:17:33 UTC
Pushed to 20.11.x for 20.11.07
Comment 14 Victor Grousset/tuxayo 2021-06-22 00:51:22 UTC
Backported: Pushed to 20.05.x branch for 20.05.13
Comment 15 wainuiwitikapark 2021-06-24 02:05:16 UTC
Should this be backported to 19.11.x? For 19.11.19?
Comment 16 wainuiwitikapark 2021-07-04 04:34:03 UTC
Should this be backported to 19.11.x? For 19.11.20?
Comment 17 wainuiwitikapark 2021-07-05 01:55:27 UTC
Backported to 19.11.x for 19.11.20
Comment 18 wainuiwitikapark 2021-07-07 01:58:03 UTC
Sorry this, hasn't actually been backported to 19.11.x yet I made an error I was meant to be updating Bug 28487
Comment 19 wainuiwitikapark 2021-07-07 02:09:52 UTC
There are conflicts when trying to backport this to 19.11.x.

Is this needed for 19.11.x?
Comment 20 Nick Clemens 2021-07-07 11:34:09 UTC
Created attachment 122646 [details] [review]
Bug 28482: [19.11.x] Unit test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Fix test rebase
Comment 21 Nick Clemens 2021-07-07 11:34:13 UTC
Created attachment 122647 [details] [review]
Bug 28482: [19.11.x] Refresh line from DB to get stored value

Before checking if the amount is 0 we get the stored value from the DB. This
ensures any amounts beyond the 6 digit precision we store will be removed.

To test:
1 - Add a processing fee of 15 to an itemtype
2 - Add an item of that type, set the replacement fee to 12.63
3 - Set MarkLostItemsAsReturned to 'On payment' only
4 - Set  WhenLostChargeReplacementFee to 'Charge'
5 - Checkout the item to a patron
6 - Mark the item lost
7 - Reload patron and confirm they are charged 27.63
8 - Go to accounting, pay amount, pay 27.63
9 - Item is still lost and not returned
10 - Apply patch
11 - Checkin the item
12 - Checkout to another patron
13 - Mark lost
14 - Patron charged 27.63
15 - Pay amount, 27.63
16 - Item returned!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 wainuiwitikapark 2021-07-08 01:42:23 UTC
Created attachment 122659 [details] [review]
Bug 28482: [19.11.x] Unit test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Fix test rebase

Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Comment 23 wainuiwitikapark 2021-07-08 01:42:48 UTC
Created attachment 122660 [details] [review]
Bug 28482: [19.11.x] Refresh line from DB to get stored value

Before checking if the amount is 0 we get the stored value from the DB. This
ensures any amounts beyond the 6 digit precision we store will be removed.

To test:
1 - Add a processing fee of 15 to an itemtype
2 - Add an item of that type, set the replacement fee to 12.63
3 - Set MarkLostItemsAsReturned to 'On payment' only
4 - Set  WhenLostChargeReplacementFee to 'Charge'
5 - Checkout the item to a patron
6 - Mark the item lost
7 - Reload patron and confirm they are charged 27.63
8 - Go to accounting, pay amount, pay 27.63
9 - Item is still lost and not returned
10 - Apply patch
11 - Checkin the item
12 - Checkout to another patron
13 - Mark lost
14 - Patron charged 27.63
15 - Pay amount, 27.63
16 - Item returned!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Comment 24 wainuiwitikapark 2021-07-08 02:11:15 UTC
Backported to 19.11.x for 19.11.20