From 22a6170a01e5818bab57a49606bf385bc2be034d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 3 Aug 2023 12:00:54 +0000 Subject: [PATCH] [IMP] mail: lint and reorder alias code, add tests CODE LINT / CLEANUP Reorder fields definition per main usage: definition, owner, parent, gateway configuration. Order computed fields accordingly. Rename some methods (notably constrains), move an inner sanitize method as a model method to allow its future usage. Perfom a quick linting of code, simplify some lines. TESTS Add some tests related to alias management, notably copy, and multi-company models using aliases. Those will help when moving to multi-company support for aliases. Add tests for current sanitation / cleaning of alias names and alias domain parameters, to be more precise about accepted / unallowed characters, support of unicode, ... Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/mail/__manifest__.py | 2 +- addons/mail/models/mail_alias.py | 154 +++++++++++--------- addons/mail/models/mail_alias_mixin.py | 14 +- addons/mail/views/mail_alias_views.xml | 14 +- addons/mail/views/mail_menus.xml | 2 +- addons/test_mail/tests/test_mail_alias.py | 166 +++++++++++++++++++--- 6 files changed, 255 insertions(+), 97 deletions(-) diff --git a/addons/mail/__manifest__.py b/addons/mail/__manifest__.py index e8860e247a1..ce06177d735 100644 --- a/addons/mail/__manifest__.py +++ b/addons/mail/__manifest__.py @@ -2,7 +2,7 @@ { 'name': 'Discuss', - 'version': '1.14', + 'version': '1.15', 'category': 'Productivity/Discuss', 'sequence': 145, 'summary': 'Chat, mail gateway and private channels', diff --git a/addons/mail/models/mail_alias.py b/addons/mail/models/mail_alias.py index d949bd1312c..4ad9a227ead 100644 --- a/addons/mail/models/mail_alias.py +++ b/addons/mail/models/mail_alias.py @@ -33,7 +33,12 @@ class Alias(models.Model): _rec_name = 'alias_name' _order = 'alias_model_id, alias_name' - alias_name = fields.Char('Alias Name', copy=False, help="The name of the email alias, e.g. 'jobs' if you want to catch emails for ") + # email definition + alias_name = fields.Char( + 'Alias Name', copy=False, + help="The name of the email alias, e.g. 'jobs' if you want to catch emails for ") + alias_domain = fields.Char('Alias domain', compute='_compute_alias_domain') + # target: create / update alias_model_id = fields.Many2one('ir.model', 'Aliased Model', required=True, ondelete="cascade", help="The model (Odoo Document Kind) to which this alias " "corresponds. Any incoming email that does not reply to an " @@ -54,17 +59,22 @@ class Alias(models.Model): 'Record Thread ID', help="Optional ID of a thread (record) to which all incoming messages will be attached, even " "if they did not reply to it. If set, this will disable the creation of new records completely.") - alias_domain = fields.Char('Alias domain', compute='_compute_alias_domain') + # owner alias_parent_model_id = fields.Many2one( 'ir.model', 'Parent Model', help="Parent model holding the alias. The model holding the alias reference " "is not necessarily the model given by alias_model_id " "(example: project (parent_model) and task (model))") - alias_parent_thread_id = fields.Integer('Parent Record Thread ID', help="ID of the parent record holding the alias (example: project holding the task creation alias)") - alias_contact = fields.Selection([ - ('everyone', 'Everyone'), - ('partners', 'Authenticated Partners'), - ('followers', 'Followers only')], default='everyone', + alias_parent_thread_id = fields.Integer( + 'Parent Record Thread ID', + help="ID of the parent record holding the alias (example: project holding the task creation alias)") + # incoming configuration (mailgateway) + alias_contact = fields.Selection( + [ + ('everyone', 'Everyone'), + ('partners', 'Authenticated Partners'), + ('followers', 'Followers only') + ], default='everyone', string='Alias Contact Security', required=True, help="Policy to post a message on the document using the mailgateway.\n" "- everyone: everyone can post\n" @@ -73,12 +83,12 @@ class Alias(models.Model): alias_bounced_content = fields.Html( "Custom Bounced Message", translate=True, help="If set, this content will automatically be sent out to unauthorized users instead of the default message.") - alias_status = fields.Selection([ - ('not_tested', 'Not Tested'), - ('valid', 'Valid'), - ('invalid', 'Invalid'), - ], - compute='_compute_alias_status', store=True, + alias_status = fields.Selection( + [ + ('not_tested', 'Not Tested'), + ('valid', 'Valid'), + ('invalid', 'Invalid'), + ], compute='_compute_alias_status', store=True, help='Alias status assessed on the last message received.') _sql_constraints = [ @@ -86,36 +96,52 @@ class Alias(models.Model): ] @api.constrains('alias_name') - def _alias_is_ascii(self): + def _check_alias_is_ascii(self): """ The local-part ("display-name" ) of an address only contains limited range of ascii characters. We DO NOT allow anything else than ASCII dot-atom formed local-part. Quoted-string and internationnal characters are to be rejected. See rfc5322 sections 3.4.1 and 3.2.3 """ - for alias in self: - if alias.alias_name and not dot_atom_text.match(alias.alias_name): - raise ValidationError(_( - "You cannot use anything else than unaccented latin characters in the alias address (%s).", - alias.alias_name, - )) - - @api.depends('alias_contact', 'alias_defaults', 'alias_model_id') - def _compute_alias_status(self): - """Reset alias_status to "not_tested" when fields, that can be the source of an error, are modified.""" - self.alias_status = 'not_tested' - - @api.depends('alias_name') - def _compute_alias_domain(self): - self.alias_domain = self.env["ir.config_parameter"].sudo().get_param("mail.catchall.domain") + for alias in self.filtered('alias_name'): + if not dot_atom_text.match(alias.alias_name): + raise ValidationError( + _("You cannot use anything else than unaccented latin characters in the alias address %(alias_name)s.", + alias_name=alias.alias_name) + ) @api.constrains('alias_defaults') def _check_alias_defaults(self): for alias in self: try: dict(ast.literal_eval(alias.alias_defaults)) - except Exception: - raise ValidationError(_('Invalid expression, it must be a literal python dictionary definition e.g. "{\'field\': \'value\'}"')) + except Exception as e: + raise ValidationError( + _('Invalid expression, it must be a literal python dictionary definition e.g. "{\'field\': \'value\'}"') + ) from e + + @api.depends('alias_domain', 'alias_name') + def _compute_display_name(self): + """Return the mail alias display alias_name, including the implicit + mail catchall domain if exists from config otherwise "New Alias". + e.g. `jobs@mail.odoo.com` or `jobs` or 'New Alias' + """ + for record in self: + if record.alias_name and record.alias_domain: + record.display_name = f"{record.alias_name}@{record.alias_domain}" + elif record.alias_name: + record.display_name = record.alias_name + else: + record.display_name = _("Inactive Alias") + + @api.depends('alias_name') + def _compute_alias_domain(self): + self.alias_domain = self.env["ir.config_parameter"].sudo().get_param("mail.catchall.domain") + + @api.depends('alias_contact', 'alias_defaults', 'alias_model_id') + def _compute_alias_status(self): + """Reset alias_status to "not_tested" when fields, that can be the source of an error, are modified.""" + self.alias_status = 'not_tested' @api.model_create_multi def create(self, vals_list): @@ -134,32 +160,19 @@ class Alias(models.Model): for vals in vals_list: if vals.get('alias_name'): vals['alias_name'] = sanitized_names[alias_names.index(vals['alias_name'])] - return super(Alias, self).create(vals_list) + return super().create(vals_list) def write(self, vals): - """"Raises UserError if given alias name is already assigned""" + """ Raise UserError with a meaningfull message instead of letting the + unicity constraint give its error. """ if vals.get('alias_name') and self.ids: if len(self) > 1: - raise UserError(_( - 'Email alias %(alias_name)s cannot be used on %(count)d records at the same time. Please update records one by one.', - alias_name=vals['alias_name'], count=len(self) - )) + raise UserError( + _('Email alias %(alias_name)s cannot be used on %(count)d records at the same time. Please update records one by one.', + alias_name=vals['alias_name'], count=len(self)) + ) vals['alias_name'] = self._clean_and_check_unique([vals.get('alias_name')])[0] - return super(Alias, self).write(vals) - - @api.depends('alias_domain', 'alias_name') - def _compute_display_name(self): - """Return the mail alias display alias_name, including the implicit - mail catchall domain if exists from config otherwise "New Alias". - e.g. `jobs@mail.odoo.com` or `jobs` or 'New Alias' - """ - for record in self: - if record.alias_name and record.alias_domain: - record.display_name = f"{record.alias_name}@{record.alias_domain}" - elif record.alias_name: - record.display_name = record.alias_name - else: - record.display_name = _("Inactive Alias") + return super().write(vals) def _clean_and_check_mail_catchall_allowed_domains(self, value): """ The purpose of this system parameter is to avoid the creation @@ -167,8 +180,10 @@ class Alias(models.Model): but that have a pattern matching an internal mail.alias . """ value = [domain.strip().lower() for domain in value.split(',') if domain.strip()] if not value: - raise ValidationError(_("Value for `mail.catchall.domain.allowed` cannot be validated.\n" - "It should be a comma separated list of domains e.g. example.com,example.org.")) + raise ValidationError( + _("Value for `mail.catchall.domain.allowed` cannot be validated.\n" + "It should be a comma separated list of domains e.g. example.com,example.org.") + ) return ",".join(value) def _clean_and_check_unique(self, names): @@ -176,15 +191,7 @@ class Alias(models.Model): part only. A sanitizing / cleaning is also performed on the name. If name already exists an UserError is raised. """ - def _sanitize_alias_name(name): - """ Cleans and sanitizes the alias name """ - sanitized_name = remove_accents(name).lower().split('@')[0] - sanitized_name = re.sub(r'[^\w+.]+', '-', sanitized_name) - sanitized_name = re.sub(r'^\.+|\.+$|\.+(?=\.)', '', sanitized_name) - sanitized_name = sanitized_name.encode('ascii', errors='replace').decode() - return sanitized_name - - sanitized_names = [_sanitize_alias_name(name) for name in names] + sanitized_names = [self._sanitize_alias_name(name) for name in names] catchall_alias = self.env['ir.config_parameter'].sudo().get_param('mail.catchall.alias') bounce_alias = self.env['ir.config_parameter'].sudo().get_param('mail.bounce.alias') @@ -208,8 +215,8 @@ class Alias(models.Model): if not matching_alias: return sanitized_names - sanitized_alias_name = _sanitize_alias_name(matching_alias.alias_name) - matching_alias_name = '%s@%s' % (sanitized_alias_name, alias_domain) if alias_domain else sanitized_alias_name + sanitized_alias_name = self._sanitize_alias_name(matching_alias.alias_name) + matching_alias_name = f'{sanitized_alias_name}@{alias_domain}' if alias_domain else sanitized_alias_name if matching_alias.alias_parent_model_id and matching_alias.alias_parent_thread_id: # If parent model and parent thread ID both are set, display document name also in the warning document_name = self.env[matching_alias.alias_parent_model_id.model].sudo().browse(matching_alias.alias_parent_thread_id).display_name @@ -225,6 +232,19 @@ class Alias(models.Model): alias_model_name=matching_alias.alias_model_id.name) ) + @api.model + def _sanitize_alias_name(self, name): + """ Cleans and sanitizes the alias name """ + sanitized_name = remove_accents(name).lower().split('@')[0] + sanitized_name = re.sub(r'[^\w+.]+', '-', sanitized_name) + sanitized_name = re.sub(r'^\.+|\.+$|\.+(?=\.)', '', sanitized_name) + sanitized_name = sanitized_name.encode('ascii', errors='replace').decode() + return sanitized_name + + # ------------------------------------------------------------ + # ACTIONS + # ------------------------------------------------------------ + def open_document(self): if not self.alias_model_id or not self.alias_force_thread_id: return False @@ -245,6 +265,10 @@ class Alias(models.Model): 'type': 'ir.actions.act_window', } + # ------------------------------------------------------------ + # MAIL GATEWAY + # ------------------------------------------------------------ + def _get_alias_bounced_body_fallback(self, message_dict): contact_description = self._get_alias_contact_description() default_email = self.env.company.partner_id.email_formatted if self.env.company.partner_id.email else self.env.company.name diff --git a/addons/mail/models/mail_alias_mixin.py b/addons/mail/models/mail_alias_mixin.py index c4efa17e3b5..28da1423a99 100644 --- a/addons/mail/models/mail_alias_mixin.py +++ b/addons/mail/models/mail_alias_mixin.py @@ -3,7 +3,7 @@ import logging -from odoo import _, api, fields, models +from odoo import api, fields, models _logger = logging.getLogger(__name__) @@ -54,7 +54,7 @@ class AliasMixin(models.AbstractModel): else: valid_vals_list.append(vals) - records = super(AliasMixin, self).create(valid_vals_list) + records = super().create(valid_vals_list) for record in records: record.alias_id.sudo().write(record._alias_get_creation_values()) @@ -66,7 +66,7 @@ class AliasMixin(models.AbstractModel): write with sudo and the other normally """ alias_vals, record_vals = self._alias_filter_fields(vals, filters=self.ALIAS_WRITEABLE_FIELDS) if record_vals: - super(AliasMixin, self).write(record_vals) + super().write(record_vals) if alias_vals and (record_vals or self.check_access_rights('write', raise_exception=False)): self.mapped('alias_id').sudo().write(alias_vals) @@ -75,13 +75,13 @@ class AliasMixin(models.AbstractModel): def unlink(self): """ Delete the given records, and cascade-delete their corresponding alias. """ aliases = self.mapped('alias_id') - res = super(AliasMixin, self).unlink() + res = super().unlink() aliases.sudo().unlink() return res @api.returns(None, lambda value: value[0]) def copy_data(self, default=None): - data = super(AliasMixin, self).copy_data(default)[0] + data = super().copy_data(default)[0] for fields_not_writable in set(self.env['mail.alias']._fields.keys()) - set(self.ALIAS_WRITEABLE_FIELDS): if fields_not_writable in data: del data[fields_not_writable] @@ -89,7 +89,7 @@ class AliasMixin(models.AbstractModel): def _init_column(self, name): """ Create aliases for existing rows. """ - super(AliasMixin, self)._init_column(name) + super()._init_column(name) if name == 'alias_id': # as 'mail.alias' records refer to 'ir.model' records, create # aliases after the reflection of models @@ -120,7 +120,7 @@ class AliasMixin(models.AbstractModel): """ return { 'alias_parent_thread_id': self.id if self.id else False, - 'alias_parent_model_id': self.env['ir.model']._get(self._name).id, + 'alias_parent_model_id': self.env['ir.model']._get_id(self._name), } def _alias_filter_fields(self, values, filters=False): diff --git a/addons/mail/views/mail_alias_views.xml b/addons/mail/views/mail_alias_views.xml index abe7afb26de..6222557588f 100644 --- a/addons/mail/views/mail_alias_views.xml +++ b/addons/mail/views/mail_alias_views.xml @@ -3,8 +3,8 @@ - - mail.alias.form + + mail.alias.view.form mail.alias
@@ -50,8 +50,8 @@ - - mail.alias.tree + + mail.alias.view.tree mail.alias @@ -78,8 +78,8 @@ - - mail.alias.search + + mail.alias.view.search mail.alias @@ -102,7 +102,7 @@ - + Aliases mail.alias { diff --git a/addons/mail/views/mail_menus.xml b/addons/mail/views/mail_menus.xml index 8a5531d19d6..9f267f3c195 100644 --- a/addons/mail/views/mail_menus.xml +++ b/addons/mail/views/mail_menus.xml @@ -30,7 +30,7 @@ sequence="10"/>