Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16736 closed Bug (fixed)

AdminEmailHandler ignores user args to log message.

Reported by: saverio.trioni@… Owned by: strioni
Component: Core (Other) Version: 1.3
Severity: Normal Keywords: logging
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

When using AdminEmailHandler to log own code, the mail subject and content has the raw LogRecord msg attribute instead of the LogRecord.getMessage() merged string.

This appears when using the standard log API

foo, bar = 'hello', 'world'
logger.error("The configuration is %s %s", foo, bar)

The email subject should be "The configuration is hello world" (barring configured prefixes), but is "The configuration is %s %s". The same happens to the email content.

This can be solved using record.getMessage() instead of record.msg to get the message.

Attachments (4)

adminemail_getmessage_patch.2.diff (2.3 KB) - added by strioni 4 years ago.
adminemail_getmessage_patch.3.diff (1.5 KB) - added by strioni 4 years ago.
Sorry, there was a typo due to excessive vimming…
adminemail_getmessage_patch.diff (2.3 KB) - added by strioni 4 years ago.
This is the final patch, ignore the other ones.
16736.adminemailhandler-formatted-subject.diff (2.8 KB) - added by julien 4 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by strioni

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset

I find it difficult to write a regression test for this, as the AdminEmailHandler.emit is sending an email directly. Anyway a patch is attached.

comment:2 Changed 4 years ago by strioni

  • Owner changed from nobody to strioni
  • Status changed from new to assigned

comment:3 Changed 4 years ago by julien

  • Component changed from Uncategorized to Core (Other)
  • Triage Stage changed from Unreviewed to Accepted

This makes sense. For testing, maybe look at customising the send_log method in [1]. See also [2] for how to test emails.

[1] source:django/trunk/tests/regressiontests/views/views.py?rev=16606#L139

[2] https://docs.djangoproject.com/en/dev/topics/testing/#email-services

Changed 4 years ago by strioni

comment:4 Changed 4 years ago by strioni

New patch, with regression test.

Changed 4 years ago by strioni

Sorry, there was a typo due to excessive vimming...

Changed 4 years ago by strioni

This is the final patch, ignore the other ones.

comment:5 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Looks good to me. I've just tweaked the test a little.

comment:6 follow-up: Changed 4 years ago by strioni

  • Needs tests unset

Seems good to me, I didn't know about the override_settings decorator.

Just one point, should the EMAIL_SUBJECT_PREFIX setting be overridden too, in case the test is run with alternate settings module?

comment:7 in reply to: ↑ 6 Changed 4 years ago by julien

Replying to strioni:

Just one point, should the EMAIL_SUBJECT_PREFIX setting be overridden too, in case the test is run with alternate settings module?

Yes that's a good idea. Feel free to update the patch, otherwise it can be done later on commit.

comment:8 Changed 4 years ago by julien

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

In [16715]:

Fixed #16736 -- Enabled the merging of user-supplied arguments to format the error emails' subject in AdminEmailHandler.

comment:9 Changed 4 years ago by julien

Thank you strioni for the report and patch, and really sorry that I forgot to mention you in the commit message.

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