[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

@@ -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_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)

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),