From 99d6de7066d4ac3b9bff4df23b5c3a849aced56c Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Thu, 7 May 2026 12:44:27 +0100 Subject: [PATCH 01/26] feat(newsletters): wire wizard SubscriptionLists to wizard-bridge events --- .../newsletters/views/settings/index.js | 164 +++++++++++++----- 1 file changed, 118 insertions(+), 46 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 1f9f2db880..1c7de43a79 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -11,10 +11,35 @@ import once from 'lodash/once'; /** * WordPress dependencies */ -import { useEffect, useState, Fragment } from '@wordpress/element'; +import { useEffect, useRef, useState, Fragment } from '@wordpress/element'; import apiFetch from '@wordpress/api-fetch'; import { sprintf, __ } from '@wordpress/i18n'; -import { CheckboxControl, TextareaControl, ExternalLink, Notice } from '@wordpress/components'; +import { + CheckboxControl, + TextareaControl, + ExternalLink, + Notice, + __experimentalHStack as HStack, // eslint-disable-line @wordpress/no-unsafe-wp-apis +} from '@wordpress/components'; + +// Wizard-bridge events. Mirror of `newspack-newsletters/src/wizard-bridge/events.js` +// — kept locally so this file is self-contained without a cross-repo import. +const NN_EVENT_NAMESPACE = 'newspack-newsletters'; +const NN_EVENTS = { + BRIDGE_MOUNTED: `${ NN_EVENT_NAMESPACE }:bridge-mounted`, + OPEN_MODAL: `${ NN_EVENT_NAMESPACE }:open-local-list-modal`, + OPEN_CONFIRM_DELETE: `${ NN_EVENT_NAMESPACE }:open-local-list-confirm-delete`, + LOCAL_LIST_SAVED: `${ NN_EVENT_NAMESPACE }:local-list-saved`, + LOCAL_LIST_DELETED: `${ NN_EVENT_NAMESPACE }:local-list-deleted`, +}; +const NN_FALLBACK_TIMEOUT_MS = 500; + +let bridgeMounted = false; +if ( typeof document !== 'undefined' ) { + document.addEventListener( NN_EVENTS.BRIDGE_MOUNTED, () => { + bridgeMounted = true; + } ); +} /** * Internal dependencies @@ -252,6 +277,8 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { const [ error, setError ] = useState( false ); const [ inFlight, setInFlight ] = useState( false ); const [ lists, setLists ] = useState( [] ); + const fallbackTimerRef = useRef( null ); + const updateConfig = data => { setLists( data ); if ( typeof onUpdate === 'function' ) { @@ -285,20 +312,53 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { newLists[ index ][ name ] = value; updateConfig( newLists ); }; - // Handle provider updates. + useEffect( () => { setError( false ); if ( provider && ! lockedLists ) { - // Empty lists before fetching to prevent previous list from appearing while fetching. setLists( [] ); fetchLists(); } }, [ provider, lockedLists ] ); + useEffect( () => { + const reload = () => fetchLists(); + document.addEventListener( NN_EVENTS.LOCAL_LIST_SAVED, reload ); + document.addEventListener( NN_EVENTS.LOCAL_LIST_DELETED, reload ); + return () => { + document.removeEventListener( NN_EVENTS.LOCAL_LIST_SAVED, reload ); + document.removeEventListener( NN_EVENTS.LOCAL_LIST_DELETED, reload ); + }; + }, [] ); + + const startFallbackTimer = fallbackUrl => { + if ( bridgeMounted || ! fallbackUrl ) { + return; + } + clearTimeout( fallbackTimerRef.current ); + fallbackTimerRef.current = setTimeout( () => { + if ( ! bridgeMounted ) { + window.location.href = fallbackUrl; + } + }, NN_FALLBACK_TIMEOUT_MS ); + }; + + const dispatchOpenAdd = () => { + document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_MODAL, { detail: { mode: 'add' } } ) ); + startFallbackTimer( newspack_newsletters_wizard.new_subscription_lists_url ); + }; + const dispatchOpenEdit = list => { + document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_MODAL, { detail: { mode: 'edit', list } } ) ); + startFallbackTimer( list?.edit_link ); + }; + const dispatchConfirmDelete = list => { + document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_CONFIRM_DELETE, { detail: { list } } ) ); + startFallbackTimer( list?.edit_link ); + }; + if ( ! inFlight && ! lists?.length && ! error ) { return null; } - if ( inFlight && ! lists?.length && ! error ) { return (
@@ -325,11 +385,7 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { actionContent={ <> { newspack_newsletters_wizard.new_subscription_lists_url && ( - ) } @@ -342,42 +398,58 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { > { ! lockedLists && ! error && - lists.map( ( list, index ) => ( - { __( 'Edit', 'newspack-plugin' ) } : null - } - > - { list.active && 'local' !== list?.type && ( - <> - - - - ) } - - ) ) } + lists.map( ( list, index ) => { + const isLocal = 'local' === list?.type; + return ( + + + + + ) : list?.edit_link ? ( + { __( 'Edit', 'newspack-plugin' ) } + ) : null + } + > + { list.active && ! isLocal && ( + <> + + + + ) } + + ); + } ) } ); }; From a6bff06b2f92b3df2a9139ed5a0087b49dd1cca7 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Thu, 7 May 2026 12:47:53 +0100 Subject: [PATCH 02/26] test(newsletters): cover wizard SubscriptionLists bridge wiring --- .../newsletters/views/settings/index.test.js | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 src/wizards/newsletters/views/settings/index.test.js diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js new file mode 100644 index 0000000000..4afc955169 --- /dev/null +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -0,0 +1,79 @@ +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import apiFetch from '@wordpress/api-fetch'; + +import { SubscriptionLists } from './index'; + +jest.mock( '@wordpress/api-fetch', () => jest.fn() ); + +const NN_EVENTS = { + BRIDGE_MOUNTED: 'newspack-newsletters:bridge-mounted', + OPEN_MODAL: 'newspack-newsletters:open-local-list-modal', + OPEN_CONFIRM_DELETE: 'newspack-newsletters:open-local-list-confirm-delete', + LOCAL_LIST_SAVED: 'newspack-newsletters:local-list-saved', + LOCAL_LIST_DELETED: 'newspack-newsletters:local-list-deleted', +}; + +beforeAll( () => { + global.newspack_newsletters_wizard = { + new_subscription_lists_url: 'https://example.test/wp-admin/post-new.php?post_type=newspack_nl_list', + }; +} ); + +beforeEach( () => { + apiFetch.mockReset(); + apiFetch.mockResolvedValue( [ + { id: 'tag-1', name: 'Local A', type: 'local', active: false, db_id: 1, edit_link: 'https://example.test/edit-local-a' }, + { id: 'group-1', name: 'Remote group', type: 'group', active: true }, + ] ); + // Pretend the bridge bundle is loaded so the fallback timer doesn't navigate the test window. + document.dispatchEvent( new CustomEvent( NN_EVENTS.BRIDGE_MOUNTED ) ); +} ); + +describe( 'SubscriptionLists — wizard-bridge wiring', () => { + it( 'dispatches OPEN_MODAL with mode=add when Add New is clicked', async () => { + const listener = jest.fn(); + document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); + render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /^Add New$/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /^Add New$/ } ) ); + expect( listener ).toHaveBeenCalled(); + expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( { mode: 'add' } ); + document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); + } ); + + it( 'dispatches OPEN_MODAL with mode=edit + list when Edit is clicked on a local row', async () => { + const listener = jest.fn(); + document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); + render( ); + await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); + fireEvent.click( screen.getAllByRole( 'button', { name: /^Edit$/ } )[ 0 ] ); + expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( + expect.objectContaining( { mode: 'edit', list: expect.objectContaining( { db_id: 1 } ) } ) + ); + document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); + } ); + + it( 'dispatches OPEN_CONFIRM_DELETE when Delete is clicked on a local row', async () => { + const listener = jest.fn(); + document.addEventListener( NN_EVENTS.OPEN_CONFIRM_DELETE, listener ); + render( ); + await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); + fireEvent.click( screen.getByRole( 'button', { name: /^Delete$/ } ) ); + expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( expect.objectContaining( { list: expect.objectContaining( { db_id: 1 } ) } ) ); + document.removeEventListener( NN_EVENTS.OPEN_CONFIRM_DELETE, listener ); + } ); + + it( 'reloads lists when LOCAL_LIST_SAVED fires', async () => { + render( ); + await waitFor( () => expect( apiFetch ).toHaveBeenCalledTimes( 1 ) ); + document.dispatchEvent( new CustomEvent( NN_EVENTS.LOCAL_LIST_SAVED, { detail: { listId: 1, mode: 'edit' } } ) ); + await waitFor( () => expect( apiFetch ).toHaveBeenCalledTimes( 2 ) ); + } ); + + it( 'reloads lists when LOCAL_LIST_DELETED fires', async () => { + render( ); + await waitFor( () => expect( apiFetch ).toHaveBeenCalledTimes( 1 ) ); + document.dispatchEvent( new CustomEvent( NN_EVENTS.LOCAL_LIST_DELETED, { detail: { listId: 1 } } ) ); + await waitFor( () => expect( apiFetch ).toHaveBeenCalledTimes( 2 ) ); + } ); +} ); From a62ce92535851d4042bf55290ff472f728a42e31 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Thu, 7 May 2026 13:35:00 +0100 Subject: [PATCH 03/26] fix(newsletters-wizard): read bridge readiness synchronously The `bridgeMounted` flag was only set via the one-shot `newspack-newsletters:bridge-mounted` event listener. If the bridge bundle booted before this module evaluated, the event was missed and the 500ms fallback timer redirected to the legacy editor after every modal-open attempt. Replace the local mutable boolean with a sync read of `window.newspackNewslettersBridgeReady`, which the bridge now sets before dispatching the event. Refs NEWS-2152 --- .../newsletters/views/settings/index.js | 15 ++++++------- .../newsletters/views/settings/index.test.js | 22 +++++++++++++++++-- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 1c7de43a79..6fb19040c1 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -34,12 +34,11 @@ const NN_EVENTS = { }; const NN_FALLBACK_TIMEOUT_MS = 500; -let bridgeMounted = false; -if ( typeof document !== 'undefined' ) { - document.addEventListener( NN_EVENTS.BRIDGE_MOUNTED, () => { - bridgeMounted = true; - } ); -} +// Read the bridge-readiness flag synchronously rather than relying on a +// one-shot `BRIDGE_MOUNTED` event. The bridge sets the flag before +// dispatching, so listeners that register late still observe a ready +// bridge — avoiding a spurious fallback redirect. +const isBridgeReady = () => typeof window !== 'undefined' && window.newspackNewslettersBridgeReady === true; /** * Internal dependencies @@ -332,12 +331,12 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { }, [] ); const startFallbackTimer = fallbackUrl => { - if ( bridgeMounted || ! fallbackUrl ) { + if ( isBridgeReady() || ! fallbackUrl ) { return; } clearTimeout( fallbackTimerRef.current ); fallbackTimerRef.current = setTimeout( () => { - if ( ! bridgeMounted ) { + if ( ! isBridgeReady() ) { window.location.href = fallbackUrl; } }, NN_FALLBACK_TIMEOUT_MS ); diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 4afc955169..5ab5dc4d44 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -25,8 +25,12 @@ beforeEach( () => { { id: 'tag-1', name: 'Local A', type: 'local', active: false, db_id: 1, edit_link: 'https://example.test/edit-local-a' }, { id: 'group-1', name: 'Remote group', type: 'group', active: true }, ] ); - // Pretend the bridge bundle is loaded so the fallback timer doesn't navigate the test window. - document.dispatchEvent( new CustomEvent( NN_EVENTS.BRIDGE_MOUNTED ) ); + // Mark the bridge ready so the fallback timer doesn't navigate the test window. + window.newspackNewslettersBridgeReady = true; +} ); + +afterEach( () => { + delete window.newspackNewslettersBridgeReady; } ); describe( 'SubscriptionLists — wizard-bridge wiring', () => { @@ -76,4 +80,18 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { document.dispatchEvent( new CustomEvent( NN_EVENTS.LOCAL_LIST_DELETED, { detail: { listId: 1 } } ) ); await waitFor( () => expect( apiFetch ).toHaveBeenCalledTimes( 2 ) ); } ); + + it( 'does not redirect when the bridge mounted before the wizard listener registered', async () => { + // The flag is already set in beforeEach, simulating the bridge having + // completed boot before this component mounted. The fallback timer + // must NOT navigate. + jest.useFakeTimers(); + const originalHref = window.location.href; + render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /^Add New$/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /^Add New$/ } ) ); + jest.advanceTimersByTime( 600 ); + expect( window.location.href ).toBe( originalHref ); + jest.useRealTimers(); + } ); } ); From 2ea09de60c3ace296a2a51e28a43d9d7fbcd0bf8 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Thu, 7 May 2026 16:08:50 +0100 Subject: [PATCH 04/26] feat(newsletters): bundled-mode parity for ESP-list edit modal (NEWS-2168) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacks the bundled-mode side onto the same wiring as NEWS-2152's local-list parity. The wizard's SubscriptionLists view dispatches OPEN_MODAL with kind='esp' for remote rows (replacing the legacy ExternalLink to the CPT editor) and the active toggle commits immediately via PATCH /lists/{db_id}. - Edit on remote rows opens the same modal in ESP mode through the wizard-bridge (legacy edit_link still serves as the fallback when the bridge isn't ready). - Inline TextControl/TextareaControl pair removed for remote rows — the modal owns title + description. - Active toggle on every row PATCHes that one row; bulk "Save Subscription Lists" button removed. - Description string surfaced under the bold ActionCard title (with the type label kept on a smaller line below) so publishers see what customisation is in place without opening the modal. --- .../newsletters/views/settings/index.js | 110 +++++++++--------- .../newsletters/views/settings/index.test.js | 47 +++++++- 2 files changed, 96 insertions(+), 61 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 6fb19040c1..df6dcec552 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -16,7 +16,6 @@ import apiFetch from '@wordpress/api-fetch'; import { sprintf, __ } from '@wordpress/i18n'; import { CheckboxControl, - TextareaControl, ExternalLink, Notice, __experimentalHStack as HStack, // eslint-disable-line @wordpress/no-unsafe-wp-apis @@ -275,6 +274,7 @@ export const Settings = ( { export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { const [ error, setError ] = useState( false ); const [ inFlight, setInFlight ] = useState( false ); + const [ togglingId, setTogglingId ] = useState( null ); const [ lists, setLists ] = useState( [] ); const fallbackTimerRef = useRef( null ); @@ -294,22 +294,27 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { .catch( setError ) .finally( () => setInFlight( false ) ); }; - const saveLists = () => { + const handleToggleActive = async ( list, next ) => { + if ( ! list?.db_id ) { + return; + } + const snapshot = lists; + updateConfig( lists.map( row => ( row.db_id === list.db_id ? { ...row, active: next } : row ) ) ); + setTogglingId( list.db_id ); setError( false ); - setInFlight( true ); - apiFetch( { - path: '/newspack-newsletters/v1/lists', - method: 'post', - data: { lists }, - } ) - .then( updateConfig ) - .catch( setError ) - .finally( () => setInFlight( false ) ); - }; - const handleChange = ( index, name ) => value => { - const newLists = [ ...lists ]; - newLists[ index ][ name ] = value; - updateConfig( newLists ); + try { + const response = await apiFetch( { + path: `/newspack-newsletters/v1/lists/${ list.db_id }`, + method: 'PATCH', + data: { active: next }, + } ); + updateConfig( lists.map( row => ( row.db_id === list.db_id ? { ...row, ...response } : row ) ) ); + } catch ( err ) { + updateConfig( snapshot ); + setError( err ); + } finally { + setTogglingId( null ); + } }; useEffect( () => { @@ -346,8 +351,8 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_MODAL, { detail: { mode: 'add' } } ) ); startFallbackTimer( newspack_newsletters_wizard.new_subscription_lists_url ); }; - const dispatchOpenEdit = list => { - document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_MODAL, { detail: { mode: 'edit', list } } ) ); + const dispatchOpenEdit = ( list, kind ) => { + document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_MODAL, { detail: { mode: 'edit', kind, list } } ) ); startFallbackTimer( list?.edit_link ); }; const dispatchConfirmDelete = list => { @@ -382,16 +387,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { notificationLevel={ error ? 'error' : 'warning' } hasGreyHeader actionContent={ - <> - { newspack_newsletters_wizard.new_subscription_lists_url && ( - - ) } - - + ) } disabled={ inFlight || lockedLists } > @@ -399,6 +399,7 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { ! error && lists.map( ( list, index ) => { const isLocal = 'local' === list?.type; + const rowDisabled = inFlight || togglingId === list?.db_id; return ( { simple hasWhiteHeader title={ list.name } - description={ list?.type_label ? list.type_label : null } - disabled={ inFlight } - toggleOnChange={ handleChange( index, 'active' ) } + description={ () => ( + <> + { list.description } + { list.description && list?.type_label &&
} + { list?.type_label && ( + { list.type_label } + ) } + + ) } + disabled={ rowDisabled } + toggleOnChange={ next => handleToggleActive( list, next ) } toggleChecked={ list.active } className={ list?.id && ( list.id.startsWith( 'group' ) || list.id.startsWith( 'tag' ) ) @@ -416,37 +425,22 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { : '' } actionText={ - isLocal ? ( - - - + { isLocal && ( + - - ) : list?.edit_link ? ( - { __( 'Edit', 'newspack-plugin' ) } - ) : null + ) } + } - > - { list.active && ! isLocal && ( - <> - - - - ) } - + /> ); } ) } diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 5ab5dc4d44..5b1c2992f9 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -23,7 +23,7 @@ beforeEach( () => { apiFetch.mockReset(); apiFetch.mockResolvedValue( [ { id: 'tag-1', name: 'Local A', type: 'local', active: false, db_id: 1, edit_link: 'https://example.test/edit-local-a' }, - { id: 'group-1', name: 'Remote group', type: 'group', active: true }, + { id: 'group-1', name: 'Remote group', type: 'group', active: true, db_id: 2, edit_link: 'https://example.test/edit-remote' }, ] ); // Mark the bridge ready so the fallback timer doesn't navigate the test window. window.newspackNewslettersBridgeReady = true; @@ -45,18 +45,59 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); } ); - it( 'dispatches OPEN_MODAL with mode=edit + list when Edit is clicked on a local row', async () => { + it( 'dispatches OPEN_MODAL with mode=edit + kind=local when Edit is clicked on a local row', async () => { const listener = jest.fn(); document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); render( ); await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); fireEvent.click( screen.getAllByRole( 'button', { name: /^Edit$/ } )[ 0 ] ); expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( - expect.objectContaining( { mode: 'edit', list: expect.objectContaining( { db_id: 1 } ) } ) + expect.objectContaining( { mode: 'edit', kind: 'local', list: expect.objectContaining( { db_id: 1 } ) } ) ); document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); } ); + it( 'dispatches OPEN_MODAL with mode=edit + kind=esp when Edit is clicked on a remote row', async () => { + const listener = jest.fn(); + document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); + render( ); + await waitFor( () => expect( screen.getByText( 'Remote group' ) ).toBeInTheDocument() ); + // Remote rows now have an Edit button too — second one in the list. + fireEvent.click( screen.getAllByRole( 'button', { name: /^Edit$/ } )[ 1 ] ); + expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( + expect.objectContaining( { mode: 'edit', kind: 'esp', list: expect.objectContaining( { db_id: 2 } ) } ) + ); + document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); + } ); + + it( 'commits the active toggle immediately via PATCH /lists/{db_id}', async () => { + render( ); + await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); + // Configure the next response (a successful PATCH echoing the row). + apiFetch.mockResolvedValueOnce( { id: 'tag-1', db_id: 1, active: true } ); + fireEvent.click( screen.getAllByRole( 'checkbox' )[ 0 ] ); + await waitFor( () => + expect( apiFetch ).toHaveBeenLastCalledWith( { + path: '/newspack-newsletters/v1/lists/1', + method: 'PATCH', + data: { active: true }, + } ) + ); + } ); + + it( 'does not render the bulk Save Subscription Lists button', async () => { + render( ); + await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); + expect( screen.queryByRole( 'button', { name: /Save Subscription Lists/ } ) ).not.toBeInTheDocument(); + } ); + + it( 'does not render inline title/description fields on remote rows', async () => { + render( ); + await waitFor( () => expect( screen.getByText( 'Remote group' ) ).toBeInTheDocument() ); + expect( screen.queryByLabelText( /List title/ ) ).not.toBeInTheDocument(); + expect( screen.queryByLabelText( /List description/ ) ).not.toBeInTheDocument(); + } ); + it( 'dispatches OPEN_CONFIRM_DELETE when Delete is clicked on a local row', async () => { const listener = jest.fn(); document.addEventListener( NN_EVENTS.OPEN_CONFIRM_DELETE, listener ); From 5f613d654cc32f3df83f068e03e40bb970247345 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Thu, 7 May 2026 17:31:56 +0100 Subject: [PATCH 05/26] chore: ignore /build output directory --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 7aff58b919..de256cf9e2 100644 --- a/.gitignore +++ b/.gitignore @@ -29,6 +29,7 @@ Thumbs.db /vendor/ # Built files +/build /dist /packages/components/colors /packages/components/dist From 55504174f3af5f8b4d32d32d15e84857cc878552 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 18 May 2026 14:22:05 +0100 Subject: [PATCH 06/26] feat(components): shared scaffolding for newsletters settings (NEWS-2263) - integration-icons namespace with 6 brand SVGs (Active Campaign, Constant Contact, Fundraise Up, Mailchimp, Salesforce, Wisepops); fills customisable via var(--integration-icon-color) / --integration-icon-color-accent with brand-colour defaults. - useUnsavedChangesDialog hook: wraps useConfirmDialog with the standard "discard changes" messaging, intercepts outbound link clicks so the dialog fires instead of the silent native one, plus a beforeunload safety net. - CardSettingsGroup gains optional className and iconSize props. - CoreCard accepts iconSize; SCSS reads it via the new --newspack-card-icon-size custom property with fallback to existing defaults so no consumer changes are required. - SectionHeader: container margin now zeroes alongside its inner element when noMargin is set (via :has() so callsites need no change). - admin.css: chrome section header only gets margin-top when it's not the first child of newspack-wizard__content, fixing the doubled spacing on audience /edit/new/all. - Audience content-gates edit adopts useUnsavedChangesDialog so the same standardised confirm flow covers both wizards. --- .../src/card-settings-group/index.tsx | 7 +- packages/components/src/card/core-card.js | 8 +- packages/components/src/card/style-core.scss | 8 +- .../src/hooks/use-unsaved-changes-dialog.tsx | 77 +++++++++++++++++++ packages/components/src/index.js | 3 + .../src/integration-icons/active-campaign.js | 14 ++++ .../src/integration-icons/constant-contact.js | 22 ++++++ .../src/integration-icons/fundraise-up.js | 22 ++++++ .../components/src/integration-icons/index.js | 6 ++ .../src/integration-icons/mailchimp.js | 10 +++ .../src/integration-icons/salesforce.js | 10 +++ .../src/integration-icons/wisepops.js | 14 ++++ .../components/src/section-header/style.scss | 3 + src/admin/style.scss | 2 +- .../views/content-gates/edit/index.tsx | 16 ++-- 15 files changed, 209 insertions(+), 13 deletions(-) create mode 100644 packages/components/src/hooks/use-unsaved-changes-dialog.tsx create mode 100644 packages/components/src/integration-icons/active-campaign.js create mode 100644 packages/components/src/integration-icons/constant-contact.js create mode 100644 packages/components/src/integration-icons/fundraise-up.js create mode 100644 packages/components/src/integration-icons/index.js create mode 100644 packages/components/src/integration-icons/mailchimp.js create mode 100644 packages/components/src/integration-icons/salesforce.js create mode 100644 packages/components/src/integration-icons/wisepops.js diff --git a/packages/components/src/card-settings-group/index.tsx b/packages/components/src/card-settings-group/index.tsx index 28bc62dbb3..6792fb9cb7 100644 --- a/packages/components/src/card-settings-group/index.tsx +++ b/packages/components/src/card-settings-group/index.tsx @@ -11,7 +11,9 @@ import './style.scss'; const CardSettingsGroup = ( { actionType = 'none', children, + className, icon = null, + iconSize, headerAction, title = '', description = '', @@ -21,7 +23,9 @@ const CardSettingsGroup = ( { }: { actionType?: 'chevron' | 'toggle' | 'button' | 'link' | 'none'; children?: React.ReactNode; + className?: string; icon?: React.ReactNode; + iconSize?: number; title: string; headerAction?: { label: string; @@ -40,7 +44,7 @@ const CardSettingsGroup = ( { } ) => { return ( ) } { icon && ( -
- +
+
) } { actions?.length > 0 && actionType === 'toggle' && ( diff --git a/packages/components/src/card/style-core.scss b/packages/components/src/card/style-core.scss index a5c48abea1..e53da30877 100644 --- a/packages/components/src/card/style-core.scss +++ b/packages/components/src/card/style-core.scss @@ -22,8 +22,8 @@ width: wp-vars.$grid-unit-60; svg { fill: var(--wp-admin-theme-color); - height: wp-vars.$grid-unit-50; - width: wp-vars.$grid-unit-50; + height: var(--newspack-card-icon-size, wp-vars.$grid-unit-50); + width: var(--newspack-card-icon-size, wp-vars.$grid-unit-50); } .newspack-card--core__has-icon-background-color & { background: var(--wp-admin-theme-color-lighter-10); @@ -48,8 +48,8 @@ height: wp-vars.$grid-unit-50; width: wp-vars.$grid-unit-50; svg { - height: wp-vars.$grid-unit-30; - width: wp-vars.$grid-unit-30; + height: var(--newspack-card-icon-size, wp-vars.$grid-unit-30); + width: var(--newspack-card-icon-size, wp-vars.$grid-unit-30); } } } diff --git a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx new file mode 100644 index 0000000000..0ffabe4dc8 --- /dev/null +++ b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx @@ -0,0 +1,77 @@ +/** + * WordPress dependencies. + */ +import { useEffect } from '@wordpress/element'; +import { __ } from '@wordpress/i18n'; + +/** + * Internal dependencies. + */ +import useConfirmDialog from './use-confirm-dialog'; + +type UseUnsavedChangesDialogOptions = { + when: boolean; +}; + +/** + * Shared unsaved-changes guard. Wraps `useConfirmDialog` with standardized + * messaging, intercepts outbound link clicks so the dialog fires instead of + * a silent navigation, and adds a `beforeunload` listener as the last-resort + * guard for refresh / tab-close (browser-native, cannot be styled). The + * returned `confirmDialog` element must be rendered in JSX. + */ +function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { + const { confirmDialog, requestConfirm } = useConfirmDialog( { + when, + message: __( 'You have unsaved changes that will be lost. Discard changes?', 'newspack-plugin' ), + confirmButtonText: __( 'Discard changes', 'newspack-plugin' ), + hideTitle: true, + } ); + + useEffect( () => { + if ( ! when ) { + return; + } + const handler = ( e: MouseEvent ) => { + if ( e.defaultPrevented || e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button !== 0 ) { + return; + } + const target = e.target as HTMLElement | null; + const link = target?.closest( 'a[href]' ) as HTMLAnchorElement | null; + if ( ! link ) { + return; + } + const href = link.getAttribute( 'href' ); + if ( ! href || href.startsWith( '#' ) || href.startsWith( 'javascript:' ) ) { + return; + } + if ( link.target && link.target !== '_self' ) { + return; + } + e.preventDefault(); + e.stopPropagation(); + const destination = link.href; + requestConfirm( () => { + window.location.href = destination; + } ); + }; + document.addEventListener( 'click', handler, true ); + return () => document.removeEventListener( 'click', handler, true ); + }, [ when, requestConfirm ] ); + + useEffect( () => { + if ( ! when ) { + return; + } + const handler = ( e: BeforeUnloadEvent ) => { + e.preventDefault(); + e.returnValue = ''; + }; + window.addEventListener( 'beforeunload', handler ); + return () => window.removeEventListener( 'beforeunload', handler ); + }, [ when ] ); + + return { confirmDialog, requestConfirm }; +} + +export default useUnsavedChangesDialog; diff --git a/packages/components/src/index.js b/packages/components/src/index.js index c175fc0f88..fe1fbe439d 100644 --- a/packages/components/src/index.js +++ b/packages/components/src/index.js @@ -51,6 +51,9 @@ export { default as withWizardScreen } from './with-wizard-screen'; export { default as Router } from './proxied-imports/router'; export { default as hooks } from './hooks'; export { default as useConfirmDialog } from './hooks/use-confirm-dialog'; +export { default as useUnsavedChangesDialog } from './hooks/use-unsaved-changes-dialog'; +import * as integrationIcons from './integration-icons'; +export { integrationIcons }; export { default as utils } from './utils'; import './style.scss'; diff --git a/packages/components/src/integration-icons/active-campaign.js b/packages/components/src/integration-icons/active-campaign.js new file mode 100644 index 0000000000..afc1de382f --- /dev/null +++ b/packages/components/src/integration-icons/active-campaign.js @@ -0,0 +1,14 @@ +const activeCampaign = ( + + + + +); + +export default activeCampaign; diff --git a/packages/components/src/integration-icons/constant-contact.js b/packages/components/src/integration-icons/constant-contact.js new file mode 100644 index 0000000000..b423c18d00 --- /dev/null +++ b/packages/components/src/integration-icons/constant-contact.js @@ -0,0 +1,22 @@ +const constantContact = ( + + + + + + +); + +export default constantContact; diff --git a/packages/components/src/integration-icons/fundraise-up.js b/packages/components/src/integration-icons/fundraise-up.js new file mode 100644 index 0000000000..8748a950a9 --- /dev/null +++ b/packages/components/src/integration-icons/fundraise-up.js @@ -0,0 +1,22 @@ +const fundraiseUp = ( + + + + + + +); + +export default fundraiseUp; diff --git a/packages/components/src/integration-icons/index.js b/packages/components/src/integration-icons/index.js new file mode 100644 index 0000000000..8865ef8924 --- /dev/null +++ b/packages/components/src/integration-icons/index.js @@ -0,0 +1,6 @@ +export { default as activeCampaign } from './active-campaign'; +export { default as constantContact } from './constant-contact'; +export { default as fundraiseUp } from './fundraise-up'; +export { default as mailchimp } from './mailchimp'; +export { default as salesforce } from './salesforce'; +export { default as wisepops } from './wisepops'; diff --git a/packages/components/src/integration-icons/mailchimp.js b/packages/components/src/integration-icons/mailchimp.js new file mode 100644 index 0000000000..f51d98a5bf --- /dev/null +++ b/packages/components/src/integration-icons/mailchimp.js @@ -0,0 +1,10 @@ +const mailchimp = ( + + + +); + +export default mailchimp; diff --git a/packages/components/src/integration-icons/salesforce.js b/packages/components/src/integration-icons/salesforce.js new file mode 100644 index 0000000000..0523e84d0c --- /dev/null +++ b/packages/components/src/integration-icons/salesforce.js @@ -0,0 +1,10 @@ +const salesforce = ( + + + +); + +export default salesforce; diff --git a/packages/components/src/integration-icons/wisepops.js b/packages/components/src/integration-icons/wisepops.js new file mode 100644 index 0000000000..8e867d0684 --- /dev/null +++ b/packages/components/src/integration-icons/wisepops.js @@ -0,0 +1,14 @@ +const wisepops = ( + + + + +); + +export default wisepops; diff --git a/packages/components/src/section-header/style.scss b/packages/components/src/section-header/style.scss index 09b4f13777..8325c363bb 100644 --- a/packages/components/src/section-header/style.scss +++ b/packages/components/src/section-header/style.scss @@ -15,6 +15,9 @@ &:not(:first-child) { margin-top: 48px; } + &:has(> .newspack-section-header--no-margin) { + margin-top: 0; + } &.newspack-section-header--has-primary-action { display: flex; flex-direction: column; diff --git a/src/admin/style.scss b/src/admin/style.scss index 9844283e71..eb14dac689 100644 --- a/src/admin/style.scss +++ b/src/admin/style.scss @@ -199,7 +199,7 @@ h1 { } } -.newspack-wizard__content > .newspack-wizard__section-header { +.newspack-wizard__content > .newspack-wizard__section-header:not(:first-child) { margin-top: 48px; } diff --git a/src/wizards/audience/views/content-gates/edit/index.tsx b/src/wizards/audience/views/content-gates/edit/index.tsx index 3282d0d14e..d952809a8b 100644 --- a/src/wizards/audience/views/content-gates/edit/index.tsx +++ b/src/wizards/audience/views/content-gates/edit/index.tsx @@ -15,7 +15,16 @@ import { commentAuthorAvatar, currencyDollar, envelope, pencil, postList, settin * Internal dependencies */ import { AUDIENCE_CONTENT_GATES_WIZARD_SLUG } from '../consts'; -import { CardSettingsGroup, Divider, Grid, Router, SectionHeader, TextControl, useConfirmDialog } from '../../../../../../packages/components/src'; +import { + CardSettingsGroup, + Divider, + Grid, + Router, + SectionHeader, + TextControl, + useConfirmDialog, + useUnsavedChangesDialog, +} from '../../../../../../packages/components/src'; import { WIZARD_STORE_NAMESPACE } from '../../../../../../packages/components/src/wizard/store'; import { useWizardData } from '../../../../../../packages/components/src/wizard/store/utils'; import { useWizardApiFetch } from '../../../../hooks/use-wizard-api-fetch'; @@ -101,11 +110,8 @@ const Edit = ( { match, updateGatesData, slug = AUDIENCE_CONTENT_GATES_WIZARD_SL JSON.stringify( registration ) !== JSON.stringify( gate.registration ) || JSON.stringify( customAccess ) !== JSON.stringify( gate.custom_access ); - const { confirmDialog: navBlockDialog } = useConfirmDialog( { + const { confirmDialog: navBlockDialog } = useUnsavedChangesDialog( { when: isDirty && ! isSaving.current, - message: __( 'You have unsaved changes that will be lost. Discard changes?', 'newspack-plugin' ), - confirmButtonText: __( 'Discard changes', 'newspack-plugin' ), - hideTitle: true, } ); const { confirmDialog: deleteDialog, requestConfirm: requestDelete } = useConfirmDialog( { title: __( 'Are you sure?', 'newspack-plugin' ), From d9677b988a98d397e28b0b0936158496d1617f25 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 18 May 2026 14:25:52 +0100 Subject: [PATCH 07/26] feat(newsletters): rebuild Settings page (NEWS-2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Single-section wizard with sticky header, dirty-state tracking, and unsaved-changes guard via useUnsavedChangesDialog. Header chrome shows "Newsletters / Settings" with the Save button disabled until something is actually dirty. Layout (top to bottom, two-column rows with section header on the left): - Email service provider — 2x2 grid of brand-icon cards (Active Campaign, Mailchimp, Constant Contact, Manual / Other). Selecting a card sets the provider and reveals its credentials below the grid. Constant Contact shows its OAuth Authorize prompt inline when needed. - Newsletter posts — slug, comments, related-posts as ToggleControls. - Subscription lists — per-row ToggleControl with Local/ESP badge, list description as help text, dividers between rows, Edit/Delete on the right (Delete only for local lists). Below the list, a paragraph and secondary "Add new local list" button explain provider-specific syncing (label resolved per-ESP via the wizard's settings response). - Ads tracking — merged in from the previous separate tab; ToggleControls for click + impressions. The Tracking tab on the ads-list admin header is removed since the section now lives here. - Letterhead — separate section at the bottom for the Letterhead API key + help link. PHP: wizard /settings response now includes the active provider's labels (get_labels('local_list_explanation')), guarded with class_exists for standalone-newspack-plugin installs. Test selectors updated for the new card-based Add new flow. --- .../newsletters/class-newsletters-wizard.php | 15 +- src/wizards/newsletters/index.js | 58 +- .../newsletters/views/settings/index.js | 546 +++++++++++++----- .../newsletters/views/settings/index.test.js | 8 +- .../newsletters/views/settings/style.scss | 35 +- .../newsletters/views/tracking/index.js | 51 +- 6 files changed, 492 insertions(+), 221 deletions(-) diff --git a/includes/wizards/newsletters/class-newsletters-wizard.php b/includes/wizards/newsletters/class-newsletters-wizard.php index 4f9cea6f1c..c455570f65 100644 --- a/includes/wizards/newsletters/class-newsletters-wizard.php +++ b/includes/wizards/newsletters/class-newsletters-wizard.php @@ -227,9 +227,19 @@ function ( $acc, $value ) { }, [] ); + + $labels = []; + if ( class_exists( 'Newspack_Newsletters' ) ) { + $provider = Newspack_Newsletters::get_service_provider(); + if ( $provider && method_exists( $provider, 'get_labels' ) ) { + $labels = $provider::get_labels( 'local_list_explanation' ); + } + } + return [ 'configured' => $newsletters_configuration_manager->is_configured(), 'settings' => $settings, + 'labels' => $labels, ]; } @@ -424,11 +434,6 @@ private function get_tabs() { ]; } - $tabs[] = [ - 'textContent' => esc_html__( 'Tracking', 'newspack-plugin' ), - 'href' => admin_url( 'edit.php?post_type=' . Newspack_Newsletters::NEWSPACK_NEWSLETTERS_CPT . '&page=' . $this->slug . '#/tracking' ), - ]; - return $tabs; } diff --git a/src/wizards/newsletters/index.js b/src/wizards/newsletters/index.js index 403fbddaf3..2c9ae28894 100644 --- a/src/wizards/newsletters/index.js +++ b/src/wizards/newsletters/index.js @@ -1,59 +1,33 @@ import '../../shared/js/public-path'; /** - * Advertising + * Newsletters wizard entry. */ /** * WordPress dependencies. */ -import { Component, render, Fragment, createElement } from '@wordpress/element'; +import { render } from '@wordpress/element'; import { __ } from '@wordpress/i18n'; /** * Internal dependencies. */ -import { withWizard } from '../../../packages/components/src'; -import Router from '../../../packages/components/src/proxied-imports/router'; -import { Settings, Tracking } from './views'; +import { Wizard } from '../../../packages/components/src'; +import NewslettersSettings from './views/settings'; -const { HashRouter, Redirect, Route, Switch } = Router; - -class NewslettersWizard extends Component { - /** - * Render - */ - render() { - const { pluginRequirements } = this.props; - const tabs = [ +const NewslettersWizard = () => ( + - - - { pluginRequirements } - } - /> - } - /> - - - - - ); - } -} -render( createElement( withWizard( NewslettersWizard, [ 'newspack-newsletters' ] ) ), document.getElementById( 'newspack-newsletters' ) ); + ] } + /> +); + +render( , document.getElementById( 'newspack-newsletters' ) ); diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index df6dcec552..4b76342a2b 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -1,6 +1,6 @@ /* global newspack_newsletters_wizard */ /** - * Internal dependencies + * External dependencies */ import values from 'lodash/values'; import mapValues from 'lodash/mapValues'; @@ -11,15 +11,18 @@ import once from 'lodash/once'; /** * WordPress dependencies */ -import { useEffect, useRef, useState, Fragment } from '@wordpress/element'; +import { Fragment, useEffect, useRef, useState } from '@wordpress/element'; +import { useDispatch } from '@wordpress/data'; import apiFetch from '@wordpress/api-fetch'; import { sprintf, __ } from '@wordpress/i18n'; import { - CheckboxControl, ExternalLink, - Notice, + Notice as WpNotice, + ToggleControl, __experimentalHStack as HStack, // eslint-disable-line @wordpress/no-unsafe-wp-apis + __experimentalVStack as VStack, // eslint-disable-line @wordpress/no-unsafe-wp-apis } from '@wordpress/components'; +import { atSymbol } from '@wordpress/icons'; // Wizard-bridge events. Mirror of `newspack-newsletters/src/wizard-bridge/events.js` // — kept locally so this file is self-contained without a cross-repo import. @@ -43,22 +46,32 @@ const isBridgeReady = () => typeof window !== 'undefined' && window.newspackNews * Internal dependencies */ import { + Badge, Button, Card, - ActionCard, + CardSettingsGroup, + Divider, Grid, PluginInstaller, + SectionHeader, SelectControl, TextControl, Waiting, hooks, - withWizardScreen, + integrationIcons, + useUnsavedChangesDialog, } from '../../../../../packages/components/src'; +import { WIZARD_STORE_NAMESPACE } from '../../../../../packages/components/src/wizard/store'; +import Tracking from '../tracking'; import './style.scss'; +const LETTERHEAD_KEY = 'newspack_newsletters_letterhead_api_key'; + export const Settings = ( { onUpdate, + onLabels, + onLetterheadSetting, newslettersConfig, isOnboarding = true, authUrl = false, @@ -126,7 +139,15 @@ export const Settings = ( { apiFetch( { path: '/newspack/v1/wizard/newspack-newsletters/settings', } ) - .then( performConfigUpdate ) + .then( response => { + performConfigUpdate( response ); + if ( onLabels && response?.labels ) { + onLabels( response.labels ); + } + if ( onLetterheadSetting && response?.settings?.[ LETTERHEAD_KEY ] ) { + onLetterheadSetting( response.settings[ LETTERHEAD_KEY ] ); + } + } ) .catch( setError ); }; const getSelectedProviderName = () => { @@ -182,73 +203,147 @@ export const Settings = ( { onChange: value => performConfigUpdate( { settings: { [ key ]: { value } } } ), } ); - const renderProviderSettings = () => { - const providerSelectProps = getSettingProps( 'newspack_newsletters_service_provider' ); - return ( - - { __( 'Save Settings', 'newspack-plugin' ) } - - } - disabled={ inFlight } - > - - { false !== authUrl && ( - -

{ __( 'Authorize Application', 'newspack-plugin' ) }

-

- { sprintf( - // translators: %s is the name of the ESP. - __( 'Authorize %s to connect to Newspack.', 'newspack-plugin' ), - getSelectedProviderName() - ) } + const providerSelectProps = config.settings ? getSettingProps( 'newspack_newsletters_service_provider' ) : null; + + const ESP_PROVIDER_KEY = 'newspack_newsletters_service_provider'; + + const isESPSetting = setting => setting.key === ESP_PROVIDER_KEY || !! setting.provider; + const isPostSetting = setting => ! setting.provider && setting.key !== ESP_PROVIDER_KEY && setting.key !== LETTERHEAD_KEY; + + const renderSettingControl = setting => { + if ( isOnboarding && ! setting.onboarding ) { + return null; + } + switch ( setting.type ) { + case 'select': + return ; + case 'checkbox': { + const props = getSettingProps( setting.key ); + return ( + + ); + } + default: + return ( + + + { setting.help && setting.helpURL && ( +

+ { setting.help }

- - - ) } - { 'campaign_monitor' === config?.settings?.newspack_newsletters_service_provider?.value && ( - -

{ __( 'Campaign Monitor support will be deprecated', 'newspack-plugin' ) }

-

{ __( 'Please connect a different service provider to ensure continued support.', 'newspack-' ) }

- + ) } + + ); + } + }; + + const espSettings = values( config.settings ).filter( + setting => isESPSetting( setting ) && ( ! setting.provider || setting.provider === providerSelectProps?.value ) + ); + const postSettings = values( config.settings ).filter( isPostSetting ); + + const PROVIDER_ORDER = [ 'active_campaign', 'mailchimp', 'constant_contact', 'manual' ]; + const PROVIDER_ICONS = { + active_campaign: integrationIcons.activeCampaign, + mailchimp: integrationIcons.mailchimp, + constant_contact: integrationIcons.constantContact, + manual: atSymbol, + }; + const PROVIDER_ICON_SIZES = { + active_campaign: 16, + constant_contact: 18, + }; + const providerOptions = ( config.settings?.newspack_newsletters_service_provider?.options || [] ) + .filter( opt => opt.value !== '' ) + .sort( ( a, b ) => { + const aIdx = PROVIDER_ORDER.indexOf( a.value ); + const bIdx = PROVIDER_ORDER.indexOf( b.value ); + if ( aIdx === -1 && bIdx === -1 ) { + return 0; + } + if ( aIdx === -1 ) { + return 1; + } + if ( bIdx === -1 ) { + return -1; + } + return aIdx - bIdx; + } ); + const selectedProviderValue = providerSelectProps?.value; + + const renderAuthorizeBlock = () => + false !== authUrl && ( + +

{ __( 'Authorize Application', 'newspack-plugin' ) }

+

+ { sprintf( + // translators: %s is the name of the ESP. + __( 'Authorize %s to connect to Newspack.', 'newspack-plugin' ), + getSelectedProviderName() ) } - { values( config.settings ) - .filter( setting => ! setting.provider || setting.provider === providerSelectProps.value ) - .map( setting => { - if ( isOnboarding && ! setting.onboarding ) { - return null; - } - switch ( setting.type ) { - case 'select': - return ; - case 'checkbox': - return ; - default: - return ( - - - { setting.help && setting.helpURL && ( -

- { setting.help } -

- ) } - - ); - } - } ) } - - +

