diff --git a/src/Http/Controllers/TicketAttachmentController.php b/src/Http/Controllers/TicketAttachmentController.php index 929d408..ea04dfc 100644 --- a/src/Http/Controllers/TicketAttachmentController.php +++ b/src/Http/Controllers/TicketAttachmentController.php @@ -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 diff --git a/src/Http/Controllers/TicketController.php b/src/Http/Controllers/TicketController.php index ba72f22..53c11af 100644 --- a/src/Http/Controllers/TicketController.php +++ b/src/Http/Controllers/TicketController.php @@ -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(); diff --git a/src/Http/Controllers/TicketMessageController.php b/src/Http/Controllers/TicketMessageController.php index b73b19d..7a970e7 100644 --- a/src/Http/Controllers/TicketMessageController.php +++ b/src/Http/Controllers/TicketMessageController.php @@ -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', diff --git a/src/Models/Ticket.php b/src/Models/Ticket.php index 8e511b2..872a25b 100644 --- a/src/Models/Ticket.php +++ b/src/Models/Ticket.php @@ -8,6 +8,16 @@ 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', @@ -70,4 +80,20 @@ 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; + } }