Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Improve Payment Methods integration API so either onPaymentProcessing event fires for savedTokenHandler components or onCheckoutBeforeProcessing allows updating POST #3980

Closed
nerrad opened this issue Mar 19, 2021 · 2 comments · Fixed by #3982
Assignees
Labels
block: checkout Issues related to the checkout block. category: extensibility Work involving adding or updating extensibility. Useful to combine with other scopes impacted.

Comments

@nerrad
Copy link
Contributor

nerrad commented Mar 19, 2021

I was notified by the Square payment integration team (hi @mattallan 👋 ) that their saved token handling works a bit differently than the payment method integration API assumes and requires the ability to get the 3DS verification token before the initial checkout submission. The current onCheckoutSuccessAfterProcessing event won't work here because the Square extension still needs to process this buyer_verification_token server side after the authorization (to create the payment) and currently onCheckoutSuccessAfterProcessing will not have a path that re-submits to the server unless there is an error and retry is true. That ui/ux workflow won't work for the Square needs here.

Options

There are a few options that will resolve Square needs here:

1. Provide a way for onCheckoutSuccessAfterProcessing callbacks to trigger doing a resubmit to the checkout server.

This scenario would expose a way for handlers to indicate they've updated something in the checkout that requires a resubmission to the server for re-validating whatever was updated.

Pros:

  • likely the least amount of impact to the current flow and preserves the current behaviour around what events update the POSTED data.
  • checkout state is already updated by the handlers via the setComplete action (but only if ! shouldRetry( response) - which is kind of odd here, because the state won't get used anywhere in this scenario).

Cons:

  • Re-submitting at this point seems kind of counter-intuitive given the checkout has already been processed. The current flow already accounts for scenarios where if some sort of error occurs at this point, then the checkout state goes to error state and then idle (if possible shopper to fix error state).
  • Potential side-effects on the checkout server processing side if it's already been processed successfully and then re-submitted?

2. Expose onPaymentProcessing event for checkout submission including savedPaymentTokens

This scenario would refactor the existing flow so that the onPaymentProcessing event fires as a part of saved tokens being included in the checkout submission.

The reason it doesn't currently is because the assumption was made in the design that if there is a saved token, that means there is no client side payment processing needed, and thus the payment status for a saved token is SUCCESS (which is a status saying client side payment processing completed successfully).

Pros:

  • The original assumption that client side processing isn't needed for saved tokens is flawed, and including this as a legit event would keep the flow consistent anywhere payment methods are involved.
  • Modification of what gets sent through POST is updated more cleanly by payment methods as a part of an event that makes sense for payment methods.

Cons:

  • There could be some difficulty here in ensuring we preserve the submission of saved tokens without a handler where the payment status is set to 'SUCCESS' for the onPaymentProcessing event. However, I think we could resolve this by simply having a default handler set for saved tokens that don't have one explicitly defined. Alternatively (which might be better), when the onPaymentProcessing event goes through all the subscribed observers, and the PAYMENT_STATUS has not been updated to success, we could trigger the update to success so that checkout proceeds (probably should be doing this anyways if we're not).

3. Allow for updating the checkout state with the onCheckoutBeforeProcessing event.

Currently this event doesn't impact any updating of state (other than validation/error state) and is mostly used for initial checkout form validation. With this option, we'd also allow updating more of the state (in both contexts, when checkout returns to idle, and when checkout is set to Processing).

Pros:

  • exposes another event where state can be updated.

Cons:

  • The naming of this event is actually a bit unfortunate given it mostly has the purpose of validation of things in the checkout. So even though it does fire before sending to the server, it only handles validationErrors and messages that are the response from event subscriber callbacks. Might be good to consider renaming to onCheckoutValidationBeforeProcessing to make the event more clear (we could deprecate the old event function and just redirect to the new name).
  • Updating the state (beyond what is currently done) during this process will likely have unintended consequences because it currently was designed explicitly for validation of the checkout before passing on to other stages in the checkout flow (not for updating what gets sent to the server).

What do do?

I'm leaning towards Option 2 here because that seems like the most optimal improvement that resolves this situation AND also preserves the original intent behind the design of the flow. It also helps ensure anything to do with Payment methods in the checkout flow behaves consistently (event saved tokens) where if there is some form of payment method, the onPaymentProcessing event will always fire.

As an aside, I also think it be a good idea to rename the onCheckoutBeforeProcessing event to onCheckoutValidationBeforeProcessing.

@nerrad nerrad self-assigned this Mar 19, 2021
@nerrad nerrad added category: extensibility Work involving adding or updating extensibility. Useful to combine with other scopes impacted. block: checkout Issues related to the checkout block. labels Mar 19, 2021
@Aljullu
Copy link
Contributor

Aljullu commented Mar 19, 2021

Thanks for investigating this issue and proposing several possible solutions @nerrad!

In the Slack combo of this morning, I was confused by the fact that saved payment methods don't fire the onPaymentProcessing event. I can imagine 3rd-parties will be confused by this as well, so making the checkout flow more similar between saved and new payment methods would be an improvement IMO.

I leave the final decision to you, but as I was reading through this issue, the option 2 was the one I was leaning towards too, because it removes the flow difference between saved/new payment methods and because it looks like all cons listed can be worked-around one way or another. So a +1 to your conclusions in the What do do? section.

As an aside, I also think it be a good idea to rename the onCheckoutBeforeProcessing event to onCheckoutValidationBeforeProcessing.

I had a bad time thinking of use-cases for onCheckoutBeforeProcessing other than validating data, so your rationale sounds good to me.

@nerrad
Copy link
Contributor Author

nerrad commented Mar 20, 2021

I created #3984 for the rename of the onCheckoutBeforeProcessing hook.

@nerrad nerrad added needs: dev note PR that has some text that needs to be included in the release notes. and removed needs: dev note PR that has some text that needs to be included in the release notes. labels Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
block: checkout Issues related to the checkout block. category: extensibility Work involving adding or updating extensibility. Useful to combine with other scopes impacted.
Projects
None yet
2 participants