From 876d0a9a1baa26480e399d52199d62d8a8f6bed5 Mon Sep 17 00:00:00 2001 From: Derrick Koo Date: Fri, 19 Apr 2024 09:20:44 -0600 Subject: [PATCH] fix: minor bugs in reCAPTCHA + Campaigns admin UIs (#3073) * fix: minor bugs in reCAPTCHA + Campaigns admin UIs * test: fix failing JS test --- .../connections/views/main/recaptcha.js | 17 ++-- .../segment-group/SegmentGroup.test.js | 86 ++----------------- assets/wizards/popups/utils.js | 21 ++--- includes/class-recaptcha.php | 5 ++ 4 files changed, 27 insertions(+), 102 deletions(-) diff --git a/assets/wizards/connections/views/main/recaptcha.js b/assets/wizards/connections/views/main/recaptcha.js index d120e94e18..57ad81c52f 100644 --- a/assets/wizards/connections/views/main/recaptcha.js +++ b/assets/wizards/connections/views/main/recaptcha.js @@ -45,14 +45,13 @@ const Recaptcha = () => { setError( null ); setIsLoading( true ); try { - setSettings( - await apiFetch( { - path: '/newspack/v1/recaptcha', - method: 'POST', - data, - } ) - ); - setSettingsToUpdate( {} ); + const newSettings = await apiFetch( { + path: '/newspack/v1/recaptcha', + method: 'POST', + data, + } ); + setSettings( newSettings ); + setSettingsToUpdate( newSettings ); } catch ( e ) { setError( e?.message || __( 'Error updating settings.', 'newspack-plugin' ) ); } finally { @@ -129,7 +128,7 @@ const Recaptcha = () => { step="0.05" min="0" max="1" - value={ settingsToUpdate?.threshold || '' } + value={ parseFloat( settingsToUpdate?.threshold || '' ) } label={ __( 'Threshold', 'newspack-plugin' ) } onChange={ value => setSettingsToUpdate( { ...settingsToUpdate, threshold: value } ) diff --git a/assets/wizards/popups/components/segment-group/SegmentGroup.test.js b/assets/wizards/popups/components/segment-group/SegmentGroup.test.js index e340da909f..9f31bd06ef 100644 --- a/assets/wizards/popups/components/segment-group/SegmentGroup.test.js +++ b/assets/wizards/popups/components/segment-group/SegmentGroup.test.js @@ -307,12 +307,12 @@ describe( 'A segment with conflicting prompts', () => { }; } ); - it( 'renders a conflict notice for uncategorized overlays in the same segment', async () => { + it( 'renders a conflict notice for mutliple overlays in the same segment', async () => { SEGMENT.prompts = PROMPTS.overlaysUncategorized; // Expected warning text. const noticeText = - 'If multiple uncategorized overlays share the same segment, only the most recent one will be displayed.'; + 'If multiple overlays are rendered on the same pageview, only the most recent one will be displayed.'; const props = { campaignData: CAMPAIGN.campaignData, campaign: CAMPAIGN.campaignId, @@ -331,36 +331,12 @@ describe( 'A segment with conflicting prompts', () => { ); } ); - it( 'renders a conflict notice for categorized overlays in the same segment', async () => { - SEGMENT.prompts = PROMPTS.overlaysCategorized; - - // Expected warning text. - const noticeText = - 'If multiple overlays share the same segment and category filtering, only the most recent one will be displayed.'; - const props = { - campaignData: CAMPAIGN.campaignData, - campaign: CAMPAIGN.campaignId, - segment: SEGMENT, - }; - const { getByTestId } = render( ); - const notice1 = getByTestId( 'conflict-warning-3' ); - const notice2 = getByTestId( 'conflict-warning-4' ); - - // Notice text has expected shape. - expect( notice1.textContent ).toEqual( - `${ PROMPTS.overlaysCategorized[ 1 ].title }: ${ noticeText }` - ); - expect( notice2.textContent ).toEqual( - `${ PROMPTS.overlaysCategorized[ 0 ].title }: ${ noticeText }` - ); - } ); - - it( 'renders a conflict notice for uncategorized above-header prompts in the same segment', async () => { + it( 'renders a conflict notice for multiple above-header prompts in the same segment', async () => { SEGMENT.prompts = PROMPTS.aboveHeadersUncategorized; // Expected warning text. const noticeText = - 'If multiple uncategorized above-header prompts share the same segment, only the most recent one will be displayed.'; + 'If multiple above-header prompts are rendered on the same pageview, only the most recent one will be displayed.'; const props = { campaignData: CAMPAIGN.campaignData, campaign: CAMPAIGN.campaignId, @@ -379,36 +355,12 @@ describe( 'A segment with conflicting prompts', () => { ); } ); - it( 'renders a conflict notice for above-header prompts in the same segment', async () => { - SEGMENT.prompts = PROMPTS.aboveHeadersCategorized; - - // Expected warning text. - const noticeText = - 'If multiple above-header prompts share the same segment and category filtering, only the most recent one will be displayed.'; - const props = { - campaignData: CAMPAIGN.campaignData, - campaign: CAMPAIGN.campaignId, - segment: SEGMENT, - }; - const { getByTestId } = render( ); - const notice1 = getByTestId( 'conflict-warning-7' ); - const notice2 = getByTestId( 'conflict-warning-8' ); - - // Notice text has expected shape. - expect( notice1.textContent ).toEqual( - `${ PROMPTS.aboveHeadersCategorized[ 1 ].title }: ${ noticeText }` - ); - expect( notice2.textContent ).toEqual( - `${ PROMPTS.aboveHeadersCategorized[ 0 ].title }: ${ noticeText }` - ); - } ); - - it( 'renders a conflict notice for uncategorized prompts in the same custom placement and segment', async () => { + it( 'renders a conflict notice for multiple prompts in the same custom placement and segment', async () => { SEGMENT.prompts = PROMPTS.customPlacementsUncategorized; // Expected warning text. const noticeText = - 'If multiple uncategorized prompts in the same custom placement share the same segment, only the most recent one will be displayed.'; + 'If multiple prompts are rendered in the same custom placement, only the most recent one will be displayed.'; const props = { campaignData: CAMPAIGN.campaignData, campaign: CAMPAIGN.campaignId, @@ -427,30 +379,6 @@ describe( 'A segment with conflicting prompts', () => { ); } ); - it( 'renders a conflict notice for categorized prompts in the same custom placement and segment', async () => { - SEGMENT.prompts = PROMPTS.customPlacementsCategorized; - - // Expected warning text. - const noticeText = - 'If multiple prompts in the same custom placement share the same segment and category filtering, only the most recent one will be displayed.'; - const props = { - campaignData: CAMPAIGN.campaignData, - campaign: CAMPAIGN.campaignId, - segment: SEGMENT, - }; - const { getByTestId } = render( ); - const notice1 = getByTestId( 'conflict-warning-11' ); - const notice2 = getByTestId( 'conflict-warning-12' ); - - // Notice text has expected shape. - expect( notice1.textContent ).toEqual( - `${ PROMPTS.customPlacementsCategorized[ 1 ].title }: ${ noticeText }` - ); - expect( notice2.textContent ).toEqual( - `${ PROMPTS.customPlacementsCategorized[ 0 ].title }: ${ noticeText }` - ); - } ); - it( 'renders a conflict notice for conflicting prompts that have no segment', async () => { const everyone = { configuration: {}, @@ -464,7 +392,7 @@ describe( 'A segment with conflicting prompts', () => { // Expected warning text. const noticeText = - 'If multiple uncategorized overlays share the same segment, only the most recent one will be displayed.'; + 'If multiple overlays are rendered on the same pageview, only the most recent one will be displayed.'; const props = { campaignData: CAMPAIGN.campaignData, campaign: CAMPAIGN.campaignId, diff --git a/assets/wizards/popups/utils.js b/assets/wizards/popups/utils.js index d48e3edf1e..9469e9b45c 100644 --- a/assets/wizards/popups/utils.js +++ b/assets/wizards/popups/utils.js @@ -380,31 +380,24 @@ const sharesSegments = ( segmentsA, segmentsB ) => { ); }; -export const buildWarning = ( prompt, promptCategories ) => { +export const buildWarning = prompt => { if ( isOverlay( prompt ) || isAboveHeader( prompt ) ) { return sprintf( - // Translators: %1: 'uncetegorized' if no categories. %2: 'above-header prompts' if above header, 'overlays' otherwise. %3: 'and category filtering' if categories. + // Translators: %s is prompt type (above-header or overlay). __( - 'If multiple%1$s %2$s share the same segment%3$s, only the most recent one will be displayed.', + 'If multiple %s are rendered on the same pageview, only the most recent one will be displayed.', 'newspack-plugin' ), - 0 === promptCategories.length ? __( ' uncategorized', 'newspack-plugin' ) : '', isAboveHeader( prompt ) ? __( 'above-header prompts', 'newspack-plugin' ) - : __( 'overlays', 'newspack-plugin' ), - 0 < promptCategories.length ? __( ' and category filtering', 'newspack-plugin' ) : '' + : __( 'overlays', 'newspack-plugin' ) ); } if ( isCustomPlacement( prompt ) ) { - return sprintf( - // Translators: %1: 'uncetegorized' if no categories. %2: 'and category filtering' if categories. - __( - 'If multiple%1$s prompts in the same custom placement share the same segment%2$s, only the most recent one will be displayed.', - 'newspack-plugin' - ), - 0 === promptCategories.length ? __( ' uncategorized', 'newspack-plugin' ) : '', - 0 < promptCategories.length ? __( ' and category filtering', 'newspack-plugin' ) : '' + return __( + 'If multiple prompts are rendered in the same custom placement, only the most recent one will be displayed.', + 'newspack-plugin' ); } diff --git a/includes/class-recaptcha.php b/includes/class-recaptcha.php index 294229abd4..8268a655b8 100644 --- a/includes/class-recaptcha.php +++ b/includes/class-recaptcha.php @@ -194,10 +194,15 @@ public static function get_setting( $key ) { return null; } $value = \get_option( self::OPTIONS_PREFIX . $key, $config[ $key ] ); + // Use default value type for casting bool option value. if ( is_bool( $config[ $key ] ) ) { $value = (bool) $value; + } elseif ( empty( $value ) ) { + // If the stored value is empty, use the default value. + $value = $config[ $key ]; } + return $value; }