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: Dynamically fetch message ID in ActiveCampaign test emails #1666

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

claudiulodro
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Note: This is a hotfix PR.

This PR fixes an issue I discovered on some client sites which is also reproducible in my local environment.

When a new temporary campaign is created in ActiveCampaign to send a test email, it uses the message ID saved onto the post. This message is not necessarily associated with the temporary campaign, so the test send will fail.

I'm not sure if the approach I took here is the way to go, or if it would be better to somehow associate the message from the "real" campaign with the temporary campaign, so that this check isn't needed. Let me know what you think.

How to test the changes in this Pull Request:

  1. This part happens naturally for every new newsletter on my local running only the trunk version of Plugin, Blocks, Ads, and Newsletters, but it did not happen in @adekbadek's testing. Start a new newsletter from a template, add all the necessary information, save it as draft, and send a test email.
  2. Before applying this patch you should get a "Sending test campaign failed: Message not sent" error:
Screenshot 2024-10-03 at 2 20 24 PM 4. After applying this patch it should successfully send: Screenshot 2024-10-03 at 2 21 05 PM

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?

@claudiulodro claudiulodro added bug Something isn't working [Status] Needs Review labels Oct 3, 2024
@claudiulodro claudiulodro requested a review from a team as a code owner October 3, 2024 21:32
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.

I was able to replicate the issue after repeatedly sending multiple tests. This fixed the issue for me.

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

dkoo commented Oct 3, 2024

@claudiulodro if this is supposed to be a hotfix, should it be rebased on release?

@claudiulodro claudiulodro changed the base branch from trunk to release October 4, 2024 14:55
@claudiulodro
Copy link
Contributor Author

Thanks for the quick review!

I always remember to branch off release but never remember to merge to release. 😆 Fixed now. I'll merge Monday so it doesn't deploy Friday afternoon.

@claudiulodro claudiulodro merged commit fd31f61 into release Oct 8, 2024
11 checks passed
@claudiulodro claudiulodro deleted the fix/test-messages branch October 8, 2024 16:18
matticbot pushed a commit that referenced this pull request Oct 8, 2024
## [3.1.7](v3.1.6...v3.1.7) (2024-10-08)

### Bug Fixes

* dynamically fetch message ID in tests ([#1666](#1666)) ([fd31f61](fd31f61))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.7 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released [Status] Approved Ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants