Fixes sorting on alerts tables when dealing with grouped alerts.

Updates alerts-table (on dashboard and group-reports) to sort correctly. 
Sorts the inner groups first, then sorts based on the highest/lowest in each group. Also added a few alerts to the alerts-page-test to convince myself that this works correctly.

Collapsed view:
http://i.imgur.com/ukc4BIk.png

Expanded view:
http://i.imgur.com/jDsj4I6.png


BUG=catapult:#2774

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/982d024dadecb5a6b73041f349dd0032c532967e

[jessimb, 2016-11-21 20:55:53]

Description was changed from

==========
fixing lint errors


fixing lint errors


hacked a fix together

BUG=catapult:#2774
==========

to

==========
Updates alerts-table (on dashboard and group-reports) to sort correctly. 
Sorts the inner groups first, then sorts based on the highest/lowest in each
group. Also added a few alerts to the alerts-page-test to convince myself that
this works correctly.

Collapsed view:
http://i.imgur.com/ukc4BIk.png

Expanded view:
http://i.imgur.com/jDsj4I6.png


BUG=catapult:#2774
==========

[jessimb, 2016-11-21 20:58:55]

jessimb@chromium.org changed reviewers:
+ eakuefner@chromium.org

[jessimb, 2016-11-21 20:58:56]

PTAL

[eakuefner, 2016-11-23 16:33:21]

Looking good, just a few comments/questions.

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/alerts-table.html (right):

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:528: for (var i = 0; i <
this.alertList.length; i++) {
I'm probably missing something, but out of curiosity, why do you change the
iteration order here? I agree that if there's no good reason to iterate
downwards then we should just go ahead and iterate upwards, but I'm puzzled
about why it was that way before.

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:814: // in a group+group headers
(the group member with the highest
The punctuation and word choice are making this sentence difficult to parse.
Just brainstorming here, but something like:

We have two sorting levels: alerts within groups and toplevel alerts, which are
either group headers (the group member with the highest sorted value) or alerts
without a group.

allows me to see the structure a little better -- "sort" is an overloaded word,
and I think defining a term like "toplevel alert" gives a little bit more of a
mental handle on what's happening.

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:839: // If the alert has no
group, it will be added to
This comment is unnecessary; it's clear that this is the case because you are
checking alert.group in the conditional above.

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:851: // Only get the first alert
in a group for alertsToSort.
I'm not sure that the for loop is necessary here; it seems like you can write
this without using a loop.

[jessimb, 2016-11-23 17:08:09]

PTAL

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/alerts-table.html (right):

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:528: for (var i = 0; i <
this.alertList.length; i++) {
On 2016/11/23 at 16:33:20, eakuefner wrote:
> I'm probably missing something, but out of curiosity, why do you change the
iteration order here? I agree that if there's no good reason to iterate
downwards then we should just go ahead and iterate upwards, but I'm puzzled
about why it was that way before.

That's a good question. It was the reason that the groups always picked the last
member to be the header, so rather than .reverse-ing the list I made this loop
go forward. I'm not sure why it was ever downwards. There is another for loop
below that was downwards too which would correct the first one (I speculate).

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:814: // in a group+group headers
(the group member with the highest
On 2016/11/23 at 16:33:20, eakuefner wrote:
> The punctuation and word choice are making this sentence difficult to parse.
Just brainstorming here, but something like:
> 
> We have two sorting levels: alerts within groups and toplevel alerts, which
are either group headers (the group member with the highest sorted value) or
alerts without a group.
> 
> allows me to see the structure a little better -- "sort" is an overloaded
word, and I think defining a term like "toplevel alert" gives a little bit more
of a mental handle on what's happening.

I agree that this was confusing to read. Thanks for the suggestion!

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:839: // If the alert has no
group, it will be added to
On 2016/11/23 at 16:33:20, eakuefner wrote:
> This comment is unnecessary; it's clear that this is the case because you are
checking alert.group in the conditional above.

Done

https://codereview.chromium.org/2524473002/diff/20001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:851: // Only get the first alert
in a group for alertsToSort.
On 2016/11/23 at 16:33:20, eakuefner wrote:
> I'm not sure that the for loop is necessary here; it seems like you can write
this without using a loop.

Yep. I thought this was an object, not an array.
Done.

[eakuefner, 2016-11-23 17:17:49]

eakuefner@chromium.org changed reviewers:
+ sullivan@chromium.org

[eakuefner, 2016-11-23 17:17:49]

lgtm, +Annie for OWNERS

[sullivan, 2016-11-23 17:23:17]

The CQ bit was checked by sullivan@chromium.org

[sullivan, 2016-11-23 17:23:17]

lgtm

[commit-bot: I haz the power, 2016-11-23 17:23:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 17:41:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479921797160770,
"parent_rev": "476d031f85fa622d1688ba2e3c08b2a5bb8d437a", "commit_rev":
"982d024dadecb5a6b73041f349dd0032c532967e"}

[commit-bot: I haz the power, 2016-11-23 17:41:55]

Description was changed from

==========
Updates alerts-table (on dashboard and group-reports) to sort correctly. 
Sorts the inner groups first, then sorts based on the highest/lowest in each
group. Also added a few alerts to the alerts-page-test to convince myself that
this works correctly.

Collapsed view:
http://i.imgur.com/ukc4BIk.png

Expanded view:
http://i.imgur.com/jDsj4I6.png


BUG=catapult:#2774
==========

to

==========
Updates alerts-table (on dashboard and group-reports) to sort correctly. 
Sorts the inner groups first, then sorts based on the highest/lowest in each
group. Also added a few alerts to the alerts-page-test to convince myself that
this works correctly.

Collapsed view:
http://i.imgur.com/ukc4BIk.png

Expanded view:
http://i.imgur.com/jDsj4I6.png


BUG=catapult:#2774

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-11-23 17:41:56]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...