1285004 - Use angular's $httpParamSerializer in perf controllers

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

Attachment: Example diff

In various places in perfherder's controllers, we use code like the following:

cmap.links.push({
  title: 'subtests',
  href: 'perf.html#/e10s_comparesubtest?' + 
  _.map(params, function(kv) { return kv[0] + '=' + kv[1]; }).join('&') });

We should be able to use $httpParamSerializer (https://docs.angularjs.org/api/ng/service/$httpParamSerializer) to do this job for us, instead of repeating the same code pattern (map, join) all over the place (see the attachment for one example of making this change).

Running git grep, I see other examples of this that could be fixed:

Hammersmith:treeherder wlach$ git grep "\.join('&')"
ui/js/controllers/perf/alerts.js:                        }).join('&'));
ui/js/models/perf/alerts.js:            return endpoint + '?' + _.map(urlParameters, function(v, k) {return k + '=' + v;}).join('&');
ui/js/perf.js:                                                })).join('&');


Shruti, do you want to take a look at this? It would be a good tour of some of the perfherder code, preparation for bug 1273513. You should be mostly able to follow the example I provided in the attachment

Comment 1

[GitHub Autolander Bot]

Attachment: [treeherder] SJasoria:Bug_1285004 > mozilla:master

Comment 2

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

I have made some changes. I hope they are good.

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

This looks like a good start to me. There are a few other instances of this pattern I pointed out in the bug that I'd also like to see replaced.

Comment 4

[Shruti Jasoria [:ShrutiJ]]

I have a doubt: 

In https://github.com/mozilla/treeherder/blob/master/ui/js/perf.js#L372

_.map is being operated on an array and not an object, whereas, $httpParamSerializer works only on objects. According to my understanding _.map has to be used in this case. Should I leave it like that?

Comment 5

[William Lachance (:wlach)]

(In reply to Shruti Jasoria [:ShrutiJ] from comment #4)
> I have a doubt: 
> 
> In https://github.com/mozilla/treeherder/blob/master/ui/js/perf.js#L372
> 
> _.map is being operated on an array and not an object, whereas,
> $httpParamSerializer works only on objects. According to my understanding
> _.map has to be used in this case. Should I leave it like that?

For a case like that you should rewrite the code to generate an object, not an array. I haven't tested it, but I think the following input should work ok as input to $httpParamSerializer:

{
  series: [ "[mozilla-inbound,1234,1,1]","[mozilla-central,1234,1,1]" ],
  higlightedRevisions: [ "abcd", "efgh" ]
}
  

Try generating an object like that and passing it in, if you have trouble let me know!

Comment 6

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

I have made some more changes. Please take a look.

Comment 7

[William Lachance (:wlach)]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

Making progress. :)

1. Could you also include the update I provided in the diff attached to those bug?
2. Could you try testing your changes? It would be a good exercise in figuring out where the code is used, and save me from having to point out functional problems. :)

Comment 8

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

I have updated the PR. After making the changes it seemed to work well locally. :)

Please take a look and let me know if any more changes have to be made.

Comment 9

[William Lachance (:wlach)]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

Just one small nit. Otherwise this looks ready to land. :)

Comment 10

[Shruti Jasoria [:ShrutiJ]]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

I have made the changes. I hope its good now. :)

Comment 11

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/f15e0bc2a9c80197a62f377a3390f823f5ce2132
Bug 1285004 - Use angular's httpParamSerializer in perf controllers

https://github.com/mozilla/treeherder/commit/090337330f4ce83480fa9932ce29e3d8a240fb17
Merge pull request #1658 from SJasoria/Bug_1285004

Bug 1285004 - Use angular's httpParamSerializer in perf controllers

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

Looks good now, thanks!

Comment 13

[GitHub Autolander Bot]

Attachment: [treeherder] SJasoria:Bug_1285004 > mozilla:master

Comment 14

[Shruti Jasoria [:ShrutiJ]]

Attachment: Changes made to compare.js

Made a separate pull request to add httpParamSerializer functionality to compare.js. I hope its good.

Comment 15

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/618fd99ee489bf998f309653bff137c12fbd48de
Bug 1285004 - Use httpParamSerializer in compare perf controller (#1724)

Comment 16

[William Lachance (:wlach)]

Comment on attachment 8774381 [details] [review]
Changes made to compare.js

already merged this