From 58eaf6bb2881363320d06bef198d67b7b8033993 Mon Sep 17 00:00:00 2001 From: Lars George Date: Thu, 21 May 2026 13:56:00 +0200 Subject: [PATCH] =?UTF-8?q?feat(directory):=20Phase=202=20=E2=80=94=20Role?= =?UTF-8?q?s=20+=20Entitlements=20+=20Comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacks on Phase 1 frontend (#407). Migrates the multi-principal RBAC surfaces to PrincipalPicker per plan #375. UI changes: - settings/role-form-dialog.tsx — assigned_groups text-with-commas Input replaced with PrincipalPicker(multi, groups). Legacy comma-separated strings still rehydrate via the new normaliseAssignedGroups helper. - views/entitlements.tsx — Persona group-assignment "add" Select with hard-coded demo list replaced with PrincipalPicker(multi, groups). The picker renders the current selection as its own badges, so the separate Badge row is gone. Backend PUT /api/entitlements/personas/{id}/groups accepts the same List[str] payload. - comments/comment-sidebar.tsx — Audience field gains a PrincipalPicker(multi, users+groups) alongside (not replacing) the existing Teams / App Roles checkbox lists. Picked users emit as plain emails, picked groups as plain group names; existing team:* / role:* tokens flow through unchanged. parseAudienceTokens + buildAudienceTokens helpers extracted for round-trip clarity. Helpers (pure functions, fully tested): - lib/audience.ts — parseAudienceTokens / buildAudienceTokens. - lib/assigned-groups.ts — normaliseAssignedGroups. Tests (12 new, 562 total passing): - lib/audience.test.ts (6): mixed audience round-trip, defensive guards (null / empty / non-string), empty-id passthrough for prefix-only tokens. - lib/assigned-groups.test.ts (6): array passthrough, comma-string split + trim, empty / nullish / non-string-non-array → [] fallback. Per-page form tests with the picker rendered inside Radix dialogs remain deferred to Playwright E2E (matches the existing convention in this repo: role-form-dialog.test, team-form-dialog.test, and project-form-dialog.test are all skipped for the same Radix-in-jsdom reason). The new helpers cover the load-bearing data transforms. No backend changes -- existing PUT/POST endpoints accept the same payload shapes they already did. --- .../components/comments/comment-sidebar.tsx | 73 +++++++++++------ .../components/settings/role-form-dialog.tsx | 21 +++-- src/frontend/src/lib/assigned-groups.test.ts | 46 +++++++++++ src/frontend/src/lib/assigned-groups.ts | 23 ++++++ src/frontend/src/lib/audience.test.ts | 79 +++++++++++++++++++ src/frontend/src/lib/audience.ts | 50 ++++++++++++ src/frontend/src/views/entitlements.tsx | 53 +++---------- 7 files changed, 265 insertions(+), 80 deletions(-) create mode 100644 src/frontend/src/lib/assigned-groups.test.ts create mode 100644 src/frontend/src/lib/assigned-groups.ts create mode 100644 src/frontend/src/lib/audience.test.ts create mode 100644 src/frontend/src/lib/audience.ts diff --git a/src/frontend/src/components/comments/comment-sidebar.tsx b/src/frontend/src/components/comments/comment-sidebar.tsx index b1d10627..36e0fd85 100644 --- a/src/frontend/src/components/comments/comment-sidebar.tsx +++ b/src/frontend/src/components/comments/comment-sidebar.tsx @@ -28,6 +28,8 @@ import { Avatar } from '@/components/ui/avatar'; import { RelativeDate } from '@/components/common/relative-date'; import { Separator } from '@/components/ui/separator'; import { ScrollArea } from '@/components/ui/scroll-area'; +import { PrincipalPicker } from '@/components/common/principal-picker'; +import { buildAudienceTokens, parseAudienceTokens } from '@/lib/audience'; // Select components removed - unused interface CommentFormData { @@ -35,6 +37,13 @@ interface CommentFormData { comment: string; selectedTeams: string[]; selectedRoles: string[]; + /** + * Additional audience tokens collected from the PrincipalPicker. + * These are plain emails (users) and plain group names (groups) -- + * NOT prefixed with ``team:`` / ``role:``. They merge with the + * checkbox selections at submit time. + */ + selectedPrincipals: string[]; } interface TimelineEntry { @@ -83,6 +92,7 @@ const CommentSidebar: React.FC = ({ comment: '', selectedTeams: [], selectedRoles: [], + selectedPrincipals: [], }); // Ref for the ScrollArea viewport to control scrolling @@ -195,11 +205,14 @@ const CommentSidebar: React.FC = ({ return; } - // Build audience array with team: and role: prefixes - const audienceTokens: string[] = [ - ...formData.selectedTeams.map(teamId => `team:${teamId}`), - ...formData.selectedRoles.map(roleName => `role:${roleName}`), - ]; + // Build audience array. ``team:`` / ``role:`` prefixes come from + // the checkbox lists; plain emails / group names come from the + // PrincipalPicker (Directory pick) and flow through unchanged. + const audienceTokens: string[] = buildAudienceTokens({ + teams: formData.selectedTeams, + roles: formData.selectedRoles, + principals: formData.selectedPrincipals, + }); const commentData: CommentCreate | CommentUpdate = { title: formData.title || null, @@ -259,7 +272,7 @@ const CommentSidebar: React.FC = ({ } // Reset form and refresh timeline - setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [] }); + setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [], selectedPrincipals: [] }); setEditingComment(null); setIsFormOpen(false); await fetchTimeline(); @@ -302,31 +315,23 @@ const CommentSidebar: React.FC = ({ const handleEdit = (comment: Comment) => { setEditingComment(comment); - // Parse audience tokens to extract teams and roles - const teams: string[] = []; - const roles: string[] = []; - - if (comment.audience) { - comment.audience.forEach(token => { - if (token.startsWith('team:')) { - teams.push(token.substring(5)); - } else if (token.startsWith('role:')) { - roles.push(token.substring(5)); - } - }); - } - + // Parse audience tokens. ``team:`` / ``role:`` go into the + // respective checkbox state; anything else is a Directory + // principal (plain email or group name) and rehydrates the + // PrincipalPicker. + const parsed = parseAudienceTokens(comment.audience); setFormData({ title: comment.title || '', comment: comment.comment, - selectedTeams: teams, - selectedRoles: roles, + selectedTeams: parsed.teams, + selectedRoles: parsed.roles, + selectedPrincipals: parsed.principals, }); setIsFormOpen(true); }; const resetForm = () => { - setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [] }); + setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [], selectedPrincipals: [] }); setEditingComment(null); setIsFormOpen(false); }; @@ -458,7 +463,27 @@ const CommentSidebar: React.FC = ({ ))} - + + {/* Directory principals (users / groups). Sits alongside the + team / role checkbox lists -- picked values flow into the + same ``audience`` field as plain emails / group names. */} +
+ +
+ + setFormData({ ...formData, selectedPrincipals: next }) + } + placeholder="Add users or groups…" + aria-label="Audience users and groups" + /> +
+
+ {/* Display selected teams and roles */} {(formData.selectedTeams.length > 0 || formData.selectedRoles.length > 0) && (
diff --git a/src/frontend/src/components/settings/role-form-dialog.tsx b/src/frontend/src/components/settings/role-form-dialog.tsx index 6a8b654d..45425e0a 100644 --- a/src/frontend/src/components/settings/role-form-dialog.tsx +++ b/src/frontend/src/components/settings/role-form-dialog.tsx @@ -18,6 +18,8 @@ import { ACCESS_LEVEL_ORDER } from '../../lib/permissions'; import { features as orderedFeatures } from '@/config/features'; // Import the ordered features import { usePermissions } from '@/stores/permissions-store'; import { Checkbox } from '@/components/ui/checkbox'; +import { PrincipalPicker } from '@/components/common/principal-picker'; +import { normaliseAssignedGroups } from '@/lib/assigned-groups'; interface RoleFormDialogProps { isOpen: boolean; @@ -324,21 +326,16 @@ const RoleFormDialog: React.FC = ({ name="assigned_groups" control={control} render={({ field }) => { - // Convert array to string for display, keep as string for editing - const displayValue = Array.isArray(field.value) - ? field.value.join(', ') - : (field.value || ''); - + const value = normaliseAssignedGroups(field.value); return ( - field.onChange(next)} placeholder={t('roles.general.assignedGroupsPlaceholder')} - value={displayValue} - onChange={(e) => { - // Store as string, will be split on submit - field.onChange(e.target.value); - }} - className="focus:ring-2 focus:ring-offset-2" + aria-label={t('roles.general.assignedGroups')} /> ); }} diff --git a/src/frontend/src/lib/assigned-groups.test.ts b/src/frontend/src/lib/assigned-groups.test.ts new file mode 100644 index 00000000..c07a043b --- /dev/null +++ b/src/frontend/src/lib/assigned-groups.test.ts @@ -0,0 +1,46 @@ +/** + * Tests for AppRole.assigned_groups normalisation. + * + * Covers the Phase-2 acceptance criterion: pre-existing Roles + * ``assigned_groups`` strings still render and save without UI breakage. + */ + +import { describe, expect, it } from 'vitest'; + +import { normaliseAssignedGroups } from './assigned-groups'; + +describe('normaliseAssignedGroups', () => { + it('returns an array unchanged when already an array of strings', () => { + expect(normaliseAssignedGroups(['data-producers', 'data-stewards'])).toEqual([ + 'data-producers', + 'data-stewards', + ]); + }); + + it('drops empty strings from an array', () => { + expect(normaliseAssignedGroups(['a', '', 'b'])).toEqual(['a', 'b']); + }); + + it('splits a legacy comma-separated string and trims whitespace', () => { + expect(normaliseAssignedGroups('data-producers, data-stewards , compliance')).toEqual([ + 'data-producers', + 'data-stewards', + 'compliance', + ]); + }); + + it('drops trailing / repeated commas', () => { + expect(normaliseAssignedGroups('a,,b,')).toEqual(['a', 'b']); + }); + + it('returns [] for nullish / non-string-non-array inputs', () => { + expect(normaliseAssignedGroups(null)).toEqual([]); + expect(normaliseAssignedGroups(undefined)).toEqual([]); + expect(normaliseAssignedGroups(42)).toEqual([]); + expect(normaliseAssignedGroups({})).toEqual([]); + }); + + it('returns [] for an empty string', () => { + expect(normaliseAssignedGroups('')).toEqual([]); + }); +}); diff --git a/src/frontend/src/lib/assigned-groups.ts b/src/frontend/src/lib/assigned-groups.ts new file mode 100644 index 00000000..ad8b4117 --- /dev/null +++ b/src/frontend/src/lib/assigned-groups.ts @@ -0,0 +1,23 @@ +/** + * Helper for normalising the AppRole ``assigned_groups`` field. + * + * Historically this field was edited as a comma-separated text input + * and persisted as ``string[]``. With the Directory PrincipalPicker + * the field is always ``string[]``, but old role records loaded from + * the API (or other code paths) may still surface a raw comma string. + * This helper makes the picker tolerant of both shapes without + * leaking the conditional throughout the form. + */ + +export function normaliseAssignedGroups(raw: unknown): string[] { + if (Array.isArray(raw)) { + return (raw as unknown[]).filter((x): x is string => typeof x === 'string' && x.length > 0); + } + if (typeof raw === 'string' && raw.length > 0) { + return raw + .split(',') + .map((g) => g.trim()) + .filter(Boolean); + } + return []; +} diff --git a/src/frontend/src/lib/audience.test.ts b/src/frontend/src/lib/audience.test.ts new file mode 100644 index 00000000..3c4341a7 --- /dev/null +++ b/src/frontend/src/lib/audience.test.ts @@ -0,0 +1,79 @@ +/** + * Tests for the audience-token round-trip used by the comment + * composer. These cover the Phase-2 acceptance criterion: + * "an audience containing a mix of users, groups, teams (team:*), and + * roles (role:*) round-trips correctly". + */ + +import { describe, expect, it } from 'vitest'; + +import { buildAudienceTokens, parseAudienceTokens } from './audience'; + +describe('parseAudienceTokens', () => { + it('splits team:, role:, and bare principal tokens', () => { + const parsed = parseAudienceTokens([ + 'team:t-1', + 'role:Admin', + 'alice@x.com', + 'Data Producers', + ]); + expect(parsed.teams).toEqual(['t-1']); + expect(parsed.roles).toEqual(['Admin']); + expect(parsed.principals).toEqual(['alice@x.com', 'Data Producers']); + }); + + it('returns empty arrays for null / undefined / empty input', () => { + expect(parseAudienceTokens(null)).toEqual({ teams: [], roles: [], principals: [] }); + expect(parseAudienceTokens(undefined)).toEqual({ teams: [], roles: [], principals: [] }); + expect(parseAudienceTokens([])).toEqual({ teams: [], roles: [], principals: [] }); + }); + + it('skips non-string and empty tokens', () => { + const parsed = parseAudienceTokens([ + 'team:t-1', + '', + // @ts-expect-error verify defensive guard + null, + 'alice@x.com', + ]); + expect(parsed.teams).toEqual(['t-1']); + expect(parsed.principals).toEqual(['alice@x.com']); + }); + + it('treats prefix-only tokens as the empty id behind the prefix', () => { + const parsed = parseAudienceTokens(['team:', 'role:']); + // Empty ids are preserved -- the caller decides whether to filter. + expect(parsed.teams).toEqual(['']); + expect(parsed.roles).toEqual(['']); + expect(parsed.principals).toEqual([]); + }); +}); + +describe('buildAudienceTokens', () => { + it('emits team: / role: prefixes for checkbox selections, principals bare', () => { + const tokens = buildAudienceTokens({ + teams: ['t-1', 't-2'], + roles: ['Admin'], + principals: ['alice@x.com', 'Data Producers'], + }); + expect(tokens).toEqual([ + 'team:t-1', + 'team:t-2', + 'role:Admin', + 'alice@x.com', + 'Data Producers', + ]); + }); + + it('round-trips a mixed audience without loss', () => { + const original = [ + 'team:t-7', + 'role:Reviewer', + 'bob@x.com', + 'Engineering', + ]; + const parsed = parseAudienceTokens(original); + const rebuilt = buildAudienceTokens(parsed); + expect(rebuilt).toEqual(original); + }); +}); diff --git a/src/frontend/src/lib/audience.ts b/src/frontend/src/lib/audience.ts new file mode 100644 index 00000000..8eed0f35 --- /dev/null +++ b/src/frontend/src/lib/audience.ts @@ -0,0 +1,50 @@ +/** + * Audience token helpers for the comment composer. + * + * Audience tokens flow on the wire as a single ``string[]`` mixing + * three shapes: + * + * - ``team:`` — selected from the Teams checkbox list. + * - ``role:`` — selected from the App Roles checkbox list. + * - anything else — a Directory principal (plain user email/UPN or + * plain group display name) picked via the PrincipalPicker. + * + * These helpers keep the round-trip (compose -> store -> edit) honest + * across the three shapes and are pure functions so they can be unit + * tested without rendering Radix UI. + */ + +export interface ParsedAudience { + teams: string[]; + roles: string[]; + principals: string[]; +} + +const TEAM_PREFIX = 'team:'; +const ROLE_PREFIX = 'role:'; + +export function parseAudienceTokens( + audience: string[] | null | undefined, +): ParsedAudience { + const out: ParsedAudience = { teams: [], roles: [], principals: [] }; + if (!audience) return out; + for (const token of audience) { + if (typeof token !== 'string' || token.length === 0) continue; + if (token.startsWith(TEAM_PREFIX)) { + out.teams.push(token.slice(TEAM_PREFIX.length)); + } else if (token.startsWith(ROLE_PREFIX)) { + out.roles.push(token.slice(ROLE_PREFIX.length)); + } else { + out.principals.push(token); + } + } + return out; +} + +export function buildAudienceTokens(parsed: ParsedAudience): string[] { + return [ + ...parsed.teams.map((t) => `${TEAM_PREFIX}${t}`), + ...parsed.roles.map((r) => `${ROLE_PREFIX}${r}`), + ...parsed.principals, + ]; +} diff --git a/src/frontend/src/views/entitlements.tsx b/src/frontend/src/views/entitlements.tsx index 09be87bd..57064248 100644 --- a/src/frontend/src/views/entitlements.tsx +++ b/src/frontend/src/views/entitlements.tsx @@ -13,6 +13,7 @@ import { Tabs, TabsContent, TabsList, TabsTrigger } from '@/components/ui/tabs'; import { ScrollArea } from '@/components/ui/scroll-area'; import { useToast } from '@/hooks/use-toast'; import useBreadcrumbStore from '@/stores/breadcrumb-store'; +import { PrincipalPicker } from '@/components/common/principal-picker'; interface Privilege { securable_id: string; @@ -49,23 +50,6 @@ const Entitlements: React.FC = () => { const setStaticSegments = useBreadcrumbStore((state) => state.setStaticSegments); const setDynamicTitle = useBreadcrumbStore((state) => state.setDynamicTitle); - const availableGroups = [ - 'data_science_team', - 'ml_engineers', - 'data_engineering', - 'etl_team', - 'business_analysts', - 'reporting_team', - 'data_governance', - 'data_stewards', - 'compliance_team', - 'business_users', - 'report_viewers', - 'model_developers', - 'pipeline_operators', - 'analytics_users' - ]; - useEffect(() => { fetchPersonas(); setStaticSegments([]); @@ -473,35 +457,16 @@ const Entitlements: React.FC = () => {

{t('entitlements:personas.groupAssignments')}

-
- {selectedPersona.groups.map((group) => ( - - {group} - - ))} -
- + handleUpdateGroups(selectedPersona.id, next)} + placeholder={t('common:placeholders.selectGroup')} + aria-label={t('entitlements:personas.groupAssignments')} + />