From ffd976cc69138a8a70db9c466733b4d7037b3e8d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Dufays Date: Tue, 21 Feb 2023 14:12:49 +0000 Subject: [PATCH] [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),