Unhelpful SMTP error

[Matt Molyneaux]

We had a user in the IRC channel who had an issue with getting email verification setup. Based on the error they received¹ they assumed that MediaGoblin could route mail to remote hosts on its own². They were slightly confused by all this.

We should catch such errors and provide a more descriptive error that doesn't require the reading of Python tracebacks to understand. Something like "Couldn't contact mail server on <host>:<port>" would be far more helpful.

Feel free to contact me on IRC (moggers87) if you're thinking of tackling this ticket and are unfamiliar with MediaGoblin's internals.

¹: ​http://pastebin.ca/2965561
²: it cannot

[Ben Sturmfels]

Just reviewing this ticket and noticed that the Pastbin link provided has expired. To reproduce it, I've turned off my mail server, then attempted to register (which sends out the verification email), producing the attached traceback: error: [Errno 111] Connection refused.

Regards,
Ben

[jsandoval]

I just added a patch for this issue. It's my first try, so I'll be expecting suggestions and corrections.

I basically catched the exceptions and printed the suggested message.

Thanks.

[Matt Molyneaux]

Thanks for the patch! Here's a quick review:

• Rather than printing (that might not turn up in error logs) we should raise our own exception here
• Suggestion (by no means required): log the original exception via logging.debug
• Tests!

[jsandoval]

I just added a new patch.
I added two tests, created a custom exception with the message and logged to the debug level in logging.

I'll be expecting suggestions and corrections :)

[Matt Molyneaux]

Sorry for the wait! A few more things:

• On line 124, there's some unneeded whitespace
• Tests should go in mediagoblin/tests/test_utils.py with the other mail tests
• You should put the original exception's message into the debug log - otherwise you won't know what's actually happening if you go to debug things later. Something like this:

    except socket.error as original_error:
        error_message = "Couldn't contact mail server on <{}>:<{}>".format(
             mg_globals.app_config['email_smtp_host'],
             mg_globals.app_config['email_smtp_port'])
         logging.debug("Socket error: %s", original_error)
         raise NoSMTPServerError(error_message)

Thanks for working on this :)

[jsandoval]

I think I applied the suggestions. I pushed it to a git repository in: ​https://github.com/jsandovalc/mediagoblin/tree/fix-unhelpful-smtp-error-5081

The branch is: fix-unhelpful-smtp-error-5081

Thanks for the review. Let me know if more changes are needed.

[Matt Molyneaux]

Looks good to me! I'll ping someone with commit rights to get this merged.

[jsandoval]

Ok. Many thanks :)

[Boris Bobrov]

thank you, merged in 4fcd2e7