-
Notifications
You must be signed in to change notification settings - Fork 798
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
WooCommerce Analytics: Initialize the package only if is not loaded in WooCommerce Core #38236
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. |
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.
LGTM!
Not sure if we need the Needs Privacy Updates
label here, since we are not tracking any new data, just conditionally tracking the same Woo related data we used to, when WC_ANALYTICS
is not present.
@fgiannar - hi! And thanks for the privacy flag on this, @puntope. Can y'all do a quick check of the privacy section in this doc https://jetpack.com/support/privacy/woocommerce-analytics/#privacy and let me know if any verbiage needs to be added or changed? Sometimes, when a change is needed, it might just mean making the existing language a little more inclusive or wider-scoped. |
Howdy @jenhooks thanks for being proactive on this. Not sure if that needs to be changed but I can summarize what it changed so it can clarify if it should: Now WooCommerce Analytics is activated in these scenarios: User still uses an old version of WooCommerce or Jetpack that doesn't load the WooCommerce Analytics package via WooCommerce. --> In this case, the package is still loaded as always using Jetpack User uses a new version of Jetpack and WooCommerce and the package is loaded in WooCommerce 1..
For now, I wouldn't change anything yet. In the PT pcShBQ-1Vt-p2 we have a task for discussing with privacy team once everything is discussed. BTW. This claim is not correct on the page you mention
Jetpack should be activated and also connected to Wordpress.com if it is just activated it doesn't track. |
Fixes woocommerce/woocommerce#45210
PT: pcShBQ-1Vt-p2
Proposed changes:
To do that, we created a
WC_ANALYTICS
constant in the WooCommerce core version when we load the package.When this constant is present Jetpack will not initialize WooCommerce_Analytics.
This is for maintaining compatibility as it might happen that JEtpack gets updated but WooCommerce does not and vice versa.
Other information:
Jetpack product discussion
PT: pcShBQ-1Vt-p2
Does this pull request change what data or activity we track or use?
Added ✅
Testing instructions:
<script>window._wca = window._wca || [];</script>
script loadeddefine( 'WC_ANALYTICS', true )
<script>window._wca = window._wca || [];</script>
script loaded