diff --git a/projects/packages/newsletter/changelog/change-newsletter-preview-edit-gating b/projects/packages/newsletter/changelog/change-newsletter-preview-edit-gating new file mode 100644 index 00000000000..f82f6adead8 --- /dev/null +++ b/projects/packages/newsletter/changelog/change-newsletter-preview-edit-gating @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Newsletter: Only show the subscription placement "Preview and edit" link once the placement is enabled and saved. diff --git a/projects/packages/newsletter/src/settings/newsletter-settings.tsx b/projects/packages/newsletter/src/settings/newsletter-settings.tsx index 5a5fc3c4ab7..e80272d91e9 100644 --- a/projects/packages/newsletter/src/settings/newsletter-settings.tsx +++ b/projects/packages/newsletter/src/settings/newsletter-settings.tsx @@ -66,15 +66,26 @@ function normalizeSettings( settings: Record< string, unknown > ): NewsletterSet // without the flash. let cachedSettings: NewsletterSettings | null = null; +// Module-level cache of the last *persisted* settings — the saved baseline, +// as opposed to `cachedSettings` above which mirrors the optimistic (possibly +// unsaved) `data`. Kept in sync on fetch and after every successful save, and +// module-cached for the same reason: so a remount on tab-return knows the +// saved state immediately instead of treating it as unknown until the +// background refetch resolves. Consumed by the Subscriptions section to gate +// each placement's "Preview and edit" link on whether that placement is +// enabled *in the saved state* (an unsaved toggle previews nothing). +let cachedSavedSettings: NewsletterSettings | null = null; + /** - * Test-only: clear the module-level settings cache so each test starts cold. - * The cache deliberately outlives any single component instance (that's the - * whole point — it survives the tab unmount/remount), which also means it - * leaks across tests in a file. Call this in `beforeEach` to keep cold-cache + * Test-only: clear the module-level settings caches so each test starts cold. + * The caches deliberately outlive any single component instance (that's the + * whole point — they survive the tab unmount/remount), which also means they + * leak across tests in a file. Call this in `beforeEach` to keep cold-cache * assertions order-independent. */ export function __resetNewsletterSettingsCacheForTests(): void { cachedSettings = null; + cachedSavedSettings = null; } export type NewsletterSettingsBodyProps = { @@ -136,6 +147,11 @@ export function NewsletterSettingsBody( { // Seed from the module cache so a remount (e.g. returning to the Settings // tab) paints immediately instead of flashing the full-page spinner. const [ data, setData ] = useState< NewsletterSettings | null >( () => cachedSettings ); + // Last persisted settings (saved baseline), seeded from the module cache so a + // remount knows the saved state without waiting on the background refetch. + const [ savedData, setSavedData ] = useState< NewsletterSettings | null >( + () => cachedSavedSettings + ); const [ isLoading, setIsLoading ] = useState( () => ! cachedSettings ); const [ error, setError ] = useState< string | null >( null ); @@ -203,7 +219,10 @@ export function NewsletterSettingsBody( { useEffect( () => { fetchSettings() .then( ( settings: Record< string, unknown > ) => { - setData( normalizeSettings( settings ) ); + const normalized = normalizeSettings( settings ); + setData( normalized ); + // Server truth on load is both the optimistic value and the saved baseline. + setSavedData( normalized ); setIsLoading( false ); } ) .catch( ( err: Error ) => { @@ -238,6 +257,14 @@ export function NewsletterSettingsBody( { } }, [ data ] ); + // Mirror the saved baseline to the module cache so it, too, survives a + // remount (parallel to `cachedSettings` above, but for persisted values). + useEffect( () => { + if ( savedData ) { + cachedSavedSettings = savedData; + } + }, [ savedData ] ); + // Handle auto-save for newsletter toggle and email settings. const handleAutoSave = useCallback( ( updates: Partial< NewsletterSettings > ) => { @@ -277,6 +304,8 @@ export function NewsletterSettingsBody( { // Save to backend. updateSettings( updates ) .then( () => { + // Advance the saved baseline now that these values are persisted. + setSavedData( prev => ( { ...prev, ...updates } ) ); createSuccessNotice( __( 'Settings saved', 'jetpack-newsletter' ) ); } ) .catch( ( err: Error ) => { @@ -308,6 +337,7 @@ export function NewsletterSettingsBody( { updateSettings( senderNameChanges ) .then( () => { + setSavedData( prev => ( { ...prev, ...senderNameChanges } ) ); setSenderNameChanges( {} ); createSuccessNotice( __( 'Sender name saved', 'jetpack-newsletter' ) ); } ) @@ -341,6 +371,9 @@ export function NewsletterSettingsBody( { updateSettings( subscriptionChanges ) .then( () => { + // Advance the saved baseline so each placement's "Preview and edit" + // link reflects the just-saved enabled state. + setSavedData( prev => ( { ...prev, ...subscriptionChanges } ) ); setSubscriptionChanges( {} ); createSuccessNotice( __( 'Settings saved', 'jetpack-newsletter' ) ); } ) @@ -395,6 +428,8 @@ export function NewsletterSettingsBody( { updateSettings( apiUpdates ) .then( () => { + // Merge the staged (string) values so the baseline mirrors `data`'s shape. + setSavedData( prev => ( { ...prev, ...newsletterCategoriesChanges } ) ); setNewsletterCategoriesChanges( {} ); createSuccessNotice( __( 'Newsletter categories saved', 'jetpack-newsletter' ) ); } ) @@ -428,6 +463,7 @@ export function NewsletterSettingsBody( { updateSettings( welcomeEmailChanges ) .then( () => { + setSavedData( prev => ( { ...prev, ...welcomeEmailChanges } ) ); setWelcomeEmailChanges( {} ); createSuccessNotice( __( 'Welcome email message saved', 'jetpack-newsletter' ) ); } ) @@ -459,6 +495,7 @@ export function NewsletterSettingsBody( { updateSettings( subscribeModalChanges ) .then( () => { + setSavedData( prev => ( { ...prev, ...subscribeModalChanges } ) ); setSubscribeModalChanges( {} ); createSuccessNotice( __( 'Subscribe modal heading saved', 'jetpack-newsletter' ) ); } ) @@ -542,6 +579,7 @@ export function NewsletterSettingsBody( { = { interface SubscriptionsSectionProps { data: NewsletterSettings; + /** + * Last *persisted* settings. A placement's "Preview and edit" link only + * shows when the placement is enabled here (saved), not merely toggled on + * in the optimistic `data` — otherwise the link opens an empty preview. + */ + savedData: NewsletterSettings | null; onChange: ( updates: Partial< NewsletterSettings > ) => void; onSave: () => void; isSaving: boolean; @@ -60,6 +66,7 @@ interface SubscriptionsSectionProps { */ export function SubscriptionsSection( { data, + savedData, onChange, onSave, isSaving, @@ -230,20 +237,32 @@ export function SubscriptionsSection( { { __( 'Homepage and posts', 'jetpack-newsletter' ) }
- { placements.map( placement => ( - - ) ) } + { placements.map( placement => { + // Surface "Preview and edit" only when the placement is + // enabled in the SAVED state AND still shown as checked — + // otherwise the link opens an empty Site Editor preview. + // Gating on `savedData` (the persisted baseline) rather than + // "untouched since save" keeps the link reversible: toggle a + // saved-on placement off then back on and it returns, while a + // freshly enabled-but-unsaved placement stays linkless until + // the save lands. + const isEnabledAndSaved = + Boolean( data[ placement.key ] ) && Boolean( savedData?.[ placement.key ] ); + return ( + + ); + } ) }
diff --git a/projects/packages/newsletter/tests/subscriptions-section.test.tsx b/projects/packages/newsletter/tests/subscriptions-section.test.tsx new file mode 100644 index 00000000000..1ad27975d7d --- /dev/null +++ b/projects/packages/newsletter/tests/subscriptions-section.test.tsx @@ -0,0 +1,232 @@ +/** + * Tests for the "Preview and edit" link gating in the Subscriptions section. + * + * `@wordpress/ui` / `@wordpress/components` are stubbed to plain HTML so we can + * assert on the rendered links directly. The component's own visibility logic — + * show the per-placement link only when the placement is enabled AND saved, and + * only when the site supports the Site Editor link — is exercised against the + * real source. + */ + +jest.mock( '@wordpress/ui', () => ( { + __esModule: true, + Button: ( { + children, + onClick, + disabled, + }: { + children: React.ReactNode; + onClick?: () => void; + disabled?: boolean; + } ) => ( + + ), + Card: { + Root: ( { children }: { children: React.ReactNode } ) =>
{ children }
, + Header: ( { children }: { children: React.ReactNode } ) =>
{ children }
, + Title: ( { children }: { children: React.ReactNode } ) =>

{ children }

, + Content: ( { children }: { children: React.ReactNode } ) =>
{ children }
, + }, + Fieldset: { + Root: ( { children }: { children: React.ReactNode } ) =>
{ children }
, + }, + Link: ( { + children, + href, + }: { + children: React.ReactNode; + href?: string; + onClick?: () => void; + } ) => { children }, + Stack: ( { children }: { children: React.ReactNode } ) =>
{ children }
, + Text: ( { children, id }: { children: React.ReactNode; id?: string } ) => ( + { children } + ), +} ) ); + +jest.mock( '@wordpress/components', () => ( { + __esModule: true, + CheckboxControl: ( { + id, + checked, + onChange, + }: { + id?: string; + checked?: boolean; + onChange?: ( next: boolean ) => void; + } ) => ( + onChange?.( e.target.checked ) } + /> + ), + ToggleControl: ( { label }: { label: React.ReactNode } ) =>
{ label }
, +} ) ); + +jest.mock( '@automattic/jetpack-analytics', () => ( { + __esModule: true, + default: { + tracks: { recordEvent: jest.fn() }, + }, +} ) ); + +jest.mock( '@automattic/jetpack-script-data', () => ( { + getSiteType: jest.fn( () => 'jetpack' ), + getAdminUrl: jest.fn( ( p: string ) => `https://example.com/wp-admin/${ p }` ), +} ) ); + +jest.mock( '../src/settings/script-data', () => ( { + getNewsletterScriptData: jest.fn( () => ( { + isBlockTheme: true, + themeStylesheet: 'twentytwentyfive', + isSubscriptionSiteEditSupported: true, + } ) ), +} ) ); + +import { render, screen } from '@testing-library/react'; +import { getNewsletterScriptData } from '../src/settings/script-data'; +import { SubscriptionsSection } from '../src/settings/sections/subscriptions-section'; +import type { NewsletterSettings } from '../src/settings/types'; + +const OVERLAY_KEY = 'jetpack_subscribe_overlay_enabled'; + +/** + * Build a `NewsletterSettings`-shaped object with all placement keys present so + * tests can flip a single placement on/off without leaving the rest undefined. + * + * @param overrides - Subset of settings to merge over the defaults. + * @return Settings object ready to pass to ``. + */ +function buildData( overrides: Partial< NewsletterSettings > = {} ): NewsletterSettings { + return { + jetpack_subscribe_overlay_enabled: false, + sm_enabled: false, + jetpack_subscriptions_subscribe_post_end_enabled: false, + jetpack_subscribe_floating_button_enabled: false, + ...overrides, + } as NewsletterSettings; +} + +/** + * Render `` with sensible defaults so each test only has + * to override the props it cares about. + * + * @param props - Prop overrides to apply on top of the defaults. + * @return RTL render helpers. + */ +function renderSection( + props: Partial< React.ComponentProps< typeof SubscriptionsSection > > = {} +) { + const data = ( props.data as NewsletterSettings ) ?? buildData(); + // Default the saved baseline to the displayed data, so a test that only sets + // `data` exercises the "enabled + saved" case. Tests that need divergence + // (enabled-but-unsaved, reverted-to-saved) pass `savedData` explicitly. + const savedData = ( props.savedData as NewsletterSettings | null | undefined ) ?? data; + return render( + + ); +} + +/** + * Count the placement-grid "Preview and edit" links. + * + * The Navigation subgroup renders its own "Preview and edit" links — gated on + * site capability, not on this fix — and both target the `//index` template. + * Placement links never point at `//index`, so filtering those out isolates + * the grid links this fix controls. + * + * @return The placement-grid "Preview and edit" anchors. + */ +function previewLinks(): HTMLAnchorElement[] { + return ( + screen.queryAllByRole( 'link', { name: 'Preview and edit' } ) as HTMLAnchorElement[] + ).filter( link => ! link.getAttribute( 'href' )?.includes( '%2F%2Findex' ) ); +} + +describe( 'SubscriptionsSection — Preview and edit gating', () => { + beforeEach( () => { + jest.clearAllMocks(); + } ); + + it( 'shows the link for a placement that is enabled and saved', () => { + renderSection( { + data: buildData( { [ OVERLAY_KEY ]: true } ), + savedData: buildData( { [ OVERLAY_KEY ]: true } ), + } ); + expect( previewLinks() ).toHaveLength( 1 ); + } ); + + it( 'hides the link for a disabled placement', () => { + renderSection( { + data: buildData( { [ OVERLAY_KEY ]: false } ), + savedData: buildData( { [ OVERLAY_KEY ]: false } ), + } ); + expect( previewLinks() ).toHaveLength( 0 ); + } ); + + it( 'hides the link for an enabled placement that is not yet saved', () => { + renderSection( { + // Toggled on optimistically, but the saved baseline still has it off. + data: buildData( { [ OVERLAY_KEY ]: true } ), + savedData: buildData( { [ OVERLAY_KEY ]: false } ), + } ); + expect( previewLinks() ).toHaveLength( 0 ); + } ); + + it( 'keeps the link when a saved-on placement is toggled off then back on (reverted to saved)', () => { + // The regression this fix targets: gating on the saved baseline, not on + // "touched since save", so reverting an edit restores the link without a + // round-trip to the server. `changedKeys` still reports the key as dirty. + renderSection( { + data: buildData( { [ OVERLAY_KEY ]: true } ), + savedData: buildData( { [ OVERLAY_KEY ]: true } ), + changedKeys: [ OVERLAY_KEY ], + } ); + expect( previewLinks() ).toHaveLength( 1 ); + } ); + + it( 'hides the link when the site does not support the Site Editor link', () => { + ( getNewsletterScriptData as jest.Mock ).mockReturnValueOnce( { + isBlockTheme: false, + themeStylesheet: '', + isSubscriptionSiteEditSupported: false, + } ); + renderSection( { + data: buildData( { [ OVERLAY_KEY ]: true } ), + savedData: buildData( { [ OVERLAY_KEY ]: true } ), + } ); + expect( previewLinks() ).toHaveLength( 0 ); + } ); + + it( 'shows links only for the saved-enabled placements in a mixed grid', () => { + renderSection( { + data: buildData( { + jetpack_subscribe_overlay_enabled: true, // enabled + saved -> link + sm_enabled: true, // enabled but unsaved -> no link + jetpack_subscribe_floating_button_enabled: true, // enabled + saved -> link + } ), + savedData: buildData( { + jetpack_subscribe_overlay_enabled: true, + sm_enabled: false, // baseline still off + jetpack_subscribe_floating_button_enabled: true, + } ), + } ); + expect( previewLinks() ).toHaveLength( 2 ); + } ); +} );