fix(integrations): sync reader-account deletion to ESP#4731
Open
chickenn00dle wants to merge 21 commits into
Open
fix(integrations): sync reader-account deletion to ESP#4731chickenn00dle wants to merge 21 commits into
chickenn00dle wants to merge 21 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes reader-account deletion syncing to ESP/integrations and adds configurable per-integration deletion behavior.
Changes:
- Emits deletion event payloads with top-level
emailand gates legacy vs v1 deletion handlers by metadata version. - Adds account-deletion settings and a central deletion dispatcher supporting hard-delete or metadata-flag modes.
- Adds PHPUnit coverage, fixtures, and newsletter mocks for deletion routing and ESP deletion behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
includes/data-events/listeners.php |
Sends deleted reader email as a primitive payload field. |
includes/data-events/connectors/class-contact-sync-connector.php |
Gates deletion handlers and routes v1 deletions to the new dispatcher. |
includes/reader-activation/sync/class-contact-sync.php |
Adds account deletion dispatch across active integrations. |
includes/reader-activation/integrations/class-integration.php |
Adds deletion settings and default delete_contact() API. |
includes/reader-activation/integrations/class-esp.php |
Removes legacy deletion setting and implements ESP contact deletion. |
includes/reader-activation/integrations/README.md |
Documents deletion APIs, settings, and conditional fields. |
src/wizards/audience/views/integrations/configure-view.js |
Adds conditional field visibility support. |
tests/mocks/newsletters-mocks.php |
Extends newsletter mocks with delete/upsert/update-list call tracking. |
tests/unit-tests/data-events/class-contact-sync-connector.php |
Tests handler registration and deletion connector behavior. |
tests/unit-tests/data-events/class-listeners-deletion.php |
Tests deletion listener payload shape. |
tests/unit-tests/integrations/class-deletion-spy-integration.php |
Adds a spy integration fixture for deletion routing tests. |
tests/unit-tests/integrations/class-test-account-deletion.php |
Tests base deletion settings, migration, routing, and failure hooks. |
tests/unit-tests/integrations/class-test-esp.php |
Tests ESP delete delegation and removal of legacy setting declaration. |
tests/unit-tests/reader-activation-sync.php |
Adjusts reader-sync constant handling in tests. |
Comments suppressed due to low confidence (1)
includes/reader-activation/integrations/class-esp.php:173
- The ESP settings override now only adds provider-specific fields and metadata fields, so the new
sync_account_deletionandaccount_deletion_handlingfields appended by the base class are filtered out of the configure API response. That means the per-integration deletion controls described by this PR won't appear for the Newsletter ESP integration, leaving publishers unable to change these settings from the UI.
$metadata_keys = array_column( $this->get_metadata_fields(), 'key' );
foreach ( $config as $field ) {
if ( in_array( $field['key'], $metadata_keys ) ) {
$enriched[] = $config[ $field['key'] ];
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…bility - New Sync::is_syncing_allowed() exposes the NEWSPACK_ALLOW_READER_SYNC check via a filter so tests don't need to pollute global state with define(). - New Integration::supports_hard_delete() so third-party integrations that only implement push_contact_data() are not exposed as 'delete' mode in the configure UI; default for those is 'flag'. ESP overrides to true. - update_settings_field_value() works around WP's update_option(false) no-op on missing options so unchecking a checkbox persists on fresh sites.
…ondition - handle_account_deletion's flag branch now passes the contact through the newspack_esp_sync_contact filter, restoring parity with Contact_Sync::sync() so existing filters (Mailchimp status_if_new, metadata enrichment) still run. - configure-view's fieldIsVisible predicate now coerces both sides for boolean conditions so values arriving as scalar '1'/'0' strings from WP options (after lazy migration) match equals: true instead of staying hidden.
…filter - All deletion tests use the new newspack_reader_activation_is_syncing_allowed filter instead of defining NEWSPACK_ALLOW_READER_SYNC globally, so the constant no longer leaks into other suites. - Revert the markTestSkipped/defined() guards added to reader-activation-sync.php now that the filter is scoped per test class. - class-contact-sync-connector: snapshot Data_Events::$actions in set_up and restore in tear_down so reset_data_events_handlers no longer wipes handlers registered by other test classes. - class-listeners-deletion: store the dispatch listener and remove it in tear_down so closures don't pile up across the suite. - class-test-account-deletion: clear Integrations::OPTION_NAME in tear_down so test spy IDs don't leak into the enabled-integrations option. - Deletion_Spy_Integration overrides supports_hard_delete() to mirror its delete_contact implementation, so routing tests can exercise both modes. - New default-tests for account_deletion_handling: flag when hard-delete is unsupported (Sample_Integration), delete when supported (anonymous subclass).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7baf34b to
42f5491
Compare
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:
When a reader deletes their account, Newspack is supposed to propagate that deletion to the connected ESP (Mailchimp / ActiveCampaign / Constant Contact). At some point that stopped working — deletions were silently swallowed and the contact lingered in the ESP forever. This PR fixes that, and adds per-integration controls so publishers can choose what "deletion" actually means for each ESP.
dsgnews-126-deletion-sync-demo.webm
What changed for publishers
A new section appears on every integration's configure page under Audience → Integrations → Configure:
NP_Account_Deletedfield on it with the deletion timestamp. Publishers can wire their own automation (a Mailchimp journey, an ActiveCampaign automation, etc.) to act on it however they like — archive, tag, unsubscribe, anything.Sites that have already opted in to the legacy deletion sync (
sync_esp_delete = true) automatically map to "delete immediately"; sites that had it off automatically map to the unchecked state. No publisher action required.Closes DSGNEWS-126.
How to test the changes in this Pull Request:
Spin up an isolated env and connect a real ESP so outbound calls actually leave the box:
Then connect Newspack Newsletters to a test list on Mailchimp / ActiveCampaign / Constant Contact.
For each test below, create a reader (e.g. via My Account registration), confirm the contact landed in your ESP list, then delete the reader via the WP admin Users screen or My Account → Delete Account.
NP_Account_Deletedfield carrying aY-m-d H:i:stimestamp.Legacy sites. If a site is still on the legacy metadata mode, the old code path runs instead of the new dispatcher. Verify deletion still propagates and that the
sync_esp_deleteboolean is honored.Automated tests:
Technical details for reviewers
Root cause of the bug. Both deletion event handlers read
$data['user']['data']['user_email']. The listeners passed aWP_Userobject whoseArrayAccess::offsetGet('data')returnsnull, so theisset()guards short-circuited and both handlers returned early without syncing.Listener fix.
reader_deletedandreader_delete_syncnow emitemailas a top-level primitive, consistent with peer listeners (reader_logged_in,reader_verified).Handler split by metadata version. Today both the legacy
reader_deletedand the v1reader_delete_synchandlers register on every site and both fire ondelete_user. NowContact_Sync_Connector::register_handlers()registers only one of the two, gated onSync\Metadata::get_version(). Stops double-processing.Framework additions:
Integration::delete_contact( $email )— new base method, defaultWP_Error('not_implemented'). ESP overrides it to callNewspack_Newsletters_Contacts::delete().Integration::get_account_deletion_fields()— auto-appended to every integration's settings viaget_settings_fields(). Provides the two new fields described above. Includes lazy migration fromsync_esp_deletevia$legacy_option_map.Contact_Sync::handle_account_deletion()— new central dispatcher called fromreader_delete_sync. Iterates active integrations and routes each one according to its own settings. Firesnewspack_sync_contact_failedon errors (with amodediscriminator so Alert_Manager can tell delete-mode failures from flag-mode ones) and writes per-integration outcomes to the current ActionScheduler audit log, matching the patterns inpush_to_integrations.account_deletedmetadata is re-injected afterprepare_contact()with the integration's prefix (so it survives the v1 outgoing-fields filter). Format matches peer datetime metadata (Y-m-d H:i:s, Title_Case_With_Underscores key).fieldIsVisiblepredicate inconfigure-view.jshonors a newconditionshape on field declarations — generic enough that other integrations can use it too.Other information: