Bug 26076 - Paying selected accountlines in full may result in the error "You must pay a value less than or equal to $x"
Summary: Paying selected accountlines in full may result in the error "You must pay a ...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 23788
Blocks: 26536
  Show dependency treegraph
 
Reported: 2020-07-28 16:48 UTC by Kyle M Hall
Modified: 2020-10-05 10:07 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04, 19.11.10


Attachments
Bug 26076: Paying selected accountlines in full may result in the error "You must pay a value less than or equal to $x" (1.86 KB, patch)
2020-07-28 16:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26076: Paying selected accountlines in full may result in the error "You must pay a value less than or equal to $x" (1.90 KB, patch)
2020-07-29 14:09 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 26076: Sum the amount due in the database query instead of a loop in Perl (1.88 KB, patch)
2020-07-29 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26076: (QA Follow up) Prevent error by using a fresh resultset (1.72 KB, patch)
2020-07-30 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26076: Sum the amount due in the database query instead of a loop in Perl (1.88 KB, patch)
2020-07-30 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26076: (QA Follow up) Prevent error by using a fresh resultset (1.72 KB, patch)
2020-07-30 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26076: Sum the amount due in the database query instead of a loop in Perl (1.92 KB, patch)
2020-08-22 21:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26076: (QA Follow up) Prevent error by using a fresh resultset (1.76 KB, patch)
2020-08-22 21:47 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-07-28 16:48:02 UTC
This appears to be a bug due to inaccuracies in floating point math. I've been unable to reproduce it on demand. Sometimes when using the "Write off selected" button, Koha will give the error that the payment is more than the amount owed, even though they are the same. The solution I've implemented is to move the summation from Perl code to the database query. This video demonstrates the issue and afterward, shows the error goes away after the patch is applied: https://monosnap.com/file/pG69HC7iI9mU9kkuoCtbkVzVTffKlE
Comment 1 Kyle M Hall 2020-07-28 16:51:07 UTC
Created attachment 107471 [details] [review]
Bug 26076: Paying selected accountlines in full may result in the error "You must pay a value less than or equal to $x"

This appears to be a bug due to inaccuracies in floating point math. I've been unable to reproduce it on demand. Sometimes when using the "Write off selected" button, Koha will give the error that the payment is more than the amount owed, even though they are the same. The solution I've implemented is to move the summation from Perl code to the database query. This video demonstrates the issue and afterward, shows the error goes away after the patch is applied: https://monosnap.com/file/pG69HC7iI9mU9kkuoCtbkVzVTffKlE

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Verify that "Write off selected" functions as usual
Comment 2 Amit Gupta 2020-07-29 13:20:55 UTC
(In reply to Kyle M Hall from comment #1)
> Created attachment 107471 [details] [review] [review]
> Bug 26076: Paying selected accountlines in full may result in the error "You
> must pay a value less than or equal to $x"
> 
> This appears to be a bug due to inaccuracies in floating point math. I've
> been unable to reproduce it on demand. Sometimes when using the "Write off
> selected" button, Koha will give the error that the payment is more than the
> amount owed, even though they are the same. The solution I've implemented is
> to move the summation from Perl code to the database query. This video
> demonstrates the issue and afterward, shows the error goes away after the
> patch is applied: https://monosnap.com/file/pG69HC7iI9mU9kkuoCtbkVzVTffKlE
> 
> Test Plan:
> 1) Apply this patch
> 2) Restart all the things!
> 3) Verify that "Write off selected" functions as usual

Hi Kyle,

I am unable to reproduce the issue. I have checked multiple float values for ex: 114.25, 102.21, 41.36.
Comment 3 Kyle M Hall 2020-07-29 13:52:34 UTC
> I am unable to reproduce the issue. I have checked multiple float values for
> ex: 114.25, 102.21, 41.36.

Same here. The bug clearly exists as I demonstrated in the video, but I can't reproduce it on demand either.
Comment 4 Amit Gupta 2020-07-29 14:09:40 UTC
Created attachment 107503 [details] [review]
Bug 26076: Paying selected accountlines in full may result in  the error "You must pay a value less than or equal to $x"

