Skip to content
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

Use shared configuration for shortcode- and block-based checkout flows #1585

Merged
merged 30 commits into from
Jun 10, 2021

Conversation

reykjalin
Copy link
Contributor

Changes proposed in this Pull Request:

Issue: Link to the GitHub issue this PR addresses (if appropriate).
Fixes #1516

  1. Refactors the JS configuration preparation into static functions.
    • I'm a little unsure of this approach; is there a better way to make these configurations available to the Blocks Support class?
  2. Calls the static JS configuration functions and uses the result for the blocks configuration.
  3. Make sure only the Blocks JS is loaded on blocks pages.

Testing instructions

Rather convoluted; test all the general payment flows, e.g.

  • Payment with new card, saved card (Blocks + Shortcode)
  • Payment with Payment Request Buttons (PRBs) (Blocks + Shortcode)
  • 3DS with both PRBs, new card, saved cards (Blocks + Shortcode)
  • Subscription payments (Blocks + Shortcode + Subscriptions >= 3.1.0)
    • Including renewals.
  • Pre-Orders (Shortcode only, Blocks are not supported)
  • Rendering of default, branded, and custom PRBs.
  • Failed payments (Blocks + Shortcode)

Things to watch out for:

  • Any sort of JS errors.
  • Configurations that don't make it to the front-end (probably associated with JS errors).
  • Discrepancies between the Shortcode- and Blocks-based cart/checkout pages.

  • Make sure your changes respect WordPress' coding standards.
  • Did you make changes, or create a new .js file? If Gruntfile.js exists in the repo, make sure to run grunt.

@reykjalin reykjalin changed the title Use shared configuration for shortcode and checkout flows Use shared configuration for shortcode- and block-based checkout flows May 27, 2021
@reykjalin reykjalin marked this pull request as ready for review June 1, 2021 23:28
@reykjalin reykjalin requested a review from ricardo June 2, 2021 00:35
@reykjalin reykjalin requested a review from dechov June 2, 2021 00:36
@reykjalin reykjalin assigned dechov and unassigned ricardo Jun 2, 2021
Copy link
Contributor

@dechov dechov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! The refactor looks good in general, and I've verified that the "stripe.js" front-end script asset loads only on shortcode cart/checkout, not for the block 👍

I was originally seeing an error in the Checkout block due to publicKey being passed, since we do actually still want the (reverted) change here.

With that change, payments (incl. 3DS) work with new and saved cards, with both shortcode and blocks. However, Apple Pay in the Cart and Checkout blocks is failing for me, with this error:

Unhandled Promise Rejection: ReferenceError: Can't find variable: wc_stripe_payment_request_params

(I can test some more cases tomorrow.)

includes/class-wc-gateway-stripe.php Show resolved Hide resolved
includes/class-wc-stripe-blocks-support.php Outdated Show resolved Hide resolved
includes/class-wc-gateway-stripe.php Outdated Show resolved Hide resolved
includes/class-wc-gateway-stripe.php Outdated Show resolved Hide resolved
@reykjalin
Copy link
Contributor Author

I was originally seeing an error in the Checkout block due to publicKey being passed, since we do actually still want the (reverted) change here.

With that change, payments (incl. 3DS) work with new and saved cards, with both shortcode and blocks. However, Apple Pay in the Cart and Checkout blocks is failing for me, with this error:

Unhandled Promise Rejection: ReferenceError: Can't find variable: wc_stripe_payment_request_params

(I can test some more cases tomorrow.)

Ugh, sorry about this 🤦
I must've screwed the revert up while fixing the conflicts that happened during the revert. Sorry about that!

I've pushed a fix in cc9e6e6 and ran some tests to verify that it fixes the issue. Sorry about that!

Instead of not loading the shortcode JS when a Block is present on the
current page, we now load the shortcode JS when a cart or checkout
shortcode is present on the page.
Copy link
Contributor

@dechov dechov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears to work well 👍 – I'll do just a bit more testing in a bit.

Minor note: In general, are there known cases in which getStripeServerData() will be null, and all the optional chaining would make a difference? Looks like it was added to many of the calls but not all – is there a rule here?

client/blocks/credit-card/payment-method.js Outdated Show resolved Hide resolved
client/blocks/payment-request/index.js Outdated Show resolved Hide resolved
client/blocks/stripe-utils/normalize.js Outdated Show resolved Hide resolved
client/blocks/stripe-utils/type-defs.js Outdated Show resolved Hide resolved
includes/class-wc-stripe-blocks-support.php Show resolved Hide resolved
@reykjalin
Copy link
Contributor Author

In general, are there known cases in which getStripeServerData() will be null, and all the optional chaining would make a difference? Looks like it was added to many of the calls but not all – is there a rule here?

Not any known cases; at least not in terms of a successful flow. That said, Darren mentioned in #1467 (comment) on the original Blocks support PR that this function could potentially return null, and there's no real guarantee that the shape of the JS object doesn't change down the line.

So I guess you're mostly wondering if it'd be better to have the front-end crash if we change something during development rather than handle the error gracefully without any notification that's what happened?
I'm not sure what's better, but I personally lean towards handling this silently with null coalescing. What do you think?

@dechov
Copy link
Contributor

dechov commented Jun 10, 2021

Well I meant in terms of a possible flow, not necessarily successful. I'm fine with the optional chaining but was mostly wondering why it's in some places and not others, and if there's a pattern there – this example (among others) was left alone:

getStripeServerData().inline_cc_form === 'yes'

@reykjalin
Copy link
Contributor Author

Thanks for calling out the lack of optional chaining Paul!

I've fixed all instances I could find in df49b46 🙂

Copy link
Contributor

@dechov dechov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All LGTM 🎉 – tested most of the cases in the testing instructions (though not every single permutation) and some others, and tried various config like PRB settings, inline form, disallowing prepaid cards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use shared configuration for Block and Shortcode JS
3 participants