From 3cb3ed6990a2e59872a78fe580c6808afd08f67d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 4 Sep 2018 13:35:35 +0100 Subject: [PATCH 1/2] feat(ui): validate silence form data before submiting --- ui/src/Components/SilenceModal/SilenceForm.js | 3 +- .../SilenceModal/SilenceForm.test.js | 19 ++++- ui/src/Stores/SilenceFormStore.js | 18 +++++ ui/src/Stores/SilenceFormStore.test.js | 79 ++++++++++++++++++- 4 files changed, 115 insertions(+), 4 deletions(-) diff --git a/ui/src/Components/SilenceModal/SilenceForm.js b/ui/src/Components/SilenceModal/SilenceForm.js index fa231932d..7215d62e4 100644 --- a/ui/src/Components/SilenceModal/SilenceForm.js +++ b/ui/src/Components/SilenceModal/SilenceForm.js @@ -102,7 +102,8 @@ const SilenceForm = observer( event.preventDefault(); - silenceFormStore.data.inProgress = true; + if (silenceFormStore.data.isValid) + silenceFormStore.data.inProgress = true; }); render() { diff --git a/ui/src/Components/SilenceModal/SilenceForm.test.js b/ui/src/Components/SilenceModal/SilenceForm.test.js index f0d189c80..538c30756 100644 --- a/ui/src/Components/SilenceModal/SilenceForm.test.js +++ b/ui/src/Components/SilenceModal/SilenceForm.test.js @@ -3,7 +3,7 @@ import React from "react"; import { mount, shallow } from "enzyme"; import { AlertStore } from "Stores/AlertStore"; -import { SilenceFormStore } from "Stores/SilenceFormStore"; +import { SilenceFormStore, NewEmptyMatcher } from "Stores/SilenceFormStore"; import { SilenceForm } from "./SilenceForm"; let alertStore; @@ -121,7 +121,22 @@ describe(" inputs", () => { }); describe("", () => { - it("calling submit marks form as in progress", () => { + it("calling submit doesn't mark form as in progress when form is invalid", () => { + const tree = ShallowSilenceForm(); + tree.simulate("submit", { preventDefault: jest.fn() }); + expect(silenceFormStore.data.inProgress).toBe(false); + }); + + it("calling submit marks form as in progress when form is valid", () => { + const matcher = NewEmptyMatcher(); + matcher.name = "job"; + matcher.values = ["node_exporter"]; + silenceFormStore.data.matchers = [matcher]; + silenceFormStore.data.alertmanagers = [ + { label: "am1", value: "http://example.com" } + ]; + silenceFormStore.data.author = "me@example.com"; + silenceFormStore.data.comment = "fake silence"; const tree = ShallowSilenceForm(); tree.simulate("submit", { preventDefault: jest.fn() }); expect(silenceFormStore.data.inProgress).toBe(true); diff --git a/ui/src/Stores/SilenceFormStore.js b/ui/src/Stores/SilenceFormStore.js index 8ef4d3a31..0d42ac3b6 100644 --- a/ui/src/Stores/SilenceFormStore.js +++ b/ui/src/Stores/SilenceFormStore.js @@ -51,6 +51,23 @@ class SilenceFormStore { comment: "", author: "", + get isValid() { + if (this.alertmanagers.length === 0) return false; + if (this.matchers.length === 0) return false; + if ( + this.matchers.filter( + m => + m.name === "" || + m.values.length === 0 || + m.values.filter(v => v === "").length > 0 + ).length > 0 + ) + return false; + if (this.comment === "") return false; + if (this.author === "") return false; + return true; + }, + resetProgress() { this.inProgress = false; }, @@ -181,6 +198,7 @@ class SilenceFormStore { decStart: action.bound, incEnd: action.bound, decEnd: action.bound, + isValid: computed, toAlertmanagerPayload: computed, toDuration: computed }, diff --git a/ui/src/Stores/SilenceFormStore.test.js b/ui/src/Stores/SilenceFormStore.test.js index 8568e3389..ea690a1d9 100644 --- a/ui/src/Stores/SilenceFormStore.test.js +++ b/ui/src/Stores/SilenceFormStore.test.js @@ -1,7 +1,7 @@ import moment from "moment"; import { MockAlert, MockAlertGroup } from "__mocks__/Alerts.js"; -import { SilenceFormStore } from "./SilenceFormStore"; +import { SilenceFormStore, NewEmptyMatcher } from "./SilenceFormStore"; let store; beforeEach(() => { @@ -129,6 +129,83 @@ describe("SilenceFormStore.data", () => { }); }); +const MockAlertmanager = () => ({ + label: "default", + value: "http://localhost" +}); + +const MockMatcher = (name, values) => { + const matcher = NewEmptyMatcher(); + matcher.name = name; + matcher.values = values; + return matcher; +}; + +describe("SilenceFormStore.data.isValid", () => { + it("isValid returns 'false' if alertmanagers list is empty", () => { + store.data.matchers = [MockMatcher("foo", ["bar"])]; + store.data.author = "me@example.com"; + store.data.comment = "fake silence"; + expect(store.data.isValid).toBe(false); + }); + + it("isValid returns 'false' if matchers list is empty", () => { + store.data.alertmanagers = [MockAlertmanager]; + store.data.matchers = []; + store.data.author = "me@example.com"; + store.data.comment = "fake silence"; + expect(store.data.isValid).toBe(false); + }); + + it("isValid returns 'false' if matchers list is pupulated when a matcher without any name", () => { + store.data.alertmanagers = [MockAlertmanager]; + store.data.matchers = [MockMatcher("", ["bar"])]; + store.data.author = "me@example.com"; + store.data.comment = "fake silence"; + expect(store.data.isValid).toBe(false); + }); + + it("isValid returns 'false' if matchers list is pupulated when a matcher without any value ([])", () => { + store.data.alertmanagers = [MockAlertmanager]; + store.data.matchers = [MockMatcher("foo", [])]; + store.data.author = "me@example.com"; + store.data.comment = "fake silence"; + expect(store.data.isValid).toBe(false); + }); + + it("isValid returns 'false' if matchers list is pupulated when a matcher with empty value ([''])", () => { + store.data.alertmanagers = [MockAlertmanager]; + store.data.matchers = [MockMatcher("foo", [])]; + store.data.author = "me@example.com"; + store.data.comment = "fake silence"; + expect(store.data.isValid).toBe(false); + }); + + it("isValid returns 'false' if author is empty", () => { + store.data.alertmanagers = [MockAlertmanager]; + store.data.matchers = [MockMatcher("foo", ["bar"])]; + store.data.author = ""; + store.data.comment = "fake silence"; + expect(store.data.isValid).toBe(false); + }); + + it("isValid returns 'false' if comment is empty", () => { + store.data.alertmanagers = [MockAlertmanager]; + store.data.matchers = [MockMatcher("foo", ["bar"])]; + store.data.author = "me@example.com"; + store.data.comment = ""; + expect(store.data.isValid).toBe(false); + }); + + it("isValid returns 'true' if all fileds are set", () => { + store.data.alertmanagers = [MockAlertmanager]; + store.data.matchers = [MockMatcher("foo", ["bar"])]; + store.data.author = "me@example.com"; + store.data.comment = "fake silence"; + expect(store.data.isValid).toBe(true); + }); +}); + describe("SilenceFormStore.data startsAt & endsAt validation", () => { it("toDuration returns correct duration for 5d 0h 1m", () => { store.data.startsAt = moment([2000, 1, 1, 0, 0, 0]); From ff25e30121a588cae250d94e3f3cfaff06c3f308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Tue, 4 Sep 2018 14:20:51 +0100 Subject: [PATCH 2/2] feat(ui): render validation error as placeholders if form failed to validate --- .../Components/MultiSelect/ValidationError.js | 12 +++++++ .../MultiSelect/ValidationError.test.js | 14 ++++++++ .../ValidationError.test.js.snap | 22 ++++++++++++ .../SilenceModal/AlertManagerInput.js | 9 ++++- .../SilenceModal/AlertManagerInput.test.js | 15 ++++++++ .../Components/SilenceModal/LabelNameInput.js | 9 +++-- .../SilenceModal/LabelNameInput.test.js | 36 ++++++++++++++----- .../SilenceModal/LabelValueInput.js | 9 +++-- .../SilenceModal/LabelValueInput.test.js | 24 +++++++++---- ui/src/Components/SilenceModal/SilenceForm.js | 3 ++ .../Components/SilenceModal/SilenceMatch.js | 9 ++--- .../SilenceModal/SilenceMatch.test.js | 12 +++++-- ui/src/Stores/SilenceFormStore.js | 2 ++ ui/src/Stores/SilenceFormStore.test.js | 7 ++++ 14 files changed, 155 insertions(+), 28 deletions(-) create mode 100644 ui/src/Components/MultiSelect/ValidationError.js create mode 100644 ui/src/Components/MultiSelect/ValidationError.test.js create mode 100644 ui/src/Components/MultiSelect/__snapshots__/ValidationError.test.js.snap diff --git a/ui/src/Components/MultiSelect/ValidationError.js b/ui/src/Components/MultiSelect/ValidationError.js new file mode 100644 index 000000000..faf277526 --- /dev/null +++ b/ui/src/Components/MultiSelect/ValidationError.js @@ -0,0 +1,12 @@ +import React from "react"; + +import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; +import { faExclamationCircle } from "@fortawesome/free-solid-svg-icons/faExclamationCircle"; + +const ValidationError = () => ( + + Required + +); + +export { ValidationError }; diff --git a/ui/src/Components/MultiSelect/ValidationError.test.js b/ui/src/Components/MultiSelect/ValidationError.test.js new file mode 100644 index 000000000..cd31b122c --- /dev/null +++ b/ui/src/Components/MultiSelect/ValidationError.test.js @@ -0,0 +1,14 @@ +import React from "react"; + +import { shallow } from "enzyme"; + +import toDiffableHtml from "diffable-html"; + +import { ValidationError } from "./ValidationError"; + +describe("", () => { + it("matches snapshot", () => { + const tree = shallow(); + expect(toDiffableHtml(tree.html())).toMatchSnapshot(); + }); +}); diff --git a/ui/src/Components/MultiSelect/__snapshots__/ValidationError.test.js.snap b/ui/src/Components/MultiSelect/__snapshots__/ValidationError.test.js.snap new file mode 100644 index 000000000..bddec3114 --- /dev/null +++ b/ui/src/Components/MultiSelect/__snapshots__/ValidationError.test.js.snap @@ -0,0 +1,22 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` matches snapshot 1`] = ` +" + + + + + + Required + +" +`; diff --git a/ui/src/Components/SilenceModal/AlertManagerInput.js b/ui/src/Components/SilenceModal/AlertManagerInput.js index 816218569..134d5f743 100644 --- a/ui/src/Components/SilenceModal/AlertManagerInput.js +++ b/ui/src/Components/SilenceModal/AlertManagerInput.js @@ -7,6 +7,7 @@ import { observer } from "mobx-react"; import ReactSelect from "react-select"; import { MultiSelect, ReactSelectStyles } from "Components/MultiSelect"; +import { ValidationError } from "Components/MultiSelect/ValidationError"; const AlertmanagerInstancesToOptions = instances => instances.map(i => ({ @@ -75,7 +76,13 @@ const AlertManagerInput = observer( options={AlertmanagerInstancesToOptions( alertStore.data.upstreams.instances )} - placeholder="Alertmanager" + placeholder={ + silenceFormStore.data.wasValidated ? ( + + ) : ( + "Alertmanager" + ) + } isMulti onChange={this.onChange} /> diff --git a/ui/src/Components/SilenceModal/AlertManagerInput.test.js b/ui/src/Components/SilenceModal/AlertManagerInput.test.js index 75c42bb1b..ca863bbf9 100644 --- a/ui/src/Components/SilenceModal/AlertManagerInput.test.js +++ b/ui/src/Components/SilenceModal/AlertManagerInput.test.js @@ -60,6 +60,21 @@ describe("", () => { expect(tree).toMatchSnapshot(); }); + it("doesn't render ValidationError after passed validation", () => { + const tree = ShallowAlertManagerInput(); + silenceFormStore.data.wasValidated = true; + expect(tree.html()).not.toMatch(/fa-exclamation-circle/); + expect(tree.html()).not.toMatch(/Required/); + }); + + it("renders ValidationError after failed validation", () => { + const tree = ShallowAlertManagerInput(); + silenceFormStore.data.alertmanagers = []; + silenceFormStore.data.wasValidated = true; + expect(tree.html()).toMatch(/fa-exclamation-circle/); + expect(tree.html()).toMatch(/Required/); + }); + it("all available Alertmanager instances are selected by default", () => { ShallowAlertManagerInput(); expect(silenceFormStore.data.alertmanagers).toHaveLength(3); diff --git a/ui/src/Components/SilenceModal/LabelNameInput.js b/ui/src/Components/SilenceModal/LabelNameInput.js index 643825d1a..cb55c1a30 100644 --- a/ui/src/Components/SilenceModal/LabelNameInput.js +++ b/ui/src/Components/SilenceModal/LabelNameInput.js @@ -1,15 +1,18 @@ +import React from "react"; import PropTypes from "prop-types"; import { action } from "mobx"; import { observer } from "mobx-react"; import { MultiSelect } from "Components/MultiSelect"; +import { ValidationError } from "Components/MultiSelect/ValidationError"; import { FormatUnseeBackendURI } from "Stores/AlertStore"; const LabelNameInput = observer( class LabelNameInput extends MultiSelect { static propTypes = { - matcher: PropTypes.object.isRequired + matcher: PropTypes.object.isRequired, + isValid: PropTypes.bool.isRequired }; populateNameSuggestions = action(() => { @@ -70,7 +73,7 @@ const LabelNameInput = observer( } renderProps = () => { - const { matcher } = this.props; + const { matcher, isValid } = this.props; const value = matcher.name ? { label: matcher.name, value: matcher.name } @@ -80,7 +83,7 @@ const LabelNameInput = observer( instanceId: `silence-input-label-name-${matcher.id}`, defaultValue: value, options: matcher.suggestions.names, - placeholder: "Label name", + placeholder: isValid ? "Label name" : , onChange: this.onChange }; }; diff --git a/ui/src/Components/SilenceModal/LabelNameInput.test.js b/ui/src/Components/SilenceModal/LabelNameInput.test.js index 6af967fd4..221323357 100644 --- a/ui/src/Components/SilenceModal/LabelNameInput.test.js +++ b/ui/src/Components/SilenceModal/LabelNameInput.test.js @@ -20,16 +20,16 @@ beforeEach(() => { ]; }); -const ShallowLabelNameInput = () => { - return shallow(); +const ShallowLabelNameInput = isValid => { + return shallow(); }; -const MountedLabelNameInput = () => { - return mount(); +const MountedLabelNameInput = isValid => { + return mount(); }; const ValidateSuggestions = () => { - const tree = MountedLabelNameInput(); + const tree = MountedLabelNameInput(true); // click on the react-select component doesn't seem to trigger options // rendering in tests, so change the input instead tree.find("input").simulate("change", { target: { value: "f" } }); @@ -38,10 +38,28 @@ const ValidateSuggestions = () => { describe("", () => { it("matches snapshot", () => { - const tree = ShallowLabelNameInput(); + const tree = ShallowLabelNameInput(true); expect(tree).toMatchSnapshot(); }); + it("doesn't renders ValidationError after passed validation", () => { + // clear the name so placeholder is rendered + matcher.name = ""; + const tree = ShallowLabelNameInput(true); + expect(tree.html()).toMatch(/Label name/); + expect(tree.html()).not.toMatch(/fa-exclamation-circle/); + expect(tree.html()).not.toMatch(/Required/); + }); + + it("renders ValidationError after failed validation", () => { + // clear the name so placeholder is rendered + matcher.name = ""; + const tree = ShallowLabelNameInput(false); + expect(tree.html()).not.toMatch(/Label name/); + expect(tree.html()).toMatch(/fa-exclamation-circle/); + expect(tree.html()).toMatch(/Required/); + }); + it("renders suggestions", () => { const tree = ValidateSuggestions(); const options = tree.find("[role='option']"); @@ -61,7 +79,7 @@ describe("", () => { fetch .once(JSON.stringify(["name1", "name2", "name3"])) .once(JSON.stringify(["value1", "value2", "value3"])); - ShallowLabelNameInput(); + ShallowLabelNameInput(true); // use timeout since mount will call fetch setTimeout(() => { expect(matcher.suggestions.names).toHaveLength(3); @@ -79,7 +97,7 @@ describe("", () => { it("suggestions are emptied on failed fetch", done => { fetch.mockReject(new Error("fake error message")); - ShallowLabelNameInput(); + ShallowLabelNameInput(true); // use timeout since mount will call fetch setTimeout(() => { expect(matcher.suggestions.names).toHaveLength(0); @@ -88,7 +106,7 @@ describe("", () => { }); it("doesn't fetch suggestions if value is changed to empty string", () => { - const tree = MountedLabelNameInput(); + const tree = MountedLabelNameInput(true); const instance = tree.instance(); const fetchSpy = jest.spyOn(instance, "populateValueSuggestions"); instance.onChange(""); diff --git a/ui/src/Components/SilenceModal/LabelValueInput.js b/ui/src/Components/SilenceModal/LabelValueInput.js index f3eb989a2..4eb2e762c 100644 --- a/ui/src/Components/SilenceModal/LabelValueInput.js +++ b/ui/src/Components/SilenceModal/LabelValueInput.js @@ -1,14 +1,17 @@ +import React from "react"; import PropTypes from "prop-types"; import { action } from "mobx"; import { observer } from "mobx-react"; import { MultiSelect } from "Components/MultiSelect"; +import { ValidationError } from "Components/MultiSelect/ValidationError"; const LabelValueInput = observer( class LabelValueInput extends MultiSelect { static propTypes = { - matcher: PropTypes.object.isRequired + matcher: PropTypes.object.isRequired, + isValid: PropTypes.bool.isRequired }; onChange = action((newValue, actionMeta) => { @@ -25,13 +28,13 @@ const LabelValueInput = observer( }); renderProps = () => { - const { matcher } = this.props; + const { matcher, isValid } = this.props; return { instanceId: `silence-input-label-value-${matcher.id}`, defaultValue: matcher.values, options: matcher.suggestions.values, - placeholder: "Label value", + placeholder: isValid ? "Label value" : , isMulti: true, onChange: this.onChange }; diff --git a/ui/src/Components/SilenceModal/LabelValueInput.test.js b/ui/src/Components/SilenceModal/LabelValueInput.test.js index 2fc487d2d..c4699c357 100644 --- a/ui/src/Components/SilenceModal/LabelValueInput.test.js +++ b/ui/src/Components/SilenceModal/LabelValueInput.test.js @@ -20,16 +20,16 @@ beforeEach(() => { ]; }); -const ShallowLabelValueInput = () => { - return shallow(); +const ShallowLabelValueInput = isValid => { + return shallow(); }; -const MountedLabelValueInput = () => { - return mount(); +const MountedLabelValueInput = isValid => { + return mount(); }; const ValidateSuggestions = () => { - const tree = MountedLabelValueInput(); + const tree = MountedLabelValueInput(true); // click on the react-select component doesn't seem to trigger options // rendering in tests, so change the input instead tree.find("input").simulate("change", { target: { value: "f" } }); @@ -38,10 +38,22 @@ const ValidateSuggestions = () => { describe("", () => { it("matches snapshot", () => { - const tree = ShallowLabelValueInput(); + const tree = ShallowLabelValueInput(true); expect(tree).toMatchSnapshot(); }); + it("doesn't renders ValidationError after passed validation", () => { + const tree = ShallowLabelValueInput(true); + expect(tree.html()).not.toMatch(/fa-exclamation-circle/); + expect(tree.html()).not.toMatch(/Required/); + }); + + it("renders ValidationError after failed validation", () => { + const tree = ShallowLabelValueInput(false); + expect(tree.html()).toMatch(/fa-exclamation-circle/); + expect(tree.html()).toMatch(/Required/); + }); + it("renders suggestions", () => { const tree = ValidateSuggestions(); const options = tree.find("[role='option']"); diff --git a/ui/src/Components/SilenceModal/SilenceForm.js b/ui/src/Components/SilenceModal/SilenceForm.js index 7215d62e4..1526a9523 100644 --- a/ui/src/Components/SilenceModal/SilenceForm.js +++ b/ui/src/Components/SilenceModal/SilenceForm.js @@ -104,6 +104,8 @@ const SilenceForm = observer( if (silenceFormStore.data.isValid) silenceFormStore.data.inProgress = true; + + silenceFormStore.data.wasValidated = true; }); render() { @@ -125,6 +127,7 @@ const SilenceForm = observer( silenceFormStore.data.deleteMatcher(matcher.id); }} showDelete={silenceFormStore.data.matchers.length > 1} + isValid={!silenceFormStore.data.wasValidated} /> ))}