#2810 - Ability To Configure Which Fields To Search For Duplications On

[kodinkat]

• fixes: Ability to configure which fields to search for duplicates on. #2810

---

[corsacca]

@claude review?

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[corsacca]

Hey @kodinkat,

Could you look into (or clarify) these 2?

1. ids_of_non_dismissed_duplicates signature change is a breaking
   change (High)

In duplicates-merging.php, the endpoint now calls
self::ids_of_non_dismissed_duplicates( $post_type, $post_id, false ) —
changing from true (exact match) to false (fuzzy match). This is a
behavior change that affects all duplicate detection, not just the
configurability feature. This seems unintentional and would make
duplicate detection less precise globally.

2 remove Foundation defensive checks in modular-list.js and
contacts.js

[kodinkat]

Hey @kodinkat,

Could you look into (or clarify) these 2?

1. ids_of_non_dismissed_duplicates signature change is a breaking
   change (High)

In duplicates-merging.php, the endpoint now calls self::ids_of_non_dismissed_duplicates( $post_type, $post_id, false ) — changing from true (exact match) to false (fuzzy match). This is a behavior change that affects all duplicate detection, not just the configurability feature. This seems unintentional and would make duplicate detection less precise globally.

2 remove Foundation defensive checks in modular-list.js and contacts.js

@corsacca good spot re point (1); as this was changed in error. Latest updates pushed and review ready.....

[corsacca]

@kodinkat the field_settings for all post types could be directly available under dtOptionAPI. Then it can be used by other settings

[corsacca]

@kodinkat

[corsacca]

@kodinkat why the function existis?

[corsacca]

this page need to work on /wp-admin/admin.php?page=dt_options as well.

[github-actions]

Code Review

Thanks for implementing this feature - configurable duplicate detection fields is a valuable addition. Here are my findings:

---

Bugs / Functional Gaps

1. get_all_duplicates_on_post() scoring not updated for new field types

The scoring block in get_all_duplicates_on_post() (lines ~232-256 of duplicates-merging.php) still only handles text and communication_channel types. Now that tags, multi_select, textarea, and number can be added to the duplicate check config, those fields will be included in the search query via list_posts() but will not contribute any points to the duplicate score. The feature works for finding duplicates on those fields, but ordering/scoring is broken. The scoring logic needs to be extended to handle the same field types supported in extract_field_values_for_duplicate_search().

2. query_for_duplicate_searches_v2() is not updated

query_for_duplicate_searches_v2() is used by the "View Duplicates" bulk page (get_access_duplicates()). It still hard-codes only name and communication_channel fields, so the new configurability does not apply to that code path. Users who configure custom fields will see the feature work on individual record pages but not on the View Duplicates page.

---

Architecture / Code Quality

3. Form data processed inside dt_options_scripts() (enqueue function)

admin-enqueue-scripts.php now contains POST processing logic (nonce verification, JSON decoding, field sanitization). Enqueue hooks should only register/enqueue assets - this is not standard WordPress practice. This also creates duplicate logic: both admin-enqueue-scripts.php and tab-general.php::process_duplicate_fields() now process the same POST data independently. The enqueue function should only read the already-saved config from the database; let the tab class own all form processing.

4. wp_cache_delete() called on every page view

display_duplicate_fields_settings() unconditionally calls wp_cache_delete( 'dt_site_options', 'options' ) at the top of the function. This invalidates the options cache every time the General tab is viewed, degrading performance for all users on every page load. Cache-busting should only happen immediately after a save, not on every read.

5. DT_Posts::get_post_field_settings() called in a nested loop

In process_duplicate_fields() (tab-general.php around line 543), get_post_field_settings( $post_type ) is called inside the inner loop for each field key. This causes repeated database queries for the same post type within a single request. Move the call outside the inner loop.

---

Minor Issues

6. Missing translation wrapper

$this->box( 'top', 'Duplicate Detection Fields' );

The string 'Duplicate Detection Fields' is not wrapped in __( ..., 'disciple_tools' ). All user-facing strings must be translatable per the project's code style.

7. alert() in JavaScript

alert('Field selector not found');

Using alert() is poor UX and inconsistent with the rest of the admin UI. Consider console.error() or an inline error message element.

8. Indefinite setTimeout polling for custom element

initializeDuplicateFields() calls itself recursively via setTimeout(initializeDuplicateFields, 100) with no max retries or timeout limit. If the component script fails to load, this will poll indefinitely. The standard API for this pattern is customElements.whenDefined('dt-multi-select').then(...).

