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 1/9] [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"/> Date: Thu, 3 Aug 2023 13:26:30 +0000 Subject: [PATCH 2/9] [FIX] mail: force False when giving void alias name Aliases should be unique except when they are empty. Indeed each valid email address should be unique, as it targets a specific behavior e.g. creating a task in a project or a ticket in an helpdesk team. For that purpose an SQL constraint exists that enforces the unicity. Null values in DB are not considered as being the same, meaning we may have multiples aliases with Null values in DB. When voiding the alias we may end up with a void string, which is not the same as giving False to the ORM in term of DB storage. Multiple void strings break the unicity constraint where multiple False strings do not. In this commit we therefore enforce that void alias names are forced to False to avoid any constraint issue. Sanitize method is now independent from the check method, to avoid calling multiple times the sanitization as it is often used for other checks. Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/mail/models/ir_config_parameter.py | 6 ++- addons/mail/models/mail_alias.py | 62 +++++++++++++---------- addons/test_mail/tests/test_mail_alias.py | 29 ++++++++++- 3 files changed, 65 insertions(+), 32 deletions(-) diff --git a/addons/mail/models/ir_config_parameter.py b/addons/mail/models/ir_config_parameter.py index 7f9ba2bf62d..f773c993ba9 100644 --- a/addons/mail/models/ir_config_parameter.py +++ b/addons/mail/models/ir_config_parameter.py @@ -11,13 +11,15 @@ class IrConfigParameter(models.Model): def create(self, vals_list): for vals in vals_list: if vals.get('key') in ['mail.bounce.alias', 'mail.catchall.alias']: - vals['value'] = self.env['mail.alias']._clean_and_check_unique([vals.get('value')])[0] + vals['value'] = self.env['mail.alias']._sanitize_alias_name(vals.get('value')) + self.env['mail.alias']._check_unique([vals['value']]) return super().create(vals_list) def write(self, vals): for parameter in self: if 'value' in vals and parameter.key in ['mail.bounce.alias', 'mail.catchall.alias'] and vals['value'] != parameter.value: - vals['value'] = self.env['mail.alias']._clean_and_check_unique([vals.get('value')])[0] + vals['value'] = self.env['mail.alias']._sanitize_alias_name(vals.get('value')) + self.env['mail.alias']._check_unique([vals['value']]) return super().write(vals) @api.model diff --git a/addons/mail/models/mail_alias.py b/addons/mail/models/mail_alias.py index 4ad9a227ead..af486717f00 100644 --- a/addons/mail/models/mail_alias.py +++ b/addons/mail/models/mail_alias.py @@ -154,24 +154,24 @@ class Alias(models.Model): :raise UserError: if given alias_name is already assigned or there are duplicates in given vals_list; """ - alias_names = [vals['alias_name'] for vals in vals_list if vals.get('alias_name')] - if alias_names: - sanitized_names = self._clean_and_check_unique(alias_names) - for vals in vals_list: - if vals.get('alias_name'): - vals['alias_name'] = sanitized_names[alias_names.index(vals['alias_name'])] + alias_names = [self._sanitize_alias_name(vals.get('alias_name')) for vals in vals_list] + self._check_unique(alias_names) + for vals, alias_name in zip(vals_list, alias_names): + vals['alias_name'] = alias_name return super().create(vals_list) def write(self, vals): """ Raise UserError with a meaningfull message instead of letting the unicity constraint give its error. """ + if 'alias_name' in vals: + vals['alias_name'] = self._sanitize_alias_name(vals['alias_name']) if vals.get('alias_name') and self.ids: + self._check_unique([vals['alias_name']]) 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)) ) - vals['alias_name'] = self._clean_and_check_unique([vals.get('alias_name')])[0] return super().write(vals) def _clean_and_check_mail_catchall_allowed_domains(self, value): @@ -186,20 +186,23 @@ class Alias(models.Model): ) return ",".join(value) - def _clean_and_check_unique(self, names): - """When an alias name appears to already be an email, we keep the local - part only. A sanitizing / cleaning is also performed on the name. If - name already exists an UserError is raised. """ - - sanitized_names = [self._sanitize_alias_name(name) for name in names] + def _check_unique(self, sanitized_names): + """ Check unicity constraint won't be raised, otherwise raise a UserError + with a complete error message. Also check unicity against alias config + parameters. + :param list sanitized_names: a list of names (considered as sanitized + and ready to be sent to DB); + """ 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') alias_domain = self.env["ir.config_parameter"].sudo().get_param("mail.catchall.domain") + valid_domain_aliases = filter(None, (bounce_alias, catchall_alias)) + valid_names = list(filter(None, sanitized_names)) # matches catchall or bounce alias for sanitized_name in sanitized_names: - if sanitized_name in [catchall_alias, bounce_alias]: + if sanitized_name in valid_domain_aliases: matching_alias_name = '%s@%s' % (sanitized_name, alias_domain) if alias_domain else sanitized_name raise UserError( _('The e-mail alias %(matching_alias_name)s is already used as %(alias_duplicate)s alias. Please choose another alias.', @@ -208,37 +211,40 @@ class Alias(models.Model): ) # matches existing alias - domain = [('alias_name', 'in', sanitized_names)] - if self: - domain += [('id', 'not in', self.ids)] - matching_alias = self.search(domain, limit=1) + matching_alias = self.env['mail.alias'] + if valid_names: + domain = [('alias_name', 'in', valid_names)] + if self: + domain += [('id', 'not in', self.ids)] + matching_alias = self.search(domain, limit=1) if not matching_alias: - return sanitized_names - - 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 + return 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 raise UserError( _('The e-mail alias %(matching_alias_name)s is already used by the %(document_name)s %(model_name)s. Choose another alias or change it on the other document.', - matching_alias_name=matching_alias_name, + matching_alias_name=matching_alias.display_name, document_name=document_name, model_name=matching_alias.alias_parent_model_id.name) ) raise UserError( _('The e-mail alias %(matching_alias_name)s is already linked with %(alias_model_name)s. Choose another alias or change it on the linked model.', - matching_alias_name=matching_alias_name, + matching_alias_name=matching_alias.display_name, 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() + sanitized_name = name.strip() if name else '' + if sanitized_name: + sanitized_name = remove_accents(sanitized_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() + if not sanitized_name.strip(): + return False return sanitized_name # ------------------------------------------------------------ diff --git a/addons/test_mail/tests/test_mail_alias.py b/addons/test_mail/tests/test_mail_alias.py index 1dc479b2bbb..cad05318fe8 100644 --- a/addons/test_mail/tests/test_mail_alias.py +++ b/addons/test_mail/tests/test_mail_alias.py @@ -73,8 +73,10 @@ class TestMailAlias(TestMailAliasCommon): ICP.set_param('mail.catchall.alias', catchall_value) self.assertEqual(ICP.get_param('mail.catchall.alias'), expected_catchall) - # falsy values (FIXME: ' ' is currently replaced by '-' and is not falsy, should be') - for config_value in [False, None, '']: + # falsy values FIXME: fix ICP override for '' and ' ' as they make ICP + # udpate crash, they now try to write False in DB instead of removing + # the ICP + for config_value in [False, None]: with self.subTest(config_value=config_value): ICP.set_param('mail.bounce.alias', config_value) self.assertFalse(ICP.get_param('mail.bounce.alias')) @@ -191,6 +193,29 @@ class TestMailAlias(TestMailAliasCommon): alias.write({'alias_name': alias_name}) self.assertEqual(alias.alias_name, expected, msg) + @users('admin') + def test_alias_name_sanitize_false(self): + """ Check empty-like aliases are forced to False, as otherwise unique + constraint might fail with empty strings. """ + aliases = self.env['mail.alias'].create([ + { + 'alias_model_id': self.env['ir.model']._get('mail.test.container').id, + 'alias_name': falsy_name, + } + # '.' -> not allowed to start with a "." hence False + for falsy_name in [False, None, '', ' ', '.'] + ]) + for alias in aliases: + with self.subTest(alias_name=alias.alias_name): + self.assertFalse(alias.alias_name, 'Void values should resolve to False') + + # try to reset names in batch: should work + for idx, alias in enumerate(aliases): + alias.write({'alias_name': f'unique-{idx}'}) + aliases.write({'alias_name': ''}) + for alias in aliases: + self.assertEqual(alias.alias_name, False) + @users('admin') def test_alias_setup(self): """ Test various constraints / configuration of alias model""" From d61be63ed1be310fb95278034c58c15c732d7a7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 8 Aug 2023 15:26:12 +0000 Subject: [PATCH 3/9] [FIX] mail: check unicity inside alias names given for unicity check In addition to checking conflicts with existing aliases, we have to check that given name list also contains only unique names. Otherwise creating in batch with duplicates raises the SQL unicity constraint instead of the expected UserError. Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/mail/models/mail_alias.py | 12 +++++++++++- addons/test_mail/tests/test_mail_alias.py | 8 ++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/addons/mail/models/mail_alias.py b/addons/mail/models/mail_alias.py index af486717f00..4d6f38767d6 100644 --- a/addons/mail/models/mail_alias.py +++ b/addons/mail/models/mail_alias.py @@ -194,11 +194,21 @@ class Alias(models.Model): :param list sanitized_names: a list of names (considered as sanitized and ready to be sent to DB); """ + valid_names = list(filter(None, sanitized_names)) + + # list itself should be unique obviously + seen = set() + dupes = [name for name in valid_names if name in seen or seen.add(name)] + if dupes: + raise UserError( + _('Email aliases %(alias_name)s cannot be used on several records at the same time. Please update records one by one.', + alias_name=', '.join(dupes)) + ) + 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') alias_domain = self.env["ir.config_parameter"].sudo().get_param("mail.catchall.domain") valid_domain_aliases = filter(None, (bounce_alias, catchall_alias)) - valid_names = list(filter(None, sanitized_names)) # matches catchall or bounce alias for sanitized_name in sanitized_names: diff --git a/addons/test_mail/tests/test_mail_alias.py b/addons/test_mail/tests/test_mail_alias.py index cad05318fe8..53d0ac80e79 100644 --- a/addons/test_mail/tests/test_mail_alias.py +++ b/addons/test_mail/tests/test_mail_alias.py @@ -117,6 +117,14 @@ class TestMailAlias(TestMailAliasCommon): 'alias_model_id': alias_model_id, 'alias_name': 'unused.test.alias', }) + with self.assertRaises(exceptions.UserError), self.cr.savepoint(): + self.env['mail.alias'].create([ + { + 'alias_model_id': alias_model_id, + 'alias_name': alias_name, + } + for alias_name in ('new.alias.1', 'new.alias.2', 'new.alias.1') + ]) with self.assertRaises(psycopg2.errors.UniqueViolation), self.cr.savepoint(), mute_logger('odoo.sql_db'): new_mail_alias.copy({'alias_name': 'unused.test.alias'}) with self.assertRaises(exceptions.UserError), self.cr.savepoint(): From e187ba1cb386556edb9f4f2c975254c98af1495b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 7 Aug 2023 12:59:11 +0000 Subject: [PATCH 4/9] [FIX] mail: correctly override ICP set_param method for alias parameters Currently ICP parameter clean and check is done at create / write override. However it should be done at ``set_param`` level to avoid messing with the specific behavior of ir.config_parameter with falsy values. Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/mail/models/ir_config_parameter.py | 23 +++++------------ addons/mail/models/mail_alias.py | 31 ++++++++++++++--------- addons/test_mail/tests/test_mail_alias.py | 6 ++--- 3 files changed, 28 insertions(+), 32 deletions(-) diff --git a/addons/mail/models/ir_config_parameter.py b/addons/mail/models/ir_config_parameter.py index f773c993ba9..44fde74d8b4 100644 --- a/addons/mail/models/ir_config_parameter.py +++ b/addons/mail/models/ir_config_parameter.py @@ -7,21 +7,6 @@ from odoo import api, models class IrConfigParameter(models.Model): _inherit = 'ir.config_parameter' - @api.model_create_multi - def create(self, vals_list): - for vals in vals_list: - if vals.get('key') in ['mail.bounce.alias', 'mail.catchall.alias']: - vals['value'] = self.env['mail.alias']._sanitize_alias_name(vals.get('value')) - self.env['mail.alias']._check_unique([vals['value']]) - return super().create(vals_list) - - def write(self, vals): - for parameter in self: - if 'value' in vals and parameter.key in ['mail.bounce.alias', 'mail.catchall.alias'] and vals['value'] != parameter.value: - vals['value'] = self.env['mail.alias']._sanitize_alias_name(vals.get('value')) - self.env['mail.alias']._check_unique([vals['value']]) - return super().write(vals) - @api.model def set_param(self, key, value): if key == 'mail.restrict.template.rendering': @@ -35,8 +20,14 @@ class IrConfigParameter(models.Model): # remove existing users, including inactive template user # admin will regain the right via implied_ids on group_system group_user._remove_group(group_mail_template_editor) + # sanitize, normalize and check uniqueness of catchall / bounce aliases + # note that False = unlink ICP + elif key in {'mail.bounce.alias', 'mail.catchall.alias'} and value: + value = self.env['mail.alias']._sanitize_alias_name(value) + if value: + self.env['mail.alias']._check_unique([value], skip_icp_keys={key}) # sanitize and normalize allowed catchall domains elif key == 'mail.catchall.domain.allowed' and value: value = self.env['mail.alias']._clean_and_check_mail_catchall_allowed_domains(value) - return super(IrConfigParameter, self).set_param(key, value) + return super().set_param(key, value) diff --git a/addons/mail/models/mail_alias.py b/addons/mail/models/mail_alias.py index 4d6f38767d6..01b9553ae0f 100644 --- a/addons/mail/models/mail_alias.py +++ b/addons/mail/models/mail_alias.py @@ -186,7 +186,7 @@ class Alias(models.Model): ) return ",".join(value) - def _check_unique(self, sanitized_names): + def _check_unique(self, sanitized_names, skip_icp_keys=None): """ Check unicity constraint won't be raised, otherwise raise a UserError with a complete error message. Also check unicity against alias config parameters. @@ -205,20 +205,27 @@ class Alias(models.Model): alias_name=', '.join(dupes)) ) - 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') alias_domain = self.env["ir.config_parameter"].sudo().get_param("mail.catchall.domain") - valid_domain_aliases = filter(None, (bounce_alias, catchall_alias)) + icp_to_check = dict( + (icp_key, self.env["ir.config_parameter"].sudo().get_param(icp_key)) + for icp_key in {'mail.bounce.alias', 'mail.catchall.alias'} + if icp_key not in (skip_icp_keys or ()) + ) + icp_label_by_key = { + 'mail.bounce.alias': _('bounce'), + 'mail.catchall.alias': _('catchall'), + } # matches catchall or bounce alias - for sanitized_name in sanitized_names: - if sanitized_name in valid_domain_aliases: - matching_alias_name = '%s@%s' % (sanitized_name, alias_domain) if alias_domain else sanitized_name - raise UserError( - _('The e-mail alias %(matching_alias_name)s is already used as %(alias_duplicate)s alias. Please choose another alias.', - matching_alias_name=matching_alias_name, - alias_duplicate=_('catchall') if sanitized_name == catchall_alias else _('bounce')) - ) + for sanitized_name in valid_names: + for icp_key, icp_value in icp_to_check.items(): + if icp_value and sanitized_name == icp_value: + matching_alias_name = f'{sanitized_name}@{alias_domain}' if alias_domain else sanitized_name + raise UserError( + _('The e-mail alias %(matching_alias_name)s is already used as %(alias_duplicate)s alias. Please choose another alias.', + matching_alias_name=matching_alias_name, + alias_duplicate=icp_label_by_key[icp_key]) + ) # matches existing alias matching_alias = self.env['mail.alias'] diff --git a/addons/test_mail/tests/test_mail_alias.py b/addons/test_mail/tests/test_mail_alias.py index 53d0ac80e79..96af718b70e 100644 --- a/addons/test_mail/tests/test_mail_alias.py +++ b/addons/test_mail/tests/test_mail_alias.py @@ -73,10 +73,8 @@ class TestMailAlias(TestMailAliasCommon): ICP.set_param('mail.catchall.alias', catchall_value) self.assertEqual(ICP.get_param('mail.catchall.alias'), expected_catchall) - # falsy values FIXME: fix ICP override for '' and ' ' as they make ICP - # udpate crash, they now try to write False in DB instead of removing - # the ICP - for config_value in [False, None]: + # falsy values + for config_value in [False, None, '', ' ']: with self.subTest(config_value=config_value): ICP.set_param('mail.bounce.alias', config_value) self.assertFalse(ICP.get_param('mail.bounce.alias')) From 1c4b6e11e474875c4af3a234f8701cb04e8ece0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 3 Aug 2023 08:40:21 +0000 Subject: [PATCH 5/9] [IMP] account: add tests for alias management on journal Improve tests related to alias management in 'account.journal' model. In order to support multi-company based alias domains we are going to cleanup custom management of aliases in journals, moving towards a standard usage of 'mail.alias.mixin'. This commit prepares this change by adding more complete tests about current behavior of aliases. This notably improve tests from odoo/odoo@339cdffb68f91eb1455d447d1bdd7133c68723bd where the purpose is to avoid issues when installing a COA, aka creating journals in batch. This also adds tests for odoo/odoo@400b6860271a11b9914166ff7e42939c4c6192dc where we ensure alias defaults are synchronized with journal configuration. Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/account/tests/test_account_journal.py | 156 ++++++++++++++++--- 1 file changed, 136 insertions(+), 20 deletions(-) diff --git a/addons/account/tests/test_account_journal.py b/addons/account/tests/test_account_journal.py index a67b49cd5f9..241ed690b4e 100644 --- a/addons/account/tests/test_account_journal.py +++ b/addons/account/tests/test_account_journal.py @@ -1,8 +1,11 @@ # -*- coding: utf-8 -*- + +from ast import literal_eval from unittest.mock import patch from odoo.addons.account.tests.common import AccountTestInvoicingCommon from odoo.addons.account.models.account_payment_method import AccountPaymentMethod +from odoo.addons.mail.tests.common import MailCommon from odoo.tests import tagged from odoo.exceptions import UserError, ValidationError @@ -137,24 +140,137 @@ class TestAccountJournal(AccountTestInvoicingCommon): self.assertFalse(second_method.exists()) - def test_account_journal_alias_name(self): - journal = self.company_data['default_journal_purchase'] - self.assertEqual(journal.alias_name, 'vendor-bills-company_1_data') - journal.name = 'ぁ' - journal.alias_name = False - self.assertEqual(journal.alias_name, 'bill-company_1_data') - journal.code = 'ぁ' - journal.alias_name = False - self.assertEqual(journal.alias_name, 'purchase-company_1_data') - company_2_id = str(self.company_data_2['company'].id) - journal_2 = self.company_data_2['default_journal_sale'] - self.company_data_2['company'].name = 'ぁ' - journal_2.alias_name = False - self.assertEqual(journal_2.alias_name, 'customer-invoices-' + company_2_id) - journal_2.name = 'ぁ' - journal_2.alias_name = False - self.assertEqual(journal_2.alias_name, 'inv-' + company_2_id) - journal_2.code = 'ぁ' - journal_2.alias_name = False - self.assertEqual(journal_2.alias_name, 'sale-' + company_2_id) +@tagged('post_install', '-at_install', 'mail_alias') +class TestAccountJournalAlias(AccountTestInvoicingCommon, MailCommon): + + def test_alias_name_creation(self): + """ Test alias creation, notably avoid raising constraints due to ascii + characters removal. See odoo/odoo@339cdffb68f91eb1455d447d1bdd7133c68723bd """ + # check base test data + journal1 = self.company_data['default_journal_purchase'] + company1 = journal1.company_id + journal2 = self.company_data_2['default_journal_sale'] + company2 = journal2.company_id + # have a non ascii company name + company2.name = 'ぁ' + + for (aname, jname, jcode, jtype, jcompany), expected_alias_name in zip( + [ + ('youpie', 'Journal Name', 'NEW1', 'purchase', company1), + (False, 'Journal Other Name', 'NEW2', 'purchase', company1), + (False, 'ぁ', 'NEW3', 'purchase', company1), + (False, 'ぁ', 'ぁ', 'purchase', company1), + ('youpie', 'Journal Name', 'NEW1', 'purchase', company2), + (False, 'Journal Other Name', 'NEW2', 'purchase', company2), + (False, 'ぁ', 'NEW3', 'purchase', company2), + (False, 'ぁ', 'ぁ', 'purchase', company2), + ], + [ + f'youpie-{company1.name}', + f'journal-other-name-{company1.name}', + f'new3-{company1.name}', + f'purchase-{company1.name}', + f'youpie-{company2.id}', + f'journal-other-name-{company2.id}', + f'new3-{company2.id}', + f'purchase-{company2.id}', + ] + ): + with self.subTest(aname=aname, jname=jname, jcode=jcode, jtype=jtype, jcompany=jcompany): + new_journal = self.env['account.journal'].create({ + 'code': jcode, + 'company_id': jcompany.id, + 'name': jname, + 'type': jtype, + # force alias_name only if given, to check default value otherwise + **({'alias_name': aname} if aname else {}), + }) + self.assertEqual(new_journal.alias_name, expected_alias_name) + + # other types: no mail support by default + journals = self.env['account.journal'].create([{ + 'code': f'NEW{jtype}', + 'name': f'Type {jtype}', + 'type': jtype} + for jtype in ('general', 'cash', 'bank') + ]) + self.assertFalse(journals.alias_id, 'Account: no mail support via no alias for those journal types') + self.assertFalse(list(filter(None, journals.mapped('alias_name')))) + + def test_alias_from_type(self): + """ Test alias behavior on journal, especially alias_name management as + well as defaults update, see odoo/odoo@400b6860271a11b9914166ff7e42939c4c6192dc """ + journal = self.company_data['default_journal_purchase'] + + # assert base test data + company_name = 'company_1_data' + journal_code = 'BILL' + journal_name = 'Vendor Bills' + journal_alias = journal.alias_id + self.assertEqual(journal.code, journal_code) + self.assertEqual(journal.company_id.name, company_name) + self.assertEqual(journal.name, journal_name) + self.assertEqual(journal.type, 'purchase') + + # assert default creation data + self.assertEqual(journal_alias.alias_contact, 'everyone') + self.assertDictEqual( + dict(literal_eval(journal_alias.alias_defaults)), + { + 'move_type': 'in_invoice', + 'company_id': journal.company_id.id, + 'journal_id': journal.id, + } + ) + self.assertFalse(journal_alias.alias_force_thread_id, 'Journal alias should create new moves') + self.assertEqual(journal_alias.alias_model_id, self.env['ir.model']._get('account.move'), + 'Journal alias targets moves') + self.assertEqual(journal_alias.alias_name, f'vendor-bills-{company_name}') + self.assertEqual(journal_alias.alias_parent_model_id, self.env['ir.model']._get('account.journal'), + 'Journal alias owned by journal itself') + self.assertEqual(journal_alias.alias_parent_thread_id, journal.id, + 'Journal alias owned by journal itself') + + # update alias_name, various tests because it is sometimes actually funny + for alias_name, expected in [ + (False, f'vendor-bills-{company_name}'), # do not want to reset + ('', f'vendor-bills-{company_name}'), # still does not want to reset + (' ', f'-{company_name}'), # but why ? + ('.', f'-{company_name}'), # but why ? + ('😊', f'vendor-bills-{company_name}'), # resets, unicode not supported + ('ぁ', f'vendor-bills-{company_name}'), # resets, ascii not supported + ('Youpie Boum', f'youpie-boum-{company_name}'), + ]: + with self.subTest(alias_name=alias_name): + journal.write({'alias_name': alias_name}) + self.assertEqual(journal.alias_name, expected) + self.assertEqual(journal_alias.alias_name, expected) + + # changing type should void if not purchase or sale + for jtype in ('general', 'cash', 'bank'): + journal.write({'type': jtype}) + self.assertFalse(journal_alias.exists()) + self.assertFalse(journal.alias_id) + + # changing type should reset if sale or purchase + journal.company_id.write({'name': 'New Company Name'}) + journal.write({'name': 'Reset Journal', 'type': 'sale'}) + journal_alias_2 = journal.alias_id + self.assertEqual(journal_alias_2.alias_contact, 'everyone') + self.assertDictEqual( + dict(literal_eval(journal_alias_2.alias_defaults)), + { + 'move_type': 'out_invoice', + 'company_id': journal.company_id.id, + 'journal_id': journal.id, + } + ) + self.assertFalse(journal_alias_2.alias_force_thread_id, 'Journal alias should create new moves') + self.assertEqual(journal_alias_2.alias_model_id, self.env['ir.model']._get('account.move'), + 'Journal alias targets moves') + self.assertEqual(journal_alias_2.alias_name, 'reset-journal-new-company-name') + self.assertEqual(journal_alias_2.alias_parent_model_id, self.env['ir.model']._get('account.journal'), + 'Journal alias owned by journal itself') + self.assertEqual(journal_alias_2.alias_parent_thread_id, journal.id, + 'Journal alias owned by journal itself') From 6a21786f4d8c592b12a2ce0e84c3a3812270ec5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 4 Aug 2023 15:13:42 +0000 Subject: [PATCH 6/9] [IMP] mail: make alias name sanitize coherent with constraint Currently there is a constraint on alias name as we allow only a subset of valid latin characters in it aka `[a-zA-Z0-9!#$%&'*+\-/=?^_`{|}~]`. There is also an automatic sanitize of alias name at create / write that replaces any non-word characters by an hyphen. This sanitize is stricter than the constraint and it is not really coherent. In this commit we make the sanitize inlined with the constraint, allowing more characters to go through the 'mail.alias.mixin' cleaning pass notably. Linking some bug fixes about that subject (notably due to 'account.journal' model that uses aliases without going through the 'mail.alias.mixin', hence allowing to see differences between sanitize and constraint): * odoo/odoo@33bd1a951a8c97098f4a29ff3fbc5d967828640d : non ascii aliases when installing COA and generating journals automatic email aliases; * odoo/odoo@e08ee893d1e286d56dafb4b683024f2df97e245c : left-part should not begin or end with dots as well as containing dots sequence; * odoo/odoo@f1215389e85b74d302b2f494ef055c4d16e6c48c : prevent international char in aliases; Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/account/models/account_journal.py | 11 ++--------- addons/mail/models/mail_alias.py | 16 +++++++++++++++- addons/test_mail/tests/test_mail_alias.py | 6 +++--- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/addons/account/models/account_journal.py b/addons/account/models/account_journal.py index 459b4dc0778..8f7bc0741d5 100644 --- a/addons/account/models/account_journal.py +++ b/addons/account/models/account_journal.py @@ -9,13 +9,6 @@ import re _logger = logging.getLogger(__name__) -def is_encodable_as_ascii(string): - try: - remove_accents(string).encode('ascii') - except UnicodeEncodeError: - return False - return True - class AccountJournalGroup(models.Model): _name = 'account.journal.group' @@ -371,10 +364,10 @@ class AccountJournal(models.Model): journal.name, journal.code, journal.type, - ) if string and is_encodable_as_ascii(string)) + ) if string and self.env['mail.alias']._is_encodable(string)) if journal.company_id != self.env.ref('base.main_company'): - if is_encodable_as_ascii(journal.company_id.name): + if self.env['mail.alias']._is_encodable(journal.company_id.name): alias_name = f"{alias_name}-{journal.company_id.name}" else: alias_name = f"{alias_name}-{journal.company_id.id}" diff --git a/addons/mail/models/mail_alias.py b/addons/mail/models/mail_alias.py index 01b9553ae0f..c873cfee3a9 100644 --- a/addons/mail/models/mail_alias.py +++ b/addons/mail/models/mail_alias.py @@ -257,13 +257,27 @@ class Alias(models.Model): sanitized_name = name.strip() if name else '' if sanitized_name: sanitized_name = remove_accents(sanitized_name).lower().split('@')[0] - sanitized_name = re.sub(r'[^\w+.]+', '-', sanitized_name) + # cannot start and end with dot sanitized_name = re.sub(r'^\.+|\.+$|\.+(?=\.)', '', sanitized_name) + # subset of allowed characters + sanitized_name = re.sub(r'[^\w!#$%&\'*+\-/=?^_`{|}~.]+', '-', sanitized_name) sanitized_name = sanitized_name.encode('ascii', errors='replace').decode() if not sanitized_name.strip(): return False return sanitized_name + @api.model + def _is_encodable(self, alias_name, charset='ascii'): + """ Check if alias_name is encodable. Standard charset is ascii, as + UTF-8 requires a specific extension. Not recommended for outgoing + aliases. 'remove_accents' is performed as sanitization process of + the name will do it anyway. """ + try: + remove_accents(alias_name).encode(charset) + except UnicodeEncodeError: + return False + return True + # ------------------------------------------------------------ # ACTIONS # ------------------------------------------------------------ diff --git a/addons/test_mail/tests/test_mail_alias.py b/addons/test_mail/tests/test_mail_alias.py index 96af718b70e..fcb7e900fcc 100644 --- a/addons/test_mail/tests/test_mail_alias.py +++ b/addons/test_mail/tests/test_mail_alias.py @@ -60,7 +60,7 @@ class TestMailAlias(TestMailAliasCommon): ('ぁ', 'ぁぁ'), ], [ - ('bounce+b4r-r3wl_-_-', 'catchall+b4r-r3wl_-_-'), + ('bounce+b4r=*r3wl_#_-$-{}-~|-/!?&%^\'-`~', 'catchall+b4r=*r3wl_#_-$-{}-~|-/!?&%^\'-`~'), ('bounce+-', 'catchall+-'), ('bouncaide-', 'catchoiee-'), ('?', '??'), @@ -141,7 +141,7 @@ class TestMailAlias(TestMailAliasCommon): self.assertFalse(copy_1.alias_name) # test sanitize of copy with new name copy_2 = new_mail_alias.copy({'alias_name': 'test.alias.2.éè#'}) - self.assertEqual(copy_2.alias_name, 'test.alias.2.ee-') + self.assertEqual(copy_2.alias_name, 'test.alias.2.ee#') # cannot batch update, would create duplicates with self.assertRaises(exceptions.UserError): @@ -167,7 +167,7 @@ class TestMailAlias(TestMailAliasCommon): ] expected_names = [ 'bidule.inc', - 'b4r+-r3wl_-_-', + 'b4r+=*r3wl_#_-$-{}-~|-/!?&%^\'-`~', 'helene.provaider', '-', 'deboulonneur-', From 1af84e28ee8c59ff62cde1750cca5db132028d90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 7 Aug 2023 11:44:09 +0000 Subject: [PATCH 7/9] [IMP] mail: introduce 'mail.alias.mixin.optional' with optional aliases Some models would like to use the 'mail.alias.mixin' but it creates an alias for each record in the parent model. This leads to a lot of unused aliases if only a subset of those records really use aliases i.e. a lot of aliases with 'alias_name' being 'False'. In this commit we introduce a new mixin 'mail.alias.mixin.optional' that behaves like the old 'mail.alias.mixin' but without having the 'alias_id' field required i.e. without the "inherits". When creating a record without giving an 'alias_name' no alias is created. In future commit, we plan to use it notably to remove custom code in account journal model and make it more standard. Using it in more models will be done later, but it is a candidate to cleanup unused aliases related to discuss channel model. Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/mail/models/__init__.py | 1 + addons/mail/models/mail_alias_mixin.py | 105 ++----------- .../mail/models/mail_alias_mixin_optional.py | 142 ++++++++++++++++++ addons/test_mail/models/test_mail_models.py | 21 +++ addons/test_mail/security/ir.model.access.csv | 2 + addons/test_mail/tests/test_mail_alias.py | 56 +++++++ 6 files changed, 232 insertions(+), 95 deletions(-) create mode 100644 addons/mail/models/mail_alias_mixin_optional.py diff --git a/addons/mail/models/__init__.py b/addons/mail/models/__init__.py index 6dc4d57c8a2..92146602a08 100644 --- a/addons/mail/models/__init__.py +++ b/addons/mail/models/__init__.py @@ -7,6 +7,7 @@ from . import models # mixin from . import mail_activity_mixin +from . import mail_alias_mixin_optional from . import mail_alias_mixin from . import mail_render_mixin from . import mail_composer_mixin diff --git a/addons/mail/models/mail_alias_mixin.py b/addons/mail/models/mail_alias_mixin.py index 28da1423a99..d9ebd41f5f2 100644 --- a/addons/mail/models/mail_alias_mixin.py +++ b/addons/mail/models/mail_alias_mixin.py @@ -3,89 +3,30 @@ import logging -from odoo import api, fields, models +from odoo import fields, models _logger = logging.getLogger(__name__) class AliasMixin(models.AbstractModel): - """ A mixin for models that inherits mail.alias. This mixin initializes the - alias_id column in database, and manages the expected one-to-one - relation between your model and mail aliases. - """ + """ A mixin for models that inherits mail.alias to have a one-to-one relation + between the model and its alias. """ _name = 'mail.alias.mixin' + _inherit = 'mail.alias.mixin.optional' _inherits = {'mail.alias': 'alias_id'} _description = 'Email Aliases Mixin' - ALIAS_WRITEABLE_FIELDS = ['alias_name', 'alias_contact', 'alias_defaults', 'alias_bounced_content'] - alias_id = fields.Many2one('mail.alias', string='Alias', ondelete="restrict", required=True) + alias_id = fields.Many2one(required=True) + alias_name = fields.Char(inherited=True) + alias_defaults = fields.Text(inherited=True) # -------------------------------------------------- # CRUD # -------------------------------------------------- - @api.model_create_multi - def create(self, vals_list): - """ Create a record with each ``vals`` or ``vals_list`` and create a corresponding alias. """ - # prepare all alias values - alias_vals_list, record_vals_list = [], [] - for vals in vals_list: - new_alias = not vals.get('alias_id') - if new_alias: - alias_vals, record_vals = self._alias_filter_fields(vals) - alias_vals.update(self._alias_get_creation_values()) - alias_vals_list.append(alias_vals) - record_vals_list.append(record_vals) - - # create all aliases - alias_ids = [] - if alias_vals_list: - alias_ids = iter(self.env['mail.alias'].sudo().create(alias_vals_list).ids) - - # update alias values in create vals directly - valid_vals_list = [] - record_vals_iter = iter(record_vals_list) - for vals in vals_list: - new_alias = not vals.get('alias_id') - if new_alias: - record_vals = next(record_vals_iter) - record_vals['alias_id'] = next(alias_ids) - valid_vals_list.append(record_vals) - else: - valid_vals_list.append(vals) - - records = super().create(valid_vals_list) - - for record in records: - record.alias_id.sudo().write(record._alias_get_creation_values()) - - return records - - def write(self, vals): - """ Split writable fields of mail.alias and other fields alias fields will - 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().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) - - return True - - def unlink(self): - """ Delete the given records, and cascade-delete their corresponding alias. """ - aliases = self.mapped('alias_id') - res = super().unlink() - aliases.sudo().unlink() - return res - - @api.returns(None, lambda value: value[0]) - def copy_data(self, default=None): - 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] - return [data] + def _require_new_alias(self, record_vals): + """ alias_id field is always required, due to inherits """ + return not record_vals.get('alias_id') def _init_column(self, name): """ Create aliases for existing rows. """ @@ -109,29 +50,3 @@ class AliasMixin(models.AbstractModel): record.with_context(mail_notrack=True).alias_id = alias _logger.info('Mail alias created for %s %s (id %s)', record._name, record.display_name, record.id) - - # -------------------------------------------------- - # MIXIN TOOL OVERRIDE METHODS - # -------------------------------------------------- - - def _alias_get_creation_values(self): - """ Return values to create an alias, or to write on the alias after its - creation. - """ - return { - 'alias_parent_thread_id': self.id if self.id else False, - 'alias_parent_model_id': self.env['ir.model']._get_id(self._name), - } - - def _alias_filter_fields(self, values, filters=False): - """ Split the vals dict into two dictionnary of vals, one for alias - field and the other for other fields """ - if not filters: - filters = self.env['mail.alias']._fields.keys() - alias_values, record_values = {}, {} - for fname in values.keys(): - if fname in filters: - alias_values[fname] = values.get(fname) - else: - record_values[fname] = values.get(fname) - return alias_values, record_values diff --git a/addons/mail/models/mail_alias_mixin_optional.py b/addons/mail/models/mail_alias_mixin_optional.py new file mode 100644 index 00000000000..de3014dc415 --- /dev/null +++ b/addons/mail/models/mail_alias_mixin_optional.py @@ -0,0 +1,142 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +import logging + +from odoo import api, fields, models + +_logger = logging.getLogger(__name__) + + +class AliasMixinOptional(models.AbstractModel): + """ A mixin for models that handles underlying 'mail.alias' records to use + the mail gateway. Field is not mandatory and its creation is done dynamically + based on given 'alias_name', allowing to gradually populate the alias table + without having void aliases as when used with an inherits-like implementation. + """ + _name = 'mail.alias.mixin.optional' + _description = 'Email Aliases Mixin (light)' + ALIAS_WRITEABLE_FIELDS = ['alias_name', 'alias_contact', 'alias_defaults', 'alias_bounced_content'] + + alias_id = fields.Many2one('mail.alias', string='Alias', ondelete="restrict", required=False) + alias_name = fields.Char(related='alias_id.alias_name', readonly=False) + alias_domain = fields.Char('Alias domain', compute='_compute_alias_domain') + alias_defaults = fields.Text(related='alias_id.alias_defaults') + + @api.depends('alias_name') + def _compute_alias_domain(self): + self.alias_domain = self.env["ir.config_parameter"].sudo().get_param("mail.catchall.domain") + + # -------------------------------------------------- + # CRUD + # -------------------------------------------------- + + @api.model_create_multi + def create(self, vals_list): + """ Create aliases using sudo if an alias is required, notably if its + name is given. """ + # prepare all alias values + alias_vals_list, record_vals_list = [], [] + for vals in vals_list: + if vals.get('alias_name'): + vals['alias_name'] = self.env['mail.alias']._sanitize_alias_name(vals['alias_name']) + if self._require_new_alias(vals): + alias_vals, record_vals = self._alias_filter_fields(vals) + alias_vals.update(self._alias_get_creation_values()) + alias_vals_list.append(alias_vals) + record_vals_list.append(record_vals) + + # create all aliases + alias_ids = [] + if alias_vals_list: + alias_ids = iter(self.env['mail.alias'].sudo().create(alias_vals_list).ids) + + # update alias values in create vals directly + valid_vals_list = [] + record_vals_iter = iter(record_vals_list) + for vals in vals_list: + if self._require_new_alias(vals): + record_vals = next(record_vals_iter) + record_vals['alias_id'] = next(alias_ids) + valid_vals_list.append(record_vals) + else: + valid_vals_list.append(vals) + + records = super().create(valid_vals_list) + + for record in records.filtered('alias_id'): + record.alias_id.sudo().write(record._alias_get_creation_values()) + + return records + + def write(self, vals): + """ Split writable fields of mail.alias and other fields alias fields will + write with sudo and the other normally. If alias does not exist and we + try to set a name, create the alias automatically. """ + # create missing aliases + if vals.get('alias_name'): + alias_create_values = [ + dict( + record._alias_get_creation_values(), + alias_name=self.env['mail.alias']._sanitize_alias_name(vals['alias_name']), + ) + for record in self.filtered(lambda rec: not rec.alias_id) + ] + if alias_create_values: + aliases = self.env['mail.alias'].sudo().create(alias_create_values) + for record, alias in zip(self.filtered(lambda rec: not rec.alias_id), aliases): + record.alias_id = alias.id + + alias_vals, record_vals = self._alias_filter_fields(vals, filters=self.ALIAS_WRITEABLE_FIELDS) + if 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) + + return True + + def unlink(self): + """ Delete the given records, and cascade-delete their corresponding alias. """ + aliases = self.mapped('alias_id') + res = super().unlink() + aliases.sudo().unlink() + return res + + @api.returns(None, lambda value: value[0]) + def copy_data(self, default=None): + 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] + return [data] + + def _require_new_alias(self, record_vals): + """ Create only if no existing alias, and if a name is given, to avoid + creating inactive aliases (falsy name). """ + return not record_vals.get('alias_id') and record_vals.get('alias_name') + + # -------------------------------------------------- + # MIXIN TOOL OVERRIDE METHODS + # -------------------------------------------------- + + def _alias_get_creation_values(self): + """ Return values to create an alias, or to write on the alias after its + creation. + """ + return { + 'alias_parent_thread_id': self.id if self.id else False, + 'alias_parent_model_id': self.env['ir.model']._get_id(self._name), + } + + def _alias_filter_fields(self, values, filters=False): + """ Split the vals dict into two dictionnary of vals, one for alias + field and the other for other fields """ + if not filters: + filters = self.env['mail.alias']._fields.keys() + alias_values, record_values = {}, {} + for fname in values.keys(): + if fname in filters: + alias_values[fname] = values.get(fname) + else: + record_values[fname] = values.get(fname) + return alias_values, record_values diff --git a/addons/test_mail/models/test_mail_models.py b/addons/test_mail/models/test_mail_models.py index f0227892689..484787c0711 100644 --- a/addons/test_mail/models/test_mail_models.py +++ b/addons/test_mail/models/test_mail_models.py @@ -51,6 +51,27 @@ class MailTestSimpleUnfollow(models.Model): email_from = fields.Char() +class MailTestAliasOptional(models.Model): + """ A chatter model inheriting from the alias mixin using optional alias_id + field, hence no inherits. """ + _description = 'Chatter Model using Optional Alias Mixin' + _name = 'mail.test.alias.optional' + _inherit = ['mail.alias.mixin.optional'] + + name = fields.Char() + company_id = fields.Many2one('res.company', default=lambda self: self.env.company) + email_from = fields.Char() + + def _alias_get_creation_values(self): + """ Updates itself """ + values = super()._alias_get_creation_values() + values['alias_model_id'] = self.env['ir.model']._get_id('mail.test.alias.optional') + if self.id: + values['alias_force_thread_id'] = self.id + values['alias_defaults'] = {'company_id': self.company_id.id} + return values + + class MailTestGateway(models.Model): """ A very simple model only inheriting from mail.thread to test pure mass mailing features and base performances. """ diff --git a/addons/test_mail/security/ir.model.access.csv b/addons/test_mail/security/ir.model.access.csv index 77ce6e4b639..24fa71f25f9 100644 --- a/addons/test_mail/security/ir.model.access.csv +++ b/addons/test_mail/security/ir.model.access.csv @@ -1,6 +1,8 @@ id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink access_mail_performance_thread,access_mail_performance_thread,model_mail_performance_thread,base.group_user,1,1,1,1 access_mail_performance_tracking_user,mail.performance.tracking,model_mail_performance_tracking,base.group_user,1,1,1,1 +access_mail_test_alias_optional_portal,mail.test.alias.optional.portal,model_mail_test_alias_optional,base.group_portal,1,0,0,0 +access_mail_test_alias_optional_user,mail.test.alias.optional.user,model_mail_test_alias_optional,base.group_user,1,1,1,1 access_mail_test_simple_portal,mail.test.simple.portal,model_mail_test_simple,base.group_portal,1,0,0,0 access_mail_test_simple_user,mail.test.simple.user,model_mail_test_simple,base.group_user,1,1,1,1 access_mail_test_simple_unfollow_portal,mail.test.simple.unfollow.portal,model_mail_test_simple_unfollow,base.group_portal,0,0,0,0 diff --git a/addons/test_mail/tests/test_mail_alias.py b/addons/test_mail/tests/test_mail_alias.py index fcb7e900fcc..5a2ea3e1522 100644 --- a/addons/test_mail/tests/test_mail_alias.py +++ b/addons/test_mail/tests/test_mail_alias.py @@ -3,6 +3,8 @@ import psycopg2 +from ast import literal_eval + from odoo import exceptions from odoo.addons.mail.tests.common import MailCommon from odoo.tests import tagged @@ -277,6 +279,60 @@ class TestMailAliasMixin(TestMailAliasCommon): with self.assertRaises(exceptions.ValidationError): record.write({'alias_defaults': "{'custom_field': brokendict"}) + @users('employee') + @mute_logger('odoo.addons.base.models.ir_model') + def test_alias_mixin_alias_id_management(self): + """ Test alias_id being not mandatory """ + record_wo_alias, record_w_alias = self.env['mail.test.alias.optional'].create([ + { + 'name': 'Test WoAlias Name', + }, { + 'alias_name': 'Alias Name', + 'name': 'Test WoAlias Name', + } + ]) + self.assertFalse(record_wo_alias.alias_id, 'Alias record not created if not necessary (no alias_name)') + self.assertFalse(record_wo_alias.alias_id.alias_name) + self.assertFalse(record_wo_alias.alias_id.alias_defaults) + self.assertFalse(record_wo_alias.alias_name) + self.assertTrue(record_w_alias.alias_id, 'Alias record created as alias_name was given') + self.assertEqual(record_w_alias.alias_id.alias_name, 'alias-name', 'Alias name should go through sanitize') + self.assertEqual( + literal_eval(record_w_alias.alias_id.alias_defaults), + {'company_id': self.env.company.id} + ) + self.assertEqual(record_w_alias.alias_name, 'alias-name', 'Alias name should go through sanitize') + self.assertEqual( + literal_eval(record_w_alias.alias_defaults), + {'company_id': self.env.company.id} + ) + + # update existing alias + record_w_alias.write({'alias_contact': 'followers', 'alias_name': 'Updated Alias Name'}) + self.assertEqual(record_w_alias.alias_id.alias_contact, 'followers') + self.assertEqual(record_w_alias.alias_id.alias_name, 'updated-alias-name') + self.assertEqual(record_w_alias.alias_name, 'updated-alias-name') + + # update non existing alias -> creates alias + record_wo_alias.write({'alias_name': 'trying a name'}) + self.assertTrue(record_wo_alias.alias_id, 'Alias record should have been created to store the name') + self.assertEqual(record_wo_alias.alias_id.alias_name, 'trying-a-name') + self.assertEqual( + literal_eval(record_wo_alias.alias_id.alias_defaults), + {'company_id': self.env.company.id} + ) + self.assertEqual(record_wo_alias.alias_name, 'trying-a-name') + self.assertEqual( + literal_eval(record_wo_alias.alias_defaults), + {'company_id': self.env.company.id} + ) + + # reset alias -> keep the alias as void, don't remove it + existing_aliases = record_wo_alias.alias_id + record_w_alias.alias_id + (record_wo_alias + record_w_alias).write({'alias_name': False}) + self.assertEqual((record_wo_alias + record_w_alias).alias_id, existing_aliases) + self.assertFalse(list(filter(None, existing_aliases.mapped('alias_name')))) + @users('employee') def test_alias_mixin_copy_content(self): self.assertFalse(self.env.user.has_group('base.group_system'), 'Test user should not have Administrator access') From 638e0f658dc94960ca1d8abda9d1591de7f9a02a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 7 Jul 2023 08:54:41 +0000 Subject: [PATCH 8/9] [REF] mail, various: cleanup alias usage Cleanup alias usage and definition. Prepare code to ease future changes and improvements. Notably * add a 'alias_email' computed field on the mixin allowing to have the complete alias email when set, and False in case it is inactive or linked to an inactive alias domain; * remove unnecessary alias_id field definition when just the help differs from the standard definition coming from the 'mail.alias.mixin'; * use fields coming from 'inherits' instead of using alias_id and its sub- fields; notably use 'alias_display_name' and 'alias_email' fields; * remove useless custom code and management; * improve alias parameters support code in configuration parameters; Task-3453343 (Mail: Cleanup Alias Usage) Prepares Task-36879 (Mail: Support MultiCompany Aliases) Part-of: odoo/odoo#130632 --- addons/crm/data/digest_data.xml | 6 +++--- addons/crm/models/crm_lead.py | 5 ++--- addons/crm/models/crm_team.py | 4 +--- addons/crm/tests/test_crm_lead.py | 2 +- .../crm/tests/test_crm_lead_multicompany.py | 2 +- addons/crm/views/crm_helper_templates.xml | 4 ++-- addons/hr_expense/models/hr_expense.py | 3 +-- addons/hr_recruitment/data/digest_data.xml | 4 ++-- addons/hr_recruitment/models/hr_applicant.py | 11 +++++----- addons/hr_recruitment/models/hr_job.py | 4 +--- .../models/hr_recruitment_source.py | 20 +++++++++---------- .../views/hr_recruitment_source_views.xml | 3 ++- .../mail/models/mail_alias_mixin_optional.py | 10 ++++++++++ addons/mail_group/__manifest__.py | 2 +- addons/mail_group/models/mail_group.py | 20 +++++-------------- addons/mail_group/views/mail_group_views.xml | 2 +- addons/mail_group/views/portal_templates.xml | 8 ++++---- addons/maintenance/models/maintenance.py | 4 +--- addons/project/__manifest__.py | 2 +- addons/project/data/digest_data.xml | 4 ++-- addons/project/models/project_project.py | 16 +++------------ .../project/views/project_project_views.xml | 8 ++++---- addons/test_mail/models/test_mail_models.py | 3 --- 23 files changed, 63 insertions(+), 84 deletions(-) diff --git a/addons/crm/data/digest_data.xml b/addons/crm/data/digest_data.xml index 9da9d78e512..d044815d18e 100644 --- a/addons/crm/data/digest_data.xml +++ b/addons/crm/data/digest_data.xml @@ -16,9 +16,9 @@

