Page MenuHomePhabricator
Create Task
Maniphest T12235

Differential wrongly says I "Must Review" reviews rejected by group
Closed, WontfixPublic
Actions

Description

I am not entirely sure if this is a real bug or just behavior I do not understand.

What I see:

  • Differential dashboard panel shows reviews I have never rejected as Must Review

What I expect:

  • Only reviews that I have rejected or that I am a blocking reviewer of should be in my Must Review list

Reproduction Steps:

  1. User A creates a review DX and sets reviewers field to Project 1
  2. User B who is member of Project 1 sees review and rejects it
  3. User A updates review
  4. Other members of Project 1 now see review DX in their Must Review list even though they can't meaningfully do anything to move it out (Only user B can)

Version Information:

RepoCommitDate
phabricator638f2a012b2dbcc9c02b4bc4372d0f2459a8b2ef(Tue, Feb 7)
arcanistade25facfdf22aed1c1e20fed3e58e60c0be3c2b(Jan 5 2017)
phutil9d85dfab0f532d50c2343719e92d574a4827341b(Thu, Jan 12)

Related Objects

Event Timeline

jcarrillo7 created this task.Feb 9 2017, 2:28 AM
jcarrillo7 added a project: Differential.
Herald added a subscriber: eadler. · View Herald TranscriptFeb 9 2017, 2:28 AM
jcarrillo7 mentioned this in Z1336: General Chat.Feb 9 2017, 2:30 AM
epriestley added a subscriber: epriestley.Feb 9 2017, 3:37 AM

Revisions in the "Needs Revision" state (i.e., following the reject in step (2)) should never appear in "Must Review". I think the steps are missing a step "2a", where the author updates the revision with new changes?

though they can't meaningfully do anything to move it out

They can review it on behalf of Project 1, which will remove it from their queue.

Usually user B will be the one who accepts the changes on behalf of themselves and Project 1, but B may resign, or have a mixture of project and personal concerns, or whatever else.

You can't singlehandedly move the revision to "Accepted", but you can get it out of your queue by accepting for Project 1.

Does that explain the behavior?

See also T11050.

jcarrillo7 added a comment.Feb 9 2017, 4:19 AM

Yeah sorry I was missin your step 2a from above. I guess this behavior just feels weird since it's saying specifically that I must review. As a user I intuitively think of must review as something that I have personally blocked. For user B to see must review makes sense. But I don't think the same applies to every other member of the project. It's ok for the review to remain in my queue under needs review. Just not all of a sudden upgraded to must review.

jcarrillo7 added a comment.Feb 9 2017, 4:21 AM

T11050 seems to be describing a different issue. I don't mind having the review in my queue just not in the must review section.

jcarrillo7 updated the task description. (Show Details)Feb 9 2017, 4:23 AM
epriestley closed this task as Wontfix.Feb 10 2017, 3:22 PM
epriestley claimed this task.

T11050 describes a way you could work around this issue in some cases in the future, by resigning from the review (basically, saying "I'm not going to review this on behalf of Project 1 because user B is on top of it, so get it out of my queue"). That doesn't help if you want it in your queue but just not in the "Must Review" bucket.

Overall, this behavior is working as intended and I believe it's a generally reasonable behavior.

Dashboard bucketing algorithms are modular, and you can extend Phabricator with a new algorithm by subclassing DifferentialRevisionResultBucket if you'd prefer a different approach to bucketing.

jcarrillo7 added a comment.Mar 1 2017, 12:27 AM

@epriestley Since you added a new "Waiting on other reviewers" bucket (D17425) is it possible to have that bucket cover this case as well? I believe that bucket makes a lot more sense for this case as well then the current "Must Review".

epriestley added a comment.Mar 1 2017, 12:33 AM

D17425 doesn't change my thinking here.

jcarrillo7 added a comment.Mar 1 2017, 12:35 AM

ah poop worth a shot.

Phabricator · Built by Phacility