From eb75a77646d073e184d6eb101b3bf84dd3967f95 Mon Sep 17 00:00:00 2001 From: std-odoo Date: Thu, 30 Jun 2022 07:54:07 +0000 Subject: [PATCH] [REF] mail: update the message content in the mail thread model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- addons/im_livechat/models/mail_channel.py | 2 +- addons/mail/controllers/discuss.py | 8 +- addons/mail/models/mail_message.py | 19 ---- addons/mail/models/mail_thread.py | 60 +++++++++++-- addons/test_mail/tests/test_mail_message.py | 8 +- .../tests/test_mail_thread_internals.py | 88 +++++++++++++++++++ 6 files changed, 155 insertions(+), 30 deletions(-) diff --git a/addons/im_livechat/models/mail_channel.py b/addons/im_livechat/models/mail_channel.py index 26d3e4c70e5..79bb445f835 100644 --- a/addons/im_livechat/models/mail_channel.py +++ b/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.') diff --git a/addons/mail/controllers/discuss.py b/addons/mail/controllers/discuss.py index 7a748fea508..ebeacc8a6a0 100644 --- a/addons/mail/controllers/discuss.py +++ b/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, diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index 5f86648fcf7..a8bb8e81e6c 100644 --- a/addons/mail/models/mail_message.py +++ b/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 # ------------------------------------------------------ diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 661742115a2..c62648d5255 100644 --- a/addons/mail/models/mail_thread.py +++ b/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 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 diff --git a/addons/test_mail/tests/test_mail_message.py b/addons/test_mail/tests/test_mail_message.py index d43489c58f6..5de0cdc9de7 100644 --- a/addons/test_mail/tests/test_mail_message.py +++ b/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('


', attachment_ids=message.attachment_ids.ids) + record._message_update_content(message, '


', 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): diff --git a/addons/test_mail/tests/test_mail_thread_internals.py b/addons/test_mail/tests/test_mail_thread_internals.py index 7ec34dcc778..0a0cb046dad 100644 --- a/addons/test_mail/tests/test_mail_thread_internals.py +++ b/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" ', + '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="

Initial Body

", + 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, "

Initial Body

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

New Body

", + 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, "

New Body

") + + # void attachments + ticket_record._message_update_content( + message, "

Another Body, void attachments

", + attachment_ids=[] + ) + self.assertFalse(message.attachment_ids) + self.assertFalse((attachments + new_attachments).exists()) + self.assertEqual(message.body, "

Another Body, void attachments

") + + @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="

Initial Body

", + message_type="comment", + subtype_id=self.env.ref('mail.mt_comment').id, + ) + with self.assertRaises(exceptions.UserError): + ticket_record._message_update_content( + message, "

New Body

" + ) + + message.sudo().write({'subtype_id': self.env.ref('mail.mt_note')}) + ticket_record._message_update_content( + message, "

New Body

" + ) + + # 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, "

New Body

" + ) + + +@tagged('mail_thread') class TestChatterTweaks(TestMailCommon, TestRecipients): @classmethod