[IMP] Cleanup and refactoring of exception handling

Unify and refactor exception handling in framework and addons.

The generic `except_osv` is now deprecated, and replaced by more specialized exception subtypes:
 - `UserError` (renamed from Warning, as it conflicts with the built-in `Warning`) raised when a non-technical error occurs during a business operation. It could be a missing information in the data provided by the user, or a misconfiguration.
 - `AccessError`: raised when any operation is denied because the user conducting it does not have the required access rights.
 - `AccessDenied`: raised when an operation that requires authenticated access is attempted via an unauthenticated request.
 - `MissingError`: raised when an operation is attempted on a record that does not exist.
 - `ValidationError`: raised when an operation violates a SQL or Python constraint.
 - All other exceptions are internal errors due to a system problem or bug, and raised untouched to the client-side, which should display a traceback.

All exceptions take a single message argument.

The `test_exceptions` module has been updated to showcase both new and old (deprecated) exceptions.

A great many old `except_osv` had a useless title with "Error!" or "Warning", those have been removed, as this is handled by the client-side widget that displays the messages.

This commit introduces a more consistent policy for logging errors and warnings:
 - All messages that do not require administrator attention should be logged at INFO level or lower. This includes all errors that are notified to the user in a friendly manner, even for access right problems or validation errors during business operations.
 - All messages that indicate a likely misconfiguration or malicious use by the users should be logged at WARNING level, as they typically require administrator attention.
 - All other unhandled internal errors cannot typically be handled by the user and should be logged at ERROR or higher level, as they require immediate administrator attention.

8 files changed, 32 insertions, 28 deletions

diff --git a/addons/mail/mail_group.py b/addons/mail/mail_group.py
index 8ef5b56a44c..4f30a16bbf7 100644
--- a/addons/mail/mail_group.py
+++ b/addons/mail/mail_group.py
@@ -26,6 +26,7 @@ from openerp.osv import fields
 from openerp.tools.safe_eval import safe_eval as eval
 from openerp import SUPERUSER_ID
 from openerp.tools.translate import _
