Deleting a merge target un-merges what was folded into it (#1567)

`tickets.merged_into_id` is `nullOnDelete()` and `destroy()` was a one-liner, so
deleting the surviving ticket of a merge blanked the pointer on every ticket
merged into it and left `status = 'merged'` behind. The row that came out was
read-only for good (#920 guards on the status), carried a banner naming a ticket
that no longer existed, reported `merged_inconsistent = false` — the flag that
exists to catch exactly this — and dropped out of the Merged view, which was
scoped by the pointer, leaving it visible only under Resolved.

`StrandedMerge::handleTargetDeletion()` runs inside destroy()'s transaction,
before the row goes: sources are RE-POINTED at the doomed ticket's own merge
target when it has one (the messages really did travel that far up the chain),
otherwise UN-MERGED to `closed` — out of the work queues, as they were, but
editable, reopenable and answerable again — with a system note naming what was
deleted and why. #938's dangling pointers are simply cleared: those rows are
somebody's open work. The flash names every ticket that changed, and the delete
confirm names them before the click instead of the generic "cannot be undone".

The shape prod already holds gets a name: `scopeStrandedMerge()`, reported as
`merged_inconsistent` (now the union of both broken shapes) plus
`merged_stranded` so a page can tell which fix to offer, listed by the Merged
view via `scopeEverMerged()`, and released one at a time by `unmerge`. #938's
repair scope is deliberately untouched — it writes `status = 'merged'`, which a
stranded row already has, and matching it would break its idempotency.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

src/Http/Controllers/TicketController.php

@@ -14,6 +14,7 @@ use Dashboard\Ticketing\Models\TicketParticipant;
 use Dashboard\Ticketing\Models\TicketStatus;
 use Dashboard\Ticketing\Models\TicketView;
 use Dashboard\Ticketing\Support\MergedTicketRepair;
+use Dashboard\Ticketing\Support\StrandedMerge;
 use Illuminate\Http\Request;
 use Illuminate\Routing\Controller;
 use Illuminate\Support\Facades\Auth;
@@ -169,9 +170,11 @@ class TicketController extends Controller
                 '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.
+                // would hide exactly the ones worth looking at. #1567: nor is the
-                'merged' => (clone $baseQuery)->whereNotNull('merged_into_id')->count(),
+                // pointer alone enough, because a stranded row has neither a
-                'merged_inconsistent' => (clone $baseQuery)->inconsistentlyMerged()->count(),
+                // target nor a disagreeing status. Either mark of a merge counts.
+                'merged' => (clone $baseQuery)->everMerged()->count(),
+                'merged_inconsistent' => (clone $baseQuery)->mergeInconsistent()->count(),
             ]
             : [
                 'all' => (clone $baseQuery)->count(),
@@ -223,7 +226,13 @@ class TicketController extends Controller
                 // 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');
+                //
+                // #1567 — and whatever its POINTER says, too. Scoping by the
+                // pointer alone dropped a ticket whose merge target had been
+                // deleted out of this view entirely (n went 1 → 0 on the delete),
+                // leaving it visible only under Resolved: the rows most in need of
+                // a human were the ones the view could not show.
+                $query->everMerged();
             } elseif (!$request->filled('status')) {
                 $query->whereNotIn('status', $closedSlugs);
             }
@@ -491,6 +500,21 @@ class TicketController extends Controller
             'agents'         => $agents,
             'priorities'     => $priorities,
             'ticketStatuses' => TicketStatus::orderBy('sort_order')->get(),
+            /*
+             * WHAT DELETING THIS WOULD TAKE WITH IT — #1567.
+             *
+             * The delete confirm was the generic "This cannot be undone.", which
+             * told an agent tidying up the surviving ticket of a merge nothing
+             * about the tickets folded into it. `destroy()` now un-merges them
+             * rather than stranding them, and this is how the confirm can say so
+             * before the click. Numbers only, and only for the manager who is
+             * offered Delete at all.
+             */
+            'mergedSources'  => $isAgent && $this->isManager($ticket->group_id)
+                ? Ticket::where('merged_into_id', $ticket->id)
+                    ->where('status', Ticket::STATUS_MERGED)
+                    ->orderBy('id')->pluck('number')->all()
+                : [],
         ]);
     }
 
