From 420ef786039772b9538a8b96e7455385226b6032 Mon Sep 17 00:00:00 2001 From: central-worker-4 Date: Fri, 4 Sep 2026 02:53:23 -0700 Subject: [PATCH] A row the merge bug minted is visible, fixable, and repairable in bulk (#938) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #920 made a merged ticket read-only on the server and shipped NO data change, deliberately: its guard reads `status === 'merged'` and not `merged_into_id !== null`, so the rows the bug had already produced — the pointer set, the status back to `open` — stayed EDITABLE and could be corrected in the product instead of in SQL. What it left behind was rows that are simultaneously live in the All Open queue and merged into another ticket, with nothing on any screen saying so and nothing offering to fix them. Both halves of the card ship, because option A is a BEHAVIOUR RULE and a behaviour rule is a setting: B — the office fixes them one at a time. `merged_inconsistent` rides on the model (Ticket::$appends), so the list row, the index detail pane and the Show header all draw the same amber badge — "merged_into set but status open — fix?" — off one predicate rather than three copies of an expression. The badge IS the button: it opens a modal that says what will change and what will not, and posts to a new `mark-merged` verb that writes `status` and nothing else. A new "Merged" view lists every ticket folded into another one BY THE POINTER, so the consistent and the broken appear side by side; a status filter would show one or the other and never the pair worth comparing. A — the migration re-marks all of them at once, and it is INERT unless `ticketing.auto_repair_merged_on_deploy` is on. It defaults OFF, which is also what a box that has never heard of the setting does, so this can ship ahead of the operator's decision and change nothing unasked. The toggle is on the Automation tab with the live count beside it, because nobody should arm a bulk data change without seeing how many rows it touches. `MergedTicketRepair` is one implementation with three callers for one reason: "which rows are broken" and "what fixing one means" must not drift, or the badge offers a fix on a row the migration skips. THE PROD COUNT THE CARD ASKS FOR IS NOT IN HERE, and it could not be: this worker box holds no ssh private key, so central-prod is unreachable from it and there is no prod container locally. `php artisan ticketing:repair-merged --dry-run` is the read-only answer for whoever does have access — it prints the card's own SELECT COUNT and lists the rows, and `--force` (not the bare form) is the only spelling that writes. Repairing NEVER reverts. Messages, participants, assignee and title edits made to a row while it was wrongly editable are somebody's work and the only copy of it; there is no record of what the fields held before the bad save, so "reverting" would invent a previous state rather than restore one. Proved by tests/Feature/Ticketing/MergedTicketInconsistentRowsTest.php (22 tests) in dashboard-shell, over the fixture the card names — one inconsistent row beside a genuinely merged one, both produced by the real merge endpoint. The NEGCTRL half is what keeps it honest: the consistent row and an ordinary open ticket must come out of every path untouched, and a repaired row must keep the reply and the rename it was given after the merge. Co-Authored-By: Claude Opus 5 (1M context) --- ...001_repair_inconsistent_merged_tickets.php | 73 ++++++++++ src/Console/Commands/RepairMergedTickets.php | 67 +++++++++ src/Http/Controllers/TicketController.php | 52 ++++++- .../TicketingSettingsController.php | 16 +++ src/Models/Ticket.php | 48 +++++++ src/Support/MergedTicketRepair.php | 133 ++++++++++++++++++ src/TicketingServiceProvider.php | 1 + src/routes/ticketing.php | 2 + 8 files changed, 391 insertions(+), 1 deletion(-) create mode 100644 database/migrations/2026_09_04_000001_repair_inconsistent_merged_tickets.php create mode 100644 src/Console/Commands/RepairMergedTickets.php create mode 100644 src/Support/MergedTicketRepair.php diff --git a/database/migrations/2026_09_04_000001_repair_inconsistent_merged_tickets.php b/database/migrations/2026_09_04_000001_repair_inconsistent_merged_tickets.php new file mode 100644 index 0000000..7becc6e --- /dev/null +++ b/database/migrations/2026_09_04_000001_repair_inconsistent_merged_tickets.php @@ -0,0 +1,73 @@ +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; + } +} diff --git a/src/Http/Controllers/TicketController.php b/src/Http/Controllers/TicketController.php index 318d7a3..f5d50bc 100644 --- a/src/Http/Controllers/TicketController.php +++ b/src/Http/Controllers/TicketController.php @@ -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; @@ -147,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(), @@ -154,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']); @@ -181,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); } @@ -616,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)) { @@ -700,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. */ diff --git a/src/Http/Controllers/TicketingSettingsController.php b/src/Http/Controllers/TicketingSettingsController.php index 5786a4f..cd5720f 100644 --- a/src/Http/Controllers/TicketingSettingsController.php +++ b/src/Http/Controllers/TicketingSettingsController.php @@ -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.'); } } diff --git a/src/Models/Ticket.php b/src/Models/Ticket.php index 872a25b..435c10f 100644 --- a/src/Models/Ticket.php +++ b/src/Models/Ticket.php @@ -2,6 +2,7 @@ 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; @@ -37,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', ]; @@ -96,4 +110,38 @@ class Ticket extends Model { 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(); + } } diff --git a/src/Support/MergedTicketRepair.php b/src/Support/MergedTicketRepair.php new file mode 100644 index 0000000..8621f33 --- /dev/null +++ b/src/Support/MergedTicketRepair.php @@ -0,0 +1,133 @@ +inconsistentlyMerged()->count(); + } + + /** + * The rows themselves, for a human to look at before deciding. + * + * @return \Illuminate\Database\Eloquent\Collection + */ + 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; + } +} diff --git a/src/TicketingServiceProvider.php b/src/TicketingServiceProvider.php index 3167a72..c50c6e6 100644 --- a/src/TicketingServiceProvider.php +++ b/src/TicketingServiceProvider.php @@ -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, ]); } } diff --git a/src/routes/ticketing.php b/src/routes/ticketing.php index 6017c1f..312a53e 100644 --- a/src/routes/ticketing.php +++ b/src/routes/ticketing.php @@ -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');