Checklists Closure Gating Review

Checklists — closure gating: what changed and why

The second round of the same exercise as Checklists module — house style: what changed between the feat/template-level-checklist-gating branch as written and what was committed, and the reasoning for each change.

Written mostly for Santhosh Srinivasan (Sandy), who designed and built it, and for anyone else contributing to FreeITSM later.

Source: discussion #138 · shipped as changelog #1874–#1881.

For how the module works, see Checklists & SOPs — developer guide.

---

Before anything else

The design is right, and the argument for it was made before a line was written. Three things are worth saying plainly:

• The problem was found in use, not in theory. "Optional onboarding tips" should never trap an urgent ticket; "offboarding access revocation" should never be waved through. One install-wide switch cannot express both, and no amount of care in choosing its value fixes that. The fix is to move the decision to the person who wrote the procedure, and that is what this does.
• It went in at the right layer. The gate is resolved in ChecklistsService and enforced from assertClosureAllowed(), which TicketsService::updateTicket() already calls before the write. Nothing in the inbox, in bulk actions, in the REST API or in the workflow engine had to learn the new rule, and none of them can get round it.
• It found a bug nobody was looking for. attach_template was writing NOW() where every other timestamp in FreeITSM is UTC_TIMESTAMP() — a leftover of #126 that survived the first review. Fixed in passing, unprompted.

Most of what follows is conformance work. Four items are not, and those come first.

---

1. 🔴 A workflow must never write to ticket_notes

As written, the new "closed with no checklist" note did this:

} else {
    $conn->prepare(
        "INSERT INTO ticket_notes (ticket_id, analyst_id, note_text, is_internal, created_datetime)
         VALUES (?, 1, ?, 1, UTC_TIMESTAMP())"
    )->execute([$ticketId, "[Workflow Note]\n" . $note]);
}
} catch (Throwable $e) {}

Three problems, compounding:

`analyst_id` INT NOT NULL,
CONSTRAINT `fk_notes_analysts` FOREIGN KEY (`analyst_id`) REFERENCES `analysts` (`id`)

• analyst_id = 1 may not exist. On an install where the first analyst has been deleted, the insert raises a foreign-key violation.
• The catch is empty, so that violation is swallowed. The close succeeds and the audit entry silently is not written.
• Where analyst 1 does exist, an automated close is stamped with a real colleague's name. Someone reading the timeline sees a person who did not do it.

This is the module's own lesson pointed at its own audit trail. From round one:

An unenforced gate is worse than no gate — it reports control that is not there.

An audit note that silently is not written is the same failure. A compliance gate whose evidence is missing on exactly the installs where it matters is worse than no gate, because the absence is invisible.

Now, both closure notes go through one private writer:

private static function writeClosureNote(PDO $conn, ActorContext $ctx, int $ticketId, string $note): void
{
    try {
        if ($ctx->actorId > 0) {
            // a person: an internal note on the ticket
        } else {
            $conn->prepare(
                "INSERT INTO ticket_audit (ticket_id, analyst_id, field_name, old_value, new_value, created_datetime)
                 VALUES (?, NULL, 'Workflow Note', NULL, ?, UTC_TIMESTAMP())"
            )->execute([$ticketId, $note]);
        }
    } catch (Throwable $e) {
        error_log('[checklists] could not record closure override for ticket ' . $ticketId . ': ' . $e->getMessage());
    }
}

ticket_audit.analyst_id is nullable precisely so an action with no human behind it can be recorded honestly, and it is where the workflow engine's own notes already go. The existing recordClosureOverride() did this correctly and carried a comment saying why; the new branch was added beside it rather than through it, so the reasoning was not in the path of the person writing the second one. Both go through the same writer now — that is the actual fix. The comment is only a comment; the shared function is the thing that cannot be walked past.

🔑 If you find yourself writing a second copy of something that has a 🔴 comment on it, the comment is in the wrong place. Put the behaviour in one function instead.

---

2. 🔴 Both halves of one rule must read the same company

// assertClosureAllowed()      — decides whether the close is REFUSED
ticketChecklistEmptyClosureMode($conn, $tenantId)

// recordClosureOverride()     — decides whether it is RECORDED
ticketChecklistEmptyClosureMode($conn, null)      // ← install-wide

