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.

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

fix: dont html encode ampersands in subject #1686

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

leogermani
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Fixes a small issue where ampersands are HTML encoded in the Subject.

Not sure if there's a better way to do it. But it's weird because the only character that is being HTML encoded is the ampersand, so loading a library to html_decode everything looked unnecessary.

How to test the changes in this Pull Request:

  1. In trunk edit a newsletters
  2. add a & to the title and see that $amp; is added to the Subject input in the Newsletter sidebar
  3. Checkout this branch and do the same
  4. See that & is preserved as is

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@leogermani leogermani self-assigned this Oct 30, 2024
@leogermani leogermani requested a review from a team as a code owner October 30, 2024 20:05
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 20.38%. Comparing base (44c1b12) to head (858cb56).

Files with missing lines Patch % Lines
...or/class-newspack-newsletters-campaign-monitor.php 0.00% 2 Missing ⚠️
...ign/class-newspack-newsletters-active-campaign.php 0.00% 1 Missing ⚠️
...ct/class-newspack-newsletters-constant-contact.php 0.00% 1 Missing ⚠️
...mailchimp/class-newspack-newsletters-mailchimp.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk    #1686   +/-   ##
=========================================
  Coverage     20.38%   20.38%           
  Complexity     2664     2664           
=========================================
  Files            48       48           
  Lines         10619    10619           
=========================================
  Hits           2165     2165           
  Misses         8454     8454           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

@leogermani this didn't work for me for some reason—the ampersands were still coming in as & in the the subject text input when editing the post title field, and when synced to the ESP the subject still contained &. I think WP converts certain characters into HTML entities when the strings are saved into the DB.

Here's what I had to do to fix the issue: #1687

This doesn't try to save the title with unescaped characters, it just displays the unescaped string in the sidebar field, and then decodes the HTML entities in the sync payload that gets sent to the ESP. Let me know what you think!

@leogermani
Copy link
Contributor Author

@dkoo Your PR looks much more robust! Feel free to take it over!

fix: decode HTML entities in both editor JS and sync payloads
@dkoo
Copy link
Contributor

dkoo commented Oct 31, 2024

@leogermani I've merged #1678 into this PR. Paging @Automattic/newspack-product for another review!

@dkoo dkoo requested a review from a team October 31, 2024 20:48
@dkoo dkoo self-assigned this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants