Fix - No GA code when `Enable Enhanced eCommerce` option is active but WooCommerce is not active #10092

htdat · 2018-09-03T16:56:30Z

👋from a HE. My first PR. Thanks for the feedback in advance :)

Changes proposed in this Pull Request:

Currently, Jetpack does not output the GA tracking code in a specific case, which can be replicated as you see on #10091 :

Enable Enhanced eCommerce option is active
But WooCommerce is not active

Testing instructions:

Activate WooCommerce on a Premium or Pro site.
In https://wordpress.com/settings/traffic/my-site.com, activate Enable Enhanced eCommerce.
Deactivate WooCommerce
Visit the site without login as an admin.
Check the HTML output and find the GA code by searching Jetpack Google Analytics.

Proposed changelog entry for your changes:

Fix Bug - Do not output the Google Analytics code when Enable Enhanced eCommerce option is active but WooCommerce is not active

jetpackbot · 2018-09-03T16:58:37Z

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

jeherve

Nice catch. This looks good to me, your fix seems to solve the issue.

@allendav Do you want to take a look at this as well? This may conflict with the work you've been doing in #9333.

kraftbj

Small suggested change, but not a blocker.

kraftbj · 2018-09-14T19:14:06Z

modules/google-analytics/wp-google-analytics.php

