-
Notifications
You must be signed in to change notification settings - Fork 219
Add payment method api and components to checkout steps #1349
Conversation
e33ae7c
to
4c1e5de
Compare
@nerrad On the payment side, is it safe to do payments on client side? I worry about tampering. I was thinking we'd need to a) create an order (draft) on server side so values are correct, then b) trigger payment from client side, to be handled on server side by a gateway. e.g. Ref an order ID and send the payment handler (on server) a token, or payment details, or simply process and return something such as a redirect URL. |
payments won't be handled by handled by the new checkout flow. they will be handled by existing payment methods hooked into the checkout flow (so their existing process for handling payments should remain intact for the most part) |
@nerrad Ok, I was looking at |
|
65774f3
to
be42c0a
Compare
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 have some thoughts, some of them trivial. This looks promising so far :)
assets/js/base/hooks/payment-methods/use-active-payment-method.js
Outdated
Show resolved
Hide resolved
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.
Very nice work here. I'd love to talk though the similarities/differences with CompositeCheckout to see how both could be improved!
activePaymentMethod, | ||
setActivePaymentMethod, | ||
} = useActivePaymentMethod(); | ||
const getRenderedTab = useCallback( |
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.
What's the purpose of memoizing this function? Is it to preserve something in the payment method component?
If it's just to inject the selectedTab
as a render prop, could we use a custom hook (eg: getSelectedTab
) instead?
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.
Probably no need to memoize actually.
If it's just to inject the selectedTab as a render prop, could we use a custom hook (eg: getSelectedTab) instead?
Hmm... I'm not sure I follow what you are suggesting here. Do you mean useSelectedTab
?
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.
Hmm... I'm not sure I follow what you are suggesting here. Do you mean useSelectedTab?
Oh, um, yes. I think that is probably what I meant. 😅 Sorry.
be42c0a
to
8acd0bc
Compare
51216a2
to
d6c7e4f
Compare
( Object.keys( paymentMethods ).length === 0 && isInitialized ) | ||
) { | ||
// @todo this can be a placeholder informing the user there are no | ||
// payment methods setup? |
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.
...but only in the editor. Might want to consider something for the frontend to communicate to customers that payments can't be completed?
assets/js/blocks-registry/payment-methods/express-payment-method-config.js
Outdated
Show resolved
Hide resolved
assets/js/blocks-registry/payment-methods/express-payment-method-config.js
Outdated
Show resolved
Hide resolved
289bfe4
to
484771e
Compare
Looking good! |
d7ac549
to
8149971
Compare
this breaks the failing unit tests
I feel good about the state of this pull, getting it merged, and continuing on with tasks in followups (see #1349 (comment) for already created list). So this could use a final review to catch any major issues and get it merged in? |
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.
All the feedback I provided above has been addressed (thanks @nerrad!) and taking an overall look at the code it looks good to me. I added some suggestions after a second look at the code. They are all nitpicking comments in case you want to consider them, but I'm ok with how this PR is now so approving. 👍
Addresses: #1347 and #1302
This pull is a reference pull (so may not be directly implemented as is) for a proposed approach to implementing payment methods in the new checkout block. It takes some inspiration from the composite checkout api in calypso, however, there are aspects of this that differ due to the need for extensibility in a WordPress plugin environment.
Some goals of this proposal:
With this in mind, this pull documents a structural approach to handling payment method integration with the new checkout block:
API
CheckoutContext (commit b85b11a)
The checkout context is the mechanism by which payment method and checkout data etc is made available to all components within the checkout component tree.
The
CheckoutProvider
initializes the api for the checkout and allows for setting an initialActivePaymentMethod (which may be something configured in Woo Core?) .Consumers of the context should generally not reference the various elements on the context directly but instead consume from the more granular hooks. These hooks will be documented further in later paragraphs including the api attached to them.
You can view the
CheckoutProvider
implemented in the checkout block here: f0509e8Checkout Hooks (see 316656d)
These are various hooks that can be used in various checkout components (along with being implemented in payment method components) to expose different slices of the checkout context api.
useCheckoutData
For this example, this is hardcoded. However, most likely this will be wired up to any
wp.data
stores retrieving cart/checkout data. This data is passed through to the registered payment method components. There is also aupdateCheckoutData
callback that could be used by other components in the checkout form (when things change?).Questions:
useCheckoutEvents
The
useCheckoutEvents
hook exposes the following from the Checkout context.checkoutComplete
andsetCheckoutComplete
. This event state would be set when the checkout has completed and provides a callback for setting that state.checkoutHasError
andsetCheckoutHasError
. This event state would be used when indicating that the checkout currently has an error state. Errors would likely be exposed via notices (seeuseCheckoutNotice
). It might also influence validation schemes (which aren't a part of this pull currently).isCalculating
andsetIsCalculating
. This checkout event state could be utilized when the checkout is recalculating totals. Things that might trigger this are shipping selections, changes in product quantity, variation adjustments etc.Questions:
useCheckoutNotices
This hook still isn't fully fleshed out, but it would be utilized as a way for managing notices in the checkout context. Three interfaces are exposed via this hook:
notices
- an array of current notices.addNotice
- a callback for adding a notice.removeNotice
- a callback for removing a notice.In building out this api, it'd probably be good to formalize a
Notice
object so we can validate type etc.useCheckoutRedirectUrls
This hook allows for setting different urls that the checkout should redirect too on completion. There are 4 interfaces exposed by this hook:
successRedirectUrl
andsetSuccessRedirectUrl
: This is the url that would be redirected to on successful payment.failureRedirectUrl
andsetFailureRedirectUrl
: This is the url that would be redirected to on failed payment.Questions:
Payment Method Hooks (see 30bd5f1)
These hooks expose various api and interfaces to payment methods that help with communicating payment and payment method state to the checkout components.
useActivePaymentMethod
This hook exposes
activePaymentMethod
andsetActivePaymentMethod
.activePaymentMethod
is the slug of the active registered payment method in the regular payment area (not express payment methods) and the callback (setActivePaymentMethod
) is used to update the active payment method when the user selects a tab. This can be fed to payment method components so the payment method component can know when it is active and have the correct logic.If
initialActivePaymentMethod
is not configured on theCheckoutProvider
then theactivePaymentMethod
will be initialized to the first registered payment method.usePaymentEvents
The
usePaymentEvents
hook exposes the current payment status and a callback for setting the payment status viaselect
anddispatch
callbacks. Passed through to payment methods, the payment method uses this to indicate it's current state to the checkout (and also the payment method itself). Payment event status would be one of the following:pristine
: (select.isPristine()
ordispatch.pristine()
). This status is when payment processing is idle and in an initialized state.started
: (select.isStarted()
ordispatch.started()
). This status indicates that payment processing has started.finished
: (select.isFinished()
ordispatch.finished()
). This status indicates the payment processing has finished. I think we might drop this one, since I don't think this state would exist considering the following three...has_error
: (select.hasError()
ordispatch.error()
). Indicates there was an error in payment processing (may or may not be finished, but is interrupted by the error. I'm assuming this state would be used for a recoverable error where the payment method could be tried again (validation error, invalid credit card, temporary communication error etc).failed
: (select.hasFailed()
ordispatch.failed()
. Indicates the payment processing failed. I'm assuming the checkout would use this state to trigger redirect to the failure url.success
: (select.isSuccessful
ordispatch.success()
. Indicates the payment processing succeeded. I'm assuming the checkout would use this state to trigger redirect to the success url.Questions:
Payment method registration api (see 83e14ad)
To fulfill the goal of keeping payment method logic separate from the checkout (and extendable). The registration api enables payment methods to register themselves with the checkout. The api is exposed on the
@woocommerce/block-registry
(orwc.wcBlocksRegistry
global via thewc-blocks-registry
handle) and would be implemented by payment methods by enqueuing their components via something like this php side:Then the paypal component javascript would register it's components and logic via the following api client side:
registerPaymentMethod
This api expects two arguments. The first,
slug
is expected to be a unique slug identifying the payment method. The second is expected to be an object with the following properties and values:tab
content
ariaLabel
Something we may need to add to this api will be
billingFields
if a payment method exposes billing fields for embedding in a checkout step (as opposed to part of a payment method modal process).registerExpressPaymentMethod
This api interface expects two arguments. The first,
slug
is expected to be a unique slug identifying the payment method. The second is expected to be a react component that returns a ui interface for triggering the express payment process. Similar to the registered payment methods mentioned earlier, this component will receive the necessary interfaces from the checkout context as props.Payment Method Wrapper Components (see 0b430a2)
These components are the wrappers around the payment methods and express payment methods registered via the payment method registration api. They handle prepping the payment methods and feeding them the needed props from the checkout context.
Example Demo of Payment Method registration/implementation (see 0b430a2)
In this commit, you can view an example of how payment methods might get registered and implemented. Obviously this is not fully functional, but illustrates how pms are kept distinct from the checkout. They can serve as the shell from which to build out functional payment methods for our initial MVP.
Next steps:
Now that this reference pull is in place, the following should happen as next steps:
Once the above is done, I would suggest breaking this pull down into smaller individual pulls that can be reviewed and merged in separately (or we could do this all in one merge and iterate...)
@wordpress/components
or some other source (or just iterate on what I started here - if we do that, we may want to look into the tab component in reakitwc.wcBlockComponents
global (and potentially move to@woocommerce/components
and serve from@woocommerce/components
). Components like various form fields/cc fields etc (can we grab some of these from the composite-checkout work?)