Add "Retry" button to regular JS error notices in the plugin

[felixarntz]

Follow-up to #5236: In addition to the ReportErrors, a retry button should also be added to similar JS-based errors that appear outside of Site Kit widgets - specifically in module setup flows and module settings.

---

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

Acceptance criteria

• The ErrorNotice component (primarily used throughout setup flows and module settings) should also display a "Retry" button if the provided error comes with the necessary selectorData defined on it.
• The logic for whether to display the "Retry" button or not should be similar to #5236.
• When the user clicks the button, the corresponding store selector should also be invalidated.

Implementation Brief

• In the ErrorNotice component, when an error has a selectorData argument: render a Button component with the following:
  • Children should be __( 'Retry' ).
  • Define a callback function for the retry handler which dispatches the invalidateResolution action like in #5236/https://github.com/google/site-kit-wp/blob/741869a1aeef844d01d946de37a9d1af60d699f8/assets/js/components/ReportError.js#L132-L140.
  • Pass the above callback function to the Button component's onClick prop.
• It would be worth extracting at least the logic for what errors are considered "retry-able" from https://github.com/google/site-kit-wp/blob/741869a1aeef844d01d946de37a9d1af60d699f8/assets/js/components/ReportError.js#L122-L128 into a shared function used between both ErrorNotice and ReportError. Any other code/logic that can be shared as well, should be.

Test Coverage

• Add tests to ensure the error notice renders/doesn't render the button and runs the callback based on the error passed to it.

QA Brief

