Uh oh!
There was an error while loading. Please reload this page.
Fix todo update dropping untouched fields - #413
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where partial updates to todos were dropping untouched fields. The BC3 API clears fields by omission, so when a partial update was made (e.g., only updating the due date), other fields weren't being included in the request and would be cleared by the API.
Changes:
- Modified the non-clear update path to fetch the existing todo and seed the UpdateTodoRequest with current values before applying user overrides
- Added two new tests (
TestTodosUpdateDueDatePreservesExistingFieldsandTestTodosUpdateTitlePreservesExistingFields) to verify that partial updates preserve untouched fields
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/commands/todos.go | Seeds UpdateTodoRequest with existing todo values before applying user overrides to prevent BC3 API from clearing untouched fields |
| internal/commands/todos_test.go | Adds test coverage for due date-only and title-only updates to verify preservation of untouched fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Nmans01
commented
May 15, 2026
It would be a huge improvement to have this merged 👍 |
The non-clear update path sent only user-provided fields in the PUT body. Because the BC3 API clears fields by omission, this caused partial updates (e.g. --due alone) to silently wipe content, description, dates, and assignees. Fetch the existing todo first and seed the request with its current values before applying overrides — matching what the clear path already does.
robzolkosforce-pushed
the
fix/todo-update-preserves-fields
branch
from
June 29, 2026 15:01
666c3e8 to
49cdc81CompareUh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 17, 2026
Closed
jeremy added a commit
that referenced
this pull request
Jul 18, 2026
* Fix todos update silently clearing completion subscribers (#538) The BC3 todos PUT has replace semantics: any field omitted from the body is cleared. Neither branch of todos update included completion_subscriber_ids, so every update wiped the todo's "When done, notify" list. The #412/#413 read-modify-write merge couldn't preserve it because the SDK's Todo model drops completion_subscribers in todoFromGenerated (basecamp/basecamp-sdk#355) — the merge had nothing to carry forward. Preservation now works via a raw GET of the flat /todos/{id}.json route (which serves the field the SDK model drops), feeding the current subscriber ids into both the typed-merge and raw-clear PUT paths. The read fails closed: an HTTP error, malformed JSON, or a response missing the completion_subscribers key aborts the command before any PUT rather than risking a silent clear. The helper is commented as temporary and goes away once the SDK round-trips the field. Alongside the fix, subscribers become directly editable: - todos create/update --notify-on-completion <names or ids> sets them (comma-separated, with people-name tab completion) - todos update --no-notify-on-completion clears them (by omission — explicit intent skips the preservation read entirely) Unit tests cover preservation in both branches, the flat-route assertion, explicit set/clear bypassing the read, the fail-closed matrix (missing key / malformed JSON / HTTP 500 in each branch), flag conflicts, and the create body. A live smoke test exercises the full sequence: create with subscriber, title-only update preserves, --no-due preserves, --no-notify-on-completion clears. .surface gains the three new flag records by hand (verified identical to fresh generation); a full regeneration is deferred because the snapshot has unrelated drift since #499 changed the generator script. * Use completion-subscriber wording in --notify-on-completion errors --notify-on-completion resolved people through resolveAssigneeIDs, so failures surfaced as assignee errors ("No valid assignees provided", "Assignee ID must be a positive number") — misleading for a flag that sets completion subscribers. Parameterize the resolver with a role label: resolvePersonRoleID(s) carry the wording, and resolveAssigneeID(s) stay as assignee-labeled wrappers so existing call sites and messages are unchanged. The subscriber call sites now go through resolveCompletionSubscriberIDs. Tests cover both subscriber-worded messages and assert no PUT occurs on resolution failure. * Test the name-resolution failure path for --notify-on-completion The subscriber-wording tests covered the numeric-validation and empty-list errors but not the ResolvePerson miss, so the "failed to resolve completion subscriber" formatting was only exercised indirectly. The update mock now matches the flat preservation route exactly (/todos/999.json) instead of any .json GET, and serves /people routes an empty directory so name resolution deterministically misses. New subtest asserts the subscriber-worded resolve error and that no PUT occurs. The explicit set/clear tests tighten their no-preservation-read assertions to the exact route. * Address review: SDK error conversion + exact mock route match The preservation read's GET error now runs through convertSDKError before wrapping, so structured codes/hints (rate limit, circuit breaker) survive with the subscriber-specific context — matching every other raw Account() call site. The update mock's preservation branch now matches the account-scoped flat route exactly (/99999/todos/999.json) instead of by suffix, so a bucket-scoped GET can never satisfy it. * Reject missing or invalid subscriber ids in the preservation read A completion_subscribers element without a positive id would have put a zero into completion_subscriber_ids and reached the PUT, slipping past the fail-closed contract. The preservation read now errors on any non-positive id, and the fail-closed test matrix covers the case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
todos update --due(and other partial updates) silently wipeduntouched fields because the BC3 API clears fields by omission.
basecamp todos update <todo_id> --title "new title"wipes the existing description #458 — title-onlytodos updatepreserved the new title but cleared theexisting description and due date.
--dueflag accepted but not persisted on todo create/update (due_on stays null) #485 —--duenow persists on the update path; create-with-due is alsoverified against the current branch.
request with current values before applying user overrides, matching what the
clear path (
--no-due,--no-description) already did.--due-only and title-only updates preservecontent, description, dates, and assignees.
Live verification
Tested against the Rob Zolkos account in the Verdant Coffee Roasters project,
Fusilli todolist, using a locally built rebased branch.
--dueflag accepted but not persisted on todo create/update (due_on stays null) #485: Created a temporary todo, ranbasecamp todos update <id> --due 2027-01-15, and confirmedtodos showreturneddue_on: "2027-01-15"while preserving title and description.
basecamp todos update <todo_id> --title "new title"wipes the existing description #458: Ranbasecamp todos update <id> --title ...and confirmedtodos showpreserved the existing description and due date.
--dueflag accepted but not persisted on todo create/update (due_on stays null) #485: Created the temporary todo with--due 2026-12-31and confirmed thecreated todo returned
due_on: "2026-12-31".Fixes#412
Fixes#458
Fixes#485
Summary by cubic
Fixes partial
todos updatewiping untouched fields. The command now fetches the existing todo and seeds the PUT body so content, description, due/start dates, and assignees are preserved.--due-only and title-only updates to confirm content, description, due_on, starts_on, and assignee_ids are retained.Written for commit 49cdc81. Summary will update on new commits.