Page MenuHomePhabricator

In "arc diff", warn when some reviewers are away even if not everyone is away

Authored by epriestley on Feb 14 2019, 6:29 PM.



Ref T13249. See PHI810. We currently warn you when all reviewers are away, but not when only some reviewers are away.

This makes some amount of sense under the "anyone can accept anything" rules we sort of recommend, but a lot of installs realistically have tons of owner/package rules now.

Instead, if any reviewers are away, show the user exactly who is away and until when, then make sure they don't want to make any adjustments.

(We can do a better job of this after the toolsets change when we can use the new APIs, but this is an easy fix for now.)

Test Plan

Created a revision with multiple reviewers, either some or all of whom were away. Got appropriate output and prompt behavior.

Diff Detail

rARC Arcanist
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 14 2019, 6:29 PM
epriestley requested review of this revision.Feb 14 2019, 6:29 PM
amckinley accepted this revision.Feb 15 2019, 9:00 PM

This is clearly the smallest possible change that fixes PHI810, but what would be cooler would be detecting "this revision is unapprovable because of the combination of current away statuses and review requirements" so we only show this message when it's definitely a problem. Don't we have most of that code server-side already for deciding when a revision has reached the "approved" status?

If we don't want to go down that road, I'd change the text here to make it clear that there only might be a problem and that the user should use their own judgment. Something like

Some of the reviewers you have selected are currently away, which may delay the review and approval of this revision. Continue anyway with the current reviewers?


"even though some reviewers"

This revision is now accepted and ready to land.Feb 15 2019, 9:00 PM
epriestley added a comment.EditedFeb 15 2019, 9:49 PM

Today, this (normally/usually?) only fires against reviewers that the user actually typed in using their real keyboard, so I think we have some kind of reasonable-ish expectation that they believe those reviewers are currently (or soon) active.

It also fires before we actually create a revision, so Herald hasn't acted yet and hasn't added packages/projects/etc. We could fire this later in the flow, but then if the user says "n" ("no, let me pick different reviewers") we're in some amount of trouble since we already created the revision.

If this was regularly firing against a bunch of random auto-filled junk (today or in the future) I generally agree that we'd want to move toward making a stronger effort to raise this only if it's actionable. And it's possible that we'll want to move in that direction for whatever reason.

We could also do stuff like ignore busy states ending in the next 1-2 hours (e.g., prompt probably isn't useful if it's just telling you that I'm currently in a meeting for the next 20 minutes), and show the user what events the reviewers are attending (if they can see them).

T731 might motivate some of this, e.g. a "At least 3 reviewers must approve this change" with only 1 reviewer. But maybe just dumping that kind of stuff as warnings at the end of the output would be good enough? I guess we can cross that bridge when we come to it.

This revision was automatically updated to reflect the committed changes.