+from openerp.exceptions import UserError
 
 class mail_group(osv.Model):
     """ A mail_group is a collection of users sharing messages in a discussion
@@ -171,7 +172,7 @@ class mail_group(osv.Model):
         except ValueError:
             all_emp_group = None
         if all_emp_group and all_emp_group in ids:
-            raise osv.except_osv(_('Warning!'), _('You cannot delete those groups, as the Whole Company group is required by other modules.'))
+            raise UserError(_('You cannot delete those groups, as the Whole Company group is required by other modules.'))
         res = super(mail_group, self).unlink(cr, uid, ids, context=context)
         # Cascade-delete mail aliases as well, as they should not exist without the mail group.
         self.pool.get('mail.alias').unlink(cr, SUPERUSER_ID, alias_ids, context=context)
diff --git a/addons/mail/mail_mail.py b/addons/mail/mail_mail.py
index 0034c15a87b..5a54de14094 100644
--- a/addons/mail/mail_mail.py
+++ b/addons/mail/mail_mail.py
@@ -313,7 +313,7 @@ class mail_mail(osv.Model):
                             # mail item -> ignore error to avoid blocking
                             # delivery to next recipients, if any. If this is
                             # the only recipient, the mail will show as failed.
-                            _logger.warning("Ignoring invalid recipients for mail.mail %s: %s",
+                            _logger.info("Ignoring invalid recipients for mail.mail %s: %s",
                                             mail.message_id, email.get('email_to'))
                         else:
                             raise
diff --git a/addons/mail/mail_message.py b/addons/mail/mail_message.py
index fe433657077..2780a659008 100644
--- a/addons/mail/mail_message.py
+++ b/addons/mail/mail_message.py
@@ -26,10 +26,11 @@ from openerp import tools
 from email.header import decode_header
 from email.utils import formataddr
 from openerp import SUPERUSER_ID, api
-from openerp.osv import osv, orm, fields
+from openerp.osv import osv, fields
 from openerp.tools import html_email_clean
 from openerp.tools.translate import _
 from HTMLParser import HTMLParser
+from openerp.exceptions import UserError, AccessError
 
 _logger = logging.getLogger(__name__)
 
@@ -174,7 +175,7 @@ class mail_message(osv.Model):
             return formataddr((this.name, '%s@%s' % (this.alias_name, this.alias_domain)))
         elif this.email:
             return formataddr((this.name, this.email))
-        raise osv.except_osv(_('Invalid Action!'), _("Unable to send email, please configure the sender's email address or alias."))
+        raise UserError(_("Unable to send email, please configure the sender's email address or alias."))
 
     def _get_default_author(self, cr, uid, context=None):
         return self.pool.get('res.users').browse(cr, SUPERUSER_ID, uid, context=context).partner_id.id
@@ -770,10 +771,10 @@ class mail_message(osv.Model):
         other_ids = other_ids.difference(set(document_related_ids))
         if not other_ids:
             return
-        raise orm.except_orm(_('Access Denied'),
-                             _('The requested operation cannot be completed due to security restrictions. Please contact your system administrator.\n\n(Document type: %s, Operation: %s)') %
+        raise AccessError(_('The requested operation cannot be completed due to security restrictions. Please contact your system administrator.\n\n(Document type: %s, Operation: %s)') %
                              (self._description, operation))
 
+
     def _get_record_name(self, cr, uid, values, context=None):
         """ Return the related document name, using name_get. It is done using
             SUPERUSER_ID, to be sure to have the record name correctly stored. """
diff --git a/addons/mail/mail_template.py b/addons/mail/mail_template.py
index af4ea07fa4e..7525fe2fd81 100644
--- a/addons/mail/mail_template.py
+++ b/addons/mail/mail_template.py
@@ -33,6 +33,7 @@ from openerp.osv import osv, fields
 from openerp import tools, api
 from openerp.tools.translate import _
 from urllib import urlencode, quote as quote
+from openerp.exceptions import UserError
 
 _logger = logging.getLogger(__name__)
 
@@ -183,7 +184,7 @@ class mail_template(osv.osv):
         try:
             template = mako_template_env.from_string(tools.ustr(template))
         except Exception:
-            _logger.exception("Failed to load template %r", template)
+            _logger.info("Failed to load template %r", template, exc_info=True)
             return results
 
         # prepare template variables
@@ -200,7 +201,8 @@ class mail_template(osv.osv):
             try:
                 render_result = template.render(variables)
             except Exception:
-                _logger.exception("Failed to render template %r using values %r" % (template, variables))
+                _logger.info("Failed to render template %r using values %r" % (template, variables), exc_info=True)
+                raise UserError(_("Failed to render template %r using values %r")% (template, variables))
                 render_result = u""
             if render_result == u"False":
                 render_result = u""
@@ -353,7 +355,7 @@ class mail_template(osv.osv):
                     ir_values_obj = self.pool.get('ir.values')
                     ir_values_obj.unlink(cr, SUPERUSER_ID, template.ref_ir_value.id, context)
             except Exception:
-                raise osv.except_osv(_("Warning"), _("Deletion of the action record failed."))
+                raise UserError(_("Deletion of the action record failed."))
         return True
 
     def unlink(self, cr, uid, ids, context=None):
@@ -549,7 +551,7 @@ class mail_template(osv.osv):
         # create a mail_mail based on values, without attachments
         values = self.generate_email(cr, uid, template_id, res_id, context=context)
         if not values.get('email_from'):
-            raise osv.except_osv(_('Warning!'), _("Sender email is missing or empty after template rendering. Specify one to deliver your message"))
+            raise UserError(_("Sender email is missing or empty after template rendering. Specify one to deliver your message"))
         values['recipient_ids'] = [(4, pid) for pid in values.get('partner_ids', list())]
         attachment_ids = values.pop('attachment_ids', [])
         attachments = values.pop('attachments', [])
diff --git a/addons/mail/mail_thread.py b/addons/mail/mail_thread.py
index db1ea225713..254cd193285 100644
--- a/addons/mail/mail_thread.py
+++ b/addons/mail/mail_thread.py
@@ -46,6 +46,7 @@ from openerp.osv import fields, osv, orm
 from openerp.osv.orm import BaseModel
 from openerp.tools.safe_eval import safe_eval as eval
 from openerp.tools.translate import _
+from openerp.exceptions import AccessError
 
 _logger = logging.getLogger(__name__)
 
@@ -649,7 +650,7 @@ class mail_thread(osv.AbstractModel):
                 try:
                     model_obj.check_access_rule(cr, uid, [res_id], 'read', context=context)
                     action = model_obj.get_access_action(cr, uid, res_id, context=context)
-                except (osv.except_osv, orm.except_orm):
+                except AccessError:
                     pass
             action.update({
                 'context': {
@@ -803,7 +804,7 @@ class mail_thread(osv.AbstractModel):
             mail_mail.send(cr, uid, [mail_id], context=context)
 
         def _warn(message):
-            _logger.warning('Routing mail with Message-Id %s: route %s: %s',
+            _logger.info('Routing mail with Message-Id %s: route %s: %s',
                                 message_id, route, message)
 
         # Wrong model
@@ -1352,7 +1353,7 @@ class mail_thread(osv.AbstractModel):
                 else:
                     stored_date = parsed_date.astimezone(tz=pytz.utc)
             except Exception:
-                _logger.warning('Failed to parse Date header %r in incoming mail '
+                _logger.info('Failed to parse Date header %r in incoming mail '
                                 'with message-id %r, assuming current date/time.',
                                 message.get('Date'), message_id)
                 stored_date = datetime.datetime.now()
@@ -1698,7 +1699,7 @@ class mail_thread(osv.AbstractModel):
             try:
                 self.check_access_rights(cr, uid, 'read')
                 self.check_access_rule(cr, uid, ids, 'read')
-            except (osv.except_osv, orm.except_orm):
+            except AccessError:
                 return False
         else:
             self.check_access_rights(cr, uid, 'write')
diff --git a/addons/mail/tests/test_mail_group.py b/addons/mail/tests/test_mail_group.py
index c131ce0bd0b..25a4d5b199e 100644
--- a/addons/mail/tests/test_mail_group.py
+++ b/addons/mail/tests/test_mail_group.py
@@ -21,7 +21,6 @@
 
 from openerp.addons.mail.tests.common import TestMail
 from openerp.exceptions import AccessError
-from openerp.osv.orm import except_orm
 from openerp.tools import mute_logger
 
 
@@ -35,7 +34,7 @@ class TestMailGroup(TestMail):
         # Do: Bert reads Jobs -> ok, public
         self.mail_group.read(cr, user_noone_id, [self.group_jobs_id])
         # Do: Bert read Pigs -> ko, restricted to employees
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_group.read(cr, user_noone_id, [self.group_pigs_id])
         # Do: Raoul read Pigs -> ok, belong to employees
         self.mail_group.read(cr, user_employee_id, [self.group_pigs_id])
@@ -50,7 +49,7 @@ class TestMailGroup(TestMail):
         self.mail_group.read(cr, user_noone_id, [new_group_id])
 
         # Do: Raoul reads Priv -> ko, private
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_group.read(cr, user_employee_id, [self.group_priv_id])
         # Do: Raoul added in follower, read -> ok, in followers
         self.mail_group.message_subscribe_users(cr, uid, [self.group_priv_id], [user_employee_id])
@@ -62,7 +61,7 @@ class TestMailGroup(TestMail):
         with self.assertRaises(AccessError):
             self.mail_group.write(cr, user_noone_id, [self.group_priv_id], {'name': 're-modified'})
         # Test: Bert cannot unlink the group
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_group.unlink(cr, user_noone_id, [self.group_priv_id])
         # Do: Raoul unlinks the group, there are no followers and messages left
         self.mail_group.unlink(cr, user_employee_id, [self.group_priv_id])
diff --git a/addons/mail/tests/test_mail_message.py b/addons/mail/tests/test_mail_message.py
index 9883449d754..3bd089bd87c 100644
--- a/addons/mail/tests/test_mail_message.py
+++ b/addons/mail/tests/test_mail_message.py
@@ -21,7 +21,6 @@
 
 from openerp.addons.mail.tests.common import TestMail
 from openerp.exceptions import AccessError
-from openerp.osv.orm import except_orm
 from openerp.tools import mute_logger
 
 
@@ -218,7 +217,7 @@ class TestMailMessage(TestMail):
         message_id = self.mail_message.create(cr, uid, {'body': 'My Body', 'attachment_ids': [(4, attachment_id)]})
 
         # Test: Bert reads the message, crash because not notification/not in doc followers/not read on doc
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_message.read(cr, user_bert_id, message_id)
         # Do: message is pushed to Bert
         notif_id = self.mail_notification.create(cr, uid, {'message_id': message_id, 'partner_id': partner_bert_id})
@@ -229,10 +228,10 @@ class TestMailMessage(TestMail):
         # Do: remove notification
         self.mail_notification.unlink(cr, uid, notif_id)
         # Test: Bert reads the message, crash because not notification/not in doc followers/not read on doc
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_message.read(cr, self.user_bert_id, message_id)
         # Test: Bert downloads attachment, crash because he can't read message
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_message.download_attachment(cr, user_bert_id, message_id, attachment_id)
         # Do: Bert is now the author
         self.mail_message.write(cr, uid, [message_id], {'author_id': partner_bert_id})
@@ -241,7 +240,7 @@ class TestMailMessage(TestMail):
         # Do: Bert is not the author anymore
         self.mail_message.write(cr, uid, [message_id], {'author_id': partner_raoul_id})
         # Test: Bert reads the message, crash because not notification/not in doc followers/not read on doc
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_message.read(cr, user_bert_id, message_id)
         # Do: message is attached to a document Bert can read, Jobs
         self.mail_message.write(cr, uid, [message_id], {'model': 'mail.group', 'res_id': self.group_jobs_id})
@@ -250,7 +249,7 @@ class TestMailMessage(TestMail):
         # Do: message is attached to a document Bert cannot read, Pigs
         self.mail_message.write(cr, uid, [message_id], {'model': 'mail.group', 'res_id': self.group_pigs_id})
         # Test: Bert reads the message, crash because not notification/not in doc followers/not read on doc
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_message.read(cr, user_bert_id, message_id)
 
         # ----------------------------------------
@@ -270,12 +269,12 @@ class TestMailMessage(TestMail):
         # Do: Raoul creates a message on Jobs -> ok, write access to the related document
         self.mail_message.create(cr, user_raoul_id, {'model': 'mail.group', 'res_id': self.group_jobs_id, 'body': 'Test'})
         # Do: Raoul creates a message on Priv -> ko, no write access to the related document
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_message.create(cr, user_raoul_id, {'model': 'mail.group', 'res_id': self.group_priv_id, 'body': 'Test'})
         # Do: Raoul creates a private message -> ok
         self.mail_message.create(cr, user_raoul_id, {'body': 'Test'})
         # Do: Raoul creates a reply to a message on Priv -> ko
-        with self.assertRaises(except_orm):
+        with self.assertRaises(AccessError):
             self.mail_message.create(cr, user_raoul_id, {'model': 'mail.group', 'res_id': self.group_priv_id, 'body': 'Test', 'parent_id': priv_msg_id})
         # Do: Raoul creates a reply to a message on Priv-> ok if has received parent
         self.mail_notification.create(cr, uid, {'message_id': priv_msg_id, 'partner_id': self.partner_raoul_id})
diff --git a/addons/mail/update.py b/addons/mail/update.py
index 7f9ec9727f4..92ea8440fde 100644
--- a/addons/mail/update.py
+++ b/addons/mail/update.py
@@ -12,6 +12,7 @@ from openerp.osv import osv
 from openerp.tools.translate import _
 from openerp.tools.config import config
 from openerp.tools import misc
+from openerp.exceptions import UserError
 
 _logger = logging.getLogger(__name__)
 
@@ -88,7 +89,7 @@ class publisher_warranty_contract(AbstractModel):
                 if cron_mode:   # we don't want to see any stack trace in cron
                     return False
                 _logger.debug("Exception while sending a get logs messages", exc_info=1)
-                raise osv.except_osv(_("Error"), _("Error during communication with the publisher warranty server."))
+                raise UserError(_("Error during communication with the publisher warranty server."))
             # old behavior based on res.log; now on mail.message, that is not necessarily installed
             IMD = self.pool['ir.model.data']
             user = self.pool['res.users'].browse(cr, SUPERUSER_ID, SUPERUSER_ID)
@@ -101,7 +102,7 @@ class publisher_warranty_contract(AbstractModel):
                 try:
                     poster.message_post(body=message, subtype='mt_comment', partner_ids=[user.partner_id.id])
                 except Exception:
-                    _logger.warning('Cannot send ping message', exc_info=True)
+                    pass
         except Exception:
             if cron_mode:
                 return False    # we don't want to see any stack trace in cron