Repository navigation
Premium Analytics: Keep the Stats menu under My Home, not below Upgrades - #54626
Conversation
The dashboard asked for menu position 2, which the Dashboard holds. Core then gives the second entry a hashed fraction (2.36713 as "Stats v2", 2.21518 as "Stats"), and the menu's natural sort reads that fraction as a whole number, so on WordPress.com the entry landed after Hosting (2.98) and Upgrades (2.99). Ask for 2.1 instead: free, so core adds nothing, and it sorts after My Home (2.01) and classic Stats, before Hosting. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! 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. Premium Analytics 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. Stats Data 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. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
adamwoodnz
left a comment
There was a problem hiding this comment.
Thanks @dognose24, this fixes the cause rather than the symptom, and the test reproduces core's natural sort over the real WordPress.com neighbours instead of asserting the constant, so it catches a regression in the ordering and not just a changed number. One non-blocking comment inline.
jetpack test php packages/premium-analytics passes.
| // A fixed key rather than the slug, which carries a build-specific suffix. No gate: | ||
| // the dashboard has no My Jetpack product class and no module to name. | ||
| Admin_Menu::add_top_level_menu( $menu_title, $menu_title, $capability, self::MENU_PAGE_SLUG, $render_callback, 'dashicons-chart-bar', 2, array( 'key' => 'jetpack-premium-analytics' ) ); | ||
| // @phan-suppress-next-line PhanTypeMismatchArgument -- Handed on to add_menu_page(), which takes a float. |
There was a problem hiding this comment.
Nit, not blocking: this treats the symptom. The cause is packages/admin-ui/src/class-admin-menu.php:433, which documents $position as int|null while its own docblock says the parameters mirror add_menu_page(), and core's takes int|float. Widening that @param to int|float|null would remove the need for a suppression here and for every future caller that wants a fractional position; as written, the next one copies the suppression. Reasonable as an interim step, and the -- <reason> justification is there as the repo requires. This is about where the fix belongs, not whether this PR is safe.
There was a problem hiding this comment.
Agreed, and taken in 4f03915: the docblock now reads int|float|null, as add_menu_page() takes, and the suppression is gone. Docblock only, so the admin-ui entry is comment-only and its version stays put.
chihsuan
left a comment
There was a problem hiding this comment.
Thanks @dognose24 this looks good. Confirmed Stats stays under My Home in both English and German. 👍
Just sharing: classic Stats still asks for 2, so its spot follows the translated label. In French and German it lands below Upgrades. Since it's being retired, I think that's fine.
add_top_level_menu() hands $position on to add_menu_page(), which takes int|float, but its docblock said int|null, so a fractional position needed a Phan suppression at the call. Widen the docblock instead and drop the suppression from Premium Analytics. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Follow-up to the review of #54454, which found it and notes that Simple sites already have the same order since #54452. Independent of #54454, so it can land first.
Proposed changes
On WordPress.com the Stats v2 dashboard's menu entry sat below Hosting and Upgrades instead of under My Home, where classic Stats sits. With classic Stats retired, that is where Stats itself ends up.
2, which the Dashboard already holds.add_menu_page()then gives the second entry a fraction hashed from its slug and title:2.36713as "Stats v2",2.21518as "Stats".wp-admin/menu.phpsorts the menu withstrnatcasecmp, which reads that fraction as a whole number, so2.21518sorts after Hosting's2.98and Upgrades'2.99. Classic Stats asks for2as well and only sorts first because its fraction,2.09281, starts with a zero.2.1, through aMENU_POSITIONconstant used by both registration paths. Nothing else is there, so core adds no fraction, and it sorts after My Home (2.01) and classic Stats and before Hosting.Admin_Menu::add_top_level_menu()documented$positionasint|null, though it hands the value on toadd_menu_page(), which takesint|float. The docblock now saysint|float|null, so no Phan suppression is needed at the call. Docblock only, with a comment-only change entry for admin-ui; no version bump.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
The order only shows next to the entries WordPress.com adds, so either test on a WordPress.com site or stand them in locally.
On a local Jetpack site with Stats v2 enabled, add the two neighbours at the positions jetpack-mu-wpcom gives them:
On trunk, Stats v2 is below Upgrades. On this branch it is above Hosting, right after the Dashboard (and after Stats, where the classic page has a top-level entry).
Retire classic Stats by adding this next to the snippet above, in an mu-plugin (the flag is read on
plugins_loaded, so the filter has to be there before that):The entry is now labelled Stats and stays above Hosting. On trunk it is below Upgrades.
On a Simple or Atomic site with this branch, Stats sits under My Home in wp-admin, and in the WordPress.com sidebar, which reads the same menu.
What was run for this PR:
Step 3 was not run: nothing here was tested on a Simple or Atomic site. The screenshots above are from that local site, with the three WordPress.com entries stood in.
🤖 Generated with Claude Code