With the caveat that hasn't received a ton of testing yet, all this stuff seems to work. Overall, I think this should be usable as-is today (I've added some rules to this install, although I haven't written much code after that):

• Reviewer status is tracked and shown per-user.
• Projects can be added as reviewers.
• You can manage project reviewers from the command line (Reviewers: alincoln, #security).
• Herald rules can add reviewers.
• Herald rules can add "blocking" reviewers, including project reviewers.
• Herald rules can now be conditional on the author's project memberships, which allows you to, for example, exempt members of the "Security" team from blocking review by Security. Alternatively, you can let the rule fire unconditionally, which creates the stricter rule "everyone, including members of the security team, must receive security review from some non-author user who is a member of the security team".
• Revisions don't switch to "accepted" until there are no rejects and no blocking reviewers.
• Revisions with a project you are a member of as a reviewer appear on your dashboard.
• Projects are available in Differential ApplicationSearch (main page) queries.
• Accepting a revision accepts it on behalf of your projects.
• You can now accept revisions which someone else has already accepted (possibly clearing project blocks), and reject revisions someone else has already rejected (raising additional issues).

Outstanding stuff and future work:

• Email: Although pending reviews for projects you're a member of will show up on your Differential dashboard, project members won't currently receive email about these reviews.
  • This is somewhat tricky to implement as "mail everyone in the project" from a technical point of view, because we currently split mail at a higher level than we resolve recipients -- no other recipient type can yield more than one mailable address. Not insurmountable, but not trivial.
  • It's not clear that a project reviewer should even mean "mail everyone on the project". Maybe a better approach is to let you set a mailing list address for each project (like security@yourcompany.com), and mail that instead. This is much simpler, although it would be somewhat desirable to switch Projects to ApplicationTransactions before implementing it.
  • If we do stick with "email everyone in the project", we probably need more options around managing, marking, and categorizing project mail. In particular, I expect users will want to, e.g., split their project mail from their normal mail. I don't have a sense of what this should look like. Mailing lists are an attractive alternative partly because they make this issue go away.
• Policies: Membership in a project which is a reviewer for a revision is not currently sufficient to punch a hole through revision policies, but probably should be. This starts getting messy to implement, though, to the point that it might be worth doing some level of project membership caching on User objects. Any such reviews today will just be stuck indefinitely: they will require review by users who can't see them. This probably won't actually happen in the wild, though.
• Design: There are a couple of things we're looking at improving.
  • There's a z-index issue with the keyboard shortcut element in some cases.
  • The "you have authority for this review" highlight looks a little out of place and feels a bit heavy. Some discussion in D7248.
  • There could be more visual distinction between projects and users in tokenizers. Some discussion and mocks in D7250.
• "Unblock" / "Sign-Off" Action: Currently, "Accept" accepts on behalf of projects you are a member of, but also "Accepts" on behalf of you, which is sufficient to transition a revision to "Accepted" on its own in the absence of other reviewers. It may be useful to have a "weak accept", which unblocks projects but does not accept on your behalf. This weak accept basically says "The security stuff looks fine, but someone who knows what this code is doing should actually look at the main change". This is not technically difficult to implement, but I'd like to get confirmation that it's desirable in the field before doing so. (I also haven't come up with language which feels really clear for this.)
• Revision Update Policies: Differential still uses "sticky accept", which means an accept covers future updates. Before this change, it was technically straightforward to disable this (T3202), but it is less straightforward now because we must distinguish between "accepts which used to be blocking" and "accepts which used to be nonblocking", and treat only the former as blocking. I generally think "sticky accept" is a good thing so I want to motivate this with real need in the field. Even without sticky accept, a user can make changes before pushing/landing -- and sticky accept helps get sunlight on those changes.
• Dashboard Bucketing: Bucketing of revisions into "Blocking Others", "Action Required", and "Waiting on Others" now produces some questionable results in edge cases. Notably, revisions you have accepted but which still have other blocking reviewers are listed under "Blocking Others", when they should probably be "Waiting on Others". It's prohibitively expensive to compute that we should move them without some other schema changes, though (if the accept is of an older diff, the bucket should probably change post-update, and we can't pull the current diff ID cheaply right now). These schema changes are also motivated elsewhere (T1081).
• Documentation: Although usage should mostly be straightforward for users familiar with Differential and Herald, I want to write a long screed about how using these features can impose huge process costs on your organization and should be used only with great reluctance. There are also some useful patterns (like the "author projects" thing) that we should call out.
• Ad-Hoc Blocking Reviewers: There's no way to add blocking reviewers from Differential or the CLI (T731). Despite some user interest, I still have a hard time understanding the need for voluntarily adding blocking reviewers. My hope is that a combination of Herald rules (for actually blocking reviewers) and the clearer per-reviewer UI (for at-a-glance per-reviewer status) will satisfy this. It's not technically difficult to implement this, but does create some UI problems (e.g., how do you add a blocking reviewer from the CLI? Maybe alincoln!, #security!)?
• Complex Reviewer Predicates: There's been some interest in arbitrary logic on reviewers (alincoln OR htaft) AND (#rules_committee) but unless we have to roll all of this back for some reason I no longer plan to ever pursue this. You can express OR rules by sticking the users in a project if you really insist, and AND rules through soft constraints or possibly through hard constraints if we implement ad-hoc blockers.
• Project Hashtags: You can't customize/select/edit project hashtags right now, which may lead to some awkward names; they will appear in commit messages. At some point I plan to let you bind #quality_assurance to #qa (the full name will still work) to make this more convenient.
• Removing Reviewers: You can get around blocking reviewers today by editing the revision and removing them. This is intentional, as without it there's no other way to unstick a review in unusual circumstances or work around someone writing a nonsense Herald rule, other than just committing it. It's also a workaround if there are any bugs with any of this. This may be worth examining in more detail in the future.
  • There's no audit trial for this action today, but clearly should be. But it's blocked by T2222, which is heavy.
  • We could make blocking reviewers immutable without undue technical effort.
  • Or, ideally, we could leave this and just ramp up the severity of the UI (e.g., scary prompt, spooky ghost icon, prompt from CLI) to make it implausible for a user to accidentally remove reviewers.

Likely as a consequence of these changes, this sequence of actions has a surprising result:

• Reviewer accepts.
• Author plans changes.
• Author updates.

The current behavior is, apparently, that the Reviewer is still in "accept". This is probably new, after the changes here, and is likely an awkward consequence of "Sticky Accept" interacting with per-reviewer unusually. A more desirable/expected behavior is that the revision ends up back in "Needs Review", without "accepts".

◀ Merged tasks: T5412.

◀ Merged tasks: T5984.

As part of this task, rPc6add6ae7329efc5f16d278f2c9df66662285205 has been done (a while ago).

It change the behavior such as a reviewer requesting changes become de facto a blocking reviewer. I'd like to change this behavior to get back to "anyone accept, it is good to go". Sure, going against someone request change is usually a dick move, but we'd like to strictly enforce that something is reviewed to get in, so in some cases, it can slow down quite a bit, for instance is the person that requested changes in the first place is not available.

We'd like to trust people good judgement on that. If 2 person (it the commiter and the accepting reviewer) think it is worthwhile to bypass one's request change, they most likely have a good reason to do so, and even if they don't, we can revert, or fix the thing.

Is there a way, by changing some configuration or adding an herald rule or whatever to get this behavior altered ?

You can "Edit Revision" and remove the user as a reviewer to override their block (you can move them to CC at the same time to avoid kicking them off the revision entirely). This is a strong action, but overriding their rejection is also strong.

There are countless way to work around this, but doesn't really fix things.

I've I'd be to work on a patch to add a config for this, if there any chances that this is going to be accepted ? Could you give me some pointer on where to start with that ?

I've I'd be to work on a patch to add a config for this, if there any chances that this is going to be accepted ?

There is no chance this would be accepted.

Why ?

This:

to add a config for this

Generally adding configs comes at a significant cost to the upstream (support, testing, maintenance) and additional cost for setting up Phabricator for admins. We've very anti-config options because of this, and certainly if we just added options every time someone asked, it'd be a very complicated product. We prefer installs with such specific needs to maintain local patches/forks instead.

In T1279#42118, @epriestley wrote:

• Ad-Hoc Blocking Reviewers: There's no way to add blocking reviewers from Differential or the CLI (T731). Despite some user interest, I still have a hard time understanding the need for voluntarily adding blocking reviewers.

We have a tool that can figure out who should review a particular diff based on some fairly complex rules. Integrating that tool with arcanist works, but we prefer that all reviewers approve or remove themselves from the review (which could be accomplished either by adding blocking reviewers from the CLI or T731) as it is our "Waiting on me" queues are pretty unhelpful.

Other use cases include 1 off situations where I worked with joe on a change and I really think he needs to review the diff in addition to the regular people herald will add. The most convenient path would be to add him as a blocking reviewer from the CLI or Web UI just this once instead of making a whole herald rule for the situation, and then simply have the review show up in my queue as "Action Required" only after he has reviewed it.

Hi, when can we expect this feature to be available?

Hi @msatish! Please see Planning for help understanding planning and prioritization. Thanks!

In T1279#176531, @epriestley wrote:

"reviewer list" referred to an element which was removed in 2013, and replaced with this one, as part of this task:

This resolves the primary concern for me. Thanks for all your work on this @epriestley !