web: fix url parsing

work around elm/url#32. #4313 Signed-off-by: Jamie Klassen <cklassen@pivotal.io>
concourse · Oct 22, 2019 · 0143325 · 0143325
1 parent 530a979
commit 0143325
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 78 deletions.
diff --git a/web/elm/src/Routes.elm b/web/elm/src/Routes.elm
@@ -17,6 +17,7 @@ module Routes exposing
 
 import Concourse
 import Concourse.Pagination as Pagination exposing (Direction(..))
+import Maybe.Extra
 import Url
 import Url.Builder as Builder
 import Url.Parser
@@ -194,7 +195,13 @@ pipeline =
 dashboard : Parser (Route -> a) a
 dashboard =
     oneOf
-        [ map (Normal >> Dashboard) (top <?> Query.string "search")
+        [ map
+            (Maybe.map (String.replace "+" " ")
+                -- https://github.com/elm/url/issues/32
+                >> Normal
+                >> Dashboard
+            )
+            (top <?> Query.string "search")
         , map (Dashboard HighDensity) (s "hd")
         ]
 
@@ -332,108 +339,106 @@ sitemap =
         ]
 
 
-pageToQueryString : Maybe Pagination.Page -> String
-pageToQueryString page =
+pageToQueryParams : Maybe Pagination.Page -> List Builder.QueryParameter
+pageToQueryParams page =
     case page of
         Nothing ->
-            ""
+            []
 
         Just { direction, limit } ->
-            "?"
-                ++ (case direction of
-                        Since id ->
-                            "since=" ++ String.fromInt id
+            [ case direction of
+                Since id ->
+                    Builder.int "since" id
 
-                        Until id ->
-                            "until=" ++ String.fromInt id
+                Until id ->
+                    Builder.int "until" id
 
-                        From id ->
-                            "from=" ++ String.fromInt id
+                From id ->
+                    Builder.int "from" id
 
-                        To id ->
-                            "to=" ++ String.fromInt id
-                   )
-                ++ "&limit="
-                ++ String.fromInt limit
+                To id ->
+                    Builder.int "to" id
+            , Builder.int "limit" limit
+            ]
 
 
 toString : Route -> String
 toString route =
     case route of
         Build { id, highlight } ->
-            "/teams/"
-                ++ id.teamName
-                ++ "/pipelines/"
-                ++ id.pipelineName
-                ++ "/jobs/"
-                ++ id.jobName
-                ++ "/builds/"
-                ++ id.buildName
+            Builder.absolute
+                [ "teams"
+                , id.teamName
+                , "pipelines"
+                , id.pipelineName
+                , "jobs"
+                , id.jobName
+                , "builds"
+                , id.buildName
+                ]
+                []
                 ++ showHighlight highlight
 
         Job { id, page } ->
-            "/teams/"
-                ++ id.teamName
-                ++ "/pipelines/"
-                ++ id.pipelineName
-                ++ "/jobs/"
-                ++ id.jobName
-                ++ pageToQueryString page
+            Builder.absolute
+                [ "teams"
+                , id.teamName
+                , "pipelines"
+                , id.pipelineName
+                , "jobs"
+                , id.jobName
+                ]
+                (pageToQueryParams page)
 
         Resource { id, page } ->
-            "/teams/"
-                ++ id.teamName
-                ++ "/pipelines/"
-                ++ id.pipelineName
-                ++ "/resources/"
-                ++ id.resourceName
-                ++ pageToQueryString page
+            Builder.absolute
+                [ "teams"
+                , id.teamName
+                , "pipelines"
+                , id.pipelineName
+                , "resources"
+                , id.resourceName
+                ]
+                (pageToQueryParams page)
 
         OneOffBuild { id, highlight } ->
-            "/builds/"
-                ++ String.fromInt id
+            Builder.absolute
+                [ "builds"
+                , String.fromInt id
+                ]
+                []
                 ++ showHighlight highlight
 
         Pipeline { id, groups } ->
-            "/teams/"
-                ++ id.teamName
-                ++ "/pipelines/"
-                ++ id.pipelineName
-                ++ (case groups of
-                        [] ->
-                            ""
-
-                        gs ->
-                            "?group=" ++ String.join "&group=" gs
-                   )
+            Builder.absolute
+                [ "teams"
+                , id.teamName
+                , "pipelines"
+                , id.pipelineName
+                ]
+                (groups |> List.map (Builder.string "group"))
 
         Dashboard (Normal (Just search)) ->
-            "/?search=" ++ search
+            Builder.absolute [] [ Builder.string "search" search ]
 
         Dashboard (Normal Nothing) ->
-            "/"
+            Builder.absolute [] []
 
         Dashboard HighDensity ->
-            "/hd"
-
-        FlySuccess noop (Just flyPort) ->
-            "/fly_success?fly_port="
-                ++ String.fromInt flyPort
-                ++ (if noop then
-                        "&noop=true"
-
-                    else
-                        ""
-                   )
-
-        FlySuccess noop Nothing ->
-            "/fly_success"
-                ++ (if noop then
-                        "?noop=true"
-
-                    else
-                        ""
-                   )
+            Builder.absolute [ "hd" ] []
+
+        FlySuccess noop flyPort ->
+            Builder.absolute [ "fly_success" ] <|
+                (flyPort
+                    |> Maybe.map (Builder.int "fly_port")
+                    |> Maybe.Extra.toList
+                )
+                    ++ (if noop then
+                            [ Builder.string "noop" "true" ]
+
+                        else
+                            []
+                       )
 
 
 parsePath : Url.Url -> Maybe Route

diff --git a/web/elm/tests/RoutesTests.elm b/web/elm/tests/RoutesTests.elm
@@ -9,18 +9,18 @@ import Url
 all : Test
 all =
     describe "Routes"
-        [ test "parses dashboard search query" <|
+        [ test "parses dashboard search query respecting space" <|
             \_ ->
                 Routes.parsePath
                     { protocol = Url.Http
                     , host = ""
                     , port_ = Nothing
                     , path = "/"
-                    , query = Just "search=asdf"
+                    , query = Just "search=asdf+sd"
                     , fragment = Nothing
                     }
                     |> Expect.equal
-                        (Just (Routes.Dashboard (Routes.Normal (Just "asdf"))))
+                        (Just (Routes.Dashboard (Routes.Normal (Just "asdf sd"))))
         , test "fly success has noop parameter" <|
             \_ ->
                 Routes.parsePath

diff --git a/web/elm/tests/TopBarTests.elm b/web/elm/tests/TopBarTests.elm
@@ -1231,7 +1231,7 @@ all =
                                             Tuple.second
                                                 >> Expect.equal
                                                     [ Effects.ModifyUrl
-                                                        "/?search=status: "
+                                                        "/?search=status%3A%20"
                                                     ]
                                         ]
                                    ]