Skip to content

Commit d12e9f3

Browse files
authored
Fail when Basecamp drops a requested assignee (#981)
* Fail when Basecamp drops a requested assignee Basecamp ignores an assignee_ids entry it cannot assign on the recording, such as a card column's ID or a person outside the project, and saves the rest. `cards create --to <column id>` therefore exited 0 with an unassigned card, and `cards update --assignee <bad id>` cleared the card's real assignees just as quietly. Every command that sets assignees now compares what it asked for with the assignees on the record Basecamp returns, and fails with a usage error naming the record, the dropped IDs and what the record was saved with. The check needs no extra request, and it uses the server's own answer, which no client-side person lookup could reproduce: bc3 filters through reachable_people(include_admissible_people: true). cards create's --to help now says it is the person to assign, and that the column is --column. * Name the items a partial assign batch failed on `assign A B --to <person>` exits 0 when it assigns some of the items, by design, and --agent prints only the data. An item Basecamp dropped the person from, or any other item that failed, was therefore invisible there. The batch now carries a diagnostic naming each failed item and why, which quiet mode writes to stderr. * Keep the saved assignees and the project in the recovery hint Following the subtask hint would have replaced the assignees the subtask kept, since subtasks update replaces the whole set, so the hint now lists them before <person>. The assign hints name --project, which assign needs when no default project is configured.
1 parent 1cf6df0 commit d12e9f3

8 files changed

Lines changed: 301 additions & 18 deletions

File tree

‎internal/commands/assign.go‎

Lines changed: 79 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ func assignItems(cmd *cobra.Command, args []string, assignee *string, project st
152152
}
153153

154154
var results []*assignResult
155-
var failed []string
155+
var failed, failures []string
156156
var firstErr error
157157
var assigneeResolved bool
158158
var assigneeID string
@@ -167,6 +167,7 @@ func assignItems(cmd *cobra.Command, args []string, assignee *string, project st
167167
return fatal.err
168168
}
169169
failed = append(failed, itemID)
170+
failures = append(failures, fmt.Sprintf("%s: %s", itemID, err))
170171
if firstErr == nil {
171172
firstErr = err
172173
}
@@ -206,22 +207,26 @@ func assignItems(cmd *cobra.Command, args []string, assignee *string, project st
206207
Description: "Remove assignee",
207208
}}
208209

210+
respOpts := []output.ResponseOption{
211+
output.WithSummary(summary),
212+
output.WithBreadcrumbs(batchBreadcrumbs...),
213+
}
214+
// --agent and --quiet print only the data, so the failures would
215+
// otherwise be invisible there: a dropped assignee among them.
216+
if len(failures) > 0 {
217+
respOpts = append(respOpts, output.WithDiagnostic("Not assigned: "+strings.Join(failures, "; ")))
218+
}
219+
209220
if len(results) == 1 {
210-
return app.OK(results[0].item,
211-
output.WithSummary(summary),
212-
output.WithBreadcrumbs(batchBreadcrumbs...),
213-
)
221+
return app.OK(results[0].item, respOpts...)
214222
}
215223

216224
items := make([]any, len(results))
217225
for i, r := range results {
218226
items[i] = r.item
219227
}
220228

221-
return app.OK(items,
222-
output.WithSummary(summary),
223-
output.WithBreadcrumbs(batchBreadcrumbs...),
224-
)
229+
return app.OK(items, respOpts...)
225230
}
226231

