Skip to content

fix: clarify policy deletion error when pending requests exist#1906

Open
krisxcrash wants to merge 3 commits into
mainfrom
kw/timeoff-delete-policy-error-clarity
Open

fix: clarify policy deletion error when pending requests exist#1906
krisxcrash wants to merge 3 commits into
mainfrom
kw/timeoff-delete-policy-error-clarity

Conversation

@krisxcrash
Copy link
Copy Markdown
Contributor

Summary

  • Catches the 422 error from the deactivate-policy endpoint and checks for pending/approved request mentions.
  • Replaces the raw API text with a clear, actionable message explaining that requests must be declined/cancelled before deletion.
  • Adds i18n keys for deletion-related error messages.

Fixes bug: unclear "there are pending or approved time off requests" error on policy deletion.

Test plan

  • Existing PolicyList.test.tsx passes (26/26)
  • Manually verify: attempt to delete a policy with pending requests → clear message about what action to take

The raw API error about pending/approved time off requests was confusing.
Now catches the 422 and provides a clear, actionable message explaining
what needs to be done before the policy can be deleted.
@krisxcrash krisxcrash self-assigned this May 21, 2026
@krisxcrash krisxcrash marked this pull request as ready for review May 21, 2026 23:34
@krisxcrash krisxcrash requested a review from a team as a code owner May 21, 2026 23:34
@krisxcrash krisxcrash enabled auto-merge (squash) May 21, 2026 23:34
@gusto-fresh-eyes
Copy link
Copy Markdown

Fresh Eyes Review

Found 2 issues in this PR.

PR Description Issues

  • Minor | [fresh_eyes]: description-check: Testing section mentions existing tests pass and manual verification, but no new automated test covers the error-handling path (the try/catch with UnprocessableEntityError detection). For a user-facing behavior change, a unit test asserting the friendly error message appears when the API returns a 422 with 'pending'/'approved' would strengthen confidence.
Info (1)
  • 🔵 Info | [fresh_eyes]: test-coverage | src/components/TimeOff/PolicyList/PolicyList.tsx:L155-L161
    The existing test for the 'pending requests' 422 error path (line 438 in test file) verifies the dialog stays open but does not assert the new user-friendly translated error message ('errors.pendingRequestsBlockDeletion') is rendered to the user. Consider adding an assertion that the clarified message appears.

Download findings.json — drag the file into Claude or use /add to propose fixes


Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews

Comment on lines +163 to +165
throw new SDKInternalError(messages.join('. ') || t('errors.deleteFailed'), 'api_error')
}
throw err
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor | [fresh_eyes]: test-coverage

New code path untested: when a 422 error does NOT contain 'pending'/'approved' keywords, the code concatenates error messages or falls back to t('errors.deleteFailed'). No test covers this branch.


Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant