Persist status-change comments transactionally with the update (#37)

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV

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);
+// Post the comment and apply the status change in one transaction, so a
-if (empty($updateResult['success'])) {
+// failure partway through can't leave a "reason" comment persisted with no
-    error_log('ticket_status_api: updateTicket failed for ticket ' . $ticketId
+// matching status change (previously these were two independent writes with
-        . ': ' . ($updateResult['error'] ?? 'unknown'));
+// 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;

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'] ?? ''));
+                    $statusChangeComment = trim((string)($data['comment'] ?? $data['comment_text'] ?? ''));
-                    if ($comment === '') {
+                    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
+            // A comment accompanying a status change (required or optional) is
-            $expectedUpdatedAt = $data['expected_updated_at'] ?? null;
+            // persisted in the SAME transaction as the status update below, so
-            $result = $this->ticketModel->updateTicket($updateData, $this->userId, $expectedUpdatedAt);
+            // 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
+            $result = null;
-            if (!$result['success']) {
+            $this->conn->begin_transaction();
-                $response = [
+            try {
-                    'success' => false,
+                if ($statusChangeComment !== '' && $currentTicket['status'] !== $updateData['status']) {
-                    'error' => $result['error'] ?? 'Failed to update ticket in database'
+                    $commentResult = $this->commentModel->addComment($id, [
-                ];
+                        'user_name' => $this->currentUser['display_name'] ?? $this->currentUser['username'] ?? 'User',
-                if (!empty($result['conflict'])) {
+                        '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']) && $this->userId) {
-            if (isset($data['visibility'])) {
+                $this->auditLog->log(
-                $visResult = $this->ticketModel->updateVisibility($id, $data['visibility'], $visibilityGroups, $this->userId);
+                    $this->userId,
-                if ($visResult && $this->userId) {
+                    'update',
-                    $this->auditLog->log(
+                    'ticket',
-                        $this->userId,
+                    (string)$id,
-                        'update',
+                    [
-                        'ticket',
+                        'field'      => 'visibility',
-                        (string)$id,
+                        'from'       => $currentTicket['visibility'] ?? 'public',
-                        [
+                        'to'         => $data['visibility'],
-                            'field'      => 'visibility',
+                        'groups'     => $visibilityGroups
-                            'from'       => $currentTicket['visibility'] ?? 'public',
+                    ]
-                            'to'         => $data['visibility'],
+                );
-                            'groups'     => $visibilityGroups
-                        ]
-                    );
-                }
             }
 
             // Log ticket update to audit log — only the changed fields (delta)

assets/js/base.js

@@ -2858,8 +2858,11 @@
      TICKET STATUS CHANGE (comment-aware)
      lt.ticketStatus.submit(ticketId, newStatus, { comment? }) → Promise<data>
      Posts /api/update_ticket.php. If the server rejects with
-     requires_comment, opens a comment modal, persists the comment via
+     requires_comment, opens a comment modal, then retries the update once
-     /api/add_comment.php, then retries the update once WITH the comment.
+     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.
+          // Retry with the comment included — update_ticket.php persists it
-          return api.post('/api/add_comment.php', { ticket_id: id, comment_text: comment })
+          // transactionally with the status update itself.
-            .then(() => api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment }));
+          return api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment });
         });
       });
     },

assets/js/ticket.js

@@ -713,12 +713,13 @@ function updateTicketStatus() {
                 return;
             }
             cleanup(true);
-            // Post comment first (persists it), then change status with the same
+            // The comment is sent as part of the status-change request itself
-            // comment included so the server's requires_comment check passes.
+            // (update_ticket.php persists it in the same DB transaction as the
-            const ticketId = getTicketIdFromUrl();
+            // status update) rather than as a separate prior add_comment.php
-            lt.api.post('/api/add_comment.php', { ticket_id: ticketId, comment_text: comment })
+            // call — previously those were two independent, non-transactional
-            .then(() => performStatusChange(statusSelect, selectedOption, newStatus, comment))
+            // writes, so a failure partway through could leave the "reason"
-            .catch(() => performStatusChange(statusSelect, selectedOption, newStatus, comment));
+            // 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);