Opened 11 years ago
Closed 11 years ago
#20832 closed New feature (fixed)
Add html password reset email
Reported by: | Justin Michalicek | Owned by: | Justin Michalicek |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I would like django.contrib.auth.views.password_reset and the associated django.contrib.auth.forms.PasswordResetForm to be able to send an optional multipart text/html email. This could be done by adding an optional html_email_template parameter to the view's parameters and to the form's save() method.
This was discussed on google groups in this thread: https://groups.google.com/forum/#!topic/django-developers/_mXKP9JZzK0 (the other change mentioned there was already accepted and implemented)
I intend to implement the change on https://github.com/jmichalicek/django/commits/add_html_password_reset_email
Change History (11)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 11 years ago
Just a quick update, actual changes are done. I'm hoping to knock out test cases and documentation on Aug 1.
comment:5 by , 11 years ago
Replying to timo:
#17431 is related and might be useful to consider.
Nice find. I do like the idea of splitting the email construction out to a separate method to make it easier for someone to do something completely different if they need to, I may have to borrow that idea.
I also really like the idea that someone mentioned on that ticket about being able to pass additional template context into PasswordResetForm.save() and it's a very small change. I'm not sure if that would be considered outside of the scope of this ticket or not.
comment:6 by , 11 years ago
It's generally easiest to split up distinct ideas into smaller commits and separate tickets as it makes review a bit easier, but use your best judgement.
comment:7 by , 11 years ago
I've got this ready to go. I've left the splitting things out for a later ticket for now. I do have one question before I make a pull request.
I've added an html_email_template_name as the last param for django.contrib.auth.views.password_reset like so:
@csrf_protect def password_reset(request, is_admin_site=False, template_name='registration/password_reset_form.html', email_template_name='registration/password_reset_email.html', subject_template_name='registration/password_reset_subject.txt', password_reset_form=PasswordResetForm, token_generator=default_token_generator, post_reset_redirect=None, from_email=None, current_app=None, extra_context=None, html_email_template_name=None):
The documentation doesn't list current_app or extra_context as parameters to the view. Since I did add html_email_template_name, should I move it directly after the from_email parameter so that the parameter order matches what is documented in case someone just passes unnamed args based on the docs? Or should the current_app and extra_context params actually be documented and show in the parameter list as well?
comment:8 by , 11 years ago
current_app
and extra_context
should be documented. It would be great to audit the two commits that added these changes [07705ca1] and [cc64fb5c] as those parameters were added elsewhere, but not documented. Would you like to do that or should we make that a separate ticket? Ideally, we'd merge that first in a separate commit from this ticket.
comment:9 by , 11 years ago
Sounds good. I can create a new ticket and get those documented first on that ticket.
comment:10 by , 11 years ago
Just sent the pull request for https://github.com/jmichalicek/django/tree/add_html_password_reset_email
I did not use any ideas from #17431 just to keep the size/scope down.
Some of the test case bits may be redundant such as checking for is_multipart() and also checking the length of outbox[0].alternatives but I left it like that since I've never been bit by too much testing.
comment:11 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Seems like a good idea to me, as long as it's opt-in and implemented in a backwards compatible way.