Page MenuHomePhabricator
Create Task
Maniphest T4168

bug with removing reviewers from accepted diffs
Closed, ResolvedPublic
Actions

Description

Another buggy workflow:

  1. Brent submits diff -> diff status is "Needs Review"
  2. Evan approves diff -> diff status changes to "Approved"
  3. Brent removes Evan as reviewer -> diff status is still "Approved".

Seems like step 3 should change the status back to "Needs Review", as a diff should only be considered accepted if one of the reviewers' status is accepted, and Evan is no longer a reviewer.

Event Timeline

brent created this task.Nov 27 2013, 10:50 AM
brent assigned this task to epriestley.
brent raised the priority of this task from to Needs Triage.
brent updated the task description. (Show Details)
brent added a project: Differential.
brent added a subscriber: brent.
epriestley triaged this task as Low priority.Nov 27 2013, 3:21 PM

I think we should probably change this behavior to work as you expect (it now works this way when removing rejecting reviewers) although one consequence is that "Accept + Abandon Later" (i.e., "Just pushing this out of my queue, it's fine whenever you want to land it") will no longer leave the revision accepted. I think this is rare and probably not useful to retain.

Since this is pretty low impact, it probably won't be fixed until after T2222 / the next round of changes on T1279.

epriestley closed this task as Resolved.Mar 14 2014, 4:06 AM

I believe this should be resolved in HEAD.

Phabricator · Built by Phacility