Add resolvers for key dependent selectors of `getModules`

[aaemnnosttv]

Feature Description

There are a number of selectors on the core/modules store that depend on the result from getModules which has a resolver while its dependents do not. This results in us needing to await getModules manually in some cases which is unintuitive as the result is left unused.

E.g. https://github.com/google/site-kit-wp/blob/9d2f78ea330ac592e0953316a1264867aa398540/assets/js/feature-tours/index.js#L42-L43

Instead, we should add resolvers for a few key selectors such as isModuleConnected so that we can simply resolve select it directly.

The resolver itself in this case would only need to await getModules, as is already done in some places as well https://github.com/google/site-kit-wp/blob/9d2f78ea330ac592e0953316a1264867aa398540/assets/js/googlesitekit/modules/datastore/modules.js#L464-L466

This keeps the dependency in the store and avoids odd side-effect implementation details to leak out into consuming code.

---

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

Acceptance criteria

• The following selectors of the core/modules store should have resolvers added for them which simply await the resolution of getModules:
  • getModule
  • isModuleActive
  • isModuleConnected
• Any existing use of await registry.__experimentalResolveSelect( CORE_MODULES ).getModules() should be refactored to resolve select the value from the enhanced selector instead (see description above for examples)

Implementation Brief

Test Coverage

QA Brief

Changelog entry

[FlicHollis]

Hi @techanvil are you still planning on working on this? If not, please unassign yourself so someone else can pick up - thanks!

[techanvil]

Thanks for calling this out @FlicHollis, I've removed my assignment :+1:

[FlicHollis]

Hey @hussain-t I notice this was assigned to you a while ago. Has this IB been started? If not, feel free to unassign yourself so that any of the team can pick it up. Thanks so much!

[hussain-t]

Thanks for calling out @FlicHollis. I have unassigned myself.

[eugene-manuilov]

• Remove the use of await registry.__experimentalResolveSelect( CORE_MODULES ).getModules() within the codebase.

@asvinb, we can't just remove it from the codebase because it will cause failure. What we need to do is to replace getModules calls with a selector name that is actually needed. For example, if a function needs to check whether a module is connected, then we need to replace the resolveSelect call for the getModules selector with const moduleConnected = await registry.__experimentalResolveSelect( CORE_MODULES ).isModuleConnected( slug ).

[asvinb]

@eugene-manuilov Apologies. The wording was wrong in the IB. I updated the IB, basically we no longer need to call await registry.__experimentalResolveSelect( CORE_MODULES ).getModules() before calling getModule, isModuleActive or isModuleConnected. Let me know if it's clearer now 😁

[eugene-manuilov]

... basically we no longer need to call await registry.__experimentalResolveSelect( CORE_MODULES ).getModules() before calling getModule, isModuleActive or isModuleConnected...

@asvinb, yes, we don't need to call getModules anymore, but we do need to call those selectors with __experimentalResolveSelect because otherwise selectors will return early with undefined results since resolvers won't be finished yet.

So, instead of https://github.com/google/site-kit-wp/blob/98bd026ca0f95f9db25536c0a99eeb7bf00b97cc/assets/js/feature-tours/all-traffic-widget.js#L41-L47 we should do it this way now:

const connected = await registry.__experimentalResolveSelect( CORE_MODULES ).isModuleConnected( 'analytics' );
if ( ! connected ) {
    return false;
}

[asvinb]

Thanks @eugene-manuilov ! IB has been updated!

[eugene-manuilov]

Thanks, @asvinb. IB ✔️

[wpdarren]

QA Update: ✅

Verified:

• Tested feature tours and they load correctly, without any errors: both idea hub tours (dashboard and pages) and all traffic widget tour. I also tested to make sure that the dashboard sharing and the dashboard navigation menu tour worked too.
• There are no visual regressions or JS errors.

Screenshots / Screencasts

https://user-images.githubusercontent.com/73545194/194036880-746d0d58-95ed-4d60-8867-44d7270880fa.mp4

https://user-images.githubusercontent.com/73545194/194036904-a0533254-0e67-48d7-a200-a700080ccfcc.mp4

https://user-images.githubusercontent.com/73545194/194036940-40f1edf1-a5bd-4009-b110-66c3617dfecc.mp4

[aaemnnosttv]

⚠️ Approval

Looking at the code, there are a few instances where refactoring was missed, as well as a few places where something was refactored improperly.

Will open a follow-up PR shortly.

[tofumatt]

Moving this back to QA as a few small changes/refactoring were made to the code so it's worth testing this again before sending to approval again, but the QA steps are the same 👍🏻

[wpdarren]

QA Update: ❌

@aaemnnosttv @tofumatt the feature tours are triggered other than Idea hub draft posts tour. I've tested this via the forced version option in the tester plugin. Last time around when I was on the posts screen and ran the code in console, it triggered the tour, but it isn't doing that anymore. Could you look into this for me?

It's saying undefined:

 const tours = await googlesitekit.data.select('core/user').getAllFeatureTours();
 await googlesitekit.data.dispatch('core/user').triggerTour(tours[3]);

[asvinb]

@wpdarren Sorry if that was missed in the QAB, but you'll need to delete the wp_googlesitekitpersistent_dismissed_tours user meta since we are saving which tours were dismissed in the database. Via WP-CLI, you can use the following command: wp user meta delete 1 wp_googlesitekitpersistent_dismissed_tours where 1 is the user id or you delete the meta directly in the database in the wp_usermeta table. Another option would be to set up a new burner site and test 😁

[wpdarren]

QA Update: ✅

Verified:

• Tested feature tours and they load correctly, without any errors: both idea hub tours (dashboard and pages) and all traffic widget tour. I also tested to make sure that the dashboard sharing and the dashboard navigation menu tour worked too.
• There are no visual regressions or JS errors.

Screenshots / Screencasts

https://user-images.githubusercontent.com/73545194/194036880-746d0d58-95ed-4d60-8867-44d7270880fa.mp4

https://user-images.githubusercontent.com/73545194/194036904-a0533254-0e67-48d7-a200-a700080ccfcc.mp4

https://user-images.githubusercontent.com/73545194/194036940-40f1edf1-a5bd-4009-b110-66c3617dfecc.mp4