On a single-company install these are the same value and the bug is invisible. On a multi-company install they are not: one company's setting could refuse the close while the audit decision came from the install default. A rule that is enforced per company and audited install-wide is two rules.

recordClosureOverride() now takes ?int $tenantId and both call sites — TicketsService::updateTicket() and workflow/includes/engine.php — pass the same $closeTenant they already pass to the refusal.

⚠️ null is a real value here, not "unknown". ticketTenantId() returns null for a ticket that belongs to no company, so $a ?? $b on a tenant id silently falls through to something else. There was one of these in the endpoint too ($ticketTenantId ?? ($tenantId ?? null)); it is now the ticket's tenant, full stop.

---

3. 🔴 A tooltip that looked translated and never could be

scored.forEach(({ template: t }) => {
    // ...
    typeof t === "function" ? t("tickets.checklists.mandatory_for_closure") : ""

t is the template row. It shadows the global t() translation function, so typeof t === "function" is always false, the expression always yields "", and the tooltip always falls back to the English default. The code reads exactly like code that handles internationalisation and cannot ever do it.

This is the third defect in this module of a shape worth naming: it is invisible to a locale completeness score. The key exists in lang/en, every locale can translate it, every percentage reads 100% — and the string never reaches the screen in any language but English.

Now, the way assets/js/calendar.js already does it:

function chkT(key, fallback) {
    const v = (typeof window.t === 'function') ? window.t(key) : '';
    return (v && v !== key) ? v : fallback;
}

window.t cannot be shadowed by a local variable.

The same 400-character icon expression had been pasted into three places — the inline panel, the popup modal and the attach dialogue — two of them inside those shadowed loops. It is one closureLockIcon(effectiveMode, px) function now. Pasting is how one bug became three.

---

4. 🔴 ENUM('inherit','warn','block') where nothing inherits

Nothing ever wrote inherit: the editor writes warn or block, and effectiveClosureMode() mapped anything that was not block to warn. So the column had three values, two meanings, and a name that described behaviour it did not have. A later reader would reasonably assume inherit follows the company setting — and write code depending on it.

ENUM('warn','block') NOT NULL DEFAULT 'warn' in both database/freeitsm.sql and includes/db_verify_schema.php.

The schema was declared in both places correctly — that is the rule from round one and it was followed without prompting. dbVerifyColumnSelfCheck() reports zero drift across the whole schema.

---

5. Smaller conformance points

PDO $conn comes first. effectiveClosureMode(string $mode, PDO $conn, ...) → effectiveClosureMode(PDO $conn, string $mode, ...). Every other method in the service takes the connection first; an argument order that is right everywhere except one place is a trap for whoever calls it next.

There is already a helper for that. The branch hand-rolled SELECT tenant_id FROM tickets WHERE id = ? twice. ticketTenantId(PDO $conn, int $ticketId): ?int lives in includes/tenant_settings.php, which the endpoint already required — and it handles the missing-table case the hand-rolled version did not.

One way of calling t(), not three. Within about sixty lines of inbox.js there were three: a local safeT() helper that guessed whether a key was missing by testing v.startsWith('tickets.'), a bare t(...), and t(...) || 'fallback'. The sniff would also discard a legitimate translation that happened to begin "tickets.". The file's own convention is a bare t() — the keys are in lang/en, so they exist.

A dialogue whose Cancel does nothing is not a dialogue. Two of the new showConfirm() calls were awaited and their answer discarded, then the same thing happened either way. showConfirm() has no single-button mode, so rather than fake one both buttons now mean something: the close is refused regardless, and OK is a shortcut to the thing that would unblock it — the attach dialogue, or the checklist itself.

An invalid setting must not quietly loosen a gate. save_checklist_settings.php rejected an unknown mode but silently defaulted an unknown empty_mode to off. That is the wrong direction for a compliance control: it reports success while relaxing the thing somebody tightened. Both refuse now.

Deleted comments. The branch removed the docblock from save_checklist_settings.php explaining why the setting is install-wide and how tenantSetting() will resolve a per-company override later, the note that setting_key is the primary key so the upsert is safe, the one about reading the value back rather than echoing the input, and an inline // or the dropdown shows a status never applied in inbox.js. All restored.

🔑 This is why the branch was applied to the working tree and reworked before committing, rather than merged and repaired afterwards. Repair-after-merge makes a restored comment look like a new addition in the follow-up diff, and anything missed stays deleted with nothing to show it ever existed. Prose disappearing is the hardest thing to see in a diff. Applied as a patch, every deletion has to be justified before it can be committed.

---

6. Terminology, and what it costs

Standardising on Checklist — over a mixture of "SOP checklist", "Attach a procedure" and "SOP step ticked off" — is right, and it went in across the inbox, ticket view, editor, workflow editor, settings, help page and demo data.

One consequence to be aware of, because it is not obvious from the diff:

⚠️ It is an English-only change. The twelve locales at 100% still say SOP-Checkliste, Lista kontrolna SOP, SOP-sjekkliste, SOP-tjekliste. Until a fan-out run over the renamed strings, the product says "Checklist" in English and "SOP checklist" in twelve other languages. The cleanup is half done until then, and that is the half nobody sees when reviewing in English.

The new keys have the same effect in the other direction: about twenty-five additions to lang/en immediately move every 100% locale off 100%.

Also fixed: checklists.help.steps_intro contains intended <em> markup and help.php was passing it through htmlspecialchars(), so the help page literally printed <em>set the account up</em>. Spotted and fixed in the branch. Recorded here because several neighbouring strings in the same file are echoed raw and this one was the odd one out for no reason — the general rule is still to escape, and a string that carries markup should be obvious from its key or its neighbours.

And a translator trap made worse before it was fixed: lang/en/tickets.php carried a 🔴 TRANSLATORS: this is a WARNING, not a refusal note, and the new blocked_title / blocked_message — which are refusals — were added directly beneath it. The group is now split into labelled WARNING and REFUSAL blocks. Six dialogue strings that had been filed under settings.checklists moved to checklists, next to the dialogues they belong to.

---

7. Two design notes

The company override only ever tightens. block_all overrides every template; there is deliberately no setting that relaxes a Critical checklist to a warning. An administrator who wants that should edit the checklist, where the change is visible to the people following the procedure — rather than have a setting three screens away quietly disarm a gate its author chose.

The "no checklist attached" rule is install-wide, and will probably want a scope. Most desks should leave it off: a password reset needs no checklist, and insisting on one adds friction to exactly the tickets that least deserve it. It went in as offered because as a default-off switch it harms nothing, but the natural shape is per ticket type or per category rather than per install. Worth knowing before anybody builds on it.

On Tasks: the suggestion to badge the Tasks scope as roadmap rather than build it is the right call. Subtasks already carry assignment and due dates; checklists are in-the-moment procedure. Offering both inside Tasks invites teams to use them interchangeably, and the distinction is hard to re-establish once that has happened.

---

8. How this was checked

• Through TicketsService::updateTicket(), not through ChecklistsService directly — that is the choke point the inbox, bulk actions, the REST API and workflows all share, and testing the rule rather than the path is how the original gate came to be unenforced.
• Every refusal paired with a positive control. A Critical checklist refuses; a Standard one with the same outstanding step still closes; a Critical one whose step is ticked closes. A gate that refuses everything looks identical to a gate that works.
• The workflow fix asserted directly: an automated close writes zero rows to ticket_notes and one to ticket_audit with a NULL analyst.
• One process per scenario. tenantSetting() memoises into a static $cache — right for a web request, fatal to a test that changes a setting and reads it back. The first run of this suite reported four false failures for exactly that reason.

😐 That trap is written down in section 9 of the round-one page, from the first review of this same module, and it was still walked into. Written-down knowledge only helps when somebody re-reads it — which is an argument for tests that fail loudly rather than notes that explain why they failed.

A fifth apparent failure was a LIKE '%steps outstanding%' pattern that could not match a note reading step(s) outstanding. Worth mentioning only because it is the same category: a check that can fail for its own reasons will, and every red needs reading before it is believed.

---

9. Related

• Checklists module — house style — round one
• Checklists & SOPs — developer guide · Checklists & SOPs
• Service layer · Architecture · Database Verification
• Internationalisation · Multi-tenancy