From c1a6ef5d0d9d41d742c5de793a1fc9580dd35c1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 15 May 2023 10:26:43 +0000 Subject: [PATCH 1/7] [IMP] test_mail(_sms): update counters to latest runbot Update counters to match current runbot state. Several changes (ORM, mail code organization) lead to some counters being obsolete. Task-3322905 Part-of: odoo/odoo#121104 --- addons/test_mail/tests/test_performance.py | 52 +++++++++---------- .../tests/test_mail_performance.py | 2 +- .../tests/test_sms_performance.py | 6 +-- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 031775f6b29..f8dc9895ce7 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -348,7 +348,7 @@ class TestMailAPIPerformance(BaseMailPerformance): 'partner_ids': [(4, customer_id)], }) - with self.assertQueryCount(admin=36, employee=36): + with self.assertQueryCount(admin=35, employee=35): composer._action_send_mail() @users('admin', 'employee') @@ -369,7 +369,7 @@ class TestMailAPIPerformance(BaseMailPerformance): 'partner_ids': [(4, customer.id)], }) - with self.assertQueryCount(admin=37, employee=37): + with self.assertQueryCount(admin=35, employee=35): composer._action_send_mail() @users('admin', 'employee') @@ -379,7 +379,7 @@ class TestMailAPIPerformance(BaseMailPerformance): test_record, _test_template = self._create_test_records() customer = self.env['res.partner'].browse(self.customer.ids) attachments = self.env['ir.attachment'].with_user(self.env.user).create(self.test_attachments_vals) - with self.assertQueryCount(admin=10, employee=10): + with self.assertQueryCount(admin=9, employee=9): composer_form = Form( self.env['mail.compose.message'].with_context({ 'default_composition_mode': 'comment', @@ -393,7 +393,7 @@ class TestMailAPIPerformance(BaseMailPerformance): composer_form.attachment_ids.add(attachment) composer = composer_form.save() - with self.assertQueryCount(admin=49, employee=49): # tm+com 48/48 + with self.assertQueryCount(admin=47, employee=47): # tm+com 46/46 composer._action_send_mail() # notifications @@ -415,7 +415,7 @@ class TestMailAPIPerformance(BaseMailPerformance): 'default_template_id': test_template.id, }).create({}) - with self.assertQueryCount(admin=119, employee=122), self.mock_mail_gateway(): + with self.assertQueryCount(admin=108, employee=111), self.mock_mail_gateway(): composer._action_send_mail() self.assertEqual(len(self._new_mails), 10) @@ -437,7 +437,7 @@ class TestMailAPIPerformance(BaseMailPerformance): 'partner_ids': [(4, customer_id)], }) - with self.assertQueryCount(admin=36, employee=36): + with self.assertQueryCount(admin=35, employee=35): composer._action_send_mail() @users('admin', 'employee') @@ -447,7 +447,7 @@ class TestMailAPIPerformance(BaseMailPerformance): test_record, test_template = self._create_test_records() test_template.write({'attachment_ids': [(5, 0)]}) - with self.assertQueryCount(admin=25, employee=25): # tm 15/15 / com 23/23 + with self.assertQueryCount(admin=25, employee=25): # tm 15/15 / com 24/24 composer = self.env['mail.compose.message'].with_context({ 'default_composition_mode': 'comment', 'default_model': test_record._name, @@ -455,7 +455,7 @@ class TestMailAPIPerformance(BaseMailPerformance): 'default_template_id': test_template.id, }).create({}) - with self.assertQueryCount(admin=36, employee=36): + with self.assertQueryCount(admin=35, employee=35): composer._action_send_mail() # notifications @@ -471,7 +471,7 @@ class TestMailAPIPerformance(BaseMailPerformance): def test_mail_composer_w_template_attachments(self): test_record, test_template = self._create_test_records() - with self.assertQueryCount(admin=25, employee=25): # tm 15/15 / com 23/23 + with self.assertQueryCount(admin=25, employee=25): # tm 15/15 / com 24/24 composer = self.env['mail.compose.message'].with_context({ 'default_composition_mode': 'comment', 'default_model': test_record._name, @@ -479,7 +479,7 @@ class TestMailAPIPerformance(BaseMailPerformance): 'default_template_id': test_template.id, }).create({}) - with self.assertQueryCount(admin=44, employee=44): + with self.assertQueryCount(admin=42, employee=42): composer._action_send_mail() # notifications @@ -500,7 +500,7 @@ class TestMailAPIPerformance(BaseMailPerformance): test_template.write({'attachment_ids': [(5, 0)]}) customer = self.env['res.partner'].browse(self.customer.ids) - with self.assertQueryCount(admin=38, employee=38): # tm 28/28 / com 36/36 + with self.assertQueryCount(admin=38, employee=38): # tm 28/28 / com 37/37 composer_form = Form( self.env['mail.compose.message'].with_context({ 'default_composition_mode': 'comment', @@ -511,7 +511,7 @@ class TestMailAPIPerformance(BaseMailPerformance): ) composer = composer_form.save() - with self.assertQueryCount(admin=46, employee=46): + with self.assertQueryCount(admin=44, employee=44): composer._action_send_mail() # notifications @@ -530,7 +530,7 @@ class TestMailAPIPerformance(BaseMailPerformance): test_record, test_template = self._create_test_records() customer = self.env['res.partner'].browse(self.customer.ids) - with self.assertQueryCount(admin=37, employee=37): # tm 27/27 / com 35/35 + with self.assertQueryCount(admin=37, employee=37): # tm 27/27 / com 36/36 composer_form = Form( self.env['mail.compose.message'].with_context({ 'default_composition_mode': 'comment', @@ -541,7 +541,7 @@ class TestMailAPIPerformance(BaseMailPerformance): ) composer = composer_form.save() - with self.assertQueryCount(admin=64, employee=64): + with self.assertQueryCount(admin=61, employee=61): composer._action_send_mail() # notifications @@ -568,7 +568,7 @@ class TestMailAPIPerformance(BaseMailPerformance): # use another user already pre-defined with the email notification type, # so the ormcache is preserved. record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(admin=39, employee=39): + with self.assertQueryCount(admin=38, employee=38): record.write({ 'user_id': self.user_test_email.id, }) @@ -651,7 +651,7 @@ class TestMailAPIPerformance(BaseMailPerformance): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(admin=31, employee=31): + with self.assertQueryCount(admin=30, employee=30): record.message_post( body=Markup('

Test Post Performances with an email ping

'), partner_ids=self.customer.ids, @@ -793,7 +793,7 @@ class TestMailComplexPerformance(BaseMailPerformance): 'mail_message_id': message.id, 'recipient_ids': [(4, pid) for pid in self.partners.ids], }) - with self.assertQueryCount(admin=9, employee=9): + with self.assertQueryCount(admin=7, employee=7): self.env['mail.mail'].sudo().browse(mail.ids).send() @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') @@ -832,7 +832,7 @@ class TestMailComplexPerformance(BaseMailPerformance): unlinked_mails |= set(records.ids) unlinked_mails = set() - with self.assertQueryCount(admin=32, employee=32), \ + with self.assertQueryCount(admin=30, employee=30), \ patch.object(type(self.env['mail.mail']), 'unlink', _patched_unlink): self.env['mail.mail'].sudo().browse(mails.ids).send() @@ -852,7 +852,7 @@ class TestMailComplexPerformance(BaseMailPerformance): record = self.container.with_user(self.env.user) # about 20 (19?) queries per additional customer group - with self.assertQueryCount(admin=56, employee=55): + with self.assertQueryCount(admin=55, employee=54): record.message_post( body=Markup('

Test Post Performances

