impress-org / givewp

GiveWP - The #1 Donation Plugin for WordPress. Easily accept donations and fundraise using your WordPress website.
https://givewp.com/
GNU General Public License v3.0
345 stars 191 forks source link

Amount passed to Stripe should match the stored value #5822

Closed canny[bot] closed 3 years ago

canny[bot] commented 3 years ago

Sometimes, depending on the amount donated using Stripe + Fee Recovery, GiveWP will pass the wrong amount to Stripe, although it shows the correct amount on the GiveWP dashboard. Example:

  1. GiveWP shows $257.78 ($250 + $7.78 in fees) but Stripe dashboard shows a payment of $257.77 (resulting in net payment of $249.99 and $7.78 in fees)
  2. GiveWP shows $74.46 ($70 + $4.46 in fees) but Stripe dashboard shows a payment of $74.45 (resulting in net payment of $69.99 and $4.46 in fees)

https://givewp.canny.io/admin/board/bug-reports/p/stripe-fee-recovery-should-round-up-the-last-digit-so-it-matches-the-amount-corr

canny[bot] commented 3 years ago

This issue has been linked to a Canny post: Stripe + Fee Recovery should round up the last digit so it matches the amount correctly :tada:

kjohnson commented 3 years ago

I think I've tracked where it is going wrong:

// includes/gateways/stripe/includes/class-give-stripe-gateway.php
public function format_amount( $amount ) {
    return Money::of( $amount, give_get_currency() )->getMinorAmount();
}

price passed in is 257.78 formatted amount returned is 25777

kjohnson commented 3 years ago

Failing test to confirm the bug:

public function testMinorAmountIsNotChanged() {
    $money = Money::of( 257.78, 'USD' );
    $this->assertEquals( 25778, $money->getMinorAmount() );
}
1) MoneyTest::testMinorAmountIsNotChanged
Failed asserting that 25777 matches expected 25778.
kjohnson commented 3 years ago

I'll add that this is not specific to Fee Recovery, rather it is related to decimal values which can be reproduced with the Legacy Form Templates (which does support a decimal value as a custom donation amount).

kjohnson commented 3 years ago

As I expected, there is a float value being cast as an integer, which has the potential of data loss - due to the way floating point precision is handled. I've run across this before in Ninja Forms when making charges using Stripe (off by one cent).