Opened 6 years ago

Closed 4 years ago

#5081 closed defect (fixed)

Unhelpful SMTP error

Reported by: Matt Molyneaux Owned by:
Priority: trivial Milestone:
Component: programming Keywords: bitesized
Cc: Parent Tickets:

Description

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

Subtickets

Attachments (3)

traceback.txt (3.4 KB) - added by Ben Sturmfels 5 years ago.
issue_5081.patch (1.8 KB) - added by jsandoval 5 years ago.
issue_5081_rev1.patch (6.9 KB) - added by jsandoval 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by Matt Molyneaux

Keywords: bitesized added; unhelpful removed

comment:2 Changed 5 years ago by 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

Changed 5 years ago by Ben Sturmfels

Attachment: traceback.txt added

Changed 5 years ago by jsandoval

Attachment: issue_5081.patch added

comment:3 Changed 5 years ago by 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.

Last edited 5 years ago by jsandoval (previous) (diff)

comment:4 Changed 5 years ago by Matt Molyneaux

Status: newreview

comment:5 Changed 5 years ago by Matt Molyneaux

Owner: set to jsandoval
Status: reviewin_progress

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!

Changed 5 years ago by jsandoval

Attachment: issue_5081_rev1.patch added

comment:6 Changed 5 years ago by 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 :)

comment:7 Changed 5 years ago by jsandoval

Owner: jsandoval deleted
Status: in_progressreview

comment:8 Changed 5 years ago by Matt Molyneaux

Owner: set to jsandoval
Status: reviewin_progress

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 :)

comment:9 Changed 5 years ago by 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.

comment:10 Changed 5 years ago by jsandoval

Owner: jsandoval deleted
Status: in_progressreview

comment:11 Changed 5 years ago by Matt Molyneaux

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

comment:12 Changed 4 years ago by jsandoval

Ok. Many thanks :)

comment:13 Changed 4 years ago by Boris Bobrov

Resolution: fixed
Status: reviewclosed

thank you, merged in 4fcd2e7

Note: See TracTickets for help on using tickets.