Replace django mail functions with app engine compatible versions about google-app-engine-django HOT 9 CLOSED

Hi Ryan, 

Option 3 is certainly the best option. The approach that we've taken with the 
helper
is to monkey patch the bits of various modules that need changing rather than
providing a completely new implementation.

The helper needs to support both the current stable release and trunk, in most 
cases
so far this hasn't been too much extra work to achieve.


If that sounds reasonable to you we'd be happy to receive any patches you can 
supply.
You'll need to sign the Google CLA at
http://code.google.com/legal/individual-cla-v1.0.html before we can accept any 
code.

Thanks.

Hi Matt - Here is a patch that should enable the traditional django email api 
for
both 0.96 and 0.97+.  This should be the only minor dependency that was holding 
back
inclusion of CommonMiddleware.

Turns out that the /django/core/mail.py code changed considerably, so I had to
implement the functionality two different ways.  As we move to 0.97 we should 
be able
to trim the new appengine_django/mail.py file.

Let me know if you have any questions.

Hi Ryan, 

Thanks for your work on this. It looks good, a few minor comments:

* You monkey patch the revised methods in both mail.py and __init__.py, please
confine all monkey patching to the Install... method that you create within 
__init__.py.
* Once you do this, you can remove the if VERSION statements from mail.py and 
leave
it up to the Install method to decide which implementation to monkey patch into 
use.
* Please ensure that there are no lines over 80 characters in length. 
* The docstring for send_mass_mail looks to be copy/pasted from Django and 
doesn't
match the implemenation (eg. EMAIL_HOST_USER and EMAIL_HOST_PASSWORD are not 
actually
used), Please fix this comment to match the actual behaviour.
* Please re-order and group the import statements so they are alpabetically 
listed,
with standard Python imports first, followed by django imports and imports from
google.appengine in the final group. 

Finally, do you have any tests that we can use to verify that this code works
correctly in future?

Thanks.

Updated Diff based on comments from Matt.  Note that I didnt want to stick the
GoogleSMPTConnection class in __init__.py, so I need to at least do a check in 
the
mail.py file for the existence of its parent (otherwise an exception is 
thrown).  If
the parent class does not exist, we are in 0.96 and we dont want to create the 
class.
  One alternative to this check would be to define two different .py files -one that
is loaded in 0.96 and the other in 0.97+. (ick)  I'm new to python though, so if
there is a better way to handle this let me know.

I dont have unit tests for this right now.  Will have to work on those later.  I
tested on a sample app that I had been working on (and had seen the original 
issue).

You can use an idiom such as the following to deal with parent class differences
between 0.96 and 0.97:

try:
  # 0.97 path
  from django.core.mail import SMTPConnection
except ImportError
  # 0.96 path
  SMTPConnection = object

class GoogleSMTPConnection(SMTPConnection)

Obviously this will result in a class that doesn't have all the required
functionality when running under 0.96, but that's OK, because the crippled 
class will
never be monkey patched into place.

See appengine_django/db/base.py for an example of this in practice.


Lines 67, 103, 110 and 117 of your mail.py still contain monkey patching code 
that
should be moved into the Install function.

I think this should do the trick.  All version info moved out of mail.py as 
well as
all references to overwriting core django code.

Hi Ryan, 

The monkey patching code looks good now. 

* Please add copyright and Apache 2.0 license headers to mail.py.
* The warnings on lines 44-50 of mail.py are still > 80 characters.
* In general I think lots of the logging could be reduced, it seems very 
verbose.

A few more comments on the implementation, now that I've studied how Django 
does it
more closely, sorry for not picking these up in the initial review:

* Why do you need to replace mail_admins and mail_managers?
* For the Django 0.97 SMTPConnection class, you could re-implement just the 
_send
method with faked open and closed methods that just ensure that the .connection
member is True. Then you don't have to replace the send_messages method which is
where all the logic is. This will make the class easier to maintain in future 
and
will retain the ability to send multi-part messages.
* For Django 0.96 you could provide a replacement implementation of smtplib.SMTP
instead of modifying the send_mass_mail method. This would preserve the ability 
to
send multi-part messages.

* Please add copyright and Apache 2.0 license headers to mail.py. (Done)
* The warnings on lines 44-50 of mail.py are still > 80 characters. (Done)
* In general I think lots of the logging could be reduced, it seems very 
verbose.
(Removed some, moved others to logging.info...)
* Why do you need to replace mail_admins and mail_managers? (I created a new 
method,
called mail_group, to add some additional functionality.  This allows you to use
either the settings.ADMINS or MANAGERS if defined.  If not, it defaults to the 
Google
App Engine admin group.)
* For the Django 0.97 SMTPConnection class, you could re-implement just the 
_send
method with faked open and closed methods that just ensure that the .connection
member is True. Then you don't have to replace the send_messages method which is
where all the logic is. This will make the class easier to maintain in future 
and
will retain the ability to send multi-part messages. (Ok, did this but it just 
moves
the logic to another method.  Guess it does decouple from the counting logic 
though
so that is nice...)
* For Django 0.96 you could provide a replacement implementation of smtplib.SMTP
instead of modifying the send_mass_mail method. This would preserve the ability 
to
send multi-part messages. (Unclear what you mean here - you mean the 
attachments? 
Doesnt look like 0.96 supported attachments at all - the api doesnt have the 
ability
to specify them.  I could be wrong.  Also, email attachment conversion is 
outside of
the scope of this patch.)

Apologies for the delay in turning this around.

Thanks Ryan, 

I've committed this code in r16.