ambitioninc / django-entity-emailer

:mailbox: Send email to Entities.
MIT License
8 stars 8 forks source link

Email Immediate Send #2

Closed swans-one closed 10 years ago

swans-one commented 10 years ago

@wesleykendall @jmcriffey This pull request implements the feature of having an email sent immediately upon saving it to the database. It implements this through a post-save hook, and spawns a celery task to actually send the email.

Of all the code in this pull request, I'm most interested in hearing feedback on the following:

  1. Does the code in tasks.SendEmailAsyncNow.run look sensible? This is the piece that actually sends the email out.
  2. This code sends an email with the "from" field of the email based off of settings.DEFAULT_FROM_EMAIL. Is that reasonable?
  3. Is my documentation clear?
swans-one commented 10 years ago

@wesleykendall This build is failing, and it looks like it's because of something in django-entity. It runs fine locally, but when run on travis it spits out this error:

error: /home/travis/build/Wilduck/django-entity-emailer/django_entity-0.3.5-py2.7.egg/entity/migrations: Not a directory

It seems that django-entity is missing a "migrations" directory? Any thoughts?

swans-one commented 10 years ago

@wesleykendall @jmcriffey Comments addressed. Thanks for looking over this.

jeffrifwald commented 10 years ago

Looks good to me. :guitar:

wesleykendall commented 10 years ago

@Wilduck had one comment about a query you were doing

wesleykendall commented 10 years ago

@Wilduck you may want to use ENTITY_EMAILER_DEFAULT_FROM as the setting variable instead. I bet many people use a DEFAULT_FROM_EMAIL variable for other things

swans-one commented 10 years ago

@wesleykendall I implemented your performance optimization, and also created the option for a user to set settings.ENTITY_EMAILER_FROM_EMAIL. If that value is set, it will be sent in the from field, otherwise the DEFAULT_FROM_EMAIL will be used.

wesleykendall commented 10 years ago

@Wilduck you left a print statement in there. ready to merge when its gone