fix(people): keep a suspended manager link on edit and never fail the org layout on the People lookup

Author: waleedlatif1

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 <OrganizationAccessDenied />
   }
-  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'
 

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<Partial<Record<keyof PersonBody, string>>>({})
   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 = <K extends keyof PersonBody>(field: K, value: PersonBody[K]) => {
     setDraft((current) => ({ ...current, [field]: value }))

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<typeof personSchema>

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)

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,17 +97,20 @@ 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,
   rows: readonly PersonRow[]
 ): Promise<PersonRecord[]> {
   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,
   }))
 }
 

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<string, unknown> & { manager: { memberId: string } | null }
+    person: Record<string, unknown> & {
+      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<string, unknown>, 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<string, number> = {}
     for (let attempt = 0; attempt < 20; attempt++) {
       const rows = await sql<