From 041e9924c7abd8956639ac0c646f5ba7dad6fb76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 5 Jan 2023 17:12:27 +0000 Subject: [PATCH] [REF] mail: improve default record-based data in composer Purpose is to ease understanding of those default values and prepare move towards editable stored computed fields by rewriting a bit the code to better understand its purpose. While being at it, some tests introduced recently are moved at their right place now that everything is ready to rewrite the posting API and the composer. Some tests are added to ensure notification-specific methods are called along with the newly-introduced '_message_compute_subject', aka methods to add custom header and custom mail values. Task-2710804 (Mail: Clean MailThread Posting API) Prepares Task-2088884 (Mail: Use editable computed stored fields in compo Part-of: odoo/odoo#99482 --- addons/mail/models/mail_thread.py | 23 +++---- addons/mail/wizard/mail_compose_message.py | 36 ++++++----- addons/test_mail/models/test_mail_models.py | 15 +++++ addons/test_mail/tests/test_mail_composer.py | 67 +++++++++++++------- addons/test_mail/tests/test_message_post.py | 34 +++++----- 5 files changed, 107 insertions(+), 68 deletions(-) diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index aa1a5621414..4896930c645 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -2544,10 +2544,10 @@ class MailThread(models.AbstractModel): return parent_id def _message_compute_subject(self): - """Overridable method to get the default subject for a message related to this record. + """ Get the default subject for a message posted in this record's + discussion thread. - :return str: - """ + :return str: default subject """ self.ensure_one() return self.name_get()[0][1] @@ -3107,17 +3107,14 @@ class MailThread(models.AbstractModel): to notify (mail_message_id and its fields), server, references, subject. """ mail_subject = message.subject - if not message.subject: - thread = self - if not thread and message.model: - thread = self.env[message.model].browse(message.res_id) - if not thread: - mail_subject = message.record_name - else: - mail_subject = thread._message_compute_subject() + if not mail_subject and self: + mail_subject = self._message_compute_subject() + if not mail_subject: + mail_subject = message.record_name + if mail_subject: + # replace new lines by spaces to conform to email headers requirements + mail_subject = ' '.join(mail_subject.splitlines()) - # Replace new lines by spaces to conform to email headers requirements - mail_subject = ' '.join((mail_subject or '').splitlines()) # compute references: set references to the parent and add current message just to # have a fallback in case replies mess with Messsage-Id in the In-Reply-To (e.g. amazon # SES SMTP may replace Message-Id and In-Reply-To refers an internal ID not stored in Odoo) diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index d3cca56b7d7..76a23ba305b 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -290,25 +290,31 @@ class MailComposer(models.TransientModel): wizard when sending an email related a previous email (parent_id) or a document (model, res_id). This is based on previously computed default values. """ - result, subject = {}, False + result, record_name, subject = {}, False, False + model, res_id = values.get('model'), values.get('res_id') if values.get('parent_id'): parent = self.env['mail.message'].browse(values.get('parent_id')) - result['record_name'] = parent.record_name - if not values.get('model'): - result['model'] = parent.model - if not values.get('res_id'): - result['res_id'] = parent.res_id - partner_ids = values.get('partner_ids', list()) + parent.partner_ids.ids - result['partner_ids'] = partner_ids - record = self.env[values.get('model') or result['model']].browse(values.get('res_id') or result['res_id']) - parent_subject = tools.ustr(parent.subject or '') - subject = parent_subject or record._message_compute_subject() - elif values.get('model') and values.get('res_id'): - record = self.env[values['model']].browse(values['res_id']) - doc_name = record.display_name - result['record_name'] = doc_name or '' + if not model: + model = parent.model + result['model'] = model + if not res_id: + res_id = parent.res_id + result['res_id'] = res_id + result['partner_ids'] = values.get('partner_ids', list()) + parent.partner_ids.ids + record_name = parent.record_name + subject = tools.ustr(parent.subject or '') + if (not subject or not record_name) and model and res_id: + record = self.env[model].browse(res_id) + if not record_name: + record_name = record.display_name + if not subject: + subject = record._message_compute_subject() + elif model and res_id: + record = self.env[model].browse(res_id) + record_name = record.display_name or '' subject = record._message_compute_subject() + result['record_name'] = record_name result['subject'] = subject return result diff --git a/addons/test_mail/models/test_mail_models.py b/addons/test_mail/models/test_mail_models.py index 60976cb9c9f..dee3b5da83d 100644 --- a/addons/test_mail/models/test_mail_models.py +++ b/addons/test_mail/models/test_mail_models.py @@ -14,6 +14,21 @@ class MailTestSimple(models.Model): name = fields.Char() email_from = fields.Char() + def _message_compute_subject(self): + """ To ease mocks """ + _a = super()._message_compute_subject() + return _a + + def _notify_by_email_get_final_mail_values(self, *args, **kwargs): + """ To ease mocks """ + _a = super()._notify_by_email_get_final_mail_values(*args, **kwargs) + return _a + + def _notify_by_email_get_headers(self): + headers = super()._notify_by_email_get_headers() + headers['X-Custom'] = 'Done' + return headers + class MailTestGateway(models.Model): """ A very simple model only inheriting from mail.thread to test pure mass diff --git a/addons/test_mail/tests/test_mail_composer.py b/addons/test_mail/tests/test_mail_composer.py index dfabe4cb5d7..93ff0335c3d 100644 --- a/addons/test_mail/tests/test_mail_composer.py +++ b/addons/test_mail/tests/test_mail_composer.py @@ -121,6 +121,8 @@ class TestComposerForm(TestMailComposer): self.assertFalse(composer_form.reply_to_force_new) self.assertEqual(composer_form.res_id, self.test_record.id) self.assertEqual(composer_form.subject, self.test_record._message_compute_subject()) + self.assertIn(f'Ticket for {self.test_record.name}', composer_form.subject, + 'Check effective content') self.assertEqual(composer_form.subtype_id, self.env.ref('mail.mt_comment')) @users('employee') @@ -626,6 +628,8 @@ class TestComposerInternals(TestMailComposer): self.assertEqual(composer.record_name, 'Manual update', 'TODO: Reset not called') self.assertEqual(composer.subject, self.test_record._message_compute_subject()) + self.assertIn(f'Ticket for {self.test_record.name}', composer.subject, + 'Check effective content') else: self.assertFalse(composer.body) # self.assertFalse(composer.mail_server_id.id) @@ -693,25 +697,6 @@ class TestComposerInternals(TestMailComposer): self.assertEqual(composer.record_name, 'CustomName') self.assertEqual(composer.subject, 'My amazing subject') - @users('employee') - def test_mail_composer_default_subject(self): - """Make sure the default subject is applied in the composer.""" - simple_record = self.env['mail.test.simple'].create({'name': 'TestA'}) - ticket_record = self.env['mail.test.ticket'].create({'name': 'Test1'}) - # default behaviour, use the record name - ctx = self._get_web_context(simple_record, add_web=False, default_composition_mode='comment') - _, message = self.env['mail.compose.message'].with_context(ctx).create({ - 'body': '

Test Body

', - })._action_send_mail() - - self.assertEqual(message.subject, simple_record.name) - # custom subject - ctx = self._get_web_context(ticket_record, add_web=False, default_composition_mode='comment') - _, messages = self.env['mail.compose.message'].with_context(ctx).create({ - 'body': '

Test Body

', - })._action_send_mail() - self.assertEqual(messages.subject, ticket_record._message_compute_subject()) - @users('employee') @mute_logger('odoo.models.unlink') def test_mail_composer_recipients(self): @@ -884,10 +869,12 @@ class TestComposerInternals(TestMailComposer): def test_mail_composer_parent(self): """ Test specific management in comment mode when having parent_id set: record_name, subject, parent's partners. """ - subject = "Parent Subject" - parent = self.test_record.message_post(subject=subject, - body='Test', - partner_ids=(self.partner_1 + self.partner_2).ids) + parent_subject = "Parent Subject" + parent = self.test_record.message_post( + body='Test', + partner_ids=(self.partner_1 + self.partner_2).ids, + subject=parent_subject, + ) composer = self.env['mail.compose.message'].with_context( self._get_web_context(self.test_record, add_web=False, default_parent_id=parent.id) @@ -900,7 +887,7 @@ class TestComposerInternals(TestMailComposer): self.assertEqual(composer.parent_id, parent) self.assertEqual(composer.partner_ids, self.partner_1 + self.partner_2) self.assertEqual(composer.record_name, self.test_record.name) - self.assertEqual(composer.subject, subject) + self.assertEqual(composer.subject, parent_subject) @users('user_rendering_restricted') @mute_logger('odoo.tests', 'odoo.addons.base.models.ir_rule', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') @@ -992,6 +979,38 @@ class TestComposerResultsComment(TestMailComposer): """ Test global output of composer used in comment mode. Test notably notification and emails generated during this process. """ + @users('employee') + def test_mail_composer_default_subject(self): + """ Make sure the default subject is applied in the composer. """ + simple_record = self.env['mail.test.simple'].create({'name': 'TestSimple'}) + ticket_record = self.env['mail.test.ticket'].create({'name': 'TestTicket'}) + + # default behavior: use record name + _, message = self.env['mail.compose.message'].with_context( + self._get_web_context(simple_record, add_web=False, composition_mode='comment') + ).create({ + 'body': '

Test Body

', + })._action_send_mail() + self.assertEqual(message.subject, simple_record.name) + + # overridden in model + _, message = self.env['mail.compose.message'].with_context( + self._get_web_context(ticket_record, add_web=False, composition_mode='comment') + ).create({ + 'body': '

Test Body

', + })._action_send_mail() + self.assertEqual(message.subject, ticket_record._message_compute_subject()) + self.assertIn(f'Ticket for {ticket_record.name}', message.subject) + + # forced value + _, message = self.env['mail.compose.message'].with_context( + self._get_web_context(ticket_record, add_web=False, composition_mode='comment') + ).create({ + 'body': '

Test Body

', + 'subject': 'Forced Subject', + })._action_send_mail() + self.assertEqual(message.subject, 'Forced Subject',) + @users('employee') @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') def test_mail_composer_notifications_delete(self): diff --git a/addons/test_mail/tests/test_message_post.py b/addons/test_mail/tests/test_message_post.py index 2e1f05ccf0b..fe492e9c55d 100644 --- a/addons/test_mail/tests/test_message_post.py +++ b/addons/test_mail/tests/test_message_post.py @@ -325,20 +325,6 @@ class TestMessageNotify(TestMessagePostCommon): subject='Notify Subject', ) - @users('employee') - def test_notify_default_subject(self): - """ Test notify in batch. Currently not supported. """ - test_partner = self.env['res.partner'].sudo().create([{'name': 'John', 'email': 'john@test.lan'}]) - test_record = self.env['mail.test.ticket'].create({'name': 'Test'}) - mt_comment = self.env['ir.model.data']._xmlid_to_res_id('mail.mt_comment') - test_record.message_subscribe([test_partner.id], subtype_ids=[mt_comment]) - - with self.mock_mail_gateway(), self.mock_mail_app(): - test_record.message_post(body="Test Message", message_type="comment", subtype_id=mt_comment) - - self.assertEqual(len(self._new_msgs), 1) - self.assertEqual(self._new_msgs.subject, test_record._message_compute_subject()) - @users('employee') @mute_logger('odoo.addons.mail.models.mail_mail') def test_notify_from_user_id(self): @@ -630,20 +616,35 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase): partner_ids=self.partner_portal.ids, ) - @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') + @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink', 'odoo.tests') @users('employee') def test_message_post_defaults(self): """ Test default values when posting a classic message. """ + _original_compute_subject = MailTestSimple._message_compute_subject + _original_notify_headers = MailTestSimple._notify_by_email_get_headers + _original_notify_mailvals = MailTestSimple._notify_by_email_get_final_mail_values test_record = self.env['mail.test.simple'].create([{'name': 'Defaults'}]) creation_msg = test_record.message_ids self.assertEqual(len(creation_msg), 1) - with self.mock_mail_app(): + with patch.object(MailTestSimple, '_message_compute_subject', + autospec=True, side_effect=_original_compute_subject) as mock_compute_subject, \ + patch.object(MailTestSimple, '_notify_by_email_get_headers', + autospec=True, side_effect=_original_notify_headers) as mock_notify_headers, \ + patch.object(MailTestSimple, '_notify_by_email_get_final_mail_values', + autospec=True, side_effect=_original_notify_mailvals) as mock_notify_mailvals, \ + self.mock_mail_app(): new_message = test_record.message_post( body='Body', partner_ids=[self.partner_employee_2.id], ) + self.assertEqual(mock_compute_subject.call_count, 1, + 'Should call model-based subject computation for outgoing emails') + self.assertEqual(mock_notify_headers.call_count, 1, + 'Should call model-based headers computation for outgoing emails') + self.assertEqual(mock_notify_mailvals.call_count, 1, + 'Should call model-based headers computation for outgoing emails') self.assertMessageFields( new_message, {'author_id': self.partner_employee, @@ -657,6 +658,7 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase): 'record_name': test_record.name, 'reply_to': formataddr((f'{self.company_admin.name} {test_record.name}', f'{self.alias_catchall}@{self.alias_domain}')), 'res_id': test_record.id, + 'subject': test_record.name, 'subtype_id': self.env.ref('mail.mt_note'), } )