diff --git a/src/Http/Controllers/TicketController.php b/src/Http/Controllers/TicketController.php index 7fb1d80..136b169 100644 --- a/src/Http/Controllers/TicketController.php +++ b/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. - 'merged' => (clone $baseQuery)->whereNotNull('merged_into_id')->count(), - 'merged_inconsistent' => (clone $baseQuery)->inconsistentlyMerged()->count(), + // would hide exactly the ones worth looking at. #1567: nor is the + // pointer alone enough, because a stranded row has neither a + // 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(); - return redirect()->route('ticketing.index'); + $number = $ticket->number; + + $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, unmerged: list, cleared: list} $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()); diff --git a/src/Models/Ticket.php b/src/Models/Ticket.php index 435c10f..e1363af 100644 --- a/src/Models/Ticket.php +++ b/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(); } } diff --git a/src/Support/StrandedMerge.php b/src/Support/StrandedMerge.php new file mode 100644 index 0000000..0cba1e4 --- /dev/null +++ b/src/Support/StrandedMerge.php @@ -0,0 +1,202 @@ +strandedMerge()->count(); + } + + /** + * The stranded rows themselves. + * + * @return \Illuminate\Database\Eloquent\Collection + */ + 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, unmerged: list, cleared: list} + * 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; + } +} diff --git a/src/routes/ticketing.php b/src/routes/ticketing.php index 312a53e..dad7af1 100644 --- a/src/routes/ticketing.php +++ b/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');