'), message_type='comment', @@ -870,7 +870,7 @@ class TestMailComplexPerformance(BaseMailPerformance): template = self.env.ref('test_mail.mail_test_container_tpl') # about 20 (19 ?) queries per additional customer group - with self.assertQueryCount(admin=63, employee=62): + with self.assertQueryCount(admin=62, employee=61): record.message_post_with_source( template, message_type='comment', @@ -960,7 +960,7 @@ class TestMailComplexPerformance(BaseMailPerformance): }) rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(admin=39, employee=39): + with self.assertQueryCount(admin=38, employee=38): rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec1.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) # write tracking message @@ -980,7 +980,7 @@ class TestMailComplexPerformance(BaseMailPerformance): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(admin=94, employee=94): + with self.assertQueryCount(admin=92, employee=92): rec = self.env['mail.test.ticket'].create({ 'name': 'Test', 'container_id': container_id, @@ -1009,7 +1009,7 @@ class TestMailComplexPerformance(BaseMailPerformance): rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) self.assertEqual(len(rec1.message_ids), 1) - with self.assertQueryCount(admin=59, employee=59): + with self.assertQueryCount(admin=58, employee=58): rec.write({ 'name': 'Test2', 'container_id': self.container.id, @@ -1046,7 +1046,7 @@ class TestMailComplexPerformance(BaseMailPerformance): rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(admin=65, employee=65): + with self.assertQueryCount(admin=64, employee=64): rec.write({ 'name': 'Test2', 'container_id': container_id, @@ -1079,7 +1079,7 @@ class TestMailComplexPerformance(BaseMailPerformance): rec1 = rec.with_context(active_test=False) # to see inactive records self.assertEqual(rec1.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) - with self.assertQueryCount(admin=33, employee=33): + with self.assertQueryCount(admin=32, employee=32): rec.write({ 'name': 'Test2', 'customer_id': customer_id, @@ -1309,7 +1309,7 @@ class TestMailHeavyPerformancePost(BaseMailPerformance): attachments = self.env['ir.attachment'].with_user(self.env.user).create(self.test_attachments_vals) # enable_logging = self.cr._enable_logging() if self.warm else nullcontext() # with self.assertQueryCount(employee=63), enable_logging: - with self.assertQueryCount(employee=62): + with self.assertQueryCount(employee=60): record_container.with_context({}).message_post( body=Markup('

Test body

'), subject='Test Subject', diff --git a/addons/test_mail_full/tests/test_mail_performance.py b/addons/test_mail_full/tests/test_mail_performance.py index f21866eefd7..eb36ab99bd5 100644 --- a/addons/test_mail_full/tests/test_mail_performance.py +++ b/addons/test_mail_full/tests/test_mail_performance.py @@ -82,7 +82,7 @@ class TestMailPerformance(BaseMailPerformance): record_ticket = self.env['mail.test.ticket.mc'].browse(self.record_ticket.ids) attachments = self.env['ir.attachment'].create(self.test_attachments_vals) - with self.assertQueryCount(employee=91): # tmf: 90 + with self.assertQueryCount(employee=89): # test_mail_full: 88 new_message = record_ticket.message_post( attachment_ids=attachments.ids, body=Markup('

Test Content

'), diff --git a/addons/test_mail_sms/tests/test_sms_performance.py b/addons/test_mail_sms/tests/test_sms_performance.py index f1e7f9e21a6..585b22ee616 100644 --- a/addons/test_mail_sms/tests/test_sms_performance.py +++ b/addons/test_mail_sms/tests/test_sms_performance.py @@ -35,7 +35,7 @@ class TestSMSPerformance(BaseMailPerformance, sms_common.SMSCase): def test_message_sms_record_1_partner(self): record = self.test_record.with_user(self.env.user) pids = self.customer.ids - with self.mockSMSGateway(sms_allow_unlink=True), self.assertQueryCount(employee=26): + with self.mockSMSGateway(sms_allow_unlink=True), self.assertQueryCount(employee=25): messages = record._message_sms( body='Performance Test', partner_ids=pids, @@ -50,7 +50,7 @@ class TestSMSPerformance(BaseMailPerformance, sms_common.SMSCase): def test_message_sms_record_10_partners(self): record = self.test_record.with_user(self.env.user) pids = self.partners.ids - with self.mockSMSGateway(sms_allow_unlink=True), self.assertQueryCount(employee=26): + with self.mockSMSGateway(sms_allow_unlink=True), self.assertQueryCount(employee=25): messages = record._message_sms( body='Performance Test', partner_ids=pids, @@ -64,7 +64,7 @@ class TestSMSPerformance(BaseMailPerformance, sms_common.SMSCase): @warmup def test_message_sms_record_default(self): record = self.test_record.with_user(self.env.user) - with self.mockSMSGateway(sms_allow_unlink=True), self.assertQueryCount(employee=28): + with self.mockSMSGateway(sms_allow_unlink=True), self.assertQueryCount(employee=27): messages = record._message_sms( body='Performance Test', ) From 2d19d07152167aa5416a51f23ede224aac471cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 31 Mar 2023 10:25:39 +0000 Subject: [PATCH 2/7] [IMP] test_mail: improve message_format performance test setup Purpose is to improve the depth of 'message_format' performance tests by being multirecords-enabled, in addition to being multimessages enabled. We also add more 2many fields in order to better see their effect (notably link previews and reactions, recently added). Task-3322905 Part-of: odoo/odoo#121104 --- addons/test_mail/tests/test_performance.py | 282 ++++++++++++--------- 1 file changed, 160 insertions(+), 122 deletions(-) diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index f8dc9895ce7..05ce38169e4 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -35,12 +35,20 @@ class BaseMailPerformance(MailCommon, TransactionCaseWithUserDemo): 'groups_id': [(6, 0, [cls.env.ref('base.group_user').id])], }) - cls.customer = cls.env['res.partner'].with_context(cls._test_context).create({ - 'country_id': cls.env.ref('base.be').id, - 'email': 'customer.test@example.com', - 'name': 'Test Customer', - 'mobile': '0456123456', - }) + cls.customers = cls.env['res.partner'].with_context(cls._test_context).create([ + { + 'country_id': cls.env.ref('base.be').id, + 'email': 'customer.test@example.com', + 'name': 'Test Customer', + 'mobile': '0456123456', + }, { + 'country_id': cls.env.ref('base.be').id, + 'email': 'customer.test.2@example.com', + 'name': 'Test Customer 2', + 'mobile': '0456654321', + } + ]) + cls.customer = cls.customers[0] cls.test_attachments_vals = cls._generate_attachments_data(3, 'mail.compose.message', 0) @@ -741,33 +749,136 @@ class TestMailAPIPerformance(BaseMailPerformance): @tagged('mail_performance', 'post_install', '-at_install') class TestMailComplexPerformance(BaseMailPerformance): - def setUp(self): - super(TestMailComplexPerformance, self).setUp() - self.user_portal = self.env['res.users'].with_context(self._test_context).create({ + @classmethod + def setUpClass(cls): + super().setUpClass() + + cls.user_portal = cls.env['res.users'].with_context(cls._test_context).create({ 'name': 'Olivia Portal', 'login': 'port', 'email': 'p.p@example.com', 'signature': '--\nOlivia', 'notification_type': 'email', - 'groups_id': [(6, 0, [self.env.ref('base.group_portal').id])], + 'groups_id': [(6, 0, [cls.env.ref('base.group_portal').id])], }) - self.container = self.env['mail.test.container'].with_context(mail_create_nosubscribe=True).create({ - 'name': 'Test Container', - 'customer_id': self.customer.id, - 'alias_name': 'test-alias', - }) - Partners = self.env['res.partner'].with_context(self._test_context) - self.partners = self.env['res.partner'] - for x in range(0, 10): - self.partners |= Partners.create({'name': 'Test %s' % x, 'email': 'test%s@example.com' % x}) - self.container.message_subscribe(self.partners.ids, subtype_ids=[ - self.env.ref('mail.mt_comment').id, - self.env.ref('test_mail.st_mail_test_container_child_full').id + cls.containers = cls.env['mail.test.container'].with_context(mail_create_nosubscribe=True).create([ + { + 'alias_name': 'test-alias', + 'customer_id': cls.customers[0].id, + 'name': 'Test Container', + }, { + 'alias_name': 'test-alias-2', + 'customer_id': cls.customers[1].id, + 'name': 'Test Container 2', + }, + ]) + Partners = cls.env['res.partner'].with_context(cls._test_context) + cls.partners = Partners.create([ + { + 'name': f'Test {idx}', + 'email': f'test{idx}@example.com', + } + for idx in range(10) + ]) + cls.containers.message_subscribe(cls.partners.ids, subtype_ids=[ + cls.env.ref('mail.mt_comment').id, + cls.env.ref('test_mail.st_mail_test_container_child_full').id + ]) + cls.container = cls.containers[0] + + name_field = cls.env['ir.model.fields']._get(cls.container._name, 'name') + customer_id_field = cls.env['ir.model.fields']._get(cls.container._name, 'customer_id') + comment_subtype_id = cls.env['ir.model.data']._xmlid_to_res_id('mail.mt_comment') + + cls.messages_all = cls.env['mail.message'].sudo().create([ + { + 'attachment_ids': [ + (0, 0, { + 'datas': 'data', + 'name': f'Test file {att_idx}', + 'res_id': record.id, + 'res_model': record._name, + }) + for att_idx in range(2) + ], + 'author_id': cls.partners[msg_idx].id, + 'body': f'

