-
Notifications
You must be signed in to change notification settings - Fork 815
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
add woo analytics event props for block/shortcode use in cart/checkout #15127
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: April 7, 2020. |
modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php
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.
This tests well for me. I only have minor remarks.
modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php
Outdated
Show resolved
Hide resolved
modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php
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.
I agree with the other reviewers' comments. Otherwise, looks good to me.
I only reviewed code, and did not test.
I'm concerned how this approach will work a year from now. If many features, such as these blocks, will result in properties being added to every woocommerceanalytics_
event, then the event data could become quite heavy. Especially with the verbose names of the new properties. I do like the self-explanatory names, though.
This is a non-issue if these are the only such properties that will be added in the foreseeable future. But, if more are coming, perhaps the properties should only be added to relevant events. Or a once-per-visit event could be sent with the details for this url
.
- consistent with other boolean tracks event props
5efc95c
to
7a6b95b
Compare
Good point @deltaWhiskey - I would definitely prefer to log these more "static" props using such an event. Let's keep this in mind as we iterate - I believe there's no standard mechanism for sending a once-per-visit event right now, so we'd need to develop that. |
Feedback addressed & rebased the branch - please re-review when ready :) @jeherve Should I address
|
No need, I don't think it's really worth it. Our CodeClimate configuration is still a work in progress; we need to update its configuration a bit more so it can provide more useful suggestions. Right now it can sometimes be helpful or provide an interesting point of view on how some things could be refactored, but really most of the time it's not super helpful imo. :) |
), | ||
); | ||
|
||
set_transient( 'jetpack-woocommerce-analytics-cart-checkout-info-cache', $info, DAY_IN_SECONDS ); |
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.
You'll want to update the transient name here as well, or make it a variable maybe to avoid further issues.
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.
D'oh! Fixed here: 6bfc817
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.
Ready for review again @jeherve :)
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 looks good in my tests. 🚢
* Initial changelog entry * Changelog: add #14904 * Changelog: add #14910 * Changelog: add #14913 * Changelog: add #14916 * Changelog: add #14922 * Changelog: add #14924 * Changelog: add #14925 * Changelog: add #14928 * Changelog: add #14840 * Changelog: add #14841 * Changelog: add #14842 * Changelog: add #14826 * Changelog: add #14835 * Changelog: add #14859 * Changelog: add #14884 * Changelog: add #14888 * Changelog: add #14817 * Changelog: add #14814 * Changelog: add #14819 * Changelog;: add #14797 * Changelog: add #14798 * Changelog: add #14802 * Changelog: add #13676 * Changelog: add #13744 * Changelog: add #13777 * Changelog: add #14446 * Changelog: add #14739 * Changelog: add #14770 * Changelog: add #14784 * Changelog: add #14897 * Changelog: add #14898 * Changelog: add #14968 * Changelog: add #14985 * Changelog: add #15044 * Changelog: add #15052 * Update to remove Podcast since it remains in Beta * Changelog: add #14803 * Changelog: add #15028 * Changelog: add #15065 * Changelog:add #14886 * Changelog: add #15118 * Changelog: add #14990 * Changelog: add #14528 * Changelog: add #15120 * Changelog: add #15126 * Changelog: add #15049 * Chanegelog: add #14852 * Changelog: add #15090 * Changelog: add #15138 * Changelog: add #15124 * Changelog:add #15055 * Changelog: add #15017 * Changelog: add #15109 * Changelog: add #15145 * Changelog:add #15096 * Changelog:add #15153 * Changelog: add #15133 * Changelog: add #14960 * Changelog: add #15127 * Changelog: add #15056 * Copy current changelog to changelog archive. * Clarify changelog description
This PR adds additional props to
woocommerceanalytics
events so we can filter based on how stores implement their cart & checkout pages.New cart & checkout blocks are coming soon in the WooCommerce Blocks plugin. We're planning to evaluate these blocks by measuring conversion rates from the existing
woocommerceanalytics
events, in particularadd_to_cart
,product_checkout
andproduct_purchase
. To compare traditional and block-based UX, we need to add these new props to the events.The new props are as follows. Values are
0
or1
, consistent with other boolean tracks props.cart_page_contains_cart_block
cart_page_contains_cart_shortcode
checkout_page_contains_checkout_block
checkout_page_contains_checkout_shortcode
Changes proposed in this Pull Request:
woocommerceanalytics
events.jetpack-woocommerce-analytics-cart-checkout-info-cache
).Is this a new feature or does it add/remove features to an existing part of Jetpack?
This adds features to the existing
woocommerce-analytics
module.Testing instructions:
Cash on delivery
payment method for easy checkout.return true;
at the start ofJetpack_WooCommerce_Analytics::shouldTrackStore()
.pixel.wp.com
.woocommerceanalytics_product_view
event. This should have new properties (parameters) as above. Check that the values are correct. You'll probably have1
for shortcodes and0
for blocks.If you install the current
master
branch of WooCommerce Blocks, you can add the cart & checkout blocks to the cart & checkout pages and retest - the values should show the correct info. If don't have blocks plugin and want to hack some more, you can fake this by adding an html comment that looks like the start of one of the blocks.You can also test edge cases such as:
Note that this is cached in a transient for a day, so if you change your setup, you'll need to wait a day to retest, or delete the transient:
yarn docker:wp transient delete "jetpack-woocommerce-analytics-cart-checkout-info-cache"
Note: right now, the events are not all consistently firing for block-based cart & checkout (e.g. woocommerce/woocommerce-blocks#2031). For example,
product_checkout
only works with shortcode checkout. This is a separate issue and will be fixed in a separate PR. Please be aware of this when testing with blocks :)Proposed changelog entry for your changes: