#2895 - Bulk Edit - User Select Typeahead

[kodinkat]

• fixes: Bulk Edit: User select typeahead #2895

---

[github-actions]

PR Review: Bulk Edit - User Select Typeahead

Good direction overall. Removing ~113 net lines of custom typeahead wiring in favour of the existing dt-users-connection web component is the right call, and the screenshot confirms the UI renders correctly.

A few items worth discussing before merge:

---

Potential Bug: Value format mismatch in conditional removal

In do_each() (around line 244), the conditional removal for user_select compares the record's assigned-to field value (always a plain string like "user-5") against valueToRemove, which before this PR was also set via the typeahead as "user-5".

After this PR the special-case branch in collectFieldValue() is removed. Execution now falls through to the generic path that calls ComponentService.convertValue(component.tagName, component.value). If that function returns an object/array for dt-users-connection (as it does for dt-connection), the strict equality check currentValue === valueToRemove will always be false, silently breaking the "remove if current value matches" flow.

Please confirm what ComponentService.convertValue returns for dt-users-connection in single mode, or add a normalisation step inside the user_select branch of do_each() similar to the key_select handling already present there.

---

Potential Bug: Update payload format

The same concern applies when a value is being set (not removed). For all non-special-cased field types the update payload builder falls into the generic else branch:

updatePayload[fieldKey] = fieldValue;

The DT backend's user_select handler expects the value in "user-{id}" format. If ComponentService.convertValue wraps the id in {values: [...]}, the API call will silently send the wrong shape and the field won't be updated. Worth confirming the round-trip or documenting the expected format.

---

Minor: initializeBulkEditFieldHandlers is now a comment-only stub

The function still exists but its body is only comments. If no per-type work is needed any more, removing the function entirely would be cleaner. If it is kept as a future extension point, that is fine — just worth a note.

---

Minor: hasValues check after component switch

In handleModeToggleChange(), user_select falls into:

} else if (fieldType === 'key_select' || fieldType === 'user_select') {
    hasValues = true;
}

This was safe when currentValue was always a non-empty string. Now that it comes from ComponentService.convertValue(), please verify that an empty component (nothing selected) returns null/undefined/"" rather than an empty object or array — otherwise hasValues could be true when nothing is selected, showing a misleading "will be removed" panel.

---

Positive observations

• The defensive multi-shape handling added to renderValuesToRemoveDisplay() (array / object / string) is sensible.
• The _list.scss addition of dt-users-connection label to the shared label selector is correct and necessary.
• Net -113 lines is a meaningful complexity reduction and aligns with the goal in issue Bulk Edit: User select typeahead #2895.

---

Summary: The UI and architecture look right. The main open question is whether ComponentService.convertValue('DT-USERS-CONNECTION', value) with the single attribute produces the "user-{id}" string the backend and the do_each() conditional-removal comparison both expect. Confirming (or fixing) that would make this safe to merge.

[github-actions]

Code Review: #2895 - Bulk Edit - User Select Typeahead

The overall direction is correct — replacing the custom typeahead with dt-users-connection reduces complexity and aligns with the component system used everywhere else. One critical bug needs to be fixed before merge.

---

Critical Bug: normalizeUserSelectValue doesn't handle number primitives

ComponentService.convertValue('DT-USERS-CONNECTION', value) — the function called inside collectFieldValue() — returns a plain integer for a selected user, not a string or object. This is confirmed in dt-assets/build/components/index.js:

case "dt-users-connection": {
    const n = [], r = s.filter(c => !c.delete);
    if (r.length <= 1) {
        s = r.length === 1 ? parseInt(r[0].id, 10) : "";
        break;
    }
}

normalizeUserSelectValue checks typeof string → Array.isArray → typeof object and then falls through to return null for a number. This silently breaks three things:

1. Update payload is always empty for user_select

// fieldValue = 5 (integer from ComponentService.convertValue)
const normalizedUser = normalizeUserSelectValue(5); // → null
if (normalizedUser !== null) {                      // never entered
    updatePayload[fieldKey] = normalizedUser;
}

A selected user is never saved to the API.

2. hasValues is always false in remove mode

normalizedUserValueToRemove = normalizeUserSelectValue(currentValue); // 5 → null
hasValues = !!normalizedUserValueToRemove;                            // false

Even when a user is selected, the toggle always falls into the "clear" path instead of the "selective removal" path.

3. Conditional removal in do_each() never matches
valueToRemove in _conditionalRemovals is set to null (from normalizedUserValueToRemove), so currentValue === valueToRemove is always false.

Fix: add a number check to normalizeUserSelectValue:

if (typeof rawValue === 'number') {
    return rawValue > 0 ? `user-${rawValue}` : null;
}

---

Minor: unsafe string fallback in normalizeUserSelectValue

When rawValue is a string that doesn't start with 'user-' and isn't all digits, the function returns trimmed as-is:

return trimmed; // could be a display name or any unexpected value

This would silently pass a malformed value to the API. Consider returning null here to fail explicitly rather than forwarding bad data.

---

Minor: initializeBulkEditFieldHandlers is now a comment-only no-op

The function body is entirely comments after this PR. If it's kept as an extension point that's fine, but it's worth a note either way. (This was also flagged in the prior CI review.)

---

Positive observations

• do_each() lines 233–241 already correctly normalize the API-response shape to "user-{id}" — that part is solid and doesn't need changes, just the valueToRemove that feeds the comparison.
• Using usersComponent.value (raw, pre-convertValue) for rawValueWithLabels is correct since the display needs the richer {id, label} shape.
• The _list.scss selector addition and the shared-functions.js HTML simplification are both correct.
• Net −155 lines is a real win in complexity.

---

Summary: The architecture is right, but normalizeUserSelectValue missing the typeof rawValue === 'number' branch means user selection is silently dropped on every save. Adding that one guard (and optionally tightening the string fallback) should make this safe to merge.

[github-actions]

Code Review: PR 2899 - Bulk Edit User Select Typeahead

The previous automated reviews flagged two critical bugs -- both appear to have been fixed in the current commit:

1. Missing number handling in normalizeUserSelectValue -- the typeof number branch IS present, correctly converting integer 5 to the string user-5.
2. String fallback returning trimmed for non-matching strings -- the current code explicitly returns null for that case.

---

Round-trip confirmed

Tracing the full flow:

1. User selects a user in dt-users-connection (with single attribute)
2. collectFieldValue() calls ComponentService.convertValue which returns an integer
3. normalizeUserSelectValue converts the integer to user-{id} string format
4. API payload receives user-{id} -- matches what the DT backend expects

Conditional-removal in do_each() (lines 233-245) also works: the record value is extracted as fieldData['assigned-to'] (already in user-{id} format), and valueToRemove is now normalizedUserValueToRemove (also user-{id}), so strict equality holds.

---

Minor: Object branch in normalizeUserSelectValue does not guard possibleId > 0

The top-level number branch guards rawValue > 0 before building the user-N string, but the inner object branch does not apply the same guard when possibleId is a number. WordPress user IDs are always >= 1, so this is not a real-world risk, but mirroring the guard would be consistent.

---

Minor: initializeBulkEditFieldHandlers is a comment-only no-op

The function body is entirely comments after this PR. If kept as an extension point a JSDoc would help signal intent. If it is genuinely dead code, removing it would be cleaner. Both prior automated reviews flagged this as well.

---

Positive observations

• The do_each comparison logic (lines 233-245) is already correct, and the new normalized format aligns with it perfectly.
• normalizeShareComponentItems correctly returns [{id, label}] items for dt-users-connection, making rawValueWithLabels reliable for the display path.
• The multi-shape handling in renderValuesToRemoveDisplay() (array / object / string) covers the real cases clearly.
• _list.scss adding dt-users-connection label to the shared selector is correct and necessary.
• Removing ~150 lines of custom typeahead lifecycle management is a clear win for maintainability.

---

Summary: The critical bugs from earlier in the review cycle have been addressed in the current commit. The two remaining notes are low-risk polish items. This looks safe to merge.

[kodinkat]

@corsacca review ready....

[corsacca]

@kodinkat this looks like a whole lot of code handling maybes.
Can we double check the actual values being passed arround?

[corsacca]

@kodinkat, my question still stands. Why handle every single possible value, instead of pulling from what the component actually returns?

[github-actions]

Code Review

This PR successfully replaces the legacy jQuery Typeahead implementation for user_select fields in bulk edit with the dt-users-connection web component, making it consistent with other connection field types. Overall the approach is solid. A few observations:

---

Good

• normalizeUserSelectValue() is well-documented and correctly handles the three formats that ComponentService.convertValue('DT-USERS-CONNECTION', ..., 'single') can produce (integer, "user-{id}" string, bare numeric string). Returning null for zero/empty prevents accidentally zeroing out fields.
• Conditional-removal path in do_each() (lines ~233–246) still reads fieldData['assigned-to'] from the API response, which is in "user-{id}" format. normalizedUserValueToRemove is also in that format, so the equality check stays correct — no changes needed there.
• SCSS addition (dt-users-connection label) mirrors the existing pattern for all other web component labels in the bulk-edit panel.

---

Questions / Minor Issues

1. initializeBulkEditFieldHandlers() is now a no-op

After the removal of the typeahead block the function body contains only a comment:

function initializeBulkEditFieldHandlers(fieldKey, fieldType) {
    // For all web components (including user_select via dt-users-connection), ...
    // No per-field-type handlers are required at this time.
}

It is still called from two places (lines 2126 and 2486). Either remove the function and its call sites, or leave a deliberate stub with the comment — just make sure the choice is intentional. Leaving an empty-but-called function is a mild code smell.

---

2. Dead branches in renderValuesToRemoveDisplay() for user_select

The new display code handles three shapes for valuesToRemove:

if (Array.isArray(valuesToRemove)) { ... }      // used when rawValueWithLabels is set
else if (typeof valuesToRemove === 'object') { ... }  // never reached for user_select
else if (typeof valuesToRemove === 'string') { ... }  // used when no component value

