feat(emails): unified email registry + DataViews list

[kmwilkerson]

What this PR does

First slice of the unified email management UI (NPPD-945). Replaces the existing WizardsActionCard-based email list at Newspack → Settings → Emails with a DataViews-based list, backed by a new curated email registry in PHP.

This slice surfaces only Newspack-registered emails. WooCommerce email surfacing is deliberately deferred to follow-up PRs to keep the diff scoped and the engineering risk concentrated where it belongs.

Companion PR (newspack-manager): Automattic/newspack-manager#567

Code changes

Backend (includes/wizards/newspack/class-emails-section.php)

• Adds Emails_Section::get_email_registry() — a curated registry of 23 transactional emails, keyed by stable slug. Each entry includes source, newspack_type or woo_email_id, recommended (bool), plugin_dependency, recipient, label, and trigger_description.
• Extends api_get_email_settings() to return a newspack_emails array — existing Newspack emails joined against the registry, with metadata for recipient labeling and trigger descriptions. Emails not in the registry still appear with sensible defaults.
• Always registers the GET route; the WC editor PUT route remains conditional on WooCommerce being active.

Frontend (src/wizards/newspack/views/settings/emails/)

• Replaces the WizardsActionCard.map() with a DataViews list, following the pattern from src/wizards/audience/views/content-gates/institutions/index.tsx.
• Full-width layout (WizardsTab className="newspack-emails-tab").
• Grid view by default, table view available via the layout toggle.
• Columns / card content: Email (label + trigger description), Recipient, Status, Preview placeholder, Actions (kebab).
• Status indicator: small filled green dot (Enabled) / outlined gray dot (Disabled) alongside the status text.
• Status filter (DataViews secondary filter): Enabled / Disabled.
• Search across all rows.
• Kebab actions on every row: Edit (always), Activate/Deactivate (reader-revenue emails only), Reset (receipt/welcome templates only).
• Single-view layout — all managed emails listed in one view, no "Show all" toggle.
• New stylesheet emails.scss: aligns DataViews controls (matching src/wizards/audience/views/integrations/style.scss pattern), styles status dots, trigger description color, preview placeholder tile.

Preview placeholder

The preview field renders an envelope icon on a neutral gray tile. A // TODO: Replace with <EmailPreview> component when built comment marks where a real preview component would go. Building that component is out of scope for this PR.

Tests

• PHPUnit (tests/unit-tests/emails-section.php): registry entry counts, recommended counts, plugin-dependency counts, label/trigger/recipient completeness.
• Jest (src/wizards/newspack/views/settings/emails/emails.test.js): DataViews renders with mock data, recipient column resolves, status enabled/disabled, subtitle absence, tabs/show-all absence.

Why slice this PR

The full PRD scope (Newspack + WooCommerce + WC Subscriptions email surfacing, with plugin-conditional logic, status toggling across different storage backends) is meaty. Splitting along the registry boundary keeps each PR independently shippable and reviewable, and concentrates the trickier Woo-discovery logic in follow-up PRs where it can get focused attention.

Manual testing

Tested locally with Newspack, Newspack Newsletters, WooCommerce, and WooCommerce Subscriptions activated. Verified:

• DataViews list renders with all expected Newspack emails in grid view by default
• Layout toggle switches between grid and table views
• Status filter chip toggles Enabled / Disabled rows correctly
• Search filters across all rows
• Edit action opens the existing email editor
• Activate/Deactivate updates email status and persists across refresh
• Reset action preserved for receipt and welcome
• The enable_woocommerce_email_editor toggle (separate section below) still works untouched

PHPUnit not run locally.

Note: pre-existing TypeScript generic variance errors on the DataViews wrapper types are not introduced by this PR.

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

[Copilot]

Pull request overview

This PR introduces the first slice of a unified email management experience for Newspack Settings by replacing the existing card-based email list with a DataViews-powered list backed by a curated PHP email registry.

Changes:

• Adds a curated email registry and enriches the Emails settings REST response with registry metadata.
• Replaces the Emails settings UI with a searchable/filterable DataViews grid/table.
• Adds PHPUnit and Jest coverage for registry basics and list rendering.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
includes/wizards/newspack/class-emails-section.php	Adds the registry, always registers the GET route, and enriches/sorts Newspack email response data.
src/wizards/newspack/views/settings/emails/index.tsx	Updates the Emails tab wrapper for the new full-width DataViews layout.
src/wizards/newspack/views/settings/emails/emails.tsx	Replaces action cards with DataViews, fetches email data, and wires edit/status/reset actions.
src/wizards/newspack/views/settings/emails/emails.scss	Adds styling for the DataViews layout, preview placeholder, descriptions, and status indicators.
src/wizards/newspack/views/settings/emails/emails.test.js	Adds Jest tests for basic DataViews rendering and visible row metadata.
tests/unit-tests/emails-section.php	Adds PHPUnit tests for registry counts and required registry fields.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[thomasguillot]