227232
func unassignItems(cmd *cobra.Command, args []string, assignee *string, project string, isCard, isStep bool) error {
@@ -564,6 +569,9 @@ func doAssignTodo(cmd *cobra.Command, app *appctx.App, todoIDStr, assigneeID str
564569
if err != nil {
565570
return nil, convertSDKError(err)
566571
}
572+
if err := checkAssigneesSaved(assigneeTodo, updated.ID, assigneeIDs, updated.Assignees); err != nil {
573+
return nil, err
574+
}
567575

568576
assigneeName := findAssigneeName(updated.Assignees, assigneeIDInt)
569577

@@ -610,6 +618,9 @@ func doAssignCard(cmd *cobra.Command, app *appctx.App, cardIDStr, assigneeID str
610618
if err != nil {
611619
return nil, convertSDKError(err)
612620
}
621+
if err := checkAssigneesSaved(assigneeCard, updated.ID, assigneeIDs, updated.Assignees); err != nil {
622+
return nil, err
623+
}
613624

614625
assigneeName := findAssigneeName(updated.Assignees, assigneeIDInt)
615626

@@ -656,6 +667,9 @@ func doAssignStep(cmd *cobra.Command, app *appctx.App, stepIDStr, assigneeID str
656667
if err != nil {
657668
return nil, convertSDKError(err)
658669
}
670+
if err := checkAssigneesSaved(assigneeCardStep, updated.ID, assigneeIDs, updated.Assignees); err != nil {
671+
return nil, err
672+
}
659673

660674
assigneeName := findAssigneeName(updated.Assignees, assigneeIDInt)
661675

@@ -782,6 +796,62 @@ func existingAssigneeIDs(people []basecamp.Person) []int64 {
782796
return ids
783797
}
784798

799+
// assigneeRecord is a kind of record that takes assignees, with the command
800+
// that assigns someone to one that already exists. assignHint is formatted
801+
// with the record's ID and the people to name; when the command replaces the
802+
// whole set, those people include the assignees the record kept.
803+
type assigneeRecord struct {
804+
noun string
805+
assignHint string
806+
replaces bool
807+
}
808+
809+
var (
810+
assigneeTodo = assigneeRecord{noun: "to-do", assignHint: "basecamp assign %d --to %s --project <project>"}
811+
assigneeCard = assigneeRecord{noun: "card", assignHint: "basecamp assign %d --card --to %s --project <project>"}
812+
assigneeCardStep = assigneeRecord{noun: "card step", assignHint: "basecamp assign %d --step --to %s --project <project>"}
813+
assigneeSubtask = assigneeRecord{noun: "subtask", assignHint: "basecamp subtasks update %d --assignees %s", replaces: true}
814+
)
815+
816+
// checkAssigneesSaved fails when the saved record lacks a requested assignee.
817+
// Basecamp does not reject an assignee ID it cannot assign there (a column ID,
818+
// a person outside the project): it drops it and saves the rest. The record
819+
// exists by then, so the error names it and what it was saved with, and the
820+
// hint fixes that record rather than inviting a second create.
821+
func checkAssigneesSaved(kind assigneeRecord, id int64, requested []int64, saved []basecamp.Person) error {
822+
savedIDs := existingAssigneeIDs(saved)
823+
var dropped []string
824+
for _, want := range requested {
825+
if !containsID(savedIDs, want) {
826+
dropped = append(dropped, strconv.FormatInt(want, 10))
827+
}
828+
}
829+
if len(dropped) == 0 {
830+
return nil
831+
}
832+
833+
people := "<person>"
834+
if kind.replaces {
835+
for i := len(savedIDs) - 1; i >= 0; i-- {
836+
people = strconv.FormatInt(savedIDs[i], 10) + "," + people
837+
}
838+
}
839+
840+
state := "it has no assignees"
841+
if len(saved) > 0 {
842+
names := make([]string, len(saved))
843+
for i, p := range saved {
844+
names[i] = p.Name
845+
}
846+
state = "its assignees are " + strings.Join(names, ", ")
847+
}
848+
return output.ErrUsageHint(
849+
fmt.Sprintf("%s #%d was saved, but Basecamp did not assign %s: not a person who can be assigned there; %s",
850+
kind.noun, id, strings.Join(dropped, ", "), state),
851+
fmt.Sprintf("Assign it with "+kind.assignHint+", naming someone from basecamp people list --project <project>", id, people),
852+
)
853+
}
854+
785855
// containsID checks if a slice contains a given ID.
786856
func containsID(ids []int64, target int64) bool {
787857
for _, id := range ids {

‎internal/commands/assign_test.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -756,7 +756,18 @@ func (m *mockStepAssignTransport) RoundTrip(req *http.Request) (*http.Response,
756756
if err := req.Body.Close(); err != nil {
757757
return nil, err
758758
}
759-
return &http.Response{StatusCode: 200, Body: io.NopCloser(strings.NewReader(stepJSON)), Header: header}, nil
759+
var put struct {
760+
AssigneeIDs []int64 `json:"assignee_ids"`
761+
}
762+
if err := json.Unmarshal(body, &put); err != nil {
763+
return nil, err
764+
}
765+
assignees := make([]string, len(put.AssigneeIDs))
766+
for i, id := range put.AssigneeIDs {
767+
assignees[i] = fmt.Sprintf(`{"id": %d, "name": "Person %d"}`, id, id)
768+
}
769+
saved := fmt.Sprintf(`{"id": 456, "title": "Existing step", "assignees": [%s]}`, strings.Join(assignees, ","))
770+
return &http.Response{StatusCode: 200, Body: io.NopCloser(strings.NewReader(saved)), Header: header}, nil
760771
default:
761772
return nil, fmt.Errorf("unexpected request method: %s", req.Method)
762773
}
Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
package commands
2+
3+
import (
4+
"bytes"
5+
"encoding/json"
6+
"errors"
7+
"fmt"
8+
"io"
9+
"net/http"
10+
"regexp"
11+
"strings"
12+
"testing"
13+
14+
"github.com/spf13/cobra"
15+
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/require"
17+
18+
"github.com/basecamp/basecamp-sdk/go/pkg/basecamp"
19+
20+
"github.com/basecamp/basecamp-cli/internal/output"
21+
)
22+
23+
// assigneeEchoTransport plays Basecamp for every command that sets assignees.
24+
// A record takes its ID from the request path, #999 when the path has none. A
25+
// write answers with the assignee_ids it was sent, or, when drop matches the
26+
// path, with none of them: what Basecamp does with an ID it cannot assign,
27+
// such as a card column's.
28+
type assigneeEchoTransport struct {
29+
drop func(path string) bool
30+
}
31+
32+
var trailingRecordID = regexp.MustCompile(`/(\d+)(\.json)?$`)
33+
34+
func recordJSON(path, assignees string) string {
35+
id := "999"
36+
if m := trailingRecordID.FindStringSubmatch(path); m != nil {
37+
id = m[1]
38+
}
39+
return `{"id": ` + id + `, "title": "Thing", "content": "Thing", "assignees": [` + assignees + `]}`
40+
}
41+
42+
func dropAll(string) bool { return true }
43+
44+
func (m *assigneeEchoTransport) RoundTrip(req *http.Request) (*http.Response, error) {
45+
header := http.Header{"Content-Type": []string{"application/json"}}
46+
respond := func(status int, body string) (*http.Response, error) {
47+
return &http.Response{StatusCode: status, Body: io.NopCloser(strings.NewReader(body)), Header: header}, nil
48+
}
49+
50+
path := req.URL.Path
51+
switch req.Method {
52+
case http.MethodGet:
53+
switch {
54+
case strings.HasSuffix(path, "/projects.json"):
55+
return respond(200, `[{"id": 123, "name": "Test Project"}]`)
56+
case strings.HasSuffix(path, "/projects/123.json"):
57+
return respond(200, `{"id": 123, "dock": [{"name": "kanban_board", "id": 789, "title": "Card Table"}]}`)
58+
case strings.HasSuffix(path, "/people.json"):
59+
return respond(200, `[{"id": 42, "name": "Annie Bryan"}]`)
60+
}
61+
return respond(200, recordJSON(path, ""))
62+
case http.MethodPost, http.MethodPut:
63+
var body struct {
64+
AssigneeIDs []int64 `json:"assignee_ids"`
65+
}
66+
if err := json.NewDecoder(req.Body).Decode(&body); err != nil {
67+
return nil, err
68+
}
69+
assignees := []string{}
70+
if m.drop == nil || !m.drop(path) {
71+
for _, id := range body.AssigneeIDs {
72+
assignees = append(assignees, fmt.Sprintf(`{"id": %d, "name": "Person %d"}`, id, id))
73+
}
74+
}
75+
status := http.StatusOK
76+
if req.Method == http.MethodPost {
77+
status = http.StatusCreated
78+
}
79+
return respond(status, recordJSON(path, strings.Join(assignees, ",")))
80+
}
81+
return nil, fmt.Errorf("unexpected %s %s", req.Method, path)
82+
}
83+
84+
type assigneeWrite struct {
85+
name string
86+
cmd func() *cobra.Command
87+
args []string
88+
}
89+
90+
// Every command that sets assignees, given the ID of a card column.
91+
var assigneeWrites = []assigneeWrite{
92+
{"cards create --to", NewCardsCmd, []string{"create", "Thing", "--column", "111", "--to", "822400739"}},
93+
{"cards update", NewCardsCmd, []string{"update", "999", "--assignee", "822400739"}},
94+
{"cards step create", NewCardsCmd, []string{"step", "create", "Thing", "--card", "999", "--assignees", "822400739"}},
95+
{"cards step update", NewCardsCmd, []string{"step", "update", "999", "--assignees", "822400739"}},
96+
{"todos create", NewTodosCmd, []string{"create", "Thing", "--list", "555", "--assignee", "822400739"}},
97+
{"todos update", NewTodosCmd, []string{"update", "999", "--assignee", "822400739"}},
98+
{"subtasks create", NewSubtasksCmd, []string{"create", "999", "Thing", "--assignees", "822400739"}},
99+
{"subtasks update", NewSubtasksCmd, []string{"update", "999", "--assignees", "822400739"}},
100+
{"assign", NewAssignCmd, []string{"999", "--to", "822400739"}},
101+
{"assign --card", NewAssignCmd, []string{"999", "--card", "--to", "822400739"}},
102+
{"assign --step", NewAssignCmd, []string{"999", "--step", "--to", "822400739"}},
103+
}
104+
105+
func TestAssigneeWritesFailWhenBasecampDropsTheAssignee(t *testing.T) {
106+
for _, w := range assigneeWrites {
107+
t.Run(w.name, func(t *testing.T) {
108+
app := setupCardsMockApp(t, &assigneeEchoTransport{drop: dropAll})
109+
110+
err := executeCommand(w.cmd(), app, w.args...)
111+
112+
var e *output.Error
113+
require.True(t, errors.As(err, &e), "want an output error, got %v", err)
114+
assert.Equal(t, output.CodeUsage, e.Code)
115+
assert.Contains(t, e.Message, "#999")
116+
assert.Contains(t, e.Message, "822400739")
117+
})
118+
}
119+
}
120+
121+
func TestAssigneeWritesSucceedWhenBasecampKeepsTheAssignee(t *testing.T) {
122+
for _, w := range assigneeWrites {
123+
t.Run(w.name, func(t *testing.T) {
124+
app := setupCardsMockApp(t, &assigneeEchoTransport{})
125+
126+
require.NoError(t, executeCommand(w.cmd(), app, w.args...))
127+
})
128+
}
129+
}
130+
131+
// A batch that assigns some items still exits 0, so under --agent, which
132+
// prints only the data, the item Basecamp dropped the person from has to be
133+
// named on stderr.
134+
func TestAssignBatchNamesTheItemThatDroppedTheAssignee(t *testing.T) {
135+
transport := &assigneeEchoTransport{drop: func(path string) bool { return strings.Contains(path, "/222") }}
136+
app := setupCardsMockApp(t, transport)
137+
var stdout, stderr bytes.Buffer
138+
app.Output = output.New(output.Options{Format: output.FormatQuiet, Writer: &stdout, ErrWriter: &stderr})
139+
140+
require.NoError(t, executeCommand(NewAssignCmd(), app, "111", "222", "--to", "822400739"))
141+
142+
assert.Contains(t, stdout.String(), "111")
143+
assert.Contains(t, stderr.String(), "notice: Not assigned: 222: to-do #222 was saved, but Basecamp did not assign 822400739")
144+
}
145+
146+
// Following the hint must not undo what was saved: subtasks update replaces
147+
// the whole set, so its hint carries the kept assignees, and assign needs the
148+
// project.
149+
func TestCheckAssigneesSavedHintKeepsWhatWasSaved(t *testing.T) {
150+
kept := []basecamp.Person{{ID: 7, Name: "Ann"}, {ID: 8, Name: "Bo"}}
151+
152+
var e *output.Error
153+
require.True(t, errors.As(checkAssigneesSaved(assigneeSubtask, 5, []int64{7, 8, 9}, kept), &e))
154+
assert.Contains(t, e.Hint, "basecamp subtasks update 5 --assignees 7,8,<person>")
155+
156+
require.True(t, errors.As(checkAssigneesSaved(assigneeCard, 5, []int64{7, 8, 9}, kept), &e))
157+
assert.Contains(t, e.Hint, "basecamp assign 5 --card --to <person> --project <project>")
158+
}
159+
160+
func TestCheckAssigneesSavedNamesWhatTheRecordKept(t *testing.T) {
161+
err := checkAssigneesSaved(assigneeCard, 7, []int64{1, 2, 3}, []basecamp.Person{{ID: 2, Name: "Ann"}})
162+
163+
var e *output.Error
164+
require.True(t, errors.As(err, &e))
165+
assert.Contains(t, e.Message, "card #7")
166+
assert.Contains(t, e.Message, "1, 3")
167+
assert.Contains(t, e.Message, "Ann")
168+
}

‎internal/commands/cards.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1095,6 +1095,9 @@ Use - as the body argument to read the body from stdin:
10951095
}
10961096
return fmt.Errorf("card %d created but assignment failed: %w", createdCardID, sdkErr)
10971097
}
1098+
if err := checkAssigneesSaved(assigneeCard, card.ID, []int64{assigneeID}, card.Assignees); err != nil {
1099+
return err
1100+
}
10981101
}
10991102

11001103
// Build breadcrumbs - only include --card-table when known
@@ -1137,8 +1140,8 @@ Use - as the body argument to read the body from stdin:
11371140
}
11381141

11391142
cmd.Flags().StringVarP(&column, "column", "c", "", "Column ID or name (defaults to first column)")
1140-
cmd.Flags().StringVar(&assignee, "assignee", "", "Assignee ID or name")
1141-
cmd.Flags().StringVar(&assignee, "to", "", "Assignee (alias for --assignee)")
1143+
cmd.Flags().StringVar(&assignee, "assignee", "", "Person to assign: ID, name, or email")
1144+
cmd.Flags().StringVar(&assignee, "to", "", "Person to assign (alias for --assignee); the column is --column")
11421145
cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (natural language or YYYY-MM-DD)")
11431146
cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)")
11441147

@@ -1263,6 +1266,9 @@ You can pass either a card ID or a Basecamp URL:
12631266
if err != nil {
12641267
return convertSDKError(err)
12651268
}
1269+
if err := checkAssigneesSaved(assigneeCard, card.ID, req.AssigneeIDs, card.Assignees); err != nil {
1270+
return err
1271+
}
12661272

12671273
respOpts := []output.ResponseOption{
12681274
output.WithSummary(fmt.Sprintf("Updated card #%s", cardIDStr)),
@@ -3077,6 +3083,9 @@ func newCardsStepCreateCmd(project *string) *cobra.Command {
30773083
if err != nil {
30783084
return convertSDKError(err)
30793085
}
3086+
if err := checkAssigneesSaved(assigneeCardStep, step.ID, req.AssigneeIDs, step.Assignees); err != nil {
3087+
return err
3088+
}
30803089

30813090
return app.OK(step,
30823091
output.WithSummary(fmt.Sprintf("Created step: %s", step.Title)),
@@ -3165,6 +3174,9 @@ You can pass either a step ID or a Basecamp URL:
31653174
if err != nil {
31663175
return convertSDKError(err)
31673176
}
3177+
if err := checkAssigneesSaved(assigneeCardStep, step.ID, req.AssigneeIDs, step.Assignees); err != nil {
3178+
return err
3179+
}
31683180

31693181
return app.OK(step,
31703182
output.WithSummary(fmt.Sprintf("Updated step #%s", stepIDStr)),

0 commit comments

Comments
 (0)