#2863 - List Bulk Edit - Share And Follow Functionality Missing

[kodinkat]

• fixes: List Bulk Edit - Share and Follow functionality missing. #2863

---

[kodinkat]

@corsacca review ready....

[corsacca]

Added: Option to remove share from a person: https://github.com//issues/2858

@kodinkat can the share select use a dt-component instead of the old typeahead field?

[kodinkat]

@corsacca review ready.....

[corsacca]

@kodinkat bulk operation belong in modular-list-bulk.js now. Can you move this there (or is it there already?)

[corsacca]

THis section might not be needed.

[corsacca]

Concerns

1. ~928 lines of duplicated code in modular-list.js
   The comments say "Bulk edit functionality has been moved to modular-list-bulk.js", yet the PR
   adds 928 lines to modular-list.js that duplicate the entire bulk edit infrastructure: process(),
   do_each(), do_done(), all the typeahead initializations, share handling, date pickers, etc.
   This appears to be a copy-paste of existing code back into the file it was supposedly moved out
   of.

Curious about this one:
3. Overly defensive value parsing (~150 lines)
processShareComponentValue and the share branch of collectFieldValue have extensive defensive
parsing for every possible value type (string, array, object, null, "null", "[]", "undefined",
etc.). This is ~150 lines of code that could be simplified significantly. The component should
have a well-defined value type.

[github-actions]

test comment

[github-actions]

Code Review

Good progress on adding share and follow to bulk edit. A few issues worth addressing before merge.

[github-actions]

Deleting previous test comments - please ignore comments 3972661425 and 3972663037

[github-actions]

Code Review

Good progress on adding share and follow to bulk edit. A few issues worth addressing before merge:

---

Bug: current_user_id is not in scope

In the new follow branch of collectFieldValue, the fallback uses a bare variable that is not defined in that scope. current_user_id only exists as a local variable inside the button.follow click handler at line 412. This would throw a ReferenceError if window.wpApiNotifications is undefined.

The existing follow handler uses DT_List.current_user_id — that should be the fallback here too:

const currentUserId =
  window.wpApiNotifications?.current_user_id || DT_List.current_user_id;

---

Missing unshare functionality

The new shareFieldSelected code path hardcodes unshare: false, so the new UI can only add shares, never remove them. If #2863 requires bulk unshare support, this path will not cover it. If unshare-via-bulk is intentionally out of scope here, a comment clarifying that would help.

---

Performance: fetching all users for field restoration

When restoring a previously selected user in the share field, the code fetches the entire user list (get_users?s=&get_all=1) just to look up one display name. This is expensive on larger deployments. Since shareUserLabels is already stored on fieldData, the restoration could read the cached label and skip the network call entirely.

---

Unhandled promise rejection in change listener

The change event listener is async but the awaited call has no catch. If processShareComponentValue rejects, the error is silently swallowed. Add a .catch() or a try/catch around the await.

---

processShareComponentValue is unnecessarily async

The function body has no await calls — all operations are synchronous. The async keyword just adds a promise wrapper for no benefit and makes callers error handling less obvious.

---

Minor: redundant some + find

In the share field section, some and find use the same predicate, iterating bulkEditSelectedFields twice. Use only find and check if the result is non-null.

---

Existing note on normalizeShareComponentItems

Agree with @corsacca — the function defensively handles string "null", "[]", object .value, and unexpected primitives that dt-users-connection presumably will not emit. If the component has a well-defined value API, most of those branches can be removed.

---

Summary: The current_user_id ReferenceError is a concrete bug that will cause follow bulk edits to fail when window.wpApiNotifications is unavailable. The other items are code quality and performance concerns worth a cleanup pass.

[github-actions]

Note: comments 3972661425 and 3972663037 and 3972663836 above were test/cleanup comments and can be ignored. The actual review is in the comment immediately below.

[github-actions]

Code Review

Good progress on adding share and follow to bulk edit. One regression stands out as a blocker, along with a few other items.

---

Regression: field clearing is broken for all field types

renderBulkEditField now calls wrapper.find('.bulk-edit-clear-field-btn').show(), but the HTML template in archive-template.php:723 only contains a .bulk-edit-mode-toggle element — there is no .bulk-edit-clear-field-btn in the DOM. The selector will match nothing, so the "Remove Values" toggle will never appear, and the handleModeToggleChange event listener is now never registered for any field.

This affects all clearable field types, not just share/follow.

Fix: either keep the original modeToggle.show() + addEventListener pattern (just add 'share' and 'follow' to the exclusion list), or add .bulk-edit-clear-field-btn to the PHP template and wire up a click handler.

---

Dead code: new share payload format is never triggered

The queue processor now has two branches for share payloads:

// Never reached by new code path
if (share && share.user_id && share.action === 'add') { ... }
// Always used by new code path (legacy format)
else if (share && share['users']) { ... }

The new shareFieldData code path always constructs { users: shareUserIds, unshare: false }, so the first branch is unreachable. Either remove the new branch or change the payload construction to use that format consistently.

---

Unshare is hardcoded to false

The new share UI always sets unshare: false. If a user wants to bulk-remove a share, that is not possible through the new component path. If this is intentionally out of scope, add a comment. If it is needed, the dt-users-connection component may need a separate "unshare" mode or an additional toggle.

---

Potential HTML injection in follow label

Translation strings are embedded in the follow field HTML without escaping:

'label="' + (window.wpApiShare?.translations?.follow || 'Follow') + '"'

If a translation contains " or >, it will break the attribute or inject markup. Use window.SHAREDFUNCTIONS.escapeHTML() (already used nearby for initialValue).

---

Code quality: normalizeShareComponentItems is over-engineered

The function defensively handles string "null", string "[]", { value: [...] } objects, and unexpected primitives. If dt-users-connection has a defined value contract (an array or JSON string of {id, label} objects), most of those branches can be removed, reducing ~40 lines to ~10.

---

Minor: double iteration on bulkEditSelectedFields

In the share/follow fieldKey handlers, some() and find() iterate the same array with identical predicates. A single find() (checking for a null result) is sufficient.

[github-actions]

Code Review

Good work adding Share and Follow to the bulk edit field selector — the feature fills a clear gap from #2863. A few issues to address:

---

Bug: Missing i18n translation keys

The new labels rely on window.wpApiShare?.translations?.share, ...?.follow, and ...?.follow_help, but none of these keys are defined in enqueue-scripts.php's wpApiShare.translations array (lines 129–139). As a result, the labels will always fall back to the hardcoded English strings 'Share', 'Follow', and 'Toggle to follow or unfollow records' regardless of the site's locale.

These translation keys need to be added to enqueue-scripts.php:

// in the wpApiShare translations array
'share'       => __( 'Share', 'disciple_tools' ),
'follow'      => __( 'Follow', 'disciple_tools' ),
'follow_help' => __( 'Toggle to follow or unfollow records', 'disciple_tools' ),

---

Dead code: single-user add branch in do_each()

The new block added around line 290:

if (share && share.user_id && share.action === 'add') { ... }

is never reached. Every code path in this PR produces a sharePayload in the { users: [...], unshare: false } format (either via the new shareFieldData branch or the legacy shareInput branch). No code produces { user_id, action }. Either remove this block or add a comment linking to a concrete future issue so reviewers know it's intentional.

---

Redundant component value restoration

In renderBulkEditFieldInput for the share field, initialValue is already built from fieldData.shareUserIds / previousUserId and passed as the HTML value attribute. Then inside requestAnimationFrame, there's a second shareComponent.value = value assignment for the previousUserId case. The two paths overlap — if previousUserId is set but label is empty (falsy), the re-assignment is silently skipped even though a user ID exists. Consider removing the secondary restoration block and relying solely on the attribute-based initialValue.

---

Minor

• Silent error swallowing: The componentService.initialize() catch block has no logging. Add console.error(e) so initialization failures surface during development.

• normalizeShareComponentItems complexity: The function handles ~7 value shapes (null, undefined, string 'null', string '[]', JSON string, Array, plain object) totaling ~50 lines. A brief comment explaining which shapes are actually observed from dt-users-connection in practice would help future maintainers understand which branches are defensive vs. necessary.

• processShareComponentValue closure: The function is redefined on every invocation of renderBulkEditFieldInput for the share field. Extracting it to module scope would avoid unnecessary allocations.

---

The unshare-via-bulk-edit limitation (hardcoded unshare: false) is clearly documented in the code — that's fine as long as it's tracked as a follow-up.

[github-actions]

test

[github-actions]

Code Review

