Skip to content

fix(active-campaign): keep signups working after field tag edits #573

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.

Sign up for GitHub

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

Open
wil-gerken wants to merge 4 commits into
base: release
Choose a base branch
from
Open

fix(active-campaign): keep signups working after field tag edits #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca4104f
fix(active-campaign): match fields by title and tolerate create failures
wil-gerken Jul 8, 2026
f47ad5c
chore(active-campaign): satisfy phpcs in field-matching tests
wil-gerken Jul 8, 2026
fa00231
chore(active-campaign): ignore strict contacts sniff in provider tests
wil-gerken Jul 8, 2026
80a9ca0
chore(active-campaign): strip trailing whitespace in test file
wil-gerken Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions ...includes/service-providers/active_campaign/class-newspack-newsletters-active-campaign.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1999,8 +1999,40 @@ public function add_contact( $contact, $list_id = false ) {
$existing_fields = $this->get_all_contact_fields();
foreach ( $contact['metadata'] as $field_title => $value ) {
$field_perstag = strtoupper( str_replace( '-', '_', sanitize_title( $field_title ) ) );
/** For optimization, don't add the field if it already exists. */
if ( is_wp_error( $existing_fields ) || false === array_search( $field_perstag, array_column( $existing_fields, 'perstag' ) ) ) {

/**
* For optimization, don't add the field if it already exists.
* Match by perstag first, then by title — an ActiveCampaign
* admin may have renamed a field's perstag, and creating a
* field with a duplicate title fails.
*/
$existing_index = false;
if ( ! is_wp_error( $existing_fields ) ) {
// Index-preserving lookups: array_column() would reindex and skip
// rows missing the key, mapping a match to the wrong field.
foreach ( $existing_fields as $index => $existing_field ) {
if ( isset( $existing_field['perstag'] ) && $existing_field['perstag'] === $field_perstag ) {
$existing_index = $index;
break;
}
}
if ( false === $existing_index ) {
foreach ( $existing_fields as $index => $existing_field ) {
if (
isset( $existing_field['title'], $existing_field['perstag'] ) &&

@dkoo dkoo Jul 10, 2026

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.

Suggestion: Field-match guards check isset() not !empty() — an empty perstag can match — The perstag and title-match guards check isset( $existing_field['perstag'] ) rather than ! empty( ... ), so a field row whose perstag is an empty string can still match. When it does, $field_perstag is set to '' (line 2034) and the payload emits a malformed field[%%,0] key — and two such fields would collide on it. There's no cross-field data-leak risk: a value is never routed into a different real named field, and in practice ActiveCampaign auto-assigns perstags, so an empty one is effectively unreachable. Still, tightening both guards to ! empty( $existing_field['perstag'] ) (and skipping the field, or falling through to create, when the generated perstag is empty) is cheap defense and would round out the existing malformed-row test coverage — the current test_title_match_survives_field_rows_without_title covers a missing perstag key but not an empty-string one.

0 === strcasecmp( trim( $existing_field['title'] ), trim( $field_title ) )
) {
$existing_index = $index;
break;
}
}
}
}

if ( false !== $existing_index ) {
/** Use the existing field's actual perstag — it may differ from the generated one. */
$field_perstag = $existing_fields[ $existing_index ]['perstag'];
} else {
$field_res = $this->api_v3_request(
'fields',
'POST',
Expand All @@ -2018,7 +2050,9 @@ public function add_contact( $contact, $list_id = false ) {
]
);
if ( \is_wp_error( $field_res ) ) {
return $field_res;
/** A field that can't be registered must never block the signup — sync the contact without it. */
Newspack_Newsletters_Logger::log( 'Error creating ActiveCampaign field "' . $field_title . '": ' . $field_res->get_error_message() );

@dkoo dkoo Jul 10, 2026

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.

Suggestion: Field-create failure log is level-gated, so a dropped RAS field is silent on a default site — Making a failed field-create non-fatal is the right call — one unregisterable field should never abort a reader's whole signup. Worth weighing, though: the metadata synced here is Reader Activation contact data (registration, engagement, subscription, donation, content-gate status) that ActiveCampaign segments and automations key on, so a silently dropped field isn't purely cosmetic. The common real failure — the duplicate-title 422 — is now caught by the new title match before any create is attempted, and the remaining path is self-healing (retried on the next signup), which keeps this low-risk. The catch: Newspack_Newsletters_Logger::log() no-ops entirely unless NEWSPACK_LOG_LEVEL is defined (class-newspack-newsletters-logger.php:20), which it isn't on a default production site — so a field that keeps failing to create leaves no trace at all. Consider making just this one failure surface unconditionally (e.g. a direct error_log) so an under-populated segment is diagnosable.

Risk if not addressed: If a field-create keeps failing for a non-title reason on a site that has not defined NEWSPACK_LOG_LEVEL, a load-bearing Reader Activation field (donation, subscription, or membership status) is silently dropped on every signup with no log trace, so readers who should enter an AC segment or automation keyed on that field are never added to it — invisible until a publisher notices the segment is under-populated.

continue;
}
/** Set list relation. */
$this->api_v3_request(
Expand Down
245 changes: 245 additions & 0 deletions plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,245 @@
<?php // phpcs:ignore WordPress.Files.FileName.InvalidClassFileName
/**
* Tests for ActiveCampaign custom-field matching during contact upsert.
*
* Background: `add_contact()` registers metadata fields on ActiveCampaign
* before syncing the contact. A field is looked up by its generated perstag;
* when an AC admin has renamed a field's perstag, the lookup misses, the
* field-create call fails ("There is already a field with this title"), and
* the whole signup aborts. Fields must also match by title, the payload must
* carry the field's actual perstag, and a field-create failure must never
* block the contact sync.
*
* @package Newspack_Newsletters
*/

/**
* Test ActiveCampaign field matching in add_contact().
*/
class ActiveCampaignFieldMatchingTest extends WP_UnitTestCase {

/**
* Every ActiveCampaign action invoked through the mocked HTTP layer, in order.
*
* @var string[]
*/
private $called_actions = [];

/**
* Body of the last contact_sync/contact_add request.
*
* @var array
*/
private $contact_payload = [];

/**
* Fields the mocked AC account reports via GET /api/3/fields.
*
* @var array
*/
private $remote_fields = [];

/**
* Set up: configure credentials and intercept all outbound HTTP.
*/
public function set_up() {
parent::set_up();
$this->called_actions = [];
$this->contact_payload = [];
$this->remote_fields = [];
Newspack_Newsletters_Active_Campaign::instance()->set_api_credentials(
[
'url' => 'https://example.api-us1.com',
'key' => 'test-key',
]
);
add_filter( 'pre_http_request', [ $this, 'mock_http' ], 10, 3 );
}

/**
* Tear down.
*/
public function tear_down() {
remove_filter( 'pre_http_request', [ $this, 'mock_http' ], 10 );
parent::tear_down();
}

/**
* Intercept outbound requests and play an AC account with $remote_fields.
*
* @param mixed $preempt Short-circuit value.
* @param array $args HTTP request arguments.
* @param string $url Request URL.
*
* @return array
*/
public function mock_http( $preempt, $args, $url ) {
$respond = function ( $code, $body ) {
return [
'response' => [
'code' => $code,
'message' => 200 === $code ? 'OK' : 'Unprocessable Entity',
],
'body' => wp_json_encode( $body ),
];
};

if ( false !== strpos( $url, '/api/3/fields' ) && 'GET' === $args['method'] ) {
$this->called_actions[] = 'v3:fields:list';
return $respond(
200,
[
'fields' => $this->remote_fields,
'meta' => [ 'total' => count( $this->remote_fields ) ],
]
);
}
if ( false !== strpos( $url, '/api/3/fields' ) && 'POST' === $args['method'] ) {
$this->called_actions[] = 'v3:fields:create';
return $respond(
422,
[
'errors' => [
[
'code' => 'duplicate',
'title' => 'There is already a field with this title',
],
],
]
);
}
if ( false !== strpos( $url, '/api/3/contacts' ) ) {
$this->called_actions[] = 'v3:contacts';
return $respond( 200, [ 'contacts' => [] ] );
}
if ( false !== strpos( $url, 'admin/api.php' ) ) {
$action = isset( $args['body']['api_action'] ) ? $args['body']['api_action'] : 'v1:unknown';
$this->called_actions[] = $action;
if ( in_array( $action, [ 'contact_add', 'contact_sync', 'contact_edit' ], true ) ) {
$this->contact_payload = $args['body'];
}
return $respond( 200, [ 'result_code' => 1, 'subscriber_id' => 42 ] ); // phpcs:ignore WordPress.Arrays.ArrayDeclarationSpacing.AssociativeArrayFound
}
$this->called_actions[] = 'unmatched:' . $url;
return $respond( 200, [ 'result_code' => 1 ] ); // phpcs:ignore WordPress.Arrays.ArrayDeclarationSpacing.AssociativeArrayFound
}

/**
* A field whose perstag was renamed on the AC side must be matched by
* title, and the contact payload must carry the field's ACTUAL perstag.
*/
public function test_field_matched_by_title_when_perstag_renamed() {
$this->remote_fields = [
[
'id' => '7',
'title' => 'Newsletter Subscription Method',
'perstag' => 'NEWSLETTERSSUBSCRIPTIONMETHOD',
'type' => 'text',
],
];

// phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods, phpcsSniffs.Newsletters.ForbiddenContactsMethods.ForbiddenContactsMethods -- this test exercises the provider method itself.
$result = Newspack_Newsletters_Active_Campaign::instance()->add_contact(
[
'email' => 'reader@example.net',
'metadata' => [ 'Newsletter Subscription Method' => 'newsletter-block' ],
],
'1'
);

$this->assertFalse( is_wp_error( $result ), 'Signup must not fail when a field title exists with a renamed perstag. Got: ' . ( is_wp_error( $result ) ? $result->get_error_message() : '' ) );
$this->assertNotContains( 'v3:fields:create', $this->called_actions, 'No field-create should be attempted when a field with the same title exists.' );
$this->assertArrayHasKey( 'field[%NEWSLETTERSSUBSCRIPTIONMETHOD%,0]', $this->contact_payload, 'Contact payload must use the existing field\'s actual perstag.' );
}

/**
* A failed field-create must not block the contact sync.
*/
public function test_field_create_failure_does_not_block_signup() {
$this->remote_fields = []; // Field genuinely missing; create will fail with 422.

// phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods, phpcsSniffs.Newsletters.ForbiddenContactsMethods.ForbiddenContactsMethods -- this test exercises the provider method itself.
$result = Newspack_Newsletters_Active_Campaign::instance()->add_contact(
[
'email' => 'reader2@example.net',
'metadata' => [ 'Newsletter Subscription Method' => 'newsletter-block' ],
],
'1'
);

$this->assertFalse( is_wp_error( $result ), 'A field-create failure must not abort the signup.' );
$this->assertContains( 'v3:fields:create', $this->called_actions, 'Field creation should have been attempted.' );
$this->assertTrue( in_array( 'contact_add', $this->called_actions, true ) || in_array( 'contact_sync', $this->called_actions, true ), 'Contact sync must still run after a failed field-create.' );
}

/**
* The perstag-match fast path is unchanged: no field-create attempted,
* generated perstag used.
*/
public function test_field_matched_by_perstag_unchanged() {
$this->remote_fields = [
[
'id' => '7',
'title' => 'Newsletter Subscription Method',
'perstag' => 'NEWSLETTER_SUBSCRIPTION_METHOD',
'type' => 'text',
],
];

// phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods, phpcsSniffs.Newsletters.ForbiddenContactsMethods.ForbiddenContactsMethods -- this test exercises the provider method itself.
$result = Newspack_Newsletters_Active_Campaign::instance()->add_contact(
[
'email' => 'reader3@example.net',
'metadata' => [ 'Newsletter Subscription Method' => 'newsletter-block' ],
],
'1'
);

$this->assertFalse( is_wp_error( $result ) );
$this->assertNotContains( 'v3:fields:create', $this->called_actions );
$this->assertArrayHasKey( 'field[%NEWSLETTER_SUBSCRIPTION_METHOD%,0]', $this->contact_payload );
}

/**
* Title matching must survive malformed field rows: array_column() skips
* rows missing the key and reindexes, which can map a match back to the
* wrong field (and thus the wrong perstag).
*/
public function test_title_match_survives_field_rows_without_title() {
$this->remote_fields = [
[
'id' => '3',
'perstag' => 'UNRELATED_FIELD',
'type' => 'text',
// No title key at all.
],
[
'id' => '5',
'title' => 'Some Broken Field',
'type' => 'text',
// Title but no perstag: unusable for payload, must not match.
],
[
'id' => '7',
'title' => 'newsletter subscription method ',
'perstag' => 'NEWSLETTERSSUBSCRIPTIONMETHOD',
'type' => 'text',
// Re-cased + padded title: AC treats titles as duplicates
// case-insensitively, so this must still match.
],
];

// phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods, phpcsSniffs.Newsletters.ForbiddenContactsMethods.ForbiddenContactsMethods -- this test exercises the provider method itself.
$result = Newspack_Newsletters_Active_Campaign::instance()->add_contact(
[
'email' => 'reader4@example.net',
'metadata' => [ 'Newsletter Subscription Method' => 'newsletter-block' ],
],
'1'
);

$this->assertFalse( is_wp_error( $result ) );
$this->assertArrayHasKey( 'field[%NEWSLETTERSSUBSCRIPTIONMETHOD%,0]', $this->contact_payload, 'Title match must resolve to the matched field\'s perstag, not a reindexed neighbor\'s.' );
$this->assertArrayNotHasKey( 'field[%UNRELATED_FIELD%,0]', $this->contact_payload, 'Value must not be written to an unrelated field.' );
}
}
Loading