search

sunscrapers / django-templated-mail

Send emails using Django template system.
MIT License
95 stars 21 forks source link

Inheriting from EmailMessage causes problems #28

Open spookylukey opened 3 years ago

spookylukey commented 3 years ago

The current implementation inherits from a Django EmailMessage class and then adds stuff, including adding request and inheriting from ContextMixin. This has several issues:

  1. ContextMixin is designed for views, and this class has got nothing to do with views, so this is a really unhelpful mixing of completely different layers in my opinion. I cannot see that ContextMixin is adding anything here.
  2. Adding request as an attribute breaks the ability of the resulting message to be pickled, which breaks django-mailer - see https://github.com/pinax/django-mailer/issues/144 which means that people can't use djoser with django-mailer.
  3. It's a completely unnecessary use of inheritance in my opinion.

This is what I think the overall process looks like:

  1. We prepare context data to be passed to a template, optionally using a request object or Site object to populate some context data variables.
  2. We render a template, with some special code to be able to extract certain blocks, allowing us to have a single template for the subject/plain text/html parts. This part of the process would be especially easy using django-render-block (which apparently was partly inspired by django-templated-mail.)
  3. We put these 3 parts together into an EmailMessage and return it, using only the public interface of EmailMessage (or EmailMultiAlternatives) to construct the message.

None of this requires subclassing EmailMessage. I think a good API would be just this:

def build_email_from_template(template_name, *, extra_context=None, request=None, site=None):
    ...

This gets rid of subclassing and defining get_context_data() - passing in the context directly is a far superior API than subclassing and overriding methods.

I'm willing to bet that the resulting code will be significantly clearer (no passing things around implicitly on self etc) if done this way, and also probably a bit shorter, while avoiding all the issues described.

thelittlebug commented 3 years ago

im pretty sure this is more a "hack" than a solution but i think i'm not able to do the things @spookylukey has suggested without breaking djoser. i think sunscrapers has to coordinate that refactoring.

but in the meantime i'm knocked out by this issue so i made this: https://github.com/sunscrapers/django-templated-mail/pull/29

spookylukey commented 3 years ago

@thelittlebug Thanks so much for your reply, and #29 does look like it will at least provide a solution for https://github.com/pinax/django-mailer/issues/144