Improved Validation #827

w0rm · 2017-05-24T14:16:59Z

Extracted affected alerts out of the silence type
Unified ApiData
Added separate validation messages for each field to validate on blur
Changed Update/Preview messages so they create the silence from the form or trigger validation errors
If there was an error from the backend when saving a silence, it will be displayed

I thought of changing the form styles in this PR, but I think it's too much.

mxinden · 2017-05-24T15:28:19Z

ui/app/src/Main.elm

@@ -8,7 +8,7 @@ import Types
        ( Route(..)
        , Msg
            ( NavigateToSilenceList
-            , NavigateToSilence
+            , NavigateToSilenceView


Not a big thing: Why adding View to NavigateToSilence msg? We don't do it with the other ones.

Because I renamed Silence to SilenceView, which represents a page. I didn't want to confuse it with a Silence type.

mxinden · 2017-05-24T15:28:36Z

ui/app/src/Main.elm

@@ -54,7 +54,7 @@ init location =
                _ ->
                    nullFilter
    in
-        update (urlUpdate location) (Model initSilenceList Loading initSilenceForm initAlertList route filter initStatusModel)
+        update (urlUpdate location) (Model initSilenceList initSilenceView initSilenceForm initAlertList route filter initStatusModel)


mxinden · 2017-05-24T15:37:26Z

ui/app/src/Utils/Types.elm

    = Initial
    | Loading
-    | Failure e
+    | Failure String


Having a string instead of a type variable forces us to drop information, doesn't it? Can't we keep the type variable here and convert an error to a string in the view functions?

I don't see where we require this information. I need to create the error myself, when e.g. validation fails. It is easier to create it from String, rather than manually create Http.Error, I don't think this is even possible.

We can always change the Failure String to be a Failure of our own union type if we need to extract additional information.

mxinden · 2017-05-24T15:40:33Z

ui/app/src/Views/SilenceForm/Types.elm

+        ( initialField
+        , ValidationState(..)
+        , ValidatedField
+        , validate


I smell a library forming here

mxinden · 2017-05-24T15:48:23Z

ui/app/src/Views/SilenceForm/Types.elm

+            }
+        )
+        (stringNotEmpty comment.value)
+        (List.foldr appendMatcher (Ok []) matchers)


When I don't specify any matchers and create a silence, I get redirected to the /silences page without any error messages in the UI. The API call fails with silence invalid: at least one matcher required. Should we enforce having at least one matcher in this PR or in an upcoming one? Sorry, I missed this requirement in my last changes as well.

I forgot to remove the redirect. Now it should display the error from the backend. It is not pretty, but I think we should address this later.

This is definitely good enough for now. Thanks.

Oh, I've got an idea. We should just hide the delete button, when there is only one matcher left. Will do it in another pull request after this is merged.

mxinden · 2017-05-24T15:51:07Z

ui/app/src/Utils/FormValidation.elm

-        )
-        agg
+validate : (String -> Result String a) -> ValidatedField -> ValidatedField
+validate validate field =


Could we do validate validator field = instead? A little confusing to see the same term twice.

mxinden · 2017-05-24T15:52:08Z

ui/app/src/Utils/FormValidation.elm

-validatedMatcherToMatcher matcher =
-    Result.map2 (\n v -> Matcher n v matcher.isRegex) matcher.name matcher.value
-        |> Result.mapError second
+stringNotEmpty : String -> Result String String


In the long run we should trim the string as well so a space is not valid either.

Added String.trim

mxinden · 2017-05-24T15:53:19Z

ui/app/src/Utils/Types.elm

 import Time


-type ApiResponse e a


If I see it correctly we get rid of ApiResponse? 👍 for removing an extra layer of complexity.

Yeah, I had no idea of why both were there. It was pretty confusing

mxinden · 2017-05-25T04:43:09Z

Related to #798.

mxinden

This looks good to me. Thanks @w0rm

mxinden · 2017-05-25T04:53:06Z

ui/app/src/Views/SilenceView/Views.elm

@@ -27,8 +28,8 @@ view model =
            error msg


-silence2 : Silence -> Html Msg
-silence2 silence =
+silence2 : ApiData (List Alert) -> Silence -> Html SilenceViewMsg


Just a random question: Is there a particular reason we called this silence2?

Maybe because silence is taken. I guess it's a question to @stuartnelson3

I renamed it to viewSilence

stuartnelson3

Looks awesome!

