Compare commits

...
Author SHA1 Message Date
jaredandClaude Sonnet 5 4aa83ffe58 Merge development into main: bulk-op atomicity docs + double-submit guard (#33, #36)
Lint / PHP (phpcs PSR-12) (push) Successful in 18s
Lint / JS (eslint) (push) Successful in 8s
Lint / PHP requirements (version + extensions) (push) Successful in 20s
Lint / Notify on failure (push) Skipped
Security / PHP Security (semgrep) (push) Successful in 1m5s
Lint / Deploy (push) Successful in 2s
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV
2026-09-11 21:58:45 -04:00
jaredandClaude Sonnet 5 844677bbce Fix atomicity docblock and surface per-ticket bulk-op errors (#33)
Lint / PHP (phpcs PSR-12) (push) Successful in 19s
Lint / JS (eslint) (push) Successful in 7s
Lint / PHP requirements (version + extensions) (push) Successful in 23s
Lint / Notify on failure (push) Skipped
Security / PHP Security (semgrep) (push) Successful in 1m4s
Lint / Deploy (push) Successful in 2s
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV
2026-09-11 21:56:26 -04:00
jaredandClaude Sonnet 5 3fcd1cbf0e 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV
2026-09-11 21:56:19 -04:00
3 changed files with 46 additions and 17 deletions
+9 -2
View File
@@ -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);
}
+28 -12
View File
@@ -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; });
}
/**
+9 -3
View File
@@ -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)
{