From bfc48b1420031a82799c3f687876a1a596863fd6 Mon Sep 17 00:00:00 2001 From: m Date: Wed, 6 May 2026 17:16:17 +0200 Subject: [PATCH] fix(t-paliad-143): derived team members all show 'Attorney' + Herkunft collapses multi-unit users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related bugs on /projects/{id} Team tab → "Abgeleitet (Partner Unit)": 1. **All derived members labeled 'Attorney'.** Migration 055 added partner_unit_members.unit_role with DEFAULT 'attorney' but never exposed the column in the admin UI. So 100% of pum rows are 'attorney' and Siemens AG's derive_unit_roles=['pa','senior_pa','attorney'] config surfaces every member as 'attorney' even when they're really PAs. 2. **Multi-unit users collapsed to one source.** ListDerivedMembers used ROW_NUMBER() OVER (PARTITION BY user_id) WHERE rn=1 — closest-attachment wins, every other unit-membership dropped. Judith Molarinho Vaz + Sabrina Franken belong to BOTH Lehment AND Plassmann; UI showed only one. **Backend** (internal/services/derivation_service.go): - DerivedMember.Memberships []DerivedMembership replaces scalar UnitID/UnitName/UnitRole. DeriveGrantsAuthority becomes bool_or across all source attachments (any granting → true). - ListDerivedMembers SQL: jsonb_agg(DISTINCT jsonb_build_object(...)) + bool_or(derive_grants_authority), GROUP BY user. One row per user, every (unit, role) pair preserved. Memberships sorted by unit_name in Go (PG doesn't allow ORDER BY inside DISTINCT-aggregated jsonb_agg). - DerivedMembershipList implements sql.Scanner so the jsonb column maps directly into the Go struct. Pinned by unit test. **Frontend** (projects-detail.ts): - DerivedMember interface mirrors the new shape. Herkunft renders every (unit, role) source — single-unit users render as before ("über: **Lehment** [Sicht]"); multi-unit users render "über: **Lehment** (Attorney), **Plassmann** (PA) [Sicht & 4-Augen]". - Role column shows distinct unit_role values. **Frontend** (admin-partner-units.ts): - Member modal gains a per-row ${roleOptions} + + + `; + }) .join(""); list.querySelectorAll(".pu-remove-btn").forEach((b) => b.addEventListener("click", () => removeMember(b.dataset.user!)), ); + list.querySelectorAll(".pu-role-select").forEach((s) => + s.addEventListener("change", () => setMemberRole(s.dataset.user!, s.value, s)), + ); +} + +async function setMemberRole(userID: string, role: string, sel: HTMLSelectElement): Promise { + if (!activeUnitID) return; + // Snapshot the prior selection so we can roll back on failure. + const u = units.find((x) => x.id === activeUnitID); + const prior = u?.members.find((m) => m.user_id === userID)?.unit_role; + sel.disabled = true; + const resp = await fetch( + `/api/partner-units/${activeUnitID}/members/${userID}/role`, + { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ unit_role: role }), + }, + ); + sel.disabled = false; + if (!resp.ok) { + if (prior !== undefined) sel.value = prior; + const body = await resp.json().catch(() => ({ error: resp.statusText })); + showFeedback(body.error || "Rolle konnte nicht gespeichert werden.", true); + return; + } + await loadUnits(); + renderMemberList(); + render(); + showFeedback(tDyn("admin.partner_units.feedback.role_updated") || "Rolle aktualisiert.", false); } function wireSuggestions(): void { diff --git a/frontend/src/client/i18n.ts b/frontend/src/client/i18n.ts index 4d9a1f0..d13f9f5 100644 --- a/frontend/src/client/i18n.ts +++ b/frontend/src/client/i18n.ts @@ -1544,6 +1544,7 @@ const translations: Record> = { "admin.partner_units.feedback.created": "Angelegt.", "admin.partner_units.feedback.updated": "Aktualisiert.", "admin.partner_units.feedback.deleted": "Gelöscht.", + "admin.partner_units.feedback.role_updated": "Rolle aktualisiert.", "admin.partner_units.member.heading": "Mitglieder verwalten", "admin.partner_units.member.empty": "Noch keine Mitglieder.", "admin.partner_units.member.add": "Mitglied hinzufügen", @@ -1551,6 +1552,7 @@ const translations: Record> = { "admin.partner_units.member.remove": "Entfernen", "admin.partner_units.member.confirm_remove": "Mitglied entfernen?", "admin.partner_units.member.placeholder": "Name oder E-Mail", + "admin.partner_units.member.role": "Rolle", "admin.audit.loading": "Lade…", "admin.audit.empty": "Keine Ereignisse für die gewählten Filter.", "admin.audit.loadmore": "Weitere laden", @@ -3255,6 +3257,7 @@ const translations: Record> = { "admin.partner_units.feedback.created": "Created.", "admin.partner_units.feedback.updated": "Updated.", "admin.partner_units.feedback.deleted": "Deleted.", + "admin.partner_units.feedback.role_updated": "Role updated.", "admin.partner_units.member.heading": "Manage members", "admin.partner_units.member.empty": "No members yet.", "admin.partner_units.member.add": "Add member", @@ -3262,6 +3265,7 @@ const translations: Record> = { "admin.partner_units.member.remove": "Remove", "admin.partner_units.member.confirm_remove": "Remove member?", "admin.partner_units.member.placeholder": "Name or email", + "admin.partner_units.member.role": "Role", "admin.audit.loading": "Loading…", "admin.audit.empty": "No events match the selected filters.", "admin.audit.loadmore": "Load more", diff --git a/frontend/src/client/projects-detail.ts b/frontend/src/client/projects-detail.ts index 06681a6..1590ba9 100644 --- a/frontend/src/client/projects-detail.ts +++ b/frontend/src/client/projects-detail.ts @@ -47,14 +47,21 @@ interface ProjectTeamMember { } // t-paliad-139 — derived team member from a partner-unit attachment. +// One DerivedMember per user; users in multiple attached units carry one +// DerivedMembership per (unit, role) pair so the Herkunft column can list +// every source (t-paliad-143). +interface DerivedMembership { + unit_id: string; + unit_name: string; + unit_role: string; +} + interface DerivedMember { user_id: string; user_email: string; user_display_name: string; user_office: string; - unit_role: string; - unit_id: string; - unit_name: string; + memberships: DerivedMembership[]; derive_grants_authority: boolean; } @@ -1736,7 +1743,25 @@ function renderDerivedMembers() { section.style.display = ""; body.innerHTML = derivedMembers .map((m) => { - const roleLabel = tDyn(`unit_role.${m.unit_role}`) || m.unit_role; + const memberships = m.memberships || []; + // Role column shows distinct unit_role values (usually one — only + // diverges if the user has different roles in different units). + const distinctRoles = Array.from(new Set(memberships.map((x) => x.unit_role))); + const roleLabel = distinctRoles + .map((r) => tDyn(`unit_role.${r}`) || r) + .join(", "); + // Herkunft column lists every (unit, role) pair so multi-unit users + // surface all their sources, not just the closest one (t-paliad-143). + // Multi-unit: bold each unit name and append the role in parentheses. + // Single-unit: bold the one unit name (matches the legacy rendering). + const sourceLabel = memberships + .map((x) => { + const name = `${esc(x.unit_name)}`; + if (memberships.length === 1) return name; + const role = esc(tDyn(`unit_role.${x.unit_role}`) || x.unit_role); + return `${name} (${role})`; + }) + .join(", "); const officeLabel = m.user_office ? tDyn("office." + m.user_office) || m.user_office : ""; const authBadge = m.derive_grants_authority ? `${esc(t("projects.team.derived.authority") || "Sicht & 4-Augen")}` @@ -1745,7 +1770,7 @@ function renderDerivedMembers() { ${esc(m.user_display_name || m.user_email)} · ${esc(m.user_email)}${officeLabel ? " · " + esc(officeLabel) : ""} ${esc(roleLabel)} - ${esc(t("projects.team.derived.from") || "über")}: ${esc(m.unit_name)} ${authBadge} + ${esc(t("projects.team.derived.from") || "über")}: ${sourceLabel} ${authBadge} `; }) .join(""); diff --git a/frontend/src/i18n-keys.ts b/frontend/src/i18n-keys.ts index 9c0ee6f..bd28f03 100644 --- a/frontend/src/i18n-keys.ts +++ b/frontend/src/i18n-keys.ts @@ -167,6 +167,7 @@ export type I18nKey = | "admin.partner_units.error.user_required" | "admin.partner_units.feedback.created" | "admin.partner_units.feedback.deleted" + | "admin.partner_units.feedback.role_updated" | "admin.partner_units.feedback.updated" | "admin.partner_units.heading" | "admin.partner_units.loading" @@ -177,6 +178,7 @@ export type I18nKey = | "admin.partner_units.member.heading" | "admin.partner_units.member.placeholder" | "admin.partner_units.member.remove" + | "admin.partner_units.member.role" | "admin.partner_units.new" | "admin.partner_units.new.heading" | "admin.partner_units.subtitle" diff --git a/frontend/src/styles/global.css b/frontend/src/styles/global.css index 88a3768..6df425e 100644 --- a/frontend/src/styles/global.css +++ b/frontend/src/styles/global.css @@ -8911,6 +8911,44 @@ dialog.quick-add-sheet::backdrop { background: var(--color-bg-lime-tint); } +/* /admin/partner-units member modal — list of (display_name, role-select, + remove) rows. The role-select is wired to PATCH …/members/{user}/role + (t-paliad-143). */ +.partner-unit-member-list { + list-style: none; + margin: 0 0 1rem 0; + padding: 0; +} + +.partner-unit-member-item { + display: flex; + align-items: center; + justify-content: space-between; + gap: 0.75rem; + padding: 0.4rem 0; + border-bottom: 1px solid var(--color-border); +} + +.partner-unit-member-item:last-child { + border-bottom: none; +} + +.partner-unit-member-actions { + display: flex; + align-items: center; + gap: 0.5rem; + flex-shrink: 0; +} + +.pu-role-select { + padding: 0.25rem 0.4rem; + font-size: 0.82rem; + border: 1px solid var(--color-border); + border-radius: var(--radius); + background: var(--color-surface); + color: var(--color-text); +} + .admin-team-input { width: 100%; padding: 0.3rem 0.45rem; diff --git a/internal/services/derivation_membership_scan_test.go b/internal/services/derivation_membership_scan_test.go new file mode 100644 index 0000000..d7ba1aa --- /dev/null +++ b/internal/services/derivation_membership_scan_test.go @@ -0,0 +1,71 @@ +package services + +import ( + "testing" + + "github.com/google/uuid" +) + +// TestDerivedMembershipListScan covers the sql.Scanner over a Postgres +// jsonb column — the wire format that ListDerivedMembers' jsonb_agg +// returns. Pinned because if a future migration changes the JSON shape +// (e.g. drops a key), the rendered Herkunft column on /projects/{id} +// silently breaks (t-paliad-143). +func TestDerivedMembershipListScan(t *testing.T) { + unitA := uuid.New() + unitB := uuid.New() + + cases := []struct { + name string + src any + want []DerivedMembership + }{ + { + name: "nil", + src: nil, + want: nil, + }, + { + name: "single membership as bytes", + src: []byte(`[{"unit_id":"` + unitA.String() + `","unit_name":"Lehment","unit_role":"attorney"}]`), + want: []DerivedMembership{{UnitID: unitA, UnitName: "Lehment", UnitRole: "attorney"}}, + }, + { + name: "two memberships as string", + src: `[ + {"unit_id":"` + unitA.String() + `","unit_name":"Lehment","unit_role":"attorney"}, + {"unit_id":"` + unitB.String() + `","unit_name":"Plassmann","unit_role":"pa"} + ]`, + want: []DerivedMembership{ + {UnitID: unitA, UnitName: "Lehment", UnitRole: "attorney"}, + {UnitID: unitB, UnitName: "Plassmann", UnitRole: "pa"}, + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var got DerivedMembershipList + if err := got.Scan(tc.src); err != nil { + t.Fatalf("Scan: %v", err) + } + if len(got) != len(tc.want) { + t.Fatalf("len: got %d want %d", len(got), len(tc.want)) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("row %d: got %+v want %+v", i, got[i], tc.want[i]) + } + } + }) + } +} + +// TestDerivedMembershipListScanRejectsUnknown ensures we don't silently +// accept random column types and produce an empty list (which would mask +// a schema regression). +func TestDerivedMembershipListScanRejectsUnknown(t *testing.T) { + var l DerivedMembershipList + if err := l.Scan(123); err == nil { + t.Fatal("expected error scanning int into DerivedMembershipList, got nil") + } +} diff --git a/internal/services/derivation_service.go b/internal/services/derivation_service.go index 844fbea..a901c4e 100644 --- a/internal/services/derivation_service.go +++ b/internal/services/derivation_service.go @@ -14,6 +14,7 @@ package services import ( "context" "database/sql" + "encoding/json" "errors" "fmt" @@ -49,18 +50,49 @@ type AttachedUnit struct { DerivedMemberCount int `db:"derived_member_count" json:"derived_member_count"` } -// DerivedMember is one user who currently derives onto a project via an -// attached partner unit. Used by the Team tab "Abgeleitet (Partner Unit)" -// subsection. +// DerivedMembership is one (unit, role) pair through which a user currently +// derives onto a project. A multi-unit user has one DerivedMembership per +// unit they belong to that's attached to the project (or one of its +// ancestors) AND whose unit_role is in the attachment's derive_unit_roles. +type DerivedMembership struct { + UnitID uuid.UUID `json:"unit_id"` + UnitName string `json:"unit_name"` + UnitRole string `json:"unit_role"` +} + +// DerivedMembershipList is a []DerivedMembership that scans from a Postgres +// jsonb column (the array_agg/jsonb_agg payload in ListDerivedMembers). +type DerivedMembershipList []DerivedMembership + +// Scan implements sql.Scanner over a jsonb array. +func (l *DerivedMembershipList) Scan(src any) error { + if src == nil { + *l = nil + return nil + } + var raw []byte + switch v := src.(type) { + case []byte: + raw = v + case string: + raw = []byte(v) + default: + return fmt.Errorf("DerivedMembershipList.Scan: unsupported type %T", src) + } + return json.Unmarshal(raw, (*[]DerivedMembership)(l)) +} + +// DerivedMember is one user who currently derives onto a project. The user +// may derive via multiple units (e.g. a PA who works with two partners); +// each is one entry in Memberships. DeriveGrantsAuthority is true if any +// of the source attachments have authority enabled. type DerivedMember struct { - UserID uuid.UUID `db:"user_id" json:"user_id"` - Email string `db:"email" json:"user_email"` - DisplayName string `db:"display_name" json:"user_display_name"` - Office string `db:"office" json:"user_office"` - UnitRole string `db:"unit_role" json:"unit_role"` - UnitID uuid.UUID `db:"unit_id" json:"unit_id"` - UnitName string `db:"unit_name" json:"unit_name"` - DeriveGrantsAuthority bool `db:"derive_grants_authority" json:"derive_grants_authority"` + UserID uuid.UUID `db:"user_id" json:"user_id"` + Email string `db:"email" json:"user_email"` + DisplayName string `db:"display_name" json:"user_display_name"` + Office string `db:"office" json:"user_office"` + Memberships DerivedMembershipList `db:"memberships" json:"memberships"` + DeriveGrantsAuthority bool `db:"derive_grants_authority" json:"derive_grants_authority"` } // AttachUnitOptions controls how a unit is attached. Empty values use the @@ -190,9 +222,12 @@ func (s *DerivationService) ListAttachedUnits(ctx context.Context, callerID, pro // down to descendants — derivation honours the same direction as // can_see_project. // -// Dedupe: if the same user derives via multiple (unit, ancestor) pairs, -// the closest-attachment row wins (smallest path-distance). One row per -// user. +// One row per user. Multi-unit users (e.g. a PA working across two partner +// units, both of which are attached to the project's path) carry every +// (unit, role) pair in Memberships so the Herkunft column can list them +// all (t-paliad-143). DeriveGrantsAuthority is bool_or across the +// underlying attachments — a user with at least one authority-granting +// derivation source qualifies as authority-bearing for approval purposes. func (s *DerivationService) ListDerivedMembers(ctx context.Context, callerID, projectID uuid.UUID) ([]DerivedMember, error) { project, err := s.projects.GetByID(ctx, callerID, projectID) if err != nil { @@ -209,44 +244,40 @@ func (s *DerivationService) ListDerivedMembers(ctx context.Context, callerID, pr SELECT ppu.project_id AS attach_project_id, ppu.partner_unit_id, ppu.derive_unit_roles, - ppu.derive_grants_authority, - array_position($1::uuid[], ppu.project_id) AS depth_rank + ppu.derive_grants_authority FROM paliad.project_partner_units ppu WHERE ppu.project_id = ANY($1::uuid[]) - ), - candidate AS ( - SELECT pum.user_id, pum.unit_role, - a.partner_unit_id, a.derive_grants_authority, a.depth_rank - FROM attached a - JOIN paliad.partner_unit_members pum ON pum.partner_unit_id = a.partner_unit_id - WHERE pum.unit_role = ANY(a.derive_unit_roles) - ), - ranked AS ( - SELECT c.*, - ROW_NUMBER() OVER ( - PARTITION BY c.user_id - -- Closest attachment wins: highest depth_rank in path - -- (path is root→…→self, array_position returns 1-based, - -- so larger = nearer to self). - ORDER BY c.depth_rank DESC NULLS LAST - ) AS rn - FROM candidate c ) - SELECT r.user_id, + SELECT pum.user_id, u.email, u.display_name, u.office, - r.unit_role, - r.partner_unit_id AS unit_id, - pu.name AS unit_name, - r.derive_grants_authority - FROM ranked r - JOIN paliad.users u ON u.id = r.user_id - JOIN paliad.partner_units pu ON pu.id = r.partner_unit_id - WHERE r.rn = 1 + jsonb_agg(DISTINCT jsonb_build_object( + 'unit_id', a.partner_unit_id, + 'unit_name', pu.name, + 'unit_role', pum.unit_role + )) AS memberships, + bool_or(a.derive_grants_authority) AS derive_grants_authority + FROM attached a + JOIN paliad.partner_unit_members pum ON pum.partner_unit_id = a.partner_unit_id + JOIN paliad.users u ON u.id = pum.user_id + JOIN paliad.partner_units pu ON pu.id = a.partner_unit_id + WHERE pum.unit_role = ANY(a.derive_unit_roles) + GROUP BY pum.user_id, u.email, u.display_name, u.office ORDER BY u.display_name`, pq.StringArray(ancestorIDs)) if err != nil { return nil, fmt.Errorf("list derived members: %w", err) } + // jsonb_agg(DISTINCT …) doesn't support ORDER BY in the same call. + // Sort each member's memberships by unit_name in Go so the Herkunft + // column renders deterministically. + for i := range rows { + ms := rows[i].Memberships + for j := 1; j < len(ms); j++ { + for k := j; k > 0 && ms[k-1].UnitName > ms[k].UnitName; k-- { + ms[k-1], ms[k] = ms[k], ms[k-1] + } + } + } return rows, nil }