Add missing handling API errors on UI #1115

jerabekjiri · 2021-10-20T12:55:05Z

A few Examples of error alerts:
(ignore the alert description "Request failed with status code 403", just for testing purposes :))
show success alert after editing namespace (after the redirect to namespace-detail). This wasn't possible before, because of local state alerts and local component AlertList

I was thinking maybe it would be a better idea to move from local alerts in every component to push alerts to global state (context) and display it from one place always? in this case I added it to App component. As I mentioned before, this would make it easier to show alerts after changing URL (redirecting). What do you think about this? @himdel @ZitaNemeckova

Also if there's an API error in fetching table list, currently I just show an empty state and display error alert. I guess this might be confusing for users, this would be probably solved by the 500 error screen, so maybe I should do follow-up PR for that (since we already have UI mockups for it) and later add 500 error screen to all table list responses?

src/loaders/standalone/standalone-loader.tsx

src/components/collection-detail/collection-info.tsx

src/components/execution-environment/repository-form.tsx

src/components/permissions/obect-permission-field.tsx

src/containers/about-modal/about-modal.tsx

jerabekjiri · 2021-11-06T14:58:58Z

The question is should we let the user finish the action (create a new namespace for example) even if this field isn't mandatory (group field is optional, therefore possibly he could create a new namespace and set this later in the app)? or never let user finish the action and keep the create button disabled if an error occurred? @himdel @ZitaNemeckova

himdel · 2021-11-08T15:44:57Z

@jerabekjiri I think we can keep this simple ... the error itself shouldn't stop the user from finishing actions.

If the field is required, the group will never be selected, so the user will not be able to save anyway.

jerabekjiri · 2021-11-08T16:48:54Z

If the field is required, the group will never be selected, so the user will not be able to save anyway.

makes sense, fixed :)

src/components/namespace-modal/namespace-modal.tsx

src/components/namespace-form/namespace-form.tsx

src/components/execution-environment/repository-form.tsx

src/containers/namespace-list/namespace-list.tsx

src/components/user-form/user-form.tsx

src/containers/edit-namespace/edit-namespace.tsx

jerabekjiri · 2021-11-18T14:44:55Z

@himdel thanks for pointing out mistakes, I completely forgot about l10n here, sorry about that

himdel · 2021-11-22T19:45:36Z

No worries, looks like it's all fixed now, not seeing any new issues 👍 :)

jerabekjiri · 2021-11-25T11:13:53Z

@himdel Resolved conflicts, could you review one more before merging, please? :)

jerabekjiri · 2021-11-29T21:10:40Z

I must have messed it up during the conflict resolving, should be fixed and updated now

awcrosby · 2021-12-01T22:19:27Z

(adding this to a few recently updated PRs…) We need CI in this repo that enforces this, but for now please:

Add a line to at least one of your commits that says Issue: AAH-#### (in the future it is best to be the first commit as it gets included in the initial PR description by default)
Ensure when merging that the Issue: AAH-#### is part of commit message via the “Squash ‘n’ Merge” description

Issue: AAH-517

jerabekjiri · 2021-12-03T12:52:59Z

Added Issue: AAH-517 to the latest commit description

ZitaNemeckova

LGTM 👍

himdel · 2022-01-11T17:48:38Z

backporting #1220 (via #1497) depends on this one, backporting as well (#1496)

* add group and colleciton-info missing erros * add EE catch alerts * add more catch alerts * fix double alert popup move alerts from local state to context state * catch errors for (my)namespace * catch alerts in editing namespace catch alerts if something goes wrong while editing namespace + add success alert on edit namespace to global context alerts * show alert on load group error * add more context catch alerts * add catch alert on loading token err * remove global alert * drop about modal alerts * add inline form errors to repository modal * add namespace form errors and rename to onError * add namespace form errors * move alerts to collection detail * rollback context alerts to local * add insights alerts * remove disabling button on error * add l10n to alerts * remove dead code * remove useless alert code * remove isDisabled attr Issue: AAH-517 (cherry picked from commit 446c384)

* add group and colleciton-info missing erros * add EE catch alerts * add more catch alerts * fix double alert popup move alerts from local state to context state * catch errors for (my)namespace * catch alerts in editing namespace catch alerts if something goes wrong while editing namespace + add success alert on edit namespace to global context alerts * show alert on load group error * add more context catch alerts * add catch alert on loading token err * remove global alert * drop about modal alerts * add inline form errors to repository modal * add namespace form errors and rename to onError * add namespace form errors * move alerts to collection detail * rollback context alerts to local * add insights alerts * remove disabling button on error * add l10n to alerts * remove dead code * remove useless alert code * remove isDisabled attr Issue: AAH-517 (cherry picked from commit 446c384) Co-authored-by: Jiří Jeřábek <Jerabekjirka@email.cz>

hendersonreed · 2022-02-16T18:47:34Z

