feat(payment): PAYPAL-3520 Max capture amount failure #2485
Conversation
bc-nick
requested review from
serhii-tkachenko,
andriiVitvitskyi1990 and
bc-dronov
| if (isPaypalCommerceProviderError(error)) { | ||
| const paypalProviderError = error?.errors?.filter((e: any) => e.provider_error) || []; | ||
|
|
||
| return paypalProviderError[0].provider_error?.code === '2046'; |
There was a problem hiding this comment.
Maybe it would be better to create a separate constant for '2046' error? How do you think?
There was a problem hiding this comment.
Sorry for the late response. I think it is not necessary, we use this code only here and nowhere else
bc-nick
force-pushed
the
PAYPAL-3520
branch
3 times, most recently
from
e336fc6
to
ad8f56a
Compare
| @@ -9,7 +9,8 @@ import { | |||
| PaymentStrategyFactory, | |||
| toResolvableModule, | |||
| } from '@bigcommerce/checkout-sdk/payment-integration-api'; | |||
| import { Overlay } from '@bigcommerce/checkout-sdk/ui'; | |||
| import { LOADING_INDICATOR_STYLES } from '@bigcommerce/checkout-sdk/paypal-commerce-utils'; | |||
There was a problem hiding this comment.
We should not use PayPal Commerce specific code in Braintree integration. We have 2 ways to solve this issue:
- move
LOADING_INDICATOR_STYLESintouipackage and use it from there (but leave ppcp refactoring for another PR); - make a copy of
LOADING_INDICATOR_STYLESand paste it somewhere in braintree related package;
| @@ -316,4 +484,35 @@ export default class BraintreePaypalPaymentStrategy implements PaymentStrategy { | |||
|
|
|||
| throw new PaymentMethodFailedError(error.message); | |||
| } | |||
|
|
|||
| private isProviderError(error: unknown): boolean { | |||
| if (isPaypalCommerceProviderError(error)) { | |||
There was a problem hiding this comment.
We should not use paypal commerce related code inside brainteree strategy. Please create another typeguard for that.
| } | ||
| } | ||
|
|
||
| private renderPayPalMessages(initializationData?: any) { |
There was a problem hiding this comment.
any type is forbidden for any new code in checkout sdk
There was a problem hiding this comment.
Made decision to not pass this parameter, moved it to higher level
| onError(new Error('INSTRUMENT_DECLINED')); | ||
| } | ||
|
|
||
| reject(); |
There was a problem hiding this comment.
Why do we need to reject promise in this case?
There was a problem hiding this comment.
We need to do this to avoid the execution code freeze. Code will not execute correctly without reject calling
| return Promise.resolve({ | ||
| ...payment, | ||
| paymentData: this.formattedPayload(token), | ||
| }); |
There was a problem hiding this comment.
It is not necessary to make Promise.resolve here
| return Promise.resolve({ | |
| ...payment, | |
| paymentData: this.formattedPayload(token), | |
| }); | |
| return { | |
| ...payment, | |
| paymentData: this.formattedPayload(token), | |
| }; |
| } | ||
| } | ||
|
|
||
| private async tokenizePayment( |
There was a problem hiding this comment.
| private async tokenizePayment( | |
| private async tokenizePaymentOrThrow( |
| this.renderPayPalMessages(braintreeOptions?.bannerContainerId); | ||
| (braintreePaypalCheckout) => { | ||
| this.renderPayPalMessages(initializationData); | ||
| this.renderPayPalButton(braintreePaypalCheckout); |
There was a problem hiding this comment.
It looks a bit complicated.. because when you read the code you can see that loadPaypalCheckoutInstance method might be called up to 3 times, and it is not quite clear to me because we don't really render PayPal message or Button in all cases.
We can put it on hold and refactor in the future, just want to let you know that here is a place, where the code should be updated.
| await new Promise((_resolve, reject) => { | ||
| if (onError && typeof onError === 'function') { | ||
| onError(new Error('INSTRUMENT_DECLINED')); | ||
| } | ||
|
|
||
| reject(); | ||
| }); |
There was a problem hiding this comment.
I don't really see why do we need this promise.. To avoid code freeze we can simply remove the promise, since there is no reason to wait for something.
| await new Promise((_resolve, reject) => { | |
| if (onError && typeof onError === 'function') { | |
| onError(new Error('INSTRUMENT_DECLINED')); | |
| } | |
| reject(); | |
| }); | |
| if (onError && typeof onError === 'function') { | |
| onError(new Error('INSTRUMENT_DECLINED')); | |
| } |
| @@ -5,7 +5,7 @@ module.exports = { | |||
| 'ts-jest': { | |||
| tsconfig: '<rootDir>/tsconfig.spec.json', | |||
| diagnostics: false, | |||
| } | |||
There was a problem hiding this comment.
It is not necessary to do this change in current PR
What?
Handling an specific error related to max capture failure
Why?
To show the error message with an appropriate information for users
Testing / Proof
Manual testing
Tested with mock data
PSD2_braintree.mov
@bigcommerce/team-checkout @bigcommerce/team-payments