A few Ad Blocking Recovery components are rendered unconditionally

[kuasha420]

Bug Description

As spotted by the eagle-eyed @aaemnnosttv, a few of the Ad Blocking Recovery "entrypoint" component are being rendered unconditionally instead of only being rendered when the adBlockerDetection feature flag is enabled which is not correct.

While this is not causing any bugs at the moment (because the components themselves doesn't cause any side effect that depends on things (ie. API endpoints) behind the the feature flag, it should always be rendered when the feature flag is enabled for consistently and to avoid the problems similar to one reported here for Key Metrics.

---

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

Acceptance criteria

• Any Ad Blocking Recovery components that are "entrypoint" to the Ad Blocking Recovery feature should only be rendered when the feature flag is enabled. This includes notification, Setup CTA, Settings CTA, Settings view and edit component, notices, banners etc.
• Any Ad Blocking Recovery related logic (ie. call to any Ad Blocking Recovery specific API, actions or selectors) should be encapsulated inside the aforementioned components and should not bleed outwards.
• Any redundant check for the adBlockerDetection feature flag inside the component or any of it's children should be removed as the component will not be rendered at all if the flag is not enabled.
• Currently the AdBlockingRecoveryCTA and AdBlockingRecoveryToggle components are rendered unconditionally in SettingsForm and SettingsView but this may not be an exhausting list.

Implementation Brief

• Conditionally render

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

  Lines 128 to 130 in b997e6e

  	<AdBlockingRecoveryCTA />
  	<AdBlockingRecoveryToggle />

  when the useFeature( 'adBlockerDetection' ) hook returns true
• Remove the feature flag checks at

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

  Line 40 in b997e6e

  const adBlockerDetectionEnabled = useFeature( 'adBlockerDetection' );

  and

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

  Line 46 in b997e6e

  const adBlockerDetectionEnabled = useFeature( 'adBlockerDetection' );

  , making sure the components still work without them
• Don't worry about

  site-kit-wp/assets/js/googlesitekit-ad-blocking-recovery.js

  Line 39 in b997e6e

  <AdBlockingRecoveryApp />

  , because that entire page shouldn't be rendered without the feature flag enabled.

Test Coverage

• No new tests needed; make sure the existing tests pass

QA Brief

• Enable the adBlockerDetection feature flag using a site that has AdSense set up and can see the Ad Blocker Detection settings in the AdSense settings.
• Visit Site Kit Settings and load the AdSense module.
• Ad Blocking Detection CTA should still be visible. There should be no browser console errors.
• Disable the adBlockerDetection feature flag and visit AdSense settings. Ad Blocking Detection CTA should not appear in "Edit" mode for AdSense and there should be no browser console errors.

Changelog entry

• Ensure Ad Blocking Recovery elements are only present when the feature is enabled.

[techanvil]

AC ✅

[aaemnnosttv]

I've added notes on where they can be removed, but to whoever reviews the IB: what about amending these ACs to allow the existing checks to remain? 🤔

@tofumatt it depends on the case; I think most will not make sense to keep because checking this within a component that itself should only render when that feature/condition should be guarding its render would also require that you use that feature within every hook which may not be possible in some cases. So that's largely the issue here, which is that the check within the component does not protect against side-effects from hooks. For example, we wouldn't check if a module is active in every component, right? It depends on the component, but we always need to be mindful of the unconditional nature of hooks.

This same issue happened very recently with KMW: #6262 (comment)

If we really want to apply it at the component level, then we'd need to leverage an HOC, e.g. withFeature( 'foo' )( Component ) similar to how we use whenActive() for dashboard widgets. That still relies on implementing it at the individual component level for each though. Our convention has been to check the feature and { featureEnabled && <Component /> } everywhere else I can think of so I'd be a bit concerned about introducing a component-level guard like that which hides that condition if you were reading through and only saw <Component /> the way it is here.

• Don't worry about

  site-kit-wp/assets/js/googlesitekit-ad-blocking-recovery.js

  Line 39 in b997e6e

  <AdBlockingRecoveryApp />

  
  , because that entire page shouldn't be rendered without the feature flag enabled.

The majority of ABD components will be rendered here so I'd so all of those shouldn't be a concern for this same reason.

Otherwise IB LGTM but I haven't verified the exhaustiveness of the components affected here.

[tofumatt]

@aaemnnosttv Fair point! I did think that as well—if the feature flag weren't enabled the selectors in those components would still run, so removing them makes sense.

I did check the components exhaustively, so this IB should be good-to-go now 🙂

[aaemnnosttv]

IB ✅

[wpdarren]

QA Update: ⚠️

@tofumatt this might not be related to this ticket but I have noticed on Site Kit Dashboard within the Monetization area, that the CTA for setting up ABR is not appearing. I am sure it was displaying previously, but maybe when I tested it, we used code in the console to trigger it - not 100% sure. Should it be appearing at this stage in engineering?

Verified:

• With adBlockerDetection feature flag enabled and using a site that has AdSense set up, I can see the Ad Blocker Detection CTA in the AdSense edit settings. There are no browser console errors.

• With adBlockerDetection feature flag disabled and using a site that has AdSense set up, the Ad Blocker Detection CTA in the AdSense edit settings. There are no browser console errors.

[tofumatt]

Do you mean it's not appearing when the Ad Blocker Detection feature flag is disabled? If so that should be fine.

If it was previously appearing with the feature flag enabled and still isn't… I don't think this should've caused that change, but let me know which it is. 🙂

[wpdarren]

@tofumatt the feature flag is enabled and the CTA is not appearing on the Site Kit dashboard. If it's not related to this issue I can move this on and flag this in the channel to Arafat.

[wpdarren]

QA Update: ✅

The issue with the ABR CTA not appeaing on the Site Kit dashboard is outside of this ticket. Have raised it in Slack.

Verified:

• With adBlockerDetection feature flag enabled and using a site that has AdSense set up, I can see the Ad Blocker Detection CTA in the AdSense edit settings. There are no browser console errors.

• With adBlockerDetection feature flag disabled and using a site that has AdSense set up, the Ad Blocker Detection CTA in the AdSense edit settings. There are no browser console errors.