Closed Bug 1075473 Opened 10 years ago Closed 10 years ago

Add back the "Cancel all jobs" button for non-try repos, but improve the UX

Categories

(Tree Management :: Treeherder, defect, P3)

Product:
Tree Management
Component:
Treeherder
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: emorley, Assigned: KWierso)

References

Details

Attachments

(1 file)

Link to pr 253
49 bytes, text/x-github-pull-request
Details | Review 
TBPL only showed the "cancel all jobs" button for Try repos, since using it on non-try causes pending jobs for unrelated pushes to be cancelled.

Treeherder shows the button (and with no prompt; bug 1074858) on all repos.

I think we should grey the button out on non-try (preferable to hiding it, since it causes confusing UX) and give an appropriate tooltip.

Or even better, let's do that, but allow users with is_staff to use it on the risky trees anyway (but with an even sterner prompt message).
(In reply to Ed Morley [:edmorley] from comment #0)
> Or even better, let's do that, but allow users with is_staff to use it on
> the risky trees anyway (but with an even sterner prompt message).

+1. I'm just going to cancel via self-serve otherwise, which has less than ideal UX (orphaned jobs).
Bug 1089020 (whose commit message is accidentally in comment 2) hid the button for non-try trees, which reduces the priority of this for now. Let's make this bug about re-adding the button for non-try trees, but making it greyed out/with a tooltip (so users understand why they can't use it) and also possibly allowing sheriffs to use it still.
No longer blocks: treeherder-dev-transition
Priority: P2 → P3
Summary: Cancel all jobs is allowed for repos where using it may be risky → Add back the "Cancel all jobs" button for non-try repos, but improve the UX
Attached file Link to pr 253 — 
This still has the button showing up for some users in some places but not in others, but it will at least let sheriffs and other is_staff users use the button on non-try trees. 

The prompt added in bug 1074858 should probably land first, otherwise I'm back to my accidental clicks accidentally canceling all jobs on an m-c merge.

If you know how I could conditionally style the button so non-is_staff users on non-try trees would see a 'disabled' button state while leaving it enabled for is_staff users everywhere and enabled for any user on try trees, I'd be happy to do that instead.
Attachment #8513699 - Flags: review?(emorley)
(In reply to Wes Kocher (:KWierso) from comment #4)
> If you know how I could conditionally style the button so non-is_staff users
> on non-try trees would see a 'disabled' button state while leaving it
> enabled for is_staff users everywhere and enabled for any user on try trees,
> I'd be happy to do that instead.

I think it might need two buttons, one with no onclick (and disabled styling) and the other as is, with different ng-shows for each.

Cameron, can you think of a better way? :-)
Assignee: nobody → kwierso
Status: NEW → ASSIGNED
Flags: needinfo?(cdawson)
Comment on attachment 8513699 [details] [review]
Link to pr 253

Actually, I'm wondering whether with bug 1074858 landed, if perhaps we still want to show the button to all users after all?
Attachment #8513699 - Flags: review?(emorley)
you can use the ``ng-class`` directive to set a conditional class of ``disabled``.  It will add to the other classes set in the ``class`` attribute.  Or, like Ed said, you could have two, and use ``ng-if`` to decide which one to show.

I'm not sure what the policy on comment 6 should be.  Should a non-sheriff be able to cancel all jobs on a m-i resultset?  I suppose, alternately, we could either show the "are you sure" dialog, or if the user isn't qualified, show them an "you can't do that" dialog.  

I think I prefer graying the button with a tooltip explaining why they can't do it is better, imo.
Flags: needinfo?(cdawson)
We talked about this in the sheriff meeting today. I think the consensus at this point is that there's no real point restricting the button to is_staff since anyone can go into self-serve and hit the "cancel all" button in there with just an LDAP account (and that "cancel all" button leaves orphaned jobs, unlike the button in TH).

With the prompt added in bug 1074858, there's hopefully no more accidental button presses, which was the main problem with it on non-try repos.

It might be good in the long run to swap the cancel all buttons with the mc-merge link, so that "Cancel all" is hidden in the menu for each push, and mc-merge gets a top-level button instead.

We may also want to make the prompt message a little more explicit about the dangers of using it on non-try trees ("Are you really super extra sure you want to do this?") if everyone can see it.
Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/cf4dad0378f4d63721f2cac809f564ea83461c62
Bug 1075473 - Allow is_staff users to see the cancell all button

https://github.com/mozilla/treeherder-ui/commit/64392a28353e2c3abda3e88556add929db7c8e5a
Merge pull request #253 from KWierso/CANCELISSTAFFNONTRY

Bug 1075473 - Allow is_staff users to see the cancell all button
(In reply to Wes Kocher (:KWierso) from comment #8)
> It might be good in the long run to swap the cancel all buttons with the
> mc-merge link, so that "Cancel all" is hidden in the menu for each push, and
> mc-merge gets a top-level button instead.

Cancel all was moved from the menu to the bar, after people struggled to find it (on Try) iirc.

> We may also want to make the prompt message a little more explicit about the
> dangers of using it on non-try trees ("Are you really super extra sure you
> want to do this?") if everyone can see it.

This sounds like a good idea, would you mind filing a bug for it? :-)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to Wes Kocher (:KWierso) from comment #11)
> Split that off into bug 1095167.

Thanks :-)
Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/ae1d302bf350bbbaeedef7e4108a8dd944b53e55
Bug 1075473 - Hide the cancel all button for all repos that aren't in the try group.

https://github.com/mozilla/treeherder/commit/9f6f1a83f9ce1e5bfbbc898f3943a622e5dc8d63
Merge pull request #249 from KWierso/KILLCANCEL

Bug 1075473 - Hide the cancel all button for repos not in the 'try' group

https://github.com/mozilla/treeherder/commit/4cb026147c5442b0b5860ba97da4b511d65180c1
Bug 1075473 - Allow is_staff users to see the cancell all button

https://github.com/mozilla/treeherder/commit/5dd1da762448d6c40725e79663f31633228eed11
Merge pull request #253 from KWierso/CANCELISSTAFFNONTRY

Bug 1075473 - Allow is_staff users to see the cancell all button
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: