Page MenuHomePhabricator

Allow resigning from a review you are not directly responsible for
Closed, ResolvedPublic

Description

On our team we have groups created (i.e. one for designers, developers, etc.) and then we use those groups to add subscribers and reviewers.
When I wish to remove myself as a reviewer, I have to remove the group itself, then re-add each member except for myself.

One potential solution to this problem would be to make it so that clicking the group "expands" it so that all the members are shown individually in the field, similar to what is possible in Outlook. Then it would be easy to remove the individuals without fear of forgetting to include someone.

Event Timeline

chad added a subscriber: chad.May 27 2016, 6:54 PM

This feels bad in general that you would need such a feature. For example, if I'm not qualified to review something, I just don't and it has no effect to my daily flow. Can you go into more detail about why your getting so many review requests you shouldn't be getting and how your using Dashboards perhaps to keep track of stuff that you actually should be reviewing?

One reason it pops up is that we put all develops as reviewers and then when one person actually decides to claim it for review they remove everyone else. We put all developers as subscribers, though, so that everyone stays notified when things change. I often want to remove myself as a subscriber so that I don't get emails about revisions I don't care about.

Outside of that specific case I've run into a few occasions where I wanted to add an entire group except for one or two members. Right now I can't find a convenient way to do that.

chad added a comment.May 27 2016, 7:03 PM

I want to say Pinterest had a similar workflow and built a custom herald rule to manage it, not sure if that sounds interesting to you: https://engineering.pinterest.com/blog/shipping-faster-new-pr-processing-system

Neat thanks! That does look like it would help with our problem but I still think that expanding a group would be a useful feature.

Basically any time you could use Projects + Owners + Herald to be more tactical and less shotgun-like in assigning reviews is likely much more beneficial to correct coverage and lessening workload.

I'll leave it to @epriestley to decide the right path for this task, but seems OK? to let anyone resign from a review.\

Sounds good to me. Thanks!

I think it may be reasonable to allow users to resign from reviews they are only indirect reviewers for, but wouldn't pursue this until after T10967.

It might also be reasonable to allow users to forcefully unsubscribe from objects they are indirect subscribers to. I believe "subscribe + unsubscribe" may actually accomplish this today (the "subscribe" adds you as an explicit subscriber, and then "unsubscribe" changes your relationship to "explicitly unsubscribed").

There are a number of other mechanisms available which may be better fits for your problem than adding a large group as a subscriber to every revision:

  • Use feed.
  • Use custom Differential queries.
  • Use dashboards.
  • Use "only the first time ... send an email" instead of "every time ... add a subscriber" emails to alert members once when a change is created.
  • Use Herald rules to add individuals.
  • Add an "engineering-review-notify" project which users are free to leave if they don't want review-notify mail.
  • Just let individuals write their own Herald rules to customize what mail they get.

I don't currently plan to let you unbundle reviewer or subscriber groups. This would need to handle projects unbundling into thousands of members, which seems difficult and unmanageable.

We could theoretically support a Reviewers: x, -y, -z syntax to mean "members of X, except y and z" but this use case seems a little bizarre to me. Why do you want to add everyone except a few specific people?

Broadly, I think these systems tend to work better if interested users are actively watching revisions (e.g., through personal Herald rules) than if authors are blasting everything to huge lists. In my experience, adding 20 reviewers to everything leads to a diffusion of responsibility where everyone assumes that one of the other 19 reviewers will look at the change, so no one does. We've built these shotgun features mostly because users keep asking for them: at Facebook, I removed some of these features and we seemed better off for it.

chad renamed this task from It's difficult to remove yourself from a field when you were added as part of a group to Allow resigning from a review you are not directly responsible for.May 30 2016, 7:23 PM
chad added a project: Differential.
eadler added a project: Restricted Project.Jul 7 2016, 5:17 PM

This task has meandered a bit, but the current behavior is this:

  • Users may only "Resign" from a revision if they are on the "Reviewers" list as an individual user.
  • When you resign, your reviewer relationship is just deleted.
  • So you can get stuck with revisions you don't plan to review on your dashboard because you are a member of a project or package reviewer, since there's no way to say "even though I'm a member of the 'Language Police' project, this change is in Cambodian and I only speak Ancient Irish so I can't possibly review it".

The planned behavior is instead:

  • Users may "Resign" from a revision if they have authority over any package or project which is a reviewer, not just if they are an individual reviewer.
  • When users resign, we will store a "this user resigned" relationship (after T10967) explicitly, so users who are not reviewers and users who have resigned from a review are stored differently.
  • Query constraints and the UI will update appropriately (maybe resigned stuff stays on your dashboard but drops down to a new "Stuff you resigned from" bucket -- or maybe it just vanishes completely -- and you probably are no longer a reviewer for the purposes of the "Reviewers" constraint).

Worth mentioning is that you can query for exact(username) to get a dashboard for only individual reviews, but this is a partial solution; there's no way to get a dashboard for "all my stuff, except stuff I've explicitly said I don't care about".

This sounds like a great improvement for the use cases that I am interested in! Thanks!