1184311 - Get classification looping correctly again with 1st/last pinned

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Jonathan French (:jfrench)]

A follow on bug from the recent n,p selection changes for the n,p unclassified hang fix in bug 1179260 (PR705, PR744).

To reproduce:
o load any push with >1 unclassified failure
o select the last unclassified failure in the push
o (spacebar) to pin it
o 'n' to navigate (loop back to the start)

Expected
The first unclassified failure in the push should be selected.

Observed
Treeherder puts up a thNotify yellow warning there are no more unclassified failures available.

The same can be observed going the other direction. Select the first unclassified, pin it, and hit 'p' to loop backwards to the last unclassified failure.

Comment 1

[Cameron Dawson [:camd]]

Attachment: PR

Comment 2

[Cameron Dawson [:camd]]

Well, dang me!  I didn't test that...  But this PR looks to fix it and still work in the other circumstances.

Comment 3

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/93b5c9e0b50bcf83788ce10f31f6b307971379d2
Bug 1184311 - Get classification looping correctly again with 1st/last pinned

Ensure the selectors don’t search for the next job in the pinboard.

Comment 4

[Jonathan French (:jfrench)]

So it looks like there's a bit more to do here. By default n,p puts up our yellow thNotify saying there's no unclassified failures available. Cameron is going to revert the change in master for now.

Comment 5

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/55c316314b9a9215e91cc6b2f7901107c8c35601
Revert "Bug 1184311 - Get classification looping correctly again with 1st/last pinned"

This reverts commit 93b5c9e0b50bcf83788ce10f31f6b307971379d2.

Comment 6

[Cameron Dawson [:camd]]

Sigh... I made a mistake... again...  Thanks to Jfrench for pointing out to me.

This line in the previous "fix":

        jobs = $(".th-view-content").filter(jobNavSelector.selector).filter(":visible, .selected-job");

SHOULD have been:

        jobs = $(".th-view-content").find(jobNavSelector.selector).filter(":visible, .selected-job");

The ``filter`` above only works on a list (``.th-view-content`` is NOT a list).  Whereas the ``find`` creates a list based on the children of ``.th-view-content`` and the selector.

I must have switched it while experimenting and it slipped through the cracks...  Dang.  Glad it only got to staging.

Comment 7

[Cameron Dawson [:camd]]

Attachment: Fixed PR

Comment 8

[Jonathan French (:jfrench)]

Comment on attachment 8634877 [details] [review]
Fixed PR

Ok so I've re-tested with Nightly and Chrome Release under OSX 10.10.3, and all related bugs appear to be fixed:

(original hang on repeated n,p events with classified failures)
1) bug 1179260

(after a save classification n,p should select next unclassified failure not cycle to the top)
2) variant https://bugzilla.mozilla.org/show_bug.cgi?id=1179260#c16

(this bug, n,p failing to loop after classifying final job in a push)
3) https://bugzilla.mozilla.org/show_bug.cgi?id=1184311

(the new .filter vs. .find n,p navigation failure)
4) variant https://bugzilla.mozilla.org/show_bug.cgi?id=1184311#c4

I've done additional save/delete workflows with navigation and I think we are ok to re-push to stage and have Ryan test it there. The diff between the reverted commit and this new PR look correct to me also. r+

Comment 9

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/19af914b68f89115390bd9ccf11de68b7d12b3a5
Bug 1184311 - Get classification looping correctly again with 1st/last pinned

Ensure the selectors don’t search for the next job in the pinboard.

FIXED: My earlier attempt used a JQuery ``filter`` where it should have used a
``find``

Comment 10

[Cameron Dawson [:camd]]

Thanks for the help, Jon!