Page MenuHomePhabricator

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

Authored by epriestley on Feb 14 2019, 6:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 20, 4:35 PM
Unknown Object (File)
Thu, Apr 11, 9:01 AM
Unknown Object (File)
Sun, Apr 7, 6:01 AM
Unknown Object (File)
Wed, Apr 3, 10:04 AM
Unknown Object (File)
Sun, Mar 31, 3:29 PM
Unknown Object (File)
Feb 3 2024, 9:22 PM
Unknown Object (File)
Jan 15 2024, 8:59 AM
Unknown Object (File)
Dec 27 2023, 1:08 PM
Subscribers
None

Details

Summary

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

Repository
rARC Arcanist
Branch
away1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22001
Build 30054: Run Core Tests
Build 30053: arc lint + arc unit

Event Timeline

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?

src/workflow/ArcanistDiffWorkflow.php
1904

"even though some reviewers"

This revision is now accepted and ready to land.Feb 15 2019, 9:00 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.