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 newsletter html containing style tag content #6876

Merged
merged 3 commits into from
Dec 1, 2020
Merged

Fix newsletter html containing style tag content #6876

merged 3 commits into from
Dec 1, 2020

Conversation

ahukkanen
Copy link
Contributor

@ahukkanen ahukkanen commented Nov 17, 2020 •
edited
Loading

🎩 What? Why?

Decidim is using Premailer to convert the HTML to plain text. A part of the message is displayed in the beginning of the HTML emails as well.

In #5887 newsletter templates were added which apparently contain inline <style> tags within the HTML content of the message:

decidim/decidim-core/app/cells/decidim/newsletter_templates/image_text_cta/show.erb

Lines 2 to 6 in 98a2709

<style>
table.button table td {
background: <%= organization_primary_color %> !important
}
</style>

The problem is that premailer does not strip out the style tags by default and it leaves the content inside the <style> tag to the final plain text email:
https://github.com/premailer/premailer/blob/ae21ddab9302cf3e3b9dad9c3cd1115487f06acb/lib/premailer/html_to_plain_text.rb#L16-L129

This causes the beginning of the emails to contain the contents of the inline <style> tags within the emails (see the screenshot).

This PR fixes the issue by customizing the Premailer adapter for stripping out the <style> tags before passing the HTML content to the Premailer's method.

📌 Related Issues

Testing

  • Create a Decidim instance with the default seed data
  • Make sure you have defined the organization primary color
  • Send a newsletter email
  • See the beginning of the email text

📋 Checklist

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

An image how this shows up in the email:
Style tag contents at the beginning of the emails

@Leusev Leusev self-requested a review November 25, 2020 07:55
@Leusev Leusev self-assigned this Nov 25, 2020
@Leusev Leusev added in-review module: core type: bug Issues that describe a bug labels Nov 25, 2020
@ahukkanen
Copy link
Contributor Author

Not particularly related to fixing this issue but just to mention also that this does not have any effect right now for the email:

decidim/decidim-core/app/cells/decidim/newsletter_templates/image_text_cta/show.erb

Lines 2 to 6 in 98a2709

<style>
table.button table td {
background: <%= organization_primary_color %> !important
}
</style>

It only has an effect when viewing the newsletter in browser.

In the email it would have exactly the same effect if it wasn't there because the elements will get inline styles based on the style tags in the <head> of the page.

It was introduced by #5887.

@Leusev
Copy link
Contributor

Leusev commented Nov 27, 2020

Really interesting fix here @ivan-mr @tramuntanal . This was what we were talking about in last daily meeting about that weird html code in emails.. We'll take note to make corresponding backports if so !

ivan-mr
ivan-mr approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@ivan-mr ivan-mr left a comment

Choose a reason for hiding this comment

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

Great job @ahukkanen ! thanks.

@ivan-mr ivan-mr merged commit dcd33fe into decidim:develop Dec 1, 2020
@ahukkanen ahukkanen deleted the fix/newsletter-html-containing-style-tags branch December 1, 2020 15:23
This was referenced Dec 1, 2020
Merged
Merged
entantoencuanto added a commit that referenced this pull request Dec 2, 2020
@entantoencuanto
Merge branch 'develop' into feature/process_groups_improvements-meeti…
844f4cd 
…ngs_content_block

