Modernize cfieldsTprojectAssign

Modernize: Custom Fields → Test Project (`cfieldsTprojectAssign`)

Refs #1816 (enhancement) · suite Issue #1816 = 49/49 PASS

Why this screen

The TODO section of docs/MODERNIZATION-STATUS.md is empty — every ASIDE entry already maps to a modern .html screen plus a BFF. The smallest coherent legacy-only slice left was Custom Fields → Assign to Test Project: lib/cfields/cfieldsTprojectAssign.php + gui/templates/dashio/cfields/cfieldsTprojectAssign.tpl, the definition ↔ test-project link screen, reachable only from the Custom Fields manager.

Authorization holes in the legacy controller

#	Hole	Legacy	Now
1	Cross-project authorization	a user with a project role of Test Project Manager / Admin / Leader on project A was authorized for project B whenever the global role was `testproject_manager` or higher (roles 5/9/10 — role 7/3 leaked through)	`cfield_management` checked on the addressed project on every route, including the project switcher
2	Submitted ids	`cfield_id[]` from the POST body was never proven to exist	every id intersected with the real field set, and the assign path with the not-yet-linked set
3	Addressed project	never proven to exist	`get_node_hierarchy_info()` → 404 `tproject_not_found`
4	Write verb	assign / unassign / save all ran on GET	POST-only behind a same-origin CSRF proof

Modern screen

gui/templates/cfields/cfieldsTprojectAssign.html — Dashio: teal header, TLi18n locale switcher, dark toolbar, and two cards:

Custom fields assigned to this test project — NAME / LABEL / TYPE / AVAILABLE ON / DISPLAY ORDER / LOCATION / ACTIVE / REQUIRED / MONITORABLE, with a confirm-modal Unassign.
Available custom fields — not yet assigned to any test project, with Assign.

The project switcher is new: legacy read the project from the session, so moving between projects required hand-editing the URL. The LOCATION dropdown renders only for design-time test-case fields and — for everything else, exactly as tpl:91-98 did.

BFF

api/cfieldstproject/index.php — GET ?action=init|projects, POST ?action=assign|unassign|save. Stable machine codes, each mapped to an i18n key: 400 nothing_selected|unknown_cfield · 401 · 403 no_right · 404 tproject_not_found · 405 · 409 already_assigned.

Legacy lib/cfields/cfieldsTprojectAssign.php is now a session-guarded shim: 302 to the modern screen for a browser navigation (carrying the session project), 405 for anything that looks like a write, login.php?note=expired for an anonymous caller.

Bugs found by the browser pass

Each filed with --label bug, each fixed in its own commit.

Issue	Defect
#1817	`testproject::get_list()` does not exist → `?action=projects` answered HTTP 500 with a zero-length body. Only reproducible with a session carrying a test project, which is why a bare `curl` login looked like a 200. Replaced with `get_accessible_for_user()`, which is also the semantically right source (it drops private projects the user holds no role on).
#1818	`guard()` set `busy=true` but only `saveAll()` released it, so after one Assign every button became a silent no-op. The dangerous part: a discarded Save leaves the edited values in the inputs, so a lost write looks exactly like a successful one (measured against the DB — `display_order` stayed `1`).
#1819	The assign/unassign toast printed the raw `{count}` placeholder. Save was unaffected because it did pass the params, which is what made it easy to miss.
#1820	`audit_cfield_location_changed` (missing in 15/19 server locales) and `audit_cfield_monitorable_on/_off` (15/19 — and the key is built from a variable, so a grep for the literal finds nothing but `zh_CN`) meant two `LOCALIZATION` warnings in the Event Viewer on every save.
#1821	Check / uncheck all was completely dead: `toggleAll()` called `invalidateSearch()` on an object that has no such method in DataTables 1.13.7, throwing before the rows were ticked. The master checkbox still flipped natively, so it looks like a styling quirk.
#1822	`link_to_testproject()` does an unconditional `INSERT` against PK `(field_id,testproject_id)`, so a duplicate assign served a DB Access Error HTML page as HTTP 200.

