Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inheriting from EmailMessage causes problems #28

Open
spookylukey opened this issue Jun 18, 2021 · 2 comments
Open

Inheriting from EmailMessage causes problems #28

spookylukey opened this issue Jun 18, 2021 · 2 comments

Comments

@spookylukey
Copy link

spookylukey commented Jun 18, 2021

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 Having problem on django-mailer on using with djoser pinax/django-mailer#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
Copy link

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: #29

@spookylukey
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants