diff --git a/internal/commands/config.go b/internal/commands/config.go index 0f7ede307..cf81944a1 100644 --- a/internal/commands/config.go +++ b/internal/commands/config.go @@ -791,6 +791,13 @@ Without --project, an interactive picker is shown.`, if err != nil { return err } + // A numeric ID resolves without a name; read the project to + // confirm it exists before saving it + if id, parseErr := strconv.ParseInt(resolvedID, 10, 64); projectName == "" && parseErr == nil { + if projectName, err = app.Names.ProjectName(cmd.Context(), id); err != nil { + return err + } + } if projectName == "" { return output.ErrNotFound("project", projectArg) } diff --git a/internal/commands/config_test.go b/internal/commands/config_test.go index dd800efdb..cd06efbae 100644 --- a/internal/commands/config_test.go +++ b/internal/commands/config_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" @@ -459,10 +460,19 @@ func setupConfigProjectTestApp(t *testing.T) (*appctx.App, *bytes.Buffer, *httpt server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode([]map[string]any{ - {"id": 12345, "name": "Project Alpha"}, - {"id": 67890, "name": "Project Beta"}, - }) + switch { + case strings.HasSuffix(r.URL.Path, "/projects/12345.json"): + json.NewEncoder(w).Encode(map[string]any{"id": 12345, "name": "Project Alpha"}) + case strings.HasSuffix(r.URL.Path, "/projects/67890.json"): + json.NewEncoder(w).Encode(map[string]any{"id": 67890, "name": "Project Beta"}) + case strings.Contains(r.URL.Path, "/projects/"): + http.NotFound(w, r) + default: + json.NewEncoder(w).Encode([]map[string]any{ + {"id": 12345, "name": "Project Alpha"}, + {"id": 67890, "name": "Project Beta"}, + }) + } })) t.Cleanup(server.Close) diff --git a/internal/commands/templates_test.go b/internal/commands/templates_test.go index 820677e04..8e484ebd0 100644 --- a/internal/commands/templates_test.go +++ b/internal/commands/templates_test.go @@ -154,14 +154,14 @@ func TestTemplatesCopySendsExplicitPeopleConfirmation(t *testing.T) { "copy", "3", "--project", "123", "--todoset", "9", "--confirm-adding-people") require.NoError(t, err) + // A numeric --project is used as given: no /projects.json walk requests := transport.recorded() - require.Len(t, requests, 4) - assert.Equal(t, "/99999/projects.json", requests[0].Path) - assert.Equal(t, "/99999/todosets/9", requests[1].Path) - assert.Equal(t, "/99999/projects/123.json", requests[2].Path) + require.Len(t, requests, 3) + assert.Equal(t, "/99999/todosets/9", requests[0].Path) + assert.Equal(t, "/99999/projects/123.json", requests[1].Path) assert.JSONEq(t, `{"template_recording_id":3,"destination_parent_id":9,"adding_people_confirmed":true}`, - requests[3].Body, + requests[2].Body, ) } @@ -175,7 +175,7 @@ func TestTemplatesCopyRejectsTodosetFromAnotherProject(t *testing.T) { var outputErr *output.Error require.True(t, errors.As(err, &outputErr)) assert.Contains(t, outputErr.Message, "--todoset 9 belongs to project 456, not 123") - require.Len(t, transport.recorded(), 2, "copy request must not be sent") + require.Len(t, transport.recorded(), 1, "copy request must not be sent") } func TestTemplatesCopyRejectsDisabledTodoset(t *testing.T) { @@ -197,7 +197,7 @@ func TestTemplatesCopyRejectsDisabledTodoset(t *testing.T) { var outputErr *output.Error require.True(t, errors.As(err, &outputErr)) assert.Contains(t, outputErr.Message, "--todoset 9 is disabled for project 123") - require.Len(t, transport.recorded(), 3, "copy request must not be sent") + require.Len(t, transport.recorded(), 2, "copy request must not be sent") } func TestTemplatesCopyExplainsPeopleConfirmation(t *testing.T) { diff --git a/internal/commands/timeline.go b/internal/commands/timeline.go index 1ed477a9a..302a0cd87 100644 --- a/internal/commands/timeline.go +++ b/internal/commands/timeline.go @@ -163,6 +163,11 @@ func runProjectTimeline(cmd *cobra.Command, project string, opts *basecamp.Timel return convertSDKError(err) } + // A numeric ID resolves without a name; the events carry their project's + if projectName == "" && len(timelineResult.Events) > 0 && timelineResult.Events[0].Bucket != nil { + projectName = timelineResult.Events[0].Bucket.Name + } + summary := fmt.Sprintf("%d events in %s", len(timelineResult.Events), projectName) if projectName == "" { summary = fmt.Sprintf("%d events in project #%s", len(timelineResult.Events), resolvedProjectID) diff --git a/internal/names/resolver.go b/internal/names/resolver.go index 0130c4e6f..7191310af 100644 --- a/internal/names/resolver.go +++ b/internal/names/resolver.go @@ -102,22 +102,27 @@ func (r *Resolver) forAccount() *basecamp.AccountClient { } // ResolveProject resolves a project name or ID to an ID. -// Returns the ID and the project name for display. +// Returns the ID and the project name for display. For a numeric ID the name +// is "" unless the session already listed projects; see ProjectName. func (r *Resolver) ResolveProject(ctx context.Context, input string) (string, string, error) { - // Numeric ID passthrough + // Numeric ID passthrough. An ID needs no resolving, so it costs no + // request: this path runs on every command given --in , and it used + // to walk every page of /projects.json just to find a name most callers + // discard. The name comes back only when the session already holds the + // project list; a caller that needs it otherwise asks ProjectName. if id, err := strconv.ParseInt(input, 10, 64); err == nil { - // Validate the ID exists by fetching projects - projects, err := r.getProjects(ctx) - if err != nil { - return "", "", err - } + r.mu.RLock() + projects := r.projects + r.mu.RUnlock() for _, p := range projects { if p.ID == id { return strconv.FormatInt(id, 10), p.Name, nil } } - // ID not found - return as-is but let API handle validation - return input, "", nil + // Unknown here: return the canonical ID and let the API validate. + // Callers compare this string with bucket IDs, so "00123" must come + // back as "123". + return strconv.FormatInt(id, 10), "", nil } // Fetch projects for name resolution @@ -412,6 +417,37 @@ func (r *Resolver) getMe(ctx context.Context) (*Person, error) { return r.me, nil } +// ProjectName returns the name of the project with the given ID, or "" when +// the project is not found or not visible. It reads the session's project +// list when one was already fetched, and otherwise that one project. The list +// holds only active projects, so a miss there still reads the project itself. +func (r *Resolver) ProjectName(ctx context.Context, id int64) (string, error) { + r.mu.RLock() + projects := r.projects + r.mu.RUnlock() + + for _, p := range projects { + if p.ID == id { + return p.Name, nil + } + } + + resp, err := r.forAccount().Get(ctx, "/projects/"+strconv.FormatInt(id, 10)+".json") + if err != nil { + var sdkErr *basecamp.Error + if errors.As(err, &sdkErr) && (sdkErr.Code == basecamp.CodeNotFound || sdkErr.Code == basecamp.CodeForbidden) { + return "", nil + } + return "", convertSDKError(err) + } + + var project Project + if err := json.Unmarshal(resp.Data, &project); err != nil { + return "", err + } + return project.Name, nil +} + func (r *Resolver) getProjects(ctx context.Context) ([]Project, error) { r.mu.RLock() if r.projects != nil { diff --git a/internal/names/resolver_test.go b/internal/names/resolver_test.go index 8393737c9..5b9a2ab56 100644 --- a/internal/names/resolver_test.go +++ b/internal/names/resolver_test.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "sync" "sync/atomic" "testing" @@ -427,6 +428,129 @@ func TestResolverResolveProjectNumericID(t *testing.T) { assert.Equal(t, "Project Alpha", name) } +// newProjectServer serves one project at //projects/.json and +// counts every request by path, so a test can assert which endpoints a +// resolution touched. +func newProjectServer(t *testing.T, accountID string, projectID int64, status int) (*Resolver, func() map[string]int) { + t.Helper() + var mu sync.Mutex + hits := map[string]int{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + hits[r.URL.Path]++ + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + if r.URL.Path != fmt.Sprintf("/%s/projects/%d.json", accountID, projectID) { + http.NotFound(w, r) + return + } + if status != http.StatusOK { + w.WriteHeader(status) + _, _ = w.Write([]byte(`{"error":"nope"}`)) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"id": projectID, "name": "Project Alpha"}) + })) + t.Cleanup(server.Close) + + sdkClient := basecamp.NewClient( + &basecamp.Config{BaseURL: server.URL}, + testTokenProvider{}, + basecamp.WithMaxRetries(1), + ) + snapshot := func() map[string]int { + mu.Lock() + defer mu.Unlock() + out := make(map[string]int, len(hits)) + for k, v := range hits { + out[k] = v + } + return out + } + return NewResolver(sdkClient, nil, accountID), snapshot +} + +func TestResolverResolveProjectNumericID_MakesNoRequest(t *testing.T) { + r, hits := newProjectServer(t, "99999", 12345, http.StatusOK) + + id, name, err := r.ResolveProject(context.Background(), "12345") + require.NoError(t, err) + assert.Equal(t, "12345", id) + assert.Empty(t, name, "the name is only known once the session has listed projects") + assert.Empty(t, hits(), "a numeric ID should never walk /projects.json, or read anything else") +} + +func TestResolverProjectName_ReadsOneProject(t *testing.T) { + r, hits := newProjectServer(t, "99999", 12345, http.StatusOK) + + name, err := r.ProjectName(context.Background(), 12345) + require.NoError(t, err) + assert.Equal(t, "Project Alpha", name) + assert.Equal(t, map[string]int{"/99999/projects/12345.json": 1}, hits()) +} + +func TestResolverProjectName_NotVisibleIsEmpty(t *testing.T) { + for _, status := range []int{http.StatusNotFound, http.StatusForbidden} { + t.Run(http.StatusText(status), func(t *testing.T) { + r, hits := newProjectServer(t, "99999", 12345, status) + + name, err := r.ProjectName(context.Background(), 12345) + require.NoError(t, err) + assert.Empty(t, name) + assert.Equal(t, map[string]int{"/99999/projects/12345.json": 1}, hits()) + }) + } +} + +func TestResolverProjectName_ServerErrorFails(t *testing.T) { + r, _ := newProjectServer(t, "99999", 12345, http.StatusInternalServerError) + + _, err := r.ProjectName(context.Background(), 12345) + require.Error(t, err) +} + +func TestResolverProjectName_UsesSessionList(t *testing.T) { + r := newMockResolver() + r.setProjects([]Project{{ID: 12345, Name: "Project Alpha"}}) + + name, err := r.ProjectName(context.Background(), 12345) + require.NoError(t, err) + assert.Equal(t, "Project Alpha", name) +} + +func TestResolverProjectName_ReadsProjectMissingFromSessionList(t *testing.T) { + r, hits := newProjectServer(t, "99999", 12345, http.StatusOK) + r.projects = []Project{{ID: 67890, Name: "Project Beta"}} + + name, err := r.ProjectName(context.Background(), 12345) + require.NoError(t, err) + assert.Equal(t, "Project Alpha", name, "the session list holds only active projects, so a miss must read the project itself") + assert.Equal(t, map[string]int{"/99999/projects/12345.json": 1}, hits()) +} + +func TestResolverResolveProjectNumericID_Canonical(t *testing.T) { + for _, input := range []string{"00123", "+123"} { + t.Run(input, func(t *testing.T) { + r, hits := newProjectServer(t, "99999", 123, http.StatusOK) + + id, _, err := r.ResolveProject(context.Background(), input) + require.NoError(t, err) + assert.Equal(t, "123", id, "callers compare the resolved ID with bucket IDs as strings") + assert.Empty(t, hits()) + }) + } +} + +func TestResolverResolveProjectNumericID_NotInSessionList(t *testing.T) { + r := newMockResolver() + r.setProjects([]Project{{ID: 67890, Name: "Project Beta"}}) + + id, name, err := r.ResolveProject(context.Background(), "12345") + require.NoError(t, err) + assert.Equal(t, "12345", id) + assert.Empty(t, name) +} + func TestResolverResolveProjectByName(t *testing.T) { r := newMockResolver() r.setProjects([]Project{