Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#9383 closed (fixed)

skip mail_admins/mail_managers if ADMINS or MANAGERS is empty

Reported by: Jesse Young Owned by: nobody
Component: Core (Other) Version: 1.0
Severity: Keywords: 500 error mail_admins
Cc: Malcolm Tredinnick Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As discussed on http://groups.google.com/group/django-developers/browse_thread/thread/5c27ac4703da33a9, when DEBUG = False, internal server errors will always cause mail_admins to be called (unless overridden in a custom handler class), and the current implementation of mail_admins always opens a connection to the SMTP server even if the email has no recipients (i.e., ADMINS=[]).

This patch makes mail_admins and mail_managers a no-op if there are no recipients. It also avoids constructing the internal server error email if it has no recipients.

Attachments (1)

mail_admins.diff (4.4 KB ) - added by Jesse Young 16 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by James Bennett, 16 years ago

Component: HTTP handlingCore framework
Needs documentation: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Seems like a good idea; this needs documentation, though (to explain the behavior when ADMINS and/or MANAGERS is empty), and potentially unit tests (though I'm not certain how you'd go about testing this behavior).

comment:2 by Jesse Young, 16 years ago

In regards to documentation, I'm not sure what there is to document, or where to document it. The only functional difference is that instead of opening a connection to the SMTP server and then immediately closing it without sending anything, it just doesn't do anything at all.

As far as unit tests, perhaps something like this?

settings.ADMINS = []
old_smtp_connection = django.core.mail.SMTPConnection
django.core.mail.SMTPConnection = None # overwrite it with something that would fail if we tried to use it
mail_admins('hi','there') # should fail if it actually tries to make an SMTP connection
django.core.mail.SMTPConnection = old_smtp_connection # clean up

by Jesse Young, 16 years ago

Attachment: mail_admins.diff added

comment:3 by Jesse Young, 16 years ago

Needs documentation: unset
Patch needs improvement: unset

okay, my new diff includes a regression test that fails before and passes after, as well as a minor update to the documentation for mail_admins.

comment:4 by Malcolm Tredinnick, 16 years ago

I'm not going to worry about the documentation change, since whether or not a network connection is made is an implementation detail. It doesn't affect user-visible functionality. I'm also dropping the change to core/handlers/, since the overhead isn't really significant and it's breaking encapsulation a bit (the code at that level doesn't care about settings.ADMIN; it just calls mail_admins() and is done with it).

I've introduced an extra bit to the EmailMessage class so that it also doesn't create the network connection if there's no intended recipients. After all, that's really the more extensible interface for sending email, so we should be consistent and make it possible there. I've still kept the checks for settings.ADMIN and settings.MANAGERS in place, although it's a bit borderline. It's only two lines of code in each case and we already use the settings in those functions anyway.

comment:5 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [9248]) Fixed #9383 -- Don't open a network connection for sending email if there's
nothing to send. Saves a bit of time when, for example, processing 500-error
emails with no ADMINs configured. Based on a patch from Jesse Young.

comment:6 by Malcolm Tredinnick, 16 years ago

(In [9250]) [1.0.X] Fixed #9383 -- Don't open a network connection for sending email if
there's nothing to send. Saves a bit of time when, for example, processing
500-error emails with no ADMINs configured. Based on a patch from Jesse Young.

Backport of r9248 from trunk.

comment:7 by Malcolm Tredinnick, 16 years ago

(In [9253]) [1.0.X] Backed out r9250. I committed this to the branch by mistake; there's no
bug in functionality fixed by this. Refs #9383.

comment:8 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

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