@@ -673,14 +697,110 @@ class TicketController extends Controller
         return response()->json(['ok' => true]);
     }
 
+    /**
+     * DELETE, WITHOUT STRANDING WHAT WAS MERGED IN — [central-ticketing] #1567.
+     *
+     * `merged_into_id` is `nullOnDelete()`, so this used to be a one-liner that
+     * quietly broke every ticket folded into the row it deleted: pointer blanked,
+     * `status = 'merged'` left behind, permanently read-only, missing from the
+     * Merged view. {@see StrandedMerge::handleTargetDeletion()} is the whole fix,
+     * and it runs INSIDE the transaction and BEFORE the delete — it needs the
+     * doomed row's number for the system notes it writes, and its own
+     * `merged_into_id` to re-point the sources up the chain.
+     *
+     * The flash names what happened, because the agent who deleted this ticket
+     * was not looking at those tickets and has no other way to find out.
+     */
     public function destroy(Ticket $ticket)
     {
         if (!$this->isManager($ticket->group_id)) {
             abort(403);
         }
 
-        $ticket->delete();
+        $number = $ticket->number;
-        return redirect()->route('ticketing.index');
+
+        $handled = DB::transaction(function () use ($ticket) {
+            $handled = StrandedMerge::handleTargetDeletion($ticket, 'user '.Auth::id());
+            $ticket->delete();
+
+            return $handled;
+        });
+
+        return redirect()->route('ticketing.index')
+            ->with('success', $this->deletionFlash($number, $handled));
+    }
+
+    /**
+     * One sentence per thing that happened to somebody else's ticket. #1567.
+     *
+     * @param array{repointed: list<string>, unmerged: list<string>, cleared: list<string>} $handled
+     */
+    private function deletionFlash(string $number, array $handled): string
+    {
+        $parts = ["Ticket {$number} deleted."];
+
+        if ($handled['unmerged'] !== []) {
+            $parts[] = sprintf(
+                '%s %s merged into it and %s been un-merged (status %s) so %s stay usable: %s.',
+                count($handled['unmerged']),
+                count($handled['unmerged']) === 1 ? 'ticket was' : 'tickets were',
+                count($handled['unmerged']) === 1 ? 'has' : 'have',
+                Ticket::STATUS_UNMERGED,
+                count($handled['unmerged']) === 1 ? 'it' : 'they',
+                implode(', ', $handled['unmerged']),
+            );
+        }
+
+        if ($handled['repointed'] !== []) {
+            $parts[] = sprintf(
+                '%s %s merged into it and %s now recorded as merged into its own merge target: %s.',
+                count($handled['repointed']),
+                count($handled['repointed']) === 1 ? 'ticket was' : 'tickets were',
+                count($handled['repointed']) === 1 ? 'is' : 'are',
+                implode(', ', $handled['repointed']),
+            );
+        }
+
+        return implode(' ', $parts);
+    }
+
+    /**
+     * UN-MERGE A STRANDED TICKET — [central-ticketing] #1567.
+     *
+     * The recovery for the rows minted before `destroy()` learned to clean up
+     * after itself: `status = 'merged'` with no `merged_into_id`, read-only on the
+     * server (#920 reads the status) and pointing at nothing, so there was no
+     * sequence of clicks that could free one. This is that sequence.
+     *
+     * Scoped to {@see Ticket::isStrandedMerge()} and nothing else, so it cannot be
+     * used to un-merge a LIVE merge — a ticket whose target still exists is
+     * correctly read-only and stays that way.
+     */
+    public function unmerge(Ticket $ticket)
+    {
+        if (! $this->isAgent($ticket->group_id)) {
+            abort(403);
+        }
+
+        if (! $ticket->isStrandedMerge()) {
+            // Not an error: the button only renders on a stranded row, so getting
+            // here means a colleague already dealt with it, or the ticket is
+            // merged into something that still exists.
+            return redirect()->route('ticketing.show', $ticket)
+                ->with('success', $ticket->isMerged()
+                    ? 'That ticket is merged into a ticket that still exists — nothing to un-merge.'
+                    : 'That ticket is not merged — nothing to un-merge.');
+        }
+
+        StrandedMerge::release(
+            $ticket,
+            'The ticket this one had been merged into no longer exists, so this ticket was '
+            .'un-merged and is editable again.',
+            'user '.Auth::id(),
+        );
+
+        return redirect()->route('ticketing.show', $ticket)
+            ->with('success', "Ticket {$ticket->number} is un-merged and is no longer read-only.");
     }
 
     /**
@@ -769,8 +889,14 @@ class TicketController extends Controller
             // 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.
+            //
+            // #1567 gave `merged_inconsistent` a second shape, and "already
+            // consistent" would be a lie on one of them: a stranded row is still
+            // flagged, and what it needs is `unmerge()`, not this.
             return redirect()->route('ticketing.show', $ticket)
-                ->with('success', 'That ticket is already consistent — nothing to fix.');
+                ->with('success', $ticket->isStrandedMerge()
+                    ? 'That ticket is merged into a ticket that no longer exists — un-merge it instead.'
+                    : 'That ticket is already consistent — nothing to fix.');
         }
 
         MergedTicketRepair::repair($ticket, 'user '.Auth::id());

src/Models/Ticket.php

@@ -12,6 +12,22 @@ class Ticket extends Model
     /** Set only by TicketController::merge(); never offered by any form. */
     public const STATUS_MERGED = 'merged';
 
