Site-kit-wp: Fix loading state logic ignoring errors in components

Created on 9 Sep 2020 · 7Comments · Source: google/site-kit-wp

This started as part of #1859, but was since separated out to keep issue scope more manageable. Some module components (mostly setup/settings-related) currently have the problem that the logic for displaying the overall ProgressBar depends on data being returned from an API - however, if an error occurs during an API request, that data will never be populated, keeping the component in an infinite loading state.

Instead of relying on selector-based data to exist, it should be relied upon whether the respective resolver has finished, which will work for both the success and error case.

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

Acceptance criteria

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.

Implementation Brief

Update conditionals used for early return with <ProgressBar> when data selected from the datastore === undefined to use ! hasFinishedResolution selector instead (only for selectors with a resolver).
Note, not all selectors have a dedicated selector but use a parent selector which has a resolver. In this case, the "closest" resolver in the selector chain should be checked.
Also, special care should be taken to ensure bugs are not introduced by changing the conditional to return when something is undefined. It will now be possible for these selected values to be undefined (such as in the event of an error) beyond the progress bar conditional, so we should pay extra attention to ensure potential type errors are handled.
- assets/js/modules/adsense/components/common/AccountSelect.js
- assets/js/modules/adsense/components/common/SiteSteps.js
- assets/js/modules/adsense/components/common/UserProfile.js
- assets/js/modules/adsense/components/settings/SettingsEdit.js
- assets/js/modules/adsense/components/setup/SetupMain.js
- assets/js/modules/analytics/components/common/AccountSelect.js
- assets/js/modules/analytics/components/common/AccountCreate/index.js
- assets/js/modules/tagmanager/components/common/AccountCreate.js
- assets/js/modules/tagmanager/components/common/AccountSelect.js
- assets/js/modules/tagmanager/components/common/ContainerSelect.js
Similar to the last bullet, some conditionals used in the same way are missing this check and are not checking undefined so they only need the same resolver check added
_You can see an example of this done correctly in assets/js/modules/analytics/components/settings/SettingsEdit.js_
- assets/js/modules/analytics/components/common/AccountCreateLegacy.js
- assets/js/modules/analytics/components/common/ProfileSelect.js
- assets/js/modules/analytics/components/common/PropertySelect.js

QA Brief

Set up Site Kit on your site;
Activate and connect AdSense, Analytics, and Tag Manager modules;
Make sure that setup and settings forms of these modules work as it worked before:
- when data is being fetched, the progress bar is shown;
- when data has been loaded, the progress bar is hidden, and the actual form elements appear.

Changelog entry

Fix several UI loading state issues across module setup flows.

P1 Bug

Source

felixarntz

All 7 comments

@aaemnnosttv I've copied the relevant piece of the IB from #1859, assigning to you for double-checking and providing an estimate.

felixarntz on 9 Sep 2020

👍1

IB ✅

felixarntz on 23 Sep 2020

@eugene-manuilov Can you add QA Brief for this one?

felixarntz on 27 Oct 2020

QA Brief is added.

eugene-manuilov on 28 Oct 2020

👍1

QA ⚠️

@eugene-manuilov While testing something else, I ran into a problem which is likely a regression caused by the changes here. What I did was the following:

Go to Analytics setup.
Choose an Analytics account and property, or rely on the preselected ones if there are.
Choose another account.
Now I get an error web property: UA-xxxxxx-x not found.. For some reason, after updating the account, the plugin makes a request to the profiles datapoint with the _previously_ selected property. That property of course is not part of the newly selected account though (you can already tell that because the xxxxxx out of the UA-xxxxxx-x ID must match the account ID), therefore it isn't found. I assume there is some out of sync inconsistency introduced here: When selecting another account, we need to update both the account ID and property ID (the latter needs to be immediately set to its "empty" value) to avoid such a request.

Screenshot:
Screenshot 2020-10-30 at 06 33 34