3 Commits

Author SHA1 Message Date
central-worker-4
420ef78603 A row the merge bug minted is visible, fixable, and repairable in bulk (#938)
#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) <noreply@anthropic.com>
2026-09-04 02:53:23 -07:00
central-worker-4
5315afc239 Put filterMessagesForRole's docblock back on filterMessagesForRole
The #920 guard landed between the comment and the function it describes.
2026-09-03 14:13:44 -07:00
central-worker-4
c0225d7691 A merged ticket is read-only on the server, not just on the page (#920)
The Show page announced "This ticket has been merged and is read-only.",
hid the reply composer and withheld the Merge button — and that was the
whole guard. It was presentation, on one page.

The Edit link and the Split button were still rendered, `edit()` still
served the form (with its Status select blank, because `merged` was not
one of the five slugs the form hard-coded), and `update()` took the save
with `errors: {}`: status went back to `open` while `merged_into_id` kept
pointing at the merge target. The row that came out was live and
assignable in the All Open queue AND merged into another ticket, holding
none of its own messages because the merge had moved them — an empty
shell that reads as a real duplicate of the ticket it was folded into.

So the rule now lives on the server, once — `Ticket::isMerged()` and
`refuseIfMerged()` — and every write the page presents as unavailable
calls it: update, edit, split, participants add/remove, and a reply or an
attachment (both hidden by the banner, neither refused by its route).

Two deliberate choices:

  - the guard reads `status === 'merged'`, NOT `merged_into_id !== null`.
    Reading the pointer would look stricter and would freeze every row
    this bug already minted (`status = open, merged_into_id = 56`), so
    the only way to correct one would be SQL.
  - it refuses with a redirect carrying the reason, not `abort(403)`.
    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 sentence than by an error screen.

The blank Status select was the report's second symptom, and it was the
same defect one layer up: the five hard-coded options meant any ticket
whose status was not among 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 happened to be showing. The
options now come from the same table `update()` validates against.
2026-09-03 13:59:24 -07:00
10 changed files with 504 additions and 5 deletions

View File

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

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

View File

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

View File

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

View File

@@ -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',

View File

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

View File

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

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

View File

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

View File

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