From ca4104f5de2509f776f9b5ab7cc8565c25b3bd5d Mon Sep 17 00:00:00 2001 From: Wil Gerken Date: Wed, 8 Jul 2026 10:16:37 -0700 Subject: [PATCH 1/4] fix(active-campaign): match fields by title and tolerate create failures Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KLfKx43ZWQnJYM8fcyaPpN --- ...s-newspack-newsletters-active-campaign.php | 40 ++- .../test-active-campaign-field-matching.php | 231 ++++++++++++++++++ 2 files changed, 268 insertions(+), 3 deletions(-) create mode 100644 plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php diff --git a/plugins/newspack-newsletters/includes/service-providers/active_campaign/class-newspack-newsletters-active-campaign.php b/plugins/newspack-newsletters/includes/service-providers/active_campaign/class-newspack-newsletters-active-campaign.php index f01f6dd959..245d59aa2c 100644 --- a/plugins/newspack-newsletters/includes/service-providers/active_campaign/class-newspack-newsletters-active-campaign.php +++ b/plugins/newspack-newsletters/includes/service-providers/active_campaign/class-newspack-newsletters-active-campaign.php @@ -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'] ) && + 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', @@ -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() ); + continue; } /** Set list relation. */ $this->api_v3_request( diff --git a/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php new file mode 100644 index 0000000000..1513cd2050 --- /dev/null +++ b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php @@ -0,0 +1,231 @@ +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', + ], + ]; + + $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. + + $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', + ], + ]; + + $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. + ], + ]; + + $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.' ); + } +} From f47ad5c30154ef62bfa07e7e8737396b64b2d658 Mon Sep 17 00:00:00 2001 From: Wil Gerken Date: Wed, 8 Jul 2026 13:55:32 -0700 Subject: [PATCH 2/4] chore(active-campaign): satisfy phpcs in field-matching tests Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KLfKx43ZWQnJYM8fcyaPpN --- .../test-active-campaign-field-matching.php | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php index 1513cd2050..33aedf3c43 100644 --- a/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php +++ b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php @@ -96,7 +96,17 @@ public function mock_http( $preempt, $args, $url ) { } 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' ] ] ] ); + 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'; @@ -128,6 +138,7 @@ public function test_field_matched_by_title_when_perstag_renamed() { ], ]; + // phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods -- this test exercises the provider method itself. $result = Newspack_Newsletters_Active_Campaign::instance()->add_contact( [ 'email' => 'reader@example.net', @@ -147,6 +158,7 @@ public function test_field_matched_by_title_when_perstag_renamed() { 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 -- this test exercises the provider method itself. $result = Newspack_Newsletters_Active_Campaign::instance()->add_contact( [ 'email' => 'reader2@example.net', @@ -174,6 +186,7 @@ public function test_field_matched_by_perstag_unchanged() { ], ]; + // phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods -- this test exercises the provider method itself. $result = Newspack_Newsletters_Active_Campaign::instance()->add_contact( [ 'email' => 'reader3@example.net', @@ -201,9 +214,9 @@ public function test_title_match_survives_field_rows_without_title() { // No title key at all. ], [ - 'id' => '5', - 'title' => 'Some Broken Field', - 'type' => 'text', + 'id' => '5', + 'title' => 'Some Broken Field', + 'type' => 'text', // Title but no perstag: unusable for payload, must not match. ], [ @@ -216,6 +229,7 @@ public function test_title_match_survives_field_rows_without_title() { ], ]; + // phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods -- this test exercises the provider method itself. $result = Newspack_Newsletters_Active_Campaign::instance()->add_contact( [ 'email' => 'reader4@example.net', From fa00231c520ff63fb4049a2a6b2f467dd7747f20 Mon Sep 17 00:00:00 2001 From: Wil Gerken Date: Wed, 8 Jul 2026 14:27:25 -0700 Subject: [PATCH 3/4] chore(active-campaign): ignore strict contacts sniff in provider tests Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KLfKx43ZWQnJYM8fcyaPpN --- .../tests/test-active-campaign-field-matching.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php index 33aedf3c43..021a2a6fe1 100644 --- a/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php +++ b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php @@ -138,7 +138,7 @@ public function test_field_matched_by_title_when_perstag_renamed() { ], ]; - // phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods -- this test exercises the provider method itself. + // 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', @@ -158,7 +158,7 @@ public function test_field_matched_by_title_when_perstag_renamed() { 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 -- this test exercises the provider method itself. + // 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', @@ -186,7 +186,7 @@ public function test_field_matched_by_perstag_unchanged() { ], ]; - // phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods -- this test exercises the provider method itself. + // 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', @@ -229,7 +229,7 @@ public function test_title_match_survives_field_rows_without_title() { ], ]; - // phpcs:ignore phpcsSniffs.Newsletters.ForbiddenMethods.PossibleForbiddenContactsMethods -- this test exercises the provider method itself. + // 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', From 80a9ca010ed0eb425ca94949c99d15449886e93b Mon Sep 17 00:00:00 2001 From: Wil Gerken Date: Wed, 8 Jul 2026 14:40:57 -0700 Subject: [PATCH 4/4] chore(active-campaign): strip trailing whitespace in test file Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KLfKx43ZWQnJYM8fcyaPpN --- .../tests/test-active-campaign-field-matching.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php index 021a2a6fe1..66ac4a687d 100644 --- a/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php +++ b/plugins/newspack-newsletters/tests/test-active-campaign-field-matching.php @@ -105,7 +105,7 @@ public function mock_http( $preempt, $args, $url ) { 'title' => 'There is already a field with this title', ], ], - ] + ] ); } if ( false !== strpos( $url, '/api/3/contacts' ) ) {