Skip to content

Commit acb6194

Browse files
aaronlee777claudejeffredoddcursoragentkrisxcrash
authored
fix: hide assigned employees from time-off add list (SDK-894) (#1843)
* fix: hide assigned employees from time-off add list (SDK-894) Refactor SelectEmployees to be add-only with full client-side filter, search, and pagination. Resolves the empty-page bug where server-paginated results were filtered client-side, producing visually empty pages after adding employees from the first page. Also filters out employees with a future hire_date to prevent the "ineligible employees" 422 from the add-employees endpoint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: gate DataTable select-all on row count, drop local override Mirror DataCards' empty-data behavior in DataTable so the select-all header checkbox auto-hides when there are no rows. Removes the consumer-side hideSelectAll={employees.length === 0} from SelectEmployeesPresentation since it's now automatic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: direct unit coverage for hideSelectAll/hideSearch/isStartedByToday + multi-page fetch - DataTable & DataCards: tests for hideSelectAll opt-out and empty-data auto-hide of the select-all control. - EmployeeTable: tests for hideSearch prop (search container hidden when true, rendered when false). - isStartedByToday: edge-case unit tests (undefined, empty string, today exactly, past, +1 day, far future) with system time pinned. - useSelectEmployeesData multi-page: integration tests covering the x-total-pages header branch — no extra queries when totalPages is 1, buildEmployeesListQuery fires for pages 2..N when >1, and pages merge into the displayed list. Also documents the implicit per-origin concurrency ceiling in useSelectEmployeesData (no explicit cap on parallel page fetches). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: direct coverage for pagination handlers and safeCurrentPage clamp renderHook-based tests for useSelectEmployeesData pagination state machine: - handleFirstPage / handleLastPage / handleNextPage / handlePreviousPage including clamps at 1 and totalPages - handleItemsPerPageChange resets currentPage to 1 and is a no-op when the value matches - handleSearchChange / handleSearchClear reset currentPage to 1 - safeCurrentPage clamps when a search shrinks totalPages below the user's current page Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: replace custom pagination with useClientPagination (#1887) * refactor: replace custom pagination with useClientPagination Drops the hand-rolled in-memory pagination/search state machine from `useSelectEmployeesData` in favor of the newly merged `useClientPagination` hook. Same external behavior — multi-page fetch, future-hire-date filter, hide-assigned filter, search + pagination — but ~60 fewer lines and one shared implementation across the SDK. The hook's built-in 120ms search debounce is now active for this flow as a bonus. Tests updated to wait for the debounce instead of asserting synchronously after `userEvent.type`. Co-authored-by: Cursor <cursoragent@cursor.com> * docs: regenerate endpoint inventory after SelectEmployees add-only refactor Aaron's #1843 dropped the `removeEmployees` mutations from SelectEmployeesHoliday and SelectEmployeesTimeOff (the add-only flow per SDK-894), but the derived endpoint inventory wasn't regenerated. Picked up by the pre-commit hook on this stacked PR. No source change here — just the output of `build/deriveEndpointInventory.ts`. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: restore isHolidayPolicy prop on SelectEmployeesPresentation When the SDK-894 branch was rebased onto main, the add-only SelectEmployees components were checked out wholesale from the pre-rebase tip, which predated Kristine's #1849 fix. That re-introduced the bug where the description ternary keyed off `showReassignmentWarning` — meaning sick and vacation policies (which often skip the reassignment warning) showed the holiday description by mistake. Reapply #1849's `isHolidayPolicy` prop: - `SelectEmployeesPresentationTypes.ts`: add the prop - `SelectEmployeesPresentation.tsx`: gate the description ternary on it - `SelectEmployeesHoliday.tsx`: pass it - stories: add a HolidayPolicy story - tests: assert on `isHolidayPolicy` (and a default "renders description") Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> * fix: improve HolidayPolicyDetail pagination test reliability Replace waitFor + getByText with findByText for pagination assertions to make the test more robust in CI environments. findByText has built-in retrying and better timeout handling for async state updates. * fix: correct API mock format in HolidayPolicyDetail tests The embedded-api expects: - Array response (not wrapped in object) - snake_case field names (auto-transformed to camelCase) - Pagination headers (x-total-pages, x-total-count) Also improved test reliability by using findByText instead of waitFor + getByText for better async handling in CI environments. * fix: use findByText for all async assertions in pagination test Changed Person11 assertion to also use findByText for consistent async handling across all pagination-related assertions. * fix: improve HolidayPolicyDetail test isolation - Move userEvent.setup() into beforeEach for proper test isolation - Increase findByText timeout to 5000ms for pagination tests to handle timing variations when running with full test suite - Prevents test failures due to shared userEvent instance * fix: use waitFor wrapper for findByText in pagination test Wrap findByText in waitFor with explicit timeout to ensure proper timing when navigating to second page in multi-test environment. * fix: increase HolidayPolicyDetail pagination test timeout for CI The pagination test timed out in CI (5s default) because Suspense resolution + user interaction + state updates exceed that under load. - Replace nested waitFor(findByText) antipattern with plain findByText - Add explicit 15s test timeout to both pagination tests --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Jeffrey D Johnson <jeffredodd@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Kristine White <kristine.white@gusto.com> Co-authored-by: Kristine White <kristinenicolemartin@gmail.com>
1 parent ad252dc commit acb6194

26 files changed

Lines changed: 836 additions & 638 deletions

docs/reference/endpoint-inventory.json

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1517,10 +1517,6 @@
15171517
{
15181518
"method": "PUT",
15191519
"path": "/v1/time_off_policies/:timeOffPolicyUuid/add_employees"
1520-
},
1521-
{
1522-
"method": "PUT",
1523-
"path": "/v1/time_off_policies/:timeOffPolicyUuid/remove_employees"
15241520
}
15251521
],
15261522
"variables": [
@@ -1560,10 +1556,6 @@
15601556
{
15611557
"method": "PUT",
15621558
"path": "/v1/companies/:companyUuid/holiday_pay_policy/add"
1563-
},
1564-
{
1565-
"method": "PUT",
1566-
"path": "/v1/companies/:companyUuid/holiday_pay_policy/remove"
15671559
}
15681560
],
15691561
"variables": [

docs/reference/endpoint-reference.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -286,14 +286,12 @@ import inventory from '@gusto/embedded-react-sdk/endpoint-inventory.json'
286286
| | GET | `/v1/time_off_policies/:timeOffPolicyUuid` |
287287
| | PUT | `/v1/time_off_policies/:timeOffPolicyUuid` |
288288
| | PUT | `/v1/time_off_policies/:timeOffPolicyUuid/add_employees` |
289-
| | PUT | `/v1/time_off_policies/:timeOffPolicyUuid/remove_employees` |
290289
| **TimeOff.HolidaySelectionForm** | GET | `/v1/companies/:companyUuid/holiday_pay_policy` |
291290
| | POST | `/v1/companies/:companyUuid/holiday_pay_policy` |
292291
| | PUT | `/v1/companies/:companyUuid/holiday_pay_policy` |
293292
| **TimeOff.AddEmployeesHoliday** | GET | `/v1/companies/:companyId/employees` |
294293
| | GET | `/v1/companies/:companyUuid/holiday_pay_policy` |
295294
| | PUT | `/v1/companies/:companyUuid/holiday_pay_policy/add` |
296-
| | PUT | `/v1/companies/:companyUuid/holiday_pay_policy/remove` |
297295
| **TimeOff.ViewHolidayEmployees** | GET | `/v1/companies/:companyId/employees` |
298296
| | GET | `/v1/companies/:companyUuid/holiday_pay_policy` |
299297
| | PUT | `/v1/companies/:companyUuid/holiday_pay_policy/remove` |

src/components/Common/DataView/DataCards/DataCards.test.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,13 @@ describe('DataCards', () => {
114114
await userEvent.click(screen.getAllByRole('checkbox')[0] as HTMLElement)
115115
expect(onSelectAllMock).toHaveBeenCalledWith(false, testData)
116116
})
117+
118+
test('does not render select-all checkbox when hideSelectAll is true', () => {
119+
renderCards({ ...selectableProps, hideSelectAll: true })
120+
expect(screen.queryByLabelText('Select all rows')).not.toBeInTheDocument()
121+
// Per-row checkboxes should still render.
122+
expect(screen.getAllByRole('checkbox')).toHaveLength(testData.length)
123+
})
117124
})
118125

