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
createErrorStorefunction should be expanded so that the store includes a newgetErrors()selector, which returns an array of all errors in the store. It should also include ahasErrors()selector which returns a boolean based ongetErrors(). - There should be a new
ErrorNoticecomponent inassets/js/components/ErrorNotice.jswhich behaves similarly to the existingStoreErrorNoticecomponent (i.e. including the logic for whether or not to returnnull), except that:- It receives an
errorobject as prop instead of thestoreName. - It displays the passed
errorinstead of getting it from the store.
- It receives an
- The existing
StoreErrorNoticeshould internally rely onErrorNotice, simply passing the retrievederror. - There should be a
StoreErrorNoticescomponent inassets/js/components/StoreErrorNotices.jswhich behaves similarly like theStoreErrorNoticecomponent (see #1749), except that it displays potentially multiple errors, using the newgetErrors()selector on the store passed. If ashouldDisplayErrorprop is provided, every error should be run through it (as well as the other checks likeisPermissionScopeError) to see if it should be displayed. Then the component should display all errors using anErrorTextcomponent for each.- Instead of replicating the logic which is now handled by the
ErrorNoticeabove, it should render a<ErrorNotice>for every error.
- Instead of replicating the logic which is now handled by the
- Every module’s
ErrorNoticecomponent should be renamed toErrorNoticesand use the newStoreErrorNotices. All references should be updated to use that component.
Implementation Brief
createErrorStore additions
- Add a new
selectors.getErrorswhich returnsObject.values( state.errors )as well asstate.errorif set in a single array- Because
errorsmay already containerror, the list should be reduced to a list of unique errors based onerror.codeanderror.message
- Because
selectors.hasErrorsalready exists, but it should be updated to usegetErrorsrather than the current logic which does not account forstate.error
New components
- Create a new
ErrorNoticecomponent inassets/js/components/ErrorNotice.js- Should accept an optional
errorobject as a prop - Everything else is the same as
StoreErrorNotice, except theerroris sourced from the prop, rather than the store - This component should include the same logic for returning
nullas is currently present inStoreErrorNotice
- Should accept an optional
- Create a new
StoreErrorNoticescomponent inassets/js/components/StoreErrorNotices.jsas a multi-error-capable variant ofStoreErrorNotice- Requires a
storeNameprop to be provided - Should accept the same
shouldDisplayErrorprop asErrorNotice - Selects all errors from the store using the new
getErrorsselector - Map the errors into
ErrorNoticecomponents, passing through theerrorandshouldDisplayErroras props
- Requires a
Changes to existing components
- Update
StoreErrorNoticeto useErrorNoticeinternally, passing through theerrorfrom the store as a prop - Rename
assets/js/modules/adsense/components/common/ErrorNotice.jstoErrorNoticesand be updated to use the newStoreErrorNoticesinternally - Update references to
StoreErrorNoticeinmodules/*/componentsto useStoreErrorNoticesinstead
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.receiveErroractions.clearErrorselectors.getErrorForSelectorselectors.getErrorForActionselectors.getErrorselectors.hasErrors- Add Jest tests for the added
getErrorstocreateErrorStore - Add Jest tests for
StoreErrorNoticeandStoreErrorNoticescomponents
QA Brief
- Install the developer/helper plugin as documented here:
https://sitekit.withgoogle.com/documentation/using-site-kit-on-a-staging-environment/ - Go to the “Developer Settings” page for Site Kit.
- Enter an invalid URL for the “Custom Site URL” field.
- After saving, clear the browser’s session storage
- 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)
@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
clearErrorsandclearErroras 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.