Milo: Add an option to hide green steps

Milo: Add an option to hide green steps

This CL contains 2 things
1. Fixes buildbot build reporting of interesting vs normal steps.
2. Client-side hiding of normal steps.  Changed from single checkbox to 3 radio boxes.

BUG=673636
Review-Url: https://codereview.chromium.org/2886353002
Committed: https://github.com/luci/luci-go/commit/99f8e4b1c4b786ee903f40e5c71012bae7200cd1

[Ryan Tseng, 2017-05-18 00:26:46]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 00:26:50]

Dry run: CQ is trying da patch.

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

[Ryan Tseng, 2017-05-18 00:27:52]

hinoka@google.com changed reviewers:
+ nodir@chromium.org

[Ryan Tseng, 2017-05-18 00:27:52]

ptal, this fixes a pet peeve of mine

Demo:
https://1736-a5d341b-tainted-hinoka-dot-luci-milo-dev.appspot.com/buildbot/tr...

[commit-bot: I haz the power, 2017-05-18 00:33:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 00:34:00]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-05-18 02:41:04]

Nice!

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/buildbot/bui...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:231: case resp.Running, resp.Failure,
resp.InfraFailure, resp.Exception, resp.Warning, resp.WaitingDependency:
it seems that all but Success is interesting? 
if we add another non-successful status, it would be interesting too?
maybe do (bc.Status != resp.Sucess) instead?

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
File milo/appengine/frontend/templates/pages/build.html (right):

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:154: <input type="radio"
name="hider" id="showFail"><label for="showFail">Failure</label>
"Failures"? Since there may be multiple failures

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:332: var things =
$("#steps");
not used

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:346: }
I think this entire function can be implemented in one line

$("#steps").removeClass("all standard fail").addClass(filter)

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:349:
check($("#showFail").is(":checked"), "standard");
i think you meant check("standard")

[nodir, 2017-05-18 03:36:16]

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
File milo/appengine/frontend/templates/pages/build.html (right):

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:349:
check($("#showFail").is(":checked"), "standard");
On 2017/05/18 02:41:04, nodir wrote:
> i think you meant check("standard")

i recommend reviewing code yourself before sending for review. Then it is hard
to miss stuff like this.
I always do it for all my patchsets.

[Ryan Tseng, 2017-05-18 20:59:24]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:59:27]

Dry run: 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, 2017-05-18 21:04:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 21:04:46]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36362d61dd007710)

[Ryan Tseng, 2017-05-18 21:30:23]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 21:30:25]

Dry run: 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, 2017-05-18 21:37:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 21:37:23]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-05-18 22:50:22]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 22:50:32]

Dry run: CQ is trying da patch.

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

[Ryan Tseng, 2017-05-18 22:52:49]

Added some code to re-call the hook if navigating away from the page, then
clicking back.  It retains the old checkbox state, but the page state doesn't
follow.  This bug is now fixed.

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/buildbot/bui...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:231: case resp.Running, resp.Failure,
resp.InfraFailure, resp.Exception, resp.Warning, resp.WaitingDependency:
On 2017/05/18 02:41:04, nodir wrote:
> it seems that all but Success is interesting? 
> if we add another non-successful status, it would be interesting too?
> maybe do (bc.Status != resp.Sucess) instead?

Done.

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
File milo/appengine/frontend/templates/pages/build.html (right):

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:154: <input type="radio"
name="hider" id="showFail"><label for="showFail">Failure</label>
On 2017/05/18 02:41:04, nodir wrote:
> "Failures"? Since there may be multiple failures

Actually i'll say "Interesting Only".  A bit verbose, but since "Running" is
included as interesting I think Failure(s) is the wrong word.

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:332: var things =
$("#steps");
On 2017/05/18 02:41:04, nodir wrote:
> not used

Done.

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:346: }
On 2017/05/18 02:41:04, nodir wrote:
> I think this entire function can be implemented in one line
> 
> $("#steps").removeClass("all standard fail").addClass(filter)

That works, that's awesome.

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:349:
check($("#showFail").is(":checked"), "standard");
On 2017/05/18 02:41:04, nodir wrote:
> i think you meant check("standard")

Done.

https://codereview.chromium.org/2886353002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/pages/build.html:349:
check($("#showFail").is(":checked"), "standard");
On 2017/05/18 03:36:16, nodir wrote:
> On 2017/05/18 02:41:04, nodir wrote:
> > i think you meant check("standard")
> 
> i recommend reviewing code yourself before sending for review. Then it is hard
> to miss stuff like this.
> I always do it for all my patchsets.

