jwed/central-ticketing
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

A ticket reference is never re-issued, and notifications name their recipient (#224, #228)

Browse Source 
#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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ec3REU4LQmhuchRRn6dZHk
This commit is contained in:
Joel Wedemire
2026-08-27 19:37:06 -04:00
parent a4a365da3b
commit 0e48949114
5 changed files with 173 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

82
database/migrations/2026_08_27_000001_add_ticket_number_sequence_to_ticketing_groups.php Normal file
View File

@@ -0,0 +1,82 @@
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;
/**
* A ticket reference must never be issued twice.
*
* `TicketingGroup::nextTicketNumber()` derived the next reference from the MAX
* suffix over the group's **live** tickets. Tickets are hard-deleted (no
* SoftDeletes on the model), so deleting the newest ticket in a group put the
* number back in the pool and the next ticket was issued the same reference —
* two different tickets both carrying `QAX-0004`.
*
* That matters well outside Ticketing, because the reference is the correlation
* key that leaves the app: it is the `[QAX-0004]` token in outbound mail subject
* lines, so a reply can thread onto the wrong ticket; the shell Notification Log
* is an audit trail keyed on that subject; and `TicketController::merge()` looks
* a ticket up by `number` and takes `->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.
}
};

7
src/Http/Controllers/TicketController.php
View File

@@ -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}",
'<p>'.e(Auth::user()?->name ?? 'Somebody').' assigned this ticket to you.</p>'
.'<p><a href="'.e(route('ticketing.show', $ticket->id)).'">View the ticket</a></p>',

11
src/Http/Controllers/TicketMessageController.php
View File

@@ -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}",
'<p>'.e($author).' replied to your ticket:</p>'
.'<blockquote>'.nl2br(e($message->body)).'</blockquote>'

55
src/Models/TicketingGroup.php
View File

@@ -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);
}
}

33
src/Support/TicketingNotifier.php
View File

@@ -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;
}