From f039f57eb8a8d01228b747089686a5210facf133 Mon Sep 17 00:00:00 2001 From: Renaud Thiry Date: Tue, 6 Sep 2022 13:05:33 +0000 Subject: [PATCH] [IMP] Add more detailed default email subjects from chatter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All emails sent from the chatter start with "Re:" followed by the name of the record. The name of the record alone is sometimes not enough for the followers to understand what the mail is about. Additionally, "Re:" does not make sense when starting a conversation. This commit gives better default subject for event registrations and allows thread models to override the default subject of messages. This also removes "Re:" from default mail subjects. Task-2833215 closes odoo/odoo#95817 Signed-off-by: Stéphane Debauche (std) --- addons/event/models/event_registration.py | 33 +++++----- addons/im_livechat/tests/test_message.py | 1 + addons/mail/models/mail_message.py | 10 +-- addons/mail/models/mail_thread.py | 19 +++++- .../static/src/components/message/message.xml | 2 +- addons/mail/static/src/models/message.js | 25 +++++--- .../mock_server/models/mail_message.js | 2 + .../helpers/mock_server/models/mail_thread.js | 11 ++++ .../helpers/mock_server/models/res_fake.js | 9 +++ .../components/chatter_tests.js | 63 ++++++++++++++++++- .../tours/mail_full_composer_test_tour.js | 4 +- addons/mail/wizard/mail_compose_message.py | 14 ++--- addons/test_mail/models/test_mail_models.py | 4 ++ addons/test_mail/tests/test_mail_composer.py | 34 +++++++--- addons/test_mail/tests/test_message_post.py | 16 ++++- 15 files changed, 196 insertions(+), 51 deletions(-) diff --git a/addons/event/models/event_registration.py b/addons/event/models/event_registration.py index c94040fbc61..d1ee3e7a368 100644 --- a/addons/event/models/event_registration.py +++ b/addons/event/models/event_registration.py @@ -152,25 +152,9 @@ class EventRegistration(models.Model): return ret def name_get(self): - """ Custom name_get implementation to better differentiate registrations - linked to a given partner but with different name (one partner buying - several registrations) - - * name, partner_id has no name -> take name - * partner_id has name, name void or same -> take partner name - * both have name: partner + name + """ Custom name_get in case a registration is nott linked to an attendee """ - ret_list = [] - for registration in self: - if registration.partner_id.name: - if registration.name and registration.name != registration.partner_id.name: - name = '%s, %s' % (registration.partner_id.name, registration.name) - else: - name = registration.partner_id.name - else: - name = registration.name - ret_list.append((registration.id, name)) - return ret_list + return [(registration.id, registration.name or f"#{registration.id}") for registration in self] def toggle_active(self): pre_inactive = self - self.filtered(self._active_name) @@ -252,6 +236,19 @@ class EventRegistration(models.Model): # MAILING / GATEWAY # ------------------------------------------------------------ + def _message_compute_subject(self): + if self.name: + return _( + "%(event_name)s - Registration for %(attendee_name)s", + event_name=self.event_id.name, + attendee_name=self.name, + ) + return _( + "%(event_name)s - Registration #%(registration_id)s", + event_name=self.event_id.name, + registration_id=self.id, + ) + def _message_get_suggested_recipients(self): recipients = super(EventRegistration, self)._message_get_suggested_recipients() public_users = self.env['res.users'].sudo() diff --git a/addons/im_livechat/tests/test_message.py b/addons/im_livechat/tests/test_message.py index 7ef9dde9c5a..3957660a45c 100644 --- a/addons/im_livechat/tests/test_message.py +++ b/addons/im_livechat/tests/test_message.py @@ -54,6 +54,7 @@ class TestImLivechatMessage(TransactionCase): 'guestAuthor': [('clear',)], 'history_partner_ids': [], 'id': message.id, + 'default_subject': channel_livechat_1.name, 'is_discussion': False, 'is_note': True, 'is_notification': False, diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index 1ca454b6897..8f28628dc9c 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -846,13 +846,12 @@ class Message(models.Model): 'name': message_sudo.author_guest_id.name, } if message_sudo.author_guest_id else [('clear',)] if message_sudo.model and message_sudo.res_id: - record_name = self.env[message_sudo.model] \ - .browse(message_sudo.res_id) \ - .sudo() \ - .with_prefetch(thread_ids_by_model_name[message_sudo.model]) \ - .display_name + record_sudo = self.env[message_sudo.model].browse(message_sudo.res_id).sudo() + record_name = record_sudo.with_prefetch(thread_ids_by_model_name[message_sudo.model]).display_name + default_subject = record_sudo._message_compute_subject() else: record_name = False + default_subject = False reactions_per_content = defaultdict(lambda: self.env['mail.message.reaction']) for reaction in message_sudo.reaction_ids: reactions_per_content[reaction.content] |= reaction @@ -868,6 +867,7 @@ class Message(models.Model): allowed_tracking_ids = message_sudo.tracking_value_ids.filtered(lambda tracking: not tracking.field_groups or self.env.is_superuser() or self.user_has_groups(tracking.field_groups)) vals.update({ 'author': author, + 'default_subject': default_subject, 'guestAuthor': guestAuthor, 'notifications': message_sudo.notification_ids._filtered_for_web_client()._notification_format(), 'attachment_ids': message_sudo.attachment_ids._attachment_format() if not legacy else message_sudo.attachment_ids._attachment_format(legacy=True), diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 7b6e412e194..763889896dd 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -2320,6 +2320,14 @@ class MailThread(models.AbstractModel): parent_id = current_ancestor.parent_id.id if current_ancestor.parent_id else parent_id return parent_id + def _message_compute_subject(self): + """Overridable method to get the default subject for a message related to this record. + + :return str: + """ + self.ensure_one() + return self.name_get()[0][1] + def _message_create(self, values_list): if not isinstance(values_list, (list)): values_list = [values_list] @@ -2739,7 +2747,16 @@ class MailThread(models.AbstractModel): :param dict base_mail_values: base mail.mail values, holding message to notify (mail_message_id and its fields), server, references, subject. """ - mail_subject = message.subject or (message.record_name and 'Re: %s' % message.record_name) # in cache, no queries + 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() + # 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 diff --git a/addons/mail/static/src/components/message/message.xml b/addons/mail/static/src/components/message/message.xml index b31b587da8d..ed5c98dfcfd 100644 --- a/addons/mail/static/src/components/message/message.xml +++ b/addons/mail/static/src/components/message/message.xml @@ -119,7 +119,7 @@ 'border-warning bg-warning-light opacity-50': message.isHighlighted and (message.isDiscussionOrNotification or message.message_type === 'sms'), }" /> - Subject: + Subject:
} + * Simulate `_message_compute_subject` on `mail.thread` + */ + mockMailThread_MessageComputeSubject(model, ids) { + const records = this.getRecords(model, [["id", "in", ids]]); + return new Map(records.map(record => [record.id, record.name || ''])); + }, /** * Simulates `_message_add_suggested_recipient` on `mail.thread`. * diff --git a/addons/mail/static/tests/helpers/mock_server/models/res_fake.js b/addons/mail/static/tests/helpers/mock_server/models/res_fake.js index a4791d8f7a7..8cbbc7822c5 100644 --- a/addons/mail/static/tests/helpers/mock_server/models/res_fake.js +++ b/addons/mail/static/tests/helpers/mock_server/models/res_fake.js @@ -35,4 +35,13 @@ patch(MockServer.prototype, "mail/models/res_fake", { } return result; }, + /** + * @override + */ + mockMailThread_MessageComputeSubject(model, ids) { + if (model === 'res.fake') { + return new Map(ids.map(id => [id, "Custom Default Subject"])); + } + return this._super(model, ids); + }, }); diff --git a/addons/mail/static/tests/qunit_suite_tests/components/chatter_tests.js b/addons/mail/static/tests/qunit_suite_tests/components/chatter_tests.js index ca6fa36e807..107ad30848a 100644 --- a/addons/mail/static/tests/qunit_suite_tests/components/chatter_tests.js +++ b/addons/mail/static/tests/qunit_suite_tests/components/chatter_tests.js @@ -371,7 +371,7 @@ QUnit.module("mail", {}, function () { ); QUnit.test( - "should display subject when subject is not the same as the thread name", + "should display subject when subject isn't infered from the record", async function (assert) { assert.expect(2); @@ -428,11 +428,70 @@ QUnit.module("mail", {}, function () { assert.containsNone( document.body, ".o_MessageView_subject", - "should not display subject of the message" + "should not display subject of the message", ); } ); + QUnit.test( + "should not display subject when subject is the same as the default subject", + async function (assert) { + assert.expect(1); + + const pyEnv = await startServer(); + const fakeRecordId = pyEnv["res.fake"].create({ + name: "Salutations, voyageur", + }); + pyEnv["mail.message"].create({ + body: "not empty", + model: "res.fake", + res_id: fakeRecordId, + subject: "Custom Default Subject", // default subject for res.fake, set on the model + }); + const { openView } = await start(); + await openView({ + res_id: fakeRecordId, + res_model: "res.fake", + views: [[false, "form"]], + }); + + assert.containsNone( + document.body, + ".o_MessageView_subject", + "should not display subject of the message", + ); + } + ); + + QUnit.test( + "should not display subject when subject is the same as the thread name with custom default subject", + async function (assert) { + assert.expect(1); + + const pyEnv = await startServer(); + const fakeRecordId = pyEnv["res.fake"].create({ + name: "Salutations, voyageur", + }); + pyEnv["mail.message"].create({ + body: "not empty", + model: "res.fake", + res_id: fakeRecordId, + subject: "Salutations, voyageur", + }); + const { openView } = await start(); + await openView({ + res_id: fakeRecordId, + res_model: "res.fake", + views: [[false, "form"]], + }); + + assert.containsNone( + document.body, + ".o_MessageView_subject", + "should not display subject of the message", + ); + } + ); QUnit.test( "should not display user notification messages in chatter", async function (assert) { diff --git a/addons/mail/static/tests/tours/mail_full_composer_test_tour.js b/addons/mail/static/tests/tours/mail_full_composer_test_tour.js index 2f0c3621764..71a971d6e6b 100644 --- a/addons/mail/static/tests/tours/mail_full_composer_test_tour.js +++ b/addons/mail/static/tests/tours/mail_full_composer_test_tour.js @@ -54,9 +54,9 @@ tour.register( trigger: '[name="subject"] input', run() { const subjectValue = document.querySelector('[name="subject"] input').value; - if (subjectValue !== "Re: Test User") { + if (subjectValue !== "Test User") { console.error( - `Full composer should have "Re: Test User" in subject input (actual: ${subjectValue})` + `Full composer should have "Test User" in subject input (actual: ${subjectValue})` ); } }, diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index da77bf22510..f766f2019c6 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -191,21 +191,21 @@ class MailComposer(models.TransientModel): if values.get('parent_id'): parent = self.env['mail.message'].browse(values.get('parent_id')) result['record_name'] = parent.record_name - subject = tools.ustr(parent.subject or parent.record_name or '') 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'): - doc_name_get = self.env[values.get('model')].browse(values.get('res_id')).name_get() - result['record_name'] = doc_name_get and doc_name_get[0][1] or '' - subject = tools.ustr(result['record_name']) + record = self.env[values['model']].browse(values['res_id']) + doc_name = record.display_name + result['record_name'] = doc_name or '' + subject = record._message_compute_subject() - re_prefix = _('Re:') - if subject and not (subject.startswith('Re:') or subject.startswith(re_prefix)): - subject = "%s %s" % (re_prefix, subject) 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 284e86956f6..d27591afd6e 100644 --- a/addons/test_mail/models/test_mail_models.py +++ b/addons/test_mail/models/test_mail_models.py @@ -115,6 +115,10 @@ class MailTestTicket(models.Model): user_id = fields.Many2one('res.users', 'Responsible', tracking=1) container_id = fields.Many2one('mail.test.container', tracking=True) + def _message_compute_subject(self): + self.ensure_one() + return f"Ticket for {self.name} on {self.datetime.strftime('%m/%d/%Y, %H:%M:%S')}" + def _message_get_default_recipients(self): return dict( (record.id, { diff --git a/addons/test_mail/tests/test_mail_composer.py b/addons/test_mail/tests/test_mail_composer.py index b9b3072f106..c6d9f6bedf5 100644 --- a/addons/test_mail/tests/test_mail_composer.py +++ b/addons/test_mail/tests/test_mail_composer.py @@ -120,9 +120,7 @@ class TestComposerForm(TestMailComposer): self.assertFalse(composer_form.reply_to) self.assertFalse(composer_form.reply_to_force_new) self.assertEqual(composer_form.res_id, self.test_record.id) - self.assertEqual( - composer_form.subject, 'Re: %s' % self.test_record.name, - 'MailComposer: comment mode should have default subject Re: record_name') + self.assertEqual(composer_form.subject, self.test_record._message_compute_subject()) self.assertEqual(composer_form.subtype_id, self.env.ref('mail.mt_comment')) @users('employee') @@ -503,7 +501,7 @@ class TestComposerInternals(TestMailComposer): # (aka subject for comment mode) if composition_mode == 'comment': self.assertFalse(composer.body) - self.assertEqual(composer.subject, 'Re: %s' % self.test_record.name) + self.assertEqual(composer.subject, self.test_record._message_compute_subject()) # TDE FIXME: server id is kept, not sure why # self.assertFalse(composer.mail_server_id.id) self.assertEqual(composer.mail_server_id, self.template.mail_server_id) @@ -572,6 +570,25 @@ class TestComposerInternals(TestMailComposer): self.assertEqual(composer.mail_server_id.id, False) self.assertEqual(composer.record_name, 'CustomName') + @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, 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, 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): @@ -736,7 +753,10 @@ 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. """ - parent = self.test_record.message_post(body='Test', partner_ids=(self.partner_1 + self.partner_2).ids) + subject = "Parent Subject" + parent = self.test_record.message_post(subject=subject, + body='Test', + partner_ids=(self.partner_1 + self.partner_2).ids) composer = self.env['mail.compose.message'].with_context( self._get_web_context(self.test_record, add_web=False, default_parent_id=parent.id) @@ -749,7 +769,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, 'Re: %s' % self.test_record.name) + self.assertEqual(composer.subject, subject) @users('user_rendering_restricted') @mute_logger('odoo.tests', 'odoo.addons.base.models.ir_rule', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') @@ -970,7 +990,7 @@ class TestComposerResultsComment(TestMailComposer): message = self.test_record.message_ids[0] self.assertEqual(message.author_id, self.user_employee.partner_id) self.assertEqual(message.body, '

Test Body

') - self.assertEqual(message.subject, 'Re: %s' % self.test_record.name) + self.assertEqual(message.subject, self.test_record._message_compute_subject()) self.assertEqual(message.subtype_id, self.env.ref('mail.mt_comment')) self.assertEqual(message.partner_ids, self.partner_1 | self.partner_2) diff --git a/addons/test_mail/tests/test_message_post.py b/addons/test_mail/tests/test_message_post.py index 49472beec66..eaf18c15b14 100644 --- a/addons/test_mail/tests/test_message_post.py +++ b/addons/test_mail/tests/test_message_post.py @@ -325,6 +325,20 @@ 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): @@ -911,7 +925,7 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase): [self.partner_2], body_content='

Test Answer Bis

', reply_to=msg.reply_to, - subject='Re: %s' % self.test_record.name, + subject=self.test_record.name, references_content='openerp-%d-mail.test.simple' % self.test_record.id, references='%s %s' % (parent_msg.message_id, new_msg.message_id), )