1080408 - Adjust wording for page shown when using &revision=SHA and no result sets found

Status: VERIFIED FIXED

(Tree Management :: Treeherder, defect, P2)

Description

[Ed Morley [:emorley]]

Bug 1048043 added an error message, if the specified revision is unknown to treeherder:

This can happen in two cases:
1) The revision is invalid
2) The revision has not yet been ingested from the pushlog.

#2 is extremely common in the pushing to try workflow, since the hg.mozilla.org response (and also the email sent to the pusher) both refer to the new push with a treeherder URL, that is often used within seconds of pushing to the repository.

The current failure text is:

"Unknown revision ID.
This could be because your push has not been processed yet, or the revision ID could be invalid. This page will refresh occasionally, so your push should show up within a few minutes if it does exist."

Today this caused some confusion in IRC, since the "This page will refresh occasionally" part hadn't been seen.

I think we should:
a) Tweak the failure text to reduce the chance of confusion in the common case (#2).
b) Perhaps add a note explaining why this occurs with treeherder and not TBPL (at least until bug 1065567 is fixed).
c) Add a spinner of some sorts, so that it's obvious the page is refreshing.

Possible new wording:

"Waiting for a result set matching revision [hg.m.o/foo/rev/<SHA> <SHA>]... <spinner>

Your push has either not been processed yet or the revision is invalid. This page will refresh automatically, so your result set will show up within a few minutes of pushing if it does exist."

Comment 1

[Jonathan French (:jfrench)]

Ed feel free to assign this to me if you haven't gotten to it yet, I had a go and think I have it fixed.

Comment 2

[Ed Morley [:emorley]]

Cool, thank you

Comment 3

[Jonathan French (:jfrench)]

Attachment: proposedFixAppearance

Here's a quick screen grab fwiw. The SHA/etc revision isn't being injected (I'm just turning the message on in the markup to get a screen grab), but it would be if the unknown condition was met.

I opened a new line after the revision for clarity, and a couple minor word tweaks.

Comment 4

[Jonathan French (:jfrench)]

Attachment: treeherder-ui-PR#234

Please see the above PR for review and status.

Comment 5

[Ed Morley [:emorley]]

Looks good - just one tweak - could the revision link to hg.m.o like the SHAs do in the result set commit list please? :-)

Comment 6

[Jonathan French (:jfrench)]

Sure, will do.

Comment 7

[Jonathan French (:jfrench)]

(In reply to Ed Morley (Away 17th Oct) [:edmorley] from comment #5)
> Looks good - just one tweak - could the revision link to hg.m.o like the
> SHAs do in the result set commit list please? :-)

I spent some time on this but I must be missing something. Based on the code it seems the message will only appear if result_sets.length == 0. Since there is no resultset in that condition, I don't believe there is a head revision SHA accessible, to inject.

I have a wip branch I can share with :camd next week. Maybe there is something accessible off result_sets[.] other than its length above, that I am missing here.

Comment 8

[Ed Morley [:emorley]]

(In reply to Jonathan French (:jfrench) from comment #7)
> I spent some time on this but I must be missing something. Based on the code
> it seems the message will only appear if result_sets.length == 0. Since
> there is no resultset in that condition, I don't believe there is a head
> revision SHA accessible, to inject.

We need to use the revision param from the URL, not that from a loaded result set - since as you say, this is intended for when the result set is unknown to treeherder.

As for the URL, we can derive it once we have the rev, a la:
https://github.com/mozilla/treeherder-ui/blob/a583d6584f899a48b75defedffe225d78a31bb00/webapp/app/partials/main/revisionListModal.html#L2

Comment 9

[Ed Morley [:emorley]]

Actually even better might be to use the pushloghtml variant URL, so that we link to the entire push and not just the topmost commit:
https://github.com/mozilla/treeherder-ui/blob/6c5d237c14bf6c529108e507abc4756ffa71921f/webapp/app/index.html#L138

This might help more with the concerns in bug 1084493 - since the user doesn't have to wait for the pushlog to be ingested before they can click though to hgweb and view their commits there.

Comment 10

[Jonathan French (:jfrench)]

Is there any scenario where this condition could occur in the main repo tab view also? Or is it just when explicitly viewing a single resultset with an 'invalid' revision in the URL.

I have a wip local branch which kinda-sorta does the former, if that is a valid case with the platform
http://pasteboard.co/18rrkWKg.jpg

So in the above scenario, you might have a sequence of resultsets in a repo view with job content, and some without - which would include that message depending on whether they have not been processed yet.

Or are all resultsets in an main repo view by definition, processed?

Comment 11

[Jonathan French (:jfrench)]

Ping'ing Ed on the above, as I'm still a bit unclear on it :) A STR for a local server would help me also I think.

Comment 12

[Ed Morley [:emorley]]

