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

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.

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Comments: 17 (4 by maintainers)

Most upvoted comments

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

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