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

Async email support #11

Closed
PedroBern opened this issue Mar 4, 2020 · 8 comments · Fixed by #33 or #35
Closed

Async email support #11

PedroBern opened this issue Mar 4, 2020 · 8 comments · Fixed by #33 or #35
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers
Projects

Comments

@PedroBern
Copy link
Owner

Currently, all emails are sent in the mixins.py but would be great to have async support.

I think the easiest way to make an optional plug and play support would be creating a new setting to enter a function that wraps each send email call.

Actually it's not an async email support, but make easy to integrate with your own async solution.

# settings
EMAIL_ASYNC_TASK = None

Then it accepts a function path"

EMAIL_ASYNC_TASK: "path/to/task"

The task need to accept the email send function and its arguments, usage with celery would be something like this:

from celery import task
 
@task
def graphql_auth_async_email(func, *args):
    """
    Task to send an e-mail for the graphql_auth package
    """
 
    return func(*args)

Then, in the mixins.py, we need to change all send email calls, for example:

# from
user.status.send_activation_email(info)

# to
if app_settings.EMAIL_ASYNC_TASK:
    app_settings.EMAIL_ASYNC_TASK(user.status.send_activation_email, info)
else:
    user.status.send_activation_email(info)

Of course, to make this work we must import the function from its path in the settings.py, probably using:

from django.utils.module_loading import import_string
@PedroBern PedroBern added the good first issue Good for newcomers label Mar 4, 2020
@PedroBern PedroBern added this to To do in TODO Mar 4, 2020
@PedroBern PedroBern added the enhancement New feature or request label Mar 6, 2020
@yanivtoledano
Copy link
Contributor

Linked to the topic of emails (but not necessarily the same) is perhaps to have a feature to include other email providers as well (e.g., by enabling django-anymail).

At the moment, I believe emails in general are only customizable via templates - it would be useful for users to be able to use templates / template IDs from their email providers (like Sendgrid, Mailgun, etc.) which as far as I can tell is impossible now.

@yanivtoledano
Copy link
Contributor

I see this has been mentioned here. Was not aware this is easily achievable with the current setup however?

@PedroBern
Copy link
Owner Author

Was not aware this is easily achievable with the current setup however?

You can use your favorite email provider, just follow the anymail 1-2-3.

At the moment, I believe emails in general are only customizable via templates

Yes currently the email templates are customizable only by django templates, it can be enhanced in the future, or you can propose changes and make a pull request :D

@github-actions
Copy link

github-actions bot commented Apr 9, 2020

This issue has been automatically marked as stale. It will be closed if no further activity occurs.

@nazmulhasan85
Copy link

It's a great idea! I want to add something more here.

Is it possible to bring some hook-able option so that User can handle email sending part by themselves?

For example, I've all the email template in mandrill. So I just wanted to reuse all those templates with the arguments.

@PedroBern
Copy link
Owner Author

Is it possible to bring some hook-able option so that User can handle email sending part by themselves?

@nazmulhasan85 Yes, you can do that! The easiest way right now is to simply:

  • add a hook inside de UserStatus.send;
  • take the self, context, template, _subject, message, html_message as arguments. Although you'd probably build the last 2 or 3 with the context, it would be better to support it.
  • Return the hook before the return line in case a hook function is specified in the settings;
  • Create a setting for this that takes one function path (just like here)to handle all email sending (and in the function the user would need to distinguish the email type using the template argument);
  • Update the docs, settings part, and warn that this custom function should raise the same possible exceptions as django.core.mail.send_email in order to work.

Suggestions? What you think?

Also, it would be useful to solve #22 along.

@nazmulhasan85
Copy link

@PedroBern Sounds good

Lemme have a look. If i can manage sometime I will definitely jump on it.

@PedroBern PedroBern linked a pull request Apr 24, 2020 that will close this issue
@PedroBern
Copy link
Owner Author

PedroBern commented Apr 27, 2020

Started to work on this with #33 and adding a new branch, but I found some problems:

  • In RegisterMixin, SendSecondaryEmailActivationMixin if the email is in use, it will still send a successful response.
  • In ResendActivationEmailMixin if the user is already verified, it will still send a successful response.

The only way to fix it is by moving the logic of the exception out of the model, bringing it to the mixins. It will give a lot more work than expected. Not planning to work on this soon, because of it, I'm closing.

@PedroBern PedroBern added wontfix This will not be worked on and removed wontfix This will not be worked on labels Apr 27, 2020
@PedroBern PedroBern reopened this Apr 27, 2020
@PedroBern PedroBern linked a pull request Apr 27, 2020 that will close this issue
@PedroBern PedroBern self-assigned this Apr 27, 2020
@PedroBern PedroBern moved this from To do to Done in TODO Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
TODO
  
Done
3 participants