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: avoid Redux usage and related errors in non-newsletter email editors #1688

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Oct 31, 2024

All Submissions:

Changes proposed in this Pull Request:

#1679 introduced errors in non-newsletter CPT email editors. All email editors use the MJML component to convert post block content into MJML and then into email-compliant HTML. Only the newsletter CPT editor registers a Redux store to handle newsletter campaign data, so store methods should only be called while editing a newsletter CPT post while connected to a supported ESP.

This PR makes the MJML component more standalone once again, and only lets it call Redux store methods when it detects a supported ESP connection (which will only happen in the newsletter editor, not other email editors). It also does some light refactoring of this component to avoid a Promise chain in favor of a less verbose and more readable async function.

How to test the changes in this Pull Request:

  1. On trunk, edit a custom Newspack email (such as the custom receipt in Reader Revenue > Emails, or the RAS emails in Engagement > Reader Activation) and a newsletter ad. In both cases, observe an error notice: Uncaught TypeError: Cannot read properties of undefined (reading 'getIsRefreshingHtml')
  2. Check out this branch and refresh both editors and confirm that the error doesn't appear.
  3. Smoke test making edits to the custom email and the newsletter ad and confirm that the changes are persisted.
  4. Smoke test sending a test email from the custom email editor and confirm that the changes are reflected in the sent test email.
  5. Smoke test updating a newsletter post, saving, sending a test email, and confirm that all changes made are reflected in test emails and in synced campaign in the connected ESP.

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?

@dkoo dkoo self-assigned this Oct 31, 2024
@dkoo dkoo requested a review from a team as a code owner October 31, 2024 20:39
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Works well.

I've noticed two little things, not sure if related or fixable in this PR

  1. On custom emails: Changes done while on trunk were not present in the sent email. (I guess this is expected because there was an error there)

  2. On custom emails: If I send a test email before saving, a little popup says the email was saved but the sent newsletter does not include the latest changes

@github-actions github-actions bot added [Status] Approved Ready to merge and removed [Status] Needs Review labels Nov 1, 2024
@dkoo
Copy link
Contributor Author

dkoo commented Nov 1, 2024

  1. On custom emails: Changes done while on trunk were not present in the sent email. (I guess this is expected because there was an error there)

Yes, this PR should fix that issue!

  1. On custom emails: If I send a test email before saving, a little popup says the email was saved but the sent newsletter does not include the latest changes

This is the same race condition that #1679 attempted to fix for newsletter CPT posts. The reason the race condition happens is:

  • You click "test" and it initiates a save request
  • When the save request completes, the MJML editor component triggers a refresh of email HTML via the MJML JS library, and then saves the refreshed HTML to post meta. We need to wait until the save is complete before doing this, otherwise recent changes in the editor may not be available on the server when the updated HTML is written.
  • But also when the save request completes, the custom email editor sends a test request to send the test email, but this is likely to get sent before the refreshed email HTML gets saved to post meta as it can start at the same time as the MJML request

It's relatively easy to fix this race condition for newsletter CPTs, in which the MJML and newsletter editor components can both access the Redux store to check when the MJML request completes (and that's when we send the test email for newsletters).

But because the custom email editor is controlled by JS in the main Newspack plugin, it's more complicated to fix this for custom emails as the main plugin can't access the Redux store registered by Newsletters. I suggest we leave this as-is for now, unless you have any suggestions for fixing the race condition. I thought about adding a setTimeout delay to the test request for custom emails to try and give the MJML request time to complete, but that seems ugly to me.

@dkoo dkoo requested a review from leogermani November 1, 2024 22:35
@dkoo dkoo merged commit d3f1f37 into trunk Nov 5, 2024
8 checks passed
@dkoo dkoo deleted the fix/mjml-errors-other-email-editors branch November 5, 2024 17:49
matticbot pushed a commit that referenced this pull request Nov 6, 2024
# [3.4.0-alpha.1](v3.3.2...v3.4.0-alpha.1) (2024-11-06)

### Bug Fixes

* avoid race condition between post-save sync & test ([#1679](#1679)) ([7bde119](7bde119))
* avoid Redux usage and related errors in non-newsletter email editors ([#1688](#1688)) ([d3f1f37](d3f1f37))
* dont html encode ampersands in subject ([#1686](#1686)) ([f178b23](f178b23))
* **mailchimp:** avoid duplicate audiences in Mailchimp UIs ([#1685](#1685)) ([44c1b12](44c1b12))
* move Preview, Send buttons to match Publish button location ([#1689](#1689)) ([72897f2](72897f2))
* preview & send buttons on WP 6.7 ([49450d3](49450d3))

### Features

* display list remote name on settings page ([#1672](#1672)) ([562d396](562d396))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.4.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Nov 11, 2024
# [3.4.0](v3.3.3...v3.4.0) (2024-11-11)

### Bug Fixes

* avoid double notice components ([#1703](#1703)) ([b8e9130](b8e9130))
* avoid race condition between post-save sync & test ([#1679](#1679)) ([7bde119](7bde119))
* avoid Redux usage and related errors in non-newsletter email editors ([#1688](#1688)) ([d3f1f37](d3f1f37))
* correct button stacking on sent newsletters ([#1695](#1695)) ([4e2688e](4e2688e))
* dont html encode ampersands in subject ([#1686](#1686)) ([f178b23](f178b23))
* **mailchimp:** avoid duplicate audiences in Mailchimp UIs ([#1685](#1685)) ([44c1b12](44c1b12))
* move Preview, Send buttons to match Publish button location ([#1689](#1689)) ([72897f2](72897f2))
* preview & send buttons on WP 6.7 ([49450d3](49450d3))
* remove behavior to hide post title in newsletter editor ([#1701](#1701)) ([8a15cf5](8a15cf5))

### Features

* display list remote name on settings page ([#1672](#1672)) ([562d396](562d396))
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.

3 participants