Allow Analytics to be removed as an available module without breaking Site Kit

[jamesozzie]

Bug Description

If Google Analytics has been disabled via googlesitekit_available_modules (https://github.com/google/site-kit-wp/issues/3993) ensure that the option to setup Analytics isn't included for other admins who decide to set up the plugin.

Screenshots

https://user-images.githubusercontent.com/41326532/162797693-acce4a38-0451-48a3-be7c-a4c84fcff6d3.mp4

Steps to reproduce

1. Disable Analytics via the below filter
2. Install and setup SK
3. The option to add GA remains

Note that GA is never added to the setup steps, so the setup flow does complete successfully if a user proceeds. Google Analytics isn't connected at any stage, and the option to connect doesn't appear, nor for the setup CTAs.

add_filter(
	'googlesitekit_available_modules',
	function ( $modules ) {
		return array( 'search-console' );
	}
);

---

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

Acceptance criteria

• Any references to modules/analytics store selectors or actions from a general component or store should only occur after verifying that the module is available
  • In other words, this issue should cover all accesses within assets/js/components.
  • A good way to tackle this is to search for any MODULES_ANALYTICS references in that above directory and adjust/wrap them accordingly.
  • At a minimum, the check needs to ensure that the respective MODULES_ANALYTICS datastore is available before calling a selector or action on it.
  • Use reasonable fallback values (e.g. false for a boolean, null for an object, etc.).
• It is okay not to worry about potentially "weird" looking UI due to not showing certain Analytics information, since this is a special case that we are not going to specifically support through clean UI. In other words, e.g. the Analytics setup CTAs are perfectly fine to be replaced with nothing, i.e. simply white-space. The same applies to the Analytics notice on the splash screen, which should simply not show and therefore behave in a similar way as if Analytics was already active (in which case it already doesn't show today).

Implementation Brief

As outlined in my comment below, I think the ACs need adjusting because they suggest a particular implementation, but in this case I think we can simplify the approach a fair bit. 🙂

The issue above stems from the fact that we're using false-y checks for isModuleConnected instead of explicitly checking for === false. As @felixartnz mentioned below, rather than "simply" adding a === null check we should add a new selector: isModuleAvailable( slug ) that allows us to check whether a module is at all available before selecting anything from its store or rendering data related to it.

• Add a new selector to the CORE_MODULES data store: isModuleAvailable( slug ). It should accept a module slug as the sole argument, and return:

  • undefined if getModule( slug ) === undefined
  • false if getModule( slug ) === null
  • true if neither of the above conditions are met

• Adjust components to not output Analytics component when the module is not available. Here's a list of places we're checking for a false-y isModuleConnected where we should also use the new isModuleAvailable( slug ) selector before trying to select/render any module-related data:

• https://github.com/google/site-kit-wp/blob/d9956a60b82eed776a24c78f069ff2ee64c2bfec/assets/js/components/adminbar/AdminBarWidgets.js#L134-L146

• https://github.com/google/site-kit-wp/blob/d9956a60b82eed776a24c78f069ff2ee64c2bfec/assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/index.js#L325-L341 (Update not to show CTAs when isAnalyticsConnected === null

• https://github.com/google/site-kit-wp/blob/d9956a60b82eed776a24c78f069ff2ee64c2bfec/assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/Overview.js#L225-L239

Everywhere else the sole isModuleConnected(slug) ("false-y") checks are accurate and can remain.

• We should also adjust places where we're checking for isModuleActive( slug ) and first check isModuleAvailable( slug ) to ensure the module is available before we check if it's active. This can be done everywhere we use isModuleActive()

Test Coverage

• Add tests to the setup file (and components that render CTAs) that check to see if the Analytics CTAs are output when Analytics is not available.
• If the datastore approach is taken, we should test to make sure we don't make fetch requests when Analytics selectors with resolvers are called when Analytics is disabled.

QA Brief

• Add the filter below to your WordPress site, e.g. in your theme's functions.php.
• Set up Site Kit and ensure the Analytics CTA doesn't show on the splash screen.
• Access the various Site Kit areas (main dashboard, entity dashboard, WP dashboard widget, admin bar menu), and ensure there are no errors due to Analytics not being available.
• Check the Tag Manager module still works.
• Note that the Optimize module will also be removed due to its dependence on Analytics.

Filter

add_filter(
	'googlesitekit_available_modules',
	function( $modules ) {
		$index = array_search( 'analytics', $modules, true );
		if ( false !== $index ) {
			unset( $modules[ $index ] );
			return array_values( $modules );
		}
		return $modules;
	}
);

Changelog entry

[tofumatt]

Looks like we thought ahead with our API and have a separate return value for isModuleActive: https://github.com/google/site-kit-wp/blob/dce817b95df607384f4ed5e039bd6836e28d9475/assets/js/googlesitekit/modules/datastore/modules.js#L773-L775

We should use the null return value to mean the module isn't available—I tested this out by using the filter above and useSelect( ( select ) => select( CORE_MODULES ).isModuleConnected( 'analytics' ) ); returned null as-expected, so we'll want to use that null return value to change output/prevent selectors from running.

I think the components are the wrong place to prevent the selectors' resolvers from running—the resolvers themselves should check to see if the module is enabled and resolve immediately (with "empty" values) if the module isn't enabled. That's a cleaner and more-scalable solution… either one is a fair bit of work but I think doing it in the datastore is better.

For components I tried checking for null instead of false-y isModuleConnected and it worked great… here's a proof-of-concept PR for the screen used in the issue summary: #5120

[tofumatt]

I think the ACs here might've lead me to think we want to really protect against any Analytics requests and build in a robust protection against running code when a module is disabled, but I don't think that's needed.

Maybe I misunderstood the intent of the ACs, but they seemed kind of broad in scope to me. I've written up an IB to fix this issue and ones like it throughout the codebase for Analytics, but I think it can be discussed a bit before moving this to execution 🙂

[eugene-manuilov]

If we adjust components to not output Analytics component when the module is not available everything should work fine. Here's a list of places we're checking for a false-y isModuleConnected where we should explicitly check for === false instead:

@tofumatt, yes, I think it should help but I would defer it to @felixarntz to make the final decision.

Just want to add that Analytics widgets won't be rendered anyway because the whenActive hook won't render them, so we don't need to update selectors there. What we need to update are widgets in AdSense and Search Console modules as well as for WP dashboard and admin bar to make sure that they get Analytics reports only if the Analytics module is active.

Plus, we need to update the SetupUsingProxyWithSignIn component not to render the ActivateAnalyticsNotice element if the Analytics module is not available.

[felixarntz]

@tofumatt Definitely +1 to making sure that both isModuleActive and isModuleConnected return null in case the module doesn't exist (and false only if it exists but the conditions aren't met). I think this is an enhancement worth making regardless of the rest of the approach.

Still, I think the IB is a bit overly simplistic, as in some areas we rely on isModuleActive( 'analytics' ) and we can't just replace that with isModuleConnected( 'analytics' ) in every case. For example, on the splash screen the CTA is explicitly only relevant if Analytics isn't active, but not if it's active but not connected.

Also, +1 to @eugene-manuilov's comment that we shouldn't need to update most Analytics widgets if they already have the whenActive wrapper.

Overall, while per my first paragraph I support the idea, I consider the differentiation between null and false a rather error-prone, almost "hidden", functionality that may be easily missed also in the future. Someone reading isModuleConnected === false in the future may just randomly replace it with ! isModuleConnected, thinking that it is the same. That's why I'd be in favor of adding an additional check, maybe via a new boolean selector hasModule( moduleSlug ) on the core/modules store.

[tofumatt]

Overall, while per my first paragraph I support the idea, I consider the differentiation between null and false a rather error-prone, almost "hidden", functionality that may be easily missed also in the future. Someone reading isModuleConnected === false in the future may just randomly replace it with ! isModuleConnected, thinking that it is the same. That's why I'd be in favor of adding an additional check, maybe via a new boolean selector hasModule( moduleSlug ) on the core/modules store.

That's fair, and something we already occasionally encounter with our "undefined-as-loading/not-yet-loaded" approach to selector return values. I actually wanted to highlight that here because while it might be tempting to say "Oh, but we already do this", I think in retrospect that's not even the best solution and one we should be careful to protect against in the future. (If I could, I'd change all of the loading values to be a magic constant or something we could better check against! 😅 )

A hasModule-type selector (that ultimately relies on the existing null return value) seems like a good plan—it's more explicit but still not a lot of effort. I'll update the IB with that approach 👍🏻

[felixarntz]

@tofumatt IB mostly looks good, a few small notes:

• Add a new selector to the CORE_MODULES data store: isModuleAvailable( slug ). It should accept a module slug as the sole argument, and return:
• undefined if isModuleConnected( slug ) === undefined
• false if isModuleConnected( slug ) === null
• true if neither of the above conditions are met

It strikes me a bit odd to rely on isModuleConnected here. This is simply about whether the module exists, and while I'm all for reusing code, I think using getModule here instead better represents its purpose. isModuleConnected also checks something else in addition to whether the module exists, we essentially don't care about its true vs false state. Hence relying on getModule here is sufficient and a bit closer to the purpose.

• Adjust components to not output Analytics component when the module is not available. Here's a list of places we're checking for a false-y isModuleConnected

I think we may want to also reference any false-y isModuleActive selector calls here, since that can also apply in a few places. Like anywhere that we render some sort of CTA for Analytics when it's not active, we should also not render such a thing, i.e. add the isModuleAvailable check to the condition.

[FlicHollis]

Hey @tofumatt just following up on this one as this looks like one that's nearly ready + it will unblock / inform 5072 + 5074.

[felixarntz]

IB ✅

[mohitwp]

QA Update ✅

• Verified on dev and latest environment.
• I'm able to reproduce issue on latest environment.
• Verified following steps mentioned under QAB.
• Verified that the Analytics CTA doesn't show on the splash screen.
• Verified that user is able to access site kit dashboard successfully.
• Verified that successful message for analytics set up is not showing.
• Verified that analytics and optimize module removed successfully.
• Verified that Site Kit not raising errors due to the Analytics module being programmatically removed as an available module.
• Verified that main, entity dashboard, WP dashboard and WP menu bar not showing any error.
• Verified tag manager is connected successfully, not showing any error and working.
• Verified GTM snippet in page source.

https://user-images.githubusercontent.com/94359491/193585244-0f6cd10f-d8e6-4b1e-b106-c5eabcbd4d8c.mp4