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.

Sign up for GitHub

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

Jump to bottom

Sharing: do not record stats if the stats module is disabled. #10120

Merged
merged 1 commit into from
Sep 17, 2018
Merged

Sharing: do not record stats if the stats module is disabled. #10120

merged 1 commit into from
Sep 17, 2018

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Sep 10, 2018

Fixes #9678

Changes proposed in this Pull Request:

When the Stats module is disabled, do not use any tracking pixel on pages using sharing buttons.

Testing instructions:

  1. Enable Sharing and Stats.
  2. Add a Facebook button to your posts.
  3. Load a post and check requests in the network tab; you will see 2 requests to pixel.wp.com; one for regular stats and one for sharing.
  4. Disable the Stats module (under the old Module settings screen)
  5. Reload the post's page; you should see no pixel.wp.com request.

Proposed changelog entry for your changes:

  • Sharing: do not record stats if the stats module is disabled.

@jeherve
Sharing: do not record stats if the stats module is disabled.
118a989 
Fixes #9678

When the Stats module is disabled, do not use any tracking pixel on pages using sharing buttons.
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Sharing Post sharing, sharing buttons [Status] Needs Review This PR is ready for review. [Pri] High labels Sep 10, 2018
@jeherve jeherve added this to the 6.6 milestone Sep 10, 2018
@jeherve jeherve requested a review from a team as a code owner September 10, 2018 10:38
@jeherve jeherve self-assigned this Sep 10, 2018
@jetpackbot
Copy link
Collaborator

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

Generated by 🚫 dangerJS

oskosk
oskosk approved these changes Sep 14, 2018
View reviewed changes
Copy link
Contributor

@oskosk oskosk 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 works as expected! :shipit:

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 14, 2018
@zinigor zinigor merged commit ff5e53e into master Sep 17, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 17, 2018
@zinigor zinigor deleted the fix/stats-sharing-9678 branch September 17, 2018 16:35
dereksmart
dereksmart reviewed Sep 17, 2018
View reviewed changes
modules/sharedaddy/sharing-service.php
/** This filter is documented in modules/sharedaddy/sharing-service.php */
'counts' => apply_filters( 'jetpack_sharing_counts', true ),
'counts' => apply_filters( 'jetpack_sharing_counts', true ),
'is_stats_active' => Jetpack::is_module_active( 'stats' ),
Copy link
Member

Choose a reason for hiding this comment

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

This method does not exist in wpcom, so this change cannot be synced back as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, sorry. I did not think about that. D18449-code should solve those kinds of issues.

jeherve added a commit that referenced this pull request Sep 18, 2018
@jeherve
Sharing: handle WordPress.com's situation (stats always active)
40d7884 
We cannot rely on `Jetpack::is_module_active()` on WordPress.com,
so we must do an additional check before to use the function.

@see #10120 (comment)
@jeherve jeherve mentioned this pull request Sep 18, 2018
Closed
jeherve added a commit that referenced this pull request Sep 24, 2018
@jeherve
changelog: add #10120
bc0d5ea
jeherve added a commit that referenced this pull request Sep 25, 2018
@jeherve
Changelog for 6.6 release (#10107)
581489a 
* 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
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D18844-code. (newly created revision)

dereksmart pushed a commit that referenced this pull request Apr 23, 2019
@zinigor @dereksmart
Add new wrapper function on WordPress.com to match Jetpack-only funct…
1b619fd 
…ion.

Summary:
When syncing Jetpack files with WordPress.com, we sometimes run into issues
because Jetpack and WordPress.com have 2 different functions to check
if a module is active: `is_active_module` on WordPress.com, `is_module_active` in Jetpack.

This diff fixes that by adding a new `is_module_active` function on WordPress.com.

Test Plan:
Once this is merged, we should be able to sync PRs like this one with no issues.
#10120

Reviewers: dereksmart, kraftbj, mdawaffe, migueluy, jeherve

Reviewed By: dereksmart, kraftbj, mdawaffe

Subscribers: gibrown, mdawaffe, kraftbj

Tags: #touches_jetpack_files

Differential Revision: https://[private link]

Merges r190254-wpcom.
@dereksmart dereksmart mentioned this pull request Apr 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dereksmart dereksmart dereksmart left review comments

@oskosk oskosk oskosk approved these changes

Assignees

@jeherve jeherve

Labels
[Feature] Sharing Post sharing, sharing buttons [Pri] High Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
6.6
Development

Successfully merging this pull request may close these issues.
6 participants
@jeherve @jetpackbot @matticbot @oskosk @dereksmart @zinigor