1408671 - Make Filter-by-job-field classification filters, display their name not their id

Status: RESOLVED FIXED

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

Description

[Jonathan French (:jfrench)]

When using Filter by job field, an active filter using 'failure classification' appears to display its object id number, not its name.

To reproduce:
o click Navbar > Filter by job field 
o Select Filter Field > failure classification
o Select Value > (any classification value, eg. intermittent)
o click Add

Expected:
An active filter "failure_classification_id: intermittent"

Observed:
An active filter "failure_classification_id: 4"

Comment 1

[Jonathan French (:jfrench)]

And perhaps related, getFilterValue doesn't appear to be called anywhere.

https://github.com/mozilla/treeherder/search?utf8=%E2%9C%93&q=getFilterValue
https://github.com/mozilla/treeherder/blob/e04a9afe651917762b9f3ed925ac4f26ced80395/ui/js/controllers/main.js#L599-L609

Comment 2

[Jonathan French (:jfrench)]

I have a UI side fix so far, which changes the URl bar searchStr from eg. "filter-failure_classification_id=4" to a human readable "filter-failure_classification_id=intermittent" - which then also renders the value correctly in the filter bar. Still to figure out is how to get the main jobs table to present correctly on that change.

Comment 3

[Jonathan French (:jfrench)]

I put up a WIP branch filterbar-classification-display on my own GitHub which roughly mocks what I want to do:

(done) applying the name not the id number to the URL searchStr and filter bar
(tbd) applying the name to the job object, so it is filtered in the job table

Plus lots of other areas of interest marked during investigation, which may or may not be involved in the change.

Comment 4

[Jonathan French (:jfrench)]

Talking w/camd on IRC we agree it's more pain than gain to migrate the failure_classification_id from int to string on the backend, to support job filtering everywhere(URL, job table, filter bar) by string, per comment#3.

So the plan is instead to just convert the value to its string in our little light blue job filter bar. The URL filter will remain an int, as will the failure_classification_id on each job object.

Comment 5

[GitHub Autolander Bot]

Attachment: [treeherder] tojon:filterbar-classification-display > mozilla:master

Comment 6

[Jonathan French (:jfrench)]

Comment on attachment 8926242 [details] [review]
[treeherder] tojon:filterbar-classification-display > mozilla:master

At your leisure :) I've annotated the PR with hopefully some helpful info for the review.

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8926242 [details] [review]
[treeherder] tojon:filterbar-classification-display > mozilla:master

Just a couple nit-picks for now.  Awesome fix though!  :)

Comment 8

[Jonathan French (:jfrench)]

Comment on attachment 8926242 [details] [review]
[treeherder] tojon:filterbar-classification-display > mozilla:master

Nits done, let me know if there's anything else and I will squash before merge.

Comment 9

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/94739118f9f746ee7fd2c47af22896539d71cf17
Bug 1408671 - Display job field filter classification by name not id (#2922)

Comment 10

[Cameron Dawson [:camd]]

Comment on attachment 8926242 [details] [review]
[treeherder] tojon:filterbar-classification-display > mozilla:master

I went ahead and squashed it for you.  Thanks for doing this!  :)

Comment 11

[Jonathan French (:jfrench)]

Cool, thank you sir :)

Comment 12

[Jonathan French (:jfrench)]

Sigh. With existing classification_type filter(s) we lose the filter text after the reload. I am sure I tested this but perhaps related to the bs4 update or my own yarn update I just had to do locally to sync to master. Re-opening.

Comment 13

[Jonathan French (:jfrench)]

Adding a needinfo just as a reminder for camd for the revert.

Comment 14

[GitHub Autolander Bot]

Attachment: [treeherder] tojon:revert-filterbar-classification-display > mozilla:master

Comment 15

[Jonathan French (:jfrench)]

Comment on attachment 8928836 [details] [review]
[treeherder] tojon:revert-filterbar-classification-display > mozilla:master

Revert ahoy.

Comment 16

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/ec10d3364dec58ef5be1bb4995d827dcf7dda2e4
Revert "Bug 1408671 - Display job field filter classification by name not id (#2922)" (#2955)

This reverts commit 94739118f9f746ee7fd2c47af22896539d71cf17.

Comment 17

[Jonathan French (:jfrench)]

Thank you so much Ed. I'm working on a fix for the problem behavior now.

Comment 18

[GitHub Autolander Bot]

Attachment: [treeherder] tojon:field-filters-classification > mozilla:master

Comment 19

[Jonathan French (:jfrench)]

Comment on attachment 8929219 [details] [review]
[treeherder] tojon:field-filters-classification > mozilla:master

Ok, here's a more minimalist approach. It seems to behave properly in all my test cases.

Comment 20

[Cameron Dawson [:camd]]

Comment on attachment 8929219 [details] [review]
[treeherder] tojon:field-filters-classification > mozilla:master

Looks great!

Comment 21

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/cc61d2acc71c4617530ec641f7a10ad57d3e8e77
Bug 1408671 - Display job field filter classification by name not id