1160616 - when loading a direct url in perfherder, the error handling could look prettier

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect, P4)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

right now we indicate what is not working with the url (bad project, bad revision, etc.).  For example, if we are on the top level comparison mode, we should switch back to the chooser.  Possibly we could build a chooser for the test/platform (subtest view), that is optionally invoked.

Upon error we should make the error more web style (not raw text) and get back to the chooser screen for self remediation.

Comment 1

[William Lachance (:wlach)]

See also bug 1169340, which is about making the compare chooser friendlier.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

now that the comparechooser is friendlier, we can determine if we have bad values input in the URL (at least for revision, branch) and send the end user to the comparechooser screen.  Ideally we could have the proper error messages/handling taken care of there.

this brings up a question- we should be careful as we could get recursive here if we redirect to comparechooser for validation, then upon validation, load a URL.  Ideally a shared bit of code and upon failure, we redirect the user to comparechooser.

Likewise we can try to make most of our links to compare view actually link to compare chooser :)

Comment 3

[MikeLing]

(In reply to Joel Maher (:jmaher) from comment #2)

Hi Joel, I would like to work on this bug ;)

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

oh, this would be great, do let me know what questions you have.

Comment 5

[MikeLing]

Attachment: PR for bug 1160616

which way are you prefer? redirect to the comperchooser after 5 second waiting or redirect to it immediately with top notification.

Comment 6

[William Lachance (:wlach)]

Comment on attachment 8638595 [details] [review]
PR for bug 1160616

I would say neither actually. :) This is actually much less likely to happen now that we pre-validate the revision information before we submit it (bug 1169340). I would say in fact that there's nothing we should do here: the only way I could see a user getting here is if they followed a bad link, in which case going back to the compare chooser isn't a normal action they'd want to take. They might in fact want to link to this page and complain to us that we sent them here (since it was likely us that produced it). :)

With that in mind, the only change I might make is putting the text inside a danger class: <p class="bg-danger">...</p> 

http://getbootstrap.com/css/#helper-classes-backgrounds

That doesn't seem like part of this bug, but feel free to file a new bug/PR for that. I'm going to close this one as INVALID, sorry for the confusion. :(

Comment 7

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #6)
> Comment on attachment 8638595 [details] [review]
> With that in mind, the only change I might make is putting the text inside a
> danger class: <p class="bg-danger">...</p> 
> 
> http://getbootstrap.com/css/#helper-classes-backgrounds
> 
> That doesn't seem like part of this bug, but feel free to file a new bug/PR
> for that. I'm going to close this one as INVALID, sorry for the confusion. :(

Actually, on second thought, let's just do that as part of this bug. If you want to provide a simple patch which does just that, please go ahead. :)

Comment 8

[Matthew N. [:MattN]]

I regularly hit a related problem since it's normal to want to remove the query parameters to get back to the form but for some reason we use 2 different URLs for the form and results and so I end up with a blank page at: https://treeherder.mozilla.org/perf.html#/compare

Comment 9

[William Lachance (:wlach)]

(In reply to Matthew N. [:MattN] from comment #8)
> I regularly hit a related problem since it's normal to want to remove the
> query parameters to get back to the form but for some reason we use 2
> different URLs for the form and results and so I end up with a blank page
> at: https://treeherder.mozilla.org/perf.html#/compare

Agreed, in the case no parameters are specified, it would make sense to redirect to the chooser.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

I have to disagree here, it is very often that we hand edit urls and as it stands now I mess this up about once every 8-10 times I use compare.  Until we have a whole series of great tools, I would like to make sure we have something that helps out.

Comment 11

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #10)
> I have to disagree here, it is very often that we hand edit urls and as it
> stands now I mess this up about once every 8-10 times I use compare.  Until
> we have a whole series of great tools, I would like to make sure we have
> something that helps out.

Ok, agreed, it sounds like we should at least make it easy to link to the compare chooser.

I still don't like auto-redirection because it makes debugging problems more difficult: as a developer, if someone has a bug report, I'd rather they be able to link to a persisted URL so I can debug what's going on. Also, as a user, you might miss the error. IMO, there's a reason why almost no websites do this.

I think adding a link in the error banner that says something like:

<a href="#/comparechooser?originalProject=<chosenProject>&originalRevision=<chosen revision>...">Modify choice</a> should make things easier without the negative side effects.

My proposal in summary:

1. Put the error message in a danger class so it's easier to see (see comment 6).
2. Add a link to the compare chooser (with the input prefilled for ease of modification).

Sound good?

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

sounds good!

Comment 13

[William Lachance (:wlach)]

Comment on attachment 8638595 [details] [review]
PR for bug 1160616

I like where this is going. As often happens I have some further requests for improvement. :)

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8638595 [details] [review]
PR for bug 1160616

Very close, just a few more tweaks and I think this will be good to merge. Thanks for your patience!

Comment 15

[William Lachance (:wlach)]

Comment on attachment 8638595 [details] [review]
PR for bug 1160616

Thanks for finishing this. :)