Skip to content
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

make values starts with https or http clickable in build metadata #3398

Merged
merged 1 commit into from Mar 18, 2019

Conversation

@Twiknight
Copy link
Contributor

Twiknight commented Feb 28, 2019

A partial implementation for #429

It makes all strings that looks like a url in area build -> resource -> metadata clickable.

This is very useful when your resource returns information from an external website, for example GitHub or GitLab like below case:

image

  • It only handles url-like strings (String starts with "http://" or "https://"), since in elm 0.19 we'll have official url lib, it would be duplication to implement https://tools.ietf.org/html/rfc3986 here

  • It only works in build page with the resource metadata. The reason is, to make all url-like stuff clickable would be larger change that impacts several files and ten more functions. I chose to start with build metadata, where in most cases, you would want a clickable link. When we decide this is a proper way or find a better way, more changes can be applied to the rest parts.

Copy link
Contributor

pivotal-jamie-klassen left a comment

As you might have noticed, we've increased our unit test count from 82 in October to 1093 now, and a feature like this is one that I could imagine regressing in subsequent dramatic refactors. How would you feel about adding a test? On your last PR I backfilled. While this one might be a trickier test to write, there are a bunch of examples to refer to in BuildTests. I'd really appreciate your feedback on the contribution experience, if you've got the bandwidth.

I could imagine something like this -- handleCallback (PlanAndResourcesFetched ...) where the build plan consisted of just a single get step, and then use update (BuildEventsMsg (Events (Ok (Array.fromList [ FinishGet origin exitStatus version metadata ])))) to simulate the get step completing (metadata should include some value that has a URL value), and then you call view on the resulting model and query for the build step you want, asserting that it contains an anchor with the given URL text, href to the right URL, the underline style and the 'open in new window' (i.e. target "_blank") behaviour.

I'd be happy to discuss further with you, either on this PR or elsewhere -- looks like you are on our Discord, too!

web/elm/src/Build/StepTree.elm Outdated Show resolved Hide resolved
@Twiknight

This comment has been minimized.

Copy link
Contributor Author

Twiknight commented Mar 4, 2019

@pivotal-jamie-klassen
Thanks for the comments and the back-fill patch on my last pr.
Actually I felt strange that it did not break any case when I was creating these patches.

I'll try to cover the changed lines with tests. But it might take some time for me to understand the test framework. (And your back-fill patch looks like a helpful example on my first sight : ) ).

I'll come back to you when I have some updates or I need some help.

@pivotal-jamie-klassen

This comment has been minimized.

Copy link
Contributor

pivotal-jamie-klassen commented Mar 4, 2019

@Twiknight glad to hear it! While you're looking at my follow-up PR maybe using elm-format will help you to more easily interpret the changes (as a lot of them were whitespace-related): #3425

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 4, 2019

folding into #3401

@vito vito closed this Mar 4, 2019
@pivotal-jamie-klassen

This comment has been minimized.

Copy link
Contributor

pivotal-jamie-klassen commented Mar 4, 2019

whoops, we told @vito the wrong PR.

@Twiknight

This comment has been minimized.

Copy link
Contributor Author

Twiknight commented Mar 5, 2019

@pivotal-jamie-klassen I've nearly done, but the test case looks horrible in code smell.
The problem is, I tried to pass several urls to the test function, but I could not find a utility in elm-test that applies to this case.

Currently it looks like below:

, test "get step metadata should show hyperlink on url-like content" <|
    \() -> Expect.all
        [ (\f -> f "http://metadata-url.com" ())
        , (\f -> f "https://metadata-url.com" ())
        ]
        (\url ->
            fetchPlanWithGetStep
                >> Build.update
                    (Build.Msgs.BuildEventsMsg <|
                        Build.Msgs.Events <|
                            Ok <|
                                Array.fromList
                                    [ Models.FinishGet
                                        { source = "stdout", id = "plan" }
                                        0
                                        (Dict.fromList [ ( "foo", "bar" ) ])
                                        [ { name = "url", value = url } ]
                                    ]
                    )
                >> Tuple.first
                >> Build.update (Build.Msgs.ToggleStep "plan")
                >> Tuple.first
                >> Build.view UserState.UserStateLoggedOut
                >> Query.fromHtml
                >> Query.find
                    [ containing [ text url ]
                    ]
                >> Query.has
                    [ tag "a"
                    , style [ ( "text-decoration-line", "underline" ) ]
                    , attribute <| Attr.target "_blank"
                    , attribute <| Attr.href url
                    ]
        )

Any ideas how can I make it readable?

@pivotal-jamie-klassen

This comment has been minimized.

Copy link
Contributor

pivotal-jamie-klassen commented Mar 5, 2019

@Twiknight in this case I would just duplicate the assertions, i.e. (test "get step metadata should show hyperlink on value starting with 'http://'" and test "get step metadata should show hyperlink on value starting with 'https://'") as both cases impact the implementation logic. Maybe a case for not showing a link when the value starts with something else, too. I'd make the case here for preferring duplication over the wrong abstraction.

@pivotal-jamie-klassen

This comment has been minimized.

Copy link
Contributor

pivotal-jamie-klassen commented Mar 6, 2019

@Twiknight awesome!

@evanchaoli

This comment has been minimized.

Copy link
Collaborator

evanchaoli commented Mar 6, 2019

@vito This code change has been approved, can you please help merge and include it in 5.0?

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 10, 2019

Merged a refactor and now there are merge conflicts here - sorry! Hopefully they're not too much of a pain to resolve.

@evanchaoli This didn't make it in to 5.0, so it'll be in the next release (5.1).

@evanchaoli

This comment has been minimized.

Copy link
Collaborator

evanchaoli commented Mar 11, 2019

@vito 5.1 is fine. Thanks a lot.

@Twiknight

This comment has been minimized.

Copy link
Contributor Author

Twiknight commented Mar 11, 2019

After the rebasing, seems the test cases broke, I'm fixing and will update later

Signed-off-by: Twiknight <Twiknight@outlook.com>
@Twiknight

This comment has been minimized.

Copy link
Contributor Author

Twiknight commented Mar 15, 2019

@pivotal-jamie-klassen @vito ,

I've finished rewritten the cases to match current paradigm of data flow.

Any idea why a UI change would fail a Go test?

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 15, 2019

@Twiknight Looks like a flake. I've retriggered it and PR'd a fix for the flake: #3516

@vito
vito approved these changes Mar 18, 2019
Copy link
Member

vito left a comment

works great - thanks!

@vito vito merged commit 766f200 into concourse:master Mar 18, 2019
5 checks passed
5 checks passed
DCO DCO
Details
WIP Ready for review
Details
concourse-ci/testflight Concourse CI build success
Details
concourse-ci/unit Concourse CI build success
Details
concourse-ci/watsjs Concourse CI build success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.