[REF] mail: update the message content in the mail thread model

Purpose
=======

Move the code which updates the mail message from the message model to mail
thread. Most other thread methods (like `_message_update_content_after_hook`)
are defined at record model level. It makes sense to delegate the update to
documents and not to the message. Message is a low-level technical object
that should not really hold business code.

While modifying this code, an update is done in the update content. We now
also allow to update the body without removing all attachments.

Finally tests are added as this feature was added without really testing
model code.

Task-2207626 (Rating: Delay rating notification to ease feedback)

Part-of: odoo/odoo#95623
Co-authored-by: Thibault Delavallée <tde@odoo.com>

addons/im_livechat/models/mail_channel.py

@@ -148,7 +148,7 @@ class MailChannel(models.Model):
                 'id': message.id,
                 'body': message.body,
             })
-        super()._message_update_content_after_hook(message=message)
+        return super()._message_update_content_after_hook(message=message)
 
     def _get_visitor_leave_message(self, operator=False, cancel=False):
         return _('Visitor has left the conversation.')

addons/mail/controllers/discuss.py

@@ -241,7 +241,13 @@ class DiscussController(http.Controller):
         message_sudo = guest.env['mail.message'].browse(message_id).sudo().exists()
         if not message_sudo.is_current_user_or_guest_author and not guest.env.user._is_admin():
             raise NotFound()