+ + ); - }; + + const renderProviderControls = () => ( + + { error && ( + + { error?.message || __( 'Something went wrong.', 'newspack-plugin' ) } + + ) } + { 'campaign_monitor' === selectedProviderValue && ( + +

{ __( 'Campaign Monitor support will be deprecated', 'newspack-plugin' ) }

+

{ __( 'Please connect a different service provider to ensure continued support.', 'newspack-plugin' ) }

+ + ) } + { isOnboarding ? ( + values( config.settings ).map( renderSettingControl ) + ) : ( + <> + + { providerOptions.map( option => ( + providerSelectProps.onChange( option.value ) } + onHeaderClick={ () => providerSelectProps.onChange( option.value ) } + /> + ) ) } + + { selectedProviderValue && ( + + { selectedProviderValue === 'constant_contact' && renderAuthorizeBlock() } + { espSettings.filter( s => s.provider === selectedProviderValue ).map( renderSettingControl ) } + + ) } + + ) } + { isOnboarding && ( + + + + ) } + + ); + if ( ! error && isEmpty( config ) ) { return (
@@ -266,12 +361,46 @@ export const Settings = ( { onStatus={ ( { complete } ) => complete && fetchConfiguration() } /> ) } - { config.configured === true && renderProviderSettings() } + { config.configured === true && + ( isOnboarding ? ( + renderProviderControls() + ) : ( + <> + + + { renderProviderControls() } + + { postSettings.length > 0 && ( + <> + + + + + { postSettings.map( renderSettingControl ) } + + + + ) } + + ) ) } ); }; -export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { +export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = {} } ) => { const [ error, setError ] = useState( false ); const [ inFlight, setInFlight ] = useState( false ); const [ togglingId, setTogglingId ] = useState( null ); @@ -363,87 +492,111 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider } ) => { if ( ! inFlight && ! lists?.length && ! error ) { return null; } - if ( inFlight && ! lists?.length && ! error ) { - return ( -
- -
- ); - } - /* eslint-disable no-nested-ternary */ - const notification = lockedLists - ? __( 'Please save your ESP settings before changing your subscription lists.', 'newspack-plugin' ) - : error - ? error?.message || __( 'Something went wrong.', 'newspack-plugin' ) - : null; + const showAddNew = !! newspack_newsletters_wizard.new_subscription_lists_url; return ( - - { __( 'Add New', 'newspack-plugin' ) } - - ) - } - disabled={ inFlight || lockedLists } - > - { ! lockedLists && - ! error && - lists.map( ( list, index ) => { - const isLocal = 'local' === list?.type; - const rowDisabled = inFlight || togglingId === list?.db_id; - return ( - ( - <> - { list.description } - { list.description && list?.type_label &&
} - { list?.type_label && ( - { list.type_label } - ) } - - ) } - disabled={ rowDisabled } - toggleOnChange={ next => handleToggleActive( list, next ) } - toggleChecked={ list.active } - className={ - list?.id && ( list.id.startsWith( 'group' ) || list.id.startsWith( 'tag' ) ) - ? 'newspack-newsletters-sub-list-item' - : '' - } - actionText={ - - + { isLocal && ( + + ) } + + + + ); + } ) } + { ! lockedLists && ! error && showAddNew && ( + <> + + +

+ { labels?.local_list_explanation + ? sprintf( + // translators: %s is the provider-specific local list label, e.g. "Mailchimp Group" or "Active Campaign Tag". + __( 'Local lists are managed in WordPress and synced to your ESP as: %s.', 'newspack-plugin' ), + labels.local_list_explanation + ) + : __( 'Local lists are managed in WordPress and synced to an entity in your ESP.', 'newspack-plugin' ) } +

+ + - { isLocal && ( - - ) } - } - /> - ); - } ) } - + + + ) } + + + ); }; @@ -452,13 +605,75 @@ const NewslettersSettings = () => { const [ provider, setProvider ] = useState( '' ); const [ lockedLists, setLockedLists ] = useState( false ); const [ authUrl, setAuthUrl ] = useState( false ); + const [ inFlight, setInFlight ] = useState( false ); + const [ error, setError ] = useState( false ); + const [ savedConfig, setSavedConfig ] = useState( null ); + const [ labels, setLabels ] = useState( {} ); + const [ letterheadSetting, setLetterheadSetting ] = useState( null ); + const { setHeaderData } = useDispatch( WIZARD_STORE_NAMESPACE ); + + useEffect( () => { + if ( savedConfig === null && newslettersConfig && Object.keys( newslettersConfig ).length > 0 ) { + setSavedConfig( newslettersConfig ); + } + }, [ newslettersConfig, savedConfig ] ); + + const isDirty = savedConfig !== null && JSON.stringify( newslettersConfig ) !== JSON.stringify( savedConfig ); + + const saveSettings = async () => { + setError( false ); + setInFlight( true ); + try { + const response = await apiFetch( { + path: '/newspack/v1/wizard/newspack-newsletters/settings', + method: 'POST', + data: newslettersConfig, + } ); + setProvider( newslettersConfig?.newspack_newsletters_service_provider ); + setLockedLists( false ); + setSavedConfig( newslettersConfig ); + if ( response?.labels ) { + setLabels( response.labels ); + } + } catch ( err ) { + setError( err ); + } finally { + setInFlight( false ); + } + }; + + useEffect( () => { + setHeaderData( { + sectionName: __( 'Settings', 'newspack-plugin' ), + sectionTitle: __( 'Settings', 'newspack-plugin' ), + actions: [ + { + type: 'primary', + label: __( 'Save', 'newspack-plugin' ), + action: saveSettings, + disabled: inFlight || ! isDirty, + }, + ], + } ); + }, [ inFlight, isDirty, newslettersConfig ] ); + + const { confirmDialog: navBlockDialog } = useUnsavedChangesDialog( { + when: isDirty && ! inFlight, + } ); return ( <> -

{ __( 'Settings', 'newspack-plugin' ) }

+ { navBlockDialog } + { error && ( + + { error?.message || __( 'Something went wrong.', 'newspack-plugin' ) } + + ) } updateConfiguration( { newslettersConfig: config } ) } + onLabels={ setLabels } + onLetterheadSetting={ setLetterheadSetting } authUrl={ authUrl } newslettersConfig={ newslettersConfig } provider={ provider } @@ -466,9 +681,38 @@ const NewslettersSettings = () => { setAuthUrl={ setAuthUrl } setLockedLists={ setLockedLists } /> - + + + { letterheadSetting && ( + <> + + + + + + updateConfiguration( { newslettersConfig: { [ letterheadSetting.key ]: value } } ) } + withMargin={ false } + /> + { letterheadSetting.help && letterheadSetting.helpURL && ( +

+ { letterheadSetting.help } +

+ ) } + + + + + ) } ); }; -export default withWizardScreen( () => ); +export default NewslettersSettings; diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 5b1c2992f9..6079acdc48 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -38,8 +38,8 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { const listener = jest.fn(); document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); render( ); - await waitFor( () => expect( screen.getByRole( 'button', { name: /^Add New$/ } ) ).toBeEnabled() ); - fireEvent.click( screen.getByRole( 'button', { name: /^Add New$/ } ) ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); expect( listener ).toHaveBeenCalled(); expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( { mode: 'add' } ); document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); @@ -129,8 +129,8 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { jest.useFakeTimers(); const originalHref = window.location.href; render( ); - await waitFor( () => expect( screen.getByRole( 'button', { name: /^Add New$/ } ) ).toBeEnabled() ); - fireEvent.click( screen.getByRole( 'button', { name: /^Add New$/ } ) ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); jest.advanceTimersByTime( 600 ); expect( window.location.href ).toBe( originalHref ); jest.useRealTimers(); diff --git a/src/wizards/newsletters/views/settings/style.scss b/src/wizards/newsletters/views/settings/style.scss index 13d450df8b..85f58c18ed 100644 --- a/src/wizards/newsletters/views/settings/style.scss +++ b/src/wizards/newsletters/views/settings/style.scss @@ -1,3 +1,36 @@ +@use "~@wordpress/base-styles/colors" as wp-colors; +@use "~@wordpress/base-styles/variables" as wp-vars; + .newspack-newsletters-sub-list-item { - margin-left: 40px !important; + padding-inline-start: wp-vars.$grid-unit-50; +} + +.newspack-newsletters-settings-stack { + .components-base-control { + margin-bottom: 0; + } +} + +.newspack-newsletters-list-item__content { + min-width: 0; +} + +.newspack-newsletters-list-item__badge { + margin-inline-start: wp-vars.$grid-unit-50; +} + + +.newspack-newsletters-esp-card { + &--active-campaign { + --integration-icon-color: #{wp-colors.$white}; + .newspack-card--core__icon { + background: #004CFF; + } + } + &--mailchimp .newspack-card--core__icon { + background: #FFE01B; + } + &--constant-contact .newspack-card--core__icon { + background: wp-colors.$gray-100; + } } diff --git a/src/wizards/newsletters/views/tracking/index.js b/src/wizards/newsletters/views/tracking/index.js index da6ff87413..71f57852ed 100644 --- a/src/wizards/newsletters/views/tracking/index.js +++ b/src/wizards/newsletters/views/tracking/index.js @@ -3,16 +3,17 @@ */ import { __ } from '@wordpress/i18n'; import { useEffect, useState } from '@wordpress/element'; +import { ToggleControl, __experimentalVStack as VStack } from '@wordpress/components'; // eslint-disable-line @wordpress/no-unsafe-wp-apis import apiFetch from '@wordpress/api-fetch'; /** * Internal dependencies */ -import { ActionCard, withWizardScreen } from '../../../../../packages/components/src'; +import { Divider, Grid, SectionHeader } from '../../../../../packages/components/src'; const apiPath = '/newspack/v1/wizard/newspack-newsletters/settings/tracking'; -export default withWizardScreen( () => { +const Tracking = () => { const [ inFlight, setInFlight ] = useState( false ); const [ tracking, setTracking ] = useState( {} ); @@ -52,21 +53,35 @@ export default withWizardScreen( () => { return ( <> -

{ __( 'Ads Tracking', 'newspack-plugin' ) }

- - + + + + + + + + ); -} ); +}; + +export default Tracking; From 4b1d7848abe3faa19755a3f7770cdc85951297b8 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 18 May 2026 15:32:02 +0100 Subject: [PATCH 08/26] fix(newsletters): address PR review feedback (NEWS-2263) - useUnsavedChangesDialog: track confirmed navigations via a ref so the beforeunload guard skips the native prompt after the user confirms via our custom dialog. - SubscriptionLists: per-row toggles use functional setState and a Set of in-flight db_ids so two quick toggles can't clobber each other if responses resolve out of order; rollback reverts the specific row to its previous active value rather than restoring a whole-list snapshot. - SubscriptionLists: render the locked-state warning even when no list data has loaded yet, so a provider change that sets lockedLists before a successful fetch still explains itself. - Settings (onboarding path): restore the provider filter so unselected ESPs' credential fields don't appear in the setup Services card. --- .../src/hooks/use-unsaved-changes-dialog.tsx | 10 +++- .../newsletters/views/settings/index.js | 48 ++++++++++++------- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx index 0ffabe4dc8..df5d58c04c 100644 --- a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx +++ b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx @@ -1,7 +1,7 @@ /** * WordPress dependencies. */ -import { useEffect } from '@wordpress/element'; +import { useEffect, useRef } from '@wordpress/element'; import { __ } from '@wordpress/i18n'; /** @@ -28,6 +28,10 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { hideTitle: true, } ); + // Tracks navigation the user has already approved via our custom dialog so + // the beforeunload guard doesn't fire a second native prompt on top of it. + const isNavigatingRef = useRef( false ); + useEffect( () => { if ( ! when ) { return; @@ -52,6 +56,7 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { e.stopPropagation(); const destination = link.href; requestConfirm( () => { + isNavigatingRef.current = true; window.location.href = destination; } ); }; @@ -64,6 +69,9 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { return; } const handler = ( e: BeforeUnloadEvent ) => { + if ( isNavigatingRef.current ) { + return; + } e.preventDefault(); e.returnValue = ''; }; diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 4b76342a2b..1722a48206 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -309,7 +309,9 @@ export const Settings = ( { ) } { isOnboarding ? ( - values( config.settings ).map( renderSettingControl ) + values( config.settings ) + .filter( setting => ! setting.provider || setting.provider === selectedProviderValue ) + .map( renderSettingControl ) ) : ( <> @@ -403,15 +405,18 @@ export const Settings = ( { export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = {} } ) => { const [ error, setError ] = useState( false ); const [ inFlight, setInFlight ] = useState( false ); - const [ togglingId, setTogglingId ] = useState( null ); + const [ togglingIds, setTogglingIds ] = useState( () => new Set() ); const [ lists, setLists ] = useState( [] ); const fallbackTimerRef = useRef( null ); - const updateConfig = data => { - setLists( data ); - if ( typeof onUpdate === 'function' ) { - onUpdate( data ); - } + const updateLists = updater => { + setLists( prev => { + const nextLists = typeof updater === 'function' ? updater( prev ) : updater; + if ( typeof onUpdate === 'function' ) { + onUpdate( nextLists ); + } + return nextLists; + } ); }; const fetchLists = () => { setError( false ); @@ -419,7 +424,7 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { apiFetch( { path: '/newspack-newsletters/v1/lists', } ) - .then( updateConfig ) + .then( updateLists ) .catch( setError ) .finally( () => setInFlight( false ) ); }; @@ -427,22 +432,31 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { if ( ! list?.db_id ) { return; } - const snapshot = lists; - updateConfig( lists.map( row => ( row.db_id === list.db_id ? { ...row, active: next } : row ) ) ); - setTogglingId( list.db_id ); + const dbId = list.db_id; + const previousActive = list.active; + updateLists( prev => prev.map( row => ( row.db_id === dbId ? { ...row, active: next } : row ) ) ); + setTogglingIds( prev => { + const updated = new Set( prev ); + updated.add( dbId ); + return updated; + } ); setError( false ); try { const response = await apiFetch( { - path: `/newspack-newsletters/v1/lists/${ list.db_id }`, + path: `/newspack-newsletters/v1/lists/${ dbId }`, method: 'PATCH', data: { active: next }, } ); - updateConfig( lists.map( row => ( row.db_id === list.db_id ? { ...row, ...response } : row ) ) ); + updateLists( prev => prev.map( row => ( row.db_id === dbId ? { ...row, ...response } : row ) ) ); } catch ( err ) { - updateConfig( snapshot ); + updateLists( prev => prev.map( row => ( row.db_id === dbId ? { ...row, active: previousActive } : row ) ) ); setError( err ); } finally { - setTogglingId( null ); + setTogglingIds( prev => { + const updated = new Set( prev ); + updated.delete( dbId ); + return updated; + } ); } }; @@ -489,7 +503,7 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { startFallbackTimer( list?.edit_link ); }; - if ( ! inFlight && ! lists?.length && ! error ) { + if ( ! inFlight && ! lists?.length && ! error && ! lockedLists ) { return null; } @@ -525,7 +539,7 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { ! error && lists.map( ( list, index ) => { const isLocal = 'local' === list?.type; - const rowDisabled = inFlight || togglingId === list?.db_id; + const rowDisabled = inFlight || togglingIds.has( list?.db_id ); const isSubList = list?.id && ( list.id.startsWith( 'group' ) || list.id.startsWith( 'tag' ) ); return ( From a7e1950362fdac90f2def94436ccff1d89bad570 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 18 May 2026 15:41:19 +0100 Subject: [PATCH 09/26] fix(newsletters): expose labels response with explicit key (NEWS-2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace `Provider::get_labels( 'local_list_explanation' )` (where the arg is a context hint, not a key selector — easy to misread) with a direct `Provider::label( 'local_list_explanation' )` call assigned to an explicit `local_list_explanation` key. Same runtime payload, but the intent is now obvious at the call site and the response surfaces only what the client actually consumes. --- includes/wizards/newsletters/class-newsletters-wizard.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/includes/wizards/newsletters/class-newsletters-wizard.php b/includes/wizards/newsletters/class-newsletters-wizard.php index c455570f65..ce60177ca7 100644 --- a/includes/wizards/newsletters/class-newsletters-wizard.php +++ b/includes/wizards/newsletters/class-newsletters-wizard.php @@ -231,8 +231,10 @@ function ( $acc, $value ) { $labels = []; if ( class_exists( 'Newspack_Newsletters' ) ) { $provider = Newspack_Newsletters::get_service_provider(); - if ( $provider && method_exists( $provider, 'get_labels' ) ) { - $labels = $provider::get_labels( 'local_list_explanation' ); + if ( $provider && method_exists( $provider, 'label' ) ) { + $labels = [ + 'local_list_explanation' => $provider::label( 'local_list_explanation' ), + ]; } } From c84e066d1ffc443b0b7ed367bf75454ac98a63da Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 18 May 2026 16:27:00 +0100 Subject: [PATCH 10/26] fix(newsletters): a11y labels on per-row Edit/Delete (NEWS-2263) --- src/wizards/newsletters/views/settings/index.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 1722a48206..7c268a2be0 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -569,6 +569,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { variant="link" onClick={ () => dispatchOpenEdit( list, isLocal ? 'local' : 'esp' ) } disabled={ rowDisabled } + aria-label={ sprintf( + // translators: %s is the list name. + __( 'Edit %s', 'newspack-plugin' ), + list.name + ) } > { __( 'Edit', 'newspack-plugin' ) } @@ -578,6 +583,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { isDestructive onClick={ () => dispatchConfirmDelete( list ) } disabled={ rowDisabled } + aria-label={ sprintf( + // translators: %s is the list name. + __( 'Delete %s', 'newspack-plugin' ), + list.name + ) } > { __( 'Delete', 'newspack-plugin' ) } From 952e3c45771cb2873038d0374e68867cca516ddc Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 13:53:03 +0100 Subject: [PATCH 11/26] fix(newsletters): address second-pass review feedback (NEWS-2263) --- .../newsletters/class-newsletters-wizard.php | 3 + .../src/card-settings-group/index.tsx | 3 - packages/components/src/card/core-card.js | 8 +- packages/components/src/card/style-core.scss | 8 +- .../src/hooks/use-unsaved-changes-dialog.tsx | 49 ++++- .../src/integration-icons/active-campaign.js | 10 +- .../src/integration-icons/constant-contact.js | 18 +- .../src/integration-icons/fundraise-up.js | 18 +- .../src/integration-icons/mailchimp.js | 6 +- src/wizards/newsletters/views/index.js | 1 - .../newsletters/views/settings/index.js | 78 ++++--- .../newsletters/views/settings/index.test.js | 200 ++++++++++++++++-- .../newsletters/views/settings/style.scss | 7 +- .../index.js => settings/tracking.js} | 0 14 files changed, 319 insertions(+), 90 deletions(-) rename src/wizards/newsletters/views/{tracking/index.js => settings/tracking.js} (100%) diff --git a/includes/wizards/newsletters/class-newsletters-wizard.php b/includes/wizards/newsletters/class-newsletters-wizard.php index ce60177ca7..33425206d7 100644 --- a/includes/wizards/newsletters/class-newsletters-wizard.php +++ b/includes/wizards/newsletters/class-newsletters-wizard.php @@ -232,6 +232,9 @@ function ( $acc, $value ) { if ( class_exists( 'Newspack_Newsletters' ) ) { $provider = Newspack_Newsletters::get_service_provider(); if ( $provider && method_exists( $provider, 'label' ) ) { + // `label()` accepts a second `$context` arg (usually a list public id). + // Intentionally omitted here: this is the generic explanation copy + // shown above the "Add new local list" button, not a per-list label. $labels = [ 'local_list_explanation' => $provider::label( 'local_list_explanation' ), ]; diff --git a/packages/components/src/card-settings-group/index.tsx b/packages/components/src/card-settings-group/index.tsx index 6792fb9cb7..c82f4963ca 100644 --- a/packages/components/src/card-settings-group/index.tsx +++ b/packages/components/src/card-settings-group/index.tsx @@ -13,7 +13,6 @@ const CardSettingsGroup = ( { children, className, icon = null, - iconSize, headerAction, title = '', description = '', @@ -25,7 +24,6 @@ const CardSettingsGroup = ( { children?: React.ReactNode; className?: string; icon?: React.ReactNode; - iconSize?: number; title: string; headerAction?: { label: string; @@ -60,7 +58,6 @@ const CardSettingsGroup = ( { onToggle: onEnable, icon, iconBackgroundColor: true, - iconSize, isActive, title, } } diff --git a/packages/components/src/card/core-card.js b/packages/components/src/card/core-card.js index f61d858ceb..7eea8b07a4 100644 --- a/packages/components/src/card/core-card.js +++ b/packages/components/src/card/core-card.js @@ -34,7 +34,6 @@ const CoreCard = ( { footerStyle, icon, iconBackgroundColor, - iconSize, isActive, isDraggable, isFirstTarget, @@ -118,11 +117,8 @@ const CoreCard = ( {
) } { icon && ( -
- +
+
) } { actions?.length > 0 && actionType === 'toggle' && ( diff --git a/packages/components/src/card/style-core.scss b/packages/components/src/card/style-core.scss index e53da30877..a5c48abea1 100644 --- a/packages/components/src/card/style-core.scss +++ b/packages/components/src/card/style-core.scss @@ -22,8 +22,8 @@ width: wp-vars.$grid-unit-60; svg { fill: var(--wp-admin-theme-color); - height: var(--newspack-card-icon-size, wp-vars.$grid-unit-50); - width: var(--newspack-card-icon-size, wp-vars.$grid-unit-50); + height: wp-vars.$grid-unit-50; + width: wp-vars.$grid-unit-50; } .newspack-card--core__has-icon-background-color & { background: var(--wp-admin-theme-color-lighter-10); @@ -48,8 +48,8 @@ height: wp-vars.$grid-unit-50; width: wp-vars.$grid-unit-50; svg { - height: var(--newspack-card-icon-size, wp-vars.$grid-unit-30); - width: var(--newspack-card-icon-size, wp-vars.$grid-unit-30); + height: wp-vars.$grid-unit-30; + width: wp-vars.$grid-unit-30; } } } diff --git a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx index df5d58c04c..d3852c3183 100644 --- a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx +++ b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx @@ -13,12 +13,39 @@ type UseUnsavedChangesDialogOptions = { when: boolean; }; +// Module-level active-instance counter used only to warn in development when +// multiple consumers mount the same guard simultaneously. The click handler +// is document-level capture, so a second active instance would fire a second +// dialog on top of the first. Consumers should ensure only one instance is +// active at a time. +let activeInstances = 0; + +/** + * Returns true when `href` resolves to the same origin as the current page — + * i.e. it is an internal navigation that would unload the wizard. External + * `https://...` links, `mailto:`, `tel:`, and other schemes navigate to a new + * context (new tab, mail client, dialer) without unloading the wizard, and + * must not trigger the discard-changes prompt. + */ +function isSameOriginNavigation( link: HTMLAnchorElement ): boolean { + try { + const url = new URL( link.href, window.location.href ); + return url.origin === window.location.origin && ( url.protocol === 'http:' || url.protocol === 'https:' ); + } catch ( e ) { + return false; + } +} + /** * Shared unsaved-changes guard. Wraps `useConfirmDialog` with standardized - * messaging, intercepts outbound link clicks so the dialog fires instead of - * a silent navigation, and adds a `beforeunload` listener as the last-resort + * messaging, intercepts same-origin link clicks so the dialog fires instead + * of a silent navigation, and adds a `beforeunload` listener as the last-resort * guard for refresh / tab-close (browser-native, cannot be styled). The * returned `confirmDialog` element must be rendered in JSX. + * + * Single-consumer constraint: the click handler is attached at the document + * level in capture phase. Two simultaneously-active instances will both fire + * a dialog. A development-only warning surfaces this. */ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { const { confirmDialog, requestConfirm } = useConfirmDialog( { @@ -36,6 +63,14 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { if ( ! when ) { return; } + activeInstances += 1; + if ( process.env.NODE_ENV !== 'production' && activeInstances > 1 ) { + // eslint-disable-next-line no-console + console.warn( + 'useUnsavedChangesDialog: more than one active instance detected. ' + + 'Document-level click capture will fire a dialog per instance — ensure only one guard is active at a time.' + ); + } const handler = ( e: MouseEvent ) => { if ( e.defaultPrevented || e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button !== 0 ) { return; @@ -52,6 +87,11 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { if ( link.target && link.target !== '_self' ) { return; } + // Skip mailto:, tel:, external origins, and any non-http(s) scheme — + // they don't unload the wizard, so a discard prompt would be wrong. + if ( ! isSameOriginNavigation( link ) ) { + return; + } e.preventDefault(); e.stopPropagation(); const destination = link.href; @@ -61,7 +101,10 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { } ); }; document.addEventListener( 'click', handler, true ); - return () => document.removeEventListener( 'click', handler, true ); + return () => { + document.removeEventListener( 'click', handler, true ); + activeInstances -= 1; + }; }, [ when, requestConfirm ] ); useEffect( () => { diff --git a/packages/components/src/integration-icons/active-campaign.js b/packages/components/src/integration-icons/active-campaign.js index afc1de382f..5cce79199f 100644 --- a/packages/components/src/integration-icons/active-campaign.js +++ b/packages/components/src/integration-icons/active-campaign.js @@ -1,12 +1,12 @@ const activeCampaign = ( - + ); diff --git a/packages/components/src/integration-icons/constant-contact.js b/packages/components/src/integration-icons/constant-contact.js index b423c18d00..ac08f5dfc3 100644 --- a/packages/components/src/integration-icons/constant-contact.js +++ b/packages/components/src/integration-icons/constant-contact.js @@ -1,20 +1,20 @@ const constantContact = ( - + ); diff --git a/packages/components/src/integration-icons/fundraise-up.js b/packages/components/src/integration-icons/fundraise-up.js index 8748a950a9..38145a2123 100644 --- a/packages/components/src/integration-icons/fundraise-up.js +++ b/packages/components/src/integration-icons/fundraise-up.js @@ -1,20 +1,16 @@ const fundraiseUp = ( - + - ); diff --git a/packages/components/src/integration-icons/mailchimp.js b/packages/components/src/integration-icons/mailchimp.js index f51d98a5bf..38ff8da764 100644 --- a/packages/components/src/integration-icons/mailchimp.js +++ b/packages/components/src/integration-icons/mailchimp.js @@ -1,8 +1,8 @@ const mailchimp = ( - + ); diff --git a/src/wizards/newsletters/views/index.js b/src/wizards/newsletters/views/index.js index 0bf75f1d8d..ab6044375c 100644 --- a/src/wizards/newsletters/views/index.js +++ b/src/wizards/newsletters/views/index.js @@ -1,2 +1 @@ export { default as Settings } from './settings'; -export { default as Tracking } from './tracking'; diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 7c268a2be0..cc1273cea7 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -11,7 +11,7 @@ import once from 'lodash/once'; /** * WordPress dependencies */ -import { Fragment, useEffect, useRef, useState } from '@wordpress/element'; +import { Fragment, useCallback, useEffect, useRef, useState } from '@wordpress/element'; import { useDispatch } from '@wordpress/data'; import apiFetch from '@wordpress/api-fetch'; import { sprintf, __ } from '@wordpress/i18n'; @@ -24,16 +24,20 @@ import { } from '@wordpress/components'; import { atSymbol } from '@wordpress/icons'; -// Wizard-bridge events. Mirror of `newspack-newsletters/src/wizard-bridge/events.js` -// — kept locally so this file is self-contained without a cross-repo import. +// Wizard-bridge event contract. The newsletters bridge bundle exposes its +// event names on `window.newspackNewslettersEvents`; we fall back to the +// hand-rolled mirror so this file works in isolation (tests, partial loads) +// and so the two repos stay in sync without coordinated edits once the +// bridge ships the global. const NN_EVENT_NAMESPACE = 'newspack-newsletters'; -const NN_EVENTS = { +const NN_FALLBACK_EVENTS = { BRIDGE_MOUNTED: `${ NN_EVENT_NAMESPACE }:bridge-mounted`, OPEN_MODAL: `${ NN_EVENT_NAMESPACE }:open-local-list-modal`, OPEN_CONFIRM_DELETE: `${ NN_EVENT_NAMESPACE }:open-local-list-confirm-delete`, LOCAL_LIST_SAVED: `${ NN_EVENT_NAMESPACE }:local-list-saved`, LOCAL_LIST_DELETED: `${ NN_EVENT_NAMESPACE }:local-list-deleted`, }; +const getNNEvents = () => ( typeof window !== 'undefined' && window.newspackNewslettersEvents ) || NN_FALLBACK_EVENTS; const NN_FALLBACK_TIMEOUT_MS = 500; // Read the bridge-readiness flag synchronously rather than relying on a @@ -62,7 +66,7 @@ import { useUnsavedChangesDialog, } from '../../../../../packages/components/src'; import { WIZARD_STORE_NAMESPACE } from '../../../../../packages/components/src/wizard/store'; -import Tracking from '../tracking'; +import Tracking from './tracking'; import './style.scss'; @@ -70,6 +74,7 @@ const LETTERHEAD_KEY = 'newspack_newsletters_letterhead_api_key'; export const Settings = ( { onUpdate, + onConfigured, onLabels, onLetterheadSetting, newslettersConfig, @@ -141,6 +146,9 @@ export const Settings = ( { } ) .then( response => { performConfigUpdate( response ); + if ( onConfigured ) { + onConfigured( response?.configured === true ); + } if ( onLabels && response?.labels ) { onLabels( response.labels ); } @@ -256,10 +264,6 @@ export const Settings = ( { constant_contact: integrationIcons.constantContact, manual: atSymbol, }; - const PROVIDER_ICON_SIZES = { - active_campaign: 16, - constant_contact: 18, - }; const providerOptions = ( config.settings?.newspack_newsletters_service_provider?.options || [] ) .filter( opt => opt.value !== '' ) .sort( ( a, b ) => { @@ -320,7 +324,6 @@ export const Settings = ( { key={ option.value } className={ `newspack-newsletters-esp-card newspack-newsletters-esp-card--${ option.value.replace( /_/g, '-' ) }` } icon={ PROVIDER_ICONS[ option.value ] } - iconSize={ PROVIDER_ICON_SIZES[ option.value ] || 24 } title={ option.name } isActive={ option.value === selectedProviderValue } onEnable={ () => providerSelectProps.onChange( option.value ) } @@ -469,15 +472,21 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { }, [ provider, lockedLists ] ); useEffect( () => { + const { LOCAL_LIST_SAVED, LOCAL_LIST_DELETED } = getNNEvents(); const reload = () => fetchLists(); - document.addEventListener( NN_EVENTS.LOCAL_LIST_SAVED, reload ); - document.addEventListener( NN_EVENTS.LOCAL_LIST_DELETED, reload ); + document.addEventListener( LOCAL_LIST_SAVED, reload ); + document.addEventListener( LOCAL_LIST_DELETED, reload ); return () => { - document.removeEventListener( NN_EVENTS.LOCAL_LIST_SAVED, reload ); - document.removeEventListener( NN_EVENTS.LOCAL_LIST_DELETED, reload ); + document.removeEventListener( LOCAL_LIST_SAVED, reload ); + document.removeEventListener( LOCAL_LIST_DELETED, reload ); }; }, [] ); + // Clear the fallback timer on unmount — otherwise a scheduled redirect + // would fire after the component is gone, navigating the user away + // unexpectedly. + useEffect( () => () => clearTimeout( fallbackTimerRef.current ), [] ); + const startFallbackTimer = fallbackUrl => { if ( isBridgeReady() || ! fallbackUrl ) { return; @@ -491,15 +500,15 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { }; const dispatchOpenAdd = () => { - document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_MODAL, { detail: { mode: 'add' } } ) ); + document.dispatchEvent( new CustomEvent( getNNEvents().OPEN_MODAL, { detail: { mode: 'add' } } ) ); startFallbackTimer( newspack_newsletters_wizard.new_subscription_lists_url ); }; const dispatchOpenEdit = ( list, kind ) => { - document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_MODAL, { detail: { mode: 'edit', kind, list } } ) ); + document.dispatchEvent( new CustomEvent( getNNEvents().OPEN_MODAL, { detail: { mode: 'edit', kind, list } } ) ); startFallbackTimer( list?.edit_link ); }; const dispatchConfirmDelete = list => { - document.dispatchEvent( new CustomEvent( NN_EVENTS.OPEN_CONFIRM_DELETE, { detail: { list } } ) ); + document.dispatchEvent( new CustomEvent( getNNEvents().OPEN_CONFIRM_DELETE, { detail: { list } } ) ); startFallbackTimer( list?.edit_link ); }; @@ -634,7 +643,8 @@ const NewslettersSettings = () => { const [ savedConfig, setSavedConfig ] = useState( null ); const [ labels, setLabels ] = useState( {} ); const [ letterheadSetting, setLetterheadSetting ] = useState( null ); - const { setHeaderData } = useDispatch( WIZARD_STORE_NAMESPACE ); + const [ isConfigured, setIsConfigured ] = useState( false ); + const { setHeaderData, addNotice } = useDispatch( WIZARD_STORE_NAMESPACE ); useEffect( () => { if ( savedConfig === null && newslettersConfig && Object.keys( newslettersConfig ).length > 0 ) { @@ -644,27 +654,42 @@ const NewslettersSettings = () => { const isDirty = savedConfig !== null && JSON.stringify( newslettersConfig ) !== JSON.stringify( savedConfig ); - const saveSettings = async () => { + // Only seed `letterheadSetting` once. The setting metadata is stable — + // subsequent fetches would re-set the same value and churn renders. + const handleLetterheadSetting = useCallback( setting => { + setLetterheadSetting( prev => ( prev ? prev : setting ) ); + }, [] ); + + const saveSettings = useCallback( async () => { + // Snapshot the payload before the await so `savedConfig` reflects the + // exact data sent to the server, even if the user edits the form + // while the request is in flight. + const payload = newslettersConfig; setError( false ); setInFlight( true ); try { const response = await apiFetch( { path: '/newspack/v1/wizard/newspack-newsletters/settings', method: 'POST', - data: newslettersConfig, + data: payload, } ); - setProvider( newslettersConfig?.newspack_newsletters_service_provider ); + setProvider( payload?.newspack_newsletters_service_provider ); setLockedLists( false ); - setSavedConfig( newslettersConfig ); + setSavedConfig( payload ); if ( response?.labels ) { setLabels( response.labels ); } + addNotice( { + id: 'newsletters-settings-saved', + type: 'success', + message: __( 'Settings saved.', 'newspack-plugin' ), + } ); } catch ( err ) { setError( err ); } finally { setInFlight( false ); } - }; + }, [ newslettersConfig, addNotice ] ); useEffect( () => { setHeaderData( { @@ -679,7 +704,7 @@ const NewslettersSettings = () => { }, ], } ); - }, [ inFlight, isDirty, newslettersConfig ] ); + }, [ inFlight, isDirty, saveSettings, setHeaderData ] ); const { confirmDialog: navBlockDialog } = useUnsavedChangesDialog( { when: isDirty && ! inFlight, @@ -696,8 +721,9 @@ const NewslettersSettings = () => { updateConfiguration( { newslettersConfig: config } ) } + onConfigured={ setIsConfigured } onLabels={ setLabels } - onLetterheadSetting={ setLetterheadSetting } + onLetterheadSetting={ handleLetterheadSetting } authUrl={ authUrl } newslettersConfig={ newslettersConfig } provider={ provider } @@ -706,7 +732,7 @@ const NewslettersSettings = () => { setLockedLists={ setLockedLists } /> - + { isConfigured && } { letterheadSetting && ( <> diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 6079acdc48..c6e5b4c6a9 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -1,7 +1,9 @@ -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; import apiFetch from '@wordpress/api-fetch'; +import { dispatch, select } from '@wordpress/data'; -import { SubscriptionLists } from './index'; +import NewslettersSettings, { Settings, SubscriptionLists } from './index'; +import { WIZARD_STORE_NAMESPACE } from '../../../../../packages/components/src/wizard/store'; jest.mock( '@wordpress/api-fetch', () => jest.fn() ); @@ -13,6 +15,38 @@ const NN_EVENTS = { LOCAL_LIST_DELETED: 'newspack-newsletters:local-list-deleted', }; +const SUBSCRIPTION_LISTS_FIXTURE = [ + { id: 'tag-1', name: 'Local A', type: 'local', active: false, db_id: 1, edit_link: 'https://example.test/edit-local-a' }, + { id: 'group-1', name: 'Remote group', type: 'group', active: true, db_id: 2, edit_link: 'https://example.test/edit-remote' }, +]; + +const SETTINGS_FIXTURE = { + configured: true, + labels: { local_list_explanation: 'Mailchimp Group' }, + settings: { + newspack_newsletters_service_provider: { + key: 'newspack_newsletters_service_provider', + description: 'Service Provider', + value: '', + type: 'select', + options: [ + { value: '', name: '-- Select --' }, + { value: 'mailchimp', name: 'Mailchimp' }, + { value: 'active_campaign', name: 'Active Campaign' }, + { value: 'constant_contact', name: 'Constant Contact' }, + { value: 'manual', name: 'Manual / Other' }, + ], + }, + newspack_newsletters_mailchimp_api_key: { + key: 'newspack_newsletters_mailchimp_api_key', + description: 'Mailchimp API Key', + value: '', + type: 'text', + provider: 'mailchimp', + }, + }, +}; + beforeAll( () => { global.newspack_newsletters_wizard = { new_subscription_lists_url: 'https://example.test/wp-admin/post-new.php?post_type=newspack_nl_list', @@ -21,16 +55,14 @@ beforeAll( () => { beforeEach( () => { apiFetch.mockReset(); - apiFetch.mockResolvedValue( [ - { id: 'tag-1', name: 'Local A', type: 'local', active: false, db_id: 1, edit_link: 'https://example.test/edit-local-a' }, - { id: 'group-1', name: 'Remote group', type: 'group', active: true, db_id: 2, edit_link: 'https://example.test/edit-remote' }, - ] ); + apiFetch.mockResolvedValue( SUBSCRIPTION_LISTS_FIXTURE ); // Mark the bridge ready so the fallback timer doesn't navigate the test window. window.newspackNewslettersBridgeReady = true; } ); afterEach( () => { delete window.newspackNewslettersBridgeReady; + delete window.newspackNewslettersEvents; } ); describe( 'SubscriptionLists — wizard-bridge wiring', () => { @@ -50,7 +82,7 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); render( ); await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); - fireEvent.click( screen.getAllByRole( 'button', { name: /^Edit$/ } )[ 0 ] ); + fireEvent.click( screen.getByRole( 'button', { name: 'Edit Local A' } ) ); expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( expect.objectContaining( { mode: 'edit', kind: 'local', list: expect.objectContaining( { db_id: 1 } ) } ) ); @@ -62,8 +94,7 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); render( ); await waitFor( () => expect( screen.getByText( 'Remote group' ) ).toBeInTheDocument() ); - // Remote rows now have an Edit button too — second one in the list. - fireEvent.click( screen.getAllByRole( 'button', { name: /^Edit$/ } )[ 1 ] ); + fireEvent.click( screen.getByRole( 'button', { name: 'Edit Remote group' } ) ); expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( expect.objectContaining( { mode: 'edit', kind: 'esp', list: expect.objectContaining( { db_id: 2 } ) } ) ); @@ -73,7 +104,6 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { it( 'commits the active toggle immediately via PATCH /lists/{db_id}', async () => { render( ); await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); - // Configure the next response (a successful PATCH echoing the row). apiFetch.mockResolvedValueOnce( { id: 'tag-1', db_id: 1, active: true } ); fireEvent.click( screen.getAllByRole( 'checkbox' )[ 0 ] ); await waitFor( () => @@ -103,7 +133,7 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { document.addEventListener( NN_EVENTS.OPEN_CONFIRM_DELETE, listener ); render( ); await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); - fireEvent.click( screen.getByRole( 'button', { name: /^Delete$/ } ) ); + fireEvent.click( screen.getByRole( 'button', { name: 'Delete Local A' } ) ); expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( expect.objectContaining( { list: expect.objectContaining( { db_id: 1 } ) } ) ); document.removeEventListener( NN_EVENTS.OPEN_CONFIRM_DELETE, listener ); } ); @@ -123,9 +153,6 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { } ); it( 'does not redirect when the bridge mounted before the wizard listener registered', async () => { - // The flag is already set in beforeEach, simulating the bridge having - // completed boot before this component mounted. The fallback timer - // must NOT navigate. jest.useFakeTimers(); const originalHref = window.location.href; render( ); @@ -135,4 +162,149 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { expect( window.location.href ).toBe( originalHref ); jest.useRealTimers(); } ); + + it( 'clears the fallback timer on unmount so a redirect cannot fire after the component is gone', async () => { + // Simulate the bridge NOT being ready so the fallback timer arms. + delete window.newspackNewslettersBridgeReady; + jest.useFakeTimers(); + const originalHref = window.location.href; + const { unmount } = render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + unmount(); + jest.advanceTimersByTime( 600 ); + expect( window.location.href ).toBe( originalHref ); + jest.useRealTimers(); + } ); + + it( 'reads event names from window.newspackNewslettersEvents when the bridge exposes them', async () => { + window.newspackNewslettersEvents = { + ...NN_EVENTS, + OPEN_MODAL: 'custom:open-modal', + }; + const listener = jest.fn(); + document.addEventListener( 'custom:open-modal', listener ); + render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + expect( listener ).toHaveBeenCalled(); + document.removeEventListener( 'custom:open-modal', listener ); + } ); +} ); + +describe( 'Settings — ESP card grid', () => { + beforeEach( () => { + apiFetch.mockReset(); + apiFetch.mockResolvedValue( SETTINGS_FIXTURE ); + } ); + + const renderSettings = ( overrides = {} ) => { + const props = { + isOnboarding: false, + newslettersConfig: { newspack_newsletters_service_provider: '' }, + onUpdate: jest.fn(), + onConfigured: jest.fn(), + onLabels: jest.fn(), + onLetterheadSetting: jest.fn(), + setProvider: jest.fn(), + setAuthUrl: jest.fn(), + setLockedLists: jest.fn(), + ...overrides, + }; + return { props, ...render( ) }; + }; + + it( 'renders all four ESP cards in the configured order', async () => { + renderSettings(); + await waitFor( () => expect( screen.getByText( 'Mailchimp' ) ).toBeInTheDocument() ); + expect( screen.getByText( 'Active Campaign' ) ).toBeInTheDocument(); + expect( screen.getByText( 'Constant Contact' ) ).toBeInTheDocument(); + expect( screen.getByText( 'Manual / Other' ) ).toBeInTheDocument(); + } ); + + it( 'fires onConfigured(true) once the wizard endpoint reports configured=true', async () => { + const { props } = renderSettings(); + await waitFor( () => expect( props.onConfigured ).toHaveBeenCalledWith( true ) ); + } ); + + it( 'fires onLabels with the label payload from the response', async () => { + const { props } = renderSettings(); + await waitFor( () => + expect( props.onLabels ).toHaveBeenCalledWith( expect.objectContaining( { local_list_explanation: 'Mailchimp Group' } ) ) + ); + } ); + + it( 'selecting a provider card calls onUpdate with the chosen provider', async () => { + const { props } = renderSettings(); + await waitFor( () => expect( screen.getByText( 'Mailchimp' ) ).toBeInTheDocument() ); + // CardSettingsGroup wraps the icon+title in a clickable header. + fireEvent.click( screen.getByText( 'Mailchimp' ) ); + await waitFor( () => + expect( props.onUpdate ).toHaveBeenCalledWith( expect.objectContaining( { newspack_newsletters_service_provider: 'mailchimp' } ) ) + ); + } ); +} ); + +describe( 'NewslettersSettings — dirty tracking, save flow, snackbar', () => { + beforeEach( () => { + apiFetch.mockReset(); + apiFetch.mockResolvedValue( SETTINGS_FIXTURE ); + // Reset wizard store between tests so prior notices/header don't leak. + dispatch( WIZARD_STORE_NAMESPACE ).resetNotices(); + dispatch( WIZARD_STORE_NAMESPACE ).resetHeaderData(); + } ); + + const getSaveAction = () => select( WIZARD_STORE_NAMESPACE ).getHeaderData()?.actions?.[ 0 ]; + + it( 'registers a Save header action that is initially disabled (no dirty state)', async () => { + render( ); + await waitFor( () => expect( getSaveAction() ).toEqual( expect.objectContaining( { label: 'Save', disabled: true } ) ) ); + } ); + + it( 'fires a success snackbar on a successful save', async () => { + render( ); + await waitFor( () => expect( getSaveAction() ).toBeDefined() ); + // Save endpoint echoes the response on POST. + apiFetch.mockResolvedValueOnce( SETTINGS_FIXTURE ); + await act( async () => { + await getSaveAction().action(); + } ); + const notices = select( WIZARD_STORE_NAMESPACE ).getNotices(); + expect( notices ).toEqual( + expect.arrayContaining( [ expect.objectContaining( { type: 'success', message: expect.stringMatching( /saved/i ) } ) ] ) + ); + } ); + + it( 'does not render Tracking until the wizard endpoint reports configured=true', async () => { + // Unconfigured response — Tracking should stay hidden so it doesn't + // hit the tracking endpoint on installs without the newsletters plugin. + apiFetch.mockResolvedValue( { ...SETTINGS_FIXTURE, configured: false } ); + render( ); + await waitFor( () => expect( getSaveAction() ).toBeDefined() ); + expect( screen.queryByText( /Ads tracking/i ) ).not.toBeInTheDocument(); + } ); + + it( 'clears the dirty flag after save even if a fetch resolves during the request', async () => { + // Captures the payload at save-call time so the saved snapshot reflects + // what was actually sent, not a later edit. + let resolveSave; + const savePromise = new Promise( resolve => { + resolveSave = resolve; + } ); + apiFetch.mockImplementation( config => { + if ( config?.method === 'POST' ) { + return savePromise; + } + return Promise.resolve( SETTINGS_FIXTURE ); + } ); + render( ); + await waitFor( () => expect( getSaveAction() ).toBeDefined() ); + const pending = act( async () => { + await getSaveAction().action(); + } ); + resolveSave( SETTINGS_FIXTURE ); + await pending; + // After save, header Save action should be disabled again. + await waitFor( () => expect( getSaveAction() ).toEqual( expect.objectContaining( { disabled: true } ) ) ); + } ); } ); diff --git a/src/wizards/newsletters/views/settings/style.scss b/src/wizards/newsletters/views/settings/style.scss index 85f58c18ed..533a26e053 100644 --- a/src/wizards/newsletters/views/settings/style.scss +++ b/src/wizards/newsletters/views/settings/style.scss @@ -21,11 +21,8 @@ .newspack-newsletters-esp-card { - &--active-campaign { - --integration-icon-color: #{wp-colors.$white}; - .newspack-card--core__icon { - background: #004CFF; - } + &--active-campaign .newspack-card--core__icon { + background: #004CFF; } &--mailchimp .newspack-card--core__icon { background: #FFE01B; diff --git a/src/wizards/newsletters/views/tracking/index.js b/src/wizards/newsletters/views/settings/tracking.js similarity index 100% rename from src/wizards/newsletters/views/tracking/index.js rename to src/wizards/newsletters/views/settings/tracking.js From 9ccdf4f2faaf5a379153c141bcf7f7440cb44f70 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 13:56:50 +0100 Subject: [PATCH 12/26] fix(newsletters): disable settings controls during save (NEWS-2263) --- .../newsletters/views/settings/index.js | 47 ++++++++++++++----- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index cc1273cea7..866be05f6e 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -80,6 +80,7 @@ export const Settings = ( { newslettersConfig, isOnboarding = true, authUrl = false, + isSaving = false, provider, setProvider = () => {}, setAuthUrl = () => {}, @@ -88,6 +89,11 @@ export const Settings = ( { const [ inFlight, setInFlight ] = useState( false ); const [ error, setError ] = useState( false ); const [ config, updateConfig ] = hooks.useObjectState( {} ); + // Combine the local fetch/verify in-flight state with the parent's save + // state so all editing controls are disabled while the outer Save is + // in flight — prevents a race where a user changes the ESP between + // "Save click" and "POST resolve". + const isDisabled = inFlight || isSaving; // Handle provider updates. useEffect( () => { const newProvider = newslettersConfig?.newspack_newsletters_service_provider || ''; @@ -198,7 +204,7 @@ export const Settings = ( { }; useEffect( fetchConfiguration, [] ); const getSettingProps = key => ( { - disabled: inFlight, + disabled: isDisabled, value: config.settings[ key ]?.value || '', checked: Boolean( config.settings[ key ]?.value ), label: config.settings[ key ]?.description, @@ -319,17 +325,30 @@ export const Settings = ( { ) : ( <> - { providerOptions.map( option => ( - providerSelectProps.onChange( option.value ) } - onHeaderClick={ () => providerSelectProps.onChange( option.value ) } - /> - ) ) } + { providerOptions.map( option => { + // Short-circuit while saving so the user can't switch ESPs + // between Save click and POST resolve. + const onSelect = () => { + if ( isDisabled ) { + return; + } + providerSelectProps.onChange( option.value ); + }; + return ( + + ); + } ) } { selectedProviderValue && ( @@ -341,7 +360,7 @@ export const Settings = ( { ) } { isOnboarding && ( - @@ -720,6 +739,7 @@ const NewslettersSettings = () => { ) } updateConfiguration( { newslettersConfig: config } ) } onConfigured={ setIsConfigured } onLabels={ setLabels } @@ -749,6 +769,7 @@ const NewslettersSettings = () => { label={ letterheadSetting.description } value={ newslettersConfig?.[ letterheadSetting.key ] || '' } onChange={ value => updateConfiguration( { newslettersConfig: { [ letterheadSetting.key ]: value } } ) } + disabled={ inFlight } withMargin={ false } /> { letterheadSetting.help && letterheadSetting.helpURL && ( From ffeb834d349e5acd95ff2008fb9f67dfdd2ce0d6 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 14:06:20 +0100 Subject: [PATCH 13/26] fix(newsletters): hide Subscription Lists for manual provider (NEWS-2263) --- src/wizards/newsletters/views/settings/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 866be05f6e..0b45e4e8c8 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -751,7 +751,7 @@ const NewslettersSettings = () => { setAuthUrl={ setAuthUrl } setLockedLists={ setLockedLists } /> - + { provider !== 'manual' && } { isConfigured && } { letterheadSetting && ( <> From f0bdaf1128c6b923446ad6f033fe8dfc231d35d1 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 14:28:58 +0100 Subject: [PATCH 14/26] fix(newsletters): a11y + bridge-event robustness (NEWS-2263) --- .../src/card-settings-group/index.tsx | 3 + packages/components/src/card/core-card.js | 6 +- packages/components/src/card/style-core.scss | 7 ++ .../newsletters/views/settings/index.js | 85 ++++++++++++------- 4 files changed, 71 insertions(+), 30 deletions(-) diff --git a/packages/components/src/card-settings-group/index.tsx b/packages/components/src/card-settings-group/index.tsx index c82f4963ca..91836fb0e2 100644 --- a/packages/components/src/card-settings-group/index.tsx +++ b/packages/components/src/card-settings-group/index.tsx @@ -12,6 +12,7 @@ const CardSettingsGroup = ( { actionType = 'none', children, className, + disabled = false, icon = null, headerAction, title = '', @@ -23,6 +24,7 @@ const CardSettingsGroup = ( { actionType?: 'chevron' | 'toggle' | 'button' | 'link' | 'none'; children?: React.ReactNode; className?: string; + disabled?: boolean; icon?: React.ReactNode; title: string; headerAction?: { @@ -56,6 +58,7 @@ const CardSettingsGroup = ( { headerAction, onHeaderClick, onToggle: onEnable, + disabled, icon, iconBackgroundColor: true, isActive, diff --git a/packages/components/src/card/core-card.js b/packages/components/src/card/core-card.js index 7eea8b07a4..5fe392f8b5 100644 --- a/packages/components/src/card/core-card.js +++ b/packages/components/src/card/core-card.js @@ -32,6 +32,7 @@ const CoreCard = ( { headerStyle, childrenStyle, footerStyle, + disabled, icon, iconBackgroundColor, isActive, @@ -62,6 +63,7 @@ const CoreCard = ( { icon && 'newspack-card--core__has-icon', iconBackgroundColor && 'newspack-card--core__has-icon-background-color', isActive && 'newspack-card--core__is-active', + disabled && 'newspack-card--core__is-disabled', children && 'newspack-card--core__has-children', noMargin && 'newspack-card--core__no-margin', hasGreyHeader && 'newspack-card--core__has-grey-header' @@ -91,7 +93,9 @@ const CoreCard = ( { style={ headerStyle } size={ sizeProps } gap={ 4 } - onClick={ onHeaderClick } + onClick={ disabled ? undefined : onHeaderClick } + disabled={ onHeaderClick && disabled ? true : undefined } + aria-disabled={ onHeaderClick && disabled ? true : undefined } > { isDraggable && (
diff --git a/packages/components/src/card/style-core.scss b/packages/components/src/card/style-core.scss index a5c48abea1..571f6eaac6 100644 --- a/packages/components/src/card/style-core.scss +++ b/packages/components/src/card/style-core.scss @@ -57,6 +57,13 @@ &__no-margin .components-card__body { padding: 0 !important; } + &__is-disabled { + cursor: not-allowed; + opacity: 0.5; + .newspack-card--core__header { + pointer-events: none; + } + } &__header { color: wp-colors.$gray-700; flex-wrap: wrap; diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 0b45e4e8c8..4260396fcb 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -325,30 +325,18 @@ export const Settings = ( { ) : ( <> - { providerOptions.map( option => { - // Short-circuit while saving so the user can't switch ESPs - // between Save click and POST resolve. - const onSelect = () => { - if ( isDisabled ) { - return; - } - providerSelectProps.onChange( option.value ); - }; - return ( - - ); - } ) } + { providerOptions.map( option => ( + providerSelectProps.onChange( option.value ) } + onHeaderClick={ () => providerSelectProps.onChange( option.value ) } + /> + ) ) } { selectedProviderValue && ( @@ -491,13 +479,52 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { }, [ provider, lockedLists ] ); useEffect( () => { - const { LOCAL_LIST_SAVED, LOCAL_LIST_DELETED } = getNNEvents(); const reload = () => fetchLists(); - document.addEventListener( LOCAL_LIST_SAVED, reload ); - document.addEventListener( LOCAL_LIST_DELETED, reload ); + // Listen on both the fallback names and any names exposed on + // `window.newspackNewslettersEvents` so this still fires if the + // bridge ships a renamed event in a future version. Set + // deduplicates when the two are identical (the case today). + const collectNames = () => { + const live = getNNEvents(); + return { + saved: new Set( [ NN_FALLBACK_EVENTS.LOCAL_LIST_SAVED, live.LOCAL_LIST_SAVED ] ), + deleted: new Set( [ NN_FALLBACK_EVENTS.LOCAL_LIST_DELETED, live.LOCAL_LIST_DELETED ] ), + }; + }; + + let { saved, deleted } = collectNames(); + const attach = () => { + saved.forEach( name => document.addEventListener( name, reload ) ); + deleted.forEach( name => document.addEventListener( name, reload ) ); + }; + const detach = () => { + saved.forEach( name => document.removeEventListener( name, reload ) ); + deleted.forEach( name => document.removeEventListener( name, reload ) ); + }; + + attach(); + + // If the bridge wasn't ready at mount, re-resolve event names when + // it announces itself. The bridge dispatches BRIDGE_MOUNTED using + // whatever names it exposes, so listen on both the fallback name + // and the live name. + const bridgeMountedNames = new Set( [ NN_FALLBACK_EVENTS.BRIDGE_MOUNTED, getNNEvents().BRIDGE_MOUNTED ] ); + const onBridgeMounted = () => { + const next = collectNames(); + const changed = [ ...next.saved ].some( n => ! saved.has( n ) ) || [ ...next.deleted ].some( n => ! deleted.has( n ) ); + if ( ! changed ) { + return; + } + detach(); + saved = next.saved; + deleted = next.deleted; + attach(); + }; + bridgeMountedNames.forEach( name => document.addEventListener( name, onBridgeMounted ) ); + return () => { - document.removeEventListener( LOCAL_LIST_SAVED, reload ); - document.removeEventListener( LOCAL_LIST_DELETED, reload ); + detach(); + bridgeMountedNames.forEach( name => document.removeEventListener( name, onBridgeMounted ) ); }; }, [] ); From 5201805074ca8af86339b7c960948eabb47c2cf6 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 14:35:49 +0100 Subject: [PATCH 15/26] fix(newsletters): queue bridge dispatch when not yet ready (NEWS-2263) --- .../newsletters/views/settings/index.js | 46 +++++++++++++++---- .../newsletters/views/settings/index.test.js | 24 ++++++++++ 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 4260396fcb..00a5394c1f 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -418,6 +418,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { const [ togglingIds, setTogglingIds ] = useState( () => new Set() ); const [ lists, setLists ] = useState( [] ); const fallbackTimerRef = useRef( null ); + // When the bridge isn't ready at click time, we queue the dispatch here + // instead of firing it into the void. The bridge-mounted handler + // (registered below) flushes this; the fallback timer navigates to the + // legacy URL if the bridge never mounts. + const pendingActionRef = useRef( null ); const updateLists = updater => { setLists( prev => { @@ -510,6 +515,15 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { // and the live name. const bridgeMountedNames = new Set( [ NN_FALLBACK_EVENTS.BRIDGE_MOUNTED, getNNEvents().BRIDGE_MOUNTED ] ); const onBridgeMounted = () => { + // Flush any action queued while the bridge wasn't ready — the + // original dispatch fired into the void, so replay it now that + // listeners are guaranteed to be installed. + const pending = pendingActionRef.current; + if ( pending ) { + pendingActionRef.current = null; + clearTimeout( fallbackTimerRef.current ); + pending.dispatch(); + } const next = collectNames(); const changed = [ ...next.saved ].some( n => ! saved.has( n ) ) || [ ...next.deleted ].some( n => ! deleted.has( n ) ); if ( ! changed ) { @@ -533,29 +547,41 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { // unexpectedly. useEffect( () => () => clearTimeout( fallbackTimerRef.current ), [] ); - const startFallbackTimer = fallbackUrl => { - if ( isBridgeReady() || ! fallbackUrl ) { + // Dispatch a bridge event, or queue it for replay if the bridge isn't + // ready yet. Dispatching while the bridge has no listeners installed + // would silently drop the event — the queue + BRIDGE_MOUNTED flush + // guarantees delivery, with a 500ms fallback to the legacy URL if the + // bridge never shows up. + const dispatchOrQueue = ( eventName, detail, fallbackUrl ) => { + const dispatch = () => document.dispatchEvent( new CustomEvent( eventName, { detail } ) ); + if ( isBridgeReady() ) { + dispatch(); return; } + pendingActionRef.current = { dispatch, fallbackUrl }; clearTimeout( fallbackTimerRef.current ); + if ( ! fallbackUrl ) { + return; + } fallbackTimerRef.current = setTimeout( () => { - if ( ! isBridgeReady() ) { - window.location.href = fallbackUrl; + // Bridge never mounted in time — clear the queue and navigate + // to the legacy URL so the user isn't left with a dead click. + const pending = pendingActionRef.current; + pendingActionRef.current = null; + if ( pending && pending.fallbackUrl ) { + window.location.href = pending.fallbackUrl; } }, NN_FALLBACK_TIMEOUT_MS ); }; const dispatchOpenAdd = () => { - document.dispatchEvent( new CustomEvent( getNNEvents().OPEN_MODAL, { detail: { mode: 'add' } } ) ); - startFallbackTimer( newspack_newsletters_wizard.new_subscription_lists_url ); + dispatchOrQueue( getNNEvents().OPEN_MODAL, { mode: 'add' }, newspack_newsletters_wizard.new_subscription_lists_url ); }; const dispatchOpenEdit = ( list, kind ) => { - document.dispatchEvent( new CustomEvent( getNNEvents().OPEN_MODAL, { detail: { mode: 'edit', kind, list } } ) ); - startFallbackTimer( list?.edit_link ); + dispatchOrQueue( getNNEvents().OPEN_MODAL, { mode: 'edit', kind, list }, list?.edit_link ); }; const dispatchConfirmDelete = list => { - document.dispatchEvent( new CustomEvent( getNNEvents().OPEN_CONFIRM_DELETE, { detail: { list } } ) ); - startFallbackTimer( list?.edit_link ); + dispatchOrQueue( getNNEvents().OPEN_CONFIRM_DELETE, { list }, list?.edit_link ); }; if ( ! inFlight && ! lists?.length && ! error && ! lockedLists ) { diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index c6e5b4c6a9..75a1de9472 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -163,6 +163,30 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { jest.useRealTimers(); } ); + it( 'queues the dispatch when the bridge is not ready and replays it on BRIDGE_MOUNTED', async () => { + delete window.newspackNewslettersBridgeReady; + jest.useFakeTimers(); + const listener = jest.fn(); + document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); + render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + // Initial dispatch fires into the void (bridge listener doesn't exist) — listener captures nothing yet. + expect( listener ).not.toHaveBeenCalled(); + // Bridge becomes ready and announces itself within the fallback window. + window.newspackNewslettersBridgeReady = true; + document.dispatchEvent( new CustomEvent( NN_EVENTS.BRIDGE_MOUNTED ) ); + // Replay fires the queued OPEN_MODAL event so the bridge can handle it. + expect( listener ).toHaveBeenCalledTimes( 1 ); + expect( listener.mock.calls[ 0 ][ 0 ].detail ).toEqual( { mode: 'add' } ); + // Timer fires — should be cleared and not navigate. + const originalHref = window.location.href; + jest.advanceTimersByTime( 600 ); + expect( window.location.href ).toBe( originalHref ); + document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); + jest.useRealTimers(); + } ); + it( 'clears the fallback timer on unmount so a redirect cannot fire after the component is gone', async () => { // Simulate the bridge NOT being ready so the fallback timer arms. delete window.newspackNewslettersBridgeReady; From 573337eeba6e0d9cd62bee9392f0f0be98580a9f Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 14:43:46 +0100 Subject: [PATCH 16/26] fix(newsletters): resolve bridge event name at replay time (NEWS-2263) --- .../newsletters/views/settings/index.js | 14 +++++++----- .../newsletters/views/settings/index.test.js | 22 +++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 00a5394c1f..0157f6425b 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -551,9 +551,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { // ready yet. Dispatching while the bridge has no listeners installed // would silently drop the event — the queue + BRIDGE_MOUNTED flush // guarantees delivery, with a 500ms fallback to the legacy URL if the - // bridge never shows up. - const dispatchOrQueue = ( eventName, detail, fallbackUrl ) => { - const dispatch = () => document.dispatchEvent( new CustomEvent( eventName, { detail } ) ); + // bridge never shows up. The event KEY is stored (not the resolved + // name) so a late-mounting bridge that exposes renamed events still + // receives the correctly-named replay. + const dispatchOrQueue = ( eventKey, detail, fallbackUrl ) => { + const dispatch = () => document.dispatchEvent( new CustomEvent( getNNEvents()[ eventKey ], { detail } ) ); if ( isBridgeReady() ) { dispatch(); return; @@ -575,13 +577,13 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { }; const dispatchOpenAdd = () => { - dispatchOrQueue( getNNEvents().OPEN_MODAL, { mode: 'add' }, newspack_newsletters_wizard.new_subscription_lists_url ); + dispatchOrQueue( 'OPEN_MODAL', { mode: 'add' }, newspack_newsletters_wizard.new_subscription_lists_url ); }; const dispatchOpenEdit = ( list, kind ) => { - dispatchOrQueue( getNNEvents().OPEN_MODAL, { mode: 'edit', kind, list }, list?.edit_link ); + dispatchOrQueue( 'OPEN_MODAL', { mode: 'edit', kind, list }, list?.edit_link ); }; const dispatchConfirmDelete = list => { - dispatchOrQueue( getNNEvents().OPEN_CONFIRM_DELETE, { list }, list?.edit_link ); + dispatchOrQueue( 'OPEN_CONFIRM_DELETE', { list }, list?.edit_link ); }; if ( ! inFlight && ! lists?.length && ! error && ! lockedLists ) { diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 75a1de9472..68073dde37 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -163,6 +163,28 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { jest.useRealTimers(); } ); + it( 'replays a queued dispatch with the live event name when the bridge mounts with renamed events', async () => { + delete window.newspackNewslettersBridgeReady; + jest.useFakeTimers(); + const renamedListener = jest.fn(); + const fallbackListener = jest.fn(); + document.addEventListener( 'custom:open-modal', renamedListener ); + document.addEventListener( NN_EVENTS.OPEN_MODAL, fallbackListener ); + render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + // Bridge mounts and exposes a renamed OPEN_MODAL — replay must + // resolve the event name at replay time, not at click time. + window.newspackNewslettersBridgeReady = true; + window.newspackNewslettersEvents = { ...NN_EVENTS, OPEN_MODAL: 'custom:open-modal' }; + document.dispatchEvent( new CustomEvent( NN_EVENTS.BRIDGE_MOUNTED ) ); + expect( renamedListener ).toHaveBeenCalledTimes( 1 ); + expect( fallbackListener ).not.toHaveBeenCalled(); + document.removeEventListener( 'custom:open-modal', renamedListener ); + document.removeEventListener( NN_EVENTS.OPEN_MODAL, fallbackListener ); + jest.useRealTimers(); + } ); + it( 'queues the dispatch when the bridge is not ready and replays it on BRIDGE_MOUNTED', async () => { delete window.newspackNewslettersBridgeReady; jest.useFakeTimers(); From baa3ddda3820516ada7037ab19978987e8883ce3 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 14:47:44 +0100 Subject: [PATCH 17/26] fix(newsletters): flush queue if bridge is ready at fallback (NEWS-2263) --- .../newsletters/views/settings/index.js | 15 ++++++++++--- .../newsletters/views/settings/index.test.js | 22 +++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 0157f6425b..db02d17046 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -566,11 +566,20 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { return; } fallbackTimerRef.current = setTimeout( () => { - // Bridge never mounted in time — clear the queue and navigate - // to the legacy URL so the user isn't left with a dead click. const pending = pendingActionRef.current; pendingActionRef.current = null; - if ( pending && pending.fallbackUrl ) { + if ( ! pending ) { + return; + } + // Belt-and-braces: if the bridge IS ready but its BRIDGE_MOUNTED + // event was renamed (so our listener missed it), flush the queue + // instead of navigating — the dispatch will land on the live + // listeners that the readiness flag implies. + if ( isBridgeReady() ) { + pending.dispatch(); + return; + } + if ( pending.fallbackUrl ) { window.location.href = pending.fallbackUrl; } }, NN_FALLBACK_TIMEOUT_MS ); diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 68073dde37..5ed4341cc2 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -185,6 +185,28 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { jest.useRealTimers(); } ); + it( 'flushes the queue on the fallback timeout when the bridge is ready but its mounted event was renamed', async () => { + delete window.newspackNewslettersBridgeReady; + jest.useFakeTimers(); + const listener = jest.fn(); + document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); + render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + // Bridge becomes ready but announces itself under a renamed event + // that our listener (registered on the fallback name only at mount) + // doesn't catch. + window.newspackNewslettersBridgeReady = true; + // Timer fires — should flush instead of navigating since + // `isBridgeReady()` is now true. + const originalHref = window.location.href; + jest.advanceTimersByTime( 600 ); + expect( listener ).toHaveBeenCalledTimes( 1 ); + expect( window.location.href ).toBe( originalHref ); + document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); + jest.useRealTimers(); + } ); + it( 'queues the dispatch when the bridge is not ready and replays it on BRIDGE_MOUNTED', async () => { delete window.newspackNewslettersBridgeReady; jest.useFakeTimers(); From 2a3fcbb4c7bf22e0cbc9c8bbdde49d24ed94e106 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 14:50:39 +0100 Subject: [PATCH 18/26] fix(newsletters): clear stale queue on immediate dispatch (NEWS-2263) --- .../newsletters/views/settings/index.js | 5 ++++ .../newsletters/views/settings/index.test.js | 23 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index db02d17046..c85b2557ba 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -557,6 +557,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { const dispatchOrQueue = ( eventKey, detail, fallbackUrl ) => { const dispatch = () => document.dispatchEvent( new CustomEvent( getNNEvents()[ eventKey ], { detail } ) ); if ( isBridgeReady() ) { + // Any prior queued action from before the bridge was ready is + // stale now — clear it so the armed fallback timer can't + // double-dispatch it after this call. + pendingActionRef.current = null; + clearTimeout( fallbackTimerRef.current ); dispatch(); return; } diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 5ed4341cc2..a7186e6f4f 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -185,6 +185,29 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { jest.useRealTimers(); } ); + it( 'does not replay a stale queued action after a newer click dispatches immediately', async () => { + delete window.newspackNewslettersBridgeReady; + jest.useFakeTimers(); + const listener = jest.fn(); + document.addEventListener( NN_EVENTS.OPEN_MODAL, listener ); + render( ); + await waitFor( () => expect( screen.getByRole( 'button', { name: /Add new local list/ } ) ).toBeEnabled() ); + // First click: bridge not ready → queues action A, arms 500ms timer. + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + expect( listener ).not.toHaveBeenCalled(); + // Bridge becomes ready but BRIDGE_MOUNTED never reaches our handler. + window.newspackNewslettersBridgeReady = true; + // Second click: bridge ready → dispatches immediately. The prior + // queued action and armed timer must be cleared, or the timer + // would fire a stale duplicate. + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + expect( listener ).toHaveBeenCalledTimes( 1 ); + jest.advanceTimersByTime( 600 ); + expect( listener ).toHaveBeenCalledTimes( 1 ); + document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); + jest.useRealTimers(); + } ); + it( 'flushes the queue on the fallback timeout when the bridge is ready but its mounted event was renamed', async () => { delete window.newspackNewslettersBridgeReady; jest.useFakeTimers(); From 5e1f3cc55ecb1f2a3e94c6fb215294209baab1db Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 15:02:00 +0100 Subject: [PATCH 19/26] fix(newsletters): friendlier error for missing PATCH endpoint (NEWS-2263) --- src/wizards/newsletters/views/settings/index.js | 15 ++++++++++++++- .../newsletters/views/settings/index.test.js | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index c85b2557ba..5541a18275 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -465,7 +465,20 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { updateLists( prev => prev.map( row => ( row.db_id === dbId ? { ...row, ...response } : row ) ) ); } catch ( err ) { updateLists( prev => prev.map( row => ( row.db_id === dbId ? { ...row, active: previousActive } : row ) ) ); - setError( err ); + // `rest_no_route` means the PATCH /lists/{id} endpoint isn't + // registered yet — the newsletters plugin needs the NEWS-2168 + // changes. Surface a friendlier message than WordPress's + // generic "No route was found...". + if ( err?.code === 'rest_no_route' ) { + setError( { + message: __( + 'This action requires a newer version of Newspack Newsletters. Update the newsletters plugin and try again.', + 'newspack-plugin' + ), + } ); + } else { + setError( err ); + } } finally { setTogglingIds( prev => { const updated = new Set( prev ); diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index a7186e6f4f..ac00090137 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -101,6 +101,22 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { document.removeEventListener( NN_EVENTS.OPEN_MODAL, listener ); } ); + it( 'surfaces a friendlier error when the PATCH endpoint is missing (newsletters plugin too old)', async () => { + render( ); + await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); + // Simulate WP returning rest_no_route for the PATCH call. + apiFetch.mockRejectedValueOnce( { + code: 'rest_no_route', + message: 'No route was found matching the URL and request method.', + } ); + fireEvent.click( screen.getAllByRole( 'checkbox' )[ 0 ] ); + // Plugin should translate to a helpful upgrade prompt instead of + // surfacing WordPress's generic message. (`a11y-speak` mirrors + // the notice into a screen-reader region too, hence getAllByText.) + await waitFor( () => expect( screen.getAllByText( /newer version of Newspack Newsletters/ ).length ).toBeGreaterThan( 0 ) ); + expect( screen.queryByText( /No route was found/ ) ).not.toBeInTheDocument(); + } ); + it( 'commits the active toggle immediately via PATCH /lists/{db_id}', async () => { render( ); await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); From 30fc56e4bbcc4a386b9db470898cce5b922e7622 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 15:17:20 +0100 Subject: [PATCH 20/26] fix(components,newsletters): hash-router guard + reload listener reattach (NEWS-2263) --- .../src/hooks/use-unsaved-changes-dialog.tsx | 8 +++- .../newsletters/views/settings/index.js | 41 ++++++++++++++----- .../newsletters/views/settings/index.test.js | 35 ++++++++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx index d3852c3183..45e5a72df7 100644 --- a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx +++ b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx @@ -81,7 +81,13 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { return; } const href = link.getAttribute( 'href' ); - if ( ! href || href.startsWith( '#' ) || href.startsWith( 'javascript:' ) ) { + if ( ! href || href.startsWith( 'javascript:' ) ) { + return; + } + // Allow plain in-page anchors (`#`, `#section`) — they're scroll + // targets, not navigation. Guard HashRouter paths (`#/something`) + // since they switch wizard views and drop the dirty form state. + if ( href.startsWith( '#' ) && ! href.startsWith( '#/' ) ) { return; } if ( link.target && link.target !== '_self' ) { diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 5541a18275..465da61d86 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -423,6 +423,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { // (registered below) flushes this; the fallback timer navigates to the // legacy URL if the bridge never mounts. const pendingActionRef = useRef( null ); + // Exposed by the reload-listener effect so other readiness paths (the + // fallback timer, the dispatchOrQueue happy-path) can force a re-resolve + // of listener names when the bridge becomes ready but its mounted-event + // rename means our `BRIDGE_MOUNTED` listener never fired. + const reattachReloadListenersRef = useRef( null ); const updateLists = updater => { setLists( prev => { @@ -522,6 +527,22 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { attach(); + const reattachIfChanged = () => { + const next = collectNames(); + const changed = [ ...next.saved ].some( n => ! saved.has( n ) ) || [ ...next.deleted ].some( n => ! deleted.has( n ) ); + if ( ! changed ) { + return; + } + detach(); + saved = next.saved; + deleted = next.deleted; + attach(); + }; + // Expose to other readiness paths (fallback timer / immediate + // dispatch) so they can also force a re-resolve when the bridge + // becomes ready by a path other than our BRIDGE_MOUNTED listener. + reattachReloadListenersRef.current = reattachIfChanged; + // If the bridge wasn't ready at mount, re-resolve event names when // it announces itself. The bridge dispatches BRIDGE_MOUNTED using // whatever names it exposes, so listen on both the fallback name @@ -537,21 +558,14 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { clearTimeout( fallbackTimerRef.current ); pending.dispatch(); } - const next = collectNames(); - const changed = [ ...next.saved ].some( n => ! saved.has( n ) ) || [ ...next.deleted ].some( n => ! deleted.has( n ) ); - if ( ! changed ) { - return; - } - detach(); - saved = next.saved; - deleted = next.deleted; - attach(); + reattachIfChanged(); }; bridgeMountedNames.forEach( name => document.addEventListener( name, onBridgeMounted ) ); return () => { detach(); bridgeMountedNames.forEach( name => document.removeEventListener( name, onBridgeMounted ) ); + reattachReloadListenersRef.current = null; }; }, [] ); @@ -575,6 +589,10 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { // double-dispatch it after this call. pendingActionRef.current = null; clearTimeout( fallbackTimerRef.current ); + // Bridge may have become ready via a renamed BRIDGE_MOUNTED we + // didn't observe — make sure our reload listeners are on the + // live names before the dispatch produces save/delete events. + reattachReloadListenersRef.current?.(); dispatch(); return; } @@ -592,8 +610,11 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { // Belt-and-braces: if the bridge IS ready but its BRIDGE_MOUNTED // event was renamed (so our listener missed it), flush the queue // instead of navigating — the dispatch will land on the live - // listeners that the readiness flag implies. + // listeners that the readiness flag implies. Also re-resolve + // reload-listener names so subsequent save/delete events from + // the bridge still trigger a list refresh. if ( isBridgeReady() ) { + reattachReloadListenersRef.current?.(); pending.dispatch(); return; } diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index ac00090137..f72a464340 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -224,6 +224,41 @@ describe( 'SubscriptionLists — wizard-bridge wiring', () => { jest.useRealTimers(); } ); + it( 'reattaches reload listeners to live event names when the bridge mounts via a renamed event missed by our handler', async () => { + delete window.newspackNewslettersBridgeReady; + jest.useFakeTimers(); + const renamedSavedListener = jest.fn(); + const fallbackSavedListener = jest.fn(); + const { default: apiFetchMock } = await import( '@wordpress/api-fetch' ); + render( ); + await waitFor( () => expect( apiFetchMock ).toHaveBeenCalled() ); + // Bridge appears with renamed events; our BRIDGE_MOUNTED handler + // won't fire because the new mounted-event name was unknown at + // mount time. The fallback-timer recovery path should re-resolve + // the reload listeners. + window.newspackNewslettersBridgeReady = true; + window.newspackNewslettersEvents = { + ...NN_EVENTS, + LOCAL_LIST_SAVED: 'custom:local-list-saved', + }; + // Click Add new local list to trigger the queue → fallback timer path. + fireEvent.click( screen.getByRole( 'button', { name: /Add new local list/ } ) ); + // Spy on a future bridge save event under the renamed name. + document.addEventListener( 'custom:local-list-saved', renamedSavedListener ); + document.addEventListener( NN_EVENTS.LOCAL_LIST_SAVED, fallbackSavedListener ); + // Fallback timer fires — flushes queue + reattaches reload listeners. + jest.advanceTimersByTime( 600 ); + // Bridge emits a save event under the renamed name; our reload + // listener must be on this live name now. + const reloadCallsBefore = apiFetchMock.mock.calls.length; + document.dispatchEvent( new CustomEvent( 'custom:local-list-saved' ) ); + // fetchLists should have been called again. + await waitFor( () => expect( apiFetchMock.mock.calls.length ).toBeGreaterThan( reloadCallsBefore ) ); + document.removeEventListener( 'custom:local-list-saved', renamedSavedListener ); + document.removeEventListener( NN_EVENTS.LOCAL_LIST_SAVED, fallbackSavedListener ); + jest.useRealTimers(); + } ); + it( 'flushes the queue on the fallback timeout when the bridge is ready but its mounted event was renamed', async () => { delete window.newspackNewslettersBridgeReady; jest.useFakeTimers(); From 14625e8f69b3843e1a6a854fa95895fbd372da71 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Tue, 19 May 2026 15:31:03 +0100 Subject: [PATCH 21/26] fix(newsletters): defer hash-router guard to ConfirmDialog; wrap tests in Router (NEWS-2263) --- .../src/hooks/use-unsaved-changes-dialog.tsx | 14 +++++++------- .../newsletters/views/settings/index.test.js | 14 ++++++++++---- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx index 45e5a72df7..10e8e956f5 100644 --- a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx +++ b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx @@ -81,13 +81,13 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { return; } const href = link.getAttribute( 'href' ); - if ( ! href || href.startsWith( 'javascript:' ) ) { - return; - } - // Allow plain in-page anchors (`#`, `#section`) — they're scroll - // targets, not navigation. Guard HashRouter paths (`#/something`) - // since they switch wizard views and drop the dirty form state. - if ( href.startsWith( '#' ) && ! href.startsWith( '#/' ) ) { + if ( ! href || href.startsWith( '#' ) || href.startsWith( 'javascript:' ) ) { + // All `#`-prefixed links are skipped here: plain anchors + // (`#section`) are scroll targets, and HashRouter paths + // (`#/route`) are intercepted by `ConfirmDialog`'s built-in + // `history.block` listener instead. Routing them through + // `window.location.href` here would trigger a hashchange + // that the still-active block re-catches, double-prompting. return; } if ( link.target && link.target !== '_self' ) { diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index f72a464340..7fd7ed5aef 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -1,10 +1,16 @@ import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; import apiFetch from '@wordpress/api-fetch'; import { dispatch, select } from '@wordpress/data'; +import { HashRouter } from 'react-router-dom'; import NewslettersSettings, { Settings, SubscriptionLists } from './index'; import { WIZARD_STORE_NAMESPACE } from '../../../../../packages/components/src/wizard/store'; +// NewslettersSettings mounts useUnsavedChangesDialog → useConfirmDialog → +// ConfirmDialog, which calls `useHistory()`. In production it runs inside +// Wizard's HashRouter; in tests we provide the same Router context. +const renderWithRouter = ui => render( { ui } ); + jest.mock( '@wordpress/api-fetch', () => jest.fn() ); const NN_EVENTS = { @@ -399,12 +405,12 @@ describe( 'NewslettersSettings — dirty tracking, save flow, snackbar', () => { const getSaveAction = () => select( WIZARD_STORE_NAMESPACE ).getHeaderData()?.actions?.[ 0 ]; it( 'registers a Save header action that is initially disabled (no dirty state)', async () => { - render( ); + renderWithRouter( ); await waitFor( () => expect( getSaveAction() ).toEqual( expect.objectContaining( { label: 'Save', disabled: true } ) ) ); } ); it( 'fires a success snackbar on a successful save', async () => { - render( ); + renderWithRouter( ); await waitFor( () => expect( getSaveAction() ).toBeDefined() ); // Save endpoint echoes the response on POST. apiFetch.mockResolvedValueOnce( SETTINGS_FIXTURE ); @@ -421,7 +427,7 @@ describe( 'NewslettersSettings — dirty tracking, save flow, snackbar', () => { // Unconfigured response — Tracking should stay hidden so it doesn't // hit the tracking endpoint on installs without the newsletters plugin. apiFetch.mockResolvedValue( { ...SETTINGS_FIXTURE, configured: false } ); - render( ); + renderWithRouter( ); await waitFor( () => expect( getSaveAction() ).toBeDefined() ); expect( screen.queryByText( /Ads tracking/i ) ).not.toBeInTheDocument(); } ); @@ -439,7 +445,7 @@ describe( 'NewslettersSettings — dirty tracking, save flow, snackbar', () => { } return Promise.resolve( SETTINGS_FIXTURE ); } ); - render( ); + renderWithRouter( ); await waitFor( () => expect( getSaveAction() ).toBeDefined() ); const pending = act( async () => { await getSaveAction().action(); From 6bd7cbc5b7205f9dfc36861c5b63f5e4a451763d Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 1 Jun 2026 19:23:16 +0100 Subject: [PATCH 22/26] fix(newsletters): re-render lists on ESP switch-back and notice on delete fallback --- .../newsletters/views/settings/index.js | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 465da61d86..75ebd093f5 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -78,6 +78,7 @@ export const Settings = ( { onLabels, onLetterheadSetting, newslettersConfig, + savedProvider = '', isOnboarding = true, authUrl = false, isSaving = false, @@ -100,8 +101,9 @@ export const Settings = ( { if ( provider !== newProvider ) { setError( false ); setProvider( newProvider ); - // Don't lock lists if we are setting the initial provider and a key is already set. - if ( ! provider && hasSelectedProviderKey() ) { + // Unlock when the selection is the saved provider (initial load, or + // switching back to it) and its key is set; otherwise lock until saved. + if ( ( ! provider || newProvider === savedProvider ) && hasSelectedProviderKey() ) { setLockedLists( false ); } else { setLockedLists( true ); @@ -581,7 +583,7 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { // bridge never shows up. The event KEY is stored (not the resolved // name) so a late-mounting bridge that exposes renamed events still // receives the correctly-named replay. - const dispatchOrQueue = ( eventKey, detail, fallbackUrl ) => { + const dispatchOrQueue = ( eventKey, detail, { fallbackUrl, onUnavailable } = {} ) => { const dispatch = () => document.dispatchEvent( new CustomEvent( getNNEvents()[ eventKey ], { detail } ) ); if ( isBridgeReady() ) { // Any prior queued action from before the bridge was ready is @@ -596,9 +598,9 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { dispatch(); return; } - pendingActionRef.current = { dispatch, fallbackUrl }; + pendingActionRef.current = { dispatch, fallbackUrl, onUnavailable }; clearTimeout( fallbackTimerRef.current ); - if ( ! fallbackUrl ) { + if ( ! fallbackUrl && ! onUnavailable ) { return; } fallbackTimerRef.current = setTimeout( () => { @@ -620,18 +622,29 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { } if ( pending.fallbackUrl ) { window.location.href = pending.fallbackUrl; + return; } + pending.onUnavailable?.(); }, NN_FALLBACK_TIMEOUT_MS ); }; + const bridgeUnavailableError = () => + setError( { + message: __( + 'This action requires a newer version of Newspack Newsletters. Update the newsletters plugin and try again.', + 'newspack-plugin' + ), + } ); + const dispatchOpenAdd = () => { - dispatchOrQueue( 'OPEN_MODAL', { mode: 'add' }, newspack_newsletters_wizard.new_subscription_lists_url ); + dispatchOrQueue( 'OPEN_MODAL', { mode: 'add' }, { fallbackUrl: newspack_newsletters_wizard.new_subscription_lists_url } ); }; const dispatchOpenEdit = ( list, kind ) => { - dispatchOrQueue( 'OPEN_MODAL', { mode: 'edit', kind, list }, list?.edit_link ); + dispatchOrQueue( 'OPEN_MODAL', { mode: 'edit', kind, list }, { fallbackUrl: list?.edit_link } ); }; const dispatchConfirmDelete = list => { - dispatchOrQueue( 'OPEN_CONFIRM_DELETE', { list }, list?.edit_link ); + // No safe legacy delete URL, so surface a notice instead of navigating. + dispatchOrQueue( 'OPEN_CONFIRM_DELETE', { list }, { onUnavailable: bridgeUnavailableError } ); }; if ( ! inFlight && ! lists?.length && ! error && ! lockedLists ) { @@ -849,6 +862,7 @@ const NewslettersSettings = () => { onLetterheadSetting={ handleLetterheadSetting } authUrl={ authUrl } newslettersConfig={ newslettersConfig } + savedProvider={ savedConfig?.newspack_newsletters_service_provider || '' } provider={ provider } setProvider={ setProvider } setAuthUrl={ setAuthUrl } From a366e00bd5a0c7652f35e3c274c0397a3cdd290f Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 1 Jun 2026 19:23:19 +0100 Subject: [PATCH 23/26] fix(components): structural single-consumer guard for unsaved-changes hook --- .../src/hooks/use-unsaved-changes-dialog.tsx | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx index 10e8e956f5..839866aaa3 100644 --- a/packages/components/src/hooks/use-unsaved-changes-dialog.tsx +++ b/packages/components/src/hooks/use-unsaved-changes-dialog.tsx @@ -13,12 +13,10 @@ type UseUnsavedChangesDialogOptions = { when: boolean; }; -// Module-level active-instance counter used only to warn in development when -// multiple consumers mount the same guard simultaneously. The click handler -// is document-level capture, so a second active instance would fire a second -// dialog on top of the first. Consumers should ensure only one instance is -// active at a time. -let activeInstances = 0; +// Stack of mounted guards; the last entry owns the document-level handler so +// only the most-recently-mounted guard prompts when several are active. Using a +// stack keeps ownership correct under out-of-order unmounts. +const activeGuards: symbol[] = []; /** * Returns true when `href` resolves to the same origin as the current page — @@ -63,8 +61,9 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { if ( ! when ) { return; } - activeInstances += 1; - if ( process.env.NODE_ENV !== 'production' && activeInstances > 1 ) { + const ownerId = Symbol( 'unsaved-changes-guard' ); + activeGuards.push( ownerId ); + if ( process.env.NODE_ENV !== 'production' && activeGuards.length > 1 ) { // eslint-disable-next-line no-console console.warn( 'useUnsavedChangesDialog: more than one active instance detected. ' + @@ -72,6 +71,10 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { ); } const handler = ( e: MouseEvent ) => { + // Only the top-of-stack guard prompts, so concurrent guards can't stack dialogs. + if ( activeGuards[ activeGuards.length - 1 ] !== ownerId ) { + return; + } if ( e.defaultPrevented || e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button !== 0 ) { return; } @@ -109,7 +112,10 @@ function useUnsavedChangesDialog( { when }: UseUnsavedChangesDialogOptions ) { document.addEventListener( 'click', handler, true ); return () => { document.removeEventListener( 'click', handler, true ); - activeInstances -= 1; + const idx = activeGuards.indexOf( ownerId ); + if ( idx !== -1 ) { + activeGuards.splice( idx, 1 ); + } }; }, [ when, requestConfirm ] ); From e946a4c9b129539db3405a80ccfef544fa3de49c Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Mon, 1 Jun 2026 19:23:20 +0100 Subject: [PATCH 24/26] fix(newsletters): drop stale tracking-admin remove_action --- includes/wizards/newsletters/class-newsletters-wizard.php | 1 - 1 file changed, 1 deletion(-) diff --git a/includes/wizards/newsletters/class-newsletters-wizard.php b/includes/wizards/newsletters/class-newsletters-wizard.php index 33425206d7..1126bc20eb 100644 --- a/includes/wizards/newsletters/class-newsletters-wizard.php +++ b/includes/wizards/newsletters/class-newsletters-wizard.php @@ -80,7 +80,6 @@ public function __construct() { // Menu removals. remove_action( 'admin_menu', [ Newspack_Newsletters_Settings::class, 'add_plugin_page' ] ); - remove_action( 'admin_menu', [ Newspack_Newsletters_Tracking_Admin::class, 'add_settings_page' ] ); // Customize the Newsletter ads menu titles. add_filter( From a076b3cbe39fa4af649410513a5d67ee8b2bf9f9 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Wed, 3 Jun 2026 19:29:21 +0100 Subject: [PATCH 25/26] fix(newsletters): sync subscription lists with ESP connection status --- .../newsletters/class-newsletters-wizard.php | 13 ++- .../newsletters/views/settings/index.js | 67 ++++++++++----- .../newsletters/views/settings/index.test.js | 84 +++++++++++++++++++ 3 files changed, 138 insertions(+), 26 deletions(-) diff --git a/includes/wizards/newsletters/class-newsletters-wizard.php b/includes/wizards/newsletters/class-newsletters-wizard.php index 1126bc20eb..87fd450162 100644 --- a/includes/wizards/newsletters/class-newsletters-wizard.php +++ b/includes/wizards/newsletters/class-newsletters-wizard.php @@ -241,9 +241,10 @@ function ( $acc, $value ) { } return [ - 'configured' => $newsletters_configuration_manager->is_configured(), - 'settings' => $settings, - 'labels' => $labels, + 'configured' => $newsletters_configuration_manager->is_configured(), + 'esp_connected' => (bool) $newsletters_configuration_manager->is_esp_set_up(), + 'settings' => $settings, + 'labels' => $labels, ]; } @@ -267,6 +268,12 @@ public function api_update_newsletters_settings( $request ) { $args = $request->get_params(); $newsletters_configuration_manager = Configuration_Managers::configuration_manager_class_for_plugin_slug( 'newspack-newsletters' ); $newsletters_configuration_manager->update_settings( $args ); + // The provider instance is memoized on `init`, before this save runs, so + // refresh it before reading credential status — otherwise a provider switch + // reports the previously-active provider's connection state. + if ( method_exists( 'Newspack_Newsletters', 'memoize_service_provider' ) ) { + \Newspack_Newsletters::memoize_service_provider(); + } return $this->api_get_newsletters_settings(); } diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index 75ebd093f5..ce89bfe6ae 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -72,6 +72,16 @@ import './style.scss'; const LETTERHEAD_KEY = 'newspack_newsletters_letterhead_api_key'; +// Signature over every credential field of a provider (keys sourced from the +// settings metadata), so any change — key, URL, secret — is detected. +const providerCredentialSignature = ( config, settings, provider ) => + Object.values( settings || {} ) + .filter( setting => setting?.provider && setting.provider === provider ) + .map( setting => setting.key ) + .sort() + .map( key => config?.[ key ] ?? '' ) + .join( '|' ); + export const Settings = ( { onUpdate, onConfigured, @@ -79,6 +89,8 @@ export const Settings = ( { onLetterheadSetting, newslettersConfig, savedProvider = '', + espConnected = false, + onEspConnected = () => {}, isOnboarding = true, authUrl = false, isSaving = false, @@ -101,15 +113,12 @@ export const Settings = ( { if ( provider !== newProvider ) { setError( false ); setProvider( newProvider ); - // Unlock when the selection is the saved provider (initial load, or - // switching back to it) and its key is set; otherwise lock until saved. - if ( ( ! provider || newProvider === savedProvider ) && hasSelectedProviderKey() ) { - setLockedLists( false ); - } else { - setLockedLists( true ); - } } - }, [ newslettersConfig?.newspack_newsletters_service_provider ] ); + // Unlock only the saved, connected provider (initial load or switch-back). + // Kept outside the provider-change guard so a late `espConnected` still applies. + const isSavedProvider = ! provider || newProvider === savedProvider; + setLockedLists( ! ( newProvider && isSavedProvider && espConnected ) ); + }, [ newslettersConfig?.newspack_newsletters_service_provider, provider, savedProvider, espConnected ] ); // Verify token for OAuth providers. useEffect( () => { verifyToken( newslettersConfig?.newspack_newsletters_service_provider ); @@ -157,6 +166,7 @@ export const Settings = ( { if ( onConfigured ) { onConfigured( response?.configured === true ); } + onEspConnected( response?.esp_connected === true ); if ( onLabels && response?.labels ) { onLabels( response.labels ); } @@ -171,15 +181,6 @@ export const Settings = ( { const value = configItem?.value; return configItem?.options?.find( option => option.value === value )?.name; }; - const hasSelectedProviderKey = () => { - const selectedProvider = newslettersConfig?.newspack_newsletters_service_provider; - if ( ! selectedProvider ) { - return false; - } - const regex = new RegExp( `${ selectedProvider }.*key` ); - const configKeys = Object.keys( newslettersConfig ).filter( key => regex.test( key ) ); - return configKeys.some( key => !! newslettersConfig[ key ] ); - }; const handleAuth = () => { if ( authUrl ) { const authWindow = window.open( authUrl, 'esp_oauth', 'width=500,height=600' ); @@ -414,7 +415,7 @@ export const Settings = ( { ); }; -export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = {} } ) => { +export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = {}, reloadToken = 0 } ) => { const [ error, setError ] = useState( false ); const [ inFlight, setInFlight ] = useState( false ); const [ togglingIds, setTogglingIds ] = useState( () => new Set() ); @@ -501,7 +502,7 @@ export const SubscriptionLists = ( { lockedLists, onUpdate, provider, labels = { setLists( [] ); fetchLists(); } - }, [ provider, lockedLists ] ); + }, [ provider, lockedLists, reloadToken ] ); useEffect( () => { const reload = () => fetchLists(); @@ -779,6 +780,8 @@ const NewslettersSettings = () => { const [ labels, setLabels ] = useState( {} ); const [ letterheadSetting, setLetterheadSetting ] = useState( null ); const [ isConfigured, setIsConfigured ] = useState( false ); + const [ espConnected, setEspConnected ] = useState( false ); + const [ listsReloadToken, setListsReloadToken ] = useState( 0 ); const { setHeaderData, addNotice } = useDispatch( WIZARD_STORE_NAMESPACE ); useEffect( () => { @@ -808,8 +811,22 @@ const NewslettersSettings = () => { method: 'POST', data: payload, } ); - setProvider( payload?.newspack_newsletters_service_provider ); - setLockedLists( false ); + const savedProviderValue = savedConfig?.newspack_newsletters_service_provider; + const nextProviderValue = payload?.newspack_newsletters_service_provider; + const connected = response?.esp_connected === true; + setProvider( nextProviderValue ); + setEspConnected( connected ); + setLockedLists( ! connected ); + // Same provider still connected but credentials changed (key rotation): + // nudge the lists to refetch, since provider and lock state didn't change. + if ( + connected && + nextProviderValue === savedProviderValue && + providerCredentialSignature( payload, response?.settings, nextProviderValue ) !== + providerCredentialSignature( savedConfig || {}, response?.settings, nextProviderValue ) + ) { + setListsReloadToken( token => token + 1 ); + } setSavedConfig( payload ); if ( response?.labels ) { setLabels( response.labels ); @@ -824,7 +841,7 @@ const NewslettersSettings = () => { } finally { setInFlight( false ); } - }, [ newslettersConfig, addNotice ] ); + }, [ newslettersConfig, savedConfig, addNotice ] ); useEffect( () => { setHeaderData( { @@ -863,12 +880,16 @@ const NewslettersSettings = () => { authUrl={ authUrl } newslettersConfig={ newslettersConfig } savedProvider={ savedConfig?.newspack_newsletters_service_provider || '' } + espConnected={ espConnected } + onEspConnected={ setEspConnected } provider={ provider } setProvider={ setProvider } setAuthUrl={ setAuthUrl } setLockedLists={ setLockedLists } /> - { provider !== 'manual' && } + { provider !== 'manual' && ( + + ) } { isConfigured && } { letterheadSetting && ( <> diff --git a/src/wizards/newsletters/views/settings/index.test.js b/src/wizards/newsletters/views/settings/index.test.js index 7fd7ed5aef..fa7bf6b555 100644 --- a/src/wizards/newsletters/views/settings/index.test.js +++ b/src/wizards/newsletters/views/settings/index.test.js @@ -432,6 +432,90 @@ describe( 'NewslettersSettings — dirty tracking, save flow, snackbar', () => { expect( screen.queryByText( /Ads tracking/i ) ).not.toBeInTheDocument(); } ); + it( 'locks the subscription lists and shows the warning when the saved provider has no key', async () => { + const configuredFixture = { + ...SETTINGS_FIXTURE, + esp_connected: true, + settings: { + ...SETTINGS_FIXTURE.settings, + newspack_newsletters_service_provider: { + ...SETTINGS_FIXTURE.settings.newspack_newsletters_service_provider, + value: 'mailchimp', + }, + newspack_newsletters_mailchimp_api_key: { + ...SETTINGS_FIXTURE.settings.newspack_newsletters_mailchimp_api_key, + value: 'abc-key', + }, + }, + }; + // The backend reports the ESP disconnected once the key is removed. + const unconfiguredFixture = { ...configuredFixture, esp_connected: false }; + apiFetch.mockImplementation( config => { + if ( config?.path === '/newspack-newsletters/v1/lists' ) { + return Promise.resolve( SUBSCRIPTION_LISTS_FIXTURE ); + } + if ( config?.method === 'POST' ) { + return Promise.resolve( unconfiguredFixture ); + } + return Promise.resolve( configuredFixture ); + } ); + renderWithRouter( ); + const ignore = 'script, style, .a11y-speak-region'; + // Lists load with a connected provider — no warning yet. + await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); + expect( screen.queryByText( /Please save your ESP settings/i, { ignore } ) ).not.toBeInTheDocument(); + + // Remove the API key, then save. + await act( async () => { + fireEvent.change( screen.getByLabelText( 'Mailchimp API Key' ), { target: { value: '' } } ); + } ); + await act( async () => { + await getSaveAction().action(); + } ); + + await waitFor( () => + expect( screen.getByText( /Please save your ESP settings before changing your subscription lists/i, { ignore } ) ).toBeInTheDocument() + ); + } ); + + it( 'reloads the subscription lists when the saved provider key is rotated', async () => { + const configuredFixture = { + ...SETTINGS_FIXTURE, + esp_connected: true, + settings: { + ...SETTINGS_FIXTURE.settings, + newspack_newsletters_service_provider: { + ...SETTINGS_FIXTURE.settings.newspack_newsletters_service_provider, + value: 'mailchimp', + }, + newspack_newsletters_mailchimp_api_key: { + ...SETTINGS_FIXTURE.settings.newspack_newsletters_mailchimp_api_key, + value: 'abc-key', + }, + }, + }; + apiFetch.mockImplementation( config => { + if ( config?.path === '/newspack-newsletters/v1/lists' ) { + return Promise.resolve( SUBSCRIPTION_LISTS_FIXTURE ); + } + return Promise.resolve( configuredFixture ); + } ); + const listCalls = () => apiFetch.mock.calls.filter( ( [ c ] ) => c?.path === '/newspack-newsletters/v1/lists' ).length; + renderWithRouter( ); + await waitFor( () => expect( screen.getByText( 'Local A' ) ).toBeInTheDocument() ); + const before = listCalls(); + + // Rotate the key to a different valid value, then save. + await act( async () => { + fireEvent.change( screen.getByLabelText( 'Mailchimp API Key' ), { target: { value: 'xyz-key' } } ); + } ); + await act( async () => { + await getSaveAction().action(); + } ); + + await waitFor( () => expect( listCalls() ).toBeGreaterThan( before ) ); + } ); + it( 'clears the dirty flag after save even if a fetch resolves during the request', async () => { // Captures the payload at save-call time so the saved snapshot reflects // what was actually sent, not a later edit. From 137215ff87fe59a96e361749f78a2cd7e97c5d17 Mon Sep 17 00:00:00 2001 From: Thomas Guillot Date: Wed, 3 Jun 2026 19:47:18 +0100 Subject: [PATCH 26/26] refactor(newsletters): rename signature helper to match its scope --- src/wizards/newsletters/views/settings/index.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wizards/newsletters/views/settings/index.js b/src/wizards/newsletters/views/settings/index.js index ce89bfe6ae..2f15b2efb9 100644 --- a/src/wizards/newsletters/views/settings/index.js +++ b/src/wizards/newsletters/views/settings/index.js @@ -72,9 +72,9 @@ import './style.scss'; const LETTERHEAD_KEY = 'newspack_newsletters_letterhead_api_key'; -// Signature over every credential field of a provider (keys sourced from the -// settings metadata), so any change — key, URL, secret — is detected. -const providerCredentialSignature = ( config, settings, provider ) => +// Signature over a provider's settings (keys sourced from the settings +// metadata), so any credential change — key, URL, secret — is detected. +const providerSettingsSignature = ( config, settings, provider ) => Object.values( settings || {} ) .filter( setting => setting?.provider && setting.provider === provider ) .map( setting => setting.key ) @@ -822,8 +822,8 @@ const NewslettersSettings = () => { if ( connected && nextProviderValue === savedProviderValue && - providerCredentialSignature( payload, response?.settings, nextProviderValue ) !== - providerCredentialSignature( savedConfig || {}, response?.settings, nextProviderValue ) + providerSettingsSignature( payload, response?.settings, nextProviderValue ) !== + providerSettingsSignature( savedConfig || {}, response?.settings, nextProviderValue ) ) { setListsReloadToken( token => token + 1 ); }