fix(newsletters): require the send list to resolve against the connected ESP - #979
Open
jason10lee wants to merge 3 commits into
Open
fix(newsletters): require the send list to resolve against the connected ESP#979jason10lee wants to merge 3 commits into
jason10lee wants to merge 3 commits into
Conversation
…ted ESP Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Newsletters editor send guard to prevent sending when a newsletter’s saved send_list_id no longer resolves against the currently connected ESP (e.g., after switching providers), addressing a UX gap where Send could remain enabled for an orphaned audience.
Changes:
- Add
getSettledSendListsto gate list-resolution checks on the ESPretrievelifecycle instead of sidebar-only list fetches. - Extend
validateNewsletterto optionally validate that the saved list id exists in the resolved ESP lists (while avoiding false negatives during loading). - Add unit tests covering the new “settled vs. unsettled lists” behavior and unresolved-list validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugins/newspack-newsletters/src/newsletter-editor/utils.test.js | Adds focused unit coverage for settled list gating and unresolved saved list validation. |
| plugins/newspack-newsletters/src/newsletter-editor/utils.js | Introduces getSettledSendLists and enhances validateNewsletter to require saved list resolution once data is settled. |
| plugins/newspack-newsletters/src/components/send-button/index.js | Wires the send button validation to use settled ESP lists so orphaned lists disable Send reliably (even when the sidebar panel stays collapsed). |
Suppressed comments (1)
plugins/newspack-newsletters/src/newsletter-editor/utils.js:73
- The
sendListsJSDoc implies onlynullrepresents an unsettled fetch, but the implementation treats an empty array as “unknown” as well (it won’t report an unresolved list). Updating the doc to reflect the actual contract will prevent future callers from assuming[]is a settled/authoritative roster.
* @param {?Object[]} sendLists Send lists fetched from the connected ESP,
* or null when that fetch hasn't settled.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
We've addressed the suppressed finding in Copilot's review above by updating the docblock to reflect that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All Submissions:
Changes proposed in this Pull Request:
A newsletter keeps the list it was saved with when a site switches email service providers. The new provider has never heard of that list, so the send confirmation has nothing to show, drops the "You're sending a newsletter to…" line, and leaves Send enabled anyway. The author sees a newsletter that looks ready to go and an audience that does not exist.
Send now requires the saved list to resolve against the connected provider, rather than only to be set. A newsletter with an orphaned list is blocked the same way one with no list at all already was.
The hard part is telling "this list is gone" apart from "the lists have not loaded yet". Get that wrong and Send is disabled on healthy newsletters, which is worse than the bug. The signal the check trusts is the editor's
retrieverequest, because every active provider asks that request for the saved list by id and widens only when the lookup fails. Once it has completed, a saved id still missing from the result did not resolve. That reasoning also survives the cap on how many listsretrievereturns for the autocomplete — ActiveCampaign returns the first ten — because the saved id is requested explicitly rather than hoped for among the first page.The obvious alternative was the store's
hasRetrievedListsflag, which reads exactly like "the lists are loaded". It was the first approach taken here, and it does not work: only the sidebar's own fetch ever sets it, so the guard stayed asleep whenever the Newsletter Campaign panel was collapsed. That is also the case where the sidebar's warning cannot render at all, so it is precisely where the author would otherwise get no signal. Unit tests passed against that version; running it against a live provider is what exposed the gap.Two things are deliberately out of scope. The saved id is not cleared, because every provider's send path reads an explicitly unset sublist id as "send to the whole list", and writing a null to mean "could not resolve" would disarm that guard. And nothing renders the reason on screen: a disabled Send button matches how a missing list already behaves, and the missing explanation is tracked separately in NPPM-3231.
Reference: NPPM-3138
How to test the changes in this Pull Request:
Setup: a site with Newspack Newsletters and an email service provider connected.
wp post meta update <id> send_list_id not-a-real-list.Same site and provider, differing only in the saved list. A valid list keeps Send available:
A list the provider does not have blocks it:
With the panel expanded, the existing sidebar warning and the blocked Send appear together:
Other information:
Verified against a live ActiveCampaign account with a Mailchimp audience id as the saved list, so the orphan is a real cross-provider one rather than a fabricated value. Both directions were checked with the Newsletter Campaign panel expanded and collapsed. The suite is 343 tests; the new cases fail against the pre-change code.
One thing this does not establish: what the provider does if a send is somehow triggered with an unresolvable list. This change removes the route to it from the editor, which is the argument for why it stops mattering, but that argument is untested.
Release risk: Low
newspack-newslettersonly. No PHP, no schema, no cross-plugin contract.