Repository navigation
Newsletter Mode: add WordPress.com page shell and routes - #50973
davemart-in wants to merge 2 commits into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! |
Code Coverage SummaryCoverage changed in 1 file.
4 files are newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
There was a problem hiding this comment.
Took this for a spin on an Atomic test site and read through it in detail. Nice, well-scoped slice — the gating is clean, it follows the ai-launchpad pattern closely, and the PHP tests asserting capability behaviour indirectly through has_action( 'toplevel_page_…' ) are a nice touch, since that exercises add_menu_page's real capability check rather than reimplementing it.
This is the UI that I saw.
@dhasilva and @CGastrell Notice the the negative margin on the AdminPage. Is that expected?
Can we remove this everywhere?
No blockers. Everything below is a suggestion. I ran the package's PHP tests, JS tests and Phan locally against this branch — all green. (Phan flagged two PhanPluginNeverReturnFunction hits in generated build/pages/*/page.php, but one of those is the pre-existing site-setup page, so that's a build-artifact effect in my worktree, not something this PR introduces.)
I've left five inline notes. Two more that don't have a single line to hang off:
6. No PHP coverage for the init() page-request / capability gate. The suite covers enrollment, the kill switch, admin-vs-frontend, loader wiring and register_menu capability behaviour — good coverage overall. What's missing is is_page_request() and the current_user_can branch inside init(), which is exactly the code path in my inline note on class-newsletter-mode.php.
7. Testing instructions could name the carrier plugin. "Build and sync jetpack-mu-wpcom using your normal development workflow" is a bit under-specified for Atomic. The package needs to be synced via wpcomsh there. I initially synced it via mu-wpcom-plugin instead and got a site-wide fatal — Call to undefined function wpcom_site_has_feature() from 100-year-plan/enhanced-ownership.php and wpcom-global-styles/index.php — because that plugin ships the package without the wpcom platform functions its other features depend on. Nothing to do with this PR, but naming wpcomsh explicitly in step 1 would save the next reviewer the same detour.
|
|
||
| add_action( 'admin_menu', array( __CLASS__, 'register_menu' ) ); | ||
|
|
||
| if ( ! self::is_page_request() || ! current_user_can( self::ACCESS_CAPABILITY ) ) { |
There was a problem hiding this comment.
The capability gets evaluated twice, at two different points in the request, and the two can disagree.
This check runs at plugins_loaded (via load_wpcom_sites_features()). add_menu_page() then checks the same capability again at admin_menu — and init fires in between (wp-settings.php: plugins_loaded at L622, init at L771).
So if anything grants publish_posts through a user_has_cap / map_meta_cap filter registered on init — common in membership and role-editor plugins — the two disagree: build.php never loads here, but the menu still registers at admin_menu with the __return_empty_string fallback. The user sees "Newsletter", clicks it, and gets a completely blank page with no error.
Loading build.php early isn't the thing to change — the generated registrars hook wp_default_scripts / wp_default_styles, so it genuinely has to be required before WP_Scripts is first instantiated. It's the early capability check that could go:
if ( ! self::is_page_request() ) {
return;
}add_menu_page() already gates access, which is what ai-launchpad relies on. Registering assets for a user who can't reach the page is harmless — nothing enqueues them. It also avoids resolving and caching the current user before init as a side effect.
| min-block-size: 12rem; | ||
| padding-block: 2rem; | ||
| padding-inline: 2rem; | ||
| background: var(--wpds-color-background-surface-neutral); |
There was a problem hiding this comment.
Worth double-checking this token name. --wpds-color-background-surface-neutral doesn't appear anywhere else in the monorepo — every other use I could find is the -strong variant, and in plugins/jetpack it always carries a fallback:
background: var(--wpds-color-background-surface-neutral-strong, #fff);If the token isn't defined in the admin context, the section just renders with no background. A fallback would make that safe either way.
| "test": "pnpm run test:node && pnpm run test:jest", | ||
| "test:jest": "jest", | ||
| "test:node": "node --test --test-reporter spec src/features/write/test/undo-history.test.mjs", | ||
| "test-coverage": "c8 --report-dir=\"$COVERAGE_DIR\" --temp-directory=\"$ARTIFACTS_DIR/v8\" pnpm run test", |
There was a problem hiding this comment.
test now runs test:node && test:jest, but test-coverage is still the c8 wrapper — and c8 doesn't meaningfully instrument Jest, so the new .tsx files will produce no coverage data.
The other Jest-based packages in the monorepo use Jest's own coverage instead:
"test-coverage": "pnpm run test --coverage"That's my best guess at why the "Code coverage requirement" check is currently red, though I didn't run the coverage job to confirm. Since the Coverage tests to be added later label is on, this may well be a deliberate defer — feel free to ignore if so.
| testMatch: [ | ||
| '<rootDir>/src/features/newsletter-mode/**/*.test.[jt]s?(x)', | ||
| '<rootDir>/routes/newsletter-mode-*/**/*.test.[jt]s?(x)', | ||
| ], |
There was a problem hiding this comment.
Since testMatch is an allowlist, only newsletter-mode paths run. Any future *.test.tsx added elsewhere in jetpack-mu-wpcom silently won't execute — no failure, just silence, which is an easy one to lose a day to.
A short comment explaining the deliberate scoping would be enough, or broaden the match and exclude what you don't want.
| @@ -0,0 +1,19 @@ | |||
| /// <reference types="jest" /> | |||
There was a problem hiding this comment.
Minor: the filename doesn't quite describe what's under test — this renders the two route stages and never exercises NewsletterModePageShell directly. Something like routes.test.tsx would match, or keep the name and add an actual page-shell unit test alongside.
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
|
Closing this out for now. |
Proposed changes
Add the initial WordPress.com-only Newsletter Mode page shell to
jetpack-mu-wpcom.This is the first replacement slice for #50680. It establishes the page structure and routes without adding any Newsletter Mode feature content yet.
The PR:
@wordpress/build.AdminPagechrome and standard Jetpack footer.wpcom_newsletter_mode_enabledoption.WPCOM_NEWSLETTER_MODE_DISABLEDemergency kill switch.publish_postscapability (author and above).add_menu_page()capability handling for menu registration while retaining an explicit capability check before loading direct page requests.No customer-facing Newsletter Mode functionality is included yet.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Run the automated checks:
To test the shell manually on a WordPress.com Simple or Atomic test site:
Sign in as an administrator or author and confirm that a top-level Newsletter menu appears.
Confirm these pages render the expected placeholder content inside the Jetpack page chrome:
Sign in as a contributor or subscriber and confirm the Newsletter menu is not available.
Remove enrollment and confirm the menu disappears:
WPCOM_NEWSLETTER_MODE_DISABLEDas true and confirm it overrides enrollment.