From 20fe457377e1d366e1fa1d03768bfee0edf5680d Mon Sep 17 00:00:00 2001 From: Renaud Thiry Date: Mon, 6 Mar 2023 11:50:31 +0000 Subject: [PATCH 1/4] [FIX] mail: set error_msg in template_preview If the preview wizard was used to preview a template on a model that has no record, error_msg would not be set. This means the field is never set in that case and creating the wizard results in a cache miss on that field. The fix is to simply set it, and we add a test to cover that flow of the wizard. task-3162320 X-original-commit: 0976ce53e0a97a1f49dc2a262cf66355242fde25 Part-of: odoo/odoo#118710 --- addons/mail/wizard/mail_template_preview.py | 5 +++-- .../test_mail/tests/test_mail_template_preview.py | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/addons/mail/wizard/mail_template_preview.py b/addons/mail/wizard/mail_template_preview.py index 68ca33421cc..aea4beddcc3 100644 --- a/addons/mail/wizard/mail_template_preview.py +++ b/addons/mail/wizard/mail_template_preview.py @@ -71,6 +71,7 @@ class MailTemplatePreview(models.TransientModel): """ Preview the mail template (body, subject, ...) depending of the language and the record reference, more precisely the record id for the defined model of the mail template. If no record id is selectable/set, the inline_template placeholders won't be replace in the display information. """ + error_msg = False mail_template = self.mail_template_id.with_context(lang=self.lang) if not self.resource_ref or not self.resource_ref.id: self._set_mail_attributes() @@ -82,10 +83,10 @@ class MailTemplatePreview(models.TransientModel): self._MAIL_TEMPLATE_FIELDS )[self.resource_ref.id] self._set_mail_attributes(values=mail_values) - self.error_msg = False except (ValueError, UserError) as user_error: self._set_mail_attributes() - self.error_msg = user_error.args[0] + error_msg = user_error.args[0] + self.error_msg = error_msg def _set_mail_attributes(self, values=None): for field in self._MAIL_TEMPLATE_FIELDS: diff --git a/addons/test_mail/tests/test_mail_template_preview.py b/addons/test_mail/tests/test_mail_template_preview.py index 213f9e25e43..df223d6fd97 100644 --- a/addons/test_mail/tests/test_mail_template_preview.py +++ b/addons/test_mail/tests/test_mail_template_preview.py @@ -20,6 +20,20 @@ class TestMailTemplateTools(TestMailTemplateCommon): self.assertEqual(len(self.test_template.partner_to.split(',')), 2) self.assertTrue(self.test_record.email_from) + def test_mail_template_preview_empty_database(self): + """Check behaviour of the wizard when there is no record for the target model.""" + self.env['mail.test.lang'].search([]).unlink() + test_template = self.env['mail.template'].browse(self.test_template.ids) + preview = self.env['mail.template.preview'].create({ + 'mail_template_id': test_template.id, + }) + + self.assertFalse(preview.error_msg) + for field in preview._MAIL_TEMPLATE_FIELDS: + if field in ['partner_to', 'report_template_ids']: + continue + self.assertEqual(test_template[field], preview[field]) + def test_mail_template_preview_force_lang(self): test_record = self.env['mail.test.lang'].browse(self.test_record.ids) test_record.write({ From 5c146ec219bf57788495c4989d531a961fd5a391 Mon Sep 17 00:00:00 2001 From: Renaud Thiry Date: Tue, 7 Mar 2023 21:02:18 +0000 Subject: [PATCH 2/4] [FIX] mail: compute resource_ref Before this, the resource_ref field is computed once in the default_get. This means if the cache is invalidated, its value is unrecoverable. Notably, the cache is invalidated when an attachment is generated (triggering a commit). So calling _generate_template could invalidate resource_ref, which we use as a key to retrieve its result. This results in a traceback for the user when trying to preview a template where the first record hasn't yet generated its attachment. As an example, trying to preview `Sales: Send Quotation` on fresh installs. task-3162320 X-original-commit: b4bd93c9b61736a7f41d70afe5db977a6e4b35e0 Part-of: odoo/odoo#118710 --- addons/mail/wizard/mail_template_preview.py | 27 ++++++++++--------- .../tests/test_mail_template_preview.py | 25 +++++++++++++++++ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/addons/mail/wizard/mail_template_preview.py b/addons/mail/wizard/mail_template_preview.py index aea4beddcc3..0f9708aa554 100644 --- a/addons/mail/wizard/mail_template_preview.py +++ b/addons/mail/wizard/mail_template_preview.py @@ -28,21 +28,16 @@ class MailTemplatePreview(models.TransientModel): def _selection_languages(self): return self.env['res.lang'].get_installed() - @api.model - def default_get(self, fields): - result = super(MailTemplatePreview, self).default_get(fields) - if not result.get('mail_template_id') or 'resource_ref' not in fields: - return result - mail_template = self.env['mail.template'].browse(result['mail_template_id']).sudo() - model = mail_template.model - res = self.env[model].search([], limit=1) - if res: - result['resource_ref'] = '%s,%s' % (model, res.id) - return result mail_template_id = fields.Many2one('mail.template', string='Related Mail Template', required=True) model_id = fields.Many2one('ir.model', string='Targeted model', related="mail_template_id.model_id") - resource_ref = fields.Reference(string='Record', selection='_selection_target_model') + resource_ref = fields.Reference( + string='Record', + compute='_compute_resource_ref', + compute_sudo=False, readonly=False, + selection='_selection_target_model', + store=True + ) lang = fields.Selection(_selection_languages, string='Template Preview Language') no_record = fields.Boolean('No Record', compute='_compute_no_record') error_msg = fields.Char('Error Message', compute='_compute_mail_template_fields') @@ -88,6 +83,14 @@ class MailTemplatePreview(models.TransientModel): error_msg = user_error.args[0] self.error_msg = error_msg + @api.depends('mail_template_id') + def _compute_resource_ref(self): + for preview in self: + mail_template = preview.mail_template_id.sudo() + model = mail_template.model + res = self.env[model].search([], limit=1) + preview.resource_ref = f'{model},{res.id}' if res else False + def _set_mail_attributes(self, values=None): for field in self._MAIL_TEMPLATE_FIELDS: if field in ('partner_to', 'report_template_ids'): diff --git a/addons/test_mail/tests/test_mail_template_preview.py b/addons/test_mail/tests/test_mail_template_preview.py index df223d6fd97..9bac5987e89 100644 --- a/addons/test_mail/tests/test_mail_template_preview.py +++ b/addons/test_mail/tests/test_mail_template_preview.py @@ -34,6 +34,31 @@ class TestMailTemplateTools(TestMailTemplateCommon): continue self.assertEqual(test_template[field], preview[field]) + def test_mail_template_preview_dynamic_attachment(self): + """Check behaviour with templates that use reports.""" + test_record = self.env['mail.test.lang'].browse(self.test_record.ids) + test_report = self.env['ir.actions.report'].sudo().create({ + 'name': 'Test Report', + 'model': test_record._name, + 'print_report_name': "'TestReport for %s' % object.name", + 'report_type': 'qweb-pdf', + 'report_name': 'test_mail.mail_test_ticket_test_template', + }) + self.test_template.write({ + 'report_template_ids': test_report.ids, + 'attachment_ids': False, + }) + + preview = self.env['mail.template.preview'].with_context({ + 'force_report_rendering': False, # this also invalidates the test records... + }).create({ + 'mail_template_id': self.test_template.id, + 'resource_ref': test_record, + }) + + self.assertEqual(preview.body_html, f'

