From 3fcd1cbf0e1e6d807f794b4a3806dcc96b6b54a0 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 21:56:19 -0400 Subject: [PATCH 1/2] Guard bulk-action buttons against double-submit (#36) The 4 bulk-action confirm buttons (close/assign/priority/status) called their performBulk*() handler directly on click with no in-flight guard. Double-clicking a confirm button fired two concurrent POST /api/ bulk_operation.php requests for the same ticket IDs, duplicating the close-reason comment, audit-log entry, and Matrix notification on every affected ticket. Each performBulk*() function now takes the clicked button, no-ops if it's already disabled, disables it before firing the request, and re-enables it in .finally() regardless of outcome. Verified by extracting performBulkAssign() from the real source and driving it with a mock lt.api.post() that never resolves until told to: a simulated rapid double-click fired exactly one request (the second call was a no-op while the button was disabled), and a subsequent click after the request resolved correctly fired a new request. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- assets/js/dashboard.js | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/assets/js/dashboard.js b/assets/js/dashboard.js index fca33fd..f0369dc 100644 --- a/assets/js/dashboard.js +++ b/assets/js/dashboard.js @@ -140,25 +140,25 @@ document.addEventListener('DOMContentLoaded', function() { break; // Bulk operation perform actions case 'perform-bulk-assign': - performBulkAssign(); + performBulkAssign(target); break; case 'close-bulk-assign-modal': closeBulkAssignModal(); break; case 'perform-bulk-priority': - performBulkPriority(); + performBulkPriority(target); break; case 'close-bulk-priority-modal': closeBulkPriorityModal(); break; case 'perform-bulk-status': - performBulkStatusChange(); + performBulkStatusChange(target); break; case 'close-bulk-status-modal': closeBulkStatusModal(); break; case 'perform-bulk-close': - performBulkCloseAction(); + performBulkCloseAction(undefined, target); break; case 'close-bulk-close-modal': closeBulkCloseModal(); @@ -491,7 +491,10 @@ function closeBulkCloseModal() { if (modal) setTimeout(() => modal.remove(), 300); } -function performBulkCloseAction(ticketIds) { +function performBulkCloseAction(ticketIds, btn) { + if (btn && btn.disabled) return; // already in flight — guards against a double-click firing two requests + if (btn) btn.disabled = true; + ticketIds = ticketIds || getSelectedTicketIds(); const commentEl = document.getElementById('bulkCloseComment'); const comment = commentEl ? commentEl.value.trim() : ''; @@ -524,7 +527,8 @@ function performBulkCloseAction(ticketIds) { } closeBulkCloseModal(); lt.toast.error('Bulk close failed: ' + error.message, 5000); - }); + }) + .finally(() => { if (btn) btn.disabled = false; }); } var _bulkAssignUserId = null; @@ -596,7 +600,8 @@ function closeBulkAssignModal() { if (modal) setTimeout(() => modal.remove(), 300); } -function performBulkAssign() { +function performBulkAssign(btn) { + if (btn && btn.disabled) return; // already in flight — guards against a double-click firing two requests const userId = _bulkAssignUserId; const ticketIds = getSelectedTicketIds(); @@ -605,6 +610,8 @@ function performBulkAssign() { return; } + if (btn) btn.disabled = true; + lt.api.post('/api/bulk_operation.php', { operation_type: 'bulk_assign', ticket_ids: ticketIds, @@ -625,7 +632,8 @@ function performBulkAssign() { }) .catch(error => { lt.toast.error('Bulk assign failed: ' + error.message, 5000); - }); + }) + .finally(() => { if (btn) btn.disabled = false; }); } function showBulkPriorityModal() { @@ -672,7 +680,8 @@ function closeBulkPriorityModal() { if (modal) setTimeout(() => modal.remove(), 300); } -function performBulkPriority() { +function performBulkPriority(btn) { + if (btn && btn.disabled) return; // already in flight — guards against a double-click firing two requests const priorityEl = document.getElementById('bulkPriority'); if (!priorityEl) return; const priority = priorityEl.value; @@ -683,6 +692,8 @@ function performBulkPriority() { return; } + if (btn) btn.disabled = true; + lt.api.post('/api/bulk_operation.php', { operation_type: 'bulk_priority', ticket_ids: ticketIds, @@ -703,7 +714,8 @@ function performBulkPriority() { }) .catch(error => { lt.toast.error('Bulk priority update failed: ' + error.message, 5000); - }); + }) + .finally(() => { if (btn) btn.disabled = false; }); } // Make table rows clickable @@ -786,7 +798,8 @@ function closeBulkStatusModal() { if (modal) setTimeout(() => modal.remove(), 300); } -function performBulkStatusChange() { +function performBulkStatusChange(btn) { + if (btn && btn.disabled) return; // already in flight — guards against a double-click firing two requests const bulkStatusEl = document.getElementById('bulkStatus'); if (!bulkStatusEl) return; const status = bulkStatusEl.value; @@ -800,6 +813,8 @@ function performBulkStatusChange() { const commentEl = document.getElementById('bulkStatusComment'); const comment = commentEl ? commentEl.value.trim() : ''; + if (btn) btn.disabled = true; + lt.api.post('/api/bulk_operation.php', { operation_type: 'bulk_status', ticket_ids: ticketIds, @@ -829,7 +844,8 @@ function performBulkStatusChange() { } closeBulkStatusModal(); lt.toast.error('Bulk status change failed: ' + error.message, 5000); - }); + }) + .finally(() => { if (btn) btn.disabled = false; }); } /** From 844677bbcec2353ff3535a4214cb6125fe1f7e3e Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 21:56:26 -0400 Subject: [PATCH 2/2] Fix atomicity docblock and surface per-ticket bulk-op errors (#33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit processBulkOperation()'s docblock claimed the transaction "ensures atomicity - either all tickets are updated or none are," but that's only true when $atomic = true is passed, and the only real caller (api/bulk_operation.php) never passes it — the actual default is best-effort: per-ticket failures are skipped and recorded, and every other ticket in the batch still commits. Reworded the docblock to describe the actual default behavior and when $atomic changes it. The model already collected per-ticket failure reasons into $result['errors'] (dashboard.js's bulkResultMessage() already reads data.errors to render them), but api/bulk_operation.php's success response dropped that field entirely, so admins only ever saw a bare "N succeeded, M failed" count with no way to see which tickets failed or why. Added 'errors' to the response when present. Verified against real MariaDB: a bulk_status operation against a Closed ticket (no transition defined) and an Open ticket (Open->Pending defined) correctly processed 1/1, and the API response now includes errors: ["Ticket ...: transition not allowed (Closed -> Pending)"]. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- api/bulk_operation.php | 11 +++++++++-- models/BulkOperationsModel.php | 12 +++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/api/bulk_operation.php b/api/bulk_operation.php index 57fa6ec..bc3a14f 100644 --- a/api/bulk_operation.php +++ b/api/bulk_operation.php @@ -131,12 +131,19 @@ if (isset($result['error'])) { if ($inaccessibleCount > 0) { $message .= " ($inaccessibleCount skipped - no access)"; } - echo json_encode([ + $response = [ 'success' => true, 'operation_id' => $operationId, 'processed' => $result['processed'], 'failed' => $result['failed'], 'skipped' => $inaccessibleCount, 'message' => $message - ]); + ]; + // Best-effort batches (the default; see processBulkOperation()'s docblock) + // can partially fail — surface the per-ticket reasons so the admin isn't + // just told a count. The dashboard's bulkResultMessage() already expects this. + if (!empty($result['errors'])) { + $response['errors'] = $result['errors']; + } + echo json_encode($response); } diff --git a/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index 47dee2c..afd64be 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -106,12 +106,18 @@ class BulkOperationsModel /** * Process a bulk operation * - * Uses database transaction to ensure atomicity - either all tickets - * are updated or none are (on failure, changes are rolled back). + * Runs the whole batch inside one database transaction, but by default + * ($atomic = false, which is what api/bulk_operation.php uses) that + * transaction is always committed: a per-ticket failure (e.g. a + * disallowed workflow transition) is recorded in $failed/$errors and + * skipped, while every other ticket in the batch still succeeds. This + * is a best-effort batch, not an all-or-nothing one — set $atomic to + * true to roll back the entire batch when any ticket fails. * * @param int $operationId Operation ID * @param bool $atomic If true, rollback all changes on any failure - * @return array Result with processed and failed counts + * @return array Result with processed/failed counts and an errors[] list of + * per-ticket failure reasons (surfaced to the admin by the caller) */ public function processBulkOperation($operationId, bool $atomic = false) {