feat(emails): move Emails screen to Audience > Configuration (NPPD-1538)#197
Open
kmwilkerson wants to merge 6 commits into
Conversation
…inned (NPPD-1538) Move the Emails wizard from Newspack > Settings to a tab inside Audience > Configuration. Files stay at their current paths for this commit; a follow-up will relocate them under src/wizards/audience/views/setup/emails/ so the registration-site change can be bisected independently of the file move. Registration site: - Audience_Wizard accepts $args and forwards to parent so it can host Wizard_Section instances; Emails_Section moves into Audience's sections array in class-wizards.php - SSR-bootstrap data moves from $newspackSettings.emails.sections.emails to $newspackAudience.emails (same shape, flatter key path) - Settings's get_local_data() drops the emails block - Settings's sections.tsx drops the emails component import + fullWidth flag - Audience setup's tab list grows an "Emails" entry that mounts the existing Emails component inside withWizardScreen with the full-width className so DataViews keeps its wide layout REST surface: - Migrate GET /emails and POST /emails/{id}/toggle to use REST_BASE (recon-report Part 3 incorrectly claimed these were already pinned — verified against actual code, only /emails/settings was). All four Emails_Section endpoints now use REST_BASE, so the wizard_slug flip doesn't touch any REST path. Frontend hardcoded paths (newspack-settings) remain correct. - Emails_Section's $wizard_slug default updates from 'newspack-settings' to 'newspack-audience' to reflect the new home (overridden at construction time, but the default should match reality). - Class + REST_BASE docblocks updated to past-tense the move. Window types: - Remove emails entry from newspackSettings window type - Add emails entry to newspackAudience window type - Update emails.test.js + settings-modal.test.js mocks to seed window.newspackAudience instead of window.newspackSettings Out of scope (later commits): - Frontend file relocation (commit 2) - Old-URL redirect handler (commit 3) - Architectural-lock-in tests (commit 4) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ils/ (NPPD-1538) Move the 10 Emails-tab source files from src/wizards/newspack/views/settings/emails/ to src/wizards/audience/views/setup/emails/ so the source location matches the screen's new home (Audience > Configuration > Emails). All moves are pure renames — file contents are unchanged at this commit. Files renamed (git tracks all 10 as 100% renames, blame history preserved): - index.tsx - emails.tsx - emails.scss - emails.test.js - email-preview.tsx - email-preview.test.js - email-preview.scss - settings-modal.tsx - settings-modal.test.js - settings.tsx The Audience setup view's Emails import updates from the old cross-tree path '../../../newspack/views/settings/emails' to the colocated './emails'. REST paths remain pinned to /newspack/v1/wizard/newspack-settings/emails* per REST_BASE (NPPD-1535) — the file move is invisible to API consumers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…NPPD-1538) Two-layer redirect from the deprecated Emails route to its new home: Server-side (Newspack_Settings::maybe_redirect_legacy_emails_url, hooked on admin_init): - Fires only when ?page=newspack-settings AND ?emails=1 are both present - Bare ?page=newspack-settings (no emails marker) passes through — the Settings page itself still hosts other sections - wp_safe_redirect() with default 302 (not 301) so browsers don't cache the redirect — matches the Newspack::admin_redirects() pattern - Guarded by wp_doing_ajax(), is_network_admin(), and current_user_can('manage_options') Client-side (top of sections.tsx, module load): - Runs before <Wizard> mounts and before HashRouter parses the hash, so users never see a Settings-page flash - Catches the common bookmark form ?page=newspack-settings#/emails (the URL fragment is invisible to the server, so this layer is essential) - Uses window.location.replace() (not assign) so the old URL doesn't enter browser history — back button still works - Destination derived from window.newspack_urls.dashboard so subdirectory WordPress installs work correctly The redirect handler lives in class-newspack-settings.php (the wizard whose URL is being deprecated) rather than the existing Newspack::admin_redirects() method — that one is single-purpose (post-activation flow). Tying the new redirect to its owning wizard keeps the concern local and makes it easier to test. A `dashboard: string` field was added to the window.newspack_urls type to reflect what the Wizard base class already localizes at runtime (was previously only `site` was declared). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) Add Bucket G to tests/unit-tests/emails-section.php — five tests pinning the post-move invariants so a future regression fails loudly rather than silently breaking either the UI move or the API stability commitment. Tests added: - test_emails_section_wizard_slug_is_audience: reflection-reads the registered section instance and asserts wizard_slug === 'newspack-audience'. Catches re-registration under Newspack_Settings. - test_rest_base_is_pinned_to_newspack_settings: locks the Emails_Section::REST_BASE constant value against drift. Pairs with the "DO NOT change" docblock note. - test_no_wizard_slug_in_rest_route_registration: inspects the REST server's registered routes; asserts the listing endpoint lives at REST_BASE and no Emails_Section endpoint leaked into the wizard_slug namespace ('wizard/newspack-audience/emails*'). Audience Wizard's reset endpoint at /audience-management/emails is filtered out by the negative regex. - test_legacy_emails_url_redirects_with_hint: simulates ?page=newspack- settings&emails=1, captures the redirect URL via the wp_redirect filter (pattern from tests/unit-tests/content-gate/group-subscriptions.php), asserts target is page=newspack-audience#/emails. - test_legacy_emails_url_no_redirect_without_hint: bare ?page=newspack- settings must pass through. wp_redirect filter is hooked to fail the test if it fires. Documentation comments: - class-newspack-settings.php's maybe_redirect_legacy_emails_url gets a paragraph explaining the handler is currently speculative — no in-tree caller generates ?emails=1 URLs. It's a forward-compatible deep-link entry point for external integrations. - sections.tsx's module-load redirect gets a paragraph explaining the intentional module-side-effect: doing the hash check + replace inside a useEffect would render a Settings-page frame first, producing a visible flash. The hash-check must run at module top. Test count delta: +5 tests, +12 assertions in the emails-section file. Full PHP suite: 1349 tests / 4331 assertions / 5 skipped, all passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…athname (NPPD-1538) Smoketest caught a regression on localhost: visiting ?page=newspack-settings#/emails landed at ?page=newspack-settings#/ (the Wizard component's catch-all <Redirect to={ displayedSections[ 0 ].path } /> firing because the /emails route no longer exists in the Settings wizard). Root cause: the JS redirect was gated on `window.newspack_urls?.dashboard` being truthy. When that optional- chain check failed (timing edge case with localized-data evaluation order, or browser cache mismatch on the entry chunk), the redirect short-circuited silently and the Wizard's HashRouter fell back to the root section. Fix: derive the destination from `window.location.pathname` (always populated at module-load, no localized-data dependency). The user is already inside wp-admin when this code runs, so pathname is the correct admin base on both root and subdirectory WordPress installs. Strictly equivalent behavior to the original intent — but with one less failure mode. Also reverts the newspack_urls.dashboard type addition from commit 3 since the simplified redirect no longer references that field. The runtime data does include `dashboard`, but adding a type entry just for code I'm no longer using bloats the type surface. Verified on localhost: ?page=newspack-settings#/emails now redirects to ?page=newspack-audience#/emails as intended. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Moves the unified Emails management UI from Newspack > Settings into Audience > Configuration while preserving REST route stability via a pinned Emails_Section::REST_BASE, and adds legacy URL redirects + lock-in tests to prevent regressions.
Changes:
- Re-register
Emails_Sectionunder the Audience wizard (new Emails tab) and SSR-bootstrap its initial email payload viawindow.newspackAudience.emails. - Keep REST endpoints pinned under
wizard/newspack-settings/emailsand add client/server redirects for legacy Settings URLs. - Add PHPUnit “architectural lock-in” tests and update JS tests/types for the new
window.newspackAudienceshape.
Reviewed changes
Copilot reviewed 10 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugins/newspack-plugin/tests/unit-tests/emails-section.php | Adds architectural lock-in tests for wizard registration, REST_BASE pinning, route leakage, and legacy URL redirects. |
| plugins/newspack-plugin/src/wizards/types/window.d.ts | Types window.newspackAudience.emails SSR bootstrap payload. |
| plugins/newspack-plugin/src/wizards/newspack/views/settings/sections.tsx | Adds module-load redirect for #/emails hash and removes Emails section wiring from Settings. |
| plugins/newspack-plugin/src/wizards/newspack/types/index.d.ts | Removes Settings emails tab typing now that Emails no longer lives under Settings. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/index.js | Adds an Audience “Emails” tab and routes it via withWizardScreen for full-width layout. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/index.tsx | New Audience Emails tab wrapper component. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/emails.tsx | Updates Emails view to read SSR seed from window.newspackAudience.emails. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/emails.test.js | Updates tests to use window.newspackAudience bootstrap shape. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/emails.scss | Adds styling for the Audience Emails tab layout + preview/link affordances. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/email-preview.tsx | Adds lazy-loaded email preview thumbnail renderer (iframe + Intersection/ResizeObserver). |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/email-preview.test.js | Adds unit tests for EmailPreview fetch/lifecycle behavior. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/email-preview.scss | Styles for the email preview thumbnail container/placeholder. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/settings.tsx | Adds a “Transactional emails” settings action card in the Emails tab. |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/settings-modal.tsx | Adds settings modal for sender/contact transactional email settings (GET/POST pinned endpoint). |
| plugins/newspack-plugin/src/wizards/audience/views/setup/emails/settings-modal.test.js | Updates modal tests to use window.newspackAudience bootstrap shape. |
| plugins/newspack-plugin/includes/wizards/newspack/class-newspack-settings.php | Adds server-side legacy URL redirect handler; removes Emails tab local-data from Settings wizard. |
| plugins/newspack-plugin/includes/wizards/newspack/class-emails-section.php | Moves section “home” to Audience while keeping REST registration pinned to REST_BASE. |
| plugins/newspack-plugin/includes/wizards/audience/class-audience-wizard.php | Accepts wizard args and localizes SSR bootstrap for Emails tab under newspackAudience. |
| plugins/newspack-plugin/includes/class-wizards.php | Re-registers Emails section under Audience wizard instead of Settings. |
Batch of fixes from xhigh-effort code review + Copilot review.
Must-fix / correctness:
1. Reorder maybe_redirect_legacy_emails_url guards — $_GET checks now
run first (cheap isset + string compare), then ajax/network guards,
then current_user_can. Previously every admin_init request paid the
capability check before the URL match could short-circuit. On sites
with custom-role plugins (user_has_cap filter chains, per-request
role mapping) the cap check is non-trivial.
2. Preserve other query args across the server redirect — utm_source,
highlight=..., debug flags etc. now carry through to the new
Audience URL via add_query_arg. The handler is documented as a
forward-compatible deep-link entry point; dropping context on
redirect defeats that purpose.
3. Loosen JS hash redirect to match #/emails, #/emails/, #/emails?...,
#/emails/preview/123 — strict equality on the original silently
dropped trailing-slash and nested-route bookmark variants, letting
the Wizard's catch-all <Redirect> bounce the user to Connections.
Hash suffix is preserved across the redirect so nested routes can
resolve under Audience's own router.
4. Rewrite test_no_wizard_slug_in_rest_route_registration to use the
fresh-WP_REST_Server pattern already established in this file
(mirrors test_post_settings_args_use_text_field_sanitizer_for_emails).
Addresses Copilot finding about REST server state determinism.
Adds add_filter( 'newspack_woocommerce_active', '__return_true' )
so the toggle endpoint is exercised — without it, the toggle was
skipped by the WC-gated branch and a regression that reintroduced
$wizard_slug interpolation on the toggle would have slipped past.
Uses a deliberately-mismatched 'regression-canary-slug' to surface
any leftover $this->wizard_slug interpolation; the listing endpoint
and toggle endpoint must both still register under REST_BASE.
5. Defensive read on window.newspackAudience.emails — falls back to
{ dependencies: {}, postType: '', initial: undefined } if the
localized payload is missing. The TS type declares the field
required, but runtime can have it undefined on any non-Audience
admin page or if a plugin filter strips the global.
Should-fix / cleanup:
6. Apply withWizardScreen at the Emails component's own export to
match sibling tabs (Setup, ContentGating, Payment, Campaign,
Complete) — each applies the HOC at its own module's default
export. Drops the inline EmailsTab wrapper from
audience/views/setup/index.js. Eliminates the arrow-wrapper
props-discard footgun that the original
`withWizardScreen( () => <Emails /> )` had.
7. Remove $wizard_slug class default — Wizard_Section::__construct
unconditionally overwrites the property with $args['wizard_slug']
anyway, so any default declared at the class level is dead code.
Updated docblock to make this explicit so future contributors
don't add a misleading default back.
8. Update stale comment in emails.scss — was still referencing
the removed `fullWidth: true` flag and sections.tsx registration
site. Now points at the new wiring (className passed to
withWizardScreen-wrapped tab in audience/views/setup/index.js).
9. Remove stale `isEmailEnhancementsActive: false` from emails.test.js
fixture — field isn't in window.d.ts, isn't read by emails.tsx,
isn't emitted by PHP. Mechanically migrated from the old
`window.newspackSettings.emails.sections.emails` shape but never
pruned.
Verification: PHPCS exit 0, ESLint exit 0 (only pre-existing `any`
warning unchanged), tsc 213 errors total (matches baseline, no new),
JS suite 22/217 unchanged, PHP suite 1349/4333/5 skipped (+2
assertions from the rewritten Bucket G test asserting both the
listing endpoint AND the toggle endpoint plus the canary-slug
absence).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Moves the Emails screen from Newspack > Settings to Audience > Configuration. Same UI, different parent menu. This is the last build piece of v1 of the Transactional Email Management Consolidation project — after this lands, the publisher-facing consolidation story is complete.
Resolves NPPD-1538.
Stacking
main. Review order: #137 → #144 → #146 → #162 → this.The stacking choice: #162 has the
REST_BASEconstant declaration and the most recent UI work (chip bar HStack, settings modal). Stacking on #162 means this PR relocates the latest UI state, not an older snapshot. Both #158 and #162 declareREST_BASEindependently with byte-identical text, so the merge order between them doesn't matter — whichever lands first, the other rebases cleanly.Architectural choices
Path A: Emails becomes a tab inside Audience > Configuration
Audience > Configuration already hosts tabs for Content Gating, Checkout & Payment, etc. via an internal HashRouter. Emails joins that same router as a sibling tab. The alternative (sibling sub-wizard under Audience, parallel to Campaigns/Content Gates) was considered but rejected — the ticket framed this as a Configuration tab, and joining the existing tab structure keeps the Audience parent menu less cluttered.
Cost of Path A:
Audience_Wizard::__construct()needed a small change to accept\$argsand forward to its parent so it can hostWizard_Sectioninstances. The signature change is backward-compatible (\$args = []default) — verified that all five sibling Audience wizards (Campaigns, Content_Gates, Donations, Integrations, Subscriptions) still construct correctly without passing args.REST_BASE stays pinned to
wizard/newspack-settings/emailsThe REST_BASE constant was introduced by NPPD-1535 (#158) and NPPD-1566 (#162) specifically to preserve REST API stability across this move. The docblock says "DO NOT change"; this PR honors that — the wizard slug changes from
newspack-settingstonewspack-audience, but the REST routes stay at the pinned path.Critical pre-merge finding: the recon report claimed only one endpoint needed REST_BASE migration. Code audit during implementation found two endpoints (
GET /emailslisting +POST /emails/{id}/toggle) still interpolated\$wizard_slugdirectly — making them not yet pinned. Both were migrated in commit 1 to useREST_BASEdirectly. All 4 endpoints inEmails_Sectionnow useREST_BASE; the architectural-lock-in testtest_no_wizard_slug_in_rest_route_registrationprevents future regression.Emails_Section class file stays in
includes/wizards/newspack/The PHP class file did NOT move. The namespace
Newspack\Wizards\Newspackdoesn't need to match the wizard the section belongs to — it's just a class location. Moving it would force a namespace change, composer dump-autoload regeneration, and updates to the include_once chain — for zero behavioral benefit. The\$wizard_slugauto-flips whenWizard_Section::__constructgets called by the new parent wizard.Frontend files relocated
The frontend folder moved from
src/wizards/newspack/views/settings/emails/tosrc/wizards/audience/views/setup/emails/. All 10 files moved viagit mvto preserve blame history (100% similarity score per file). Only line-level change: the import path insrc/wizards/audience/views/setup/index.js.Deep-link redirect
Both server-side and client-side handlers, with the client-side doing the work in practice:
Client-side (load handler): module-top
window.location.replace()insrc/wizards/newspack/views/settings/sections.tsxfires before the React tree mounts. Bookmarks and external links pointing at the old?page=newspack-settings#/emailsURL get redirected before any Settings UI flashes on-screen. Strict equality on#/emailshash — partial matches like#/emails-somethingdon't redirect.Server-side (admin_init hook):
Newspack_Settings::maybe_redirect_legacy_emails_url()handles the?page=newspack-settings&emails=1case. Currently no caller generates?emails=1URLs, so this handler is speculative future-proofing — intentionally available for programmatic deep-linking (e.g., external integrations) without depending on JS execution. The code is commented explaining this intent so a future contributor doesn't wonder where?emails=1URLs come from.Both handlers use
wp_safe_redirect()with default 302 (matches existingadmin_redirects()pattern inclass-newspack.php). The 302 (not 301) avoids browser cache poisoning if we ever change direction on this URL.Tests
5 architectural-lock-in tests in
tests/unit-tests/emails-section.php(new Bucket G):test_emails_section_wizard_slug_is_audience— asserts\$wizard_slug === 'newspack-audience'after Newspack init, catches re-registration drifttest_rest_base_is_pinned_to_newspack_settings— locks the docblock invariant against future drifttest_no_wizard_slug_in_rest_route_registration— inspectsrest_get_server()->get_routes(), asserts no routes under/wizard/newspack-audience/emails*(filters out Audience's unrelated/audience-management/emails/{id}reset endpoint via negative regex). Prevents a future contributor from re-introducing the\$wizard_sluginterpolation patterntest_legacy_emails_url_redirects_with_hint— simulates?page=newspack-settings&emails=1, asserts server-side redirect to new URL via throw-from-filter pattern (matchestests/unit-tests/content-gate/group-subscriptions.php)test_legacy_emails_url_no_redirect_without_hint— bare?page=newspack-settings, asserts handler does NOT redirectPHP suite: 1344 → 1349 (+5 tests / +12 assertions, no other counts shifted). JS suite: 217/217 unchanged.
Mid-deploy continuity
wizard/newspack-settings/emails/*continue working.newspack-wizards.HASH.js) prevents stale-bundle issues.