diff --git a/internal/commands/assign.go b/internal/commands/assign.go index 072b4db1..dc069b47 100644 --- a/internal/commands/assign.go +++ b/internal/commands/assign.go @@ -644,7 +644,10 @@ func doAssignStep(cmd *cobra.Command, app *appctx.App, stepIDStr, assigneeID str } assigneeIDs = append(assigneeIDs, assigneeIDInt) + // The API rejects step updates without a title, so carry over the + // current one. updated, err := app.Account().CardSteps().Update(cmd.Context(), stepID, &basecamp.UpdateStepRequest{ + Title: step.Title, AssigneeIDs: assigneeIDs, }) if err != nil { @@ -746,7 +749,10 @@ func doUnassignStep(cmd *cobra.Command, app *appctx.App, stepIDStr string, assig assigneeIDs := removeID(existingAssigneeIDs(step.Assignees), assigneeIDInt) + // The API rejects step updates without a title, so carry over the + // current one. updated, err := app.Account().CardSteps().Update(cmd.Context(), stepID, &basecamp.UpdateStepRequest{ + Title: step.Title, AssigneeIDs: assigneeIDs, }) if err != nil { diff --git a/internal/commands/assign_test.go b/internal/commands/assign_test.go index 181c7b71..1c455d88 100644 --- a/internal/commands/assign_test.go +++ b/internal/commands/assign_test.go @@ -3,6 +3,7 @@ package commands import ( "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -706,3 +707,82 @@ func TestAssignBatchAllFailNeverResolvesAssignee(t *testing.T) { "person resolution should not be attempted when all items fail validation") } } + +// mockStepAssignTransport serves the current step on GET and captures the +// PUT body so the assign/unassign step paths can be checked for the +// carried-over title the API requires. +type mockStepAssignTransport struct { + capturedPut []byte +} + +func (m *mockStepAssignTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + stepJSON := `{"id": 456, "title": "Existing step", "completed": false, "assignees": [{"id": 11, "name": "Existing Person"}]}` + + switch req.Method { + case "GET": + var body string + switch { + case strings.Contains(req.URL.Path, "/card_tables/steps/"): + body = stepJSON + case strings.Contains(req.URL.Path, "/projects.json"): + body = `[{"id": 123, "name": "Test Project"}]` + case strings.Contains(req.URL.Path, "/projects/"): + body = `{"id": 123, "dock": [{"name": "kanban_board", "id": 789, "title": "Card Table"}]}` + case strings.Contains(req.URL.Path, "/people.json"): + body = `[{"id": 11, "name": "Existing Person"}, {"id": 99, "name": "New Person"}]` + default: + return nil, fmt.Errorf("unexpected GET path: %s", req.URL.Path) + } + return &http.Response{StatusCode: 200, Body: io.NopCloser(strings.NewReader(body)), Header: header}, nil + case "PUT": + if !strings.HasSuffix(req.URL.Path, "/card_tables/steps/456") { + return nil, fmt.Errorf("unexpected PUT path: %s", req.URL.Path) + } + body, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + m.capturedPut = body + if err := req.Body.Close(); err != nil { + return nil, err + } + return &http.Response{StatusCode: 200, Body: io.NopCloser(strings.NewReader(stepJSON)), Header: header}, nil + default: + return nil, fmt.Errorf("unexpected request method: %s", req.Method) + } +} + +// TestAssignStepCarriesTitle verifies that assigning a person to a step sends +// the current title in the update, which the API requires. +func TestAssignStepCarriesTitle(t *testing.T) { + transport := &mockStepAssignTransport{} + app := setupCardsMockApp(t, transport) + + cmd := NewAssignCmd() + err := executeAssignCommand(cmd, app, "456", "--step", "--to", "99") + require.NoError(t, err) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) + assert.Equal(t, "Existing step", body["title"]) + assert.Equal(t, []any{float64(11), float64(99)}, body["assignee_ids"]) +} + +// TestUnassignStepCarriesTitle verifies that removing a person from a step +// also sends the current title in the update. +func TestUnassignStepCarriesTitle(t *testing.T) { + transport := &mockStepAssignTransport{} + app := setupCardsMockApp(t, transport) + + cmd := NewUnassignCmd() + err := executeAssignCommand(cmd, app, "456", "--step", "--from", "11") + require.NoError(t, err) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) + assert.Equal(t, "Existing step", body["title"]) + assert.Equal(t, []any{}, body["assignee_ids"]) +} diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 49f52e4a..f64361ef 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -2009,6 +2009,14 @@ You can pass either a step ID or a Basecamp URL: req := &basecamp.UpdateStepRequest{} if title != "" { req.Title = title + } else { + // The API rejects step updates without a title, so carry + // over the current one when only other fields change. + current, err := app.Account().CardSteps().Get(cmd.Context(), stepID) + if err != nil { + return convertSDKError(err) + } + req.Title = current.Title } if dueOn != "" { req.DueOn = dateparse.Parse(dueOn) diff --git a/internal/commands/cards_test.go b/internal/commands/cards_test.go index e6ba6884..473de00d 100644 --- a/internal/commands/cards_test.go +++ b/internal/commands/cards_test.go @@ -189,6 +189,93 @@ func TestCardsStepUpdateRequiresFields(t *testing.T) { assert.NoError(t, err, "expected help output, not error") } +// mockStepUpdateTransport serves the current step on GET and captures the +// update body on PUT. It only answers the single-step endpoint so a stray +// call to the wrong path fails the test instead of passing on stale data. +type mockStepUpdateTransport struct { + getCount int + capturedPut []byte +} + +func (t *mockStepUpdateTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + if !strings.HasSuffix(req.URL.Path, "/card_tables/steps/456") { + return nil, fmt.Errorf("unexpected request path: %s", req.URL.Path) + } + + switch req.Method { + case "GET": + t.getCount++ + case "PUT": + body, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + t.capturedPut = body + if err := req.Body.Close(); err != nil { + return nil, err + } + default: + return nil, fmt.Errorf("unexpected request method: %s", req.Method) + } + + // Echo the title back so callers see the effective value, not a constant. + title := "Current title" + if t.capturedPut != nil { + var put struct { + Title string `json:"title"` + } + if err := json.Unmarshal(t.capturedPut, &put); err == nil && put.Title != "" { + title = put.Title + } + } + stepJSON := fmt.Sprintf(`{"id": 456, "title": %q, "completed": false, "assignees": []}`, title) + + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(stepJSON)), + Header: header, + }, nil +} + +// TestCardsStepUpdateAssigneesOnlyCarriesTitle verifies that updating only +// assignees fetches the current step and includes its title in the request — +// the API rejects step updates without a title. +func TestCardsStepUpdateAssigneesOnlyCarriesTitle(t *testing.T) { + transport := &mockStepUpdateTransport{} + app := setupCardsMockApp(t, transport) + + cmd := newCardsStepUpdateCmd() + err := executeCommand(cmd, app, "456", "--assignees", "789") + require.NoError(t, err) + + assert.Equal(t, 1, transport.getCount) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) + assert.Equal(t, "Current title", body["title"]) + assert.Equal(t, []any{float64(789)}, body["assignee_ids"]) +} + +// TestCardsStepUpdateWithTitleSkipsFetch verifies that an explicit title is +// sent as-is without fetching the current step. +func TestCardsStepUpdateWithTitleSkipsFetch(t *testing.T) { + transport := &mockStepUpdateTransport{} + app := setupCardsMockApp(t, transport) + + cmd := newCardsStepUpdateCmd() + err := executeCommand(cmd, app, "456", "New title") + require.NoError(t, err) + + assert.Equal(t, 0, transport.getCount) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) + assert.Equal(t, "New title", body["title"]) +} + // TestCardsStepMoveRequiresCard tests that --card is required for step move. func TestCardsStepMoveShowsHelp(t *testing.T) { app, _ := setupTestApp(t)