Page MenuHomePhabricator

Hint to blocking reviewers that they're blocking in earlier acceptance emails
Closed, WontfixPublic

Description

Several times now I've had an interaction like:

  1. Zach sends a review to me and Ben.
  2. I don't want to review the whole diff but I see something that I want to make sure gets fixed, so I reject the diff.
  3. Zach updates the diff to fix my complaint.
  4. Ben reviews the whole diff and accepts it.
  5. I see Ben's acceptance in my email and forget that I rejected it previously.
  6. Zach gets mad at me because he can't land his diff.

When Ben accepted the diff, I got an email that says:

benkomalo accepted this revision.
benkomalo added a comment.

LG on my end still

If I was paying better attention, I'd've noticed that it doesn't say "This revision is now accepted and ready to land." but that's an easy clue to miss. Ideally it could add another line that says something like

This revision is still awaiting acceptance from alpert.

or

You're an idiot and rejected this diff before; remember to accept it.

I'm particularly interested in the rejected-previous-diff case but I assume the same logic would apply to blocking reviewers.

Event Timeline

sophiebits raised the priority of this task from to Needs Triage.
sophiebits updated the task description. (Show Details)
sophiebits added a project: Differential.
sophiebits added a subscriber: sophiebits.

Can this be solved with Dashboards? People don't read emails, so I don't see this solving anything.

My Phabricator workflow is essentially completely via email and I believe many of my coworkers work the same way (and @epriestley as well, last I heard).

This dovetails somewhat with T731 (producing instructions describing next steps for a revision).

There's also been discussion of "soft reject" ("Request an Update") which would help with this specific problem, although I don't think that's filed anywhere.

A "rescind rejection" would also help me, which might or might not be simpler than soft rejection (but doesn't help the blocking reviewers case which I've also seen here).

chad triaged this task as Low priority.Aug 22 2014, 12:08 AM
eadler added a project: Restricted Project.Jul 7 2016, 5:00 PM
epriestley claimed this task.

See PHI54 for some additional discussion.

Although it's possible we might pursue this eventually, I'm going to close it for now since it's very old and I expect it to be resolved by T10448, which I expect to provide support for writing client-side mail rules that cover this case. Particularly, T10448 should let you write rules like "if I'm a blocking reviewer, add a red flag" (or similar) in your mail client, which probably moots this while solving a wide range of other problems.

If this is still an issue after T10448, feel free to follow up.