Skip to content
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: make it available as module #15187

Merged
merged 6 commits into from
Mar 31, 2020
Merged

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Mar 30, 2020

Changes proposed in this Pull Request:

WooCommerce Analytics was always enabled until now, and could not be disabled unless you were to hook into jetpack_tools_to_include.

This PR makes the feature a proper module.

  • The module is automatically enabled so should be automatically enabled on plugin update, unless folks turned that off with a filter like jetpack_get_default_modules.
  • It will only appear as available in the full module list (at wp-admin/admin.php?page=jetpack_modules)
  • It will be greyed out there if you do not use WooCommerce, or use an old version of WooCommerce:

image

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

  • p7Ldg5-ru-p2

Testing instructions:

Proposed changelog entry for your changes:

  • N/A

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Feature] WooCommerce Analytics labels Mar 30, 2020
@jeherve jeherve added this to the 8.4 milestone Mar 30, 2020
@jeherve jeherve requested a review from a team as a code owner March 30, 2020 09:18
@jeherve jeherve self-assigned this Mar 30, 2020
@jetpackbot
Copy link

jetpackbot commented Mar 30, 2020

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 31c98ee

@brbrr brbrr self-requested a review March 30, 2020 15:37
brbrr
brbrr previously approved these changes Mar 30, 2020
Copy link
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me. Although I wasn't able to get new parameters as explained in #15127

Copy link
Contributor

@haszari haszari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and confirmed this is all working well:

  • Woo analytics can be enabled and disabled.
  • Woo analytics only sends events when it is enabled.
  • Woo analytics is unavailable and disabled when Woo plugin is not active.

I noticed that woo analytics module was disabled by default – when active Jetpack is updated to the version with the module, or after Jetpack is freshly activated. Is this as expected – is my dev setup or some config on my end breaking this somehow?

@haszari
Copy link
Contributor

haszari commented Mar 31, 2020

Is this as expected – is my dev setup or some config on my end breaking this somehow?

Ah I see – to test activation by default, I'd need to simulate updating from 8.3 => 8.4, by changing the version number. Or, test the beta build which will be available soon :)

Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jeherve this is looking great and testing well for me! I can toggle on/off and see the effect on requests for t.gif.

Activate WooCommerce. You should see the module available and active.

The module isn't active by default for me, I have to click "Activate" after re-activating WooCommerce.

On a site that does not run WooCommerce

I only de-activated instead of deleting or running on a site that never had Woo. Perhaps that is the reason?

psealock
psealock previously approved these changes Mar 31, 2020
Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I read previous comments here and p7Ldg5-ru-p2. Sounds good to me 🚢

@jeherve jeherve dismissed stale reviews from psealock and brbrr via 31c98ee March 31, 2020 07:17
@jeherve
Copy link
Member Author

jeherve commented Mar 31, 2020

@brbrr I just pushed 31c98ee to ensure auto-activation happens properly when the version update happens. Do you think you could give this another review?

Thank you!

Copy link
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

@brbrr brbrr added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 31, 2020
@jeherve jeherve merged commit df5a13f into master Mar 31, 2020
@jeherve jeherve deleted the add/woo-analytis branch March 31, 2020 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WooCommerce Analytics [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants