add extra properties to woocommerceanalytics events #15028

haszari · 2020-03-18T03:45:21Z

This PR adds some extra properties to woocommerceanalytics events. For example, these properties allow us to filter events on WooCommerce version.

Changes proposed in this Pull Request:

Adds url property to all woocommerceanalytics events.
Adds woo_version property to all woocommerceanalytics events.

pb0Spc-ou-p2

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:

This requires Jetpack and WooCommerce – if necessary, install WooCommerce, set it up (use the onboarding wizard), and add some products. Use Cash on delivery payment method for easy test checkout - you can enable this in WooCommerce > Settings > Payment.
The events are disabled for development sites. You can circumvent (hack) this by adding return true; at the start of Jetpack_WooCommerce_Analytics::shouldTrackStore().

Open network tab in browser dev tools, search for pixel.wp.com.
View a product on the front end of your site. You should see a request for woocommerceanalytics_product_view event. This should have new properties (parameters) url and woo_version, and the values should match the home url and Woo version (respectively).
Add/remove products from cart, view cart / checkout, complete purchase - the relevant events should all include the new properties.

Proposed changelog entry for your changes:

Added url and woo_version to WooCommerce Analytics events.

(Not sure if this needs a changelog entry.)

jetpackbot · 2020-03-18T03:47:27Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 40e0171

zinigor

Works as expected, code looks fine!

jeherve

Since we have data we want to send out for every event (such as blog ID, user ID, and now the site URL or the Woo version in this PR, I wonder if this may be a good time to extract all this into its own method to avoid duplicating code. what do you think?

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php

haszari · 2020-03-18T21:33:10Z

I wonder if this may be a good time to extract all this into its own method to avoid duplicating code. what do you think?

Thanks for the nudge @jeherve – I was thinking the same thing.

I've now added a PHP function for logging a standard event (to reduce boilerplate), and a utility for getting the common props, as a js argument string (fragment). The arg-string version is needed for the events that are triggered client-side.

Ready for another review :)

I'm going to do some more testing on this on an ephemeral site before merge, to ensure I haven't broken any existing events/props (e.g. user id, product type etc) in the refactor. cc @deltaWhiskey – might be worth a review or testing from you to ensure the existing events & props are intact.

haszari · 2020-03-18T21:38:48Z

This PR / branch has been rebased.

haszari · 2020-03-18T21:52:20Z

Not sure if this needs GDPR review but added the tag just in case.

haszari · 2020-03-19T01:18:19Z

FYI @psealock 👋

jeherve · 2020-03-19T17:59:35Z

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php

 		$blogid = Jetpack::get_option( 'id' );

-		// check for previous add-to-cart cart events
+		return "'blog_id': '" . esc_js( $blogid ) . "',


If we were to use sprintf here, would it be more readable? Or by building an array first, like you did for extra properties in record_event?

Good point! What I've done is to split the definition of the common props and rendering (as js object props fragment) into separate methods (40e0171).

It's still using the string-concatenation style, with embedded esc_js call, for consistency with the existing code (cc @deltaWhiskey). But now we can deal with common/server-side props with PHP arrays and then call render_properties_as_js to render them in a standard way.

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php

deltaWhiskey

It looks like it should work. It's worth making sure the endpoint is getting what's expected. If you'd like to work together on that, let me know.

I added some organizational / coding style suggestions.

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php

deltaWhiskey · 2020-03-19T18:48:09Z

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php

+			"_wca.push( {
+					'_en': '" . esc_js( $event_name ) . "',
+					'pi': '" . esc_js( $product_id ) . "',
+					'pn': '" . esc_js( $product_details['name'] ) . "',
+					'pc': '" . esc_js( $product_details['category'] ) . "',
+					'pp': '" . esc_js( $product_details['price'] ) . "',
+					'pt': '" . esc_js( $product_details['type'] ) . "'," .
+					$extra_properties . $this->get_common_properties() . '
+				} );'


There are two other places in this PR that generate very similar _wca.push(...) strings (the woocommerceanalytics_remove_from_cart events.) If we moved the generation of this wca.push(...) string to another function, then we could call that other function from all three places.

That would add some new product detail properties to the remove_from_cart events, but that seems OK to me.

The new function could be called event_push() or something. There's probably a better name for it, though.

