[IMP] mail: improve body / template sync check in composer mixin

RATIONALE

Purpose of this commit is to add helper field knowing if body any model
inheriting from the 'mail.composer.mixin' is synchronized with their
template value.

SPECIFICATIONS

Add a field to check if body of a composer is the same as its template

  * compare both current (raw) values and sanitized values, as sanitization
    process may change some bits of html code;
  * correctly check for void content using the tool;

Obviously this may be broken if editor usage changes the source template value
DOM when saving it in the composer. However we can't do much more than using
the original and sanitized values from template and compare them with the
current composer values.

It is used in this commit to simplify a bit code in '_render_field' override.
It is used to detect any change in body based on template. It highlights
potential changes in QWeb directives which requires usage of editor group.
This method is also cleaned in order to have less calls to super() in nested
if-elif structures. As some additional code will be added soon it is better
to have a simpler code flow.

Finally add some tests about current state of fields synchronization notably
when setting a template with void values, or removing the template.

Task-3046371 (Mail: Better Language Support in Composer)

Part-of: odoo/odoo#106177

addons/mail/models/mail_composer_mixin.py

@@ -25,6 +25,10 @@ class MailComposerMixin(models.AbstractModel):
     body = fields.Html(
         'Contents', compute='_compute_body', readonly=False, store=True,
         render_engine='qweb', render_options={'post_process': True}, sanitize=False)
+    body_has_template_value = fields.Boolean(
+        'Body content is the same as tmeplate',
+        compute='_compute_body_has_template_value',
+    )
     template_id = fields.Many2one('mail.template', 'Mail Template', domain="[('model', '=', render_model)]")
     # Language: override mail.render.mixin field, copy template value
     lang = fields.Char(compute='_compute_lang', precompute=True, readonly=False, store=True)
@@ -48,11 +52,24 @@ class MailComposerMixin(models.AbstractModel):
             elif not composer_mixin.body:
                 composer_mixin.body = False
 
+    @api.depends('body', 'template_id')
+    def _compute_body_has_template_value(self):
+        """ Computes if the current body is the same as the one from template.
+        Both real and sanitized values are considered, to avoid editor issues
+        as much as possible. """
+        for composer_mixin in self:
+            if not tools.is_html_empty(composer_mixin.body) and composer_mixin.template_id:
+                template_value = composer_mixin.template_id.body_html
+                sanitized_template_value = tools.html_sanitize(template_value)
+                composer_mixin.body_has_template_value = composer_mixin.body in (template_value, sanitized_template_value)
+            else:
+                composer_mixin.body_has_template_value = False
+
     @api.depends('template_id')
     def _compute_lang(self):
+        """ Take value form template when set. When removing the template
+        reset the value to avoid keeping part of template configuration. """
         for composer_mixin in self:
-            """ Take value form template when set. When removing the template
-            reset the value to avoid keeping part of template configuration. """
             if composer_mixin.template_id.lang:
                 composer_mixin.lang = composer_mixin.template_id.lang
             elif not composer_mixin.template_id:
@@ -73,35 +90,40 @@ class MailComposerMixin(models.AbstractModel):
             )
 
     def _render_field(self, field, *args, **kwargs):