Good work adding Share and Follow to the bulk edit UI — this fills a real usability gap (#2863). The overall structure is sound and the new fields integrate cleanly with the existing bulk-edit pipeline. A few things worth addressing before merge:

---

Potential Regression – Mode Toggle Event Delegation

The PR refactors how the "Remove Values" mode toggle (.bulk-edit-mode-toggle) is wired:

Before – a direct event listener was attached per-field when renderBulkEditField ran:

modeToggle[0].addEventListener('change', function () {
  handleModeToggleChange(this);
});

After – that listener is removed and replaced by a single document-level delegated handler:

$(document).on('change', '.bulk-edit-mode-toggle', function () {
  handleModeToggleChange(this);
});

Event delegation only fires for events that bubble. The dt-toggle web component's change event needs to have { bubbles: true } for delegation to work. If it doesn't bubble, the "Remove Values" toggle will silently stop working for all clearable fields (not just share/follow), which would be a significant regression unrelated to this PR's goal. Please verify that dt-toggle dispatches a bubbling change event.

---

Dead Code Path

The new single-user add-share format is added with a comment saying no code path produces it:

// Single-user add format { user_id, action: 'add' } — reserved for future use;
// no code path produces it yet
if (share && share.user_id && share.action === 'add') { ... }

Dead code that is explicitly "reserved for future use" adds maintenance burden. Consider removing it and adding it in the PR that actually introduces the consuming code.

---

Silent Failure When currentUserId Is Missing

In collectFieldValue for the follow field type, if currentUserId is falsy the function returns null without any feedback. The bulk submit will proceed and show success, but the follow state won't change. A console.warn at minimum would help with debugging.

---

"Legacy" Label Is Misleading

The comment at the else if (share && share['users']) branch calls the format "Legacy share format support (backward compatibility)," but the new share UI also produces this exact { users, unshare } structure. A clearer comment would be "multi-user share format (array of user IDs)."

---

Minor – Redundant State Properties

processShareComponentValue tracks four related properties on fieldData: shareUserId / shareUserLabel (first user, labeled "backward compat") and shareUserIds / shareUserLabels (full array). Since this is new code with no existing callers of the singular properties, the singular forms could be removed or a comment should point to the actual consumer.

---

What Looks Good

• escapeHTML() is correctly applied to the initialValue attribute and to translated label/help-text strings before HTML interpolation.
• Synthetic "Share" and "Follow" entries in the field dropdown are cleanly separated from real post fields.
• The follow payload format ({ values: [{ value: userId, delete: !toggleValue }] }) matches how the existing single-record follow button works.
• Translation strings (share, follow, follow_help) go into wpApiShare.translations where the JS accesses them, and fallback strings match the PHP defaults.
• Excluding share and follow from the clearable-fields mode toggle is correct.

[kodinkat]

@corsacca review ready.....

[corsacca]

Thank you @kodinkat!

[github-actions]

Code Review

The PR fills a real gap - Share and Follow were missing from bulk edit (#2863). The overall approach is sound, and the iteration since the first review has addressed several earlier concerns (module-scope helpers, correct currentUserId fallback, event-listener removal per field). A few issues remain worth resolving before merge.

---

1. Potential Regression - Mode Toggle Event Delegation (most important)

The PR removes the per-field direct event listener on renderBulkEditField and replaces it with a single document-level delegated handler. Event delegation only works if the change event bubbles. Web component custom events do not bubble by default. If dt-toggle does not produce a bubbling change event, the "Remove Values" toggle will silently stop working for all clearable fields (connection, multi_select, date, text, etc.), not just share/follow. That would be a significant regression unrelated to this PR goal.

Please verify that dt-toggle dispatches a bubbling change event before this ships.

---

2. Dead Code Path

The single-user add block (if (share && share.user_id && share.action === 'add')) is explicitly commented as having no producer and reserved for future use. Dead code marked as reserved for future use is a maintenance liability. Remove it now and add it in the PR that actually introduces the consuming code path.

---

3. Silent Failure When currentUserId is Missing

In collectFieldValue for the follow field type, if currentUserId is falsy (both window.wpApiNotifications and DT_List.current_user_id unavailable), the function returns null with no feedback. The bulk submit will proceed and show success, but the follow state is never changed. A console.warn would make this detectable during debugging.

---

4. Comment Accuracy

The new share UI added in this PR also produces the { users, unshare } format, so calling the else if (share && share['users']) branch "Legacy share format support (backward compatibility)" is misleading. Something like "Multi-user share format (array of user IDs)" would be more accurate.

---

5. Translation Key Placement (minor)

share, follow, and follow_help are added to wpApiShare.translations, but the rest of modular-list-bulk.js uses list_settings.translations for list-UI strings (make_selections_below, delete_selections_below, sent, etc.). The list-specific translations array in enqueue-scripts.php via the dt_list_js_translations filter would be the more consistent home for these. Not a blocker.

---

What Looks Good

• escapeHTML() is correctly applied to both the initialValue attribute and to translated label/help-text strings before HTML interpolation.
• normalizeShareComponentItems and processShareComponentValue are now at module scope - the per-render function redefinition concern from the last review is resolved.
• Synthetic Share and Follow entries are cleanly separated from real post fields.
• The follow payload format matches how the existing single-record follow button constructs its payload.
• Excluding share and follow from supportsFieldClearing is correct.
• ComponentService initialization errors now log to console.error rather than swallowing silently.