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
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
This commit is contained in:
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user