fix(people): clearable manager and department pickers, exact spreadsheet dates and row numbers, loop checks through suspended members

Author: waleedlatif1

apps/sim/app/o/[organizationId]/people/[memberId]/person-modal.tsx

@@ -20,6 +20,9 @@ import { useDebounce } from '@/hooks/use-debounce'
 
 type TextField = 'title' | 'department' | 'location'
 
+/** The picker's value for no manager; membership ids are never this. */
+const NO_MANAGER = 'no-manager'
+
 const TEXT_FIELDS: readonly { field: TextField; title: string; placeholder: string }[] = [
   { field: 'title', title: 'Title', placeholder: 'Staff Engineer' },
   { field: 'department', title: 'Department', placeholder: 'Engineering' },
@@ -51,11 +54,16 @@ export function PersonModal({ organizationId, person, onClose }: PersonModalProp
     useDebounce(managerSearch.trim(), SEARCH_DEBOUNCE_MS),
     ''
   )
-  const managerOptions = (candidates.data?.pages ?? []).flatMap((page) =>
-    page.people
-      .filter((candidate) => candidate.memberId !== person.memberId)
-      .map((candidate) => ({ value: candidate.memberId, label: candidate.name }))
+  const found = (candidates.data?.pages ?? []).flatMap((page) =>
+    page.people.filter((candidate) => candidate.memberId !== person.memberId)
   )
+  const managerOptions = [
+    { value: NO_MANAGER, label: 'No manager' },
+    ...found.map((candidate) => ({
+      value: candidate.memberId,
+      label: `${candidate.name} (${candidate.email})`,
+    })),
+  ]
   const managerLabel = (draft.managerMemberId && managerName) || 'No manager'
 
   const change = <K extends keyof PersonBody>(field: K, value: PersonBody[K]) => {
@@ -122,20 +130,20 @@ export function PersonModal({ organizationId, person, onClose }: PersonModalProp
           {() => (
             <ChipCombobox
               options={managerOptions}
-              value={draft.managerMemberId ?? ''}
+              value={draft.managerMemberId ?? NO_MANAGER}
               onChange={(value) => {
-                setManagerName(
-                  managerOptions.find((option) => option.value === value)?.label ?? null
-                )
-                change('managerMemberId', value || null)
+                const manager = found.find((candidate) => candidate.memberId === value)
+                setManagerName(manager?.name ?? null)
+                change('managerMemberId', manager?.memberId ?? null)
               }}
               overlayContent={managerLabel}
               searchable
               onSearchChange={setManagerSearch}
               searchPlaceholder='Search people...'
-              isLoading={candidates.isFetching}
-              showAllOption
-              allOptionLabel='No manager'
+              isLoading={candidates.isFetching && !candidates.isFetchingNextPage}
+              hasMore={candidates.hasNextPage}
+              isLoadingMore={candidates.isFetchingNextPage}
+              onLoadMore={() => void candidates.fetchNextPage()}
               emptyMessage='No one matches'
               disabled={update.isPending}
               className='w-full'

apps/sim/app/o/[organizationId]/people/people-import-modal.tsx

@@ -30,39 +30,54 @@ interface PeopleSheet {
 }
 
 /**
- * The first sheet as text rows keyed by header. Delimited text is read verbatim so the server
- * interprets dates the same way for every file; spreadsheet date cells become `YYYY-MM-DD`.
+ * The first sheet as text rows keyed by header, one per sheet row (blank ones included) so the
+ * row numbers in the preview match the spreadsheet. Delimited text is read verbatim so the server
+ * interprets dates the same way for every file. A spreadsheet date cell is read from its stored
+ * value, not the text its locale displays, so `31/01/2024` and `1/31/24` both arrive as
+ * `2024-01-31`.
  */
 async function readPeopleSheet(file: File): Promise<PeopleSheet> {
   if (file.size > MAX_FILE_BYTES) throw new Error('Choose a file under 5 MB')
   const XLSX = await import('xlsx')
-  const delimited = /\.(csv|tsv)$/i.test(file.name)
   const workbook = XLSX.read(await file.arrayBuffer(), {
     type: 'array',
-    raw: delimited,
-    cellDates: !delimited,
+    raw: /\.(csv|tsv)$/i.test(file.name),
     dense: true,
   })
   const sheet = workbook.Sheets[workbook.SheetNames[0] ?? '']
-  if (!sheet) throw new Error('The file has no sheets')
-  const [headerRow = [], ...body] = XLSX.utils.sheet_to_json<unknown[]>(sheet, {
-    header: 1,
-    raw: false,
-    dateNF: 'yyyy-mm-dd',
-    defval: '',
-    blankrows: false,
-  })
-  const headers = headerRow.map((value) => String(value).trim())
-  const columns = headers.flatMap((header, index) => (header ? [{ header, index }] : []))
+  if (!sheet?.['!ref']) throw new Error('The file has no rows')
+  const range = XLSX.utils.decode_range(sheet['!ref'])
+  const grid = sheet['!data'] ?? []
+  const text = (row: number, column: number): string => {
+    const cell = grid[row]?.[column]
+    if (!cell || cell.v === undefined || cell.v === null) return ''
+    if (cell.t === 'n' && XLSX.SSF.is_date(cell.z ?? '')) {
+      const date = XLSX.SSF.parse_date_code(Number(cell.v))
+      return `${date.y}-${String(date.m).padStart(2, '0')}-${String(date.d).padStart(2, '0')}`
+    }
+    return String(cell.w ?? cell.v).trim()
+  }
+  const columns: { header: string; index: number }[] = []
+  for (let index = range.s.c; index <= range.e.c; index++) {
+    const header = text(range.s.r, index)
+    if (!header) continue
+    if (columns.some((column) => column.header === header)) {
+      throw new Error(`The ${header} column appears more than once`)
+    }
+    columns.push({ header, index })
+  }
   if (!columns.length) throw new Error('The first row should name the columns')
-  if (body.length > PEOPLE_IMPORT_MAX_ROWS) {
+  const rowCount = range.e.r - range.s.r
+  if (rowCount > PEOPLE_IMPORT_MAX_ROWS) {
     throw new Error(`Import at most ${PEOPLE_IMPORT_MAX_ROWS} rows at a time`)
   }
   return {
     fileName: file.name,
     headers: columns.map(({ header }) => header),
-    rows: body.map((row) =>
-      Object.fromEntries(columns.map(({ header, index }) => [header, String(row[index] ?? '')]))
+    rows: Array.from({ length: rowCount }, (_, offset) =>
+      Object.fromEntries(
+        columns.map(({ header, index }) => [header, text(range.s.r + 1 + offset, index)])
+      )
     ),
   }
 }

apps/sim/app/o/[organizationId]/people/people.tsx

@@ -35,6 +35,9 @@ import { useDepartments, usePeople } from '@/hooks/queries/people'
 import { useDebouncedSearchSetter } from '@/hooks/use-debounced-search-setter'
 import { useSearchFilterValue } from '@/hooks/use-search-filter-value'
 
+/** The filter's value for every department: stored text can't hold a NUL, so no department is this. */
+const ALL_DEPARTMENTS = '\u0000all'
+
 function personCell(person: PersonSummary) {
   return {
     icon: <Avatar size='xs' name={person.name} src={person.image} aria-hidden />,
@@ -117,18 +120,19 @@ export function People({ organizationId }: PeopleProps) {
             <ResourceFilterPanel>
               <ResourceFilterSection label='Department'>
                 <ChipCombobox
-                  options={departments.map(({ name, count }) => ({
-                    value: name,
-                    label: `${name} (${count})`,
-                  }))}
-                  value={department}
-                  onChange={setDepartment}
+                  options={[
+                    { value: ALL_DEPARTMENTS, label: 'All' },
+                    ...departments.map(({ name, count }) => ({
+                      value: name,
+                      label: `${name} (${count})`,
+                    })),
+                  ]}
+                  value={department || ALL_DEPARTMENTS}
+                  onChange={(value) => setDepartment(value === ALL_DEPARTMENTS ? '' : value)}
                   overlayContent={department || 'All'}
                   searchable
                   searchPlaceholder='Search departments...'
-                  showAllOption
-                  allOptionLabel='All'
-                  emptyMessage='No departments recorded yet'
+                  emptyMessage='No departments match'
                   className='w-full'
                 />
               </ResourceFilterSection>

apps/sim/lib/api/contracts/people.ts

@@ -106,7 +106,14 @@ export const importPeopleContract = defineRouteContract({
   params,
   body: z
     .object({
-      headers: z.array(z.string().max(200)).min(1).max(100),
+      headers: z
+        .array(z.string().max(200))
+        .min(1)
+        .max(100)
+        .refine(
+          (headers) => new Set(headers).size === headers.length,
+          'Each column must have its own name'
+        ),
       rows: z
         .array(z.record(z.string().max(200), z.string().max(1000)))
         .min(1, 'The file has no rows')

apps/sim/lib/people/application/use-cases.ts

@@ -24,18 +24,19 @@ interface PersonInput extends OrganizationInput {
 /** Issues past this many are counted, not listed, so a broken file can't flood the response. */
 const MAX_REPORTED_ISSUES = 200
 
-async function authorizeDirectory({
+async function authorizeDirectory<I extends OrganizationInput>({
   input,
   context,
-}: OrganizationUseCaseContext<OrganizationInput>): Promise<void> {
+}: OrganizationUseCaseContext<I>): Promise<void> {
   await Promise.all([
     requirePeopleEnabled(input.organizationId),
     requireOrganizationMemberDirectory(input.organizationId, context.role),
   ])
 }
 
-const authorizeAdministration = ({ input }: OrganizationUseCaseContext<OrganizationInput>) =>
-  requirePeopleEnabled(input.organizationId)
+const authorizeAdministration = <I extends OrganizationInput>({
+  input,
+}: OrganizationUseCaseContext<I>) => requirePeopleEnabled(input.organizationId)
 
 export const listPeople = defineAuthorizedOrganizationUseCase({
   operation: peopleOperations.list,

apps/sim/lib/people/import.test.ts

@@ -14,9 +14,9 @@ function profile(fields: Partial<MemberProfileFields>): MemberProfileFields {
   }
 }
 
-const ada = { memberId: 'm-ada', email: 'ada@example.com', profile: null }
-const bo = { memberId: 'm-bo', email: 'bo@example.com', profile: null }
-const cy = { memberId: 'm-cy', email: 'cy@example.com', profile: null }
+const ada = { memberId: 'm-ada', email: 'ada@example.com', profile: null, active: true }
+const bo = { memberId: 'm-bo', email: 'bo@example.com', profile: null, active: true }
+const cy = { memberId: 'm-cy', email: 'cy@example.com', profile: null, active: true }
 
 function plan(
   headers: string[],
@@ -313,6 +313,31 @@ describe('planPeopleImport', () => {
     expect(result.issues).toEqual([expect.objectContaining({ row: 2, severity: 'skipped' })])
   })
 
+  it('follows a suspended manager when checking for loops, but never matches them', () => {
+    const suspended = {
+      memberId: 'm-sus',
+      email: 'sus@example.com',
+      active: false,
+      profile: profile({ managerMemberId: 'm-ada' }),
+    }
+    const reportsToSuspended = { ...bo, profile: profile({ managerMemberId: 'm-sus' }) }
+    const result = plan(
+      ['email', 'manager email'],
+      [
+        { email: 'ada@example.com', 'manager email': 'bo@example.com' },
+        { email: 'cy@example.com', 'manager email': 'sus@example.com' },
+        { email: 'sus@example.com', 'manager email': '' },
+      ],
+      [ada, reportsToSuspended, cy, suspended]
+    )
+    expect(result.changes).toEqual([])
+    expect(result.issues.map((issue) => [issue.row, issue.severity])).toEqual([
+      [2, 'warning'],
+      [3, 'warning'],
+      [4, 'skipped'],
+    ])
+  })
+
   it('combines location parts, leaving out the blank ones', () => {
     const result = plan(
       ['email', 'city', 'state', 'country'],

apps/sim/lib/people/import.ts

@@ -92,6 +92,8 @@ export interface PeopleImportMember {
   memberId: string
   email: string
   profile: MemberProfileFields | null
+  /** Suspended members keep their reporting lines for loop checks but are never matched. */
+  active: boolean
 }
 
 interface PeopleImportIssue {
@@ -177,7 +179,9 @@ export function planPeopleImport({
   rows: readonly Record<string, string>[]
   members: readonly PeopleImportMember[]
 }): PeopleImportPlan {
-  const byEmail = new Map(members.map((member) => [member.email.toLowerCase(), member]))
+  const byEmail = new Map(
+    members.filter((member) => member.active).map((member) => [member.email.toLowerCase(), member])
+  )
   const managers = new Map(
     members.map((member) => [member.memberId, member.profile?.managerMemberId ?? null])
   )

apps/sim/lib/people/repository.ts

@@ -267,7 +267,10 @@ export async function isInReportingChain(
   return row?.found === true
 }
 
-/** Every active member with the email they are matched by and their stored profile. */
+/**
+ * Every member with the email they are matched by, their stored profile, and whether they are
+ * active: a suspended member's reporting lines still close loops.
+ */
 export async function listMemberProfiles(
   executor: DbOrTx,
   organizationId: string
@@ -282,14 +285,16 @@ export async function listMemberProfiles(
       startDate: memberProfile.startDate,
       managerMemberId: memberProfile.managerMemberId,
       hasProfile: sql<boolean>`${memberProfile.memberId} IS NOT NULL`,
+      active: sql<boolean>`${user.suspendedAt} IS NULL`,
     })
     .from(member)
     .innerJoin(user, eq(user.id, member.userId))
     .leftJoin(memberProfile, eq(memberProfile.memberId, member.id))
-    .where(and(eq(member.organizationId, organizationId), ACTIVE_USER))
-  return rows.map(({ memberId, email, hasProfile, ...profile }) => ({
+    .where(eq(member.organizationId, organizationId))
+  return rows.map(({ memberId, email, hasProfile, active, ...profile }) => ({
     memberId,
     email,
+    active,
     profile: hasProfile ? profile : null,
   }))
 }

apps/sim/scripts/test-people-e2e.ts

@@ -166,8 +166,8 @@ async function run() {
     const { people } = await api(ada, '', {})
     assert(Array.isArray(people) && people.length === 3, `Expected 3 people, got ${people}`)
     assert.deepEqual((await api(ada, '/departments', {})).departments, [])
-    await api({ ...outsider, orgId }, '', { expected: 403 })
-    await api({ ...outsider, orgId }, `/${ada.memberId}`, { expected: 403 })
+    await api({ ...outsider, orgId }, '', { expected: 404 })
+    await api({ ...outsider, orgId }, `/${ada.memberId}`, { expected: 404 })
     await api(outsider, `/${ada.memberId}`, { expected: 404 })
   })