fix CCs when sending emails (issue4657057)

[st...@thumbtack.com]

Reviewers: Andi Albrecht,

Description:
Django 1.3 introduced the cc constructor kwarg for
django.core.mail.EmailMessage. CC addresses do not appear to work at
all in trunk, and do appear to work with this patch. Not sure if you're
trying to be compatible with Django <1.3, if so I guess an alternate
patch is necessary.

Please review this at http://codereview.appspot.com/4657057/

Affected files:
M gae2django/gaeapi/appengine/api/mail.py

Index: gae2django/gaeapi/appengine/api/mail.py
===================================================================
--- gae2django/gaeapi/appengine/api/mail.py (revision 166)
+++ gae2django/gaeapi/appengine/api/mail.py (working copy)
@@ -101,14 +101,10 @@

def send(self):
headers = {}
- if self.cc:
- headers['Cc'] = ', '.join(self.cc)
- if self.bcc:
- headers['Bcc'] = self.bcc
if self.reply_to:
headers['Reply-To'] = ', '.join(self.reply_to)
msg = _EmailMessage(self.subject, self.body, self.sender,
- self.to, self.bcc, headers=headers)
+ self.to, self.bcc, cc=self.cc, headers=headers)
msg.send(fail_silently=True)

[albrec...@googlemail.com]

Hi Steve,

thanks for your patch and reporting this issue. But I wasn't able to
reproduce it. What I've tested/verified so far:

- with Django 1.2.5 and Django 1.3 the CC header is in the mail
- with Django 1.3 there's no issue when CC is in headers (current
situation), it's still in the outgoing message

However, Rietveld removes the issue owner from CCs before sending. Maybe
you've seen this behavior?

http://codereview.appspot.com/4657057/

[Steve Howard]

Hi Andi,

Sorry, I should have been more specific.  The behavior I observed was that the message itself included the correct CC header, but the message was not actually delivered to CC recipients.  This fooled me at first because I tested it by sending an email To: me and Cc: coworkers, and the email looked fine to me.  But it was not delivered to my coworkers, as I was able to confirm by sending an email To: a coworker and Cc: me.

From my cursory reading of the SMTP wikipedia page, my understanding is that the mail client specifies recipients independently of the message body, which includes the To: and Cc: headers.  This is supported by looking at the code in django.core.mail -- message.EmailMessage.recipients() returns to + cc + bcc, while EmailMessage.message() adds the To and Cc headers.  And smtp.EmailBackend._send() uses the recipients() list in transmitting the email, converting the message itself to a string before passing it to smtplib -- it never examines the headers itself.

So I think the correct behavior is to pass the CC list to the constructor of the django.core.mail.message.EmailMessage, and *not* add the Cc: or Bcc: headers in client code -- Django's EmailMessage adds the Cc: header itself, and the Bcc: header should not be added at all because it would then be visible to all recipients.

Hope this clarifies things, and thank you for your great work on gae2django, it's really handy :)

Steve

[Andi Albrecht]

Hi Steve,

thanks for the explanations! See inline reply below.

Steve Howard <st...@thumbtack.com> writes:

> Hi Andi,
>
> Sorry, I should have been more specific. The behavior I observed was that
> the message itself included the correct CC header, but the message was not
> actually delivered to CC recipients. This fooled me at first because I
> tested it by sending an email To: me and Cc: coworkers, and the email looked
> fine to me. But it was not delivered to my coworkers, as I was able to
> confirm by sending an email To: a coworker and Cc: me.
>
> From my cursory reading of the SMTP wikipedia page, my understanding is that
> the mail client specifies recipients independently of the message body,
> which includes the To: and Cc: headers. This is supported by looking at the
> code in django.core.mail -- message.EmailMessage.recipients() returns to +
> cc + bcc, while EmailMessage.message() adds the To and Cc headers. And
> smtp.EmailBackend._send() uses the recipients() list in transmitting the
> email, converting the message itself to a string before passing it to
> smtplib -- it never examines the headers itself.

Yes, you're right! The problem is that manual CC headers are not taken
into account by Django when recipients() is called. So, the way
gae2django handles the Cc argument doesn't work and there's no way to
make it work with Django < 1.3. As you've pointed out, starting with
Django 1.3 it could actually work, when the CC keyword is passed to the
constructor of message.EmailMessage.

