fix CCs when sending emails

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.

[Andi Albrecht, 2011-06-30 07:31:20]

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?

[steve2, 2011-06-30 19:53:30]

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

On Thu, Jun 30, 2011 at 12:31 AM, <albrecht.andi@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, 2011-07-01 12:27:46]

Hi Steve,

thanks for the explanations! See inline reply below.

Steve Howard <steve@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, <albrecht.andi@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, 2011-07-01 12:31:31]

On Wed, Jun 29, 2011 at 12:41 AM,  <steve@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...

>
> 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)
>
>
>
>
>

[steve2, 2011-07-01 23:58:25]

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

> Hi Steve,
>
> thanks for the explanations! See inline reply below.
>
> Steve Howard <steve@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



>
> >
> > 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, <albrecht.andi@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, 2011-07-04 13:25:15]

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

http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/a...
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/a...
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.

[steve2, 2011-07-04 21:31:34]

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

>
> http://codereview.appspot.com/**4657057/diff/6001/gae2django/**
>
gaeapi/appengine/api/mail.py<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<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<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/<http://codereview.appspot.com/4657057/>
>

[Andi Albrecht, 2011-07-05 04:35:10]

On Mon, Jul 4, 2011 at 11:31 PM, Steve Howard <steve@thumbtack.com> wrote:
> On Mon, Jul 4, 2011 at 6:25 AM, <albrecht.andi@googlemail.com> wrote:
>>
>>
>>
http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/a...
>> File gae2django/gaeapi/appengine/api/mail.py (right):
>>
>>
>>
http://codereview.appspot.com/4657057/diff/6001/gae2django/gaeapi/appengine/a...
>> 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/a...
>> 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

> 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/
>
>