Skip to content

Refactor screen to fix integrations newsletter setup blocker#258

Open
claudiulodro wants to merge 1 commit into
mainfrom
fix/newsletter-setup
Open

Refactor screen to fix integrations newsletter setup blocker#258
claudiulodro wants to merge 1 commit into
mainfrom
fix/newsletter-setup

Conversation

@claudiulodro

@claudiulodro claudiulodro commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

On a fresh Newspack install, it seems impossible to successfully 'Connect' the Newsletter integration in Integrations. Here's a full explanation, but tl;dr the integration says 'Connect' until you select a master list, but the screen to select a master list is not reachable while in the 'Connect' state:

The label logic (settings-section.js:81 (plugins/newspack-plugin/src/wizards/audience/views/integrations/settings-section.js#L81)): the button says "Connect" when is_set_up is false, "Enable" when it's true (and once enabled, the card flips to "Configure" via CardFeature).

The catch is what is_set_up means for the ESP integration (class-esp.php:53 (plugins/newspack-plugin/includes/reader-activation/integrations/class-esp.php#L53)). It requires two conditions:

A newsletter provider is connected in Newspack Newsletters (is_service_provider_configured() — API key set, etc.), and

A master list ID is stored in the integration settings (mailchimp_audience_id, active_campaign_master_list, or constant_contact_list_id).

So merely connecting a provider does not change the button — it keeps saying "Connect" until a master list/audience is also chosen. And this creates a dead-end loop in the UI:

When is_set_up is false, needsSetup is true, so clicking "Connect" navigates to the Newspack Newsletters settings page (setup_url) — even if the provider is already connected there and the only missing piece is the master list.

The master list picker lives in the integration's own configure view (#/settings/esp), but that view is unreachable from the card while it's in the "Connect" state: the configure button and the "more" menu only render when the integration is enabled, and the enable action is also hijacked to goToSetup while needsSetup is true.

So a user who connects Mailchimp and comes back to the Integrations screen sees "Connect", clicks it, lands back on the Newsletters settings page, and has no path from this screen to select the audience that would complete setup — unless the master list was already set elsewhere (e.g. previously configured via the Audience Configuration wizard).

In short: your observation is correct in the most common real-world case (provider connected, no master list selected yet). The fix would likely be to make the "Connect"-state routing distinguish "provider not connected" (→ Newsletters settings) from "provider connected but master list missing" (→ the integration's configure view), or relax is_set_up/needsSetup on the frontend so a connected provider routes to #/settings/esp.

How to test the changes in this Pull Request:

  1. Before applying this PR, you'll need a fresh Newspack site not connected to an ESP. On Integrations, click the Connect and input your API key. Observe the screen still says 'Connect' after saving and always takes you back to the Newsletter API key settings screen. It is impossible to reach the screen to select a master list via the UI.
Screenshot 2026-06-09 at 11 03 45 AM
  1. Apply this PR. Observe after inputting your API key, the button says 'Finish set up', which takes you to the master list select/field select screen. Select a master list.
Screenshot 2026-06-09 at 11 30 33 AM
  1. Observe the button then says 'Enable'. This feels like a lot of steps, so IMO it might make more sense to automatically enable it after setting the master list:
Screenshot 2026-06-09 at 11 42 56 AM

One architecture thing I want to flag is that this change introduces a new is_connected state, so the class has is_set_up and is_connected. We can probably avoid this if you hate it, or we can keep it if you think it's an important distinction.

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 run tests with your changes locally?

@claudiulodro claudiulodro requested a review from a team as a code owner June 9, 2026 18:45
Copilot AI review requested due to automatic review settings June 9, 2026 18:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a UI dead-end in the Audience → Integrations screen for the Newsletter ESP integration by separating “provider connected” from “integration fully configured”, so users can reach the master list/audience configuration step after entering provider credentials.

Changes:

  • Introduces an is_connected() integration state (separate from is_set_up()) and exposes it in the integrations settings payload.
  • Updates the Audience Integrations card action logic to route:
    • Not connected → external setup URL (“Connect”)
    • Connected but not set up → integration configure view (“Finish setup”)
    • Set up → enable action (“Enable”)
  • Adds/extends unit tests (PHP + JS) to cover the new state and routing behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
plugins/newspack-plugin/includes/reader-activation/integrations/class-integration.php Adds is_connected() default implementation + documentation.
plugins/newspack-plugin/includes/reader-activation/integrations/class-esp.php Implements is_connected() for ESP and refactors is_set_up() accordingly.
plugins/newspack-plugin/includes/reader-activation/class-integrations.php Surfaces is_connected in the integrations settings payload.
plugins/newspack-plugin/includes/reader-activation/integrations/README.md Documents the new is_connected() override contract and UI behavior.
plugins/newspack-plugin/src/wizards/audience/views/integrations/settings-section.js Updates card label/routing logic based on is_connected vs is_set_up.
plugins/newspack-plugin/src/wizards/audience/views/integrations/settings-section.test.js Adds JS coverage for the updated card action routing.
plugins/newspack-plugin/tests/unit-tests/integrations/class-test-integrations.php Adds PHP coverage for default is_connected() and payload surfacing.

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