-        """Render the given field on the given records.
-        This method bypass the rights when needed to
-        be able to render the template values in mass mode.
+        """ Render the given field on the given records. This method enters
+        sudo mode to allow qweb rendering (which is otherwise reserved for
+        the 'mail template editor' group') if we consider it safe. Safe
+        means content comes from the template which is a validated master
+        data. As a summary the heuristic is :
+
+          * if no template, do not bypass the check;
+          * if current user is a template editor, do not bypass the check;
+          * if record value and template value are the same (or equals the
+            sanitized value in case of an HTML field), bypass the check;
+          * for body: if current user cannot edit it, force template value back
+            then bypass the check;
         """
         if field not in self._fields:
             raise ValueError(_("The field %s does not exist on the model %s", field, self._name))
 
-        composer_value = self[field]
-
-        if (
-            not self.template_id
-            or self.is_mail_template_editor
-        ):
+        if not self.template_id:
             # Do not need to bypass the verification
-            return super(MailComposerMixin, self)._render_field(field, *args, **kwargs)
+            return super()._render_field(field, *args, **kwargs)
 
+        composer_value = self[field]
         template_field = 'body_html' if field == 'body' else field
         assert template_field in self.template_id._fields
         template_value = self.template_id[template_field]
 
-        if field == 'body':
-            sanitized_template_value = tools.html_sanitize(template_value)
-            if not self.can_edit_body or composer_value in (sanitized_template_value, template_value):
-                # Take the previous body which we can trust without HTML editor reformatting
-                self.body = self.template_id.body_html
-                return super(MailComposerMixin, self.sudo())._render_field(field, *args, **kwargs)
+        call_sudo = False
+        if (not self.is_mail_template_editor and field == 'body' and
+            (not self.can_edit_body or self.body_has_template_value)):
+            call_sudo = True
+            # Take the previous body which we can trust without HTML editor reformatting
+            self.body = self.template_id.body_html
+        if (not self.is_mail_template_editor and field != 'body' and
+              composer_value == template_value):
+            call_sudo = True
 
-        elif composer_value == template_value:
-            # The value is the same as the mail template so we trust it
-            return super(MailComposerMixin, self.sudo())._render_field(field, *args, **kwargs)
-
-        return super(MailComposerMixin, self)._render_field(field, *args, **kwargs)
+        record = self.sudo() if call_sudo else self
+        return super(MailComposerMixin, record)._render_field(field, *args, **kwargs)

addons/test_mail/tests/test_mail_composer_mixin.py

@@ -41,19 +41,62 @@ class TestMailComposerMixin(TestMailCommon, TestRecipients):
     def test_content_sync(self):
         """ Test updating template updates the dynamic fields accordingly. """
         source = self.test_record.with_env(self.env)
+        template = self.mail_template.with_env(self.env)
+        template_void = template.copy()
+        template_void.write({
+            'body_html': '<p><br /></p>',
+            'lang': False,
+            'subject': False,
+        })
+
         composer = self.env['mail.test.composer.mixin'].create({
             'name': 'Invite',
-            'template_id': self.mail_template.id,
+            'template_id': template.id,
             'source_ids': [(4, source.id)],
         })
-        self.assertEqual(composer.body, self.mail_template.body_html)
-        self.assertEqual(composer.lang, self.mail_template.lang)
-        self.assertEqual(composer.subject, self.mail_template.subject)
+        self.assertEqual(composer.body, template.body_html)
+        self.assertTrue(composer.body_has_template_value)
+        self.assertEqual(composer.lang, template.lang)
+        self.assertEqual(composer.subject, template.subject)
 
-        subject = composer._render_field('subject', source.ids)[source.id]
-        self.assertEqual(subject, f'EnglishSubject for {source.name}')
+        # check rendering
         body = composer._render_field('body', source.ids)[source.id]
         self.assertEqual(body, f'<p>EnglishBody for {source.name}</p>')
+        subject = composer._render_field('subject', source.ids)[source.id]
+        self.assertEqual(subject, f'EnglishSubject for {source.name}')
+
+        # manual values > template default values
+        composer.write({
+            'body': '<p>CustomBody for <t t-out="object.name"/></p>',
+            'subject': 'CustomSubject for {{ object.name }}',
+        })
+        self.assertFalse(composer.body_has_template_value)
+
+        body = composer._render_field('body', source.ids)[source.id]
+        self.assertEqual(body, f'<p>CustomBody for {source.name}</p>')
+        subject = composer._render_field('subject', source.ids)[source.id]
+        self.assertEqual(subject, f'CustomSubject for {source.name}')
+
+        # template with void values: should not force void (TODO)
+        composer.template_id = template_void.id
+        self.assertEqual(composer.body, f'<p><br /></p>',
+                         'TODO: should not force void value')
+        self.assertFalse(composer.body_has_template_value)
+        self.assertEqual(composer.lang, template.lang)
+        self.assertEqual(composer.subject, False,
+                         'TODO: should not force void value')
+        # temporarily reput values
+        composer.body = template.body_html
+        composer.subject = template.subject
+
+        # reset template TOOD should reset
+        composer.write({'template_id': False})
+        self.assertEqual(composer.body, self.mail_template.body_html,
+                         'TODO: should reset')
+        self.assertFalse(composer.body_has_template_value)
+        self.assertFalse(composer.lang)
+        self.assertEqual(composer.subject, self.mail_template.subject,
+                         'TODO: should reset')
 
     @users("employee")
     def test_rendering_custom(self):