feat(payments-plugin): allow partial payments #2092
Conversation
| isAuthorized: true, | ||
| authorizedAsOwnerOnly: false, | ||
| channel: await server.app.get(ChannelService).getDefaultChannel(), | ||
| describe('Payment intent creation', () => { |
There was a problem hiding this comment.
Sorry for the large diff, but I grouped the testcases for readability:
| * Return to number of cents | ||
| */ | ||
| export function amountToCents(amount: Amount): number { | ||
| return Math.round(Number(amount.value) * 100); // Round because floating point errors after Number() conversion |
There was a problem hiding this comment.
I would suggest to use a dedicated library for any currency conversions/math to avoid rounding issues in general:
There was a problem hiding this comment.
@martijnvdbrug Interesting - I was wondering about this and I see from the Mollie SDK that amount.value is a string like '10.00'. Checked some values and indeed you do get rounding issues!
Number('10.21') * 100
// => 1021.0000000000001In general @StampixSMO I agree we need to be cautious with math on currency - that's why Vendure only stores integers for currency values. In this case, I think the handling is fine because we're dealing here with an artifact of "floating-point numbers in binary" rather than an actual rounding error caused by multiplying by a non-integer, so the error is only ever so tiny as to be ignored.
There was a problem hiding this comment.
Mollie returns a string on purpose, it avoids FP errors due to frameworks along the way. It's meant to be used with a professional library like currency.js for math and parsing.
Thanks for confirming the Vendure strategy. With integers, you implicitly mean that you store the least significant currency unit, right? E.g. dollarcents, eurocents, ... instead of dollars and euros.
There was a problem hiding this comment.
Oke, the library is tiny, so I'll add it :-)
There was a problem hiding this comment.
@StampixSMO yes, we store the minor unit.
| adminClient, | ||
| order.lines[0].id, | ||
| 10, | ||
| order.payments[2].id, |
There was a problem hiding this comment.
Use the payments[2] now instead of [1] here, because we've added an extra test with partial payment
martijnvdbrug
requested review from
michaelbromley and
StampixSMO
and removed request for
michaelbromley and
StampixSMO
|
@michaelbromley Reallyyyy..., now its only the MySQL test that's failing. Same error. I might need your help on this, still don't quite understand whats going on under the hood with the multiple refunds. |
No description provided.