Page MenuHomePhabricator

Comment emails generated from parsing incoming email replies should not go to people already on the To/CC of the incoming email
Closed, ResolvedPublic

Event Timeline

klimek raised the priority of this task from to Needs Triage.
klimek updated the task description. (Show Details)
klimek added a subscriber: klimek.

I've read this a few times, maybe I am dense, can you explain in a little more detail the issue?

We have a setup where:

  • all phab reviews also go to a mailing list as well as to the specified reviewers (if any)
  • people reply on the mailing list to phab review emails via reply-all
  • we do not want phab to be in the critical path for reviews, thus a reply-on-to-phab setup is not an option

If phab parses the reply mail, it generates a "comment" action, and then sends email for that comment action, which goes to exactly the same set of people that already got the original reply. Sending that mail again is redundant.

I hope that explains it somewhat better - let me know if I'm missing something :)

nikivinvinod claimed this task.
nikivinvinod added a subscriber: nikivinvinod.
chad removed nikivinvinod as the assignee of this task.
chad removed a subscriber: nikivinvinod.

We have the exact same issue. it makes Phabricator "feel" spammy.

I think that if Phabricator would cull out users that were on the email it received (their addresses were in the TO/CC), then it would significantly improve this situation.
Don't know if this is hard to do in the current design, though.

@klimek - why not just 'little r' and then let Phabricator send out all the pertinent emails? if people just little r to emails Phabricator sends out your scenario works perfectly. In fact, I think things would actually work better because user settings about Phabricator-related emails would definitely be respected (as opposed to usurped by people big r replying in the email, peppering your inbox directly with stuff you may not care about.)

We implemented this when I worked at Facebook for the internal tools there, but reverted it in favor of folks little r replying / learning how the tool works and not spamming everybody. The "automagic" version caused confusion in the following ways:

  • users have preferences about when internal tools email them. users who have selected not to be emailed for certain tools / actions become confused; users trying to reach certain users via email become confused when the users they want to reach do not receive it.
  • users have multiple email addresses some of which may not be registered with the tools. any such unregistered email can not receive this automagical treatment.
  • users may 'little r' incorrectly once other people are throwing in some 'big r' into the mix. e.g. i big r to something, and you little r to the copy from me and not the copy from Phabricator, meaning the communication never makes it to everyone.

...additionally, supporting big r replies as described basically overrides user preferences. You may think you want this, but you can still always FWD something to get into someone's inbox.

Big ideas here conceptually are your email should be as clean as possible (i.e. communication within tools that don't hit your email is a good thing), tools should work as simply as possible (i.e. automagic swallowing of emails should be hesitantly added at best), and the burden of not spamming one another can safely be placed on one another within an organization.

The problem is that many people only use the mailing list and don't do
reviews via phab themselves (but still want to be part of other people's
review threads). I think having different flows for phab vs non phab is not
going to fly for us.
btrahan added a subscriber: btrahan.
btrahan added a comment.

@klimek - why not just 'little r' and then let Phabricator send out all the
pertinent emails? if people just little r to emails Phabricator sends out
your scenario works perfectly. In fact, I think things would actually work
better because user settings about Phabricator-related emails would
definitely be respected (as opposed to usurped by people big r replying in
the email, peppering your inbox directly with stuff you may not care about.)

We implemented this when I worked at Facebook for the internal tools there,
but reverted it in favor of folks little r replying / learning how the tool
works and not spamming everybody. The "automagic" version caused confusion
in the following ways:

  • users have preferences about when internal tools email them. users who

have selected not to be emailed for certain tools / actions become
confused; users trying to reach certain users via email become confused
when the users they want to reach do not receive it.

  • users have multiple email addresses some of which may not be registered

with the tools. any such unregistered email can not receive this
automagical treatment.

  • users may 'little r' incorrectly once other people are throwing in some

'big r' into the mix. e.g. i big r to something, and you little r to the
copy from me and not the copy from Phabricator, meaning the communication
never makes it to everyone.

...additionally, supporting big r replies as described basically overrides
user preferences. You may think you want this, but you can still always FWD
something to get into someone's inbox.

Big ideas here conceptually are your email should be as clean as possible
(i.e. communication within tools that don't hit your email is a good
thing), tools should work as simply as possible (i.e. automagic swallowing
of emails should be hesitantly added at best), and the burden of not
spamming one another can safely be placed on one another within an
organization.

TASK DETAIL

https://secure.phabricator.com/T5185

REPLY HANDLER ACTIONS

Reply to comment or attach files, or !close, !claim, !unsubscribe or

!assign <username>.

To: btrahan
Cc: klimek, chad, talshiri, btrahan

So it turns out this feature set was actually built into Phabricator too. I still stand by what I said before, but since its here already I'm not going to rip it out or anything, and in fact intend to fix it.

It looked to me to be broken for Differential only. The patch I just committed probably fixes it.

Could folks update to HEAD, see how things go and let me know? I apologize for not testing this change yet but its excessively difficult in my dev environment.

I just wanted to try it - any reason why this was reverted again?

Ah, sorry, just read the commit comment...

Another reason is that the email parsing doesn't work for inline replies, and we basically always do inline replies.
So if we replied only to phab, we'd basically restrict how users are able to write emails, which they will simply not accept.

Let me take a stab at this, I think @btrahan's work demonstrated that there's definitely a real bug in the upstream here.