Code

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#9383 closed (fixed)

skip mail_admins/mail_managers if ADMINS or MANAGERS is empty

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

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 adunar 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by ubernostrum

  • Component changed from HTTP handling to Core framework
  • Needs documentation set
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 6 years ago by adunar

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

Changed 6 years ago by adunar

comment:3 Changed 6 years ago by adunar

  • 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 Changed 6 years ago by mtredinnick

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 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(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 Changed 6 years ago by mtredinnick

(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 Changed 6 years ago by mtredinnick

(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 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.