Page MenuHomePhabricator

Retracting requested changes can leave revision in "Needs Revision" state
Closed, ResolvedPublic

Description

Steps to reproduce:

  • (user A) create a code review with one individual reviewer (user B) and one blocking group reviewer
  • (user B) request changes
  • (user B) accept as individual

Expected result: Revision should now be in the "Needs Review" state, since it's now only pending review from the group reviewer.

Actual result: Revision remains in the "Needs Revision" state.

Screenshot 2017-07-13 11.55.29.png (2×2 px, 386 KB)

An easy workaround exists in the form of the "Request Review" action.

Event Timeline

From reading the code, resigning (instead of accepting) probably also leaves the revision in "Needs Revision".

That sounds less surprising to me, in the sense that it's not an explicit reversal of having requested changes. I can imagine arguments in both directions for the resign case.

I think we end up more consistent overall if "Resign" and "Accept" have the same behavior in this case.

Elsewhere, for example, if you Accept (instead of Reject), then Resign, we null-and-void your Accept. That one seems more clear (when you resign, your accept doesn't count anymore) and I think it makes sense to have Reject use the same rule (if you resign, your reject doesn't count anymore either).