Acknowledged.

[commit-bot: I haz the power, 2017-05-18 22:56:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 22:56:42]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-05-22 16:36:38]

lgtm

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
File milo/appengine/frontend/templates/pages/build.html (right):

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
milo/appengine/frontend/templates/pages/build.html:152: <input type="radio"
name="hider" id="showAll"><label for="showAll">All</label>
nit: maybe move label to the next line so it is consistent with other checkboxes

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
milo/appengine/frontend/templates/pages/build.html:333: var check =
function(filter) {
i think the parameter here is a css class name. filter may be easily confused
with jquery filter (e.g. .classSelector or #id)

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
milo/appengine/frontend/templates/pages/build.html:348: if
($(this).prop("checked") == true) {
why not `if ($(this).prop("checked")) {` ?

[Ryan Tseng, 2017-05-22 17:29:33]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 17:29:38]

Dry run: CQ is trying da patch.

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

[Ryan Tseng, 2017-05-22 17:29:49]

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
File milo/appengine/frontend/templates/pages/build.html (right):

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
milo/appengine/frontend/templates/pages/build.html:152: <input type="radio"
name="hider" id="showAll"><label for="showAll">All</label>
On 2017/05/22 16:36:38, nodir wrote:
> nit: maybe move label to the next line so it is consistent with other
checkboxes

Done.

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
milo/appengine/frontend/templates/pages/build.html:333: var check =
function(filter) {
On 2017/05/22 16:36:38, nodir wrote:
> i think the parameter here is a css class name. filter may be easily confused
> with jquery filter (e.g. .classSelector or #id)

Done.

https://codereview.chromium.org/2886353002/diff/50001/milo/appengine/frontend...
milo/appengine/frontend/templates/pages/build.html:348: if
($(this).prop("checked") == true) {
On 2017/05/22 16:36:38, nodir wrote:
> why not `if ($(this).prop("checked")) {` ?

I saw it written the other way in another example snippet.  I'm not sure why
they did it that way either.  I chalked it up to javascript conventions.

[Ryan Tseng, 2017-05-22 17:29:57]

The CQ bit was unchecked by hinoka@google.com

[Ryan Tseng, 2017-05-22 17:30:00]

The CQ bit was checked by hinoka@google.com

[Ryan Tseng, 2017-05-22 17:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2886353002/#ps70001
(title: "Reviewg")

[commit-bot: I haz the power, 2017-05-22 17:30:08]

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, 2017-05-22 17:38:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 17:38:05]

Try jobs failed on following builders:
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/364a0aac31257d10)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/364a0aac9a043d10)

[Ryan Tseng, 2017-05-22 17:45:54]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 17:46:05]

Dry run: 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, 2017-05-22 17:52:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 17:52:20]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-05-22 18:07:19]

The CQ bit was checked by hinoka@google.com

[Ryan Tseng, 2017-05-22 18:07:20]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2886353002/#ps90001
(title: "Retrain")

[commit-bot: I haz the power, 2017-05-22 18:07:29]

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, 2017-05-22 18:12:26]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1495476439707160,
"parent_rev": "9d852289caad98d19a53501c9cf22853f33d1351", "commit_rev":
"99f8e4b1c4b786ee903f40e5c71012bae7200cd1"}

[commit-bot: I haz the power, 2017-05-22 18:12:29]

Description was changed from

==========
Milo: Add an option to hide green steps

This CL contains 2 things
1. Fixes buildbot build reporting of interesting vs normal steps.
2. Client-side hiding of normal steps.  Changed from single checkbox to 3 radio
boxes.

BUG=673636
==========

to

==========
Milo: Add an option to hide green steps

This CL contains 2 things
1. Fixes buildbot build reporting of interesting vs normal steps.
2. Client-side hiding of normal steps.  Changed from single checkbox to 3 radio
boxes.

BUG=673636

Review-Url: https://codereview.chromium.org/2886353002
Committed:
https://github.com/luci/luci-go/commit/99f8e4b1c4b786ee903f40e5c71012bae7200cd1
==========

[commit-bot: I haz the power, 2017-05-22 18:12:30]

Committed patchset #6 (id:90001) as
https://github.com/luci/luci-go/commit/99f8e4b1c4b786ee903f40e5c71012bae7200cd1