diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 2e428119..71b0f417 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -16,12 +17,26 @@ import ( "charm.land/lipgloss/v2" "github.com/spf13/cobra" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" "github.com/basecamp/basecamp-cli/internal/urlarg" ) +// genericLookupTypeHint is the usage hint emitted when the generic +// /recordings/.json lookup cannot resolve the recording — either +// because the endpoint returns 204 (some recording types) or 404 (cards, +// and any other type whose recording is not addressable without bucket +// scope). Callers need to specify --type (or pass a URL, from which the +// type is parsed) so the typed endpoint can be used instead. +// +// Kept command-agnostic: fetchItemContent is shared by `attachments list` +// and `attachments download`, so the hint must not tell a download caller +// to re-run the list command (or vice versa). +const genericLookupTypeHint = "Re-run with --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|answer|forward|upload, or pass a URL (which encodes the type)" + // NewAttachmentsCmd creates the attachments command group. func NewAttachmentsCmd() *cobra.Command { cmd := &cobra.Command{ @@ -401,6 +416,20 @@ func isGenericType(recordType string) bool { } } +// shouldSuggestType reports whether the --type usage hint is appropriate +// for the current recordType. It fires only when no explicit type was +// provided. The "line"/"lines"/"replies" aliases route through the same +// generic /recordings lookup for parent discovery, but the user *did* +// pass --type — telling those callers to "specify --type" is misleading. +func shouldSuggestType(recordType string) bool { + switch recordType { + case "", "recording", "recordings": + return true + default: + return false + } +} + // fetchItemContent retrieves the HTML content field from a Basecamp item. // Uses the same recording-type discovery pattern as show.go. func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string) (string, error) { @@ -416,13 +445,29 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string resp, err := app.Account().Get(cmd.Context(), endpoint) if err != nil { + // The generic /recordings/.json endpoint returns 404 for + // recording types that require bucket scope (e.g. Kanban::Card). + // Convert to the same usage hint the 204 branch below emits so + // the user is told to pass --type instead of seeing a bare + // "Resource not found". Only fires when no explicit type was + // provided — a 404 under --type line|replies means the ID is + // wrong, not that --type is missing. + if shouldSuggestType(recordType) { + var sdkErr *basecamp.Error + if errors.As(err, &sdkErr) && sdkErr.Code == basecamp.CodeNotFound { + return "", output.ErrUsageHint( + fmt.Sprintf("Item %s not found or type required", id), + genericLookupTypeHint, + ) + } + } return "", convertSDKError(err) } if resp.StatusCode == http.StatusNoContent { - if isGenericType(recordType) { + if shouldSuggestType(recordType) { return "", output.ErrUsageHint( fmt.Sprintf("Item %s not found or type required", id), - "Specify a type: basecamp attachments list --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|answer|forward|upload", + genericLookupTypeHint, ) } return "", output.ErrNotFound("item", id) diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index 7a7c9a67..2816a1cf 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -13,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" ) @@ -300,16 +302,16 @@ func TestWithAttachmentMeta(t *testing.T) { func TestAttachmentMeta(t *testing.T) { t.Run("uses DisplayURL", func(t *testing.T) { atts := []richtext.ParsedAttachment{ - {URL: "https://example.com/a.png", Href: "https://example.com/a-href.png", Filename: "a.png", ContentType: "image/png", Filesize: "1024"}, - {Href: "https://example.com/b.txt"}, + {URL: "https://preview.example.com/a-preview", Href: "https://storage.example.com/download/a.png", Filename: "a.png", ContentType: "image/png", Filesize: "1024"}, + {URL: "https://preview.example.com/b-preview"}, } result := attachmentMeta(atts, nil) assert.Len(t, result, 2) assert.Equal(t, "a.png", result[0]["filename"]) assert.Equal(t, "image/png", result[0]["content_type"]) assert.Equal(t, "1024", result[0]["filesize"]) - assert.Equal(t, "https://example.com/a.png", result[0]["url"]) - assert.Equal(t, "https://example.com/b.txt", result[1]["url"]) + assert.Equal(t, "https://storage.example.com/download/a.png", result[0]["url"]) + assert.Equal(t, "https://preview.example.com/b-preview", result[1]["url"]) _, hasFilename := result[1]["filename"] assert.False(t, hasFilename) }) @@ -509,3 +511,77 @@ func TestFetchItemContentRefetchesLineWithLargeParentID(t *testing.T) { assert.Contains(t, reqs[0], "/recordings/111.json") assert.Contains(t, reqs[1], "/chats/"+largeID+"/lines/111.json") } + +// TestAttachmentsList404GenericPathSuggestsType verifies that when the +// generic /recordings/.json endpoint returns 404 and no --type is +// provided, the CLI surfaces the "Specify a type" usage hint rather than +// a bare "Resource not found". Cards are the concrete case: their +// recording is only addressable via the bucket-scoped card_tables +// endpoint, so the generic lookup returns 404. +func TestAttachmentsList404GenericPathSuggestsType(t *testing.T) { + const cardID = "12345" + + transport := &showTrackingTransport{ + responder: func(path string) (int, string) { + if strings.Contains(path, "/recordings/"+cardID) { + return 404, `{"error":"not found"}` + } + return 200, `{}` + }, + } + app := showTestApp(t, transport) + + cmd := NewAttachmentsCmd() + cmd.SetArgs([]string{"list", cardID}) + ctx := appctx.WithApp(context.Background(), app) + cmd.SetContext(ctx) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := cmd.Execute() + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected output.Error, got %T: %v", err, err) + assert.Contains(t, e.Message, cardID) + assert.Contains(t, e.Message, "type required") + assert.Contains(t, e.Hint, "--type") + assert.Contains(t, e.Hint, "card") +} + +// TestAttachmentsList404WithExplicitTypeDoesNotSuggestType verifies the +// hint gate is narrow: when the user already passed --type line (or any +// non-suggest-appropriate type that routes through the generic /recordings +// lookup for parent discovery), a 404 must not produce the "Specify a +// type" hint — they already did. +func TestAttachmentsList404WithExplicitTypeDoesNotSuggestType(t *testing.T) { + const lineID = "12345" + + transport := &showTrackingTransport{ + responder: func(path string) (int, string) { + if strings.Contains(path, "/recordings/"+lineID) { + return 404, `{"error":"not found"}` + } + return 200, `{}` + }, + } + app := showTestApp(t, transport) + + cmd := NewAttachmentsCmd() + cmd.SetArgs([]string{"list", lineID, "--type", "line"}) + ctx := appctx.WithApp(context.Background(), app) + cmd.SetContext(ctx) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := cmd.Execute() + require.Error(t, err) + + var e *output.Error + if errors.As(err, &e) { + assert.NotContains(t, e.Hint, "--type", + "line callers already provided --type; hint would be misleading") + assert.NotContains(t, e.Message, "type required", + "should not claim type is required when it was provided") + } +} diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index cf7b4474..65557443 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1248,9 +1248,16 @@ func (a *ParsedAttachment) DisplayName() string { } // DisplayURL returns the best available URL for the attachment. +// Href is preferred because it points at the real blob download endpoint +// (storage.3.basecamp.com/.../download/). URL points at the +// preview endpoint (preview.3.basecamp.com/.../previews/full), which for +// non-image content types returns a generic SVG file-type icon instead of +// the real file. Every internal caller is a download path, so preferring +// Href is correct; URL is retained as a fallback for the rare case where +// an attachment has no downloadable blob (e.g. externally hosted images). func (a *ParsedAttachment) DisplayURL() string { - if a.URL != "" { - return a.URL + if a.Href != "" { + return a.Href } - return a.Href + return a.URL } diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index c1881bec..1e468fd8 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -2200,8 +2200,9 @@ func TestParsedAttachmentDisplayURL(t *testing.T) { att ParsedAttachment expected string }{ - {"URL wins", ParsedAttachment{URL: "https://a.com", Href: "https://b.com"}, "https://a.com"}, - {"href fallback", ParsedAttachment{Href: "https://b.com"}, "https://b.com"}, + {"Href wins over preview URL", ParsedAttachment{URL: "https://preview.example.com/icon", Href: "https://storage.example.com/download/file.csv"}, "https://storage.example.com/download/file.csv"}, + {"URL fallback when Href missing", ParsedAttachment{URL: "https://preview.example.com/icon"}, "https://preview.example.com/icon"}, + {"Href only", ParsedAttachment{Href: "https://storage.example.com/download/file.csv"}, "https://storage.example.com/download/file.csv"}, {"empty", ParsedAttachment{}, ""}, } for _, tt := range tests {