From 0e48949114726b2aa35bdebcf954000a19e4697f Mon Sep 17 00:00:00 2001 From: Joel Wedemire Date: Thu, 27 Aug 2026 19:37:06 -0400 Subject: [PATCH] A ticket reference is never re-issued, and notifications name their recipient (#224, #228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #224 — the next number came from the live rows, so deleting a ticket freed its reference and two tickets could both be QAX-0004. An old email then points at a different ticket than it did. The sequence is now held on the group and only ever moves forward. #228 — ticketing notifications carried no user, so the Notification Log showed the message type where the recipient belongs and the per-person opt-out was ignored: somebody who had opted out still received them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ec3REU4LQmhuchRRn6dZHk --- ...et_number_sequence_to_ticketing_groups.php | 82 +++++++++++++++++++ src/Http/Controllers/TicketController.php | 7 +- .../Controllers/TicketMessageController.php | 11 ++- src/Models/TicketingGroup.php | 55 +++++++++++-- src/Support/TicketingNotifier.php | 33 +++++++- 5 files changed, 173 insertions(+), 15 deletions(-) create mode 100644 database/migrations/2026_08_27_000001_add_ticket_number_sequence_to_ticketing_groups.php diff --git a/database/migrations/2026_08_27_000001_add_ticket_number_sequence_to_ticketing_groups.php b/database/migrations/2026_08_27_000001_add_ticket_number_sequence_to_ticketing_groups.php new file mode 100644 index 0000000..97370d6 --- /dev/null +++ b/database/migrations/2026_08_27_000001_add_ticket_number_sequence_to_ticketing_groups.php @@ -0,0 +1,82 @@ +first()`, so a duplicate makes it merge + * into an arbitrary one of the two. + * + * This column is that pool: a per-group high-water mark that only ever goes up, + * independent of whether the ticket it was issued to still exists. + * + * **The backfill is the load-bearing half of this migration.** Left at 0, the + * first ticket created after deploy would be issued `-0001` on a group that has + * been running for months — turning a bug that reused one number into one that + * reuses every number. So each group is seeded with the same MAX the old code + * would have read, scoped to its CURRENT prefix exactly as the old query was. + * The next reference issued after this migration is therefore identical to the + * one the old code would have issued; all that changes is that it can no longer + * go backwards afterwards. + * + * Expand-only: one column added with a default, nothing dropped or renamed, and + * existing rows keep their meaning. Note there is deliberately NO unique index + * on `tickets.number` — production already contains duplicates created by this + * bug (the report evidences at least `QAX-0001`–`QAX-0004`), so adding one here + * would fail the deploy. De-duplicating the existing rows is its own decision, + * with its own migration, and is not made silently here. + */ +return new class extends Migration +{ + public function up(): void + { + if (! Schema::hasColumn('ticketing_groups', 'last_ticket_number')) { + Schema::table('ticketing_groups', function (Blueprint $table) { + $table->unsignedInteger('last_ticket_number')->default(0)->after('prefix'); + }); + } + + // Seed the high-water mark from the numbers already issued. Same shape as + // the query this replaces: the group's own tickets, restricted to its + // current prefix, suffix cast to an integer. + foreach (DB::table('ticketing_groups')->select('id', 'prefix')->get() as $group) { + $prefix = $group->prefix.'-'; + + $max = (int) DB::table('tickets') + ->where('group_id', $group->id) + ->where('number', 'like', $prefix.'%') + ->max(DB::raw('CAST(SUBSTRING(number, '.(strlen($prefix) + 1).') AS UNSIGNED)')); + + // Never lower a counter that already holds a larger value — this + // migration must be safe to re-run and must never hand back a number. + DB::table('ticketing_groups') + ->where('id', $group->id) + ->where('last_ticket_number', '<', $max) + ->update(['last_ticket_number' => $max]); + } + } + + public function down(): void + { + // Deliberate NO-OP, matching the archived_at precedent above it: + // DashboardDoctor greps the whole migration file for drop/rename patterns, + // so a dropColumn even in down() raises "pending destructive migration" on + // every host where this is still pending. An integer column with a default + // costs nothing to leave behind — and dropping it would silently restore + // the reference-reuse bug. + } +}; diff --git a/src/Http/Controllers/TicketController.php b/src/Http/Controllers/TicketController.php index 89bd606..59fa4d6 100644 --- a/src/Http/Controllers/TicketController.php +++ b/src/Http/Controllers/TicketController.php @@ -515,12 +515,13 @@ class TicketController extends Controller if ($handedOver) { $agent = \App\Models\User::find($newAssignee); - $email = trim((string) ($agent?->email ?? '')); - if ($email !== '') { + // The agent, not their address — see TicketingNotifier::deliver(). + // A bare string loses to_user_id and to_name and skips the opt-out. + if ($agent) { \Dashboard\Ticketing\Support\TicketingNotifier::deliver( \Dashboard\Ticketing\Support\TicketingNotifier::ASSIGNED, - $email, + $agent, "[{$ticket->number}] Assigned to you: {$ticket->title}", '

'.e(Auth::user()?->name ?? 'Somebody').' assigned this ticket to you.

' .'

View the ticket

', diff --git a/src/Http/Controllers/TicketMessageController.php b/src/Http/Controllers/TicketMessageController.php index bd02632..b73b19d 100644 --- a/src/Http/Controllers/TicketMessageController.php +++ b/src/Http/Controllers/TicketMessageController.php @@ -73,18 +73,21 @@ class TicketMessageController extends Controller private function notifySubmitter(Ticket $ticket, TicketMessage $message): void { $submitter = \App\Models\User::find($ticket->submitter_id); - $email = trim((string) ($submitter?->email ?? '')); - if ($email === '') { - return; // a ticket raised on behalf of somebody with no address + if (! $submitter) { + return; // a ticket raised on behalf of somebody with no account } $url = route('ticketing.show', $ticket->id); $author = $message->user_id ? (\App\Models\User::find($message->user_id)?->name ?? 'An agent') : 'An agent'; + // The submitter, not their address. Handing Notify a string dropped + // to_user_id and to_name (so the Log printed the message type where the + // name goes) and skipped the preference check entirely, so the opt-out for + // this category did nothing. Notify resolves the address itself. \Dashboard\Ticketing\Support\TicketingNotifier::deliver( \Dashboard\Ticketing\Support\TicketingNotifier::REPLIED, - $email, + $submitter, "[{$ticket->number}] {$ticket->title}", '

'.e($author).' replied to your ticket:

' .'
'.nl2br(e($message->body)).'
' diff --git a/src/Models/TicketingGroup.php b/src/Models/TicketingGroup.php index f939556..2d1476e 100644 --- a/src/Models/TicketingGroup.php +++ b/src/Models/TicketingGroup.php @@ -5,6 +5,7 @@ namespace Dashboard\Ticketing\Models; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\HasMany; +use Illuminate\Support\Facades\DB; class TicketingGroup extends Model { @@ -65,15 +66,59 @@ class TicketingGroup extends Model return $this->hasMany(EmailConnection::class, 'group_id'); } + /** + * Issue the next ticket reference for this group. + * + * This used to be MAX(suffix) over the group's live tickets. Tickets are + * hard-deleted, so that made the reference a *reusable* handle: delete the + * newest ticket and its number went back in the pool, and the next ticket was + * issued the same one. Two tickets carrying `QAX-0004` is not a cosmetic + * problem — the reference is the `[QAX-0004]` token in outbound mail subjects + * (so a reply threads onto the wrong ticket), the key the shell Notification + * Log is audited by, and what `merge()` resolves a ticket by. + * + * The number now comes from `last_ticket_number`, a per-group high-water mark + * that is only ever raised. Deleting a ticket cannot lower it, because it does + * not consult the tickets table for the answer. + * + * Two details that are not decoration: + * + * - `lockForUpdate` inside a transaction. Read-then-write is a race: two people + * submitting at the same instant would otherwise both read N and both be + * issued N+1, reintroducing exactly the duplicate this method exists to + * prevent. The row lock serialises them. + * - The counter is raised to clear any live ticket that is already numbered + * past it before being incremented. Not every row gets its number from here — + * `TicketingDemoSeeder` writes them directly, and imports and manual repairs + * can too — and a counter that has fallen behind such a row would issue a + * number that is already in use. Taking the max of the two can only ever move + * the counter forwards, so this preserves the never-goes-backwards property + * rather than weakening it. + */ public function nextTicketNumber(): string { $prefix = $this->prefix . '-'; - $last = $this->tickets() - ->where('number', 'like', $prefix . '%') - ->orderByRaw('CAST(SUBSTRING(number, ?) AS UNSIGNED) DESC', [strlen($prefix) + 1]) - ->value('number'); - $next = $last ? ((int) substr($last, strlen($prefix))) + 1 : 1; + $next = DB::transaction(function () use ($prefix) { + $counter = (int) static::query() + ->whereKey($this->getKey()) + ->lockForUpdate() + ->value('last_ticket_number'); + + $liveMax = (int) $this->tickets() + ->where('number', 'like', $prefix . '%') + ->max(DB::raw('CAST(SUBSTRING(number, ' . (strlen($prefix) + 1) . ') AS UNSIGNED)')); + + $next = max($counter, $liveMax) + 1; + + // Query-builder update, so `last_ticket_number` need not be fillable — + // it is a sequence, never something a form should be able to set. + static::query()->whereKey($this->getKey())->update(['last_ticket_number' => $next]); + $this->setAttribute('last_ticket_number', $next); + + return $next; + }); + return $prefix . str_pad($next, 4, '0', STR_PAD_LEFT); } } diff --git a/src/Support/TicketingNotifier.php b/src/Support/TicketingNotifier.php index 03b8d48..edf36e8 100644 --- a/src/Support/TicketingNotifier.php +++ b/src/Support/TicketingNotifier.php @@ -2,6 +2,7 @@ namespace Dashboard\Ticketing\Support; +use App\Models\User; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Facades\Log; @@ -92,19 +93,45 @@ final class TicketingNotifier /** * Queue one email through the platform. * - * @param string $to recipient address, verbatim + * PASS THE PERSON, NOT THE ADDRESS, whenever we have one. + * + * This took a bare string, and `Notify::sendAll()` derives everything + * user-shaped from the recipient it is handed: `$user = $recipient instanceof + * User ? $recipient : null`. With a string there is no `$user`, so three + * things silently went missing at once, all from the same line: + * + * - `to_user_id` was null, so the send never appeared in that person's + * notification history; + * - `to_name` was null, so the shared Notification Log fell through to its + * "we don't know who this went to" rendering and printed the message TYPE + * where the recipient's name belongs — for people we plainly do know, since + * both our notifications go to a Central user by definition (a ticket's + * submitter, or the agent it was just assigned to); + * - the preference branch is guarded by `if ($user && ! $mandatory && …)`, so + * it was skipped entirely and **the opt-out did nothing**. Neither of our + * categories is mandatory, so My Notifications offered a switch that was + * never read. + * + * A bare string is still accepted, for a future send to an address with no + * account behind it. It is simply no longer what our own callers hand over. + * + * Note we no longer resolve the address by hand either: `Notify` resolves a + * User school → personal → login, which is the platform's answer to "where do + * we email this person" and is what every other app gets. + * + * @param User|string $to the recipient — a User wherever one is known * @return array{handled:bool,status:?string,error:?string,id:?int} */ public static function deliver( string $category, - string $to, + User|string $to, string $subject, string $bodyHtml, ?Model $context = null, ): array { $unhandled = ['handled' => false, 'status' => null, 'error' => null, 'id' => null]; - if (! self::available() || trim($to) === '') { + if (! self::available() || (is_string($to) && trim($to) === '')) { return $unhandled; }