-
Notifications
You must be signed in to change notification settings - Fork 672
Conversation
Need to be merged together with saleor/saleor#7452 |
confirmationData: any; | ||
confirmationNeeded: boolean; | ||
}) => Promise<any>; | ||
submitPayment: (data?: object) => Promise<any>; |
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.
Isn't there a specific type we could use for the returned Promise?
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.
Types for data and promise can be more specific
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.
And if there isn't, it should be of type unknown
, not any
.
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.
Fixed
import { IFormError } from "@types"; | ||
|
||
import { StripeCreditCardForm } from "../StripeCreditCardForm"; | ||
import { IProps } from "./types"; | ||
|
||
const messageDescription = "Stripe payment gateway error"; | ||
|
||
export const stripeErrorMessages = defineMessages({ |
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.
Maybe this stuff could go into intl.ts?
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.
Sure, moved to separate intl.ts file for Stripe
]); | ||
return; | ||
} | ||
let confirmation; |
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.
This could probably be done via const inside the try/catch block, followed by optional chaining wherever a potentially undefined key of this variable is accessed.
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.
Not sure if I understand correctly, const or let are scoped variables, so cannot be accessed outside of try/catch. Nevertheless, I have extracted these try/catch to the separate functions with immediate value return and I have added early return instead of if/else in this function, so I hope it is a little bit cleaner now.
), | ||
]); | ||
} else { | ||
let paymentAction; |
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.
This could probably be done via const inside the try/catch block, followed by optional chaining wherever a potentially undefined key of this variable is accessed.
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.
Same here
/** | ||
* Method to call on gateway payment submission. | ||
*/ | ||
submitPayment: (data?: object) => Promise<any>; |
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.
Same as earlier, would it be possible to be more specific about the Promise return type?
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.
fixed
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.
Generally, control flow in StripePaymentGateway.tsx can be potentially improved.
confirmationData: any; | ||
confirmationNeeded: boolean; | ||
}) => Promise<any>; | ||
submitPayment: (data: object) => Promise<any>; |
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.
Types for data and promise can be more specific
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.
Data type cannot be easily guessed due to the unknown Adyen gateway object, which will be returned, so I would rather stay with an object for now and also this object is simply passed to the backend where it is processed. Promise type is fixed.
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.
As I can see, the overall structure of the object (all possible properties) is known, so even if objects in use may differ on case-by-case basis, there is still some room to apply type constraints. Something that can be considered in future, if not now.
confirmationData: any; | ||
confirmationNeeded: boolean; | ||
}) => Promise<any>; | ||
submitPayment: (data?: object) => Promise<any>; |
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.
Types for data and promise can be more specific
export const stripeErrorMessages = defineMessages({ | ||
gatewayMisconfigured: { | ||
defaultMessage: "Stripe gateway misconfigured. Api key not provided.", | ||
description: messageDescription, | ||
}, | ||
paymentSubmissionError: { | ||
defaultMessage: | ||
"Payment submission error. Stripe gateway returned no payment method in payload.", | ||
description: messageDescription, | ||
}, | ||
geytwayDisplayError: { | ||
defaultMessage: | ||
"Stripe payment gateway couldn't be displayed. Stripe elements were not provided.", | ||
description: messageDescription, | ||
}, | ||
paymentMethodNotCreated: { | ||
defaultMessage: "Payment method has not been created.", | ||
description: messageDescription, | ||
}, | ||
}); |
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.
Maybe worth using shorthand notation for description property?
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.
fixed
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.
404 is shown to user at the end of checkout process.
f900d4e
to
2ab9714
Compare
2ab9714
to
a938f99
Compare
I want to merge this change because... it adds new Stripe payment gateway with 3DS support.
Backend: saleor/saleor#7452
Screenshots
Pull Request Checklist
Test Environment Config
API_URI=https://feature-stripe.api.saleor.rocks/graphql/