Stats: add self-hosted admin bar link#50264
Conversation
|
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 🤖 Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
…ats-admin-bar-link-trunk
There was a problem hiding this comment.
Review depth: thorough (auto-upgraded — diff adds a current_user_can() check)
Summary
Adds a Stats link to the site-name admin bar submenu for self-hosted sites, gated behind view_stats and skipped on WP.com platform sites. Small, well-contained, additive change.
Testing performed
- End-to-end in a local self-hosted Jetpack docker env (logged in as an administrator with Stats active):
- Front end, site-name dropdown →
Statsappears alongsideDashboard, href resolves toadmin.php?page=stats. ✅ matches PR screenshots. wp-admin, site-name dropdown → onlyVisit Site/Visit Storepresent, noStatslink (core showsVisit Sitethere instead ofDashboard, and the code correctly uses the absence of thedashboardnode as its front-end proxy — verified this matcheswp_admin_bar_site_menu()in WP core). ✅- Capability gate verified directly:
current_user_can('view_stats')istruefor an administrator andfalsefor a subscriber on this install, matching the new PHPUnit coverage. ✅ - Confirmed the pre-existing
stats_admin_bar_menu()sparkline (different hook, different node id) is untouched by this diff — no interaction risk.
- Front end, site-name dropdown →
- Unit tests: ran
Stats_Admin_Bar_Testlocally (PHP 8.3) —OK (4 tests, 8 assertions). All 4 scenarios (capability granted/denied, dashboard node present/absent, WP.com platform) pass. - CI: all PHP lint (7.2–8.5), PHPUnit matrix, JS tests, and coverage checks are green on the PR.
Code review
- Logic correctly mirrors WP core's own frontend-detection approach (checking for the
dashboardnode) rather than reinventing it — robust if core's conditions change. Host::is_wpcom_platform()and theview_statscapability check are the right primitives;automattic/jetpack-statusis already a declared dependency of the plugin, no version-skew concern.- Output is safe: title is a translated static string, href comes from
admin_url()— no unescaped user input. - Changelog entry is well-formed (patch/enhancement, imperative, punctuated).
- Test coverage is solid: covers grant/deny, dashboard-node presence, and WP.com platform exclusion, using existing test conventions in the codebase (
Constants,Status\Cache,WP_UnitTestCase_Fix).
Verdict
Looks good — approving.
|
Non-blocking follow-up thought (not asking for changes here — happy to ship this as-is and iterate): This gates on Atomic sites run the actual Jetpack plugin (that's what If we wanted to tighten that up later, it'd be a coordinated two-sided change: narrow this guard to |
Part of STATS-287.
Proposed changes
view_statscapability.jetpack-mu-wpcomadmin bar link remains responsible there.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Statslink appears underDashboardand points to/wp-admin/admin.php?page=stats.Statslink does not appear there, where core showsVisit Siteinstead ofDashboard.view_statscapability do not see the Stats link.Automated testing:
php -l projects/plugins/jetpack/modules/stats.phpphp -l projects/plugins/jetpack/tests/php/modules/stats/Stats_Admin_Bar_Test.phpxmllint --noout projects/plugins/jetpack/phpunit.9.xml.dist projects/plugins/jetpack/phpunit.11.xml.distgit diff --checkNot run:
pnpm jetpack docker phpunit jetpack --name statslink --php 8.3 -- --filter=Stats_Admin_Bar_Test- local Docker test environment issue.