[Django] #20832: Add html password reset email
Django
------------------------------+--------------------
Reporter: jmichalicek | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+--------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20832>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jmichalicek | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by russellm):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
Seems like a good idea to me, as long as it's opt-in and implemented in a
backwards compatible way.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:1>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jmichalicek):
* status: new => assigned
* owner: nobody => jmichalicek
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:2>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jmichalicek):
Just a quick update, actual changes are done. I'm hoping to knock out
test cases and documentation on Aug 1.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:3>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
#17431 is related and might be useful to consider.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:4>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jmichalicek):
Replying to [comment:4 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:5>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:6>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jmichalicek):
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:
{{{#!python
@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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:7>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jmichalicek):
Sounds good. I can create a new ticket and get those documented first on
that ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:9>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:8>
Django
Reporter: jmichalicek | Owner: jmichalicek
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jmichalicek):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:10>
Django
Reporter: jmichalicek | Owner: jmichalicek
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6d88d47be6d37234aab86d0e863e371f28347d12"]:
{{{
#!CommitTicketReference repository=""
revision="6d88d47be6d37234aab86d0e863e371f28347d12"
Fixed #20832 -- Enabled HTML password reset email
Added optional html_email_template_name parameter to password_reset view
and PasswordResetForm.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20832#comment:11>