Page MenuHomePhabricator

Request Review action should clear "accepted older" status
Closed, ResolvedPublic

Description

Motivation:

  1. coder A puts up a revision with reviewers R1 and R2
  2. R1 accepts the diff
  3. R2 comments with something profound, causing A to doubt the very core of their being (and their diff)
  4. A updates the revision with a new approach, and requests re-review from both reviewers
  5. R1 doesn't notice that the diff requires their attention, because it's way down in the Other Revisions list at the bottom of Differential, instead of in the Ready to review list.

It seems to me that the 'request review' action should bring the revision to the attention of all reviewers, regardless of whether they've accepted an older diff.

Event Timeline

avivey renamed this task from [Differential] "Ready to review" list doesn't include NEEDS_REVIEW revisions with reviewer status ACCEPTED_OLDER to Request Review action should clear "accepted older" status.Aug 24 2016, 5:00 PM
avivey added a project: Differential.
eadler added a project: Restricted Project.Sep 15 2016, 6:03 PM
epriestley claimed this task.
epriestley added a subscriber: epriestley.

Oh, I think the edit to the title here is actually misleading, and the author in this scenario never used "Request Review" (this action is only available if the author has previously used "Plan Changes", which they did not appear to in this scenario).

This behavior described is configurable with differential.sticky-accept. See Differential User Guide: FAQ for discussion and rationale.

In this particular case, I believe profound changes in step (4) are very rare, and not common enough to justify adding a "Undo Everyone's Review And Require Them To All Review it Again" action, separate from differential.sticky-accept. Even with that option enabled, the author can already: explicitly ping reviewers asking them to re-review; or remove them, then add them again; or abandon the revision and create a new one ("See previous discussion in Dxxx, but this is a completely different approach based on feedback there.") if they want to be certain the change gets a second look.

Other future changes (like T2543) are likely to add other actions, and we already have a lot of actions today. I want to resist adding additional actions here if they would be rarely used and reasonable workarounds already exist.

After EditEngine (T11114), third-party extensions will also be able to provide supplemental actions.

I want to resist adding additional actions here if they would be rarely used and reasonable workarounds already exist. After EditEngine (T11114), third-party extensions will also be able to provide supplemental actions.

A typical scenario we've been running into is that when a revision is "waiting for review", reviewer will comment to ask for clarification but not requesting code changes, and the revision remains in the "waiting for review" where it will get forgotten for a while. We've been recently discussing some things along the lines of being able to request for comment on a revision that would put the revision back into the author's "todo" bucket. Would that be possible with T11114 and third-party extensions? For now we're looking to resolve this problem with memes and occasionally "Request Changes" when feeling aggressive.

Yes and no. So: maybe?

You can copy/paste DifferentialRevisionRejectTransaction and change all of the strings from "Request Changes" to "Gently Please Request Clarification Thank You Very Much (You Are a Lovely Person BTW)". This will create an action which is functionally identical to "Request Changes" but has different icons, action text, error text, email subject text, and transaction timeline text. Perhaps that is sufficient. (It will still put the revision in a "Changes Requested" state, and the reviewer in a "Requested Changes" state, and you will not be able to control those strings.)

You can also copy/paste DifferentialRevisionRequiredActionResultBucket and create a new bucketing algorithm. You could have that algorithm look at the comment history for each revision to try to re-bucket things, possibly, although that might be somewhat involved and/or slow. So instead of introducing a new action, you'd try to add a rule like "if you authored a revision and it's waiting for review, but someone else is the most recent commenter, move it to a different bucket like 'has unreplied-to-comments', as though that comment was really a very gentle reject".

(You can now write a "Undo Everyone's Review And Make Them Review This Again" action, too, although it probably isn't trivial, and if you did so today you'd probably need to adjust it after T10967.)

You can't add new reviewer states (like "Waiting for Feedback", as distinct from "Requested Changes") or new revision states. This may become possible after T2543, although I'm not sure how overboard I'm going to go on modularizing things. (T10574 may also intersect here to some small degree.) Note also that T10967 is likely to disrupt this at least somewhat, even if it becomes possible before then.

At one point, I was more formally considering making comment act like a "soft reject", and you can probably find some old discussion of this if you dig around. Interest in this seems to have waned, and the last round of bucketing updates to Differential mostly seems to have quieted the discontent here, so I'm less keen on pursuing changes in that vein now. One broad consideration is that the author is blocking themselves on these revisions by not responding to them / missing the comments / not following up, so they aren't creating a situation where incentives are misaligned. If you miss a comment and then forget about a revision you authored for a while, it probably wasn't that critical?

I appreciate the details and pointers - we may look at a custom reject transaction and/or required action bucket. I just went digging here for related discussion to see if others may have run across similar scenarios.

If you miss a comment and then forget about a revision you authored for a while, it probably wasn't that critical?

I can investigate further what issues people are having with the current solutions - I'm not sure being critical is the concern but more-so there being a delay for the change to land/verify without it being clear who should be the next step (presumably from the dashboard/bucket?).