1239841 - when viewing a specific alert id, clicking on the top level 'alerts' link yields no action

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

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

I am viewing:
https://treeherder.allizom.org/perf.html#/alerts?id=1773

at the top of the page, I click 'Alerts' and expect to see the full list of alerts, instead nothing happens.

Comment 1

[William Lachance (:wlach)]

Hmm. Setting window.location.href to 'https://treeherder.allizom.org/perf.html#/alerts' in the console seems to work, so I'm not sure why this doesn't.

Comment 2

[Julien Pagès (:parkouss)]

Wow. I tried by adding

ui-sref-opts="{reload: true}"

to the <a> link, and now it refresh the url "perf.html#/alerts?id=1773", even if firefox show at the bottom that the link is "perf.html#/alerts"! :(

Eh, the web becomes a lot complex. I'll dig a bit into https://github.com/angular-ui/ui-router since I don't know the framework.

Comment 3

[William Lachance (:wlach)]

(In reply to Julien Pagès (:parkouss) from comment #2)
> Wow. I tried by adding
> 
> ui-sref-opts="{reload: true}"
> 
> to the <a> link, and now it refresh the url "perf.html#/alerts?id=1773",
> even if firefox show at the bottom that the link is "perf.html#/alerts"! :(
> 
> Eh, the web becomes a lot complex. I'll dig a bit into
> https://github.com/angular-ui/ui-router since I don't know the framework.

I don't *think* solving this should require delving into angular, but I could be wrong.

Comment 4

[Julien Pagès (:parkouss)]

So, applying this patch do the work:

> 1 file changed, 1 insertion(+), 1 deletion(-)
> ui/perf.html | 2 +-
> 
> modified   ui/perf.html
> @@ -30,7 +30,7 @@
>        </span>
>        <a ng-class="{active: $state.includes('graphs')}" class="btn btn-view-nav" ui-sref="graphs">Graphs</a>
>        <a ng-class="{active: $state.current.name.indexOf('compare') >= 0}" class="btn btn-view-nav" ui-sref="comparechooser">Compare</a>
> -      <a ng-class="{active: $state.current.name.indexOf('alerts') >= 0}" class="btn btn-view-nav" ui-sref="alerts">Alerts</a>
> +      <a ng-class="{active: $state.current.name.indexOf('alerts') >= 0}" class="btn btn-view-nav" href="perf.html#/alerts">Alerts</a>
>        <div class="nav navbar-nav navbar-right">
>          <!-- Help Menu -->
>          <span class="dropdown">

but I guess we want to use this ui-sref attribute :) So I'll dive in the doc.

Comment 5

[GitHub Autolander Bot]

Attachment: [treeherder] parkouss:alerts-link > mozilla:master

Comment 6

[Julien Pagès (:parkouss)]

Attachment: fix alerts link

So, I tried to update ui-router (using latest git revision on master), no luck.

I narrowed down the issue. ui-router does merge the current parameters with the new ones to build the new parameters, then it compare them to know if somethings needs to be done. Yead, sounds funny, here is the link (in state.transitionTo method):

https://github.com/mozilla/treeherder/blob/master/ui/vendor/angular/angular-ui-router.js#L3144

this is done because inherit is set to True by the state.go method,

https://github.com/mozilla/treeherder/blob/master/ui/vendor/angular/angular-ui-router.js#L3068

You can look at the implementation of inheritParams, this basically merge dict params. So, this looks like this:

fromParams: {id: "1973"} // this is our current params
toParams: {}  // this is the value of the next params we ask for. empty dict, since we don't want any param

now the call to inheritParams will transform toParam by merging it with fromParams, and we will have...

toParams: {id: "1973"}

So now toParams == fromParams, so ui-router does nothing. My patch here really looks like a hack, it pass a param option {id: null} so it is not merged and all works.

Will, tell me what you think. My guess is that it IS a hack - but I don't see any other way to fix it, and found nothing on the web. Maybe we can report the issue in ui-router, and wait for them to find a solution. maybe a new syntax, like passing null as parameters (instead of having to list the parameters names!) would be a solution to not do that merge?

Comment 7

[William Lachance (:wlach)]

Comment on attachment 8711514 [details] [review]
fix alerts link

I think this looks fine, so much so that I'm just going to give it an r+. :) 

I agree that this is a hack, but in practice I've found that this type of hackery is sometimes required when doing web development.

It might be worth opening an issue with ui-router if you're feeling motivated to do so, though I believe the future of angular routing is in this project:

https://github.com/angular/router

Comment 8

[Julien Pagès (:parkouss)]

Cool, feel free to land this then. :)

I'll look into the opened bugs in https://github.com/angular/router, and may open one if I can't see anything related to that.

Comment 9

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla/treeherder/commit/ce4efc143a92d44fc33c11dca27c27b893953a40

Comment 10

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/56345c4a960c757e9aed8866f8b66d1e034d15f0
Bug 1239841 - clicking on the top level 'alerts' link yields no action

This happen when we already are viewing a specific alert id.

The problem is that ui-router merge the parameters from the current url
to determine the new parameters state. So the current is is merged, and
ui-router thinks that there is nothing to do since the states and
parameters are equivalent...

This fixes the issue, but it looks like a hack.

Comment 11

[William Lachance (:wlach)]

This is fixed now, thanks again Julien!