Requests for settings fail for view-only users on dashboard

[aaemnnosttv]

Bug Description

In #4815, we added conditional logic to prevent source links for widgets from displaying because we don't know if the user will be able to follow the deep link or not due to their lack of Google authentication. This issue addressed the visual aspect of the problem but there is now a residual technical problem related to the selectors used to generate the deep link.

The SourceLink itself no longer renders in view-only contexts but the URL given to it is selected in the parent. Since view-only users do not currently have access to module settings endpoints, the settings do not get preloaded and when requested on the client (triggered by a setting selector for some data needed in a service URL), it triggers a fetch request which again fails for the same reason it was not preloaded (lack of permissions).

Steps to reproduce

1. Set up dashboard sharing and share Analytics with a non-admin role
2. Log in as the non-admin user that was shared with
3. Go to the Site Kit dashboard
4. See console error for a request to Analytics settings that failed

---

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

Acceptance criteria

• The use of source links for dashboard widgets that use getService*URL selectors should be updated such that the selector is only invoked in a non-view-only context
  • This should ideally be done in a consistent way without requiring inline updates in each usage.
• Module settings should not be requested in a view-only context (this should be addressed by the previous point but here for completeness)

Implementation Brief

• Files to update:

  • assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidget.js
  • assets/js/modules/adsense/components/module/ModuleOverviewWidget/Footer.js
  • assets/js/modules/analytics/components/dashboard/DashboardOverallPageMetricsWidget.js
  • assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidget/index.js
  • assets/js/modules/analytics/components/module/ModulePopularPagesWidget/Footer.js
  • assets/js/modules/search-console/components/dashboard/DashboardPopularKeywordsWidget.js
  • assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/Footer.js
  • Note Confirm that DataBlock.js and LayoutFooter.js contain the <SourceLink> but they or their parents do not currently make getService*URL requests.

• Within the files to update above, identify the getService*URL selectors the result of which are eventually used within the <SourceLink> component. In those selectors, return null early if the useViewOnly hook is true. E.g. in ModuleOverviewWidget/Footer.js:

  const viewOnlyDashboard = useViewOnly();
  const accountSiteURL = useSelect( ( select ) => {
    	if ( viewOnlyDashboard ) {
    		return null;
    	}
    	select( MODULES_ADSENSE ).getServiceReportURL(
    		generateDateRangeArgs( dateRangeDates )
    	);
    } );
  
    return (
    	<SourceLink
    		href={ accountSiteURL }
    		name={ _x( 'AdSense', 'Service name', 'google-site-kit' ) }
    		external
    	/>
    );

Test Coverage

• Add tests similar to SourceLink.test.js for simpler components in the files to update list above. Simple components could be those which only render the <SourceLink> component with a single or couple of selectors. E.g. assets/js/modules/analytics/components/module/ModulePopularPagesWidget/Footer.js

QA Brief

• Setup SiteKit using an admin so that all widgets on the dashboard are rendered. Verify the Source Links in the footer of each widget still links to the appropriate pages as before.
• Share all modules with a non-admin user.
• Login as the non-admin user and view the 'View Only' dashboard. Verify that no Source Links appear in any of the widgets as before. Verify that console errors (similar to the example below) for module settings requests have reduced (as per this comment, not all errors are resolved after implementing this issue). Google Site Kit API Error method:GET datapoint:settings type:modules identifier:analytics error:"Sorry, you are not allowed to do that."

Changelog entry

• Prevent errors on the view-only dashboard from requesting module settings unnecessarily.

[jimmymadon]

This should ideally be done in a consistent way without requiring inline updates in each usage.

@aaemnnosttv Did you have any idea on how this could be achieved? I wasn't quite sure how this could be possible. Would be ideal though! 🙂

[tofumatt]

@jimmymadon and I chatted about this and while ideally this could be done without changes to each selector call, I don't think it's possible. At best you'd need to use a modified useSelect hook that always returned null in certain view contexts, but that would still require changes to each file and knowledge to use said hook in future components.

The straightforward and probably best way is the approach suggested in the IB.

IB looks good, but are there places where we can add tests for this behaviour? Any of these components that have testing infrastructure in place would be easy candidates to add a test for this—can we add some tests to this IB before moving it forward?

[aaemnnosttv]

Did you have any idea on how this could be achieved? I wasn't quite sure how this could be possible. Would be ideal though! 🙂

I was thinking maybe we could create an alternate SourceLink component (or enhance the current) to accept a function for selecting the href. This way the logic for building the link could remain in the parent/consumer but it would only run in a non-view-only context.

This would still require changes in each use but we could avoid adding conditions inline to each selector we have now. How does that sound @tofumatt ?

[tofumatt]

@aaemnnosttv That sounds more roundabout to me… we'd need to pass some sort of function to control whether or not a selector is used in that component… and also logic on how to assemble it because the link URLs rely on report data arguments.

For instance, in DashboardTopEarningPagesWidget, we'd need to pass at least the report type ('content-publisher-overview') as an argument:

