Allow revisions to have alternate acceptance conditions
Open, NormalPublic

Tags
Tokens
"Love" token, awarded by tandr."Like" token, awarded by Beltran-rubo."Like" token, awarded by Kwisatz."Like" token, awarded by aravindh."Like" token, awarded by itwasntandy."Like" token, awarded by edouard.kaiser."Like" token, awarded by austinstoker."Love" token, awarded by sharwell.
Assigned To
Authored By
ericfrenkiel, Jan 4 2012

Description

Currently, the acceptance condition for revisions is always:

  • no blocking reviewers;
  • no reject-current reviewers;
  • no reject-previous reviewers; and
  • at least one accept-current from a user (not a project).

When these conditions are satisfied, the revision transitions to "Accepted". Several installs have expressed interest in alternate acceptance conditions, particularly: all reviewers must accept and at least N reviewers must accept.

In the modern codebase, implementing alternate conditions per se is fairly straightforward. What's difficult is the stuff at the edges -- like configuring this stuff (the original request asks for this to be a per-revision option, and even if it's a global option we probably need to track it per-revision so things don't go goofy when it's changed, and most of the "N reviewers" use cases don't sound like users should be able to change it easily), making sure users understand what they need to do in order to get a revision accepted (i.e., "you need 2 more accepts" vs "you still need alincoln and htaft to accept"), getting revisions to bucket properly on the default revision list based on the acceptance criteria, and making reviewer status clear under these alternate conditions (e.g., if the rule is "all", every reviewer is essentially a blocking reviewer, and the UI should probably reflect that). Essentially, changing acceptance conditions touches a lot of interfaces, even though the core of it is a very small change.

Another area of interest is specific reviewers must accept, but blocking reviewers seem do do a reasonable job of that. T1279 discusses some improvements to make them less unwieldy.

(Once T1279 implements the reviewer! syntax, the original request would probably be served with Reviewers: a!, b!, too.)


Original Description

We have some complex code that often requires sign off from more than one engineer when a change is pushed. Can phabricator add an "all or nothing" review approval checkbox such that a review must be approved by each person CC'd or added as a reviewer? At the discretion of the developer to click the checkbox.

There are a very large number of changes, so older changes are hidden. Show Older Changes
eadler added a subscriber: eadler.Jun 11 2015, 8:12 PM
avivey changed the visibility from "All Users" to "Public (No Login Required)".Jun 22 2015, 9:45 PM
gabe added a subscriber: gabe.Jul 10 2015, 2:01 AM

My current team also has the workflow that a diff can't be landed until everyone on the team has reviewed it. We're working around this socially for the moment as suggested, but it would definitely be handy to have a "require all reviewers to accept before accepting" toggle. Judging by past experience with the phabulous phabricator team though... when this feature comes it will be well thought out and work very well. Thanks for all the work!

This issue is a big deal. We would modify the Phabricator code to use our policy, but not all teams sharing the Phabricator instance use the same policy we do.

Current rules:

  • no blocking reviewers;
  • no reject-current reviewers;
  • no reject-previous reviewers; and
  • at least one accept-current from a user (not a project).

Additional rules our team follows:

  • all reviewers which are users are treated as blocking reviewers (unblock by resigning or accepting)
  • at least two accept from a user (the second accept does not have to be an accept-current)
jhurwitz added a project: Restricted Project.Aug 3 2015, 10:25 PM
nickz added a subscriber: nickz.Aug 18 2015, 5:04 PM
Dru89 added a subscriber: Dru89.Sep 29 2015, 9:52 PM
jhurwitz added a comment.EditedOct 8 2015, 6:19 PM

Some people on our install are asking about this task.

Our problem seems to be rooted in the following: Many people use their "blocking others" and "action required" queues as the source of truth for what diffs they need to review. This doesn't work for diffs in the "all reviewers must accept" category after any one reviewer accepts (because it's moved to "waiting on others" in the queues of all other reviewers).

