ZeroDataStateNotifications component should not attempt data requests for recoverable modules in view-only context

[techanvil]

Bug Description

The ZeroDataStateNotifications component calls the isGatheringData and hasZeroData selectors for the Analytics and Search Console modules, and each of these selectors in turn relies on a request to retrieve report data for the corresponding module. However when in view-only context and the modules are in a recoverable state, the user does not have permission to request module data, so these requests fail and a JS error is seen in the console.

We should avoid calling these selectors when in view-only context for recoverable modules.

Steps to reproduce

1. Share Analytics and/or Search Console and put into a recoverable state.
2. Navigate to the view-only Dashboard as a user the modules are shared with.
3. Note that failed requests are made to the module report endpoints and JS errors are seen in the console.

Screenshots

Additional Context

• PHP Version: any
• OS: any
• Browser: any
• Plugin Version: 1.77.0
• Device: any

---

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

Acceptance criteria

• When in view-only context and either Analytics or Search Console is in a recoverable state:
  • Viewing the ZeroDataStateNotifications component (by navigating to the Main or Entity Dashboard) should not result in any requests for the recoverable module's report data being made.
  • As a result no related errors should appear in the JS console.

Implementation Brief

• In assets/js/components/notifications/ZeroDataStateNotifications/index.js:
  • Use the useSelect hook to call the getRecoverableModules selector from the core/modules store and assign it to a new variable named recoverableModules.
  • Check if the keys of the object recoverableModules includes analytics and assign the boolean value to a new variable named isAnalyticsRecoverable.
  • Check if the keys of the object recoverableModules includes search-console and assign the boolean value to a new variable named isSearchConsoleRecoverable.
  • Locate the isGatheringData and hasZeroData selector calls from the modules/analytics and modules/search-console stores. They are currently assigned to the following variables:
    • analyticsGatheringData
    • searchConsoleGatheringData
    • analyticsHasZeroData
    • searchConsoleHasZeroData
  • Use the useViewOnly hook (currently assigned to the viewOnly variable) to check if it is a view-only context.
  • For analyticsGatheringData and analyticsHasZeroData, only call the selectors if beside the current conditions, the following conditions are met:
    • viewOnly is falsey. OR
    • viewOnly is truthy but isAnalyticsRecoverable is falsey.
  • For searchConsoleGatheringData and searchConsoleHasZeroData, only call the selectors if beside the current conditions, the following conditions are met:
    • viewOnly is falsey. OR
    • viewOnly is truthy but isSearchConsoleRecoverable is falsey.

Test Coverage

No tests need to be added/updated.

QA Brief

Changelog entry

[aaemnnosttv]

Thanks for raising this, I mentioned this on a previous related issue but apparently it's not entirely fixed yet.

Feel free to add ACs!

[techanvil]

Thanks @aaemnnosttv, I have added ACs, please can you take a look?

[aaemnnosttv]

@techanvil this is probably what you meant, but right now it reads a bit like the change is coupled to both modules in a recoverable state when it really applies to both independently. Let's reword it to be more clear about this, then it should be good to go 👍

[techanvil]

Thanks @aaemnnosttv, that is a good point and I've tweaked the AC accordingly.

[techanvil]

Hi @nfmohit, this IB is technically fine and I am happy to :white_check_mark: it as it is.

However, I would say that it's a bit on the verbose side and pretty much transliterates code into text, which is a bit beyond what an IB needs to achieve. There are some details we could quite happily do without, and, for the sake of illustration, I'd suggest that it could be reduced to something like the following:

• In assets/js/components/notifications/ZeroDataStateNotifications/index.js:
  • Retrieve the set of recoverable modules from the getRecoverableModules selector. This returns an object keyed by module slug - if a module is recoverable its slug will be present.
  • Locate the isGatheringData and hasZeroData selector calls from the modules/analytics and modules/search-console stores.
  • For each of these selector calls, short-circuit the corresponding useInViewSelect to return false when viewOnly is true AND the corresponding module is recoverable.

[techanvil]

IB :white_check_mark:

[nfmohit]

However, I would say that it's a bit on the verbose side and pretty much transliterates code into text, which is a bit beyond what an IB needs to achieve. There are some details we could quite happily do without, and, for the sake of illustration, I'd suggest that it could be reduced to something like the following:

Thank you very much for the kind and detailed feedback here, @techanvil. Looking at your example, I realise that the IB I added could indeed be much more concise. I'll keep it in mind for the future ones. Thank you!

[techanvil]

Cheers Nahid! Glad you find it useful. :tada:

[wpdarren]

QA Update: ✅

Verified:

• No requests are made to the following endpoints:
  • /wp-json/google-site-kit/v1/modules/search-console/data/searchanalytics
  • /wp-json/google-site-kit/v1/modules/analytics/data/report
• No errors similar to the ones shown in the issue description screenshots show up in the browser console.

Screenshots