Move ApiTicketController out of update_ticket.php into its own file (#111)
The partial-update controller (status transitions incl. requires_comment with the comment in the same transaction, field edits, visibility, audit delta, status-change notifications) was defined inline inside api/update_ticket.php, so nothing else could reuse it. Moved it verbatim to controllers/ApiTicketController.php so the MCP update_status tool can run the exact same code path as the web UI; update_ticket.php now just require_once's it. The class body is byte-identical to the original (diffed against HEAD, modulo the 4-space dedent). Verified the web endpoint over real HTTP with a real session + CSRF: Open -> In Progress succeeds, In Progress -> Closed without a comment is refused with requires_comment (400), and with a comment closes the ticket and persists the reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X
This commit is contained in:
Claude Opus 5.5
+1
-256
@@ -62,262 +62,7 @@ try {
|
||||
$userId = $currentUser['user_id'];
|
||||
$isAdmin = $currentUser['is_admin'] ?? false;
|
||||
|
||||
// Updated controller class that handles partial updates
|
||||
class ApiTicketController
|
||||
{
|
||||
private $conn;
|
||||
private $ticketModel;
|
||||
private $commentModel;
|
||||
private $auditLog;
|
||||
private $workflowModel;
|
||||
private $userId;
|
||||
private $isAdmin;
|
||||
private $currentUser;
|
||||
|
||||
public function __construct($conn, $userId = null, $isAdmin = false, $currentUser = [])
|
||||
{
|
||||
$this->conn = $conn;
|
||||
$this->ticketModel = new TicketModel($conn);
|
||||
$this->commentModel = new CommentModel($conn);
|
||||
$this->auditLog = new AuditLogModel($conn);
|
||||
$this->workflowModel = new WorkflowModel($conn);
|
||||
$this->userId = $userId;
|
||||
$this->isAdmin = $isAdmin;
|
||||
$this->currentUser = $currentUser;
|
||||
}
|
||||
|
||||
public function update($id, $data)
|
||||
{
|
||||
// First, get the current ticket data to fill in missing fields
|
||||
$currentTicket = $this->ticketModel->getTicketById($id);
|
||||
if (!$currentTicket) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Ticket not found'
|
||||
];
|
||||
}
|
||||
|
||||
// Visibility check: return 404 for tickets the user cannot access
|
||||
if (!$this->ticketModel->canUserAccessTicket($currentTicket, $this->currentUser)) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Ticket not found',
|
||||
'http_status' => 404
|
||||
];
|
||||
}
|
||||
|
||||
// Any authenticated team member can update tickets.
|
||||
// Admin-only operations (delete, bulk actions) are enforced separately.
|
||||
|
||||
// Merge current data with updates, keeping existing values for missing fields
|
||||
$updateData = [
|
||||
'ticket_id' => $id,
|
||||
'title' => $data['title'] ?? $currentTicket['title'],
|
||||
'description' => $data['description'] ?? $currentTicket['description'],
|
||||
'category' => $data['category'] ?? $currentTicket['category'],
|
||||
'type' => $data['type'] ?? $currentTicket['type'],
|
||||
'status' => $data['status'] ?? $currentTicket['status'],
|
||||
'priority' => isset($data['priority']) ? (int)$data['priority'] : (int)$currentTicket['priority']
|
||||
];
|
||||
|
||||
// Validate required fields
|
||||
if (empty($updateData['title'])) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Title cannot be empty',
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
|
||||
// Validate priority range
|
||||
if ($updateData['priority'] < 1 || $updateData['priority'] > 5) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Priority must be between 1 and 5',
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
|
||||
// Validate visibility BEFORE any DB write so a bad payload can't leave the
|
||||
// ticket half-updated (core fields committed but request reported as failed).
|
||||
$visibilityGroups = null;
|
||||
if (isset($data['visibility'])) {
|
||||
$visibilityGroups = $data['visibility_groups'] ?? null;
|
||||
// Convert array to comma-separated string if needed
|
||||
if (is_array($visibilityGroups)) {
|
||||
$visibilityGroups = implode(',', array_map('trim', $visibilityGroups));
|
||||
}
|
||||
|
||||
// Authorization: only an admin or the ticket's creator may change
|
||||
// visibility. Enforce only when the requested visibility actually
|
||||
// differs so ordinary edits that re-send the same value aren't blocked.
|
||||
$currentVisibility = $currentTicket['visibility'] ?? 'public';
|
||||
$currentGroups = $currentTicket['visibility_groups'] ?? null;
|
||||
$groupsProvided = array_key_exists('visibility_groups', $data);
|
||||
$visibilityChanged = ($data['visibility'] !== $currentVisibility)
|
||||
|| ($groupsProvided && (string)$visibilityGroups !== (string)$currentGroups);
|
||||
if ($visibilityChanged) {
|
||||
$isCreator = $this->userId !== null
|
||||
&& (int)($currentTicket['created_by'] ?? 0) === (int)$this->userId;
|
||||
if (!$this->isAdmin && !$isCreator) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'You do not have permission to change ticket visibility',
|
||||
'http_status' => 403
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
// Internal visibility requires at least one group
|
||||
if ($data['visibility'] === 'internal' && (empty($visibilityGroups) || trim($visibilityGroups) === '')) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Internal visibility requires at least one group to be specified',
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
// Validate status transition using workflow model
|
||||
if ($currentTicket['status'] !== $updateData['status']) {
|
||||
$allowed = $this->workflowModel->isTransitionAllowed(
|
||||
$currentTicket['status'],
|
||||
$updateData['status'],
|
||||
$this->isAdmin
|
||||
);
|
||||
|
||||
if (!$allowed) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Status transition not allowed: ' . $currentTicket['status'] . ' → ' . $updateData['status']
|
||||
];
|
||||
}
|
||||
|
||||
// Enforce requires_comment transitions server-side.
|
||||
if ($this->workflowModel->transitionRequiresComment($currentTicket['status'], $updateData['status'])) {
|
||||
$statusChangeComment = trim((string)($data['comment'] ?? $data['comment_text'] ?? ''));
|
||||
if ($statusChangeComment === '') {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'A comment is required for this status change',
|
||||
'requires_comment' => true,
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 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'] ?? ''));
|
||||
|
||||
$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;
|
||||
}
|
||||
|
||||
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)
|
||||
if ($this->userId) {
|
||||
$trackFields = ['title', 'priority', 'status', 'description', 'category', 'type'];
|
||||
$delta = [];
|
||||
foreach ($trackFields as $field) {
|
||||
$oldVal = (string)($currentTicket[$field] ?? '');
|
||||
$newVal = (string)($updateData[$field] ?? '');
|
||||
if ($oldVal !== $newVal) {
|
||||
$delta[$field] = ['from' => $oldVal, 'to' => $newVal];
|
||||
}
|
||||
}
|
||||
if (!empty($delta)) {
|
||||
$this->auditLog->logTicketUpdate($this->userId, $id, $delta);
|
||||
}
|
||||
}
|
||||
|
||||
// Notify on status change (global notify list + watchers)
|
||||
if ($currentTicket['status'] !== $updateData['status']) {
|
||||
$changedBy = $this->currentUser['display_name'] ?? $this->currentUser['username'] ?? null;
|
||||
NotificationHelper::sendStatusChangeNotification(
|
||||
$id,
|
||||
$currentTicket['status'],
|
||||
$updateData['status'],
|
||||
$updateData['title'],
|
||||
$changedBy,
|
||||
$currentTicket['visibility'] ?? 'public'
|
||||
);
|
||||
NotificationHelper::notifyWatchers(
|
||||
$this->conn,
|
||||
$id,
|
||||
$updateData['title'],
|
||||
'status_changed',
|
||||
['old_status' => $currentTicket['status'], 'new_status' => $updateData['status'], 'changed_by' => $changedBy],
|
||||
(int)$this->userId,
|
||||
$currentTicket['visibility'] ?? 'public'
|
||||
);
|
||||
}
|
||||
|
||||
return [
|
||||
'success' => true,
|
||||
'status' => $updateData['status'],
|
||||
'priority' => $updateData['priority'],
|
||||
'updated_at' => date('Y-m-d H:i:s'),
|
||||
'message' => 'Ticket updated successfully',
|
||||
'csrf_token' => $GLOBALS['newCsrfToken'] ?? null
|
||||
];
|
||||
}
|
||||
}
|
||||
require_once dirname(__DIR__) . '/controllers/ApiTicketController.php';
|
||||
|
||||
// Use centralized database connection
|
||||
$conn = Database::getConnection();
|
||||
|
||||
@@ -0,0 +1,275 @@
|
||||
<?php
|
||||
|
||||
/**
|
||||
* Partial ticket updates for the web UI (api/update_ticket.php) and the MCP
|
||||
* update_status tool: workflow-validated status transitions (including
|
||||
* requires_comment, with the comment persisted in the same transaction),
|
||||
* priority/field edits, visibility changes (admin or creator only), audit
|
||||
* log delta, and status-change notifications. Returns result arrays; callers
|
||||
* own HTTP concerns (sessions, CSRF, response codes) and must invalidate the
|
||||
* stats cache on success.
|
||||
*
|
||||
* Moved verbatim out of api/update_ticket.php so the MCP endpoint runs the
|
||||
* exact same code path as the web UI (tinker_tickets#111).
|
||||
*/
|
||||
|
||||
require_once dirname(__DIR__) . '/models/TicketModel.php';
|
||||
require_once dirname(__DIR__) . '/models/CommentModel.php';
|
||||
require_once dirname(__DIR__) . '/models/AuditLogModel.php';
|
||||
require_once dirname(__DIR__) . '/models/WorkflowModel.php';
|
||||
require_once dirname(__DIR__) . '/helpers/NotificationHelper.php';
|
||||
class ApiTicketController
|
||||
{
|
||||
private $conn;
|
||||
private $ticketModel;
|
||||
private $commentModel;
|
||||
private $auditLog;
|
||||
private $workflowModel;
|
||||
private $userId;
|
||||
private $isAdmin;
|
||||
private $currentUser;
|
||||
|
||||
public function __construct($conn, $userId = null, $isAdmin = false, $currentUser = [])
|
||||
{
|
||||
$this->conn = $conn;
|
||||
$this->ticketModel = new TicketModel($conn);
|
||||
$this->commentModel = new CommentModel($conn);
|
||||
$this->auditLog = new AuditLogModel($conn);
|
||||
$this->workflowModel = new WorkflowModel($conn);
|
||||
$this->userId = $userId;
|
||||
$this->isAdmin = $isAdmin;
|
||||
$this->currentUser = $currentUser;
|
||||
}
|
||||
|
||||
public function update($id, $data)
|
||||
{
|
||||
// First, get the current ticket data to fill in missing fields
|
||||
$currentTicket = $this->ticketModel->getTicketById($id);
|
||||
if (!$currentTicket) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Ticket not found'
|
||||
];
|
||||
}
|
||||
|
||||
// Visibility check: return 404 for tickets the user cannot access
|
||||
if (!$this->ticketModel->canUserAccessTicket($currentTicket, $this->currentUser)) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Ticket not found',
|
||||
'http_status' => 404
|
||||
];
|
||||
}
|
||||
|
||||
// Any authenticated team member can update tickets.
|
||||
// Admin-only operations (delete, bulk actions) are enforced separately.
|
||||
|
||||
// Merge current data with updates, keeping existing values for missing fields
|
||||
$updateData = [
|
||||
'ticket_id' => $id,
|
||||
'title' => $data['title'] ?? $currentTicket['title'],
|
||||
'description' => $data['description'] ?? $currentTicket['description'],
|
||||
'category' => $data['category'] ?? $currentTicket['category'],
|
||||
'type' => $data['type'] ?? $currentTicket['type'],
|
||||
'status' => $data['status'] ?? $currentTicket['status'],
|
||||
'priority' => isset($data['priority']) ? (int)$data['priority'] : (int)$currentTicket['priority']
|
||||
];
|
||||
|
||||
// Validate required fields
|
||||
if (empty($updateData['title'])) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Title cannot be empty',
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
|
||||
// Validate priority range
|
||||
if ($updateData['priority'] < 1 || $updateData['priority'] > 5) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Priority must be between 1 and 5',
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
|
||||
// Validate visibility BEFORE any DB write so a bad payload can't leave the
|
||||
// ticket half-updated (core fields committed but request reported as failed).
|
||||
$visibilityGroups = null;
|
||||
if (isset($data['visibility'])) {
|
||||
$visibilityGroups = $data['visibility_groups'] ?? null;
|
||||
// Convert array to comma-separated string if needed
|
||||
if (is_array($visibilityGroups)) {
|
||||
$visibilityGroups = implode(',', array_map('trim', $visibilityGroups));
|
||||
}
|
||||
|
||||
// Authorization: only an admin or the ticket's creator may change
|
||||
// visibility. Enforce only when the requested visibility actually
|
||||
// differs so ordinary edits that re-send the same value aren't blocked.
|
||||
$currentVisibility = $currentTicket['visibility'] ?? 'public';
|
||||
$currentGroups = $currentTicket['visibility_groups'] ?? null;
|
||||
$groupsProvided = array_key_exists('visibility_groups', $data);
|
||||
$visibilityChanged = ($data['visibility'] !== $currentVisibility)
|
||||
|| ($groupsProvided && (string)$visibilityGroups !== (string)$currentGroups);
|
||||
if ($visibilityChanged) {
|
||||
$isCreator = $this->userId !== null
|
||||
&& (int)($currentTicket['created_by'] ?? 0) === (int)$this->userId;
|
||||
if (!$this->isAdmin && !$isCreator) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'You do not have permission to change ticket visibility',
|
||||
'http_status' => 403
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
// Internal visibility requires at least one group
|
||||
if ($data['visibility'] === 'internal' && (empty($visibilityGroups) || trim($visibilityGroups) === '')) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Internal visibility requires at least one group to be specified',
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
// Validate status transition using workflow model
|
||||
if ($currentTicket['status'] !== $updateData['status']) {
|
||||
$allowed = $this->workflowModel->isTransitionAllowed(
|
||||
$currentTicket['status'],
|
||||
$updateData['status'],
|
||||
$this->isAdmin
|
||||
);
|
||||
|
||||
if (!$allowed) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Status transition not allowed: ' . $currentTicket['status'] . ' → ' . $updateData['status']
|
||||
];
|
||||
}
|
||||
|
||||
// Enforce requires_comment transitions server-side.
|
||||
if ($this->workflowModel->transitionRequiresComment($currentTicket['status'], $updateData['status'])) {
|
||||
$statusChangeComment = trim((string)($data['comment'] ?? $data['comment_text'] ?? ''));
|
||||
if ($statusChangeComment === '') {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'A comment is required for this status change',
|
||||
'requires_comment' => true,
|
||||
'http_status' => 400
|
||||
];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 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'] ?? ''));
|
||||
|
||||
$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;
|
||||
}
|
||||
|
||||
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)
|
||||
if ($this->userId) {
|
||||
$trackFields = ['title', 'priority', 'status', 'description', 'category', 'type'];
|
||||
$delta = [];
|
||||
foreach ($trackFields as $field) {
|
||||
$oldVal = (string)($currentTicket[$field] ?? '');
|
||||
$newVal = (string)($updateData[$field] ?? '');
|
||||
if ($oldVal !== $newVal) {
|
||||
$delta[$field] = ['from' => $oldVal, 'to' => $newVal];
|
||||
}
|
||||
}
|
||||
if (!empty($delta)) {
|
||||
$this->auditLog->logTicketUpdate($this->userId, $id, $delta);
|
||||
}
|
||||
}
|
||||
|
||||
// Notify on status change (global notify list + watchers)
|
||||
if ($currentTicket['status'] !== $updateData['status']) {
|
||||
$changedBy = $this->currentUser['display_name'] ?? $this->currentUser['username'] ?? null;
|
||||
NotificationHelper::sendStatusChangeNotification(
|
||||
$id,
|
||||
$currentTicket['status'],
|
||||
$updateData['status'],
|
||||
$updateData['title'],
|
||||
$changedBy,
|
||||
$currentTicket['visibility'] ?? 'public'
|
||||
);
|
||||
NotificationHelper::notifyWatchers(
|
||||
$this->conn,
|
||||
$id,
|
||||
$updateData['title'],
|
||||
'status_changed',
|
||||
['old_status' => $currentTicket['status'], 'new_status' => $updateData['status'], 'changed_by' => $changedBy],
|
||||
(int)$this->userId,
|
||||
$currentTicket['visibility'] ?? 'public'
|
||||
);
|
||||
}
|
||||
|
||||
return [
|
||||
'success' => true,
|
||||
'status' => $updateData['status'],
|
||||
'priority' => $updateData['priority'],
|
||||
'updated_at' => date('Y-m-d H:i:s'),
|
||||
'message' => 'Ticket updated successfully',
|
||||
'csrf_token' => $GLOBALS['newCsrfToken'] ?? null
|
||||
];
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user