Fix Go todo list status filters for completed and lifecycle states#243
Fix Go todo list status filters for completed and lifecycle states#243robzolkos wants to merge 2 commits intobasecamp:mainfrom
Conversation
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the Go todos wrapper to correctly translate filter values into the query parameters expected by the Basecamp API. The issue was that TodoListOptions.Status exposed completion filtering, but the BC3 API uses two separate query parameters: status=active|archived|trashed for lifecycle states and completed=true for completed items.
Changes:
- Map
Status: "completed"tocompleted=truequery parameter - Treat
"pending"and"incomplete"as API defaults (omit both query parameters) - Pass lifecycle statuses (
"active","archived","trashed") through thestatusparameter - Update documentation to clarify supported status values
- Add comprehensive test coverage for the three filtering scenarios
- Update examples to demonstrate the filtering functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go/pkg/basecamp/todos.go | Updated TodoListOptions struct documentation and refactored List method to properly translate filter values to API parameters |
| go/pkg/basecamp/todos_test.go | Added test cases for lifecycle statuses and three service-level tests verifying the parameter translation logic |
| go/pkg/basecamp/example_test.go | Updated example to demonstrate the "completed" filter |
| go/pkg/basecamp/doc.go | Updated documentation with examples showing both default and filtered todo listing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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="go/pkg/basecamp/todos_test.go">
<violation number="1" location="go/pkg/basecamp/todos_test.go:1017">
P3: Use r.URL.Query().Has("status") / Has("completed") to assert absence. Get returns "" for both missing and empty values, so this test can’t reliably detect that the query param was omitted.
(Based on your team's feedback about checking query param absence with Has instead of Get.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Fixed — updated the todo query omission assertions to use URL.Query().Has for presence checks instead of relying on Get returning an empty string. |
Summary
The Go todos wrapper exposed completion filtering through
TodoListOptions.Status, but the Basecamp todos endpoint usescompleted=truefor completed items andstatus=active|archived|trashedfor lifecycle state. This PR keeps the existing Go API intact while translating those filters into the query parameters the API actually expects, so todo listing returns the correct results for completed and lifecycle-filtered requests.Refs basecamp/basecamp-cli#403
Changes
Status: "completed"tocompleted=truependingandincompleteas the default API behavior by omitting both query paramsactive,archived, andtrashedthroughstatusSummary by cubic
Fixes todo list filtering in the Go client so completed and lifecycle states return correct results.
TodoListOptions.Statusnow maps to the API’scompletedandstatusquery params, and docs/examples clarify that todo APIs taketodolistID/todoIDwithoutprojectID.Status: "completed"tocompleted=true; omit params forpending/incomplete(API default)."active","archived","trashed") viastatus.projectIDfor todos, add completed list example) and add tests for completed, pending/incomplete, and lifecycle filters.Written for commit 1c86f1e. Summary will update on new commits.