Checklists Module House Style

Checklists module — house style

What changed between PR #141 as contributed and what shipped in 2.0.0, and why. Written mostly for Santhosh Srinivasan (Sandy), who built the module, and for anyone else contributing a module to FreeITSM later.

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

---

Before anything else

This is FreeITSM's first community-contributed module — not a patch, a whole feature, designed and built by somebody who describes himself as a finance person who oversees an IT department and codes at weekends.

A few things in it are better than the equivalent code already in the product:

• The template/instance split. ticket_checklist_items copies the step text rather than pointing at the template, so editing a template next year cannot rewrite the history of tickets that used it. That is a decision about time and evidence, and plenty of professional developers get it wrong.
• Every query uses a prepared statement. 2,834 lines, no string interpolation into SQL anywhere.
• The module found requireModuleAccessJson() and wired into the real access-control system rather than inventing a check.
• The problem was argued before it was built. #138 reasons about why Knowledge and Tasks do not fit first. Most feature requests do not do that.

Everything below is conformance work, not corrections of judgement. The list is long because FreeITSM has a lot of conventions, and none of them are discoverable from outside.

---

1. 🔴 The one that stopped it working: a fallback to a column that never existed

As contributed:

$stmt = $conn->prepare("SELECT id, template_id, title,
        COALESCE(created_datetime, created_at)  AS created_datetime,
        COALESCE(completed_by_name, completed_by) AS completed_by_name,
        COALESCE(completed_datetime, completed_at) AS completed_datetime
   FROM ticket_checklists WHERE ticket_id = ?");

Shipped:

$stmt = $conn->prepare("SELECT id, template_id, title, created_datetime
   FROM ticket_checklists WHERE ticket_id = ? ORDER BY id ASC");

created_at, completed_at and completed_by are created by nothing in the PR — not the module bootstrap, not database/freeitsm.sql, not includes/db_verify_schema.php — and nothing ever writes them. MySQL treats an unknown column in a SELECT list as a hard error, so on any install but the author's, every ticket asking for its checklist got:

{"success":false,"error":"Unknown column 'created_at' in 'field list'"}

⭐ Why it passed testing, which is the useful part

The author's database still carried the older column names alongside the new ones, because it had grown through earlier versions of the module. COALESCE always found one of the two. On his machine the feature worked perfectly.

This is the oldest bug in software and it catches everybody. The lesson worth taking is not "be more careful" — it is the argument for §2: a schema defined in one place cannot drift away from the code that reads it.

🔑 And toggle_item wrote the same phantom columns. Fixing the read path alone looked like success. It was only caught because the test drove the HTTP endpoints rather than calling the functions — see §9.

---

1b. 🔴 The same shape again: a URL that only resolves at the web root

Every API call in the ticket-side panel was root-absolute:

fetch('/api/tickets/ticket_checklists.php?action=get_ticket_checklists&ticket_id=' + ticketId);

FreeITSM can be installed in a subdirectory. There, all five calls 404 and the SOP panel silently does nothing — no error, no empty state, just a feature that is not there. On one machine, the same request:

/api/tickets/ticket_checklists.php                404
/freeitsm-app/api/tickets/ticket_checklists.php   200

Shipped:

// tickets/index.php already publishes window.API_BASE, derived from BASE_URL,
// and therefore correct whether the app is at the root or in a subdirectory.
const CHK_API = (window.API_BASE || '../api/tickets/') + 'ticket_checklists.php';

🔑 Never start a URL with /. BASE_URL (PHP) and window.API_BASE (JS) exist for this. It is the same family as §1 — code that is correct on the machine it was written on and nowhere else — and the two together are the best argument in this whole page for testing on an install shaped differently from your own.

---

2. A schema should have exactly one definition

The six tables were created in five places: the module bootstrap (checklists/includes/db_schema.php), database/freeitsm.sql, includes/db_verify_schema.php, and — inside catch (Throwable $e) {} — by checklists/index.php and checklists/settings/index.php as a page rendered.

Two of the five disagreed:

// checklists/includes/db_schema.php, freeitsm.sql, db_verify_schema.php
created_datetime DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP

// checklists/index.php and checklists/settings/index.php, inside a silent catch
created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP

So whichever page an operator happened to open first decided what the column was called. That is the machinery behind §1: the COALESCE fallbacks were the author defending against his own race, and they were a reasonable response to a real problem.

TIMESTAMP and DATETIME are not interchangeable either — MySQL converts TIMESTAMP on read and write, which is precisely wrong for a product that stores UTC at rest.

Shipped: database/freeitsm.sql for fresh installs, includes/db_verify_schema.php for grown ones, nothing else. The bootstrap file is deleted.

The house rule

New tables go in database/freeitsm.sql AND includes/db_verify_schema.php. Nowhere else. A module never creates its own tables at runtime. See Database Verification.

⭐ FreeITSM's own tooling then found two more

Running System → Database Verification reported, unprompted:

Index checklist_templates.idx_tpl_category is in freeitsm.sql but missing from the backfill list.
…5 indexes…
🟡 Declared differently in the two files: checklist_templates.id
   (freeitsm.sql has INT NOT NULL, Verification expects INT(11) NOT NULL) …

• Five indexes existed for fresh installs and never reached upgraded ones. Fixed by php scripts/gen_db_verify_indexes.php.
• Sixteen columns were declared int(11) against INT elsewhere — every int(11) in that 235 KB file was in the checklist block. Display widths are deprecated in MySQL 8.0.17+.

If you add tables, run Database Verification afterwards and read what it says. It is checking the two schema files against each other and it is rarely wrong.

---

3. Deletes must remove their own children

As contributed:

$stmt = $conn->prepare("DELETE FROM checklist_templates WHERE id = ?");
$stmt->execute([$id]);

Shipped:

$conn->beginTransaction();
try {
    $conn->prepare("DELETE FROM checklist_template_items WHERE template_id = ?")->execute([$id]);
    $conn->prepare("DELETE FROM checklist_templates      WHERE id = ?")->execute([$id]);
    $conn->commit();
} catch (Throwable $e) { $conn->rollBack(); throw $e; }

The PR's own freeitsm.sql declares ON DELETE CASCADE, so on a fresh install this worked. But Database Verification never creates foreign keys, so an upgraded install has none — and the steps were stranded permanently. Two runs of the review harness left twelve orphaned rows before anybody looked.

🔑 Both install shapes are supported, so nothing may depend on a cascade. Delete children explicitly. (Database Integrity)

The same applied to deleting a ticket: api/tickets/permanently_delete_ticket.php already removes notes, audit rows, time entries and emails by hand, for this exact reason. The two checklist tables joined that list.

---

4. UTC at rest

As contributed: $now = date("Y-m-d H:i:s"); — PHP's wall clock. Shipped: completed_datetime = UTC_TIMESTAMP() in the SQL.

Measured on the reference dev box, in September:

old code stored      2026-09-14 19:42:54
correct UTC instant  2026-09-14 18:42:54

An hour in the future, because PHP runs Europe/London and MySQL runs UTC. FreeITSM stores UTC and renders in the viewer's timezone; a locally-stamped time is wrong for every reader including the one who wrote it. This had its own incident (#126, 302 rows).

api/tickets/ uses UTC_TIMESTAMP() 54 times against one NOW(). Follow the 54.

The endpoint also now reads the stored value back instead of echoing what it believed it wrote, so the response cannot disagree with the next GET.

---

5. Business rules go in a service, not in the browser

This is the biggest structural change, and the most useful one to understand.

As contributed — assets/js/inbox.js:

if (pendingMandatory.length > 0) {
    select.value = oldValue;
    alert("⚠️ Cannot close/resolve ticket.\n\n…");
    return;
}

The inbox obeyed it. The v1 REST API, bulk actions, the workflow engine and one line of curl did not. For a feature whose purpose is ISO/SOP compliance, a gate only the UI honours is worse than no gate: it reports a control that is not there.

Shipped — includes/services/checklists.php, called from TicketsService::updateTicket():

class ChecklistsService {
    public static function assertClosureAllowed(PDO $conn, int $ticketId, ?int $tenantId = null): void
    { /* throws ServiceError when the operator chose 'block' */ }

    public static function recordClosureOverride(PDO $conn, ActorContext $ctx, int $ticketId): array
    { /* writes an internal note naming the skipped steps */ }
}

updateTicket() is the choke point every closure path goes through, so one call covers all of them.

The three rules a service obeys

1. method(PDO $conn, ActorContext $ctx, array $input) — no superglobals. No $_SESSION, no $_POST.
2. No transport. Never echo, header(), exit. Return data or throw ServiceError.
3. Typed errors. ServiceError($kind, $code, $message); the adapter maps $kind to an HTTP status.

The endpoint becomes a thin adapter:

try {
    $id = ChecklistsService::saveLookup($conn, ActorContext::fromSession($conn), $kind, $data);
    echo json_encode(['success' => true, 'id' => $id]);
} catch (ServiceError $e) {
    echo json_encode(['success' => false, 'error' => $e->getMessage()]);
}

Full detail: Service layer — one implementation, two interfaces.

And the rule became a setting

FreeITSM's position, already written in inbox.js three lines below where the checklist gate was inserted:

"closing a ticket that still has unfinished tasks WARNS, it never blocks … a warning that cannot be shown must not become a block that cannot be cleared."

A hard block traps any ticket whose remaining step has become impossible. So Tickets → Settings → Checklists offers both, warn by default, and the override is recorded either way. An auditor wants the exception attributable, not impossible.

🔑 This is a general habit here: offer the setting, not a binary. When two reasonable organisations would want different answers, that is usually a setting.

---

6. Where files live

	As contributed	House
Endpoints	checklists/api.php with ?action=	api/<module>/<action>.php
JavaScript	checklists/ticket_view.js	assets/js/
CSS	checklists/ticket_checklist.css	assets/css/
Schema	five places	freeitsm.sql + db_verify_schema.php
Editor	a modal	its own screen, like forms/edit/

⚠️ On ?action= dispatch specifically: it is a minority pattern, not a banned one. 73 of FreeITSM's 842 API files use it. One-file-per-action is the dominant convention and what new code should follow, but the contributed shape was not wrong so much as unusual.

---

7. The UI conventions nobody can guess

These caused more review comments than anything architectural, and none of them are inferable from the outside.

Toggles, not tickboxes — and the class names are a trap:

<!-- 🔴 .switch / .slider do not exist anywhere in FreeITSM.
     Using them renders a bare checkbox, which the Time tracking tab
     did for months before anybody noticed. -->
<label class="toggle-switch">
  <input type="checkbox"><span class="toggle-slider"></span>
</label>

Icon buttons in settings, text buttons on main screens. Settings rows use action-btn / action-btn delete with the exact SVGs from tickets/settings/index.php — copy them, do not redraw them.

🔴 And the handler must use closest(), never classList:

// the click lands on the inline <svg> or its <path>, NEVER on the button
if (e.target.closest('.lk-del')) { … }

This broke the ticket-categories tab once already when its text buttons became icons.

showConfirm() and showToast(), not confirm() and alert(). Both are loaded by the waffle menu, so they are already available on any module page — no script tag needed.

Sentence case for headings and labels. One-word buttons — "Save", not "Save Template".

Full-width settings pages, and this one has a trap of its own:

/* ⚠️ max-width alone is NOT enough. inbox.css sets `.container { margin: 30px auto }`,
   and an auto cross-axis margin inside a flex column cancels the stretch — the page
   keeps its gutters and reads exactly as though the cap were still there. */
.container { max-width: none; width: 100%; margin: 0; }

---

8. Two smaller things worth knowing

i18n. Every user-visible string goes through t('namespace.key') in PHP and window.t() in JavaScript, with the English in lang/en/<module>.php. FreeITSM ships 24 locales, five of them at 100%. A hardcoded string is invisible to that machinery.

Demo data. A dataset at database/demo-data/<module>.json lets somebody evaluate the module without inventing content. Every table it writes to needs an is_demo column, because removal is DELETE FROM <table> WHERE is_demo = 1 — an earlier unqualified version of that delete emptied real tables on a live system.

🔴 And a subtlety found by Ed clicking around: editing a seeded template rewrote its steps, and a freshly inserted row defaults to is_demo = 0 while the parent stayed 1. Removing the demo data then took the template and stranded its steps. Children now inherit the parent's flag. If you add demo data, try editing it and then removing it.

---

9. How this was checked

Offered because the method found things reading would not have.

• Every test drove the HTTP endpoint, not the function. Fixing the phantom columns in the read path looked like success; toggle_item wrote the same columns and only a request revealed it.
• Negative controls throughout. A test that only ever sees the good outcome cannot tell "works" from "always fires". The closure-rule test asserts that a clean close writes no override note; the drag test asserts that a drag started from a text input does not reorder.
• The browser, for anything visual. A .toggle-switch whose CSS never loaded is an invisible checkbox that passes every DOM assertion, so the test measures the slider's rendered width. Clicks are dispatched at what elementFromPoint actually returns, which for an icon button is the <svg>.
• One mode per process, for settings. tenantSetting() memoises in a static array, so flipping a setting mid-run and reading it back returns the first answer and the test passes for the wrong reason.

Three of the review's own findings were wrong before they were right — a parser that reported six missing columns that were present, an orphan query that asked "does a demo template have non-demo steps" instead of "does a step have no template at all", and a headless page-load that reported no console errors because it had served the login page. A check that can fail quietly is worse than no check.

---

10. Related

• Checklists — closure gating: what changed and why — round two, the per-template Standard/Critical gate
• Checklists & SOPs — developer guide
• Service layer · Architecture · Database Integrity
• Database Verification · Internationalisation