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: Dynamic Content in emails (legacy builder)- default content is not saved properly #11887

Merged
merged 2 commits into from
Feb 11, 2023

Conversation

irfanhanfi
Copy link

@irfanhanfi irfanhanfi commented Jan 20, 2023

Q A
Bug fix? (use the a.b branch) [4.x]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #11788

Description:

This PR is to fix the issue mentioned here - #11788

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Jan 20, 2023
@escopecz escopecz mentioned this pull request Jan 20, 2023
16 tasks
@MadlenF
Copy link

MadlenF commented Jan 20, 2023

Works as expected from user-perpective!

@mabumusa1 mabumusa1 self-requested a review January 22, 2023 13:53
@RCheesley RCheesley added bug Issues or PR's relating to bugs code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged builder-legacy Anything related to the legacy email or landing page builders labels Jan 27, 2023
@RCheesley RCheesley mentioned this pull request Jan 27, 2023
21 tasks
kuzmany
kuzmany previously approved these changes Jan 27, 2023
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Works for me 👍
Just app.js should not be part of the PR. It's regenerated during the release process.

@kuzmany kuzmany added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Jan 30, 2023
@RCheesley RCheesley mentioned this pull request Feb 3, 2023
27 tasks
@beschulz
Copy link

beschulz commented Feb 8, 2023

I believe this fix is faulty for html content. We experienced a double escaping issue. Doing the following resolved this:

        if (dynConContent.data('froala.editor') && Mautic.getActiveBuilderName() === 'legacy') {
            dynConContent = dynConContent.froalaEditor('html.get');
        } else {
            dynConContent = dynConContent.text();
        }

(i.e. using .text() instead of .html().

The way I understand the procedure here is that during init (when the initial html is build from the template) dynConContent isn't yet a froalaEditor - but it does have the ".editor" class. Insepcting the DOM revealed that the content from the database was inserted into the DOM as text, not html. I have the suspicion that this is the actual problem for the double escape and that my proposed fix just plasters over the cracks.

@mabumusa1
Copy link
Member

@irfanhanfi thank you for this PR, there is a feedback, I think it is worth considering

@mabumusa1 mabumusa1 added pending-feedback PR's and issues that are awaiting feedback from the author pending-companion-pr Waiting for the author to create a PR on the other branch. and removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Feb 10, 2023
@mabumusa1 mabumusa1 mentioned this pull request Feb 10, 2023
23 tasks
@irfan-synerzip
Copy link
Contributor

@irfanhanfi thank you for this PR, there is a feedback, I think it is worth considering

I have addressed that change.

@beschulz
Copy link

@irfan-synerzip thanks a lot. Although I wonder if this breaks the grapejs stuff. Maybe the whole block should be:

if (Mautic.getActiveBuilderName() === 'legacy') {
    if (dynConContent.data('froala.editor')){
        dynConContent = dynConContent.froalaEditor('html.get');
    } else {
        dynConContent = dynConContent.text();
    }
} else {
    dynConContent = dynConContent.html();
}

@irfanhanfi irfanhanfi force-pushed the issue_11788 branch 2 times, most recently from 1e1c2c9 to b944636 Compare February 10, 2023 14:18
@irfan-synerzip
Copy link
Contributor

@beschulz I did not see the issue with the GrapesJS editor but your approach looks safer. I have made the changes as per the latest comment.

Thank you!

@mabumusa1 mabumusa1 changed the base branch from 4.x to 4.4 February 11, 2023 06:23
@mabumusa1 mabumusa1 changed the base branch from 4.4 to 4.x February 11, 2023 06:23
@mabumusa1 mabumusa1 changed the base branch from 4.x to 4.4 February 11, 2023 06:28
Copy link
Member

@mabumusa1 mabumusa1 left a comment

Choose a reason for hiding this comment

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

I tested this PR and rebased it to 4.4, it looks goo to be merged

@mabumusa1 mabumusa1 added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-feedback PR's and issues that are awaiting feedback from the author pending-companion-pr Waiting for the author to create a PR on the other branch. labels Feb 11, 2023
@mabumusa1 mabumusa1 added this to the 4.4.7 milestone Feb 11, 2023
@mabumusa1
Copy link
Member

@allcontributors please add @irfanhanfi for code

@allcontributors
Copy link
Contributor

@mabumusa1

I've put up a pull request to add @irfanhanfi! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs builder-legacy Anything related to the legacy email or landing page builders cla-signed The PR contributors have signed the contributors agreement code-review-needed PR's that require a code review before merging ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants