[FIX] util/snippets: generate HTML instead of XML for mailing.mailing #58

bso-odoo · 2024-03-20T12:35:42Z

The convert_html_content tool writes XML values if a change happened. This is a problem for the fields of mailing.mailing because they strictly expect HTML content.
E.g. if <img src="..."> becomes <img src="..."/>, the wysiwyg editor displays an empty template instead of the actual content.

This commit avoids this by making the writes to the fields of mailing.mailing formatted in HTML.

task-3797854

The `convert_html_content` tool writes XML values if a change happened. This is a problem for the fields of `mailing.mailing` because they strictly expect HTML content. E.g. if `<img src="...">` becomes `<img src="..."/>`, the wysiwyg editor displays an empty template instead of the actual content. This commit avoids this by making the writes to the fields of `mailing.mailing` formatted in HTML. task-3797854

robodoo · 2024-03-20T12:36:05Z

Pull request status dashboard.

bso-odoo · 2024-03-20T12:36:40Z

Related to https://github.com/odoo/upgrade/pull/5808

qsm-odoo · 2024-05-07T16:19:01Z

src/util/snippets.py

@@ -322,6 +329,8 @@ def convert_html_content(
    :param dict kwargs: extra keyword arguments to pass to :func:`convert_html_column`
    """

+    if "set_format" in dir(converter_callback):


Do we need this if ?

Since upgrade-util is an open repo, it might be used with a converter_callback that is not the expected HTMLConverter from this class.

You could use isinstance(converter_callback, HTMLConverter) for this. Although I'd just suggest you add an assert isinstance(converter_callback, HTMLConverter), and modify the docstring to be explicit we accept here only instances of HTMLConverter.

:param :class:`HTMLConverter` converter_callback: conversion function that converts the HTML ...

qsm-odoo · 2024-05-07T16:30:03Z

src/util/snippets.py

+            # mass_mailing requires HTML
+            converter = "html" if table == "mailing_mailing" else "xml"
+            converter_callback.set_format(converter)


Should not this be a thing per column instead of per table ?

Can't this be somehow guessed by checking the column definition ? Somewhere in our codebase, there must already be that kind of check no? When you save that column or when you try to translate it, don't we have any code guessing that?

E.g. if <img src="..."> becomes <img src="..."/>, the wysiwyg
editor displays an empty template instead of the actual content.

Actually, why is that a problem? I don't understand how adding the auto-close marker would make the HTML field invalid.

Still trying to find out "why" this is a problem, but it is a display problem: if an <img src="..."/> is in the mail body_arch and body_html, the backend is fine with it, it gets sent as is to the client as the result of web_read, but the display shows an empty iframe - and an error occurs if you start dragging a block.

Maybe it's related to what the mailing editor does. I think they create an empty iframe then fill it with content. Maybe that content goes through the jQuery parser first or something like that? 🤷

It could be also that it's invalid XML, still valid HTML, and some intermediate parser is too strict... I have no idea what's done in the frontend -- XHTML maybe? :/

The closing <img/> tag makes the browser put the HTML content inside <style id="design-element"> instead of after - thus not actually displaying it. Happens with: iframeTarget.innerHTML = value;.
E.g.:

el.innerHTML='<div class="o_layout oe_unremovable oe_unmovable bg-200 o_empty_theme" data-name="Mailing"><style id="design-element"/><div class="container o_mail_wrapper o_mail_regular oe_unremovable"><div class="row"><div class="col o_mail_no_options o_mail_wrapper_td bg-white oe_structure o_editable theme_selection_done"><div class="s_text_image o_mail_snippet_general pt32 pb32" data-snippet="s_image_text" data-name="Image - Text">\n <div class="container">\n <div class="row align-items-center">\n <div class="col-lg-6 o_cc px-0">\n <img src="/web/image/240-9c7d1e2e/s_default_image_image_text.png" class="img w-100" data-original-id="214" data-original-src="/mass_mailing/static/src/img/theme_default/s_default_image_image_text.jpg" data-mimetype="image/png" data-shape="web_editor/geometric_round/geo_round_circle" data-file-name="s_default_image_image_text.png" data-shape-colors=";;;;"/>\n </div>\n <div class="col-lg-6 o_cc pt16 pb16">\n <h3 class="o_default_snippet_text">Omnichannel sales</h3>\n <p style="text-align: justify;" class="o_default_snippet_text">Get your inside sales (CRM) fully integrated with online sales (eCommerce), in-store sales (Point of Sale) and marketplaces like eBay and Amazon.</p>\n <div style="text-align: left;">\n <a href="#" class="btn btn-link o_default_snippet_text">Read More</a>\n </div>\n </div>\n </div>\n </div>\n </div></div></div></div></div>';

Can't this be somehow guessed by checking the column definition ? Somewhere in our codebase, there must already be that kind of check no? When you save that column or when you try to translate it, don't we have any code guessing that?

Unfortunately there is nothing inside the ir_model_fields table that gives this kind of information. And in upgrade we cannot even look at the value of the translate attribute of the Html fields - and if we could, it would not be complete.
Given that we did not notice other issues with XML-mode until now, I think we can only rely on a white list.

We could also go for a more general approach: XML only for arch_db, and use HTML for all Html fields. But doesn't that sound risky ?

The closing <img/> tag makes the browser put the HTML content inside <style id="design-element"> instead of after - thus not actually displaying it. Happens with: iframeTarget.innerHTML = value;. E.g.:

el.innerHTML='<div class="o_layout oe_unremovable oe_unmovable bg-200 o_empty_theme" data-name="Mailing"><style id="design-element"/><div class="container o_mail_wrapper o_mail_regular oe_unremovable"><div class="row"><div class="col o_mail_no_options o_mail_wrapper_td bg-white oe_structure o_editable theme_selection_done"><div class="s_text_image o_mail_snippet_general pt32 pb32" data-snippet="s_image_text" data-name="Image - Text">\n <div class="container">\n <div class="row align-items-center">\n <div class="col-lg-6 o_cc px-0">\n <img src="/web/image/240-9c7d1e2e/s_default_image_image_text.png" class="img w-100" data-original-id="214" data-original-src="/mass_mailing/static/src/img/theme_default/s_default_image_image_text.jpg" data-mimetype="image/png" data-shape="web_editor/geometric_round/geo_round_circle" data-file-name="s_default_image_image_text.png" data-shape-colors=";;;;"/>\n </div>\n <div class="col-lg-6 o_cc pt16 pb16">\n <h3 class="o_default_snippet_text">Omnichannel sales</h3>\n <p style="text-align: justify;" class="o_default_snippet_text">Get your inside sales (CRM) fully integrated with online sales (eCommerce), in-store sales (Point of Sale) and marketplaces like eBay and Amazon.</p>\n <div style="text-align: left;">\n <a href="#" class="btn btn-link o_default_snippet_text">Read More</a>\n </div>\n </div>\n </div>\n </div>\n </div></div></div></div></div>';

@bso-odoo Is not that line the problem? .innerHTML = "<style id="stuff"/>" is not something valid to do 😱 Style is not an auto-closing tag, you cannot code that the jQuery way 😬 Surprised it's not already full of bugs without this migration issue 😮

I think this should be fixed then we can continue just parsing everything as XML? (Unless of course there is more I don't know about between XML/HTML parsing)

@bso-odoo In fact, it of course would still be great to also use the right formatter (but still need a fix for the innerHTML code). I see we always parse as HTML then re-transform as XML, weird indeed. Can't there be problem in XML too because of that?

Random idea: what about guessing which seems to be the best by:

parse original content with html, reformat it again as an HTML string

parse original content with etree, reformat it again as an XML string

check which one is the closest as the original string => this is the one to use to upgrade the content.

Not sure though, to investigate. But my guess is that:

when doing that, if you do not manage to guess: using either XML or HTML would be fine (but use XML, IMO less chance to break properly coded functional code)

when doing that, if you do see one that change the original content more, then it's very probably not the one to use: if it is a mistake, then the original Odoo code that saved that content as it was is properly flawed in the first place.

To see how doing that would impact perf but my guess is that it would be negligible?

qsm-odoo requested changes May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] util/snippets: generate HTML instead of XML for mailing.mailing #58

[FIX] util/snippets: generate HTML instead of XML for mailing.mailing #58

bso-odoo commented Mar 20, 2024

robodoo commented Mar 20, 2024

bso-odoo commented Mar 20, 2024

qsm-odoo May 7, 2024

bso-odoo May 8, 2024

aj-fuentes May 8, 2024

qsm-odoo May 7, 2024

bso-odoo May 8, 2024

qsm-odoo May 8, 2024

aj-fuentes May 8, 2024

bso-odoo May 8, 2024 •

edited

bso-odoo May 8, 2024

qsm-odoo May 10, 2024

qsm-odoo May 10, 2024

[FIX] util/snippets: generate HTML instead of XML for mailing.mailing #58

Are you sure you want to change the base?

[FIX] util/snippets: generate HTML instead of XML for mailing.mailing #58

Conversation

bso-odoo commented Mar 20, 2024

robodoo commented Mar 20, 2024

bso-odoo commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bso-odoo May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bso-odoo May 8, 2024 •

edited