+    /**
+     * WHERE AN UN-MERGED TICKET LANDS — [central-ticketing] #1567.
+     *
+     * A merge is a way of closing a ticket, so a ticket that is un-merged because
+     * its target was deleted goes to `closed` and not to `open`: it stays out of
+     * the All Open / Mine / Unassigned work queues, exactly as it was before, and
+     * the only thing that changes is that it is no longer read-only, so an agent
+     * can reopen it, reply to it or reassign it. Un-merging to `open` would push
+     * somebody's tidied-away ticket back into a live queue without being asked.
+     *
+     * It is one of the six values the `tickets.status` ENUM allows and it is
+     * `is_closed` in `ticketing_statuses`, so both the column and every count
+     * that reads `$closedSlugs` already understand it.
+     */
+    public const STATUS_UNMERGED = 'closed';
+
     /**
      * 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
@@ -48,8 +64,12 @@ class Ticket extends Model
      * `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.
+     *
+     * #1567 added `merged_stranded` beside it, for the same reason: there are now
+     * TWO ways a merge can fail to add up and they need different offers, so the
+     * page is handed which one it is rather than working it out from the columns.
      */
-    protected $appends = ['merged_inconsistent'];
+    protected $appends = ['merged_inconsistent', 'merged_stranded'];
 
     protected $casts = [
         'due_date' => 'date:Y-m-d',
@@ -139,9 +159,93 @@ class Ticket extends Model
         return $this->merged_into_id !== null && ! $this->isMerged();
     }
 
