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) { if ($inaccessibleCount > 0) {
$message .= " ($inaccessibleCount skipped - no access)"; $message .= " ($inaccessibleCount skipped - no access)";
} }
echo json_encode([ $response = [
'success' => true, 'success' => true,
'operation_id' => $operationId, 'operation_id' => $operationId,
'processed' => $result['processed'], 'processed' => $result['processed'],
'failed' => $result['failed'], 'failed' => $result['failed'],
'skipped' => $inaccessibleCount, 'skipped' => $inaccessibleCount,
'message' => $message '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; break;
// Bulk operation perform actions // Bulk operation perform actions
case 'perform-bulk-assign': case 'perform-bulk-assign':
performBulkAssign(); performBulkAssign(target);
break; break;
case 'close-bulk-assign-modal': case 'close-bulk-assign-modal':
closeBulkAssignModal(); closeBulkAssignModal();
break; break;
case 'perform-bulk-priority': case 'perform-bulk-priority':
performBulkPriority(); performBulkPriority(target);
break; break;
case 'close-bulk-priority-modal': case 'close-bulk-priority-modal':
closeBulkPriorityModal(); closeBulkPriorityModal();
break; break;
case 'perform-bulk-status': case 'perform-bulk-status':
performBulkStatusChange(); performBulkStatusChange(target);
break; break;
case 'close-bulk-status-modal': case 'close-bulk-status-modal':
closeBulkStatusModal(); closeBulkStatusModal();
break; break;
case 'perform-bulk-close': case 'perform-bulk-close':
performBulkCloseAction(); performBulkCloseAction(undefined, target);
break; break;
case 'close-bulk-close-modal': case 'close-bulk-close-modal':
closeBulkCloseModal(); closeBulkCloseModal();
@@ -491,7 +491,10 @@ function closeBulkCloseModal() {
if (modal) setTimeout(() => modal.remove(), 300); 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(); ticketIds = ticketIds || getSelectedTicketIds();
const commentEl = document.getElementById('bulkCloseComment'); const commentEl = document.getElementById('bulkCloseComment');
const comment = commentEl ? commentEl.value.trim() : ''; const comment = commentEl ? commentEl.value.trim() : '';
@@ -524,7 +527,8 @@ function performBulkCloseAction(ticketIds) {
} }
closeBulkCloseModal(); closeBulkCloseModal();
lt.toast.error('Bulk close failed: ' + error.message, 5000); lt.toast.error('Bulk close failed: ' + error.message, 5000);
}); })
.finally(() => { if (btn) btn.disabled = false; });
} }
var _bulkAssignUserId = null; var _bulkAssignUserId = null;
@@ -596,7 +600,8 @@ function closeBulkAssignModal() {
if (modal) setTimeout(() => modal.remove(), 300); 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 userId = _bulkAssignUserId;
const ticketIds = getSelectedTicketIds(); const ticketIds = getSelectedTicketIds();
@@ -605,6 +610,8 @@ function performBulkAssign() {
return; return;
} }
if (btn) btn.disabled = true;
lt.api.post('/api/bulk_operation.php', { lt.api.post('/api/bulk_operation.php', {
operation_type: 'bulk_assign', operation_type: 'bulk_assign',
ticket_ids: ticketIds, ticket_ids: ticketIds,
@@ -625,7 +632,8 @@ function performBulkAssign() {
}) })
.catch(error => { .catch(error => {
lt.toast.error('Bulk assign failed: ' + error.message, 5000); lt.toast.error('Bulk assign failed: ' + error.message, 5000);
}); })
.finally(() => { if (btn) btn.disabled = false; });
} }
function showBulkPriorityModal() { function showBulkPriorityModal() {
@@ -672,7 +680,8 @@ function closeBulkPriorityModal() {
if (modal) setTimeout(() => modal.remove(), 300); 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'); const priorityEl = document.getElementById('bulkPriority');
if (!priorityEl) return; if (!priorityEl) return;
const priority = priorityEl.value; const priority = priorityEl.value;
@@ -683,6 +692,8 @@ function performBulkPriority() {
return; return;
} }
if (btn) btn.disabled = true;
lt.api.post('/api/bulk_operation.php', { lt.api.post('/api/bulk_operation.php', {
operation_type: 'bulk_priority', operation_type: 'bulk_priority',
ticket_ids: ticketIds, ticket_ids: ticketIds,
@@ -703,7 +714,8 @@ function performBulkPriority() {
}) })
.catch(error => { .catch(error => {
lt.toast.error('Bulk priority update failed: ' + error.message, 5000); lt.toast.error('Bulk priority update failed: ' + error.message, 5000);
}); })
.finally(() => { if (btn) btn.disabled = false; });
} }
// Make table rows clickable // Make table rows clickable
@@ -786,7 +798,8 @@ function closeBulkStatusModal() {
if (modal) setTimeout(() => modal.remove(), 300); 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'); const bulkStatusEl = document.getElementById('bulkStatus');
if (!bulkStatusEl) return; if (!bulkStatusEl) return;
const status = bulkStatusEl.value; const status = bulkStatusEl.value;
@@ -800,6 +813,8 @@ function performBulkStatusChange() {
const commentEl = document.getElementById('bulkStatusComment'); const commentEl = document.getElementById('bulkStatusComment');
const comment = commentEl ? commentEl.value.trim() : ''; const comment = commentEl ? commentEl.value.trim() : '';
if (btn) btn.disabled = true;
lt.api.post('/api/bulk_operation.php', { lt.api.post('/api/bulk_operation.php', {
operation_type: 'bulk_status', operation_type: 'bulk_status',
ticket_ids: ticketIds, ticket_ids: ticketIds,
@@ -829,7 +844,8 @@ function performBulkStatusChange() {
} }
closeBulkStatusModal(); closeBulkStatusModal();
lt.toast.error('Bulk status change failed: ' + error.message, 5000); 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 * Process a bulk operation
* *
* Uses database transaction to ensure atomicity - either all tickets * Runs the whole batch inside one database transaction, but by default
* are updated or none are (on failure, changes are rolled back). * ($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 int $operationId Operation ID
* @param bool $atomic If true, rollback all changes on any failure * @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) public function processBulkOperation($operationId, bool $atomic = false)
{ {