-
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
Add additional properties to WC Analytics events #20812
Conversation
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 (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
Hi @haszari ✋, we would love to include this PR in the next release on September 7th. Could you please review this PR when you get a chance? Github is recommending you since you have worked on the files included in this PR 😄 |
No problem, will take a look! Also pinging in @deltaWhiskey to get some eyes from Woo Data (side note – is there a GitHub team for woo data?) |
Hi @kraftbj ✋ we would love to include this PR in the next release on September 7th. Could you please review this PR when you get a chance? 🙇 It looks like you have worked on the WC Analytic module. According to the field guide, I also need one approval from a member of Jetpack team. |
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.
Please do not merge this PR yet. Changes in this PR depend on the newly added action in WC Pay. We need to merge these changes after releasing the WC Pay version 2.9.
we would love to include this PR in the next release on September 7th
When do you think this new version of WCPay will be released? Can we merge this PR before next Monday, so it can be included in the next release?
if ( 'Google Pay (WooCommerce Payments)' === $payment_option_title ) { | ||
$express_checkout = array( 'google_pay' ); | ||
} elseif ( 'Apple Pay (WooCommerce Payments)' === $payment_option_title ) { |
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.
Does this support sites using a different language?
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.
Thank you for the catch 👍 This is a test case I missed testing.
I've tested this PR with two different languages and they both worked as expected.
WC Pay uses the exact string to update the post meta table according to this block.
...pack/modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-universal.php
Show resolved
Hide resolved
...pack/modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-universal.php
Show resolved
Hide resolved
'guest_checkout' => $guest_checkout, | ||
'create_account' => $create_account, |
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.
Since this comes from options, would it be overkill to validate that we have the right letters 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.
I think that's a good idea 👍 change was also simple enough. Updated in b2505f7
...pack/modules/woocommerce-analytics/classes/class-jetpack-woocommerce-analytics-universal.php
Outdated
Show resolved
Hide resolved
@jeherve Thank you for the review 👍 I've pushed a few changes to address your suggestions. WC Pay 2.9.0 has been released yesterday and I also pushed a commit to make this PR work with previous versions of WC Pay. I've updated the description accordingly. I want to merge this before next Monday if everything goes okay. |
Thank you 👍 for the review and for catching the bug. I've pushed 9bcef13 to fix the null issue. |
@moon0326 FYI you can also test without hacking the code – just use a different browser session to purchase as a guest or non-admin user. |
@haszari Thank you! Looking at the code, that should work as well. Could you take a look at the comments I left to your comments? 🙇 I want to ship the current changes tomorrow (code freeze) then follow up with Stripe plugin and JS code inclusion changes in the next release. |
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 gave this a really quick test – there are lots of combinations to test, and they are all somewhat complex to set up!
Here's what I tested:
- Checkout event shows correct store options (create account, allow guest, available payment options).
- When an express option available, it's included in checkout event. Google Pay is the only one I can get going:
express_checkout: google_pay
. - Purchase with wcpay shows correct gateway
woocommerce_payments
. - Only tested with core payment options (COD and Cheque), and WooCommerce Payments.
- WooCommerce Payments was in test mode.
I didn't test the following:
- If these events and props all work correctly with checkout or cart blocks from WooCommerce Blocks.
- Create account on purchase flow.
- Any express payments – Google pay fails for me (
Billing Street address is a required field. Billing Town / City is a required field. Billing Postcode is a required field.
).
Feedback on events and props
There's a lot of new info getting tracked here. That's great if we need it all - though I'm curious if some of this info might be redundant or not essential. Store settings are also tracked in WooCommerce Tracker.
After we ship new events and props it's harder to remove things or adjust naming, especially if the intention and use is not clearly defined.
Let's follow this up – @pmcpinto could you P2 the question(s) you want to answer with this info>
Regarding the actual naming of the new props, here are some suggestions. The goal here is to make each prop as clear and unambiguous as possible.
woocommerceanalytics_product_checkout
payment_options
available_payment_options
oravailable_regular_payment_options
(to distinguish from express)express_checkout
available_express_payment_options
guest_checkout
store_allows_guest_checkout
create_account
store_allows_create_account_at_checkout
tbc - let's clarify what we are tracking here; if we stick withdevice
wp_is_mobile
perhaps we can nameis_mobile
oruseragent_is_mobile
woocommerceanalytics_product_purchase
payment_option
user_payment_option
oruser_regular_payment_option
if this is empty/null for expressexpress_checkout
user_express_payment_option
- Or - would
user_payment_option
andis_user_payment_option_express
work better? guest_checkout
is_guest_checkout
create_account
is_checkout_create_account
or similar- otherwise use same props as
checkout
event
For all these properties, please use standard types and formats as recommended by data team (e.g. boolean props).
Blocks and other express checkout
Noting a couple of places that I haven't tested:
- Express payment on product page.
- Express payment on cart page.
- Checkout page using block.
If we want to track checkout/purchase anywhere they happen on a site, we should test those flows too (and follow up if needed).
Next steps
I've approved this PR – if it's urgent to get this shipped in next Jetpack, so we can start run funnels, I don't want to hold it up.
At the same time, I'd love to find out more about what questions this tracking will answer so we can ensure we're collecting the right info (and no more). If it's not urgent, maybe it's better to have that discussion before shipping. cc @pmcpinto
$wcpay_version = get_option( 'woocommerce_woocommerce_payments_version' ); | ||
$has_required_wcpay_version = version_compare( $wcpay_version, '2.9.0', '>=' ); | ||
// Check express payment availablity only if WC Pay is enabled and express checkout (payment request) is enabled. | ||
if ( in_array( 'woocommerce_payments', $enabled_payment_options, true ) && $has_required_wcpay_version ) { |
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'm curious if Woo has an API for express payment methods. Ideally we could call a standard API to determine what express options are available, and avoid tight coupling to specific gateway plugins.
WooCommerce Blocks checkout block has some support/APIs for express payment options, so might be worth discussing with that team for advice.
$create_account = 'N'; | ||
} | ||
|
||
$guest_checkout = $order->get_user() ? 'N' : 'Y'; |
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.
Let's standardise the format of boolean props (1 0
, Y N
, yes no
etc).
Note that elsewhere in this file, 1 0
is used for boolean props (e.g. cart_page_contains_cart_block
). This is compact and unambiguous; I'd recommend sticking with this, or whatever data team recommends.
$express_checkout = 'null'; | ||
// When the payment option is woocommerce_payment | ||
// See if Google Pay or Apple Pay was used. | ||
if ( 'woocommerce_payments' === $payment_option ) { |
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 block of code is tightly coupled to WooCommerce payments. Perhaps it could be factored out into a function so it's clear it depends on another plugin.
Or perhaps use a hook/filter to provide a standard interface to any plugin that might need to supply this info (e.g. other gateway plugins).
Also see above comment – if there's a Woo API for express payment methods, we should use that. If there isn't an API - should we add one?
'pq' => $order_item->get_quantity(), | ||
'oi' => $order->get_order_number(), | ||
'pq' => $order_item->get_quantity(), | ||
'device' => wp_is_mobile() ? 'mobile' : 'desktop', |
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.
Best to check in with data team about how we plan to segment data / analyse by device (mobile, screen size, etc) before adding a new prop. Tracks events may already have a generic mechanism for this; User Agent
is sent with all tracks requests.
$session->set( 'wc_checkout_createaccount_used', true ); | ||
$session->save_data(); | ||
} | ||
} |
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.
Is this the best way to detect that a new account was requested? Maybe there's another hook fired when the account is created.
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 to me. 🚢
Thank you @jeherve 👍 I'll give one last test (just in case) shortly and merge it before the end of today. |
bc0a9ef
to
9bcef13
Compare
3236fe6
to
9bcef13
Compare
…-wc-analytic-events
* master: (34 commits) Fix issue templates (#20880) Social Icon Widget: Fix Icons Not Saving (#20862) Dashboard: Add a specific message for a site that is too new to have a rewind status (#20863) Add additional properties to WC Analytics events (#20812) Connection: add spinner for the "Connect" button (#20872) Update storybook monorepo to v6.3.7 (#20877) Lightweight PHPCS (#20876) Recommendations: Add Jetpack Product Suggestions step (#20814) Update babel monorepo (#20875) Update wordpress monorepo (#20873) cli: Check subprocess exit statuses in docker commands (#20861) Facebook Widget: Fix URL error when adding widget (#20864) Stats: make nudges collapsible (#20765) Remove temporary Newspack block override css (#20868) Allow ZIP uploads via Calypso (#20860) Sync Package Release v1.26.0 (#20870) Connection: remove in-place in secondary flows (#20739) BUILD_DIR -> BUILD_BASE in initial checks (#20857) E2E tests: Fix missing action for atomic workflow (#20866) Boost: Fix skip proxy origins to load css during critical CSS generation (#20793) ...
Please do not merge this PR yet. Changes in this PR depend on the newly added action in WC Pay. We need to merge these changes after releasing the WC Pay version 2.9.WC Pay version 2.9 has been released and I've updated this PR to work with previous versions of WC Payments in 4e9a4fbFixes woocommerce/woocommerce-admin#7301
Changes proposed in this Pull Request:
array
value to be added as an event property 4f2d1bc - This is needed to addpayment_options
value.woocommerceanalytics_product_purchase
andwoocommerceanalytics_product_checkout_view
. The additional properties are listed in the issue.from WC Pay when the express checkout methods are enabled.
Jetpack product discussion
Does this pull request change what data or activity we track or use?
We're adding the following properties to
woocommerceanalytics_product_purchase
andwoocommerceanalytics_product_checkout_view
events.woocommerceanalytics_product_purchase
payment_option: the option that was used. Eg Stripe or COD
express_checkout: if the express checkout option was used: Eg: Apple Pay or Null
guest_checkout: if the guest checkout was used
create_account: check if the shopper created an account (Y/N)
device: mobile/desktop
woocommerceanalytics_product_checkout_view
payment_options: an array with the options available. Eg [paypal, stripe, cod, amazon]
express_checkout: an array with the options available. Eg [apple_pay] or [null] if they don’t display them
guest_checkout: Yes/No. Identify if the store allows users to checkout without an account
create_account: Yes/No Identify if the store allows users to create an account on checkout
device: mobile/desktop
Testing instructions:
Please use WC Pay 2.9.0
If you're new to WC Analytic testing, please perform the following steps to enable WC Analytic.
Direct bank transfer
in WooCommerce -> Settings -> Payments.return true
to force WC Analytic to track during the testing./shop
to see the details.t.gif
request.If you don't see
t.gif
request for some reason, make sure WooCommerce Analytics module is activated in/wp-admin/admin.php?page=jetpack_modules
Case 1 -- Without WC Payment installed/activated
t.gif
t.gif
request.woocommerceanalytics_product_checkout_view
event.Direct bank transfer
selected.t.gif
request on theOrder received
page forwoocommerceanalytics_product_purchase
event. Confirm that the required properties are included in the URL.Bonus: Add multiple products and confirm you get
t.gif
request for each product.You can use tools like https://www.freeformatter.com/url-parser-query-string-splitter.html to parse the URL into a table.
Case 2 -- WC Payment activated + express checkout methods enabled.
https
and publicly accessible address.Express Checkouts
are enabled in Payments -> Settingsexpress_checkout
value.woocommerceanalytics_product_checkout_view
should have eithergoogle_pay
orapple_pay
woocommerceanalytics_product_purchase
should havenull
Case 3 -- WC Payment activated and express checkout methods disabled.
https
and publicly accessible address.Express Checkouts
express_checkout
value isnull
in both events.Case 4 -- WC Payment activated + express checkout methods enabled + checkout using Google/Apple pay.
https
and publicly accessible address.Express Checkouts
express_checkout
value.both events should have
google_pay
orapple_pay
Case 5 -- WC Payment version lower than 2.9.0.
The action used in this PR was added in WC Payment version 2.9.0. If the user has WC Payment version lower than 2.9.0, then the action should not be used.
wp.hooks.addAction