-        message_sudo._update_content(body=body, attachment_ids=attachment_ids)
+        if not message_sudo.model or not message_sudo.res_id:
+            raise NotFound()
+        request.env[message_sudo.model].browse([message_sudo.res_id])._message_update_content(
+            message_sudo,
+            body,
+            attachment_ids=attachment_ids
+        )
         return {
             'id': message_sudo.id,
             'body': message_sudo.body,

addons/mail/models/mail_message.py

@@ -808,25 +808,6 @@ class Message(models.Model):
         reaction.unlink()
         self.env[self.model].browse(self.res_id)._message_remove_reaction_after_hook(message=self, content=content)
 
-    def _update_content(self, body, attachment_ids):
-        self.ensure_one()
-        thread = self.env[self.model].browse(self.res_id)
-        thread._check_can_update_message_content(self)
-        self.body = body
-        if not attachment_ids:
-            self.attachment_ids._delete_and_notify()
-        else:
-            message_values = {
-                'model': self.model,
-                'body': body,
-                'res_id': self.res_id,
-            }
-            attachement_values = thread._message_post_process_attachments([], attachment_ids, message_values)
-            self.update(attachement_values)
-        # Cleanup related message data if the message is empty
-        self.sudo()._filter_empty()._cleanup_side_records()
-        thread._message_update_content_after_hook(self)
-
     # ------------------------------------------------------
     # MESSAGE READ / FETCH / FAILURE API
     # ------------------------------------------------------

addons/mail/models/mail_thread.py

@@ -433,14 +433,20 @@ class MailThread(models.AbstractModel):
             return self.with_context(lang=self.env.user.lang)
         return self
 
-    def _check_can_update_message_content(self, message):
-        """" Checks that the current user can update the content of the message. """
+    def _check_can_update_message_content(self, messages):
+        """" Checks that the current user can update the content of the message.
+        Current heuristic is
+
+          * limited to note;
+          * if no tracking;
+          * only for user generated content;
+        """
         note_id = self.env['ir.model.data']._xmlid_to_res_id('mail.mt_note')
-        if not message.subtype_id.id == note_id:
+        if any(message.subtype_id.id != note_id for message in messages):
             raise exceptions.UserError(_("Only logged notes can have their content updated on model '%s'", self._name))
-        if message.tracking_value_ids:
+        if messages.tracking_value_ids:
             raise exceptions.UserError(_("Messages with tracking values cannot be modified"))
-        if not message.message_type == 'comment':
+        if any(message.message_type != 'comment' for message in messages):
             raise exceptions.UserError(_("Only messages type comment can have their content updated"))
 
     # ------------------------------------------------------
@@ -3122,8 +3128,52 @@ class MailThread(models.AbstractModel):
         msg_not_comment.write(msg_vals)
         return True
 
+    def _message_update_content(self, message, body, attachment_ids=None,
+                                strict=True):
+        """ Update message content. Currently does not support attachments
+        specific code (see ``_message_post_process_attachments``), to be added
+        when necessary.
+
+        Private method to use for tooling, do not expose to interface as editing
+        messages should be avoided at all costs (think of: notifications already
+        sent, ...).
+
+        :param <mail.message> message: message to update, should be linked to self through
+          model and res_id;
+        :param str body: new body (None to skip its update);
+        :param list attachment_ids: list of new attachments IDs, replacing old one (None
+          to skip its update);
+        :param bool strict: whether to check for allowance before updating
+          content. This should be skipped only when really necessary as it
+          creates issues with already-sent notifications, lack of content
+          tracking, ...
+        """
+        self.ensure_one()
+        if strict:
+            self._check_can_update_message_content(message.sudo())
+
+        msg_values = {'body': body} if body is not None else {}
+        if attachment_ids:
+            msg_values.update(
+                self._message_post_process_attachments([], attachment_ids, {
+                    'body': body,
+                    'model': self._name,
+                    'res_id': self.id,
+                })
+            )
+        elif attachment_ids is not None:  # None means "no update"
+            message.attachment_ids._delete_and_notify()
+        if msg_values:
+            message.write(msg_values)
+
+        # cleanup related message data if the message is empty
+        message.sudo()._filter_empty()._cleanup_side_records()
+
+        return self._message_update_content_after_hook(message)
+
     def _message_update_content_after_hook(self, message):
         """ Hook to add custom behavior after having updated the message content. """
+        return True
 
     # ------------------------------------------------------
     # CONTROLLERS

addons/test_mail/tests/test_mail_message.py

@@ -65,13 +65,13 @@ class TestMessageValues(TestMailCommon):
         self.assertFalse(message.sudo().tracking_value_ids)
 
         # Reset body case
-        message._update_content('<p><br /></p>', attachment_ids=message.attachment_ids.ids)
+        record._message_update_content(message, '<p><br /></p>', attachment_ids=message.attachment_ids.ids)
         self.assertTrue(is_html_empty(message.body))
         self.assertFalse(message.sudo()._filter_empty(), 'Still having attachments')
 
         # Subtype content
         note_subtype.write({'description': 'Very important discussions'})
-        message._update_content('', None)
+        record._message_update_content(message, '', [])
         self.assertFalse(message.attachment_ids)
         self.assertEqual(message.notified_partner_ids, self.partner_admin)
         self.assertEqual(message.starred_partner_ids, self.partner_admin)
@@ -80,7 +80,7 @@ class TestMessageValues(TestMailCommon):
         # Completely void now
         note_subtype.write({'description': ''})
         self.assertEqual(message.sudo()._filter_empty(), message)
-        message._update_content('', None)
+        record._message_update_content(message, '', [])
         self.assertFalse(message.notified_partner_ids)
         self.assertFalse(message.starred_partner_ids)
 
@@ -93,7 +93,7 @@ class TestMessageValues(TestMailCommon):
         self.assertFalse(tracking_message.subtype_id.description)
         self.assertFalse(tracking_message.sudo()._filter_empty(), 'Has tracking values')
         with self.assertRaises(UserError, msg='Tracking values prevent from updating content'):
-            tracking_message._update_content('', None)
+            record._message_update_content(tracking_message, '', [])
 
     @mute_logger('odoo.models.unlink')
     def test_mail_message_format(self):

addons/test_mail/tests/test_mail_thread_internals.py

@@ -12,6 +12,94 @@ from odoo.tests.common import tagged, HttpCase, users
 from odoo.tools import mute_logger
 
 
+@tagged('mail_thread')
+class TestAPI(TestMailCommon, TestRecipients):
+
+    @classmethod
+    def setUpClass(cls):
+        super(TestAPI, cls).setUpClass()
+        cls.ticket_record = cls.env['mail.test.ticket'].with_context(cls._test_context).create({
+            'email_from': '"Paulette Vachette" <paulette@test.example.com>',
+            'name': 'Test',
+            'user_id': cls.user_employee.id,
+        })
+
+    @mute_logger('openerp.addons.mail.models.mail_mail')
+    @users('employee')
+    def test_message_update_content(self):
+        """ Test updating message content. """
+        ticket_record = self.ticket_record.with_env(self.env)
+        attachments = self.env['ir.attachment'].create(
+            self._generate_attachments_data(2, 'mail.compose.message', 0)
+        )
+
+        # post a note
+        message = ticket_record.message_post(
+            attachment_ids=attachments.ids,
+            body="<p>Initial Body</p>",
+            message_type="comment",
+            partner_ids=self.partner_1.ids,
+        )
+        self.assertEqual(message.attachment_ids, attachments)
+        self.assertEqual(set(message.mapped('attachment_ids.res_id')), set(ticket_record.ids))
+        self.assertEqual(set(message.mapped('attachment_ids.res_model')), set([ticket_record._name]))
+        self.assertEqual(message.body, "<p>Initial Body</p>")
+        self.assertEqual(message.subtype_id, self.env.ref('mail.mt_note'))
+
+        # update the content with new attachments
+        new_attachments = self.env['ir.attachment'].create(
+            self._generate_attachments_data(2, 'mail.compose.message', 0)
+        )
+        ticket_record._message_update_content(
+            message, "<p>New Body</p>",
+            attachment_ids=new_attachments.ids
+        )
+        self.assertEqual(message.attachment_ids, attachments + new_attachments)
+        self.assertEqual(set(message.mapped('attachment_ids.res_id')), set(ticket_record.ids))
+        self.assertEqual(set(message.mapped('attachment_ids.res_model')), set([ticket_record._name]))
+        self.assertEqual(message.body, "<p>New Body</p>")
+
+        # void attachments
+        ticket_record._message_update_content(
+            message, "<p>Another Body, void attachments</p>",
+            attachment_ids=[]
+        )
+        self.assertFalse(message.attachment_ids)
+        self.assertFalse((attachments + new_attachments).exists())
+        self.assertEqual(message.body, "<p>Another Body, void attachments</p>")
+
+    @mute_logger('openerp.addons.mail.models.mail_mail')
+    @users('employee')
+    def test_message_update_content_check(self):
+        """ Test cases where updating content should be prevented """
+        ticket_record = self.ticket_record.with_env(self.env)
+
+        # cannot edit user comments (subtype)
+        message = ticket_record.message_post(
+            body="<p>Initial Body</p>",
+            message_type="comment",
+            subtype_id=self.env.ref('mail.mt_comment').id,
+        )
+        with self.assertRaises(exceptions.UserError):
+            ticket_record._message_update_content(
+                message, "<p>New Body</p>"
+            )
+
+        message.sudo().write({'subtype_id': self.env.ref('mail.mt_note')})
+        ticket_record._message_update_content(
+            message, "<p>New Body</p>"
+        )
+
+        # cannot edit notifications
+        for message_type in ['notification', 'user_notification', 'email']:
+            message.sudo().write({'message_type': message_type})
+            with self.assertRaises(exceptions.UserError):
+                ticket_record._message_update_content(
+                    message, "<p>New Body</p>"
+                )
+
+
+@tagged('mail_thread')
 class TestChatterTweaks(TestMailCommon, TestRecipients):
 
     @classmethod