fieldData.valuesToRemove is always set to normalizedUserValueToRemove (a string) or null. The rawValueWithLabels path passes an array from normalizeShareComponentItems(). So the typeof valuesToRemove === 'object' branch is unreachable for user_select. Not a bug, but worth a comment or removal to avoid confusion during future maintenance.

---

3. dt:get-data handler and the field-options endpoint for user_select

When dt-users-connection fires dt:get-data with the field key, the global handler at line 1511 calls:

/dt-posts/v2/{postType}/field-options?field={fieldKey}&s={query}

This endpoint is designed for field option lookups. For user_select fields (e.g. assigned_to), please verify:

• The backend's field-options handler returns user records in the format dt-users-connection expects ([{ id, label }] or equivalent).
• Typeahead search (typing a partial name) triggers a fresh dt:get-data request and the component populates results correctly.

If field-options doesn't handle user-search queries for user_select, the component dropdown will be empty. The screenshot in the PR description suggests this works, but it's worth a test note in the PR confirming which endpoint the component ultimately hits.

---

4. shared-functions.js — baseAttrs for dt-users-connection

return `<dt-users-connection ${baseAttrs} single></dt-users-connection>`;

baseAttrs is id="..." name="..." label="...". For dt-connection, a postType attribute is also passed. Does dt-users-connection need any additional attributes (e.g. a post-type to scope user search, or an allow-add flag)? Worth confirming against the component's API docs or other usages.

---

Summary

The refactor is clean and the removal of ~120 lines of typeahead boilerplate is welcome. The main thing to confirm before merging is item #3 — that the field-options endpoint provides the right data shape for dt-users-connection in search mode. If that's confirmed (or the component uses its own internal user-search endpoint independent of dt:get-data), this looks good to merge.

[github-actions]

Code Review: PR #2899 - Bulk Edit User Select Typeahead

This is a clean refactor. Replacing the bespoke jQuery Typeahead lifecycle with dt-users-connection reduces complexity significantly and aligns user_select with the component system used for every other field type in bulk edit. The screenshot confirms the UI renders.

Previous automated reviews surfaced two critical bugs (missing typeof number branch, unsafe string fallback returning the raw value). Both are fixed in the current commit — normalizeUserSelectValue is correct.

A few remaining items worth addressing before merge:

---

Minor: unreachable branch in renderValuesToRemoveDisplay() for user_select

fieldData.valuesToRemove for user_select is always set to normalizedUserValueToRemove, which is a string in "user-{id}" format (or null). The rawValueWithLabels path passes an array from normalizeShareComponentItems(). That means the typeof valuesToRemove === 'object' branch in the new display code is never reachable for user_select:

if (Array.isArray(valuesToRemove)) { ... }           // reached via rawValueWithLabels
else if (typeof valuesToRemove === 'object') { ... }  // dead branch for user_select
else if (typeof valuesToRemove === 'string') { ... }  // reached via normalizedUserValueToRemove

Not a bug, but it adds noise and could mislead future maintainers. Worth a comment or removal.

---

Worth confirming: dt:get-data / field-options for user_select in search mode

The global dt:get-data handler calls:

/dt-posts/v2/{postType}/field-options?field={fieldKey}&s={query}

For connection fields this endpoint returns records in a shape dt-connection understands. Does the same endpoint return user records in a shape that dt-users-connection can consume when the user types a search query?

If dt-users-connection uses its own internal user-search endpoint (independent of the dt:get-data event), this is a non-issue — but it's worth confirming and ideally noting in the PR description. The screenshot shows the component rendering with a pre-selected value, but doesn't show the typeahead search working. A quick note confirming "typed search works via X endpoint" would give reviewers confidence.

---

Minor: baseAttrs may be under-specified for dt-users-connection

The shared-functions.js change outputs:

return `<dt-users-connection ${baseAttrs} single></dt-users-connection>`;

baseAttrs includes id, name, and label. For dt-connection the component also receives a post-type attribute to scope what it searches. Does dt-users-connection need any equivalent attribute (e.g. to scope user roles or a specific site)? If not, a brief note confirming the component defaults are appropriate here would be helpful.

---

Positive observations

• normalizeUserSelectValue is well-structured and handles all three formats ComponentService.convertValue('DT-USERS-CONNECTION', ...) can return (integer, "user-{n}" string, bare numeric string). Returning null for zero/empty is correct.
• The conditional-removal comparison in do_each() (which reads fieldData['assigned-to'] in "user-{id}" format from the API response) aligns correctly with normalizedUserValueToRemove being in the same format.
• The hasValues check is now genuinely conditional on a user actually being selected — the prior hasValues = true shortcut was a latent bug waiting to bite when an empty component was toggled.
• _list.scss addition of dt-users-connection label mirrors the correct pattern used for all other web-component labels.
• Net −155 lines is a real maintainability win.

---

Summary: The critical fixes from earlier in the review cycle are in place. The main open item to confirm (or document) before merge is item #2 — that dt-users-connection search works correctly in the bulk-edit context. The other items are low-risk polish.