Use angular's $httpParamSerializer in perf controllers

RESOLVED FIXED

Status

Tree Management
Perfherder
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: wlach, Assigned: ShrutiJ)

Tracking

Details

Attachments

(4 attachments)

Created attachment 8768539 [details] [diff] [review]
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
Created attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master
(Assignee)

Comment 2

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

I have made some changes. I hope they are good.
Attachment #8768655 - Flags: feedback?(wlachance)
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.
Attachment #8768655 - Flags: feedback?(wlachance) → feedback+
(Assignee)

Comment 4

2 years ago
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?
Flags: needinfo?(wlachance)
(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!
Flags: needinfo?(wlachance)
(Assignee)

Comment 6

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

I have made some more changes. Please take a look.
Attachment #8768655 - Flags: feedback+ → feedback?(wlachance)
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. :)
Attachment #8768655 - Flags: feedback?(wlachance) → feedback+
(Assignee)

Comment 8

2 years ago
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.
Attachment #8768655 - Flags: review?(wlachance)
Comment on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

Just one small nit. Otherwise this looks ready to land. :)
Attachment #8768655 - Flags: review?(wlachance) → review-
(Assignee)

Comment 10

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

I have made the changes. I hope its good now. :)
Attachment #8768655 - Flags: review- → review?(wlachance)

Comment 11

2 years ago
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 on attachment 8768655 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master

Looks good now, thanks!
Attachment #8768655 - Flags: review?(wlachance) → review+
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Created attachment 8774375 [details] [review]
[treeherder] SJasoria:Bug_1285004 > mozilla:master
(Assignee)

Comment 14

2 years ago
Created attachment 8774381 [details] [review]
Changes made to compare.js

Made a separate pull request to add httpParamSerializer functionality to compare.js. I hope its good.
Attachment #8774381 - Flags: review?(wlachance)

Comment 15

2 years ago
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 on attachment 8774381 [details] [review]
Changes made to compare.js

already merged this
Attachment #8774381 - Flags: review?(wlachance) → review+
You need to log in before you can comment on or make changes to this bug.