Add config option for always sending emails from server email.
Review Request #9209 — Created Sept. 22, 2017 and submitted
Review Board tries to send e-mails on behalf of users by spoofing their
e-mail address, in order to offer better comment threads. While it tries
to be smart about when to do this (by checking DNS records to see whether
it can safely spoof the e-mail address), it's sometimes ideal to simply
turn off this behavior and send from the server's configured e-mail
address instead.This change adds an option to force the server's e-mail address to be
used for all e-mails. When enabled, e-mails will still appear to come
from the user, but only by name (using "<Name> via Review Board
server@email" as the From address).In order for this change to correctly send e-mails using the "<Name> via
Review Board server@email" format, the mail message in djblets must be
updated to send e-mails using that format whenemail_smart_spoofingis
set toFalse.
The notifications tests were updated to check that the email used is
settings.DEFAULT_FROM_EMAILinstead ofsettings.SERVER_EMAIL. The
reviewboard.notifications.testssuite was run. Tests added for new
review emails and review reply emails with enable_smart_spoofing
disabled.
| Description | From | Last Updated |
|---|---|---|
|
The description should wrap at 70-75 characters. I'd like to see some changes to how this is described. How about: … |
 chipx86
|
|
|
This change needs unit tests that cover the new option. There should be tests for loading/saving the option (based on … |
chipx86
|
|
|
Not sure whether we should be switching the default to SERVER_EMAIL in this change. That will have an impact on … |
chipx86
|
|
|
E501 line too long (82 > 79 characters) |
 reviewbot
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
I'd suggest reversing this option and making it about sending on behalf of users, enabled by default. So something like … |
chipx86
|
|
|
Missing a period at the end of the sentence. |
chipx86
|
|
|
Blank line between these. You want one separating block-creating statements (like an if/else) from other code (like an assignment). |
chipx86
|
|
|
EmailMessage takes an enable_smart_spoofing option, which you should turn off based on the setting. Otherwise, it's going to try to … |
chipx86
|
- Change Summary:
-
Fix line too long from search and replace on
settings.SERVER_EMAILtosettings.DEFAULT_FROM_EMAIL. - Commit:
-
5f9dafe83fab36955bbd019a17a837fe936fcfbc58f32ae9494b22477b8072d8eb6aa38fd60caaac
Checks run (2 succeeded)
-
-
The description should wrap at 70-75 characters.
I'd like to see some changes to how this is described. How about:
Review Board tries to send e-mails on behalf of users by spoofing their e-mail address, in order to offer better comment threads. While it tries to be smart about when to do this (by checking DNS records to see whether it can safely spoof the e-mail address), it's sometimes ideal to simply turn off this behavior and send from the server's configured e-mail address instead. This change adds an option to force the server's e-mail address to be used for all e-mails. When enabled, e-mails will still appear to come from the user, but only by name (using "<Name> via Review Board <server@email>" as the From address). -
This change needs unit tests that cover the new option. There should be tests for loading/saving the option (based on my suggestions in this review), and for sending e-mail with spoofing disabled.
-
Not sure whether we should be switching the default to
SERVER_EMAILin this change. That will have an impact on existing Review Board installs. Best to keep this change solely focused on the new option instead. In general, you want your changes to be heavily focused and not do "Also fixed ..." work (this isn't a black-and-white rule, but is a good goal). -
I'd suggest reversing this option and making it about sending on behalf of users, enabled by default. So something like
mail_enable_smart_spoofing(to keep consistent with internal terminology and changing the label/help text accordingly.Right now, we actually have something in
setting.pyfor this, which isEMAIL_ENABLE_SMART_SPOOFING. Some companies are going to have this turn on/off, so we should back the siteconfig value from this setting and sync both ways (look in thesettings_mapvariable inadmin/siteconfig.py). -
-
Blank line between these. You want one separating block-creating statements (like an if/else) from other code (like an assignment).
-
EmailMessagetakes anenable_smart_spoofingoption, which you should turn off based on the setting. Otherwise, it's going to try to see if it needs to spoof the default header instead.
- Change Summary:
-
Update to address comments on review.
- Description:
-
~ Emails get sent with the user's email address in the 'From' header. This can get those emails caught as spam. With the new option, the 'From' header is set to match the configured 'Sender e-mail address'.
~ Review Board tries to send e-mails on behalf of users by spoofing their
+ e-mail address, in order to offer better comment threads. While it tries + to be smart about when to do this (by checking DNS records to see whether + it can safely spoof the e-mail address), it's sometimes ideal to simply + turn off this behavior and send from the server's configured e-mail + address instead. ~ The 'From' address used is also changed to be the
settings.DEFAULT_FROM_EMAILsetting instead of thesettings.SERVER_EMAILsetting which is inherited from django (root@localhost).~ This change adds an option to force the server's e-mail address to be
+ used for all e-mails. When enabled, e-mails will still appear to come + from the user, but only by name (using "<Name> via Review Board + server@email" as the From address). + + In order for this change to correctly send e-mails using the "<Name> via
+ Review Board server@email" format, the mail message in djblets must be + updated to send e-mails using that format when email_smart_spoofingis+ set to False. - Testing Done:
-
~ The notifications tests were updated to check that the email used is
settings.DEFAULT_FROM_EMAILinstead ofsettings.SERVER_EMAIL. Thereviewboard.notifications.testssuite was run.~ The notifications tests were updated to check that the email used is
+ settings.DEFAULT_FROM_EMAILinstead ofsettings.SERVER_EMAIL. The+ reviewboard.notifications.testssuite was run. Tests added for new+ review emails and review reply emails with enable_smart_spoofing + disabled. - Depends On:
-
- Commit:
58f32ae9494b22477b8072d8eb6aa38fd60caaac175f7c26cba55430839670d46a57febd06b1d686- Diff:
Revision 3 (+106 -19)
Checks run (2 succeeded)
flake8 passed.JSHint passed.
