feat(users): Roles tab wired to real endpoints (#36718) - #37082
Conversation
Ships the Users portlet Create/Edit dialog shell with the Profile tab fully wired and placeholders for the three sibling tabs (delivered by #36718, #36719, #36720). Dialog / Profile tab: - 4-tab strip with Profile as the only functional tab; Roles, Permissions, and API Tokens render "Coming soon" placeholders - Header with avatar + name + Active status chip - Account section: first/last name, email, password + confirm, Active - Additional Info section: prefix/suffix/title/company/website - Access section: disabled (values informational only), shows admin / backend / frontend / hasConsoleAccess pulled from the loaded user - Delete User section (edit mode) with required replacement-user picker and email-typed confirmation List CRUD: - DotUsersService gains getUser/createUser/updateUser using POST/PUT/GET /api/v1/users; roles field intentionally omitted on update so backend preserves role membership (see UserResource#processRoles) - DotUsersListStore gains createUser / updateUser / deleteSingleUser - Bulk delete on the list toolbar now shows the same replacement picker instead of the old p-confirmDialog - /users route now resolves to the new users-beta portlet id Shared: - DotUsersReplacementPickerComponent — server-backed autocomplete used by both delete flows; excludes the users being deleted from suggestions client-side Test coverage: 56 tests in the portlet + 46 in data-access all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the Roles tab to the Create/Edit User dialog. Dual-panel shuttle picker (Available tree on the left, Granted list on the right) with independent filters and Grant/Revoke arrows. Currently backed by a mock catalog in dot-users-roles.data.ts — wiring against `GET /api/v1/roles` and the "user's assigned roles" source of truth is a follow-up decision (see the parent ticket discussion around DWR vs a small REST addition). Also swaps the placeholder tabpanel from #36717 for the real component, and adds the tab-specific i18n keys. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the mock catalog with real backend data and lifts state up
so the shell's save payload actually carries the granted role list.
Service:
- New `getUserRoles(userIdOrEmail)` — user's currently assigned roles
via GET /api/v1/roles/users/{userIdOrEmail}.
- New `getAllRoles()` — full system role tree. Uses
/api/v1/roles?loadChildrenRoles=true for roots + immediate children,
then recursively fetches /api/v1/roles/{id}?loadChildrenRoles=true
per non-root discovered so far until a round yields no new roles.
Bounded by the number of non-root roles in the system. `_search`
would be simpler but returns SmallRoleView (no parent, no hierarchy)
and can't be reconstructed into a client-side tree.
- New `DotRoleView` interface + `flattenRoleTree` no longer needed —
the recursive walker flattens on the fly and sets `parent` on every
descendant.
Roles tab:
- Deletes the visual-only `dot-users-roles.data.ts` mock catalog.
- `initialGrantedKeys: string[]` input + `grantedChange: string[]`
output. An effect seeds `granted` from the first non-empty input
value; later parent mutations don't clobber in-flight edits.
- Drops the synthetic System/Custom bucketing — root roles
(`System`, `Categories`, `Intranet`, `Publisher / Legal`) are the
natural top-level nodes and their children indent underneath.
- Bulk-select via parent checkbox: checking a parent selects every
grantable-leaf descendant in one action; indeterminate state when
some are selected; unchecking clears them all. Precomputed
`grantableLeavesByRole` keeps this O(1) per row.
- Grantable = leaf only (parents excluded so their checkbox is a
bulk-select shortcut, not a role grant). `roleKey` is preferred
when the backend has one; `role.id` used as fallback so legacy /
manually-imported keyless roles still work in the shuttle.
- Root rows get extra top spacing and a bold, slightly larger label
for visual hierarchy.
Shell:
- `loadUserDetail` now forkJoins getUser + getUserRoles. Roles fetch
catches errors to [] so a failed lookup doesn't block profile
hydration.
- New signals: `initialGrantedRoleKeys` (down to the tab) and
`currentRoleKeys` (mirror updated on every grantedChange). Both
seeded from the initial fetch so save preserves membership even
when the user never opens the Roles tab.
- `buildPayload` sends `payload.roles` only when the current key
list is non-empty — an empty array on PUT would wipe membership
(processRoles calls removeRoles then re-adds), a missing field
is the safer "leave untouched" signal.
Test coverage: 56 tests + lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop the user's personal role (roleKey === userId) from the save payload so `UserResource.processRoles` no longer trips its `Cannot alter users on this role` guard on editUsers=false roles. - Prune parents from the Available tree once every grantable descendant has been moved to Granted, so an empty root can no longer linger as a dead container. Workflow-only branches with zero grantable descendants (Publisher / Legal) are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
Area : Frontend
|
|
||
| const roleKeys = roles | ||
| .map((role) => role.roleKey) | ||
| .filter((key): key is string => !!key); |
There was a problem hiding this comment.
Dropping keyless roles here means they're invisible in the Granted panel and revoked on save. roleKey is optional — roles created from the Roles portlet often have none (dot-roles-add.component.ts defaults it to ''). The backend's processRoles wipes every direct role and then re-adds only the keys we send, so a role that never made it into this list is gone after a save the admin thought was a no-op on that role.
The identity we send has to be able to name a keyless role, otherwise the Roles tab can't be safe. Worth pairing with the grantIdentifier thread in the tab.
| * so the shell can still preserve/save the assignment. | ||
| */ | ||
| private grantIdentifier(role: RoleOption): string { | ||
| return role.roleKey || role.id; |
There was a problem hiding this comment.
Falling back to role.id doesn't reach the backend: loadRoleByKey runs from Role where role_key = ?, so a UUID resolves to null and UserHelper.addRole logs "does NOT exist… Ignoring it" and skips. Because processRoles has already wiped every role by then, the grant just doesn't happen.
So the note above is right about the mechanism but reads too gently — the user moves a keyless role across, saves, reopens, and it isn't there. That's test-plan item 2 failing for exactly the roles that most need this tab.
| function toRoleOption(role: DotRoleView): RoleOption { | ||
| return { | ||
| id: role.id, | ||
| roleKey: (role.roleKey ?? '').trim(), |
There was a problem hiding this comment.
editUsers is on DotRoleView but dropped here, and isGrantableLeaf only asks "does this role have children" — so the claim up at the grantableLeavesByRole doc ("non-user-assignable roles contribute nothing to the map") isn't actually implemented anywhere.
Consequence: any editUsers=false leaf gets a checkbox and can be moved across, and addRoleToUser throws Cannot alter users on this role on save — the whole PUT is transactional, so the profile edit rolls back too. Carrying editUsers through into RoleOption and folding it into isGrantableLeaf makes the comment true.
| {{ 'users.dialog.roles.granted.empty' | dm }} | ||
| </p> | ||
| } @else { | ||
| @for (role of grantedList(); track role.roleKey) { |
There was a problem hiding this comment.
track role.roleKey is empty for keyless roles (toRoleOption normalises them to ''), so granting two of them gives @for duplicate keys — Angular flags that (NG0955) and the rows can render wrong. role.id is unique and already on the shape. Same for the data-testid a few lines down.
| const outbound = personalRoleKey | ||
| ? roleKeys.filter((key) => key !== personalRoleKey) | ||
| : roleKeys; | ||
| if (outbound.length > 0) { |
There was a problem hiding this comment.
Revoking everything is a silent no-op: outbound is empty so roles is left off the payload, and the backend treats a missing (or empty) list as "don't touch". The admin clears the Granted panel, saves, gets a success toast, reopens, and every role is back.
Given the backend can't express "remove all" through this field, the honest frontend move is to stop the save and say so — something like blocking submit while Granted is empty rather than shipping a request that quietly does nothing.
| protected readonly canRevoke = computed(() => this.selectedGranted().length > 0); | ||
|
|
||
| constructor() { | ||
| this.loadRoles(); |
There was a problem hiding this comment.
loadRoles() in the constructor means the fan-out fires whenever the dialog opens, not when someone visits the Roles tab — the shell renders all four p-tabpanels eagerly, so editing a name pays for the whole role tree too.
Worth noting the fan-out itself is unavoidable: loadChildrenRoles=true only returns one level (each child comes back with an empty children list), so walking the depth per node is the only option. Gating the load on tab activation is the part that's in our hands.
| protected readonly availableFilter = signal(''); | ||
| protected readonly grantedFilter = signal(''); | ||
| protected readonly collapsed = signal<Record<string, boolean>>({}); | ||
| protected readonly isLoading = signal(false); |
There was a problem hiding this comment.
isLoading never gets read in the template, so while the tree is being walked the Available panel shows the "no roles" empty message instead — a slow load and a genuinely empty tree look the same. Same for the error path: httpErrorManager.handle fires but the panel still reads as empty.
| ], | ||
| templateUrl: './dot-users-roles-tab.component.html', | ||
| styleUrl: './dot-users-roles-tab.component.scss', | ||
| changeDetection: ChangeDetectionStrategy.OnPush, |
There was a problem hiding this comment.
changeDetection can go (OnPush is the framework default now) and standalone: true is implied. The signals want the $ prefix and the injected deps # instead of private — dot-users-filter-by in this portlet already does all three. Also host: 'flex flex-col gap-4 block' sets display twice; drop block.
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| host: { class: 'flex flex-col gap-4 block' } | ||
| }) | ||
| export class DotUsersRolesTabComponent { |
There was a problem hiding this comment.
488 lines and no spec. The tree logic is the part that really wants one — grantableLeavesByRole memoisation, isPartiallyChecked on a multi-level parent, and the pruning in availableTree (a root disappearing once its last grantable leaf moves across) are all easy to break and impossible to eyeball.
| (click)="toggleGrantedSelection(role.roleKey)" | ||
| [attr.data-testid]="'users-roles-granted-item-' + role.roleKey"> | ||
| <span | ||
| class="material-symbols-rounded text-base! text-surface-500" |
There was a problem hiding this comment.
text-base! — three !importants in this file with no reason beside them. A short comment if the Material Symbols sizing needs it, otherwise plain text-base should hold.
2 participants
Closes #36718
Depends on #36990 (Profile tab shell). This PR's diff currently includes the shell commits; once #36990 is merged to main, this branch will be rebased onto the new main and the diff will show only the Roles tab changes.
Summary
GET /api/v1/roles?loadChildrenRoles=true+ per-node fan-out on non-root roles. Parent nodes get a bulk-select checkbox that toggles every grantable leaf underneath.GET /api/v1/roles/users/{userId}and refreshed on every move; the parent dialog picks the change up viagrantedChangeand folds it intoPUT /api/v1/usersas therolesfield.Notable non-obvious calls
roleKey === userId,editUsers=false). The backend's `UserResource#processRoles` calls `removeAllRolesFromUser` then loops `addRoleToUser`; the personal role trips the `editUsers` guard and rolls the save back with "Cannot alter users on this role". Filtering it in the outbound payload sidesteps the guard. Proper fix belongs on the backend.Test plan
🤖 Generated with Claude Code