Re: [Django] #11400: Add fail_silently parameter to User.email_user

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by vanschelven):

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?

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted

fail_silently | Needs documentation: 1

Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0

Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by timo):

* needs_docs: 0 => 1
* easy: 0 => 1

Comment:

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/test_models.py`. 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:9>

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Comment (by susan):

@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/test_models.py` 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="sus...@domain.com",
recipient_list="sus...@domain.com", **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?

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:10>

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Comment (by timo):

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

You can read about testing email here:
https://docs.djangoproject.com/en/dev/topics/testing/overview/#email-
services

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:11>

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Comment (by susan):

@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/test_models.py`. How do I test this file?
The auth/tests direcotry does not have a runtests.py.

I've referred to this doc for running tests:
https://docs.djangoproject.com/en/1.5/intro/contributing/#running-your-
new-test

Below is the new test:
{{{
@skipIfCustomUser
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="fr...@domain.com",
recipient_list="t...@domain.com", **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')
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:12>

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Comment (by timo):

`runtests.py 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`.

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:13>

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Comment (by susan):

I have a PR ready for your review and testing:
https://github.com/django/django/pull/1469

Feedback is welcome, as always. I've commented out the email_user function
in `django.contrib.auth` models.py 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:14>

[Django]

#11400: Add fail_silently parameter to User.email_user
-------------------------------------+-------------------------------------

Reporter: Jug_ | Owner: susan
Type: New feature | Status: assigned

Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0

Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by susan):

* cc: susan.tan.fleckerl@… (added)
* owner: nobody => susan
* status: new => assigned
* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:15>

[Django]

#11400: Pass kwargs from AbstractUser.email_user() to send_mail()

-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: susan
Type: New feature | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted

fail_silently | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1

Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Changes (by timo):

* needs_better_patch: 0 => 1
* needs_docs: 1 => 0

Comment:

Comments on PR.

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:16>

[Django]

#11400: Pass kwargs from AbstractUser.email_user() to send_mail()
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: susan
Type: New feature | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Comment (by susan):

All comments incorporated. See updated PR:
https://github.com/django/django/pull/1469 Let me know if there's still
any more feedback.

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:17>

[Django]

#11400: Pass kwargs from AbstractUser.email_user() to send_mail()
-------------------------------------+-------------------------------------
Reporter: Jug_ | Owner: susan

Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed

Keywords: email user | Triage Stage: Accepted
fail_silently | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Changes (by Tim Graham <timograham@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"71c491972eecae8783cf46e69fac7e5f9f83fc59"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/11400#comment:18>