1072940 - Remove failure classification of "not classified"

Status: NEW

(Tree Management :: Treeherder, defect, P3)

Description

[Ryan VanderMeulen [:RyanVM]]

In order to usefully dogfood Treeherder, I need to mass-star the failures that were already starred on TBPL (instead of wasting time doing duplicate work). I found today that if I use the "not classified" option, the annotation gets saved by the jobs don't show up as starred.

Is that on purpose?

Comment 1

[Ed Morley [:emorley]]

I guess there are two problems:
a) A UX issue, since "not classified" in this case really means "remove current classification if set" I think?
b) A lack of an "other" or some other "ignore me" bin for failure classifications.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Given bug 1146491 and this bug, should we just get rid of "not classified" as a classification option? I've never used it in the three years of using Treeherder.

Comment 3

[Ed Morley [:emorley]]

Yeah let's remove it :-)

Comment 4

[Aditya Addepalli]

Do I remove line no. 146 and 147 in https://github.com/mozilla/treeherder/blob/75e2766e7b55f8b92b5f813369b64f1e3e65210c/treeherder/etl/jobs.py ?

Comment 5

[Ed Morley [:emorley]]

There are more places that need adjusting:
https://github.com/mozilla/treeherder/search?utf8=%E2%9C%93&q=%22not+classified%22&type=

Comment 6

[GitHub Autolander Bot]

Attachment: [treeherder] Dyex719:Dyex719-patch-2 > mozilla:master

Comment 7

[Farhan]

Hi Can I work on this? if no one else is doing it.

Comment 8

[Aditya Addepalli]

Can you please review the patch?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Just as before, no. Redirecting to Ed like last time.

Comment 10

[Ed Morley [:emorley]]

For mentored bugs, if the PR is ready for review, ask for review from the mentor via the "details" link on the attachment above :-)

(For non-mentored bugs, use the "suggested reviewers" feature inside the "details" section)

Comment 11

[Ed Morley [:emorley]]

(In reply to Farhan from comment #7)
> Hi Can I work on this? if no one else is doing it.

Generally if the bug has recent activity from someone else (such as the open PR in this bug from only 10 days ago), then it is being worked on by someone, so you'll be best off looking at another bug.

Comment 12

[Ed Morley [:emorley]]

Comment on attachment 8920163 [details] [review]
[treeherder] Dyex719:Dyex719-patch-2 > mozilla:master

Cameron is mentoring this bug (see the "Mentor" field and the inline bug change history)

Comment 13

[Cameron Dawson [:camd]]

After taking a look at the PR, this is not going to be as simple as removing "not classified" from the list of possible classifications.  We COULD do that, but I'm leaning against it.

If we remove "not classified" from the possible failure classifications, we'll need an expensive migration.  The ``job.failure_classification_id`` field in the DB is "Not Null" so we can't use the value of "Null" for "not classified" without changing the schema.

So our two options would be:

1. Change the schema to allow nulls on ``job.failure_classification``.  Do an update query to modify all the values of "1" in that field to be null.  Then modify the UI to leave the field null on ingestion and only set it when we classify.

2. Leave the schema as-is and just don't allow the user to choose "not classified" as an option when classifying.  

I think we should take approach 2 for a few reasons:

1. We wouldn't have to do an expensive db migration and change all the back-end code.
2. The changes would be only on the front-end and remove the workflow option of "classifying" a job as "not classified".  Instead, you'd just delete the classification and it would go back to the value of "1" being "not classified"
3. The existing filtering for "Unstarred Only" relies on a failure_classification_id of "1" and the filter code would be more complicated to look for null.  Not that bad, but a hassle.

So, let's go that way.  Just remove it as an option in the pinboard dropdown select for the classification when the user goes to classify a job and be done with it.

Here's where we populate that select: https://github.com/mozilla/treeherder/blob/51dc3cc938234c441c6b95a9715ac0daf5f03f9c/ui/partials/main/thPinboardPanel.html#L47-L49

You might create a separate ``$scope`` array that's used for that list which omits the "not classified" option.

Comment 14

[Cameron Dawson [:camd]]

Comment on attachment 8920163 [details] [review]
[treeherder] Dyex719:Dyex719-patch-2 > mozilla:master

Please see comment above on the approach I'd rather we take on this.  Thanks for your work on this!  :)

Comment 15

[Ed Morley [:emorley]]

Unassigning for inactivity. If you'd still like to work on it, let us know :-)

Comment 16

[Shiv Kumar]

Can I be assigned this if no one is working on this?

Comment 17

[Pavan Hegde]

Hi I wanted to work on this bug if no one has assigned this bug.

Comment 18

[asfiyab95]

Hi, can I take this up? Can someone please guide me on how/where to get started on this?

Comment 19

[Shivansh Srivastava]

Hi , Can I work on this bug now? Please also guide me how to proceed with this.

Comment 20

[sumit.rajgure123]

What should I do to fix this issue? Please provide the details.
I would like to fix it.

Comment 21

[Suyash Salampuria]

@camd
We can add the $scope here https://github.com/mozilla/treeherder/blob/master/ui/job-view/App.jsx#L98
and do the required filtering.
Shall I go on to proceed to solve this issue this way?

Comment 22

[Venkatesh Prabhu :vprabhu]

Hello. I would like to work on it. How can I get started?

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hello Venkatesh, Cam is gone from here and this not a good bug for people new to Treeherder (see comment 13).