Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the placement of the feature flag check for Ad Blocking Recovery settings #7203

Merged
merged 7 commits into from
Jun 27, 2023
Merged

Change the placement of the feature flag check for Ad Blocking Recovery settings #7203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9fa3d85
Change the placement of the feature flag check for Ad Blocking Recove…
tofumatt Jun 23, 2023
c23ded8
Remove unneeded feature flag from stories.
tofumatt Jun 26, 2023
05344e4
Remove unused feature flag from test.
tofumatt Jun 26, 2023
ae0d7c0
Remove unused feature flags from tests/stories.
tofumatt Jun 26, 2023
1e552b8
Merge branch 'develop' into fix/7179.
nfmohit Jun 26, 2023
94ecff3
Remove remaining unnecessary checks for `adBlockerDetection` in tests…
nfmohit Jun 26, 2023
8fc8d6e
Guard ABR elements in SettingsView.
aaemnnosttv Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions assets/js/modules/adsense/components/common/AdBlockingRecoveryCTA.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,9 @@ import { CORE_SITE } from '../../../../googlesitekit/datastore/site/constants';
import SupportLink from '../../../../components/SupportLink';
import Badge from '../../../../components/Badge';
import { ACCOUNT_STATUS_READY, SITE_STATUS_READY } from '../../util';
import { useFeature } from '../../../../hooks/useFeature';
const { useSelect } = Data;