That being said, this task seems like quite a behemoth to me, and nobody's yet fully described what a solution might look like. I wonder if it might be possible to tackle this task in two intermediate steps -- eg, one step in which you can toggle a diff's acceptance condition between "any reviewer" and "all reviewers" (since this seems like it may be the most common alternate acceptance condition?), and then only later tackle the more generalized problem some people are describing here.

@epriestley, do you have a sense of

  • whether upstream wants to wait until it can tackle the generalized problem, or whether you'd be okay chipping away at smaller use cases first,
  • when in the roadmap this might fit in, and
  • how much work it would take to build?

whether upstream wants to wait until it can tackle the generalized problem, or whether you'd be okay chipping away at smaller use cases first,

I don't think they're very separable.

I expect the cost to go from 1 ruleset to 2 rulesets is 99% of the cost here, and that adding additional rulesets will be mostly trivial. We have a fairly well established pattern for modular extensibility at this point and everything we apply it to breaks down more or less like that -- getting the core is the hard part. We can start with "all must accept" only, but I suspect this won't meaningfully reduce the implementation cost (and my gut is that doing both "all must accept" and "N of M must accept" at the same time will probably result in fewer API changes later and be a net benefit overall, although perhaps the cost to select "N" for rules like "N of M" or "at least X% of M" is high enough to justify holding off on it).

The implementation will look like essentially all other modern modular extension points look:

abstract class DifferentialReviewerRuleset { ... }

final class DifferentialReviewerAnyRuleset { ... }

final class DifferentialReviewerAllRuleset { ... }

These classes will have some methods like (for "all reviewers"):

public function shouldMarkRevisionAccepted(DifferentialRevision $revision, array $reviewers) { 
  if (!$reviewers) {
    return false;
  }

  $all_accepted = true;
  foreach ($reviewers as $reviewer) {
    switch ($reviewer->getStatus()) {
      case DifferentialReviewerStatus::ACCEPTED:
      case DifferentialReviewerStatus::ACCEPTED_PREVIOUS:
        break;
      default:
        $all_accepted = false;
        break;
    }
  }

  return $all_accepted;
}

The UI and Editor classes will instantiate the ruleset and make a call to it. The block of logic in DifferentialTransactionEditor->applyFinalEffects() which implements the "any reviewer" ruleset will be replaced with, approximately:

$object->getReviewerRuleset()->shouldMarkRevisionAccepted(
  $object,
  $object->getReviewerStatus());

Particularly, this will let you implement a ruleset yourself:

final class CustomAllReviewersUnlessTheyAreASeniorEngineerInWhichCaseJustThemRuleset { ... }

Possibly, these rulesets should really be generalized across Differential + Audit, although I think "all must accept" is more clear-cut in audit.

So this part is very straightforward, and would take only a few minutes to implement. However, it would be very difficult to understand or use and would almost certainly imply a substantial support cost for us by generating a lot of questions from users. I'm tentatively fine with implementing a very small version of this class, but I'm not sure a small version exists that isn't confusing / won't create a support problem in the upstream. Most of the big problems here are support / usability problems. In particular:

  • How do we decide which ruleset a revision uses? How does a user choose it? How do we show it in the UI? How do we explain what the rulesets mean to users?
  • How do we show users what the requirements to get a revision to move to "Accepted" are?
  • Can users change rulesets? How? What, if anything, prevents users from changing from a strict ruleset to a liberal ruleset to avoid review rules?
  • The bucketing problem is its own challenge and described separately in T9372, although I think it is avoidable for the "All Reviewers" ruleset, specifically, because the bucketing rules are not different. For "M of N", the bucketing rules are different, and are not reasonably database-expressible.

Some of those aren't too hard and we can brute force our way through them to some degree (e.g., it's reasonable to put ruleset explanations behind some kind of "What does this mean?" link) but some of those questions have simple answers which would make Differential significantly more daunting for new users and/or harder to support for us. I suspect we can find much better answers to most of those questions by designing the interactions more carefully.