One thing I noticed: When deleting all matchers and then "previewing" a silence, it returns all available alerts (since we aren't filtering by anything).

I think it would make sense to disable the preview button if len(matchers) == 0.

stuartnelson3 · 2017-05-27T21:57:56Z

ui/app/src/Types.elm

 import Utils.Filter exposing (Filter)


 type alias Model =
    { silenceList : SilenceList.Model
-    , silence : ApiData Silence
+    , silenceView : SilenceView.Model


mmm happy to see the final part of our top-level model refactored into its own model

stuartnelson3 · 2017-05-27T22:05:25Z

ui/app/src/Utils/Types.elm

@@ -30,10 +29,6 @@ type alias Label =
    ( String, String )


-type alias ApiData a =
-    ApiResponse Http.Error a


I think I originally brought this in from reading the RemoteData blog post. I think the other had a generic type, and then made a specific type alias for dealing with http requests.

Since that thing was a library, it had to be more generic. We don't need more than String for error at the moment. Changing this to smth else later won't be a problem.

stuartnelson3 · 2017-05-27T22:07:29Z

ui/app/src/Utils/Views.elm

-        Ok inputValue ->
+validatedField : (List (Attribute msg) -> List (Html msg) -> Html msg) -> String -> String -> (String -> msg) -> msg -> ValidatedField -> Html msg
+validatedField htmlField labelText classes inputMsg blurMsg field =
+    case field.validationState of


The three different cases in this function differ only by the css classes has-success, has-danger, or the absence of these two. Would it be possible to just write the HTML once and switch on the css class?

@stuartnelson3 another class is applied to the input field (form-control-success or form-control-danger), plus there is an error message in the error case. I think this is fine.

stuartnelson3 · 2017-05-27T22:08:34Z

ui/app/src/Utils/Views.elm

-            ]
+    div []
+        [ p [] [ text <| "Error: " ++ err ]
+        ]


+1 for moving this logic out of the view

stuartnelson3 · 2017-05-27T22:09:58Z

ui/app/src/Views/SilenceForm/Types.elm

+        ( initialField
+        , ValidationState(..)
+        , ValidatedField
+        , validate


I smell a library forming here

stuartnelson3 · 2017-05-27T22:11:24Z

ui/app/src/Views/SilenceForm/Types.elm

+validateForm : SilenceForm -> SilenceForm
+validateForm { id, createdBy, comment, startsAt, endsAt, duration, matchers } =
+    { id = id
+    , createdBy = validate stringNotEmpty createdBy


mmm love that validate function

stuartnelson3 · 2017-05-27T22:19:30Z

ui/app/src/Views/SilenceForm/Types.elm

-    , form : SilenceForm
+    { form : SilenceForm
+    , silenceId : ApiData String
+    , alerts : ApiData (List Alert)


I like having alerts exist independently on this and the silence view model, +1

stuartnelson3 · 2017-05-27T22:32:01Z

ui/app/src/Views/SilenceForm/Updates.elm

-            case silence of
-                Success id ->
-                    ( model, Navigation.newUrl ("/#/silences/" ++ id) )
+        SilenceCreate silenceId ->


We can definitely re-think these names I came up with. SilenceCreate and CreateSilence could definitely be better.

(note for the future)

w0rm · 2017-05-28T08:07:34Z

@stuartnelson3 I made it impossible to delete the last matcher, but accidentally committed True instead of using showDeleteButton. Thanks, fixed now!

stuartnelson3

Thanks!

This closes prometheus#827.

Andrey Kuzmin added 6 commits May 24, 2017 12:10

Restructure the validation

e78a52e

Validate on blur

f3a48ab

Extract alerts from the silence

aec9e42

Use string for the error type

427c157

Use ApiData everywhere

60d277c

Validate the form

b80ed7c

w0rm requested review from mxinden and stuartnelson3 May 24, 2017 14:25

mxinden requested changes May 24, 2017

View reviewed changes

Andrey Kuzmin added 2 commits May 24, 2017 18:08

Remove redirect on failure

9e12b6d

Code review fixes

5033ea0

mxinden mentioned this pull request May 25, 2017

Rework SilenceForm #798

Closed

mxinden approved these changes May 25, 2017

View reviewed changes

Andrey Kuzmin added 2 commits May 26, 2017 16:39

Hide the delete button if there is one matcher

d7e82c6

Rename silence2

10ce242

w0rm force-pushed the validation branch from 81c7903 to 10ce242 Compare May 27, 2017 13:40

Update bindata.go

a73cf94

w0rm force-pushed the validation branch from 83931c5 to a73cf94 Compare May 27, 2017 13:48

Fix the tests

0b14c90

stuartnelson3 suggested changes May 27, 2017

View reviewed changes

Actually hide the delete button

04042b3

Update bindata.go

3a6b4a5

w0rm force-pushed the validation branch from 68f158a to 3a6b4a5 Compare May 28, 2017 08:13

stuartnelson3 approved these changes May 28, 2017

View reviewed changes

stuartnelson3 merged commit 562f995 into master May 28, 2017

stuartnelson3 deleted the validation branch May 28, 2017 10:53

hh pushed a commit to ii/alertmanager that referenced this pull request Feb 28, 2018

Drop exec_ in boot_timestamp_seconds on *bsd

d5a9c0d

This closes prometheus#827.

hh pushed a commit to ii/alertmanager that referenced this pull request Mar 8, 2018

Drop exec_ in boot_timestamp_seconds on *bsd (prometheus#839)

424ca8e

This closes prometheus#827.

Improved Validation #827

Improved Validation #827

Conversation

w0rm commented May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w0rm May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented May 25, 2017

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartnelson3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w0rm commented May 28, 2017 • edited Loading

stuartnelson3 left a comment

Choose a reason for hiding this comment

w0rm commented May 24, 2017 •

edited

Loading

w0rm May 24, 2017 •

edited

Loading

w0rm commented May 28, 2017 •

edited

Loading