site-kit-wp: Requests for settings fail for view-only users on dashboard

@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?

https://github.com/google/site-kit-wp/blob/6874c2e65ac45e4c1da65473a9a66669a8db3d6e/assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidget.js#L121

https://github.com/google/site-kit-wp/blob/1f37af065308315c8b8db350b08a5e46de7e8e10/assets/js/components/DashboardMainApp.js#L70

Thanks for raising those, @jimmymadon. I’ve created an issue for the getAdSenseLinked call: https://github.com/google/site-kit-wp/issues/5493. The getViewableModules one is no longer an issue, as the useSelect is now short circuited in view-only mode.

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.

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.

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.

I too am not big on the lack of composability with the proof-of-concept approach in PR #5428. I ultimately think all other suggested ways of dealing with this are more complicated than placing the view-only guard directly in the selectors. It gives us the most flexibility and clarity around when/how selectors are called.

Let’s go with that approach as I think, most importantly, it’s the most cognitively straightforward. I think it’s also easiest to implement, but I think mostly it’s easiest to wrap one’s head around 🙂

@jimmymadon looking at your POC, I like the simplicity of solving this with a custom hook but I’m a bit hesitant to go this route because these special useSelect variants aren’t composable. That is, if we wanted an in-view select that was also not-view-only, we would be forced to use one and add the condition inline or create a new hook that did both to keep the same kind of API which is also less clean the more conditions are added.

I suppose the argument for useInView select at the time was that we had a large existing code base to apply it to where adding inline conditions to them all would be a substantially larger effort.

I also think the conditional rendering + hooks problem will come up again, so maybe a generic <ViewOnlyGuard value={bool}>children to conditionally render</ViewOnlyGuard> might be a better solution here? Again, this would require moving the selection of the href URL to a useSelect that is called within the conditionally rendered part. This would truly make the hook conditionally invoked.

e.g.

<ViewOnlyGuard value={false}>
	<SelectSourceLink href={ (select) => select(...).getServiceURL() } />
</ViewOnlyGuard>

With this structure, SourceLinks wouldn’t need the view-only condition baked in either.

In this case, there isn’t many instances to update I think so perhaps it’s not worth it but I see this being the most scalable.

Unless @tofumatt changes his mind on the approach here, let’s go with the approved IB.

@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?