EnglishBody for {test_record.name}

') + self.assertFalse(preview.attachment_ids, 'Reports should not be listed in attachments') + def test_mail_template_preview_force_lang(self): test_record = self.env['mail.test.lang'].browse(self.test_record.ids) test_record.write({ From f7e5fd7aabeb0e2ff89af47073dd7426dbd760eb Mon Sep 17 00:00:00 2001 From: Renaud Thiry Date: Tue, 7 Mar 2023 21:18:16 +0000 Subject: [PATCH 3/4] [FIX] base: use html method from lxml Prior to this change the convert tool did could mangle the value of html fields when importing static data. This is because html does not support 'self-closing' except for HTML5 where it is allowed on void elements (such as img). Browsers will assume that they are opening tags, left unclosed. becomes They will then try to repair them with variying degrees of success. Example where it fails: `` should become `` but it becomes `` i.e. instead of closing the 'self-closing' tag immediately, it puts everything inside a single t node More concretely: ``` ``` becomes ``` ``` which is invalid ------------------------- The fix is simply to tell lxml that we want to print the xml nodes as HTML nodes. This will make sure the output is compliant with the standard and keep the semantic clear for the browser. The issue does not appear before 16.2, as jquery used to fix it for us until an update here 9c41ee5091ac06ac3ca71aeac607195c70061e4a task-3162320 X-original-commit: 8ff2e1018264972107f19755ecda352d78dfa829 Part-of: odoo/odoo#118710 --- .../addons/test_convert/tests/test_convert.py | 22 +++++++++++++++++-- odoo/tools/convert.py | 2 +- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/odoo/addons/test_convert/tests/test_convert.py b/odoo/addons/test_convert/tests/test_convert.py index 5a2a34af2a9..549b19acd69 100644 --- a/odoo/addons/test_convert/tests/test_convert.py +++ b/odoo/addons/test_convert/tests/test_convert.py @@ -265,6 +265,24 @@ class TestEvalXML(common.TransactionCase): def test_xml(self): pass - @unittest.skip("not tested") def test_html(self): - pass + self.assertEqual( + self.eval_xml(Field(ET.fromstring( + """ + + + + + + + """), type="html")), + """ + + + + + + + """, + "Evaluating an HTML field should give empty nodes instead of self-closing tags" + ) diff --git a/odoo/tools/convert.py b/odoo/tools/convert.py index 36b70b54b4a..0065cb3d612 100644 --- a/odoo/tools/convert.py +++ b/odoo/tools/convert.py @@ -142,7 +142,7 @@ def _eval_xml(self, node, env): return '\n'\ +_process("".join(etree.tostring(n, encoding='unicode') for n in node)) if t == 'html': - return _process("".join(etree.tostring(n, encoding='unicode') for n in node)) + return _process("".join(etree.tostring(n, method='html', encoding='unicode') for n in node)) data = node.text if node.get('file'): From 3fa9aa2c05e676a1f65e9f4c705d827b3bb592d3 Mon Sep 17 00:00:00 2001 From: Renaud Thiry Date: Mon, 13 Mar 2023 15:10:35 +0000 Subject: [PATCH 4/4] [FIX] mail: exclude abstract models from templates Templates could previously be created for abstract models. The methods are not written with that in mind and most useful ones will raise an exception when calling them on that template. task-3162320 X-original-commit: 0d4b473ee54d2376f4f71efd864aa0e16822d4ba Part-of: odoo/odoo#118710 --- addons/mail/models/mail_template.py | 12 +++++++++++- addons/mail/tests/test_mail_template.py | 22 +++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/addons/mail/models/mail_template.py b/addons/mail/models/mail_template.py index a72cad177db..3de352dad0b 100644 --- a/addons/mail/models/mail_template.py +++ b/addons/mail/models/mail_template.py @@ -6,7 +6,7 @@ import itertools import logging from odoo import _, api, fields, models, tools, Command -from odoo.exceptions import UserError +from odoo.exceptions import ValidationError, UserError from odoo.tools import is_html_empty from odoo.tools.safe_eval import safe_eval, time @@ -146,12 +146,22 @@ class MailTemplate(models.Model): record.attachment_ids.write({'res_model': record._name, 'res_id': record.id}) return self + def _check_abstract_models(self, vals_list): + model_names = self.sudo().env['ir.model'].browse(filter(None, ( + vals.get('model_id') for vals in vals_list + ))).mapped('model') + for model in model_names: + if self.env[model]._abstract: + raise ValidationError(_('You may not define a template on an abstract model: %s', model)) + @api.model_create_multi def create(self, vals_list): + self._check_abstract_models(vals_list) return super().create(vals_list)\ ._fix_attachment_ownership() def write(self, vals): + self._check_abstract_models([vals]) super().write(vals) self._fix_attachment_ownership() return True diff --git a/addons/mail/tests/test_mail_template.py b/addons/mail/tests/test_mail_template.py index 4d26f315b80..e0d4f9133cf 100644 --- a/addons/mail/tests/test_mail_template.py +++ b/addons/mail/tests/test_mail_template.py @@ -3,7 +3,7 @@ from markupsafe import Markup from odoo.addons.mail.tests.common import MailCommon -from odoo.exceptions import AccessError, UserError +from odoo.exceptions import AccessError, ValidationError, UserError from odoo.modules.module import get_module_resource from odoo.tests import Form, tagged, users from odoo.tools import convert_file @@ -50,6 +50,26 @@ class TestMailTemplate(MailCommon): self.assertEqual(values[self.partner_employee.id]['subject'], '6', 'We must trust mail template values') self.assertIn('13', values[self.partner_employee.id]['body_html'], 'We must trust mail template values') + @users('admin') + def test_mail_template_abstract_model(self): + """Check abstract models cannot be set on templates.""" + # create + with self.assertRaises(ValidationError), self.cr.savepoint(): + self.env['mail.template'].create({ + 'name': 'Test abstract template', + 'model_id': self.env['ir.model']._get('mail.thread').id, # abstract model + }) + # write + template = self.env['mail.template'].create({ + 'name': 'Test abstract template', + 'model_id': self.env['ir.model']._get('res.partner').id, + }) + with self.assertRaises(ValidationError), self.cr.savepoint(): + template.write({ + 'name': 'Test abstract template', + 'model_id': self.env['ir.model']._get('mail.thread').id, + }) + def test_mail_template_acl(self): # Sanity check self.assertTrue(self.user_admin.has_group('mail.group_mail_template_editor'))