Page MenuHomePhabricator

Option to CC or use a mailing list for project reviewers
Open, Needs TriagePublic

Description

On my team, Differential revisions often have specific reviewers as well as project reviewers. My project tends to get a lot of review requests, so I'd really like to apply a server-side mail filter that only matches messages about revisions where I am specifically listed as a reviewer. For example, I might choose to keep emails about reviews for me in my inbox and move the rest to a "Reviews" folder.

It looks like Phabricator has great support for this via the X-Phabricator-To header. Unfortunately, my server-side mail rule infrastructure doesn't allow matching on custom headers, so I'm not able to use it to set up this kind of filter.

My team's current code review system is configured to send email with the explicitly-specified users in the "To" field and the review project users in the "Cc" field. Because the server-side rules do allow matching on these headers, this allows me to set up a rule that does what I want.

It would be super helpful if Phabricator offered a way to do something similar, either by Cc'ing project reviewers or, alternatively, by providing an option to send email to a mailing list address associated with a project instead of addressing each individual member of the project.

Event Timeline

edibiase updated the task description. (Show Details)
edibiase added a project: Differential.
edibiase added a subscriber: edibiase.

I think this is T5791, that is you can write a Herald rule to say ignore when you're a project reviewer but send email when you're a direct reviewer.

Thanks for the quick reply, @chad! If I understand it correctly, it sounds like you're right—T5791 is a superset of this request. Being able to write a Herald rule that drops outbound mail when I'm a project reviewer but allows it when I'm a direct reviewer would pretty much cover my use case.

The only downside to that approach is that some folks on my team want to receive email when they are a project reviewer or a direct reviewer, but filter them into different folders. Maybe T5791 would be powerful enough to handle that case, though? Based on my limited understanding, it seems like maybe it would allow a Herald rule like "If I'm a project reviewer, allow the message but move me from To to Cc", which would allow us to write server-side rules that did the right filtering.

Also, it seems like I might already be able to configure Phabricator to only send me email when I'm a direct reviewer, by turning off my email notifications for Differential and then adding a Herald rule that sends me an email if I'm in the list of reviewers. Do you think that might suffice for my use case?

Hrm; I added a Herald rule that would email me if I'm in the list of reviewers, and changed all of the Differential Revision notification preferences to "Notify", but I didn't get an email when I changed a revision where I was a direct reviewer. (I verified that the Herald rule matched the revision in question using the test console.)

It's possible I did something wrong, of course, or maybe the fact that this doesn't work is by design and was part of the motivation for T5791 :-)

You can look at /mail/ and get more information about mail delivery.

Thanks! It looks like the message delivery was voided because of my preference not to receive mail from Differential:

This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences).

providing an option to send email to a mailing list address associated with a project

I think you can create a "Mailing List" user and add them to the project to accomplish this. This is in addition to other mail delivery.

I think you can create a "Mailing List" user and add them to the project to accomplish this. This is in addition to other mail delivery.

Thanks; that's pretty cool! In this case, I don't think it helps with my ability to filter, though, since it's in addition to the individual project reviewers, not instead of.

Okay, so:

Ideally, just use a mail client with filters that are powerful enough to do full header matching.

If your client can do regexp-based body text matching, but not header matching, you might be able to match the "To:" / "Cc:" footers in the mail body. These show the original aggregate recipients.

T5791 is essentially us implementing sufficiently powerful Phabricator-side server-side mail filtering for users who use Gmail or other client pipelines that can't do this, or possibly users whose client pipelines are powerful enough but who aren't sufficiently technical or motivated to configure the rules. This will be sufficient once it's more complete, but there are a lot of social and cultural issues surrounding this (see discussion in T9161; see also T4103) that I'd like a clearer path forward on before dedicating to Herald outbound rules.

In particular, I worry that Herald outbound rules are probably a very bad solution to many of the things they'd be used for today if we waved a magical wand and finished their implementation, and that moving forward here may burden us with a lot of support costs down the road arising from dubious, off-label use cases where, say, an administrator uses a global rule to affect mail delivery for users in an unclear, undesirable way. As I've moved forward with the implementation of T5791, I think it is increasingly likely that we do not want to implement global outbound rules at all, and that we need to dramatically increase the observability of mail delivery before we can reasonably recommend its use.

The number of things that I think these rules are a good solution to is pretty small (this use case isn't unreasonable, and the original sorting-based-on-repository use case in T5791 is also not unreasonable) and finishing T5791 is, to some degree, "Implement Gmail in Phabricator", so the product and technical pathways both give me a bad feeling even though I think the path forward is relatively clear.

It would almost certainly be less work, in theory, for Gmail and other client pipelines to implement header filtering than for us to implement outbound mail rules. It's fairly ridiculous that we're implementing them at all because this problem is so well suited to resolution in clients and was a standard feature of clients for ~20 years. I don't know what your pipeline is, but if it's open source or you have any control over it, getting header-based rules into it might conceivably be faster than waiting for us to finish implementing what is essentially a sizable fraction of a web mail client. Certainly, I would probably spend fewer engineering hours adding arbitrary header filtering to Gmail than I will spend implementing Gmail in Phabricator in the completion of T5791.

Existential angst about T5791 aside, I think the closest match to your request, per se, is:

  • Add a mailing list user to the project, so the mailing list gets mail.
  • Have all users who don't want to get review mail unsubscribe from the project. You will not receive mail from projects you are unsubscribed from.
  • If they do not want project review mail, but do want other types of project mail, there's no great solution. Maybe create separate "Team-Reviews" and a "Team" projects and put everyone in both? This is cumbersome, but perhaps more palatable if "Team-Reviews" is being added by something like Herald.

You could also just use a mailing list user instead of a project, but this will mean the revisions are not shown to users by default on their standard dashboard since nothing connects them to the reviewer. They could create a separate query and promise to check it regularly, but realistically no one will.

I think you're right; the best way forward for us to is to get more powerful filtering rules on the server side. In the meantime, your suggestion for using a mailing list or separate teams seems like a reasonable workaround. Thanks!

Technically, we could implement "To", "Cc", "Any Recipient", and maybe "Sending application" or "Object type" outbound Herald fields today so you could write some variant of this:

When:
[ Sending application ] [ is any of ] [ Differential ]
[ Any recipient ] [ does not include ] [ epriestley ]
Take these actions:
[ Deliver as notification ]

The "Any recipient does not include ..." line would cause it to match mail where you were a recipient only through membership in a project, since "Any recipient' would evaluate to the top-level, aggregate recipients (eventually, there might be an "Any resolved recipient" rule or similar, too).

An additional field check like:

[ Any recipient ] [ includes any of ][ #x, #y, #z ]

...would let you select which projects you didn't want mail for (or an inverse rule would let you select projects you do want mail for).

This doesn't necessarily have to wait for the whole dependency chain to unravel before we can pursue it, since it doesn't really invoke any of the use-case peril I'm worried about with global outbound rules. However, it does make outbound rules useful enough to actually use, without really providing enough tooling to help users understand them, so it still potentially creates a bit of a support issue.

I'm generally just a little hesitant to push ahead further here before unwinding the T9161 -> T4103 stack of onboarding / "too much mail" stuff first, but that's unfortunately a deep, complex stack with as many cultural/social product design questions as technical concerns.

eadler added a project: Restricted Project.Apr 7 2016, 6:13 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 7 2016, 6:14 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:00 PM