Page MenuHomePhabricator

Requesting changes as Reviewer and member of blocking Group Reviewer, makes Group Reviewer no longer blocking
Closed, ResolvedPublic

Assigned To
Authored By
claassen
Apr 6 2017, 7:50 AM
Referenced Files
F4697786: Schermafbeelding 2017-04-07 om 09.13.57.png
Apr 7 2017, 7:23 AM
F4697837: Schermafbeelding 2017-04-07 om 09.14.29.png
Apr 7 2017, 7:23 AM
F4697825: Schermafbeelding 2017-04-07 om 09.14.18.png
Apr 7 2017, 7:23 AM
F4697781: Schermafbeelding 2017-04-07 om 09.13.40.png
Apr 7 2017, 7:23 AM
F4655507: Screen Shot 2017-04-06 at 5.05.19 AM.png
Apr 6 2017, 12:14 PM
F4655644: Screen Shot 2017-04-06 at 5.12.40 AM.png
Apr 6 2017, 12:14 PM
F4655540: Screen Shot 2017-04-06 at 5.07.20 AM.png
Apr 6 2017, 12:14 PM
F4655494: Screen Shot 2017-04-06 at 5.04.42 AM.png
Apr 6 2017, 12:14 PM

Description

A colleague of mine created a new Differential Revision today. I was added as the reviewer. Herald added a project as Blocking Reviewer of which I'm a member. When I requested changes to the revision, the project was no longer a Blocking Reviewer but it had become just a normal reviewer.

So far, this isn't really a big deal, because the Revision is still in the state of Changes Requested and nothing bad can happen.

But we also have a workflow, where our interns first review each other. The same project is added as a Blocking Reviewer to those reviews and when the interns have accepted the revision they add one of the project members as a reviewer to do a final check. When the project member now requests changes to the revision, the project becomes a normal Reviewer instead of a Blocking Reviewer as well. But because the intern is still listed as an accepting reviewer, the revision goes to the accepted state, and the author can land the revision without acceptance from any member of the project that was supposed to be a Blocking Reviewer.

Version information of our instance:
phabricator 8f7983a5be3a56db5b79dc7c3a0eb470f1d7ca02 (Sat, Mar 25) (branched from b4effdf26c3e7d5de0d010cf14626c5d8d404e04 on origin)
arcanist 60aaee0ed3f5a1e4384ac7d7f2efd2c64cecbe44 (Sat, Mar 25) (branched from d1db9a72b552151613a918e3d49fa72433387a68 on origin)
phutil b133c277014868d476f08b4ebecde2ea795509e4 (Sat, Mar 25) (branched from c0bc116bedc895fd617799a13549f8707edfd3fb on origin)

Event Timeline

I'm having trouble reproducing this, but maybe I'm not fully understanding what you're running into?

The expectation is that the project becomes a normal reviewer but with status "reject", and that this prevents the revision from becoming "Accepted" until someone accepts on behalf of the reviewer. That is, "Rejected" is a stronger status than "Blocking", so nothing is lost by replacing "Blocking" with "Rejected".

Here's what I did:

First, I added a user (@dog) and a project (Dog Project II) as blocking reviewers:

Screen Shot 2017-04-06 at 5.04.42 AM.png (276×448 px, 24 KB)

Then, I requested changes to the revision as @dog. The project reviewer has moved to the "rejected"/"changes requested" state:

Screen Shot 2017-04-06 at 5.05.19 AM.png (223×344 px, 20 KB)

Next, I accepted the revision as @ducksey:

Screen Shot 2017-04-06 at 5.07.20 AM.png (398×448 px, 37 KB)

Note that the revision is still in the "Needs Revision" state, and that the project reviewer is still in the "rejected"/"changes requested" state.

I also tried this:

  • Add an "intern" (@ducksey) and a blocking project reviewer (Dog Project II).
  • Accept as @ducksey.
  • Request changes as @dog, who has authority over the project.

This produced a similar state:

Screen Shot 2017-04-06 at 5.12.40 AM.png (391×397 px, 42 KB)

But because the intern is still listed as an accepting reviewer

This isn't how the rules are supposed to work: if any reviewer is in the "rejected" state, the revision should never transition to "Accepted". In my test cases above, this appears to work correctly.

Can you give me a more specific series of steps which allows me to reproduce a problematic state?

I've repeated this again, taking some screen dumps. This time I did it on a Phacility hosted instance, instead of on our self-hosted instance.

First, I've created a new revision with a blocking group reviewer "Test Project" and a normal reviewer "dennis", which is a member of the project.

Schermafbeelding 2017-04-07 om 09.13.40.png (282×544 px, 27 KB)

On the "Edit Revision" page it looks like this:
Schermafbeelding 2017-04-07 om 09.13.57.png (42×416 px, 9 KB)

I didn't actually edit anything, but it shows the test project clearly as blocking.

Next, I've requested changes as the reviewer "dennis". Of course, this marks the group as Requested Changes as well:

Schermafbeelding 2017-04-07 om 09.14.18.png (251×417 px, 26 KB)

But, going to the Edit Revision page again, now shows the Group Reviewer is no longer a blocking reviewer. See this dump:

Schermafbeelding 2017-04-07 om 09.14.29.png (40×359 px, 8 KB)

Version info of the Phacility instance:
phabricator 061375fde6525ea3e6eb701ddc570c5a422d7ffc (Wed, Apr 5) (branched from 163e1ec4426eb40e15ad46f831a731f13b275918 on origin)
arcanist 60aaee0ed3f5a1e4384ac7d7f2efd2c64cecbe44 (Sat, Mar 25) (branched from d1db9a72b552151613a918e3d49fa72433387a68 on origin)
phutil f568eb7b9542259cd3c0dcb3405cc9a83c90a2f5 (Mon, Apr 3) (branched from c581e769f10c6d2b427900897edba74e01a572bd on origin)
libcore 3eebdfca5b325792fdd2003a261e1ab94b919322 (Fri, Mar 24)
services 198eb67bd82296b6938d038836c7269c84bad98f (Feb 13 2017) (branched from 772620edd80ce593b104dba7721db42b9eb020a2 on origin)

epriestley claimed this task.

That's expected, and not a bug. "Blocking" is currently a top-level status. When a reviewer becomes "Rejecting", they are no longer "Blocking". This doesn't matter in terms of the workflow, because both "Rejecting" and "Blocking" prevent a revision from transitioning to "Accepted".

...the author can land the revision without acceptance from any member of the project that was supposed to be a Blocking Reviewer.

Particularly, I believe this isn't true, and your workflow doesn't demonstrate it occurring. When the reviewer stops being "blocking", they become "rejecting", which stops the revision from changing to "Accepted".