diff --git a/plugins/newspack-plugin/includes/class-alert-manager.php b/plugins/newspack-plugin/includes/class-alert-manager.php index f780688c10..d412402716 100644 --- a/plugins/newspack-plugin/includes/class-alert-manager.php +++ b/plugins/newspack-plugin/includes/class-alert-manager.php @@ -31,6 +31,15 @@ class Alert_Manager { */ const FAILURE_LOG_OPTION = 'newspack_alert_failure_log'; + /** + * Window during which a repeating health-check failure for the same + * integration + error-code signature emits at most one Slack alert. + * + * Private because no external caller needs to read it; keeping the + * surface minimal lets the value evolve without breaking consumers. + */ + private const HEALTH_CHECK_DEDUP_INTERVAL = DAY_IN_SECONDS; + /** * Default pattern rules. * Each rule defines a grouping dimension, threshold, and time interval. @@ -454,14 +463,52 @@ private static function format_interval( $seconds ) { /** * Handle integration health check failure. * + * Deduplicates by integration + error-code + error-message signature + * for HEALTH_CHECK_DEDUP_INTERVAL so an hourly cron does not repeat + * the same Slack alert all day. A new error code OR a changed message + * on the same integration (e.g. "list missing" escalating to "auth + * fully revoked") falls outside the key and alerts immediately. + * + * Known boundaries of the dedup contract: + * - Message text is part of the key, so locale shifts between cron + * passes (e.g. `switch_to_locale()` in a multilingual context) can + * produce a different key for the same underlying error and + * re-alert. Newspack ESP error messages are static per code today, + * so this is theoretical; revisit if dynamic content lands in + * error strings. + * - The dedup key is stored as a transient, so on hosts backed by a + * persistent object cache (memcached) the entry can be evicted + * under LRU pressure before HEALTH_CHECK_DEDUP_INTERVAL elapses. + * The failure mode is re-alerting on the next hourly cron — the + * alternative (writing to the options table on every cron tick) + * has its own cost; transient + accepted re-alert risk is the + * intentional trade-off here. + * * @param array $payload Health check failure data. */ public static function handle_health_check_failed( $payload ) { - $error = $payload['error'] ?? null; + $error = $payload['error'] ?? null; + $integration_id = $payload['integration_id'] ?? 'unknown'; + $error_codes = is_wp_error( $error ) ? $error->get_error_codes() : []; + if ( empty( $error_codes ) ) { + $error_codes = [ 'unknown' ]; + } + $error_messages = is_wp_error( $error ) ? $error->get_error_messages() : []; + + $dedup_key = self::get_health_check_dedup_key( $integration_id, $error_codes, $error_messages ); + if ( get_transient( $dedup_key ) ) { + return; + } + + // Set the dedup transient BEFORE dispatch so a `newspack_alert` + // handler that throws (e.g. transient Slack POST failure) cannot + // defeat dedup by leaving the key unset for the next hourly cron. + set_transient( $dedup_key, time(), self::HEALTH_CHECK_DEDUP_INTERVAL ); + $message = sprintf( 'Integration "%s" health check failed: %s', $payload['integration_name'] ?? 'unknown', - is_wp_error( $error ) ? implode( '; ', $error->get_error_messages() ) : 'unknown error' + is_wp_error( $error ) ? implode( '; ', $error_messages ) : 'unknown error' ); /** This action is documented in includes/class-alert-manager.php */ @@ -476,5 +523,22 @@ public static function handle_health_check_failed( $payload ) { ] ); } + + /** + * Get the deduplication transient key for a health-check failure. + * + * @param string $integration_id The integration identifier. + * @param string[] $error_codes The WP_Error codes from the failure. + * @param string[] $error_messages The WP_Error messages from the failure. + * + * @return string Transient key. + */ + private static function get_health_check_dedup_key( $integration_id, $error_codes, $error_messages = [] ) { + $codes = array_map( 'strval', $error_codes ); + sort( $codes ); + $messages = array_map( 'strval', $error_messages ); + sort( $messages ); + return 'newspack_alert_hc_' . md5( $integration_id . ':' . implode( ',', $codes ) . ':' . implode( '|', $messages ) ); + } } Alert_Manager::init(); diff --git a/plugins/newspack-plugin/includes/reader-activation/class-integrations.php b/plugins/newspack-plugin/includes/reader-activation/class-integrations.php index 17ffe4a9d4..ad4ec8d5a4 100644 --- a/plugins/newspack-plugin/includes/reader-activation/class-integrations.php +++ b/plugins/newspack-plugin/includes/reader-activation/class-integrations.php @@ -364,6 +364,29 @@ public static function get_active_integrations() { return $active; } + /** + * Get active integrations whose external prerequisites are also configured. + * + * Iteration sites that perform real I/O on each integration (health checks, + * contact pushes, pulls, retries) MUST use this instead of + * `get_active_integrations()` to avoid alerting on, or scheduling AS + * retries for, integrations the admin has not yet finished setting up. + * + * `is_set_up()` is a stored-state check by contract — see ESP's override. + * Treating it as a runtime probe here would silently drop traffic on + * transient provider failures. + * + * @return Integration[] Integrations that are both enabled AND set up. + */ + public static function get_active_configured_integrations() { + return array_filter( + self::get_active_integrations(), + function ( $integration ) { + return $integration->is_set_up(); + } + ); + } + /** * Get a specific integration by ID. * @@ -742,13 +765,17 @@ public static function deactivate_health_check() { } /** - * Run health checks on all active integrations. + * Run health checks on all active and configured integrations. + * + * Routed through `get_active_configured_integrations()` so a missing + * provider or unconfigured master list — a setup-incomplete state, not a + * runtime incident — surfaces in the integrations UI rather than the + * alerts channel. * * Logs failures and fires an action for the Alert Manager. */ public static function run_health_checks() { - $active = self::get_active_integrations(); - foreach ( $active as $integration ) { + foreach ( self::get_active_configured_integrations() as $integration ) { $result = $integration->health_check(); if ( is_wp_error( $result ) ) { Logger::error( diff --git a/plugins/newspack-plugin/includes/reader-activation/integrations/class-contact-pull.php b/plugins/newspack-plugin/includes/reader-activation/integrations/class-contact-pull.php index 23c3f9b8f2..93b528dcf0 100644 --- a/plugins/newspack-plugin/includes/reader-activation/integrations/class-contact-pull.php +++ b/plugins/newspack-plugin/includes/reader-activation/integrations/class-contact-pull.php @@ -121,7 +121,7 @@ public static function is_stale( $timestamp ) { */ public static function pull_sync( $integrations = [] ) { if ( empty( $integrations ) ) { - $integrations = Integrations::get_active_integrations(); + $integrations = Integrations::get_active_configured_integrations(); } Logger::log( 'Synchronous pull started for user "' . get_current_user_id() . '".', self::LOGGER_HEADER ); @@ -158,7 +158,7 @@ public static function pull_sync( $integrations = [] ) { * @return true|\WP_Error True if all succeeded, WP_Error with combined messages. */ public static function pull_all( $user_id ) { - $active_integrations = Integrations::get_active_integrations(); + $active_integrations = Integrations::get_active_configured_integrations(); $errors = []; foreach ( $active_integrations as $integration ) { @@ -225,6 +225,14 @@ public static function handle_ajax_pull() { wp_send_json_error( 'Integration not found or not enabled.', 404 ); } + // Defense-in-depth: pull_sync already filters to set-up integrations, + // but a direct AJAX call could still arrive here for an unconfigured + // integration. Skip silently with success — "not set up" is a no-op, + // not an error worth surfacing to the loopback caller. + if ( ! $integration->is_set_up() ) { + wp_send_json_success(); + } + $user_id = get_current_user_id(); if ( ! $user_id ) { wp_send_json_error( 'No user context.', 403 ); @@ -418,6 +426,11 @@ public static function execute_integration_retry( $retry_data ) { return; } + if ( ! $integration->is_set_up() ) { + Logger::log( sprintf( 'Integration "%s" no longer set up on pull retry %d; aborting retry chain.', $integration_id, $retry_count ), self::LOGGER_HEADER ); + return; + } + Logger::log( sprintf( 'Executing pull retry %d/%d for integration "%s" of user %d.', $retry_count, self::MAX_RETRIES, $integration_id, $user_id ), self::LOGGER_HEADER ); $result = self::pull_single_integration( $user_id, $integration ); diff --git a/plugins/newspack-plugin/includes/reader-activation/integrations/class-esp.php b/plugins/newspack-plugin/includes/reader-activation/integrations/class-esp.php index 13bcbd6685..9a7cdd1f22 100644 --- a/plugins/newspack-plugin/includes/reader-activation/integrations/class-esp.php +++ b/plugins/newspack-plugin/includes/reader-activation/integrations/class-esp.php @@ -37,14 +37,28 @@ public function __construct() { /** * Whether the ESP integration is ready to sync. * - * Mirrors the readiness gate used by get_settings_config() so the configure - * UI never advertises a card as set up while the underlying settings call - * short-circuits to an empty config. + * Checks STORED configuration only — provider option set + master list + * option set. Does NOT call the live provider API. A live `get_lists()` + * call here would mean: every gate that consults `is_set_up()` + * (`Integrations::get_active_configured_integrations()`, the retry-time + * guards in `Contact_Sync` / `Contact_Pull`) would silently skip and + * lose data on any transient provider failure — exactly the failure + * mode the AS retry system was built to survive. The setup question + * "did the admin finish configuring this?" must be answered from local + * state; "is the provider reachable right now?" is `health_check()`'s + * job. * - * @return bool True if an ESP provider is selected and at least one list is active. + * @return bool True if a provider is selected and a master list ID is stored. */ public function is_set_up() { - return Reader_Activation::is_esp_configured(); + $newsletters_configuration_manager = Configuration_Managers::configuration_manager_class_for_plugin_slug( 'newspack-newsletters' ); + if ( ! $newsletters_configuration_manager || is_wp_error( $newsletters_configuration_manager ) ) { + return false; + } + if ( ! $newsletters_configuration_manager->is_esp_set_up() ) { + return false; + } + return (bool) $this->get_master_list_id(); } /** diff --git a/plugins/newspack-plugin/includes/reader-activation/sync/class-contact-sync.php b/plugins/newspack-plugin/includes/reader-activation/sync/class-contact-sync.php index 454cc75719..546f3026c4 100644 --- a/plugins/newspack-plugin/includes/reader-activation/sync/class-contact-sync.php +++ b/plugins/newspack-plugin/includes/reader-activation/sync/class-contact-sync.php @@ -162,7 +162,7 @@ private static function push_to_integrations( $contact, $context, $existing_cont * @param string $context The context of the sync. */ $contact = \apply_filters( 'newspack_esp_sync_contact', $contact, $context ); - $integrations = Integrations::get_active_integrations(); + $integrations = Integrations::get_active_configured_integrations(); $errors = []; // Resolve user ID for retry scheduling. @@ -372,6 +372,11 @@ public static function execute_integration_retry( $retry_data ) { return; } + if ( ! $integration->is_set_up() ) { + static::log( sprintf( 'Integration "%s" no longer set up on retry %d; aborting retry chain.', $integration_id, $retry_count ) ); + return; + } + static::log( sprintf( 'Executing retry %d/%d for integration "%s" sync of user %d (%s).', $retry_count, self::MAX_RETRIES, $integration_id, $user_id, $contact['email'] ?? 'unknown' ) ); /** This filter is documented in includes/reader-activation/sync/class-contact-sync.php */ diff --git a/plugins/newspack-plugin/tests/mocks/newsletters-mocks.php b/plugins/newspack-plugin/tests/mocks/newsletters-mocks.php index 84443014af..1688cd27d5 100644 --- a/plugins/newspack-plugin/tests/mocks/newsletters-mocks.php +++ b/plugins/newspack-plugin/tests/mocks/newsletters-mocks.php @@ -67,6 +67,19 @@ public static function get_fields( $list_id = null ) { class Newspack_Newsletters { const EMAIL_HTML_META = 'newspack_email_html'; + /** + * Default return value of is_service_provider_configured(). Tests that + * exercise the "provider not selected" branch can flip this to false + * via reset_calls() / direct assignment, then restore it. + * + * @var bool + */ + public static $is_service_provider_configured = true; + + public static function reset_calls() { + self::$is_service_provider_configured = true; + } + public static function service_provider() { return get_option( 'newspack_newsletters_service_provider', false ); } @@ -76,7 +89,7 @@ public static function get_service_provider() { } public static function is_service_provider_configured() { - return true; + return self::$is_service_provider_configured; } } } diff --git a/plugins/newspack-plugin/tests/unit-tests/alert-manager.php b/plugins/newspack-plugin/tests/unit-tests/alert-manager.php index 268a034300..c2a58417df 100644 --- a/plugins/newspack-plugin/tests/unit-tests/alert-manager.php +++ b/plugins/newspack-plugin/tests/unit-tests/alert-manager.php @@ -657,4 +657,176 @@ public function test_pattern_scan_is_scheduled() { 'Pattern scan cron event should be scheduled.' ); } + + /** + * Helper: build a health-check failure payload with the given codes. + * + * @param string $integration_id Integration ID. + * @param string[] $codes WP_Error codes to attach. + * @return array + */ + private function make_health_check_payload( $integration_id, array $codes ) { + $error = new \WP_Error(); + foreach ( $codes as $code ) { + $error->add( $code, sprintf( 'Mock: %s', $code ) ); + } + return [ + 'integration_id' => $integration_id, + 'integration_name' => 'Mock ' . $integration_id, + 'error' => $error, + ]; + } + + /** + * Test that a repeated health-check failure with the same integration + + * error codes only triggers one Slack-bound newspack_alert within the + * dedup interval. + */ + public function test_health_check_failed_dedupes_repeated_fires() { + $fire_count = 0; + add_action( + 'newspack_alert', + function ( $data ) use ( &$fire_count ) { + if ( 'integration_health_check_failed' === ( $data['type'] ?? '' ) ) { + $fire_count++; + } + } + ); + + $payload = $this->make_health_check_payload( 'dedup-a', [ 'master_list_missing' ] ); + + do_action( 'newspack_integration_health_check_failed', $payload ); + do_action( 'newspack_integration_health_check_failed', $payload ); + do_action( 'newspack_integration_health_check_failed', $payload ); + + $this->assertEquals( 1, $fire_count, 'Identical health-check failures should dedupe to a single alert.' ); + } + + /** + * Test that a different error-code set on the same integration fires a + * fresh alert — the dedup is per signature, not per integration. + */ + public function test_health_check_failed_alerts_on_new_error_codes() { + $fire_count = 0; + add_action( + 'newspack_alert', + function ( $data ) use ( &$fire_count ) { + if ( 'integration_health_check_failed' === ( $data['type'] ?? '' ) ) { + $fire_count++; + } + } + ); + + do_action( + 'newspack_integration_health_check_failed', + $this->make_health_check_payload( 'dedup-b', [ 'master_list_missing' ] ) + ); + do_action( + 'newspack_integration_health_check_failed', + $this->make_health_check_payload( 'dedup-b', [ 'master_list_missing' ] ) + ); + do_action( + 'newspack_integration_health_check_failed', + $this->make_health_check_payload( 'dedup-b', [ 'connection_failed' ] ) + ); + + $this->assertEquals( 2, $fire_count, 'A new error-code set on the same integration should bypass the dedup.' ); + } + + /** + * Test that two distinct integrations alert independently even if they + * fail with the same error codes. + */ + public function test_health_check_failed_alerts_per_integration() { + $fire_count = 0; + add_action( + 'newspack_alert', + function ( $data ) use ( &$fire_count ) { + if ( 'integration_health_check_failed' === ( $data['type'] ?? '' ) ) { + $fire_count++; + } + } + ); + + do_action( + 'newspack_integration_health_check_failed', + $this->make_health_check_payload( 'dedup-c1', [ 'master_list_missing' ] ) + ); + do_action( + 'newspack_integration_health_check_failed', + $this->make_health_check_payload( 'dedup-c2', [ 'master_list_missing' ] ) + ); + + $this->assertEquals( 2, $fire_count, 'Distinct integration IDs should each alert independently.' ); + } + + /** + * Test that a same-code but escalated-message failure fires a fresh alert. + * + * The dedup key folds in `WP_Error::get_error_messages()` so an escalating + * failure that retains the same code(s) but carries a worse message + * (e.g. "list missing" → "auth fully revoked") still reaches Slack + * instead of being suppressed for the full HEALTH_CHECK_DEDUP_INTERVAL. + */ + public function test_health_check_failed_alerts_on_new_error_messages() { + $fire_count = 0; + add_action( + 'newspack_alert', + function ( $data ) use ( &$fire_count ) { + if ( 'integration_health_check_failed' === ( $data['type'] ?? '' ) ) { + $fire_count++; + } + } + ); + + $first = [ + 'integration_id' => 'dedup-msg', + 'integration_name' => 'Mock dedup-msg', + 'error' => new \WP_Error( 'connection_failed', 'Provider returned 401: list missing.' ), + ]; + $second = [ + 'integration_id' => 'dedup-msg', + 'integration_name' => 'Mock dedup-msg', + 'error' => new \WP_Error( 'connection_failed', 'Provider returned 401: auth fully revoked.' ), + ]; + + do_action( 'newspack_integration_health_check_failed', $first ); + do_action( 'newspack_integration_health_check_failed', $first ); + do_action( 'newspack_integration_health_check_failed', $second ); + + $this->assertEquals( 2, $fire_count, 'A same-code, changed-message failure should bypass the dedup.' ); + } + + /** + * Test that the dedup transient is set BEFORE dispatching `newspack_alert` + * so a handler that throws cannot leave the key unset and defeat dedup + * on the next hourly cron. + */ + public function test_health_check_failed_sets_dedup_before_dispatch() { + $listener = function () { + throw new \RuntimeException( 'Simulated handler failure.' ); + }; + add_action( 'newspack_alert', $listener ); + + $payload = $this->make_health_check_payload( 'dedup-pre', [ 'master_list_missing' ] ); + + try { + try { + do_action( 'newspack_integration_health_check_failed', $payload ); + } catch ( \RuntimeException $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch + // Expected — handler is intentionally throwing. + } + + // Reflect the dedup-key contract — the transient must exist even + // though dispatch threw. + $reflection = new \ReflectionMethod( Alert_Manager::class, 'get_health_check_dedup_key' ); + $reflection->setAccessible( true ); + $key = $reflection->invoke( null, 'dedup-pre', [ 'master_list_missing' ], [ 'Mock: master_list_missing' ] ); + + $this->assertNotFalse( get_transient( $key ), 'Dedup transient must be set even when alert handler throws.' ); + } finally { + remove_action( 'newspack_alert', $listener ); + delete_transient( $key ?? '' ); + } + } } diff --git a/plugins/newspack-plugin/tests/unit-tests/integrations/class-failing-sample-integration.php b/plugins/newspack-plugin/tests/unit-tests/integrations/class-failing-sample-integration.php index 45f856baab..476903bac8 100644 --- a/plugins/newspack-plugin/tests/unit-tests/integrations/class-failing-sample-integration.php +++ b/plugins/newspack-plugin/tests/unit-tests/integrations/class-failing-sample-integration.php @@ -25,6 +25,21 @@ class Failing_Sample_Integration extends Integration { */ public static $push_count = 0; + /** + * Count of pull_contact_data calls. + * + * @var int + */ + public static $pull_count = 0; + + /** + * Value returned by is_set_up(). Tests that simulate an + * enabled-but-unconfigured integration set this to false. + * + * @var bool + */ + public static $is_set_up_value = true; + /** * Register settings fields (test implementation). */ @@ -56,9 +71,19 @@ public function push_contact_data( $contact, $context = '', $existing_contact = * @return array */ public function pull_contact_data( $user_id ) { + self::$pull_count++; return []; } + /** + * Whether this integration's external prerequisites are configured. + * + * @return bool + */ + public function is_set_up() { + return self::$is_set_up_value; + } + /** * Whether contacts can be synced. * @@ -82,7 +107,9 @@ public function get_available_incoming_fields() { * Reset state between tests. */ public static function reset() { - self::$should_fail = false; - self::$push_count = 0; + self::$should_fail = false; + self::$push_count = 0; + self::$pull_count = 0; + self::$is_set_up_value = true; } } diff --git a/plugins/newspack-plugin/tests/unit-tests/integrations/class-sample-integration.php b/plugins/newspack-plugin/tests/unit-tests/integrations/class-sample-integration.php index dcb9040d15..395a464dee 100644 --- a/plugins/newspack-plugin/tests/unit-tests/integrations/class-sample-integration.php +++ b/plugins/newspack-plugin/tests/unit-tests/integrations/class-sample-integration.php @@ -18,6 +18,22 @@ class Sample_Integration extends Integration { */ public static $handler_args = null; + /** + * Value returned by is_set_up(). Tests that simulate an + * enabled-but-unconfigured integration set this to false. + * + * @var bool + */ + public static $is_set_up_value = true; + + /** + * Error codes can_sync() should add when $return_errors is true. + * Tests use this to drive the health check into a failure path. + * + * @var string[] + */ + public static $can_sync_error_codes = []; + /** * Menu item declaration returned by get_my_account_menu_item(). * @@ -70,7 +86,23 @@ public function pull_contact_data( $user_id ) { * @return bool|WP_Error True if contacts can be synced, false otherwise. WP_Error if return_errors is true. */ public function can_sync( $return_errors = false ) { - return $return_errors ? new \WP_Error() : true; + if ( ! $return_errors ) { + return empty( self::$can_sync_error_codes ); + } + $errors = new \WP_Error(); + foreach ( self::$can_sync_error_codes as $code ) { + $errors->add( $code, sprintf( 'Mock can_sync error: %s', $code ) ); + } + return $errors; + } + + /** + * Whether this integration's external prerequisites are configured. + * + * @return bool + */ + public function is_set_up() { + return self::$is_set_up_value; } /** @@ -102,7 +134,9 @@ public function handle_test_event( $timestamp, $data, $client_id ) { * Reset captured state between tests. */ public static function reset() { - self::$handler_args = null; + self::$handler_args = null; + self::$is_set_up_value = true; + self::$can_sync_error_codes = []; } /** diff --git a/plugins/newspack-plugin/tests/unit-tests/integrations/class-test-integrations.php b/plugins/newspack-plugin/tests/unit-tests/integrations/class-test-integrations.php index 023c96e329..e11191200e 100644 --- a/plugins/newspack-plugin/tests/unit-tests/integrations/class-test-integrations.php +++ b/plugins/newspack-plugin/tests/unit-tests/integrations/class-test-integrations.php @@ -178,6 +178,37 @@ public function test_get_active_integrations() { $this->assertArrayNotHasKey( 'disabled', $active ); } + /** + * Test get_active_configured_integrations filters by is_set_up too. + * + * This is the central skip site for the integration-walking paths + * (health checks, sync push, pull). A regression here silently + * re-introduces the alert/retry flood on unconfigured integrations. + */ + public function test_get_active_configured_integrations_filters_by_is_set_up() { + $configured = new Sample_Integration( 'configured', 'Configured' ); + $unconfigured = new class( 'unconfigured', 'Unconfigured' ) extends Sample_Integration { + /** + * Force this mock to report itself as not yet set up. + * + * @return bool + */ + public function is_set_up() { + return false; + } + }; + + Integrations::register( $configured ); + Integrations::register( $unconfigured ); + Integrations::enable( 'configured' ); + Integrations::enable( 'unconfigured' ); + + $result = Integrations::get_active_configured_integrations(); + + $this->assertArrayHasKey( 'configured', $result, 'A set-up integration must be included.' ); + $this->assertArrayNotHasKey( 'unconfigured', $result, 'An unconfigured integration must be excluded.' ); + } + /** * Test get_available_integrations returns all registered. */ @@ -823,6 +854,252 @@ public function test_connection() { $this->assertEquals( 'Fatal: something exploded', $result->get_error_message() ); } + /** + * Test run_health_checks skips integrations that are not yet set up. + * + * An enabled-but-unconfigured integration (e.g. ESP enabled by default + * but provider/master list never selected) is a setup-incomplete state, + * not a runtime incident — the hourly cron must not generate an alert. + */ + public function test_run_health_checks_skips_unconfigured_integrations() { + $integration = new Sample_Integration( 'unconfigured', 'Unconfigured' ); + Integrations::register( $integration ); + Integrations::enable( 'unconfigured' ); + + Sample_Integration::$is_set_up_value = false; + Sample_Integration::$can_sync_error_codes = [ 'ras_esp_master_list_id_not_found' ]; + + $fired = false; + $listener = function () use ( &$fired ) { + $fired = true; + }; + add_action( 'newspack_integration_health_check_failed', $listener ); + + try { + Integrations::run_health_checks(); + $this->assertFalse( $fired, 'health_check_failed action must not fire when is_set_up() is false.' ); + } finally { + remove_action( 'newspack_integration_health_check_failed', $listener ); + } + } + + /** + * Test run_health_checks fires the action for set-up integrations whose + * health check fails — the control case for the skip behavior above. + */ + public function test_run_health_checks_fires_when_set_up_and_failing() { + $integration = new Sample_Integration( 'failing', 'Failing' ); + Integrations::register( $integration ); + Integrations::enable( 'failing' ); + + Sample_Integration::$is_set_up_value = true; + Sample_Integration::$can_sync_error_codes = [ 'ras_esp_master_list_id_not_found' ]; + + $payload = null; + $listener = function ( $data ) use ( &$payload ) { + $payload = $data; + }; + add_action( 'newspack_integration_health_check_failed', $listener ); + + try { + Integrations::run_health_checks(); + $this->assertNotNull( $payload, 'health_check_failed action must fire when is_set_up() is true and health_check fails.' ); + $this->assertSame( 'failing', $payload['integration_id'] ); + $this->assertInstanceOf( \WP_Error::class, $payload['error'] ); + $this->assertContains( 'ras_esp_master_list_id_not_found', $payload['error']->get_error_codes() ); + } finally { + remove_action( 'newspack_integration_health_check_failed', $listener ); + } + } + + /** + * Test Contact_Pull::pull_sync defaults to set-up integrations only. + * + * The synchronous loopback path (run from Contact_Cron when the user's + * last pull is stale) must NOT fire a loopback for an integration whose + * `is_set_up()` is false — otherwise it both wastes a blocking remote + * call and logs a spurious failure. + */ + public function test_pull_sync_skips_unconfigured_integrations() { + $user_id = $this->factory()->user->create(); + wp_set_current_user( $user_id ); + + $configured = new class( 'sync-configured', 'Sync Configured' ) extends Sample_Integration { + /** + * Pull mock returning data so the loopback would succeed if reached. + * + * @param int $user_id WordPress user ID. + * @return array + */ + public function pull_contact_data( $user_id ) { + return [ 'favorite_color' => 'green' ]; + } + }; + $configured->update_enabled_incoming_fields( [ 'favorite_color' ] ); + Integrations::register( $configured ); + Integrations::enable( 'sync-configured' ); + + $unconfigured = new class( 'sync-unconfigured', 'Sync Unconfigured' ) extends Sample_Integration { + /** + * Force this mock to report itself as not yet set up. + * + * @return bool + */ + public function is_set_up() { + return false; + } + /** + * Pull mock that would return data if reached — must not be called. + * + * @param int $user_id WordPress user ID. + * @return array + */ + public function pull_contact_data( $user_id ) { + return [ 'favorite_color' => 'red' ]; + } + }; + $unconfigured->update_enabled_incoming_fields( [ 'favorite_color' ] ); + Integrations::register( $unconfigured ); + Integrations::enable( 'sync-unconfigured' ); + + // Capture which integration_ids the loopback receives. + $loopback_hits = []; + $this->loopback_filter = function ( $preempt, $parsed_args, $url ) use ( &$loopback_hits ) { + if ( false === strpos( $url, 'action=' . Contact_Pull::AJAX_ACTION ) ) { + return $preempt; + } + $loopback_hits[] = $parsed_args['body']['integration_id'] ?? ''; + return [ + 'response' => [ 'code' => 200 ], + 'body' => '{"success":true}', + ]; + }; + add_filter( 'pre_http_request', $this->loopback_filter, 10, 3 ); + + Contact_Pull::pull_sync(); + + $this->assertContains( 'sync-configured', $loopback_hits, 'Configured integration must receive a loopback.' ); + $this->assertNotContains( 'sync-unconfigured', $loopback_hits, 'Unconfigured integration must NOT receive a loopback.' ); + } + + /** + * Test Contact_Pull::pull_all skips integrations whose is_set_up() is false. + * + * Mirrors the run_health_checks skip: an unconfigured integration must not + * be invoked, must not generate retry rows in ActionScheduler. + */ + public function test_pull_all_skips_unconfigured_integrations() { + if ( ! function_exists( 'as_schedule_single_action' ) ) { + $this->markTestSkipped( 'ActionScheduler not available.' ); + } + + $integration = new class( 'pull-skip', 'Pull Skip' ) extends Sample_Integration { + /** + * Count of pull_contact_data calls. + * + * @var int + */ + public static $pull_count = 0; + + /** + * Pull mock that counts invocations. + * + * @param int $user_id WordPress user ID. + * @return array + */ + public function pull_contact_data( $user_id ) { + self::$pull_count++; + return [ 'favorite_color' => 'blue' ]; + } + }; + $integration::$pull_count = 0; + $integration->update_enabled_incoming_fields( [ 'favorite_color' ] ); + + Sample_Integration::$is_set_up_value = false; + + Integrations::register( $integration ); + Integrations::enable( 'pull-skip' ); + + \as_unschedule_all_actions( Contact_Pull::RETRY_HOOK ); + + $user_id = $this->factory()->user->create(); + + Contact_Pull::pull_all( $user_id ); + + $this->assertSame( 0, $integration::$pull_count, 'pull_contact_data must not be called when is_set_up() is false.' ); + + $pending = \as_get_scheduled_actions( + [ + 'hook' => Contact_Pull::RETRY_HOOK, + 'group' => Integrations::get_action_group( 'pull-skip' ), + 'status' => \ActionScheduler_Store::STATUS_PENDING, + ], + 'ARRAY_A' + ); + $this->assertEmpty( $pending, 'No pull retry should be scheduled for an unconfigured integration.' ); + } + + /** + * Test Contact_Pull::execute_integration_retry aborts when the integration + * is no longer set up — drains existing retry rows without scheduling more. + */ + public function test_pull_execute_integration_retry_aborts_when_not_set_up() { + if ( ! function_exists( 'as_schedule_single_action' ) ) { + $this->markTestSkipped( 'ActionScheduler not available.' ); + } + + $integration = new class( 'pull-retry-abort', 'Pull Retry Abort' ) extends Sample_Integration { + /** + * Count of pull_contact_data calls. + * + * @var int + */ + public static $pull_count = 0; + + /** + * Pull mock returning WP_Error so retry would be scheduled if reached. + * + * @param int $user_id WordPress user ID. + * @return \WP_Error + */ + public function pull_contact_data( $user_id ) { + self::$pull_count++; + return new \WP_Error( 'mock_error', 'Mock pull failed' ); + } + }; + $integration::$pull_count = 0; + $integration->update_enabled_incoming_fields( [ 'favorite_color' ] ); + + Sample_Integration::$is_set_up_value = false; + + Integrations::register( $integration ); + Integrations::enable( 'pull-retry-abort' ); + + \as_unschedule_all_actions( Contact_Pull::RETRY_HOOK ); + + $user_id = $this->factory()->user->create(); + + Contact_Pull::execute_integration_retry( + [ + 'integration_id' => 'pull-retry-abort', + 'user_id' => $user_id, + 'retry_count' => 1, + ] + ); + + $this->assertSame( 0, $integration::$pull_count, 'pull_contact_data must not be called when is_set_up() returns false at retry time.' ); + + $pending = \as_get_scheduled_actions( + [ + 'hook' => Contact_Pull::RETRY_HOOK, + 'group' => Integrations::get_action_group( 'pull-retry-abort' ), + 'status' => \ActionScheduler_Store::STATUS_PENDING, + ], + 'ARRAY_A' + ); + $this->assertEmpty( $pending, 'No new pull retry should be scheduled when integration becomes unconfigured mid-chain.' ); + } + /** * Test get_metadata_prefix returns default 'NP_' when no custom prefix is set. */ diff --git a/plugins/newspack-plugin/tests/unit-tests/reader-activation-sync.php b/plugins/newspack-plugin/tests/unit-tests/reader-activation-sync.php index a7962d3d04..254247775d 100644 --- a/plugins/newspack-plugin/tests/unit-tests/reader-activation-sync.php +++ b/plugins/newspack-plugin/tests/unit-tests/reader-activation-sync.php @@ -98,6 +98,46 @@ public function test_esp_integration_checks() { $this->assertFalse( $errors->has_errors(), 'No errors should be returned with a force constant' ); } + /** + * Test that ESP::is_set_up() reads stored configuration only — never makes + * a live provider call. Protects every gate that consults is_set_up() + * (Integrations::get_active_configured_integrations and the retry-time + * guards in Contact_Sync / Contact_Pull) from silently dropping traffic + * on transient ESP failures, which the AS retry system is meant to survive. + */ + public function test_esp_is_set_up_reads_stored_state() { + $esp = new Integrations\ESP(); + + // Master list ID not stored → setup is incomplete. + $esp->update_settings_field_value( 'mailchimp_audience_id', '' ); + $this->assertFalse( $esp->is_set_up(), 'is_set_up() must be false when master list ID is not stored.' ); + + // Admin selects a list → setup is complete. + $esp->update_settings_field_value( 'mailchimp_audience_id', '123' ); + $this->assertTrue( $esp->is_set_up(), 'is_set_up() must be true when provider + master list are stored.' ); + } + + /** + * Test that ESP::is_set_up() returns false when no provider is selected — + * the exact default-state scenario the hotfix targets (ESP auto-enabled on + * fresh installs while `newspack_newsletters_service_provider` is unset). + */ + public function test_esp_is_set_up_returns_false_when_no_provider_selected() { + $esp = new Integrations\ESP(); + $esp->update_settings_field_value( 'mailchimp_audience_id', '123' ); + + // Even with a master list stored, an unconfigured provider must short-circuit. + \Newspack_Newsletters::$is_service_provider_configured = false; + try { + $this->assertFalse( + $esp->is_set_up(), + 'is_set_up() must be false when no provider is selected, even if a master list ID is stored.' + ); + } finally { + \Newspack_Newsletters::reset_calls(); + } + } + /** * Test contact data sync to ESP. */ @@ -576,6 +616,55 @@ function ( $data ) use ( &$hook_fired, &$hook_data ) { $this->assertArrayHasKey( 'reason', $hook_data ); } + /** + * Test that execute_integration_retry aborts the retry chain when the + * integration becomes unconfigured between schedule and execute — drains + * existing flood without scheduling further attempts. + * + * The initial-push gate at Contact_Sync::push_to_integrations is the + * structural twin of Integrations::run_health_checks's gate (tested in + * class-test-integrations.php) and Contact_Pull::pull_all's gate (also + * tested there); a direct reflection-based unit test on push_to_integrations + * is not feasible because the iteration also touches the live ESP + * integration whose push_contact_data hits Newspack_Newsletters_Contacts + * (not loaded in the unit-test env). + */ + public function test_execute_integration_retry_aborts_when_not_set_up() { + if ( ! function_exists( 'as_schedule_single_action' ) ) { + $this->markTestSkipped( 'ActionScheduler not available.' ); + } + + Failing_Sample_Integration::reset(); + Failing_Sample_Integration::$should_fail = true; + Failing_Sample_Integration::$is_set_up_value = false; + $this->register_failing_integration( 'retry_abort_mock' ); + + as_unschedule_all_actions( Contact_Sync::RETRY_HOOK ); + + $user_id = $this->factory()->user->create( [ 'user_email' => 'retry-abort@test.com' ] ); + + Contact_Sync::execute_integration_retry( + [ + 'integration_id' => 'retry_abort_mock', + 'user_id' => $user_id, + 'context' => 'Test', + 'retry_count' => 1, + ] + ); + + $this->assertSame( 0, Failing_Sample_Integration::$push_count, 'push_contact_data must not be called when is_set_up() returns false at retry time.' ); + + $pending = as_get_scheduled_actions( + [ + 'hook' => Contact_Sync::RETRY_HOOK, + 'group' => Integrations::get_action_group( 'retry_abort_mock' ), + 'status' => \ActionScheduler_Store::STATUS_PENDING, + ], + 'ARRAY_A' + ); + $this->assertEmpty( $pending, 'No new retry should be scheduled when integration becomes unconfigured mid-chain.' ); + } + /** * Test that invalid retry data is handled gracefully. */