Tip: Convert incoming emails into opportunities

- -

Did you know emails sent to generate opportunities in your pipeline?
- Try sending an email to your CRM. This email address is configurable by sales team members.

+ +

Did you know emails sent to generate opportunities in your pipeline?
+ Try sending an email to your CRM. This email address is configurable by sales team members.

Did you know emails sent to a Sales Team alias generate opportunities in your pipeline?

diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index 1bcea9ed084..d8c789384e1 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -1276,10 +1276,9 @@ class Lead(models.Model): ('alias_force_thread_id', '=', False) ], limit=1) - if alias_record and alias_record.alias_domain and alias_record.alias_name: - email = f'{alias_record.alias_name}@{alias_record.alias_domain}' + if alias_record.alias_domain and alias_record.alias_name: sub_title = Markup(_('Use the New button, or send an email to %(email_link)s to test the email gateway.')) % { - 'email_link': Markup("%s") % (email, email), + 'email_link': Markup("%s") % (alias_record.display_name, alias_record.display_name), } return super().get_empty_list_help( f'

{help_title}

{sub_title}

' diff --git a/addons/crm/models/crm_team.py b/addons/crm/models/crm_team.py index 4ada6a3b3b2..40ccee93b3d 100644 --- a/addons/crm/models/crm_team.py +++ b/addons/crm/models/crm_team.py @@ -24,9 +24,7 @@ class Team(models.Model): use_leads = fields.Boolean('Leads', help="Check this box to filter and qualify incoming requests as leads before converting them into opportunities and assigning them to a salesperson.") use_opportunities = fields.Boolean('Pipeline', default=True, help="Check this box to manage a presales process with opportunities.") - alias_id = fields.Many2one( - 'mail.alias', string='Alias', ondelete="restrict", required=True, - help="The email address associated with this channel. New emails received will automatically create new leads assigned to the channel.") + alias_id = fields.Many2one(help="The email address associated with this channel. New emails received will automatically create new leads assigned to the channel.") # assignment assignment_enabled = fields.Boolean('Lead Assign', compute='_compute_assignment_enabled') assignment_auto_enabled = fields.Boolean('Auto Assignment', compute='_compute_assignment_enabled') diff --git a/addons/crm/tests/test_crm_lead.py b/addons/crm/tests/test_crm_lead.py index 0b5e33f0492..8cf4b4a20f3 100644 --- a/addons/crm/tests/test_crm_lead.py +++ b/addons/crm/tests/test_crm_lead.py @@ -627,7 +627,7 @@ class TestCRMLead(TestCrmCommon): new_lead = self.format_and_process( INCOMING_EMAIL, 'unknown.sender@test.example.com', - '%s@%s' % (self.sales_team_1.alias_name, self.alias_domain), + self.sales_team_1.alias_email, subject='Delivery cost inquiry', target_model='crm.lead', ) diff --git a/addons/crm/tests/test_crm_lead_multicompany.py b/addons/crm/tests/test_crm_lead_multicompany.py index 7b6118d5b3a..ff1d57f9544 100644 --- a/addons/crm/tests/test_crm_lead_multicompany.py +++ b/addons/crm/tests/test_crm_lead_multicompany.py @@ -314,7 +314,7 @@ class TestCRMLeadMultiCompany(TestCrmCommon): new_lead = self.format_and_process( INCOMING_EMAIL, customer_company.email, - '%s@%s' % (self.sales_team_1.alias_name, self.alias_domain), + self.sales_team_1.alias_email, subject='Team having partner in company', target_model='crm.lead', ) diff --git a/addons/crm/views/crm_helper_templates.xml b/addons/crm/views/crm_helper_templates.xml index f3610e25e58..6cd44f01253 100644 --- a/addons/crm/views/crm_helper_templates.xml +++ b/addons/crm/views/crm_helper_templates.xml @@ -1,11 +1,11 @@