From 6adbb29964c2a43bd85f432881ec4403baa8012d Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 12:37:37 -0400 Subject: [PATCH] Fire notifications and fix audit_log shape for bulk status changes (#67, #74) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BulkOperationsModel's bulk_close/bulk_status paths had zero references to NotificationHelper — the exact same status transition (e.g. Open->Closed) silently produced no Matrix/watcher notification when performed via bulk actions, while the single-ticket edit page and Bearer API both notify on every status change. Separately, their audit_log entries used a bare ['status' => 'Closed', ...] shape instead of the {'status': {'from': X, 'to': Y}} shape every other status-change path uses, which broke two downstream consumers: TicketView.php's timeline fell back to a generic "updated this ticket" instead of "updated status", and notifications.php's $details['status']['from'] on a string produced a broken "? -> ?" notification title. Fixed the audit_log shape for both operation types, and added a notification queue collected during the per-ticket loop and flushed only after a successful commit (so atomic-mode rollback correctly sends zero notifications, matching how nothing else about a rolled- back batch takes effect either). Also fixed an incidental bug found while matching this to the single-ticket path: update_ticket.php's notifyWatchers() call never passed the ticket's visibility, silently defaulting to 'public' and always including the shared notify list even for confidential/internal tickets — the exact leak #71 fixed elsewhere in NotificationHelper itself, just never reaching this call site. Verified against real MariaDB with a real local webhook-capturing server: bulk_close correctly fires sendStatusChangeNotification() + notifyWatchers() with the right old/new status and a redacted title for a confidential ticket; audit_log rows show the correct {from,to} shape; and an atomic-mode rollback (one ticket's transition invalid) sends zero notifications and leaves both tickets unchanged. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv --- api/update_ticket.php | 3 +- models/BulkOperationsModel.php | 63 ++++++++++++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/api/update_ticket.php b/api/update_ticket.php index 67e42a8..dded313 100644 --- a/api/update_ticket.php +++ b/api/update_ticket.php @@ -276,7 +276,8 @@ try { $updateData['title'], 'status_changed', ['old_status' => $currentTicket['status'], 'new_status' => $updateData['status'], 'changed_by' => $changedBy], - (int)$this->userId + (int)$this->userId, + $currentTicket['visibility'] ?? 'public' ); } diff --git a/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index 0b601d4..a5d68ef 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -125,13 +125,23 @@ class BulkOperationsModel $processed = 0; $failed = 0; $errors = []; + // Status-change notifications collected during the loop below and + // sent only after a successful commit, matching how the single-ticket + // and Bearer API paths never notify for a change that didn't durably + // land (and how an atomic-mode rollback must not fire any at all). + $notificationQueue = []; // Load required models require_once dirname(__DIR__) . '/models/TicketModel.php'; require_once dirname(__DIR__) . '/models/AuditLogModel.php'; + require_once dirname(__DIR__) . '/models/UserModel.php'; + require_once dirname(__DIR__) . '/helpers/NotificationHelper.php'; $ticketModel = new TicketModel($this->conn); $auditLogModel = new AuditLogModel($this->conn); + $userModel = new UserModel($this->conn); + $actor = $operation['performed_by'] ? $userModel->getUserById((int)$operation['performed_by']) : null; + $changedByDisplay = $actor['display_name'] ?? $actor['username'] ?? null; // Batch load all tickets in one query to eliminate N+1 problem $ticketsById = $ticketModel->getTicketsByIds($ticketIds); @@ -221,8 +231,18 @@ class BulkOperationsModel 'update', 'ticket', $ticketId, - ['status' => 'Closed', 'bulk_operation_id' => $operationId] + [ + 'status' => ['from' => $currentTicket['status'], 'to' => 'Closed'], + 'bulk_operation_id' => $operationId, + ] ); + $notificationQueue[] = [ + 'ticketId' => $ticketId, + 'title' => $currentTicket['title'], + 'visibility' => $currentTicket['visibility'] ?? 'public', + 'oldStatus' => $currentTicket['status'], + 'newStatus' => 'Closed', + ]; } } break; @@ -291,8 +311,18 @@ class BulkOperationsModel 'update', 'ticket', $ticketId, - ['status' => $parameters['status'], 'bulk_operation_id' => $operationId] + [ + 'status' => ['from' => $currentTicket['status'], 'to' => $parameters['status']], + 'bulk_operation_id' => $operationId, + ] ); + $notificationQueue[] = [ + 'ticketId' => $ticketId, + 'title' => $currentTicket['title'], + 'visibility' => $currentTicket['visibility'] ?? 'public', + 'oldStatus' => $currentTicket['status'], + 'newStatus' => $parameters['status'], + ]; } } } @@ -364,6 +394,35 @@ class BulkOperationsModel @unlink($path); } } + + // Fire the same Matrix/watcher notifications the single-ticket and + // Bearer API status-change paths send, now that every change in + // this batch is durably committed. Best-effort: a notification + // failure must never turn an otherwise-successful bulk operation + // into an error. + foreach ($notificationQueue as $n) { + try { + NotificationHelper::sendStatusChangeNotification( + $n['ticketId'], + $n['oldStatus'], + $n['newStatus'], + $n['title'], + $changedByDisplay, + $n['visibility'] + ); + NotificationHelper::notifyWatchers( + $this->conn, + $n['ticketId'], + $n['title'], + 'status_changed', + ['old_status' => $n['oldStatus'], 'new_status' => $n['newStatus'], 'changed_by' => $changedByDisplay], + (int)$operation['performed_by'], + $n['visibility'] + ); + } catch (Throwable $e) { + error_log("Bulk operation $operationId: notification failed for ticket {$n['ticketId']}: " . $e->getMessage()); + } + } } catch (Exception $e) { // Rollback on any unexpected error $this->conn->rollback();