Test {msg_idx}

', + 'email_from': cls.partners[msg_idx].email_formatted, + 'link_preview_ids': [ + (0, 0, { + 'source_url': 'https://www.odoo.com', + }), (0, 0, { + 'source_url': 'https://www.example.com', + }), + ], + 'message_type': 'comment', + 'model': record._name, + 'notification_ids': [ + (0, 0, { + 'is_read': False, + 'notification_type': 'inbox', + 'res_partner_id': cls.partners[(record_idx * 5) + (msg_idx * 2)].id, + }), + (0, 0, { + 'is_read': True, + 'notification_type': 'email', + 'notification_status': 'sent', + 'res_partner_id': cls.partners[(record_idx * 5) + (msg_idx * 2) + 1].id, + }), + (0, 0, { + 'is_read': True, + 'notification_type': 'email', + 'notification_status': 'exception', + 'res_partner_id': cls.partners[(record_idx * 5) + (msg_idx * 2) + 2].id, + }), + ], + 'partner_ids': [ + (4, cls.partners[(record_idx * 5) + msg_idx].id), + (4, cls.partners[(record_idx * 5) + msg_idx + 1].id), + ], + 'reaction_ids': [ + (0, 0, { + 'content': '\U0001F4E7', + 'partner_id': cls.partners[(record_idx * 5)].id + }), (0, 0, { + 'content': '\U0001F4E8', + 'partner_id': cls.partners[(record_idx * 5) + 1].id + }), + ], + 'res_id': record.id, + 'starred_partner_ids': [ + (4, cls.partners[(record_idx * 5) + msg_idx].id), + (4, cls.partners[(record_idx * 5) + (msg_idx * 2) + 1].id), + ], + 'subject': f'Test Container {msg_idx}', + 'subtype_id': comment_subtype_id, + 'tracking_value_ids': [ + (0, 0, { + 'field': name_field.id, + 'field_desc': 'Name', + 'new_value_char': 'new 0', + 'old_value_char': 'old 0', + }), + (0, 0, { + 'field': customer_id_field.id, + 'field_desc': 'Customer', + 'new_value_char': 'new 1', + 'new_value_integer': cls.partners[(record_idx * 5)].id, + 'old_value_char': 'old 1', + 'old_value_integer': cls.partners[(record_idx * 5) + 1].id, + }), + ] + } + for msg_idx in range(2) + for record_idx, record in enumerate(cls.containers) ]) # `test_complex_mail_mail_send` - self.env.flush_all() + cls.env.flush_all() + + def test_assert_initial_values(self): + self.assertEqual(len(self.messages_all), 2*2) @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') @users('admin', 'employee') @@ -1100,115 +1211,42 @@ class TestMailComplexPerformance(BaseMailPerformance): @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') @users('employee') @warmup - def test_message_format(self): + def test_message_format_multi(self): """Test performance of `_message_format` and of `message_format` with multiple messages with multiple attachments, different authors, various notifications, and different tracking values. + Those messages might not make sense functionally but they are crafted to cover as much of the code as possible in regard to number of queries. + + Setup : + * 2 records (self.containers -> 2 mail.test.container record, with + a different customer_id each) + * 2 messages / record + * 2 attachments / message + * 3 notifications / message + * 2 tracking values / message """ - name_field = self.env['ir.model.fields']._get(self.container._name, 'name') - customer_id_field = self.env['ir.model.fields']._get(self.container._name, 'customer_id') + messages_all = self.messages_all.with_env(self.env) - messages = self.env['mail.message'].sudo().create([{ - 'subject': 'Test 0', - 'body': '

Test 0

', - 'author_id': self.partners[0].id, - 'email_from': self.partners[0].email, - 'model': 'mail.test.container', - 'res_id': self.container.id, - 'subtype_id': self.env['ir.model.data']._xmlid_to_res_id('mail.mt_comment'), - 'attachment_ids': [ - (0, 0, { - 'name': 'test file 0 - %d' % j, - 'datas': 'data', - }) for j in range(2) - ], - 'notification_ids': [ - (0, 0, { - 'res_partner_id': self.partners[3].id, - 'notification_type': 'inbox', - }), - (0, 0, { - 'res_partner_id': self.partners[4].id, - 'notification_type': 'email', - 'notification_status': 'exception', - }), - (0, 0, { - 'res_partner_id': self.partners[6].id, - 'notification_type': 'email', - 'notification_status': 'exception', - }), - ], - 'tracking_value_ids': [ - (0, 0, { - 'field': name_field.id, - 'field_desc': 'Name', - 'old_value_char': 'old 0', - 'new_value_char': 'new 0', - }), - (0, 0, { - 'field': customer_id_field.id, - 'field_desc': 'Customer', - 'old_value_integer': self.partners[7].id, - 'new_value_integer': self.partners[8].id, - }), - ] - }, { - 'subject': 'Test 1', - 'body': '

Test 1

', - 'author_id': self.partners[1].id, - 'email_from': self.partners[1].email, - 'model': 'mail.test.container', - 'res_id': self.container.id, - 'subtype_id': self.env['ir.model.data']._xmlid_to_res_id('mail.mt_note'), - 'attachment_ids': [ - (0, 0, { - 'name': 'test file 1 - %d' % j, - 'datas': 'data', - }) for j in range(2) - ], - 'notification_ids': [ - (0, 0, { - 'res_partner_id': self.partners[5].id, - 'notification_type': 'inbox', - }), - (0, 0, { - 'res_partner_id': self.partners[6].id, - 'notification_type': 'email', - 'notification_status': 'exception', - }), - ], - 'tracking_value_ids': [ - (0, 0, { - 'field': name_field.id, - 'field_desc': 'Name', - 'old_value_char': 'old 1', - 'new_value_char': 'new 1', - }), - (0, 0, { - 'field': customer_id_field.id, - 'field_desc': 'Customer', - 'old_value_integer': self.partners[7].id, - 'new_value_integer': self.partners[8].id, - }), - ] - }]) + with self.assertQueryCount(employee=24): # test_mail: 24 + res = messages_all.message_format() - with self.assertQueryCount(employee=7): - res = messages.message_format() - self.assertEqual(len(res), 2) - for message in res: - self.assertEqual(len(message['attachment_ids']), 2) + self.assertEqual(len(res), 2*2) + for message in res: + self.assertEqual(len(message['attachment_ids']), 2) - self.env.flush_all() - self.env.invalidate_all() + @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') + @users('employee') + @warmup + def test_message_format_single(self): + message = self.messages_all[0].with_env(self.env) - with self.assertQueryCount(employee=18): - res = messages.message_format() - self.assertEqual(len(res), 2) - for message in res: - self.assertEqual(len(message['attachment_ids']), 2) + with self.assertQueryCount(employee=21): # test_mail: 21 + res = message.message_format() + + self.assertEqual(len(res), 1) + self.assertEqual(len(res[0]['attachment_ids']), 2) @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') @users('employee') From 23bbd8860bd12c9e62942415a8e3cff084380d73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 3 Apr 2023 11:45:59 +0000 Subject: [PATCH 3/7] [REF] portal(_rating): make portal message_format standalone RATIONALE Purpose of this change to rewrite the formatting done on messages displayed on simple frontend i.e. portal chatter widget and project chatter. We stop calling 'message_format' which computes a lot of unnecessary data and sends too much information to frontend. We choose to instead manually handcraft the returned data, already tailored for frontend widget. SPECIFICATIONS Remove call to '_message_format' in 'portal_message_format'. Instead have a list of properties (fields or computation based on fields e.g. rating publisher information) that can be overridden in sub-addons. Use those to generate the data used by frontend chatter widget. Remove extra formatting or data computation done in JS files. Do it directly in 'portal_message_format' in order to have clean information sent. This change targets mainly portal and portal_rating. Making those modules Independent from backend message formatting allows to save queries and also to avoid sending useless information to the frontend. Task-3322905 Part-of: odoo/odoo#121104 --- addons/portal/controllers/mail.py | 2 +- addons/portal/models/mail_message.py | 123 +++++++++++++++--- addons/portal/static/src/js/portal_chatter.js | 5 - .../portal/static/src/xml/portal_chatter.xml | 2 +- .../controllers/portal_chatter.py | 23 ---- .../controllers/portal_rating.py | 4 +- addons/portal_rating/models/mail_message.py | 69 +++++++--- .../static/src/js/portal_chatter.js | 17 +-- .../components/chatter/chatter_container.js | 12 -- .../components/chatter/chatter_messages.xml | 2 +- 10 files changed, 173 insertions(+), 86 deletions(-) diff --git a/addons/portal/controllers/mail.py b/addons/portal/controllers/mail.py index 20467ca104d..f99c8fc2bac 100644 --- a/addons/portal/controllers/mail.py +++ b/addons/portal/controllers/mail.py @@ -213,7 +213,7 @@ class PortalChatter(http.Controller): domain = expression.AND([Message._get_search_domain_share(), domain]) Message = request.env['mail.message'].sudo() return { - 'messages': Message.search(domain, limit=limit, offset=offset).portal_message_format(), + 'messages': Message.search(domain, limit=limit, offset=offset).portal_message_format(options=kw), 'message_count': Message.search_count(domain) } diff --git a/addons/portal/models/mail_message.py b/addons/portal/models/mail_message.py index 5e94f4ea70f..173057580b7 100644 --- a/addons/portal/models/mail_message.py +++ b/addons/portal/models/mail_message.py @@ -2,26 +2,117 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from odoo import models +from odoo.http import request +from odoo.tools import format_datetime class MailMessage(models.Model): _inherit = 'mail.message' - def portal_message_format(self): - self.check_access_rule('read') - return self._portal_message_format([ - 'id', 'body', 'date', 'author_id', 'email_from', # base message fields - 'message_type', 'subtype_id', 'is_internal', 'subject', # message specific - 'model', 'res_id', 'record_name', # document related - ]) + def portal_message_format(self, options=None): + """ Simpler and portal-oriented version of 'message_format'. Purpose + is to prepare, organize and format values required by frontend widget + (frontend Chatter). - def _portal_message_format(self, fields_list): - vals_list = self._message_format(fields_list, legacy=True) - message_subtype_note_id = self.env['ir.model.data']._xmlid_to_res_id('mail.mt_note') - IrAttachmentSudo = self.env['ir.attachment'].sudo() - for vals in vals_list: - vals['is_message_subtype_note'] = message_subtype_note_id and (vals.get('subtype_id') or [False])[0] == message_subtype_note_id - for attachment in vals.get('attachment_ids', []): - if not attachment.get('access_token'): - attachment['access_token'] = IrAttachmentSudo.browse(attachment['id']).generate_access_token()[0] + This public API asks for read access on messages before doing the + actual computation in the private implementation. + + :param dict options: options, used notably for inheritance and adding + specific fields or properties to compute; + + :return list: list of dict, one per message in self. Each dict contains + values for either fields, either properties derived from fields. + """ + self.check_access_rule('read') + return self._portal_message_format( + self._portal_get_default_format_properties_names(options=options) + ) + + def _portal_get_default_format_properties_names(self, options=None): + """ Fields and values to compute for portal format. + + :param dict options: options, used notably for inheritance and adding + specific fields or properties to compute; + + :return set: fields or properties derived from fields + """ + return { + 'attachment_ids', + 'author_avatar_url', + 'author_id', + 'body', + 'date', + 'id', + 'is_internal', + 'is_message_subtype_note', + 'published_date_str', + 'subtype_id', + } + + def _portal_message_format(self, properties_names): + """ Format messages for portal frontend. This private implementation + does not check for access that should be checked beforehand. + + Notes: + * when asking for attachments: ensure an access token is present then + access them (using sudo); + + :param set properties_names: fields or properties derived from fields + for which we are going to compute values; + + :return list: list of dict, one per message in self. Each dict contains + values for either fields, either properties derived from fields. + """ + message_to_attachments = {} + if 'attachment_ids' in properties_names: + properties_names.remove('attachment_ids') + attachments_sudo = self.sudo().attachment_ids + attachments_sudo.generate_access_token() + related_attachments = { + att_read_values['id']: att_read_values + for att_read_values in attachments_sudo.read( + ["access_token", "checksum", "id", "mimetype", "name", "res_id", "res_model"] + ) + } + message_to_attachments = { + message.id: [ + self._portal_message_format_attachments(related_attachments[att_id]) + for att_id in message.attachment_ids.ids + ] + for message in self.sudo() + } + + fnames = { + property_name for property_name in properties_names + if property_name in self._fields + } + vals_list = self._read_format(fnames) + + note_id = self.env['ir.model.data']._xmlid_to_res_id('mail.mt_note') + for message, values in zip(self, vals_list): + if message_to_attachments: + values['attachment_ids'] = message_to_attachments.get(message.id, {}) + if 'author_avatar_url' in properties_names: + values['author_avatar_url'] = f'/web/image/mail.message/{message.id}/author_avatar/50x50' + if 'is_message_subtype_note' in properties_names: + values['is_message_subtype_note'] = (values.get('subtype_id') or [False, ''])[0] == note_id + if 'published_date_str' in properties_names: + values['published_date_str'] = format_datetime(self.env, values['date']) if values.get('date') else '' return vals_list + + def _portal_message_format_attachments(self, attachment_values): + """ From 'attachment_values' get an updated version formatted for + frontend display. + + :param dict attachment_values: values coming from reading attachments + in database; + + :return dict: updated attachment_values + """ + safari = request and request.httprequest.user_agent and request.httprequest.user_agent.browser == 'safari' + attachment_values['filename'] = attachment_values['name'] + attachment_values['mimetype'] = ( + 'application/octet-stream' if safari and + 'video' in (attachment_values["mimetype"] or "") + else attachment_values["mimetype"]) + return attachment_values diff --git a/addons/portal/static/src/js/portal_chatter.js b/addons/portal/static/src/js/portal_chatter.js index 004cc2af4cd..169f151ba99 100644 --- a/addons/portal/static/src/js/portal_chatter.js +++ b/addons/portal/static/src/js/portal_chatter.js @@ -3,14 +3,11 @@ import core from "web.core"; import dom from "web.dom"; import publicWidget from "web.public.widget"; -import time from "web.time"; import portalComposer from "portal.composer"; import {Markup} from "web.utils"; import { range } from "@web/core/utils/numbers"; -import { sprintf } from "@web/core/utils/strings"; var qweb = core.qweb; -var _t = core._t; /** * Widget PortalChatter @@ -103,8 +100,6 @@ var PortalChatter = publicWidget.Widget.extend({ */ preprocessMessages(messages) { messages.forEach((m) => { - m['author_avatar_url'] = sprintf('/web/image/%s/%s/author_avatar/50x50', 'mail.message', m.id); - m['published_date_str'] = sprintf(_t('Published on %s'), moment(time.str_to_datetime(m.date)).format('MMMM Do YYYY, h:mm:ss a')); m['body'] = Markup(m.body); }); return messages; diff --git a/addons/portal/static/src/xml/portal_chatter.xml b/addons/portal/static/src/xml/portal_chatter.xml index a100553e704..03473de8b0d 100644 --- a/addons/portal/static/src/xml/portal_chatter.xml +++ b/addons/portal/static/src/xml/portal_chatter.xml @@ -86,7 +86,7 @@
-

