1321798 - Rewrite autoclassification UI for better performance and maintainability

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect)

Description

[James Graham [:jgraham]]

The current autoclassification UI is slow and buggy enough that sheriffs are reluctant to use it. At the same time, the "giant blob of code" design is hard to understand and maintain. This rewrite will focus on the following things:

* Make the get/put operations a single HTTP request rather than multiple requests (later the GET can probably be merged with the general job details endpoint request).

* Reorganise the code into a set of angular 1.5 components.

* Experiment with workflow improvements to draw attention to only the lines that require specific input, and that make it easy to verify correctness of the autoclassifier or bug suggestions in other cases.

Comment 1

[James Graham [:jgraham]]

The current version of this code is running at http://treeherder-prototype.herokuapp.com

Comment 2

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

Left some feedback on the PR (only on the frontend stuff). As requested, I didn't take a look at the backend.

Comment 4

[Cameron Dawson [:camd]]

James and I are due to go over his PR when he gets back from PTO.  Before we do that, though, James would you please rebase this on latest master?  I'm going to clear the review flag for now.  Please reassign when you get back and have it rebased.

Thanks!  Looking forward to going over this with you.  :)

Comment 5

[Cameron Dawson [:camd]]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

This all looks pretty reasonable to me.  Nice to have it broken up a bit into the components.  I made a few comments.

Comment 6

[James Graham [:jgraham]]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

Rebased onto latest master, initial comments addressed, and migrations rewritten to use raw SQL (I will try deploying on -prototype and see how it goes).

Comment 7

[James Graham [:jgraham]]

Deploy onto -prototype went pretty well I think.

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

See PR for requested changes.

Comment 9

[Ed Morley [:emorley]]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

I've left the first round of review comments on the PR :-)

I'm really keen to keep this PR as small as possible (particularly given we discussed the problems of large PRs at the time of #1414 and agreed to be more strict about them in the future), so if any of the pieces of this can be split out as earlier separate PRs, that would be really helpful for us reviewing.

Comment 10

[William Lachance (:wlach)]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

Could you rebase this on top of master first?

Comment 11

[Ed Morley [:emorley]]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

Pending rebase after merge of bug 1339510 / bug 1339506.

Comment 12

[Ed Morley [:emorley]]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

I've only skim read briefly this cycle (reached my saturation point now due to the size of the PR unfortunately), but the production-affecting parts (migrations/celery tasks/settings.py etc) seem ok.

Comment 13

[William Lachance (:wlach)]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

Travis is failing and again this doesn't seem to be a pull request against master.

If I'm missing something and there's a specific way that you want me to look at these PR's, please let me know. But as it is I'm not sure how to review these.

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

I am happy for this to land as long as the nits I pointed out in the review are fixed (though of course the other parts will need to land again first).

Comment 15

[Cameron Dawson [:camd]]

Comment on attachment 8827922 [details] [review]
[treeherder] mozilla:autoclassify_frontend_rewrite > mozilla:master

This all looks pretty reasonable.  I really like the new organization.  And thanks heaps for adding more comments so some of the logic is easier to follow.

Comment 16

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/8e56de8590f9a085d2e77ba2279f243dfdbad1a5
Bug 1321798 - Rewrite the autoclassification UI (#2089)

This rewrite is intended to address the numerous problems that prevented
the old autoclassify UI from recieving wide adoption, and makes the
following changes:

* Moves the code to use angular 1.5 components, for maintainability

* Reduces the number of HTTP requests to one GET / one POST (associated
  with the TextLogError objects), for performance.

* Adds the concept of a selection so that operations can be performed on
  multiple lines at once

* Reorganises the panel a bit to fit better with the rest of the
  treeherder UI.

* Removes the "Ignore Others" action, which, though occasionally helpful,
  was based on an unreliable heuristic.

* Reduces the number of unhelpful bug suggestions displayed by default
  where there are many, and ensures that the most plausible suggestions
  are shown first.

* Adds keybindings for all operations, so that it is possible to use the
  autoclassify panel without touching the mouse.

* Adds basic heuristics for which option to select by default.