* develop: (22 commits)
  Fix email CTA alignment on Outlook and Windows Mail (#6895)
  Fix mailer meeting registration invitation using path instead of URL (#6965)
  Fix the data portability exporter when zip is not in the gemfile (#6969)
  Convert technical docs to Antora (#6526)
  New Crowdin updates (#6957)
  Bugfix - moderated meetings are displayed in the meetings index page  (#6927)
  Add HTML Content Blocks in Process Groups  (#6823)
  Prevent error in view due to optional html not showing (#6942)
  Improve layout for standalone T&C page (#6944)
  chore: move rubocop ruby config to own file (#6952)
  Fix some strings (#6958)
  Fix newsletter html containing style tag content (#6876)
  New Crowdin updates (#6945)
  New Crowdin updates (#6926)
  Localize a string in conference speaker (#6866)
  Fix broken dashboard action logs under certain conditions (#6857)
  Fix traceability logs with invalid record (#6879)
  Allow user to drag address on proposal map (#6291)
  New Crowdin updates (#6898)
  Update release notes documentation (#6809)
  ...
entantoencuanto added a commit that referenced this pull request Dec 2, 2020
@entantoencuanto
Merge branch 'develop' into feature/process_groups_improvements-stats…
16b6a73 
…_content_block

* develop: (22 commits)
  Fix email CTA alignment on Outlook and Windows Mail (#6895)
  Fix mailer meeting registration invitation using path instead of URL (#6965)
  Fix the data portability exporter when zip is not in the gemfile (#6969)
  Convert technical docs to Antora (#6526)
  New Crowdin updates (#6957)
  Bugfix - moderated meetings are displayed in the meetings index page  (#6927)
  Add HTML Content Blocks in Process Groups  (#6823)
  Prevent error in view due to optional html not showing (#6942)
  Improve layout for standalone T&C page (#6944)
  chore: move rubocop ruby config to own file (#6952)
  Fix some strings (#6958)
  Fix newsletter html containing style tag content (#6876)
  New Crowdin updates (#6945)
  New Crowdin updates (#6926)
  Localize a string in conference speaker (#6866)
  Fix broken dashboard action logs under certain conditions (#6857)
  Fix traceability logs with invalid record (#6879)
  Allow user to drag address on proposal map (#6291)
  New Crowdin updates (#6898)
  Update release notes documentation (#6809)
  ...
entantoencuanto added a commit that referenced this pull request Dec 2, 2020
@entantoencuanto
Merge branch 'develop' into feature/process_groups_improvements-parti…
6950976 
…cipatory_processes_content_block

* develop: (22 commits)
  Fix email CTA alignment on Outlook and Windows Mail (#6895)
  Fix mailer meeting registration invitation using path instead of URL (#6965)
  Fix the data portability exporter when zip is not in the gemfile (#6969)
  Convert technical docs to Antora (#6526)
  New Crowdin updates (#6957)
  Bugfix - moderated meetings are displayed in the meetings index page  (#6927)
  Add HTML Content Blocks in Process Groups  (#6823)
  Prevent error in view due to optional html not showing (#6942)
  Improve layout for standalone T&C page (#6944)
  chore: move rubocop ruby config to own file (#6952)
  Fix some strings (#6958)
  Fix newsletter html containing style tag content (#6876)
  New Crowdin updates (#6945)
  New Crowdin updates (#6926)
  Localize a string in conference speaker (#6866)
  Fix broken dashboard action logs under certain conditions (#6857)
  Fix traceability logs with invalid record (#6879)
  Allow user to drag address on proposal map (#6291)
  New Crowdin updates (#6898)
  Update release notes documentation (#6809)
  ...
entantoencuanto added a commit that referenced this pull request Dec 2, 2020
@entantoencuanto
Merge branch 'develop' into feature/process_groups_improvements-edit_…
a401ade 
…link

* develop: (22 commits)
  Fix email CTA alignment on Outlook and Windows Mail (#6895)
  Fix mailer meeting registration invitation using path instead of URL (#6965)
  Fix the data portability exporter when zip is not in the gemfile (#6969)
  Convert technical docs to Antora (#6526)
  New Crowdin updates (#6957)
  Bugfix - moderated meetings are displayed in the meetings index page  (#6927)
  Add HTML Content Blocks in Process Groups  (#6823)
  Prevent error in view due to optional html not showing (#6942)
  Improve layout for standalone T&C page (#6944)
  chore: move rubocop ruby config to own file (#6952)
  Fix some strings (#6958)
  Fix newsletter html containing style tag content (#6876)
  New Crowdin updates (#6945)
  New Crowdin updates (#6926)
  Localize a string in conference speaker (#6866)
  Fix broken dashboard action logs under certain conditions (#6857)
  Fix traceability logs with invalid record (#6879)
  Allow user to drag address on proposal map (#6291)
  New Crowdin updates (#6898)
  Update release notes documentation (#6809)
  ...
entantoencuanto added a commit that referenced this pull request Dec 2, 2020
@entantoencuanto
Merge branch 'develop' into feature/process_groups_improvements-highl…
6c9ebfe 
…ighted_groups

* develop: (22 commits)
  Fix email CTA alignment on Outlook and Windows Mail (#6895)
  Fix mailer meeting registration invitation using path instead of URL (#6965)
  Fix the data portability exporter when zip is not in the gemfile (#6969)
  Convert technical docs to Antora (#6526)
  New Crowdin updates (#6957)
  Bugfix - moderated meetings are displayed in the meetings index page  (#6927)
  Add HTML Content Blocks in Process Groups  (#6823)
  Prevent error in view due to optional html not showing (#6942)
  Improve layout for standalone T&C page (#6944)
  chore: move rubocop ruby config to own file (#6952)
  Fix some strings (#6958)
  Fix newsletter html containing style tag content (#6876)
  New Crowdin updates (#6945)
  New Crowdin updates (#6926)
  Localize a string in conference speaker (#6866)
  Fix broken dashboard action logs under certain conditions (#6857)
  Fix traceability logs with invalid record (#6879)
  Allow user to drag address on proposal map (#6291)
  New Crowdin updates (#6898)
  Update release notes documentation (#6809)
  ...
entantoencuanto added a commit that referenced this pull request Dec 2, 2020
@entantoencuanto
Merge branch 'develop' into feature/process_groups_improvements-group…
f0066f7 
…s_and_processes_block

* develop: (22 commits)
  Fix email CTA alignment on Outlook and Windows Mail (#6895)
  Fix mailer meeting registration invitation using path instead of URL (#6965)
  Fix the data portability exporter when zip is not in the gemfile (#6969)
  Convert technical docs to Antora (#6526)
  New Crowdin updates (#6957)
  Bugfix - moderated meetings are displayed in the meetings index page  (#6927)
  Add HTML Content Blocks in Process Groups  (#6823)
  Prevent error in view due to optional html not showing (#6942)
  Improve layout for standalone T&C page (#6944)
  chore: move rubocop ruby config to own file (#6952)
  Fix some strings (#6958)
  Fix newsletter html containing style tag content (#6876)
  New Crowdin updates (#6945)
  New Crowdin updates (#6926)
  Localize a string in conference speaker (#6866)
  Fix broken dashboard action logs under certain conditions (#6857)
  Fix traceability logs with invalid record (#6879)
  Allow user to drag address on proposal map (#6291)
  New Crowdin updates (#6898)
  Update release notes documentation (#6809)
  ...
@ivan-mr ivan-mr mentioned this pull request Dec 14, 2020
Merged
12 tasks
@paulinebessoles paulinebessoles mentioned this pull request Feb 23, 2021
Closed
@mrcasals mrcasals added type: fix PRs that implement a fix for a bug and removed type: bug Issues that describe a bug labels Feb 26, 2021
@andreslucena andreslucena mentioned this pull request Jan 11, 2022
Closed
@ahukkanen ahukkanen mentioned this pull request Jul 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivan-mr ivan-mr ivan-mr approved these changes

@Leusev Leusev Awaiting requested review from Leusev

Assignees

@Leusev Leusev

Labels
in-review module: core type: fix PRs that implement a fix for a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ahukkanen @Leusev @ivan-mr @mrcasals