I have given the signoff but unable reproduce the issue.
Comment 5 Amit Gupta 2020-07-29 14:11:05 UTC
(In reply to Kyle M Hall from comment #3)
> > I am unable to reproduce the issue. I have checked multiple float values for
> > ex: 114.25, 102.21, 41.36.
> 
> Same here. The bug clearly exists as I demonstrated in the video, but I
> can't reproduce it on demand either.

Adding Katrin
Comment 6 Joonas Kylmälä 2020-07-29 14:34:29 UTC
How to reproduce:
 1) Create manual invoice => amount 1.00
 2) Create manual credit => amount 0.011111
 3) Make a payment => Click Apply -0.01
 4) Click "Pay amount" and then on the next page "Confirm"
Comment 7 Joonas Kylmälä 2020-07-29 14:49:37 UTC
What I described in comment #6 could be of the same origin as the summation issue originally described here, maybe because in db we have 6 decimals and in GUI only 2 decimals?

The proposed fix might solve that one case and we could apply this patch to fix that, but I would just like to have the commit message in that case still be fixed, now it describes the bug and not the change, please see <https://wiki.koha-community.org/wiki/Commit_messages#Examples>. Marking this FQA until the commit message is fixed (or a more generic patch is submitted to solve this class of bugs).
Comment 8 Joonas Kylmälä 2020-07-29 15:08:55 UTC
What I gather, the issue I mentioned in comment #6 seems to be just weird behaviour caused by not having RoundFinesAtPayment systempref enabled and not related to this.
Comment 9 Kyle M Hall 2020-07-29 15:45:26 UTC
Created attachment 107516 [details] [review]
Bug 26076: Sum the amount due in the database query instead of a loop in Perl

This appears to be a bug due to inaccuracies in floating point math. I've been unable to reproduce it on demand. Sometimes when using the "Write off selected" button, Koha will give the error that the payment is more than the amount owed, even though they are the same. The solution I've implemented is to move the summation from Perl code to the database query. This video demonstrates the issue and afterward, shows the error goes away after the patch is applied: https://monosnap.com/file/pG69HC7iI9mU9kkuoCtbkVzVTffKlE

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Verify that "Write off selected" functions as usual

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>
Comment 10 Kyle M Hall 2020-07-29 15:46:59 UTC
I should also note that the library in my video has RoundFinesAtPayment disabled.
Comment 11 Joonas Kylmälä 2020-07-30 08:22:34 UTC
Thanks for the commit message fix, Kyle! I was about to pass QA for this... But then I ended up with this error on my last test:

DBIx::Class::Storage::DBI::_dbh_execute(): Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause at /kohadevbox/koha/members/paycollect.pl line 157
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Without the patch provided here the error doesn't happen. How to reproduce: 

0) Apply patch provided in this bug report & restart plack
1) Have RoundFinesAtPayment = No (not sure this is relevant though)
2) Create 3 manual invoices with the amount of 3.33
3) Make a payment > write off selected
4) The error should appear.
Comment 12 Kyle M Hall 2020-07-30 13:35:04 UTC
(In reply to Joonas Kylmälä from comment #11)
> Thanks for the commit message fix, Kyle! I was about to pass QA for this...
> But then I ended up with this error on my last test:
> 
> DBIx::Class::Storage::DBI::_dbh_execute(): Mixing of GROUP columns
> (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no
> GROUP BY clause at /kohadevbox/koha/members/paycollect.pl line 157
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77
> 
> Without the patch provided here the error doesn't happen. How to reproduce: 
> 
> 0) Apply patch provided in this bug report & restart plack
> 1) Have RoundFinesAtPayment = No (not sure this is relevant though)
> 2) Create 3 manual invoices with the amount of 3.33
> 3) Make a payment > write off selected
> 4) The error should appear.

Interesting, I have no idea why I didn't get this error in my testing! Followup incoming!
Comment 13 Kyle M Hall 2020-07-30 13:37:01 UTC
Created attachment 107589 [details] [review]
Bug 26076: (QA Follow up) Prevent error by using a fresh resultset
Comment 14 Kyle M Hall 2020-07-30 13:37:28 UTC
Created attachment 107590 [details] [review]
Bug 26076: Sum the amount due in the database query instead of a loop in Perl

