From 4924df5743822ff0d71c914d6ed5196c8f310c99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 20 Oct 2022 14:11:23 +0000 Subject: [PATCH] [REF] mail: move composer 'author_id' / 'email_from' fields from onchange to computed RATIONALE Remove _onchange_template_id / defaults usage for composer fields. It lacks control and make value computation hard to understand and predict. PURPOSE Split _onchange_template_id + default_get + get_record_data usage into editable computed stored fields. Each field should have its own method with its triggers (template, composition mode, ...). SPECIFICATIONS Improve composer to correctly compute fields ``email_from`` and ``author_id`` in compute methods. Email_from computation is coming either from template, either from context. When having a template with a value set, copy it (in batch mode) or render it (in monorecord comment mode) on the composer. Otherwise try to take current user's email. When removing the template, fallback on default thread behavior (which is current user's email). Author computation is not controllable from the template currently. We therefore try to synchronize it with the given email_from (in rendered mode to avoid trying to find partner based on qweb expressions), or fallback on current user. Task-2088884 (Mail: Use editable computed stored fields in composer) Part-of: odoo/odoo#107356 --- .../tests/test_mail_performance.py | 16 ++- addons/mail/wizard/mail_compose_message.py | 100 ++++++++++++++---- addons/test_mail/tests/test_mail_composer.py | 55 +++++----- 3 files changed, 117 insertions(+), 54 deletions(-) diff --git a/addons/l10n_generic_coa/tests/test_mail_performance.py b/addons/l10n_generic_coa/tests/test_mail_performance.py index 7a4d19b58b3..eb32053b6f4 100644 --- a/addons/l10n_generic_coa/tests/test_mail_performance.py +++ b/addons/l10n_generic_coa/tests/test_mail_performance.py @@ -269,10 +269,8 @@ class TestAccountComposerPerformance(BaseMailAccountPerformance): self.assertEqual(len(print_msg.attachment_ids), 3) self.assertNotIn(self.attachments, print_msg.attachment_ids, 'Attachments should be duplicated, not just linked') - self.assertEqual(print_msg.author_id, self.env.user.partner_id, - 'TODO: not synchronized with email_from choice') - # self.assertEqual(print_msg.author_id, self.user_account_other.partner_id, - # 'Should take invoice_user_id partner') + self.assertEqual(print_msg.author_id, self.user_account_other.partner_id, + 'Should take invoice_user_id partner') self.assertEqual(print_msg.email_from, self.user_account_other.email_formatted, 'Should take invoice_user_id email') self.assertEqual(print_msg.notified_partner_ids, test_customer + self.user_accountman.partner_id) @@ -285,7 +283,7 @@ class TestAccountComposerPerformance(BaseMailAccountPerformance): self.assertMailMail( test_customer, 'sent', - author=self.user_account.partner_id, # author: current user, not synchronized with email_from of template + author=self.user_account_other.partner_id, # author: synchronized with email_from of template content=f'TemplateBody for {test_move.name}', email_values={ 'attachments_info': [ @@ -373,10 +371,8 @@ class TestAccountComposerPerformance(BaseMailAccountPerformance): self.assertEqual(len(print_msg.attachment_ids), 3) self.assertNotIn(self.attachments, print_msg.attachment_ids, 'Attachments should be duplicated, not just linked') - self.assertEqual(print_msg.author_id, self.env.user.partner_id, - 'TODO: not synchronized with email_from choice') - # self.assertEqual(print_msg.author_id, self.user_account_other.partner_id, - # 'Should take invoice_user_id partner') + self.assertEqual(print_msg.author_id, self.user_account_other.partner_id, + 'Should take invoice_user_id partner') self.assertEqual(print_msg.email_from, self.user_account_other.email_formatted, 'Should take invoice_user_id email') self.assertEqual(print_msg.notified_partner_ids, test_customer + self.user_accountman.partner_id) @@ -389,7 +385,7 @@ class TestAccountComposerPerformance(BaseMailAccountPerformance): self.assertMailMail( test_customer, 'sent', - author=self.user_account.partner_id, # author: current user, not synchronized with email_from of template + author=self.user_account_other.partner_id, # author: synchronized with email_from of template content=f'SpanishBody for {test_move.name}', # translated version email_values={ 'attachments_info': [ diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index bda09c5cc3d..0ac29ba80b2 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -9,7 +9,7 @@ import logging from odoo import _, api, fields, models, tools, Command from odoo.exceptions import UserError, ValidationError from odoo.osv import expression -from odoo.tools import email_re +from odoo.tools import email_re, is_html_empty _logger = logging.getLogger(__name__) @@ -65,16 +65,6 @@ class MailComposer(models.TransientModel): result = super().default_get(fields_list) - # author - missing_author = 'author_id' in fields_list and 'author_id' not in result - missing_email_from = 'email_from' in fields_list and 'email_from' not in result - if missing_author or missing_email_from: - author_id, email_from = self.env['mail.thread']._message_compute_author(result.get('author_id'), result.get('email_from'), raise_on_email=False) - if missing_email_from: - result['email_from'] = email_from - if missing_author: - result['author_id'] = author_id - # record context management if 'model' in fields_list and 'model' not in result: result['model'] = self.env.context.get('active_model') @@ -134,9 +124,12 @@ class MailComposer(models.TransientModel): email_layout_xmlid = fields.Char('Email Notification Layout', copy=False) email_add_signature = fields.Boolean(default=True) # origin - email_from = fields.Char('From', help="Email address of the sender. This field is set when no matching partner is found and replaces the author_id field in the chatter.") + email_from = fields.Char( + 'From', compute='_compute_authorship', readonly=False, store=True, + help="Email address of the sender. This field is set when no matching partner is found and replaces the author_id field in the chatter.") author_id = fields.Many2one( - 'res.partner', 'Author', + 'res.partner', string='Author', + compute='_compute_authorship', readonly=False, store=True, help="Author of the message. If not set, email_from may hold an email address that did not match any partner.") # composition composition_mode = fields.Selection( @@ -214,6 +207,47 @@ class MailComposer(models.TransientModel): for composer in self: composer._evaluate_res_domain() + @api.depends('composition_mode', 'email_from', 'model', + 'res_domain', 'res_ids', 'template_id') + def _compute_authorship(self): + """ Computation is coming either from template, either from context. + When having a template with a value set, copy it (in batch mode) or + render it (in monorecord comment mode) on the composer. Otherwise + try to take current user's email. When removing the template, fallback + on default thread behavior (which is current user's email). + + Author is not controllable from the template currently. We therefore + try to synchronize it with the given email_from (in rendered mode to + avoid trying to find partner based on qweb expressions), or fallback + on current user. """ + Thread = self.env['mail.thread'].with_context(active_test=False) + for composer in self: + rendering_mode = composer.composition_mode == 'comment' and not composer.composition_batch + updated_author_id = None + + # update email_from first as it is the main used field currently + if composer.template_id.email_from: + composer._set_value_from_template('email_from') + # removing template or void from -> fallback on current user as default + elif not composer.template_id or not composer.email_from: + composer.email_from = self.env.user.email_formatted + updated_author_id = self.env.user.partner_id.id + + # Update author. When being in rendered mode: link with rendered + # email_from or fallback on current user if email does not match. + # When changing template in raw mode or resetting also fallback. + if composer.email_from and rendering_mode and not updated_author_id: + updated_author_id, _ = Thread._message_compute_author( + None, composer.email_from, raise_on_email=False, + ) + if not updated_author_id: + updated_author_id = self.env.user.partner_id.id + if not rendering_mode or not composer.template_id: + updated_author_id = self.env.user.partner_id.id + + if updated_author_id: + composer.author_id = updated_author_id + @api.depends('res_domain', 'res_ids') def _compute_composition_batch(self): """ Determine if batch mode is activated: @@ -349,8 +383,7 @@ class MailComposer(models.TransientModel): template = self.env['mail.template'].browse(template_id) values = dict( (field, template[field]) - for field in ('email_from', - 'reply_to', + for field in ('reply_to', 'scheduled_date', 'subject', ) @@ -372,7 +405,6 @@ class MailComposer(models.TransientModel): ('attachment_ids', 'body_html', 'email_cc', - 'email_from', 'email_to', 'mail_server_id', 'partner_ids', @@ -405,7 +437,6 @@ class MailComposer(models.TransientModel): ).default_get(['attachment_ids', 'body', 'composition_mode', - 'email_from', 'mail_server_id', 'model', 'parent_id', @@ -419,7 +450,6 @@ class MailComposer(models.TransientModel): (key, default_values[key]) for key in ('attachment_ids', 'body', - 'email_from', 'mail_server_id', 'partner_ids', 'reply_to', @@ -1144,3 +1174,37 @@ class MailComposer(models.TransientModel): if not tools.is_list_of(res_ids, int): raise ValidationError(error_msg) return res_ids + + def _set_value_from_template(self, template_fname, composer_fname=False): + """ Set composer value from its template counterpart. In monorecord + comment mode, we get directly the rendered value, giving the real + value to the user. Otherwise we get the raw (unrendered) value from + template, as it will be rendered at send time (for mass mail, whatever + the number of contextual records to mail) or before posting on records + (for comment in batch). + + :param str template_fname: name of field on template model, used to + fetch the value (and maybe render it); + :param str composer_fname: name of field on composer model, when field + names do not match (e.g. body_html on template used to populate body + on composer); + """ + self.ensure_one() + composer_fname = composer_fname or template_fname + + # fetch template value, check if void + template_value = self.template_id[template_fname] if self.template_id else False + if template_value and template_fname == 'body_html': + template_value = template_value if not is_html_empty(template_value) else False + + if template_value: + if self.composition_mode == 'comment' and not self.composition_batch: + res_ids = self._evaluate_res_ids() + rendering_res_ids = res_ids or [0] + self[composer_fname] = self.template_id._generate_template( + rendering_res_ids, + {template_fname}, + )[rendering_res_ids[0]][template_fname] + else: + self[composer_fname] = self.template_id[template_fname] + return self[composer_fname] diff --git a/addons/test_mail/tests/test_mail_composer.py b/addons/test_mail/tests/test_mail_composer.py index 92025741e3e..3cf2d6e2646 100644 --- a/addons/test_mail/tests/test_mail_composer.py +++ b/addons/test_mail/tests/test_mail_composer.py @@ -79,7 +79,7 @@ class TestMailComposer(TestMailCommon, TestRecipients): } ]) - cls.test_from = '"John Doe" ' + cls.test_from = '"John Doe" ' cls.template = cls.env['mail.template'].create({ 'auto_delete': True, @@ -207,7 +207,8 @@ class TestComposerForm(TestMailComposer): )) self.assertTrue(composer_form.auto_delete, 'Should take template value') self.assertFalse(composer_form.auto_delete_keep_log, 'MailComposer: keep_log makes no sense in comment mode, only auto_delete') - self.assertEqual(composer_form.author_id, self.env.user.partner_id) + self.assertEqual(composer_form.author_id, self.user_employee_2.partner_id, + 'MailComposer: author is synchronized with email_from when possible') self.assertEqual(composer_form.body, f'

