[FIX] mail: fix mail template onchange

The stable behaviour of the mail composer is the following:
- if a template adds new attachments, the composer only has this list
- if a template doesn't add attachments, the list is unchanged
- if no template is set, the list is cleared up
In the case of templates, we also check that both static and dynamic attachments
are added to the list.

We clean up after c6d718f2118 and subsequently 516f22c3987 which failed to take
into account the fact that the attachement list could be a blend of commands
and ids.
Transforming that is taken care of by _convert_to_write, which semantics was
changed by c6d718f2118.
To keep the second behaviour, we thus need to add a (5,) command.

opw 2003197

closes odoo/odoo#33707

Signed-off-by: Nans Lefebvre (len) <len@odoo.com>

2 files changed, 18 insertions, 18 deletions

diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py
index 913ca46bce9..117049d52e9 100644
--- a/addons/mail/wizard/mail_compose_message.py
+++ b/addons/mail/wizard/mail_compose_message.py
@@ -376,7 +376,6 @@ class MailComposer(models.TransientModel):
             - normal mode: return rendered values
             /!\ for x2many field, this onchange return command instead of ids
         """
-        attachment_ids = []
         if template_id and composition_mode == 'mass_mail':
             template = self.env['mail.template'].browse(template_id)
             fields = ['subject', 'body_html', 'email_from', 'reply_to', 'mail_server_id']
@@ -392,6 +391,7 @@ class MailComposer(models.TransientModel):
             values = self.generate_email_for_composer(template_id, [res_id])[res_id]
             # transform attachments into attachment_ids; not attached to the document because this will
             # be done further in the posting process, allowing to clean database if email not send
+            attachment_ids = []
             Attachment = self.env['ir.attachment']
             for attach_fname, attach_datas in values.pop('attachments', []):
                 data_attach = {
@@ -403,6 +403,8 @@ class MailComposer(models.TransientModel):
                     'type': 'binary',  # override default_type from context, possibly meant for another model!
                 }
                 attachment_ids.append(Attachment.create(data_attach).id)
+            if values.get('attachment_ids', []) or attachment_ids:
+                values['attachment_ids'] = [(5,)] + values.get('attachment_ids', []) + attachment_ids
         else:
             default_values = self.with_context(default_composition_mode=composition_mode, default_model=model, default_res_id=res_id).default_get(['composition_mode', 'model', 'res_id', 'parent_id', 'partner_ids', 'subject', 'body', 'email_from', 'reply_to', 'attachment_ids', 'mail_server_id'])
             values = dict((key, default_values[key]) for key in ['subject', 'body', 'partner_ids', 'email_from', 'reply_to', 'attachment_ids', 'mail_server_id'] if key in default_values)
@@ -414,10 +416,7 @@ class MailComposer(models.TransientModel):
         # ORM handle the assignation of command list on new onchange (api.v8),
         # this force the complete replacement of x2many field with
         # command and is compatible with onchange api.v7
-        attachment_ids += values.pop('attachment_ids' , [])
         values = self._convert_to_write(values)
-        if attachment_ids:
-            values.update(attachment_ids=[(6, 0, attachment_ids)])
 
         return {'value': values}
 
diff --git a/addons/test_mail/tests/test_mail_template.py b/addons/test_mail/tests/test_mail_template.py
index 4258fff577b..d899c6829b4 100644
--- a/addons/test_mail/tests/test_mail_template.py
+++ b/addons/test_mail/tests/test_mail_template.py
@@ -242,7 +242,7 @@ class TestMailTemplate(BaseFunctionalTest, MockEmails, TestRecipients):
         static attachments are not duplicated and while reports are re-generated,
         and that intermediary attachments are dropped."""
 
-        composer = self.env['mail.compose.message'].create({})
+        composer = self.env['mail.compose.message'].with_context(default_attachment_ids=[]).create({})
         report_template = self.env.ref('web.action_report_externalpreview')
         template_1 = self.email_template.copy({
             'report_template': report_template.id,
@@ -252,20 +252,21 @@ class TestMailTemplate(BaseFunctionalTest, MockEmails, TestRecipients):
             'report_template': report_template.id,
         })
 
-        onchange_templates = [template_1, template_2, template_1]
-        attachments_onchange = []
+        onchange_templates = [template_1, template_2, template_1, False]
+        attachments_onchange = [composer.attachment_ids]
         # template_1 has two static attachments and one dynamically generated report,
         # template_2 only has the report, so we should get 3, 1, 3 attachments
-        attachment_numbers = [3, 1, 3]
-
-        for template in onchange_templates:
-            onchange = composer.onchange_template_id(
-                template.id, 'comment', 'mail.test', self.test_record.id
-            )
-            values = composer._convert_to_record(composer._convert_to_cache(onchange['value']))
-            attachments = values['attachment_ids']
-            composer.attachment_ids = attachments  # we apply the onchange
-            attachments_onchange.append(attachments)
+        # and when there is no template, no attachments
+        attachment_numbers = [0, 3, 1, 3, 0]
+
+        with self.env.do_in_onchange():
+            for template in onchange_templates:
+                onchange = composer.onchange_template_id(
+                    template.id if template else False, 'comment', 'mail.test.simple', self.test_record.id
+                )
+                values = composer._convert_to_record(composer._convert_to_cache(onchange['value']))
+                attachments_onchange.append(values['attachment_ids'])
+                composer.update(onchange['value'])
 
         self.assertEqual(
             [len(attachments) for attachments in attachments_onchange],
@@ -273,6 +274,6 @@ class TestMailTemplate(BaseFunctionalTest, MockEmails, TestRecipients):
         )
 
         self.assertTrue(
-            len(attachments_onchange[0] & attachments_onchange[2]) == 2,
+            len(attachments_onchange[1] & attachments_onchange[3]) == 2,
             "The two static attachments on the template should be common to the two onchanges"
         )