From f8c7f2e5bb2495a0a7d68a15033a94a90f51dd9b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Dufays Date: Tue, 21 Feb 2023 13:41:23 +0000 Subject: [PATCH 1/4] [MOV] mail: move main attachment feature to mixin Main attachment is quite costly and as it is implemented in mail.thread, is 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 models that actually use that feature. Technical note: with the current implementation, even if the font-end view expect the model to implement the main attachment feature and the model doesn't, it doesn't crash. Indeed, the main attachment on the front-end is only used to know the first image to display in the image 'carousel' on the right (widget rendered through the presence of the class o_attachment_preview). And as 'register_as_main_attachment' of ir.attachment doesn't crash even if the model doesn't support main attachment feature, the only consequence is that the first image to display is not persisted (if you refresh after changing the main attachment, it will get back to the previous one). Task-2648976 Part-of: odoo/odoo#110734 --- addons/mail/models/__init__.py | 1 + addons/mail/models/mail_thread.py | 17 --------- .../models/mail_thread_main_attachment.py | 36 +++++++++++++++++++ 3 files changed, 37 insertions(+), 17 deletions(-) create mode 100644 addons/mail/models/mail_thread_main_attachment.py diff --git a/addons/mail/models/__init__.py b/addons/mail/models/__init__.py index 03bcd6eb3c1..aebd5ca5fbe 100644 --- a/addons/mail/models/__init__.py +++ b/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 diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index f9d3da88868..2b3ed38fc6b 100644 --- a/addons/mail/models/mail_thread.py +++ b/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, diff --git a/addons/mail/models/mail_thread_main_attachment.py b/addons/mail/models/mail_thread_main_attachment.py new file mode 100644 index 00000000000..dec236784ef --- /dev/null +++ b/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: + 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 _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 From 4991dbc9a832c107c55b0f35b9897a290ea3869f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Dufays Date: Tue, 21 Feb 2023 13:50:56 +0000 Subject: [PATCH 2/4] [REF] mail: adapt test using new main attachment mixin Task-2648976 Part-of: odoo/odoo#110734 --- .../mock_server/controllers/discuss.js | 9 ++++++--- addons/test_mail/models/test_mail_models.py | 6 ++++++ addons/test_mail/security/ir.model.access.csv | 2 ++ addons/test_mail/tests/test_message_post.py | 19 +++++++++++++++---- 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/addons/mail/static/tests/helpers/mock_server/controllers/discuss.js b/addons/mail/static/tests/helpers/mock_server/controllers/discuss.js index 7b94b9f2b46..71d39bb58d9 100644 --- a/addons/mail/static/tests/helpers/mock_server/controllers/discuss.js +++ b/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([ diff --git a/addons/test_mail/models/test_mail_models.py b/addons/test_mail/models/test_mail_models.py index 62f2494e1c4..d479cea1ce0 100644 --- a/addons/test_mail/models/test_mail_models.py +++ b/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. """ diff --git a/addons/test_mail/security/ir.model.access.csv b/addons/test_mail/security/ir.model.access.csv index 0c36bef86ce..94d31f29d8a 100644 --- a/addons/test_mail/security/ir.model.access.csv +++ b/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 diff --git a/addons/test_mail/tests/test_message_post.py b/addons/test_mail/tests/test_message_post.py index 881d60c2199..f9da43dd00a 100644 --- a/addons/test_mail/tests/test_message_post.py +++ b/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')), set([test_record._name])) + self.assertEqual(set(msg.attachment_ids.mapped('res_id')), set([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), From 4cec1e12d86c49678f326842c062a0a887770f28 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Dufays Date: Tue, 21 Feb 2023 14:00:20 +0000 Subject: [PATCH 3/4] [REF] various: apply main attachment mixin on models that use the feature Add mail.thread.main.attachment in the inheritance chain of all models that use the main attachment feature. Note that we remove the main attachment from crm_case_15 of the demo data as crm_lead doesn't use that feature. Task-2648976 Part-of: odoo/odoo#110734 --- addons/account/models/account_move.py | 2 +- addons/account/models/account_payment.py | 2 +- addons/crm/data/crm_lead_demo.xml | 4 ---- addons/hr/models/hr_employee.py | 2 +- addons/hr_expense/models/hr_expense.py | 4 ++-- addons/hr_holidays/models/hr_leave.py | 2 +- addons/hr_recruitment/models/hr_applicant.py | 2 +- 7 files changed, 7 insertions(+), 11 deletions(-) diff --git a/addons/account/models/account_move.py b/addons/account/models/account_move.py index db90f984acd..494ffe94c79 100644 --- a/addons/account/models/account_move.py +++ b/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' diff --git a/addons/account/models/account_payment.py b/addons/account/models/account_payment.py index 20eb353866c..c8634590845 100644 --- a/addons/account/models/account_payment.py +++ b/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 diff --git a/addons/crm/data/crm_lead_demo.xml b/addons/crm/data/crm_lead_demo.xml index 8cbc3c03a4a..31385b1349d 100644 --- a/addons/crm/data/crm_lead_demo.xml +++ b/addons/crm/data/crm_lead_demo.xml @@ -444,10 +444,6 @@ Andrew

]]> - - - - Re: Plan to buy RedHat servers crm.lead diff --git a/addons/hr/models/hr_employee.py b/addons/hr/models/hr_employee.py index a851e18d937..81c807f801a 100644 --- a/addons/hr/models/hr_employee.py +++ b/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 diff --git a/addons/hr_expense/models/hr_expense.py b/addons/hr_expense/models/hr_expense.py index 2910a4db5cc..bdb4654c6a5 100644 --- a/addons/hr_expense/models/hr_expense.py +++ b/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 diff --git a/addons/hr_holidays/models/hr_leave.py b/addons/hr_holidays/models/hr_leave.py index a03bc41a9f7..9612ed30749 100644 --- a/addons/hr_holidays/models/hr_leave.py +++ b/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 diff --git a/addons/hr_recruitment/models/hr_applicant.py b/addons/hr_recruitment/models/hr_applicant.py index 0a091639ba7..e9730b74a46 100644 --- a/addons/hr_recruitment/models/hr_applicant.py +++ b/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' From ffd976cc69138a8a70db9c466733b4d7037b3e8d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Dufays Date: Tue, 21 Feb 2023 14:12:49 +0000 Subject: [PATCH 4/4] [IMP] {test_}mail: make some improvements to the code, no functional changes Some code cleanup done when moving the main attachment into its own mixin: - improved docstring - early returns - code simplification No functional changes should occur with this commit. Task-2648976 Part-of: odoo/odoo#110734 --- addons/mail/models/ir_attachment.py | 26 ++++++++++--------- .../models/mail_thread_main_attachment.py | 10 +++---- addons/test_mail/tests/test_message_post.py | 4 +-- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/addons/mail/models/ir_attachment.py b/addons/mail/models/ir_attachment.py index 277d7c35004..487aef52fb7 100644 --- a/addons/mail/models/ir_attachment.py +++ b/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: diff --git a/addons/mail/models/mail_thread_main_attachment.py b/addons/mail/models/mail_thread_main_attachment.py index dec236784ef..82ab9b7306f 100644 --- a/addons/mail/models/mail_thread_main_attachment.py +++ b/addons/mail/models/mail_thread_main_attachment.py @@ -23,11 +23,11 @@ class MailMainAttachmentMixin(models.AbstractModel): def _message_set_main_attachment_id(self, attachment_ids): 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 + # 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) diff --git a/addons/test_mail/tests/test_message_post.py b/addons/test_mail/tests/test_message_post.py index f9da43dd00a..7d90526ceea 100644 --- a/addons/test_mail/tests/test_message_post.py +++ b/addons/test_mail/tests/test_message_post.py @@ -960,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([test_record._name])) - self.assertEqual(set(msg.attachment_ids.mapped('res_id')), set([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),