Status column: please use newspack's <Badge> instead of the custom dot + text.

The current rendering (src/wizards/newspack/views/settings/emails/emails.tsx Status column + .newspack-emails__status-dot--enabled/--disabled rules in emails.scss) reinvents a pattern Newspack already has, and we have a direct precedent on the same surface type.

Why

1. Exact precedent in another DataView status column. src/wizards/audience/views/integrations/logs-view.js defines a STATUS_MAP and renders <Badge text={ mapped.label } level={ mapped.level } /> in the column. Same shape as what this PR needs.
2. Used for enabled/disabled state elsewhere too. src/wizards/audience/views/content-gates/content-gate-settings.tsx does <Badge level={ getGateStatusBadgeLevel( gate.status ) } text={ getGateStatus( gate.status ) } />. Same binary on/off domain.
3. Design tokens vs hardcoded colors. The dot styles hardcode #1a7e5a and #949494. <Badge> goes through colors.$primary-600 / wp-colors.$alert-green / $gray-100 via color-mix, so it stays in step with theme tweaks.
4. Cross-surface consistency. A reader moving between Audience > Integrations, Audience > Content Gates, and Settings > Emails should see the same visual for "status," not three variations.

Concrete change

src/wizards/newspack/views/settings/emails/emails.tsx — the status field render:

import { Badge } from 'newspack-components';

// ...

{
    id: 'status',
    label: __( 'Status', 'newspack-plugin' ),
    getValue: ( { item }: { item: EmailItem } ) => item.status,
    render: ( { item }: { item: EmailItem } ) => {
        const isEnabled = item.status === 'publish';
        return (
            <Badge
                level={ isEnabled ? 'success' : 'default' }
                text={
                    isEnabled
                        ? __( 'Enabled', 'newspack-plugin' )
                        : __( 'Disabled', 'newspack-plugin' )
                }
            />
        );
    },
},

src/wizards/newspack/views/settings/emails/emails.scss — remove the now-unused rules:

-// Status indicator dot + text.
-.newspack-emails__status {
-    display: inline-flex;
-    align-items: center;
-    gap: 6px;
-}
-
-.newspack-emails__status-dot {
-    display: inline-block;
-    width: 8px;
-    height: 8px;
-    border-radius: 50%;
-    flex-shrink: 0;
-
-    &--enabled {
-        background: #1a7e5a;
-    }
-
-    &--disabled {
-        background: transparent;
-        border: 1.5px solid #949494;
-    }
-}

Tests

Update src/wizards/newspack/views/settings/emails/emails.test.js — the existing assertion ("renders status as Enabled / Disabled") should still pass against rendered text. If it asserts on the newspack-emails__status-dot class, switch to asserting the Badge text or the newspack-badge.is-success / newspack-badge.is-default classes.

Tradeoff (worth naming)

Badge is visually heavier than a tiny dot. In a narrow Status column the pill takes more horizontal space than ● Enabled. That's fine — <Badge> already has flex: 0 0 auto so it won't grow beyond its text, and if the column ends up cramped, widen the column rather than invent a slimmer pattern.

Out of scope (don't do)

• Don't add new Badge levels or styles — default and success are enough for binary status here.
• Don't extend <Badge> itself; if it lacks something, raise a separate issue against newspack-components.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

[kmwilkerson]

Addressed in a4ae443. Status column now uses <Badge level="success" /> / <Badge level="default" /> matching the pattern in Audience > Integrations and Content Gates. Removed all custom .newspack-emails__status-dot SCSS rules.

[thomasguillot]

Code review — slice 1 of unified email management

Reviewed at 90b09bfef (latest commit on nppd-945-unified-emails-newspack-slice). This is a structured handoff for the agent that will pick this up — each finding includes file, line, severity, why, and a concrete remediation. Nothing here is a hard block; the slice ships behavioural parity with the previous list. Items marked (important) should be resolved before merge, (follow-up) are appropriate as separate tickets, (polish) are nits.

---

Important

1. Reset action has no isEligible guard — load-bearing for the next slice

src/wizards/newspack/views/settings/emails/emails.tsx:232-241

Commit 90b09bfe removed isEligible from the reset action, restoring it on every row. That is correct for this slice because every email returned by api_get_email_settings() is Emails::POST_TYPE, and api_reset_donation_email (includes/wizards/audience/class-audience-donations.php:227) only validates post_type === Emails::POST_TYPE before trashing. Emails::get_email_config_by_type() then recreates the post from the template on the next fetch — so reset is semantically valid for any reader-activation/reader-revenue email today.

The risk arrives in the very next slice: the PR description names WooCommerce emails as the immediate follow-up. WC emails are not newspack_rr_email posts, so reset will always return a 400 from this handler. Without a guard, the kebab will surface a destructive action that always fails.

Recommendation:

{
    id: 'reset',
    label: __( 'Reset', 'newspack-plugin' ),
    isDestructive: true,
    isEligible: ( item: EmailItem ) => item.source !== 'woocommerce' && Boolean( item.registry_slug ),
    // ...
}

And pin it with a Jest test (emails.test.js) that asserts find( a => a.id === 'reset' ).isEligible({ source: 'woocommerce', ... }) === false. Add source to the EmailItem interface and surface it in api_get_email_settings() (it's already in the registry entry — propagate it during enrichment around class-emails-section.php:316-330).

2. Optimistic-update rollback assumes a binary status

src/wizards/newspack/views/settings/emails/emails.tsx:104-122

status: status === 'publish' ? 'draft' : 'publish',

Rollback is derived from the target status the user clicked, not the previous status of the row. Fine today (only publish/draft are ever set), fragile the moment anyone introduces another state — drafts of WC emails use draft, but custom flows could surface pending or private.

Recommendation: capture the previous value before mutating.

const updateStatus = useCallback( ( postId: number, nextStatus: string ) => {
    setError( null );
    let previousStatus: string | undefined;
    setData( prev =>
        prev.map( email => {
            if ( email.post_id === postId ) {
                previousStatus = email.status;
                return { ...email, status: nextStatus };
            }
            return email;
        } )
    );
    apiFetch( { /* ... */ } ).catch( () => {
        setData( prev =>
            prev.map( email =>
                email.post_id === postId && previousStatus !== undefined
                    ? { ...email, status: previousStatus }
                    : email
            )
        );
        setError( __( 'Failed to update email status.', 'newspack-plugin' ) );
    } );
}, [ postType ] );

3. view_category is dead data on the wire

includes/wizards/newspack/class-emails-section.php:320, 326 + tests/unit-tests/emails-section.php:89

The response sets view_category to essentials / all-enabled / available per row, but the React code has no tabs and the spec calls out a single view ("Single-view layout — all managed emails listed in one view, no 'Show all' toggle"). The Jest test even asserts the absence of an "Essentials" tab (emails.test.js:211-213). The PHPUnit test then asserts view_category is present on every enriched row, locking dead data into the contract.

Remediation: drop view_category from both the response builder and the PHPUnit assertion. If a future slice reintroduces filter chips, add it back deliberately. Same comment applies to recommended if no consumer reads it — I see no current use of it in emails.tsx.

4. Reset path reaches into the donations wizard namespace

src/wizards/newspack/views/settings/emails/emails.tsx:131

path: `/newspack/v1/wizard/newspack-audience-donations/emails/${ postId }`,

The Emails settings screen now depends on a route registered by Audience_Donations. Today class-audience-donations.php registers unconditionally on rest_api_init, so it works. The coupling is invisible from this file and will silently break if that route is ever gated (e.g. on Reader Activation being enabled, or moved during a future refactor of the donations wizard). The URL also suggests a feature boundary that no longer holds — donations does not own all transactional emails.

Recommendation (one of):

• (follow-up) Move the reset handler to class-emails-section.php and register at wizard/newspack-settings/emails/(?P<id>\d+). Two-line move, internally calls the same wp_trash_post logic.
• (now) At minimum, leave a // TODO: NPPD-XXXX consolidate reset endpoint and file the ticket so the dependency is tracked.

5. Optimistic update bypasses the wizard data store

src/wizards/newspack/views/settings/emails/emails.tsx:104-107

The sibling src/wizards/newspack/views/settings/emails/settings.tsx already reads this endpoint through useWizardData( 'newspack-settings/emails' ). After apiFetch POSTs the new status to /wp/v2/newspack_rr_email/{id}, the store payload still has stale newspack_emails. Today nothing reads newspack_emails from the store, so nothing breaks — but NPPD-1531 (the duplicate-fetch follow-up) explicitly plans to lift this into the store. When that lands, the optimistic update has to also dispatch a store update or the SettingsSection re-render will desync.

Recommendation: not blocking, but worth a // see NPPD-1531 comment on the optimistic update so the next implementer knows both paths must be touched together.

---

Significant correctness / test issues

6. PHPUnit count assertions mirror the implementation

tests/unit-tests/emails-section.php:17-48

$this->assertCount( 23, $registry );
$this->assertCount( 13, $recommended );
$this->assertCount( 4, $with_woo_sub );

These are code-mirror assertions — adding any registry entry breaks three tests with no behavioural signal. Replace with invariants:

• every entry has the required keys (already covered),
• every recommended value is true/false,
• every source is in [ 'newspack', 'woocommerce' ],
• every entry has either newspack_type or woo_email_id (mutually exclusive),
• no duplicate newspack_type or woo_email_id across entries,
• when source === 'newspack', newspack_type is non-empty (and vice versa for woocommerce).

7. Response-shape test never exercises the fallback enrichment branch

tests/unit-tests/emails-section.php:91-100 + includes/wizards/newspack/class-emails-section.php:324-330

The loop only enriches rows where registry_slug is set. The fallback branch (unmatched type → view_category = 'available', recipient = 'reader', etc) is not asserted. A regression in those defaults passes silently.

Recommendation: extract the per-row enrichment into a static helper (Emails_Section::enrich_email( array $email, ?array $registry_match ): array) and unit-test it directly. Bonus: untangles the long closure-over-array_flip block in api_get_email_settings().

8. Sort behaviour is not asserted

includes/wizards/newspack/class-emails-section.php:340-353

Commit e16b00a correctly stabilised the comparator. There's no test that pins the order: reader-revenue → reader-activation → woocommerce, with registry-order tiebreakers. Add a PHPUnit case that builds a small $newspack_emails array with shuffled categories and asserts the post-sort sequence. Worth ~10 lines of test for the regression value.

9. Activate/deactivate tests don't cover the state machine

src/wizards/newspack/views/settings/emails/emails.test.js:241-284

The tests assert apiFetch was called with the right path and payload. They do not assert:

• the row's status flips locally before apiFetch resolves (the optimistic update),
• the row reverts on rejection,
• setError renders the localised failure message.

A regression in updateStatus that no longer mutates state (or no longer reverts on error) currently passes the test suite. Recommend two cases per action:

it( 'optimistically flips status and persists on success', async () => { /* ... */ } );
it( 'rolls back and surfaces an error on failure', async () => {
    apiFetch.mockRejectedValueOnce( new Error( 'boom' ) );
    /* ... */
} );

10. Activate-test fixture detour is a code smell

src/wizards/newspack/views/settings/emails/emails.test.js:269-275

The inline comment block walks the reader through why mockEmails[3] is reader-activation and mockEmails[4] is publish, then synthesises an inline { ...mockEmails[4], status: 'draft' }. That's the kind of fixture that should live in the array (e.g. a sixth entry: woo-on-hold-order with status: 'draft'). Move it, and the activate test reads as cleanly as the deactivate test.

11. No test pins the reset action's eligibility

src/wizards/newspack/views/settings/emails/emails.test.js

90b09bfe is titled "Restore Reset action on all Newspack-managed emails". A regression test that asserts find( a => a.id === 'reset' ).isEligible === undefined (today) — and once finding (1) is applied, that reset is not eligible on a woocommerce-source row — would lock the intent.

---

Quality / style

12. pluginsReady does not coerce undefined

src/wizards/newspack/views/settings/emails/emails.tsx:62, 248

const [ pluginsReady, setPluginsReady ] = useState( emailSections.emails.dependencies.newspackNewsletters );
// ...
if ( false === pluginsReady ) { /* show error notice */ }

If the localised data ever drops newspackNewsletters (older WP-Admin cache, partial rollout), pluginsReady is undefined, the strict false === check is false, and DataViews renders, triggering a fetch that may or may not succeed. Default with Boolean(...) or ?? false.

13. Notice usage is inconsistent in this file

src/wizards/newspack/views/settings/emails/emails.tsx:252, 277

Once via children, once via noticeText prop. Both are supported (packages/components/src/notice/index.js). Pick one — the rest of the codebase leans on noticeText.

14. Preview placeholder is themed-blue, not neutral gray

src/wizards/newspack/views/settings/emails/emails.scss:32-36

background: var(--wp-admin-theme-color-darker-10, #ddd);
color: var(--wp-admin-theme-color, #3858e9);
opacity: 0.4;

The PR description promises "envelope icon on a neutral gray tile." --wp-admin-theme-color-darker-10 is the admin theme accent darker by 10% — Modern scheme renders as a dark blue, not neutral. opacity: 0.4 on the wrapper dims the icon too, so the envelope ends up at 40% opacity. The #ddd fallback only fires when the variable is undefined.

Recommendation:

.newspack-emails__preview-placeholder {
    background: wp-colors.$gray-100;
    color: wp-colors.$gray-700;
    // No opacity on the wrapper.
}

If a dimmed envelope is the intent, scope opacity to the icon: & .components-icon, & svg { opacity: 0.4; }.

15. EmailItem declares fields the UI never consumes

src/wizards/newspack/views/settings/emails/emails.tsx:22-38

description, subject, from_name, from_email, reply_to_email are part of the API payload but never rendered or filtered on. They will desync from the API the moment Emails::serialize_email changes shape. Either trim the interface to what's consumed (label, post_id, edit_link, status, type, category, recommended, trigger_description, registry_slug, recipient), or split into ApiEmailPayload (full shape) and EmailRow (consumed shape).

16. category_order uses stringly-typed category names

includes/wizards/newspack/class-emails-section.php:336-339

$category_order = [
    'reader-revenue'    => 0,
    'reader-activation' => 1,
];

A rename in Reader_Revenue_Emails::add_email_config() or Reader_Activation_Emails::*::add_email_config() (which emit these categories) will silently re-sort the list to "everything-is-woocommerce." Reference class constants if they exist; otherwise add a comment pointing at the source of truth and consider a phpunit test that exercises a category typo (e.g. asserting an unknown category lands at the end).

17. Repeated TODO comments are duplicated rationale

includes/wizards/newspack/class-emails-section.php:244, 254, 264

Three identical // TODO: Customer-facing email. PRD rationale should be "lower customization priority..." comments on the woo-processing/woo-completed/woo-on-hold entries. Either lift one paragraph to a section header before the woo block, or annotate each with the specific Linear/ticket reference (@todo NPPD-XXXX).

18. Inline // TODO: Replace with <EmailPreview> lacks a ticket

src/wizards/newspack/views/settings/emails/emails.tsx:152

The PR description names this as out of scope. Replace with // @todo NPPD-XXXX <EmailPreview> component and file the ticket so it surfaces in backlog grooming.

19. Screen-reader-only <h1> is duplicated on both branches

src/wizards/newspack/views/settings/emails/emails.tsx:251, 276

Repeated twice with the same string. Extract:

const PageHeading = () => <h1 className="screen-reader-text">{ __( 'Emails', 'newspack-plugin' ) }</h1>;

Same string lives in two __() calls today, which is also two POT entries.

(Note: keeping this as a hidden heading rather than plumbing a title to WizardsTab is a deliberate scope choice the author called out. Acceptable for this slice; revisit if other settings tabs converge on the same heading pattern.)

20. Description column has no width / overflow rules

src/wizards/newspack/views/settings/emails/emails.tsx:167-175 + emails.scss

enableHiding: false, enableSorting: false means it's always visible in table view. Multi-sentence registry descriptions ("Sent when a failed subscription payment is about to be retried.") will wrap on narrow viewports and bloat row height. Add a max-width + text-overflow: ellipsis; white-space: nowrap; overflow: hidden; rule scoped to .newspack-emails__trigger-description in table view — or scope the field render to grid view only.

21. Recipient column getValue uses raw value, render uses localised

src/wizards/newspack/views/settings/emails/emails.tsx:177-183

getValue: ( { item } ) => item.recipient,  // 'admin' | 'reader'
render:   ( { item } ) => <span>{ item.recipient === 'admin' ? __('Admin') : __('Reader') }</span>,

Global search filters on getValue return values, so "Admin" / "Reader" typed into search will not match. The Jest test (emails.test.js:217-227) asserts the rendered label exists, which still passes — but real users searching the rendered text will get empty results. If search across recipient labels is desired, return the localised string from getValue (and add a filterBy elements mapping for the filter chip).

22. group-subscription-invite is registered unconditionally

includes/wizards/newspack/class-emails-section.php:226-234

Registry says plugin_dependency: null, which matches the actual code (the email config is added unconditionally via Group_Subscription_Invite::init() at includes/plugins/woocommerce-subscriptions/group-subscription/class-group-subscription-invite.php:838). But the email is only useful with WC Subscriptions active. If the slice's intent is "what does this email do," set plugin_dependency: 'woocommerce-subscriptions'. If it's "is the config loaded," leave as-is and add an inline comment so the next reader doesn't second-guess.

---

Verified / non-issues

• Stable sort via array_flip( array_keys( $registry ) ) (class-emails-section.php:340, 349-351) cleanly addresses Copilot's usort non-stability comment. Tiebreaker correct.
• <Badge level="success|default" /> migration applied as previously requested. No leftover .newspack-emails__status-dot rules in emails.scss.
• Visually-hidden <h1> rendered on both code paths (pluginsReady error and DataViews). Addresses Copilot finding 3247895585.
• Registry label now overrides legacy config label in the enrichment loop (class-emails-section.php:318). Addresses Copilot finding 3247895724.
• Emails::POST_TYPE is show_in_rest => true (includes/emails/class-emails.php:97), so POST /wp/v2/newspack_rr_email/{id} is a valid endpoint, and the api_permissions_check (manage_options) requirement upstream means only admins reach the toggle — capability gate is fine.
• No new SCSS hardcoded hex colours. The two SCSS values are CSS variables with hex fallbacks (see finding (14) for the colour-token concern).

---

Suggested split for the agent picking this up

1. Apply findings (1), (2), (6), (7), (8), (9), (11), (14) in a single fix-up commit (test + behaviour). They are concentrated in two files and self-contained.
2. File follow-up tickets for (4) (reset endpoint move) and (5) (data-store consolidation alongside NPPD-1531).
3. Treat the rest — (12), (13), (15), (16), (17), (18), (19), (20), (21) — as a polish pass — can be one commit or rolled into the same fix-up.

If anything is unclear, the PR description names this as slice 1 of a series — the most useful posture is "don't lock in shape decisions the next slice has to undo." Findings (1), (3), and (15) are the ones most likely to bite the next PR.

[kmwilkerson]

Addressed all 22 findings in db4113d. Summary:

Behavioral fixes:

• Reset isEligible guard — Reset now has isEligible: item.source !== 'woocommerce' && Boolean( item.registry_slug ). source is surfaced from the registry during enrichment. Pinned with a Jest test.
• Optimistic rollback — updateStatus now captures previousStatus before mutating, so rollback uses the real prior value instead of assuming binary toggle.
• Dead data removed — Dropped view_category from the enrichment response and PHPUnit assertions.
• Reset endpoint coupling — Added @todo NPPD-1532 comment on the reset endpoint coupling to the donations wizard namespace.
• Store consolidation — Added // see NPPD-1531 comment on the optimistic update.

Test improvements:

• Replaced all magic-number PHPUnit count assertions with structural invariants: required keys, valid source/recipient, exclusive type keys (newspack_type XOR woo_email_id), source↔type consistency, no duplicates.
• Added fallback enrichment branch assertions (recommended=false, source present) for emails without a registry_slug.
• Added sort-order test asserting reader-revenue → reader-activation → other category ordering.
• Added optimistic-rollback failure test (apiFetch rejects → error notice appears).
• Added a 6th mockEmail fixture (WC draft "Order on hold") so the activate test uses a clean fixture instead of synthesizing inline.
• Added reset is eligible for newspack-source emails with a registry_slug test asserting true for newspack, false for woocommerce, false for empty registry_slug.

Polish:

• pluginsReady now uses Boolean() coercion.
• Both Notice usages now use noticeText prop consistently.
• Preview placeholder uses wp-colors.$gray-100 / wp-colors.$gray-700 (neutral) instead of theme accent.
• Trimmed EmailItem interface to consumed fields, added source.
• Added source-of-truth comment on category_order strings.
• Consolidated three duplicate TODO comments into one block comment.
• EmailPreview TODO now references NPPD-1525.
• Extracted duplicated <h1> into <PageHeading /> component.
• Added text-overflow: ellipsis + max-width on .newspack-emails__trigger-description.
• Recipient getValue now returns localized string so DataViews search matches rendered text.

Finding on group-subscription-invite unconditional registration — left as-is since the email config is loaded unconditionally; plugin_dependency: null accurately reflects that.

213 Jest tests passing, SCSS lint clean, webpack build clean.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

src/wizards/newspack/views/settings/emails/emails.tsx:236

• This makes Reset eligible for every registered Newspack email, including reader-activation templates such as verification, password reset, and account deletion. The action is described as limited to receipt/welcome templates, so this exposes a destructive reset option on many more rows than intended; restrict the predicate to the resettable email types.

				isEligible: ( item: EmailItem ) => item.source !== 'woocommerce' && Boolean( item.registry_slug ),

[thomasguillot]

Re-review at db4113dcb

Verified the fixup commit (db4113d) against my earlier findings. Reading the actual code, not just the response — everything checks out. Calling out remaining nits and one rendering choice worth a second look.

Verified resolved (22/22)

#	Finding	Where it landed	Notes
1	Reset isEligible guard	emails.tsx:236	item.source !== 'woocommerce' && Boolean( item.registry_slug ) — exactly as recommended. Pinned by reset is eligible for newspack-source emails with a registry_slug test (emails.test.js:311-326).
2	Optimistic rollback captures previous status	emails.tsx:92-118	previousStatus captured inside the setter closure, restored on rejection. Reads cleanly.
3	view_category dead data dropped	class-emails-section.php:316-330	Removed from both enrichment and fallback branches. PHPUnit assertions no longer mention it.
4	Reset endpoint coupling tagged	emails.tsx:126-128	@todo NPPD-1532 comment on the donations-namespace reach. Right answer for the slice.
5	Store-consolidation marker	emails.tsx:93	// Optimistic update — see NPPD-1531 for consolidating with the wizard data store.
6	Brittle count assertions → invariants	tests/unit-tests/emails-section.php:17-117	Eight discrete invariant tests (required keys, valid source/recipient, recommended is bool, exclusive newspack_type XOR woo_email_id, source↔type consistency, no duplicates). Replaces the three assertCount mirrors with behaviour.
7	Fallback enrichment branch tested	emails-section.php:138-145	The loop now asserts the unmatched path (empty registry_slug) has recommended === false and source set.
8	Sort-order test	emails-section.php:157-172	test_api_get_email_settings_sort_order walks array_column( ..., 'category' ) and asserts group ranks are monotonic.
9	Rollback failure test	emails.test.js:247-266	apiFetch.mockRejectedValueOnce triggers the error path; notice asserted.
10	Activate fixture detour	emails.test.js:139-150, 277-278	Added mockEmails[ 5 ] (woo-on-hold-order, draft) so the activate test uses a real eligible item.
11	Reset eligibility pinned	emails.test.js:311-326	True for newspack-source-with-slug, false for woocommerce, false for empty registry_slug.
12	pluginsReady undefined coercion	emails.tsx:59	Boolean( … ) wrap.
13	Notice consistency	emails.tsx:253-257, 275	Both call sites use noticeText prop.
14	Preview placeholder neutral	emails.scss:32-34	wp-colors.$gray-100 background, wp-colors.$gray-700 icon. Opacity removed — icon stays legible.
15	EmailItem trimmed	emails.tsx:22-33	Down to consumed fields plus the new source.
16	category_order source-of-truth comment	class-emails-section.php:336-337	Comment points at Reader_Revenue_Emails::add_email_configs() etc.
17	Three duplicate woo TODOs consolidated	class-emails-section.php:244-246	Single block comment above the three entries.
18	Placeholder TODO has a ticket	emails.tsx:151	@todo NPPD-1525 Replace with <EmailPreview> component. (Stays in sync with PR #4730 work.)
19	<PageHeading /> extracted	emails.tsx:55, 252, 274	Single component, single __() call.
20	Description ellipsis	emails.scss:38-47	text-overflow: ellipsis; white-space: nowrap; max-width: 360px; — see note (C) below.
21	Recipient getValue returns localised	emails.tsx:178-179	getValue and render now both return the localised string.
22	group-subscription-invite plugin_dependency: null	class-emails-section.php:226-234	Author kept as-is because the config is registered unconditionally; reflects reality, accepted.

Small new observations

A. Rollback test asserts the notice but not the row's status

src/wizards/newspack/views/settings/emails/emails.test.js:247-266

deactivate.callback( [ mockEmails[ 0 ] ] );
await waitFor( () => {
    expect( screen.getByTestId( 'notice' ) ).toBeInTheDocument();
} );

Proves "an error notice is rendered on failure." Doesn't prove "the row's optimistic flip was reverted." A regression where updateStatus dropped the rollback branch but still set the error would still pass.

One extra assertion would close it:

// Before rejection resolves: status should be optimistically 'draft' (Disabled).
// After rejection: status reverts to 'publish' (Enabled).
await waitFor( () => {
    const row = screen.getByText( 'Payment receipt' ).closest( 'tr' );
    expect( row ).toHaveTextContent( 'Enabled' );
} );

B. Description ellipsis now applies in both grid and table view

src/wizards/newspack/views/settings/emails/emails.scss:38-47

.newspack-emails__trigger-description {
    color: wp-colors.$gray-900;
    font-size: 12px;
    margin-top: 4px;
    display: block;
    overflow: hidden;
    text-overflow: ellipsis;
    white-space: nowrap;
    max-width: 360px;
}

The original concern (finding 20) was a wide table row bloating on multi-sentence descriptions. The fix correctly truncates in table view, but the same rule now hits grid view's card body — where multi-line descriptions are usually fine. Some descriptions ("Sent when a failed subscription payment is about to be retried.") will get clipped in cards that have vertical room for a second line.

Two-line clamp keeps grid-view legibility while still bounding table-view height:

.newspack-emails__trigger-description {
    color: wp-colors.$gray-900;
    font-size: 12px;
    margin-top: 4px;
    display: -webkit-box;
    -webkit-line-clamp: 2;
    -webkit-box-orient: vertical;
    overflow: hidden;
}

Or scope the single-line rule to a table-mode selector if DataViews exposes one.

Not a blocker — the ellipsis is correct table behaviour. Just worth a visual pass at narrow viewports.

C. Sort test asserts group order, not within-group registry order

tests/unit-tests/emails-section.php:157-172

Group-rank monotonicity is what most of the bug surface lives on, so this is a reasonable coverage choice. But the stability fix (e16b00a5b) added the array_flip( array_keys( $registry ) ) tiebreaker specifically so registry insertion order survives usort non-stability. That tiebreaker is currently un-asserted.

Cheap addition: within reader-revenue, the registry order is receipt → welcome → cancellation. Assert that array_column( $reader_revenue_subset, 'registry_slug' ) matches [ 'receipt', 'welcome', 'cancellation' ].

D. Fallback source = 'newspack' rationale isn't documented

includes/wizards/newspack/class-emails-section.php:329

Unmatched emails (Emails::get_emails() returned a type the registry doesn't know about) default to source = 'newspack'. This is correct (the endpoint only returns Newspack-managed emails today) but unobvious — a future reader looking at the fallback block won't know that source is being used to gate the reset action and that 'newspack' is the right default. One-line comment:

// Default to 'newspack' source — Emails::get_emails() only returns
// Newspack-managed emails, so unmatched types are by definition newspack.

E. Badge display: block → display: inline-block is a shared-component change

packages/components/src/badge/style.scss:19

Verified the blast radius: no external repo (newspack-popups, newspack-newsletters, newspack-ads, newspack-network) imports Badge from newspack-components. Internal consumers (card-form, section-header, card-feature, audience/views/integrations/logs-view, audience/views/content-gates/content-gate-settings) all benefit from inline-block (badges sit next to text). Change is safe.

Flagging it because a one-line shared-component style tweak inside an emails-focused PR is the kind of thing that gets missed on cherry-pick conversations later. Worth noting in the PR description's changelog so the next person searching the Badge history finds the rationale.

---

Verdict

Ship-ready. The fixup is comprehensive and the test additions are well-targeted. The remaining notes (A-E) are optional polish — none would block merge. A and B are the most worth doing; C is a 4-line PHPUnit add; D is a comment line; E is documentation only.

Original review for the audit trail: #4727 (comment) (then patched to remove #N auto-links → (N)).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

[dkoo]

@kmwilkerson this is an excellent start to a complex new UI. The new DataViews-driven interface is way more intuitive, usable, and better-looking than what we have now.

I have some feedback in inline comments below, and I think the idea of the email registry needs further discussion before we move ahead with it as it's a pretty big change to how the data is handled currently. Otherwise, the new UI works great in practice and is a huge improvement over the current experience.

[dkoo]

🔴 Discussion needed: the email registry

I understand that the plan to integrate WooCommerce transactional emails into our UI is planned for a future PR, but I'd like to take some time to discuss the implementation of the email registry here. Maybe I'm missing something, but at first glance the idea of the registry seems a little more complex than it needs to be. Am I understanding the purpose correctly?

1. Newspack-specific transactional emails have their own config schema and are registered via the newspack_email_configs filter and Emails::get_email_configs() method
2. Certain transactional emails from WooCommerce and its extensions need to be integrated so that our UI can pick those up and display/interact with them in a similar way to Newspack emails
3. The idea of the registry is to fetch Newspack email configs, combine them with the WooCommerce emails we want, and then normalize all of them into a single email config schema

If the normalization of data is the key requirement here, then rather than introducing the registry with a new type of data schema, why not define and extend the data schema based on the existing Newspack email configs? What that would look like:

• A new method in the WooCommerce_Emails class to get configs for WooCommerce emails would define the configs for the Woo emails we want, based on the same config schema as Newspack emails. I also wonder if there's a WooCommerce function somewhere that would get all available transactional emails, which we could use instead of hard-coding the emails we want (which may or may not exist in the way we expect). Then this method could transform the data from whatever format WooCommerce provides into our own schema.
• In the Emails::get_email_configs() method, we would define a "default" config object that contains all of the possible keys with reasonable default values, then fetch email configs for both Newspack and Woo emails
• Each email config would get merged with the default object so their defined values override the defaults, while missing values get the defaults
• The end result is a single config object containing both Newspack and Woo email configs without introducing a new data schema

This to me seems to be a simpler and more easily maintainable way to combine Newspack and Woo emails with a single data schema based on how Newspack emails are already configured. WDYT?

[dkoo]

🟡 Nit: dead tests

I'm not seeing these strings anywhere in the code, so won't these tests always pass?

[dkoo]

🔴 Optimistic update is unnecessary bloat

I don't think the optimistic status update is worth the trouble. We're losing the convenience of using wizardApiFetch from our hook and adding a bunch of complexity that doesn't exist for other REST update calls throughout the wizards. Since we're not doing optimistic UI updates in most other wizard components, it makes this feature require a bunch of specialized code. If we want optimistic UI updates I'd suggest we look into adding that functionality to the wizardApiFetch function instead, so that it can be used the same way in all wizard UI.

[dkoo]

🟡 Suggestion: clickable thumbnail + title

Can we make the email thumbnail and name clickable to the edit view for that email?

[dkoo]

🔴 Double data fetch on initial load

Since we're now fetching the emails via REST API on view mount, we should no longer need to have this same data localized at the window.newspackSettings.emails.all object. Removing the localized data would help avoid a redundant double fetch for the initial page load.

[dkoo]

🟡 Nit: enableGlobalSearch for recipient?

enableGlobalSearch is set on name and trigger_description but not recipient. Should it be?