feat(SDK-1161): add print payroll checks alert and modal - #2572
feat(SDK-1161): add print payroll checks alert and modal#2572krisxcrash wants to merge 11 commits into
Conversation
Employees paid by check need a way to generate printable checks from the payroll overview screen. Redesigns the check-payment alert to status="info" with a secondary "View and print checks" action (shown once the payroll is processed), which opens a new modal for choosing check stock and generating the check PDF via the existing printable-payroll-checks API. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nt payload - buildRequestBody used a truthy check on startingCheckNumber, so a value of 0 (the pre-filled default) was silently dropped from the request instead of being sent as the override. - RUN_PAYROLL_PRINT_CHECKS_FAILED now fires consistently whether the generate mutation is rejected outright or polling reports a failed status, and carries the generated document when one exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
serikjensen
left a comment
serikjensen
left a comment
There was a problem hiding this comment.
Looking good! Left some feedback here to update this to be consistent with the other Payroll modal components!
| {isProcessed && ( | ||
| <div> | ||
| <Button variant="secondary" onClick={onPrintChecksOpen}> | ||
| {t('alerts.printChecksCta')} | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
i think you can possibly set this via the action prop available on Alert? that way partners can manage this with their component adapter setup.
it'd look like
<Alert action={isProcessed && <Button ...>....</Button>} ...>...</Alert>
| </GustoTestProvider> | ||
| </I18nLoader> | ||
| </Suspense> | ||
| ), |
There was a problem hiding this comment.
Nit: it's been a bit since i've added stories, isn't there some sort of global configuration for this so we don't have to duplicate these for each story?
There was a problem hiding this comment.
Let me look into that!
| expect(generateResolver.mock.invocationCallOrder[0]!).toBeLessThan( | ||
| getDocumentResolver.mock.invocationCallOrder[0]!, | ||
| ) |
There was a problem hiding this comment.
Nit: I would push claude on this one since it is likely doing something hacky if it has to assert like this
| return ( | ||
| <Modal isOpen={isOpen} onClose={onClose} footer={footer}> | ||
| {phase === 'succeeded' && ( | ||
| <Flex flexDirection="column" gap={16}> | ||
| <Heading as="h2">{t('succeededTitle')}</Heading> | ||
| <Text variant="supporting">{t('succeededDescription')}</Text> | ||
| {documentUrl && ( | ||
| <Link href={documentUrl} target="_blank" rel="noreferrer"> | ||
| {t('viewChecksCta')} | ||
| </Link> | ||
| )} | ||
| </Flex> | ||
| )} | ||
| {phase === 'failed' && ( | ||
| <Flex flexDirection="column" gap={16}> | ||
| <Alert status="error" disableScrollIntoView label={t('failedTitle')}> | ||
| {errorMessage} | ||
| </Alert> | ||
| <Button variant="secondary" onClick={onRetry}> | ||
| {t('retryCta')} | ||
| </Button> | ||
| </Flex> | ||
| )} | ||
| {isFormPhase && ( | ||
| <FormProvider {...formMethods}> | ||
| <Form id={formId} onSubmit={formMethods.handleSubmit(onSubmit)}> | ||
| <Flex flexDirection="column" gap={20}> | ||
| <RadioGroupField | ||
| name="printingFormat" | ||
| label={t('modalTitle')} | ||
| options={printingFormatOptions} | ||
| isRequired | ||
| isDisabled={isGenerating} | ||
| /> | ||
| {printingFormat === PrintingFormat.Bottom && ( | ||
| <NumberInputField | ||
| name="startingCheckNumber" | ||
| label={t('startingCheckNumberLabel')} | ||
| description={t('startingCheckNumberDescription')} | ||
| errorMessage={startingCheckNumberErrorMessage} | ||
| isDisabled={isGenerating} | ||
| min={0} | ||
| /> | ||
| )} | ||
| </Flex> | ||
| </Form> | ||
| </FormProvider> | ||
| )} | ||
| </Modal> |
There was a problem hiding this comment.
A few things on this one that we need to think about
- We end up tracking the progress through the modal and transitioning screens by phase, i would recommend navigating that via state machine as the de facto way of managing screen transitions
- We end up needing to do manual tracking of retries and form reset because the modal opens and closes but the content never unmounts
- We also are unable to export the individual pieces of this print checks experience for usage in case a partner wants to utilize those components
I think the best reference implementation to claude to work with for this is the ConfirmWireDetails or RecoveryCases components inside payroll
Those ones will show precedent for
- Creating a modal that will just open close
- Placing a BaseComponent as the root component of the modal content
- Having api calls occur within the modal so that we can catch errors and display them inside the modal and use the existing BaseComponent error handling consistent with other components
- Managing the transitions between screens with a lightweight state machine
- Allow for exporting the individual component pieces in isolation which are then connected by the state machine
I would recommend just having those components manage their api calls internally rather than centralizing the functionality in a hook. If we do provide hooks at this point, we'd want them to align with the existing codebase hooks patterns which is likely more than you'd want to take on right now
So in the end you'd end up with
- PrintChecks <- Creates state machine, renders triggering banner with CTA and modal
- PrintChecksBanner <- Banner component renders the button and the banner launch
- PrintChecksForm
- PrintChecksFailure <- Failure state when the polling indicates a failure status
- PrintChecksSummary <- Success, link to other checks
Lmk if you'd like to pair on this!
| }) | ||
|
|
||
| /** @internal */ | ||
| export function usePrintChecksModal({ |
There was a problem hiding this comment.
See my longer comment, my rec would be to get away from a hook for this implementation as it will end up increasing the scope here. We likely only want a hook if this is being used in multiple locations and this is an internal helper to centralize some duplicated logic. Or, alternatively, if we want to make the hook available to partners, in which case we'd want to make sure this aligns with the hook guidelines. I don't think that's necessarily worth worrying about this pass
| useEffect(() => { | ||
| if (!isOpen) { | ||
| formMethods.reset({ printingFormat: PrintingFormat.Top, startingCheckNumber: 0 }) | ||
| setPhase('form') | ||
| setRequestUuid(null) | ||
| setDocumentUrl(null) | ||
| setErrorMessage(null) | ||
| setIsPolling(false) | ||
| printWindowRef.current?.close() | ||
| printWindowRef.current = null | ||
| } | ||
| }, [isOpen, formMethods.reset]) |
There was a problem hiding this comment.
This goes away with my recommendation to create dedicated standalone components that only get rendered when the modal mounts. Modal is rendering always in this case which means the component is always mounted for this which is why we need the manual reset.
…ine component Rebuilds PrintChecksModal into Payroll.PrintChecks, following the same robot3 state-machine pattern as ConfirmWireDetails/RecoveryCases per review feedback: a top-level orchestrator drives standalone PrintChecksBanner/Form/Failure/Summary pieces (each owning its own API calls via BaseComponent) instead of one hook + a phase-driven presentation component. Unmounting on transition replaces the old manual reset-on-close effect, and the alert now uses Alert's `action` prop instead of a manually nested button. Also: - Rename the payrollUuid prop to payrollId so the SDK Dev App's static registry analysis picks it up as an auto-provisioned entity ID (matches every other public Payroll component's convention). - Drop the window.open()/window.location popup-priming in PrintChecksForm — gws-flows never opens a JS-triggered window; it redirects an already-open tab. Since our form lives in a modal with no separate tab, we rely on the existing "View checks" link on the summary screen instead of trying to synthesize one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…etch The previous approach fetched the generated checks PDF client-side and downloaded it via a blob: URL to sidestep browsers ignoring the `download` attribute on cross-origin links. In practice this broke downloads entirely: the signed S3 URL doesn't allow cross-origin fetch() (no Access-Control-Allow-Origin), so the request failed with a CORS error before the blob could ever be created. The URL already has `response-content-disposition: attachment` baked into its query string, so S3 forces the download via that response header on a direct browser-level request — which isn't subject to CORS the way a JS fetch() is. Switch to a synthetic anchor click pointed straight at the URL; this still never opens a new tab or navigates the host page, since the browser intercepts the download instead of rendering a response. Also corrects the summary screen's copy, which referenced the old "opens in a new tab" behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both were previously only exercised indirectly via PrintChecks.test.tsx's end-to-end success/failure flow assertions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Splitting this into a stack of smaller, more reviewable PRs per feedback on this PR's size:
Each PR is stacked on the previous and was verified to build/lint/test independently. Closing this one in favor of that stack. |
2 participants
Summary
Employees paid by check need a way to generate printable checks from the payroll overview screen. This redesigns the existing check-payment alert (
status="warning"→status="info", same copy) and adds a "View and print checks" action, shown once the payroll is processed, that opens a new modal for choosing check stock and generating the check PDF via the existing printable-payroll-checks API.Changes
PrintChecksModal(hook + presentation + container) handling the check-stock choice, the conditional starting-check-number field, and the generate → poll → PDF flow viausePayrollsGeneratePrintableChecksMutation/useGeneratedDocumentsGet(already available at the current pinned API version, no bump needed)PayrollOverviewPresentation's check-payment alert switched tostatus="info", with the button placed in its own row below the description, matching the established alert+CTA pattern elsewhere in the codebasePayroll.PrintChecksModali18n namespace; three newrunPayroll/printChecks/*eventsRelated
Testing
npm run test -- --run— full suite passes (336 files / 3765 tests)npm run buildandnpm run lint:check— cleanDomain/Payroll/PrintChecksModal(Default, BlankCheckStockSelected, Generating, Succeeded, Failed) andDomain/Payroll/PayrollOverview(WithCheckPaymentEmployeeUnprocessed, WithCheckPaymentEmployeeProcessed)Screenshots