From 9d982ab73f9fb1acbff61350c482f186e23603d0 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 7 Aug 2026 22:35:36 -0400 Subject: [PATCH 1/2] Bulk status/close: enforce Workflow Designer rules (#21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bulk status changes previously bypassed the workflow entirely — the model carried an explicit "admin-only escape hatch" note — so bulk edit could drive tickets through transitions the designer forbids and skip comments the designer requires. BulkOperationsModel now applies the same rules as the single-ticket path: - Transitions absent from status_transitions are refused per ticket and reported with a reason, instead of being forced through. - requires_comment is checked up front across the whole selection, so a batch is rejected before any ticket is mutated rather than half-applied. - The reason is persisted as a comment on each ticket changed, matching what a single-ticket close records. - Tickets already in the target status are a no-op success, not a failure. requires_admin needs no extra check: api/bulk_operation.php already gates the endpoint on admin. Client: both bulk modals now collect a reason, the close path gets a real modal instead of a bare confirm, and per-ticket skip reasons surface in the result toast instead of a bare failure count. --- api/bulk_operation.php | 11 ++- assets/js/dashboard.js | 103 +++++++++++++++++++++++--- models/BulkOperationsModel.php | 128 +++++++++++++++++++++++++++++++-- 3 files changed, 225 insertions(+), 17 deletions(-) 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/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index 713e28f..5ae4533 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++; @@ -350,6 +437,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. */ From 1d03800ab2db1373e50c336b1cf97e5be68dd63f Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 7 Aug 2026 22:40:48 -0400 Subject: [PATCH 2/2] Widen bulk_operations.status so partial bulk results can be recorded (#21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while verifying #21 against the live schema: the model writes 'completed_with_errors' (21 chars) when a bulk operation finishes with per-ticket failures, but bulk_operations.status was varchar(20), so the write failed with "Data too long for column 'status'". This was latent — bulk status changes previously forced every transition through, so failed was always 0. Now that they honour the Workflow Designer, a partially-skipped batch is a normal outcome and hits it. - migrations/001 widens the column to varchar(32) (idempotent). - The baseline is updated to match, for fresh installs. - The bookkeeping UPDATE is wrapped in a try/catch: it runs after the ticket changes are committed, so an instance deployed ahead of its migrations must not turn a completed operation into an error response. Verified against the live database with a disposable-ticket harness: comment-required rejection changes nothing, undefined transitions are refused per ticket with a reason, allowed transitions still work, mixed batches apply the valid half, and an already-Closed ticket is a no-op. --- migrations/000_baseline.sql | 3 ++- .../001_widen_bulk_operations_status.sql | 12 ++++++++++ models/BulkOperationsModel.php | 24 ++++++++++++------- 3 files changed, 30 insertions(+), 9 deletions(-) create mode 100644 migrations/001_widen_bulk_operations_status.sql 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 5ae4533..0b601d4 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -377,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)) {