jwed/central-ticketing
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

A merged ticket is read-only on the server, not just on the page (#920)

Browse Source 
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.
This commit is contained in:
central-worker-4
2026-09-03 13:59:24 -07:00
parent ef4d0c4f93
commit c0225d7691
4 changed files with 113 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

73
src/Http/Controllers/TicketController.php
View File

@@ -50,6 +50,40 @@ class TicketController extends Controller
/**
* Filter messages for submitters — strip internal notes.
*/
/**
* 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);
}
private function filterMessagesForRole($messages, bool $isAgent)
{
if ($isAgent) {
@@ -417,12 +451,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 +473,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 +492,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 +507,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',
@@ -609,7 +662,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 +691,7 @@ class TicketController extends Controller
// Mark source as merged
$source->update([
'status' => 'merged',
'status' => Ticket::STATUS_MERGED,
'merged_into_id' => $ticket->id,
]);
});
@@ -656,6 +709,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 +811,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 +836,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();