New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: render errors in Loading component #32464

Merged
merged 7 commits into from Nov 21, 2018

Conversation

Projects
None yet
4 participants
@celiala
Copy link
Contributor

celiala commented Nov 19, 2018

Previously, data fetching errors were not being consistently surfaced to
the user. This commit updates all existing uses of the Loading component
to surface these errors:

  • Some parent components renders data from multiple sources,
    potentially having multiple errors to show the user. This commit also
    extends the Loading component to handle multiple errors.
  • The Certificates page had custom loading logic. This commit replaces
    logic with Loading component.

Also in this commit is some minor cleanup (copy, layout).

Fixes: #24011

Release note (admin ui change): This commit wires up data errors to all
existing uses of the Loading component. Previously, data errors weren't
consistently surfaced.

@celiala celiala requested a review from cockroachdb/admin-ui-prs as a code owner Nov 19, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 19, 2018

This change is Reviewable

@celiala celiala added the fixitday label Nov 19, 2018

@couchand
Copy link
Collaborator

couchand left a comment

This is great, thanks for taking on this task! I left a handful of minor nits & questions, let me know if anything is unclear. Thanks!

Reviewed 2 of 2 files at r1, 11 of 11 files at r2, 1 of 1 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 4 of 4 files at r7.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 37 at r7 (raw file):

import { TreeNode, TreePath } from "./tree";
import "./index.styl";
import {selectLivenessRequestStatus, selectNodeRequestStatus} from "oss/src/redux/nodes";

you should only need to prefix an import with oss if it's CCL code importing the OSS version of precisely the same file (for fallback purposes). otherwise, starting import paths with src should always pick up the right file.

I know that Pete's editor sometimes automatically adds it, perhaps you have the same editor config...


