fix: block switching between unlimited and accrual-based time off policy types#1900
Conversation
krisxcrash
force-pushed
the
kw/timeoff-sick-to-unlimited
branch
from
70fc8a0 to
b66082d
Compare
krisxcrash
changed the title
Fresh Eyes ReviewFound 1 issue in this PR. Download findings.json — drag the file into Claude or use Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews |
![gusto-fresh-eyes[bot]](https://avatars.githubusercontent.com/in/3457483?s=60&v=4){kind=small}
| @@ -166,7 +166,6 @@ function buildUpdateRequestBody( | |||
| accrualRate: null, | |||
| accrualRateUnit: null, | |||
| policyResetDate: null, | |||
There was a problem hiding this comment.
🟡 Minor | [fresh_eyes]: description-check
Undocumented behavioral change: complete: true was removed from the request body in buildUpdateRequestBody when switching to unlimited. This changes what gets sent to the API (previously the policy was marked complete on switch-to-unlimited). The change makes sense given that switching is now only possible for incomplete policies, but this wire-format change should be noted in the PR description.
Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews
There was a problem hiding this comment.
Intentional change. With the new locking logic, only incomplete policies can reach this code path (complete policies have the "Unlimited" radio disabled). Sending complete: true for an incomplete policy would incorrectly mark it as complete just because the user selected unlimited accrual — completeness is handled elsewhere in the creation wizard flow.
| "rate":string; | ||
| "nonZeroRate":string; | ||
| "rateExemptThreshold":string; | ||
| "title":string; |
There was a problem hiding this comment.
🟡 Minor | [fresh_eyes]: description-check
Undocumented change: Two unrelated translation keys (effectiveDate and effectiveDateBeforeHire) were removed from the EmployeeCompensation type. This is unrelated to the time off policy changes described in the PR. Likely a side effect of running npm run i18n:generate, but should be acknowledged in the description or split out.
Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews
There was a problem hiding this comment.
This is a correct deduplication from running npm run i18n:generate. The source JSON (Employee.Compensation.json) has duplicate keys for effectiveDate and effectiveDateBeforeHire in the validations object (lines 68-69 and 80-81). The old generated types had duplicate interface properties — regenerating correctly produces one entry per key. No translation was actually removed; the runtime behavior is unchanged (JSON.parse uses the last occurrence).
jeffredodd
force-pushed
the
kw/timeoff-sick-to-unlimited
branch
from
b66082d to
a1eec68
Compare
jeffredodd
force-pushed
the
kw/timeoff-sick-to-unlimited
branch
2 times, most recently
from
e6f7d7b to
b4d1b9e
Compare
…icy types When editing a complete time off policy, prevent changing between unlimited and accrual-based accrual types in the UI: - Complete unlimited policies: all accrual method radios are disabled - Complete accrual-based policies: the "Unlimited" radio is disabled, but switching between "Based on hours worked" and "Fixed amount per year" is still allowed - Incomplete policies (still in creation wizard): no restrictions This matches the behavior in gws-flows, where the assumption is that migrating settings between these fundamentally different policy types is too complex. Users should create a new policy instead.
jeffredodd
force-pushed
the
kw/timeoff-sick-to-unlimited
branch
from
b4d1b9e to
e9b6e7d
Compare
| @@ -166,7 +166,6 @@ function buildUpdateRequestBody( | |||
| accrualRate: null, | |||
| accrualRateUnit: null, | |||
| policyResetDate: null, | |||
There was a problem hiding this comment.
🟡 Minor | [fresh_eyes]: description-check
Undocumented behavioral change: removal of complete: true from buildUpdateRequestBody's unlimited branch changes what's sent to the API (incomplete policies switching to unlimited no longer get force-marked as complete). The PR description only mentions UI locking of radio buttons but doesn't explain this API-level change or why it's correct.
Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews
2 participants
Summary
This matches gws-flows behavior where switching between fundamentally different policy types is blocked because migrating all settings is too complex — users should create a new policy and reassign employees.
Applies to both sick and vacation policy types (the component handles both via
policyTypeprop).Test plan
PolicyConfigurationForm.test.tsxtests pass (45/45)