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

Feature/async email support #33

Merged

Conversation

bzhr
Copy link
Contributor

@bzhr bzhr commented Apr 24, 2020

This PR follows the guides from #11
Adds ASYNC_EMAIL_TASK setting to settings.

In mixins.py first, if ASYNC_EMAIL_TASK setting is defined and it's a string, will be imported in top.
Then before sending each email, first it's checked if the async function is defined and if it is, call the function with the email send function and it's arguments, else just send the email.

This is done for:

  1. Send activation email
  2. Resend Activation Email
  3. Password reset
  4. Secondary email activation

Right now when I run tests, there's no test case for async functions, so either a test case needs to be provided, or we need to add async email support in the example app.

@PedroBern please review and let me know what you think.

@PedroBern
Copy link
Owner

PedroBern commented Apr 24, 2020

@PedroBern please review and let me know what you think.

It's all perfect!

Right now when I run tests, there's no test case for async functions, so either a test case needs to be provided, or we need to add async email support in the example app.

As it's actually a pseudo async support, we don't need to write an async test case. However, the testing will be not so trivial, we need to:

  • Create a new file tests/pseudo_async_email_support.py with a regular function to be imported as the async function;
  • Create tests/settings_c.py, just like the settings_b.py, but only with the new setting, importing the function created early;
  • Edit tox.ini file, just copy and paste the py.test --ds=tests.settings_b ..., but for settings_c;
  • Create one test for each of the supported functions, inside each respective file, these tests must mock the pseudo function, and check if it was called. If possible, check if it was called with the right arguments;
  • Mark these new tests with the @mark.settings_c decorator;

Feel free to do what you judge the best, these steps were just what came into my mind about creating these tests.

It will simply test if the function was called. I guess we need only this, the async implementation can be handled outside of the package.

@PedroBern PedroBern linked an issue Apr 24, 2020 that may be closed by this pull request
@PedroBern PedroBern mentioned this pull request Apr 27, 2020
@PedroBern PedroBern changed the base branch from master to async-email-support April 27, 2020 13:45
@PedroBern
Copy link
Owner

PedroBern commented Apr 27, 2020

I've changed the base branch to merge this, I can create the tests :) I'm merging now

@PedroBern PedroBern merged commit 7276072 into PedroBern:async-email-support Apr 27, 2020
@PedroBern PedroBern mentioned this pull request Apr 27, 2020
@PedroBern
Copy link
Owner

PedroBern commented Apr 27, 2020

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 mentioned this pull request Apr 27, 2020
@PedroBern
Copy link
Owner

@bzhr this was released on v0.3.10 today :)

@bzhr
Copy link
Contributor Author

bzhr commented Apr 28, 2020

@PedroBern Thanks for notifying me. I thought that you're cancelling the PR, but you were just cancelling the tests for now?

@PedroBern
Copy link
Owner

Yes, I made some changes (used a tuple as the second argument in all function calls and some adjustments on SendPasswordResetEmailMixin). Made the tests in the testproject, live testing.

The reason is that this pattern of many setting files is annoying me, I would like to use a @override_settings decorator to handle this but some months ago I wasn't successful (if you can please tell me how, something in the testing looks like making it not work), then I created the settings_b. I will see if I get back to it and try to make it work, lettings only the settings_b because it uses a different user model. However, if I can't, I will add tests as I said, with a settings_c.

@bzhr
Copy link
Contributor Author

bzhr commented Apr 28, 2020

Awesome! I agree that creating multiple settings files isn't very efficient way to handle the settings/testing. This is ok for simple use cases, we can discuss how to further improve it.

@PedroBern
Copy link
Owner

Exactly. Thanks for your openness, any ideas feel free to tell me! I will have a look at this in the future when I have more time.

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

Successfully merging this pull request may close these issues.

Async email support
2 participants