>
> So I think the correct behavior is to pass the CC list to the constructor of
> the django.core.mail.message.EmailMessage, and *not* add the Cc: or Bcc:
> headers in client code -- Django's EmailMessage adds the Cc: header itself,
> and the Bcc: header should not be added at all because it would then be
> visible to all recipients.

Passing Bcc to the constructor of message.EmailMessage should be fine
since both APIs (Django, App Engine) provide this keyword.

>
> Hope this clarifies things, and thank you for your great work on gae2django,
> it's really handy :)

Thanks! Happy to hear that :)

-Andi

>
> Steve
>
> On Thu, Jun 30, 2011 at 12:31 AM, <albrec...@googlemail.com> wrote:
>
>> Hi Steve,
>>
>> thanks for your patch and reporting this issue. But I wasn't able to
>> reproduce it. What I've tested/verified so far:
>>
>> - with Django 1.2.5 and Django 1.3 the CC header is in the mail
>> - with Django 1.3 there's no issue when CC is in headers (current
>> situation), it's still in the outgoing message
>>
>> However, Rietveld removes the issue owner from CCs before sending. Maybe
>> you've seen this behavior?
>>
>>

>> http://codereview.appspot.com/**4657057/<http://codereview.appspot.com/4657057/>
>>

[Andi Albrecht]

On Wed, Jun 29, 2011 at 12:41 AM, <st...@thumbtack.com> wrote:
> Reviewers: Andi Albrecht,
>
> Description:
> Django 1.3 introduced the cc constructor kwarg for
> django.core.mail.EmailMessage.  CC addresses do not appear to work at
> all in trunk, and do appear to work with this patch.  Not sure if you're
> trying to be compatible with Django <1.3, if so I guess an alternate
> patch is necessary.

Yes, gae2django should be compatible with Django 1.2 for now...

[Steve Howard]

On Fri, Jul 1, 2011 at 5:27 AM, Andi Albrecht <albrec...@googlemail.com> wrote:

Hi Steve,

thanks for the explanations! See inline reply below.

Steve Howard <st...@thumbtack.com> writes:

> Hi Andi,
>
> Sorry, I should have been more specific.  The behavior I observed was that
> the message itself included the correct CC header, but the message was not
> actually delivered to CC recipients.  This fooled me at first because I
> tested it by sending an email To: me and Cc: coworkers, and the email looked
> fine to me.  But it was not delivered to my coworkers, as I was able to
> confirm by sending an email To: a coworker and Cc: me.
>
> From my cursory reading of the SMTP wikipedia page, my understanding is that
> the mail client specifies recipients independently of the message body,
> which includes the To: and Cc: headers.  This is supported by looking at the
> code in django.core.mail -- message.EmailMessage.recipients() returns to +
> cc + bcc, while EmailMessage.message() adds the To and Cc headers.  And
> smtp.EmailBackend._send() uses the recipients() list in transmitting the
> email, converting the message itself to a string before passing it to
> smtplib -- it never examines the headers itself.

Yes, you're right! The problem is that manual CC headers are not taken
into account by Django when recipients() is called. So, the way
gae2django handles the Cc argument doesn't work and there's no way to
make it work with Django < 1.3. As you've pointed out, starting with
Django 1.3 it could actually work, when the CC keyword is passed to the
constructor of message.EmailMessage.

What do you think about the new patch set?  As far as I can see this is the only way to do it correctly in Django <1.3: add the Cc: header explicitly and include the CC addresses in the bcc constructor kwarg.  Django's EmailMessage class only uses the bcc kwarg in recipients(), so this has exactly the desired effect: add the CC addresses to the list of recipients and include them in the Cc: header.

FYI, I tested it with Django's console email backend:

    EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'

but in order to check the recipients list, I patched django/core/mail/backends/console.py as follows:

--- a/django/core/mail/backends/console.py

+++ b/django/core/mail/backends/console.py

@@ -23,6 +23,8 @@ class EmailBackend(BaseEmailBackend):

             try:

                 stream_created = self.open()

                 for message in email_messages:

+                    self.stream.write('New email from %r to %r\n'

+                                      % (message.from_email, message.recipients()))

                     self.stream.write('%s\n' % message.message().as_string())

                     self.stream.write('-'*79)

                     self.stream.write('\n')

 

>
> So I think the correct behavior is to pass the CC list to the constructor of
> the django.core.mail.message.EmailMessage, and *not* add the Cc: or Bcc:
> headers in client code -- Django's EmailMessage adds the Cc: header itself,
> and the Bcc: header should not be added at all because it would then be
> visible to all recipients.

