diff --git a/plugins/newspack-popups/includes/class-newspack-segments-model.php b/plugins/newspack-popups/includes/class-newspack-segments-model.php index ab09758eb2..ca6ae0bc77 100644 --- a/plugins/newspack-popups/includes/class-newspack-segments-model.php +++ b/plugins/newspack-popups/includes/class-newspack-segments-model.php @@ -518,7 +518,117 @@ public static function get_segment_from_term( WP_Term $segment ) { } // Ensure we got the segment in its latest version. - return Newspack_Segments_Migration::migrate_criteria_configuration( $segment ); + $segment = Newspack_Segments_Migration::migrate_criteria_configuration( $segment ); + + // Drop disabled criteria (e.g. number-based criteria the editor saved with {min:0, max:0}). + if ( isset( $segment['criteria'] ) ) { + $segment['criteria'] = self::filter_criteria( $segment['criteria'] ); + } + + return $segment; + } + + /** + * Filter out disabled criteria from a segment's criteria array. + * + * A criterion is considered disabled when its value carries no constraint + * (`null`, empty string, string/numeric `0`, boolean `false`, empty array, + * or a nested array whose entries are all themselves empty by these + * rules). This prevents disabled number-based criteria like + * `{ min: 0, max: 0 }` from leaking through to the front-end criteria + * array and being evaluated as active constraints. + * + * Extensibility: + * - `newspack_popups_is_criteria_value_empty` — short-circuit the + * per-value emptiness check (return a non-null boolean to override). + * - `newspack_popups_filter_segment_criteria` — final filter over the + * returned criteria array, e.g. for custom criteria that need to + * survive the default filter. + * + * @param mixed $criteria Criteria array from segment meta. Non-arrays return []. + * @return array Filtered criteria, re-indexed. + */ + public static function filter_criteria( $criteria ) { + if ( ! is_array( $criteria ) ) { + return []; + } + $filtered = array_values( + array_filter( + $criteria, + function( $item ) { + return is_array( $item ) + && isset( $item['criteria_id'] ) + && ! self::is_criteria_value_empty( $item['value'] ?? null ); + } + ) + ); + + /** + * Filters the criteria array after disabled entries have been stripped. + * + * Lets custom criteria registered via `newspack_popups_default_criteria` + * opt out of, or override, the default filtering — e.g. a criterion + * whose semantically-meaningful value is `0` / `''` / `false`. + * + * @param array $filtered Criteria after the default empty-value filter. + * @param array $criteria Raw criteria as received. + */ + return apply_filters( 'newspack_popups_filter_segment_criteria', $filtered, $criteria ); + } + + /** + * Determine whether a criterion value represents "no constraint". + * + * Walks nested arrays so fully-disabled values (e.g. `{ min: 0, max: 0 }`) + * are recognised as empty. Booleans are treated as empty when `false` — + * mirroring the convention used by the existing `is_disabled` toggles — + * so custom boolean criteria don't need bespoke handling. + * + * @param mixed $value Criterion value. + * @return bool True if the value carries no constraint. + */ + private static function is_criteria_value_empty( $value ) { + /** + * Short-circuit the per-value emptiness check for `filter_criteria()`. + * + * Return a boolean to override; return `null` (default) to fall through + * to the built-in rules. Useful when a custom criterion treats a + * normally-empty value (e.g. `0`, `''`, `false`) as meaningful. + * + * @param mixed $value The criterion value being evaluated. + */ + $override = apply_filters( 'newspack_popups_is_criteria_value_empty', null, $value ); + if ( is_bool( $override ) ) { + return $override; + } + + if ( null === $value ) { + return true; + } + if ( is_bool( $value ) ) { + return false === $value; + } + if ( is_string( $value ) ) { + return '' === $value || '0' === $value; + } + if ( is_int( $value ) ) { + return 0 === $value; + } + if ( is_float( $value ) ) { + return 0.0 === $value; + } + if ( is_array( $value ) ) { + if ( [] === $value ) { + return true; + } + foreach ( $value as $sub ) { + if ( ! self::is_criteria_value_empty( $sub ) ) { + return false; + } + } + return true; + } + return false; } /** @@ -579,7 +689,7 @@ public static function update_segment( $segment ) { } update_term_meta( $segment['id'], 'updated_at', gmdate( 'Y-m-d' ) ); - update_term_meta( $segment['id'], 'criteria', $segment['criteria'] ?? [] ); + update_term_meta( $segment['id'], 'criteria', self::filter_criteria( $segment['criteria'] ?? [] ) ); update_term_meta( $segment['id'], 'configuration', $segment['configuration'] ); return self::get_segments(); diff --git a/plugins/newspack-popups/tests/test-segments.php b/plugins/newspack-popups/tests/test-segments.php index a2aa1e5c26..0ed9329ce2 100644 --- a/plugins/newspack-popups/tests/test-segments.php +++ b/plugins/newspack-popups/tests/test-segments.php @@ -625,4 +625,219 @@ public function test_create_preserves_order() { $this->assertSame( $key, $last_segment['name'] ); } } + + /** + * Disabled number-based criteria (`{ min: 0, max: 0 }`) should be dropped + * from a segment's criteria so they never reach the front-end criteria + * array. Regression test for NPPM-2890. + */ + public function test_update_segment_drops_disabled_criteria() { + Newspack_Popups_Segmentation::create_segment( $this->complete_and_valid ); + $segment = Newspack_Popups_Segmentation::get_segments()[0]; + + $segment['criteria'] = [ + [ + 'criteria_id' => 'articles_read', + 'value' => [ + 'min' => 5, + 'max' => 20, + ], + ], + [ + 'criteria_id' => 'articles_read_in_session', + 'value' => [ + 'min' => 0, + 'max' => 0, + ], + ], + [ + 'criteria_id' => 'favorite_categories', + 'value' => [], + ], + [ + 'criteria_id' => 'newsletter', + 'value' => '', + ], + [ + 'criteria_id' => 'donation', + 'value' => 'donors', + ], + ]; + + $result = Newspack_Popups_Segmentation::update_segment( $segment ); + + $this->assertCount( + 2, + $result[0]['criteria'], + 'Only criteria with non-empty values should be persisted.' + ); + $criteria_ids = wp_list_pluck( $result[0]['criteria'], 'criteria_id' ); + $this->assertSame( [ 'articles_read', 'donation' ], $criteria_ids ); + + // Verify the raw term meta was written in its filtered form (not just the read path). + $raw_criteria = get_term_meta( $segment['id'], 'criteria', true ); + $this->assertCount( 2, $raw_criteria, 'Disabled criteria should not be persisted to term meta.' ); + $this->assertSame( [ 'articles_read', 'donation' ], wp_list_pluck( $raw_criteria, 'criteria_id' ) ); + } + + /** + * Already-stored disabled criteria should be filtered out on read so + * publishers don't have to re-save every segment to recover. + */ + public function test_get_segment_filters_legacy_disabled_criteria() { + Newspack_Popups_Segmentation::create_segment( $this->complete_and_valid ); + $segment_id = Newspack_Popups_Segmentation::get_segments()[0]['id']; + + // Simulate legacy data: criteria written directly to term meta, bypassing the save-time filter. + update_term_meta( + $segment_id, + 'criteria', + [ + [ + 'criteria_id' => 'articles_read', + 'value' => [ + 'min' => 5, + 'max' => 20, + ], + ], + [ + 'criteria_id' => 'articles_read_in_session', + 'value' => [ + 'min' => 0, + 'max' => 0, + ], + ], + [ + 'criteria_id' => 'sources_to_match', + 'value' => [], + ], + ] + ); + + $segment = Newspack_Popups_Segmentation::get_segment( $segment_id ); + + $this->assertCount( 1, $segment['criteria'] ); + $this->assertSame( 'articles_read', $segment['criteria'][0]['criteria_id'] ); + } + + /** + * Partially-disabled range criteria (one of min/max set) must be preserved. + */ + public function test_filter_criteria_preserves_partial_range() { + $criteria = [ + [ + 'criteria_id' => 'articles_read', + 'value' => [ + 'min' => 3, + 'max' => 0, + ], + ], + [ + 'criteria_id' => 'articles_read_in_session', + 'value' => [ + 'min' => 0, + 'max' => 5, + ], + ], + ]; + + $filtered = Newspack_Segments_Model::filter_criteria( $criteria ); + + $this->assertCount( 2, $filtered ); + } + + /** + * Criteria without a `criteria_id` or with non-array entries should be dropped. + */ + public function test_filter_criteria_drops_malformed_entries() { + $criteria = [ + 'not-an-array', + [ 'value' => 5 ], + [ + 'criteria_id' => 'articles_read', + 'value' => 5, + ], + ]; + + $filtered = Newspack_Segments_Model::filter_criteria( $criteria ); + + $this->assertCount( 1, $filtered ); + $this->assertSame( 'articles_read', $filtered[0]['criteria_id'] ); + } + + /** + * Boolean criterion values: `false` is treated as empty (dropped), + * `true` is preserved. + */ + public function test_filter_criteria_handles_booleans() { + $criteria = [ + [ + 'criteria_id' => 'custom_bool_active', + 'value' => true, + ], + [ + 'criteria_id' => 'custom_bool_inactive', + 'value' => false, + ], + ]; + + $filtered = Newspack_Segments_Model::filter_criteria( $criteria ); + + $this->assertSame( + [ 'custom_bool_active' ], + wp_list_pluck( $filtered, 'criteria_id' ), + '`true` should survive; `false` should be dropped.' + ); + } + + /** + * Third parties can register a custom criterion whose semantically-meaningful + * value is normally treated as empty (e.g. `0`). The + * `newspack_popups_is_criteria_value_empty` filter lets them opt out per-value. + */ + public function test_filter_criteria_value_empty_filter_can_override() { + $callback = function ( $override, $value ) { + // Treat integer `0` as a meaningful value for any criterion. + if ( 0 === $value ) { + return false; + } + return $override; + }; + add_filter( 'newspack_popups_is_criteria_value_empty', $callback, 10, 2 ); + + $filtered = Newspack_Segments_Model::filter_criteria( + [ + [ + 'criteria_id' => 'zero_donations', + 'value' => 0, + ], + ] + ); + + remove_filter( 'newspack_popups_is_criteria_value_empty', $callback, 10 ); + + $this->assertCount( 1, $filtered, 'Override should keep the otherwise-empty criterion.' ); + } + + /** + * Third parties can post-process the filtered criteria array via + * `newspack_popups_filter_segment_criteria` — e.g. to inject defaults. + */ + public function test_filter_segment_criteria_filter_runs() { + $callback = function ( $filtered ) { + $filtered[] = [ + 'criteria_id' => 'injected', + 'value' => 'yes', + ]; + return $filtered; + }; + add_filter( 'newspack_popups_filter_segment_criteria', $callback ); + + $filtered = Newspack_Segments_Model::filter_criteria( [] ); + + remove_filter( 'newspack_popups_filter_segment_criteria', $callback ); + + $this->assertCount( 1, $filtered ); + $this->assertSame( 'injected', $filtered[0]['criteria_id'] ); + } }