1401518 - Running "Add new jobs" without a decision task doesn't revert menu entry

Status: REOPENED

(Tree Management :: Treeherder, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

If you select "Add new jobs" while the build doesn't have a decision task yet, a warning is printed. But it also changes the menu entry to "Hide runnable jobs", which is wrong. Instead it should keep "Add new jobs" until there is actually something to display, and nothing errors out.

Comment 1

[Cameron Dawson [:camd]]

This is actually a Treeherder bug.  Just a simple mistake in the UI.  Should be easy to fix.

Comment 2

[GitHub Bugzilla PR Linker]

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

Comment 3

[Jonathan French (:jfrench)]

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

For review at your leisure :) It seems to be working ok in local testing.

Comment 4

[Cameron Dawson [:camd]]

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

Just a couple nits, but worth fixing as we move forward with es6 syntax.  Thanks!

Comment 5

[Jonathan French (:jfrench)]

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

Ok, tweakage done :)

Comment 6

[GitHub Bugzilla PR Linker]

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

Comment 7

[Jonathan French (:jfrench)]

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

Bugzilla PR Linker linked this because I happened to mention the bug number in the commit, but it really isn't related. So marking this 'obsolete' to not confuse things.

Comment 8

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/d41c3181737a6fc3aa9b8863cae70e7f21b3c189
Bug 1401518 - Make Add new jobs menu require a decision task

Comment 9

[Cameron Dawson [:camd]]

Thanks Jon!

Comment 10

[GitHub Bugzilla PR Linker]

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

Comment 11

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/8e4c7db6c50e0f1432d6c79d039a178b511b7d8d
Revert "Bug 1401518 - Make Add new jobs menu require a decision task" (#3186)

This reverts commit d41c3181737a6fc3aa9b8863cae70e7f21b3c189.

Comment 12

[Ed Morley [:emorley]]

Sorry I had to revert this for causing bug 1435023.

Comment 13

[Jonathan French (:jfrench)]

I have a prelim fix for the revert that may work. I'll open a separate PR for it.

Comment 14

[GitHub Bugzilla PR Linker]

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

Comment 15

[Jonathan French (:jfrench)]

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

This is a prelim WIP PR with a console print I still need to change. The tweak appears to quiet the new Angular 1.6 'Possibly unhandled rejection' errors to the console. The other change, appears to reduce the hits on the retrieve endpoint from 21 to 2 on initial page load, in my local instance. However I fear we'd still end up with lots of retrieve endpoint hits on prod, so I am somewhat unconvinced this is a solution.

Comment 16

[Cameron Dawson [:camd]]

I have a PR that should be landing somewhat soon that will change this code from angular to React.  Could we table this PR until then, since the approach and logic will change a bit?

Nevertheless, I think we should make sure that this check for whether the menu item is enabled and what the tooltip should be needs to not make an API call to the server.  So that might need a new function on ``resultsets_store.js`` or elsewhere to examine the decision task and just see if it exists and any characteristics that we can glean from it to determine the state of that menu item.  Otherwise we're making API calls for some people that won't even be clicking that menu.

I hope to have Bug 1401518 landed within a week or so and then this will MOSTLY be React code.  I'd be happy to chat on Vidyo and give you a primer on how that code will work.  But I think you'll find it much less confusing than AngularJS is, tbh.  Things are much more encapsulated.

Clearing the review for now.

Comment 17

[Jonathan French (:jfrench)]

10-4, sounds good.

Comment 18

[Jonathan French (:jfrench)]

(In reply to Cameron Dawson [:camd] from comment #16)
> I hope to have Bug 1401518 landed within a week or so and then this will
> MOSTLY be React code.

I noticed the bug number above is referencing this bug, rather than the one being landed :) This bug has languished a bit. If you want me to close my completed PR and someone can Reactify my changes in a new PR, or something, let me know.

Comment 19

[Cameron Dawson [:camd]]

Rats.  Sorry about my wrong bug number.  I meant to reference bug 1434677.  The new React code has landed now.  

Yeah, we can probably close that PR.  I can do that.  If you're still up for doing this, you may give it a shot.  I find the React code MUCH more straightforward, tbh. I'd be happy to help coach you on it, if you like.  Otherwise we could wait for someone else to address this.

This change would now happen in /ui/job-view/PushActionMenu.jsx.

Comment 20

[Jonathan French (:jfrench)]

For now I'll unassign myself then, feel free to harvest any logic out of my old PR. It has all the various workflows and corresponding tooltip dialogues covered.

Comment 21

[Cameron Dawson [:camd]]

I can confirm this still happens.  I think this fix should be accompanied by a notification that you need a decision task to add new jobs.