Reported, not fixed

#1823 — ro_RO and most other server locales lack the node-type and custom-field location labels (before_summary is in only 5/19 locales, hide_because_is_used_as_variable in 4/19), so the Location dropdown renders English. The BFF calls lang_get() correctly and falls back to en_GB as designed — this is missing translation data, not a screen defect. Authoring ~8 keys × 14 locales is a translation task.

Why the suite asserts state, not "did it crash"

Every one of #1818, #1819 and #1821 still looked fine when clicked — #1821's master checkbox flipped natively — and would have passed an eyeball check while silently doing nothing. So every case measures the resulting state: checked, indeterminate, the (n) selection counter, disabled, and a direct SQL cross-check of every write.

Verification

Suite Issue #1816 — 49/49 PASS (tmp/TLU_Test_Cases.md), incl. the full rights matrix with a real cross-project write attempt (403 on all three verbs), CSRF with a missing and with a foreign Origin, and the duplicate-assign 409.
TLU_REQUIRE_SUITE="Issue #1816" bash ai/verify_test_suites.sh → 7 PASS / 0 FAIL, 74 → 75 suites, none lost.
Browser console: 0 errors. Event Viewer: 0 new Error/Warning rows.
Fixture tmp/fixtures_1816.php — two projects, five custom fields covering every shape the screen renders, two permission users (one project-scoped, one with no rights at all).

Commits