This appears to be a bug due to inaccuracies in floating point math. I've been unable to reproduce it on demand. Sometimes when using the "Write off selected" button, Koha will give the error that the payment is more than the amount owed, even though they are the same. The solution I've implemented is to move the summation from Perl code to the database query. This video demonstrates the issue and afterward, shows the error goes away after the patch is applied: https://monosnap.com/file/pG69HC7iI9mU9kkuoCtbkVzVTffKlE

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Verify that "Write off selected" functions as usual

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>
Comment 15 Kyle M Hall 2020-07-30 13:37:37 UTC
Created attachment 107591 [details] [review]
Bug 26076: (QA Follow up) Prevent error by using a fresh resultset
Comment 16 Katrin Fischer 2020-08-15 10:28:14 UTC
I was able to reproduce the error: When adding the manual invoices for testing, enter values with more than 2 decimals. But it only prevents me from paying when the amount outstanding and the amount being paid are off by one cent.

I believe this patch doesn't hurt things, but it would be great to see what's going on in the db when this error appears.
Comment 17 Katrin Fischer 2020-08-15 10:39:02 UTC
Found Joonas comment now:

(In reply to Joonas Kylmälä from comment #6)
> How to reproduce:
>  1) Create manual invoice => amount 1.00
>  2) Create manual credit => amount 0.011111
>  3) Make a payment => Click Apply -0.01
>  4) Click "Pay amount" and then on the next page "Confirm"

Following this test plan, I can still not pay the remaining fine off, the error is as before:

You must pay a value less than or equal to 0.99.
Comment 18 Joonas Kylmälä 2020-08-17 06:37:18 UTC
(In reply to Katrin Fischer from comment #17)
> Found Joonas comment now:
> 
> (In reply to Joonas Kylmälä from comment #6)
> > How to reproduce:
> >  1) Create manual invoice => amount 1.00
> >  2) Create manual credit => amount 0.011111
> >  3) Make a payment => Click Apply -0.01
> >  4) Click "Pay amount" and then on the next page "Confirm"
> 
> Following this test plan, I can still not pay the remaining fine off, the
> error is as before:
> 
> You must pay a value less than or equal to 0.99.

Katrin, this error above I erroneously identified of being related to this. The error Kyle reported here was a rounding error and this error I put steps to reproduce intentionally creates more decimals than the intranet interface can show or accepts payments for. Even if the intranet interface was fixed the rounding error would still be there. Moving back to "Signed off".
Comment 19 Katrin Fischer 2020-08-17 06:45:55 UTC
> > You must pay a value less than or equal to 0.99.
> 
> Katrin, this error above I erroneously identified of being related to this.
> The error Kyle reported here was a rounding error and this error I put steps
> to reproduce intentionally creates more decimals than the intranet interface
> can show or accepts payments for. Even if the intranet interface was fixed
> the rounding error would still be there. Moving back to "Signed off".

Thx Joonas. So we still have no way to recreate the rounding issue, is that correct?
Comment 20 Katrin Fischer 2020-08-18 06:22:16 UTC
Joonas, I think you are in deeper here than me - could you check the follow-up and finish up here?
Comment 21 Katrin Fischer 2020-08-22 21:47:34 UTC
Created attachment 108938 [details] [review]
Bug 26076: Sum the amount due in the database query instead of a loop in Perl

This appears to be a bug due to inaccuracies in floating point math. I've been unable to reproduce it on demand. Sometimes when using the "Write off selected" button, Koha will give the error that the payment is more than the amount owed, even though they are the same. The solution I've implemented is to move the summation from Perl code to the database query. This video demonstrates the issue and afterward, shows the error goes away after the patch is applied: https://monosnap.com/file/pG69HC7iI9mU9kkuoCtbkVzVTffKlE

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Verify that "Write off selected" functions as usual

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2020-08-22 21:47:38 UTC
Created attachment 108939 [details] [review]
Bug 26076: (QA Follow up) Prevent error by using a fresh resultset

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2020-08-22 21:48:27 UTC
As the error has proven hard to replicate I am passing this mostly on base of regression testing.
Comment 24 Jonathan Druart 2020-08-24 10:33:33 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 25 Lucas Gass 2020-09-04 17:58:07 UTC
backported to 20.05.x for 20.05.04
Comment 26 Aleisha Amohia 2020-09-10 04:05:14 UTC
backported to 19.11.x for 19.11.10
Comment 27 Victor Grousset/tuxayo 2020-09-20 18:16:20 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.