From ef4d0c4f93b78325b5fd3d71b9d83530ae67fa0c Mon Sep 17 00:00:00 2001 From: Joel Wedemire Date: Sat, 29 Aug 2026 11:09:42 -0400 Subject: [PATCH] A system note is a ticket message source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge and Split both 500ed. The audit note each writes uses `source = 'system'` and the column is `enum('web','email')`, so the insert threw inside the transaction and nothing committed — which is why the operator saw no state change at all rather than a half-done merge. Widened to an enum rather than a varchar, and the reasoning is the same test that made `tickets.status` a varchar in `2026_04_17_000002`: statuses became operator-definable rows, so their domain is open. `source` is the opposite — all three values are set by code, an operator can never add a fourth, and the enum is what keeps a typo'd source loud instead of a silently unfilterable row. `TicketMessage::system()` is now the only place a system note is written. Three scattered literals is how this reopens. A SECOND 500 ON THE SAME ACTION, unreported and one longer title away. `split()` appended ' (split)' to a title the product validates at max:255, so splitting a full-length title produced 263 characters and the identical hard failure. Bounded at the write rather than widened, and the distinction is written down: widen when the data is right and the column is wrong; bound when the column expresses a real product rule. 255 is the product's own limit, so a 263-character title would be un-editable forever — the edit form would bounce it every time. And `split()` now honours its own comment. Line 682 has always said "exclude system notes" while the code copied them; it never mattered because no system row could exist. The moment the enum accepts them, splitting a merged ticket would carry "Merged from ticket QAX-0005." onto a ticket that was never merged. --- ...system_note_is_a_ticket_message_source.php | 66 ++++++++++++++++ src/Http/Controllers/TicketController.php | 76 +++++++++++++------ src/Models/TicketMessage.php | 38 ++++++++++ 3 files changed, 157 insertions(+), 23 deletions(-) create mode 100644 database/migrations/2026_08_29_000001_a_system_note_is_a_ticket_message_source.php diff --git a/database/migrations/2026_08_29_000001_a_system_note_is_a_ticket_message_source.php b/database/migrations/2026_08_29_000001_a_system_note_is_a_ticket_message_source.php new file mode 100644 index 0000000..bcd9b12 --- /dev/null +++ b/database/migrations/2026_08_29_000001_a_system_note_is_a_ticket_message_source.php @@ -0,0 +1,66 @@ +source` verbatim when duplicating a thread, which + * introduces no fourth value. So the set is closed at three and this runs once. + * + * ── WHY AN ENUM AND NOT A VARCHAR ─────────────────────────────────────────── + * + * `tickets.status` went the other way — `2026_04_17_000002` deliberately widened it + * from an enum to `varchar(50)` — and the difference is the test worth writing down: + * statuses became OPERATOR-DEFINABLE rows in `ticketing_statuses`, so no enum could + * ever hold them. `source` is the opposite: all three values are set by code, an + * operator has no way to add a fourth, and the enum is what makes a typo'd source a + * loud failure rather than a silently unfilterable row. + * + * The write side is bounded too — `TicketMessage::system()` is now the only place a + * system note is created, so the three scattered `'source' => 'system'` literals that + * caused this cannot reappear one at a time. + */ +return new class extends Migration +{ + public function up(): void + { + DB::statement( + "ALTER TABLE ticket_messages + MODIFY COLUMN source ENUM('web','email','system') NOT NULL DEFAULT 'web'" + ); + } + + public function down(): void + { + // Rewrite the rows first: narrowing an enum that still holds 'system' turns + // those rows into the empty string rather than failing, which would leave the + // merge and split notes present but unattributable. 'web' is the honest + // fallback — they were written by a person pressing a button in the app. + DB::table('ticket_messages')->where('source', 'system')->update(['source' => 'web']); + + DB::statement( + "ALTER TABLE ticket_messages + MODIFY COLUMN source ENUM('web','email') NOT NULL DEFAULT 'web'" + ); + } +}; diff --git a/src/Http/Controllers/TicketController.php b/src/Http/Controllers/TicketController.php index 59fa4d6..ba72f22 100644 --- a/src/Http/Controllers/TicketController.php +++ b/src/Http/Controllers/TicketController.php @@ -626,13 +626,7 @@ class TicketController extends Controller ->update(['ticket_id' => $ticket->id]); // Add a system note marking the merge - TicketMessage::create([ - 'ticket_id' => $ticket->id, - 'user_id' => Auth::id(), - 'body' => "Merged from ticket {$source->number}.", - 'is_internal' => true, - 'source' => 'system', - ]); + TicketMessage::system($ticket, "Merged from ticket {$source->number}."); // Add source's submitter as a participant (if not already the submitter of target) if ($source->submitter_id !== $ticket->submitter_id) { @@ -672,16 +666,31 @@ class TicketController extends Controller 'submitter_id' => $ticket->submitter_id, 'assigned_to' => $ticket->assigned_to, 'project_id' => $ticket->project_id, - 'title' => $ticket->title . ' (split)', + 'title' => static::splitTitle($ticket->title), 'description' => $ticket->description, 'status' => 'open', 'priority_id' => $ticket->priority_id, 'due_date' => $ticket->due_date, ]); - // Duplicate messages (exclude system notes) + /* + * Duplicate messages, EXCLUDING system notes. + * + * This comment has always said "exclude" and the code never did. It cost + * nothing while the enum rejected 'system', because no system row could + * exist to copy — #415 makes them writable, so without this a split of a + * previously-merged ticket would carry "Merged from ticket QAX-0005." into + * a brand-new ticket that was never merged with anything. + * + * A system note is an audit record of something that happened to THIS + * ticket. The copy gets its own, written below. + */ $ticket->load('messages'); foreach ($ticket->messages as $msg) { + if ($msg->isSystem()) { + continue; + } + TicketMessage::create([ 'ticket_id' => $newTicket->id, 'user_id' => $msg->user_id, @@ -693,20 +702,8 @@ class TicketController extends Controller } // Cross-reference notes on both tickets - TicketMessage::create([ - 'ticket_id' => $ticket->id, - 'user_id' => Auth::id(), - 'body' => "Split: new ticket {$newTicket->number} created from this ticket.", - 'is_internal' => true, - 'source' => 'system', - ]); - TicketMessage::create([ - 'ticket_id' => $newTicket->id, - 'user_id' => Auth::id(), - 'body' => "Split from ticket {$ticket->number}.", - 'is_internal' => true, - 'source' => 'system', - ]); + TicketMessage::system($ticket, "Split: new ticket {$newTicket->number} created from this ticket."); + TicketMessage::system($newTicket, "Split from ticket {$ticket->number}."); return $newTicket; }); @@ -715,6 +712,39 @@ class TicketController extends Controller ->with('success', "Ticket {$newTicket->number} created from split."); } + /** + * The split ticket's title: the original plus a suffix, guaranteed to fit. + * + * `tickets.title` is varchar(255) and both store() and update() validate the + * title at `max:255`. Appending " (split)" to a title already at the limit + * produced 263 characters and the same hard 500 as #415 — a second, unreported + * instance of it on the very action being fixed, needing only a long enough + * title to trigger. + * + * ── WHY THIS IS BOUNDED AND NOT WIDENED ───────────────────────────────── + * + * The sibling defect in central-radius (#410) was fixed the other way: a + * 510-character skip reason was widened into the column rather than shortened, + * because the reason was the point and the column was simply too small. + * + * The rule that separates them is what the limit MEANS. 255 here is not an + * incidental column width — it is the product's own declared, user-facing title + * limit, enforced by two validation rules. Widening the column to fit 263 would + * mint rows the edit form then refuses to save, so the ticket could never be + * corrected by anyone: a row the product will not let you fix is worse than a + * truncated title. So: widen when the data is right and the column is wrong; + * bound when the column expresses a real product rule. + * + * Multibyte-safe by character, because varchar(255) counts characters. + */ + private static function splitTitle(string $title): string + { + $suffix = ' (split)'; + $room = 255 - mb_strlen($suffix); + + return (mb_strlen($title) > $room ? mb_substr($title, 0, $room) : $title).$suffix; + } + /** * Add a participant to a ticket. */ diff --git a/src/Models/TicketMessage.php b/src/Models/TicketMessage.php index ab5cd91..08cabac 100644 --- a/src/Models/TicketMessage.php +++ b/src/Models/TicketMessage.php @@ -5,9 +5,20 @@ namespace Dashboard\Ticketing\Models; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\HasMany; +use Illuminate\Support\Facades\Auth; class TicketMessage extends Model { + /** + * Every value `ticket_messages.source` accepts, written down once. + * + * The column is an enum and this is the same list, so the two can be compared + * rather than remembered. #415 was three `'source' => 'system'` literals against + * a column that only knew 'web' and 'email' — the value existed in the code for + * as long as merge and split have, and nowhere in the schema. + */ + public const SOURCES = ['web', 'email', 'system']; + protected $fillable = [ 'ticket_id', 'user_id', 'author_email', 'body', 'is_internal', 'source', 'email_message_id', @@ -17,6 +28,33 @@ class TicketMessage extends Model 'is_internal' => 'boolean', ]; + /** + * THE ONLY WAY A SYSTEM NOTE IS WRITTEN. + * + * Merge writes one and split writes two, and before this they were three + * separate literals in one controller — which is how all three carried a + * `source` the column rejected, and how a fourth would have joined them. + * + * A system note is always internal: it is the audit trail of an agent action, + * addressed to whoever works the ticket next, and never to the submitter. + */ + public static function system(Ticket|int $ticket, string $body): self + { + return static::create([ + 'ticket_id' => $ticket instanceof Ticket ? $ticket->id : $ticket, + 'user_id' => Auth::id(), + 'body' => $body, + 'is_internal' => true, + 'source' => 'system', + ]); + } + + /** A note this class wrote, rather than a person or a mailbox. */ + public function isSystem(): bool + { + return $this->source === 'system'; + } + public function ticket(): BelongsTo { return $this->belongsTo(Ticket::class, 'ticket_id');