@himdel I'm trying to move this to Done in Jira, and was interested in your thoughts about adding tests for error handling down the line. I think we can stub 500 network responses using https://docs.cypress.io/guides/guides/network-requests#Stub-Responses and then verify that errors appear as they ought to.

I don't think it's something that we need to necessarily add to this in order to consider this work done but wanted to mention it here and get your thoughts on it.

himdel · 2022-02-27T21:50:11Z

@hendersonreed Great idea, I agree we should be testing these as well, and stubbing the responses sounds like a perfect solution. (I guess if we should also test a couple of real failure states where we can, but stubbing allows us to test everything.)

(I've at least added a link from himdel#1 for now.)

The error handling was added in ansible#1115, but in this case, this will always error the first time. The expected flow is: * TokenPage loads, componentDidMount runs getOfflineToken() and fails, render displays a "{{ user_token }}" placeholder * user clicks the "Load token" button, loadToken() calls doOffline() which triggers a reload * TokenPage loads, componentDidMount runs getOfflineToken() and succeds, render displays the token If there's a way of doing this without a full page reload, or of telling that doOffline just happened successfully but getOfflineToken still fails, we should keep the error. But I don't know of either right now. (Short of a double redirect maybe.) No-Issue

…or (#1739) The error handling was added in #1115, but in this case, this will always error the first time. The expected flow is: * TokenPage loads, componentDidMount runs getOfflineToken() and fails, render displays a "{{ user_token }}" placeholder * user clicks the "Load token" button, loadToken() calls doOffline() which triggers a reload * TokenPage loads, componentDidMount runs getOfflineToken() and succeds, render displays the token If there's a way of doing this without a full page reload, or of telling that doOffline just happened successfully but getOfflineToken still fails, we should keep the error. But I don't know of either right now. (Short of a double redirect maybe.) No-Issue

the code has been there from the start, only makes sense in insights mode, and unused since ansible#183 used to be just getUser->setState->loadNamespace, before ansible#1115 added error handling, changing to just loadNamespace

* translate sync alert * RepoSelector: move insights mode switch to BaseHeader, fix extra top padding in insights mode because BaseHeader wraps RepoSelector when present, but not rendering is still present * insights-loader: drop older nav event type support (comes from #715, pre-chrome 2.0) * ActiveUserAPI: simplify getUser, return getActiveUser.data * insights-loader: add FeatureFlagsAPI call; move insights and standalone a step closer * UI to use feature flags in insights mode Issue: AAH-1800 * edit-namespace, namespace-form: drop unused userId, extra getUser call the code has been there from the start, only makes sense in insights mode, and unused since #183 used to be just getUser->setState->loadNamespace, before #1115 added error handling, changing to just loadNamespace * InsightsUserType is unused, index.d.ts has a conflicting type for getUser, update and move there * ActiveUserAPI.getUser, getActiveUser - merge into getUser, no users of these functions can really deal with the insights version * insights-loader - drop auth.getUser wait, already happening in api base *before* getUser * remove extra border in collections documentation navitems in insights mode * convert src/loaders/insights/ to typescript * standalone-loader: sort context params * insights-loader: change activeUser to user * loadContext: shared code to load alerts, featureFlags, settings, user * insights-loader: update title using identifyApp instead of document.title as per https://github.com/RedHatInsights/insights-chrome/blob/master/docs/api.md#update-document-title * ActiveAPI: type getToken, simplify promises

jerabekjiri requested review from himdel and ZitaNemeckova October 20, 2021 13:09

himdel reviewed Oct 27, 2021

View reviewed changes

src/loaders/standalone/standalone-loader.tsx Outdated Show resolved Hide resolved

himdel reviewed Oct 27, 2021

View reviewed changes

src/components/collection-detail/collection-info.tsx Outdated Show resolved Hide resolved

himdel reviewed Oct 27, 2021

View reviewed changes

src/components/execution-environment/repository-form.tsx Outdated Show resolved Hide resolved

himdel reviewed Oct 27, 2021

View reviewed changes

src/components/execution-environment/repository-form.tsx Outdated Show resolved Hide resolved

himdel reviewed Oct 27, 2021

View reviewed changes

src/components/permissions/obect-permission-field.tsx Outdated Show resolved Hide resolved

himdel reviewed Oct 27, 2021

View reviewed changes

src/containers/about-modal/about-modal.tsx Outdated Show resolved Hide resolved

jerabekjiri force-pushed the missing-api-errors branch from 9eaf067 to 7798510 Compare November 6, 2021 11:35

jerabekjiri force-pushed the missing-api-errors branch from c8be045 to 6af5b27 Compare November 7, 2021 01:58

ZitaNemeckova mentioned this pull request Nov 9, 2021

Add check that EE name is correct when typed in #1220

Merged

jerabekjiri requested a review from himdel November 10, 2021 14:21