9. No enforcement that name is always included

Code comments note that name is important for all post types, but the save logic does not enforce this. A user can save a config without name, which may produce unexpected results. Consider enforcing it server-side or making it non-removable in the UI.

10. Unnecessary complexity in tags / multi_select extraction

The switch cases for tags and multi_select guard against both string and array elements. DT_Posts::get_post() returns these as simple arrays of strings, so the is_array branch is dead code that adds confusion.

---

Summary

The core concept is solid and the new extract_field_values_for_duplicate_search() helper is a clean refactor. The most important issues to address before merging:

1. Extend scoring in get_all_duplicates_on_post() to handle the newly supported field types
2. Update query_for_duplicate_searches_v2() to respect the configurable fields setting
3. Remove POST processing from the enqueue function - let the tab class own form handling
4. Remove unconditional cache invalidation in display_duplicate_fields_settings()

[github-actions]

Updated Code Review

Reviewing the current state of the PR. Several issues from the earlier automated review appear to be addressed — scoring in get_all_duplicates_on_post() is extended for new field types and query_for_duplicate_searches_v2() is now using the configurable fields. Good progress. Here are remaining issues:

---

Bug: query_for_duplicate_searches_v2() hardcodes 'contacts' post type for the name field

In duplicates-merging.php, the SQL for matching on the name / post_title field is:

WHERE p.post_type = 'contacts' AND p.post_title $op '...'

This should use $post_type (the function parameter). The other branches already do this correctly — e.g. the else branch at the end uses esc_sql( $post_type ). As written, if any non-contacts post type is configured to check the name field, the View Duplicates bulk page will silently search inside contacts instead of the correct post type.

Fix: replace hardcoded 'contacts' with esc_sql( $post_type ).

---

Performance: get_post_field_settings() called inside inner loop

In tab-general.php::process_duplicate_fields(), DT_Posts::get_post_field_settings( $post_type ) is called once per field key rather than once per post type. Move it outside the inner foreach.

---

Security: in_array() without strict comparison

