site-kit-wp: Add "Retry" button to ReportError component

@hussain-t @bethanylang @aaemnnosttv @tofumatt Based on today’s conversation, I suggest the following refinement here:

• Instead of showing the Retry button on all ReportErrors (i.e. the red border in-widget errors), we limit it by adding a number of exceptions where the button should not be shown if any of the following is true:
  • If the selectorName is not getReport (because then it could be any other type of error that is not from the report API request) - @eugene-manuilov raised this earlier today, and it makes sense to me.
  • If the error is an “insufficient permissions error”, i.e. the user doesn’t have access to this data. In that case, they will have to coordinate with the admin who does have access so that they can get access to. That’s not exactly a quick task (unless they’re sitting right next to them), so we probably shouldn’t show the Retry button there as it’s not going to help.
  • If the error is a “permission scope error”, i.e. the user hasn’t granted certain OAuth scopes. In that case they will see a banner notification to fix this anyway, and retrying the request won’t do it.
  • If the error has a reconnectURL with it, i.e. there is already a CTA URL present which can fix the problem - again retrying the request won’t do it in this case.

In order to implement the last three without duplicating too much, we should rely on helper functions like isInsufficientPermissionsError( error ), isPermissionScopeError( error ), and isAuthError( error ) which we can then call here. The last two should also be used in the existing dispatchAPIError function (which I used as a reference to define the last two conditions above). The first two helper functions already exist, so let’s add an isAuthError to them. We can then use these small helpers in both ReportError and dispatchAPIError as applicable.

Let me know what you think.

@mohitwp When I tested this I was using an HTTP proxy (Proxyman, though Charles or other proxies would work too) and adding a breakpoint to all requests using Tools > Breakpoints > Add Rule:

Whatever HTTP proxy you’re using, adding breakpoints to http://sitekit.10uplabs.com/wp-json/google-site-kit/v1/* will let you control the response from those requests. You can change http://sitekit.10uplabs.com to whatever your testing site’s URL is, and this will let you control whether the Site Kit API request goes through or not.

If you abort the request, it will be an HTTP fetch failure that should trigger the retry button. It’s exactly what I did when testing:

Alternatively, you could probably insert code into the plugin to cause that request to fail, eg: https://github.com/google/site-kit-wp/blob/affa782aab7b74ad44d8dbb55cdc6a5f2d854806/includes/Modules/Search_Console.php#L213. Adding a die('Foo.'); right before this line should work, but I proposed the HTTP proxy because it means using exactly the production code to test.

Does that help?

I’ve narrowed down the issue, and it’s here: https://github.com/google/site-kit-wp/blob/94633b1b3707fc36acdba73fe7647928224685be/assets/js/modules/adsense/components/module/ModuleOverviewWidget/index.js#L113-L127

Essentially, the issue is that we, somewhat simplistically, return the first error we encounter in the AdSense ModuleOverviewWidget as the error to pass to the WidgetReportError component we render. It detects an error that can be retried and offers the user a “reload” button, which actually does reload that resolver’s request. The issue is, as soon as that request reloads successfully (without error), the next error is returned to that variable.

There are four getReport calls with different arguments, each of which fail when the network tab is set to “offline”. If you click “Retry” four times, it actually works:

https://user-images.githubusercontent.com/90871/176799449-6db673d3-9005-4d4b-93ba-aac1f114eb2c.mp4

But that’s obviously not ideal. I don’t think there’s a quick, easy fix for this though; we should probably implement a way for WidgetReportError to—optionally—get an array of selectors to reset rather than just one. I think the best way to do this would be to add a way to pass multiple errors to WidgetReportError. Probably an errors prop or even just allowing the error prop to be a single error or an array of errors. When it’s an array, the retry button would invalidate every resolver.

So I think to solve this, we should:

1. No longer return the first error to the error variable, but instead return each error at https://github.com/google/site-kit-wp/blob/94633b1b3707fc36acdba73fe7647928224685be/assets/js/modules/adsense/components/module/ModuleOverviewWidget/index.js#L113-L127 as an entry in an errors array.
2. Allow WidgetReportError’s error prop to accept an array of errors or a single error. If an array is passed, we run the same checks on each error as we would a single one, ensuring that all errors in the array are “retry-able” before showing the “retry” button.
3. If error is an array and the “Retry” button is clicked, each error’s resolver is invalidated.

Doing this should fix the bug and generally make this retry functionality much more robust.

I don’t think we use this sort of “assign the first error to an error variable passed to WidgetReportError” pattern elsewhere in the plugin, but if we do we should fix this behaviour there as well.

QA Update: ✅

Followed the steps to recreate the issue reported by Felix.

Then, I refreshed main branch. On AdSense Widget on the dashboard, the error message appeared with ‘retry’ button. When I clicked on the button, the data appeared as expected within the widget. I also went through the other modules, i.e. analytics that errored out and ensured that when the retry button was clicked, the data was loaded.

I also ran through the tests highlighted in the original QA relating to the other error messages and the expected behaviour.

https://user-images.githubusercontent.com/73545194/177178144-9f5d6c9d-08a4-4343-a6ea-5cfca003358d.mp4

It seems a page reload (after disabling the “offline” network throttling) fixes it, eg. will allow the content to load. So it isn’t a sessionStorage or localStorage cache issue, as those would persist between page refreshes.

Instead, it’s likely that we aren’t clearing all of the relevant errors/invalidating all the relevant selectors for the AdSense widget’s requests. 🤔

@tofumatt Hmm, could you try with AdSense too? Maybe it’s indeed something odd there only. You should be able to mock everything around AdSense with the tester plugin to get it set up without a real account with data etc.

Make sure to also have the adsenseSetupV2 active, at least I had that active when encountering this problem.

@mohitwp the permission scope error using OAuth can be triggered by following these steps:

1. Set up a module and choose your Google account.
2. Next will appear the OAuth page which is titled Site Kit wants additional access to your Google Account
3. Click on the ‘Cancel’ button and you will be redirected back to Site Kit dashboard and the message will appear Error connecting Site Kit with a description of The Site Kit setup was interrupted because you did not grant the necessary permissions.

Does that help you produce this error message?

QA Update ⚠️

@wpdarren I verified below listed scenarios as per QAB. Can you please review this PR if any other error is pending. I’m not able to produce "permission scope error” using OAuth.

Verified:

• Newly added story has a Retry button. https://google.github.io/site-kit-wp/storybook/develop/?path=/story/components-reporterror--report-error-with-retry-button

• If the error is getReport error than retry button is showing. Tested using proxy as mentioned here

• If the error is an “insufficient permissions error” then Retry button not showing.

• If the error has a with it then also retry button not showing.

am I right in thinking that we were preparing a list of places where these different errors appear so we can test this thoroughly?

@wpdarren These would specifically be in dashboard widgets, but not widget or error-specific; the functionality is generic.

In addition, should we not be testing this with the staging server, to test that the retry buttons go to the correct endpoint. I maybe misunderstood, but thought that some work was done on the service side.

The integration with the service side is specific to the learn more/help links which are not yet added here – there is no service aspect to test here 👍 As such, the current QAB should be sufficient.

@aaemnnosttv @hussain-t I’ve added these additional requirements to the ACs now.

@felixarntz This makes sense to me! Of course support will work with the QA team to test this thoroughly to ensure that this catches all of the necessary exceptions.

IB ✔️