site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

JS errors if Tag Manager is disabled via googlesitekit_available_modules

Open felixarntz opened this issue 2 years ago • 5 comments

Bug Description

Similar to #5072, but for covering the case that the Tag Manager module is not available (e.g. via googlesitekit_available_modules filter): There will be JS errors at least in the Analytics setup as it unconditionally calls modules/tagmanager selectors.


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

Acceptance criteria

  • Any references to modules/tagmanager store selectors or actions from a component or store that is part of another module should only occur after verifying that the module is available (available, regardless of whether it is active or not).
    • In other words, this issue should cover all accesses within assets/js/modules/* other than assets/js/modules/tagmanager.
    • A good way to tackle this is to search for any MODULES_TAGMANAGER references in those other module directories and adjust/wrap them accordingly.
    • At a minimum, the check needs to ensure that the respective MODULES_TAGMANAGER datastore is available before calling a selector or action on it.

Implementation Brief

  • Find all instances of select( MODULES_TAGMANAGER )/dispatch( MODULES_TAGMANAGER ) and ensure that isModuleAvailable( MODULES_TAGMANAGER ) is true before using those selectors/dispatching those actions.
  • Find instances of isModuleActive( MODULES_TAGMANAGER ) and check for isModuleAvailable( MODULES_TAGMANAGER ).

Note: any code in any module should be checked, not just Tag Manager specific code. Be sure to look for Tag Manager selectors/action dispatches across the whole plugin when making these changes.

Test Coverage

  • Add tests to any components with existing tests that disable Tag Manager; ensure they still render without errors/console warnings.

QA Brief

  • Add the filter below to your WordPress site, e.g. in your theme's functions.php.
  • Set up the Analytics module and ensure there are no errors due to Tag Manager not being available.

Filter

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

Changelog entry

felixarntz avatar Apr 11 '22 20:04 felixarntz

(This can probably follow the approach used in #5071, whatever we decide, so it might be worth waiting on the discussion in that issue before tackling this.)

tofumatt avatar Apr 21 '22 19:04 tofumatt

@hussain-t I see this was assigned a month ago before the IB/approach in #5071 was finalised. Since I wrote up that IB I'm going to handle this and #5072, because there's a lot of overlap 🙂

tofumatt avatar Jun 13 '22 12:06 tofumatt

  • Find instances of isModuleActive( MODULES_TAGMANAGER ) and first check for isModuleAvailable( MODULES_ANALYTICS ) (do the same with MODULES_TAGMANAGER).

@tofumatt why do we need to check whether the Analytics module is available when we check if the Tag Manager module is active? Perhaps you meant "and first check for isModuleAvailable( MODULES_TAGMANAGER )"?

Also could you please add a note in IB to mention that those changes should be done in other modules that use actions and selectors from the Tag Manager module? Just for clarity. Thanks.

eugene-manuilov avatar Sep 06 '22 13:09 eugene-manuilov

The thinking there was that Tag Manager requires Analytics, so if Analytics is disabled Tag Manager would also be disabled… but you're right, I don't think that's needed and just complicates things. I've removed that 👍🏻

I've added a note to the IB as well, thanks 👍🏻

tofumatt avatar Sep 27 '22 22:09 tofumatt

IB ✔️

eugene-manuilov avatar Sep 29 '22 21:09 eugene-manuilov

QA Update ✅

  • Verified on both latest and dev environment using QAB.
  • On latest environment I'm able to reproduce issue.
  • On dev environment errors are not showing if Tag Manager module is not available.

https://user-images.githubusercontent.com/94359491/199218497-59bdd92f-7bef-4b13-9c96-0e87a969d4f2.mp4

mohitwp avatar Nov 01 '22 10:11 mohitwp