Skip to content

Stats: do not update the WPCOM blog details - #21441

Merged
kbrown9 merged 1 commit into
masterfrom
update/stats_update_blog_details
Oct 22, 2021
Merged

kbrown9 merged 1 commit into
masterfrom
update/stats_update_blog_details

Conversation

@kbrown9

@kbrown9 kbrown9 commented Oct 15, 2021 •

Copy link
Copy Markdown
Member

Changes proposed in this Pull Request:

Currently, it's possible for the Stats module to change the WPCOM side siteurl value when the site is in an identity crisis, which is not the expected behavior. Only the Identity Crisis process should update the WPCOM side siteurl and home option values. Also, the data that is sent by the Stats module is handled by the Sync process, so it shouldn't be necessary for the Stats module to send it.

  • Deprecate the stats_update_blog() function.
  • Remove the stats_update_blog() call from stats_upgrade_options().

An alternate approach to this solve the identity crisis problem would be to just remove the siteurl option from the data that's sent in the jetpack.updateBlog request. However, it seems redundant to send any of this data since Sync handles it.

Jetpack product discussion

  • n/a

Does this pull request change what data or activity we track or use?

  • no

Testing instructions:

Suggested code review:

Confirm that the jetpack.updateBlog request sends data that is handled by Sync. The values that are sent by the call to jetpack.updateBlog are found in the stats_get_blog function. The default options that are sent by Sync are found in the Defaults::default_options_whitelist property. The siteurl option is synced via the site_url callable here.

Manual test:
  1. Install and activate this branch of Jetpack.
  2. Connect to WPCOM.
  3. Get the blog id for your test site.
  4. Using your sandbox, verify that that your test site's options that were updated by the call to jetpack.updateBlog are correct. (blogname, siteurl, stats_api, stats_options, etc.)

@kbrown9 kbrown9 added [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Status] In Progress labels Oct 15, 2021
@kbrown9 kbrown9 added this to the jetpack/10.3 milestone Oct 15, 2021
@kbrown9 kbrown9 self-assigned this Oct 15, 2021

@test-case-reminder test-case-reminder Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here are some suggested test cases for this PR.

Stats

  • Visit home page, make sure that page view is recorded in stats
  • Visit home page in AMP view
  • Visit post page in non-AMP and AMP view

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

@github-actions github-actions Bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Oct 15, 2021
@github-actions

github-actions Bot commented Oct 15, 2021 •

Copy link
Copy Markdown
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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 (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: November 2, 2021.
  • Scheduled code freeze: October 25, 2021.

The Stats module shouldn't update the wpcom blog details. Tthe blog details are kept up to
date by Sync and Identity Crisis.
 * Deprecate the stats_update_blog() function.
 * Remove the stats_update_blog() call from stats_upgrade_options().
@kbrown9
kbrown9 force-pushed the update/stats_update_blog_details branch from e2c25bc to ab4b32e Compare October 20, 2021 15:35
@kbrown9
kbrown9 requested a review from zinigor October 22, 2021 15:57
@kbrown9 kbrown9 added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Oct 22, 2021

@leogermani leogermani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've verified that all the options that we updated via jetpack.updateBlog are handled by Sync.

From the options list, I saw that host, path, stats_api and stats_version were not handled by Sync, but these were also not handled by the XMLRPC endpoint, as we can see in Jetpack_XMLRPC_Server::sanitize_blog_option.

All options of my site looked fine even without this call (I watched my sandbox to see no calls coming in)

I also made sure to delete stats_options locally and saw it getting synced to WPCOM

@sdixon194 sdixon194 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 Oct 22, 2021
@kbrown9
kbrown9 merged commit f7f9d66 into master Oct 22, 2021
@kbrown9
kbrown9 deleted the update/stats_update_blog_details branch October 22, 2021 19:56
@github-actions github-actions Bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 22, 2021
davidlonjon added a commit that referenced this pull request Oct 26, 2021
* master: (27 commits)
  Admin Page: Update upgrade buttons Tracks events (#21483)
  Debug Helper IDC Simulator: Add the ability to spoof the home url (#21516)
  Map block: Add `address` block attribute (#21412)
  Update wordpress monorepo (#21522)
  Stats Page: Add VideoPress nudge (#21513)
  Improve Jetpack products illustrations file size (#21514)
  Update dependency mediawiki/mediawiki-codesniffer to v38 (#21523)
  Update dependency yoast/phpunit-polyfills to v1.0.2 (#21524)
  Widgets: add Strava to Social Icons Widget and Social Menu (#21518)
  Boost: Add active modules to speed score request (#21471)
  Markdown: Better accessibility for Footnotes (#21495)
  Jetpack Plugin: Add VideoPress upgrade page (#21497)
  Admin Menu: moves Add new (plugin) menu item to the top for Atomic sites (#21506)
  General: Check the return value of get_comment() (#21080)
  Instagram Widget: link to user connection if user is not connected (#21512)
  Search: Create a new search package, rename Search plugin (#21502)
  Identity Crisis: Remove the unused jetpack_idc_option transient (#21492)
  Stats: do not update the wpcom blog details (#21441)
  Admin: do not display upgrade messages when on offline mode (#21444)
  jetpack: Catch calls to `exit()` and `die()` in PHPUnit (#21043)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants