Show per-reviewer status in Differential revision reviewer list
Closed, ResolvedPublic

Description

It would be desirable to show the individual status of each reviewer on a revision in the reviewer list. Specifically, we currently render:

Reviewers: alincoln, ugrant, htaft

It would be better to render:

Reviewers: alincoln (accepted), ugrant (accepted previous revision), htaft (where is he?)

...although probably with pretty icons instead.

This would give users an at-a-glance way to check on all reviewers (versus overall revision status) and ease some of the desire for more complicated, high-process workflows like T731, without adding any process.

The biggest blocker is that we don't store <user, status> data right now, so we could figure this out but only by replaying the comment history, which is a bit silly. We should migrate revision requests to Edges and start storing this data, then use it to render a more useful reviewers list.

Some tricky cases are update-after-accept and update-after-reject. We can probably handle those by distinguishing between "accepted" and "accepted previous version".

This should be accompanied by a minor workflow change: allow accepting an accepted diff, and allow requesting changes on an already-requested diff.

Once this data is available, we can expose it to custom fields and kind of hand-wave a quasi-solution to T731.

Details

Differential Revisions
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Commits
Restricted Differential Revision / rP3c19f363bf47: Provide result type information in tokenizers
D7360 / rPc69beb7988d1: Stop writes to the old Relationship table
Restricted Differential Revision / rP059860047615: Always pass handles to tokenizers, not `<phid -> name>` maps
Restricted Differential Revision / rPc6f9316a778b: Add missing `case` for personal ruls which create blocking reviewers
Restricted Differential Revision / rP953ff197bf26: Allow Herald rules to be disabled, instead of deleted
Restricted Differential Revision / rPc6add6ae7329: Make "reject" and "blocking reviewer" block acceptance in Differential
Restricted Differential Revision / rP929ad86b5707: Allow accepting accepted revisions, and rejecting rejected revisions
Restricted Differential Revision / rP8aa8ef49dae1: Provide an "Add blocking reviewer..." Herald action
Restricted Differential Revision / rPd518f3c9debc: When a user accepts a revision, accept for all projects the user has authority…
Restricted Differential Revision / rP3d3d3b6d8005: Move determination of reviewer authority into DifferentialRevisionQuery
Restricted Differential Revision / rPc80a4f51c161: Highlight reviews the viewer is responsible for in Differential
Restricted Differential Revision / rPb7297a3278c6: Add an "Author's projects" Herald field to Differential
Restricted Differential Revision / rP4c0ec01ce5b3: Allow Herald rules to add reviewers
Restricted Differential Revision / rP2d733f88a16a: Split users apart from projects/packages in reviewer and audit UIs
Restricted Differential Revision / rP9434df9d7cdd: Accommodate project reviewers in Differential search
Restricted Differential Revision / rPcf4eb3109e77: Allow projects to review revisions
Restricted Differential Revision / rP370c7635a788: Track "accepted" and "commented" in per-reviewer status
Restricted Differential Revision / rP4d8707df132d: Use status list UI to show reviewers in Differential
Restricted Differential Revision / rP65ddefad8b9c: Migrate all Differential reviewer data to edges
Restricted Differential Revision / rPbd8071bb3350: Add a `(dst, type, src)` key to Differential's edge table
Restricted Differential Revision / rP2e13bb0a5ec3: Migrate all Differential reviewer data to edges
Restricted Differential Revision / rPd6ce5a31e3a0: Use DifferentialRevisionQuery in conduit when reviewers are needed

Related Objects

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley edited this Maniphest Task.Oct 5 2013, 9:12 PM
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.Oct 6 2013, 12:42 PM
epriestley edited this Maniphest Task.Oct 6 2013, 12:54 PM
epriestley edited this Maniphest Task.Oct 6 2013, 1:09 PM
epriestley edited this Maniphest Task.Oct 6 2013, 1:38 PM
epriestley edited this Maniphest Task.Oct 6 2013, 2:33 PM
epriestley edited this Maniphest Task.Oct 6 2013, 5:42 PM
epriestley edited this Maniphest Task.Oct 6 2013, 8:06 PM
epriestley edited this Maniphest Task.Oct 6 2013, 9:44 PM
epriestley edited this Maniphest Task.Oct 7 2013, 12:08 AM
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.Oct 7 2013, 12:10 AM
epriestley edited this Maniphest Task.

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.
epriestley edited this Maniphest Task.Oct 7 2013, 6:00 PM
epriestley edited this Maniphest Task.Oct 7 2013, 6:18 PM
zeeg added a subscriber: charles.Oct 7 2013, 6:23 PM
epriestley edited this Maniphest Task.Oct 7 2013, 7:51 PM
epriestley edited this Maniphest Task.Oct 19 2013, 11:41 AM
epriestley edited this Maniphest Task.Oct 21 2013, 11:59 PM
chad edited this Maniphest Task.Nov 5 2013, 3:14 AM
chad edited this Maniphest Task.
frgtn added a subscriber: frgtn.Nov 8 2013, 8:12 PM

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".

epriestley edited this Maniphest Task.Feb 14 2014, 6:24 PM
chad added a subscriber: Firehed.Jun 18 2014, 10:51 PM

◀ Merged tasks: T5412.

turadg added a subscriber: turadg.Jul 21 2014, 11:53 PM
chad added subscribers: vm, seshness.Aug 28 2014, 5:49 PM

◀ Merged tasks: T5984.

charles removed a subscriber: charles.Apr 15 2015, 9:08 PM
eadler added a subscriber: eadler.Apr 30 2015, 4:25 AM

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.

chad added a comment.EditedJun 25 2015, 6:11 PM

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.

sharwell added a subscriber: sharwell.
chad changed the visibility from "All Users" to "Public (No Login Required)".Aug 26 2015, 1:29 AM
  • 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.

eadler added a project: Restricted Project.Feb 23 2016, 12:26 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
scode added a subscriber: scode.Feb 23 2016, 12:28 AM
joshma added a subscriber: joshma.Feb 25 2016, 12:17 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Mar 9 2016, 10:12 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 8 2016, 6:40 PM

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

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

scode moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 20 2016, 11:31 PM
epriestley closed this task as Resolved.May 22 2016, 1:43 PM

I'm going to close this as resolved, since I think we've substantially addressed all of the followups or there are now more narrow tasks with more focused problem descriptions available. In particular, from above:

Email: Project (and package) reviewers have been mailable for some time.

Policies: We have not implemented any special behavior here but also haven't seen complaints about it. T4411 is a more general followup task.

Design: I think all this stuff was addressed.

"Unblock" / "Sign-Off" / "Accept Some" / "Partial Accept" Action: It is now possible to, e.g., remove some blocking project reviewers by editing them via the web UI. T10939 discusses this in greater detail. We may still pursue this, but it's not clear how valuable it is given that the action is possible anyway.

Revision Updates: I think this logic got a little better on its own, and doesn't seem to be causing problems in practice. After T10967 I would likely store "blocking" as separate state to completely resolve this.

Dashboard Bucketing: This is better discussed in T9372. Work connected to T10939 implements improved bucketing. This still isn't perfect, but the problems with it are largely technical things which are unlikely to affect users.

Documentation: Planned for my memoir, "From Abra to Zubat: The One Correct Way to Develop Software".

Ad-Hoc Blocking Reviewers: Now supported with reviewer! from the CLI and blocking(reviewer) from the web UI.

Reviewer Predicates: See T10574 and T731.

Project Hashtags: These have evolved since introduction.

Removing Reviewers: Seems fine in its current state.


One feature this does not implement which some users may have been interested in is showing current reviewer status on the main dashboard page. Although the title is somewhat misleading in the context of the modern software, this task was never about that -- "reviewer list" referred to an element which was removed in 2013, and replaced with this one, as part of this task:

Prior to the introduction of this element, the UI just had a single line like this, with no per-reviewer status:

Reviewers: epriestley, joshuaspence

Via T10939, D15926 also proposed adding these hints to the ApplicationSearch results page:

However, we are declining to upstream this, at least for now. It feels pretty cluttered to us and it isn't really clear which use cases it resolves. Almost all feedback we've received asking for this feature is asking for it as a way to either work around expectations with bucketing or as a way to assist with "All Reviewers Must Accept" or other alternate acceptance conditions.

If remaining use cases exist, please upgrade to the new dashboards, use them for a bit to make sure they don't solve your problem, read T10574 to make sure that also doesn't describe your problem, then file a new issue outlining what problem this information would help you solve (see also Describing Root Problems).

"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 !