fix(float): fix create_time_off people_ids field and logged time array response#350
fix(float): fix create_time_off people_ids field and logged time array response#350Shubhank-Jonnada wants to merge 5 commits into
Conversation
🔍 Integration Validation ResultsCommit: Changed directories:
✅ Structure Check output✅ Code Check output✅ Tests Check output✅ README Check output
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4967e82143
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| result = await float_integration.execute_action( | ||
| "create_time_off", |
There was a problem hiding this comment.
Clean up created time off in the destructive test
When the documented destructive suite is run, this call creates a real time-off entry for the first person but the test never captures the timeoff_id for cleanup or calls delete_time_off afterward. Each run leaves a July 1, 2026 absence in the connected Float account, which can affect schedules/reports and future test runs; please make this a lifecycle test or delete the created entry in a cleanup path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed create_time_off to send people_ids as an array and unwrapped the array response from create_logged_time/update_logged_time - 306ec07
…nd logged time lifecycle tests
TheRealAgentK
left a comment
There was a problem hiding this comment.
Integration PR review
Applied sub-skills: building-integration, writing-unit-tests, writing-integration-tests
🚫 Blockers
None found.
⚠️ Should-fix
float/tests/test_float_integration.pynew destructive tests: cleanup only happens at the end of the happy path. If an assertion fails after creating a time-off or logged-time record, the test can leave real data behind. Please wrap cleanup intry/finallyoncetimeoff_id/logged_time_idis known.float/tests/test_float_integration.pynew destructive tests: they use the first real person/project plus a fixed date (2026-07-01). That can collide with sandbox data and make reruns flaky. Prefer test-specific resources from env vars, disposable resources created by the test, or at least a generated future date to reduce collisions.float/float.pylogged-time array unwrap: this new behavior is currently only covered by destructive integration tests. Please add focused unit tests withFetchResponse(data=[...])for bothcreate_logged_timeandupdate_logged_timeso the regression is caught without real API credentials. There currently appears to be no unit coverage forupdate_logged_time.float/config.jsoncreate_time_offoutput schema still documentspeople_id, while the new live test assertspeople_ids. If Float now returnspeople_ids, please include that array field so the public action contract matches successful output.
💡 Suggestions
- For the logged-time unwrap, if Float unexpectedly returns an empty list,
response.data[0]will become anIndexErrorand then anActionError. A clearer guard/message would make failures easier to debug.
Validation
- Inspected GitHub checks: green; merge state appears blocked for non-code reasons.
- Ran locally:
validate_integration.py float✅check_code.py --base-ref origin/master float✅pytest float/ -v✅119 passedcheck_readme.py origin/master float✅check_version_bump.py origin/master float✅ with warning suggesting minor bump due detected new test classes/features
- Integration tests were invoked but skipped locally because
FLOAT_API_KEYis not set:pytest float/tests/test_float_integration.py -m "integration and not destructive" -qpytest float/tests/test_float_integration.py -m "integration and destructive" -q
- Manual diff/security review found no committed secrets; Bandit/security checks passed. GitHub secret scanning could not be run here because GHAS is not enabled for the repo.
Summary
Two bugs in the Float integration were causing action failures at runtime. Both were caught via live API testing against the Float sandbox and fixed at the source.
What was broken
create_time_off— wrong field nameThe Float API expects
people_ids(an array), but the integration was sendingpeople_id(a single integer). This caused a422every time: "People IDs cannot be blank." Fixed by wrapping the value:"people_ids": [inputs["people_id"]].create_logged_time/update_logged_time— array response not unwrappedFloat returns these mutations as
[{...}]instead of{...}. The integration was passing the raw array straight through, which caused a schema validation error even though the data was being written correctly on Float's side. Fixed by unwrapping:response.data[0] if isinstance(response.data, list) else response.data.GET /logged-time/{id}returns an object and didn't need touching.Changes
float/float.py— two targeted fixes, nothing else touchedfloat/config.json— version bumped2.0.0→2.0.1float/tests/test_float_timeoff_unit.py— updated assertion to match new field namefloat/tests/test_float_integration.py— new integration test suite (see below)Tests
Read-only (safe to run anytime):
pytest float/tests/test_float_integration.py -m "integration and not destructive"Destructive (creates/deletes real data on the connected account):
pytest float/tests/test_float_integration.py -m "integration and destructive"Set
FLOAT_API_KEYin.envbefore running.