pkg/ui/src/views/reports/containers/range/allocator.tsx, line 67 at r7 (raw file):

    // case, but making major changes to the CachedDataReducer or util.api seems
    // fraught at this point.  We should revisit this soon.
    if (allocator.lastError && allocator.lastError.message === "Forbidden") {

It's hard to know if the other code that does just if (allocator) and loading={!allocator} is being excessively defensive or not, but it makes me nervous to move this dereference before one of those checks without some kind of validation.

It seems like, since the type is just CachedDataReducerState, it should be initialized by Redux on app startup and we shouldn't ever get here with it being falsey. If that logic is sound, we can probably remove the (apparently) excessively defensive checks in the rest of the code. On the other hand, if there's a flaw in that logic, we should add one of those checks here.


pkg/ui/src/views/reports/containers/settings/index.tsx, line 87 at r7 (raw file):

        </Helmet>
        <h1>Cluster Settings</h1>
        <h2></h2> {/* TODO(vilterp): why is this here? just for spacing? */}

looks like it's always been there. 0bf60a6#diff-7fb3b6a2810f0f9be12ec4c2e3f4dff4R81

seems safe to delete unless it throws off the page layout significantly, and even then there's probably a clearer way to achieve the goal.


pkg/ui/src/views/shared/components/loading/index.tsx, line 40 at r7 (raw file):

    } else {
      // Remove null values from Error[].
      const validErrors = _.filter(errors, (e) => { return !!e; });

It doesn't seem like lodash is really helping here.

also the closure can be much terser: e => !!e


pkg/ui/src/views/shared/components/loading/index.tsx, line 80 at r7 (raw file):

      );
    }
    return (

There doesn't seem to be a lot of value special casing a single error now.


pkg/ui/src/views/shared/components/loading/loading.spec.tsx, line 40 at r7 (raw file):

describe("<Loading>", () => {
  describe("renders correctly for various states.", () => {

In test results the nested describe titles are just concatenated with the it titles. There's an opportunity to hack them to make each one just a word or phrase so that they read as sentences when all put together.

I also like to use outer describes structurally to setup each part of the scenario. For instance, I'd recommend organizing these tests something like:

describe "<Loading>"
  describe "when error is null"
    describe "when loading=false"
      it "renders content."
    describe "when loading=true"
      it "renders loading spinner."
  describe "when error is a single error"
    describe "when loading=false"
      it "renders error."
    describe "when loading=true"
      it "renders error anyway"
  describe "when error is a list of errors"
    describe "when no errors are null"
      ...
    describe "when some errors are null"
      ...
    describe "when all errors are null"
      ...

Personally I prefer something like that to come conjunctions and conditionals in a given test title.

Just to clarify, these tests do cover the necessary logic, so I'm happy with them as-is, just thought I'd take the opportunity to share some thoughts here.


pkg/ui/src/views/shared/components/loading/loading.spec.tsx, line 166 at r7 (raw file):

    // Single errors are rendered inside a <pre></pre>;
    // Multiple errors are listed in <ul><li> list.
    const expectedSelector = props.errorCount === 1 ? "pre" : "li";

The markup wraps pres in lis, so could we just always look for the pres? Does ReactWrapper#find do deep searching?

I ask because I really don't like conditionals in a test if we can avoid it.


pkg/ui/src/views/shared/components/loading/loading.spec.tsx, line 170 at r7 (raw file):

      wrapper.find("div." + ERROR_CLASS_NAME).find(expectedSelector),
      props.errorCount,
      "number of errors was not " + props.errorCount,

I really like the message attached to the above assertion, because it translates from what (the length is wrong) to why (element should/should not be visible). This one just rehashes what the default "expected/actual" would already say, so it doesn't really add anything.

@celiala celiala force-pushed the celiala:celiala-use-loading-error branch from 120c0b3 to 89687a8 Nov 21, 2018

@celiala
Copy link
Contributor

celiala left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 37 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

you should only need to prefix an import with oss if it's CCL code importing the OSS version of precisely the same file (for fallback purposes). otherwise, starting import paths with src should always pick up the right file.

I know that Pete's editor sometimes automatically adds it, perhaps you have the same editor config...

Done.


pkg/ui/src/views/reports/containers/range/allocator.tsx, line 67 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

It's hard to know if the other code that does just if (allocator) and loading={!allocator} is being excessively defensive or not, but it makes me nervous to move this dereference before one of those checks without some kind of validation.

It seems like, since the type is just CachedDataReducerState, it should be initialized by Redux on app startup and we shouldn't ever get here with it being falsey. If that logic is sound, we can probably remove the (apparently) excessively defensive checks in the rest of the code. On the other hand, if there's a flaw in that logic, we should add one of those checks here.

Done.

It looks like allocator is derived from KeyedCachedDataReducerState[range_id_key], which could potentially be undefined, so I'll add an extra if (allocator && .. check here.


pkg/ui/src/views/reports/containers/settings/index.tsx, line 87 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

looks like it's always been there. 0bf60a6#diff-7fb3b6a2810f0f9be12ec4c2e3f4dff4R81

seems safe to delete unless it throws off the page layout significantly, and even then there's probably a clearer way to achieve the goal.

Done.


pkg/ui/src/views/shared/components/loading/index.tsx, line 40 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

It doesn't seem like lodash is really helping here.

also the closure can be much terser: e => !!e

Done.


pkg/ui/src/views/shared/components/loading/index.tsx, line 80 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

There doesn't seem to be a lot of value special casing a single error now.

Done.


pkg/ui/src/views/shared/components/loading/loading.spec.tsx, line 40 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

In test results the nested describe titles are just concatenated with the it titles. There's an opportunity to hack them to make each one just a word or phrase so that they read as sentences when all put together.

I also like to use outer describes structurally to setup each part of the scenario. For instance, I'd recommend organizing these tests something like:

describe "<Loading>"
  describe "when error is null"
    describe "when loading=false"
      it "renders content."
    describe "when loading=true"
      it "renders loading spinner."
  describe "when error is a single error"
    describe "when loading=false"
      it "renders error."
    describe "when loading=true"
      it "renders error anyway"
  describe "when error is a list of errors"
    describe "when no errors are null"
      ...
    describe "when some errors are null"
      ...
    describe "when all errors are null"
      ...

Personally I prefer something like that to come conjunctions and conditionals in a given test title.

Just to clarify, these tests do cover the necessary logic, so I'm happy with them as-is, just thought I'd take the opportunity to share some thoughts here.

Done.


pkg/ui/src/views/shared/components/loading/loading.spec.tsx, line 166 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

The markup wraps pres in lis, so could we just always look for the pres? Does ReactWrapper#find do deep searching?

I ask because I really don't like conditionals in a test if we can avoid it.

Done.


pkg/ui/src/views/shared/components/loading/loading.spec.tsx, line 170 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

I really like the message attached to the above assertion, because it translates from what (the length is wrong) to why (element should/should not be visible). This one just rehashes what the default "expected/actual" would already say, so it doesn't really add anything.

Done.

@celiala

This comment has been minimized.

Copy link
Contributor

celiala commented Nov 21, 2018

Thanks for the feedback (incorporated). PTAL, thanks! :)

@couchand
Copy link
Collaborator

couchand left a comment

Awesome, good work.

Reviewed 1 of 21 files at r8, 11 of 11 files at r9, 1 of 1 files at r11, 1 of 1 files at r12, 2 of 2 files at r13, 4 of 4 files at r14.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ui/src/views/reports/containers/settings/index.tsx, line 82 at r2 (raw file):

  render() {
    return (
      <div className="section">

It seems like having just the one className="section" up here is cleaner... is there a reason you moved it below?

@celiala
Copy link
Contributor

celiala left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ui/src/views/reports/containers/settings/index.tsx, line 82 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

It seems like having just the one className="section" up here is cleaner... is there a reason you moved it below?

Oops, was tinkering around and forgot to clean up; will revert back to one className="section" and add a .styl sheet to add padding to just the <p> note.

vilterp and others added some commits Oct 30, 2018

ui: update description of Localities page in debug index
If it was just localities, you could use the data distribution page. The
Localities page offers locations as well.

Release note: None
ui: render errors in Loading component
Previously, data fetching errors were not being consistently surfaced to
the user. This commit updates all existing uses of the Loading component
to surface these errors:

 - Some parent components render data from multiple sources,
   potentially having multiple errors to show the user. This commit also
   extends the Loading component to handle multiple errors.
 - The Certificates page had custom loading logic. This commit replaces
   logic with Loading component.

Also in this commit is some minor cleanup (copy, layout).

Fixes: #24011

Release note (admin ui change): This commit wires up data errors to all
existing uses of the Loading component. Previously, data errors weren't
consistently surfaced.

@celiala celiala force-pushed the celiala:celiala-use-loading-error branch from 89687a8 to 3a08407 Nov 21, 2018

@celiala

This comment has been minimized.

Copy link
Contributor

celiala commented Nov 21, 2018

bors r=couchand
TFTR!

craig bot pushed a commit that referenced this pull request Nov 21, 2018

Merge #32464
32464: ui: render errors in Loading component r=couchand a=celiala

Previously, data fetching errors were not being consistently surfaced to
the user. This commit updates all existing uses of the Loading component
to surface these errors:

 - Some parent components renders data from multiple sources,
   potentially having multiple errors to show the user. This commit also
   extends the Loading component to handle multiple errors.
 - The Certificates page had custom loading logic. This commit replaces
   logic with Loading component.

Also in this commit is some minor cleanup (copy, layout).

Fixes: #24011

Release note (admin ui change): This commit wires up data errors to all
existing uses of the Loading component. Previously, data errors weren't
consistently surfaced.

Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com>
Co-authored-by: Celia La <celia@cockroachlabs.com>
@vilterp

This comment has been minimized.

Copy link
Contributor

vilterp commented Nov 21, 2018

w00t!

@craig

This comment has been minimized.

Copy link

craig bot commented Nov 21, 2018

Build succeeded

@craig craig bot merged commit 3a08407 into cockroachdb:master Nov 21, 2018

2 of 3 checks passed

GitHub CI (Cockroach) TeamCity build started
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@celiala celiala deleted the celiala:celiala-use-loading-error branch Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment