diff --git a/apps/sim/app/o/[organizationId]/layout.tsx b/apps/sim/app/o/[organizationId]/layout.tsx index 7ff0b566493..081b2b437a2 100644 --- a/apps/sim/app/o/[organizationId]/layout.tsx +++ b/apps/sim/app/o/[organizationId]/layout.tsx @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { getErrorMessage } from '@sim/utils/errors' import { dehydrate, HydrationBoundary } from '@tanstack/react-query' import { cookies } from 'next/headers' import { redirect } from 'next/navigation' @@ -65,8 +66,20 @@ export default async function OrganizationLayout({ if (!context) { return } - const peopleEnabled = await canBrowsePeople(organizationId, context.viewer.role) - if (!context.mothershipAvailable && !context.searchAccess.memberScoped && !peopleEnabled) + const peopleEnabledLookup = canBrowsePeople(organizationId, context.viewer.role).catch( + (error) => { + logger.warn('Could not resolve People navigation access', { + organizationId, + error: getErrorMessage(error), + }) + return false + } + ) + if ( + !context.mothershipAvailable && + !context.searchAccess.memberScoped && + !(await peopleEnabledLookup) + ) redirect(WORKSPACE_SETTINGS_PATH) const [ @@ -79,6 +92,7 @@ export default async function OrganizationLayout({ changelogEnabled, projectsEnabled, simLinksEnabled, + peopleEnabled, ] = await Promise.all([ prefetchOrganizationSidebar( queryClient, @@ -97,6 +111,7 @@ export default async function OrganizationLayout({ isChangelogEnabled(organizationId), isProjectsEnabled(), isSimLinksEnabled(organizationId), + peopleEnabledLookup, ]) const initialSidebarCollapsed = cookieStore.get('sidebar_collapsed')?.value === '1' diff --git a/apps/sim/app/o/[organizationId]/people/[memberId]/person-modal.tsx b/apps/sim/app/o/[organizationId]/people/[memberId]/person-modal.tsx index 0573dab5afa..ff5fa4f383b 100644 --- a/apps/sim/app/o/[organizationId]/people/[memberId]/person-modal.tsx +++ b/apps/sim/app/o/[organizationId]/people/[memberId]/person-modal.tsx @@ -43,11 +43,14 @@ export function PersonModal({ organizationId, person, onClose }: PersonModalProp department: person.department, location: person.location, startDate: person.startDate, - managerMemberId: person.manager?.memberId ?? null, + managerMemberId: person.managerMemberId, }) const [errors, setErrors] = useState>>({}) const [managerSearch, setManagerSearch] = useState('') - /** The chosen manager's name, kept with the choice: the search results may no longer include them. */ + /** + * The chosen manager's name, kept with the choice: the search results may no longer include them. + * Only a stored manager who is suspended has an id and no name. + */ const [managerName, setManagerName] = useState(person.manager?.name ?? null) const candidates = usePeople( organizationId, @@ -64,7 +67,7 @@ export function PersonModal({ organizationId, person, onClose }: PersonModalProp label: `${candidate.name} (${candidate.email})`, })), ] - const managerLabel = (draft.managerMemberId && managerName) || 'No manager' + const managerLabel = draft.managerMemberId ? (managerName ?? 'Suspended member') : 'No manager' const change = (field: K, value: PersonBody[K]) => { setDraft((current) => ({ ...current, [field]: value })) diff --git a/apps/sim/lib/api/contracts/people.ts b/apps/sim/lib/api/contracts/people.ts index 7c904ec4104..4d0d1c88b20 100644 --- a/apps/sim/lib/api/contracts/people.ts +++ b/apps/sim/lib/api/contracts/people.ts @@ -24,7 +24,10 @@ const personSchema = personSummarySchema.extend({ location: z.string().nullable(), /** `YYYY-MM-DD`. */ startDate: z.string().nullable(), + /** The manager as shown; `null` when there is none or they are suspended. */ manager: personSummarySchema.nullable(), + /** The stored manager link, kept while that manager is suspended. */ + managerMemberId: z.string().nullable(), joinedAt: z.iso.datetime(), }) export type Person = z.output diff --git a/apps/sim/lib/people/application/use-cases.ts b/apps/sim/lib/people/application/use-cases.ts index 289aae0ea68..f2a481b46ad 100644 --- a/apps/sim/lib/people/application/use-cases.ts +++ b/apps/sim/lib/people/application/use-cases.ts @@ -79,8 +79,9 @@ export const getPerson = defineAuthorizedOrganizationUseCase({ }) /** - * Replaces one member's profile. The manager must be another active member of the same - * organization who doesn't already report, directly or not, to this person. + * Replaces one member's profile. A new manager must be another active member of the same + * organization who doesn't already report, directly or not, to this person. The stored manager, + * sent back unchanged, is kept even while suspended, as an import that leaves it out keeps it. */ export const updatePerson = defineAuthorizedOrganizationUseCase({ operation: peopleOperations.update, @@ -93,20 +94,19 @@ export const updatePerson = defineAuthorizedOrganizationUseCase({ } return db.transaction(async (tx) => { await repository.lockProfiles(tx, organizationId) - const active = await repository.findActiveMembers( - tx, - organizationId, - managerMemberId ? [memberId, managerMemberId] : [memberId] - ) - if (!active.has(memberId)) throw new OrchestrationError('not_found', 'Person not found') - if (managerMemberId && !active.has(managerMemberId)) { - throw new OrchestrationError( - 'validation', - 'The manager must be a member of this organization' - ) - } - if (managerMemberId && (await repository.isInReportingChain(tx, memberId, managerMemberId))) { - throw new OrchestrationError('validation', 'That manager reports to this person') + const current = await repository.getPerson(tx, organizationId, memberId) + if (!current) throw new OrchestrationError('not_found', 'Person not found') + if (managerMemberId && managerMemberId !== current.managerMemberId) { + const active = await repository.findActiveMembers(tx, organizationId, [managerMemberId]) + if (!active.has(managerMemberId)) { + throw new OrchestrationError( + 'validation', + 'The manager must be a member of this organization' + ) + } + if (await repository.isInReportingChain(tx, memberId, managerMemberId)) { + throw new OrchestrationError('validation', 'That manager reports to this person') + } } await repository.upsertProfiles(tx, [{ memberId, ...fields }], context.userId) const person = await repository.getPerson(tx, organizationId, memberId) diff --git a/apps/sim/lib/people/repository.ts b/apps/sim/lib/people/repository.ts index e8584b6c0e5..05f91db2936 100644 --- a/apps/sim/lib/people/repository.ts +++ b/apps/sim/lib/people/repository.ts @@ -27,6 +27,7 @@ export interface PersonRecord extends PersonSummary { location: string | null startDate: string | null manager: PersonSummary | null + managerMemberId: string | null joinedAt: Date } @@ -96,7 +97,10 @@ async function summariesById( return new Map(rows.map((row) => [row.memberId, row])) } -/** Joins each person to their manager, read once for the whole page. */ +/** + * Joins each person to their manager, read once for the whole page. A suspended manager stays + * linked by `managerMemberId` but has no summary, since no read lists suspended people. + */ async function withManagers( executor: DbOrTx, organizationId: string, @@ -104,9 +108,9 @@ async function withManagers( ): Promise { const managerIds = [...new Set(rows.flatMap((row) => row.managerMemberId ?? []))] const managers = await summariesById(executor, organizationId, managerIds) - return rows.map(({ sortName, managerMemberId, ...row }) => ({ + return rows.map(({ sortName, ...row }) => ({ ...row, - manager: managerMemberId ? (managers.get(managerMemberId) ?? null) : null, + manager: row.managerMemberId ? (managers.get(row.managerMemberId) ?? null) : null, })) } diff --git a/apps/sim/scripts/test-people-e2e.ts b/apps/sim/scripts/test-people-e2e.ts index abacc77cc56..73edb777113 100644 --- a/apps/sim/scripts/test-people-e2e.ts +++ b/apps/sim/scripts/test-people-e2e.ts @@ -86,7 +86,10 @@ async function person(member: Member, memberId: string) { const result = await api(member, `/${memberId}`, {}) assert(isRecordLike(result.person), 'Expected a person') return result as { - person: Record & { manager: { memberId: string } | null } + person: Record & { + manager: { memberId: string } | null + managerMemberId: string | null + } chain: { memberId: string }[] reports: { memberId: string }[] } @@ -297,6 +300,62 @@ async function run() { assert.equal(row?.manager_member_id, owner.memberId) }) + await check( + "an edit keeps a suspended manager it doesn't change, and refuses one as a new manager", + async () => { + const margaret = await seedMember(orgId, 'member', 'Margaret') + const edit = (memberId: string, body: Record, expected = 200) => + api(owner, `/${memberId}`, { method: 'PUT', body, expected }) + await edit(ada.memberId, { + title: 'Engineer', + department: 'Platform', + location: 'Berlin, Germany', + startDate: '2021-03-04', + managerMemberId: margaret.memberId, + }) + await sql`update "user" set suspended_at = now() where id = ${margaret.userId}` + + const { person: shown } = await person(owner, ada.memberId) + assert.equal(shown.manager, null, 'A suspended manager is not shown') + assert.equal(shown.managerMemberId, margaret.memberId, 'The stored link is still reported') + const saved = await edit(ada.memberId, { + title: 'Staff Engineer', + department: shown.department, + location: shown.location, + startDate: shown.startDate, + managerMemberId: shown.managerMemberId, + }) + assert(isRecordLike(saved.person), 'Expected a person') + assert.equal(saved.person.managerMemberId, margaret.memberId) + const row = await profileRow(ada.memberId) + assert.equal(row?.title, 'Staff Engineer') + assert.equal(row?.manager_member_id, margaret.memberId) + + const refused = await edit( + linus.memberId, + { + title: 'Principal Engineer', + department: null, + location: null, + startDate: '2019-07-01', + managerMemberId: margaret.memberId, + }, + 400 + ) + assert.equal(refused.error, 'The manager must be a member of this organization') + assert.equal((await profileRow(linus.memberId))?.manager_member_id, owner.memberId) + + await edit(ada.memberId, { + title: 'Staff Engineer', + department: shown.department, + location: shown.location, + startDate: shown.startDate, + managerMemberId: owner.memberId, + }) + assert.equal((await profileRow(ada.memberId))?.manager_member_id, owner.memberId) + } + ) + await check( 'leaving the organization removes the profile and clears reporting lines', async () => { @@ -308,7 +367,7 @@ async function run() { ) await check('imports and edits are audited', async () => { - const expected = { 'org_people.imported': 3, 'org_member.profile_updated': 1 } + const expected = { 'org_people.imported': 3, 'org_member.profile_updated': 4 } let counts: Record = {} for (let attempt = 0; attempt < 20; attempt++) { const rows = await sql<