+

Published on

diff --git a/addons/portal_rating/controllers/portal_chatter.py b/addons/portal_rating/controllers/portal_chatter.py index 55ab7c716b8..2ac7bfd52cd 100644 --- a/addons/portal_rating/controllers/portal_chatter.py +++ b/addons/portal_rating/controllers/portal_chatter.py @@ -14,31 +14,8 @@ class PortalChatter(mail.PortalChatter): fields += ['rating_value', 'rating_feedback'] return fields - def _portal_rating_stats(self, res_model, res_id, **kwargs): - # get the rating statistics for the record - if kwargs.get('rating_include'): - record = request.env[res_model].browse(res_id) - if hasattr(record, 'rating_get_stats'): - return {'rating_stats': record.sudo().rating_get_stats()} - return {} - @http.route() def portal_chatter_post(self, res_model, res_id, message, attachment_ids='', attachment_tokens='', **kwargs): if kwargs.get('rating_value'): kwargs['rating_feedback'] = kwargs.pop('rating_feedback', message) return super(PortalChatter, self).portal_chatter_post(res_model, res_id, message, attachment_ids=attachment_ids, attachment_tokens=attachment_tokens, **kwargs) - - @http.route() - def portal_chatter_init(self, res_model, res_id, domain=False, limit=False, **kwargs): - result = super(PortalChatter, self).portal_chatter_init(res_model, res_id, domain=domain, limit=limit, **kwargs) - result.update(self._portal_rating_stats(res_model, res_id, **kwargs)) - return result - - @http.route() - def portal_message_fetch(self, res_model, res_id, domain=False, limit=False, offset=False, **kw): - # add 'rating_include' in context, to fetch them in portal_message_format - if kw.get('rating_include'): - request.update_context(rating_include=True) - result = super(PortalChatter, self).portal_message_fetch(res_model, res_id, domain=domain, limit=limit, offset=offset, **kw) - result.update(self._portal_rating_stats(res_model, res_id, **kw)) - return result diff --git a/addons/portal_rating/controllers/portal_rating.py b/addons/portal_rating/controllers/portal_rating.py index 6225f9c0243..a95c996d6c2 100644 --- a/addons/portal_rating/controllers/portal_rating.py +++ b/addons/portal_rating/controllers/portal_rating.py @@ -17,4 +17,6 @@ class PortalRating(http.Controller): return {'error': _('Invalid rating')} rating.write({'publisher_comment': publisher_comment}) # return to the front-end the created/updated publisher comment - return rating.read(['publisher_comment', 'publisher_id', 'publisher_datetime'])[0] + return request.env['mail.message']._portal_message_format_rating( + rating.read(['publisher_comment', 'publisher_id', 'publisher_datetime'])[0] + ) diff --git a/addons/portal_rating/models/mail_message.py b/addons/portal_rating/models/mail_message.py index f400334b8d4..707a5d35d6c 100644 --- a/addons/portal_rating/models/mail_message.py +++ b/addons/portal_rating/models/mail_message.py @@ -2,27 +2,66 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from odoo import models +from odoo.tools import format_datetime class MailMessage(models.Model): _inherit = 'mail.message' - def _portal_message_format(self, field_list): - # inlude rating value in data if requested - if self._context.get('rating_include'): - field_list += ['rating_value'] - return super(MailMessage, self)._portal_message_format(field_list) + def _portal_get_default_format_properties_names(self, options=None): + """ Add request for rating information - def _message_format(self, fnames, format_reply=True, legacy=False): - """ Override the method to add information about a publisher comment - on each rating messages if requested, and compute a plaintext value of it. + :param dict options: supports 'rating_include' option allowing to + conditionally include rating information; """ - vals_list = super(MailMessage, self)._message_format(fnames, format_reply=format_reply, legacy=legacy) + properties_names = super()._portal_get_default_format_properties_names() + if options and options.get('rating_include'): + properties_names |= { + 'rating', + 'rating_value', + } + return properties_names + + def _portal_message_format(self, properties_names): + """ If requested, add rating information to returned formatted values. + + Note: rating information combine both statistics (see 'rating_get_stats' + if available on model) and rating / publication information. """ + vals_list = super()._portal_message_format(properties_names) + if not 'rating' in properties_names: + return vals_list + + related_rating = self.env['rating.rating'].sudo().search_read( + [('message_id', 'in', self.ids)], + ["id", "publisher_comment", "publisher_id", "publisher_datetime", "message_id"] + ) + message_to_rating = { + rating['message_id'][0]: self._portal_message_format_rating(rating) + for rating in related_rating + } + + for message, values in zip(self, vals_list): + values["rating"] = message_to_rating.get(message.id, {}) + + record = self.env[message.model].browse(message.res_id) + if hasattr(record, 'rating_get_stats'): + values['rating_stats'] = record.sudo().rating_get_stats() - if self._context.get('rating_include'): - infos = ["id", "publisher_comment", "publisher_id", "publisher_datetime", "message_id"] - related_rating = self.env['rating.rating'].sudo().search([('message_id', 'in', self.ids)]).read(infos) - mid_rating_tree = dict((rating['message_id'][0], rating) for rating in related_rating) - for vals in vals_list: - vals["rating"] = mid_rating_tree.get(vals['id'], {}) return vals_list + + def _portal_message_format_rating(self, rating_values): + """ From 'rating_values' get an updated version formatted for frontend + display. + + :param dict rating_values: values coming from reading ratings + in database; + + :return dict: updated rating_values + """ + publisher_id, publisher_name = rating_values['publisher_id'] or [False, ''] + rating_values['publisher_avatar'] = f'/web/image/res.partner/{publisher_id}/avatar_128/50x50' if publisher_id else '' + rating_values['publisher_comment'] = rating_values['publisher_comment'] or '' + rating_values['publisher_datetime'] = format_datetime(self.env, rating_values['publisher_datetime']) + rating_values['publisher_id'] = publisher_id + rating_values['publisher_name'] = publisher_name + return rating_values diff --git a/addons/portal_rating/static/src/js/portal_chatter.js b/addons/portal_rating/static/src/js/portal_chatter.js index 8f6ff066933..15c3037a6b4 100644 --- a/addons/portal_rating/static/src/js/portal_chatter.js +++ b/addons/portal_rating/static/src/js/portal_chatter.js @@ -3,7 +3,6 @@ import core from "web.core"; import portalChatter from "portal.chatter"; import { roundPrecision } from "@web/core/utils/numbers"; -import time from "web.time"; var _t = core._t; var PortalChatter = portalChatter.PortalChatter; @@ -192,17 +191,13 @@ PortalChatter.include({ var ratingData = { id: rawRating.id, mes_index: messageIndex, - publisher_datetime: rawRating.publisher_datetime ? moment(time.str_to_datetime(rawRating.publisher_datetime)).format('MMMM Do YYYY, h:mm:ss a') : "", - publisher_comment: rawRating.publisher_comment ? rawRating.publisher_comment : '', + publisher_avatar: rawRating.publisher_avatar, + publisher_comment: rawRating.publisher_comment, + publisher_datetime: rawRating.publisher_datetime, + publisher_id: rawRating.publisher_id, + publisher_name: rawRating.publisher_name, }; - - // split array (id, display_name) of publisher_id into publisher_id and publisher_name - if (rawRating.publisher_id && rawRating.publisher_id.length >= 2) { - ratingData.publisher_id = rawRating.publisher_id[0]; - ratingData.publisher_name = rawRating.publisher_id[1]; - ratingData.publisher_avatar = `/web/image/res.partner/${ratingData.publisher_id}/avatar_128/50x50`; - } - var commentData = Object.assign(this._newPublisherCommentData(messageIndex), ratingData); + var commentData = {...this._newPublisherCommentData(messageIndex), ...ratingData}; return commentData; }, diff --git a/addons/project/static/src/project_sharing/components/chatter/chatter_container.js b/addons/project/static/src/project_sharing/components/chatter/chatter_container.js index ae82fcc533a..fb942183581 100644 --- a/addons/project/static/src/project_sharing/components/chatter/chatter_container.js +++ b/addons/project/static/src/project_sharing/components/chatter/chatter_container.js @@ -1,8 +1,6 @@ /** @odoo-module */ -import { formatDateTime, parseDateTime } from "@web/core/l10n/dates"; import { useService } from "@web/core/utils/hooks"; -import { sprintf } from '@web/core/utils/strings'; import { ChatterComposer } from "./chatter_composer"; import { ChatterMessageCounter } from "./chatter_message_counter"; import { ChatterMessages } from "./chatter_messages"; @@ -124,16 +122,6 @@ export class ChatterContainer extends Component { preprocessMessages(messages) { return messages.map(m => ({ ...m, - author_avatar_url: sprintf('/web/image/mail.message/%s/author_avatar/50x50', m.id), - published_date_str: sprintf( - this.env._t('Published on %s'), - formatDateTime( - parseDateTime( - m.date, - { format: 'MM-dd-yyy HH:mm:ss' }, - ), - ) - ), body: markup(m.body), })); } diff --git a/addons/project/static/src/project_sharing/components/chatter/chatter_messages.xml b/addons/project/static/src/project_sharing/components/chatter/chatter_messages.xml index 28ae6988e1d..e9aa6f9fd2f 100644 --- a/addons/project/static/src/project_sharing/components/chatter/chatter_messages.xml +++ b/addons/project/static/src/project_sharing/components/chatter/chatter_messages.xml @@ -28,7 +28,7 @@
-

+

Published on

From e600d78f87c3a9c5bd94bb42ee8f7f8ce5bfa81b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 3 Apr 2023 13:56:22 +0000 Subject: [PATCH 4/7] [REM] mail: remove 'legacy' support in message_format 'legacy' parameter was present only for a specific part of attachments formatting. It was used only for portal, as the widget used in frontend was not using the 'discuss' orm-like convention. As portal formatting is now independent from mail/discuss formatting this can be safely removed, simplifying the API. Task-3322905 Part-of: odoo/odoo#121104 --- addons/im_livechat/models/mail_message.py | 4 ++-- addons/mail/models/ir_attachment.py | 23 ++++++++--------------- addons/mail/models/mail_message.py | 4 ++-- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/addons/im_livechat/models/mail_message.py b/addons/im_livechat/models/mail_message.py index d6e08bc511e..998289ef39b 100644 --- a/addons/im_livechat/models/mail_message.py +++ b/addons/im_livechat/models/mail_message.py @@ -7,7 +7,7 @@ from odoo import models class MailMessage(models.Model): _inherit = 'mail.message' - def _message_format(self, fnames, format_reply=True, legacy=False): + def _message_format(self, fnames, format_reply=True): """Override to remove email_from and to return the livechat username if applicable. A third param is added to the author_id tuple in this case to be able to differentiate it from the normal name in client code. @@ -17,7 +17,7 @@ class MailMessage(models.Model): This allows the frontend display to include the additional features (e.g: Show additional buttons with the available answers for this step). """ - vals_list = super()._message_format(fnames=fnames, format_reply=format_reply, legacy=legacy) + vals_list = super()._message_format(fnames=fnames, format_reply=format_reply) for vals in vals_list: message_sudo = self.browse(vals['id']).sudo().with_prefetch(self.ids) discuss_channel = self.env['discuss.channel'].browse(message_sudo.res_id) if message_sudo.model == 'discuss.channel' else self.env['discuss.channel'] diff --git a/addons/mail/models/ir_attachment.py b/addons/mail/models/ir_attachment.py index 8d9b2855d97..707b7383a0d 100644 --- a/addons/mail/models/ir_attachment.py +++ b/addons/mail/models/ir_attachment.py @@ -50,29 +50,22 @@ class IrAttachment(models.Model): self.ensure_one() return self.env.user.partner_id - def _attachment_format(self, legacy=False): + def _attachment_format(self): safari = request and request.httprequest.user_agent and request.httprequest.user_agent.browser == 'safari' - res_list = [] - for attachment in self: - res = { + return [ + { 'checksum': attachment.checksum, 'id': attachment.id, 'filename': attachment.name, 'name': attachment.name, 'mimetype': 'application/octet-stream' if safari and attachment.mimetype and 'video' in attachment.mimetype else attachment.mimetype, - } - if not legacy: - res['originThread'] = [('insert', { + 'originThread': [('insert', { 'id': attachment.res_id, 'model': attachment.res_model, - })] - else: - res.update({ - 'res_id': attachment.res_id, - 'res_model': attachment.res_model, - }) - res_list.append(res) - return res_list + })], + } + for attachment in self + ] @api.model def _get_upload_env(self, request, thread_model, thread_id): diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index 156b7bf35e0..4ab8d098b61 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -866,7 +866,7 @@ class Message(models.Model): # MESSAGE READ / FETCH / FAILURE API # ------------------------------------------------------ - def _message_format(self, fnames, format_reply=True, legacy=False): + def _message_format(self, fnames, format_reply=True): """Reads values from messages and formats them for the web client.""" vals_list = self._read_format(fnames) thread_ids_by_model_name = defaultdict(set) @@ -908,7 +908,7 @@ class Message(models.Model): '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), + 'attachment_ids': message_sudo.attachment_ids._attachment_format(), 'trackingValues': allowed_tracking_ids._tracking_value_format(), 'linkPreviews': message_sudo.link_preview_ids._link_preview_format(), 'messageReactionGroups': reaction_groups, From c64ac528542db789f061e20c5c87b367a3aa8e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 12 May 2023 11:34:43 +0000 Subject: [PATCH 5/7] [FIX] portal_rating: synchronize rating publisher fields at create Currently when modifying publisher comment, publisher partner id and date are synchronized if not given in order to always have coherent values. In this commit we do the same at create. Even if won't happen frequently better ensure values coherency. Moreover it helps writing tests. Task-3322905 Part-of: odoo/odoo#121104 --- addons/portal_rating/models/rating_rating.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/addons/portal_rating/models/rating_rating.py b/addons/portal_rating/models/rating_rating.py index a2b21cf47f2..b66eb2e9524 100644 --- a/addons/portal_rating/models/rating_rating.py +++ b/addons/portal_rating/models/rating_rating.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- -from odoo import fields, models, exceptions, _ +from odoo import api, fields, models, exceptions, _ class Rating(models.Model): @@ -12,7 +12,20 @@ class Rating(models.Model): ondelete='set null', readonly=True) publisher_datetime = fields.Datetime("Commented on", readonly=True) + @api.model_create_multi + def create(self, values_list): + for values in values_list: + self._synchronize_publisher_values(values) + return super().create(values_list) + def write(self, values): + self._synchronize_publisher_values(values) + return super().write(values) + + def _synchronize_publisher_values(self, values): + """ Force publisher partner and date if not given in order to have + coherent values. Those fields are readonly as they are not meant + to be modified manually, behaving like a tracking. """ if values.get('publisher_comment'): if not self.env.user.has_group("website.group_website_restricted_editor"): raise exceptions.AccessError(_("Only the publisher of the website can change the rating comment")) @@ -20,4 +33,4 @@ class Rating(models.Model): values['publisher_datetime'] = fields.Datetime.now() if not values.get('publisher_id'): values['publisher_id'] = self.env.user.partner_id.id - return super(Rating, self).write(values) + return values From 2f196bf7c41e8f08e8b04225c64720dc6faf1eb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 12 May 2023 14:48:49 +0000 Subject: [PATCH 6/7] [FIX] (portal_)rating: do not crash if website is not installed Current rule to edit publisher fields (comment, ...) is to have the editor group. However when having only portal, this group does not exist. The feature should work as a standalone feature without depending on website. The rule is then updated as follow: either the current user belongs to the editor group (if it exists), either it can write on the related record (given res_model / res_id). Currently no crash in standard addons occur because its main usage is in eLearning application, where editor can answer and update publisher comment from the frontend (course review). Task-3322905 Part-of: odoo/odoo#121104 --- addons/portal_rating/models/rating_rating.py | 26 +++++++++++++-- addons/rating/models/rating.py | 34 ++++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/addons/portal_rating/models/rating_rating.py b/addons/portal_rating/models/rating_rating.py index b66eb2e9524..4a9ed3b094b 100644 --- a/addons/portal_rating/models/rating_rating.py +++ b/addons/portal_rating/models/rating_rating.py @@ -16,19 +16,39 @@ class Rating(models.Model): def create(self, values_list): for values in values_list: self._synchronize_publisher_values(values) - return super().create(values_list) + ratings = super().create(values_list) + if any(rating.publisher_comment for rating in ratings): + ratings._check_synchronize_publisher_values() + return ratings def write(self, values): self._synchronize_publisher_values(values) return super().write(values) + def _check_synchronize_publisher_values(self): + """ Either current user is a member of website restricted editor group + (done here by fetching the group record then using has_group, as it may + not be defined and we do not want to make a complete bridge module just + for that). Either write access on document is granted. """ + editor_group = self.env['ir.model.data']._xmlid_to_res_id('website.group_website_restricted_editor') + if editor_group and self.env.user.has_group('website.group_website_restricted_editor'): + return + for model, model_data in self._classify_by_model().items(): + records = self.env[model].browse(model_data['record_ids']) + try: + records.check_access_rights('write') + records.check_access_rule('write') + except exceptions.AccessError as e: + raise exceptions.AccessError( + _("Updating rating comment require write access on related record") + ) from e + def _synchronize_publisher_values(self, values): """ Force publisher partner and date if not given in order to have coherent values. Those fields are readonly as they are not meant to be modified manually, behaving like a tracking. """ if values.get('publisher_comment'): - if not self.env.user.has_group("website.group_website_restricted_editor"): - raise exceptions.AccessError(_("Only the publisher of the website can change the rating comment")) + self._check_synchronize_publisher_values() if not values.get('publisher_datetime'): values['publisher_datetime'] = fields.Datetime.now() if not values.get('publisher_id'): diff --git a/addons/rating/models/rating.py b/addons/rating/models/rating.py index 80948a0bfd4..b47e04d751d 100644 --- a/addons/rating/models/rating.py +++ b/addons/rating/models/rating.py @@ -108,6 +108,10 @@ class Rating(models.Model): for rating in self: rating.rating_text = rating_data._rating_to_text(rating.rating) + # ------------------------------------------------------------ + # CRUD + # ------------------------------------------------------------ + @api.model_create_multi def create(self, vals_list): for values in vals_list: @@ -141,6 +145,10 @@ class Rating(models.Model): data['parent_res_id'] = parent_res_model.id return data + # ------------------------------------------------------------ + # ACTIONS + # ------------------------------------------------------------ + def reset(self): for record in self: record.write({ @@ -158,3 +166,29 @@ class Rating(models.Model): 'res_id': self.res_id, 'views': [[False, 'form']] } + + # ------------------------------------------------------------ + # TOOLS + # ------------------------------------------------------------ + + def _classify_by_model(self): + """ To ease batch computation of various ratings related methods they + are classified by model. Ratings not linked to a valid record through + res_model / res_id are ignored. + + :return dict: for each model having at least one rating in self, have + a sub-dict containing + * ratings: ratings related to that model; + * record IDs: records linked to the ratings of that model, in same + order; + """ + data_by_model = {} + for rating in self.filtered(lambda act: act.res_model and act.res_id): + if rating.res_model not in data_by_model: + data_by_model[rating.res_model] = { + 'ratings': self.env['rating.rating'], + 'record_ids': [], + } + data_by_model[rating.res_model]['ratings'] += rating + data_by_model[rating.res_model]['record_ids'].append(rating.res_id) + return data_by_model From f11f9f221ee470704cdd1f30bf0bc5e6ad605cd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 11 May 2023 10:39:51 +0000 Subject: [PATCH 7/7] [IMP] test_mail_full: add performance tests for portal_message_format Purpose of this commit is to add performance tests for 'portal_message_format' that is used when displaying chatter in frontend (e.g. customer portal). It allows to keep an eye and to optimize part of that code allowing cross-apps optimization. Task-3322905 Part-of: odoo/odoo#121104 --- .../models/test_mail_models_mail.py | 1 - .../tests/test_mail_performance.py | 264 ++++++++++++++++-- 2 files changed, 243 insertions(+), 22 deletions(-) diff --git a/addons/test_mail_full/models/test_mail_models_mail.py b/addons/test_mail_full/models/test_mail_models_mail.py index 12db1ac29c8..7deea6c1d87 100644 --- a/addons/test_mail_full/models/test_mail_models_mail.py +++ b/addons/test_mail_full/models/test_mail_models_mail.py @@ -104,7 +104,6 @@ class MailTestRatingThread(models.Model): - partner_id: value returned by the base _rating_get_partner method - user_id: value returned by the base _rating_get_operator method """ - _description = 'Model for testing rating without the rating mixin' _name = 'mail.test.rating.thread' _inherit = 'mail.thread' diff --git a/addons/test_mail_full/tests/test_mail_performance.py b/addons/test_mail_full/tests/test_mail_performance.py index eb36ab99bd5..c9b44bec40d 100644 --- a/addons/test_mail_full/tests/test_mail_performance.py +++ b/addons/test_mail_full/tests/test_mail_performance.py @@ -1,7 +1,9 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +from datetime import datetime, timedelta from markupsafe import Markup + from odoo.addons.mail.tests.common import mail_new_test_user from odoo.addons.test_mail.tests.test_performance import BaseMailPerformance from odoo.tests.common import users, warmup @@ -10,11 +12,11 @@ from odoo.tools import mute_logger @tagged('mail_performance', 'post_install', '-at_install') -class TestMailPerformance(BaseMailPerformance): +class FullBaseMailPerformance(BaseMailPerformance): @classmethod def setUpClass(cls): - super(TestMailPerformance, cls).setUpClass() + super().setUpClass() # users / followers cls.user_emp_email = mail_new_test_user( @@ -38,36 +40,43 @@ class TestMailPerformance(BaseMailPerformance): name='Paul Portal', ) cls.customers = cls.env['res.partner'].create([ - {'country_id': cls.env.ref('base.be').id, - 'email': 'customer.full.test.1@example.com', - 'name': 'Test Full Customer 1', - 'mobile': '0456112233', - 'phone': '0456112233', + { + 'country_id': cls.env.ref('base.be').id, + 'email': f'customer.full.test.{idx}@example.com', + 'name': f'Test Full Customer {idx}', + 'mobile': f'045600000{idx}', + 'phone': f'045611111{idx}', + } for idx in range(5) + ]) + cls.test_users = cls.user_employee + cls.user_test + cls.user_test_email + cls.user_emp_email + cls.user_portal + + # records + cls.record_containers = cls.env['mail.test.container.mc'].create([ + { + 'alias_name': 'test-alias-0', + 'customer_id': cls.customers[0].id, + 'name': 'Test Container 1', }, - {'country_id': cls.env.ref('base.be').id, - 'email': 'customer.full.test.2@example.com', - 'name': 'Test Full Customer 2', - 'mobile': '0456223344', - 'phone': '0456112233', + { + 'alias_name': 'test-alias-1', + 'customer_id': cls.customers[1].id, + 'name': 'Test Container 2', }, ]) - - # record - cls.record_container = cls.env['mail.test.container.mc'].create({ - 'alias_name': 'test-alias', - 'customer_id': cls.customer.id, - 'name': 'Test Container', - }) cls.record_ticket = cls.env['mail.test.ticket.mc'].create({ 'email_from': 'email.from@test.example.com', - 'container_id': cls.record_container.id, + 'container_id': cls.record_containers[0].id, 'customer_id': False, 'name': 'Test Ticket', 'user_id': cls.user_emp_email.id, }) cls.record_ticket.message_subscribe(cls.customers.ids + cls.user_admin.partner_id.ids + cls.user_portal.partner_id.ids) - def test_initial_values(self): + +@tagged('mail_performance', 'post_install', '-at_install') +class TestMailPerformance(FullBaseMailPerformance): + + def test_assert_initial_values(self): """ Simply ensure some values through all tests """ record_ticket = self.env['mail.test.ticket.mc'].browse(self.record_ticket.ids) self.assertEqual(record_ticket.message_partner_ids, @@ -95,3 +104,216 @@ class TestMailPerformance(BaseMailPerformance): new_message.notified_partner_ids, self.user_emp_email.partner_id + self.user_admin.partner_id + self.customers + self.user_portal.partner_id ) + + +@tagged('mail_performance', 'post_install', '-at_install') +class TestPortalFormatPerformance(FullBaseMailPerformance): + """Test performance of `portal_message_format` with multiple messages + with multiple attachments, with ratings. + + Those messages might not make sense functionally but they are crafted to + cover as much of the code as possible in regard to number of queries. + + Setup : + * 5 records (self.containers -> 5 mail.test.rating records, with + a different customer_id each) + * 2 messages / record + * 2 attachments / message + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + + # rating-enabled test records + cls.record_ratings = cls.env['mail.test.rating'].create([ + { + 'customer_id': cls.customers[idx].id, + 'name': f'TestRating_{idx}', + 'user_id': cls.test_users[idx].id, + + } + for idx in range(5) + ]) + + # messages and ratings + user_id_field = cls.env['ir.model.fields']._get(cls.record_ratings._name, 'user_id') + comment_subtype_id = cls.env['ir.model.data']._xmlid_to_res_id('mail.mt_comment') + cls.messages_all = cls.env['mail.message'].sudo().create([ + { + 'attachment_ids': [ + (0, 0, { + 'datas': 'data', + 'name': f'Test file {att_idx}', + 'res_id': record.id, + 'res_model': record._name, + }) + for att_idx in range(2) + ], + 'author_id': record.customer_id.id, + 'body': f'

Test {msg_idx}

', + 'date': datetime(2023, 5, 15, 10, 30, 5), + 'email_from': record.customer_id.email_formatted, + 'link_preview_ids': [ + (0, 0, { + 'source_url': 'https://www.odoo.com', + }), (0, 0, { + 'source_url': 'https://www.example.com', + }), + ], + 'notification_ids': [ + (0, 0, { + 'is_read': False, + 'notification_type': 'inbox', + 'res_partner_id': cls.customers[(msg_idx * 2)].id, + }), + (0, 0, { + 'is_read': True, + 'notification_type': 'email', + 'notification_status': 'sent', + 'res_partner_id': cls.customers[(msg_idx * 2) + 1].id, + }), + ], + 'message_type': 'comment', + 'model': record._name, + 'partner_ids': [ + (4, cls.customers[(msg_idx * 2)].id), + (4, cls.customers[record_idx].id), + ], + 'reaction_ids': [ + (0, 0, { + 'content': 'https://www.odoo.com', + 'partner_id': cls.customers[(msg_idx * 2) + 1].id + }), (0, 0, { + 'content': 'https://www.example.com', + 'partner_id': cls.customers[record_idx].id + }), + ], + 'res_id': record.id, + 'subject': f'Test Rating {msg_idx}', + 'subtype_id': comment_subtype_id, + 'starred_partner_ids': [ + (4, cls.customers[(msg_idx * 2)].id), + (4, cls.customers[(msg_idx * 2) + 1].id), + ], + 'tracking_value_ids': [ + (0, 0, { + 'field': user_id_field.id, + 'field_desc': 'Responsible', + 'new_value_char': 'new 1', + 'new_value_integer': record.user_id.id, + 'old_value_char': 'old 1', + 'old_value_integer': cls.user_admin.id, + }), + ] + } + for msg_idx in range(2) + for record_idx, record in enumerate(cls.record_ratings) + ]) + + cls.messages_records = [cls.env[message.model].browse(message.res_id) for message in cls.messages_all] + # ratings values related to rating-enabled records + cls.ratings_all = cls.env['rating.rating'].sudo().create([ + { + 'consumed': True, + 'message_id': message.id, + 'partner_id': record.customer_id.id, + 'publisher_comment': 'Comment', + 'publisher_id': cls.user_admin.partner_id.id, + 'publisher_datetime': datetime(2023, 5, 15, 10, 30, 5) - timedelta(days=2), + 'rated_partner_id': record.user_id.partner_id.id, + 'rating': 4, + 'res_id': message.res_id, + 'res_model_id': cls.env['ir.model']._get_id(message.model), + } + for rating_idx in range(2) + for message, record in zip(cls.messages_all, cls.messages_records) + ]) + + def test_assert_initial_values(self): + self.assertEqual(len(self.messages_all), 5 * 2) + self.assertEqual(len(self.ratings_all), len(self.messages_all) * 2) + + @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') + @users('employee') + @warmup + def test_portal_message_format_norating(self): + messages_all = self.messages_all.with_user(self.env.user) + + with self.assertQueryCount(employee=31): + # res = messages_all.portal_message_format(options=None) + res = messages_all.portal_message_format(options={'rating_include': False}) + + comment_subtype = self.env.ref('mail.mt_comment') + self.assertEqual(len(res), len(messages_all)) + for format_res, message, record in zip(res, messages_all, self.messages_records): + self.assertEqual(len(format_res['attachment_ids']), 2) + self.assertEqual( + format_res['attachment_ids'], + [ + { + 'access_token': message.attachment_ids[0].access_token, + 'checksum': message.attachment_ids[0].checksum, + 'filename': 'Test file 1', + 'id': message.attachment_ids[0].id, + 'mimetype': 'application/octet-stream', + 'name': 'Test file 1', + 'res_id': record.id, + 'res_model': record._name, + }, { + 'access_token': message.attachment_ids[1].access_token, + 'checksum': message.attachment_ids[1].checksum, + 'filename': 'Test file 0', + 'id': message.attachment_ids[1].id, + 'mimetype': 'application/octet-stream', + 'name': 'Test file 0', + 'res_id': record.id, + 'res_model': record._name, + } + ] + ) + self.assertEqual(format_res['author_id'], (record.customer_id.id, record.customer_id.display_name)) + self.assertEqual(format_res['author_avatar_url'], f'/web/image/mail.message/{message.id}/author_avatar/50x50') + self.assertEqual(format_res['date'], datetime(2023, 5, 15, 10, 30, 5)) + self.assertEqual(format_res['published_date_str'], 'May 15, 2023, 10:30:05 AM') + self.assertEqual(format_res['id'], message.id) + self.assertFalse(format_res['is_internal']) + self.assertFalse(format_res['is_message_subtype_note']) + self.assertEqual(format_res['subtype_id'], (comment_subtype.id, comment_subtype.name)) + # should not be in, not asked + self.assertNotIn('rating', format_res) + self.assertNotIn('rating_stats', format_res) + self.assertNotIn('rating_value', format_res) + + @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') + @users('employee') + @warmup + def test_portal_message_format_rating(self): + messages_all = self.messages_all.with_user(self.env.user) + + with self.assertQueryCount(employee=45): + res = messages_all.portal_message_format(options={'rating_include': True}) + + self.assertEqual(len(res), len(messages_all)) + for format_res, _message, _record in zip(res, messages_all, self.messages_records): + self.assertEqual(format_res['rating']['publisher_avatar'], f'/web/image/res.partner/{self.partner_admin.id}/avatar_128/50x50') + self.assertEqual(format_res['rating']['publisher_comment'], 'Comment') + self.assertEqual(format_res['rating']['publisher_id'], self.partner_admin.id) + self.assertEqual(format_res['rating']['publisher_datetime'], 'May 13, 2023, 10:30:05 AM') + self.assertEqual(format_res['rating']['publisher_name'], self.partner_admin.display_name) + self.assertDictEqual( + format_res['rating_stats'], + {'avg': 4.0, 'total': 4, 'percent': {1: 0.0, 2: 0.0, 3: 0.0, 4: 100.0, 5: 0.0}} + ) + self.assertEqual(format_res['rating_value'], 4) + + @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') + @users('employee') + @warmup + def test_portal_message_format_monorecord(self): + message = self.messages_all[0].with_user(self.env.user) + + with self.assertQueryCount(employee=18): + res = message.portal_message_format(options={'rating_include': True}) + + self.assertEqual(len(res), 1)