Improve error reporting when subscribing to a newsletter #36

Closed
jleclanche opened this Issue Nov 6, 2012 · 5 comments

Comments

Projects
None yet
2 participants
Contributor

jleclanche commented Nov 6, 2012

One of my client's sites runs SES in production to send emails. It's using ConsoleBackend in debug mode.

If, for any reason, SES cannot send the subscription email (servers busy, bad API key, address not verified...), django-newsletter displays an error page. However, the error is not reported through Django's error report system.

This is bad; either django-newsletter should let django handle the error (and display a generic 500 page; i'd recommend doing it this way) or it should report the error itself somehow.

Owner

dokterbob commented Nov 19, 2012

First: let's make sure we're talking about the same code: https://github.com/dokterbob/django-newsletter/blob/master/newsletter/views.py#L181

Actually, we had some problems with email submissions as well - once. This is why the email submission logs an exception - when something like Sentry is used to parse error messages this yields a full stack trace.

However, on first sight, it looks like we shouldn't be catching errros here at all - and rather make sure a 500 is raised. I wonder why I did that (a long ... long time ago) and will take some time to figure it out.

Meanwhile, does not having the email submission in a try...except clause makes sense to you? Any chance you could perhaps give it a go in some kind of (testing, staging... production?) environment?

Owner

dokterbob commented Nov 19, 2012

Looking more thoroughly at the code, I remember the error template variable being set to True when the email fails.

The reason for this was that ofter people make a typo in their e-mail, causing subsequent submission to fail (some mail servers are configured not to accept mail for non-existent domains). Hence, these users are helped a great deal with a notification stating that submission of the email failed over a generic 500-page.

I reckon this is not clear in the current state of the code. Hence, I'd like to propose the following:

  1. Add explanatory comments to the code about this use case and behaviour.
  2. Change error to something more meaningful such as email_error.
  3. Make sure the default templates make use of this variable to display a human-friendly email failure notification.

Does this sound sensible or might there be something I'm missing?

Contributor

jleclanche commented Nov 19, 2012

It makes a lot of sense to me not to be catching the error OR only be catching the specific errors you're looking for and re-raising those once you handle them.

Owner

dokterbob commented Nov 19, 2012

This turns out to be related to #12 and I'm closing it as a dub for now. (I'm not dismissing the issue - just think we should move discussion there.)

dokterbob closed this Nov 19, 2012

Contributor

jleclanche commented Nov 19, 2012

Alright, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment