diff --git a/api/bulk_operation.php b/api/bulk_operation.php index f25f73c..fe4f521 100644 --- a/api/bulk_operation.php +++ b/api/bulk_operation.php @@ -107,10 +107,17 @@ $result = $bulkOpsModel->processBulkOperation($operationId); if (isset($result['error'])) { $conn->close(); - echo json_encode([ + $response = [ 'success' => false, 'error' => $result['error'] - ]); + ]; + // Let the client know it should collect a comment and retry, rather than + // showing the failure as a dead end. + if (!empty($result['requires_comment'])) { + $response['requires_comment'] = true; + http_response_code(400); + } + echo json_encode($response); } else { // Invalidate stats cache so dashboard tiles reflect changes immediately require_once dirname(__DIR__) . '/models/StatsModel.php'; diff --git a/assets/js/dashboard.js b/assets/js/dashboard.js index 526ef54..f0e64c9 100644 --- a/assets/js/dashboard.js +++ b/assets/js/dashboard.js @@ -157,6 +157,12 @@ document.addEventListener('DOMContentLoaded', function() { case 'close-bulk-status-modal': closeBulkStatusModal(); break; + case 'perform-bulk-close': + performBulkCloseAction(); + break; + case 'close-bulk-close-modal': + closeBulkCloseModal(); + break; case 'perform-bulk-delete': performBulkDelete(); break; @@ -515,24 +521,59 @@ function bulkClose() { return; } - showConfirmModal( - `Close ${ticketIds.length} Ticket(s)?`, - 'Are you sure you want to close these tickets?', - 'warning', - () => performBulkCloseAction(ticketIds) - ); + // Closing needs a reason: the default workflow marks every → Closed transition + // requires_comment, so collect it here instead of failing server-side. + const modalHtml = ` + + `; + + document.body.insertAdjacentHTML('beforeend', modalHtml); + lt.modal.open('bulkCloseModal'); +} + +function closeBulkCloseModal() { + lt.modal.close('bulkCloseModal'); + const modal = document.getElementById('bulkCloseModal'); + if (modal) setTimeout(() => modal.remove(), 300); } function performBulkCloseAction(ticketIds) { + ticketIds = ticketIds || getSelectedTicketIds(); + const commentEl = document.getElementById('bulkCloseComment'); + const comment = commentEl ? commentEl.value.trim() : ''; lt.api.post('/api/bulk_operation.php', { operation_type: 'bulk_close', - ticket_ids: ticketIds + ticket_ids: ticketIds, + parameters: { comment: comment } }) .then(data => { + closeBulkCloseModal(); if (data.success) { if (data.failed > 0) { - lt.toast.warning(`Bulk close: ${data.processed} succeeded, ${data.failed} failed`, 5000); + lt.toast.warning(bulkResultMessage('Bulk close', data), 6000); } else { lt.toast.success(`Successfully closed ${data.processed} ticket(s)`, 4000); } @@ -542,6 +583,14 @@ function performBulkCloseAction(ticketIds) { } }) .catch(error => { + // Missing required comment — keep the modal open so it can be entered. + if (error && error.data && error.data.requires_comment) { + lt.toast.warning(error.data.error || 'A close reason is required', 6000); + const ta = document.getElementById('bulkCloseComment'); + if (ta) ta.focus(); + return; + } + closeBulkCloseModal(); lt.toast.error('Bulk close failed: ' + error.message, 5000); }); } @@ -777,6 +826,15 @@ function showBulkStatusModal() { ${(window.TICKET_STATUSES || ['Open','Pending','In Progress','Closed']).map(s => ``).join('')} + + +

+ Required for transitions the Workflow Designer marks as needing a comment + (e.g. closing a ticket). Posted as a comment on every ticket changed. +

@@ -807,16 +865,19 @@ function performBulkStatusChange() { return; } + const commentEl = document.getElementById('bulkStatusComment'); + const comment = commentEl ? commentEl.value.trim() : ''; + lt.api.post('/api/bulk_operation.php', { operation_type: 'bulk_status', ticket_ids: ticketIds, - parameters: { status: status } + parameters: { status: status, comment: comment } }) .then(data => { closeBulkStatusModal(); if (data.success) { if (data.failed > 0) { - lt.toast.warning(`Status update: ${data.processed} succeeded, ${data.failed} failed`, 5000); + lt.toast.warning(bulkResultMessage('Status update', data), 6000); } else { lt.toast.success(`Successfully updated status for ${data.processed} ticket(s)`, 4000); } @@ -826,10 +887,32 @@ function performBulkStatusChange() { } }) .catch(error => { + // Workflow needs a comment for at least one selected ticket — keep the + // modal open so the reason can be typed in without re-selecting. + if (error && error.data && error.data.requires_comment) { + lt.toast.warning(error.data.error || 'A comment is required for this status change', 6000); + const ta = document.getElementById('bulkStatusComment'); + if (ta) ta.focus(); + return; + } + closeBulkStatusModal(); lt.toast.error('Bulk status change failed: ' + error.message, 5000); }); } +/** + * Build a result message for a partially-successful bulk operation, surfacing the + * per-ticket reasons (e.g. "transition not allowed") instead of a bare count. + */ +function bulkResultMessage(label, data) { + let msg = `${label}: ${data.processed} succeeded, ${data.failed} failed`; + if (Array.isArray(data.errors) && data.errors.length) { + msg += ' — ' + data.errors.slice(0, 3).join('; '); + if (data.errors.length > 3) msg += ` (+${data.errors.length - 3} more)`; + } + return msg; +} + // Bulk Delete function showBulkDeleteModal() { const ticketIds = getSelectedTicketIds(); diff --git a/migrations/000_baseline.sql b/migrations/000_baseline.sql index 42af57c..dc7ddc8 100644 --- a/migrations/000_baseline.sql +++ b/migrations/000_baseline.sql @@ -59,7 +59,8 @@ CREATE TABLE IF NOT EXISTS `bulk_operations` ( `ticket_ids` text NOT NULL, `performed_by` int(11) NOT NULL, `parameters` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL CHECK (json_valid(`parameters`)), - `status` varchar(20) DEFAULT 'pending', + -- 32, not 20: 'completed_with_errors' is 21 chars (see 001_widen_bulk_operations_status.sql) + `status` varchar(32) DEFAULT 'pending', `total_tickets` int(11) DEFAULT NULL, `processed_tickets` int(11) DEFAULT 0, `failed_tickets` int(11) DEFAULT 0, diff --git a/migrations/001_widen_bulk_operations_status.sql b/migrations/001_widen_bulk_operations_status.sql new file mode 100644 index 0000000..0ded271 --- /dev/null +++ b/migrations/001_widen_bulk_operations_status.sql @@ -0,0 +1,12 @@ +-- Widen bulk_operations.status +-- +-- The code writes 'completed_with_errors' (21 chars) when a bulk operation +-- finishes with per-ticket failures, but the column was varchar(20), so the +-- write failed with "Data too long for column 'status'". This was unreachable +-- while bulk status changes forced every transition through; now that they +-- honour the Workflow Designer, partial failures are a normal outcome. +-- +-- Safe to re-run. + +ALTER TABLE `bulk_operations` + MODIFY COLUMN `status` varchar(32) DEFAULT 'pending'; diff --git a/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index 713e28f..0b601d4 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -7,11 +7,47 @@ class BulkOperationsModel { private $conn; + /** @var WorkflowModel|null Lazily created; only needed by status-changing operations */ + private $workflowModel = null; + + /** @var CommentModel|null Lazily created; only needed when a status change carries a comment */ + private $commentModel = null; + + /** @var array user_id → display name, resolved once per request */ + private $userNames = []; + public function __construct($conn) { $this->conn = $conn; } + /** + * Workflow model, created on first use. + */ + private function workflow(): WorkflowModel + { + if ($this->workflowModel === null) { + require_once dirname(__DIR__) . '/models/WorkflowModel.php'; + $this->workflowModel = new WorkflowModel($this->conn); + } + return $this->workflowModel; + } + + /** + * The status a bulk operation is trying to move tickets into, or null for + * operations that don't change status. + */ + private function targetStatusFor(string $operationType, array $parameters): ?string + { + if ($operationType === 'bulk_close') { + return 'Closed'; + } + if ($operationType === 'bulk_status') { + return isset($parameters['status']) ? (string)$parameters['status'] : null; + } + return null; + } + /** * Create a new bulk operation record * @@ -100,6 +136,30 @@ class BulkOperationsModel // Batch load all tickets in one query to eliminate N+1 problem $ticketsById = $ticketModel->getTicketsByIds($ticketIds); + // Status-changing operations honour the Workflow Designer. If any ticket in + // the selection needs a comment for its transition, reject the whole batch + // before mutating anything so the client can collect one — a partially + // applied batch is worse than none. + $targetStatus = $this->targetStatusFor($operation['operation_type'], is_array($parameters) ? $parameters : []); + $bulkComment = trim((string)($parameters['comment'] ?? '')); + if ($targetStatus !== null && $bulkComment === '') { + foreach ($ticketIds as $tid) { + $t = $ticketsById[trim($tid)] ?? null; + if (!$t || $t['status'] === $targetStatus) { + continue; + } + if ($this->workflow()->transitionRequiresComment($t['status'], $targetStatus)) { + return [ + 'processed' => 0, + 'failed' => count($ticketIds), + 'error' => 'A comment is required to change status from ' + . $t['status'] . ' → ' . $targetStatus, + 'requires_comment' => true, + ]; + } + } + } + // Start transaction for data consistency $this->conn->begin_transaction(); @@ -113,11 +173,32 @@ class BulkOperationsModel $success = false; try { - // NOTE: bulk_status / bulk_close intentionally do NOT run - // WorkflowModel::isTransitionAllowed(). Bulk operations are an - // admin-only escape hatch for forcing ticket states (e.g. mass - // re-opening), so they bypass the workflow transition rules that - // the single-ticket update path enforces. This is by design. + // bulk_status / bulk_close enforce the same Workflow Designer + // rules as the single-ticket path: a transition the designer + // doesn't define is refused, and requires_comment is honoured + // (checked up front, above). requires_admin is satisfied because + // api/bulk_operation.php already gates the endpoint on admin. + if ($targetStatus !== null) { + $currentTicket = $ticketsById[$ticketId] ?? null; + if ($currentTicket && $currentTicket['status'] === $targetStatus) { + // Already in the requested state — nothing to do, and + // reporting a no-op as a failure would just confuse. + $processed++; + continue; + } + $allowed = $currentTicket === null || $this->workflow()->isTransitionAllowed( + $currentTicket['status'], + $targetStatus, + true + ); + if (!$allowed) { + $failed++; + $errors[] = "Ticket $ticketId: transition not allowed (" + . $currentTicket['status'] . ' → ' . $targetStatus . ')'; + continue; + } + } + switch ($operation['operation_type']) { case 'bulk_close': // Get current ticket from pre-loaded batch @@ -232,6 +313,12 @@ class BulkOperationsModel } if ($success) { + // Persist the status-change reason as a real comment, so a + // bulk close is as auditable on the ticket as a single close + // (where the client posts the comment before updating). + if ($targetStatus !== null && $bulkComment !== '') { + $this->postBulkComment($ticketId, (int)$operation['performed_by'], $bulkComment); + } $processed++; } else { $failed++; @@ -290,14 +377,22 @@ class BulkOperationsModel ]; } - // Update operation status - $status = $failed > 0 ? 'completed_with_errors' : 'completed'; - $sql = "UPDATE bulk_operations SET status = ?, processed_tickets = ?, failed_tickets = ?, - completed_at = NOW() WHERE operation_id = ?"; - $stmt = $this->conn->prepare($sql); - $stmt->bind_param("siii", $status, $processed, $failed, $operationId); - $stmt->execute(); - $stmt->close(); + // Update operation status. This is bookkeeping only and runs after the + // ticket changes are committed, so a failure here (e.g. the status column + // not yet widened by 001_widen_bulk_operations_status.sql on an instance + // deployed ahead of its migrations) must not turn a completed operation + // into an error response. + try { + $status = $failed > 0 ? 'completed_with_errors' : 'completed'; + $sql = "UPDATE bulk_operations SET status = ?, processed_tickets = ?, failed_tickets = ?, + completed_at = NOW() WHERE operation_id = ?"; + $stmt = $this->conn->prepare($sql); + $stmt->bind_param("siii", $status, $processed, $failed, $operationId); + $stmt->execute(); + $stmt->close(); + } catch (Throwable $e) { + error_log("Bulk operation $operationId completed but status bookkeeping failed: " . $e->getMessage()); + } $result = ['processed' => $processed, 'failed' => $failed]; if (!empty($errors)) { @@ -350,6 +445,37 @@ class BulkOperationsModel return null; } + /** + * Post the bulk status-change reason as a comment on one ticket. + * + * Runs inside the caller's transaction, so a rollback drops the comment along + * with the status change. + */ + private function postBulkComment(string $ticketId, int $userId, string $text): void + { + require_once dirname(__DIR__) . '/models/CommentModel.php'; + + if ($this->commentModel === null) { + $this->commentModel = new CommentModel($this->conn); + } + if (!isset($this->userNames[$userId])) { + $stmt = $this->conn->prepare( + "SELECT COALESCE(NULLIF(display_name, ''), username) AS name FROM users WHERE user_id = ? LIMIT 1" + ); + $stmt->bind_param("i", $userId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + $this->userNames[$userId] = $row['name'] ?? 'User'; + } + + $this->commentModel->addComment($ticketId, [ + 'user_name' => $this->userNames[$userId], + 'comment_text' => $text, + 'markdown_enabled' => 0, + ], $userId); + } + /** * Check whether a user ID exists. */