-		if ( Jetpack_Google_Analytics_Options::enhanced_ecommerce_tracking_is_enabled() ) {
+		// At this time, we only leverage universal analytics when enhanced ecommerce is selected and WooCommerce is active.
+		// Otherwise, don't bother emitting the tracking ID or fetching analytics.js
+		if ( Jetpack_Google_Analytics_Options::enhanced_ecommerce_tracking_is_enabled() && class_exists( 'WooCommerce' ) ) {


This is extremely nitpicky, but sharing to help frame something to keep in mind. With an && operation, it checks left-to-right and fails on the first failed condition—so the enhanced_ecommerce_tracking_is_enabled, then the class_exists check. The class_exists check is pretty cheap, but enhanced_ecommerce_tracking_is_enabled eventually calls get_option, which may or may not be a db hit (didn't look to see if the option is autoloaded or not).

In this case, it probably isn't a huge thing, but for the sake of performance, thinking about the order things are checked can help you out.

That makes sense to me! Thanks Kraft 👍

I've pushed a new commit to reorder the condition positions

That would help the performance a bit.

allendav · 2018-09-18T15:17:12Z

Hi @htdat - what's the use case that this addresses? Thx!

kraftbj · 2018-09-18T16:10:01Z

@allendav The use case I'm testing for is someone who had a WooCommerce site and started tracking with enhanced eCommerce, then stopped using WooCommerce (for whatever reason). Before the PR, it would disable all Google Analytics code since WooCommerce wasn't active.

kraftbj

Works as described.

allendav · 2018-09-18T16:54:05Z

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

@@ -49,12 +49,6 @@ public function wp_head() {
 			return;
 		}

-		// At this time, we only leverage universal analytics for enhanced ecommerce. If WooCommerce is not
-		// present, don't bother emitting the tracking ID or fetching analytics.js
-		if ( ! class_exists( 'WooCommerce' ) ) {


Why remove this? Although the other change you made will prevent this class from being instantiated unless WooCommerce is active, this is a nice extra layer of protection that avoids this class becoming dependent on another class for sanity.

allendav · 2018-09-18T16:55:40Z

modules/google-analytics/wp-google-analytics.php

@@ -50,7 +50,9 @@ class Jetpack_Google_Analytics {
 	 * @return void
 	 */
 	private function __construct() {
-		if ( Jetpack_Google_Analytics_Options::enhanced_ecommerce_tracking_is_enabled() ) {
+		// At this time, we only leverage universal analytics when enhanced ecommerce is selected and WooCommerce is active.
+		// Otherwise, don't bother emitting the tracking ID or fetching analytics.js


Maybe reword or even completely remove this comment because it is a little confusing since the else condition will emit a tracking ID ala legacy.

allendav · 2018-09-18T16:57:35Z

@kraftbj wrote

@allendav The use case I'm testing for is someone who had a WooCommerce site and started tracking with enhanced eCommerce, then stopped using WooCommerce (for whatever reason). Before the PR, it would disable all Google Analytics code since WooCommerce wasn't active.

OK, fair enough. So we want to "fall back" to legacy analytics if WooCommerce is deactivated while this "enhanced eCommerce" option is active - that's cool.

@see

* Readme: add boilerplate for next release, 6.6 * Add 6.5 to the changelog.txt file * Set boilerplate testing list for 6.6 * Readme: update stable tag to 6.5 * Add bullets to 6.5 changelog items * Readme: add link to previous changelogs This will help folks who want to know more about past releases, while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release. * Changelog: add information at the top of the changelog file. * Changelog: add #10054 * Changelog: add #10078 * Changelog: add #10079 * Changelog: add #10064 * Changelog: add #10094 * Changelog: add #10096 * Testing list: add more information based on #10087 * Changelog: add #9847 * Changelog: add #10084 * Changelog: add #9918 * Changelog: add #7614 * Changelog: add #10116 * Changelog: add #10108 * Changelog: add #10041 * Changelog: add #10121 * Changelog: add #10134 * Changelog: add #10130 * Changelog: add #10109 * changelog: add #10137 * changelog: add #9952 * changelog: add #10120 * changelog: add #10162 * Changelog: add #10163 * Changelog: add #10092 * changelog: add #10156 * Changelog: add #10154 * changelog: add #10122 * Changelog: add #10101 * changelog: add #10105 * changelog: add #10190 * Changelog: add #10196 * changelog: add #10152 * Changelog: add #10153 * Testing list: add more details to Site Verification testing steps. @see #10143 (comment) * changelog: add #10194 * Changelog: add #10193

Rewrite the conditions to load Google Analytics Universal code

d92118f

htdat requested review from kraftbj, zinigor and jeherve September 3, 2018 16:56

htdat requested a review from a team as a code owner September 3, 2018 16:56

htdat added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended [Feature] WooCommerce Analytics [Pri] Low labels Sep 3, 2018

jeherve added [Feature] Google Analytics [Pri] Normal and removed [Feature] WooCommerce Analytics [Pri] Low labels Sep 5, 2018

jeherve approved these changes Sep 7, 2018

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 7, 2018

jeherve added this to the 6.6 milestone Sep 11, 2018

jeherve assigned htdat Sep 13, 2018

kraftbj reviewed Sep 14, 2018

View reviewed changes

Reorder the condition positions

9c07436

That would help the performance a bit.

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 18, 2018

htdat added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 18, 2018

kraftbj self-assigned this Sep 18, 2018

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 fellow Jetpack developers. Label will be renamed soon. labels Sep 18, 2018

kraftbj approved these changes Sep 18, 2018

View reviewed changes

allendav reviewed Sep 18, 2018

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! 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 Sep 18, 2018

jeherve merged commit 8653fa8 into master Sep 19, 2018

matticbot added the [Status] Needs Changelog label Sep 19, 2018

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 19, 2018

jeherve deleted the fix/no-ga-code-when-woo-inactive branch September 19, 2018 10:12

jeherve added [Status] Needs Changelog and removed [Status] Needs Changelog labels Sep 19, 2018

jeherve added a commit that referenced this pull request Sep 24, 2018

Changelog: add #10092

b0a6a88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix - No GA code when `Enable Enhanced eCommerce` option is active but WooCommerce is not active #10092

Fix - No GA code when `Enable Enhanced eCommerce` option is active but WooCommerce is not active #10092

htdat commented Sep 3, 2018

jetpackbot commented Sep 3, 2018 •

edited

Loading

jeherve left a comment •

edited

Loading

kraftbj left a comment

kraftbj Sep 14, 2018

htdat Sep 18, 2018

allendav commented Sep 18, 2018

kraftbj commented Sep 18, 2018

kraftbj left a comment

allendav Sep 18, 2018

allendav Sep 18, 2018

allendav commented Sep 18, 2018

Fix - No GA code when Enable Enhanced eCommerce option is active but WooCommerce is not active #10092

Fix - No GA code when Enable Enhanced eCommerce option is active but WooCommerce is not active #10092

Conversation

htdat commented Sep 3, 2018

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

jetpackbot commented Sep 3, 2018 • edited Loading

jeherve left a comment • edited Loading

Choose a reason for hiding this comment

kraftbj left a comment

Choose a reason for hiding this comment

kraftbj Sep 14, 2018

Choose a reason for hiding this comment

htdat Sep 18, 2018

Choose a reason for hiding this comment

allendav commented Sep 18, 2018

kraftbj commented Sep 18, 2018

kraftbj left a comment

Choose a reason for hiding this comment

allendav Sep 18, 2018

Choose a reason for hiding this comment

allendav Sep 18, 2018

Choose a reason for hiding this comment

allendav commented Sep 18, 2018

Fix - No GA code when `Enable Enhanced eCommerce` option is active but WooCommerce is not active #10092

Fix - No GA code when `Enable Enhanced eCommerce` option is active but WooCommerce is not active #10092

jetpackbot commented Sep 3, 2018 •

edited

Loading

jeherve left a comment •

edited

Loading