feat(offboarding): mark checklist steps as exceptions with a reason#3490
Open
Marfuen wants to merge 1 commit into
Open
feat(offboarding): mark checklist steps as exceptions with a reason#3490Marfuen wants to merge 1 commit into
Marfuen wants to merge 1 commit into
Conversation
Some offboarding steps can't be fulfilled for a given person (e.g. "Retrieve company devices" when they never had one), leaving the checklist permanently incomplete. Add an exception resolution: a required free-text reason marks a step as handled without evidence, so it clears "remaining" and stops blocking completion, while staying visually and semantically distinct from a real completion. Backend: - OffboardingChecklistCompletion gains isException + exceptionReason (additive migration). The unique (memberId, templateItemId) row is the single resolution slot, so a step is completed XOR excepted. - New POST .../member/:memberId/item/:templateItemId/exception (member:update, auto-audited). markException skips the evidence requirement and rejects the access-revocation step. getMemberChecklist surfaces the fields; an exception counts as resolved. - Evidence export summary CSV reports an "Exception" status + the reason in a new "Exception Reason" column — the audit artifact for a skipped step. Frontend: - "Mark as exception" action with an inline required-reason form, an Exception badge, a distinct status indicator, the reason shown in the row, and a "Remove exception" action. - Split the oversized OffboardingChecklistItem into focused indicator/content modules (413 -> 181 lines). Tests (TDD): service, controller, DTO, summary CSV, and the exception form + item behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 18 files
Confidence score: 2/5
apps/api/src/offboarding-checklist/offboarding-checklist.service.tsis missing a member tenant/existence guard on the exception path, so a user withmember:updatecould write exception completions for another org (or nonexistent members), creating cross-tenant data integrity/security impact if merged as-is — add the same tenant/member validation used in the normal completion flow before merging.apps/api/src/offboarding-checklist/offboarding-checklist.service.tscan throw an uncaught PrismaP2002during concurrent exception requests, returning HTTP 500 instead of the endpoint’s expected client error behavior and causing avoidable retries/noisy failures — catch and translate the unique-constraint race to the intended 400/idempotent response.OffboardingChecklist.tsxandExceptionReasonForm.tsxcurrently close the reason form even when mark-exception fails, and the submit promise can be left unobserved, so users may think the exception saved while losing their typed reason — await/handleonSubmit, keep the form open on failure, and show inline error feedback.offboarding-item-content.tsxuses a clickable<div>for evidence upload without keyboard activation, which blocks keyboard-only users from opening the picker — add proper button/label semantics with Enter/Space support before release.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklist.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklist.tsx:70">
P2: When marking an exception fails, the inline reason form is still closed, so the user can believe the exception was saved and lose the entered reason. The catch consumes the rejection while the child closes the form after a resolved callback; rethrowing after the toast would keep failed saves visible and retryable.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/ExceptionReasonForm.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/ExceptionReasonForm.tsx:38">
P2: A failed mark-exception request produces an unhandled promise rejection because this click handler does not observe the promise returned by `onSubmit`; the parent callback propagates API errors. The form should expose/handle submission errors (or otherwise catch and surface the rejection) so users can retry without a console/runtime error.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/offboarding-item-content.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/offboarding-item-content.tsx:188">
P2: Keyboard-only users cannot open the evidence picker because this upload dropzone is a clickable `<div>` without focusability or keyboard activation. Adding button semantics with Enter/Space handling, or using a `<label>` associated with the hidden input, keeps evidence upload accessible.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:280">
P1: A caller with `member:update` permission can mark a member from another organization—or a nonexistent member—as an exception because this path has no member tenant check; this can create cross-tenant completion rows or return a 500 instead of a not-found response. Validating `Member` with both `id` and `organizationId` before the completion lookup/create would keep this endpoint tenant-scoped.
(Based on your team's feedback about tenant scoping in the service layer.) [FEEDBACK_USED].</violation>
<violation number="2" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:288">
P3: The new exception path duplicates the existing completion/template lookup logic in `completeItem`, which creates a second place for resolution eligibility and tenant checks to drift. A shared private lookup/guard helper would keep both resolution paths aligned.</violation>
<violation number="3" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:307">
P2: Concurrent requests can both observe no completion, then the unique constraint makes the losing request return an uncaught Prisma `P2002` (HTTP 500) rather than the endpoint’s already-resolved 400. Catching the unique-conflict error around this create, or otherwise making resolution atomic, would preserve the API contract.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Comment on lines
+280
to
+282
| const existing = await db.offboardingChecklistCompletion.findFirst({ | ||
| where: { organizationId, memberId, templateItemId }, | ||
| }); |
Contributor
There was a problem hiding this comment.
P1: A caller with member:update permission can mark a member from another organization—or a nonexistent member—as an exception because this path has no member tenant check; this can create cross-tenant completion rows or return a 500 instead of a not-found response. Validating Member with both id and organizationId before the completion lookup/create would keep this endpoint tenant-scoped.
(Based on your team's feedback about tenant scoping in the service layer.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-checklist.service.ts, line 280:
<comment>A caller with `member:update` permission can mark a member from another organization—or a nonexistent member—as an exception because this path has no member tenant check; this can create cross-tenant completion rows or return a 500 instead of a not-found response. Validating `Member` with both `id` and `organizationId` before the completion lookup/create would keep this endpoint tenant-scoped.
(Based on your team's feedback about tenant scoping in the service layer.) .</comment>
<file context>
@@ -262,6 +264,58 @@ export class OffboardingChecklistService {
+ completedById: string;
+ reason: string;
+ }) {
+ const existing = await db.offboardingChecklistCompletion.findFirst({
+ where: { organizationId, memberId, templateItemId },
+ });
</file context>
Suggested change
| const existing = await db.offboardingChecklistCompletion.findFirst({ | |
| where: { organizationId, memberId, templateItemId }, | |
| }); | |
| const member = await db.member.findFirst({ | |
| where: { id: memberId, organizationId }, | |
| select: { id: true }, | |
| }); | |
| if (!member) { | |
| throw new NotFoundException('Member not found'); | |
| } | |
| const existing = await db.offboardingChecklistCompletion.findFirst({ | |
| where: { organizationId, memberId, templateItemId }, | |
| }); |
| await markException({ templateItemId, reason }); | ||
| toast.success('Item marked as exception'); | ||
| } catch { | ||
| toast.error('Failed to mark item as exception'); |
Contributor
There was a problem hiding this comment.
P2: When marking an exception fails, the inline reason form is still closed, so the user can believe the exception was saved and lose the entered reason. The catch consumes the rejection while the child closes the form after a resolved callback; rethrowing after the toast would keep failed saves visible and retryable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklist.tsx, line 70:
<comment>When marking an exception fails, the inline reason form is still closed, so the user can believe the exception was saved and lose the entered reason. The catch consumes the rejection while the child closes the form after a resolved callback; rethrowing after the toast would keep failed saves visible and retryable.</comment>
<file context>
@@ -54,6 +55,24 @@ export function OffboardingChecklist({
+ await markException({ templateItemId, reason });
+ toast.success('Item marked as exception');
+ } catch {
+ toast.error('Failed to mark item as exception');
+ }
+ },
</file context>
| <HStack gap="2"> | ||
| <Button | ||
| size="sm" | ||
| onClick={() => onSubmit(trimmed)} |
Contributor
There was a problem hiding this comment.
P2: A failed mark-exception request produces an unhandled promise rejection because this click handler does not observe the promise returned by onSubmit; the parent callback propagates API errors. The form should expose/handle submission errors (or otherwise catch and surface the rejection) so users can retry without a console/runtime error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/ExceptionReasonForm.tsx, line 38:
<comment>A failed mark-exception request produces an unhandled promise rejection because this click handler does not observe the promise returned by `onSubmit`; the parent callback propagates API errors. The form should expose/handle submission errors (or otherwise catch and surface the rejection) so users can retry without a console/runtime error.</comment>
<file context>
@@ -0,0 +1,55 @@
+ <HStack gap="2">
+ <Button
+ size="sm"
+ onClick={() => onSubmit(trimmed)}
+ disabled={!trimmed || isSubmitting}
+ loading={isSubmitting}
</file context>
| <div | ||
| onDrop={onFileDrop} | ||
| onDragOver={(e) => e.preventDefault()} | ||
| onClick={() => !isProcessing && dropzoneInputRef.current?.click()} |
Contributor
There was a problem hiding this comment.
P2: Keyboard-only users cannot open the evidence picker because this upload dropzone is a clickable <div> without focusability or keyboard activation. Adding button semantics with Enter/Space handling, or using a <label> associated with the hidden input, keeps evidence upload accessible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/offboarding-item-content.tsx, line 188:
<comment>Keyboard-only users cannot open the evidence picker because this upload dropzone is a clickable `<div>` without focusability or keyboard activation. Adding button semantics with Enter/Space handling, or using a `<label>` associated with the hidden input, keeps evidence upload accessible.</comment>
<file context>
@@ -0,0 +1,248 @@
+ <div
+ onDrop={onFileDrop}
+ onDragOver={(e) => e.preventDefault()}
+ onClick={() => !isProcessing && dropzoneInputRef.current?.click()}
+ className={`flex cursor-pointer flex-col items-center gap-2 rounded-md border-2 border-dashed border-muted-foreground/25 px-4 py-6 text-center transition hover:border-muted-foreground/50 hover:bg-muted/25${isProcessing ? ' pointer-events-none opacity-50' : ''}`}
+ >
</file context>
Comment on lines
+307
to
+316
| return db.offboardingChecklistCompletion.create({ | ||
| data: { | ||
| organizationId, | ||
| memberId, | ||
| templateItemId, | ||
| completedById, | ||
| isException: true, | ||
| exceptionReason: reason.trim(), | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
P2: Concurrent requests can both observe no completion, then the unique constraint makes the losing request return an uncaught Prisma P2002 (HTTP 500) rather than the endpoint’s already-resolved 400. Catching the unique-conflict error around this create, or otherwise making resolution atomic, would preserve the API contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-checklist.service.ts, line 307:
<comment>Concurrent requests can both observe no completion, then the unique constraint makes the losing request return an uncaught Prisma `P2002` (HTTP 500) rather than the endpoint’s already-resolved 400. Catching the unique-conflict error around this create, or otherwise making resolution atomic, would preserve the API contract.</comment>
<file context>
@@ -262,6 +264,58 @@ export class OffboardingChecklistService {
+ // An exception resolves the step without evidence (that's the point — the
+ // step could not or need not be done), so the evidenceRequired check that
+ // completeItem enforces is intentionally skipped.
+ return db.offboardingChecklistCompletion.create({
+ data: {
+ organizationId,
</file context>
Suggested change
| return db.offboardingChecklistCompletion.create({ | |
| data: { | |
| organizationId, | |
| memberId, | |
| templateItemId, | |
| completedById, | |
| isException: true, | |
| exceptionReason: reason.trim(), | |
| }, | |
| }); | |
| try { | |
| return await db.offboardingChecklistCompletion.create({ | |
| data: { | |
| organizationId, | |
| memberId, | |
| templateItemId, | |
| completedById, | |
| isException: true, | |
| exceptionReason: reason.trim(), | |
| }, | |
| }); | |
| } catch (err) { | |
| const isPrismaConflict = | |
| err instanceof Error && | |
| 'code' in err && | |
| (err as { code: string }).code === 'P2002'; | |
| if (isPrismaConflict) { | |
| throw new BadRequestException('Item is already resolved'); | |
| } | |
| throw err; | |
| } |
| throw new BadRequestException('Item is already resolved'); | ||
| } | ||
|
|
||
| const template = await db.offboardingChecklistTemplate.findFirst({ |
Contributor
There was a problem hiding this comment.
P3: The new exception path duplicates the existing completion/template lookup logic in completeItem, which creates a second place for resolution eligibility and tenant checks to drift. A shared private lookup/guard helper would keep both resolution paths aligned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-checklist.service.ts, line 288:
<comment>The new exception path duplicates the existing completion/template lookup logic in `completeItem`, which creates a second place for resolution eligibility and tenant checks to drift. A shared private lookup/guard helper would keep both resolution paths aligned.</comment>
<file context>
@@ -262,6 +264,58 @@ export class OffboardingChecklistService {
+ throw new BadRequestException('Item is already resolved');
+ }
+
+ const template = await db.offboardingChecklistTemplate.findFirst({
+ where: { id: templateItemId, organizationId, isEnabled: true },
+ });
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Some offboarding checklist steps can't be fulfilled for a given person — e.g. Retrieve company devices when they were never issued one. Today a step has one resolution (complete, sometimes with evidence), so an unfulfillable step stays permanently incomplete, shows as "remaining", and the offboarding never looks done. There's also no record of why it was skipped.
This adds an exception resolution: mark a step as an exception with a required free-text reason. It resolves the step (clears "remaining", counts toward progress, stops blocking completion) but is visually and semantically distinct from a real completion, and the reason is preserved for the audit log and evidence export.
Design note
An offboarding step has no per-member row until it's resolved — the
OffboardingChecklistCompletionrow is the resolution (@@unique(memberId, templateItemId)). "Completed" and "exception" are mutually-exclusive ways to resolve the same step, so this reuses that row with anisExceptiondiscriminator rather than a separate table (unlikeFindingException, which suppresses an independently-existing finding). No expiry/revoke lifecycle — "remove exception" is just deleting the row (existing uncomplete flow).Backend
OffboardingChecklistCompletiongainsisException+exceptionReason(additive migration, no backfill).POST /v1/offboarding-checklist/member/:memberId/item/:templateItemId/exception(member:update, auto-audited — the interceptor foldsreasoninto the record).markExceptionskips the evidence requirement and rejects the access-revocation step.getMemberChecklistsurfaces the fields; an exception counts as resolved.Exceptionstatus and the reason in a newException Reasoncolumn — the audit artifact for a legitimately skipped step.Frontend
OffboardingChecklistItem.tsx(413 → 181 lines) into focused indicator/content modules to stay under the 300-line limit.Testing (TDD)
markException(creates the exception row, bypasses evidence, rejects if already resolved / on the access-revocation step), controller delegation + no-user guard, DTO validation, andbuildSummaryCsv(Exception status + reason column).ExceptionReasonForm(required-reason gating, trimmed submit) andOffboardingChecklistItem(mark → form → hook call; excepted view shows reason + Remove).@trycompai/ui/lucide-reactimports.Adds a Prisma migration (
add_offboarding_checklist_exception) — runprisma migrateon deploy.🤖 Generated with Claude Code
Summary by cubic
Add an exception resolution for offboarding checklist steps with a required reason. This lets you mark unfulfillable steps as handled without evidence, counts toward progress, and is clearly shown as an exception in the UI and export.
New Features
POST /v1/offboarding-checklist/member/:memberId/item/:templateItemId/exceptionrecordsisExceptionandexceptionReason, skips evidence, and rejects the access‑revocation step.OffboardingChecklistIteminto focused indicator/content modules.Migration
prisma migrateto addisExceptionandexceptionReasontoOffboardingChecklistCompletion.Written for commit d820421. Summary will update on new commits.