-    /** @see $appends — the same answer, for the page. */
+    /**
+     * THE OTHER DIRECTION — [central-ticketing] #1567.
+     *
+     * `status = 'merged'` with NOTHING to point at. `merged_into_id` carries
+     * `nullOnDelete()`, so deleting the ticket that others had been merged INTO
+     * used to blank their pointer and leave the status behind: a row that
+     * {@see isMerged()} still calls read-only, whose banner names a ticket that
+     * no longer exists, and which the Merged view — scoped by the pointer — could
+     * not list. Nothing in the product could act on it.
+     *
+     * `destroy()` no longer mints these ({@see \Dashboard\Ticketing\Support\StrandedMerge}),
+     * but the rows already made are still out there, so the shape needs a name:
+     * this is what the badge offers to un-merge and what the Merged view now
+     * includes.
+     *
+     * Deliberately NOT folded into {@see scopeInconsistentlyMerged()}: that scope
+     * is what #938's repair writes `status = 'merged'` over, and a stranded row
+     * already HAS that status — repairing it would match forever and write
+     * nothing, which is exactly the idempotency the repair guarantees.
+     */
+    public function scopeStrandedMerge(Builder $query): Builder
+    {
+        return $query->where('status', self::STATUS_MERGED)
+            ->whereNull('merged_into_id');
+    }
+
+    /** The row-level twin of {@see scopeStrandedMerge()}. */
+    public function isStrandedMerge(): bool
+    {
+        return $this->isMerged() && $this->merged_into_id === null;
+    }
+
+    /**
+     * EITHER shape of a merge that does not add up — #938's and #1567's.
+     *
+     * The two are mutually exclusive by construction (one has a pointer and the
+     * wrong status, the other has the right status and no pointer), and the
+     * badge, the sidebar's warning count and `merged_inconsistent` all want the
+     * union: "this row's status and its pointer disagree, whichever way round".
+     * The repair scopes stay separate because the fixes differ.
+     */
+    public function scopeMergeInconsistent(Builder $query): Builder
+    {
+        return $query->where(function (Builder $q) {
+            $q->where(fn (Builder $i) => $i->inconsistentlyMerged())
+                ->orWhere(fn (Builder $s) => $s->strandedMerge());
+        });
+    }
+
+    /**
+     * Every row that carries the merge shape AT ALL — what the Merged view lists.
+     *
+     * Pointer set (whatever the status: #938's rows included) or status `merged`
+     * (whether or not the pointer survived: #1567's stranded rows included).
+     * Scoping the view by the pointer alone dropped a stranded row out of the one
+     * view whose whole job is showing merges, leaving it visible only under
+     * Resolved — so a reader looking for merged tickets never found it.
+     */
+    public function scopeEverMerged(Builder $query): Builder
+    {
+        return $query->where(function (Builder $q) {
+            $q->whereNotNull('merged_into_id')
+                ->orWhere('status', self::STATUS_MERGED);
+        });
+    }
+
+    /**
+     * @see $appends — the same answer, for the page.
+     *
+     * #1567 widened this to the union: a stranded row is inconsistent in the
+     * plainest sense (it says it is merged into nothing), and the flag existing
+     * while reading `false` on exactly that row was half of what the card
+     * reported.
+     */
     public function getMergedInconsistentAttribute(): bool
     {
-        return $this->isInconsistentlyMerged();
+        return $this->isInconsistentlyMerged() || $this->isStrandedMerge();
+    }
+
+    /**
+     * @see $appends — WHICH of the two shapes, for a page that must offer
+     * different fixes. `merged_inconsistent` says something is wrong;
+     * `merged_stranded` says the target is gone, so the offer is "un-merge"
+     * rather than #938's "mark merged".
+     */
+    public function getMergedStrandedAttribute(): bool
+    {
+        return $this->isStrandedMerge();
     }
 }

src/Support/StrandedMerge.php