For example, a naive approach would be to put a "Reviewer ruleset:" field in arc diff where you enter "all" or "any", let users change it freely, put a big blob of text on the revision view explaining what's going on, add a Herald action for changing it, and dump it into the attributes on the list view. This would technically work, but it's substantially inelegant and almost certainly not the best approach we could come up with. "Reviewer Ruleset" would be confusing and meaningless for new users, the big blob of text would be pointless for experienced users, we'd get tickets about users degrading rulesets when they shouldn't sooner or later, and so on.

when in the roadmap this might fit in,

T8783, T9378 and T9379 are from you guys so we can put it ahead of those. I'll update and reconcile Prioritized soon (next few days?) to provide a clearer picture here. T9123 and T9252 are moving out of focus (we've hosted our own builds since earlier this week) but T182 has been optioned in and some other things have shuffled around.

how much work it would take to build

I don't know what the answers to most of those UI / usability questions are yet. I'd guess it's like 8 hours of work total, but only after thinking about it for a week.

This may be more involved if it triggers migrating reviewers from edges to a dedicated table, which I think should happen at some point. Doing so may simplify some of the UI questions.


Your particular issue is also possibly fixable by sidestepping the entire issue, solving T9372 instead, and then leaving "Accepted" revisions which you have not acted on in your queue rather than moving them to "Waiting on Others". I think this would be a reasonable default, particularly if bucketing was configurable so that users who wanted to ignore these revisions could reconfigure bucketing to have a similar behavior to the previous behavior.

weyrick removed a subscriber: weyrick.Oct 9 2015, 3:15 PM

Getting a ruleset for N of M reviewers would be enormously helpful, if while you are doing this you see a good way to add a time criteria that would help us out. Our current ruleset is roughly ( (90% of reviewers approve) AND (no one requested changes) ) OR ( (it has been 8 business hours) AND (at lease 2 reviewers approve) ) We anticipate in the future we may need to prove this rule is being followed so we are very interested in this feature.

How do we decide which ruleset a revision uses? How does a user choose it? How do we show it in the UI?
Can users change rulesets? How? What, if anything, prevents users from changing from a strict ruleset to a liberal ruleset to avoid review rules?

I imagine the ruleset would be assigned as a Herald Rule and treated much the same as add reviewers and blocking reviewers. I think adding rulesets and blocking rulesets also makes sense. The only way to change which rulesets are applied would be to change the Herald rules.

kaya added a subscriber: kaya.Dec 16 2015, 5:45 PM
Kwisatz added a subscriber: Kwisatz.
eadler added a project: Restricted Project.Feb 23 2016, 6:51 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Feb 23 2016, 6:58 AM
joshma added a subscriber: joshma.Feb 24 2016, 9:47 PM
Beltran-rubo added a subscriber: Beltran-rubo.
sfrench added a subscriber: sfrench.Mar 6 2016, 1:51 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Mar 11 2016, 6:25 AM

T8887 is also related to this task

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Mar 11 2016, 5:57 PM
eadler edited projects, added Restricted Project; removed Restricted Project.
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Mar 18 2016, 4:49 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 18 2016, 12:24 AM
eadler edited projects, added Restricted Project; removed Restricted Project.
defuzz removed a subscriber: defuzz.Apr 18 2016, 7:01 PM
srijan added a subscriber: srijan.May 11 2016, 2:01 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 13 2016, 9:37 PM
tandr awarded a token.May 17 2016, 5:07 PM
tandr added a subscriber: tandr.May 17 2016, 5:11 PM
erich added a subscriber: erich.May 23 2016, 12:49 PM
kAworu added a subscriber: kAworu.May 24 2016, 9:54 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Mon, Jul 4, 9:10 PM

Add Comment