site-kit-wp/assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidget.js

Lines 107 to 110 in 7fe9ecf

	analyticsMainURL: select( MODULES_ANALYTICS ).getServiceReportURL(
	'content-publisher-overview',
	generateDateRangeArgs( { startDate, endDate } )
	),

It seems silly to do that and have to pass through a function to control the condition of the select when we can very easily add it inline to each selector—it'd be as simple as:

if ( viewOnlyDashboard ) {
	return null;
}

for any select. If anything, I'd be okay with a new hook that ran a useSelect only when in a particular context, eg. useSelectWhenNotInViewOnly(selectorFunction: Function). That's an abstraction that would be okay, though kinda wordy. but I suppose we could do that. Otherwise I think inlining the condition is much more straightforward to read and understand the logic of.

[tofumatt]

For me, this IB looks good, but assigning it to @aaemnnosttv to get his thoughts on the approach suggested and alternative approaches here.

[tofumatt]

Just to clarify here: conditions need to happen inside selector calls at some point, because the whole issue here is that selectors are triggering resolvers they shouldn't. We can't conditionally call hooks, so we need to place the conditional inside the function that calls these selectors at some point.

I think it's most straightforward to place them inside the existing useSelect calls—this is a pattern we do elsewhere and it's cognitively straightforward.

If we start passing functions that determine the conditions to trigger the selectors, well, we still need inline if ( ! viewOnly ) { [...] } in them.

So I think this IB here is good-to-go, but if I'm being daft and missing something that a proof-of-concept could show with a more straightforward approach, please leave it here or make a draft PR and we can adjust the approach taken here 😅

IB ✅

[jimmymadon]

Indeed, the custom hooks aren't composable - didn't realise that! I tried <ViewOnlyGuard> approach. In some cases, the parent component of <SourceLink> can be wrapped with <ViewOnlyGuard> removing the need to pass the selector callback. But then <SourceLink> will behave inconsistently - requiring either an href string or hrefCallback function and check which one is set. Also, I didn't see the point of the <ViewOnlyGuard> component if we have to pass the 'value'? The guard itself could check if the context is viewOnly.

The above, while comprehensible, is not as straightforward as adding useViewOnly checks in the selectors themselves. So happy to go with the IB for now - and if we end up using useViewOnly excessively in selectors and components, we can think of refactoring using one of the above approaches.

[aaemnnosttv]

But then <SourceLink> will behave inconsistently - requiring either an href string or hrefCallback function and check which one is set.

In my example above, I used a fictitious new component called SelectSourceLink which would expect a select function to be passed for the href. Alternatively, the existing SourceLink href prop could be extended to support such a function instead as well as a string.

Also, I didn't see the point of the <ViewOnlyGuard> component if we have to pass the 'value'? The guard itself could check if the context is viewOnly.

@jimmymadon the point of the value prop would be to allow the guard to work either way (I.e. props.value === useViewOnly()) rather than only guarding one way.

[jimmymadon]

@tofumatt @aaemnnosttv After replacing all getService*URL calls, I found at least two lines (linked below) which eventually end up make settings requests in a viewOnly context. Should I create a new issue to address these?

site-kit-wp/assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidget.js

Line 121 in 6874c2e

isAdSenseLinked: select( MODULES_ANALYTICS ).getAdsenseLinked(),

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

Line 70 in 1f37af0

return select( CORE_USER ).getViewableModules();

[mohitwp]

QA Update ❌

@jimmymadon

On develop branch Traffic Widget analytics source link is not navigating to correct address.

Link navigating to (Site/wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success&slug=adsense#traffic)

cc @wpdarren

[techanvil]

Thanks @mohitwp, good spot. As Jimmy's off the rest of this week I've picked this up and created a followup PR to fix it.

[aaemnnosttv]

Back to you for another pass @mohitwp 👍

[mohitwp]

QA Update ✅

Verified 👍

• Verified issue on both dev and main branch.
• Verified for admin user the Source Links in the footer of each widget still links to the appropriate pages as before.
• For Non admin user 'view only' dashboard. No source links appear in any of the widgets as before except PSI.
• Verified that less console errors are now coming on dev and main branch as mentioned in QAB.

Non admin user 'view only' dashboard:

Recording.119.mp4

Latest release branch:

dev branch:

main branch:

[techanvil]

@tofumatt @aaemnnosttv After replacing all getService*URL calls, I found at least two lines (linked below) which eventually end up make settings requests in a viewOnly context. Should I create a new issue to address these?

site-kit-wp/assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidget.js

Line 121 in 6874c2e

isAdSenseLinked: select( MODULES_ANALYTICS ).getAdsenseLinked(),

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

Line 70 in 1f37af0

return select( CORE_USER ).getViewableModules();

Thanks for raising those, @jimmymadon. I've created an issue for the getAdSenseLinked call: #5493. The getViewableModules one is no longer an issue, as the useSelect is now short circuited in view-only mode.