Validate timesheet uuid flags - #175
Conversation
Signed-off-by: Cody Bentosino <cody.bentosino@gusto.com>
Signed-off-by: Cody Bentosino <cody.bentosino@gusto.com>
Signed-off-by: Cody Bentosino <cody.bentosino@gusto.com>
tmfahey
left a comment
There was a problem hiding this comment.
Collecting the format errors into the existing validators instead of failing on the first bad flag is the right call - a caller who typos three uuids shouldn't need three round trips to find out.
Two things worth a look: --company-uuid is guarded on the flag but not on the GUSTO_COMPANY_UUID/session path that actually resolves it, and the older flag-level uuid check in contractor-payment.ts now words the same failure differently than validateUuid does. Two smaller notes inline.
|
|
||
| export function validateTimesheetList(opts: TimesheetListInput): TimesheetListValidation { | ||
| const blocked: BlockedOn[] = []; | ||
| pushUuidBlockedOn("company-uuid", opts.companyUuid, blocked); |
There was a problem hiding this comment.
The flag is guarded but the env var isn't, so --company-uuid co-1 fails fast while GUSTO_COMPANY_UUID=co-1 still gets spliced into the request and comes back as whatever the API says about co-1. timesheetListHandler picks up the fallback at src/commands/timesheet.ts:492, and create/sync do it at src/lib/api-context.ts:268 (which then builds /v1/companies/${companyUuid}/...), both after this validator has already run.
Those two lines are the only non-test callers of getCompanyUuid, so checking there instead covers the flag, the env var and the session fallback in one place - and every other command's --company-uuid with it. Given the env var is the documented way to set the company, I'd rather have the check at the resolution point than on the flag.
|
|
||
| /** blocked_on entry for a UUID-typed flag, or null when absent or valid. Absence is the caller's | ||
| * required-check to make, so a validator can collect format and presence errors in one envelope. */ | ||
| export function validateUuid(field: string, value: string | undefined): BlockedOn | null { |
There was a problem hiding this comment.
There's one older flag-level uuid check this doesn't absorb: src/commands/contractor-payment.ts:196 pushes a bare must be a valid UUID with no value echoed, so the same typo reports two different ways depending on which command you hit. Worth folding it into validateUuid, which probably means pushUuidBlockedOn moves out of timesheet.ts and lands next to this so other commands can reach it.
| export type TimesheetCreateValidation = ValidationResult<TimesheetCreateBody>; | ||
|
|
||
| interface TimesheetCreateInput { | ||
| companyUuid?: string; |
There was a problem hiding this comment.
Now that the *Input interfaces carry companyUuid, the three *Opts interfaces that extend them redeclare it for no reason (lines 255, 263, 281). Three lines to drop.
| const JOB_UUID = "1f2e3d4c-0000-1111-2222-333344445555"; | ||
| const PAY_SCHEDULE_UUID = "1a2b3c4d-0000-1111-2222-333344445555"; | ||
| const TIME_SHEET_UUID = "7a6b5c4d-0000-1111-2222-333344445555"; | ||
| const COMPANY_UUID = "3c2b1a09-0000-1111-2222-333344445555"; |
There was a problem hiding this comment.
This exists because the shared TEST_AUTH fixture (src/lib/test-support.ts:36) hardcodes co-1, which the new check rejects. Fine to keep a local constant here, but the next command to validate --company-uuid hits the same wall, so I'd flag the fixture as a follow-up even though fixing it means touching every co-1 assertion in the nine other files that import it.
|
Looks good overall and I think it's a good change to use real UUIDs in our test fixtures. Happy to re-review once Taylor's comments are addressed — especially the env variable bypass route! |
Summary
timesheet create,sync,show, andlistnow reject a malformed UUID before sending a request. Six arguments gain validation, none of which was checked before:--employee-uuid,--contractor-uuid,--job-uuid,--pay-schedule-uuid,--company-uuid, andshow's positionaltime_sheet_uuid.validateUuidalongside the existingvalidateEnuminsrc/lib/parse.ts. It returns ablocked_onentry rather than an envelope, so the flag checks live inside the three existing validators and collect into one response instead of failing on the first bad argument. A caller who typos three flags sees all three, not one per retry.show's positional stays on the fail-fastinvalidUuidpath, matching the othershowcommands.invalidUuid'shintbecomes optional andshowpasses none:timesheet listonly returns time sheet uuids for companies on third-party time tracking, so there is no single command to name, andvalidationFailurealready documents omitting a hint over offering a generic one.invalidUuidnow shares its reason string withvalidateUuidinstead of duplicating the echo-and-truncate logic, so the two surfaces cannot word the same defect differently.Test fixtures move from short placeholders (
emp-1,ps-1,ts-1,co-1) to real-shaped UUID constants, which is most of the diff intimesheet.test.ts.Test plan
bun run test:allpasses locally--agentand--humanoutput verified where touchedEvery guard was mutation-tested individually: each was neutered in turn to confirm specific tests fail, then restored. The sandbox box is unchecked because all six paths short-circuit before any request is made, so there is nothing for a sandbox run to exercise beyond what the local run already covers.
Two pre-existing tests were checked for silent degradation.
show's 404 test passed a non-uuid, which would now be rejected before the request and pass for the wrong reason; it takes a well-formed uuid and asserts the request was made. A smoke test asserting thegetalias reaches the show handler pinned exit 3 (auth) and now needs a real uuid to get there, matching the rows for the other commands that validate.DCO
git commit -s) per the DCO🤖 Generated with Claude Code