Opened 7 years ago

Closed 3 years ago

#11400 closed New feature (fixed)

Pass kwargs from AbstractUser.email_user() to send_mail()

Reported by: jug Owned by: Susan Tan
Component: contrib.auth Version: master
Severity: Normal Keywords: email user fail_silently
Cc: susan.tan.fleckerl@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no


Add fail_silently=False to User.email_user and pass it to send_mail.

Attachments (1)

r11798.diff (1.5 KB) - added by John-Scott Atlakson 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Changed 7 years ago by John-Scott Atlakson

Attachment: r11798.diff added

comment:2 Changed 7 years ago by John-Scott Atlakson

Has patch: set

Added patch so email_user() passes kwargs to send_mail.

comment:3 Changed 6 years ago by Gabriel Hurley

Component: Contrib appscontrib.auth

comment:4 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: New feature

comment:5 Changed 5 years ago by Julien Phalip

Needs tests: set

comment:6 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 Changed 4 years ago by Klaas van Schelven

Just weighing in on a ticket that seems to be living in limbo for three years at this point:

This should either be 'design decision needed', or be quickly resolved, since it seems rather trivial to me. What's needed to move this forward?

Personally, I don't think passing **kwargs around is the pinnacle of elegance. Are there more kwargs that we're interested in, or is it really just fail_silently?

As a final thought, if we consider this a desirable feature, maybe fail_silently should be customizable in more cases. Specifically, it's not easy to customize the password_reset view to have a different value for fail_silently without subclassing and completely copy/pasting PasswordResetForm. Maybe that's a separate ticket?

comment:9 Changed 3 years ago by Tim Graham

Easy pickings: set
Needs documentation: set

The patch needs to be updated to apply cleanly. It also needs tests (doesn't look like email_user is tested at all) - probably in django/contrib/auth/tests/ The doc in the current patch (docs/topics/auth.txt) has moved to docs/ref/contrib/auth.txt) and a mention in the release notes is also needed.

comment:10 Changed 3 years ago by Susan Tan

@timo, The only aspect that is the most confusing to me is the unit test. How do I test that the email sent to a user was a successful event? I see that email_user doesn't return anything from a quick read of the code. In principle, I picture that the new test inside UserManagerTestCase(TestCase) in the tests/ should look like this:

def test_send_email(self):
        keyword_args = {"fail_silently":False, "auth_user":None, "auth_password":None,
              "connection":None, "html_message":None}
        sucesss_indicator = UserManager.send_email(subject="This is subject",
            message="This is a message", from_email="",
            recipient_list="", **keyword_args)
        self.assertEqual(sucesss_indicator, "<expected output of success>")

What do you think? I'm not sure what the success_indicator is, the expected output for send_email function.
Can you point me in the right direction? Are there any other testing considerations that I missed?

comment:11 Changed 3 years ago by Tim Graham

email_user() is a method on AbstractUser not UserManager, otherwise the test looks like a reasonable start.

You can read about testing email here:

comment:12 Changed 3 years ago by Susan Tan

@timo, Thanks for the reference. I've written the test. I'm used to running tests on the django/tests directory. But in this case, the test is in django/contrib/auth/tests/ How do I test this file? The auth/tests direcotry does not have a

I've referred to this doc for running tests:

Below is the new test:

class AbstractUserTestCase(TestCase):
    def test_send_email(self):
        keyword_args = {"fail_silently":False, "auth_user":None, "auth_password":None,
              "connection":None, "html_message":None}
        AbstractUser.send_email(subject="Subject here",
            message="This is a message", from_email="",
            recipient_list="", **keyword_args)

        # Test that one message has been sent.
        self.assertEqual(self.assertEqual(len(mail.outbox), 1))

        # Verify that the subject of the first message is correct.
        self.assertEqual(mail.outbox[0].subject, 'Subject here')

comment:13 Changed 3 years ago by Tim Graham django.contrib.auth should work to run the test.

Regarding the test itself, I would format the dictionary a bit differently (space after colon, each key/value pair on a separate line).

kwargs = {
    "fail_silently": False,

Also, I don't think this test class requires the @skipIfCustomUser decorator. You'll need to instantiate an AbstractUser rather than calling send_email directly as it isn't a classmethod.

comment:14 Changed 3 years ago by Susan Tan

I have a PR ready for your review and testing:

Feedback is welcome, as always. I've commented out the email_user function in django.contrib.auth and ran the new test, which failed. Then, I un-commented out the email_user function and the tests passed, both unit test and full test suite.

comment:15 Changed 3 years ago by Susan Tan

Cc: susan.tan.fleckerl@… added
Needs tests: unset
Owner: changed from nobody to Susan Tan
Status: newassigned

comment:16 Changed 3 years ago by Tim Graham

Needs documentation: unset
Patch needs improvement: set
Summary: Add fail_silently parameter to User.email_userPass kwargs from AbstractUser.email_user() to send_mail()

Comments on PR.

comment:17 Changed 3 years ago by Susan Tan

All comments incorporated. See updated PR: ​ Let me know if there's still any more feedback.

comment:18 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 71c491972eecae8783cf46e69fac7e5f9f83fc59:

Fixed #11400 -- Passed kwargs from AbstractUser.email_user() to send_mail()

Thanks Jug_ for suggestion, john_scott for the initial patch,
and Tim Graham for code review.

Note: See TracTickets for help on using tickets.
Back to Top