-
Notifications
You must be signed in to change notification settings - Fork 801
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
Update how Cart/Checkout is tracked to be realtime. #35139
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
...pack/modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-universal.php
Show resolved
Hide resolved
94f34bd
to
509aebb
Compare
@anomiex let us know that the failing E2E test above is happening to all issues and it shouldn't prevent us from merging given everything else is passing. |
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 a logged-in user, in woocommerceanalytics_checkout_view
and woocommerceanalytics_product_checkout
events the guest_checkout
property is Yes
for me but in woocommerceanalytics_order_confirmation_view
it becomes No
.
In the testing instructions:
Do the same from the extra page you create and make sure the values are still correct.
Just noting that in this case woocommerceanalytics_checkout_view
is showing the incorrect values. Only woocommerceanalytics_product_checkout
and woocommerceanalytics_product_purchase
are correct.
...pack/modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-universal.php
Outdated
Show resolved
Hide resolved
...pack/modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-universal.php
Outdated
Show resolved
Hide resolved
} | ||
|
||
const checkoutDataStore = wp.data.select( 'wc/store/checkout' ); | ||
if ( undefined !== checkoutDataStore && checkoutDataStore.getOrderId() !== 0 ) { |
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 would check if ( 'undefined' !== typeof checkoutDataStore && checkoutDataStore.getOrderId() !== 0 )
rather than using undefined
for consistency with the above uses
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 outdated I think, because I already removed the checkoutDataStore
check because we're subscribing to it so it's always there.
.../jetpack/modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-trait.php
Show resolved
Hide resolved
.../modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-checkout-flow.php
Show resolved
Hide resolved
509aebb
to
318f2a8
Compare
@opr I addressed the review points, added tracking to the Checkout view event as well (while trying to keep it PHP only) and made sure the event triggers regardless (previously it required you to interact with Checkout for it to run, now it should run on mount). |
@@ -147,18 +147,27 @@ public function capture_checkout_view() { | |||
$is_checkout = $checkout_page_id && is_page( $checkout_page_id ) | |||
|| wc_post_content_has_shortcode( 'woocommerce_checkout' ) | |||
|| has_block( 'woocommerce/checkout', $post ) | |||
|| has_block( 'woocommerce/classic-shortcode', $post ) |
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.
There is no way to check if the block is Cart or Checkout.
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.
True, this is OK though. It's based on the template as far as I can tel. If someone has set up their Checkout page to use the Cart template then it will be an edge case, don't think we can or should try to solve for this.
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.
Happy to say this is testing OK for me, tried with several different configs and all seem to be fine for me.
🤞🏼
318f2a8
to
5a8ca44
Compare
* Update how Cart/Checkout is tracked * add extra to see from checkout is coming * address changes
if ( true === cartItem_{$cart_item_key}_logged ) { | ||
return; | ||
} | ||
wp.hooks.addAction( 'wcpay.payment-request.availability', 'wcpay', function ( args ) { |
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.
wp.hooks
is not guaranteed to be available. If it's not available, this blocks a lot of JS on the checkout from working, including coupons, "Click here to login", etc. See 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.
Thanks for raising this @jjcoffee - I agree we should add a check to ensure it's available before using.
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.
Proposed changes:
This PR changes how we're tracking Checkout block events. Instead of firing the event when Checkout loads, and assuming whatever is in the Checkout page is that, we now only react to Checkout events (shortcode and Block), and dynamically edit the
checkout_page_contains_checkout_block
andcheckout_page_contains_checkout_shortcode
values.This is done by hooking into Checkout events.
This PR also adds a
from_checkout
prop to indicate if the event came from Checkout page or not.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
checkout_page_contains_checkout_block
value is1
andcheckout_page_contains_checkout_shortcode
is0
onproduct_checkout
event, make sure thatfrom_checkout
is1
.product_purshase
event.shortcode
is 1 andblock
is 0, and thatfrom_checkout
is0
.