-
Notifications
You must be signed in to change notification settings - Fork 44
feat(modal-checkout): checkout from "My Account" dashboard #2121
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
Conversation
…ids` param; `checkout-close` ev
dkoo
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.
Very nice refactor! I really like the addition of a global newspackOpenModalCheckout function that allows other plugin code to trigger modal checkouts. I noticed some differences in the data events activity payload for checkouts that we may want to examine more closely to avoid discontinuity in analytics, but other than that this is testing well.
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.
@miguelpeixe thank you for these revisions and for starting the standardization of checkout data! In general I like the consolidation of the transaction details into the new Checkout_Data class. I think this will be much more readable and easier to follow in the future compared with how it was handled before.
I'm running into a handful of issues that I think are related to these changes. No big surprise since this is an unexpectedly big refactor of a feature that touches many different areas.
-
When doing checkouts from a paywall/content gate, the
np_modal_checkout_interactionevents don't always seem to havegate_post_idparams attached. I think this might be due to changes in how URL params are handled/passed from screen to screen within the modal iframe? In particular, theaction=openevent did have the param, butaction=loadedandaction=continuewhen transitioning between checkout screens did not. I tested againsttrunkand there, allnp_modal_checkout_interactionevents did have agate_post_idparam. -
[NON-BLOCKING, but mentioning here because it's related to the next item] This is not related to these changes, but noting in case we want to handle this edge case in the future. Variable, non-subscription products are not handled by our Checkout Button block. Attaching a variable non-subscription product to a Checkout Button and clicking the button as a reader will always result in a checkout error:
Please choose product options by visiting <a href="[product page URL] title="Product Name">Product Name</a>. -
Related to the above, on
trunkif as a reader I click a Checkout Button with a variable non-subscription product, I'll see the error message in the modal checkout window with a "Go back" button to close. On this branch, the modal checkout window will get stuck in a loading state and the error is logged to Logstash, but never shown to the reader. This is also replicable if you set a product to be limited to one per customer. Ontrunk, I see the error message below if I try to purchase a subscription product that I already own, but on this branch the modal checkout window hangs in a loading state.
Other feedback inline.
| 'product_id' => strval( $product_id ? $product_id : '' ), | ||
| 'product_type' => $product_type, | ||
| 'price_summary' => self::get_price_summary( $name, $amount, $recurrence ), | ||
| 'summary_template' => self::get_price_summary( $name, '{{PRICE}}', $recurrence ), |
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 is a neat way to build the price summary, but it's superfluous to send to GA or any other integrations using this data. Can we strip it once price_summary has been set in the data object?
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.
We need to keep the price_summary because it'll be reused every time a modal is rendered on the same page.
How about controlling the keys that are sent instead? Implemented in e74c77c
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 this approach! Makes it much easier to see and control which keys ultimately get sent where
|
The I also identified an issue with |
dkoo
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.
This is all working well now! We're just missing a handful of GA event params in our whitelist—details below. After that this should be good to merge.
| * | ||
| * @type {string[]} | ||
| */ | ||
| const eventKeys = [ |
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.
We also need action for the action parameter, e.g. seen, loaded, continue, etc. as well as all custom event params that we add in the main plugin.
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 well as all custom event params that we add in the main plugin.
Were these part of the payload before? I've never seen them here
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.
Added action in 8372718
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.
Were these part of the payload before? I've never seen them here
They're only enabled if you add a NEWSPACK_GA_ENABLE_CUSTOM_FE_PARAMS environment constant. Originally we sent them with every event, but because some publishers have custom dimensions and events in their own GA setups, they sometimes caused our GA events to exceed the parameter limit per event. They're now opt-in only via this constant.
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.
Ah, gotcha! We don't need to worry about it, then. That's hooked into googlesitekit_gtag_opt, which adds the custom parameters some other way. We don't manually send these via window.gtag( 'event' ).
Seems to work fine for me after enabling the flag.
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.
Aha, you're right! I thought I wasn't seeing them, but it's just because I was testing on a page without categories. Thanks for looking into this.
dkoo
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.
Looking good, thanks for the extensive refactoring here!
|
Hey @miguelpeixe, good job getting this PR merged! 🎉 Now, the Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label. If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label. Thank you! ❤️ |
# [4.13.0-alpha.1](v4.12.3...v4.13.0-alpha.1) (2025-06-05) ### Bug Fixes * donate block formatting ([#2133](#2133)) ([f27cb76](f27cb76)) * update stripe express checkout divider ([#2131](#2131)) ([b907452](b907452)) ### Features * **modal-checkout:** checkout from "My Account" dashboard ([#2121](#2121)) ([0c5301b](0c5301b))
|
🎉 This PR is included in version 4.13.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [4.13.0](v4.12.3...v4.13.0) (2025-06-16) ### Bug Fixes * donate block formatting ([#2133](#2133)) ([f27cb76](f27cb76)) * update stripe express checkout divider ([#2131](#2131)) ([b907452](b907452)) ### Features * **modal-checkout:** checkout from "My Account" dashboard ([#2121](#2121)) ([0c5301b](0c5301b))
|
🎉 This PR is included in version 4.13.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Introduces the
newspackOpenModalCheckout( options );function so the modal checkout can be initiated programmatically by 3rd parties.options.titlewill determine the checkout title, not overriding other modal titles the modal checkout may require (thank you/newsletters).options.actionTypewill set a customaction_typefor thecheckout_completedactivity, which currently is eithercheckout_buttonordonation.options.afterSuccesssets the after successurl,behavior, andbuttonLabelfor the checkout.options.onCheckoutCompleteis a callback to be called on thecheckout_completeRAS activity with the activitydataoptions.onCloseis a callback to be called on the newly introducedcheckout-closedcustom eventThis required a few changes to the modal checkout structure, which I commented inline.
How to test the changes in this Pull Request:
Follow instructions at Automattic/newspack-plugin#3973, but also make sure this introduces no regressions to regular modal checkout flows by donating and purchasing a subscription as a reader.
Other information: