[REF] account, hr_{expense|holidays}, mail: move main attachment to mixin

Main attachment is quite costly and as it is implemented in mail.thread and is
therefore present in most model. In order to improve performance, we remove it
here from mail.thread and add it through a new mixin,only on model that
effectively use that features.

See sub commits for more details.

Task-2648976

closes odoo/odoo#110734

Related: odoo/enterprise#36164
Related: odoo/upgrade#4262
Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>

addons/account/models/account_move.py

@@ -49,7 +49,7 @@ EMPTY = object()
 
 class AccountMove(models.Model):
     _name = "account.move"
-    _inherit = ['portal.mixin', 'mail.thread', 'mail.activity.mixin', 'sequence.mixin']
+    _inherit = ['portal.mixin', 'mail.thread.main.attachment', 'mail.activity.mixin', 'sequence.mixin']
     _description = "Journal Entry"
     _order = 'date desc, name desc, id desc'
     _mail_post_access = 'read'

addons/account/models/account_payment.py

@@ -7,7 +7,7 @@ from odoo.tools.misc import format_date, formatLang
 class AccountPayment(models.Model):
     _name = "account.payment"
     _inherits = {'account.move': 'move_id'}
-    _inherit = ['mail.thread', 'mail.activity.mixin']
+    _inherit = ['mail.thread.main.attachment', 'mail.activity.mixin']
     _description = "Payments"
     _order = "date desc, name desc"
     _check_company_auto = True

addons/crm/data/crm_lead_demo.xml

@@ -444,10 +444,6 @@ Andrew</p>]]></field>
             <field name="subtype_id" ref="mail.mt_comment"/>
             <field name="attachment_ids" eval="[(6, 0, [ref('msg_case15_attach1')])]"/>
         </record>
-        <!--Main attachment is usually registered by message_post, so here we must set it manually-->
-        <record id="crm_case_15" model="crm.lead">
-            <field name="message_main_attachment_id" ref="msg_case15_attach1"/>
-        </record>
         <record id="msg_case15_3" model="mail.message">
             <field name="subject">Re: Plan to buy RedHat servers</field>
             <field name="model">crm.lead</field>

addons/hr/models/hr_employee.py

@@ -26,7 +26,7 @@ class HrEmployeePrivate(models.Model):
     _name = "hr.employee"
     _description = "Employee"
     _order = 'name'
-    _inherit = ['hr.employee.base', 'mail.thread', 'mail.activity.mixin', 'resource.mixin', 'avatar.mixin']
+    _inherit = ['hr.employee.base', 'mail.thread.main.attachment', 'mail.activity.mixin', 'resource.mixin', 'avatar.mixin']
     _mail_post_access = 'read'
 
     # resource and user

addons/hr_expense/models/hr_expense.py

@@ -15,7 +15,7 @@ from odoo.tools import email_split, float_repr, float_compare, is_html_empty
 class HrExpense(models.Model):
 
     _name = "hr.expense"
-    _inherit = ['mail.thread', 'mail.activity.mixin', 'analytic.mixin']
+    _inherit = ['mail.thread.main.attachment', 'mail.activity.mixin', 'analytic.mixin']
     _description = "Expense"
     _order = "date desc, id desc"
     _check_company_auto = True
@@ -784,7 +784,7 @@ class HrExpenseSheet(models.Model):
         =================================================================================
     """
     _name = "hr.expense.sheet"
-    _inherit = ['mail.thread', 'mail.activity.mixin']
+    _inherit = ['mail.thread.main.attachment', 'mail.activity.mixin']
     _description = "Expense Report"
     _order = "accounting_date desc, id desc"
     _check_company_auto = True

addons/hr_holidays/models/hr_leave.py

@@ -72,7 +72,7 @@ class HolidaysRequest(models.Model):
     _name = "hr.leave"
     _description = "Time Off"
     _order = "date_from desc"
-    _inherit = ['mail.thread', 'mail.activity.mixin']
+    _inherit = ['mail.thread.main.attachment', 'mail.activity.mixin']
     _mail_post_access = 'read'
 
     @api.model

addons/hr_recruitment/models/hr_applicant.py

@@ -22,7 +22,7 @@ class Applicant(models.Model):
     _name = "hr.applicant"
     _description = "Applicant"
     _order = "priority desc, id desc"
-    _inherit = ['mail.thread.cc', 'mail.activity.mixin', 'utm.mixin']
+    _inherit = ['mail.thread.cc', 'mail.thread.main.attachment', 'mail.activity.mixin', 'utm.mixin']
     _mailing_enabled = True
     _primary_email = 'email_from'
 

addons/mail/models/__init__.py

@@ -13,6 +13,7 @@ from . import mail_composer_mixin
 from . import mail_thread
 from . import mail_thread_blacklist
 from . import mail_thread_cc
+from . import mail_thread_main_attachment
 from . import template_reset_mixin
 
 # mail models

addons/mail/models/ir_attachment.py

@@ -1,10 +1,13 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
+import contextlib
+
 from odoo import models
 from odoo.exceptions import AccessError
 from odoo.http import request
 
+
 class IrAttachment(models.Model):
     _inherit = 'ir.attachment'
 
@@ -18,23 +21,22 @@ class IrAttachment(models.Model):
     def register_as_main_attachment(self, force=True):
         """ Registers this attachment as the main one of the model it is
         attached to.
+
+        :param bool force: if set, the method always updates the existing main attachment
+            otherwise it only sets the main attachment if there is none.
         """
         self.ensure_one()
-        if not self.res_model:
+        if not self.res_model or not self.res_id:
             return
         related_record = self.env[self.res_model].browse(self.res_id)
-        if not related_record.check_access_rights('write', raise_exception=False):
+        if not related_record or \
+                not related_record.check_access_rights('write', raise_exception=False) or \
+                not hasattr(related_record, 'message_main_attachment_id'):
             return
-        # message_main_attachment_id field can be empty, that's why we compare to False;
-        # we are just checking that it exists on the model before writing it
-        if related_record and hasattr(related_record, 'message_main_attachment_id'):
-            if force or not related_record.message_main_attachment_id:
-                #Ignore AccessError, if you don't have access to modify the document
-                #Just don't set the value
-                try:
-                    related_record.message_main_attachment_id = self
-                except AccessError:
-                    pass
+
+        if force or not related_record.message_main_attachment_id:
+            with contextlib.suppress(AccessError):
+                related_record.message_main_attachment_id = self
 
     def _delete_and_notify(self):
         for attachment in self:

addons/mail/models/mail_thread.py

@@ -107,7 +107,6 @@ class MailThread(models.AbstractModel):
         'Number of errors', compute='_compute_message_has_error',
         help="Number of messages with delivery error")
     message_attachment_count = fields.Integer('Attachment Count', compute='_compute_message_attachment_count', groups="base.group_user")
-    message_main_attachment_id = fields.Many2one(string="Main Attachment", comodel_name='ir.attachment', copy=False)
 
     @api.depends('message_follower_ids')
     def _compute_message_partner_ids(self):
@@ -1971,11 +1970,6 @@ class MailThread(models.AbstractModel):
         """ Hook to add custom behavior after having posted the message. Both
         message and computed value are given, to try to lessen query count by
         using already-computed values instead of having to rebrowse things. """
-        # Set main attachment field if necessary
-        self._message_set_main_attachment_id([
-            attachment_command[1]
-            for attachment_command in (msg_values['attachment_ids'] or [])
-        ])
 
     def _message_mail_after_hook(self, mails):
         """ Hook to add custom behavior after having sent an mass mailing.
@@ -1983,16 +1977,6 @@ class MailThread(models.AbstractModel):
         :param mail.mail mails: mail.mail records about to be sent"""
         return
 
-    def _message_set_main_attachment_id(self, attachment_ids):
-        if self._name == 'mail.thread':
-            return
-        if attachment_ids and not self.message_main_attachment_id:
-            all_attachments = self.env['ir.attachment'].browse(attachment_ids)
-            prioritary_attachments = all_attachments.filtered(lambda x: x.mimetype.endswith('pdf')) \
-                                     or all_attachments.filtered(lambda x: x.mimetype.startswith('image')) \
-                                     or all_attachments
-            self.sudo().with_context(tracking_disable=True).message_main_attachment_id = prioritary_attachments[0].id
-
     def _process_attachments_for_post(self, attachments, attachment_ids, message_values):
         """ Preprocess attachments for MailTread.message_post() or MailMail.create().
         Purpose is to
@@ -3797,7 +3781,6 @@ class MailThread(models.AbstractModel):
             res['activities'] = self.activity_ids.activity_format()
         if 'attachments' in request_list:
             res['attachments'] = self._get_mail_thread_data_attachments()._attachment_format()
-            res['mainAttachment'] = {'id': self.message_main_attachment_id.id} if self.message_main_attachment_id else [('clear',)]
         if 'followers' in request_list:
             res['followers'] = [{
                 'id': follower.id,

addons/mail/models/mail_thread_main_attachment.py

@@ -0,0 +1,36 @@
+# -*- coding: utf-8 -*-
+# Part of Odoo. See LICENSE file for full copyright and licensing details.
+
+from odoo import fields, models
+
+
+class MailMainAttachmentMixin(models.AbstractModel):
+    """ Mixin that adds main attachment support to the MailThread class. """
+
+    _name = 'mail.thread.main.attachment'
+    _inherit = 'mail.thread'
+    _description = 'Mail Main Attachment management'
+
+    message_main_attachment_id = fields.Many2one(string="Main Attachment", comodel_name='ir.attachment', copy=False)
+
+    def _message_post_after_hook(self, message, msg_values):
+        """ Set main attachment field if necessary """
+        super()._message_post_after_hook(message, msg_values)
+        self._message_set_main_attachment_id([
+            attachment_command[1]
+            for attachment_command in (msg_values['attachment_ids'] or [])
+        ])
+
+    def _message_set_main_attachment_id(self, attachment_ids):
+        if attachment_ids and not self.message_main_attachment_id:
+            # Assign one of the attachments as the main according to the following priority: pdf, image, other types.
+            self.sudo().with_context(tracking_disable=True).message_main_attachment_id = max(
+                self.env['ir.attachment'].browse(attachment_ids),
+                key=lambda r: (r.mimetype.endswith('pdf'), r.mimetype.startswith('image'))
+            ).id
+
+    def _get_mail_thread_data(self, request_list):
+        res = super()._get_mail_thread_data(request_list)
+        if 'attachments' in request_list:
+            res['mainAttachment'] = {'id': self.message_main_attachment_id.id} if self.message_main_attachment_id else [('clear',)]
+        return res

addons/mail/static/tests/helpers/mock_server/controllers/discuss.js

@@ -398,9 +398,12 @@ patch(MockServer.prototype, "mail/controllers/discuss", {
             res["attachments"] = this._mockIrAttachment_attachmentFormat(
                 attachments.map((attachment) => attachment.id)
             );
-            res["mainAttachment"] = thread.message_main_attachment_id
-                ? { id: thread.message_main_attachment_id[0] }
-                : [["clear"]];
+            // Specific implementation of mail.thread.main.attachment
+            if (this.models[thread_model].fields["message_main_attachment_id"]) {
+                res["mainAttachment"] = thread.message_main_attachment_id
+                    ? { id: thread.message_main_attachment_id[0] }
+                    : [["clear"]];
+            }
         }
         if (request_list.includes("followers")) {
             const followers = this.pyEnv["mail.followers"].searchRead([

addons/test_mail/models/test_mail_models.py

@@ -32,6 +32,12 @@ class MailTestSimple(models.Model):
         return headers
 
 
+class MailTestSimpleWithMainAttachment(models.Model):
+    _description = 'Simple Chatter Model With Main Attachment Management'
+    _name = 'mail.test.simple.main.attachment'
+    _inherit = ['mail.test.simple', 'mail.thread.main.attachment']
+
+
 class MailTestGateway(models.Model):
     """ A very simple model only inheriting from mail.thread to test pure mass
     mailing features and base performances. """

addons/test_mail/security/ir.model.access.csv

@@ -3,6 +3,8 @@ access_mail_performance_thread,access_mail_performance_thread,model_mail_perform
 access_mail_performance_tracking_user,mail.performance.tracking,model_mail_performance_tracking,base.group_user,1,1,1,1
 access_mail_test_simple_portal,mail.test.simple.portal,model_mail_test_simple,base.group_portal,1,0,0,0
 access_mail_test_simple_user,mail.test.simple.user,model_mail_test_simple,base.group_user,1,1,1,1
+access_mail_test_simple_main_attachment_portal,mail.test.simple.main.attachment.portal,model_mail_test_simple_main_attachment,base.group_portal,1,0,0,0
+access_mail_test_simple_main_attachment_user,mail.test.simple.main.attachment.user,model_mail_test_simple_main_attachment,base.group_user,1,1,1,1
 access_mail_test_gateway_portal,mail.test.gateway.portal,model_mail_test_gateway,base.group_portal,1,0,0,0
 access_mail_test_gateway_user,mail.test.gateway.user,model_mail_test_gateway,base.group_user,1,1,1,1
 access_mail_test_gateway_groups_portal,mail.test.gateway.groups.portal,model_mail_test_gateway_groups,base.group_portal,1,0,0,0

addons/test_mail/tests/test_message_post.py

@@ -919,7 +919,13 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase):
 
     @users('employee')
     @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.addons.mail.models.mail_message_schedule')
-    def test_message_post_w_attachments(self):
+    def test_message_post_w_attachments_on_main_attachment_model(self):
+        """ Test posting a message with attachments on a model inheriting from
+        the mixin mail.thread.main.attachment.
+
+        As the mixin inherits from mail.thread, we test mainly features from
+        mail.thread but with the ones added of the main attachment mixin.
+        """
         _attachments = [
             ('List1', b'My first attachment'),
             ('List2', b'My second attachment'),
@@ -929,7 +935,12 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase):
         )
         _attachment_records[1].write({'mimetype': 'image/png'})  # to test message_main_attachment heuristic
 
-        test_record = self.env['mail.test.simple'].browse(self.test_record.ids)
+        test_record = self.env['mail.test.simple.main.attachment'].with_context(self._test_context).create({
+            'name': 'Test',
+            'email_from': 'ignasse@example.com',
+        })
+        self._reset_mail_context(test_record)
+        self.test_message.model = test_record._name
         self.assertFalse(test_record.message_main_attachment_id)
 
         with self.mock_mail_gateway():
@@ -949,8 +960,8 @@ class TestMessagePost(TestMessagePostCommon, CronMixinCase):
 
         # message attachments
         self.assertEqual(len(msg.attachment_ids), 5)
-        self.assertEqual(set(msg.attachment_ids.mapped('res_model')), set([self.test_record._name]))
-        self.assertEqual(set(msg.attachment_ids.mapped('res_id')), set([self.test_record.id]))
+        self.assertEqual(set(msg.attachment_ids.mapped('res_model')), {test_record._name})
+        self.assertEqual(set(msg.attachment_ids.mapped('res_id')), {test_record.id})
         self.assertEqual(set(base64.b64decode(x) for x in msg.attachment_ids.mapped('datas')),
                          set([b'AttContent_00', b'AttContent_01', b'AttContent_02', _attachments[0][1], _attachments[1][1]]))
         self.assertTrue(set(_attachment_records.ids).issubset(msg.attachment_ids.ids),