Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/commands/assign.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Comment thread
nnemirovsky marked this conversation as resolved.
if err != nil {
Expand Down Expand Up @@ -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,
})
Comment thread
nnemirovsky marked this conversation as resolved.
if err != nil {
Expand Down
80 changes: 80 additions & 0 deletions internal/commands/assign_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package commands
import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -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) {
Comment thread
nnemirovsky marked this conversation as resolved.
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
Comment thread
nnemirovsky marked this conversation as resolved.
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"])
Comment thread
nnemirovsky marked this conversation as resolved.
}

// 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"])
}
8 changes: 8 additions & 0 deletions internal/commands/cards.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
nnemirovsky marked this conversation as resolved.
}
Comment thread
nnemirovsky marked this conversation as resolved.
Comment thread
nnemirovsky marked this conversation as resolved.
Comment thread
nnemirovsky marked this conversation as resolved.
Comment thread
nnemirovsky marked this conversation as resolved.
if dueOn != "" {
req.DueOn = dateparse.Parse(dueOn)
Expand Down
87 changes: 87 additions & 0 deletions internal/commands/cards_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment thread
nnemirovsky marked this conversation as resolved.
Comment thread
nnemirovsky marked this conversation as resolved.
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)
}
Comment thread
nnemirovsky marked this conversation as resolved.
Comment thread
nnemirovsky marked this conversation as resolved.

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{
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
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"])
Comment thread
nnemirovsky marked this conversation as resolved.
}

// 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)
Expand Down
Loading