TemplateBody {self.test_record.name}

') self.assertFalse(composer_form.composition_batch) self.assertEqual(composer_form.composition_mode, 'comment') @@ -579,52 +580,54 @@ class TestComposerInternals(TestMailComposer): # changing template should update its email_from composer.write({'template_id': self.template.id}) - # currently onchange necessary - composer._onchange_template_id_wrapper() if composition_mode == 'comment' and not batch: - self.assertEqual(composer.author_id, self.partner_1, - 'MailComposer: TODO: should try to synchronize with email_from') + self.assertEqual(composer.author_id, self.test_record.user_id.partner_id, + f'MailComposer: should try to link in rendered mode: {composer.author_id.name}, expected {self.env.user.name}') self.assertEqual(composer.email_from, self.test_record.user_id.email_formatted, - f'MailComposer: should take email_from rendered from template ({composition_mode}-{batch})') + 'MailComposer: should take email_from rendered from template') else: - self.assertEqual(composer.author_id, self.partner_1, - 'MailComposer: TODO: email_from is raw, what to do with it ?') + self.assertEqual(composer.author_id, self.env.user.partner_id, + f'MailComposer: should reset to current user in raw mode: {composer.author_id.name}, expected {self.env.user.name}') self.assertEqual(composer.email_from, self.template.email_from, - f'MailComposer: should take email_from raw from template ({composition_mode}-{batch})') + 'MailComposer: should take email_from raw from template') - # manual values are kept over template values + # manual values are kept over template values; if email does not + # match any author, reset author composer.write({'email_from': self.test_from}) - self.assertEqual(composer.author_id, self.partner_1) - self.assertEqual(composer.email_from, self.test_from, - 'MailComposer: TODO: author / email_from are not synchronized') + if composition_mode == 'comment' and not batch: + self.assertEqual(composer.author_id, self.test_record.user_id.partner_id, + 'MailComposer: TODO: compute not called') + self.assertEqual(composer.email_from, self.test_from, + 'MailComposer: manual values should be kept') + else: + self.assertEqual(composer.author_id, self.env.user.partner_id, + 'MailComposer: TODO: compute not called') + self.assertEqual(composer.email_from, self.test_from, + 'MailComposer: manual values should be kept') # update with template with void values: void value is not forced in # rendering mode as well as when copying template values composer.write({'template_id': template_void.id}) - # currently onchange necessary - composer._onchange_template_id_wrapper() if composition_mode == 'comment' and not batch: - self.assertEqual(composer.author_id, self.partner_1, + self.assertEqual(composer.author_id, self.env.user.partner_id, 'MailComposer: TODO: author / email_from are not synchronized') self.assertEqual(composer.email_from, self.test_from) else: - self.assertEqual(composer.author_id, self.partner_1, + self.assertEqual(composer.author_id, self.env.user.partner_id, 'MailComposer: TODO: author / email_from are not synchronized') self.assertEqual(composer.email_from, self.test_from) # reset template: values are reset due to call to default_get composer.write({'template_id': False}) - # currently onchange necessary - composer._onchange_template_id_wrapper() if composition_mode == 'comment' and not batch: - self.assertEqual(composer.author_id, self.partner_1, + self.assertEqual(composer.author_id, self.env.user.partner_id, 'MailComposer: TODO: author / email_from are not synchronized') self.assertEqual(composer.email_from, self.env.user.email_formatted) else: - self.assertEqual(composer.author_id, self.partner_1, + self.assertEqual(composer.author_id, self.env.user.partner_id, 'MailComposer: TODO: author / email_from are not synchronized') self.assertEqual(composer.email_from, self.env.user.email_formatted) @@ -1412,7 +1415,7 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): 'Author cannot be synchronized with a raw email_from') self.assertEqual(composer.email_from, self.template.email_from) else: - author = self.env.user.partner_id + author = self.partner_employee_2 self.assertEqual(composer.author_id, author, 'Author cannot be synchronized with a raw email_from') self.assertEqual(composer.email_from, self.partner_employee_2.email_formatted) @@ -1488,7 +1491,7 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): message = test_record.message_ids[0] self.assertMailMail(self.partner_employee_2, 'sent', mail_message=message, - author=author, # author != email_from (template sets only email_from) + author=author, # author is different in batch and monorecord mode (raw or rendered email_from) email_values={ 'body_content': f'TemplateBody {test_record.name}', 'email_from': test_record.user_id.email_formatted, # set by template @@ -1506,7 +1509,7 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): ) self.assertMailMail(test_record.customer_id + new_partners, 'sent', mail_message=message, - author=author, # author != email_from (template sets only email_from) + author=author, # author is different in batch and monorecord mode (raw or rendered email_from) email_values={ 'body_content': f'TemplateBody {test_record.name}', 'email_from': test_record.user_id.email_formatted, # set by template @@ -1627,7 +1630,7 @@ class TestComposerResultsCommentStatus(TestMailComposer): self.assertMailMail( recipient, 'sent', mail_message=message, - author=self.partner_employee, # author != email_from (template sets only email_from) + author=self.partner_employee_2, # author synchronized with email_from email_values={ 'email_from': self.user_employee_2.email_formatted, # set by template },