Compare commits
6 Commits
v1.0.4
...
420ef78603
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
420ef78603 | ||
|
|
5315afc239 | ||
|
|
c0225d7691 | ||
| ef4d0c4f93 | |||
| 0e48949114 | |||
| a4a365da3b |
@@ -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.
|
||||
}
|
||||
};
|
||||
@@ -0,0 +1,66 @@
|
||||
<?php
|
||||
|
||||
use Illuminate\Database\Migrations\Migration;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
/**
|
||||
* MERGE AND SPLIT WROTE A `source` THE COLUMN DID NOT ACCEPT. [central-ticketing] #415
|
||||
*
|
||||
* Both actions write an audit note into `ticket_messages` with `source = 'system'`,
|
||||
* and the column was created as `enum('web','email')`. Under strict mode the insert
|
||||
* throws `Data truncated for column 'source'` INSIDE the transaction, so nothing
|
||||
* commits: no merge, no split, no partial state, and a generic 500 page. Every
|
||||
* guard around them worked — a missing reference, a self-merge and an absent
|
||||
* `source_number` were all handled correctly — so only the happy path was broken,
|
||||
* which is why it survived.
|
||||
*
|
||||
* ── THE WHOLE DOMAIN, NOT THE ONE IN THE TRACE ──────────────────────────────
|
||||
*
|
||||
* The trace named `'system'`. Widening to accept only that would be a guess about
|
||||
* how many more there are, so every value written to this column anywhere in the
|
||||
* platform was enumerated first:
|
||||
*
|
||||
* 'web' TicketMessageController::store(), the demo seeder
|
||||
* 'email' the demo seeder (no inbound-mail ingester exists yet)
|
||||
* 'system' TicketController::merge() and ::split(), the three notes
|
||||
*
|
||||
* `split()` also copies `$msg->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'"
|
||||
);
|
||||
}
|
||||
};
|
||||
@@ -0,0 +1,73 @@
|
||||
<?php
|
||||
|
||||
use Dashboard\Ticketing\Support\MergedTicketRepair;
|
||||
use Illuminate\Database\Migrations\Migration;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
|
||||
/**
|
||||
* OPTION A OF [central-ticketing] #938 — the repair, behind a setting that is OFF.
|
||||
*
|
||||
* The rows the merge bug minted (`merged_into_id` set, `status` back to `open`)
|
||||
* are data, not code, and #920 deliberately shipped no data change. This
|
||||
* migration is the one that would: it re-marks every such row `merged`, which
|
||||
* makes it read-only under #920's guard, and it leaves every message,
|
||||
* participant and field edit made after the merge exactly where it is.
|
||||
*
|
||||
* IT DOES NOT RUN UNLESS THE OPERATOR HAS ASKED. `MergedTicketRepair::SETTING`
|
||||
* — "auto-repair inconsistent merged tickets on deploy" — defaults off, and a
|
||||
* box that has never heard of it therefore deploys this migration and changes
|
||||
* nothing at all. That is what lets the code ship ahead of the decision: the
|
||||
* office gets the badge and the one-click fix now (option B), and turning this
|
||||
* on is the operator's separate, later, reversible act (option C).
|
||||
*
|
||||
* WHAT IT ALWAYS DOES, EITHER WAY, IS COUNT AND SAY SO. The dry-run count goes
|
||||
* to the log on every deploy whether the repair is armed or not, because the
|
||||
* number is the thing #938 was blocked on and a deploy is the one moment
|
||||
* something is definitely looking at prod's database.
|
||||
*/
|
||||
return new class extends Migration
|
||||
{
|
||||
public function up(): void
|
||||
{
|
||||
// The dry run, first and unconditionally.
|
||||
$count = MergedTicketRepair::count();
|
||||
|
||||
Log::info(sprintf(
|
||||
'[ticketing] #938 dry run: %d ticket(s) have merged_into_id set with a status other than "merged".',
|
||||
$count,
|
||||
));
|
||||
|
||||
if ($count === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (! MergedTicketRepair::enabledOnDeploy()) {
|
||||
Log::info(sprintf(
|
||||
'[ticketing] #938: leaving those %d row(s) editable — "%s" is off. '
|
||||
.'Turn it on in Ticketing → Settings → Automation, or fix them one at a '
|
||||
.'time with the "Mark merged" button on each ticket.',
|
||||
$count,
|
||||
MergedTicketRepair::SETTING,
|
||||
));
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
MergedTicketRepair::repairAll('migration 2026_09_04_000001');
|
||||
}
|
||||
|
||||
/**
|
||||
* There is no down migration, and that is a decision rather than an omission.
|
||||
*
|
||||
* Reverting would mean putting each row's status back to what it was before
|
||||
* the bad save, and nothing anywhere records what that was — the bug
|
||||
* overwrote it. Setting them all back to `open` would not be a rollback; it
|
||||
* would be re-creating the corrupt shape on rows that may have been merged
|
||||
* legitimately years ago. A row repaired in error is corrected the same way
|
||||
* any other status is: an agent edits the ticket.
|
||||
*/
|
||||
public function down(): void
|
||||
{
|
||||
// Intentionally empty — see the note above.
|
||||
}
|
||||
};
|
||||
67
src/Console/Commands/RepairMergedTickets.php
Normal file
67
src/Console/Commands/RepairMergedTickets.php
Normal file
@@ -0,0 +1,67 @@
|
||||
<?php
|
||||
|
||||
namespace Dashboard\Ticketing\Console\Commands;
|
||||
|
||||
use Dashboard\Ticketing\Support\MergedTicketRepair;
|
||||
use Illuminate\Console\Command;
|
||||
|
||||
/**
|
||||
* THE DRY-RUN COUNT, AND THE REPAIR — [central-ticketing] #938.
|
||||
*
|
||||
* The first question #938 asks is a number: how many rows on prod carry
|
||||
* `merged_into_id` with a status other than `merged`. That number decides
|
||||
* whether the office corrects them one at a time from the badge or whether a
|
||||
* deploy re-marks them all, and there was no way to ask for it that did not
|
||||
* involve typing a raw UPDATE-shaped statement into tinker on the production
|
||||
* box. So `--dry-run` is the default and is the point of this command:
|
||||
*
|
||||
* php artisan ticketing:repair-merged --dry-run # count + list, writes NOTHING
|
||||
* php artisan ticketing:repair-merged --force # re-mark them all
|
||||
*
|
||||
* `--force` is required to write, rather than `--dry-run` being required not
|
||||
* to. A repair command whose bare form repairs is one fat-finger away from
|
||||
* being the incident it was written to clean up after.
|
||||
*/
|
||||
class RepairMergedTickets extends Command
|
||||
{
|
||||
protected $signature = 'ticketing:repair-merged
|
||||
{--dry-run : Count and list the inconsistent rows; write nothing (the default).}
|
||||
{--force : Actually re-mark every inconsistent row as merged.}';
|
||||
|
||||
protected $description = 'Report — or, with --force, repair — tickets whose merged_into_id is set while their status is not "merged" (#938).';
|
||||
|
||||
public function handle(): int
|
||||
{
|
||||
$rows = MergedTicketRepair::rows();
|
||||
|
||||
$this->line(sprintf(
|
||||
'SELECT COUNT(*) FROM tickets WHERE merged_into_id IS NOT NULL AND status <> \'merged\' => %d',
|
||||
$rows->count(),
|
||||
));
|
||||
|
||||
if ($rows->isEmpty()) {
|
||||
$this->info('Nothing is inconsistent. No repair to make.');
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
$this->table(
|
||||
['id', 'number', 'status', 'merged_into_id', 'updated_at'],
|
||||
$rows->map(fn ($t) => [
|
||||
$t->id, $t->number, $t->status, $t->merged_into_id, (string) $t->updated_at,
|
||||
])->all(),
|
||||
);
|
||||
|
||||
if (! $this->option('force')) {
|
||||
$this->comment('Dry run — nothing was written. Re-run with --force to re-mark these as merged.');
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
$changed = MergedTicketRepair::repairAll('artisan ticketing:repair-merged');
|
||||
|
||||
$this->info("Re-marked {$changed} ticket(s) as merged. Their messages and participants are untouched.");
|
||||
|
||||
return 0;
|
||||
}
|
||||
}
|
||||
@@ -29,6 +29,11 @@ class TicketAttachmentController extends Controller
|
||||
abort(403);
|
||||
}
|
||||
|
||||
// Same read-only rule as a reply — see TicketMessageController::store(). #920
|
||||
if ($ticket->isMerged()) {
|
||||
return back()->withErrors(['ticket' => Ticket::READ_ONLY_REASON]);
|
||||
}
|
||||
|
||||
$request->validate([
|
||||
'file' => 'required|file|max:' . (self::MAX_SIZE_MB * 1024),
|
||||
// Bug #5: message_id must belong to THIS ticket, not any arbitrary message
|
||||
|
||||
@@ -13,6 +13,7 @@ use Dashboard\Ticketing\Models\TicketMessage;
|
||||
use Dashboard\Ticketing\Models\TicketParticipant;
|
||||
use Dashboard\Ticketing\Models\TicketStatus;
|
||||
use Dashboard\Ticketing\Models\TicketView;
|
||||
use Dashboard\Ticketing\Support\MergedTicketRepair;
|
||||
use Illuminate\Http\Request;
|
||||
use Illuminate\Routing\Controller;
|
||||
use Illuminate\Support\Facades\Auth;
|
||||
@@ -47,6 +48,40 @@ class TicketController extends Controller
|
||||
->exists();
|
||||
}
|
||||
|
||||
/**
|
||||
* REFUSE A WRITE TO A MERGED TICKET — [central-ticketing] #920.
|
||||
*
|
||||
* The Show page has always announced "This ticket has been merged and is
|
||||
* read-only." and hidden the reply composer and the Merge button. Nothing
|
||||
* enforced it: the Edit form was still linked, still rendered (with its Status
|
||||
* select blank, because `merged` is not one of its options), and `update()`
|
||||
* happily wrote `status = open` while leaving `merged_into_id` pointing at the
|
||||
* target. That produced a ticket that was live and assignable in the All Open
|
||||
* queue and simultaneously merged into another one, with none of its own
|
||||
* messages — an empty duplicate of the ticket it had been folded into.
|
||||
*
|
||||
* So the check lives HERE, on the server, once, and every write path that the
|
||||
* page presents as unavailable calls it. Returning a refusal rather than
|
||||
* `abort(403)` on purpose: the realistic way to arrive here is a page that was
|
||||
* already open when somebody else merged the ticket, and that operator is
|
||||
* better served by the reason on the ticket than by an error screen.
|
||||
* `FormErrors` renders any key it is handed, so the sentence shows up.
|
||||
*
|
||||
* @return \Illuminate\Http\RedirectResponse|null null when the write may proceed
|
||||
*/
|
||||
private function refuseIfMerged(Ticket $ticket, bool $stayPut = false)
|
||||
{
|
||||
if (! $ticket->isMerged()) {
|
||||
return null;
|
||||
}
|
||||
|
||||
$errors = ['ticket' => Ticket::READ_ONLY_REASON];
|
||||
|
||||
return $stayPut
|
||||
? back()->withErrors($errors)
|
||||
: redirect()->route('ticketing.show', $ticket)->withErrors($errors);
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter messages for submitters — strip internal notes.
|
||||
*/
|
||||
@@ -113,6 +148,11 @@ class TicketController extends Controller
|
||||
'unassigned' => (clone $baseQuery)->whereNull('assigned_to')->whereNotIn('status', $closedSlugs)->count(),
|
||||
'pending' => (clone $baseQuery)->where('status', 'pending')->count(),
|
||||
'resolved' => (clone $baseQuery)->whereIn('status', $closedSlugs)->count(),
|
||||
// #938: the pointer, not the status — this view exists to show the
|
||||
// rows where the two disagree, so filtering by `status = merged`
|
||||
// would hide exactly the ones worth looking at.
|
||||
'merged' => (clone $baseQuery)->whereNotNull('merged_into_id')->count(),
|
||||
'merged_inconsistent' => (clone $baseQuery)->inconsistentlyMerged()->count(),
|
||||
]
|
||||
: [
|
||||
'all' => (clone $baseQuery)->count(),
|
||||
@@ -120,6 +160,8 @@ class TicketController extends Controller
|
||||
'unassigned' => 0,
|
||||
'pending' => (clone $baseQuery)->where('status', 'pending')->count(),
|
||||
'resolved' => (clone $baseQuery)->whereIn('status', $closedSlugs)->count(),
|
||||
'merged' => 0,
|
||||
'merged_inconsistent' => 0,
|
||||
];
|
||||
|
||||
$query = Ticket::with(['group', 'priority', 'project']);
|
||||
@@ -147,6 +189,12 @@ class TicketController extends Controller
|
||||
$query->where('status', 'pending');
|
||||
} elseif ($request->filter === 'resolved') {
|
||||
$query->whereIn('status', $closedSlugs);
|
||||
} elseif ($request->filter === 'merged') {
|
||||
// Every ticket that has been folded into another one, whatever its
|
||||
// status says — which is what makes this the place the office
|
||||
// finds the rows #938 is about. `merged` is a closed slug, so the
|
||||
// whereNotIn below would drop the consistent half of the list.
|
||||
$query->whereNotNull('merged_into_id');
|
||||
} elseif (!$request->filled('status')) {
|
||||
$query->whereNotIn('status', $closedSlugs);
|
||||
}
|
||||
@@ -417,12 +465,17 @@ class TicketController extends Controller
|
||||
]);
|
||||
}
|
||||
|
||||
public function edit(Ticket $ticket): Response
|
||||
/** @return Response|\Illuminate\Http\RedirectResponse — merged tickets are sent back to Show. */
|
||||
public function edit(Ticket $ticket)
|
||||
{
|
||||
if (!$this->isAgent($ticket->group_id)) {
|
||||
abort(403);
|
||||
}
|
||||
|
||||
if ($refusal = $this->refuseIfMerged($ticket)) {
|
||||
return $refusal;
|
||||
}
|
||||
|
||||
$ticket->load(['group', 'priority', 'project']);
|
||||
$priorities = PriorityLevel::orderBy('sort_order')->get();
|
||||
$agentIds = TicketingAgentAccess::where('group_id', $ticket->group_id)->pluck('user_id');
|
||||
@@ -434,6 +487,16 @@ class TicketController extends Controller
|
||||
'priorities' => $priorities,
|
||||
'agents' => $agents,
|
||||
'projects' => $projects,
|
||||
/*
|
||||
* The form's Status select used to hard-code five slugs, so any ticket
|
||||
* whose status was not one of them — `merged`, or any status added on
|
||||
* the Statuses settings tab — rendered with nothing selected and the
|
||||
* first save silently rewrote the status to whatever the operator
|
||||
* happened to leave showing. Same defect as #920, one layer up: drive
|
||||
* the options from the same table `update()` validates against.
|
||||
*/
|
||||
'ticketStatuses' => TicketStatus::where('slug', '!=', Ticket::STATUS_MERGED)
|
||||
->orderBy('sort_order')->get(),
|
||||
]);
|
||||
}
|
||||
|
||||
@@ -443,6 +506,10 @@ class TicketController extends Controller
|
||||
abort(403);
|
||||
}
|
||||
|
||||
if ($refusal = $this->refuseIfMerged($ticket)) {
|
||||
return $refusal;
|
||||
}
|
||||
|
||||
// Determine effective group (may be changing via a transfer)
|
||||
$targetGroupId = $request->filled('group_id')
|
||||
? (int) $request->input('group_id')
|
||||
@@ -454,7 +521,7 @@ class TicketController extends Controller
|
||||
->toArray();
|
||||
|
||||
// Build valid status list from DB (exclude 'merged' — set only by the merge operation)
|
||||
$validStatusSlugs = TicketStatus::where('slug', '!=', 'merged')->pluck('slug')->implode(',');
|
||||
$validStatusSlugs = TicketStatus::where('slug', '!=', Ticket::STATUS_MERGED)->pluck('slug')->implode(',');
|
||||
|
||||
$rules = [
|
||||
'title' => 'sometimes|required|string|max:255',
|
||||
@@ -515,12 +582,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>',
|
||||
@@ -562,7 +630,7 @@ class TicketController extends Controller
|
||||
{
|
||||
$validated = $request->validate([
|
||||
'view_mode' => 'sometimes|in:list,kanban',
|
||||
'filter' => 'sometimes|in:all,mine,unassigned,pending,resolved',
|
||||
'filter' => 'sometimes|in:all,mine,unassigned,pending,resolved,merged',
|
||||
]);
|
||||
|
||||
if (empty($validated)) {
|
||||
@@ -608,7 +676,7 @@ class TicketController extends Controller
|
||||
if ($source->id === $ticket->id) {
|
||||
return back()->withErrors(['source_number' => 'Cannot merge a ticket into itself.']);
|
||||
}
|
||||
if ($source->status === 'merged') {
|
||||
if ($source->isMerged()) {
|
||||
return back()->withErrors(['source_number' => 'That ticket has already been merged.']);
|
||||
}
|
||||
if (! $this->isAgent($source->group_id)) {
|
||||
@@ -625,13 +693,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) {
|
||||
@@ -643,7 +705,7 @@ class TicketController extends Controller
|
||||
|
||||
// Mark source as merged
|
||||
$source->update([
|
||||
'status' => 'merged',
|
||||
'status' => Ticket::STATUS_MERGED,
|
||||
'merged_into_id' => $ticket->id,
|
||||
]);
|
||||
});
|
||||
@@ -652,6 +714,42 @@ class TicketController extends Controller
|
||||
->with('success', "Ticket {$source->number} merged in.");
|
||||
}
|
||||
|
||||
/**
|
||||
* MARK ONE INCONSISTENT ROW AS MERGED — option B of [central-ticketing] #938.
|
||||
*
|
||||
* The rows the merge bug minted carry `merged_into_id` while their status
|
||||
* says `open`. #920 left them editable ON PURPOSE, so that they could be
|
||||
* corrected in the product; this is the correction, and until it existed the
|
||||
* only way to make one was to type the right status into the Edit form and
|
||||
* know that `merged` was not in the list.
|
||||
*
|
||||
* Note what it is NOT: it is not `update()` with a status of `merged`, which
|
||||
* `refuseIfMerged()` would have to be weakened to allow, and it does not
|
||||
* touch messages, participants or any field somebody edited after the merge.
|
||||
* It writes one column, on a row that is provably in the broken shape, and
|
||||
* it is a no-op on anything else — so a double-click, a stale page and a row
|
||||
* a colleague fixed thirty seconds ago all land somewhere harmless.
|
||||
*/
|
||||
public function markMerged(Ticket $ticket)
|
||||
{
|
||||
if (! $this->isAgent($ticket->group_id)) {
|
||||
abort(403);
|
||||
}
|
||||
|
||||
if (! $ticket->isInconsistentlyMerged()) {
|
||||
// Deliberately not an error: the button is only rendered on a row in
|
||||
// the broken shape, so arriving here means somebody else has already
|
||||
// fixed it, and being told so is the right answer.
|
||||
return redirect()->route('ticketing.show', $ticket)
|
||||
->with('success', 'That ticket is already consistent — nothing to fix.');
|
||||
}
|
||||
|
||||
MergedTicketRepair::repair($ticket, 'user '.Auth::id());
|
||||
|
||||
return redirect()->route('ticketing.show', $ticket)
|
||||
->with('success', "Ticket {$ticket->number} is marked merged and is read-only again.");
|
||||
}
|
||||
|
||||
/**
|
||||
* Split this ticket — create a copy with a new number, same metadata, duplicated messages.
|
||||
*/
|
||||
@@ -661,6 +759,10 @@ class TicketController extends Controller
|
||||
abort(403);
|
||||
}
|
||||
|
||||
if ($refusal = $this->refuseIfMerged($ticket)) {
|
||||
return $refusal;
|
||||
}
|
||||
|
||||
$newTicket = DB::transaction(function () use ($ticket) {
|
||||
$group = TicketingGroup::findOrFail($ticket->group_id);
|
||||
$number = $group->nextTicketNumber();
|
||||
@@ -671,16 +773,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,
|
||||
@@ -692,20 +809,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;
|
||||
});
|
||||
@@ -714,6 +819,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.
|
||||
*/
|
||||
@@ -723,6 +861,10 @@ class TicketController extends Controller
|
||||
abort(403);
|
||||
}
|
||||
|
||||
if ($refusal = $this->refuseIfMerged($ticket, stayPut: true)) {
|
||||
return $refusal;
|
||||
}
|
||||
|
||||
$validated = $request->validate([
|
||||
'user_id' => 'required|integer|exists:users,id',
|
||||
]);
|
||||
@@ -744,6 +886,10 @@ class TicketController extends Controller
|
||||
abort(403);
|
||||
}
|
||||
|
||||
if ($refusal = $this->refuseIfMerged($ticket, stayPut: true)) {
|
||||
return $refusal;
|
||||
}
|
||||
|
||||
TicketParticipant::where('ticket_id', $ticket->id)
|
||||
->where('user_id', $userId)
|
||||
->delete();
|
||||
|
||||
@@ -29,6 +29,19 @@ class TicketMessageController extends Controller
|
||||
abort(403);
|
||||
}
|
||||
|
||||
/*
|
||||
* A merged ticket takes no new messages — [central-ticketing] #920.
|
||||
*
|
||||
* Show.vue replaces the composer with "This ticket has been merged and is
|
||||
* read-only.", and that was the whole of the guard: a POST straight to this
|
||||
* route (or from a page that was already open when the merge happened) still
|
||||
* wrote a message onto a ticket whose thread has moved to the merge target,
|
||||
* where nobody working the ticket would ever see it.
|
||||
*/
|
||||
if ($ticket->isMerged()) {
|
||||
return back()->withErrors(['ticket' => Ticket::READ_ONLY_REASON]);
|
||||
}
|
||||
|
||||
$validated = $request->validate([
|
||||
'body' => 'required|string',
|
||||
'is_internal' => 'boolean',
|
||||
@@ -73,18 +86,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>'
|
||||
|
||||
@@ -9,6 +9,7 @@ use Dashboard\Ticketing\Models\TicketingAgentAccess;
|
||||
use Dashboard\Ticketing\Models\TicketingGroup;
|
||||
use Dashboard\Ticketing\Models\TicketingProject;
|
||||
use Dashboard\Ticketing\Models\TicketStatus;
|
||||
use Dashboard\Ticketing\Support\MergedTicketRepair;
|
||||
use Illuminate\Http\Request;
|
||||
use Illuminate\Routing\Controller;
|
||||
use Illuminate\Support\Facades\Auth;
|
||||
@@ -121,6 +122,11 @@ class TicketingSettingsController extends Controller
|
||||
'isBootstrap' => $isBootstrap,
|
||||
'isSiteAdmin' => $this->isSiteAdmin(),
|
||||
'autoCloseDays' => (int) Setting::get('ticketing.auto_close_days', 0),
|
||||
// #938 — the deploy-time repair, and how many rows it would touch. The
|
||||
// count is what turns an abstract toggle into a decision somebody can
|
||||
// actually make, so it is shown beside it whether the toggle is on or off.
|
||||
'autoRepairMerged' => MergedTicketRepair::enabledOnDeploy(),
|
||||
'inconsistentMergedCount' => MergedTicketRepair::count(),
|
||||
]);
|
||||
}
|
||||
|
||||
@@ -575,10 +581,20 @@ class TicketingSettingsController extends Controller
|
||||
|
||||
$validated = $request->validate([
|
||||
'auto_close_days' => 'required|integer|min:0|max:365',
|
||||
'auto_repair_merged_on_deploy' => 'sometimes|boolean',
|
||||
]);
|
||||
|
||||
Setting::set('ticketing.auto_close_days', $validated['auto_close_days']);
|
||||
|
||||
// #938 option A is a behaviour rule, so it is a setting, and it defaults
|
||||
// off: a deploy must never rewrite rows nobody asked it to rewrite.
|
||||
if (array_key_exists('auto_repair_merged_on_deploy', $validated)) {
|
||||
Setting::set(
|
||||
MergedTicketRepair::SETTING,
|
||||
$validated['auto_repair_merged_on_deploy'] ? '1' : '0',
|
||||
);
|
||||
}
|
||||
|
||||
return back()->with('success', 'Automation settings saved.');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,19 +2,57 @@
|
||||
|
||||
namespace Dashboard\Ticketing\Models;
|
||||
|
||||
use Illuminate\Database\Eloquent\Builder;
|
||||
use Illuminate\Database\Eloquent\Model;
|
||||
use Illuminate\Database\Eloquent\Relations\BelongsTo;
|
||||
use Illuminate\Database\Eloquent\Relations\HasMany;
|
||||
|
||||
class Ticket extends Model
|
||||
{
|
||||
/** Set only by TicketController::merge(); never offered by any form. */
|
||||
public const STATUS_MERGED = 'merged';
|
||||
|
||||
/**
|
||||
* What every surface says when a write to a merged ticket is refused. The Show
|
||||
* page has displayed this sentence since merge shipped; #920 was that it was
|
||||
* only ever a sentence, with no server-side guard behind it.
|
||||
*/
|
||||
public const READ_ONLY_REASON = 'This ticket has been merged and is read-only.';
|
||||
|
||||
protected $fillable = [
|
||||
'number', 'group_id', 'submitter_id', 'assigned_to', 'project_id',
|
||||
'title', 'description', 'status', 'priority_id', 'due_date', 'merged_into_id',
|
||||
];
|
||||
|
||||
// `date:Y-m-d`, not bare `date`. The column is a DATE and every UI that shows
|
||||
// it is an <input type="date">, which accepts exactly `Y-m-d` and silently
|
||||
// renders anything else as an EMPTY field. A bare `date` cast serializes to
|
||||
// `2026-09-15T00:00:00.000000Z`, so a due date that had been stored perfectly
|
||||
// well came back as a blank input on the edit form, the show page and the
|
||||
// index pane — indistinguishable from "it was never saved", which is how this
|
||||
// was reported (a QA sweep read `input.value === ''` as silent data loss).
|
||||
//
|
||||
// The write path was never at fault: the column, the `$fillable` entry and
|
||||
// both `validate()` rules were correct throughout, and re-submitting the ISO
|
||||
// string (which Show.vue's meta form does, because it seeds itself from this
|
||||
// value) still stores the right day — measured in UTC and in
|
||||
// America/New_York. So nothing was ever lost in the database; the value
|
||||
// simply had no way of reaching the screen.
|
||||
/**
|
||||
* `merged_inconsistent` rides on every serialized ticket — #938.
|
||||
*
|
||||
* The badge and its "Mark merged" button are rendered per ROW, on the index
|
||||
* list, in the detail pane and on the Show header, so the page needs the
|
||||
* answer for each ticket it draws. Handing it the model's own predicate
|
||||
* rather than letting three Vue files each write
|
||||
* `t.merged_into_id && t.status !== 'merged'` is what keeps the client's idea
|
||||
* of "inconsistent" from drifting away from the scope the repair uses.
|
||||
* It costs no query — both columns are already loaded.
|
||||
*/
|
||||
protected $appends = ['merged_inconsistent'];
|
||||
|
||||
protected $casts = [
|
||||
'due_date' => 'date',
|
||||
'due_date' => 'date:Y-m-d',
|
||||
];
|
||||
|
||||
public function group(): BelongsTo
|
||||
@@ -56,4 +94,54 @@ class Ticket extends Model
|
||||
{
|
||||
return $this->belongsTo(Ticket::class, 'merged_into_id');
|
||||
}
|
||||
|
||||
/**
|
||||
* A merged ticket is read-only — its messages and attachments now live on the
|
||||
* ticket it was merged into, so it is a stub kept for its reference number and
|
||||
* the audit trail, not a ticket anyone can still work.
|
||||
*
|
||||
* `status === 'merged'` and NOT `merged_into_id !== null`, deliberately. #920
|
||||
* minted rows with `status = 'open', merged_into_id = 56` — the very rows this
|
||||
* guard exists to prevent — and reading the id would make each of those
|
||||
* permanently uneditable, i.e. a row the product will not let anyone correct.
|
||||
* The status is the state; the id is the pointer that goes with it.
|
||||
*/
|
||||
public function isMerged(): bool
|
||||
{
|
||||
return $this->status === self::STATUS_MERGED;
|
||||
}
|
||||
|
||||
/**
|
||||
* THE SHAPE THE MERGE BUG MINTED — [central-ticketing] #938.
|
||||
*
|
||||
* `merged_into_id` points at a target while `status` is something other than
|
||||
* `merged`: the row was merged, and then a save through the unguarded Edit
|
||||
* form (#920) put the status back. #920 stopped new ones being made and
|
||||
* deliberately left the existing ones EDITABLE — `isMerged()` reads the
|
||||
* status, not the pointer — so that the office can correct one rather than
|
||||
* needing SQL. This is the predicate that finds them.
|
||||
*
|
||||
* It is one definition and three callers read it: the badge that offers the
|
||||
* fix, the `mark-merged` verb that applies it one row at a time, and the
|
||||
* repair migration that applies it to all of them at once. Two spellings of
|
||||
* "inconsistent" that drifted apart would mean a badge on a row the repair
|
||||
* skips.
|
||||
*/
|
||||
public function scopeInconsistentlyMerged(Builder $query): Builder
|
||||
{
|
||||
return $query->whereNotNull('merged_into_id')
|
||||
->where('status', '!=', self::STATUS_MERGED);
|
||||
}
|
||||
|
||||
/** The row-level twin of {@see scopeInconsistentlyMerged()}. */
|
||||
public function isInconsistentlyMerged(): bool
|
||||
{
|
||||
return $this->merged_into_id !== null && ! $this->isMerged();
|
||||
}
|
||||
|
||||
/** @see $appends — the same answer, for the page. */
|
||||
public function getMergedInconsistentAttribute(): bool
|
||||
{
|
||||
return $this->isInconsistentlyMerged();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
133
src/Support/MergedTicketRepair.php
Normal file
133
src/Support/MergedTicketRepair.php
Normal file
@@ -0,0 +1,133 @@
|
||||
<?php
|
||||
|
||||
namespace Dashboard\Ticketing\Support;
|
||||
|
||||
use App\Models\Setting;
|
||||
use Dashboard\Ticketing\Models\Ticket;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
|
||||
/**
|
||||
* REPAIRING THE ROWS THE MERGE BUG MINTED — [central-ticketing] #938.
|
||||
*
|
||||
* #920 stopped the bug making new ones: a ticket whose `status` is `merged` is
|
||||
* read-only server-side, so the Edit form can no longer write `status = open`
|
||||
* over a row that has already been folded into another ticket. It shipped NO
|
||||
* data change, on purpose, and its guard reads the status rather than the
|
||||
* pointer precisely so the rows already carrying the corrupt shape
|
||||
* (`status = open, merged_into_id = 56`) stay editable and can be corrected
|
||||
* through the UI instead of by hand in SQL.
|
||||
*
|
||||
* This class is the correction, and it is deliberately ONE implementation with
|
||||
* three callers, because "which rows are broken" and "what fixing one means"
|
||||
* are the two things that must not drift:
|
||||
*
|
||||
* - {@see \Dashboard\Ticketing\Http\Controllers\TicketController::markMerged()}
|
||||
* — the office fixes one row, from the badge on the ticket header.
|
||||
* - {@see \Dashboard\Ticketing\Console\Commands\RepairMergedTickets}
|
||||
* — `--dry-run` is the COUNT, and it is the whole reason the class is not
|
||||
* just a line inside the migration: the operator has to be able to see how
|
||||
* many rows there are, and which, before anything writes.
|
||||
* - the `2026_09_04_000001_repair_inconsistent_merged_tickets` migration
|
||||
* — every row at once, on deploy, and ONLY when the operator has turned
|
||||
* {@see SETTING} on.
|
||||
*
|
||||
* ── WHAT REPAIRING DOES *NOT* DO ────────────────────────────────────────────
|
||||
*
|
||||
* It writes `status = 'merged'` and nothing else. Messages, participants,
|
||||
* assignee, priority and title are left exactly as they are, including edits
|
||||
* made to the row AFTER the merge — those are somebody's work, they are the
|
||||
* only copy, and there is no record of what the fields held before the bad
|
||||
* save to revert them to. The ticket becoming read-only is the whole change.
|
||||
*/
|
||||
class MergedTicketRepair
|
||||
{
|
||||
/**
|
||||
* "Auto-repair inconsistent merged tickets on deploy", and it DEFAULTS OFF.
|
||||
*
|
||||
* A deploy that silently rewrites rows nobody asked it to rewrite is the one
|
||||
* outcome #938 rules out by name, so the migration reads this and no-ops
|
||||
* when it is unset. Off is therefore the whole behaviour of a box that has
|
||||
* never heard of this setting, which is what makes shipping the migration
|
||||
* safe before the operator has decided anything.
|
||||
*/
|
||||
public const SETTING = 'ticketing.auto_repair_merged_on_deploy';
|
||||
|
||||
/** How many rows are inconsistent right now. The dry run, and it writes nothing. */
|
||||
public static function count(): int
|
||||
{
|
||||
return Ticket::query()->inconsistentlyMerged()->count();
|
||||
}
|
||||
|
||||
/**
|
||||
* The rows themselves, for a human to look at before deciding.
|
||||
*
|
||||
* @return \Illuminate\Database\Eloquent\Collection<int,Ticket>
|
||||
*/
|
||||
public static function rows()
|
||||
{
|
||||
return Ticket::query()->inconsistentlyMerged()->orderBy('id')->get();
|
||||
}
|
||||
|
||||
/** Has the operator asked for the deploy-time repair? */
|
||||
public static function enabledOnDeploy(): bool
|
||||
{
|
||||
return filter_var(Setting::get(self::SETTING, '0'), FILTER_VALIDATE_BOOLEAN);
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-mark every inconsistent row as merged. Returns how many rows changed.
|
||||
*
|
||||
* IDEMPOTENT, and it is the scope that makes it so rather than a flag: the
|
||||
* second run matches nothing, because the first run's rows no longer satisfy
|
||||
* `status <> 'merged'`. So a migration that is re-run, a command run twice
|
||||
* and a deploy that repeats all answer 0 and write nothing.
|
||||
*/
|
||||
public static function repairAll(string $by): int
|
||||
{
|
||||
$numbers = Ticket::query()->inconsistentlyMerged()->pluck('number')->all();
|
||||
|
||||
if ($numbers === []) {
|
||||
Log::info("[ticketing] #938 merged-ticket repair ({$by}): nothing to repair.");
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
$changed = Ticket::query()->inconsistentlyMerged()->update([
|
||||
'status' => Ticket::STATUS_MERGED,
|
||||
]);
|
||||
|
||||
// The numbers, not just the count. These rows are being made read-only,
|
||||
// and "which ones" is the question the office will ask afterwards.
|
||||
Log::info(sprintf(
|
||||
'[ticketing] #938 merged-ticket repair (%s): re-marked %d ticket(s) as merged — %s',
|
||||
$by,
|
||||
$changed,
|
||||
implode(', ', $numbers),
|
||||
));
|
||||
|
||||
return $changed;
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-mark ONE row. Returns false when it was not inconsistent to begin with,
|
||||
* so a double-clicked button and a stale page are both no-ops rather than a
|
||||
* second write.
|
||||
*/
|
||||
public static function repair(Ticket $ticket, string $by): bool
|
||||
{
|
||||
if (! $ticket->isInconsistentlyMerged()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
$ticket->forceFill(['status' => Ticket::STATUS_MERGED])->save();
|
||||
|
||||
Log::info(sprintf(
|
||||
'[ticketing] #938 merged-ticket repair (%s): re-marked %s as merged (merged into #%d).',
|
||||
$by,
|
||||
$ticket->number,
|
||||
$ticket->merged_into_id,
|
||||
));
|
||||
|
||||
return true;
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -26,6 +26,7 @@ class TicketingServiceProvider extends ServiceProvider
|
||||
if ($this->app->runningInConsole()) {
|
||||
$this->commands([
|
||||
\Dashboard\Ticketing\Console\Commands\AutoCloseTickets::class,
|
||||
\Dashboard\Ticketing\Console\Commands\RepairMergedTickets::class,
|
||||
]);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -76,6 +76,8 @@ Route::middleware(['web', 'auth', 'app.access:ticketing'])
|
||||
Route::post('/{ticket}/view', [TicketController::class, 'recordView']) ->name('tickets.view');
|
||||
Route::post('/{ticket}/merge', [TicketController::class, 'merge']) ->name('merge');
|
||||
Route::post('/{ticket}/split', [TicketController::class, 'split']) ->name('split');
|
||||
// #938 — the one-click fix for a row the merge bug left inconsistent.
|
||||
Route::post('/{ticket}/mark-merged', [TicketController::class, 'markMerged']) ->name('mark-merged');
|
||||
Route::post('/{ticket}/participants', [TicketController::class, 'addParticipant']) ->name('participants.store');
|
||||
Route::delete('/{ticket}/participants/{userId}', [TicketController::class, 'removeParticipant'])->name('participants.destroy');
|
||||
Route::get('/{ticket}/users/search', [TicketController::class, 'searchUsers']) ->name('users.search');
|
||||
|
||||
Reference in New Issue
Block a user