Money rounding error in TPAC credit card payment interface

Bug #1282751 reported by Remington Steed
14
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Undecided
Unassigned
2.10
Fix Released
Undecided
Unassigned
2.11
Fix Released
Undecided
Unassigned

Bug Description

While paying a fine by credit card in the TPAC, I encountered a money rounding error. The single fine was for $1.15, but the total immediately below it is displayed as $1.14 (see attached screenshot). If I attempt to pay the fine via credit card (via Stripe payment service), the confirmation page again says I am about to pay $1.14, but after the payment completes the receipt page says "Paid 1.15 via credit card". It displays correctly everywhere else in the interface (e.g. the "dashboard" at the top-right of the page, the initial fine-selection page).

This may require a larger discussion about using floating-point math vs. money-safe math, and there may be multiple places in the Evergreen code affected by this issue, as mentioned in this bug:

https://bugs.launchpad.net/evergreen/+bug/996029

Revision history for this message
Remington Steed (rjs7) wrote :
Revision history for this message
Bill Erickson (berick) wrote :

Just encountered this bug. Patron had 100+ small billings ($0.10, $0.20, etc.). The final amount to pay was "9.999999999...", which PayPal naturally rejected. I traced this to the Money.pm code that's adding the amounts to create the $total_paid amount. Using the $amount*100; $total_paid/100 trick resolves it. Will post a patch after more testing.

Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
status: New → Confirmed
Revision history for this message
Bill Erickson (berick) wrote :

After re-reading the description, I believe these are two separate issues. The issue I'm describing does not happen until the payment is actually sent to the provider. In other words, the payment API, not the TPAC display code.

By all accounts, the TPAC code building the payment page has been round-protected since EG ~2.2. See Account.pm:prepare_fines(). Not sure what's going on here. I'll open a separate ticket for my issue.

Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Revision history for this message
Bill Erickson (berick) wrote :

For reference #1474566

Revision history for this message
Bill Erickson (berick) wrote :

Returning to the original bug...

I have confirmed the original bug. It's easy to recreate. Create a single grocery bill for $8.29 then attempt a credit card payment. The total amount to pay in the UI will display as $8.28.

This can also be easily reproduced with:

perl -e 'print "no match\n" unless ((8.29 * 100) == int(8.29 * 100))';

The Perl int() docs warn against this kind of thing... I have also confirmed the AppUtils::fpsum() function, which was recently added to address these kinds of rounding issues (and does not use int()), produces the correct results.

Patch en route.

Revision history for this message
Bill Erickson (berick) wrote :

Fix pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1282751-cc-payment-rounding-error

Note that I could have accomplished the same thing by simply removing the int() calls in the summing routine (with fewer code changes), but in this case I thought it was worth it to leverage the fpsum() utility function to help reign in the proliferation of floating point summing code. Plus, I think it's slightly more readable.

To test:

1. Create a single grocery fine on a patron account for 8.29.
2. As the patron, log in to the catalog, then change the URL to https://EVERGREEN_HOST/eg/opac/myopac/main_payment_form -- It's not necessary to have CC payments enabled to view this page.
3. Confirm the total amount to pay is $8.28 before the above patch and $8.29 after the above patch.
4. To further test, add more fines and payments and confirm amounts correctly add up in the same interface.

Changed in evergreen:
milestone: none → 2.11-rc
tags: added: pullrequest
Changed in evergreen:
milestone: 2.11-rc → 2.11.1
Revision history for this message
Mike Rylander (mrylander) wrote :

I've picked this into 2.10-master. Thanks, Bill!

Changed in evergreen:
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
milestone: 2.11.1 → 2.next
status: Fix Released → Fix Committed
Changed in evergreen:
milestone: 2.next → 2.12-beta
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.