(In reply to Jonathan French (:jfrench) from comment #10)
> Is there any scenario where this condition could occur in the main repo tab
> view also? Or is it just when explicitly viewing a single resultset with an
> 'invalid' revision in the URL.

When using the normal view with no params, the UI just displays the result sets that were returns by the call to eg:
https://treeherder.mozilla.org/api/project/mozilla-central/resultset/?count=10&format=json&full=true&with_jobs=false

As such, we can't end up in the position of trying to request result sets that do not exist.

Now I'm not sure how we should handle fromchange/tochange not existing, but that's more of an edge case.

The most common case (and more critically, one that people who don't understand the treeherder model will hit frequently) is: "I clicked on the link that was given for my try push (that uses &revision=SHA) and the revision wasn't visible in treeherder".

(In reply to Jonathan French (:jfrench) from comment #11)
> Ping'ing Ed on the above, as I'm still a bit unclear on it :) A STR for a
> local server would help me also I think.

Example:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=123456789abc

Now whilst that SHA is made up (and thus we know if will never exist), as far as the handling goes here, the messaging should imply it _hopefully will exist_, since it's rare that someone will manually type in a SHA (and typo it) compared to the common case of following a link from the try email/hg.m.o response.

Comment 13

[Jonathan French (:jfrench)]

Thanks Ed, appreciated.

Comment 14

[Jonathan French (:jfrench)]

Comment on attachment 8505723 [details] [review]
treeherder-ui-PR#234

I've pushed the additional functionality change to the PR for review. Camd and I have each tested it locally, and it seems to be working correctly.

I figure there are enough changes to warrant a re-review, so if that's the correct process I'm doing that :)

Comment 15

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/9bfc31e953d379590e5e6b24ffad114c92277c84
Bug 1080408 - Adjust wording for 'Unknown revision ID' page

https://github.com/mozilla/treeherder-ui/commit/c379aafc5d59d4310bc4725acc013075a71a39ca
Merge pull request #234 from tojonmz/unknown-revision

Bug 1080408 - Adjust wording for 'Unknown revision ID' page

Comment 16

[Jonathan French (:jfrench)]

Verified the primary part of the bug is fixed and working correctly on stage.

However, between the time this PR was submitted and when it was merged, treeherder.css changed on master and the page margin class these messages were relying on, was lost. I'll open a supplemental PR for that.

Comment 17

[Jonathan French (:jfrench)]

Attachment: treeherder-ui-PR#266-supplemental

Please see the above supplemental PR for review and status.

Comment 18

[Ed Morley [:emorley]]

Current version (on dev):

```
[bold]Unknown revision ID.[/bold]
Waiting for a result set matching revision _SHA_ <spinner>
Your push has either not been processed yet or the revision is invalid. This page will refresh automatically, and your jobs will show up within a few minutes of the push if it exists.
```

Sorry to add more work, but:
* I've noticed that the current wording still includes "Unknown revision ID.", which I think is a source of the current confusion (people read the heading and not any more).
* I wonder if we need to be more explicit about the pushlog link (ie re bug 1084493 comment 6, I think it would be easy to miss it, and as such not realise that workflow doesn't have to wait for the push to appear). 
* I think perhaps I was wrong to suggest using the wording "result set", since it's really an artificial concept treeherder invented to cope with multi-repo jobs (which we don't actually make use of at the moment) and people aren't as familiar with the term compared to "push".
* I think the current wording still overemphasizes the uncommon case (invalid SHA) and is too wordy, so people stop reading.

How about:

```
[bold]Waiting for a push with revision SHA (_pushlog_)[/bold] <spinner>
This page will refresh automatically with your push once it has been processed, if it exists.
```

(Where _pushlog_ has the same link target that _SHA_ has currently)

Comment 19

[Jonathan French (:jfrench)]

Sure, my only suggestions would be to not make the first sentence bold at all, for the same reason you describe that people only read it, and not the rest of the lines. Similarly people will likely miss the SHA link with it bold, buried in a line of bold text.

It might also be helpful to do a bit of a tweak to the last sentence for the object 'it', to perhaps make it read a bit more clearly.

```
Waiting for a push with revision SHA (_pushlog_) <spinner>
If the push exists, the page will refresh automatically once it has been processed.
```

The previous "within a few minutes" in the last attempt we did, I thought was helpful, as otherwise there is no cue to the user how long they are expected to wait. Given that, we could tweak it further:

```
Waiting for a push with revision SHA (_pushlog_) <spinner>
If the push exists, the page will refresh automatically in a few minutes once it has been processed.
```

"few" is still vague, but it's at least an idea. Let me know what you think, ultimately I'll implement whatever you guys want. :)

Comment 20

[Ed Morley [:emorley]]

(In reply to Jonathan French (:jfrench) from comment #19)
> Sure, my only suggestions would be to not make the first sentence bold at
> all, for the same reason you describe that people only read it, and not the
> rest of the lines. Similarly people will likely miss the SHA link with it
> bold, buried in a line of bold text.

Agree :-)

