Summary

Improve review emails

Review Request #208 — Created Jan. 24, 2008 and discarded

Information

Owner

jbellis

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Several improvements:

 * includes user for use in templates
 * sets From and subjects to a format more easily distinguishable as autogenerated
 * does not send mail to the initiator of an action; he knows what he did

Testing Done

So there's a couple good things in here that I'd want in, but most of it is very unique to your setup and I imagine most users are going to be unhappy if these things change. We certainly can't apply these here at VMware. I have some recommendations for what we can do that satisfy most of the changes.

I'd like David's input on these as well.

david Jan. 25, 2008, 3:26 p.m.

I can't say I'm thrilled about sticking things like this in and filling settings.py with lots of special cases.  Each thing like this that we add makes it harder for admins to figure out what to do for their setup, as well as gives us more code-paths to test.

If we're aiming to improve filter-a-bility, could we instead add some special header?  I know that at least bugzilla adds a bunch of X-Bugzilla headers in the e-mails it sends out.

The cleanup looks good.

chipx86 Jan. 25, 2008, 3:56 p.m.

Okay, so to make sure I'm on the same page, keep things as they are today but add X-ReviewBoard? I'd be fine with that.

That'd get gmail to stop tagging my bug mail with "Review Request" when it says "review request:" in the bug summary ;)

I agree with the difficulty in adding new settings. I think then what I'd prefer is to see the custom e-mail stuff stripped from the patch and the special header added.

In the future when we have extensions/hooks, people can do more custom things with their e-mail by writing their own hooks to send e-mail how they see fit if they have a setup that greatly differs from what we provide.

/trunk/reviewboard/reviews/email.py (Diff revision 1)

I'm unsure about this change. This definitely will be frowned upon at many places using Review Board, because it's handy to just reply to a user if you need to or to keep filters by e-mail addresses. If this is something you really need for your install, I'd suggest a new setting. Say, settings.EMAIL_USE_CUSTOM_SENDER, and settings.EMAIL_SENDER_SUFFIX (for the "(RB)").

What's the reasoning for this change? Isn't it a good thing to see these sent from the user that made the change? It's just an automated way for the user to send out the e-mail.

This will mess with threading and conversations in several e-mail clients.

jbellis Jan. 24, 2008, 10:06 a.m.

The question is, are you email-centric, or Review Board centric?  If the latter you want people to comment on RB, not just email each other where other reviewers won't see it.

Additionally it's just bad form to screw with headers like that such that it's not immediately clear that it's not a "real" email.  As "it makes it harder to filter" appears to carry a lot of weight with you, I claim that here as well.

chipx86 Jan. 25, 2008, 7:24 a.m.

We're both. We have a lot of people who get all updates through e-mail and follow conversations through e-mail. There are times when a conversation that starting in a review moves off to e-mail because it's no longer about the change but about a higher level design or some only slightly related topic. Keeping it in the review request wouldn't be correct.

These are "real" e-mails. They're just not sent from the e-mail client the user is using. We become the e-mail client.

We're not going to be able to commit any changes that change the basic way that our e-mail setup works. Too many people rely upon the existing method.

/trunk/reviewboard/reviews/email.py (Diff revision 1)

Good cleanup. Thanks.

/trunk/reviewboard/reviews/email.py (Diff revision 1)

This will also have to be optional, defaulted to off. People will probably have our heads if they can no longer see their own review requests sent out, since it won't be in their outbox.

JB

jbellis Jan. 24, 2008, 10:04 a.m.

/trunk/reviewboard/reviews/email.py (Diff revision 1)

Using [Labels] is bad for a lot of setups like ours at VMware, where you have lots of listservs and are already filtering by [Labels]. We try to keep e-mails threaded and have them look like replies (they're more natural that way), so the Re: is going to be required.

In this case, since we have different ideas of how this should look to our users, I suggest another settings key:

settings.EMAIL_REVIEW_SUBJECT_PREFIX

Or some such. This will default to "Re: Review Request: " and you can change it to "[RB] Reviewed: " for your settings_local.py.

/trunk/reviewboard/reviews/email.py (Diff revision 1)

The message ID can't go though. This needs to stay.

jbellis Jan. 24, 2008, 10:07 a.m.

Sorry, this was a screw-up from doing manual patch generation.

/trunk/reviewboard/reviews/email.py (Diff revision 1)
```
Same here as above.
```