Compare commits
3 Commits
ef4d0c4f93
...
420ef78603
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
420ef78603 | ||
|
|
5315afc239 | ||
|
|
c0225d7691 |
@@ -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',
|
||||
@@ -563,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)) {
|
||||
@@ -609,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)) {
|
||||
@@ -638,7 +705,7 @@ class TicketController extends Controller
|
||||
|
||||
// Mark source as merged
|
||||
$source->update([
|
||||
'status' => 'merged',
|
||||
'status' => Ticket::STATUS_MERGED,
|
||||
'merged_into_id' => $ticket->id,
|
||||
]);
|
||||
});
|
||||
@@ -647,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.
|
||||
*/
|
||||
@@ -656,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();
|
||||
@@ -754,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',
|
||||
]);
|
||||
@@ -775,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',
|
||||
|
||||
@@ -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,12 +2,23 @@
|
||||
|
||||
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',
|
||||
@@ -27,6 +38,19 @@ class Ticket extends Model
|
||||
// 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:Y-m-d',
|
||||
];
|
||||
@@ -70,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();
|
||||
}
|
||||
}
|
||||
|
||||
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;
|
||||
}
|
||||
}
|
||||
@@ -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