export default function AdBlockingRecoveryCTA() {
const adBlockerDetectionEnabled = useFeature( 'adBlockerDetection' );
const adBlockingRecoverySetupStatus = useSelect( ( select ) =>
select( MODULES_ADSENSE ).getAdBlockingRecoverySetupStatus()
);
Expand All @@ -52,7 +50,6 @@ export default function AdBlockingRecoveryCTA() {
);

if (
! adBlockerDetectionEnabled ||
adBlockingRecoverySetupStatus !== '' ||
accountStatus !== ACCOUNT_STATUS_READY ||
siteStatus !== SITE_STATUS_READY
Expand Down
3 changes: 0 additions & 3 deletions assets/js/modules/adsense/components/common/AdBlockingRecoveryCTA.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ Ready.storyName = 'Ready';
Ready.scenario = {
label: 'Global/AdBlockingRecoveryCTA/Ready',
};
Ready.parameters = {
features: [ 'adBlockerDetection' ],
};

export default {
title: 'Modules/AdSense/Components/AdBlockingRecoveryCTA',
Expand Down
16 changes: 1 addition & 15 deletions assets/js/modules/adsense/components/common/AdBlockingRecoveryCTA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,26 +34,17 @@ import {

describe( 'AdBlockingRecoveryCTA', () => {
it.each( [
[
'the Ad blocker detection feature flag is not enabled',
ACCOUNT_STATUS_PENDING,
SITE_STATUS_READY,
'',
false,
],
[
'Adsense account status is not ready',
ACCOUNT_STATUS_PENDING,
SITE_STATUS_READY,
'',
true,
],
[
'Adsense site status is not ready',
ACCOUNT_STATUS_READY,
SITE_STATUS_ADDED,
'',
true,
],
[
'Ad blocking recovery status is not an empty string',
Expand All @@ -68,13 +59,9 @@ describe( 'AdBlockingRecoveryCTA', () => {
testName,
accountStatus,
siteStatus,
adBlockingRecoverySetupStatus,
adBlockerDetectionEnabled
adBlockingRecoverySetupStatus
) => {
const { container } = render( <AdBlockingRecoveryCTA />, {
features: [].concat(
adBlockerDetectionEnabled ? 'adBlockerDetection' : []
),
setupRegistry: ( registry ) => {
provideModules( registry, [
{
Expand Down Expand Up @@ -105,7 +92,6 @@ describe( 'AdBlockingRecoveryCTA', () => {

it( 'should render the CTA when Ad Blocking Recovery is not set up', () => {
const { container } = render( <AdBlockingRecoveryCTA />, {
features: [ 'adBlockerDetection' ],
setupRegistry: ( registry ) => {
provideModules( registry, [
{
Expand Down
5 changes: 1 addition & 4 deletions assets/js/modules/adsense/components/common/AdBlockingRecoveryToggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,10 @@ import {
} from '../../datastore/constants';
import Link from '../../../../components/Link';
import SettingsNotice from '../../../../components/SettingsNotice/SettingsNotice';
import { useFeature } from '../../../../hooks/useFeature';
import { CORE_FORMS } from '../../../../googlesitekit/datastore/forms/constants';
const { useSelect, useDispatch } = Data;

export default function AdBlockingRecoveryToggle() {
const adBlockerDetectionEnabled = useFeature( 'adBlockerDetection' );

const adBlockerDetectionSnippet = useSelect( ( select ) =>
select( MODULES_ADSENSE ).getUseAdBlockerDetectionSnippet()
);
Expand Down Expand Up @@ -108,7 +105,7 @@ export default function AdBlockingRecoveryToggle() {
setValues( AD_BLOCKING_FORM_SETTINGS, initialToggleValues );
} );

if ( ! adBlockerDetectionEnabled || ! adBlockingRecoverySetupStatus ) {
if ( ! adBlockingRecoverySetupStatus ) {
return null;
}

Expand Down
9 changes: 0 additions & 9 deletions assets/js/modules/adsense/components/common/AdBlockingRecoveryToggle.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@ Ready.args = {
.receiveGetSettings( validSettings );
},
};
Ready.parameters = {
features: [ 'adBlockerDetection' ],
};

export const WithAdBlockingRecoveryTagEnabled = Template.bind( {} );
WithAdBlockingRecoveryTagEnabled.storyName =
Expand All @@ -65,9 +62,6 @@ WithAdBlockingRecoveryTagEnabled.args = {
} );
},
};
WithAdBlockingRecoveryTagEnabled.parameters = {
features: [ 'adBlockerDetection' ],
};

export const WithBothTogglesEnabled = Template.bind( {} );
WithBothTogglesEnabled.storyName = 'With Both The Toggles Enabled';
Expand All @@ -80,9 +74,6 @@ WithBothTogglesEnabled.args = {
} );
},
};
WithBothTogglesEnabled.parameters = {
features: [ 'adBlockerDetection' ],
};

export default {
title: 'Modules/AdSense/Components/AdBlockingRecoveryToggle',
Expand Down
56 changes: 22 additions & 34 deletions assets/js/modules/adsense/components/common/AdBlockingRecoveryToggle.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,45 +41,35 @@ describe( 'AdBlockingRecoveryToggle', () => {
adBlockingRecoverySetupStatus: '',
};

it.each( [
[ 'the Ad blocker detection feature flag is not enabled', false ],
[ 'Ad blocker recovery setup status is empty', true ],
] )(
'should not render the Ad blocking recovery toggle when %s',
( testName, adBlockerDetectionEnabled ) => {
const { container } = render( <AdBlockingRecoveryToggle />, {
features: [].concat(
adBlockerDetectionEnabled ? 'adBlockerDetection' : []
),
setupRegistry: ( registry ) => {
provideModules( registry, [
{
slug: 'adsense',
active: true,
connected: true,
},
] );
registry
.dispatch( MODULES_ADSENSE )
.receiveGetSettings( validSettings );
},
} );
it( 'should not render the Ad blocking recovery toggle when Ad blocker recovery setup status is empty', () => {
const { container } = render( <AdBlockingRecoveryToggle />, {
setupRegistry: ( registry ) => {
provideModules( registry, [
{
slug: 'adsense',
active: true,
connected: true,
},
] );
registry
.dispatch( MODULES_ADSENSE )
.receiveGetSettings( validSettings );
},
} );

expect(
container.querySelector(
'.googlesitekit-settings-module__ad-blocking-recovery-toggles'
)
).toBeNull();
expect(
container.querySelector(
'.googlesitekit-settings-module__ad-blocking-recovery-toggles'
)
).toBeNull();

expect( container ).toBeEmptyDOMElement();
}
);
expect( container ).toBeEmptyDOMElement();
} );

it( 'should render the Ad blocking recovery toggle when the conditions are met', () => {
const { container, getByLabelText, getAllByRole } = render(
<AdBlockingRecoveryToggle />,
{
features: [ 'adBlockerDetection' ],
setupRegistry: ( registry ) => {
provideModules( registry, [
{
Expand Down Expand Up @@ -131,7 +121,6 @@ describe( 'AdBlockingRecoveryToggle', () => {
const { getByLabelText, getAllByRole, queryByLabelText } = render(
<AdBlockingRecoveryToggle />,
{
features: [ 'adBlockerDetection' ],
setupRegistry: ( registry ) => {
provideModules( registry, [
{
Expand Down Expand Up @@ -171,7 +160,6 @@ describe( 'AdBlockingRecoveryToggle', () => {
const { getByLabelText, getAllByRole, container } = render(
<AdBlockingRecoveryToggle />,
{
features: [ 'adBlockerDetection' ],
setupRegistry: ( registry ) => {
provideModules( registry, [
{
Expand Down
12 changes: 9 additions & 3 deletions assets/js/modules/adsense/components/settings/SettingsForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,12 @@ import WebStoriesAdUnitSelect from '../common/WebStoriesAdUnitSelect';
import Link from '../../../../components/Link';
import { CORE_SITE } from '../../../../googlesitekit/datastore/site/constants';
import AdBlockingRecoveryCTA from '../common/AdBlockingRecoveryCTA';
import { useFeature } from '../../../../hooks/useFeature';
const { useSelect } = Data;

export default function SettingsForm() {
const adBlockerDetectionEnabled = useFeature( 'adBlockerDetection' );

const webStoriesActive = useSelect( ( select ) =>
select( CORE_SITE ).isWebStoriesActive()
);
Expand Down Expand Up @@ -125,9 +128,12 @@ export default function SettingsForm() {

<AutoAdExclusionSwitches />

<AdBlockingRecoveryCTA />

<AdBlockingRecoveryToggle />
{ adBlockerDetectionEnabled && (
<Fragment>
<AdBlockingRecoveryCTA />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component is also rendered in the SettingsView which is still being rendered unconditionally.

site-kit-wp/assets/js/modules/adsense/components/settings/SettingsView.js

Lines 225 to 228 in 94ecff3

) }
<AdBlockingRecoveryCTA />
</div>

Apologies, I missed this in the IB too.

<AdBlockingRecoveryToggle />
</Fragment>
) }
</div>
);
}
76 changes: 42 additions & 34 deletions assets/js/modules/adsense/components/settings/SettingsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const { useSelect } = Data;

export default function SettingsView() {
const adsenseSetupV2Enabled = useFeature( 'adsenseSetupV2' );
const adBlockerDetectionEnabled = useFeature( 'adBlockerDetection' );

const accountID = useSelect( ( select ) =>
select( MODULES_ADSENSE ).getAccountID()
Expand Down Expand Up @@ -179,52 +180,59 @@ export default function SettingsView() {
</div>
) }

{ !! adBlockingRecoverySetupStatus?.length && (
<div className="googlesitekit-settings-module__meta-items">
<div className="googlesitekit-settings-module__meta-item">
<h5 className="googlesitekit-settings-module__meta-item-type">
{ __( 'Ad blocking recovery', 'google-site-kit' ) }
</h5>
{ adBlockingRecoverySetupStatus ===
'setup-confirmed' && (
<p className="googlesitekit-settings-module__meta-item-data">
{ adBlockerDetectionEnabled &&
!! adBlockingRecoverySetupStatus?.length && (
<div className="googlesitekit-settings-module__meta-items">
<div className="googlesitekit-settings-module__meta-item">
<h5 className="googlesitekit-settings-module__meta-item-type">
{ __(
'Ad blocking recovery tag is not placed',
'Ad blocking recovery',
'google-site-kit'
) }
</p>
) }
{ adBlockingRecoverySetupStatus === 'tag-placed' && (
<Fragment>
</h5>
{ adBlockingRecoverySetupStatus ===
'setup-confirmed' && (
<p className="googlesitekit-settings-module__meta-item-data">
{ __(
'Ad blocking recovery tag is placed',
'Ad blocking recovery tag is not placed',
'google-site-kit'
) }
</p>
<p className="googlesitekit-settings-module__meta-item-data">
{ createInterpolateElement(
__(
'Ad blocking recovery only works if you’ve also created and published a recovery message in AdSense. <a>Configure your message</a>',
) }
{ adBlockingRecoverySetupStatus ===
'tag-placed' && (
<Fragment>
<p className="googlesitekit-settings-module__meta-item-data">
{ __(
'Ad blocking recovery tag is placed',
'google-site-kit'
),
{
a: (
<Link
href={ privacyMessagingURL }
external
/>
) }
</p>
<p className="googlesitekit-settings-module__meta-item-data">
{ createInterpolateElement(
__(
'Ad blocking recovery only works if you’ve also created and published a recovery message in AdSense. <a>Configure your message</a>',
'google-site-kit'
),
}
) }
</p>
</Fragment>
) }
{
a: (
<Link
href={
privacyMessagingURL
}
external
/>
),
}
) }
</p>
</Fragment>
) }
</div>
</div>
</div>
) }
) }

<AdBlockingRecoveryCTA />
{ adBlockerDetectionEnabled && <AdBlockingRecoveryCTA /> }
</div>
);
}