We're doing a lot of string concatenation and repeated calls to esc_json(). That preserves the previous coding style, but I think it would be more readable and maintainable to pass arrays around, with the values unescaped.

When the string finally has to be generated (in event_push(), if we go with that approach, or else here,) that would be the moment to escape everything with an array_walk() call, and then encode with json_encode(). That would mean we need to review where single vs. double quotes are being used, wherever we output this.

If we were working with arrays before output, then get_common_properties() could be passing this a raw array, without doing part of the escaping there, and part in the various other places it gets called.

I agree it would be nice to have a single path for all these events. The issue is that those two other events are generated on the front end, and pull data from the DOM, so they call the JS api directly - we can't enqueue a pre-rendered _wca.push() call.

In future, we could render a event_push() style wrapper func on the front end which supplies all the default props. I didn't want to go that far with the refactor :)

psealock · 2020-03-20T03:07:40Z

@haszari Can you make a list of events affected? I can go through them and compare to master to make sure no changes have been made to the properties already in production.

haszari · 2020-03-20T03:12:06Z

@haszari Can you make a list of events affected? I can go through them and compare to master to make sure no changes have been made to the properties already in production.

Yes please – I'd appreciate another test pass to make sure the events are still working correctly. I've tested and compared with master, but there are lots of props to eyeball. Thanks @psealock !

All 5 events are affected (I believe these are all the WCA events?):

woocommerceanalytics_product_view
woocommerceanalytics_add_to_cart
woocommerceanalytics_remove_from_cart
woocommerceanalytics_product_checkout
woocommerceanalytics_product_purchase

- url - store/site home url - woo_version - version of woo plugin installed on the site

- prep work for having a single record event where we can add all common properties - also provides a PHP interface for logging events with custom params + tidy comments

haszari · 2020-03-22T23:17:23Z

Thanks for the reviews & feedback – this PR is all the better for your feedback! Please give this a final review & test when you're ready. @jeherve @psealock @deltaWhiskey

haszari · 2020-03-22T23:34:08Z

I see Travis is failing, I can't tell if this is due to issues caused by my code.

There are two jobs failing:

PHP Nightly seems to be something in photon, test_photon_cdn_in_rest_response_with_edit_context is failing
PHP 7.2 is timing out at 10 minutes

Please ping me / comment if there are steps I should take to resolve these!

jeherve

This looks good and tests well for me. 👍 I'll let someone more familiar with the events take a look at it as well, but on my end I think this should be good to merge.

As for Travis, it should be okay now.

haszari · 2020-03-23T18:40:57Z

@psealock Can you give these events a final test/review before merge? Thanks :)

psealock · 2020-03-23T23:49:33Z

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php

+		return array(
+			'blog_id'     => Jetpack::get_option( 'id' ),
+			'ui'          => $this->get_user_id(),
+			'url'         => home_url(),


I'm curious what the url is for. We already have browser document location in _dl property. Are you looking for a unique way to identify sites? If so, would blog_id suffice?

home_url() is to give a way to map to data collected by WC Tracker, which does not have blog_id. There have been experiments in the past with using document location, but that comes with some challenges.

psealock · 2020-03-23T23:50:05Z

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php

+			'blog_id'     => Jetpack::get_option( 'id' ),
+			'ui'          => $this->get_user_id(),
+			'url'         => home_url(),
+			'woo_version' => WC()->version,


How about keeping this inline with other property names: wv?

Prefer clarity over terseness here. See also @deltaWhiskey comment in support of clearer props names: #15028 (comment)

psealock

All the events check out, code looks good. Nice job @haszari

I just have the two questions above, otherwise looks good.

psealock

Thanks for answering my questions. Code looks good and events are working well.

haszari · 2020-03-24T01:09:48Z

@jeherve – I've merged this – I hope that's ok! Let me know if I need to do anything more to wrangle this change (e.g. changelog, other review tags).

jeherve · 2020-03-24T08:41:01Z

@haszari You're all set, nothing else to do! Thank you!

* 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

haszari requested a review from a team as a code owner March 18, 2020 03:45

haszari self-assigned this Mar 18, 2020

haszari added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Mar 18, 2020

zinigor added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] WooCommerce Analytics labels Mar 18, 2020

zinigor previously approved these changes Mar 18, 2020

View reviewed changes

jeherve added this to the 8.4 milestone Mar 18, 2020

jeherve requested changes Mar 18, 2020