Passing Bcc to the constructor of message.EmailMessage should be fine
since both APIs (Django, App Engine) provide this keyword.

I agree it must be passed to the Django EmailMessage constructor so that it'll be added to the recipient list, I just meant gae2django's EmailMessage should not add 'Bcc' to the headers dict, otherwise that header will be included in the message contents and therefore visible to all recipients (at least all who view the raw message).

Steve

[albrec...@googlemail.com]

http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py
File gae2django/gaeapi/appengine/api/mail.py (right):

http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py#newcode80
gae2django/gaeapi/appengine/api/mail.py:80: if value is not None:
Why did you add this test? None values for missing fields should be ok
(see comment below too)

http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py#newcode110
gae2django/gaeapi/appengine/api/mail.py:110: self.to, self.cc +
self.bcc, headers=headers)
This collapses CC and BCC into Bcc. IMO we need two paths for
constructing a new EmailMessage instance depending on Django version in
use here.

http://codereview.appspot.com/4657057/

[Steve Howard]

On Mon, Jul 4, 2011 at 6:25 AM, <albrec...@googlemail.com> wrote:

http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py
File gae2django/gaeapi/appengine/api/mail.py (right):

http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py#newcode80
gae2django/gaeapi/appengine/api/mail.py:80: if value is not None:
Why did you add this test? None values for missing fields should be ok
(see comment below too)

Just made it more convenient to say "self.cc + self.bcc" without having to check for Nones.

 

http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py#newcode110
gae2django/gaeapi/appengine/api/mail.py:110: self.to, self.cc +
self.bcc, headers=headers)
This collapses CC and BCC into Bcc. IMO we need two paths for
constructing a new EmailMessage instance depending on Django version in
use here.

But the bcc kwarg is only used by Django's EmailMessage class in the recipients() method, to generate the recipient list.  It's never actually used in any BCC-specific way, it doesn't add a Bcc: header or anything.  So what we're really doing here is adding the CC addresses to the recipients list and putting them in a Cc: header, which is the correct way to handle CC.  And if you look at the Django 1.3 EmailMessage class you'll see those are exactly the two things it does with the cc kwarg.

I agree that from the point of view of using Django's API, it's confusing.  The real problem is that Django's email API is lacking.  I like this solution because it works in Django 1.2 and Django 1.3, it has the exact same effect as passing the cc kwarg in Django 1.3, and it's not terribly confusing -- we could add a short comment explaining why we're sticking CC addresses into the bcc kwarg.  The only alternative I'm aware of is to subclass Django's EmailMessage, which sounds like a lot more work (from https://code.djangoproject.com/ticket/7722; I'm surprised no one in that thread mentioned the approach used here).

It's a judgment call, I leave that up to you :)

Steve

 

http://codereview.appspot.com/4657057/

[Andi Albrecht]

On Mon, Jul 4, 2011 at 11:31 PM, Steve Howard <st...@thumbtack.com> wrote:
> On Mon, Jul 4, 2011 at 6:25 AM, <albrec...@googlemail.com> wrote:
>>
>>
>> http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py
>> File gae2django/gaeapi/appengine/api/mail.py (right):
>>
>>
>> http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py#newcode80
>> gae2django/gaeapi/appengine/api/mail.py:80: if value is not None:
>> Why did you add this test? None values for missing fields should be ok
>> (see comment below too)
>
> Just made it more convenient to say "self.cc + self.bcc" without having to
> check for Nones.
>
>>
>>
>> http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/api/mail.py#newcode110
>> gae2django/gaeapi/appengine/api/mail.py:110: self.to, self.cc +
>> self.bcc, headers=headers)
>> This collapses CC and BCC into Bcc. IMO we need two paths for
>> constructing a new EmailMessage instance depending on Django version in
>> use here.
>
> But the bcc kwarg is only used by Django's EmailMessage class in the
> recipients() method, to generate the recipient list.  It's never actually
> used in any BCC-specific way, it doesn't add a Bcc: header or anything.  So
> what we're really doing here is adding the CC addresses to the recipients
> list and putting them in a Cc: header, which is the correct way to handle
> CC.  And if you look at the Django 1.3 EmailMessage class you'll see those
> are exactly the two things it does with the cc kwarg.

Agreed. The CC's are in the CC header and both CC+BCC are correctly
returned when recipients() is called. So you approach should be fine
with all current Django versions.

Thanks again for clarification and the patch. Committed as r169.

-Andi