Small Fixes 5 1 0 - edmozley/freeitsm GitHub Wiki

Small fixes in 5.1.0

Found while clearing the October code-commenting list and the 5.0.0 agent findings · Fixed in: 5.1.0 (changelog #2827, #2829–#2838)

Nine small faults, each fixed and pinned by a test. None of them loses data you can see today; most of them made something quietly not happen.

	What you saw	Module
1	A closed ticket's resolution clock kept running	SLA
2	Renaming a War Room channel to nothing looked like it saved	War Room
3	Calendar feeds could break an accented letter or emoji	Calendar
4	A SCORM 1.2 course got the wrong error for a read-only value	LMS
5	A form's actions did not save what the editor showed	Forms
6	The REST API did not say what a choice could be	REST API, Workflows
7	Undo merge lost a watcher that was on both tickets	Tickets
8	Tidying: a second drag handler on the Tasks board	Tasks
9	Routing by category: two gaps closed	Workflows

1. A closed ticket's resolution clock kept running

What you saw. A ticket that reached a closed status without a closed date — an imported ticket, or one closed by a path that set the status alone — showed its resolution SLA still counting, and in time breached.

Why. sla_compute_resolution() stops the clock at tickets.closed_datetime, and when that is empty it was meant to fall back to the first status change into a closed status. The fallback looked for _is_closed on each step of the status timeline, but nothing ever set it — so the fallback never fired.

The fix. sla_get_state() now records _is_closed on every step. The fallback walks back from the latest step and stops the clock where the ticket's current run of closed statuses began:

for ($i = count($timeline) - 1; $i >= 0 && !empty($timeline[$i]['_is_closed']); $i--) {
    $closedAt = $timeline[$i]['start'];
}

🔑 Not "the first close". Reopening a ticket clears closed_datetime, so taking the first close in the history would have stopped a reopened ticket's clock at its old close. The test runs on the old code too: with _is_closed supplied, "first close" passes the simple case and fails both reopen cases.

2. Renaming a War Room channel to nothing looked like it saved

What you saw. Clearing a channel's name in Manage channel and pressing Save closed the dialog as if it had saved; the old name stayed.

Why. warRoomUpdateChannel() returned false for a blank name, and returned rowCount() >= 0 — always true — otherwise. The endpoint ignored the answer either way and replied success: true.

The fix. A blank name now throws InvalidArgumentException('name_required'), the same key creating a channel uses, so the page says a name is required. A save that changes nothing (row count 0) is still a success.

3. Calendar feeds could break an accented letter or emoji

What you saw. In a subscribed calendar (the .ics feeds), a long event title or description could show two broken characters where an accented letter or an emoji should be.

Why. RFC 5545 folds lines at 75 bytes. icsFold() cut with substr(), which can land in the middle of a multi-byte character and split it across the fold.

The fix. mb_strcut(), which backs off to the character boundary, so every folded line is whole UTF-8 and still at most 75 bytes. The test folds accented and emoji text at four offsets; the old code fails three of them.

4. A SCORM 1.2 course got the wrong error for a read-only value

What you saw. Nothing on screen — but a SCORM 1.2 course that tried to change a read-only value (its learner's name, say) was told error 404, "Element Is Write Only". Courses that react to errors could misbehave.

Why. The two standards number it differently: read-only is 403 in SCORM 1.2 and 404 in SCORM 2004. One setter served both APIs and always answered 404.

The fix. window.API (1.2) passes '1.2' and window.API_1484_11 (2004) passes '2004' to the shared setter and error-text lookup. A 1.2 course gets 403; a 2004 course keeps 404, with 2004's own wording, Data Model Element Is Read Only.

5. A form's actions did not save what the editor showed

What you saw. In Forms → a form → Actions:

a new Add a note to the ticket action showed History only rather than A note on the ticket — the starting value Workflows has used since GH #268;
a box showing Ticket ID {{ticket.id}} saved nothing at all, so the action failed at run time with ticket_id is required unless somebody had typed over it;
Share the note with the requester was a text box holding the word false.

Why. The editor added actions with empty args and only displayed each arg's default — and the engine never fills a missing arg in at run time. It also knew the control types checkbox and number, while the engine's specs say bool and numeric.

The fix — the same rules as the Workflows canvas:

A new action (added, or its type changed) starts from its spec: new_default first, then default (faNewArgs()).
An action already saved gets any missing default stored when it is drawn, so the next save keeps what is on screen. Never new_default: a saved Add a note with no write_to stays History only, as GH #268 promised.
bool is a tick box storing a real true/false (what argBool() reads); numeric is a number box.

Verified in the real editor on a test install: a new note action stores {ticket_id: "{{ticket.id}}", write_to: "note", shared: false}, the tick box stores true, and an old action gains ticket_id and shared but not write_to.

6. The REST API did not say what a choice could be

What you saw. GET /workflow-actions listed each arg's type and default, but for a select arg not its choices — so a client could not know what to send for, say, write_to — and not new_default.

The fix. Each arg now also carries options ([{value, label}], for a select), new_default and show_when, and the OpenAPI schema (WorkflowActionArg) describes them. Nothing was removed. Send every arg you mean: the engine does not fill a missing one in.

7. Undo merge lost a watcher that was on both tickets

What you saw. Two tickets both watched by the same analyst were merged, then Undo merge — and the ticket that came back no longer had that watcher. The same happened to a CMDB object, asset, problem, change or form attached to both tickets.

Why. A merge de-duplicates those links: if both tickets have the same one, the merged-away ticket's copy is deleted rather than moved. Undo moves back what the merge recorded as moved — and a deleted row was not recorded anywhere.

The fix. The merge now keeps the whole deleted rows in ticket_merges.moved_related under _dropped, and unmergeTicket() puts them back on the returning ticket (INSERT IGNORE, so a link somebody has re-added since is not duplicated). The surviving ticket keeps its own.

⚠️ Merges made before 5.1.0 have no such record. Undo still works for them as before; a link that was on both tickets just cannot be brought back.

8. Tidying: a second drag handler on the Tasks board

assets/js/tasks.js declared endDrag() twice. The second declaration always replaced the first, so the first never ran; it is gone. Nothing changes on the board.

9. Routing by category: two gaps closed

Follow-ups to routing by category (#298), both listed there as known gaps:

Changing a ticket's type cleared its category without telling anyone. A category tied to the old type is cleared when the type changes, but A ticket's category changes did not fire, so a workflow could not catch a ticket leaving a category. It now fires, with the old category and no new one.
Old category and New category were number boxes in a condition, so you had to know a category's id. They are now category pick-lists with full paths, follow the Categories in workflows subcategory setting like Ticket · Category, and give {{old_category_name}} / {{new_category_name}}.

Files: includes/services/tickets.php, workflow/includes/engine.php (CATEGORY_FIELDS), assets/js/workflow-editor.js (?v=26), the help in six languages. Test: tests/workflow-category-routing.php, now 24 checks; against 5.0.0 the five new ones fail.

Checked and left alone

Workflow Test fire sent empty data when a workflow had conditions — already fixed in 4.0.1.
Verify SSL ignored by the AI test buttons — by design since #49: one install-wide switch, SSL_VERIFY_PEER.
The Microsoft calendar subscription address was not URL-encoded — it is a value in the request body, not part of a URL.
checklist_template_items.default_value wiped on save — the column has never been read or written by any code, so there is nothing to wipe; it now carries a comment saying so.

📁 Files changed

	File	Fix
⏱️	`includes/sla.php`	1
💬	`includes/warroom.php`	2
📅	`includes/ics.php`	3
🎓	`assets/js/scorm-api.js`, `lms/player.php`, `self-service/course.php`	4
📝	`forms/edit/index.php`	5
🔌	`api/v1/resources/workflows.php`, `api/v1/lib/openapi_schemas.php`	6
🔀	`includes/ticket_merge.php`	7
✅	`assets/js/tasks.js`, `tasks/index.php`	8

Tested

tests/bug-fixes-5-1.php — 1 to 4 and 8, 22 checks, no database. Run against 5.0.0 it fails.
tests/ticket-merge-undo.php — 7, on a throwaway database: a watcher on both tickets, one on each. 5 checks; with the old ticket_merge.php two fail.
5 was driven in the real editor on a test install (above); 6 is a straight addition to the response.