> ```
> Waiting for a push with revision SHA (_pushlog_) <spinner>
> If the push exists, the page will refresh automatically in a few minutes
> once it has been processed.
> ```

The only problem with this wording, is that technically the page will refresh every 30s/60s (I forget exactly, but it's less than a few mins), but the push itself may take up to a few mins to be returned when the page does exist.

Also I was wondering perhaps we should just let a progress bar/animation (like the blue one that appears when the page is first loading) set expectations, avoiding having to read the text? If we replace the spinner here with the blue bar, then that might be clear enough, without having to say "page refreshes automatically"?

With that change, how about:

```
Waiting for a push with revision SHA (_pushlog_) <spinner>
If the push exists, it will appear in a few minutes once it has been processed.
```

Or we could also s/appear/appear automatically/ if needs be.

Thoughts?

(Sorry wordsmithing! But seems to be a critical UX piece for people new to treeherder :-))

Comment 21

[Jonathan French (:jfrench)]

Sounds good, I'll make that change.

I also like the 'appear automatically' to remove ambiguity about what actions if any the user is supposed to do when presented with the dialogue. So I will include that.

Comment 22

[Jonathan French (:jfrench)]

Attachment: unknown_wording_rev2_1

I was wrong, that 'automatically' looked awkward. I think I am still unclear on what (_pushlog_) is supposed to be extrapolated to. If you can provide a real world example of the values I could try to match it.

I have done a couple of attempts lacking that bit of information (attached example 1 and 2). Personally I like the shorter one, especially when you see it live the the spinner.

Having the spinner elsewhere, is confusing when its running.

Comment 23

[Jonathan French (:jfrench)]

Attachment: unknown_rev2_2

And rev2_2.

Comment 24

[Jonathan French (:jfrench)]

Where in rev2_2 above, both links point to the _same pushlog url. So clicking on either would navigate you there.

Comment 25

[Ed Morley [:emorley]]

Comment on attachment 8521104 [details]
unknown_wording_rev2_1

One of the reasons I think people are not liking having to wait for the push to load (other than concerns that it won't refresh automatically), are that they sometimes want to double check the commits they pushed. As such, I thought using the word "pushlog" would make it extra clear that they can skip waiting and view the commits on hg.m.o instead.

To clarify comment 20 slightly:

Waiting for a push with revision abcdef123456 (<a href="https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b932c51b4ad9>pushlog</a>) <spinner>

Or if we wanted to mention the repo name too:

Waiting for a mozilla-inbound push with revision abcdef123456 (<a href="https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b932c51b4ad9>pushlog</a>) <spinner>

Comment 26

[William Lachance (:wlach)]

Comment on attachment 8520397 [details] [review]
treeherder-ui-PR#266-supplemental

Looks like jfrench is going to open up a new PR, cancelling for now.

Comment 27

[Jonathan French (:jfrench)]

Attachment: treeherder-ui-PR#267-supplemental

Please see the above supplemental PR for review and status.

Comment 28

[William Lachance (:wlach)]

Comment on attachment 8521485 [details] [review]
treeherder-ui-PR#267-supplemental

lgtm

Comment 29

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/c6d138baf86ac83156cc77741658591150d020a2
Bug 1080408 - Update wording for 'Unknown revision ID' page

https://github.com/mozilla/treeherder-ui/commit/a659fafbf669dff5af1ccf01cf68ea0d12888f2d
Merge pull request #267 from tojonmz/unknown-revision-supplemental

Bug 1080408 - Update wording for 'Unknown revision ID' page

Comment 30

[Jonathan French (:jfrench)]

Verified fixed and working correctly on dev.

Comment 31

[Jonathan French (:jfrench)]

Comment on attachment 8505723 [details] [review]
treeherder-ui-PR#234

PR234 was previously reviewed in github and merged my camd, so marking it as + for book keeping.

Comment 32

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/b23f94aa8e025dacd8896825686c4dd2226fd4b6
Bug 1080408 - Adjust wording for 'Unknown revision ID' page

https://github.com/mozilla/treeherder/commit/90e3c8ebcce6e84b2858161c726ee7d5dd5e70e5
Merge pull request #234 from tojonmz/unknown-revision

Bug 1080408 - Adjust wording for 'Unknown revision ID' page

https://github.com/mozilla/treeherder/commit/d1673d63c9c63e53667bdb9e15e1ef8ed1172a1f
Bug 1080408 - Update wording for 'Unknown revision ID' page

https://github.com/mozilla/treeherder/commit/0daae69ada4032884a486e18b0eaad6e52d729ba
Merge pull request #267 from tojonmz/unknown-revision-supplemental

Bug 1080408 - Update wording for 'Unknown revision ID' page