feat(dot-roles): Tools tab in Angular, roles service consolidation, and edit-gate fixes

[hmoreras]

Proposed Changes

role-portlet-progress.mp4

Follow-up work on the Roles (Beta) Angular portlet.

Consume the backends that landed — GET /v1/roles/{roleId}/users (#37070) replaces the previous pair of member-loading calls. The old id-based /rolehierarchyanduserroles fallback returned Role objects rather than users, which is why the Users tab rendered an empty EMAIL column; that column now populates. childCount and userCount (#37071) drive leaf-vs-chevron detection on first paint — no more chevrons that expand into nothing — and unblock the per-row user-count badge the design called for.

Worth flagging: the code's comments predicted /roles/{id}/users would resolve inheritance server-side and collapse the whole flow into one call. It shipped as a direct grants only resource by design, so the ancestor-chain fan-out stays and is now documented as permanent rather than as a stopgap. For the same reason the members table keeps client-side paging — a server page of one ancestor is not a page of the merged union, so [lazy]="true" would be incorrect, not pending.

Migrate the Tools tab to Angular — replaces the Dojo iframe. Scope is deliberately narrower than the JSP it replaces: it lists tool groups and toggles which ones the role gets. Creating, editing and deleting tool groups is not here — those have no v1 endpoints (only RoleAjax DWR) and belong to a Tools portlet of their own. Inherited grants render checked but locked; echoing one back into the full-replace POST would silently promote it to a direct grant. The iframe component and its two JSPs are deleted rather than left behind.

Consolidate the roles surface into DotRolesService — three libs had grown their own implementation of the same /api/v1/roles/** surface, and three TypeScript models of the same backend RoleView. Reads, writes and models now live in one place; method names drop the redundant role (createRole → create). get and search keep their legacy names — the workflow assign components inject this service and those paths are untouched. The full-hierarchy walk did not move into the service: it is one way of composing two endpoints for one UI, so it lives as a pure, HTTP-free function in dot-users, and it now prunes on childCount — most roles below the first level are leaves, which removes the large majority of the request burst the Roles tab produced on open.

Fix edit gates that were stricter than the backend — the Tools tab greyed itself out for CMS Administrator while the legacy portlet allows the edit. RoleHelper applies a different contract per operation: update/delete checks isSystem() || isLocked(), user grants check isEditUsers() alone, and saving layouts places no restriction on the target role at all.

Review fixes

A multi-agent review and hands-on QA surfaced defects that are fixed in this PR:

• Two cross-role races. saveToolGroups and the user grant/remove flows captured the role id before their await and reconciled with it afterwards, so a role switch mid-flight repainted the new role's tab with the old role's data. For tool groups this was worse than cosmetic: the tab derives its next POST payload from toolGroups, so one role's grants could be written onto another. Guarded at both ends, with regression tests verified by neutering each guard and confirming they go red.
• "Could not verify" was being rendered as "not granted". A per-ancestor catchError returned an empty set on failure, so a transient error showed an inherited tool group unchecked — and an admin trusting the grid would create a redundant direct grant. Failures now surface instead of being absorbed.
• childCount regressions. Local tree mutations left it stale, so a parent that just gained its first child rendered as a leaf and the new role was unreachable. And POST /v1/roles answers with Role.toMap(), which carries no childCount, so a freshly created role drew as a folder.
• The tree user-count badge never refreshed after a grant or revoke. It now syncs from the direct-grant count.
• dot-users-list.store.spec.ts still mocked a method deleted in this PR and never mocked DotRolesService, so the roles column ran against the real root service and was effectively untested.

Add / Edit role dialogs

Required markers use the dotFieldRequired directive (which is what makes them red) instead of hand-written text, and dot-field-validation-message is wired in — there was no validation feedback at all. The dialog title moves from an h2 in the body into the dialog header.

Parent selection becomes a p-treeSelect: indentation alone gets ambiguous past two levels. It starts collapsed, hydrates a branch on expand (the backend sends two levels per request), and its filter runs the same deep search the roles tree uses, so a role in an unloaded branch is findable.

Two PrimeNG behaviours needed working around, both from one cause — the component keeps state inside the node objects it is given, and these options come from a computed that hands it new ones. Expansion is mutated onto node.expanded, so branches snapped shut the moment their children loaded. And Tree.getRootNode() returns its cached filteredNodes once the client filter has run, ignoring value, so server results never rendered. Both are handled explicitly, with the reasoning in the code — the second reaches one internal method, which is the most upgrade-fragile line in this PR.

Design review

Empty states now use the shared DotEmptyContainerComponent; the hand-rolled dashed card they replaced exists nowhere else in the product. shield → shield_person. The + on a tree row is primary with a pointer cursor. Untranslated portlet titles no longer print as raw i18n keys.

The user-count badge takes a real grey. text-color-secondary is a PrimeFlex-era class — PrimeFlex is not installed and tailwindcss-primeui does not provide it either, so it compiles to nothing and elements inherit the label's near-black. Verified against the built CSS rather than assumed. 104 more occurrences survive across 35 files and need their own pass.

Checklist

• Tests
• Translations
• Security Implications Contemplated (add notes if applicable)

Additional Info

Verification — 36 projects linted, 35 tested, nx format:check clean, and the dotcms-ui production build green. The dot-roles suite is at 136 tests.

Security note. While tracing the tool-group endpoints, GET /api/v1/roles/layouts turned out to have no authentication gate — the only one of 16 endpoints in RoleResource without a WebResource.InitBuilder block. Confirmed at runtime: it returns 200 with the full tool-group catalog to an unauthenticated caller, while three sibling endpoints return 401 under identical conditions. It predates this work (introduced 2023-05-26, shipped since v23.06) and is filed separately as #37259. This PR does not change that behaviour, but it does put the endpoint on a more exercised path.

Standards note. ANGULAR_STANDARDS.md asks for Signal Forms on new forms, but the standard validation components are Reactive-Forms-bound — dotFieldRequired injects FormGroupDirective and dot-field-validation-message takes an AbstractControl. These dialogs stay on Reactive Forms; adopting Signal Forms would mean dropping both.

Follow-ups not included here

• loadRootRoles(true) → false is now possible thanks to childCount, and would lighten the initial payload considerably on installs with many roles. It changes lazy-load behaviour, so it deserves measuring on its own.
• onNodeExpand marks a node fetched before the load resolves, so a failed lazy-load never retries.
• Creating a role under a parent that is not currently loaded splices into a node that is not there — the role is created but does not appear until a reload.
• The text-color-secondary sweep described above.

🤖 Generated with Claude Code

This PR fixes: #36930

This PR fixes: #36930

[claude]

Claude finished @hmoreras's task in 3m 1s —— View job

---

Re-review — prior findings + fix commits

Checked the current head against the 9 prior findings and the three fix commits (33c0acac, 5570af29, 8281b3fa). Blocking findings 1–5 are resolved; 6–9 remain as non-blocking design/perf items (7 and 9 hold up as false positives on the current backend contract).

• Read store + tree-utils + both dialogs on current HEAD
• Recheck prior findings 1–9
• Check the fix commits for newly introduced bugs
• Post the review

Resolved

• ✅ update with latest SVN #1 dot-roles.store.ts:879, 945 — createRole now captures selectionOnSubmit and updateRole gates selectedRole writes on stillSelected = selectedRoleId() === roleId. The cross-role guard is applied to both mutations.
• ✅ Test Branch and Commit #2 dot-roles.tree-utils.ts:176-182 — collectAncestorChain returns [] (no fabricated stub) when the role isn't in the tree, and both loadMembers (:338) and loadToolGroups (:483) short-circuit to ERROR instead of firing a direct-grants-only fan-out.
• ✅ Add .gitignore #3 dot-roles.store.ts:871-899 — verified against Role.toMap() (Role.java:213-228): the POST payload carries system, locked, editUsers, editPermissions, editLayouts, and parent. Seeding selectedRole from it (+ childCount: 0) leaves no edit-gate flag undefined, so the permissive ?? true fallbacks aren't hit. The concern is genuinely addressed, not just re-documented.
• ✅ New Issue #4 dot-roles.store.ts:1104-1109 — the optimistic members prune now runs inside the selectedRoleId() === role.id guard, so a mid-flight role switch can't strip users from another role's slice.
• ✅ wezell's issue #5 dot-roles-add.component.ts:183-231, dot-roles-edit.component.ts:223-267 — both #runSearch paths wrap the await in try/finally (token-guarded) and explicitly $searching.set(false) on the sub-3-char short-circuit. The stuck-spinner case is closed in both dialogs.

Existing (non-blocking)

• 🟡 Medium: dot-roles.store.ts:408, 546 (test rwqrwqrwqr wq #6) — a single failed ancestor fetch still flips the whole slice to ERROR, hiding the ancestors that did resolve behind the generic error state. This is now a deliberate tradeoff (unverified → ERROR, documented at :383-405 / :537-542) chosen over rendering "could not verify" as "not granted". Defensible, but a PARTIAL state that renders the merged rows with an inline "some ancestors couldn't be verified" warning would be strictly better UX. Not a regression.
• 🟡 Medium: dot-roles.store.ts:935-936 (testing assign #7 — author flagged false positive; concur) — nextParentId = updated.parent && updated.parent !== updated.id ? … : null only misfires if the PUT response omits parent. Role.toMap() (Role.java:224) and the RoleView serializer always emit it, and root is encoded as parent === id, so there's no current path that detaches-to-root by mistake. Purely a future-serializer hardening note; a parent == null → keep-in-place fail-closed would make the contract assumption explicit but isn't required.
• 🟡 Medium: dot-roles-add.component.ts:199,222 + dot-roles-edit.component.ts:237,258 (this is added #8) — the treeViewChild._filter() private-API workaround plus the #expandedKeys/onNodeExpand/#runSearch/#toTreeNodes triad are ~90% duplicated across the two dialogs. The PR itself calls this the most upgrade-fragile line, and it exists twice — the next PrimeNG bump breaks both. Cleanup, not a bug. Extract shared picker →
• 🟡 Medium: dot-roles.store.ts:704, 218 (this is added #9 — author flagged false positive; partly concur) — the toolGroupsSaved$ debounce coalesces only the toast; each checkbox click still fires a full-replace POST + catalog GET + N ancestor GETs. Acceptable for occasional toggles, but a burst on a role with several ancestors is a real request amplification, and out-of-order silent reconciles under jitter can restamp state from a stale union (only the last resolve is guarded, not the earlier stale ones). Worth a follow-up if the Tools tab sees heavy use; not a correctness blocker today.

Fix commits — no new issues

• dot-roles-tree.component.ts:151-205 — the reveal effect keys on the ancestor path string (not the tree object), so a userCount patch on member-load doesn't re-open collapsed branches, and the #revealedPath guard makes it act only on an actual move. Reads correctly.
• dot-roles-tree.component.ts:225-247 — the lostChildren = loadedChildren === 0 && (childCount ?? 0) > 0 gate correctly re-opens the lazy-load for a branch that lost its hydrated children on reparent. Sound.
• dot-roles.tree-utils.ts:239-250 + dot-roles.store.ts:952-962 — dedupeRolesById is now applied once before either the same-parent (patchNodeInPlace) or reparent branch consumes roleChildren/childCount, so the multiplied-children response (PUT /v1/roles/{roleId} returns the role's children duplicated 4x, with childCount inflated to match #37303) no longer inflates the tree count even on a plain rename.

No new correctness or security issues found in this pass. Findings 1–5 (the pre-merge blockers) are all resolved.
· branch issue-36930-follow-up

[AP2300]

Code review — 9 findings, ranked most-severe first. Correctness bugs first, cleanup/altitude at the end. The recurring theme is that the cross-role-race guard the PR added for saveToolGroups isn't lifted to a shared helper and reapplied uniformly across mutations.

Findings referenced against the current head (d2e386b).

---

1. updateRole / createRole skip the cross-role guard the PR added for saves

dot-roles.store.ts:837-840, :881-891

Both write selectedRole / selectedRoleId unconditionally after the await. saveToolGroups and the user grant/remove flows added selectedRoleId() === roleId guards for exactly this reason.

Scenario: admin edits role A and saves, then clicks role B before the PUT resolves. selectRole clears state and loads B. PUT for A resolves after: selectedRole is overwritten with A while B is highlighted; the header, canModifyRole computeds, and the Users/Tools tabs all now describe A. A subsequent save on that panel PUTs against role A.

Consider extracting a withSelectedRoleScope((roleId) => …) helper (or the switchMap-keyed-by-roleId pattern in a shared feature slice) so every mutation acquires the guard by construction.

---

2. collectAncestorChain fabricates a parent-less stub → silent under-reporting of inherited grants

dot-roles.tree-utils.ts:163-179

When the picked role isn't in the merged tree, findRoleInTree(...) ?? { id, name: id } produces a stub with no parent; the loop exits after zero iterations and the fan-out shrinks to a single direct-grants request.

Scenario: a search-selected role whose branch hasn't been merged into roles / searchResults, or a race with a roles: [] reset (post-createRole root reload). loadMembers / loadToolGroups fire only GET /v1/roles/{targetId}/{users,tools}, silently omitting every ancestor's grants. Users tab renders an incomplete member roster and the Tools tab misses inherited grants — admin trusts the UI and re-creates a redundant direct grant. No error surfaced.

If the caller doesn't know the parent, this should fail loudly (or fall back to a getById) rather than short-circuit.

---

3. createRole seeds selectedRole with the raw POST payload — edit-gate flags are absent

dot-roles.store.ts:797-841

The comment on L799 says POST /v1/roles returns Role.toMap(), not a RoleView; the comment on L833 then claims the response is "already a hydrated RoleView" and seeds selectedRole directly. The type is asserted with a cast + childCount: 0 spread — TS doesn't catch it. system, locked, editUsers, editPermissions, editLayouts are absent, and downstream computeds fall back to permissive defaults (?? true) until the next page reload.

Scenario: admin creates a role whose contract would forbid user grants (editUsers === false per template). The just-created payload lacks the flag; the Users tab renders enabled; adds are either 403'd (bad UX) or, if the BE is lenient on fresh roles, silently promoted.

Two consistent options: (a) drop the "seed from POST" optimization and fire a follow-up GET; (b) have createRole echo a hydrated RoleView on the BE (worth a Java-side follow-up in either case, since PUT already does).

---

4. removeUsersFromRole runs the optimistic patchState before the cross-role guard

[dot-role-users-tab.component.ts grant/remove path → dot-roles.store.ts around L1006]

The guard only gates the follow-up loadMembers; the optimistic filter has already written into members, which by then may be a different role's slice.

Scenario: admin bulk-removes u1,u2 from role A, then clicks role B (inherits u1) before the DELETE resolves. selectRole flips the pane to B; the optimistic filter strips u1 from members — now B's slice — and the guarded reload skips because selectedRoleId !== A. B's member table wrongly shows u1 missing until the admin re-selects B.

Move the optimistic write behind the same guard, or key it against a captured roleId snapshot rather than the current members.

---

5. $searching never resets on stale-token drop or the <3-char short-circuit

dot-roles-add.component.ts:183-216, [dot-roles-edit.component.ts #runSearch]

$searching.set(true) is set right before await; on token !== #searchToken the function returns without $searching.set(false). Same shape in the edit dialog.

Scenario: admin types "abc" then a fourth char before the promise resolves; #searchToken bumps, old promise resolves into the guard-return branch, $searching stays true for the rest of the dialog session — the parent-picker looks permanently busy.

try/finally { $searching.set(false); } around the await would fix both.

---

6. Partial fan-out failure hides successful ancestor rows behind a generic ERROR

[dot-roles.store.ts around L397-L401 in loadMembers / loadToolGroups result mapping]

A single ancestor request failing flips membersStatus (and the tool-groups equivalent) to 'ERROR' even after the other ancestors' rows are already merged into state — the template hides the partial table behind a generic error banner while httpErrorManager fires a toast per failed request.

Scenario: one of five inherited role fetches returns a transient 5xx. Four ancestors' users are in members; template renders "Could not load" instead of them. Admin retries by re-selecting, fan-out repeats and can hit the same intermittent failure.

Introduce a 'PARTIAL' status (or a verified: boolean on the slice) so the table renders alongside an inline "some ancestors couldn't be verified" warning.

---

7. updateRole reparent branch mistakes a missing parent in the response for detach-to-root

dot-roles.store.ts:876-891

nextParentId = updated.parent && updated.parent !== updated.id ? updated.parent : null. If the PUT response ever omits parent (defensive undefined, partial view, future serializer refactor), previousParentId === nextParentId is false and the code splices the node out of its parent and appends it as a new root. childCount on the true parent isn't decremented, so the tree also shows the wrong count until reload.

Contract-wise the BE encodes root as parent === role.id; parent === undefined isn't in that contract. If it arrives, fail closed (keep the node where it was) rather than silently reparenting.

---

8. _filter private-API workaround + treeSelect state duplicated across Add and Edit

dot-roles-add.component.ts:194,215, [dot-roles-edit.component.ts equivalent]

The PR description calls out this line as "the most upgrade-fragile line in this PR" — and it exists twice. The surrounding #expandedKeys / onNodeExpand / onNodeCollapse / #searchToken / #runSearch / #toTreeNodes triad is ~90% duplicated between the two dialogs.

Cost: the next PrimeNG bump that renames _filter or restructures treeViewChild breaks both dialogs; fixes will drift over time as one gets updates the other doesn't.

Extract a shared dot-role-parent-picker (or a directive that owns the workaround) so the fragile access lives behind one seam. The two dialogs then differ only in their initial value, submit call, and the descendant-exclude filter.

---

9. Tool-group toggle fires a full reconcile per click — debounce coalesces only the toast

[dot-role-tools-tab.component.ts onToggle → dot-roles.store.ts:693 saveToolGroups], :218 toolGroupsSaved$.pipe(debounceTime(...))

The 1.5s toolGroupsSaved$ debounce coalesces only the toast; each save still triggers a full catalog GET + N ancestor tool-group GETs. A burst of 5 checkbox clicks on a role with 3 ancestors is 5 POSTs + 5 catalog GETs + 15 ancestor GETs.

Coalesce the reconcile into the same debounce (queue the id-set of dirty groups, single reconcile after the burst). Under network jitter, out-of-order reconciles can also restamp state with a stale union — the guarded write only gates the last one to resolve, not the earlier stale ones.

---

Out-of-scope for this PR but worth capturing: dot-users-list.store.ts bulk-delete path has no aggregate toast when every deletion fails (only per-request errors). Behavior pre-dates this PR; noting here so it isn't lost.

Nice work overall — the consolidation into DotRolesService, the Tools-tab Angular migration, and the tree-node child-count / reparent handling read cleanly. Findings 1–4 are the ones I'd want addressed before merge; the rest can be follow-ups.

[hmoreras]

@AP2300 the mention options was fixed; except 7 and 9 since are in some way false positive