Fix issue with dynamic content showing '{dynamiccontent = "undefined"}' after re-opening email

[irfanhanfi]

Q	A
Bug fix? (use the a.b branch)	[ 5.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 #...

This issue fixes the issue mentioned here - #13431 (comment)

Description:

The code this.editor.setComponents(components); triggers some events internally, resulting in the removal of dynamic contents and displaying undefined.

Steps to reproduce -

1. Create an Email - Channel >> Emails >> Create Email
2. Add a Dynamic Content block and set some values
3. Close the editor and reopen it using the Builder button
4. Issue - Dynamic Content is not editable and the popup does not open
5. Once you close and open it again, it shows "undefined"

Steps to test this PR:

1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

[LordRembo]

There is a 'remove' event that fires and removes the ID's from storage, when reopening a mail with a dynamic content block. The problem is in the repo for the preset for grapesjs which has this similar ticket that I think has the same root cause.

My proposed PRs to update GrapesJS, and the dependent preset, also fixes that problem.

[LordRembo]

I made a PR to resolve this issue via the grapesjs mautic preset, but it would be helpful if you could test that solution as part of my GrapesJSBuilder update PR, so we can merge the big update at once.

[LordRembo]

I checked your PR, there's no conflict between both our fixes, just a check on another level. So I'm gonna leave it alone.

[LordRembo]

The code itself makes sense: the previous setComponents overrides the Components instead, which is less clean than removing them and adding them in.
I guess this avoids some kind of caching issue which caused the stored block id to be targetted? I'd be interested to hear what you think went wrong with the 'remove' event (I think there's a bug in how the event is handled/the data is collected by GrapesJS itself), but in any case, this works.

[irfan-synerzip]

@LordRembo I don't believe there is anything wrong with the remove event. When we call setComponents, it triggers the remove event, which then removes the DynamicContent. My fix does not call those events when we set the components.

Although I haven't tested this, I believe that the approach mentioned in the linked pull request will also work - mautic/grapesjs-preset-mautic#52

If your GrapesJS upgrade works, then we can also proceed with that.

[LordRembo]

We'll see which gets merged first :D I think this one has the best chance because the change is small and easy to test. Hopefully it gets picked up and approved on Open source Friday

[irfanhanfi]

One reason that we need this PR is that in the previous changes components has been removed from grapesjs.init. There are some functionalities for which we need component data to make some block initialization.

[RCheesley]

Hi folks, this would need to be based on the 5.x branch for it to be merged in 5.1 or later, as we won't have any more releases on the 5.0 branch. Can you rebase it @irfanhanfi ?

[irfanhanfi]

Hi folks, this would need to be based on the 5.x branch for it to be merged in 5.1 or later, as we won't have any more releases on the 5.0 branch. Can you rebase it @irfanhanfi ?

I have rebased it.

[RCheesley]

Thanks @irfanhanfi - looks like a conflict to fix then we should be able to test.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.39%. Comparing base (714ba98) to head (d07cbc6).

Additional details and impacted files

@@            Coverage Diff            @@
##                5.x   #13612   +/-   ##
=========================================
  Coverage     61.39%   61.39%           
  Complexity    34034    34034           
=========================================
  Files          2240     2240           
  Lines        101714   101714           
=========================================
+ Hits          62443    62444    +1     
+ Misses        39271    39270    -1     

see 1 file with indirect coverage changes

[LordRembo]

All looking good now