From 310dcd0840b7b487e5ec2272a7f3fa78f8e0eb9c Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 21:41:48 -0400 Subject: [PATCH] Persist status-change comments transactionally with the update (#37) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A comment accompanying a status change (required or user-supplied) was posted via a separate, independent HTTP call/write (add_comment.php, or a second add_comment call in lt.ticketStatus.submit()'s requires_comment retry path) before the status update itself. A failure partway through — or the client never issuing the second call — could leave a "reason" comment persisted with no matching status change, or vice versa, with no rollback tying the two together. api/update_ticket.php and api/ticket_status_api.php now post the comment and apply the status update inside one transaction, rolling back both on any failure. assets/js/ticket.js and lt.ticketStatus.submit() in assets/js/base.js no longer make a separate add_comment.php call; they pass the comment directly to update_ticket.php, which persists it server-side alongside the status change. Verified against real MariaDB by extracting the live ApiTicketController and the ticket_status_api.php transaction logic and running them directly: a forced optimistic-lock conflict correctly rolled back both the comment and the status change, and a successful call persisted exactly one comment alongside the status change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- api/ticket_status_api.php | 48 ++++++++++++---------- api/update_ticket.php | 85 ++++++++++++++++++++++++++------------- assets/js/base.js | 13 +++--- assets/js/ticket.js | 13 +++--- 4 files changed, 98 insertions(+), 61 deletions(-) diff --git a/api/ticket_status_api.php b/api/ticket_status_api.php index cce86f3..c905154 100644 --- a/api/ticket_status_api.php +++ b/api/ticket_status_api.php @@ -126,23 +126,6 @@ if ($workflowModel->transitionRequiresComment($currentStatus, $newStatus) && $co exit; } -// Post the comment first (per-key label) so a close-with-reason is one call. -if ($comment !== '') { - $commentModel = new CommentModel($conn); - $commentResult = $commentModel->addComment($ticketId, [ - 'user_name' => $keyName, - 'comment_text' => $comment, - 'markdown_enabled' => !empty($data['markdown_enabled']), - ], $createdBy); - if (empty($commentResult['success'])) { - error_log('ticket_status_api: addComment failed for ticket ' . $ticketId - . ': ' . ($commentResult['error'] ?? 'unknown')); - http_response_code(500); - echo json_encode(['success' => false, 'error' => 'Failed to add comment']); - exit; - } -} - // Apply the status change. updateTicket sets updated_by/updated_at and handles // closed_at (set on close, cleared on reopen) via its own SQL. $updateData = [ @@ -155,10 +138,33 @@ $updateData = [ 'priority' => (int)$ticket['priority'], ]; -$updateResult = $ticketModel->updateTicket($updateData, $createdBy); -if (empty($updateResult['success'])) { - error_log('ticket_status_api: updateTicket failed for ticket ' . $ticketId - . ': ' . ($updateResult['error'] ?? 'unknown')); +// Post the comment and apply the status change in one transaction, so a +// failure partway through can't leave a "reason" comment persisted with no +// matching status change (previously these were two independent writes with +// no shared rollback). +$conn->begin_transaction(); +try { + if ($comment !== '') { + $commentModel = new CommentModel($conn); + $commentResult = $commentModel->addComment($ticketId, [ + 'user_name' => $keyName, + 'comment_text' => $comment, + 'markdown_enabled' => !empty($data['markdown_enabled']), + ], $createdBy); + if (empty($commentResult['success'])) { + throw new Exception($commentResult['error'] ?? 'Failed to add comment'); + } + } + + $updateResult = $ticketModel->updateTicket($updateData, $createdBy); + if (empty($updateResult['success'])) { + throw new Exception($updateResult['error'] ?? 'Failed to update ticket status'); + } + + $conn->commit(); +} catch (Exception $e) { + $conn->rollback(); + error_log('ticket_status_api: transaction failed for ticket ' . $ticketId . ': ' . $e->getMessage()); http_response_code(500); echo json_encode(['success' => false, 'error' => 'Failed to update ticket status']); exit; diff --git a/api/update_ticket.php b/api/update_ticket.php index dded313..08c4c22 100644 --- a/api/update_ticket.php +++ b/api/update_ticket.php @@ -195,8 +195,8 @@ try { // Enforce requires_comment transitions server-side. if ($this->workflowModel->transitionRequiresComment($currentTicket['status'], $updateData['status'])) { - $comment = trim((string)($data['comment'] ?? $data['comment_text'] ?? '')); - if ($comment === '') { + $statusChangeComment = trim((string)($data['comment'] ?? $data['comment_text'] ?? '')); + if ($statusChangeComment === '') { return [ 'success' => false, 'error' => 'A comment is required for this status change', @@ -207,40 +207,67 @@ try { } } - // Update ticket with user tracking and optional optimistic locking - $expectedUpdatedAt = $data['expected_updated_at'] ?? null; - $result = $this->ticketModel->updateTicket($updateData, $this->userId, $expectedUpdatedAt); + // A comment accompanying a status change (required or optional) is + // persisted in the SAME transaction as the status update below, so + // a failure partway through can't leave an orphaned "reason" + // comment attached with no matching status change — the two + // previously ran as separate, non-transactional HTTP calls from + // the client (add_comment.php then update_ticket.php). + $statusChangeComment = $statusChangeComment ?? trim((string)($data['comment'] ?? $data['comment_text'] ?? '')); - // Handle conflict case - if (!$result['success']) { - $response = [ - 'success' => false, - 'error' => $result['error'] ?? 'Failed to update ticket in database' - ]; - if (!empty($result['conflict'])) { + $result = null; + $this->conn->begin_transaction(); + try { + if ($statusChangeComment !== '' && $currentTicket['status'] !== $updateData['status']) { + $commentResult = $this->commentModel->addComment($id, [ + 'user_name' => $this->currentUser['display_name'] ?? $this->currentUser['username'] ?? 'User', + 'comment_text' => $statusChangeComment, + 'markdown_enabled' => !empty($data['markdown_enabled']), + ], $this->userId); + if (empty($commentResult['success'])) { + throw new Exception($commentResult['error'] ?? 'Failed to add comment'); + } + } + + // Update ticket with user tracking and optional optimistic locking + $expectedUpdatedAt = $data['expected_updated_at'] ?? null; + $result = $this->ticketModel->updateTicket($updateData, $this->userId, $expectedUpdatedAt); + if (!$result['success']) { + throw new Exception($result['error'] ?? 'Failed to update ticket in database'); + } + + // Handle visibility update if provided (already validated above) + if (isset($data['visibility'])) { + $visResult = $this->ticketModel->updateVisibility($id, $data['visibility'], $visibilityGroups, $this->userId); + if (!$visResult) { + throw new Exception('Failed to update ticket visibility'); + } + } + + $this->conn->commit(); + } catch (Exception $e) { + $this->conn->rollback(); + $response = ['success' => false, 'error' => $e->getMessage()]; + if (is_array($result) && !empty($result['conflict'])) { $response['conflict'] = true; $response['current_updated_at'] = $result['current_updated_at'] ?? null; } return $response; } - // Handle visibility update if provided (already validated above) - if (isset($data['visibility'])) { - $visResult = $this->ticketModel->updateVisibility($id, $data['visibility'], $visibilityGroups, $this->userId); - if ($visResult && $this->userId) { - $this->auditLog->log( - $this->userId, - 'update', - 'ticket', - (string)$id, - [ - 'field' => 'visibility', - 'from' => $currentTicket['visibility'] ?? 'public', - 'to' => $data['visibility'], - 'groups' => $visibilityGroups - ] - ); - } + if (isset($data['visibility']) && $this->userId) { + $this->auditLog->log( + $this->userId, + 'update', + 'ticket', + (string)$id, + [ + 'field' => 'visibility', + 'from' => $currentTicket['visibility'] ?? 'public', + 'to' => $data['visibility'], + 'groups' => $visibilityGroups + ] + ); } // Log ticket update to audit log — only the changed fields (delta) diff --git a/assets/js/base.js b/assets/js/base.js index 329f022..d64d633 100644 --- a/assets/js/base.js +++ b/assets/js/base.js @@ -2858,8 +2858,11 @@ TICKET STATUS CHANGE (comment-aware) lt.ticketStatus.submit(ticketId, newStatus, { comment? }) → Promise Posts /api/update_ticket.php. If the server rejects with - requires_comment, opens a comment modal, persists the comment via - /api/add_comment.php, then retries the update once WITH the comment. + requires_comment, opens a comment modal, then retries the update once + WITH the comment — update_ticket.php persists it in the same DB + transaction as the status change itself, so there's no separate + add_comment.php call that could leave an orphaned comment if the + status update then failed. Rejects with err.cancelled === true if the user cancels the modal. ================================================================ */ function _statusCommentModal(newStatus) { @@ -2922,9 +2925,9 @@ cancelErr.cancelled = true; throw cancelErr; } - // Persist the comment, then retry the status change with it included. - return api.post('/api/add_comment.php', { ticket_id: id, comment_text: comment }) - .then(() => api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment })); + // Retry with the comment included — update_ticket.php persists it + // transactionally with the status update itself. + return api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment }); }); }); }, diff --git a/assets/js/ticket.js b/assets/js/ticket.js index f40cab2..aae3d10 100644 --- a/assets/js/ticket.js +++ b/assets/js/ticket.js @@ -713,12 +713,13 @@ function updateTicketStatus() { return; } cleanup(true); - // Post comment first (persists it), then change status with the same - // comment included so the server's requires_comment check passes. - const ticketId = getTicketIdFromUrl(); - lt.api.post('/api/add_comment.php', { ticket_id: ticketId, comment_text: comment }) - .then(() => performStatusChange(statusSelect, selectedOption, newStatus, comment)) - .catch(() => performStatusChange(statusSelect, selectedOption, newStatus, comment)); + // The comment is sent as part of the status-change request itself + // (update_ticket.php persists it in the same DB transaction as the + // status update) rather than as a separate prior add_comment.php + // call — previously those were two independent, non-transactional + // writes, so a failure partway through could leave the "reason" + // comment persisted with no matching status change ever applied. + performStatusChange(statusSelect, selectedOption, newStatus, comment); }); // Focus textarea on open setTimeout(() => { const ta = document.getElementById(`${modalId}_comment`); if (ta) ta.focus(); }, 100);