Custom from email #13
Conversation
There was a problem hiding this comment.
Hi @lmccartney
Thanks for the pull request and your work so far. Sorry this review is a little late, I totally missed my Github notifications.
I think there is a typo causing a test to fail and there is also a flake8 error due to an unused import.
You can run the tests with tox (this will run flake8, mypy and most of the major python and django versions)
It would also be good to put a few more checks and tests around the functionality to ensure it doesn't break in the future etc.
Also thanks for the spelling fixes etc in the README.
Matt
| @@ -89,7 +89,7 @@ def send(self, request: HttpRequest) -> None: | |||
| subject=settings.EMAIL_SUBJECT, | |||
| message=plain, | |||
| recipient_list=[user.email], | |||
| from_email=djsettings.DEFAULT_FROM_EMAIL, | |||
There was a problem hiding this comment.
Could you add a new test to actually check this value is used?
Something like the test_send_email_pass_email_in_unsubscribe test in test_models.py without the unsubscribe bits - https://github.com/pyepye/django-magiclink/blob/master/tests/test_models.py#L114-L135
There was a problem hiding this comment.
I think the djsettings import at the top is now no longer used, can you remove it please?
| raise ImproperlyConfigured('"MAGICLINK_IGNORE_UNSUBSCRIBE_IF_USER" must be a boolean') | ||
|
|
||
| FROM_EMAIL = getattr(settings, 'MAGICLINK_FROM_SETTINGS', settings.DEFAULT_FROM_EMAIL) |
There was a problem hiding this comment.
I think there is a typo here which will cause this functionality not to work. A test will fail with:
AssertionError: assert '[email protected]' == '[email protected]'
| FROM_EMAIL = getattr(settings, 'MAGICLINK_FROM_SETTINGS', settings.DEFAULT_FROM_EMAIL) | |
| FROM_EMAIL = getattr(settings, 'MAGICLINK_FROM_EMAIL', settings.DEFAULT_FROM_EMAIL) |
There was a problem hiding this comment.
It could also be good to validate this and default back if it's not valid.
Something like (untested):
from django.core.exceptions import ValidationError
from django.core.validators import validate_email
...
FROM_EMAIL = getattr(settings, 'MAGICLINK_FROM_EMAIL', settings.DEFAULT_FROM_EMAIL)
try:
validate_email(FROM_EMAIL)
except ValidationError:
FROM_EMAIL = settings.DEFAULT_FROM_EMAIL
warning = ('MAGICLINK_FROM_EMAIL is not a valid email address, '
'using DEFAULT_FROM_EMAIL instead')
warnings.warn(warning, RuntimeWarning)
Then please include a test to check the email gets defaulted back to DEFAULT_FROM_EMAIL and the warning. There is also an example of a warning test here - https://github.com/pyepye/django-magiclink/blob/master/tests/test_settings.py#L92-L97
No description provided.