119126
test('should render empty state with proper accessibility structure when emptyState is provided', () => {

src/components/Common/DataView/DataCards/DataCards.tsx

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ export type DataCardsProps<T> = {
1616
onSelect?: (item: T, checked: boolean) => void
1717
onSelectAll?: (checked: boolean, visibleData: T[]) => void
1818
getIsItemSelected?: (item: T) => boolean
19+
hideSelectAll?: useDataViewPropReturn<T>['hideSelectAll']
1920
emptyState?: useDataViewPropReturn<T>['emptyState']
2021
footer?: useDataViewPropReturn<T>['footer']
2122
selectionMode?: SelectionMode
@@ -30,6 +31,7 @@ export const DataCards = <T,>({
3031
onSelect,
3132
onSelectAll,
3233
getIsItemSelected,
34+
hideSelectAll,
3335
emptyState,
3436
footer,
3537
selectionMode = 'multiple',
@@ -73,15 +75,19 @@ export const DataCards = <T,>({
7375

7476
return (
7577
<div className={cn(styles.root, isWithinBox && styles.withinBox)} data-testid="data-cards">
76-
{onSelect && getIsItemSelected && selectionMode === 'multiple' && data.length > 0 && (
77-
<div className={styles.selectAllRow}>
78-
<Components.Checkbox
79-
value={allSelected}
80-
onChange={(checked: boolean) => onSelectAll?.(checked, data)}
81-
label={t('card.selectAllRowsLabel')}
82-
/>
83-
</div>
84-
)}
78+
{onSelect &&
79+
getIsItemSelected &&
80+
selectionMode === 'multiple' &&
81+
!hideSelectAll &&
82+
data.length > 0 && (
83+
<div className={styles.selectAllRow}>
84+
<Components.Checkbox
85+
value={allSelected}
86+
onChange={(checked: boolean) => onSelectAll?.(checked, data)}
87+
label={t('card.selectAllRowsLabel')}
88+
/>
89+
</div>
90+
)}
8591
<div role="list" aria-label={label}>
8692
{data.length === 0 && emptyState && (
8793
<div role="listitem">

src/components/Common/DataView/DataTable/DataTable.test.tsx

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,17 @@ describe('DataTable Component', () => {
141141
expect(screen.queryByRole('checkbox')).not.toBeInTheDocument()
142142
expect(screen.getAllByRole('radio')).toHaveLength(testData.length)
143143
})
144+
145+
test('does not render a header checkbox when hideSelectAll is true', () => {
146+
renderTable({ ...selectableProps, hideSelectAll: true })
147+
// Only the per-row checkboxes should be present.
148+
expect(screen.getAllByRole('checkbox')).toHaveLength(testData.length)
149+
})
150+
151+
test('does not render a header checkbox when data is empty', () => {
152+
renderTable({ ...selectableProps, data: [] })
153+
expect(screen.queryByRole('checkbox')).not.toBeInTheDocument()
154+
})
144155
})
145156

146157
test('should render itemMenu when provided', () => {

src/components/Common/DataView/DataTable/DataTable.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ export type DataTableProps<T> = {
1414
onSelect?: useDataViewPropReturn<T>['onSelect']
1515
onSelectAll?: (checked: boolean, visibleData: T[]) => void
1616
getIsItemSelected?: (item: T) => boolean
17+
hideSelectAll?: useDataViewPropReturn<T>['hideSelectAll']
1718
emptyState?: useDataViewPropReturn<T>['emptyState']
1819
footer?: useDataViewPropReturn<T>['footer']
1920
isWithinBox?: TableProps['isWithinBox']
@@ -44,6 +45,7 @@ export const DataTable = <T,>({
4445
onSelect,
4546
onSelectAll,
4647
getIsItemSelected,
48+
hideSelectAll,
4749
emptyState,
4850
footer,
4951
isWithinBox,
@@ -61,7 +63,10 @@ export const DataTable = <T,>({
6163
{
6264
key: 'select-header',
6365
content:
64-
selectionMode === 'multiple' && getIsItemSelected ? (
66+
selectionMode === 'multiple' &&
67+
getIsItemSelected &&
68+
!hideSelectAll &&
69+
data.length > 0 ? (
6570
// Stop propagation so the surrounding react-aria-components
6671
// <Column> press handler doesn't intercept the checkbox click
6772
// and desync the controlled DOM state. The inner <input> is

src/components/Common/DataView/DataView.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ export type DataViewProps<T> = {
1717
onSelect?: useDataViewPropReturn<T>['onSelect']
1818
onSelectAll?: useDataViewPropReturn<T>['onSelectAll']
1919
getIsItemSelected?: (item: T) => boolean
20+
hideSelectAll?: useDataViewPropReturn<T>['hideSelectAll']
2021
breakAt?: string
2122
breakpoints?: useContainerBreakpointsProps['breakpoints']
2223
footer?: useDataViewPropReturn<T>['footer']

src/components/Common/DataView/useDataView.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ type BaseDataViewProps<T> = {
2525
emptyState?: () => React.ReactNode
2626
footer?: () => Partial<Record<FooterKeys<T>, React.ReactNode>>
2727
isFetching?: boolean
28+
/**
29+
* When true, hides the select-all header checkbox (DataTable) and the
30+
* select-all row (DataCards). Only meaningful with `selectionMode: 'multiple'`.
31+
* Per-row checkboxes are unaffected.
32+
*/
33+
hideSelectAll?: boolean
2834
}
2935

3036
type NoSelectionProps = {
@@ -69,6 +75,7 @@ export type useDataViewPropReturn<T> = {
6975
onSelect?: (item: T, checked: boolean) => void
7076
onSelectAll?: (checked: boolean, visibleData: T[]) => void
7177
getIsItemSelected?: (item: T) => boolean
78+
hideSelectAll?: boolean
7279
emptyState?: () => React.ReactNode
7380
footer?: () => Partial<Record<FooterKeys<T>, React.ReactNode>>
7481
isFetching?: boolean
@@ -82,6 +89,7 @@ export const useDataView = <T>({
8289
onSelect,
8390
onSelectAll,
8491
getIsItemSelected,
92+
hideSelectAll,
8593
pagination,
8694
emptyState,
8795
footer,
@@ -97,6 +105,7 @@ export const useDataView = <T>({
97105
onSelect,
98106
onSelectAll,
99107
getIsItemSelected,
108+
hideSelectAll,
100109
emptyState,
101110
footer,
102111
isFetching,
@@ -110,6 +119,7 @@ export const useDataView = <T>({
110119
onSelect,
111120
onSelectAll,
112121
getIsItemSelected,
122+
hideSelectAll,
113123
emptyState,
114124
footer,
115125
isFetching,

src/components/TimeOff/HolidayPolicyDetail/HolidayPolicyDetail.test.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,12 @@ describe('HolidayPolicyDetail', () => {
6464
return HttpResponse.json(mockHolidayPayPolicy)
6565
}),
6666
http.get(`${API_BASE_URL}/v1/companies/:companyId/employees`, () => {
67-
return HttpResponse.json(mockEmployees)
67+
return HttpResponse.json(mockEmployees, {
68+
headers: {
69+
'x-total-pages': '1',
70+
'x-total-count': mockEmployees.length.toString(),
71+
},
72+
})
6873
}),
6974
)
7075
})

src/components/TimeOff/TimeOffManagement/SelectEmployees/SelectEmployeesHoliday.test.tsx

Lines changed: 33 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,14 @@ const mockEmployees = [
1818
uuid: '1',
1919
firstName: 'Alice',
2020
lastName: 'Smith',
21-
jobs: [{ primary: true, title: 'Engineer' }],
21+
jobs: [{ primary: true, title: 'Engineer', hireDate: '2024-01-01' }],
2222
department: 'Engineering',
2323
},
2424
{
2525
uuid: '2',
2626
firstName: 'Bob',
2727
lastName: 'Jones',
28-
jobs: [{ primary: true, title: 'Designer' }],
28+
jobs: [{ primary: true, title: 'Designer', hireDate: '2024-01-01' }],
2929
department: 'Design',
3030
},
3131
]
@@ -71,13 +71,6 @@ vi.mock('@gusto/embedded-api/react-query/holidayPayPoliciesAddEmployees', () =>
7171
}),
7272
}))
7373

74-
vi.mock('@gusto/embedded-api/react-query/holidayPayPoliciesRemoveEmployees', () => ({
75-
useHolidayPayPoliciesRemoveEmployeesMutation: () => ({
76-
mutateAsync: vi.fn(),
77-
isPending: false,
78-
}),
79-
}))
80-
8174
vi.mock('@/components/Base/useBase', () => ({
8275
useBase: () => ({
8376
onEvent: mockOnEvent,
@@ -113,6 +106,7 @@ describe('SelectEmployeesHoliday', () => {
113106
mockAddEmployees.mockResolvedValue({
114107
holidayPayPolicy: { companyUuid: 'company-123', version: 'abc123' },
115108
})
109+
mockHolidayPolicyEmployees.length = 0
116110
})
117111

118112
it('renders employee names from API data', async () => {
@@ -150,10 +144,12 @@ describe('SelectEmployeesHoliday', () => {
150144
const input = screen.getByPlaceholderText('searchPlaceholder')
151145
await user.type(input, 'bob')
152146

147+
// useClientPagination debounces search by 120ms before the predicate runs,
148+
// so wait for the filter to settle rather than asserting immediately.
153149
await waitFor(() => {
154-
expect(screen.getByText('Bob Jones')).toBeInTheDocument()
150+
expect(screen.queryByText('Alice Smith')).not.toBeInTheDocument()
155151
})
156-
expect(screen.queryByText('Alice Smith')).not.toBeInTheDocument()
152+
expect(screen.getByText('Bob Jones')).toBeInTheDocument()
157153
})
158154

159155
it('fires CANCEL when Back is clicked', async () => {
@@ -205,7 +201,7 @@ describe('SelectEmployeesHoliday', () => {
205201
const user = userEvent.setup()
206202
renderComponent({ mode: 'standalone' })
207203

208-
// 1 select-all header + 2 employees = 3 checkboxes
204+
// select-all header + 2 employees = 3
209205
await waitFor(() => {
210206
expect(screen.getAllByRole('checkbox').length).toBe(3)
211207
})
@@ -227,52 +223,40 @@ describe('SelectEmployeesHoliday', () => {
227223
})
228224
})
229225
})
230-
})
231226

232-
describe('pre-selection', () => {
233-
it('pre-selects employees already on the holiday policy', async () => {
234-
mockHolidayPolicyEmployees.length = 0
235-
mockHolidayPolicyEmployees.push({ uuid: '1' })
227+
it('emits DONE without calling mutation when nothing is selected', async () => {
228+
const user = userEvent.setup()
229+
renderComponent({ mode: 'standalone' })
236230

237-
try {
238-
renderComponent({ mode: 'standalone' })
231+
await waitFor(() => {
232+
expect(screen.getByRole('button', { name: 'continueCta' })).toBeInTheDocument()
233+
})
239234

240-
await waitFor(() => {
241-
expect(screen.getAllByRole('checkbox').length).toBe(3)
242-
})
235+
await user.click(screen.getByRole('button', { name: 'continueCta' }))
243236

244-
const checkboxes = screen.getAllByRole('checkbox') as HTMLInputElement[]
245-
expect(checkboxes[FIRST_EMPLOYEE_CHECKBOX]).toBeChecked()
246-
expect(checkboxes[SECOND_EMPLOYEE_CHECKBOX]).not.toBeChecked()
247-
} finally {
248-
mockHolidayPolicyEmployees.length = 0
249-
}
237+
await waitFor(() => {
238+
expect(mockOnEvent).toHaveBeenCalledWith(
239+
componentEvents.TIME_OFF_HOLIDAY_ADD_EMPLOYEES_DONE,
240+
)
241+
})
242+
expect(mockAddEmployees).not.toHaveBeenCalled()
250243
})
244+
})
251245

252-
it('emits done event without calling mutations when nothing changed', async () => {
253-
mockHolidayPolicyEmployees.length = 0
246+
describe('existing assignees', () => {
247+
it('filters existing holiday policy assignees out of the selectable list', async () => {
254248
mockHolidayPolicyEmployees.push({ uuid: '1' })
255249

256-
try {
257-
const user = userEvent.setup()
258-
renderComponent({ mode: 'standalone' })
259-
260-
await waitFor(() => {
261-
expect(screen.getByRole('button', { name: 'continueCta' })).toBeInTheDocument()
262-
})
263-
264-
await user.click(screen.getByRole('button', { name: 'continueCta' }))
250+
renderComponent({ mode: 'standalone' })
265251

266-
await waitFor(() => {
267-
expect(mockOnEvent).toHaveBeenCalledWith(
268-
componentEvents.TIME_OFF_HOLIDAY_ADD_EMPLOYEES_DONE,
269-
)
270-
})
252+
await waitFor(() => {
253+
expect(screen.getByText('Bob Jones')).toBeInTheDocument()
254+
})
271255

272-
expect(mockAddEmployees).not.toHaveBeenCalled()
273-
} finally {
274-
mockHolidayPolicyEmployees.length = 0
275-
}
256+
// Alice is already on the holiday policy and must not appear in the add list
257+
expect(screen.queryByText('Alice Smith')).not.toBeInTheDocument()
258+
// select-all header + Bob = 2 (Alice already on policy)
259+
expect(screen.getAllByRole('checkbox').length).toBe(2)
276260
})
277261
})
278262

@@ -302,7 +286,7 @@ describe('SelectEmployeesHoliday', () => {
302286
const user = userEvent.setup()
303287
renderComponent({ mode: 'wizard' })
304288

305-
// 1 select-all header + 2 employees = 3 checkboxes
289+
// select-all header + 2 employees = 3
306290
await waitFor(() => {
307291
expect(screen.getAllByRole('checkbox').length).toBe(3)
308292
})

0 commit comments

Comments
 (0)