From 150d5a9e18b83ec2525ac01a7b6a514925fde9fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 12 Feb 2014 16:27:37 +0100 Subject: [PATCH 1/6] [IMP] mail: mail_thread: message routing: raise exceptions instead of using asserts. Indeed assert are statements meant to be used when developping, for debug purpose. In a production environment it is safer to use real exceptions that can be managed accordingly. bzr revid: tde@openerp.com-20140212152737-c7q339psd9hi4iwd --- addons/mail/mail_thread.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/addons/mail/mail_thread.py b/addons/mail/mail_thread.py index 18486919977..edc1b6e7ba5 100644 --- a/addons/mail/mail_thread.py +++ b/addons/mail/mail_thread.py @@ -522,7 +522,8 @@ class mail_thread(osv.AbstractModel): does not reply to an existing thread and does not match any mail alias. :return: list of [model, thread_id, custom_values, user_id] """ - assert isinstance(message, Message), 'message must be an email.message.Message at this point' + if not isinstance(message, Message): + raise TypeError('message must be an email.message.Message at this point') message_id = message.get('Message-Id') email_from = decode_header(message, 'From') email_to = decode_header(message, 'To') @@ -593,9 +594,12 @@ class mail_thread(osv.AbstractModel): thread_id = int(thread_id) except: thread_id = False - assert thread_id and hasattr(model_pool, 'message_update') or hasattr(model_pool, 'message_new'), \ - "No possible route found for incoming message from %s to %s (Message-Id %s:)." \ - "Create an appropriate mail.alias or force the destination model." % (email_from, email_to, message_id) + if not (thread_id and hasattr(model_pool, 'message_update') or hasattr(model_pool, 'message_new')): + raise ValueError( + 'No possible route found for incoming message from %s to %s (Message-Id %s:).' + 'Create an appropriate mail.alias or force the destination model.' % + (email_from, email_to, message_id) + ) if thread_id and not model_pool.exists(cr, uid, thread_id): _logger.warning('Received mail reply to missing document %s! Ignoring and creating new document instead for Message-Id %s', thread_id, message_id) @@ -679,9 +683,11 @@ class mail_thread(osv.AbstractModel): context.update({'thread_model': model}) if model: model_pool = self.pool.get(model) - assert thread_id and hasattr(model_pool, 'message_update') or hasattr(model_pool, 'message_new'), \ - "Undeliverable mail with Message-Id %s, model %s does not accept incoming emails" % \ + if not (thread_id and hasattr(model_pool, 'message_update') or hasattr(model_pool, 'message_new')): + raise ValueError( + "Undeliverable mail with Message-Id %s, model %s does not accept incoming emails" % (msg['message_id'], model) + ) # disabled subscriptions during message_new/update to avoid having the system user running the # email gateway become a follower of all inbound messages @@ -692,7 +698,8 @@ class mail_thread(osv.AbstractModel): nosub_ctx = dict(nosub_ctx, mail_create_nolog=True) thread_id = model_pool.message_new(cr, user_id, msg, custom_values, context=nosub_ctx) else: - assert thread_id == 0, "Posting a message without model should be with a null res_id, to create a private message." + if thread_id: + raise ValueError("Posting a message without model should be with a null res_id, to create a private message.") model_pool = self.pool.get('mail.thread') new_msg_id = model_pool.message_post(cr, uid, [thread_id], context=context, subtype='mail.mt_comment', **msg) From e09f5cd2646e4ada2800bf4bc13f6ed4f3f6b10a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 12 Feb 2014 16:31:41 +0100 Subject: [PATCH 2/6] [FIX] mail: fixed tests, no more assertionerror, but valueerror bzr revid: tde@openerp.com-20140212153141-ajwmd26of69tfoer --- addons/mail/tests/test_mail_gateway.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addons/mail/tests/test_mail_gateway.py b/addons/mail/tests/test_mail_gateway.py index 8b10afe82f5..e7a22065ea1 100644 --- a/addons/mail/tests/test_mail_gateway.py +++ b/addons/mail/tests/test_mail_gateway.py @@ -411,14 +411,14 @@ class TestMailgateway(TestMailBase): # -------------------------------------------------- # Do: incoming email with model that does not accepts incoming emails must raise - self.assertRaises(AssertionError, + self.assertRaises(ValueError, format_and_process, MAIL_TEMPLATE, to='noone@example.com', subject='spam', extra='', model='res.country', msg_id='<1198923581.41972151344608186760.JavaMail.new4@agrolait.com>') # Do: incoming email without model and without alias must raise - self.assertRaises(AssertionError, + self.assertRaises(ValueError, format_and_process, MAIL_TEMPLATE, to='noone@example.com', subject='spam', extra='', From bc0232cee63c2dbc79e2d9b93c8c10e034bc385f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 12 Feb 2014 17:08:57 +0100 Subject: [PATCH 3/6] [FORWARD] [FIX] Forward port of revision 6920 of 6.1 addons branch. Revid: revid:odo@openerp.com-20120727150051-e1q0m92tyrazz82f Purpose: some fixes and more robuts handling of incoming emails bzr revid: tde@openerp.com-20140212160857-s889npk2q133xvy2 --- addons/fetchmail/fetchmail.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/addons/fetchmail/fetchmail.py b/addons/fetchmail/fetchmail.py index d408e7da9ac..ae3d60cee0a 100644 --- a/addons/fetchmail/fetchmail.py +++ b/addons/fetchmail/fetchmail.py @@ -198,6 +198,7 @@ openerp_mailgate: "|/path/to/openerp-mailgate.py --host=localhost -u %(uid)d -p result, data = imap_server.search(None, '(UNSEEN)') for num in data[0].split(): result, data = imap_server.fetch(num, '(RFC822)') + imap_server.store(num, '-FLAGS', '\\Seen') res_id = mail_thread.message_process(cr, uid, server.object_id.model, data[0][1], save_original=server.original, @@ -205,8 +206,8 @@ openerp_mailgate: "|/path/to/openerp-mailgate.py --host=localhost -u %(uid)d -p context=context) if res_id and server.action_id: action_pool.run(cr, uid, [server.action_id.id], {'active_id': res_id, 'active_ids':[res_id], 'active_model': context.get("thread_model", server.object_id.model)}) - imap_server.store(num, '+FLAGS', '\\Seen') - cr.commit() + imap_server.store(num, '+FLAGS', '\\Seen') + cr.commit() count += 1 _logger.info("fetched/processed %s email(s) on %s server %s", count, server.type, server.name) except Exception: From dca7f71d71250fdd3308dbea6fcbdeb3be1c733e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 13 Feb 2014 10:01:22 +0100 Subject: [PATCH 4/6] [DOC] mail_thread: docstrings to add raised exceptions in message_process and message_route bzr revid: tde@openerp.com-20140213090122-0gwin1pjigwu42sz --- addons/mail/mail_thread.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/addons/mail/mail_thread.py b/addons/mail/mail_thread.py index edc1b6e7ba5..030daaae1a9 100644 --- a/addons/mail/mail_thread.py +++ b/addons/mail/mail_thread.py @@ -521,6 +521,8 @@ class mail_thread(osv.AbstractModel): to which this mail should be attached. Only used if the message does not reply to an existing thread and does not match any mail alias. :return: list of [model, thread_id, custom_values, user_id] + + :raises: ValueError, TypeError """ if not isinstance(message, Message): raise TypeError('message must be an email.message.Message at this point') @@ -641,6 +643,8 @@ class mail_thread(osv.AbstractModel): to which this mail should be attached. When provided, this overrides the automatic detection based on the message headers. + + :raises: ValueError, TypeError """ if context is None: context = {} From ec5291c130416164d3e6f39ddd2b2da7eb208f1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 13 Feb 2014 10:57:17 +0100 Subject: [PATCH 5/6] [IMP] fetchmail: try / except the processinf of each email and log any exception found during processing. This way, all emails in the queue are managed and all failed emails have their own exception in the logs, allowing easier debugging. Note that a failed email is set as seen to avoid processing it every time the scheduler runs. bzr revid: tde@openerp.com-20140213095717-tcwgkl143i3ujw8h --- addons/fetchmail/fetchmail.py | 41 +++++++++++++++++++++-------------- addons/mail/mail_thread.py | 2 +- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/addons/fetchmail/fetchmail.py b/addons/fetchmail/fetchmail.py index ae3d60cee0a..5bd360390da 100644 --- a/addons/fetchmail/fetchmail.py +++ b/addons/fetchmail/fetchmail.py @@ -188,7 +188,7 @@ openerp_mailgate: "|/path/to/openerp-mailgate.py --host=localhost -u %(uid)d -p for server in self.browse(cr, uid, ids, context=context): _logger.info('start checking for new emails on %s server %s', server.type, server.name) context.update({'fetchmail_server_id': server.id, 'server_type': server.type}) - count = 0 + count, failed = 0, 0 imap_server = False pop_server = False if server.type == 'imap': @@ -197,21 +197,26 @@ openerp_mailgate: "|/path/to/openerp-mailgate.py --host=localhost -u %(uid)d -p imap_server.select() result, data = imap_server.search(None, '(UNSEEN)') for num in data[0].split(): + res_id = None result, data = imap_server.fetch(num, '(RFC822)') imap_server.store(num, '-FLAGS', '\\Seen') - res_id = mail_thread.message_process(cr, uid, server.object_id.model, - data[0][1], - save_original=server.original, - strip_attachments=(not server.attach), - context=context) + try: + res_id = mail_thread.message_process(cr, uid, server.object_id.model, + data[0][1], + save_original=server.original, + strip_attachments=(not server.attach), + context=context) + except Exception: + _logger.exception('Failed to process mail from %s server %s.', server.type, server.name) + failed += 1 if res_id and server.action_id: - action_pool.run(cr, uid, [server.action_id.id], {'active_id': res_id, 'active_ids':[res_id], 'active_model': context.get("thread_model", server.object_id.model)}) + action_pool.run(cr, uid, [server.action_id.id], {'active_id': res_id, 'active_ids': [res_id], 'active_model': context.get("thread_model", server.object_id.model)}) imap_server.store(num, '+FLAGS', '\\Seen') cr.commit() count += 1 - _logger.info("fetched/processed %s email(s) on %s server %s", count, server.type, server.name) + _logger.info("Fetched %d email(s) on %s server %s; %d succeeded, %d failed.", count, server.type, server.name, (count - failed), failed) except Exception: - _logger.exception("Failed to fetch mail from %s server %s.", server.type, server.name) + _logger.exception("General fallure when trying to fetch mail from %s server %s.", server.type, server.name) finally: if imap_server: imap_server.close() @@ -224,16 +229,20 @@ openerp_mailgate: "|/path/to/openerp-mailgate.py --host=localhost -u %(uid)d -p for num in range(1, numMsgs + 1): (header, msges, octets) = pop_server.retr(num) msg = '\n'.join(msges) - res_id = mail_thread.message_process(cr, uid, server.object_id.model, - msg, - save_original=server.original, - strip_attachments=(not server.attach), - context=context) + try: + res_id = mail_thread.message_process(cr, uid, server.object_id.model, + msg, + save_original=server.original, + strip_attachments=(not server.attach), + context=context) + except Exception: + _logger.exception('Failed to process mail from %s server %s.', server.type, server.name) + failed += 1 if res_id and server.action_id: - action_pool.run(cr, uid, [server.action_id.id], {'active_id': res_id, 'active_ids':[res_id], 'active_model': context.get("thread_model", server.object_id.model)}) + action_pool.run(cr, uid, [server.action_id.id], {'active_id': res_id, 'active_ids': [res_id], 'active_model': context.get("thread_model", server.object_id.model)}) pop_server.dele(num) cr.commit() - _logger.info("fetched/processed %s email(s) on %s server %s", numMsgs, server.type, server.name) + _logger.info("Fetched %d email(s) on %s server %s; %d succeeded, %d failed.", numMsgs, server.type, server.name, (numMsgs - failed), failed) except Exception: _logger.exception("Failed to fetch mail from %s server %s.", server.type, server.name) finally: diff --git a/addons/mail/mail_thread.py b/addons/mail/mail_thread.py index 030daaae1a9..40a4fefa8a8 100644 --- a/addons/mail/mail_thread.py +++ b/addons/mail/mail_thread.py @@ -598,7 +598,7 @@ class mail_thread(osv.AbstractModel): thread_id = False if not (thread_id and hasattr(model_pool, 'message_update') or hasattr(model_pool, 'message_new')): raise ValueError( - 'No possible route found for incoming message from %s to %s (Message-Id %s:).' + 'No possible route found for incoming message from %s to %s (Message-Id %s:). ' 'Create an appropriate mail.alias or force the destination model.' % (email_from, email_to, message_id) ) From 0f111f7b9656fa545dc068bf192ece06c92e87fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 13 Feb 2014 12:29:46 +0100 Subject: [PATCH 6/6] [FIX] fetchmail: typos in logger messages bzr revid: tde@openerp.com-20140213112946-i351aazw4r2ezfz4 --- addons/fetchmail/fetchmail.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addons/fetchmail/fetchmail.py b/addons/fetchmail/fetchmail.py index 5bd360390da..3402241bf5d 100644 --- a/addons/fetchmail/fetchmail.py +++ b/addons/fetchmail/fetchmail.py @@ -216,7 +216,7 @@ openerp_mailgate: "|/path/to/openerp-mailgate.py --host=localhost -u %(uid)d -p count += 1 _logger.info("Fetched %d email(s) on %s server %s; %d succeeded, %d failed.", count, server.type, server.name, (count - failed), failed) except Exception: - _logger.exception("General fallure when trying to fetch mail from %s server %s.", server.type, server.name) + _logger.exception("General failure when trying to fetch mail from %s server %s.", server.type, server.name) finally: if imap_server: imap_server.close() @@ -244,7 +244,7 @@ openerp_mailgate: "|/path/to/openerp-mailgate.py --host=localhost -u %(uid)d -p cr.commit() _logger.info("Fetched %d email(s) on %s server %s; %d succeeded, %d failed.", numMsgs, server.type, server.name, (numMsgs - failed), failed) except Exception: - _logger.exception("Failed to fetch mail from %s server %s.", server.type, server.name) + _logger.exception("General failure when trying to fetch mail from %s server %s.", server.type, server.name) finally: if pop_server: pop_server.quit()