Skip to content

fix(popups): drop disabled criteria from segment criteria array #209

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
dkoo wants to merge 3 commits into
base: release
Choose a base branch
from
+327 −2
Open

fix(popups): drop disabled criteria from segment criteria array #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67b68a3
fix(popups): drop disabled criteria from segment criteria array
dkoo Jun 3, 2026
67f404a
Potential fix for pull request finding
dkoo Jun 3, 2026
db94622
fix(popups): make criteria filter extensible and handle booleans
dkoo Jun 3, 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
114 changes: 112 additions & 2 deletions plugins/newspack-popups/includes/class-newspack-segments-model.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Comment thread
dkoo marked this conversation as resolved.
* @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;
}

/**
Expand Down Expand Up @@ -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();
Expand Down
215 changes: 215 additions & 0 deletions plugins/newspack-popups/tests/test-segments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'] );
}
}
Loading