From 60dc58cdf4323c2f4d6934d7be7bdbdacdabbe9c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 10:25:44 +0000
Subject: [PATCH 1/8] [IMP] test_mail: clean test log with appropriate mute
 logger

Similar to what is done for the other tests, mute unnecessary log such as
`odoo.tests: skip sending email in test mode`.

Part of task-2180311
PR: #43841
---
 addons/test_mail/tests/test_performance.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index 1b1a2ca3915..fcba93efe62 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -253,7 +253,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
 
     @users('__system__', 'emp')
     @warmup
-    @mute_logger('odoo.addons.mail.models.mail_mail')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail')
     def test_mail_composer(self):
         test_record = self.env['mail.test.full'].browse(self.test_record_full.id)
         customer_id = self.customer.id
@@ -272,7 +272,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
 
     @users('__system__', 'emp')
     @warmup
-    @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
     def test_mail_composer_w_template(self):
         test_record = self.env['mail.test.full'].browse(self.test_record_full.id)
         test_template = self.env['mail.template'].browse(self.test_template_full.id)
@@ -292,7 +292,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
         # remove created partner to ensure tests are the same each run
         self.env['res.partner'].sudo().search([('email', '=', 'nopartner.test@example.com')]).unlink()
 
-    @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
     @users('__system__', 'emp')
     @warmup
     def test_message_assignation_email(self):
@@ -345,7 +345,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
                 message_type='comment',
                 subtype_xmlid='mail.mt_comment')
 
-    @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
     @users('__system__', 'emp')
     @warmup
     def test_message_post_one_email_notification(self):

From 70eeb2ece9599e18928093cb8c9f659a2ce9c806 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 10:26:29 +0000
Subject: [PATCH 2/8] [IMP] test_mail: adapt query count

Part of task-2180311
PR: #43841
---
 addons/test_mail/tests/test_performance.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index fcba93efe62..988543257c6 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -276,8 +276,8 @@ class TestMailAPIPerformance(BaseMailPerformance):
     def test_mail_composer_w_template(self):
         test_record = self.env['mail.test.full'].browse(self.test_record_full.id)
         test_template = self.env['mail.template'].browse(self.test_template_full.id)
-        # TODO XDO/TDE/MAT FIXME non deterministic between 25 and 29 queries
-        with self.assertQueryCount(__system__=28, emp=29):
+        # TODO FIXME non deterministic, last check 25 feb 2020. Runbot at most +7 compared to local.
+        with self.assertQueryCount(__system__=27, emp=28):
             composer = self.env['mail.compose.message'].with_context({
                 'default_composition_mode': 'comment',
                 'default_model': test_record._name,

From b9e1f0d700d024ffefeaefb8aaedcb9bae04354b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 10:26:38 +0000
Subject: [PATCH 3/8] [IMP] test_mail: introduce performance test for
 `message_format`

It was partially tested already as part of some other performance tests, but
this new test will cover more specific cases of `message_format` and
`_message_read_dict_postprocess`.

The goal is to ensure the performances are not made worse by the following
commits, or even to be able to notice when they are made better.

Part of task-2180311
PR: #43841
---
 addons/test_mail/tests/test_performance.py | 113 +++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index 988543257c6..f7da7907891 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -696,6 +696,119 @@ class TestMailComplexPerformance(BaseMailPerformance):
         self.assertEqual(rec1.message_ids[2].notified_partner_ids, self.partners | self.user_portal.partner_id)
         self.assertEqual(len(rec1.message_ids), 3)
 
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @users('emp')
+    @warmup
+    def test_message_format(self):
+        """Test performance of `_message_read_dict_postprocess` 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.
+        """
+        name_field = self.env['ir.model.fields']._get(self.umbrella._name, 'name')
+        customer_id_field = self.env['ir.model.fields']._get(self.umbrella._name, 'customer_id')
+
+        messages = self.env['mail.message'].sudo().create([{
+            'subject': 'Test 0',
+            'body': '<p>Test 0</p>',
+            'author_id': self.partners[0].id,
+            'email_from': self.partners[0].email,
+            'model': 'mail.test',
+            'res_id': self.umbrella.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': '<p>Test 1</p>',
+            'author_id': self.partners[1].id,
+            'email_from': self.partners[1].email,
+            'model': 'mail.test',
+            'res_id': self.umbrella.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(emp=13):
+            res = messages.message_format()
+            self.assertEqual(len(res), 2)
+            for message in res:
+                self.assertEqual(len(message['attachment_ids']), 2)
+
+        messages.flush()
+        messages.invalidate_cache()
+
+        with self.assertQueryCount(emp=19):
+            res = messages.message_format()
+            self.assertEqual(len(res), 2)
+            for message in res:
+                self.assertEqual(len(message['attachment_ids']), 2)
+
 
 @tagged('mail_performance')
 class TestMailHeavyPerformancePost(BaseMailPerformance):

From ad5454f0ef285c304590a6f7aae9f0634f280ae7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 10:34:00 +0000
Subject: [PATCH 4/8] [IMP] mail, test_mail: simplify attachment

in `_message_read_dict_postprocess`.

This makes the method easier to follow and does not increase the query count.

It actually reduces it when the attachment data were already in the cache, by
not always reading them again.

The `has_access_to_model` is removed and replaced by `sudo` because:
- from portal everything is sudo so it's pointless to check access on top of it
- from backend, it's almost impossible to trigger the case, and it's not like
  returning is_main True would leak any sensitive information to an employee

Part of task-2180311
PR: #43841
---
 addons/mail/models/mail_message.py         | 41 ++++++++++------------
 addons/test_mail/tests/test_performance.py |  4 +--
 2 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index 9b1dd26b0ed..d5f013dcaa6 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -949,9 +949,10 @@ class Message(models.Model):
             :param list messages: list of message, as get_dict result
             :param dict message_tree: {[msg.id]: msg browse record as super user}
         """
-        # 1. Aggregate partners (author_id and partner_ids), attachments and tracking values
+        safari = request and request.httprequest.user_agent.browser == 'safari'
+
+        # 1. Aggregate partners (author_id and partner_ids) and tracking values
         partners = self.env['res.partner'].sudo()
-        attachments = self.env['ir.attachment']
         message_ids = list(message_tree.keys())
         email_notification_tree = {}
         for message in message_tree.values():
@@ -961,24 +962,12 @@ class Message(models.Model):
             email_notification_tree[message.id] = message.notification_ids.filtered(
                 lambda n: n.notification_type == 'email' and n.res_partner_id.active and
                 (n.notification_status in ('bounce', 'exception', 'canceled') or n.res_partner_id.partner_share))
-            if message.attachment_ids:
-                attachments |= message.attachment_ids
         partners |= self.env['mail.notification'].concat(*email_notification_tree.values()).mapped('res_partner_id')
         # Read partners as SUPERUSER -> message being browsed as SUPERUSER it is already the case
         partners_names = partners.name_get()
         partner_tree = dict((partner[0], partner) for partner in partners_names)
 
-        # 2. Attachments as SUPERUSER, because could receive msg and attachments for doc uid cannot see
-        attachments_data = attachments.sudo().read(['id', 'name', 'mimetype'])
-        safari = request and request.httprequest.user_agent.browser == 'safari'
-        attachments_tree = dict((attachment['id'], {
-            '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'],
-        }) for attachment in attachments_data)
-
-        # 3. Tracking values
+        # 2. Tracking values
         tracking_values = self.env['mail.tracking.value'].sudo().search([('mail_message_id', 'in', message_ids)])
         message_to_tracking = dict()
         tracking_tree = dict.fromkeys(tracking_values.ids, False)
@@ -994,7 +983,7 @@ class Message(models.Model):
                     'field_type': tracking.field_type,
                 }
 
-        # 4. Update message dictionaries
+        # 3. Update message dictionaries
         for message_dict in messages:
             message_id = message_dict.get('id')
             message = message_tree[message_id]
@@ -1012,15 +1001,21 @@ class Message(models.Model):
             for notification in email_notification_tree[message.id]:
                 customer_email_data.append((partner_tree[notification.res_partner_id.id][0], partner_tree[notification.res_partner_id.id][1], notification.notification_status))
 
-            has_access_to_model = message.model and self.env[message.model].check_access_rights('read', raise_exception=False)
-            main_attachment = None
-            if message.attachment_ids and message.res_id and issubclass(self.pool[message.model], self.pool['mail.thread']) and has_access_to_model:
-                main_attachment =  self.env[message.model].browse(message.res_id).message_main_attachment_id
+            # Attachments
+            main_attachment = self.env['ir.attachment']
+            if message.attachment_ids and message.res_id and issubclass(self.pool[message.model], self.pool['mail.thread']):
+                main_attachment = self.env[message.model].sudo().browse(message.res_id).message_main_attachment_id
             attachment_ids = []
             for attachment in message.attachment_ids:
-                if attachment.id in attachments_tree:
-                    attachments_tree[attachment.id]['is_main'] = main_attachment == attachment
-                    attachment_ids.append(attachments_tree[attachment.id])
+                attachment_ids.append({
+                    '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,
+                    'is_main': main_attachment == attachment
+                })
+
+            # Tracking values
             tracking_value_ids = []
             for tracking_value_id in message_to_tracking.get(message_id, list()):
                 if tracking_value_id in tracking_tree:
diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index f7da7907891..fcf52bae9f3 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -794,7 +794,7 @@ class TestMailComplexPerformance(BaseMailPerformance):
             ]
         }])
 
-        with self.assertQueryCount(emp=13):
+        with self.assertQueryCount(emp=12):
             res = messages.message_format()
             self.assertEqual(len(res), 2)
             for message in res:
@@ -928,7 +928,7 @@ class TestMailHeavyPerformancePost(BaseMailPerformance):
         ]
         self.attachements = self.env['ir.attachment'].with_user(self.env.user).create(self.vals)
         attachement_ids = self.attachements.ids
-        with self.assertQueryCount(emp=91):
+        with self.assertQueryCount(emp=90):
             self.cr.sql_log = self.warm and self.cr.sql_log_count
             record.with_context({}).message_post(
                 body='<p>Test body <img src="cid:cid1"> <img src="cid:cid2"></p>',

From dca44f53a24ca86fbef3abc7df64f33f1830dbb0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 10:38:01 +0000
Subject: [PATCH 5/8] [IMP] mail: simplify partner in
 `_message_read_dict_postprocess`

This makes the method easier to follow and does not increase the query count.

Part of task-2180311
PR: #43841
---
 addons/mail/models/mail_message.py | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index d5f013dcaa6..25d1133778d 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -951,23 +951,16 @@ class Message(models.Model):
         """
         safari = request and request.httprequest.user_agent.browser == 'safari'
 
-        # 1. Aggregate partners (author_id and partner_ids) and tracking values
-        partners = self.env['res.partner'].sudo()
+        # 1. Aggregate notifications
         message_ids = list(message_tree.keys())
         email_notification_tree = {}
         for message in message_tree.values():
-            if message.author_id:
-                partners |= message.author_id
             # find all notified partners
             email_notification_tree[message.id] = message.notification_ids.filtered(
                 lambda n: n.notification_type == 'email' and n.res_partner_id.active and
                 (n.notification_status in ('bounce', 'exception', 'canceled') or n.res_partner_id.partner_share))
-        partners |= self.env['mail.notification'].concat(*email_notification_tree.values()).mapped('res_partner_id')
-        # Read partners as SUPERUSER -> message being browsed as SUPERUSER it is already the case
-        partners_names = partners.name_get()
-        partner_tree = dict((partner[0], partner) for partner in partners_names)
 
-        # 2. Tracking values
+        # 2. Aggregate tracking values
         tracking_values = self.env['mail.tracking.value'].sudo().search([('mail_message_id', 'in', message_ids)])
         message_to_tracking = dict()
         tracking_tree = dict.fromkeys(tracking_values.ids, False)
@@ -987,8 +980,10 @@ class Message(models.Model):
         for message_dict in messages:
             message_id = message_dict.get('id')
             message = message_tree[message_id]
+
+            # Author
             if message.author_id:
-                author = partner_tree[message.author_id.id]
+                author = (message.author_id.id, message.author_id.display_name)
             else:
                 author = (0, message.email_from)
             customer_email_status = (
@@ -999,7 +994,8 @@ class Message(models.Model):
             )
             customer_email_data = []
             for notification in email_notification_tree[message.id]:
-                customer_email_data.append((partner_tree[notification.res_partner_id.id][0], partner_tree[notification.res_partner_id.id][1], notification.notification_status))
+                partner_name_get = notification.res_partner_id.name_get()[0]
+                customer_email_data.append((partner_name_get[0], partner_name_get[1], notification.notification_status))
 
             # Attachments
             main_attachment = self.env['ir.attachment']

From 04fda71e425d44d7142003d57ecc7c4e1fd3d99b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 10:41:46 +0000
Subject: [PATCH 6/8] [IMP] mail, test_mail: simplify notification and tracking
 values

in `message_format` and in `_message_read_dict_postprocess`.

This makes the method easier to follow and does not increase the query count.

It actually reduces it when the notifications and tracking data were already in
the cache, by not always reading them again.

Part of task-2180311
PR: #43841
---
 addons/mail/models/mail_message.py         | 68 +++++++---------------
 addons/test_mail/tests/test_performance.py | 12 ++--
 2 files changed, 28 insertions(+), 52 deletions(-)

diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index 25d1133778d..606d30cd4a0 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -951,32 +951,6 @@ class Message(models.Model):
         """
         safari = request and request.httprequest.user_agent.browser == 'safari'
 
-        # 1. Aggregate notifications
-        message_ids = list(message_tree.keys())
-        email_notification_tree = {}
-        for message in message_tree.values():
-            # find all notified partners
-            email_notification_tree[message.id] = message.notification_ids.filtered(
-                lambda n: n.notification_type == 'email' and n.res_partner_id.active and
-                (n.notification_status in ('bounce', 'exception', 'canceled') or n.res_partner_id.partner_share))
-
-        # 2. Aggregate tracking values
-        tracking_values = self.env['mail.tracking.value'].sudo().search([('mail_message_id', 'in', message_ids)])
-        message_to_tracking = dict()
-        tracking_tree = dict.fromkeys(tracking_values.ids, False)
-        for tracking in tracking_values:
-            groups = tracking.field_groups
-            if not groups or self.env.is_superuser() or self.user_has_groups(groups):
-                message_to_tracking.setdefault(tracking.mail_message_id.id, list()).append(tracking.id)
-                tracking_tree[tracking.id] = {
-                    'id': tracking.id,
-                    'changed_field': tracking.field_desc,
-                    'old_value': tracking.get_old_display_value()[0],
-                    'new_value': tracking.get_new_display_value()[0],
-                    'field_type': tracking.field_type,
-                }
-
-        # 3. Update message dictionaries
         for message_dict in messages:
             message_id = message_dict.get('id')
             message = message_tree[message_id]
@@ -986,6 +960,8 @@ class Message(models.Model):
                 author = (message.author_id.id, message.author_id.display_name)
             else:
                 author = (0, message.email_from)
+
+            # Notifications
             customer_email_status = (
                 (all(n.notification_status == 'sent' for n in message.notification_ids if n.notification_type == 'email') and 'sent') or
                 (any(n.notification_status == 'exception' for n in message.notification_ids if n.notification_type == 'email') and 'exception') or
@@ -993,9 +969,12 @@ class Message(models.Model):
                 'ready'
             )
             customer_email_data = []
-            for notification in email_notification_tree[message.id]:
-                partner_name_get = notification.res_partner_id.name_get()[0]
-                customer_email_data.append((partner_name_get[0], partner_name_get[1], notification.notification_status))
+            filtered_notifications = message.notification_ids.filtered(lambda n:
+                n.notification_type == 'email' and n.res_partner_id.active and
+                (n.notification_status in ('bounce', 'exception', 'canceled') or n.res_partner_id.partner_share)
+            )
+            for notification in filtered_notifications:
+                customer_email_data.append((notification.res_partner_id.id, notification.res_partner_id.display_name, notification.notification_status))
 
             # Attachments
             main_attachment = self.env['ir.attachment']
@@ -1013,9 +992,16 @@ class Message(models.Model):
 
             # Tracking values
             tracking_value_ids = []
-            for tracking_value_id in message_to_tracking.get(message_id, list()):
-                if tracking_value_id in tracking_tree:
-                    tracking_value_ids.append(tracking_tree[tracking_value_id])
+            for tracking in message.tracking_value_ids:
+                groups = tracking.field_groups
+                if not groups or self.env.is_superuser() or self.user_has_groups(groups):
+                    tracking_value_ids.append({
+                        'id': tracking.id,
+                        'changed_field': tracking.field_desc,
+                        'old_value': tracking.get_old_display_value()[0],
+                        'new_value': tracking.get_new_display_value()[0],
+                        'field_type': tracking.field_type,
+                    })
 
             message_dict.update({
                 'author_id': author,
@@ -1118,22 +1104,12 @@ class Message(models.Model):
         com_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_comment')
         note_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_note')
 
-        # fetch notification status
-
-        notif_dict = defaultdict(lambda: defaultdict(list))
-        notifs = self.env['mail.notification'].sudo().search([('mail_message_id', 'in', list(mid for mid in message_tree)), ('res_partner_id', '!=', False)])
-
-        for notif in notifs:
-            mid = notif.mail_message_id.id
-            if notif.is_read:
-                notif_dict[mid]['history_partner_ids'].append(notif.res_partner_id.id)
-            else:
-                notif_dict[mid]['needaction_partner_ids'].append(notif.res_partner_id.id)
-
         for message in message_values:
+            message_sudo = message_tree[message['id']]
+            notifs = message_sudo.notification_ids.filtered(lambda n: n.res_partner_id)
             message.update({
-                'needaction_partner_ids': notif_dict[message['id']]['needaction_partner_ids'],
-                'history_partner_ids': notif_dict[message['id']]['history_partner_ids'],
+                'needaction_partner_ids': notifs.filtered(lambda n: not n.is_read).res_partner_id.ids,
+                'history_partner_ids': notifs.filtered(lambda n: n.is_read).res_partner_id.ids,
                 'is_note': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['id'] == note_id,
                 'is_discussion': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['id'] == com_id,
                 'subtype_description': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['description']
diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index fcf52bae9f3..135db277b0c 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -267,7 +267,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
                 'partner_ids': [(4, customer_id)],
             })
 
-        with self.assertQueryCount(__system__=36, emp=41):
+        with self.assertQueryCount(__system__=34, emp=39):
             composer.send_mail()
 
     @users('__system__', 'emp')
@@ -286,7 +286,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
             }).create({})
             composer.onchange_template_id_wrapper()
 
-        with self.assertQueryCount(__system__=44, emp=48):
+        with self.assertQueryCount(__system__=42, emp=46):
             composer.send_mail()
 
         # remove created partner to ensure tests are the same each run
@@ -307,7 +307,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     @warmup
     def test_message_assignation_inbox(self):
         record = self.env['mail.test.track'].create({'name': 'Test'})
-        with self.assertQueryCount(__system__=25, emp=27):
+        with self.assertQueryCount(__system__=23, emp=25):
             record.write({
                 'user_id': self.user_test.id,
             })
@@ -363,7 +363,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     def test_message_post_one_inbox_notification(self):
         record = self.env['mail.test.simple'].create({'name': 'Test'})
 
-        with self.assertQueryCount(__system__=20, emp=22):
+        with self.assertQueryCount(__system__=18, emp=20):
             record.message_post(
                 body='<p>Test Post Performances with an inbox ping</p>',
                 partner_ids=self.user_test.partner_id.ids,
@@ -794,7 +794,7 @@ class TestMailComplexPerformance(BaseMailPerformance):
             ]
         }])
 
-        with self.assertQueryCount(emp=12):
+        with self.assertQueryCount(emp=10):
             res = messages.message_format()
             self.assertEqual(len(res), 2)
             for message in res:
@@ -928,7 +928,7 @@ class TestMailHeavyPerformancePost(BaseMailPerformance):
         ]
         self.attachements = self.env['ir.attachment'].with_user(self.env.user).create(self.vals)
         attachement_ids = self.attachements.ids
-        with self.assertQueryCount(emp=90):
+        with self.assertQueryCount(emp=88):
             self.cr.sql_log = self.warm and self.cr.sql_log_count
             record.with_context({}).message_post(
                 body='<p>Test body <img src="cid:cid1"> <img src="cid:cid2"></p>',

From 2490676e16964eaaf5a7c5ecb2525883c15dc6ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 10:50:53 +0000
Subject: [PATCH 7/8] [IMP] mail, test_mail: simplify subtype in
 `message_format`

This makes the method easier to follow and does not increase the query count.

It actually reduces it when the subtype data were already in the cache, by
not always reading them again.

Part of task-2180311
PR: #43841
---
 addons/mail/models/mail_message.py         | 12 +++---------
 addons/test_mail/tests/test_performance.py | 12 ++++++------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index 606d30cd4a0..21d92b5193e 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -1095,12 +1095,6 @@ class Message(models.Model):
         message_tree = dict((m.id, m) for m in self.sudo())
         self._message_read_dict_postprocess(message_values, message_tree)
 
-        # add subtype data (is_note flag, is_discussion flag , subtype_description). Do it as sudo
-        # because portal / public may have to look for internal subtypes
-        subtype_ids = [msg['subtype_id'][0] for msg in message_values if msg['subtype_id']]
-        subtypes = self.env['mail.message.subtype'].sudo().browse(subtype_ids).read(['internal', 'description','id'])
-        subtypes_dict = dict((subtype['id'], subtype) for subtype in subtypes)
-
         com_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_comment')
         note_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_note')
 
@@ -1110,9 +1104,9 @@ class Message(models.Model):
             message.update({
                 'needaction_partner_ids': notifs.filtered(lambda n: not n.is_read).res_partner_id.ids,
                 'history_partner_ids': notifs.filtered(lambda n: n.is_read).res_partner_id.ids,
-                'is_note': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['id'] == note_id,
-                'is_discussion': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['id'] == com_id,
-                'subtype_description': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['description']
+                'is_note': message_sudo.subtype_id.id == note_id,
+                'is_discussion': message_sudo.subtype_id.id == com_id,
+                'subtype_description': message_sudo.subtype_id.description
             })
             message['is_notification'] = message['message_type'] == 'user_notification'
 
diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index 135db277b0c..61c2b96a61a 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -267,7 +267,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
                 'partner_ids': [(4, customer_id)],
             })
 
-        with self.assertQueryCount(__system__=34, emp=39):
+        with self.assertQueryCount(__system__=33, emp=38):
             composer.send_mail()
 
     @users('__system__', 'emp')
@@ -286,7 +286,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
             }).create({})
             composer.onchange_template_id_wrapper()
 
-        with self.assertQueryCount(__system__=42, emp=46):
+        with self.assertQueryCount(__system__=41, emp=45):
             composer.send_mail()
 
         # remove created partner to ensure tests are the same each run
@@ -307,7 +307,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     @warmup
     def test_message_assignation_inbox(self):
         record = self.env['mail.test.track'].create({'name': 'Test'})
-        with self.assertQueryCount(__system__=23, emp=25):
+        with self.assertQueryCount(__system__=22, emp=24):
             record.write({
                 'user_id': self.user_test.id,
             })
@@ -363,7 +363,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     def test_message_post_one_inbox_notification(self):
         record = self.env['mail.test.simple'].create({'name': 'Test'})
 
-        with self.assertQueryCount(__system__=18, emp=20):
+        with self.assertQueryCount(__system__=17, emp=19):
             record.message_post(
                 body='<p>Test Post Performances with an inbox ping</p>',
                 partner_ids=self.user_test.partner_id.ids,
@@ -794,7 +794,7 @@ class TestMailComplexPerformance(BaseMailPerformance):
             ]
         }])
 
-        with self.assertQueryCount(emp=10):
+        with self.assertQueryCount(emp=9):
             res = messages.message_format()
             self.assertEqual(len(res), 2)
             for message in res:
@@ -803,7 +803,7 @@ class TestMailComplexPerformance(BaseMailPerformance):
         messages.flush()
         messages.invalidate_cache()
 
-        with self.assertQueryCount(emp=19):
+        with self.assertQueryCount(emp=18):
             res = messages.message_format()
             self.assertEqual(len(res), 2)
             for message in res:

From e3afe4d067e9e4eec54e0f0eadfd7316ae57dc99 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Fri, 6 Mar 2020 11:11:29 +0000
Subject: [PATCH 8/8] [IMP] models, (test_)mail, portal(_rating): remove
 `message_tree`

from `message_format` and from `_message_read_dict_postprocess`.

The reading and initial formatting is done by `_message_read_dict_postprocess`,
which has been renamed more simply to `_message_format` due to its new goal.

This makes the methods easier to follow and does not increase the query count.

It actually reduces it when the data were already in the cache, by not always
reading them again.

Part of task-2180311
PR: #43841
---
 addons/mail/models/mail_message.py          | 65 +++++++++------------
 addons/portal/models/mail_message.py        | 12 ++--
 addons/portal_rating/models/mail_message.py | 10 ++--
 addons/test_mail/tests/test_performance.py  | 20 +++----
 odoo/models.py                              | 15 ++++-
 5 files changed, 60 insertions(+), 62 deletions(-)

diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index 21d92b5193e..cb5b71d10b8 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -941,35 +941,29 @@ class Message(models.Model):
     # MESSAGE READ / FETCH / FAILURE API
     # ------------------------------------------------------
 
-    @api.model
-    def _message_read_dict_postprocess(self, messages, message_tree):
-        """ Post-processing on values given by message_read. This method will
-            handle partners in batch to avoid doing numerous queries.
-
-            :param list messages: list of message, as get_dict result
-            :param dict message_tree: {[msg.id]: msg browse record as super user}
-        """
+    def _message_format(self, fnames):
+        """Reads values from messages and formats them for the web client."""
+        self.check_access_rule('read')
+        vals_list = self._read_format(fnames)
         safari = request and request.httprequest.user_agent.browser == 'safari'
-
-        for message_dict in messages:
-            message_id = message_dict.get('id')
-            message = message_tree[message_id]
+        for vals in vals_list:
+            message_sudo = self.browse(vals['id']).sudo().with_prefetch(self.ids)
 
             # Author
-            if message.author_id:
-                author = (message.author_id.id, message.author_id.display_name)
+            if message_sudo.author_id:
+                author = (message_sudo.author_id.id, message_sudo.author_id.display_name)
             else:
-                author = (0, message.email_from)
+                author = (0, message_sudo.email_from)
 
             # Notifications
             customer_email_status = (
-                (all(n.notification_status == 'sent' for n in message.notification_ids if n.notification_type == 'email') and 'sent') or
-                (any(n.notification_status == 'exception' for n in message.notification_ids if n.notification_type == 'email') and 'exception') or
-                (any(n.notification_status == 'bounce' for n in message.notification_ids if n.notification_type == 'email') and 'bounce') or
+                (all(n.notification_status == 'sent' for n in message_sudo.notification_ids if n.notification_type == 'email') and 'sent') or
+                (any(n.notification_status == 'exception' for n in message_sudo.notification_ids if n.notification_type == 'email') and 'exception') or
+                (any(n.notification_status == 'bounce' for n in message_sudo.notification_ids if n.notification_type == 'email') and 'bounce') or
                 'ready'
             )
             customer_email_data = []
-            filtered_notifications = message.notification_ids.filtered(lambda n:
+            filtered_notifications = message_sudo.notification_ids.filtered(lambda n:
                 n.notification_type == 'email' and n.res_partner_id.active and
                 (n.notification_status in ('bounce', 'exception', 'canceled') or n.res_partner_id.partner_share)
             )
@@ -978,10 +972,10 @@ class Message(models.Model):
 
             # Attachments
             main_attachment = self.env['ir.attachment']
-            if message.attachment_ids and message.res_id and issubclass(self.pool[message.model], self.pool['mail.thread']):
-                main_attachment = self.env[message.model].sudo().browse(message.res_id).message_main_attachment_id
+            if message_sudo.attachment_ids and message_sudo.res_id and issubclass(self.pool[message_sudo.model], self.pool['mail.thread']):
+                main_attachment = self.env[message_sudo.model].sudo().browse(message_sudo.res_id).message_main_attachment_id
             attachment_ids = []
-            for attachment in message.attachment_ids:
+            for attachment in message_sudo.attachment_ids:
                 attachment_ids.append({
                     'id': attachment.id,
                     'filename': attachment.name,
@@ -992,7 +986,7 @@ class Message(models.Model):
 
             # Tracking values
             tracking_value_ids = []
-            for tracking in message.tracking_value_ids:
+            for tracking in message_sudo.tracking_value_ids:
                 groups = tracking.field_groups
                 if not groups or self.env.is_superuser() or self.user_has_groups(groups):
                     tracking_value_ids.append({
@@ -1003,7 +997,7 @@ class Message(models.Model):
                         'field_type': tracking.field_type,
                     })
 
-            message_dict.update({
+            vals.update({
                 'author_id': author,
                 'customer_email_status': customer_email_status,
                 'customer_email_data': customer_email_data,
@@ -1011,7 +1005,7 @@ class Message(models.Model):
                 'tracking_value_ids': tracking_value_ids,
             })
 
-        return True
+        return vals_list
 
     def message_fetch_failed(self):
         messages = self.search([
@@ -1091,28 +1085,25 @@ class Message(models.Model):
                     'moderation_status': 'pending_moderation'
                 }
         """
-        message_values = self.read(self._get_message_format_fields())
-        message_tree = dict((m.id, m) for m in self.sudo())
-        self._message_read_dict_postprocess(message_values, message_tree)
+        vals_list = self._message_format(self._get_message_format_fields())
 
         com_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_comment')
         note_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_note')
 
-        for message in message_values:
-            message_sudo = message_tree[message['id']]
+        for vals in vals_list:
+            message_sudo = self.browse(vals['id']).sudo().with_prefetch(self.ids)
             notifs = message_sudo.notification_ids.filtered(lambda n: n.res_partner_id)
-            message.update({
+            vals.update({
                 'needaction_partner_ids': notifs.filtered(lambda n: not n.is_read).res_partner_id.ids,
                 'history_partner_ids': notifs.filtered(lambda n: n.is_read).res_partner_id.ids,
                 'is_note': message_sudo.subtype_id.id == note_id,
                 'is_discussion': message_sudo.subtype_id.id == com_id,
-                'subtype_description': message_sudo.subtype_id.description
+                'subtype_description': message_sudo.subtype_id.description,
+                'is_notification': vals['message_type'] == 'user_notification',
             })
-            message['is_notification'] = message['message_type'] == 'user_notification'
-
-            if message['model'] and self.env[message['model']]._original_module:
-                message['module_icon'] = modules.module.get_module_icon(self.env[message['model']]._original_module)
-        return message_values
+            if vals['model'] and self.env[vals['model']]._original_module:
+                vals['module_icon'] = modules.module.get_module_icon(self.env[vals['model']]._original_module)
+        return vals_list
 
     def _get_message_format_fields(self):
         return [
diff --git a/addons/portal/models/mail_message.py b/addons/portal/models/mail_message.py
index cfda6d5eaa0..91d40d7a891 100644
--- a/addons/portal/models/mail_message.py
+++ b/addons/portal/models/mail_message.py
@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
-from odoo import api, models
+from odoo import models
 
 
 class MailMessage(models.Model):
@@ -15,12 +15,10 @@ class MailMessage(models.Model):
         ])
 
     def _portal_message_format(self, fields_list):
-        message_values = self.read(fields_list)
-        message_tree = dict((m.id, m) for m in self.sudo())
-        self._message_read_dict_postprocess(message_values, message_tree)
+        vals_list = self._message_format(fields_list)
         IrAttachmentSudo = self.env['ir.attachment'].sudo()
-        for message in message_values:
-            for attachment in message.get('attachment_ids', []):
+        for vals in vals_list:
+            for attachment in vals.get('attachment_ids', []):
                 if not attachment.get('access_token'):
                     attachment['access_token'] = IrAttachmentSudo.browse(attachment['id']).generate_access_token()[0]
-        return message_values
+        return vals_list
diff --git a/addons/portal_rating/models/mail_message.py b/addons/portal_rating/models/mail_message.py
index 9f2be5d1a25..20716960503 100644
--- a/addons/portal_rating/models/mail_message.py
+++ b/addons/portal_rating/models/mail_message.py
@@ -13,16 +13,16 @@ class MailMessage(models.Model):
             field_list += ['rating_value']
         return super(MailMessage, self)._portal_message_format(field_list)
 
-    def _message_read_dict_postprocess(self, message_values, message_tree):
+    def _message_format(self, fnames):
         """ Override the method to add information about a publisher comment
         on each rating messages if requested, and compute a plaintext value of it.
         """
-        res = super(MailMessage, self)._message_read_dict_postprocess(message_values, message_tree)
+        vals_list = super(MailMessage, self)._message_format(fnames)
 
         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 values in message_values:
-                values["rating"] = mid_rating_tree.get(values['id'], {})
-        return res
+            for vals in vals_list:
+                vals["rating"] = mid_rating_tree.get(vals['id'], {})
+        return vals_list
diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index 61c2b96a61a..b952fe73388 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -3,7 +3,7 @@
 import base64
 
 from odoo.addons.base.tests.common import TransactionCaseWithUserDemo
-from odoo.tests.common import TransactionCase, users, warmup
+from odoo.tests.common import users, warmup
 from odoo.tests import tagged
 from odoo.tools import mute_logger, formataddr
 
@@ -267,7 +267,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
                 'partner_ids': [(4, customer_id)],
             })
 
-        with self.assertQueryCount(__system__=33, emp=38):
+        with self.assertQueryCount(__system__=31, emp=37):
             composer.send_mail()
 
     @users('__system__', 'emp')
@@ -286,7 +286,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
             }).create({})
             composer.onchange_template_id_wrapper()
 
-        with self.assertQueryCount(__system__=41, emp=45):
+        with self.assertQueryCount(__system__=39, emp=44):
             composer.send_mail()
 
         # remove created partner to ensure tests are the same each run
@@ -307,7 +307,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     @warmup
     def test_message_assignation_inbox(self):
         record = self.env['mail.test.track'].create({'name': 'Test'})
-        with self.assertQueryCount(__system__=22, emp=24):
+        with self.assertQueryCount(__system__=19, emp=22):
             record.write({
                 'user_id': self.user_test.id,
             })
@@ -363,7 +363,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     def test_message_post_one_inbox_notification(self):
         record = self.env['mail.test.simple'].create({'name': 'Test'})
 
-        with self.assertQueryCount(__system__=17, emp=19):
+        with self.assertQueryCount(__system__=15, emp=18):
             record.message_post(
                 body='<p>Test Post Performances with an inbox ping</p>',
                 partner_ids=self.user_test.partner_id.ids,
@@ -700,9 +700,9 @@ class TestMailComplexPerformance(BaseMailPerformance):
     @users('emp')
     @warmup
     def test_message_format(self):
-        """Test performance of `_message_read_dict_postprocess` and of
-        `message_format` with multiple messages with multiple attachments,
-        different authors, various notifications, and different tracking values.
+        """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.
         """
@@ -794,7 +794,7 @@ class TestMailComplexPerformance(BaseMailPerformance):
             ]
         }])
 
-        with self.assertQueryCount(emp=9):
+        with self.assertQueryCount(emp=6):
             res = messages.message_format()
             self.assertEqual(len(res), 2)
             for message in res:
@@ -928,7 +928,7 @@ class TestMailHeavyPerformancePost(BaseMailPerformance):
         ]
         self.attachements = self.env['ir.attachment'].with_user(self.env.user).create(self.vals)
         attachement_ids = self.attachements.ids
-        with self.assertQueryCount(emp=88):
+        with self.assertQueryCount(emp=86):
             self.cr.sql_log = self.warm and self.cr.sql_log_count
             record.with_context({}).message_post(
                 body='<p>Test body <img src="cid:cid1"> <img src="cid:cid2"></p>',
diff --git a/odoo/models.py b/odoo/models.py
index bbec718b5a3..a5d8ddb7900 100644
--- a/odoo/models.py
+++ b/odoo/models.py
@@ -2932,11 +2932,20 @@ Fields:
                         stored_fields.add(f.name)
         self._read(stored_fields)
 
-        # retrieve results from records; this takes values from the cache and
-        # computes remaining fields
+        return self._read_format(fnames=fields, load=load)
+
+    def _read_format(self, fnames, load='_classic_read'):
+        """Returns a list of dictionaries mapping field names to their values,
+        with one dictionary per record that exists.
+
+        The output format is similar to the one expected from the `read` method.
+
+        The current method is different from `read` because it retrieves its
+        values from the cache without doing a query when it is avoidable.
+        """
         data = [(record, {'id': record._ids[0]}) for record in self]
         use_name_get = (load == '_classic_read')
-        for name in fields:
+        for name in fnames:
             convert = self._fields[name].convert_to_read
             for record, vals in data:
                 # missing records have their vals empty