Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/util/snippets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,12 @@ class HTMLConverter:
def __init__(self, callback, selector=None):
self.selector = selector
self.callback = make_pickleable_callback(callback)
self.format = "xml"

def set_format(self, format):
# Using a format string instead of html and etree because self
# needs to be pickleable.
self.format = format

def has_changed(self, els):
if self.selector:
Expand All @@ -206,8 +212,9 @@ def __call__(self, content):
parser=utf8_parser,
)
has_changed = self.has_changed(els)
formatter = etree if self.format == "xml" else html
new_content = (
re.sub(r"(^<wrap>|</wrap>$)", "", etree.tostring(els, encoding="unicode").strip())
re.sub(r"(^<wrap>|</wrap>$)", "", formatter.tostring(els, encoding="unicode").strip())
if has_changed
else content
)
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this if ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
   ...

converter_callback.set_format('xml')
convert_html_columns(
cr,
"ir_ui_view",
Expand All @@ -332,4 +341,8 @@ def convert_html_content(
)

for table, columns in html_fields(cr):
if "set_format" in dir(converter_callback):
# mass_mailing requires HTML
converter = "html" if table == "mailing_mailing" else "xml"
converter_callback.set_format(converter)
Comment on lines +345 to +347
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :/

Copy link
Author

@bso-odoo bso-odoo May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>';

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

convert_html_columns(cr, table, columns, converter_callback, where_column=where_column, **kwargs)