Procedures Fold PR Review - edmozley/freeitsm GitHub Wiki

Empty Procedures fold with the other Work sections - PR review

PR #247 by Dr4inder · Issue #218 part B · Verdict: merge, with one small fix

How it relates to #218

#218 part B - the calmer Work view - shipped in 4.9.0. With System -> Preferences -> Empty work sections on a task set to fold, an empty Checklists, Forms or Follow-ups section shrinks to a heading such as Forms (0). The spec (Form-Fields-Design, part B) named those three sections and nothing else, so Procedures - the fourth section in the Work view - was left out. An empty Procedures section still showed No procedures and its search box in full.

The spec also asked that a section which fails to load says so instead of vanishing. 4.9.0 did that for the three sections it folded, but the Procedures loader still hid the section on a failure, so a broken procedures endpoint looked the same as Procedures on tasks being switched off. The developer guide said every loader had been changed; Procedures had not.

This PR closes both gaps. It is the rest of part B rather than a new feature: no new setting, no schema change, no new endpoint.

User page: The Work view. Internals: Folding empty Work sections - Developer Guide.

What the PR changes

All of it is in assets/js/tasks.js, plus the tasks.js?v=73 -> 74 cache-buster in tasks/index.php.

Change Effect
renderTaskProcedures() asks workSectionHead('procedures', ...) for its heading Empty Procedures folds to Procedures (0), with the same button, keyboard handling and per-task open state as the other three
workRenderPart() knows 'procedures' Opening or folding the heading redraws Procedures from procState, without fetching
loadTaskProcedures() throws on success: false and calls workSectionFailed() A failed load shows Could not load Procedures. with Retry; it is never hidden, and never shown as (0)
loadTaskProcedures() drops a response for a task that is no longer open A slow answer for the previous task cannot hide or redraw the section of the next one
tpSearch added to WORK_DRAFT_ID; renderTaskProcedures() restores drafts and re-runs the search A half-typed procedure search survives a redraw of the task window, with its results

What was checked

Question How Result
Does it reuse the part B machinery or write a second fold? Read the diff against workSectionHead(), workSectionFailed() and the drafts code Reuses all of it; Procedures is a fourth caller, as the developer guide asks
Does an empty Procedures fold, open, and keep focus? Headless browser on the test install, through the real Tasks page, preference on Procedures (0), aria-expanded="false", no search box; click opens it with the search box and focus stays on the heading
Does an opened section and a typed search survive a redraw? Typed a search, then called openDetailPanel() Still open, text back in the box, results shown again
Does adding the first procedure keep it open? Picked a result One card, plain Procedures label
Does removing the last one leave it open? Removed it Open, with the search box
What does a failure look like? Answered procedures.php with success: false, then with a network error Could not load Procedures. Retry both times, no fold button, section visible; Retry recovers to Procedures (0)
Does "switched off" still hide it? Read: data.enabled === false keeps its own branch before the fold Yes, unchanged
Preference off Same page with the preference cleared Plain heading and search box, as before
Can anything new reach the server? Diff read No. The only request added is the existing search GET, re-run after a redraw with text the user typed; no write, no new parameter
Is anything new put into the page unescaped? Diff read No. The heading goes through workSectionHead(), which escapes the translated label; the error through workSectionFailed()
Guards tests/web-exposure-guard.php, scripts/gen_db_verify_indexes.php --check, scripts/i18n_gate.php --keys All pass

What was changed after merging

Change Why
Adding a procedure drops the search draft (dropWorkDrafts('tpSearch') in postTaskProcedure()) The one regression found. The PR made the search box a draft but never dropped it, so after an add the redraw put the text back, re-ran the search and offered the article just added - pressing it again gets That procedure is already on this task. Before the PR the box came back empty after an add. Every other draft is dropped once what it was typed for is sent; this one now is too
The preference description and the in-app help name Procedures (English, German, Spanish) Both listed only Checklists, Forms and Follow-ups. The other locales fall back to English for these two strings
Feature Bingo card tasks.work_fold_empty Part B never had a card; it lights when somebody has chosen to fold
Changelog #2734 (the PR) and #2735 (the fix)
This page, the developer guide, the user page, the design page The full write-up

The PR went in under its author's name as a merge commit; the fix followed in its own commit.

Worth knowing

  • Procedures does not count towards progress. The 3 of 5 done in the Work head counts checklist steps and required forms. A procedure is something to read, not something to tick off, so it was not counted before and is not now. It does count towards whether the Work block shows at all (renderWorkHead()'s parts list).
  • The stale-task guard is only on this loader. loadTaskProcedures() now returns straight away when its answer is for a task that is no longer open. The checklist and forms loaders do not; they rely on their render functions comparing the state's taskId with selectedTaskId, and on workSectionFailed() and workLoaded() doing the same. Both work. The Procedures one is the tidier of the two, because it also stops the switched off branch from hiding the next task's section.
  • No automated test. Like the rest of part B, this is covered by the hand checks in the developer guide, now with Procedures steps, and was run here through a throwaway browser harness on the test install.

Related