@@ -0,0 +1,202 @@
+<?php
+
+namespace Dashboard\Ticketing\Support;
+
+use Dashboard\Ticketing\Models\Ticket;
+use Dashboard\Ticketing\Models\TicketMessage;
+use Illuminate\Support\Facades\Log;
+
+/**
+ * DELETING A MERGE TARGET MUST NOT STRAND WHAT WAS MERGED INTO IT — #1567.
+ *
+ * `tickets.merged_into_id` is declared `nullOnDelete()`, so deleting a ticket
+ * that other tickets had been merged INTO used to blank their pointer and walk
+ * away, leaving `status = 'merged'` behind with nothing on the other end:
+ *
+ *   - {@see Ticket::isMerged()} still calls the row read-only, so the page drops
+ *     the status / priority / assignee selects and the reply composer, and the
+ *     only control left on it is Delete. Nothing can reopen it, reassign it or
+ *     reply to it — ever.
+ *   - the banner "This ticket has been merged and is read-only." names a merge
+ *     target that no longer exists.
+ *   - the Merged view was scoped by the pointer, so the row vanished from the one
+ *     view whose job is showing merges and turned up only under Resolved.
+ *
+ * And `destroy()` gave no hint any of this was about to happen: the confirm was
+ * the generic "This cannot be undone."
+ *
+ * ── WHAT THIS DOES INSTEAD ──────────────────────────────────────────────────
+ *
+ * {@see handleTargetDeletion()} runs inside `destroy()`'s transaction, BEFORE the
+ * row goes, and gives every ticket pointing at it somewhere real to be:
+ *
+ *   RE-POINT, when the doomed ticket was itself merged into a third ticket. The
+ *   messages genuinely did travel that far — merge moves them to the target, so
+ *   merging the target onward moved them again — and the surviving ticket at the
+ *   end of the chain really is where this ticket's content lives. This is the
+ *   answer that keeps the merge true.
+ *
+ *   UN-MERGE, otherwise. There is nowhere left to point, so the row stops
+ *   claiming to be merged: {@see Ticket::STATUS_UNMERGED} (`closed`) and a NULL
+ *   pointer, which is an ordinary closed ticket — out of the work queues, but
+ *   editable, reopenable and answerable again.
+ *
+ * Either way the source gets a system note saying which ticket was deleted and
+ * what happened to it, because the row is being changed by somebody who was
+ * looking at a different ticket entirely and its own history is the only place
+ * that will ever explain it.
+ *
+ * ── THE ROWS ALREADY OUT THERE ──────────────────────────────────────────────
+ *
+ * This stops new ones. The stranded rows prod already holds are found by
+ * {@see Ticket::scopeStrandedMerge()}, now report `merged_inconsistent`, are
+ * listed by the Merged view, and are released one at a time by
+ * {@see \Dashboard\Ticketing\Http\Controllers\TicketController::unmerge()} —
+ * which calls {@see release()} here, so "un-merged" means one thing whichever
+ * door it came through.
+ *
+ * Deliberately NOT part of {@see MergedTicketRepair}: that class repairs #938's
+ * shape by writing `status = 'merged'` onto a row that has a target. This is the
+ * opposite operation on the opposite shape, and the two must not share a scope.
+ */
+class StrandedMerge
+{
+    /** How many rows are stranded right now — status `merged`, pointer NULL. */
+    public static function count(): int
+    {
+        return Ticket::query()->strandedMerge()->count();
+    }
+
+    /**
+     * The stranded rows themselves.
+     *
+     * @return \Illuminate\Database\Eloquent\Collection<int,Ticket>
+     */
+    public static function rows()
+    {
+        return Ticket::query()->strandedMerge()->orderBy('id')->get();
+    }
+
+    /**
+     * Give every ticket merged into `$target` somewhere real to be, before
+     * `$target` is deleted. Call inside the deleting transaction.
+     *
+     * Group membership is NOT re-checked per source. A source can live in another
+     * group, and the alternative to touching it is leaving it permanently frozen
+     * — this is the delete's own clean-up, not a write the operator chose to make
+     * on that ticket.
+     *
+     * @return array{repointed: list<string>, unmerged: list<string>, cleared: list<string>}
+     *         ticket numbers, by what happened to them
+     */
+    public static function handleTargetDeletion(Ticket $target, string $by): array
+    {
+        $sources = Ticket::where('merged_into_id', $target->id)->get();
+
+        $result = ['repointed' => [], 'unmerged' => [], 'cleared' => []];
+
+        if ($sources->isEmpty()) {
+            return $result;
+        }
+
+        // Where the content actually went. `merged_into_id` is nulled when ITS
+        // target is deleted, so this is either a live ticket or nothing.
+        $chain = $target->merged_into_id;
+
+        foreach ($sources as $source) {
+            // #938's shape: the pointer is stale but the status is live, so the
+            // row is an ordinary working ticket that happens to carry a dangling
+            // id. Dropping the id is the whole fix — and it is what the FK was
+            // about to do anyway; doing it here means the row is consistent
+            // inside this transaction rather than as a side effect afterwards.
+            if (! $source->isMerged()) {
+                $source->forceFill(['merged_into_id' => null])->save();
+                $result['cleared'][] = $source->number;
+                continue;
+            }
+
+            // `$chain === $source->id` cannot be produced by `merge()` (it
+            // refuses a merged source and a self-merge), but a cycle would make
+            // the row point at itself, which is worse than un-merging it.
+            if ($chain !== null && $chain !== $source->id) {
+                $chainTicket = Ticket::find($chain);
+
+                if ($chainTicket) {
+                    $source->forceFill(['merged_into_id' => $chainTicket->id])->save();
+
+                    TicketMessage::system($source, sprintf(
+                        'Ticket %s was deleted. This ticket had been merged into it, and %s '
+                        .'had itself been merged into %s — where its messages now live — so it '
+                        .'is recorded as merged into %s.',
+                        $target->number,
+                        $target->number,
+                        $chainTicket->number,
+                        $chainTicket->number,
+                    ));
+
+                    $result['repointed'][] = $source->number;
+                    continue;
+                }
+            }
+
+            self::release(
+                $source,
+                sprintf(
+                    'Ticket %s, which this ticket had been merged into, was deleted. This '
+                    .'ticket has been un-merged so that it is no longer read-only; its status '
+                    .'is now %s.',
+                    $target->number,
+                    Ticket::STATUS_UNMERGED,
+                ),
+                $by,
+            );
+
+            $result['unmerged'][] = $source->number;
+        }
+
+        Log::info(sprintf(
+            '[ticketing] #1567 deleting %s: re-pointed %d, un-merged %d, cleared %d dangling pointer(s) (%s).',
+            $target->number,
+            count($result['repointed']),
+            count($result['unmerged']),
+            count($result['cleared']),
+            $by,
+        ));
+
+        return $result;
+    }
+
+    /**
+     * Un-merge ONE ticket: status to {@see Ticket::STATUS_UNMERGED}, pointer to
+     * NULL, and a system note saying why.
+     *
+     * `forceFill`, not `update([...])`, for the same reason the rest of the merge
+     * code uses it: this is the machinery correcting its own state, and it must
+     * not be routed through anything that treats `merged` as read-only.
+     *
+     * Returns false when the row was not merged at all, so a double-clicked
+     * button and a stale page are no-ops rather than a second write.
+     */
+    public static function release(Ticket $ticket, string $reason, string $by): bool
+    {
+        if (! $ticket->isMerged()) {
+            return false;
+        }
+
+        $ticket->forceFill([
+            'status'         => Ticket::STATUS_UNMERGED,
+            'merged_into_id' => null,
+        ])->save();
+
+        TicketMessage::system($ticket, $reason);
+
+        Log::info(sprintf(
+            '[ticketing] #1567 un-merged %s to %s (%s).',
+            $ticket->number,
+            Ticket::STATUS_UNMERGED,
+            $by,
+        ));
+
+        return true;
+    }
+}

src/routes/ticketing.php

@@ -78,6 +78,8 @@ Route::middleware(['web', 'auth', 'app.access:ticketing'])
             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');
+            // #1567 — the one-click release for a row whose merge target was deleted.
+            Route::post('/{ticket}/unmerge',                  [TicketController::class, 'unmerge'])           ->name('unmerge');
             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');