1438555 - Classifying a failure in the middle of the page breaks n/p navigation

Status: RESOLVED FIXED

(Tree Management :: Treeherder: Frontend, defect, P1)

Description

[Phil Ringnalda (:philor)]

STR:
1. https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
2. Select any job in the middle of the page (anything other than the first or last shown failures) and classify it
3. Type n

Expected: go to the next unstarred failure

Actual: go to the first unstarred failure on the page

Comment 1

[Phil Ringnalda (:philor)]

Ah, the reason I've been seeing it inconsistently is that there's a step 2.5, "wait until the job is shown as classified in the UI". If you hit bug 1438315 so that you type n before your classified job disappears from unstarred, TH still knows what was n and p, but once it disappears n becomes the first failure, and p becomes the last failure.

Comment 2

[Cameron Dawson [:camd]]

Ahh shoot, ok.  Yeah, I remember having to work around this scenario in the old ui.  I think I can have a fix for that pretty quickly, though.

Comment 3

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3234

Comment 4

[Cameron Dawson [:camd]]

Comment on attachment 8951438 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3234

Philor says he didn't notice this right away, so it's not critical to workflow so not worth a hot-fix.

Comment 5

[Ed Morley [:emorley]]

Comment on attachment 8951438 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3234

Many thanks :-)

Comment 6

[Cameron Dawson [:camd]]

Comment on attachment 8951438 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3234

Clearing the approval since Philor was not thrilled with my approach at a fix. 

The next approach I will take will be to ensure that the selected job shows whether or not it would otherwise be filtered out.  Once it's not selected anymore, it can then be filtered out.

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8951438 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3234

Hey there, I hope this is straightforward enough.  I had to do some callback stuff between parent and child a bit.  :)

This actually fixes an additional bug: Bug 1438315 because it was related.  Is there a special way I'm supposed to formulate the commit to notate this?  Was it something like "Bug XXXX, YYYY - Comment" ?

Comment 8

[Ed Morley [:emorley]]

Comment on attachment 8951438 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3234

(In reply to Cameron Dawson [:camd] from comment #7)
> This actually fixes an additional bug: Bug 1438315 because it was related. 
> Is there a special way I'm supposed to formulate the commit to notate this? 
> Was it something like "Bug XXXX, YYYY - Comment" ?

Perhaps just mention "bug YYYY" in the commit body, and then when it lands mark that bug fixed and mention fixed as part of this bug? Whatever you prefer :-)

Comment 9

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/1c9b2c0e2498151e7f10c7fb6824e00a321a3e0f
Bug 1438555 -Fix n/p after classifying a failure

This also fixes Bug 1438315 - Pinned jobs take a long time
to show the annotation. These two bugs were related so it
made sense to fix them at the same time.

Comment 10

[Cameron Dawson [:camd]]

Some of my changes broke this again. I added a test which should catch this in the future.

Comment 11

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/5063