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

Feat/config mail send #183

Merged
merged 3 commits into from
Mar 7, 2022
Merged

Conversation

robwa
Copy link
Contributor

@robwa robwa commented Feb 24, 2022

Checklist

  • I read Contribution Guidelines
  • I ran the checks and the tests (make check && make test) before submitting the PR on my branch and they passed
  • I attached unit tests testing the provided code so the code coverage will not drop below 98% (should be covered by existing tests)

Description

The proposed changes make the email sending mechanism replaceable.

  • Make the three email sending functions configurable (*_EMAIL_SENDER like LOGIN_AUTHENTICATOR).
  • Adapt checks, if email sending is handled externally.
  • Don't require verification signers to have a BASE_URL set.

Why the change is needed?

Some apps send many emails and use their own mechanism for sending them. Of course, registration mails should be sent with the app's mechanism, so all of them can be treated the same way. Therefore one might want to replace email handling and just use the signers to pass the correct data with the emails.

Related issues, pull requests, links

Email handling might be done in signal handlers (see #56). But nevertheless the email handling of rest_registration would have to be disabled in this case.

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2022

Codecov Report

Merging #183 (9701f4e) into master (bdad5f1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
+ Coverage   97.42%   97.44%   +0.01%     
==========================================
  Files          43       43              
  Lines        1552     1563      +11     
  Branches      207      208       +1     
==========================================
+ Hits         1512     1523      +11     
  Misses         20       20              
  Partials       20       20              
Impacted Files Coverage Δ
rest_registration/settings_fields.py 100.00% <ø> (ø)
rest_registration/api/views/register.py 100.00% <100.00%> (ø)
rest_registration/api/views/register_email.py 100.00% <100.00%> (ø)
rest_registration/api/views/reset_password.py 100.00% <100.00%> (ø)
rest_registration/checks.py 96.95% <100.00%> (+0.13%) ⬆️
rest_registration/utils/nested_settings.py 97.29% <100.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdad5f1...9701f4e. Read the comment docs.

@apragacz
Copy link
Owner

apragacz commented Mar 2, 2022

Hi @robwa, thank you for putting a lot of work and providing that PR.

I have one big question about this: in case you need custom e-mail handling, why just not implement custom Django email backend (like the ones described here: https://docs.djangoproject.com/en/dev/topics/email/#email-backends)? I feel that solution would be much easier and would cover the case you're describing.

BTW: I'm going on short vacation, I should be back in few days.

@robwa
Copy link
Contributor Author

robwa commented Mar 3, 2022

Thank you for your answer and your suggestion, @apragacz.

The Django email backends handle sending of emails, as far as I understand them. What we need is our own mechanism for construction of emails. I.e., we extend the Django email class, handle templates our own way and so on. See e.g. django-templated-email as an example.

The PR makes it possible to use django-templated-email or any other custom email construction mechanism with django-rest-registration. One could add convenience functions for returning the signed data for a user, which is all you need from rest-registration to construct valid emails. But as I wanted to keep it as small as possible, I didn't invest into that topic.

@apragacz
Copy link
Owner

apragacz commented Mar 7, 2022

Thank you for clarifying that @robwa.

Changes look LGTM and I'm gonna merge them soon.

@apragacz apragacz merged commit 589c8d2 into apragacz:master Mar 7, 2022
@robwa robwa deleted the feat/config-mail-send branch March 8, 2022 08:34
apragacz added a commit that referenced this pull request Mar 9, 2022
Changes:

*   Make email sending replaceable (#183)
*   Drop Django 1.x support from requirements
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.

3 participants