View reviewed changes

modules/woocommerce-analytics/classes/wp-woocommerce-analytics-universal.php Outdated Show resolved Hide resolved

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 18, 2020

haszari dismissed zinigor’s stale review via 094f590 March 18, 2020 20:27

haszari force-pushed the update/woo-analytics-add-url-version branch from 6eb1319 to f89d7bb Compare March 18, 2020 21:38

haszari requested a review from psealock March 19, 2020 01:17

jeherve reviewed Mar 19, 2020

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 19, 2020

deltaWhiskey reviewed Mar 19, 2020

View reviewed changes

haszari mentioned this pull request Mar 20, 2020

Cart & Checkout Block: Add Data Tracking woocommerce/woocommerce-blocks#1273

Closed

add extra properties to woocommerceanalytics events:

37f89bf

- url - store/site home url - woo_version - version of woo plugin installed on the site

haszari added 4 commits March 23, 2020 11:13

factor out common record_event from loop_session_events:

7766ae8

- prep work for having a single record event where we can add all common properties - also provides a PHP interface for logging events with custom params + tidy comments

include common props js-triggered events via new util func

0e12300

use record_event for all remaining woo events

32b0409

fix issue with product_view event - handle no extra props

220eddb

haszari force-pushed the update/woo-analytics-add-url-version branch from f89d7bb to 220eddb Compare March 22, 2020 22:13

refactor: split definition of common props from render as js object keys

40e0171

haszari added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 22, 2020

jeherve approved these changes Mar 23, 2020

View reviewed changes

psealock reviewed Mar 23, 2020

View reviewed changes

psealock approved these changes Mar 24, 2020

View reviewed changes

haszari merged commit bf971bd into master Mar 24, 2020

haszari deleted the update/woo-analytics-add-url-version branch March 24, 2020 01:08

matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 24, 2020

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs GDPR Review [Status] Needs Tracks Review Added/removed/modified a tracks event. labels Mar 31, 2020

jeherve added a commit that referenced this pull request Mar 31, 2020

Changelog: add #15028

13e1332

haszari mentioned this pull request Apr 1, 2020

ensure woo analytics checkout event fires for checkout block #15223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add extra properties to woocommerceanalytics events #15028

add extra properties to woocommerceanalytics events #15028

haszari commented Mar 18, 2020

jetpackbot commented Mar 18, 2020 •

edited

Loading

zinigor left a comment

jeherve left a comment

haszari commented Mar 18, 2020 •

edited

Loading

haszari commented Mar 18, 2020

haszari commented Mar 18, 2020

haszari commented Mar 19, 2020

jeherve Mar 19, 2020

haszari Mar 22, 2020

deltaWhiskey left a comment

deltaWhiskey Mar 19, 2020

deltaWhiskey Mar 19, 2020

haszari Mar 20, 2020

psealock commented Mar 20, 2020

haszari commented Mar 20, 2020

haszari commented Mar 22, 2020

haszari commented Mar 22, 2020

jeherve left a comment •

edited

Loading

haszari commented Mar 23, 2020

psealock Mar 23, 2020

deltaWhiskey Mar 24, 2020

psealock Mar 23, 2020

haszari Mar 24, 2020

psealock left a comment

psealock left a comment

haszari commented Mar 24, 2020

jeherve commented Mar 24, 2020

add extra properties to woocommerceanalytics events #15028

add extra properties to woocommerceanalytics events #15028

Conversation

haszari commented Mar 18, 2020

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

jetpackbot commented Mar 18, 2020 • edited Loading

zinigor left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

haszari commented Mar 18, 2020 • edited Loading

haszari commented Mar 18, 2020

haszari commented Mar 18, 2020

haszari commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deltaWhiskey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock commented Mar 20, 2020

haszari commented Mar 20, 2020

haszari commented Mar 22, 2020

haszari commented Mar 22, 2020

jeherve left a comment • edited Loading

Choose a reason for hiding this comment

haszari commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock left a comment

Choose a reason for hiding this comment

psealock left a comment

Choose a reason for hiding this comment

haszari commented Mar 24, 2020

jeherve commented Mar 24, 2020

jetpackbot commented Mar 18, 2020 •

edited

Loading

haszari commented Mar 18, 2020 •

edited

Loading

jeherve left a comment •

edited

Loading