Skip to content

Commit

Permalink
Return a valid error when using non-existing namespaces (#1491)
Browse files Browse the repository at this point in the history
  • Loading branch information
Andres Martinez Gotor authored Feb 3, 2020
1 parent a60d687 commit 4927c70
Show file tree
Hide file tree
Showing 20 changed files with 289 additions and 15 deletions.
43 changes: 42 additions & 1 deletion dashboard/src/actions/namespace.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import {
createNamespace,
errorNamespaces,
fetchNamespaces,
getNamespace,
postNamespace,
receiveNamespace,
receiveNamespaces,
requestNamespace,
setNamespace,
} from "./namespace";

Expand Down Expand Up @@ -108,7 +111,7 @@ describe("createNamespace", () => {
expect(store.getActions()).toEqual(expectedActions);
});

it("dispatches errorNamespace if error creating a namespace", async () => {
it("dispatches errorNamespace if error getting a namespace", async () => {
const err = new Error("Bang!");
Namespace.create = jest.fn().mockImplementationOnce(() => Promise.reject(err));
const expectedActions = [
Expand All @@ -123,3 +126,41 @@ describe("createNamespace", () => {
expect(store.getActions()).toEqual(expectedActions);
});
});

describe("getNamespace", () => {
it("dispatches requested namespace", async () => {
const ns = { metadata: { name: "default" } };
Namespace.get = jest.fn(() => ns);
const expectedActions = [
{
type: getType(requestNamespace),
payload: "default",
},
{
type: getType(receiveNamespace),
payload: ns,
},
];
const r = await store.dispatch(getNamespace("default"));
expect(r).toBe(true);
expect(store.getActions()).toEqual(expectedActions);
});

it("dispatches errorNamespace if error creating a namespace", async () => {
const err = new Error("Bang!");
Namespace.get = jest.fn().mockImplementationOnce(() => Promise.reject(err));
const expectedActions = [
{
type: getType(requestNamespace),
payload: "default",
},
{
type: getType(errorNamespaces),
payload: { err, op: "get" },
},
];
const r = await store.dispatch(getNamespace("default"));
expect(r).toBe(false);
expect(store.getActions()).toEqual(expectedActions);
});
});
25 changes: 25 additions & 0 deletions dashboard/src/actions/namespace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ import { ActionType, createAction } from "typesafe-actions";
import Namespace from "../shared/Namespace";
import { IResource, IStoreState } from "../shared/types";

export const requestNamespace = createAction("REQUEST_NAMESPACE", resolve => {
return (namespace: string) => resolve(namespace);
});
export const receiveNamespace = createAction("RECEIVE_NAMESPACE", resolve => {
return (namespace: IResource) => resolve(namespace);
});

export const setNamespace = createAction("SET_NAMESPACE", resolve => {
return (namespace: string) => resolve(namespace);
});
Expand All @@ -24,6 +31,8 @@ export const errorNamespaces = createAction("ERROR_NAMESPACES", resolve => {
export const clearNamespaces = createAction("CLEAR_NAMESPACES");

const allActions = [
requestNamespace,
receiveNamespace,
setNamespace,
receiveNamespaces,
errorNamespaces,
Expand Down Expand Up @@ -60,3 +69,19 @@ export function createNamespace(
}
};
}

export function getNamespace(
ns: string,
): ThunkAction<Promise<boolean>, IStoreState, null, NamespaceAction> {
return async dispatch => {
try {
dispatch(requestNamespace(ns));
const namespace = await Namespace.get(ns);
dispatch(receiveNamespace(namespace));
return true;
} catch (e) {
dispatch(errorNamespaces(e, "get"));
return false;
}
};
}
6 changes: 3 additions & 3 deletions dashboard/src/components/DeploymentForm/DeploymentForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ class DeploymentForm extends React.Component<IDeploymentFormProps, IDeploymentFo
}

public render() {
const { namespace } = this.props;
if (this.props.error) {
const { namespace, error } = this.props;
if (error) {
return (
<ErrorSelector
error={this.props.error}
error={error}
namespace={namespace}
action="create"
resource={this.state.latestSubmittedReleaseName}
Expand Down
2 changes: 1 addition & 1 deletion dashboard/src/components/ErrorAlert/NotFoundErrorAlert.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import ErrorPageHeader from "./ErrorAlertHeader";
import { namespaceText } from "./helpers";

interface INotFoundErrorPageProps {
header?: string;
header?: string | JSX.Element;
children?: JSX.Element;
resource?: string;
namespace?: string;
Expand Down
6 changes: 6 additions & 0 deletions dashboard/src/components/ErrorBoundary/ErrorBoundary.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { mount } from "enzyme";
import * as React from "react";
import ErrorBoundary from ".";
import UnexpectedErrorPage from "../../components/ErrorAlert/UnexpectedErrorAlert";
import { UnexpectedErrorAlert } from "../ErrorAlert";

// tslint:disable:no-console
Expand Down Expand Up @@ -66,3 +67,8 @@ describe("ErrorBoundary around a component", () => {
expect(console.error).not.toHaveBeenCalled();
});
});

it("renders an error if it exists as a property", () => {
const wrapper = mount(<ErrorBoundary error={new Error("boom!")} children={<></>} />);
expect(wrapper.find(UnexpectedErrorPage).text()).toContain("boom!");
});
12 changes: 12 additions & 0 deletions dashboard/src/components/ErrorBoundary/ErrorBoundary.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import * as React from "react";

import UnexpectedErrorPage from "../../components/ErrorAlert/UnexpectedErrorAlert";
import { UnexpectedErrorAlert } from "../ErrorAlert";

interface IErrorBoundaryProps {
error?: Error;
children: React.ReactChildren | React.ReactNode | string;
}

Expand All @@ -15,6 +18,15 @@ class ErrorBoundary extends React.Component<IErrorBoundaryProps, IErrorBoundaryS

public render() {
const { error } = this.state;
if (this.props.error) {
return (
<UnexpectedErrorPage
raw={true}
showGenericMessage={false}
text={this.props.error.message}
/>
);
}
return <React.Fragment>{error ? this.renderError() : this.props.children}</React.Fragment>;
}

Expand Down
31 changes: 30 additions & 1 deletion dashboard/src/components/Header/Header.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const defaultProps = {
push: jest.fn(),
setNamespace: jest.fn(),
createNamespace: jest.fn(),
getNamespace: jest.fn(),
};
it("renders the header links and titles", () => {
const wrapper = shallow(<Header {...defaultProps} />);
Expand Down Expand Up @@ -54,9 +55,10 @@ it("renders the namespace switcher", () => {
);
});

it("call setNamespace when selecting a namespace", () => {
it("call setNamespace and getNamespace when selecting a namespace", () => {
const setNamespace = jest.fn();
const createNamespace = jest.fn();
const getNamespace = jest.fn();
const namespace = {
current: "foo",
namespaces: ["foo", "bar"],
Expand All @@ -67,6 +69,7 @@ it("call setNamespace when selecting a namespace", () => {
setNamespace={setNamespace}
namespace={namespace}
createNamespace={createNamespace}
getNamespace={getNamespace}
/>,
);

Expand All @@ -76,5 +79,31 @@ it("call setNamespace when selecting a namespace", () => {
onChange("bar");

expect(setNamespace).toHaveBeenCalledWith("bar");
expect(getNamespace).toHaveBeenCalledWith("bar");
expect(createNamespace).not.toHaveBeenCalled();
});

it("doesn't call getNamespace when selecting all namespaces", () => {
const setNamespace = jest.fn();
const getNamespace = jest.fn();
const namespace = {
current: "foo",
namespaces: ["foo", "bar"],
};
const wrapper = shallow(
<Header
{...defaultProps}
setNamespace={setNamespace}
namespace={namespace}
getNamespace={getNamespace}
/>,
);

const namespaceSelector = wrapper.find("NamespaceSelector");
expect(namespaceSelector).toExist();
const onChange = namespaceSelector.prop("onChange") as (ns: any) => void;
onChange("_all");

expect(setNamespace).toHaveBeenCalledWith("_all");
expect(getNamespace).not.toHaveBeenCalled();
});
9 changes: 8 additions & 1 deletion dashboard/src/components/Header/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { NavLink } from "react-router-dom";
import logo from "../../logo.svg";

import { INamespaceState } from "../../reducers/namespace";
import { definedNamespaces } from "../../shared/Namespace";
import HeaderLink, { IHeaderLinkProps } from "./HeaderLink";
import NamespaceSelector from "./NamespaceSelector";

Expand All @@ -22,6 +23,7 @@ interface IHeaderProps {
push: (path: string) => void;
setNamespace: (ns: string) => void;
createNamespace: (ns: string) => Promise<boolean>;
getNamespace: (ns: string) => void;
}

interface IHeaderState {
Expand Down Expand Up @@ -74,6 +76,7 @@ class Header extends React.Component<IHeaderProps, IHeaderState> {
defaultNamespace,
authenticated: showNav,
createNamespace,
getNamespace,
} = this.props;
const header = `header ${this.state.mobileOpen ? "header-open" : ""}`;
const submenu = `header__nav__submenu ${
Expand Down Expand Up @@ -119,6 +122,7 @@ class Header extends React.Component<IHeaderProps, IHeaderState> {
onChange={this.handleNamespaceChange}
fetchNamespaces={fetchNamespaces}
createNamespace={createNamespace}
getNamespace={getNamespace}
/>
<ul className="header__nav__menu" role="menubar">
<li
Expand Down Expand Up @@ -182,9 +186,12 @@ class Header extends React.Component<IHeaderProps, IHeaderState> {
};

private handleNamespaceChange = (ns: string) => {
const { pathname, push, setNamespace } = this.props;
const { pathname, push, setNamespace, getNamespace } = this.props;
const to = pathname.replace(/\/ns\/[^/]*/, `/ns/${ns}`);
setNamespace(ns);
if (ns !== definedNamespaces.all) {
getNamespace(ns);
}
if (to !== pathname) {
push(to);
}
Expand Down
8 changes: 8 additions & 0 deletions dashboard/src/components/Header/NamespaceSelector.scss
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,11 @@
bottom: -0.3em;
width: 20px;
}

.NamespaceTooltipError {
pointer-events: auto;
&:hover {
visibility: visible;
opacity: 1;
}
}
31 changes: 31 additions & 0 deletions dashboard/src/components/Header/NamespaceSelector.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const defaultProps = {
defaultNamespace: "kubeapps-user",
onChange: jest.fn(),
createNamespace: jest.fn(),
getNamespace: jest.fn(),
};

it("renders the given namespaces with current selection", () => {
Expand Down Expand Up @@ -98,3 +99,33 @@ it("opens the modal to add a new namespace and creates it", async () => {
wrapper.update();
expect(wrapper.find(NewNamespace).prop("modalIsOpen")).toBe(false);
});

it("fetches namespaces and retrive the current namespace", () => {
const fetchNamespaces = jest.fn();
const getNamespace = jest.fn();
shallow(
<NamespaceSelector
{...defaultProps}
fetchNamespaces={fetchNamespaces}
getNamespace={getNamespace}
namespace={{ current: "foo", namespaces: [] }}
/>,
);
expect(fetchNamespaces).toHaveBeenCalled();
expect(getNamespace).toHaveBeenCalledWith("foo");
});

it("doesnt' get the current namespace if all namespaces is selected", () => {
const fetchNamespaces = jest.fn();
const getNamespace = jest.fn();
shallow(
<NamespaceSelector
{...defaultProps}
fetchNamespaces={fetchNamespaces}
getNamespace={getNamespace}
namespace={{ current: "_all", namespaces: [] }}
/>,
);
expect(fetchNamespaces).toHaveBeenCalled();
expect(getNamespace).not.toHaveBeenCalled();
});
15 changes: 12 additions & 3 deletions dashboard/src/components/Header/NamespaceSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ interface INamespaceSelectorProps {
onChange: (ns: string) => any;
fetchNamespaces: () => void;
createNamespace: (ns: string) => Promise<boolean>;
getNamespace: (ns: string) => void;
}

interface INamespaceSelectorState {
Expand All @@ -26,13 +27,20 @@ class NamespaceSelector extends React.Component<INamespaceSelectorProps, INamesp
newNamespace: "",
};

get selected() {
return this.props.namespace.current || this.props.defaultNamespace;
}

public componentDidMount() {
this.props.fetchNamespaces();
if (this.selected !== definedNamespaces.all) {
this.props.getNamespace(this.selected);
}
}

public render() {
const {
namespace: { current, namespaces, error },
namespace: { namespaces, error },
defaultNamespace,
} = this.props;
const options =
Expand All @@ -43,9 +51,10 @@ class NamespaceSelector extends React.Component<INamespaceSelectorProps, INamesp
options.unshift(allOption);
const newOption = { value: "_new", label: "Create New" };
options.push(newOption);
const selected = current || defaultNamespace;
const value =
selected === definedNamespaces.all ? allOption : { value: selected, label: selected };
this.selected === definedNamespaces.all
? allOption
: { value: this.selected, label: this.selected };
return (
<div className="NamespaceSelector margin-r-normal">
<label className="NamespaceSelector__label type-tiny">NAMESPACE</label>
Expand Down
4 changes: 2 additions & 2 deletions dashboard/src/components/Layout/Layout.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from "react";

import ErrorBoundaryContainer from "containers/ErrorBoundaryContainer";
import Footer from "../../containers/FooterContainer";
import ErrorBoundary from "../ErrorBoundary";

import "./Layout.css";

Expand All @@ -17,7 +17,7 @@ class Layout extends React.Component<ILayoutProps> {
<HeaderComponent />
<main>
<div className="container">
<ErrorBoundary>{this.props.children}</ErrorBoundary>
<ErrorBoundaryContainer>{this.props.children}</ErrorBoundaryContainer>
</div>
</main>
<Footer />
Expand Down
Loading

0 comments on commit 4927c70

Please sign in to comment.