• Clear your sites session storage to get rid of any locally stored data
• Force an error by for example intercepting a module data request (http://YOUR_URL/index.php?rest_route=%2Fgoogle-site-kit%2Fv1%2Fmodules%2Fanalytics%2Fdata%2Freport&metrics%5B0%5D%5Bexpression%5D=ga%3Ausers&dimensions%5B0%5D%5Bname%5D=ga%3Adate&startDate=2022-09-21&endDate=2022-10-18&_locale=user)
  • I use tweak for this, and just set the URL above to always return a 403 error header.
  • The above example URL needs analytics enabled to work
• Reload the page and ensure that the blocked module fails, and displays a notice together with a retry button
• Without refreshing the page, turn off the request intercepting (the toggle next to the intercepted route in tweak), and click the retry button
• Ensure that the data loads correctly

Changelog entry

• Add a "Retry" button for most errors in the plugin, except for some auth and other select errors.

[felixarntz]

@eclarke1 @FlicHollis This is a follow-up task for #5236 that we had forgotten about somewhat. It should be fairly straightforward to address from an engineering perspective. With that said, this is not blocking anything, so it can happen individually without directly being tied to another effort. If we could get this added in the next month would be great.

[FlicHollis]

Hi @felixarntz thanks for highlighting this. I have added the Sprint 79 (which started today)

[eugene-manuilov]

IB ✔️

[techanvil]

Hi @felixarntz, looking into this, and having discussed with @makiost, we think the issue may need to be reconsidered.

It doesn't look like any of the existing ErrorNotice components will be retryable, as none of the corresponding errors have the necessary selectorData to achieve the retry. This is because the selectorData needed to retry an error is created by getErrorForSelector, but when you search for usage of getErrorForSelector you'll see it's not used for the errors which are used to render ErrorNotice. ErrorNotice is predominantly rendered by StoreErrorNotices which retrieves the list of errors via getErrors; it's also rendered in a couple of User Input components but again without the use of getErrorForSelector.

I'm not sure if this was simply overlooked at the point of raising the issue or if there is some additional context that would make this still viable - or if I may have missed something. Please could you take a look and see what you think?

cc @aaemnnosttv

[felixarntz]

@techanvil @aaemnnosttv Good catch, I had missed that these values were only being added as part of getErrorForSelector. In that case, we may need to define another issue though to address this problem - I do think it should be possible to get the selectorData connected to those errors more reliably.

I'm thinking we could do something like the following:

• Modify the getErrors and getError selectors to always include the selectorData on an error if applicable and then remove that logic from getErrorForSelector.
• More specifically, in getErrors, we can rely on the baseName and args that are stored in the error keys to define the necessary data (only if these are defined of course, not every error has baseName and args).
• In getError we could do the same if any error is found, simply relying on the passed parameters.
• In addition, we would need to change both getErrors and getError to use createRegistrySelector so that we can then check on the current store whether a selector with the baseName exists. If so, this error is indeed for a selector and we should amend the error object with the relevant data under selectorData. Otherwise/if not, this error is not for a selector, so we shouldn't add selectorData to it.

Let me know what you think. This should definitely be a separate issue if we agree on doing this, my take is this would be feasible though. We may want to memoize the operation so that it doesn't happen for every single selector call if the data hasn't changed.

[techanvil]

Hi @felixarntz, that sounds good to me, one thing to note is that args is MD5 encoded in the error key, so we can't extract the args from the key, I think we should instead store them as a property of the error object itself. Obviously this has some implications on memory usage but I don't think it will have much impact in practice. Does that sound OK to you?

[felixarntz]

@techanvil Ah right, I had forgotten about that. Storing the args somewhere with the error makes sense, however we should ensure the format of errors as returned from e.g. getErrors() remains the same. The args should still only be included in an error that has selectorData, as part of that.

So it may be safest to store them in a separate property in the data store, e.g. under the same baseName plus MD5 hash, and then look them up for the cases where it is needed, to add them to the returned error(s). This way there's no risk of accidentally including them in some "public" data where they shouldn't be.

[techanvil]

Hey @felixarntz, that sounds good to me. However, something else has occurred to me, which is that we'd be relying on following a pattern, which has the potential to go out of sync and cause problems like false correlation of errors to selectors.

Specifically, by using baseName to look up the existence of a selector by that name - if an error has been stored with a baseName that happens to match a selector name, but is in fact unrelated, that will result in an incorrect selector lookup with unexpected consequences.

It's not too hard to think of a scenario where this might occur, either simply through accidental use of receiveError without being aware of the pattern, or via some renaming or refactoring, or potentially by secondary use of receiveError without being aware of it (e.g. by using it via a fetch store)...

I've taken a look through the codebase and I can't see that there are currently any situations which result in a false correlation. However it does seem a bit of a risk...

It's hard to think of a great way to mitigate it. One thing that occurred to me is that instead of using baseName to lookup the selector, we instead pass selectorData as an explicit argument to receiveError (and to the generated fetch store methods which call receiveError)... However this would be a bit unwieldy and we'd need to go through a lot of code to add the selectorData args... and, ultimately it would still have potential to go out of sync!

On balance it could still be worth the risk and we just need to take care and document this pattern. What do you think?

[felixarntz]

@techanvil

Specifically, by using baseName to look up the existence of a selector by that name - if an error has been stored with a baseName that happens to match a selector name, but is in fact unrelated, that will result in an incorrect selector lookup with unexpected consequences.

Your thought makes sense to me. However, the purpose of baseName is to exclusively refer to either a selector or an action name, so there shouldn't be a chance for conflicts. baseName should not be (and I believe is not) used on any error to pass something arbitrary. Most of these usages come from the automated fetch infrastructure anyway where this is tied to an action or selector name.

It's hard to think of a great way to mitigate it. One thing that occurred to me is that instead of using baseName to lookup the selector, we instead pass selectorData as an explicit argument to receiveError (and to the generated fetch store methods which call receiveError)... However this would be a bit unwieldy and we'd need to go through a lot of code to add the selectorData args... and, ultimately it would still have potential to go out of sync!

I think this would require too much of "manually" taking care of passing this information in a ton of places across the codebase. I think this would be a greater area of potential accidental oversight, in comparison to the current baseName approach which is fairly "automatic" via the fetch store infrastructure.

On balance it could still be worth the risk and we just need to take care and document this pattern. What do you think?

I think it makes sense to stick with the current pattern, where baseName should always be either a selector name or action name (typically handled by the fetch store). +1 on enhancing the documentation around it as needed.

[techanvil]

Thanks @felixarntz - sounds sensible enough to me. I've added an issue to address this, building on your initial set of bullet points: https://github.com/google/site-kit-wp/issues/5618.

[techanvil]

Code looks good but this issue is dependant on #5618 in order to be functionally testable, as such is on hold until that issue has been implemented. @makiost, I'll assign back to you and move to Execution so you can add a QAB when this is ready for testing.

[FlicHollis]

Hi @makiost @aaemnnosttv @techanvil I am removing this ticket from Sprint 83(and will not add to Sprint 84) as we are still waiting on the resolution on the AC in 5618. We can add this to a sprint once 5618 is in execution is that sounds ok to everyone? Thanks cc @eclarke1

[mohitwp]

@wpdarren @makiost I tried to generate error using Tweak but unable to generate. @wpdarren Can you please check if it works at your end ?

[wpdarren]

@mohitwp no, I was unable to trigger the error either with Tweak chrome extension.

Please could you have a chat with @makiost and get some steps on how to run and test this?

[mohitwp]

@makiost me and @wpdarren are not able to recreate the error using tweak chrome extension. Can you please provide steps how to test this ?

[maciejost]

Hi @mohitwp @wpdarren!

When using tweak, make sure that the play button is clicked, and that the interception is toggled on. I've attached a screenshot containing my setup used to force the routes to fail. I'll also update the QA brief with some more pointers and a more reliable route right away.

[mohitwp]

QA Update ✅

• Verified on dev.
• Trigger multiple errors using tweak.
• Getting "retry button' for the errors which can be retriable.
• Not showing "retry button" for the error which is not retriable.
• On clicking "Retry Button" correct data is getting load.

https://user-images.githubusercontent.com/94359491/196734938-4719b54d-c21c-45fe-a650-f2d0b1e1d184.mp4

[felixarntz]

Approval ❌

@makiost @techanvil @tofumatt @mohitwp There are a few problems here:

• Most importantly, the QA Brief is not accurate, meaning that the above QA also does not actually cover the ACs defined here. It needs to be clarified that this affects module setup and editing module settings, not the SK dashboard. Hence, the request that needs to be intercepted cannot be a report request either (as those aren't used in module setup and module settings).
• The PR may have some issues as well (making the above point more important):
  • The comment https://github.com/google/site-kit-wp/pull/5555/files#r924525047 was never addressed, but it is indeed a problem, the getReport restriction should only be applied in the ReportError component specifically. For the ErrorNotice component, any error with selector data (except those special cases already present in the new helper function) should be considered.
  • It still accesses error.selectorData, shouldn't we now use the new selector from #5618 (which this issue was blocked by)?

[wpdarren]

QA Update: ✅

Verified:

• I followed the instructions in the QAB. The error message appears along with the 'retry' button. When the tweak rule was disabled and the button clicked, the Tag Manager screen reloaded with no error message.
• Tag Manager was successfully set up after the retry button was clicked and a Tag manager account and container was selected.

https://user-images.githubusercontent.com/73545194/197586382-8145712b-62ea-44ac-afd1-086fd202c29b.mp4

[felixarntz]

Approval ✅

Thanks @techanvil @tofumatt @wpdarren! Technically looks all good to me, just visually it's a bit odd having a primary CTA button in that inline notice, especially since there'll often be another actual primary CTA button in related UI.

That is not super critical and also wasn't outlined in the ACs here, so I've opened a separate follow-up issue #6055 to address it.