Page MenuHomePhabricator

Marking a diff as Requires Changes no longer removes it from Blocking Others
Closed, InvalidPublic

Description

In previous versions of Phabricator, marking a diff as Requires Changes would remove it from my Blocking Others section. This meant that I knew that the code author was now responsible for making changes and I didn't have to do anything at the moment. Now, the marking a diff Requires Changes leaves it in my Blocking Others section. This means it takes a bit more work to know if anyone is actually waiting on me.

Side note: accepting the change will remove it from Blocking Others.

Event Timeline

mjlyons created this task.Sep 1 2015, 6:46 PM
mjlyons updated the task description. (Show Details)
mjlyons added a project: Restricted Project.
mjlyons added subscribers: jhurwitz, angie, mjlyons.
chad added a subscriber: chad.Sep 2 2015, 12:23 AM

I think this is a duplicate, as I can't reproduce this as stated. I'm going to guess there are also other factors here (like blocking project reviewer?).

@chad - I'm not sure, but I think this is a different bug. T8080 is if the diff is accepted, which works just fine for me. This bug (T9311) is if the diff is requires changes

The only thing I can think of is that there are multiple reviews, and I'm the first to respond. There are no blocking reviewers. So maybe try this:

  1. Have someone send you a diff and add you and someone else as a reviewer
  2. Mark the diff as Requires Changes

I would expect this to take it out of the "Blocking Others" queue, but it doesn't. It did prior to a phabricator update this week.

This is very much still happening to me.

mjlyons reopened this task as Open.Sep 2 2015, 2:55 PM
chad added a comment.EditedSep 2 2015, 3:19 PM

@mjlyons I'm still not able to build a reproducible case here. Here's what I tested:

User A submits diff to User B and User C. User C sees new diff in "Blocking Others". User C requests changes. User C now sees that diff in "Waiting on Others". Is this the correct thing for me to test or am I misunderstanding some steps?

chad added a subscriber: epriestley.EditedSep 2 2015, 3:20 PM

@epriestley could anything custom affecting these buckets they may be using that isn't in vanilla Phabricator?

We haven't made any changes to this in the upstream recently that I can recall or find in git log, so if this behavior really has changed my best guess is that there's a local patch affecting it. The last upstream change which was expected to affect this was rPbcb957b (April 27).

This behavior isn't always completely intuitive (see discussion as far back as 2013: T1279#42118), so I would generally make the same assumptions you did (e.g., there's a project as a reviewer which isn't clear).

This behavior isn't modular or extensible, so it could only be adjusted with a local patch, almost certainly to DifferentialRevisionSearchEngine.

@jhurwitz, do you have such a local patch?

In particular, T9263 may have motivated development of a local patch.

See T9181 for a previous report of an issue caused by a local patch.

We haven't yet applied a local patch for T9263. None of our patches seem to be relevant here.

For what it's worth, I cannot repro this problem on our install. @mjlyons, maybe the repro conditions are more subtle than what's described here?

Ok I'll keep an eye out for next time.

epriestley closed this task as Invalid.Sep 9 2015, 11:13 PM
epriestley claimed this task.

Presuming this is just related to bucketing errors since we're aware of bucketing assignment occasionally being surprising, see T9372 for some discussion.

(If you're able to come up with specific repro steps, feel free to reopen this.)

angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Sep 14 2015, 8:41 PM