WOOA7S-1769: Premium Analytics: Add a Stats settings drawer to the page options menu - #52870
Nikschavan wants to merge 14 commits into
Conversation
Nikschavan
added
Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Package] Premium Analytics
[Package] Stats Data
[Package] Stats Admin
[Plugin] Jetpack
|
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. Wpcomsh plugin:
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. |
Code Coverage SummaryCoverage changed in 3 files.
4 files are newly checked for coverage.
|
github-actions
Bot
added
the
Admin Page
| <ToggleControl | ||
| __nextHasNoMarginBottom | ||
| label={ __( | ||
| 'Include a small chart in the admin bar', | ||
| 'jetpack-premium-analytics-pkg' | ||
| ) } | ||
| help={ __( | ||
| 'Shows views from the last 48 hours. The admin bar shows the change on the next page load.', | ||
| 'jetpack-premium-analytics-pkg' | ||
| ) } | ||
| checked={ settings.admin_bar } | ||
| disabled={ isSaving } | ||
| onChange={ ( isOn: boolean ) => update( { admin_bar: isOn } ) } | ||
| /> |
There was a problem hiding this comment.
One thing I am curious about is that this setting takes effect only when the page is reloaded - I am wondering if we should reload the page when this setting is updated. This is what I did in the stats v1 settings, but I am not a fan of reloading the SPA.
e331fb7 to
2e49624
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A combined save can partially persist settings while reporting complete failure to the drawer.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds an administrator-only Stats settings drawer to Premium Analytics, backed by shared Stats package logic and a local REST route.
Changes:
- Adds settings UI, role selection, save feedback, direct-link handling, and tracking.
- Shares settings validation between Premium Analytics and Stats Admin.
- Routes Jetpack’s settings link to the new drawer and expands tests/changelogs.
| File | Description |
|---|---|
projects/plugins/premium-analytics/changelog/wooa7s-1769-stats-settings-drawer |
Records the plugin feature. |
projects/plugins/jetpack/changelog/wooa7s-1769-stats-settings-drawer |
Records Jetpack integration. |
projects/plugins/jetpack/_inc/shared/test/analytics-url.ts |
Tests the settings URL. |
projects/plugins/jetpack/_inc/shared/analytics-url.ts |
Adds the settings destination. |
projects/plugins/jetpack/_inc/client/traffic/test/site-stats.test.jsx |
Tests the updated link. |
projects/plugins/jetpack/_inc/client/traffic/site-stats.jsx |
Links to the new drawer. |
projects/packages/stats/tests/php/Settings_Screen_Test.php |
Tests shared settings behavior. |
projects/packages/stats/src/class-settings-screen.php |
Implements shared settings operations. |
projects/packages/stats/changelog/wooa7s-1769-stats-settings-drawer |
Records the shared class. |
projects/packages/stats/AGENTS.md |
Documents the new class. |
projects/packages/stats-admin/src/class-rest-controller.php |
Delegates to shared settings logic. |
projects/packages/stats-admin/changelog/wooa7s-1769-stats-settings-drawer |
Records the refactor. |
projects/packages/premium-analytics/tests/php/REST/Settings_Controller_Test.php |
Tests permissions and REST behavior. |
projects/packages/premium-analytics/src/REST/class-settings-controller.php |
Adds the local settings endpoint. |
projects/packages/premium-analytics/src/class-analytics.php |
Registers the endpoint. |
projects/packages/premium-analytics/routes/video-detail/stage.test.tsx |
Extends route mocks. |
projects/packages/premium-analytics/routes/post-detail/stage.test.tsx |
Extends route mocks. |
projects/packages/premium-analytics/routes/author-detail/stage.test.tsx |
Extends route mocks. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/page-options-menu/settings-drawer.tsx |
Implements the settings drawer. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/page-options-menu/settings-drawer.module.scss |
Offsets the drawer below wp-admin. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/page-options-menu/role-select.tsx |
Adds role multi-selection. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/page-options-menu/role-select.module.scss |
Styles the role selector. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/page-options-menu/page-options-menu.tsx |
Adds menu and direct-link handling. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/page-options-menu/__tests__/settings-drawer.test.tsx |
Tests drawer interactions. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/page-options-menu/__tests__/page-options-menu.test.tsx |
Tests visibility and routing. |
projects/packages/premium-analytics/packages/widgets-toolkit/src/components/detail-page/__tests__/detail-page-customize.test.tsx |
Adds route mocks. |
projects/packages/premium-analytics/packages/externals/src/index.ts |
Exports Card and Drawer. |
projects/packages/premium-analytics/packages/data/src/hooks/use-stats-settings.ts |
Adds settings query hooks. |
projects/packages/premium-analytics/packages/data/src/hooks/index.ts |
Exports the hooks and types. |
projects/packages/premium-analytics/packages/data/src/api/stats-settings.ts |
Adds REST client functions. |
projects/packages/premium-analytics/changelog/wooa7s-1769-stats-settings-drawer |
Records the package feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Nikschavan
added
[Status] Needs Review
7348186 to
c01e02f
Compare
dognose24
left a comment
dognose24
left a comment
There was a problem hiding this comment.
Read the whole diff and ran the suites locally at c01e02f (details below). No blockers; two suggestions, one of which is the changelog set.
Missing plugins/wpcomsh changelog entry. jetpack-mu-wpcom bundles premium-analytics and wpcomsh bundles jetpack-mu-wpcom, so Atomic site owners get the new Settings item (only Simple hides it). The recent user-facing Premium Analytics PRs (#52714, #52747, #52690) each carried a wpcomsh entry; the wording from the plugins/jetpack entry works as is.
The Simple-site route registration I was going to raise landed in 4d5e8af while I was writing this up, with a test. 👍
Checked and clean: manage_options on both routes with role validation in Settings::update(); stats-admin keeps its route path and error codes (ERROR_PREFIX is unchanged); premium-analytics and stats-admin both hard-require jetpack-stats, so Settings_Screen needs no method_exists() guard; heading levels (Drawer.Title h2, groups h3) and the aria-labelledby on the role trigger; new SCSS is all logical properties; copy matches the Settings › Traffic card. Copilot's partial-save point is the pre-existing order moved verbatim, as you said.
Local runs at c01e02f: packages/premium-analytics JS and PHP pass; packages/stats PHP passes; packages/stats-admin PHP passes apart from four pre-existing Admin_Post_List_Test errors from a missing IntlCalendar on my machine; plugins/jetpack site-stats.test.jsx (4) and analytics-url.ts (33) pass. Phan left to CI.
|
@dognose24 I added the |
|
Ran the branch on a local site and compared the drawer with the design mock. Structure, sections, drawer placement and the footer all match; four details drift from the mock. None of them is a blocker, and the verdict from my review stands. Splitting them by how cheap they are to close:
Worth doing in this PR, both are one-line changes
Fine as follow-ups, both need input from someone else
Small: the mock says "Count logged in page views from"; the branch's "logged-in" is the better spelling, keep it. |
retrofox
left a comment
retrofox
left a comment
There was a problem hiding this comment.
The implementation works, but I think two aspects are worth rethinking.
1. Data layer
The data handling gets much simpler with core-data. This case needs to persist site settings, and the root / site entity over /wp/v2/settings already covers that: reading, pending edits, change detection, partial saves, and the error state.
With register_setting and show_in_rest, we avoid registering a new endpoint and all the code that comes with it: the controller, the fetchers, the hooks, and the draft and getChanges logic in the drawer.
Premium Analytics already works this way. The dashboard opt-in is registered like that in class-enablement-setting.php, and the dashboard resolves the site entity on load.
Two details need handling. stats_options is a serialized array with internal keys, so the sanitize_callback has to merge the incoming value with the stored one.
The list of roles and the link to the modules screen are static data and can travel in script data.
In general, I would rather avoid custom abstraction layers when core already covers the case.
2. UI
Stats v2 would be the only Jetpack product that exposes its settings in a drawer. The others use a tab or their own route, and the most common pattern is a Settings tab (Newsletter, SEO, Podcast, Social).
A tab or a route has its own URL. A drawer does not, which is why this PR needs the ?settings=1 param for the link from Jetpack › Settings › Traffic.
The WOOA7S-2055 design defines the drawer as the container. Did you consider consistency with the other products when you made that decision?
…ons menu The settings drawer holds the Stats settings that live under Jetpack > Settings > Traffic today: the admin bar chart, who can view Stats, whose logged-in views count, and Reader views. When the Jetpack plugin is active, it also links to the modules screen where Stats is switched off. Changes save only on Save, and ?settings=1 in the dashboard route opens the drawer. The settings route moves into packages/stats as /jetpack/v4/stats/settings, so Premium Analytics can use it without stats-admin. The stats-admin route for Odyssey now calls the same Settings_Screen class.
The Manage Stats settings link on the Traffic card now opens the Premium Analytics settings drawer when Stats v2 is the site's analytics UI, and keeps the classic Stats link otherwise, through a new settings view in getAnalyticsUrl(). Also mock useNavigate in the detail-page tests that render PageOptionsMenu, and tighten the drawer tests: the locked Administrator row with stored roles that leave it out, no close while a save runs, and fake timers for the React Query waits.
Closing a drawer opened from the menu must not rewrite the URL, and a save that fails without a site error code shows the generic message rather than the browser's text.
…tics Only the Stats v2 drawer calls the route, so it now registers next to the other local Premium Analytics routes as jetpack-premium-analytics/v1/settings instead of jetpack/v4/stats/settings in packages/stats. Settings_Screen stays in packages/stats as the logic both the Odyssey route and this one share; the Jetpack modules_url moves into the new controller, so packages/stats no longer knows about the Jetpack plugin.
Settings_Screen::get() is already asserted by the stats-admin route test, and the modules_url test proves this route calls it.
A refetch while the drawer is open no longer sends back stale values of fields the reader did not edit, and clearing the last non-admin role sends administrator instead of an empty list the API rejects.
The drawer is not offered on WordPress.com Simple, so the route no longer registers there either. The controller tests now register through register() and check that modules_url is present.
The admin bar help, the WordPress.com Reader group, the Activation sentence and the role labels now match the prototype, with the Learn more link to the Stats support page.
Clearing a role and ticking it again changes its order, which enabled Save although the selection was the same.
The drawer reads and edits stats_options and wpcom_reader_views_enabled as the core-data site entity over /wp/v2/settings, replacing the custom route. Writes go through the Stats package's Settings::update() from rest_pre_update_setting, so the option keeps its internal keys and version, and the role list and modules link travel in script data. Also uses upstream parts for the divider, spinner and notice, refuses unknown roles through the schema, keeps apiFetch's own network errors out of the message, and shows the load error when the site returns no Stats options.
8b4b0e3 to
ace8e6e
Compare
|
@dognose24 Thank you for comparing the drawer with the design.
The Activation card now uses that sentence, with the Jetpack modules link inline.
The Reader setting now sits under its own divider and sub-heading, and the toggle uses the mock's label.
The help text now matches the mock, and Learn more links to the Jetpack Stats support page. I dropped the reload note to stay with the approved copy.
The role summary still lists every selected role. I would like to land the "first two names and the rest" shortening together with the member counts in STATS-338.
I went with the mock's "logged in", so the drawer matches the approved design and the Settings › Traffic card. |
|
Thank you for the review @retrofox Re; Data Layer Good call, I have updated the implementation to use
I started with the merge in a sanitize_callback, and then moved it to rest_pre_update_setting. A sanitize callback runs on every update_option( 'stats_options' ), not only on writes from /wp/v2/settings. That includes the Stats package itself and the Jetpack plugin, for example the upgrade nudges, which remove collapse_nudges from the option. So the callback had to guess which writes came from the settings route. rest_pre_update_setting runs only for the settings route, so nothing has to be guessed. The filter passes the drawer's fields to Settings::update(), the same writer that Odyssey's settings use. That writer checks the roles, keeps administrator, merges with the stored option and writes the version. Social uses the same filter to merge its object settings.
I personally agree that stats v2 will be different from all other packages. That said this decision was made in WOOA7S-1769 and I was not involved in the decision. When taking this task up, I did notice there have been discussions settings screens from other Jetpack modules, which was considered and the design still landed to use drawer for stats v2. Because that comparison had already been made, i did not raise this again, which in retrospect I should have. Thank you for raising this. @garysmurray, @ederrengifo - Do you have any thoughts on this - that the settings being in drawer is different than all other Jetpack modules? |
Part of WOOA7S-1769
Proposed changes
Drawer,Card,MenuandButtonfrom@wordpress/ui, andToggleControlfrom@wordpress/components, becauseSwitchControlonly ships in@wordpress/ui0.23 and Premium Analytics is on 0.22. Changes are only saved on Save, which sends just the settings that changed, shows a "Settings saved." snackbar and closes the drawer. A failed save keeps the drawer open with the reason the site gave.admin.php?page=jetpack-premium-analytics-wp-admin&p=%2F%3Fsettings%3D1. The "Manage Stats settings" link in Jetpack > Settings > Traffic now uses it when Stats v2 is on, through the samegetAnalyticsUrl()helper the other Jetpack links to Stats v2 use (Premium Analytics: point Jetpack's analytics links at the new dashboard #50926). Without Stats v2 it keeps the classicpage=stats#!/stats/settingslink. Thesettingsparam is removed from the URL when the drawer closes, so a reload or Back does not open it again.stats-admin, which Premium Analytics does not depend on. I moved the logic into a newSettings_Screenclass inpackages/stats, and the drawer reads and saves through a newjetpack-premium-analytics/v1/settingsroute, next to the other local Premium Analytics routes. Thestats-adminroute keeps its path for Odyssey and now calls the same class, so the validation lives in one place. The link to the Jetpack modules screen comes from the Premium Analytics route, sopackages/statsdoes not need to know about the Jetpack plugin.manage_options, the same gate as "Switch off the preview".Related product discussion/links
Does this pull request change what data or activity we track or use?
Yes. It adds the
jetpack_premium_analytics_settings_saveTracks event, sent after a successful save with the names of the settings that changed (for exampleroles,admin_bar). It carries no personal data beyond the usual Tracks user andblog_id.Testing instructions
Use a site with this branch and the Stats v2 preview switched on: a Jurassic Ninja site with the branch synced, or the Jetpack Beta Tester plugin pointed at this branch.
Go to Stats v2, open the page options menu (the three dots at the top right) and pick Settings. The drawer slides in from the right, below the admin bar, and shows the current values.
Change "Allow Jetpack Stats to be viewed by" to include Editor. Save stays disabled until something changes. Click Save and confirm the "Settings saved." snackbar and that the drawer closes.
Open Jetpack > Settings > Traffic and confirm the Jetpack Stats card shows the same roles.
Open the drawer, change something, then Cancel and open it again. The unsaved change is gone.
Open
/wp-admin/admin.php?page=jetpack-premium-analytics-wp-admin&p=%2F%3Fsettings%3D1. The drawer opens on load, and closing it removessettings=1from the URL.Go to Jetpack > Settings > Traffic. With Stats v2 on, "Manage Stats settings" opens the Stats v2 settings drawer.
Switch Stats v2 off from the page options menu with "Switch off the preview". Then go to Jetpack > Settings > Traffic and click "Manage Stats settings". It takes you to the classic Stats settings at
admin.php?page=stats#!/stats/settings.Log in as an Editor with access to Stats. The Settings item is not in the menu.
Open classic Stats > Settings (Odyssey) and save a change there, to confirm the
stats-adminroute still works.I tested all of the above on my local site except the Editor step and the Odyssey Settings tab, which the
stats-adminPHP tests cover. I did not test a site running only the standalone Premium Analytics plugin, where the Activation card should not show.