Allow empty from_email param #131
Conversation
| cc=None, bcc=None, headers=None, | ||
| template_prefix=None, template_suffix=None, | ||
| template_dir=None, file_extension=None, | ||
| attachments=None, create_link=False): | ||
|
|
||
| from_email = from_email or settings.DEFAULT_FROM_EMAIL |
There was a problem hiding this comment.
We have also a TEMPLATED_EMAIL_FROM_EMAIL setting.
A user thought this was a default. See: #97 (comment)
So I think this should read: from_email = from_email or settings.TEMPLATED_EMAIL_FROM_EMAIL or settings.DEFAULT_FROM_EMAIL
Also, check again the README, see if this is well explained. And check where TEMPLATED_EMAIL_FROM_EMAIL is being used.
There was a problem hiding this comment.
From what I've seen, this TEMPLATED_EMAIL_FROM_EMAIL is used only by the Class-based mixin. It's documentation is written here. While it's not saying explicitly that the setting is not required, it does not specify that it'll fall back to DEFAULT_FROM_MAIL. I think this could be updated.
I'm not sure, however, if we should put it as a default in the vanilla-django file, since this class-based mixin is like a wrapper over the vanilla and the docs specific that this setting is used only by the class-based view approach.
There was a problem hiding this comment.
I think we should make the backend use this setting TEMPLATED_EMAIL_FROM_EMAIL. So from_email = from_email or settings.TEMPLATED_EMAIL_FROM_EMAIL or settings.DEFAULT_FROM_EMAIL seems fine for me.
There was a problem hiding this comment.
@fjsj I've updated the vanilla backend and added a note about this behavior in the readme.
…ROM_EMAIL if no email_from is passed as an arg
| @@ -313,7 +313,7 @@ Attributes: | |||
| **templated_email_send_on_failure** (default: False): | |||
| This attribute tells django-templated-email to send an email if the form is invalid. | |||
|
|
|||
| **templated_email_from_email** (default: **settings.TEMPLATED_EMAIL_FROM_EMAIL**): | |||
| **templated_email_from_email** (default: **settings.TEMPLATED_EMAIL_FROM_EMAIL**. If it's also not defined, falls back to **DEFAULT_FROM_EMAIL**): | |||
There was a problem hiding this comment.
I think we need to add a section with all the available settings and their descriptions. Check the ones mentioned in README, check the ones used in code, and add a new section to README above "Class Based Views" section.
There was a problem hiding this comment.
I agree. Then I think this overrides list can be removed to concentrate the params in one place.
There was a problem hiding this comment.
This specific list should be kept because it's the params of the CBV.
There was a problem hiding this comment.
Oops, I've misread your previous text. Ignore my suggestion.
Resolves #97
The
contextandrecipient_listparams also had to be set asdefault=Noneotherwise it wouldn't be possible to setfrom_email=None.It doesn't change the order of the params, so it's not a breaking change.
It also removes the empty context params {} which are not required anymore.