Site-kit-wp: Display errors correctly in module setup/setting flows

[felixarntz]

Bug Description

Currently, error handling in module setup flows is rudimentary, with only the last single error being displayed. The bigger problem though is that if an error occurs for the main request that is used to determine whether the setup should be in loading state, the loading bar will just keep going, and the error will never be displayed.

---

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

Acceptance criteria

• The createErrorStore function should be expanded so that the store includes a new getErrors() selector, which returns an array of all errors in the store. It should also include a hasErrors() selector which returns a boolean based on getErrors().
• There should be a new ErrorNotice component in assets/js/components/ErrorNotice.js which behaves similarly to the existing StoreErrorNotice component (i.e. including the logic for whether or not to return null), except that:
  
  
  
  • It receives an error object as prop instead of the storeName.
  
  
  • It displays the passed error instead of getting it from the store.
  
  
• The existing StoreErrorNotice should internally rely on ErrorNotice, simply passing the retrieved error.
• There should be a StoreErrorNotices component in assets/js/components/StoreErrorNotices.js which behaves similarly like the StoreErrorNotice component (see #1749), except that it displays potentially multiple errors, using the new getErrors() selector on the store passed. If a shouldDisplayError prop is provided, every error should be run through it (as well as the other checks like isPermissionScopeError) to see if it should be displayed. Then the component should display all errors using an ErrorText component for each.
  
  
  
  • Instead of replicating the logic which is now handled by the ErrorNotice above, it should render a <ErrorNotice> for every error.
  
  
• Every module's ErrorNotice component should be renamed to ErrorNotices and use the new StoreErrorNotices. All references should be updated to use that component.

Implementation Brief

createErrorStore additions

• Add a new selectors.getErrors which returns Object.values( state.errors ) as well as state.error if set in a single array
  
  
  
  • Because errors may already contain error, the list should be reduced to a list of unique errors based on error.code and error.message
  
  
• selectors.hasErrors already exists, but it should be updated to use getErrors rather than the current logic which does not account for state.error

New components

• Create a new ErrorNotice component in assets/js/components/ErrorNotice.js
  
  
  
  • Should accept an optional error object as a prop
  
  
  • Everything else is the same as StoreErrorNotice, except the error is sourced from the prop, rather than the store
  
  
  • This component should include the same logic for returning null as is currently present in StoreErrorNotice
  
  
• Create a new StoreErrorNotices component in assets/js/components/StoreErrorNotices.js as a multi-error-capable variant of StoreErrorNotice
  
  
  
  • Requires a storeName prop to be provided
  
  
  • Should accept the same shouldDisplayError prop as ErrorNotice
  
  
  • Selects all errors from the store using the new getErrors selector
  
  
  • Map the errors into ErrorNotice components, passing through the error and shouldDisplayError as props
  
  

Changes to existing components

• Update StoreErrorNotice to use ErrorNotice internally, passing through the error from the store as a prop
• Rename assets/js/modules/adsense/components/common/ErrorNotice.js to ErrorNotices and be updated to use the new StoreErrorNotices internally
• Update references to StoreErrorNotice in modules/*/components to use StoreErrorNotices instead

Tests

It seems that createErrorStore was introduced without tests so this would be a good time to add them for all existing selectors and actions (see existing create-*-store.test.js for examples)

• actions.receiveError
• actions.clearError
• selectors.getErrorForSelector
• selectors.getErrorForAction
• selectors.getError
• selectors.hasErrors
• Add Jest tests for the added getErrors to createErrorStore
• Add Jest tests for StoreErrorNotice and StoreErrorNotices components

QA Brief

1. Install the developer/helper plugin as documented here:
   https://sitekit.withgoogle.com/documentation/using-site-kit-on-a-staging-environment/
2. Go to the "Developer Settings" page for Site Kit.
3. Enter an invalid URL for the "Custom Site URL" field.
4. After saving, clear the browser's session storage
5. Go to the dashboard and for the "Search Funnel" section, there will be only error displayed compared to 2 similar errors previously.

Changelog entry

• Improve error handling during module setup and editing module settings so that any API errors are displayed.

[aaemnnosttv]

@felixarntz

For the condition whether to display a progress bar, JS components that are used in module setup or module settings must not rely on selectors returning undefined which have a resolver. Whenever a selector has a resolver, such logic should rely on hasFinishedResolution for the selector instead.

While this makes sense in general, I'm not sure it makes sense to include as part of this issue. Is it related to the error component changes?

[felixarntz]

@aaemnnosttv

While this makes sense in general, I'm not sure it makes sense to include as part of this issue. Is it related to the error component changes?

The problem is that in _some_ of the module setup components currently if an error occurs the user is never informed about it and instead continues to see the progress bar. This is because the ErrorNotice component is only used in a sub-component which isn't even rendered though because of relying on the data to be there (i.e. the request to be successful). Such components should instead rely on the resolver so that, if an error occurs, the progress bar disappears and the user instead sees the error message.

Otherwise the IB LGTM, but I think this should be added as part of this issue.

[aaemnnosttv]

@felixarntz I've updated the IB to include those changes but I'm a bit concerned that the size of the scope is making this not such a good first issue anymore, even though I don't think there is anything about what needs to be done is overly complex.

It might make more sense to split some of this into separate GFI issues:

1. Add tests for createErrorStore
2. Update conditionals used for early return with based on data selected from the datastore

Then the remaining items in this issue would depend on the above two, but those would have no dependencies of their own and each issue would have a more well-defined scope. This will also help keep the size of the changes down during review which always helps 😄

If you agree and the IB otherwise looks good, I think we can slice this up accordingly, consider the IBs done and move them all right to the backlog.

[felixarntz]

@aaemnnosttv IB LGTM. Regarding your last comment, let's split out the part of updating the progress bar logic, since that is actually entirely unrelated (it's not even blocked by this). I think the changes around the error store component should be kept together in here. Assigning this back to you just for a final pass and potential update of the estimate.

[aaemnnosttv]

@felixarntz this looks better now, thanks for splitting that out. I've brought the estimate down one notch as a result which looks better for a GFI 👍 I think most of the effort here will be around the tests as the datastore and component changes should be fairly easy to implement.

Moving this forward as IB ✅ as we discussed yesterday as there are no other changes to the IB.

[aaemnnosttv]

@asvinb would you please add a QA Brief to the issue above? Let me know if you have any questions about what's needed there, but it just needs to outline any relevant info/steps as to how this should be QA'd.

[asvinb]

@asvinb would you please add a QA Brief to the issue above? Let me know if you have any questions about what's needed there, but it just needs to outline any relevant info/steps as to how this should be QA'd.

@aaemnnosttv QA brief added

[felixarntz]

@aaemnnosttv @asvinb Since #2033 is more critical than this and therefore needs to be in this release (but originally was waiting for this to be done in order to add a few extra tests), let's add test coverage for the new clearErrors and clearError as part of this issue.

[cole10up]

Tested

Installed, activated and setup SK. Installed and activated the developer/helper plugin.

Adjusted the Custom Site URL field to an invalidurl, saved and checked both widget and non widget search console. No errors displayed.

Sending back to @asvinb for adjustments

[asvinb]

@cole10up I just deleted my cookies and local storage and errors are displayed for the widgets as per screenshot below:

@aaemnnosttv Can you confirm the data is cached and we need to clear the cookies/local storage?

[aaemnnosttv]

@asvinb that looks correct to me. Simply changing the custom URL won't trigger different requests to be made for most components on the dashboard because they use false for the url in their requests which results in the same cache key and uses the reference URL from the server side at the time of the request. That means that any existing data for these requests from a previous custom URL would be used. Given the QAB doesn't mention this, I can see why @cole10up sent it back, but to answer your question - yes the cache needs to be flushed after changing the custom URL 👍

[aaemnnosttv]

@asvinb I've updated the QAB to include the missing step. Assigning back to @cole10up for a second pass.

[cole10up]

Retested after clearing session storage:

Still unable to reproduce this.

I tried to clear the entire page cache after saving the invalid URL which then prompts me to relogin and still no errors. I also installed a plugin to clear my session storage after saving the invalid URL, navigating to the dashboard, still no errors.

[felixarntz]

@asvinb @aaemnnosttv I'm not sure I follow the thinking behind the QA Brief steps - how do the changes in this issue affect the dashboard widgets and the errors they display? In my understanding the changes here only affect module setup flows: If there are API errors, now _all_ errors should show instead of just the last one. At a minimum, this part is not currently covered in the QA Brief.

[aaemnnosttv]

Yeah you're right @felixarntz – I got mixed up with the original QAB which didn't include the setup components.

I think the easiest way for @cole10up to verify this is to load the setup for a particular module, and then set the network connection to "Offline" in the browser. Then make different selections in the component which trigger requests that will cause an error to be shown for each unique request.

It's not ideal I think because the errors all have the same message (and wouldn't work for Optimize), but it does show multiple.

Alternatively, we can dispatch receiveError from the console with different arguments to see each one shown in the component.

What do you think?

[cole10up]

Retested

Installed, activated and setup latest release candidate for SK. Navigated to Analytics module in offline mode and clicked the Configure button:

Ran a series of tests around other setup items in offline mode:

Tried changing the date filter on the dashboard in offline mode, no error displayed:

Thoughts @aaemnnosttv or @felixarntz ?

[aaemnnosttv]

QA ✅

Received multiple different errors manually for each module and verified they all displayed in the module's setup component.