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

tofumatt · 2023-06-23T22:16:41Z

Summary

Addresses issue:

A few Ad Blocking Recovery components are rendered unconditionally #7179

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…ry settings.

github-actions · 2023-06-23T22:24:47Z

Build files for 8fc8d6e have been deleted.

github-actions · 2023-06-23T22:28:11Z

Size Change: -3.84 kB (0%)

Total Size: 1.35 MB

Filename	Size	Change
`./dist/assets/js/googlesitekit-activation-********************.js`	24.2 kB	+143 B (+1%)
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	52 kB	+29 B (0%)
`./dist/assets/js/googlesitekit-adminbar-********************.js`	32.8 kB	-433 B (-1%)
`./dist/assets/js/googlesitekit-api-********************.js`	9.85 kB	-3 B (0%)
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.27 kB	-6 B (0%)
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	16.5 kB	+31 B (0%)
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	9.36 kB	-5 B (0%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	23.3 kB	+121 B (+1%)
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	66.4 kB	+15 B (0%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	80.4 kB	-8 B (0%)
`./dist/assets/js/googlesitekit-modules-********************.js`	21.1 kB	-494 B (-2%)
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	104 kB	-26 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	33.1 kB	-437 B (-1%)
`./dist/assets/js/googlesitekit-modules-analytics-********************.js`	87.9 kB	-506 B (-1%)
`./dist/assets/js/googlesitekit-modules-optimize-********************.js`	20.6 kB	-28 B (0%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	21.3 kB	-167 B (-1%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	52.7 kB	-452 B (-1%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	32.2 kB	-331 B (-1%)
`./dist/assets/js/googlesitekit-settings-********************.js`	48.7 kB	-417 B (-1%)
`./dist/assets/js/googlesitekit-splash-********************.js`	66.7 kB	-436 B (-1%)
`./dist/assets/js/googlesitekit-user-input-********************.js`	41.6 kB	-429 B (-1%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	17.6 kB	-8 B (0%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	63 kB	+7 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	49.9 kB
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11.2 kB
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	7.55 kB
`./dist/assets/js/30-********************.js`	2.8 kB
`./dist/assets/js/31-********************.js`	2.28 kB
`./dist/assets/js/32-********************.js`	3.72 kB
`./dist/assets/js/33-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	769 B
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.29 kB
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	9.93 kB
`./dist/assets/js/googlesitekit-data-********************.js`	2.18 kB
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.09 kB
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.92 kB
`./dist/assets/js/googlesitekit-polyfills-********************.js`	379 B
`./dist/assets/js/googlesitekit-vendor-********************.js`	310 kB
`./dist/assets/js/runtime-********************.js`	1.26 kB

_{compressed-size-action}

nfmohit

Thank you for the work here, @tofumatt, absolutely accurate!

I'm being very nitpicky here. I understand updating stories/tests wasn't mentioned in the IB, however, test suites for both the AdBlockingRecoveryCTA and AdBlockingRecoveryToggle components include tests to verify if the adBlockerDetection feature flag is enabled.

AdBlockingRecoveryCTA:

site-kit-wp/assets/js/modules/adsense/components/common/AdBlockingRecoveryCTA.test.js

Lines 37 to 43 in 9fa3d85

    
           [ 
        
           	'the Ad blocker detection feature flag is not enabled', 
        
           	ACCOUNT_STATUS_PENDING, 
        
           	SITE_STATUS_READY, 
        
           	'', 
        
           	false, 
        
           ],

AdBlockingRecoveryToggle:

site-kit-wp/assets/js/modules/adsense/components/common/AdBlockingRecoveryToggle.test.js

Line 45 in 9fa3d85

[ 'the Ad blocker detection feature flag is not enabled', false ],

Also, all the other tests within the above test suites set the adBlockerDetection feature flag to be enabled.

The tests still work flawlessly with them existing there. Now that the feature flag checks have been removed from the components within, do you think these checks are redundant and should be removed from the tests?

Also, the Storybook stories for AdBlockingRecoveryCTA and AdBlockingRecoveryToggle components set the adBlockerDetection feature flag to be enabled. For example:

site-kit-wp/assets/js/modules/adsense/components/common/AdBlockingRecoveryCTA.stories.js

Lines 44 to 46 in 9fa3d85

    
           Ready.parameters = { 
        
           	features: [ 'adBlockerDetection' ], 
        
           };

Do you think these should also be removed?

tofumatt · 2023-06-26T17:34:27Z

Nitpicks are okay, but also these are hardly nitpicks, great catches! 🙏🏻

The tests still work flawlessly with them existing there. Now that the feature flag checks have been removed from the components within, do you think these checks are redundant and should be removed from the tests?

Yes, they should be removed. Thank you for finding/noticing this! These it.each make for very unreadable tests in my view, and this is a prime example of why: it was totally unobvious to me that the uncommented/named variable there was adblocking detection unless I read through dozens of line of code 😆.

For future reference to any devs reading this, arrays of something like:

{
  testName: 'Adsense site status is not ready',
  accountStatus: ACCOUNT_STATUS_READY,
  siteStatus: SITE_STATUS_ADDED,
  recoverySetupStatus: '',
  adBlockerDetectionFeatureFlag: true,
},

over

[
  'Adsense site status is not ready',
  ACCOUNT_STATUS_READY,
  SITE_STATUS_ADDED,
  '',
  true,
],

for it.each would massively improve readability… though really we just shouldn't be using it.each to my mind as it's a tool for writing tests quick, and tests should be slow to write and quick to read 😉.

nfmohit · 2023-06-26T18:25:59Z

I absolutely agree, thank you for the example!

… & stories.

nfmohit

LGTM, thank you!

aaemnnosttv

Thanks @tofumatt ! This is almost there, just one more thing which is needed.

aaemnnosttv · 2023-06-26T22:49:13Z

assets/js/modules/adsense/components/settings/SettingsForm.js

-			<AdBlockingRecoveryToggle />
+			{ adBlockerDetectionEnabled && (
+				<Fragment>
+					<AdBlockingRecoveryCTA />


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.

aaemnnosttv · 2023-06-26T23:23:19Z

A few more things I noticed when looking at the SettingsView, only one of which is relevant here: the ABR setup status is only conditional based on its value, not the feature flag so this should also be addressed here

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

Line 182 in 94ecff3

{ !! adBlockingRecoverySetupStatus?.length && (

Secondarily, I don't think these messages match the descriptions

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

Lines 188 to 204 in 94ecff3

    
           { adBlockingRecoverySetupStatus === 
        
           	'setup-confirmed' && ( 
        
           	<p className="googlesitekit-settings-module__meta-item-data"> 
        
           		{ __( 
        
           			'Ad blocking recovery tag is not placed', 
        
           			'google-site-kit' 
        
           		) } 
        
           	</p> 
        
           ) } 
        
           { adBlockingRecoverySetupStatus === 'tag-placed' && ( 
        
           	<Fragment> 
        
           		<p className="googlesitekit-settings-module__meta-item-data"> 
        
           			{ __( 
        
           				'Ad blocking recovery tag is placed', 
        
           				'google-site-kit' 
        
           			) } 
        
           		</p>

I noticed this when the setup was completed and both toggles were on, but the view was showing Ad blocking recovery tag is not placed.

The tag is placed for both of these statuses – it's the blank string where they're not placed (this would probably be better as a named value as well. We also have an enum value for the statuses now as well but that's a minor detail. This one should be revisited and corrected separately.

I'll push a fix for the additional guards that were missed and send it back to you @tofumatt for MR 👍

cc: @kuasha420 ☝️

tofumatt

Looks good, thanks for catching that one @aaemnnosttv! 👍🏻

Change the placement of the feature flag check for Ad Blocking Recove…

9fa3d85

…ry settings.

nfmohit requested changes Jun 26, 2023

View reviewed changes

tofumatt and others added 5 commits June 26, 2023 19:36

Remove unneeded feature flag from stories.

c23ded8

Remove unused feature flag from test.

05344e4

Remove unused feature flags from tests/stories.

ae0d7c0

Merge branch 'develop' into fix/7179.

1e552b8

Remove remaining unnecessary checks for adBlockerDetection in tests…

94ecff3

… & stories.

nfmohit approved these changes Jun 26, 2023

View reviewed changes

aaemnnosttv requested changes Jun 26, 2023

View reviewed changes

Guard ABR elements in SettingsView.

8fc8d6e

aaemnnosttv approved these changes Jun 26, 2023

View reviewed changes

tofumatt commented Jun 27, 2023

View reviewed changes

tofumatt merged commit 1eabdfb into develop Jun 27, 2023

tofumatt deleted the fix/7179 branch June 27, 2023 13:38

kuasha420 mentioned this pull request Jun 27, 2023

Incorrect Ad Blocking Recovery tag placement status displayed on AdSense module settings #7213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tofumatt commented Jun 23, 2023 •

edited by nfmohit

Loading

github-actions bot commented Jun 23, 2023 •

edited

Loading

github-actions bot commented Jun 23, 2023 •

edited

Loading

nfmohit left a comment

tofumatt commented Jun 26, 2023 •

edited

Loading

nfmohit commented Jun 26, 2023

nfmohit left a comment

aaemnnosttv left a comment

aaemnnosttv Jun 26, 2023

aaemnnosttv commented Jun 26, 2023

tofumatt left a comment

	[
	'the Ad blocker detection feature flag is not enabled',
	ACCOUNT_STATUS_PENDING,
	SITE_STATUS_READY,
	'',
	false,
	],

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

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

Conversation

tofumatt commented Jun 23, 2023 • edited by nfmohit Loading

Summary

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Jun 23, 2023 • edited Loading

github-actions bot commented Jun 23, 2023 • edited Loading

nfmohit left a comment

Choose a reason for hiding this comment

tofumatt commented Jun 26, 2023 • edited Loading

nfmohit commented Jun 26, 2023

nfmohit left a comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv Jun 26, 2023

Choose a reason for hiding this comment

aaemnnosttv commented Jun 26, 2023

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt commented Jun 23, 2023 •

edited by nfmohit

Loading

github-actions bot commented Jun 23, 2023 •

edited

Loading

github-actions bot commented Jun 23, 2023 •

edited

Loading

tofumatt commented Jun 26, 2023 •

edited

Loading