96596c8a9 (BFF) → d4e40daeb (screen) → 382ead932 (i18n ×10 + wiring + shim) → 9a743b67f (#1817) → 3128139a4 (#1818 + #1819) → 36121574a (#1820) → 0df8f57f7 + d9969d57a (#1821) → c7532c787 (#1822) → 7c4702796 + 48dab1612 (suite + fixture).

Mandatory code review

A review subagent over the landed diff reported no BLOCKER — authorization/IDOR, CSRF/session, XSS, legacy parity and i18n all verified clean — and exactly one MINOR, fixed in cadb3ccd1:

POST ?action=save tested the submitted location against the valid codes when counting the change, but assigned $location[$id] = $v unconditionally. setDisplayLocation() just does intval() and an UPDATE, so the membership test gated the counter and not the write: a crafted {"rows":[{"id":16,"location":99}]} answered {"changed":{"location":0}} — a truthful "nothing changed" — and persisted 99 anyway.

A second half was missing entirely: supports_location. The UI renders a LOCATION dropdown only for design-time test case fields, so a crafted row could set a location on the execution-only field, which has no dropdown at all. The server now derives the same two conditions the client uses.

Lesson worth grepping the other BFFs for: a validator that only guards a counter is not a validator — "what I counted" and "what I wrote" were different variables, and a mismatch between them is silent by construction. Guarded by suite Regression — Issue #1816 (addendum), TC-1816-49..55.

cfieldsTprojectAssign - initial render

cfieldsTprojectAssign - after assigning two custom fields

#1821 — addendum: "Check / uncheck all" · why the call was removed, not relocated

The bug row above and commits 0df8f57f7 → d9969d57a were left without a regression suite, a docs/ page or a verification trail, and the issue stayed open. This addendum closes that gap.

Correction to the earlier diagnosis

An intermediate comment claimed invalidateSearch() exists on the Row API instance in 1.13.7 and that only the receiver had been wrong. Measured live, that is not the case — it is undefined there too, so all three obvious wrappers throw:

jQuery.fn.dataTable.version                       = "1.13.7"
typeof dtLinked.row(0).invalidateSearch           = "undefined"   <- Row API
typeof dtLinked.rows().invalidateSearch           = "undefined"   <- rows() collection
dtLinked.rows().every(r => r.invalidateSearch())  -> TypeError: r.invalidateSearch is not a function
dtLinked.rows().toArray()[0].invalidateSearch()  -> TypeError: ...toArray(...)[0].invalidateSearch is not a function

invalidateSearch() was removed in DataTables 1.11, superseded by the generic row.invalidate('search'). This screen loads 1.13.7 from the CDN (cfieldsTprojectAssign.html:106 → cdn.datatables.net/1.13.7/…) — not the 1.9.4 file vendored in gui/templates/dashio/lib/advanced-datatable/, which it does not use; the live page confirms it with jQuery.fn.dataTable.version === "1.13.7". So the method is gone from the whole API. That is why the fix deletes the statement instead of relocating it — a third relocation attempt would have failed the same way.

Why deletion is also the correct fix, not just the working one

invalidateSearch() invalidates DataTables' cached search index for a row whose data changed. Ticking a checkbox changes no column data, so the call was cargo-culted: even on a DataTables version that still had it, it would have been a no-op. Deleting it therefore costs nothing — verified by forcing a redraw after a check-all:

Step	linked	available	`btnUnassign`	`btnAssign`
initial	2 rows, 0 checked	2 rows, 0 checked	disabled	disabled
Check all	2 checked	2 checked	enabled — `Unassign (2)`	enabled — `Assign (2)`
Uncheck all	0 checked	0 checked	disabled	disabled
after sort redraw (`dtLinked.order([[1,'asc']]).draw(false)`)	2 checked	—	enabled	—

Selection survives the redraw, so nothing is lost. Console after the fix: no errors, no warnings. End-to-end, check-all on Available → Assign (2) → click moves the fields to the linked table (badges 2→4 and 2→0, empty state "Every custom field is already assigned to this test project.") with the toast 3 custom field(s) assigned.

gui/templates/cfields/cfieldsTprojectAssign.html:358-366 now carries a comment recording the DataTables-version finding so the call is not re-introduced. Blast radius: the only remaining hit of invalidateSearch under gui/templates/ is the explanatory comment quoted in the source — no executable call is left anywhere in the tree. It existed only here, affecting exactly the two master checkboxes (lines 249 and 298). No PHP change was needed.

Regression suite

tmp/TLU_Test_Cases.md → ## Regression — Issue #1821, TC-1821-01 … TC-1821-12, 12/12 PASS. TLU_REQUIRE_SUITE="Issue #1821" bash ai/verify_test_suites.sh → 7 PASS / 0 FAIL, 74 → 75 suites, none lost.

Fixture tmp/fixtures_1821.php — tproject CFA 1821 with 2 linked + 2 available custom fields (the freshly imported DB has no test projects at all, so a fixture is mandatory before the master checkbox has any row to act on).

Fixture gotcha: cfield_mgr::link_to_testproject($tproject_id, $cfield_ids) (lib/functions/cfield_mgr.class.php:1064) takes the project id first. Passing the field id first silently links the wrong field and raises E_WARNING Trying to access array offset on null - … cfield_mgr.class.php - Line 1085 (line 1085 does $cf[$field_id]['name'] on a missing key). That warning is a badly-called-fixture artefact, not a product defect: the screen's own Assign button performs the same assignment through the BFF and logs level-16 INFO only.

Docs mirror: docs/Bugfix-Issue-1821-cfieldsTprojectAssign-CheckUncheckAll.md.

cfieldsTprojectAssign - Check/uncheck all ticks every row, counter and buttons follow

#1817 — addendum: `?action=projects` HTTP 500 · verification trail

The bug row above and commit 9a743b67f were left without a regression suite, a docs/ page or a verification trail, and the issue stayed open. This addendum closes that gap. No source change was made in this run — the fix was already on the default branch; what was missing was proof.

Correction to the earlier report

The report claimed the 500 "only reproduced with a session that carries a test project". That nuance does not apply to this route. In the pre-fix file (96596c8a9) the handler is:

if ($action === 'projects') {
    if ($method !== 'GET') { cfpaFail(405, …); }
    $tproject_mgr = new testproject($db);
    foreach ((array) $tproject_mgr->get_list() as $p) { … }   // <- unconditional, right after auth
}

$action === 'projects' reaches get_list() for any authenticated caller — there is no test-project branch before it. The method simply does not exist on testproject:

bool(false)   <- method_exists(new testproject($db), 'get_list')
bool(true)    <- method_exists(new testproject($db), 'get_accessible_for_user')
Error: Call to undefined method testproject::get_list()

With display_errors off, that fatal is an HTTP 500 with a 0-byte body (the JSON content-type and nosniff headers are set at index.php:84-85, before the crash, which is why the screen's state card read http_500 rather than a parse error). The session observation most likely describes when the screen renders the switcher, not a server-side branch.

The fix, and why it is also the right one

foreach ((array) $tproject_mgr->get_accessible_for_user(
             intval($userId), array('output' => 'map', 'order_by' => ' ORDER BY name ')
         ) as $pid => $p) {
    $pid = intval($pid);
    if ($pid <= 0) { continue; }
    if (!cfpaRequireManage($db, $user, $pid)) { continue; }   // per-project cfield_management
    $rows[] = array('id' => $pid, 'name' => …, 'prefix' => …);
}

get_accessible_for_user(..., ['output'=>'map']) returns $pid => $row and drops private projects the user holds no role on (the "Private test project feature" WHERE clause at testproject.class.php:574-585), so the switcher never advertises a project the caller cannot reach. The per-project cfield_management filter is unchanged. This is the semantically correct source, not just an existing method — a plain get_all() would have leaked projects on which the caller has no right.

Regression matrix (curl, live at :8082)

Actor / call	HTTP	Body
`admin` `?action=projects`	200	both projects (`count:2`) — was 500 / 0 bytes
`cfp1816mgr` (rights on project 1 only) `?action=projects`	200	project 1 only (`count:1`)
`cfp1816none` (no rights) `?action=projects`	200	`[]` (`count:0`)
unauthenticated `?action=projects`	401	`not_authenticated`
`admin` `?action=init&tproject_id=1`	200	context + linked(2) + available
`cfp1816mgr` `?action=init&tproject_id=2`	403	`no_right`

Blast radius — grep -rn "\->get_list(" api/ lib/ gui/ --include=*.php finds no other call site (only the real get_list_reports() in api/resultsnav/index.php), so the report's "other BFFs may share this shape" note is resolved clean. Event Viewer: only level-16 audit rows, zero Error/Warning.

Browser verification

Open gui/templates/cfields/cfieldsTprojectAssign.html?tproject_id=1 as admin: the switcher lists CFP Main (CFPM) and CFP Other (CFPO) — the ?action=projects payload rendered — with no Could not load the data state card. Changing the switcher to CFP Other navigates to ?tproject_id=2 and loads that project's context (1 linked, 4 available), proving the payload is consumed, not merely parsed. Console: no errors.

Regression suite

tmp/TLU_Test_Cases.md → ## Regression — Issue #1817, TC-1817-01 … TC-1817-12, 12/12 PASS. TLU_REQUIRE_SUITE="Issue #1817" bash ai/verify_test_suites.sh → 7 PASS / 0 FAIL, 180 → 181 suites, none lost. Fixture tmp/fixtures_1816.php (the DB has no test projects at all).

Docs mirror: docs/Bugfix-Issue-1817-cfieldsTprojectAssign-Projects-500.md.

cfieldsTprojectAssign - the project switcher is populated (CFP Main + CFP Other), no http_500 state card

cfieldsTprojectAssign - switching to CFP Other loads that project's context

#1818 — addendum: `busy=true` latch after a successful Assign/Unassign · verification trail

The bug row above and commit 3128139a4 were left without a regression suite, a docs/ page or a verification trail, and the issue stayed open. This addendum closes that gap. No source change was made in this run — the fix was already on the default branch (commit 3128139a4, 20 seconds after the issue was filed — issue 06:20:46Z, commit 06:21:06Z); what was missing was proof.

The defect, re-measured

Reproduced with the pre-fix file served unchanged from the docroot (git show 3128139a4^:gui/templates/cfields/cfieldsTprojectAssign.html > tmp/issue1818_prefix.html, opened as /tmp/issue1818_prefix.html?tproject_id=1 — it uses only absolute /api/... and /gui/... URLs, so it behaves exactly as the screen did), fixture tmp/fixtures_1816.php:

After one Assign: window.busy === true, Refresh/Back disabled, the field itself was linked (linked=[3,1,2]) — only the mutex bookkeeping broke.
After editing Display order (1 → 7) and clicking Save display attributes: saveRequests 0 → 0, nothing in the resource timeline for action=save, the input still shows 7, DB display_order=1.

render() re-enables #btnSave and syncSelState() re-enables Assign/Unassign on every repaint, so the dead controls look alive — the click is dropped at guard()'s if (busy) return;. That is the worst failure mode for this screen: a discarded Save is indistinguishable from a saved one.

Why the landed fix is the right one

release() is now called in the .done() of doAssign (:435) and of the confirm-dialog Unassign (:464), before load(). Every write path settles through either release() (.done()) or writeError() → release() (.fail()). Auto-releasing guard() via a promise .always() was rejected as a larger change to the guard contract for no behavioural gain; dropping the busy mutex would let interleaved writes clobber each other. No PHP change was needed.

Regression matrix (live at :8082)

Action	`busy` after	Toolbar after	Request	DB / state
Assign id 3	`false`	all enabled	`action=assign`	`linked=[3,1,2]`, toast `1 custom field(s) assigned.`
Save display attributes (order 1→7)	`false`	all enabled	`action=save` ×1 (was ×0)	`display_order=7`
Refresh (idle)	`false`	—	`action=init` ×1	repainted
Save, no changes	`false`	all enabled	`action=save`	toast `No changes to save.`
Unassign id 3 via confirm modal	`false`	all enabled	`action=unassign`	`linked=[1,2]`, `available=[5,4,3]`, DB row gone

Event Viewer / events: 0 Error/Warning rows before and after.

Regression suite

tmp/TLU_Test_Cases.md → ## Regression — Issue #1818, TC-1818-01 … TC-1818-09, 9/9 PASS (TC-1818-01/02 are the pre-fix repro, TC-1818-03..07 the post-fix write matrix, TC-1818-08 the Event Viewer, TC-1818-09 the merge-base gate). TLU_REQUIRE_SUITE="Issue #1818" bash ai/verify_test_suites.sh → 7 PASS / 0 FAIL, none lost. Fixture tmp/fixtures_1816.php.

Docs mirror: docs/Bugfix-Issue-1818-cfieldsTprojectAssign-Stuck-Busy.md.

cfieldsTprojectAssign - pre-fix: after Assign the toolbar Refresh/Back are greyed out; the Save that "looks saved" never leaves the page

cfieldsTprojectAssign - post-fix: the Save display attributes POST lands and the toolbar stays live

#1819 — addendum: raw `{count}` in the Assign/Unassign toast · verification trail

The bug row above and commit 3128139a4 were left without a regression suite, a docs/ page or a verification trail, and the issue stayed open. This addendum closes that gap. No source change was made in this run — the fix was already on the default branch (commit 3128139a4, the same commit that fixed the sibling #1818); what was missing was proof.

The defect, re-measured

Reproduced with the pre-fix file served unchanged from the docroot (git show 3128139a4^:gui/templates/cfields/cfieldsTprojectAssign.html > tmp/issue1819_prefix.html, opened as /tmp/issue1819_prefix.html?tproject_id=<CFP Main>), fixture tmp/fixtures_1816.php, logged in admin/admin:

Tick one row under Available custom fields → Assign → the toast reads, literally, {count} custom field(s) assigned.
The BFF answered the very same call with the count, so the data was there to use: {"status":"ok","count":1,"tproject_id":5}.

Why the landed fix is the right one

TLi18n.t(key, params) substitutes {name} placeholders only when a params object is passed — gui/templates/i18n/i18n.js:174-188, the guard at :180:

function t(key, params) {
  var str = _strings[key] || key;
  if (params && typeof params === 'object') {  // pre-fix both handlers passed nothing
    $.each(params, function(k, v) {
      str = str.replace(new RegExp('\\{' + k + '\\}','g'), function(){ return v; });
    });
  }
  return str;
}

The bundle string is correct (gui/templates/i18n/en.json:713), "cfpa.msg.assigned": "{count} custom field(s) assigned.", so a missing-params call returns the literal. The fix passes the BFF count straight through — toast(t('cfpa.msg.assigned', { count: intOf(r.count) }), true) on :434 and the unassign twin on :463. saveAll() already interpolated (t('cfpa.msg.saved', {count:n})), which is exactly what made the bug easy to miss on a code read. Interpolating server-side was rejected: these are client-side bundle strings shared with the locale switcher. Stripping unknown {...} inside TLi18n.t was rejected too — it would hide the failing call site.

Regression matrix (live at :8082)

Action	Toast / modal	Result
Assign (post-fix)	`1 custom field(s) assigned.`	interpolated
Unassign → confirm modal	`Unassign 1 selected custom field(s) from this test project?`	interpolated
Unassign (confirmed)	`1 custom field(s) unassigned.`	interpolated
Save display attributes	`N change(s) saved.` / `No changes to save.`	unchanged, still correct
Re-assign an already-linked field	BFF `409 already_assigned` → `Those custom fields are already assigned to this test project.`	no raw placeholder

Event Viewer / events: 34 rows, all log_level=16 (INFO/audit), 0 Error/Warning before and after.

Regression suite

tmp/TLU_Test_Cases.md → ## Regression — Issue #1819, TC-1819-01 … TC-1819-09, 9/9 PASS (TC-1819-01/02 are the pre-fix repro + BFF payload, TC-1819-03..07 the post-fix interpolation matrix, TC-1819-08 the Event Viewer, TC-1819-09 the merge-base gate). TLU_REQUIRE_SUITE="Issue #1819" bash ai/verify_test_suites.sh → 7 PASS / 0 FAIL, none lost. Fixture tmp/fixtures_1816.php.

Docs mirror: docs/Bugfix-Issue-1819-cfieldsTprojectAssign-Raw-Count-Placeholder.md.

cfieldsTprojectAssign - pre-fix: the Assign toast prints the raw {count} placeholder

cfieldsTprojectAssign - post-fix: the Assign toast prints the interpolated count "1 custom field(s) assigned."

Modernize cfieldsTprojectAssign

Modernize: Custom Fields → Test Project (cfieldsTprojectAssign)

Why this screen

Authorization holes in the legacy controller

Modern screen

BFF

Bugs found by the browser pass

Reported, not fixed

Why the suite asserts state, not "did it crash"

Verification

Commits

Mandatory code review

#1821 — addendum: "Check / uncheck all" · why the call was removed, not relocated

Correction to the earlier diagnosis

Why deletion is also the correct fix, not just the working one

Regression suite

#1817 — addendum: ?action=projects HTTP 500 · verification trail

Correction to the earlier report

The fix, and why it is also the right one

Regression matrix (curl, live at :8082)

Browser verification

Regression suite

#1818 — addendum: busy=true latch after a successful Assign/Unassign · verification trail

The defect, re-measured

Why the landed fix is the right one

Regression matrix (live at :8082)

Regression suite

#1819 — addendum: raw {count} in the Assign/Unassign toast · verification trail

The defect, re-measured

Why the landed fix is the right one

Regression matrix (live at :8082)

Regression suite

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Modernize: Custom Fields → Test Project (`cfieldsTprojectAssign`)

#1817 — addendum: `?action=projects` HTTP 500 · verification trail

#1818 — addendum: `busy=true` latch after a successful Assign/Unassign · verification trail

#1819 — addendum: raw `{count}` in the Assign/Unassign toast · verification trail