25 Commits

Author SHA1 Message Date
paired-gate
b01eb91493 Merge commit '7e675e7beb25a8ddcb3f6cc6f0101f0ffd668e1e' into HEAD 2026-11-05 04:46:14 -07:00
central-worker-1
7e675e7beb #6168: Help Desk Settings can reach its email connections
The email-connection feature was whole on the server and unreachable in the
product. `ticketing_email_connections` has shipped since 2026_04_08_000008,
the three write routes are live and healthy — an empty POST answers a 422
validation bag — and index() has been computing an `emailConnections` prop on
EVERY settings render. The one thing missing was the segment that lets
somebody look at it: `email` was not in the `{tab}` whitelist, so the only
deep link anybody could guess fell through to the POST-only `/settings/email`
and answered 405 while its five siblings answered 200.

The segment is in the whitelist now, and the shell's Settings.vue grows the
tab that renders the prop (dashboard-shell, same branch).

Two things about the prop had to change before a UI could read it:

  · the stored mailbox PASSWORD no longer leaves the server. It was sitting in
    `config` on a prop that ships to everyone who can open Help Desk Settings;
    nothing rendered it before, so nothing is lost by shaping the rows by hand
    and sending `has_password` instead of the credential.

  · a save with the password box blank KEEPS the stored password. The form
    cannot prefill a password it is never sent, so an empty box has to mean
    "unchanged" — otherwise correcting a host would silently disconnect the
    mailbox, and the row would still read as configured.

Proof: tests/Feature/Ticketing/EmailConnectionsHaveADoorTest.php (shell) —
17 tests, including every tab key read out of the Vue's own TAB_KEYS and
asserted 200.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-11-04 06:55:35 -07:00
central-worker-1
16475ccac1 #6154: priorities 422 on an oversized sort_order instead of 500
`ticketing_priority_levels.sort_order` is an unsigned smallint (0..65535),
the same shape as `ticketing_statuses.sort_order`, but storePriority() and
updatePriority() validated it as `integer|min:0` with no upper bound. A value
past the ceiling passed validation and overflowed the column on INSERT/UPDATE
as an unhandled 500 — reachable by typing into the Sort Order box, which is a
plain `type="number" min="0"`.

#6081 put `max:65535` on storeStatus()/updateStatus() and left this identical
pair untouched. Same rule, same two places.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-11-04 05:53:38 -07:00
central-worker-6
e4d0622361 #6081: statuses store 422s on oversized sort_order instead of 500
sort_order was validated as integer|min:0 with no upper bound, but the
column is an unsigned smallint (0..65535). A client-supplied value
above the ceiling passed validation and overflowed the column on
INSERT/UPDATE, surfacing as an unhandled 500. Added max:65535 to both
storeStatus and updateStatus, which take the same client field through
the same rule.
2026-11-03 06:52:19 -07:00
central-worker-4
abd21f73f2 #5002: the rescue stamp is spent once the appointee uses it
VERIFY #5008 attacked the state #5002's own tests never built: not the rescue
that goes wrong and is undone within the minute, but the one that WORKS. A
group that was orphaned, was rescued, and is now being run by the person the
rescue appointed — with `recovered_by` still sitting on that person's row,
because the only thing that cleared it was the appointee re-saving their own
row, a write nobody makes.

So arm 3 of requireRemoveAccess() stayed open on the sole working manager of a
group that is plainly somebody else's business, which is the line the method's
own docblock says the stamp is narrow enough not to cross: strip the manager,
then "rescue" the group that press just orphaned, and a site admin who never
ran the desk manages it with the manager who staffed it gone. Two presses.

settleRescueStamp() clears the stamp on the actor's own row the moment they
ACT as the manager the rescue made them — staff the group, rename it, archive
it, give it a project or an email connection, remove a row: anything this file
lets a manager do to their own group. It is called from the three gates, not
from each action, because the gates are where "this write was authorised by
the actor's OWN manager row" is already decided, and a read of the page never
comes through them. The undo therefore lasts exactly as long as the mistake it
is for: until somebody takes the appointment up.

removableAgentIds follows without a change — it reads `recovered_by`, so the
button disappears with the gate.

  green: test_a_successful_rescue_does_not_leave_the_rescuer_able_to_take_the_group_over
         test_a_stamp_earned_on_one_group_is_not_a_claim_on_another
         all 17 of OrphanRecoveryIsReversibleTest, 215 Feature/Ticketing,
         3917 tests/Unit

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-10-21 06:58:49 -07:00
central-worker-4
51073fba1e #5002: the orphan recovery goes both ways
#4806 gave a site admin one write on a help desk group nobody is left to
run, and #4926 made the Person field of that form a people search. Neither
closed the other half: `storeAgent()` was gated on
requireManagerAccessOrOrphanRecovery(), `destroyAgent()` on plain
requireManagerAccess(). So the rescue could appoint and never un-appoint —
the row it had just written sat in the rescuer's own Agents list marked
"Not yours to manage", with no Remove control and a 403 behind the DELETE.

A `manager` press was worse: the write ends the orphan state, so the group
left `orphanedGroupIds` and the admin could not remove the row, appoint
anybody else, or appoint themselves. The only person who could undo it was
the person wrongly appointed — #4806's frozen group, reached through
#4806's cure.

