Connect modules briefly shows another module connected.

[ivankruchkoff]

Bug Description

When I click connect service, it always shows the right mode service as connected before redirecting to Google for auth.

Steps to reproduce

1. Go to /wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success
2. Search Console = "Connected", AdSense, Analytics and PageSpeed Insights = "Connect Service"
3. Click on any Connect Service
4. Prior to redirect for auth, PageSpeed Insights shows as Connected.

Screenshots

Added a video repro, intentionally not showing the Google Auth part:

connected-bug.mp4

---

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

Acceptance criteria

• When activating a module from the "Congrats" notification (which shows up after connecting Site Kit or a module), every module should remain in its current UI state all the way until the user is redirected to OAuth (i.e. it should not visually change to looking as connected before then).

Implementation Brief

• Comment out the automatic fetching of getModules when toggling module activation

  site-kit-wp/assets/js/googlesitekit/modules/datastore/modules.js

  Lines 219 to 222 in 47b723d

  	const { response, error } = yield fetchSetModuleActivationStore.actions.fetchSetModuleActivation( slug, active );
  	if ( response?.success === true ) {
  	// Fetch (or re-fetch) all modules, with their updated status.
  	yield fetchGetModulesStore.actions.fetchGetModules();

  • This will leave the module in the same state within the App until the page is refreshed, which is often needed anyways for the change to take effect to being setup, etc
  • Add a TODO comment to clarify that this should be restored later when Site Kit no longer relies on page reloads between module activation changes
• Remove localized workarounds added to fix this in module settings

  • site-kit-wp/assets/js/components/settings/SettingsActiveModules.js

    Lines 34 to 36 in 47b723d

    	// We store `initialActiveSlugs` separately to avoid
    	// layout shifts when deactivating a module as it would otherwise
    	// cause the module to be removed upon deactivation.

  • site-kit-wp/assets/js/components/settings/SettingsInactiveModules.js

    Lines 38 to 40 in 47b723d

    	// We store `initialInactiveSlugs` separately to avoid
    	// layout shifts when activating a module as it would otherwise
    	// cause the activated module to be removed upon activation.

• Finish and merge Receive module state on page load only #4141
  • Mostly just updates to tests left

Test Coverage

• Update tests for module activation to not refetch all modules (and thus update state)
• Add tests if not covered by the last point to ensure activating or deactivating a module does not immediately change its active/connected state

QA Brief

• Activate or deactivate a module in the settings and make sure that the list of modules doesn't change until the page is refreshed or after navigating back to the settings page.
• Activate a module from the congrats notification banner and verify that modules do not change their statuses until the page is refreshed.

Changelog entry

• Fix a bug that could cause the wrong module to show that it's being connected during module setup.

[abdullah1908]

@ivankruchkoff I am unable to reproduce this issue at my end. Would you mind to share more information so we can try to setup same environment & can replicate at our end.
cc: @jamesozzie

[mxbclang]

@ivankruchkoff Just checking in here – are you able to share some more details so that @abdullah1908 can try to run some more tests? Thanks!

[wpdarren]

@abdullah1908 @bethanylang An interesting issue that I have recreated, but the steps are not what you would expect a user to go through. That said, I will leave it up to the engineers to make the decision if we fix this.

Here are the steps I took to recreate:

1. Disconnect and reset Site Kit if you are using a site that has previously had the plugin activated.
2. Use this zip file (this was from 1.26.0 pre-release) google-site-kit.zip
3. Connect your Google account to Site Kit and go through the setup for Search Console.
4. You should be redirected to the Site Kit Dashboard ( /wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success)
5. Click on the Connect Service link for Google Analytics (or Adsense)
6. The Google authorisation page should appear. Don't authorise, instead click back on your browser.
7. You are then taken back to the Site Kit Dashboard. Click on the Connect Service link for PageSpeed Insights.
8. The Google authorisation page should appear. Don't authorise, instead click back on your browser.
9. You are then taken back to the Site Kit Dashboard. Click on the Connect Service link for Analytics.
10. What you will then see is the connect icon appears under PageSpeed Insights even though you clicked Analytics.

It then gets caught up in a bit of a loop.

Please note even when you use the latest version of Site Kit this issue still occurs.

Here's a screencast in case this helps:

Screen.Capture.on.2021-03-17.at.00-20-33.mov

[mxbclang]

@felixarntz Reassigning to you for triage. As Darren noted, this looks like a pretty isolated edge case, so probably not a priority.

[ivankruchkoff]

So the easier way to resolve this particular issue is a cache invalidation param in the selector causing it to fire again in:
https://github.com/google/site-kit-wp/blob/d8e741b/assets/js/components/ModulesList.js#L51

const cacheBust = +new Date;
const modulesData = useSelect( ( select ) => select( CORE_MODULES ).getModules( cacheBust ) );

The better solution here is to think of modules as having three states rather than two, currently we have a boolean active in assets/js/googlesitekit/modules/datastore/modules.js

site-kit-wp/assets/js/googlesitekit/modules/datastore/modules.js

Line 211 in d8e741b

*setModuleActivation( slug, active ) {

What we really need is active, inactive, and pending activation when activation is in progress, otherwise we go from inactive to active, before we've finished activation and we get unintended UI updates such as the following:

2796-2.mp4

After the UI update in the video, we are redirected to Google for auth to get permissions to activate Analytics, so any component that depends on whether or not the analytics module is active would behave the same.

What are your thoughts @felixarntz and @aaemnnosttv?

[aaemnnosttv]

@ivankruchkoff the state change you highlighted in your last comment is a known issue, but isn't this different than the issue you originally reported above?

What we really need is active, inactive, and pending activation when activation is in progress, otherwise we go from inactive to active, before we've finished activation and we get unintended UI updates such as the following:

I think this is probably the way to go, but the 3rd state should probably be a more generic "transitional" state since the same is possible when deactivating as well. The tricky thing is in settings for example, we want to show lists of all active/inactive modules that are resilient to activation changes. In this case we might need 4 states: active, activating, inactive, and deactivating.

Again, this seems to be a separate issue (although similar) to the one in this issue's description which I'm not sure is still an issue or not.

[danielgent]

Just to confirm @aaemnnosttv @ivankruchkoff that I see the original error somewhat frequently! (and I keep ruining my train of thought thinking about this ticket 😂 )

[ivankruchkoff]

Again, this seems to be a separate issue (although similar) to the one in this issue's description which I'm not sure is still an issue or not.

Yes, you're quite right.

[aaemnnosttv]

We need to invalidate the results of the selector after we've changed one of the modules activations:
const modulesData = useSelect( ( select ) => select( CORE_MODULES ).getModules() );
To do this, prior to redirecting to the auth page for a module, we call invalidateResolutionForStoreSelector( 'getModules' );

site-kit-wp/assets/js/components/ModulesList.js

Line 74 in d8e741b

navigateTo( response.moduleReauthURL );

@ivankruchkoff all of the the invalidation actions on the store only invalidate the selector's resolver (specifically the "internal" state that keeps track of if it ran) – it doesn't change the related state which is used by the selector.

Invalidating getModules would only cause its resolver to be run again, which would skip the fetch as well since modules would already be in state.

site-kit-wp/assets/js/googlesitekit/modules/datastore/modules.js

Lines 418 to 426 in df5db33

	*getModules() {
	const registry = yield Data.commonActions.getRegistry();
	const existingModules = registry.select( STORE_NAME ).getModules();
	if ( ! existingModules ) {
	yield fetchGetModulesStore.actions.fetchGetModules();
	}
	},

With that said, I've tried to reproduce the original issue and was not able to. Can you check again to see if this is still possible? cc @wpdarren

[ivankruchkoff]

Still an issue @aaemnnosttv, here's a repro screen recording.

2796-3.mp4

[aaemnnosttv]

Thanks a lot for the screencast @ivankruchkoff – I just tried this out myself and wasn't able to reproduce it; when I click "Back" it shows connected for me 🤷‍♂️ . With that said, if this only happens after navigating back then I would say this isn't worth spending time on, at least any time soon 😄 Thoughts @felixarntz ?

[felixarntz]

If it only occurs after navigating "back", I'd agree it's not highly crucial, however I'd say it's still a valid bug - maybe a P2? There may some related improvements also worth making though, for example, maybe the PSI module shouldn't show as connected until the redirect / page reload has happened. This is similar to various other enhancements that we've made (e.g. in the ActivateModuleCTA component) where it shouldn't change state before a redirect.

[felixarntz]

@aaemnnosttv For now that's okay, but it still creates an inconsistency in our datastore where it doesn't accurately reflect the current state. So we should for sure have a TODO annotation or some other comment there to clarify. I'd even say we should not remove the module fetching code, but only comment it out and explain why it is commented out. If it wasn't for our particular navigation struggle, having it there would be correct. Can you add that to the IB?

[aaemnnosttv]

I'd even say we should not remove the module fetching code, but only comment it out and explain why it is commented out. If it wasn't for our particular navigation struggle, having it there would be correct. Can you add that to the IB?

@felixarntz sure, that sounds good to me. In the future, we'll need to add some additional state for modules, or change how we represent the active state to be more flexible than a simple boolean. IB updated 👍

[felixarntz]

IB ✅

[kuasha420]

// Accidentally picked up this ticket thinking it is in the sprint without filtering by current sprint. Oops. Disregard the activity 😮‍💨

[wpdarren]

QA Update ✅

• Activated a module in the settings and the list of modules doesn't change until the page is refreshed or after navigating back to the settings page.
• Activated a module from the congrats notification banner and the modules do not change their statuses until the page is refreshed.
• Also looked back at previous comments about connecting and then going back to see if the statuses change even though the module has not been connected.