if ( !in_array( $post_type, $post_types ) ) {

Should use strict mode to avoid type-coercion surprises:

if ( !in_array( $post_type, $post_types, true ) ) {

---

Fragile ^ prefix encoding

The ^ character is prepended to values to signal "exact match", then parsed back with strpos($search_val, '^') === 0. If any field value legitimately starts with ^, it would be misinterpreted. Since $exact is already a function parameter, the cleaner approach is to pass the flag separately rather than encoding it into the value strings.

---

Minor Issues

Missing translation in tab-general.php:

$this->box( 'top', 'Duplicate Detection Fields' );
// should be:
$this->box( 'top', __( 'Duplicate Detection Fields', 'disciple_tools' ) );

alert() in JavaScript (dt-options.js): Use console.error() or an inline DOM message instead.

Indefinite setTimeout polling: initializeDuplicateFields() calls itself via setTimeout with no bail-out. Use the standard API instead:

customElements.whenDefined('dt-multi-select').then(initializeDuplicateFields);

---

Summary

The hardcoded 'contacts' in query_for_duplicate_searches_v2() is the only outright bug remaining. The performance issue in process_duplicate_fields() and missing strict comparison in in_array() are easy to fix. Everything else is minor polish.

[github-actions]

Code Review

Good progress on this feature. Several issues flagged in prior reviews appear still unresolved in the current diff. Here is a consolidated review of what remains.

Bug (High): query_for_duplicate_searches_v2() hardcodes 'contacts' post type

In the PR diff, the rewritten name-field SQL branch still hardcodes the post type:

WHERE p.post_type = 'contacts' AND p.post_title ...

The function receives $post_type as a parameter and every other branch uses it correctly (e.g. the else block uses esc_sql( $post_type )). If any non-contacts post type is configured to check the name field, the View Duplicates bulk page will silently search inside contacts records instead.

Fix: replace the hardcoded string with esc_sql( $post_type ).

---

Performance: get_post_field_settings() called inside inner loop

In tab-general.php::process_duplicate_fields(), DT_Posts::get_post_field_settings( $post_type ) is called once per field key rather than once per post type. Move it to just before the inner foreach (outside it, inside the outer loop).

---

Security: Missing strict comparison in in_array()

if ( !in_array( $post_type, $post_types ) ) {

Should pass true as the third argument for strict comparison to avoid type-coercion surprises.

---

SQL: esc_sql() used for LIKE values instead of $wpdb->esc_like()

In query_for_duplicate_searches_v2(), fuzzy-match (LIKE) values are escaped with esc_sql() only. esc_sql() does not escape % and _ wildcard characters for SQL LIKE clauses. A field value containing % or _ will produce incorrect matches.

Use $wpdb->esc_like() before esc_sql() for LIKE values:

if ( $op === 'LIKE' ){
    $esc_val = '%' . esc_sql( $wpdb->esc_like( $val ) ) . '%';
}

This applies to both the name branch and the else branch in query_for_duplicate_searches_v2().

---

Minor: Missing translation wrapper

$this->box( 'top', 'Duplicate Detection Fields' );

Per the project code style, all user-facing strings must be translatable:

$this->box( 'top', __( 'Duplicate Detection Fields', 'disciple_tools' ) );

---

Minor: alert() in JavaScript

alert('Field selector not found');

Use console.error() or an inline DOM message instead — alert() is inconsistent with the rest of the admin UI.

---

Minor: Indefinite setTimeout polling

initializeDuplicateFields() recursively calls itself via setTimeout with no termination condition. If the component script fails to load this will poll indefinitely. The standard API for this is:

customElements.whenDefined('dt-multi-select').then(initializeDuplicateFields);

---

Summary

Priority	Issue
High	Hardcoded 'contacts' in query_for_duplicate_searches_v2() name-field SQL
Medium	get_post_field_settings() called in inner loop (performance)
Medium	in_array() missing strict mode
Medium	esc_sql() for LIKE values — should use $wpdb->esc_like() first
Minor	Missing __() translation wrapper on 'Duplicate Detection Fields'
Minor	alert() in JS
Minor	Unbounded setTimeout polling

The hardcoded 'contacts' bug and the esc_like() issue are the most important to address before merge.

[github-actions]

Code Review — PR #2882: Configurable Duplicate Detection Fields

Good feature addition overall. The refactoring of extract_field_values_for_duplicate_search() into its own method is a clean improvement, and the scoring block in get_all_duplicates_on_post() is correctly extended to handle the new field types. Here are the issues I found, from most to least critical:

---

Bugs / Correctness

1. Missing post_type filter in V2 SQL for communication_channel

In query_for_duplicate_searches_v2(), the UNION block for communication_channel fields does not include AND p.post_type = ..., while the name and postmeta blocks do. This means the communication channel search could return matches from other post types (e.g., a group with a matching phone meta), producing false positives.

// communication_channel block - missing post_type filter:
$all_sql .= '... FROM ' . $wpdb->posts . ' p
    LEFT JOIN ... INNER JOIN ... as type ON ( ... AND type.meta_value = \'access\' )
    WHERE ( ' . implode( ' OR ', $where_parts ) . ' )
    AND p.ID != ' . (int) $post_id;
    // ^^^ no "AND p.post_type = ..." here, unlike the other blocks

2. Fuzzy matching on number fields is semantically wrong

When $exact = false, number fields get searched with the LIKE operator (no ^ prefix in extract_field_values_for_duplicate_search()). This means the number 123 would match any post with 1234, 9123, etc. in that field. Numbers should always use exact matching for duplicate detection.

---

Performance

3. DT_Posts::get_post_field_settings() called inside inner loop

In process_duplicate_fields() (tab-general.php), get_post_field_settings() is called on every iteration of the $fields loop:

foreach ( $fields as $field_key ) {
    $sanitized_field_key = sanitize_key( $field_key );
    $field_settings = DT_Posts::get_post_field_settings( $post_type ); // called N times!
    if ( isset( $field_settings[$sanitized_field_key] ) ) { ...

$field_settings doesn't change per-iteration and should be fetched once before the loop.

4. dt_get_option('dt_site_options') fetched on every duplicate check

Both query_for_duplicate_searches() and query_for_duplicate_searches_v2() now call dt_get_option('dt_site_options') at the start. These functions are called per-post on duplicate detection. The configured fields could be statically cached or accepted as a parameter to avoid repeated option lookups.

5. wp_cache_delete('alloptions', 'options') after save

This line (in process_duplicate_fields()) is too aggressive. Flushing the alloptions group forces all WordPress options to be re-read from the database on the next request, affecting every plugin and core function — not just dt_site_options. update_option() already handles its own cache invalidation; the custom wp_cache_delete calls are unnecessary and the alloptions one is harmful.

---

Code Quality

6. Non-translatable string in box() call

$this->box( 'top', 'Duplicate Detection Fields' );

The label should go through __():

$this->box( 'top', __( 'Duplicate Detection Fields', 'disciple_tools' ) );

7. JS: Polling instead of customElements.whenDefined()

// Current approach — polling every 100ms indefinitely:
setTimeout(initializeDuplicateFields, 100);

// Better:
customElements.whenDefined('dt-multi-select').then(initializeDuplicateFields);

The polling loop has no timeout/escape condition, so if the component fails to load it will poll forever.

8. JS: Nested $(document).ready() inside outer $(document).ready()

Line 673 calls $(document).ready() inside the already-executing outer jQuery(document).ready() block. The inner call is redundant — the document is already ready. Just call initializeDuplicateFields() directly.

9. JS: alert() for error feedback

alert('Field selector not found');

This is inconsistent with the rest of the admin UI. Consider using a visible inline error message or the existing admin notice pattern instead.

---

Minor / PHPCS

10. Missing semicolons before ?> in template

Several PHP statements in display_duplicate_fields_settings() are missing closing semicolons:

<?php esc_html_e( 'Select fields...', 'disciple_tools' ) ?>
//                                                        ^ missing ;

PHPCS will flag these. Add ; before each closing ?>.

---

Summary

#	Severity	Area
1	High	Missing post_type filter in V2 SQL for communication_channel
2	Medium	Fuzzy number matching produces false positives
3	Medium	get_post_field_settings() in inner loop
4	Low	dt_get_option() on every duplicate check
5	Medium	wp_cache_delete('alloptions') is too aggressive
6	Low	Non-translatable string
7–9	Low	JS quality issues
10	Low	PHPCS / missing semicolons

The core logic for configurable fields is sound. Issues 1, 2, and 5 are the ones worth prioritizing before merge.

[kodinkat]

@corsacca review ready....

[github-actions]

Code Review — PR #2882 (updated round)

Good progress on the fixes. Here's a focused review of what remains.

Bugs Still Outstanding

1. Missing post_type filter in V2 SQL for communication_channel (High)

The communication_channel UNION block in query_for_duplicate_searches_v2() still lacks the post type filter that every other block has. This can return false-positive matches from other post types that happen to store a meta value under the same field key.

The name and else (postmeta) blocks both include AND p.post_type = '...', but the communication_channel block only filters on type.meta_value = 'access' with no post type clause. Add AND p.post_type = esc_sql($post_type) before AND p.ID != in that block.

2. get_post_field_settings() called inside an inner loop (Medium)

In process_duplicate_fields() (tab-general.php), DT_Posts::get_post_field_settings( $post_type ) is called on every iteration of the inner $fields loop. Move it above the inner foreach:

// Fix (1 call per post type):
$field_settings = DT_Posts::get_post_field_settings( $post_type );
foreach ( $fields as $field_key ) {
    $sanitized_field_key = sanitize_key( $field_key );
    if ( isset( $field_settings[$sanitized_field_key] ) ) { ...

3. wp_cache_delete( 'alloptions', 'options' ) is too aggressive (Medium)

Both wp_cache_delete calls in process_duplicate_fields() are unnecessary — update_option() already handles its own cache invalidation. The alloptions deletion forces WordPress to re-read all options from the database on the very next request, affecting every plugin and core function, not just dt_site_options. Remove both lines.

4. in_array() missing strict mode in POST validation (Minor)

Change: if ( !in_array( $post_type, $post_types ) ) {
To: if ( !in_array( $post_type, $post_types, true ) ) {

Minor / Code Style

5. Non-translatable string in box() call

Change: $this->box( 'top', 'Duplicate Detection Fields' );
To: $this->box( 'top', __( 'Duplicate Detection Fields', 'disciple_tools' ) );

6. Missing semicolons before closing PHP tags (PHPCS)

Multiple statements in display_duplicate_fields_settings() are missing ; before the closing ?> tag, e.g. esc_html_e( '...', 'disciple_tools' ) ?> should end with ; ?>. PHPCS will flag these.

7. JS: alert() for an internal error

alert('Field selector not found') blocks the page. Use console.error() instead — consistent with how errors are surfaced elsewhere in the admin UI.

8. JS: Nested document.ready() is redundant

The document.ready() at the bottom of the duplicate fields block is already inside the outer jQuery(document).ready() that wraps the entire dt-options.js file. The document is already ready at that point — call initializeDuplicateFields() directly instead.

9. JS: Unbounded setTimeout polling

initializeDuplicateFields() will loop indefinitely if the web component never loads. Use the standard platform API instead:

customElements.whenDefined('dt-multi-select').then(initializeDuplicateFields);

What's Fixed

• ids_of_non_dismissed_duplicates exact/fuzzy parameter reverted to true
• Scoring block in get_all_duplicates_on_post() extended for tags, multi_select, textarea, number
• query_for_duplicate_searches_v2() now uses the configured fields
• Hardcoded 'contacts' in V2 SQL name block replaced with $post_type
• esc_like() used correctly for LIKE values in V2 SQL
• Unit tests added with good field-type coverage

Summary

Priority	Issue
High	Missing post_type filter in V2 SQL communication_channel block
Medium	get_post_field_settings() called N times in inner loop
Medium	wp_cache_delete('alloptions') too aggressive — remove both cache delete calls
Minor	in_array() strict mode, translation string, PHPCS semicolons, JS quality

Issue 1 is the main one to address before merge.

[github-actions]

PR #2882 Review — Configurable Duplicate Detection Fields

This is a well-structured feature addition. The previous review's highest-priority issues appear to have been addressed. Here's the current state:

---

Fixed Since Previous Review ✅

• Scoring in get_all_duplicates_on_post() — now correctly handles text, textarea, number, tags, and multi_select types.
• query_for_duplicate_searches_v2() — now reads from the configurable fields instead of hard-coding only name and communication_channel.
• POST processing moved out of enqueue function — admin-enqueue-scripts.php now only reads from the DB; form handling stays in the tab class.
• Cache invalidation — wp_cache_delete() is now only called inside process_duplicate_fields() after a verified save, not on every page view.

---

Still Present / New Issues

1. (Medium) get_post_field_settings() called inside inner loop

In tab-general.php::process_duplicate_fields(), DT_Posts::get_post_field_settings( $post_type ) is called for every single field key in the inner loop, causing redundant DB queries per save:

foreach ( $fields as $field_key ) {
    $sanitized_field_key = sanitize_key( $field_key );
    $field_settings = DT_Posts::get_post_field_settings( $post_type ); // <-- called N times per post type

Move this outside the inner loop (but inside the outer loop) to call it once per post type.

2. (Medium) query_for_duplicate_searches_v2() uses a contacts-specific JOIN for all post types

The v2 SQL queries all include INNER JOIN $wpdb->postmeta as type ON (... meta_key = 'type' AND meta_value = 'access'). This filter is specific to the contacts post type (which distinguishes 'access' vs 'user' contact types). For other post types like groups that don't have this meta, the JOIN acts as a filter that returns zero rows. The View Duplicates bulk page will silently show no results for non-contacts post types even when duplicates exist.

3. (Low) Missing translation wrapper

$this->box( 'top', 'Duplicate Detection Fields' );

Per project conventions, user-facing strings must use __( 'Duplicate Detection Fields', 'disciple_tools' ).

4. (Low) alert() in JavaScript

alert('Field selector not found');

Using alert() is inconsistent with the rest of the admin UI. Replace with console.error() or an inline error notice.

5. (Low) Indefinite setTimeout polling for custom element

initializeDuplicateFields() recurses via setTimeout with no cap on retries. If the component script never loads, this polls forever. Use the standard API instead:

customElements.whenDefined('dt-multi-select').then(() => {
    updateFieldSelector(selectedPostType);
});

6. (Low) Saving an empty field selection is silently ignored

In process_duplicate_fields(), the save is skipped when the sanitized field array is empty:

if ( !empty( $sanitized_fields ) ) {
    $duplicates_config[$post_type] = array_unique( $sanitized_fields );
}

If a user deliberately clears all fields for a post type and saves, the config for that post type is not updated — the old value persists. The behavior is inconsistent with the intent; the UI should either prevent saving an empty selection (e.g., require at least one field) or save the empty array so the backend falls back to defaults as documented.

---

Summary

The core refactor is clean and the test coverage is comprehensive. The two items worth fixing before merging are the contacts-specific JOIN leaking into all post types in query_for_duplicate_searches_v2() (issue #2) and the get_post_field_settings() call inside the inner loop (issue #1). The remaining items are low-priority polish.