destroyAgent() now asks requireRemoveAccess(): a manager of the group, or a
site admin while the group is STILL orphaned (symmetric with the write and
strictly narrower — on such a group they could appoint themselves and then
remove anything anyway), or a site admin undoing a row their own rescue
wrote. That last arm needs provenance, hence `recovered_by`: "the group
would be orphaned without this row" cannot tell a mistaken appointment from
the sole working manager of a healthy group, and admitting the second would
let any admin strip a manager and seize the desk (#4710). The stamp is
cleared the moment a manager of the group writes the row themselves.

`removableAgentIds` is that gate in prop form, so the Remove button and the
DELETE cannot disagree, and the confirmation says when a removal hands the
group back to having nobody.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-10-21 06:22:24 -07:00
central-worker-3
8b4f788e43 #4932: the picker's two-character floor counts characters, not bytes
VERIFY #4932's named red — test_a_single_non_ascii_letter_is_still_a_one_
character_query — stood: searchUsers() guarded with strlen($q) < 2 under a
comment that calls the floor "two characters". A single Ł, É, ü or a CJK
surname is one letter and two or three bytes, so one keystroke cleared the
byte floor and answered with full chip rows — the directory spill the floor
exists to prevent, on exactly the names a school has the most trouble typing.

mb_strlen, matching admin.settings.users.search (the endpoint this picker's
row shape is keyed against), which answers [] for the same query. The
verifier's test is carried onto this branch so the fix lands with its proof.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-10-20 07:28:24 -07:00
central-worker-3
a977aedc95 #4926: appointing a help desk agent is a people search, and the receipt names them
The Agents tab's add-an-agent form opened with

    <label class="form-label">User ID</label>
    <input v-model="agentForm.user_id" required type="number"
           placeholder="User ID" class="form-input" />
    <p class="form-hint">Find IDs in Admin → Users</p>

— a primary key asked of an office administrator, on the one screen #4806
documents as the only recovery for a help desk group whose last manager can no
longer act. `exists:users,id` refuses an id nobody holds, so the failure that
mattered was never a typo that errors: 47 typed for 74 is two real accounts,
and the one written is handed `agent` — or `manager` — over a whole queue, with
the person named nowhere before the press or after it. #4781's audit row was
therefore the first place anybody could find out.

Two writes here, both small:

  * `searchUsers()` — `q` over name and email, answered by `PersonChip::rows()`,
    the platform's one serializer for a searchable person (#871), so the
    operator's People setting reaches this picker like every other one. The
    gate is the page's own requireAgentAccess() and not a per-group one: the
    form picks the PERSON before the GROUP, and the write still goes through
    requireManagerAccessOrOrphanRecovery() unchanged.

  * `storeAgent()`'s flash names the appointee, through `PersonNames::display()`
    so it reads the way the chip above it does (#524). The recovery sentence
    keeps every word it had and gains the other half of itself.

The shell half — the picker's markup, its proofs and the chip-surface
registration — is on dashboard-shell card/4926-….

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-10-20 07:07:46 -07:00
10ef42d78c #4806 bring central-ticketing master into the half before the paired landing
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MPh12gFm8cqvAt2aPtPw3q
2026-10-19 23:24:18 -04:00
central-worker-8
9cc2e59b45 #4781: the bulk merged-repair is on the record too
MergedTicketRepair::repairAll() re-marks tickets as merged (permanently
read-only) per model instead of with one builder update, so each frozen
ticket gets its own Changes-log entry — the same as the per-ticket button.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-19 05:16:23 -07:00
central-worker-5
94fb6ac53c #4806: a help desk group whose last manager can no longer act is recoverable
Every write in TicketingSettingsController is requireManagerAccess() — an
existing manager of that exact group, or 403 — and settings.agents.store, the
only route that writes a `ticketing_agent_access` row, is gated the same way.
storeGroup() auto-appoints a manager only for a group it creates. So the
moment a group's last manager stopped being able to act, the group froze for
good: no rename, no archive, no delete, no project, no agent, and no ticket in
it ever assigned, worked or closed. The page's own answer on the Agents tab
was "Ask a manager of the group to add you", which on such a group names
nobody.

On dev that was every group: all 15 held exactly one agent row, the same
person on every one, and that account had been rewritten into an
admissions-only applicant (#4202/#4203) and could not reach /app/ticketing at
all. Fifteen groups, fourteen tickets, nobody who could administer any of it,
and nothing a super admin could press — the "ticketing lifecycle is DEAD on
dev" observation of 2026-10-18 is this, not a seed gap.

One carve-out, and no more: a site admin may write an agent row for a group
that has NO manager left who could still use this page. "Still" is the
operative word and canStillAdminister() is its definition — the account must
exist, be active, and hold `ticketing.settings`, which is exactly what the
route itself demands, so it answers "could this manager have done this write
themselves". A manager row outlives all three.

#4710's rule is untouched: the carve-out is a property of the GROUP, not of
the caller, so a group somebody is still running remains none of an admin's
business, and the rescue grants the rescuer nothing beyond the row they wrote
— appoint somebody else and the very next request is refused again.

The page gets `orphanedGroupIds` so the Agents tab can offer the recovery,
label those groups "— no manager", and explain what appointing one does
(shell side).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-10-19 05:15:12 -07:00
central-worker-6
adcb02863d #4781: the nightly auto-close is on the record too
`ticketing:auto-close` closed tickets with a single bulk UPDATE, which
fires no model events. With the audit declaration in place that left one
way of closing a ticket invisible: the Changes log would show a ticket
going open → resolved and then nothing, while the row itself read
`closed`. "Who closed this" is the question the trail exists to answer and
"the nightly sweep did" is a real answer — but only if the sweep writes it.

Closed one at a time through the model, chunked by id so a first sweep
over a backlog does not load every resolved ticket at once. Paging by id
is what makes that safe while the filtered column is being written.
2026-10-19 04:57:38 -07:00
central-worker-6
428a9e42c6 #4781: Help Desk writes to the shell's Changes log
`ticketing` was not one of the apps `audit_events` had ever seen — 4,378
rows, fifteen apps, none of them this one, since the table was created. A
super admin created a ticket and replied to it and the only new row
afterwards was the shell recording their own visit to the log page.

Nothing in core was wrong: AuditRegistry's docblock is explicit that
snap-ins declare their own models from their own providers and that core
never references a snap-in. Ticketing simply never declared.

TicketingServiceProvider::boot() now declares four models, in the shape
FormsServiceProvider uses for FormCollaborator — the exact precedent, for
the exact reason:

  - TicketingAgentAccess — the grant. `settings.agents.store`/`.destroy`
    hand out and take back access to a whole queue of other people's
    correspondence and left no trace at all. This is the one that matters.
  - TicketingGroup — name, prefix, email_address, archived_at, so the one
    button that archives-or-deletes a desk says which it did.
  - Ticket — created, deleted, moved, re-assigned, re-prioritised, closed,
    merged.
  - TicketParticipant — the narrower way somebody comes to see a ticket.

Nothing is declared `sensitive`: that buys read-audit, which on Ticket
would record every agent opening every ticket all day and bury the writes
this card is about. TicketMessage is deliberately not declared either —
it can only ever be created (one controller method, no edit, no delete
route), every inbound email writes one under `system`, and the thread is
already the permanent record of what was said.

removeParticipant deleted through the query builder, which fires no model
events, so it would have recorded every add and no removal — a trail that
reads as though nobody is ever taken off a ticket. It now deletes through
the model.
2026-10-19 04:53:23 -07:00
central-worker-3
eb198ab05a #4710: Help Desk Settings shows the help desk that exists
The Groups, Agents and Projects tabs were scoped to the viewer's
`ticketing_agent_access` rows. A site admin holds none of those, so the
lists came back empty and the Vue — which decided between "first run"
and "nothing for you" on list length alone — rendered the first-run
copy: "No groups yet. Create your first group above.", on a page whose
own props said `isBootstrap: false` and `isSiteAdmin: true`. The two
empty states on that page contradicted each other and the one that won
was the one inviting a duplicate of a group that already existed.

On dev that was the normal case, not a corner: every pre-existing group
had zero agent-access rows, so the whole configuration was invisible to
the one role meant to own it.

The list is now what this page is for: a site admin sees every group
(archived ones included, as before), agent, project and email
connection. What they may DO is unchanged — requireManagerAccess()
still gates every write per group, which is what GroupRetirementTest
already demanded of exactly this user. So the page also ships
`myManagedGroupIds`: the rows a viewer cannot act on say "Not yours to
manage" instead of offering an Edit that 403s, and the add-agent /
add-project group pickers offer only groups the viewer manages.

Known and deliberately left: a group with zero agent-access rows is
still frozen — adding its first agent needs a manager row on it, which
nobody holds. It is now visible and labelled rather than invisible, but
thawing it is a separate decision (operator notified).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-10-17 04:58:55 -07:00
central-worker-5
dbe3677093 #3144: this app's scheduled jobs run on the school's clock, not the server's
`dailyAt()`/`weeklyOn()` with no `->timezone()` are read against
`config('app.timezone')`, which is UTC on every box this platform runs on, so
every line here that pinned an hour was running seven or eight hours early.
Each now names \App\Support\Timezone::organisation(), the one place the
school's own zone is decided.

organisation() and not current(): there is no signed-in user on a scheduler
tick, so the two agreed only by falling through, and stop agreeing the moment
anything resolves the schedule inside a request. It reads a setting and cannot
throw — it catches its own failure and falls back to America/Vancouver.

The shell half ([dashboard-shell] #3144) carries the guard that keeps this
true: it walks the REAL schedule, so a snapins.lock bump that brings back an
unclocked line reddens the shell's run.
2026-09-28 19:27:56 -07:00
central-worker-6
0dbc81684b #2980: ticketing:auto-close is scheduled by this snap-in, not by the shell
A spawn of [dashboard-shell] #2965. The shell's routes/console.php carried

    Schedule::command('ticketing:auto-close')
        ->dailyAt(ScheduleStagger::dailyAt('ticketing:auto-close'));

for a command only this repository defines, and two repositories deploy at
different moments. On 2026-09-26 01:10 PDT production met the other end of
that: `Command "admissions:remind-unconfirmed-links" is not defined`, exit 1,
every tick for nine and a half hours, with nothing an operator could press —
the code the schedule named was not on the box yet.

TicketingServiceProvider::boot() now registers the event itself, beside the
$this->commands() call that defines AutoCloseTickets. A box has both or
neither, because they arrive in the same commit; a shell without this snap-in
gets no row in `schedule:list` rather than a nightly failure about one.

WHAT DID NOT MOVE. The time: ScheduleStagger is the shell's one place a start
time is chosen — a bare ->daily() is 00:00:00, where the hourly and
quarter-hourly maps already meet ([dashboard-shell] #1910) — so the provider
asks dailyAt() and the job stays at 00:12 exactly. And the absence of a lock:
the shell's line carried no withoutOverlapping() and this adds none, because a
mutex grown in passing is a behaviour change wearing a move's clothes.

PAIRED with the shell half on card/2980: the console line, the
TheShellSchedulesNoSnapInCommandTest::BASELINE entry and the
ScheduledJobCatalog::NOT_A_REGISTERED_JOB excuse all go in the same round as
this lands, and tests/Feature/Ops/TicketingSchedulesItsOwnAutoCloseTest
asserts both ends at once.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-27 04:29:21 -07:00
central-worker-3
7522d48ab9 Merge master (#1706 participants-picker ordering) into card/1710 2026-09-13 13:39:19 -07:00
central-worker-1
b80e200f83 The singular delete-flash agrees with itself: "so it stays usable" (#1710)
deletionFlash() (added by #1567) pluralised the count, the noun, the
auxiliary and the pronoun, but left the verb fixed at "stay" — so the
singular branch, which is the common one, shipped "1 ticket was merged
into it ... so it stay usable".

The verb agrees with the pronoun, so it belongs in the same ternary:
"it stays" / "they stay". Nothing else about the message changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-11 14:15:01 -07:00
central-worker-1
332551e2e3 The participants picker is sorted by the name it shows (#1706)
QA on [central-ticketing] #1598, reading the dropdown back after the
picker became chips: typing `an` listed "Toews, Fast, Ivanov, Mwangi,
Bergman, Anderson" — to an operator, no order at all.

It was in fact perfectly ordered. `searchUsers()` came out of #1598's
chip conversion with `->orderBy('name')`, and `users.name` is the cached
"First Last" ("Abigail Toews", "Andrew Fast", "Avery Ivanov") — the one
string a chip row no longer prints. A list labelled "Last, First" and
collated by "First Last" reads as unsorted, which is #524's own reported
defect; the sibling conversion on [central-staff-employment] #1597 names
it as the thing it had to fix at the same moment.

So the collation moves with the label: `PersonNames::orderUsers()`, the
platform's single answer, which the reference search
(`admin.settings.users.search`) and the other chipped pickers already
use, and which honours the operator's `name_format` rather than assuming
surname-first.

Not cosmetic: the sort runs before `limit(10)`, so on a common surname
the two collations offer a different set of people, not the same set
shuffled.

Three tests in the shell suite (TicketParticipantsAreChipsTest) — the
collation, the `name_format` setting, and which ten survive the limit.
The first and third are red against `orderBy('name')`.
2026-09-11 13:47:08 -07:00
central-worker-4
c492becbdb The participants picker searches, and answers, in chips (#1598)
The operator, 11:46 PDT 2026-09-10, having just had the device picker
converted: "are there other places we can search users where we're not
using the chips? please go through and check." The lead's audit found ten
person searches on the platform — four drawing chips, six not — and this
is central-ticketing's.

`ticketing.users.search` answered `DB::table('users')->get(['id', 'name',
'email'])`, and `show()` handed the Participants card the same three
columns for the submitter and for everybody already on the ticket. So the
one card on this page that is entirely about WHO SOMEBODY IS drew three
text rows in three different shapes: a name over an address in the
dropdown, a name-or-address string per participant, and a third spelling
for the submitter. An agent adding a participant to a ticket about a
child chooses between the child and the parent of the same surname with
no role dot to tell them apart.

And it was worse than untidy. Name and email were the SHIPPED DEFAULT for
a chip, so a school that configured the student chip to grade + homeroom
kept being shown an address here — on the one people surface the
operator's own setting could not reach. That is the defect #871 exists to
remove, and this endpoint was still carrying it.

Both halves now go through `PersonChip::rows()`, so what a row SAYS is
the setting rather than a select list in this file, and the person PICKED
is byte-for-byte the person picked FROM — one serializer, one answer. The
search's payload is now the same, key for key, as
`admin.settings.users.search`; the test compares the two endpoints against
each other rather than against a literal list, because a literal passes on
the day it is written and then lets this surface fall behind the next key
the shared row grows.

`PersonChip::columns()` is the select list, not `['id', 'name', 'email']`:
a chip line may name any user column, and a constrained select hands the
helper a model whose attribute is simply ABSENT — which it cannot tell
from a person who has no value, so the line goes blank for everybody with
nothing failing.

`rows()` and NOT the model, in `show()`. That map also feeds the message
bylines and the assignee, so handing the page a `User` would put every
column of `users` — `preferred_pin`, `preferred_password`,
`date_of_birth` — in front of any agent who opens a ticket. `rows()` is
`{id, name, email, person}` and `person` is the curated allow-list. There
is a test that a PIN cannot reach the page.

WHO IS SEARCHABLE DOES NOT CHANGE, per the card. Same two-character
floor, same name-or-email `like`, same limit of 10, same exclusion of the
people already on the ticket, same agent gate; `User` carries no global
scope or soft delete `DB::table('users')` did not. The name-or-email
clause stays inside its own closure on purpose — flattened beside the
exclusion it is `(name) OR (email AND NOT IN)` by SQL's own precedence,
so a name match jumps the exclusion, which is the hole the same conversion
opened on [central-inventory] #1594. `orderBy('name')` is the one visible
difference and it is new: the old query had no order at all, so the same
two letters could list the same ten people differently twice running.

The Vue half, the source guard and the feature tests are on
dashboard-shell's branch of the same name.
2026-09-10 12:34:50 -07:00
central-worker-2
2b6d9b1651 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>
2026-09-10 05:36:07 -07:00
central-worker-3
5eb2ec7bcf The Unassigned queue stops offering merged tickets as work (#1054)
The count and the list were the same question spelled two ways in one
method: the sidebar counts scope by $closedSlugs, read from
ticketing_statuses, while the `unassigned` and `mine` list branches
carried a hand-written ["resolved", "closed"] that predates the `merged`
status. So a correctly merged ticket — status merged, merged_into_id set,
its own Show page already calling it read-only — stayed in the queue
agents pull new work from, and the page contradicted itself on screen:
"Unassigned 4" beside five rendered rows.

Both queues now scope by $closedSlugs, so they exclude exactly what their
own counts exclude. The scoping is by STATUS and not by merged_into_id on
purpose: #938 rows (pointer set, status still open) are live, editable
work until a human repairs them, and dropping them out of the queue would
hide the rows that most need one. There is a NEGCTRL test for that.

Also, the secondary finding: ?filter=closed was accepted, silently
returned the All-Open set and was echoed back in props.filters as though
a Closed view existed. The sidebar has six views and that is not one of
them, so an unrecognised filter normalises to `all` and says so. The list
is now a constant that savePrefs() validates against too, rather than the
same six slugs written out twice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-04 15:06:02 -07:00
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
14 changed files with 1884 additions and 42 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,43 @@
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
/**
* WHO WROTE THIS ROW AS A RESCUE — [central-ticketing] #5002.
*
* #4806 let a site admin appoint somebody to a help desk group that has no
* manager left who could still act. That write is the only way back into such
* a group, and it ENDS the condition that allowed it: appoint a `manager` and
* the group has a manager again, so the recovery arm shuts behind the press.
* With `destroyAgent()` gated on plain manager rights, a rescuer who named the
* wrong person could not take the row back — the only person who could was the
* person wrongly appointed.
*
* `orphanedGroupIds()` cannot tell that case from a healthy group with one
* working manager: both are "orphaned if you disregard this row", and allowing
* the second would let any site admin strip a manager from a group that is
* none of their business (#4710). So the row has to say where it came from.
*
* Nullable, and null on every row written the ordinary way: this is not
* "created_by", it is "this row exists because <user> rescued the group, and
* it is theirs to undo". No foreign key — `user_id` on this table has none
* either, and a deleted rescuer should leave the row alone, not take it.
*/
return new class extends Migration
{
public function up(): void
{
Schema::table('ticketing_agent_access', function (Blueprint $table) {
$table->unsignedBigInteger('recovered_by')->nullable()->after('role');
});
}
public function down(): void
{
Schema::table('ticketing_agent_access', function (Blueprint $table) {
$table->dropColumn('recovered_by');
});
}
};

View File

@@ -22,9 +22,39 @@ class AutoCloseTickets extends Command
$cutoff = now()->subDays($days);
$count = Ticket::where('status', 'resolved')
/*
* CLOSED ONE AT A TIME, THROUGH THE MODEL — [central-ticketing] #4781.
*
* This was a single `->update(['status' => 'closed'])`, which is one
* statement and no model events. Since #4781 a ticket's status change is
* an audited fact, and a bulk update would have left exactly one way of
* closing a ticket invisible: the Changes log would show a ticket going
* open → resolved and then nothing, while the row itself said `closed`.
* "Who closed this" is the question the audit trail exists to answer,
* and "the nightly sweep did, on this date" is a real answer — but only
* if the sweep writes it.
*
* Per-ticket rather than one summary event because the answer has to be
* on THE TICKET: the per-record history panel reads rows whose subject
* is that ticket, and an aggregate "closed 14 tickets" row would not
* appear in any of their histories.
*
* `chunkById`, so a first sweep over a long backlog does not load every
* resolved ticket into memory at once. Paging by id is what makes this
* safe while the filtered column is being written: each page continues
* from the last id seen rather than from an offset, so rows dropping out
* of `status = 'resolved'` cannot cause a page to be skipped.
*/
$count = 0;
Ticket::where('status', 'resolved')
->where('updated_at', '<=', $cutoff)
->update(['status' => 'closed']);
->chunkById(200, function ($tickets) use (&$count) {
foreach ($tickets as $ticket) {
$ticket->forceFill(['status' => 'closed'])->save();
$count++;
}
});
$this->info("Auto-closed {$count} ticket(s) resolved more than {$days} day(s) ago.");

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

@@ -3,6 +3,9 @@
namespace Dashboard\Ticketing\Http\Controllers;
use App\Models\Setting;
use App\Models\User;
use App\Support\PersonChip;
use App\Support\PersonNames;
use Dashboard\Ticketing\Models\PriorityLevel;
use Dashboard\Ticketing\Models\Ticket;
use Dashboard\Ticketing\Models\TicketAttachment;
@@ -13,6 +16,8 @@ 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 Dashboard\Ticketing\Support\StrandedMerge;
use Illuminate\Http\Request;
use Illuminate\Routing\Controller;
use Illuminate\Support\Facades\Auth;
@@ -23,6 +28,13 @@ use Inertia\Response;
class TicketController extends Controller
{
/**
* The sidebar's views, and the only values `?filter=` means anything as.
* Index.vue's `visibleViews` draws exactly these; `savePrefs()` validates
* against them; `index()` normalises anything else to `all` (#1054).
*/
private const VIEW_FILTERS = ['all', 'mine', 'unassigned', 'pending', 'resolved', 'merged'];
private function agentGroupIds(): array
{
return TicketingAgentAccess::where('user_id', Auth::id())
@@ -47,6 +59,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.
*/
@@ -77,6 +123,18 @@ class TicketController extends Controller
$request->merge(['filter' => $pref['filter']]);
}
// #1054 — a `filter` the sidebar has no view for used to fall through to
// the All Open branch and then be echoed straight back in `props.filters`,
// so `?filter=closed` looked like a real view that just happened to be
// showing open tickets. There is no Closed view anywhere in the UI;
// anything unrecognised is All Open, and now says so. The list is the
// one `savePrefs()` validates against and the one Index.vue draws.
// Only rewrites a filter that was actually asked for: an absent one keeps
// meaning "no view chosen", which is what leaves `filter` out of the URL.
if ($request->filled('filter') && ! in_array($request->filter, self::VIEW_FILTERS, true)) {
$request->merge(['filter' => 'all']);
}
// If no groups exist at all, render a first-run / bootstrap state
$totalGroups = TicketingGroup::count();
if ($totalGroups === 0) {
@@ -113,6 +171,13 @@ 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. #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(),
@@ -120,6 +185,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']);
@@ -139,14 +206,36 @@ class TicketController extends Controller
if ($request->filled('project_id')) {
$query->where('project_id', $request->project_id);
}
// #1054 — the work queues scope by `$closedSlugs`, the same list the
// counts above use. They used to spell it `['resolved', 'closed']`
// by hand, which predates the `merged` status: a correctly merged
// ticket stayed in Unassigned as pickable work while the sidebar
// count next to it — already reading `$closedSlugs` — excluded it,
// so the view disagreed with its own header (4 vs 5). Scoping by
// STATUS and not by `merged_into_id` is deliberate: #938's
// inconsistent rows (pointer set, status still open) are live work
// until somebody repairs them, and hiding them from the queue would
// hide the rows that most need a human.
if ($request->filter === 'mine') {
$query->where('assigned_to', $user->id)->whereNotIn('status', ['resolved', 'closed']);
$query->where('assigned_to', $user->id)->whereNotIn('status', $closedSlugs);
} elseif ($request->filter === 'unassigned') {
$query->whereNull('assigned_to')->whereNotIn('status', ['resolved', 'closed']);
$query->whereNull('assigned_to')->whereNotIn('status', $closedSlugs);
} elseif ($request->filter === 'pending') {
$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.
//
// #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);
}
@@ -382,7 +471,39 @@ class TicketController extends Controller
->push($ticket->submitter_id)
->push($ticket->assigned_to)
->filter()->unique();
$users = DB::table('users')->whereIn('id', $userIds)->get(['id', 'name', 'email'])->keyBy('id');
/*
* EVERY PERSON ON THIS PAGE, IN THE PLATFORM'S ROW SHAPE (#1598).
*
* This was `DB::table('users')->get(['id', 'name', 'email'])`, and the
* Participants card it feeds drew each of those as
* `{{ p.user?.name || p.user?.email }}` — a bare text row where the rest
* of the platform draws a chip. `PersonChip::rows()` is the shared
* serializer, so what those rows SAY is the operator's chip setting
* rather than this method's select list, and it is the same payload the
* participant SEARCH answers with: the person picked and the row they
* were picked from cannot disagree about what they are called.
*
* `rows()` AND NOT THE MODEL. This map also feeds the message bylines
* and the assignee, so a `User` handed straight to Inertia would put
* every column of `users` — `preferred_pin`, `preferred_password`,
* `date_of_birth` — on the page for anybody who has ever replied to a
* ticket. `rows()` is exactly `{id, name, email, person}`, and `person`
* is the curated allow-list `PersonChip::FIELDS` describes.
*
* ONE BULK PASS for the whole page: `forMany()` inside `rows()` reads
* the alias catalogue and the school year once for the collection, and a
* chip built per row would repeat that for every participant, every
* message author and the assignee on a fifty-reply thread.
*
* `PersonChip::columns()` is the select list because a chip line may
* name ANY user column, and a constrained select hands the helper a
* model whose attribute is simply absent — which it cannot tell from a
* person who has no value, so the line goes blank for everybody with
* nothing failing.
*/
$users = collect(PersonChip::rows(
User::whereIn('id', $userIds)->get(PersonChip::columns())
))->keyBy('id');
$visibleMessages->each(function ($msg) use ($users) {
$msg->author = $msg->user_id ? ($users[$msg->user_id] ?? null) : null;
@@ -414,15 +535,35 @@ 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()
: [],
]);
}
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 +575,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 +594,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 +609,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 +718,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:'.implode(',', self::VIEW_FILTERS),
]);
if (empty($validated)) {
@@ -577,14 +732,112 @@ 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);
}
$number = $ticket->number;
$handled = DB::transaction(function () use ($ticket) {
$handled = StrandedMerge::handleTargetDeletion($ticket, 'user '.Auth::id());
$ticket->delete();
return redirect()->route('ticketing.index');
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 usable: %s.',
count($handled['unmerged']),
count($handled['unmerged']) === 1 ? 'ticket was' : 'tickets were',
count($handled['unmerged']) === 1 ? 'has' : 'have',
Ticket::STATUS_UNMERGED,
// The verb agrees with the pronoun, so it lives in the same ternary:
// "so it stays usable" / "so they stay usable".
count($handled['unmerged']) === 1 ? 'it stays' : 'they stay',
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.");
}
/**
@@ -609,7 +862,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 +891,7 @@ class TicketController extends Controller
// Mark source as merged
$source->update([
'status' => 'merged',
'status' => Ticket::STATUS_MERGED,
'merged_into_id' => $ticket->id,
]);
});
@@ -647,6 +900,48 @@ 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.
//
// #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', $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());
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 +951,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 +1053,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,9 +1078,21 @@ class TicketController extends Controller
abort(403);
}
if ($refusal = $this->refuseIfMerged($ticket, stayPut: true)) {
return $refusal;
}
// Fetched and deleted one model at a time rather than as a query-builder
// delete, because a builder delete fires no model events — and since
// #4781 a TicketParticipant row IS an audited fact (who can see this
// ticket's correspondence). Deleting in bulk here would have recorded
// every add and no removal, which is a worse trail than none: it reads
// as though nobody is ever taken off a ticket. ->get() rather than
// ->first(), so a duplicated pair is still fully removed.
TicketParticipant::where('ticket_id', $ticket->id)
->where('user_id', $userId)
->delete();
->get()
->each->delete();
return back()->with('success', 'Participant removed.');
}
@@ -786,6 +1101,66 @@ class TicketController extends Controller
* Search users by name/email — for the participant picker.
* Returns up to 10 matches, excluding existing participants and the submitter.
*/
/**
* WHO TO ADD AS A PARTICIPANT — and it answers in CHIPS (#1598).
*
* The operator, 11:46 PDT 2026-09-10, having just had the device picker
* converted: *"are there other places we can search users where we're not
* using the chips? please go through and check."* This endpoint was one of
* the six the lead's audit found that were not, and it is central-ticketing's.
*
* It returned `DB::table('users')->get(['id', 'name', 'email'])`, so the
* picker drew a name and an address per row and nothing else: no role dot to
* tell an agent that "D. Ellis" is the child rather than the parent of the
* same surname, and — because those two fields were once the WHOLE of a chip
* — a school that configures the student chip to grade + homeroom would have
* been left with this one picker still showing an address.
*
* `PersonChip::rows()` is the shared serializer and this is now the same
* answer, key for key, as `admin.settings.users.search` — which is the point
* rather than a coincidence: `TicketParticipantsAreChipsTest` compares the
* two payloads against each other instead of against a literal list, so a
* key added to the platform's row cannot skip this surface.
*
* WHO IS SEARCHABLE DOES NOT CHANGE. Same `like` on name-or-email, same
* two-character floor, same limit of 10, same exclusion of the people
* already on the ticket, and `User` carries no global scope or soft delete
* that `DB::table('users')` did not — measured against the model on
* 2026-09-10. The card was explicit that this one is about the DRAWING.
*
* Two things about the query are not cosmetic:
*
* - `PersonChip::columns()` and not `['id', 'name', 'email']`. A chip line
* may name any user column, and a constrained select hands the helper a
* model whose attribute is simply ABSENT — indistinguishable from a
* person who has no value, so the line is blank for everybody with
* nothing failing. `PersonChip::columns()`' own docblock has the trap;
* `PersonChipSurfacesTest::test_a_chip_is_never_built_from_a_hand_written_select`
* is the shell's guard against it.
* - The name-or-email `like` stays inside its own closure. Flattened beside
* the exclusion it would be an `OR` at the top level and an address match
* would smuggle a person already on the ticket back into the list — the
* operator-precedence hole the same shape opened on [central-inventory]
* #1594.
*
* AN ORDER AT ALL is new, and it is `PersonNames::orderUsers()` rather than
* `orderBy('name')`. The old query had no order, so ten rows arrived in
* whatever sequence the storage engine happened to return them and the same
* two letters could list the same people differently twice running. But the
* column that makes an order CONSISTENT is not automatically the one that
* makes it READABLE: `users.name` is the cached "First Last" ("Andrew Fast")
* and these rows are chips, which read "Last, First" ("Fast, Andrew"). A
* list labelled one way and collated the other is what an operator calls
* unsorted — "Toews, Fast, Ivanov, Mwangi, Bergman" is perfectly ordered by
* a string the row no longer shows. `PersonNames::orderUsers()` is the
* platform's one answer to that (#524), it is what the reference search
* (`admin.settings.users.search`) and the sibling conversions use, and it
* honours the operator's `name_format` rather than assuming surname-first.
*
* It also decides WHICH TEN come back, not merely their sequence, so this is
* not cosmetic: on a common surname the limit falls on a different set of
* people under the two collations.
*/
public function searchUsers(Request $request, Ticket $ticket)
{
if (! $this->isAgent($ticket->group_id)) {
@@ -803,16 +1178,16 @@ class TicketController extends Controller
->unique()
->values();
$users = DB::table('users')
->where(function ($query) use ($q) {
return response()->json(PersonChip::rows(
User::where(function ($query) use ($q) {
$query->where('name', 'like', "%{$q}%")
->orWhere('email', 'like', "%{$q}%");
})
->whereNotIn('id', $excludeIds)
->tap(fn ($query) => PersonNames::orderUsers($query))
->limit(10)
->get(['id', 'name', 'email']);
return response()->json($users);
->get(PersonChip::columns())
));
}
public function myTickets(): Response

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

@@ -3,12 +3,16 @@
namespace Dashboard\Ticketing\Http\Controllers;
use App\Models\Setting;
use App\Models\User;
use App\Support\PersonChip;
use App\Support\PersonNames;
use Dashboard\Ticketing\Models\EmailConnection;
use Dashboard\Ticketing\Models\PriorityLevel;
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;
@@ -30,8 +34,21 @@ class TicketingSettingsController extends Controller
*/
private function isSiteAdmin(): bool
{
$user = Auth::user();
return $user && ($user->is_super_admin || $user->can('ticketing.settings'));
return $this->isSiteAdminUser(Auth::user());
}
/**
* The same question asked about SOMEBODY ELSE — one definition, two callers.
*
* #4806 needs to know whether the manager a group already has could still
* use this page, which is isSiteAdmin()'s test applied to a user who is not
* signed in. Two copies of that rule would drift, and the drift would be
* invisible: the recovery path below opens exactly when this returns false
* for every manager of a group.
*/
private function isSiteAdminUser(?User $user): bool
{
return $user !== null && ($user->is_super_admin || $user->can('ticketing.settings'));
}
/**
@@ -53,15 +70,175 @@ class TicketingSettingsController extends Controller
}
}
/**
* Require manager rights over one group.
*
* Deliberately NOT widened to site admins: GroupRetirementTest pins that a
* super admin with no agent access is refused here, and #4710 is a
* visibility bug, not a licence to hand every `ticketing.settings` holder
* manager rights over every group. Seeing a group you do not manage is the
* fix; acting on it is a separate decision for a human to make.
*
* TWO writes do not come through here, and they are the same write in
* both directions. `storeAgent()` uses
* requireManagerAccessOrOrphanRecovery(), because a group with no manager
* left has nobody this gate could ever admit, and the group and its
* tickets are frozen for good (#4806); `destroyAgent()` uses
* requireRemoveAccess(), so that recovery is not one-way (#5002) — an
* appoint that could never be un-appointed froze the group again through
* its own cure. Every other write on somebody else's group still ends up
* here.
*/
private function requireManagerAccess(int $groupId): void
{
$isManager = TicketingAgentAccess::where('user_id', Auth::id())
if (!$this->isManagerOf($groupId)) {
abort(403);
}
$this->settleRescueStamp($groupId);
}
/**
* THE RESCUE STAMP ENDS WHEN THE APPOINTEE USES WHAT IT GAVE THEM — #5008.
*
* `recovered_by` (#5002) exists so a rescuer can take back an appointment
* they got wrong, including a `manager` press that shut the orphan arm
* behind it. It was written as an undo and it has to expire like one. As
* first built, the only thing that cleared it was the appointee re-saving
* their own row — a write nobody makes — so the stamp sat on the row for
* good, and a successful rescue left the rescuer a standing lever over a
* desk somebody else was now running: strip the manager, then "rescue" the
* group that press just orphaned, and a site admin who never ran it
* manages it (#5008). That is #4710's line, which the undo must not cross.
*
* So the stamp settles the moment the appointee ACTS as the manager they
* were made: staffs the group, renames it, archives it, gives it a project
* or an email connection — anything this file lets a manager do to their
* own group. The rescue has been taken up, the group is theirs, and the
* rescuer's claim on it is spent. Until then nothing has happened but the
* press itself, which is exactly the case the undo is for.
*
* Called from the gates and not from each action because the gates are
* where "this write was authorised by the actor's OWN manager row" is
* already decided; a read of the page never comes through them.
*/
private function settleRescueStamp(int $groupId): void
{
TicketingAgentAccess::where('user_id', Auth::id())
->where('group_id', $groupId)
->whereNotNull('recovered_by')
->update(['recovered_by' => null]);
}
/** Does the signed-in user hold a `manager` row on this group? */
private function isManagerOf(int $groupId): bool
{
return TicketingAgentAccess::where('user_id', Auth::id())
->where('group_id', $groupId)
->where('role', 'manager')
->exists();
if (!$isManager) {
abort(403);
}
/**
* Could this person still administer a help desk group today?
*
* NOT "do they hold a manager row" — the row is what we are judging. This
* asks whether, if they signed in right now, the Agents tab's own POST
* would work for them: the account has to still exist, still be active,
* and still hold the permission every route in this file sits behind.
*
* A manager row outlives all three. On dev the sole manager of all 15
* groups was a live row pointing at an account that had been rewritten
* into an admissions-only applicant (#4202/#4203) — it could not reach
* /app/ticketing at all, and the Agents tab was still telling site admins
* to go and ask them (#4806).
*/
private function canStillAdminister(?User $user): bool
{
if ($user === null || !($user->is_active ?? true)) {
return false;
}
return $this->isSiteAdminUser($user);
}
/**
* The groups nobody is left to run — every `manager` row on them points at
* somebody canStillAdminister() says no to (or there is no manager row at
* all).
*
* This is the ONLY condition under which a site admin may write an agent
* row for a group they do not manage. It is deliberately a property of the
* GROUP, not of the caller: an orphaned group cannot be rescued by anyone
* else, and a group with a working manager is still none of an admin's
* business (#4710).
*
* `$onlyGroupIds` narrows both queries for the single-group gate; `null`
* means "every group", which is what the page's prop needs.
*
* @param int[]|null $onlyGroupIds
* @return int[]
*/
private function orphanedGroupIds(?array $onlyGroupIds = null): array
{
$groupIds = TicketingGroup::when($onlyGroupIds !== null,
fn($q) => $q->whereIn('id', $onlyGroupIds))
->pluck('id');
if ($groupIds->isEmpty()) {
return [];
}
$managerRows = TicketingAgentAccess::where('role', 'manager')
->whereIn('group_id', $groupIds)
->get(['group_id', 'user_id']);
// One can() per distinct manager, not per row: the dev case is one
// person holding the manager row on all fifteen groups.
$managerIds = $managerRows->pluck('user_id')->unique();
$actingIds = $managerIds->isEmpty()
? collect()
: User::whereIn('id', $managerIds)->get()
->filter(fn($u) => $this->canStillAdminister($u))
->pluck('id');
$covered = $managerRows
->filter(fn($r) => $actingIds->contains($r->user_id))
->pluck('group_id')
->unique();
return $groupIds->reject(fn($id) => $covered->contains($id))->values()->all();
}
/**
* Manager rights over one group, OR a site admin rescuing an orphaned one.
*
* The second arm is #4806 and nothing wider: the moment a group's last
* usable manager leaves, is deactivated or is diverted elsewhere, every
* write in this file 403s for everybody — the group cannot be renamed,
* archived, given a project or given a single agent, and its tickets can
* never be assigned or closed. The documented answer on the Agents tab was
* "ask a manager of the group", which on such a group names nobody.
*
* So: a site admin may appoint somebody to a group that has no manager
* left. That one write un-orphans the group, and from the next request on
* the ordinary requireManagerAccess() rule governs it again — including
* for the admin, who gets no standing rights over it unless they appointed
* themselves.
*/
private function requireManagerAccessOrOrphanRecovery(int $groupId): void
{
if ($this->isManagerOf($groupId)) {
$this->settleRescueStamp($groupId);
return;
}
if ($this->isSiteAdmin() && $this->orphanedGroupIds([$groupId]) !== []) {
return;
}
abort(403);
}
public function index(?string $tab = null): Response
@@ -70,14 +247,38 @@ class TicketingSettingsController extends Controller
$userId = Auth::id();
$isBootstrap = $this->isBootstrapState();
$isSiteAdmin = $this->isSiteAdmin();
$myGroupIds = TicketingAgentAccess::where('user_id', $userId)->pluck('group_id');
// The groups this user may actually ACT on — requireManagerAccess() in
// prop form. A site admin now sees groups they do not manage (#4710),
// so the page has to say which rows its buttons will work on instead
// of offering an Edit that 403s.
$myManagedGroupIds = TicketingAgentAccess::where('user_id', $userId)
->where('role', 'manager')
->pluck('group_id');
// What this page is allowed to SHOW — separate from what it lets you
// DO, which stays requireManagerAccess() per group. A site admin sees
// every group: scoping the list to agent-access rows left an admin
// staring at "No groups yet. Create your first group above." with
// twelve groups in the database, inviting a duplicate (#4710).
// `null` means "no restriction".
//
// Every one of these routes sits behind `permission:ticketing.settings`
// and isSiteAdmin() is `is_super_admin || can('ticketing.settings')`,
// so today this is true for anyone who reaches the page at all. The
// scoped branch is kept because it is the correct behaviour if that
// route gate is ever loosened — it is not load-bearing right now.
$visibleGroupIds = $isSiteAdmin ? null : $myGroupIds;
// Archived groups stay in this list — this is the one place they must
// remain visible, so a manager can see what happened and restore it.
// Actives first, then archived, each alphabetical.
$groups = $isBootstrap
? collect()
: TicketingGroup::whereIn('id', $myGroupIds)
: TicketingGroup::when($visibleGroupIds !== null,
fn($q) => $q->whereIn('id', $visibleGroupIds))
// tickets_count decides archive-vs-delete, so the UI can warn
// the operator which one their click is about to do.
->withCount('tickets')
@@ -87,7 +288,10 @@ class TicketingSettingsController extends Controller
$agents = $isBootstrap
? collect()
: TicketingAgentAccess::whereIn('group_id', $myGroupIds)->with('group')->get();
: TicketingAgentAccess::when($visibleGroupIds !== null,
fn($q) => $q->whereIn('group_id', $visibleGroupIds))
->with('group')
->get();
if ($agents->isNotEmpty()) {
$agentUserIds = $agents->pluck('user_id')->unique();
@@ -95,19 +299,68 @@ class TicketingSettingsController extends Controller
$agents->each(fn($a) => $a->user = $agentUsers[$a->user_id] ?? null);
}
// #4806 — the groups with no manager left who could still use this
// page. Computed once: the page names them in the add form, and #5002
// asks the same question again for every Remove button below.
$orphanedGroupIds = $isSiteAdmin && !$isBootstrap ? $this->orphanedGroupIds() : [];
// #5002 — requireRemoveAccess() in prop form, the way myManagedGroupIds
// is requireManagerAccess()'s. It cannot be `canManageGroupId(row.group)`
// any more: a rescuer may also remove a row on a group that is still
// orphaned, and the row their own rescue wrote even after that rescue
// un-orphaned the group. A row an administrator can create has to be a
// row they can see the Remove on — the list said "Not yours to manage"
// over a row the reader had written seconds earlier.
$orphanedSet = array_map('intval', $orphanedGroupIds);
$removableAgentIds = $agents
->filter(fn ($a) => $myManagedGroupIds->contains($a->group_id)
|| ($isSiteAdmin && (in_array((int) $a->group_id, $orphanedSet, true)
|| ($a->recovered_by !== null && (int) $a->recovered_by === (int) $userId))))
->pluck('id')
->values()
->all();
$priorities = $isBootstrap
? collect()
: PriorityLevel::orderBy('sort_order')->get();
$projects = $isBootstrap
? collect()
: TicketingProject::whereIn('group_id', $myGroupIds)
: TicketingProject::when($visibleGroupIds !== null,
fn($q) => $q->whereIn('group_id', $visibleGroupIds))
->orderBy('name')
->get();
$emailConnections = $isBootstrap
// #6168 — the Email tab's rows. Shaped by hand rather than handed over
// as models for one reason: `config` holds the mailbox PASSWORD, and
// this prop ships on every settings load, so a credential left in it is
// a credential in the page source of everyone who can open Help Desk
// Settings. The form asks for a new password instead and
// updateEmailConnection() keeps the stored one when the box is blank,
// so nothing needs it on the client — `has_password` is all the UI has
// to know to say "a password is set".
$emailConnections = ($isBootstrap
? collect()
: EmailConnection::whereIn('group_id', $myGroupIds)->get();
: EmailConnection::when($visibleGroupIds !== null,
fn($q) => $q->whereIn('group_id', $visibleGroupIds))
->orderBy('id')
->get())
->map(function (EmailConnection $connection) {
$config = $connection->config ?? [];
$hasPassword = ($config['password'] ?? '') !== '';
unset($config['password']);
return [
'id' => $connection->id,
'group_id' => $connection->group_id,
'type' => $connection->type,
'active' => (bool) $connection->active,
'config' => $config,
'has_password' => $hasPassword,
'last_polled_at' => $connection->last_polled_at?->toIso8601String(),
];
})
->values();
return Inertia::render('Ticketing/Settings', [
'activeTab' => $tab,
@@ -118,9 +371,24 @@ class TicketingSettingsController extends Controller
'ticketStatuses' => TicketStatus::orderBy('sort_order')->get(),
'emailConnections' => $emailConnections,
'myGroupIds' => $myGroupIds,
'myManagedGroupIds' => $myManagedGroupIds,
// #4806 — groups with no manager left who could still use this
// page. A site admin may add an agent to these and nothing else,
// so the Agents tab can offer the recovery instead of printing
// "ask a manager of the group" about a group that has none.
// Empty for a non-admin: the recovery is not theirs to make.
'orphanedGroupIds' => $orphanedGroupIds,
// #5002 — the access rows this user may actually remove. Superset
// of "rows on a group I manage": see requireRemoveAccess().
'removableAgentIds' => $removableAgentIds,
'isBootstrap' => $isBootstrap,
'isSiteAdmin' => $this->isSiteAdmin(),
'isSiteAdmin' => $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(),
]);
}
@@ -287,6 +555,71 @@ class TicketingSettingsController extends Controller
return $request->expectsJson();
}
/**
* WHO AM I APPOINTING — the Agents tab's people search ([central-ticketing]
* #4926).
*
* Until this existed the first field of the add-an-agent form was
* `<input type="number">` labelled "User ID", with "Find IDs in Admin →
* Users" under it. That is a primary key asked of an office administrator,
* and on the ONE form the product calls "the only recovery" for a group
* with no manager left (#4806): the workflow was to leave the page, search
* a name somewhere else, read a number off a URL and type it back in.
*
* The cost is not only the detour. `storeAgent()` validates
* `exists:users,id`, so a NON-existent id is refused cleanly — but 47 typed
* for 74 is two real accounts, and the one that gets written is handed
* `manager` over a whole help desk queue: every ticket in it, plus the
* power to rename, archive and staff it. Nothing on that screen named the
* person before the press or after it, so #4781's audit row was the first
* place anybody could have found out, after the fact.
*
* So this is the same search every other Central picker is: `q` over name
* and email, answered by `PersonChip::rows()` — the one serializer for a
* searchable person (#871) — so the operator's People setting reaches this
* picker like the rest, and the page draws `<PersonChip>` rather than a
* hand-written name row.
*
* ── THE GATE IS THE PAGE'S, NOT THE WRITE'S ─────────────────────────────
*
* requireAgentAccess(), the same one index() uses, and deliberately not a
* per-group one: the form picks the PERSON before the GROUP, so there is no
* group to scope to yet, and narrowing it afterwards would mean a search
* that empties when the picker's second field changes. Nothing is widened
* by that — the write itself still goes through
* requireManagerAccessOrOrphanRecovery(), so a person found here can only
* be appointed to a group the caller manages or is rescuing. Reaching this
* route at all already means `permission:ticketing.settings`, which is a
* narrower population than TicketController::searchUsers() (any agent on
* any one ticket's group) already answers the same question for.
*/
public function searchUsers(Request $request)
{
$this->requireAgentAccess();
$q = trim((string) $request->input('q', ''));
// Two characters, like every other picker on the platform: one letter
// is most of the directory, and the dropdown is ten rows. Counted in
// CHARACTERS, like admin.settings.users.search, not bytes: an accented
// letter is two bytes and a CJK surname is three, so a byte floor lets
// a single keystroke answer with the directory on exactly the names a
// school has the most trouble typing.
if (mb_strlen($q) < 2) {
return response()->json([]);
}
return response()->json(PersonChip::rows(
User::where(function ($query) use ($q) {
$query->where('name', 'like', "%{$q}%")
->orWhere('email', 'like', "%{$q}%");
})
->tap(fn ($query) => PersonNames::orderUsers($query))
->limit(10)
->get(PersonChip::columns())
));
}
public function storeAgent(Request $request)
{
$this->requireAgentAccess();
@@ -299,25 +632,152 @@ class TicketingSettingsController extends Controller
'role' => 'required|in:agent,manager',
]);
$this->requireManagerAccess($validated['group_id']);
// #4806 — the one way back into a group whose last manager can no
// longer act. Asked BEFORE the write, because the write is what ends
// the condition: appointing yourself makes isManagerOf() true.
$isRecovery = !$this->isManagerOf($validated['group_id'])
&& $this->orphanedGroupIds([$validated['group_id']]) !== [];
$this->requireManagerAccessOrOrphanRecovery($validated['group_id']);
TicketingAgentAccess::updateOrCreate(
['user_id' => $validated['user_id'], 'group_id' => $validated['group_id']],
['role' => $validated['role']]
[
'role' => $validated['role'],
// WHO RESCUED THIS GROUP — #5002. A recovery write is the one
// write here that can lock its own author out: appoint a
// `manager` and the group is no longer orphaned, so the arm
// that allowed the press is shut by the press. Stamping the
// row is what lets destroyAgent() hand it back.
//
// Set to null on an ordinary manager's write, including a
// manager editing a row an earlier rescue wrote: once the
// group has somebody of its own touching the row, it is
// theirs and the rescuer has no further claim on it.
'recovered_by' => $isRecovery ? Auth::id() : null,
]
);
/*
* NAME THE PERSON — [central-ticketing] #4926.
*
* The flash already went to the trouble of naming the GROUP and saying
* it had nobody; naming the appointee is the other half of the same
* sentence, and it is the half somebody can check. This write hands a
* person read access to every ticket in a queue — and, as a manager,
* the power to rename, archive and staff it — so "Agent added." left
* the only record of WHO in #4781's audit log, which is somewhere you
* go after you already suspect something.
*
* `PersonNames::display()` and not `$user->name`: the confirmation has
* to read the way the rest of the platform writes a name (#524), and
* the chip the admin picked from in the form is composed the same way,
* so the two agree. A user who vanished between the `exists` rule and
* this line leaves `null` and the sentence falls back to exactly what
* it printed before — a flash is not where a race should surface.
*/
$person = User::find($validated['user_id']);
$who = $person === null ? null : PersonNames::display($person);
$asWhat = $validated['role'] === 'manager' ? 'a manager' : 'an agent';
$name = TicketingGroup::find($validated['group_id'])?->name;
// Say which group was rescued and that it had nobody: a recovery is a
// rare, consequential click and "Agent added." does not describe it.
if ($isRecovery) {
$added = $who === null ? 'Agent added' : "{$who} added as {$asWhat}";
return back()->with('success', $name === null
? "{$added} — that group had no manager who could still act."
: "{$added} to “{$name}”, which had no manager who could still act.");
}
if ($who === null) {
return back()->with('success', 'Agent added.');
}
return back()->with('success', $name === null
? "{$who} added as {$asWhat}."
: "{$who} added to “{$name}” as {$asWhat}.");
}
/**
* REMOVE IS THE SAME DOOR AS ADD — [central-ticketing] #5002.
*
* This was `requireManagerAccess()` while `storeAgent()` was
* requireManagerAccessOrOrphanRecovery(), so the recovery was ONE-WAY: the
* site admin who rescued a group could appoint and never un-appoint. The
* row they had just written appeared in their own Agents list marked "Not
* yours to manage", with no Remove control, and the DELETE answered 403 —
* the same person, the same second, the same row.
*
* Worse for a `manager` press, which is the one the card about the people
* search (#4926) called out as the expensive mistake: the write ends the
* orphan state, so the admin could not remove the row, could not appoint
* anyone else, and could not appoint themselves. The only person left who
* could undo it was the person wrongly appointed. That is #4806's frozen
* group again, reached through #4806's own cure.
*
* So two arms beyond an ordinary manager, both narrower than the write
* they undo:
*/
public function destroyAgent(TicketingAgentAccess $access)
{
$this->requireManagerAccess($access->group_id);
$this->requireRemoveAccess($access);
$access->delete();
return back()->with('success', 'Agent removed.');
}
/**
* May the signed-in user take this agent access row away?
*
* 1. A manager of the group — the ordinary rule, unchanged.
* 2. A site admin, while the group is STILL orphaned. Exactly symmetric
* with storeAgent()'s arm and strictly narrower than it: on a group
* with no manager left, an admin can already appoint themselves
* manager and then remove anything, so this grants nothing new — it
* just does not make them take the group over first to clear a
* dangling row.
* 3. A site admin undoing a row THEY wrote as a rescue (`recovered_by`).
* This is the one that reopens the door a mistaken `manager` press
* closed, and it is deliberately not "the group would be orphaned
* without this row": that test cannot tell a mistaken appointment
* from the sole working manager of a healthy group, and admitting the
* second would let any site admin strip a manager from a group that
* is none of their business, then appoint themselves (#4710). A
* rescuer taking back their own appointment claims nothing they did
* not already hold at the moment they made it.
*
* And it lasts only until the appointee USES the access — see
* settleRescueStamp(). Otherwise the undo outlives the mistake it is
* for: a rescue that WORKED leaves the stamp on the row of the person
* now running the desk, and arm 3 becomes a permanent lever for
* stripping them and taking the group over (#5008).
*
* Removing the last manager re-orphans the group, which is the correct
* end state: it is recoverable again, by whoever gets there next.
*/
private function requireRemoveAccess(TicketingAgentAccess $access): void
{
if ($this->isManagerOf($access->group_id)) {
$this->settleRescueStamp($access->group_id);
return;
}
if ($this->isSiteAdmin()) {
if ($this->orphanedGroupIds([$access->group_id]) !== []) {
return;
}
if ($access->recovered_by !== null && (int) $access->recovered_by === (int) Auth::id()) {
return;
}
}
abort(403);
}
public function storePriority(Request $request)
{
if (!$this->isSiteAdmin()) {
@@ -328,7 +788,10 @@ class TicketingSettingsController extends Controller
'name' => 'required|string|max:100',
'color' => 'required|string|regex:/^#[0-9a-fA-F]{6}$/',
'description' => 'nullable|string',
'sort_order' => 'integer|min:0',
// unsignedSmallInteger column (0..65535): without the ceiling an
// oversized client value overflows on INSERT as an unhandled 500.
// Same rule #6081 put on storeStatus()/updateStatus().
'sort_order' => 'integer|min:0|max:65535',
]);
PriorityLevel::create($validated);
@@ -346,7 +809,7 @@ class TicketingSettingsController extends Controller
'name' => 'required|string|max:100',
'color' => 'required|string|regex:/^#[0-9a-fA-F]{6}$/',
'description' => 'nullable|string',
'sort_order' => 'required|integer|min:0',
'sort_order' => 'required|integer|min:0|max:65535',
]);
$priority->update($validated);
@@ -475,12 +938,43 @@ class TicketingSettingsController extends Controller
$connection->update([
'type' => $request->type,
'active' => $request->boolean('active', true),
'config' => $request->input('config', $connection->config),
'config' => $this->mergedConnectionConfig($request, $connection),
]);
return back()->with('success', 'Email connection updated.');
}
/**
* The config to store for an edited connection.
*
* #6168 — the page is never sent the stored password (see index()), so the
* edit form's password box arrives EMPTY on every save that does not change
* it. Taking the request's config as-is would therefore erase the mailbox
* credential every time somebody renamed a host or flipped the Active
* checkbox. A blank box means "keep the one you have"; typing a new one
* replaces it, and the rest of the config is the request's, so a field
* cleared on purpose still clears.
*/
private function mergedConnectionConfig(Request $request, EmailConnection $connection): array
{
$config = $request->input('config');
$existing = $connection->config ?? [];
if (! is_array($config)) {
return $existing;
}
if (($config['password'] ?? '') === '') {
unset($config['password']);
if (($existing['password'] ?? null) !== null) {
$config['password'] = $existing['password'];
}
}
return $config;
}
public function destroyEmailConnection(EmailConnection $connection)
{
$this->requireAgentAccess();
@@ -498,7 +992,7 @@ class TicketingSettingsController extends Controller
$validated = $request->validate([
'name' => 'required|string|max:100',
'color' => 'required|string|regex:/^#[0-9a-fA-F]{6}$/',
'sort_order' => 'required|integer|min:0',
'sort_order' => 'required|integer|min:0|max:65535',
'is_closed' => 'boolean',
]);
@@ -529,7 +1023,7 @@ class TicketingSettingsController extends Controller
$validated = $request->validate([
'name' => 'required|string|max:100',
'color' => 'required|string|regex:/^#[0-9a-fA-F]{6}$/',
'sort_order' => 'required|integer|min:0',
'sort_order' => 'required|integer|min:0|max:65535',
'is_closed' => 'boolean',
]);
@@ -575,10 +1069,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,39 @@
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';
/**
* 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
* 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 +54,23 @@ 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.
*
* #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', 'merged_stranded'];
protected $casts = [
'due_date' => 'date:Y-m-d',
];
@@ -70,4 +114,138 @@ 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();
}
/**
* 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() || $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();
}
}

View File

@@ -9,7 +9,13 @@ class TicketingAgentAccess extends Model
{
protected $table = 'ticketing_agent_access';
protected $fillable = ['user_id', 'group_id', 'role'];
/**
* `recovered_by` is #5002: the site admin who wrote this row as an orphan
* recovery (#4806), and may therefore take it back. Null on every row
* written by an ordinary manager — see the migration for why this is not
* a general-purpose `created_by`.
*/
protected $fillable = ['user_id', 'group_id', 'role', 'recovered_by'];
public function group(): BelongsTo
{

View File

@@ -0,0 +1,138 @@
<?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
{
$tickets = Ticket::query()->inconsistentlyMerged()->orderBy('id')->get();
if ($tickets->isEmpty()) {
Log::info("[ticketing] #938 merged-ticket repair ({$by}): nothing to repair.");
return 0;
}
// Per model, not one builder update (#4781/#4807): `merged` makes a ticket
// permanently read-only, and a bulk statement would freeze it with nothing
// on the Changes log — while repair() below, the same change one ticket at
// a time, is recorded.
$tickets->each(fn (Ticket $ticket) => $ticket->forceFill(['status' => Ticket::STATUS_MERGED])->save());
$numbers = $tickets->pluck('number')->all();
$changed = $tickets->count();
// 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

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

View File

@@ -2,6 +2,7 @@
namespace Dashboard\Ticketing;
use Illuminate\Console\Scheduling\Schedule;
use Illuminate\Support\ServiceProvider;
class TicketingServiceProvider extends ServiceProvider
@@ -23,10 +24,200 @@ class TicketingServiceProvider extends ServiceProvider
// Notifications is absent or dark — the notifier no-ops.
\Dashboard\Ticketing\Support\TicketingNotifier::registerCategories();
// Declare this app's models to the shell's audit trail (#4781), so a
// ticket, a group retirement and above all an AGENT GRANT are on the
// Changes log. Guarded inside — central-logs is optional on a host.
$this->registerAuditedModels();
if ($this->app->runningInConsole()) {
$this->commands([
\Dashboard\Ticketing\Console\Commands\AutoCloseTickets::class,
\Dashboard\Ticketing\Console\Commands\RepairMergedTickets::class,
]);
}
$this->scheduleAutoClose();
}
/**
* THE NIGHTLY AUTO-CLOSE IS SCHEDULED HERE, BESIDE THE COMMAND IT NAMES
* ([central-ticketing] #2980, a spawn of [dashboard-shell] #2965).
*
* It used to be one line in the shell's routes/console.php — a file that
* names commands other repositories define. On 2026-09-26 01:10 PDT that
* arrangement cost production a php process a day whose entire output was
* *Command "admissions:remind-unconfirmed-links" is not defined*, exit 1: a
* schedule had reached a box nine and a half hours ahead of the code it
* scheduled, and nothing an operator could press would have fixed it,
* because the code was not there to fix.
*
* Registered from this provider, that is not expressible. The event exists
* on exactly the boxes where `AutoCloseTickets` is registered above,
* because the same `boot()` does both and both arrive in the same commit.
* A shell without central-ticketing simply has no such row in
* `schedule:list`, rather than a nightly failure about one.
*
* ── THE MINUTE IS STILL THE SHELL'S TO DECIDE ──────────────────────────
*
* `ScheduleStagger` is the one place a start time is chosen, and that is
* the whole point of it: a bare `->daily()` is 00:00:00, where every hourly
* and every quarter-hourly task on the platform already boots its own PHP
* inside the scheduler's one cgroup ([dashboard-shell] #1910). A time typed
* in this repository could not know that. Asking `dailyAt()` keeps this job
* at 00:12 — exactly where routes/console.php had it — and keeps the shell's
* map the single account of the night even for a job it no longer schedules.
*
* No `withoutOverlapping()`, and deliberately not added here: the shell's
* line carried none, and a lock is a behaviour change rather than a move.
*/
private function scheduleAutoClose(): void
{
$this->callAfterResolving(Schedule::class, function (Schedule $schedule) {
$schedule->command('ticketing:auto-close')
// …AND THE CLOCK IS THE SCHOOL'S ([dashboard-shell] #3144).
// ScheduleStagger decides the MINUTE; `dailyAt()` on its own is
// read against config('app.timezone'), which is UTC on every box
// here — so 00:12 was landing at 17:12 the previous afternoon,
// i.e. the nightly sweep ran in the middle of the school day.
->timezone(\App\Support\Timezone::organisation())
->dailyAt(\App\Support\Ops\ScheduleStagger::dailyAt('ticketing:auto-close'));
});
}
/**
* HELP DESK WRITES TO THE SHELL'S CHANGES LOG — [central-ticketing] #4781.
*
* Until this method existed, `ticketing` was not one of the apps
* `audit_events` had ever seen: 4,378 rows, fifteen distinct apps, not one
* of them this one. A QA sweep signed in as a super admin, created a ticket,
* replied to it, and found that the newest row in the table afterwards was
* the shell recording the sweep's own visit to the log page. So "who closed
* this", "who deleted that group" and — the one that actually matters —
* "who gave this person the keys to the HR desk's queue" had no answer
* anywhere in Central.
*
* Nothing in core was wrong. `AuditRegistry`'s own docblock is explicit that
* snap-ins register their own models from their own providers and that core
* never references a snap-in. Ticketing simply never declared. This is the
* declaration, in the shape `FormsServiceProvider::registerAuditedGrants()`
* uses for `FormCollaborator` and `FormSharedGroup` — the precedent is exact
* and so is the reason: granting somebody access to other people's
* correspondence is a permission change, and a permission change belongs on
* the record.
*
* Declaring a model is the whole job. The registry attaches `AuditObserver`
* itself, so every create / declared-field update / delete on these four
* classes lands in the Changes tab under app `ticketing` (derived from the
* namespace by `Audit::appForClass()`), with the actor and the subject's
* label frozen into the row at write time.
*
* ── THE FIELD LISTS ARE THE VOLUME CONTROL ───────────────────────────────
*
* Only declared fields are diffed and an update touching none of them
* records nothing at all, so these lists — not the tables' width — decide
* what the log costs. Each is the set of columns somebody would ask an
* auditor about: where a ticket lives, who is on the hook for it, what state
* it is in, and who may see it. `color` on a group and `description` on a
* ticket are left out deliberately: a palette tweak is not an audit event,
* and the body of a complaint is correspondence we are not copying into a
* second table to be kept for seven years.
*
* ── NOTHING HERE IS `sensitive`, AND THAT IS A DECISION ──────────────────
*
* `sensitive: true` buys exactly one thing: read-audit. It makes
* `RecordSensitiveViews` record a `viewed` row every time a route implicitly
* binds the model on a successful GET. For `Ticket` that is every agent
* opening every ticket all day — volume that would bury the writes this card
* is about, to answer a question nobody asked. It changes no retention
* window either (writes are kept on the operator's Changes window
* regardless; `sensitive` only lengthens what READ rows get). So: writes,
* declared; reads, not. If ticketing ever needs "who read this ticket", that
* is its own card and its own conversation about volume.
*
* ── WHAT IS DELIBERATELY NOT DECLARED ────────────────────────────────────
*
* `TicketMessage`. The reporter expected a row for the reply they posted,
* and it is the right instinct on the wrong model: a message can only ever
* be created (there is no edit route and no delete route — see
* `TicketMessageController`, which has one method), so auditing it could
* only ever mean one row per message. Every inbound email through an
* `EmailConnection` writes one, under `system`, and a merge rewrites whole
* runs of them. That is a stream of "a message exists" against the record of
* who CHANGED something, which is what the Changes tab is for — and the
* messages themselves are already the permanent, readable record of what was
* said. The ticket's own row says who opened it and who closed it; the
* thread says what was written in between.
*
* A group hard-delete cascades its agent-access rows in the DATABASE (see
* the migration's foreign key), so those grants end without a `deleted` row
* of their own. The group's deletion is recorded, which is the question
* being asked — but it is worth knowing that the two are one event here and
* not several.
*
* Guarded on the class and wrapped in a catch for the same reason Forms and
* Files are: central-logs is optional on any given host, and a declaration
* must never be the reason a box fails to boot.
*/
private function registerAuditedModels(): void
{
if (! class_exists(\Central\Logs\Support\AuditRegistry::class)) {
return;
}
try {
/*
* THE GRANT. The reason this card was filed and the first thing
* declared: `ticketing.settings.agents.store` / `.destroy` hand out
* and take back a person's access to a help desk group's entire
* queue, and did so leaving no trace at all.
*
* The label is resolved the long way round because this model has no
* `user()` relation and must not grow one — `TicketingSettingsController`
* HYDRATES `$access->user` as a plain attribute for the Settings page,
* and a relation of the same name would fight it.
*/
\Central\Logs\Support\AuditRegistry::audit(
\Dashboard\Ticketing\Models\TicketingAgentAccess::class,
fields: ['user_id', 'group_id', 'role'],
label: fn ($access) => (\App\Models\User::find($access->user_id)?->name ?? 'Deleted user')
.' on '.($access->group?->name ?? 'a help desk group'),
);
// The desk itself: renamed, re-prefixed, pointed at a different
// inbox, archived, restored, or deleted outright. `archived_at` is
// what tells archive from delete — one button does both, chosen by
// whether the group still owns tickets.
\Central\Logs\Support\AuditRegistry::audit(
\Dashboard\Ticketing\Models\TicketingGroup::class,
fields: ['name', 'prefix', 'email_address', 'archived_at'],
label: fn ($group) => ($group->name ?: 'Help desk group')
.($group->prefix ? ' ('.$group->prefix.')' : ''),
);
// The ticket: created, deleted, moved between desks, re-assigned,
// re-prioritised, closed, merged. `merged_into_id` is declared
// beside `status` because a merge is a pair of columns moving
// together and reading either one alone misreports it.
\Central\Logs\Support\AuditRegistry::audit(
\Dashboard\Ticketing\Models\Ticket::class,
fields: [
'group_id', 'status', 'assigned_to', 'priority_id',
'project_id', 'due_date', 'merged_into_id', 'title',
],
label: fn ($ticket) => trim(($ticket->number ?: 'Ticket').' '.($ticket->title ?? '')),
);
// Participants are the OTHER way somebody comes to see a ticket's
// correspondence — narrower than an agent grant (one ticket, not a
// whole queue) but the same kind of fact.
\Central\Logs\Support\AuditRegistry::audit(
\Dashboard\Ticketing\Models\TicketParticipant::class,
fields: ['ticket_id', 'user_id', 'added_by'],
label: fn ($participant) => (\App\Models\User::find($participant->user_id)?->name ?? 'Deleted user')
.' on ticket '.($participant->ticket?->number ?? '#'.$participant->ticket_id),
);
} catch (\Throwable) {
// Never let a declaration break boot.
}
}
}

View File

@@ -41,12 +41,25 @@ Route::middleware(['web', 'auth', 'app.access:ticketing'])
// ── Settings (must be before /{ticket} wildcard) ──────────────────────
Route::middleware('permission:ticketing.settings')->group(function () {
Route::get('/settings', [TicketingSettingsController::class, 'index']) ->name('settings');
// #6168 — `email` belongs in this list. It was the one tab segment
// the whitelist omitted while the page shipped an `emailConnections`
// prop on every render and the three write routes below were live, so
// the only deep link an admin could guess fell through to the POST-only
// `/settings/email` and answered 405. The Vue's TAB_KEYS and this list
// are now pinned to each other by
// tests/Unit/TicketingSettingsTabsAreOneVocabularyTest.php.
Route::get('/settings/{tab}', [TicketingSettingsController::class, 'index'])
->where('tab', 'groups|agents|priorities|statuses|projects|automation')->name('settings.tab');
->where('tab', 'groups|agents|priorities|statuses|projects|email|automation')->name('settings.tab');
Route::post('/settings/groups', [TicketingSettingsController::class, 'storeGroup']) ->name('settings.groups.store');
Route::put('/settings/groups/{group}', [TicketingSettingsController::class, 'updateGroup']) ->name('settings.groups.update');
Route::delete('/settings/groups/{group}', [TicketingSettingsController::class, 'destroyGroup']) ->name('settings.groups.destroy');
Route::post('/settings/groups/{group}/restore', [TicketingSettingsController::class, 'restoreGroup']) ->name('settings.groups.restore');
// #4926 — the add-an-agent form's people search. Before it the form's
// first field was a raw numeric user id with "Find IDs in Admin → Users"
// under it, on the one screen that is the only recovery for a help desk
// group with no manager left (#4806). Two segments, so it cannot collide
// with the single-segment `/settings/{tab}` above.
Route::get('/settings/agents/search', [TicketingSettingsController::class, 'searchUsers']) ->name('settings.agents.search');
Route::post('/settings/agents', [TicketingSettingsController::class, 'storeAgent']) ->name('settings.agents.store');
Route::delete('/settings/agents/{access}', [TicketingSettingsController::class, 'destroyAgent']) ->name('settings.agents.destroy');
Route::post('/settings/priorities', [TicketingSettingsController::class, 'storePriority']) ->name('settings.priorities.store');
@@ -76,6 +89,10 @@ 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');
// #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');