-
Notifications
You must be signed in to change notification settings - Fork 220
fix(payments): Updated payment providers vs methods #19920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
6b5861a to
01d5ee3
Compare
Because
Sentry events frequently stating the "Server Components render" error occur for the CheckoutForm component
CheckoutForm is a client component, but it is importing the Node server SDK for Stripe causing Next to try to bundle Stripe Node SDK into the client build and surfacing the Server Components render error
Validation for paymentProvider was expecting an enum
This occurred as some of the options for payment methods (e.g. Apple Pay, Google Pay) during checkout were not considered valid payment provider types.
We should distinguish payment providers and payment methods.
This pull request
Updates definition/usage of payment providers vs methods
Issue that this pull request solves
Closes: PAY-3439
StaberindeZA
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like splitting out payment method and payment provider. I left a few comments with issues and a few questions.
Additionally, I noticed that the payment_provider differed on the Glean submit event vs success event.
| { ...params }, | ||
| Object.fromEntries(searchParams), | ||
| 'external_paypal' | ||
| 'external_paypal' as SubPlatPaymentMethodType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: do not use type assertion
| { ...params }, | ||
| Object.fromEntries(searchParams), | ||
| selectedPaymentMethod as PaymentProvidersType | ||
| selectedPaymentMethod as SubPlatPaymentMethodType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: do not use type assertion
| @IsOptional() | ||
| @IsEnum(PaymentProvidersTypePartial) | ||
| paymentProvider?: PaymentProvidersType; | ||
| paymentMethod?: SubPlatPaymentMethodType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this should have a validation check that the provided values match an enum
| | 'external_paypal'; | ||
| | 'paypal'; | ||
|
|
||
| export type SubPlatPaymentMethodType = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is this type still necessary?
Is this type just a subset of values from Stripe.PaymentMethod.Type? What would the value be for an IAP payment method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this type just a subset of values from Stripe.PaymentMethod.Type?
Yes
What would the value be for an IAP payment method?
No,SubPlatPaymentMethodTypeonly includes the current accepted payment methods during Checkout.
Because
Sentry events frequently stating the "Server Components render" error occur for the CheckoutForm component
This occurred as some of the options for payment methods (e.g. Apple Pay, Google Pay) during checkout were not considered valid payment provider types.
We should distinguish payment providers and payment methods.
This pull request
Issue that this pull request solves
Closes: PAY-3439
Checklist
Put an
xin the boxes that apply