Page MenuHomePhabricator

Differential still appears in "responsible users" query after resigning as a reviewer
Closed, ResolvedPublic

Description

I am a member of an Owners package which adds me as a reviewer to Differential revisions that touch certain files. Some of these revisions I do not intend to review myself and instead, I leave it to somewhere else on my team. For these revisions, I have been utilizing the newly added "resign as reviewer". It seems, however, that when I resign from a revision it still appears in the custom search that I am using. My custom search uses the following criteria:

Response UsersCurrent Viewer
RepositoriesA specific repository
StatusOpen
Order ByDate Updated (Latest First)
BucketNo Bucketing

Is this expected? If yes, is there any way I can modify my custom search to exclude revisions from which I have resigned?

Event Timeline

chad added a subscriber: chad.Apr 11 2017, 5:56 AM

Please put your version information on all bug reports. There has been a lot of churn here so we need to know you're at HEAD before trying to reproduce.

chad renamed this task from Differential still appears in "responsible users" query after resigning as a reviewier to Differential still appears in "responsible users" query after resigning as a reviewer.
chad added a comment.Apr 11 2017, 6:56 AM

๐Ÿ‘๐Ÿป ๐Ÿ˜„

chad added a comment.Apr 11 2017, 6:59 AM

Where does it go when you turn on bucketing?

When I turn on bucketing, I can't see it actually. Although I do have a lot of diffs, so it could have overheated.

This is currently expected, although we could consider changing it.

For context, the value of the "Responsible Users" field is converted into a concrete list of PHIDs very early on. So these two fields are indistinguishable to the query or display layers:

Responsible Users: Current Viewer
Responsible Users: epriestley, <list of projects and packages>

That step happens very early, and everything else just gets a list of PHIDs with no magic. I think this is generally a good thing and would like to avoid changing it, but it means that any rule we use here can't depend on whether you typed functions or specific users/packages/projects into the typeahead.

The query layer currently just pretends that "resigned" reviewers do not exist. This is identical to the old logic, where "resigned" reviewers were actually deleted and really did not exist.

The display layer works differently if bucketing is on or off. If bucketing is off, all the results are shown -- including results which one or more of the "Responsible Users" have resigned from.

If bucketing is on, results which one or more "Responsible Users" have resigned from are excluded.


If yes, is there any way I can modify my custom search to exclude revisions from which I have resigned?

Sort of: you can enable bucketing.


If we changed the behavior to always filter these results, "Responsible Users: alice, bob" would exclude results which either has resigned from, which seems incorrect? Although "Responsible Users: alice, bob ; Bucketing on" already does this. But conceptually I think "bucketing" sort of only makes real sense with a single user anyway? We try to do our best if you provide multiple users but I think we probably end up with ambiguous results in many cases, since revisions can be in different states for the different users.

The actual use cases I'm most aware of here are:

  • "Bucketing on, responsible users: current viewer": Show you your own stuff. Correct to exclude "resigned".
  • "Bucketing off, Responsible Users: A, B, C": Show your team's stuff. Bad to exclude "resigned". Although maybe this isn't common because we don't support members(...) in this typeahead right now and I don't think we've received requests for it.

We could limit surprise in the "bucketed, multiple users" case by adding a "someone has resigned" bucket at the bottom or something if the query includes two or more user PHIDs.

We could also filter resigned users if all user PHIDs (but not all non-user PHIDs, since they normally can't resign) in the query had resigned. Maybe that's the simplest rule? However, this is a significantly more complicated rule from the perspective of actually building the query if we want to try to do the filtering at query time. It also precludes us from building a "resigned" bucket, which I wasn't sure if we'd want to build or not.

pasik added a subscriber: pasik.May 12 2018, 1:47 PM
epriestley closed this task as Resolved.Aug 10 2018, 8:00 PM
epriestley claimed this task.

I believe this was fixed by D17558 and discussed in T11050.