From e72f127bd44c364cb65535e23073e3bb22c8b6b9 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 21:55:03 -0400 Subject: [PATCH 01/11] Test commands given an occurrence URL Each command reaches the entry through a different shared extractor. On main every one of them acts on the recurring entry the occurrence belongs to, so the refusal cases fail; the entry URL, bare ID and show cases pass and guard what the fix must keep. --- internal/commands/occurrence_url_test.go | 140 +++++++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 internal/commands/occurrence_url_test.go diff --git a/internal/commands/occurrence_url_test.go b/internal/commands/occurrence_url_test.go new file mode 100644 index 000000000..df2705565 --- /dev/null +++ b/internal/commands/occurrence_url_test.go @@ -0,0 +1,140 @@ +package commands + +import ( + "errors" + "fmt" + "io" + "net/http" + "strings" + "sync" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +const ( + occurrenceURL = "https://3.basecamp.com/99999/buckets/123/schedule_entries/789/occurrences/20261022" + occurrenceEditURL = occurrenceURL + "/edit" + entryURL = "https://3.basecamp.com/99999/buckets/123/schedule_entries/789" +) + +// occurrenceTransport records every request and answers each with entry 789, +// which is enough for a command to reach the API. +type occurrenceTransport struct { + mu sync.Mutex + requests []string +} + +func (t *occurrenceTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.mu.Lock() + t.requests = append(t.requests, req.Method+" "+req.URL.Path) + t.mu.Unlock() + header := make(http.Header) + header.Set("Content-Type", "application/json") + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(`{"id": 789}`)), + Header: header, + Request: req, + }, nil +} + +type occurrenceCase struct { + name string + cmd func() *cobra.Command + args string // %s is the target +} + +// Between them, these commands reach the entry through each shared extractor. +var occurrenceCases = []occurrenceCase{ + {"schedule update", NewScheduleCmd, "update %s --summary X"}, + {"recordings trash", NewRecordingsCmd, "trash %s"}, + {"recordings archive", NewRecordingsCmd, "archive %s"}, + {"comments create", NewCommentsCmd, "create %s hello"}, + {"comments list", NewCommentsCmd, "list %s"}, + {"boost create", NewBoostsCmd, "create %s yes"}, + {"subscriptions subscribe", NewSubscriptionsCmd, "subscribe %s"}, + {"attachments list", NewAttachmentsCmd, "list %s"}, +} + +func runOccurrenceCase(t *testing.T, c occurrenceCase, target string) ([]string, error) { + t.Helper() + transport := &occurrenceTransport{} + app, _ := setupMessagesMockApp(t, transport) + err := executeMessagesCommand(c.cmd(), app, strings.Fields(fmt.Sprintf(c.args, target))...) + transport.mu.Lock() + defer transport.mu.Unlock() + return transport.requests, err +} + +func TestOccurrenceURLIsRefused(t *testing.T) { + for _, c := range occurrenceCases { + for _, target := range []string{occurrenceURL, occurrenceEditURL} { + t.Run(c.name+" "+target, func(t *testing.T) { + requests, err := runOccurrenceCase(t, c, target) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "want a usage error, got %v", err) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Message, "occurrence") + assert.Contains(t, outErr.Hint, "789", "the hint names the event's ID") + assert.Empty(t, requests) + }) + } + } +} + +func TestEntryURLAndIDStillReachTheEntry(t *testing.T) { + for _, c := range occurrenceCases { + for _, target := range []string{entryURL, "789"} { + t.Run(c.name+" "+target, func(t *testing.T) { + requests, err := runOccurrenceCase(t, c, target) + + var outErr *output.Error + if errors.As(err, &outErr) { + assert.NotContains(t, outErr.Message, "occurrence") + } + assert.True(t, slicesContainSubstring(requests, "/789"), "no request for entry 789: %v", requests) + }) + } + } +} + +func TestCommentFragmentOnOccurrenceURLNamesTheComment(t *testing.T) { + transport := &occurrenceTransport{} + app, _ := setupMessagesMockApp(t, transport) + + err := executeMessagesCommand(NewCommentsCmd(), app, "update", occurrenceURL+"#__recording_555", "edited") + require.NoError(t, err) + assert.True(t, slicesContainSubstring(transport.requests, "/comments/555"), "requests: %v", transport.requests) +} + +func TestScheduleShowOccurrenceEditURL(t *testing.T) { + transport := &mockScheduleShowTransport{} + app, _ := setupMessagesMockApp(t, transport) + + err := executeMessagesCommand(NewScheduleCmd(), app, "show", occurrenceEditURL) + require.NoError(t, err) + assert.True(t, slicesContainSubstring(transport.requests, "/schedule_entries/789/occurrences/20261022"), + "requests: %v", transport.requests) +} + +func TestShowOccurrenceEditURL(t *testing.T) { + reqs, err := runShowCmd(t, &showTrackingTransport{}, occurrenceEditURL) + require.NoError(t, err) + require.NotEmpty(t, reqs) + assert.Contains(t, reqs[0], "/schedule_entries/789/occurrences/20261022.json") +} + +func slicesContainSubstring(items []string, sub string) bool { + for _, item := range items { + if strings.Contains(item, sub) { + return true + } + } + return false +} From 8fd027f3bb4a48667251a57c85705d16b4e6ac05 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 22:02:13 -0400 Subject: [PATCH 02/11] Refuse occurrence URLs in the shared ID extractors An occurrence URL's recording ID is the recurring entry's. The extractors returned it and dropped the occurrence date, so every command that took one acted on the whole series: recordings trash trashed it, comments and boosts landed on it. The API cannot change a single occurrence, so the extractors now return an *urlarg.OccurrenceError instead, and the commands package turns it into a usage error whose hint names the entry's ID. The extractors' callers now take the error. A codemod rewrote the 73 call sites it could, the rest were edited by hand, and the compiler holds any caller that ignores it. show and schedule show read the URL with Parse and keep showing the occurrence. --- internal/commands/assign.go | 10 ++- internal/commands/assignments.go | 6 +- internal/commands/attachments.go | 19 ++-- internal/commands/attachments_test.go | 3 +- internal/commands/bookmarks.go | 6 +- internal/commands/boost.go | 25 ++++-- internal/commands/bubble_up.go | 6 +- internal/commands/calendars.go | 6 +- internal/commands/cards.go | 100 +++++++++++++++++----- internal/commands/chat.go | 3 +- internal/commands/checkins.go | 31 +++++-- internal/commands/comment.go | 17 +++- internal/commands/events.go | 5 +- internal/commands/files.go | 30 +++++-- internal/commands/forwards.go | 20 ++++- internal/commands/helpers.go | 43 +++++++--- internal/commands/lineup.go | 10 ++- internal/commands/messages.go | 25 ++++-- internal/commands/recordings.go | 10 ++- internal/commands/schedule.go | 5 +- internal/commands/subscriptions.go | 20 ++++- internal/commands/subtasks.go | 6 +- internal/commands/todolists.go | 20 ++++- internal/commands/todos.go | 30 +++++-- internal/urlarg/urlarg.go | 64 ++++++++++---- internal/urlarg/urlarg_occurrence_test.go | 62 ++++++++++++++ internal/urlarg/urlarg_test.go | 27 ++++-- internal/urlarg/urlarg_trailing_test.go | 4 +- 28 files changed, 494 insertions(+), 119 deletions(-) create mode 100644 internal/urlarg/urlarg_occurrence_test.go diff --git a/internal/commands/assign.go b/internal/commands/assign.go index 072b4db19..05351f555 100644 --- a/internal/commands/assign.go +++ b/internal/commands/assign.go @@ -143,7 +143,10 @@ func assignItems(cmd *cobra.Command, args []string, assignee *string, project st return output.ErrUsageHint("Person to assign is required", "Use --to ") } - extractedIDs := extractIDs(args) + extractedIDs, err := extractIDs(args) + if err != nil { + return err + } if len(extractedIDs) == 0 { return missingArg(cmd, "...") } @@ -236,7 +239,10 @@ func unassignItems(cmd *cobra.Command, args []string, assignee *string, project return output.ErrUsageHint("Person to unassign is required", "Use --from ") } - extractedIDs := extractIDs(args) + extractedIDs, err := extractIDs(args) + if err != nil { + return err + } if len(extractedIDs) == 0 { return missingArg(cmd, "...") } diff --git a/internal/commands/assignments.go b/internal/commands/assignments.go index 225672197..9c6794c79 100644 --- a/internal/commands/assignments.go +++ b/internal/commands/assignments.go @@ -195,7 +195,11 @@ func assignmentsListBreadcrumb() output.Breadcrumb { // assignmentRecordingID resolves the positional the Up Next verbs take. func assignmentRecordingID(arg string) (int64, error) { - id, err := strconv.ParseInt(extractID(arg), 10, 64) + idStr, err := extractID(arg) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(idStr, 10, 64) if err != nil { return 0, output.ErrUsageHint( fmt.Sprintf("%q is not a recording id or Basecamp URL", arg), diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 44bf3baa3..efda07a6e 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -94,7 +94,10 @@ You can pass either an ID or a Basecamp URL: func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { app := appctx.FromContext(cmd.Context()) - id, resolvedType := resolveAttachmentTarget(arg, recordType) + id, resolvedType, err := resolveAttachmentTarget(arg, recordType) + if err != nil { + return err + } // Validate type early (before account check) for better error messages if !isGenericType(resolvedType) && typeToEndpoint(resolvedType, id) == "" { @@ -229,7 +232,10 @@ Options: } // Resolve target — reuse the same URL/comment parsing as list - idStr, resolvedType := resolveAttachmentTarget(args[0], recordType) + idStr, resolvedType, err := resolveAttachmentTarget(args[0], recordType) + if err != nil { + return err + } // Validate type early (same guard as list) if !isGenericType(resolvedType) && typeToEndpoint(resolvedType, idStr) == "" { @@ -536,8 +542,11 @@ func extractContentField(data map[string]any) string { // resolveAttachmentTarget extracts the recording ID and type from a raw // argument (plain ID or Basecamp URL) and an optional explicit type hint. // Prefers CommentID when present and type is compatible. -func resolveAttachmentTarget(arg, recordType string) (id, resolvedType string) { - id = extractID(arg) +func resolveAttachmentTarget(arg, recordType string) (id, resolvedType string, err error) { + id, err = extractID(arg) + if err != nil { + return "", "", err + } resolvedType = recordType if parsed := urlarg.Parse(arg); parsed != nil { @@ -554,7 +563,7 @@ func resolveAttachmentTarget(arg, recordType string) (id, resolvedType string) { } } - return id, resolvedType + return id, resolvedType, nil } // normalizeShowType maps attachment type aliases to canonical types that diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index 2816a1cfe..b383d52be 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -72,7 +72,8 @@ func TestResolveAttachmentTarget(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - id, typ := resolveAttachmentTarget(tt.arg, tt.typeHint) + id, typ, err := resolveAttachmentTarget(tt.arg, tt.typeHint) + require.NoError(t, err) assert.Equal(t, tt.wantID, id) assert.Equal(t, tt.wantType, typ) }) diff --git a/internal/commands/bookmarks.go b/internal/commands/bookmarks.go index c5f4e5e47..9173098de 100644 --- a/internal/commands/bookmarks.go +++ b/internal/commands/bookmarks.go @@ -320,7 +320,11 @@ indistinguishable from a real error. // takes. With no way to browse bookmarkable recordings from this group, a // pasted URL is the natural way to name one. func bookmarkRecordingID(arg string) (int64, error) { - id, err := strconv.ParseInt(extractID(arg), 10, 64) + idStr, err := extractID(arg) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(idStr, 10, 64) if err != nil { return 0, output.ErrUsageHint( fmt.Sprintf("%q is not a recording id or Basecamp URL", arg), diff --git a/internal/commands/boost.go b/internal/commands/boost.go index b2ff3733d..cce2b3068 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -115,7 +115,10 @@ func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, event // and the SDK dropped Everything().Boosts() with it in v0.11.0 // (basecamp/basecamp-sdk#504). When it returns, this is where the // account-wide branch goes back, along with --all-projects. - recordingID, urlProjectID := extractCommentWithProject(recording) + recordingID, urlProjectID, err := extractCommentWithProject(recording) + if err != nil { + return err + } projectID := project if projectID == "" && urlProjectID != "" { @@ -204,7 +207,10 @@ You can pass either a boost ID or a Basecamp URL: return err } - boostID, urlProjectID := extractWithProject(args[0]) + boostID, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } projectID := *project if projectID == "" && urlProjectID != "" { @@ -284,7 +290,10 @@ Use --event to boost a specific event within the item.`, // Both identifiers are decidable from the arguments alone, so they // are checked before the pipe is drained; runBoostCreate parses // them again once the project is known. - recordingID, _ := extractCommentWithProject(args[0]) + recordingID, _, err := extractCommentWithProject(args[0]) + if err != nil { + return err + } if _, err := strconv.ParseInt(recordingID, 10, 64); err != nil { return output.ErrUsage("Invalid ID") } @@ -329,7 +338,10 @@ func runBoostCreate(cmd *cobra.Command, app *appctx.App, recording, project, con return err } - recordingID, urlProjectID := extractCommentWithProject(recording) + recordingID, urlProjectID, err := extractCommentWithProject(recording) + if err != nil { + return err + } projectID := project if projectID == "" && urlProjectID != "" { @@ -517,7 +529,10 @@ You can pass either a boost ID or a Basecamp URL: return err } - boostID := extractID(args[0]) + boostID, err := extractID(args[0]) + if err != nil { + return err + } boostIDInt, err := strconv.ParseInt(boostID, 10, 64) if err != nil { diff --git a/internal/commands/bubble_up.go b/internal/commands/bubble_up.go index 8b3abd792..4b4454893 100644 --- a/internal/commands/bubble_up.go +++ b/internal/commands/bubble_up.go @@ -168,7 +168,11 @@ run without checking first. // takes. With no way to browse bubble-uppable recordings from this group, a // pasted URL is the natural way to name one. func bubbleUpRecordingID(arg string) (int64, error) { - id, err := strconv.ParseInt(extractID(arg), 10, 64) + idStr, err := extractID(arg) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(idStr, 10, 64) if err != nil || id <= 0 { return 0, output.ErrUsageHint( fmt.Sprintf("%q is not a recording id or Basecamp URL", arg), diff --git a/internal/commands/calendars.go b/internal/commands/calendars.go index 175f86f29..f2cfa194a 100644 --- a/internal/commands/calendars.go +++ b/internal/commands/calendars.go @@ -169,7 +169,11 @@ func validateCalendarColor(color string) error { // calendarIDFromArg resolves the positional. With no index endpoint, a // pasted URL is the realistic way to name a calendar. func calendarIDFromArg(arg string) (int64, error) { - id, err := strconv.ParseInt(extractID(arg), 10, 64) + idStr, err := extractID(arg) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(idStr, 10, 64) if err != nil { return 0, output.ErrUsageHint( fmt.Sprintf("%q is not a calendar id or Basecamp URL", arg), diff --git a/internal/commands/cards.go b/internal/commands/cards.go index ba49183c3..e10e19d32 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -796,7 +796,10 @@ You can pass either a card ID or a Basecamp URL: } // Extract ID from URL if provided - cardIDStr := extractID(args[0]) + cardIDStr, err := extractID(args[0]) + if err != nil { + return err + } cardID, err := strconv.ParseInt(cardIDStr, 10, 64) if err != nil { @@ -1171,7 +1174,10 @@ You can pass either a card ID or a Basecamp URL: } // Extract ID from URL if provided - cardIDStr, urlProjectID := extractWithProject(args[0]) + cardIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } cardID, err := strconv.ParseInt(cardIDStr, 10, 64) if err != nil { @@ -1365,7 +1371,10 @@ the project doesn't need one.`, return err } - cardIDStr, urlProjectID := extractWithProject(args[0]) + cardIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } cardID, err := strconv.ParseInt(cardIDStr, 10, 64) if err != nil { @@ -1709,7 +1718,10 @@ func findWormholeByDestinationColumn(wormholes []basecamp.Wormhole, columnID int if wormhole.DestinationURL == nil { continue } - id := extractID(*wormhole.DestinationURL) + id, err := extractID(*wormhole.DestinationURL) + if err != nil { + continue + } if parsed, err := strconv.ParseInt(id, 10, 64); err == nil && parsed == columnID { return wormhole } @@ -1774,7 +1786,10 @@ You can pass either a card ID or a Basecamp URL: return err } - cardIDStr, urlProjectID := extractWithProject(args[0]) + cardIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } cardID, err := strconv.ParseInt(cardIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid card ID") @@ -2082,7 +2097,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID and project from URL if provided - columnIDStr, urlProjectID := extractWithProject(args[0]) + columnIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") @@ -2251,7 +2269,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID from URL if provided - columnIDStr := extractID(args[0]) + columnIDStr, err := extractID(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") @@ -2313,7 +2334,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID and project from URL if provided - columnIDStr, urlProjectID := extractWithProject(args[0]) + columnIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Column ID must be numeric") @@ -2400,7 +2424,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID from URL if provided - columnIDStr := extractID(args[0]) + columnIDStr, err := extractID(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") @@ -2438,7 +2465,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID from URL if provided - columnIDStr := extractID(args[0]) + columnIDStr, err := extractID(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") @@ -2476,7 +2506,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID and project from URL if provided - columnIDStr, urlProjectID := extractWithProject(args[0]) + columnIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") @@ -2522,7 +2555,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID and project from URL if provided - columnIDStr, urlProjectID := extractWithProject(args[0]) + columnIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") @@ -2575,7 +2611,10 @@ You can pass either a column ID or a Basecamp URL: } // Extract ID and project from URL if provided - columnIDStr, urlProjectID := extractWithProject(args[0]) + columnIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } columnID, err := strconv.ParseInt(columnIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") @@ -2791,7 +2830,10 @@ func newCardsWormholesUpdateCmd(project *string) *cobra.Command { return err } - wormholeIDStr, urlProjectID := extractWithProject(args[0]) + wormholeIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } wormholeID, err := strconv.ParseInt(wormholeIDStr, 10, 64) if err != nil || wormholeID <= 0 { return output.ErrUsage("Invalid wormhole ID") @@ -2838,7 +2880,10 @@ func newCardsWormholesDeleteCmd(project *string) *cobra.Command { return err } - wormholeIDStr, urlProjectID := extractWithProject(args[0]) + wormholeIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } wormholeID, err := strconv.ParseInt(wormholeIDStr, 10, 64) if err != nil || wormholeID <= 0 { return output.ErrUsage("Invalid wormhole ID") @@ -3083,7 +3128,10 @@ You can pass either a step ID or a Basecamp URL: } // Extract ID from URL if provided - stepIDStr := extractID(args[0]) + stepIDStr, err := extractID(args[0]) + if err != nil { + return err + } stepID, err := strconv.ParseInt(stepIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid step ID") @@ -3148,7 +3196,10 @@ You can pass either a step ID or a Basecamp URL: } // Extract ID from URL if provided - stepIDStr := extractID(args[0]) + stepIDStr, err := extractID(args[0]) + if err != nil { + return err + } stepID, err := strconv.ParseInt(stepIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid step ID") @@ -3185,7 +3236,10 @@ You can pass either a step ID or a Basecamp URL: } // Extract ID from URL if provided - stepIDStr := extractID(args[0]) + stepIDStr, err := extractID(args[0]) + if err != nil { + return err + } stepID, err := strconv.ParseInt(stepIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid step ID") @@ -3230,7 +3284,10 @@ You can pass either a step ID or a Basecamp URL: } // Extract ID from URL if provided - stepIDStr := extractID(args[0]) + stepIDStr, err := extractID(args[0]) + if err != nil { + return err + } stepID, err := strconv.ParseInt(stepIDStr, 10, 64) if err != nil { return output.ErrUsage("Step ID must be numeric") @@ -3287,7 +3344,10 @@ You can pass either a step ID or a Basecamp URL: } // Extract ID from URL if provided - stepIDStr := extractID(args[0]) + stepIDStr, err := extractID(args[0]) + if err != nil { + return err + } stepID, err := strconv.ParseInt(stepIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid step ID") diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 0a05a28d9..387696c53 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -1332,8 +1332,7 @@ You can pass either a line ID or a Basecamp line URL: // bucket whatever --in says. func chatLineRef(ctx context.Context, app *appctx.App, arg string) (lineID, projectID, chatID string, isPing bool, err error) { if !urlarg.IsURL(arg) { - lineID, projectID = extractWithProject(arg) - return lineID, projectID, "", false, nil + return arg, "", "", false, nil } if !hostutil.IsTrustedBasecampHost(arg, app.Config.BaseURL) { return "", "", "", false, output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index 18c34f6ed..b5e01e377 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -509,7 +509,11 @@ func newCheckinsQuestionAnswerersCmd() *cobra.Command { // checkinsQuestionID resolves the positional the question verbs take. func checkinsQuestionID(arg string) (int64, error) { - id, err := strconv.ParseInt(extractID(arg), 10, 64) + idStr, err := extractID(arg) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(idStr, 10, 64) if err != nil { return 0, output.ErrUsageHint( fmt.Sprintf("%q is not a question id or Basecamp URL", arg), @@ -548,7 +552,10 @@ func runCheckinsQuestionShow(cmd *cobra.Command, project, questionIDStr string, } // Extract ID and project from URL if provided - questionIDStr, urlProjectID := extractWithProject(questionIDStr) + questionIDStr, urlProjectID, err := extractWithProject(questionIDStr) + if err != nil { + return err + } // Resolve project - use URL > flag > config, with interactive fallback projectID := project @@ -769,7 +776,10 @@ You can pass either a question ID or a Basecamp URL: } // Extract ID and project from URL if provided - questionIDStr, urlProjectID := extractWithProject(args[0]) + questionIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } app := appctx.FromContext(cmd.Context()) @@ -977,7 +987,10 @@ func runCheckinsAnswers(cmd *cobra.Command, app *appctx.App, project, questionAr } // Extract ID and project from URL if provided - questionIDStr, urlProjectID := extractWithProject(questionArg) + questionIDStr, urlProjectID, err := extractWithProject(questionArg) + if err != nil { + return err + } // Resolve project - use URL > flag > config, with interactive fallback projectID := project @@ -1213,7 +1226,10 @@ func runCheckinsAnswerShow(cmd *cobra.Command, project, answerIDStr string, cf * } // Extract ID and project from URL if provided - answerIDStr, urlProjectID := extractWithProject(answerIDStr) + answerIDStr, urlProjectID, err := extractWithProject(answerIDStr) + if err != nil { + return err + } // Resolve project - use URL > flag > config, with interactive fallback projectID := project @@ -1432,7 +1448,10 @@ You can pass either an answer ID or a Basecamp URL: } // Extract ID and project from URL if provided - answerIDStr, urlProjectID := extractWithProject(args[0]) + answerIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } // Decidable from the arguments alone, so it precedes the read; the // project-resolution path parses it again once the project is known. diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 8db4ad9c8..e1ae7c354 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -137,7 +137,10 @@ func runCommentsListForItem(cmd *cobra.Command, app *appctx.App, recordingArg st } // Extract recording ID from URL if provided - recordingID := extractID(recordingArg) + recordingID, err := extractID(recordingArg) + if err != nil { + return err + } if err := ensureAccount(cmd, app); err != nil { return err @@ -1009,7 +1012,10 @@ as backslash-n.`, // Extract comment ID from URL if provided // Uses extractCommentWithProject to prefer CommentID from URL fragments - commentIDStr, urlProjectID := extractCommentWithProject(args[0]) + commentIDStr, urlProjectID, err := extractCommentWithProject(args[0]) + if err != nil { + return err + } commentID, err := strconv.ParseInt(commentIDStr, 10, 64) if err != nil { @@ -1182,14 +1188,17 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: } // Expand comma-separated IDs and extract from URLs - expandedIDs := extractIDs([]string{recordingArg}) + expandedIDs, err := extractIDs([]string{recordingArg}) + if err != nil { + return err + } // Create comments on all recordings // Convert Markdown content to HTML for Basecamp's rich text fields html := richTextToHTML(cmd, content) // Resolve inline images (![alt](./path) → upload + ) - html, err := resolveLocalImages(cmd, app, html) + html, err = resolveLocalImages(cmd, app, html) if err != nil { return err } diff --git a/internal/commands/events.go b/internal/commands/events.go index 9771cfb31..0f9b3d6bf 100644 --- a/internal/commands/events.go +++ b/internal/commands/events.go @@ -70,7 +70,10 @@ addressed-items lane of the same feed.`, } // Extract ID from URL if provided - recordingIDStr := extractID(args[0]) + recordingIDStr, err := extractID(args[0]) + if err != nil { + return err + } recordingID, err := strconv.ParseInt(recordingIDStr, 10, 64) if err != nil { diff --git a/internal/commands/files.go b/internal/commands/files.go index 0bebf7c13..cc3b97c7c 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1439,7 +1439,10 @@ You can pass either an item ID or a Basecamp URL: } // Extract ID and project from URL if provided - itemIDStr, urlProjectID := extractWithProject(args[0]) + itemIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } itemID, err := strconv.ParseInt(itemIDStr, 10, 64) if err != nil { @@ -1654,7 +1657,10 @@ You can pass either an upload ID or a Basecamp URL: return err } - uploadIDStr := extractID(args[0]) + uploadIDStr, err := extractID(args[0]) + if err != nil { + return err + } uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid upload ID") @@ -1768,7 +1774,10 @@ You can pass either an upload ID or a Basecamp URL: RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - uploadIDStr := extractID(args[0]) + uploadIDStr, err := extractID(args[0]) + if err != nil { + return err + } uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) if err != nil || uploadID <= 0 { return output.ErrUsage("Invalid upload ID") @@ -1949,7 +1958,10 @@ You can pass either an item ID or a Basecamp URL: Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { // Extract ID and project from URL if provided - itemIDStr, urlProjectID := extractWithProject(args[0]) + itemIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } itemID, err := strconv.ParseInt(itemIDStr, 10, 64) if err != nil { @@ -2529,7 +2541,10 @@ Use --out - to stream the file to stdout (for piping to other commands).`, return err } // Upload ID path — resolve project, then stream - uploadIDStr, urlProjectID := extractWithProject(args[0]) + uploadIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid upload ID") @@ -2577,7 +2592,10 @@ Use --out - to stream the file to stdout (for piping to other commands).`, } // Upload ID path: two-step download via SDK - uploadIDStr, urlProjectID := extractWithProject(args[0]) + uploadIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) if err != nil { diff --git a/internal/commands/forwards.go b/internal/commands/forwards.go index dbed16eb7..a7bab31f6 100644 --- a/internal/commands/forwards.go +++ b/internal/commands/forwards.go @@ -294,7 +294,10 @@ You can pass either a forward ID or a Basecamp URL: } // Extract ID and project from URL if provided - forwardIDStr, urlProjectID := extractWithProject(args[0]) + forwardIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } forwardID, err := strconv.ParseInt(forwardIDStr, 10, 64) if err != nil { @@ -464,7 +467,10 @@ You can pass either a forward ID or a Basecamp URL: } // Extract ID and project from URL if provided - forwardIDStr, urlProjectID := extractWithProject(args[0]) + forwardIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } forwardID, err := strconv.ParseInt(forwardIDStr, 10, 64) if err != nil { @@ -564,8 +570,14 @@ You can pass either IDs or Basecamp URLs: } // Extract IDs and project from URLs if provided - forwardIDStr, urlProjectID := extractWithProject(args[0]) - replyIDStr := extractID(args[1]) + forwardIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } + replyIDStr, err := extractID(args[1]) + if err != nil { + return err + } forwardID, err := strconv.ParseInt(forwardIDStr, 10, 64) if err != nil { diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 3fa2e48dc..39006aa61 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -558,27 +558,43 @@ func ensurePersonInProject(cmd *cobra.Command, app *appctx.App, projectID string // Instead of having to manually extract the ID: // // basecamp todos show 789 --in 456 -func extractID(arg string) string { - return urlarg.ExtractID(arg) +func extractID(arg string) (string, error) { + id, err := urlarg.ExtractID(arg) + return id, occurrenceUsage(err) } // extractWithProject extracts both recording ID and project ID from an argument. // If the argument is a Basecamp URL, extracts both; if the URL contains a project, // it can be used to auto-populate the --in flag. // Returns (recordingID, projectID). If projectID is empty, it wasn't in the URL. -func extractWithProject(arg string) (recordingID, projectID string) { - return urlarg.ExtractWithProject(arg) +func extractWithProject(arg string) (recordingID, projectID string, err error) { + recordingID, projectID, err = urlarg.ExtractWithProject(arg) + return recordingID, projectID, occurrenceUsage(err) } // extractCommentWithProject extracts a comment ID and project ID from an argument. // For URLs with a fragment (#__recording_N), returns the comment ID instead of the recording ID. -func extractCommentWithProject(arg string) (id, projectID string) { - return urlarg.ExtractCommentWithProject(arg) +func extractCommentWithProject(arg string) (id, projectID string, err error) { + id, projectID, err = urlarg.ExtractCommentWithProject(arg) + return id, projectID, occurrenceUsage(err) } // extractIDs extracts IDs from multiple arguments, handling URLs. -func extractIDs(args []string) []string { - return urlarg.ExtractIDs(args) +func extractIDs(args []string) ([]string, error) { + ids, err := urlarg.ExtractIDs(args) + return ids, occurrenceUsage(err) +} + +// occurrenceUsage turns the extractors' refusal of an occurrence URL into a +// usage error that names the entry, the one thing the CLI can act on. +func occurrenceUsage(err error) error { + var occ *urlarg.OccurrenceError + if errors.As(err, &occ) { + return output.ErrUsageHint("That URL names one occurrence of a recurring event", + fmt.Sprintf("Basecamp's API cannot change a single occurrence. Pass the event's ID, %s, to act on the whole series, "+ + "or see the occurrence with basecamp show ", occ.EntryID)) + } + return err } // requireOneParseableTarget rejects a recording argument whose every @@ -587,7 +603,11 @@ func extractIDs(args []string) []string { // nothing — and extractIDs is pure, so that is decidable from the argument // alone, before a "-" drains the producer. func requireOneParseableTarget(arg string) error { - for _, id := range extractIDs([]string{arg}) { + ids, err := extractIDs([]string{arg}) + if err != nil { + return err + } + for _, id := range ids { if _, err := strconv.ParseInt(id, 10, 64); err == nil { return nil } @@ -811,7 +831,10 @@ func batchMentionScope(cmd *cobra.Command, app *appctx.App, targets string) name if part == "" { continue } - recordingID, projectID := extractWithProject(part) + recordingID, projectID, err := extractWithProject(part) + if err != nil { + continue + } if _, err := strconv.ParseInt(recordingID, 10, 64); err != nil { continue } diff --git a/internal/commands/lineup.go b/internal/commands/lineup.go index 95c5a6225..97e65b2f1 100644 --- a/internal/commands/lineup.go +++ b/internal/commands/lineup.go @@ -160,7 +160,10 @@ You can pass either a marker ID or a Basecamp URL: return err } - markerIDStr := extractID(args[0]) + markerIDStr, err := extractID(args[0]) + if err != nil { + return err + } markerID, err := strconv.ParseInt(markerIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid marker ID") @@ -243,7 +246,10 @@ You can pass either a marker ID or a Basecamp URL: return err } - markerIDStr := extractID(args[0]) + markerIDStr, err := extractID(args[0]) + if err != nil { + return err + } markerID, err := strconv.ParseInt(markerIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid marker ID") diff --git a/internal/commands/messages.go b/internal/commands/messages.go index 013fb3e0a..9fe6044c2 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -365,7 +365,10 @@ You can pass either a message ID or a Basecamp URL: } // Extract ID from URL if provided - messageIDStr := extractID(args[0]) + messageIDStr, err := extractID(args[0]) + if err != nil { + return err + } messageID, err := strconv.ParseInt(messageIDStr, 10, 64) if err != nil { @@ -678,7 +681,10 @@ anything else is a name matched against the message's project. List a project's } // Extract ID from URL if provided - messageIDStr, urlProjectID := extractWithProject(args[0]) + messageIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } messageID, err := strconv.ParseInt(messageIDStr, 10, 64) if err != nil { @@ -782,7 +788,10 @@ You can pass either a message ID or a Basecamp URL: return err } - messageIDStr := extractID(args[0]) + messageIDStr, err := extractID(args[0]) + if err != nil { + return err + } messageID, err := strconv.ParseInt(messageIDStr, 10, 64) if err != nil { @@ -836,7 +845,10 @@ You can pass either a message ID or a Basecamp URL: } // Extract ID from URL if provided - messageIDStr := extractID(args[0]) + messageIDStr, err := extractID(args[0]) + if err != nil { + return err + } messageID, err := strconv.ParseInt(messageIDStr, 10, 64) if err != nil { @@ -890,7 +902,10 @@ You can pass either a message ID or a Basecamp URL: } // Extract ID from URL if provided - messageIDStr := extractID(args[0]) + messageIDStr, err := extractID(args[0]) + if err != nil { + return err + } messageID, err := strconv.ParseInt(messageIDStr, 10, 64) if err != nil { diff --git a/internal/commands/recordings.go b/internal/commands/recordings.go index f163c3844..8be034f14 100644 --- a/internal/commands/recordings.go +++ b/internal/commands/recordings.go @@ -337,7 +337,10 @@ func runRecordingsStatus(cmd *cobra.Command, app *appctx.App, recordingIDStr, ne if followComment { extract = extractCommentWithProject } - recordingIDStr, _ = extract(recordingIDStr) + recordingIDStr, _, err := extract(recordingIDStr) + if err != nil { + return err + } // Parse recording ID recordingID, err := strconv.ParseInt(recordingIDStr, 10, 64) @@ -411,7 +414,10 @@ You can pass either an ID or a Basecamp URL: } // Extract ID from URL if provided - recordingIDStr := extractID(args[0]) + recordingIDStr, err := extractID(args[0]) + if err != nil { + return err + } recordingID, err := strconv.ParseInt(recordingIDStr, 10, 64) if err != nil { diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index 50ffceb6f..9afb3ea21 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -639,7 +639,10 @@ You can pass either an entry ID or a Basecamp URL: // Extract ID and project from URL if provided. Purely syntactic, // so it precedes the stdin read like every other target-ID check. - entryID, urlProjectID := extractWithProject(args[0]) + entryID, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } // This command used to send a malformed ID to the server as 0 — // the ParseInt further down discarded its error. Reject it here diff --git a/internal/commands/subscriptions.go b/internal/commands/subscriptions.go index 59e6ca432..cbd6d1ee7 100644 --- a/internal/commands/subscriptions.go +++ b/internal/commands/subscriptions.go @@ -59,7 +59,10 @@ func runSubscriptionsShow(cmd *cobra.Command, recordingIDStr string) error { } // Extract ID from URL if provided - recordingIDStr = extractID(recordingIDStr) + recordingIDStr, err := extractID(recordingIDStr) + if err != nil { + return err + } recordingID, err := strconv.ParseInt(recordingIDStr, 10, 64) if err != nil { @@ -111,7 +114,10 @@ You can pass either an ID or a Basecamp URL: } // Extract ID from URL if provided - recordingIDStr := extractID(args[0]) + recordingIDStr, err := extractID(args[0]) + if err != nil { + return err + } recordingID, err := strconv.ParseInt(recordingIDStr, 10, 64) if err != nil { @@ -160,7 +166,10 @@ You can pass either an ID or a Basecamp URL: } // Extract ID from URL if provided - recordingIDStr := extractID(args[0]) + recordingIDStr, err := extractID(args[0]) + if err != nil { + return err + } recordingID, err := strconv.ParseInt(recordingIDStr, 10, 64) if err != nil { @@ -241,7 +250,10 @@ func runSubscriptionsUpdate(cmd *cobra.Command, args []string, peopleIDs, mode s } // Extract ID from URL if provided - recordingIDStr := extractID(args[0]) + recordingIDStr, err := extractID(args[0]) + if err != nil { + return err + } recordingID, err := strconv.ParseInt(recordingIDStr, 10, 64) if err != nil { diff --git a/internal/commands/subtasks.go b/internal/commands/subtasks.go index 42b459995..ea76ac127 100644 --- a/internal/commands/subtasks.go +++ b/internal/commands/subtasks.go @@ -567,7 +567,11 @@ func subtaskBreadcrumbs(subtask *basecamp.CardStep) []output.Breadcrumb { // subtaskParentID resolves the positional: the id of the // to-do or card itself, never a fragment inside its URL. func subtaskParentID(arg string) (int64, error) { - id, err := strconv.ParseInt(extractID(arg), 10, 64) + idStr, err := extractID(arg) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(idStr, 10, 64) if err != nil || id <= 0 { return 0, output.ErrUsageHint( fmt.Sprintf("%q is not a to-do or card id or Basecamp URL", arg), diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index a0b8f37b1..4c1b7fbe3 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -205,7 +205,10 @@ You can pass either a todolist ID or a Basecamp URL: } // Extract ID and project from URL if provided - todolistIDStr, urlProjectID := extractWithProject(args[0]) + todolistIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } // Resolve project - use URL > flag > config, with interactive fallback projectID := *project @@ -407,7 +410,10 @@ You can pass either a todolist ID or a Basecamp URL: } // Extract ID and project from URL if provided - todolistIDStr, urlProjectID := extractWithProject(args[0]) + todolistIDStr, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } // Parse todolist ID as int64 todolistID, err := strconv.ParseInt(todolistIDStr, 10, 64) @@ -537,7 +543,10 @@ Confirm with ` + "`basecamp todolists list`" + `.`, } // ExtractIDs drops empty segments, so a bare "," yields zero IDs. - ids := extractIDs(args) + ids, err := extractIDs(args) + if err != nil { + return err + } if len(ids) == 0 { return missingArg(cmd, "...") } @@ -661,7 +670,10 @@ Confirm with ` + "`basecamp todolists list`" + `.`, summary = fmt.Sprintf("Moved todolist #%d to position %d", parsed[0], position) // No preflight Get in single mode: resolve project from ambient // context (URL > group flag > flags > config) for the breadcrumbs. - _, urlProjectID := extractWithProject(args[0]) + _, urlProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } proj := urlProjectID if proj == "" { proj = *project diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 6b8e52f27..100d324aa 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1148,7 +1148,10 @@ You can pass either a todo ID or a Basecamp URL: } // Extract ID from URL if provided - todoIDStr := extractID(args[0]) + todoIDStr, err := extractID(args[0]) + if err != nil { + return err + } todoID, err := strconv.ParseInt(todoIDStr, 10, 64) if err != nil { @@ -1578,7 +1581,10 @@ Set or clear the people notified when the todo is completed: } // Extract ID from URL if provided - todoIDStr := extractID(args[0]) + todoIDStr, err := extractID(args[0]) + if err != nil { + return err + } todoID, err := strconv.ParseInt(todoIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid todo ID") @@ -1778,7 +1784,10 @@ func completeTodos(cmd *cobra.Command, todoIDs []string) error { } // Extract IDs from URLs (handles both plain IDs and URLs) - extractedIDs := extractIDs(todoIDs) + extractedIDs, err := extractIDs(todoIDs) + if err != nil { + return err + } var completedTodos []basecamp.Todo var failed []string @@ -2190,7 +2199,10 @@ func reopenTodos(cmd *cobra.Command, todoIDs []string) error { } // Extract IDs from URLs (handles both plain IDs and URLs) - extractedIDs := extractIDs(todoIDs) + extractedIDs, err := extractIDs(todoIDs) + if err != nil { + return err + } var reopenedTodos []basecamp.Todo var failed []string @@ -2307,7 +2319,10 @@ Move to a different todolist in the same project: } // Extract todo ID and project from URL if provided - todoIDStr, todoProjectID := extractWithProject(args[0]) + todoIDStr, todoProjectID, err := extractWithProject(args[0]) + if err != nil { + return err + } todoID, err := strconv.ParseInt(todoIDStr, 10, 64) if err != nil { @@ -2317,7 +2332,10 @@ Move to a different todolist in the same project: // Resolve destination todolist when --list is provided var parentID *int64 if list != "" { - listIDStr, listProjectID := extractWithProject(list) + listIDStr, listProjectID, err := extractWithProject(list) + if err != nil { + return err + } // When --list is a URL, validate it's a todolist URL — not a // todo, project, or collection URL that would silently extract diff --git a/internal/urlarg/urlarg.go b/internal/urlarg/urlarg.go index 4380eb8d2..77332047e 100644 --- a/internal/urlarg/urlarg.go +++ b/internal/urlarg/urlarg.go @@ -248,22 +248,44 @@ func Parse(input string) *Parsed { } } +// OccurrenceError reports a URL that names one occurrence of a recurring +// schedule entry. The occurrence is a recording of its own and the API cannot +// change it, so the extractors refuse the URL rather than return the entry's +// ID, which would act on the whole series. +type OccurrenceError struct { + EntryID string + Date string +} + +func (e *OccurrenceError) Error() string { + return "the URL names one occurrence of a recurring event" +} + +// recording returns the URL's recording ID, or an *OccurrenceError when the +// URL names an occurrence of that recording. +func (p *Parsed) recording() (string, error) { + if p.OccurrenceDate != "" { + return "", &OccurrenceError{EntryID: p.RecordingID, Date: p.OccurrenceDate} + } + return p.RecordingID, nil +} + // ExtractID extracts the primary ID from an argument. // If the argument is a Basecamp URL, extracts the recording ID. // Otherwise, returns the argument as-is (assumed to be an ID). -func ExtractID(arg string) string { +func ExtractID(arg string) (string, error) { if parsed := Parse(arg); parsed != nil { if parsed.RecordingID != "" { - return parsed.RecordingID + return parsed.recording() } if parsed.ProjectID != "" { - return parsed.ProjectID + return parsed.ProjectID, nil } if parsed.AccountID != "" { - return parsed.AccountID + return parsed.AccountID, nil } } - return arg + return arg, nil } // ExtractProjectID extracts the project (bucket) ID from an argument. @@ -278,39 +300,47 @@ func ExtractProjectID(arg string) string { // ExtractWithProject extracts both the recording ID and project ID from an argument. // Returns (recordingID, projectID). If projectID is empty, it wasn't in the URL. -func ExtractWithProject(arg string) (recordingID, projectID string) { +func ExtractWithProject(arg string) (recordingID, projectID string, err error) { if parsed := Parse(arg); parsed != nil { - return parsed.RecordingID, parsed.ProjectID + recordingID, err = parsed.recording() + return recordingID, parsed.ProjectID, err } - return arg, "" + return arg, "", nil } // ExtractCommentWithProject extracts a comment ID and project ID from an argument. // For URLs with a fragment (e.g., #__recording_789), returns the comment ID. // For URLs without a fragment, returns the recording ID (same as ExtractWithProject). // Returns (commentOrRecordingID, projectID). -func ExtractCommentWithProject(arg string) (id, projectID string) { +func ExtractCommentWithProject(arg string) (id, projectID string, err error) { if parsed := Parse(arg); parsed != nil { // Prefer comment ID from fragment when present if parsed.CommentID != "" { - return parsed.CommentID, parsed.ProjectID + return parsed.CommentID, parsed.ProjectID, nil } - return parsed.RecordingID, parsed.ProjectID + id, err = parsed.recording() + return id, parsed.ProjectID, err } - return arg, "" + return arg, "", nil } // ExtractIDs extracts IDs from multiple arguments, handling URLs and -// comma-separated values (e.g. "123,456,789"). -func ExtractIDs(args []string) []string { +// comma-separated values (e.g. "123,456,789"). One occurrence URL refuses +// them all, before the caller acts on any. +func ExtractIDs(args []string) ([]string, error) { var result []string for _, arg := range args { for part := range strings.SplitSeq(arg, ",") { part = strings.TrimSpace(part) - if part != "" { - result = append(result, ExtractID(part)) + if part == "" { + continue + } + id, err := ExtractID(part) + if err != nil { + return nil, err } + result = append(result, id) } } - return result + return result, nil } diff --git a/internal/urlarg/urlarg_occurrence_test.go b/internal/urlarg/urlarg_occurrence_test.go new file mode 100644 index 000000000..be7b1574c --- /dev/null +++ b/internal/urlarg/urlarg_occurrence_test.go @@ -0,0 +1,62 @@ +package urlarg + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + occurrence = "https://3.basecamp.com/99999/buckets/123/schedule_entries/789/occurrences/20261022" + occurrenceEdit = occurrence + "/edit" +) + +// An occurrence URL's recording ID is the recurring entry's, so returning it +// would act on the whole series. Every extractor refuses it instead. +func TestExtractorsRefuseOccurrenceURLs(t *testing.T) { + for _, url := range []string{occurrence, occurrenceEdit} { + t.Run(url, func(t *testing.T) { + _, err := ExtractID(url) + assertOccurrence(t, err) + + _, _, err = ExtractWithProject(url) + assertOccurrence(t, err) + + _, _, err = ExtractCommentWithProject(url) + assertOccurrence(t, err) + + ids, err := ExtractIDs([]string{"111", url + ",222"}) + assertOccurrence(t, err) + assert.Empty(t, ids, "a batch with an occurrence in it yields nothing to act on") + }) + } +} + +func TestExtractCommentWithProjectFollowsAFragmentOnAnOccurrence(t *testing.T) { + id, project, err := ExtractCommentWithProject(occurrence + "#__recording_555") + require.NoError(t, err) + assert.Equal(t, "555", id) + assert.Equal(t, "123", project) +} + +func TestExtractorsAcceptTheEntryItself(t *testing.T) { + for _, arg := range []string{"https://3.basecamp.com/99999/buckets/123/schedule_entries/789", "789"} { + id, err := ExtractID(arg) + require.NoError(t, err) + assert.Equal(t, "789", id) + + id, _, err = ExtractWithProject(arg) + require.NoError(t, err) + assert.Equal(t, "789", id) + } +} + +func assertOccurrence(t *testing.T, err error) { + t.Helper() + var occ *OccurrenceError + require.True(t, errors.As(err, &occ), "want an *OccurrenceError, got %v", err) + assert.Equal(t, "789", occ.EntryID) + assert.Equal(t, "20261022", occ.Date) +} diff --git a/internal/urlarg/urlarg_test.go b/internal/urlarg/urlarg_test.go index 5b8aa2e0e..5421dfebf 100644 --- a/internal/urlarg/urlarg_test.go +++ b/internal/urlarg/urlarg_test.go @@ -310,7 +310,7 @@ func TestExtractID(t *testing.T) { for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - if got := ExtractID(tt.input); got != tt.want { + if got, err := ExtractID(tt.input); err != nil || got != tt.want { t.Errorf("ExtractID(%q) = %q, want %q", tt.input, got, tt.want) } }) @@ -350,7 +350,10 @@ func TestExtractWithProject(t *testing.T) { for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - gotRecordingID, gotProjectID := ExtractWithProject(tt.input) + gotRecordingID, gotProjectID, err := ExtractWithProject(tt.input) + if err != nil { + t.Fatal(err) + } if gotRecordingID != tt.wantRecordingID { t.Errorf("recordingID = %q, want %q", gotRecordingID, tt.wantRecordingID) } @@ -379,7 +382,10 @@ func TestExtractCommentWithProject(t *testing.T) { for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - gotID, gotProj := ExtractCommentWithProject(tt.input) + gotID, gotProj, err := ExtractCommentWithProject(tt.input) + if err != nil { + t.Fatal(err) + } if gotID != tt.wantID { t.Errorf("id = %q, want %q", gotID, tt.wantID) } @@ -397,7 +403,10 @@ func TestExtractIDs(t *testing.T) { "https://3.basecamp.com/123/buckets/456/messages/222", } want := []string{"789", "111", "222"} - got := ExtractIDs(args) + got, err := ExtractIDs(args) + if err != nil { + t.Fatal(err) + } if len(got) != len(want) { t.Errorf("ExtractIDs() = %v, want %v", got, want) @@ -413,7 +422,10 @@ func TestExtractIDs(t *testing.T) { func TestExtractIDs_CommaSeparated(t *testing.T) { args := []string{"111,222,333"} want := []string{"111", "222", "333"} - got := ExtractIDs(args) + got, err := ExtractIDs(args) + if err != nil { + t.Fatal(err) + } if len(got) != len(want) { t.Errorf("ExtractIDs() = %v, want %v", got, want) @@ -429,7 +441,10 @@ func TestExtractIDs_CommaSeparated(t *testing.T) { func TestExtractIDs_CommaSeparatedWithSpaces(t *testing.T) { args := []string{"111, 222, 333"} want := []string{"111", "222", "333"} - got := ExtractIDs(args) + got, err := ExtractIDs(args) + if err != nil { + t.Fatal(err) + } if len(got) != len(want) { t.Errorf("ExtractIDs() = %v, want %v", got, want) diff --git a/internal/urlarg/urlarg_trailing_test.go b/internal/urlarg/urlarg_trailing_test.go index a2d9eb69c..41b1886e1 100644 --- a/internal/urlarg/urlarg_trailing_test.go +++ b/internal/urlarg/urlarg_trailing_test.go @@ -239,5 +239,7 @@ func TestRefusedURLIsStillURL(t *testing.T) { url := "https://app.basecamp.com/99999/buckets/123/vaults/111/children/222" assert.True(t, IsURL(url)) assert.Nil(t, Parse(url)) - assert.Equal(t, url, ExtractID(url)) + id, err := ExtractID(url) + assert.NoError(t, err) + assert.Equal(t, url, id) } From 91064cab91ce3bec9de517eddc944b5a3f282b7c Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 22:28:09 -0400 Subject: [PATCH 03/11] Read an occurrence's attachments and comments from the occurrence attachments list and download refused an occurrence URL with a comment fragment before the fragment could name the comment. The comment now wins, as everywhere else. Without a fragment they read the occurrence itself, a read like show, whose description is its event's. show and schedule show read the comments of the event's series and suggested commenting on it, though the occurrence is a recording with comments of its own. They now use the occurrence's recording ID, and schedule show drops its update breadcrumb there, since the API cannot update one occurrence. The tests that guard the entry URL and bare ID now answer each request in the shape the SDK expects and require success and the exact request. Four of the eight commands failed under the old mock and still passed. --- internal/commands/attachments.go | 127 +++++++---- internal/commands/attachments_test.go | 12 +- internal/commands/occurrence_url_test.go | 267 ++++++++++++++++++----- internal/commands/schedule.go | 11 +- internal/commands/show.go | 13 +- 5 files changed, 318 insertions(+), 112 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index efda07a6e..3a5a3aa5f 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -32,7 +32,7 @@ import ( // 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` +// Kept command-agnostic: fetchAttachmentItem 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)" @@ -94,10 +94,11 @@ You can pass either an ID or a Basecamp URL: func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { app := appctx.FromContext(cmd.Context()) - id, resolvedType, err := resolveAttachmentTarget(arg, recordType) + target, err := resolveAttachmentTarget(arg, recordType) if err != nil { return err } + id, resolvedType := target.id, target.recordType // Validate type early (before account check) for better error messages if !isGenericType(resolvedType) && typeToEndpoint(resolvedType, id) == "" { @@ -114,12 +115,25 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { // Fetch the item — same two-step pattern as show.go: // 1. Generic recording lookup to discover type // 2. Re-fetch via type-specific endpoint for full content - content, err := fetchItemContent(cmd, app, id, resolvedType) + data, err := fetchAttachmentItem(cmd, app, target) if err != nil { return err } - attachments := richtext.ParseAttachments(content) + attachments := richtext.ParseAttachments(extractContentField(data)) + + // An occurrence shows its event's description but has comments of its + // own, so reads name it by its URL and a comment goes to its recording. + readRef := id + showCmd := fmt.Sprintf("basecamp show %s", id) + if showType := normalizeShowType(resolvedType); showType != "" { + showCmd = fmt.Sprintf("basecamp show %s --type %s", id, showType) + } + if target.occurrence != "" { + id = fmt.Sprint(data["id"]) + readRef = arg + showCmd = fmt.Sprintf("basecamp show %s", arg) + } // Styled TTY output if app.Output.EffectiveFormat() == output.FormatStyled { @@ -159,17 +173,12 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { } if downloadable > 0 { fmt.Fprintln(w) - fmt.Fprintln(w, muted.Render(fmt.Sprintf("Download: basecamp attachments download %s", id))) + fmt.Fprintln(w, muted.Render(fmt.Sprintf("Download: basecamp attachments download %s", readRef))) } return nil } // JSON/agent output - showCmd := fmt.Sprintf("basecamp show %s", id) - if showType := normalizeShowType(resolvedType); showType != "" { - showCmd = fmt.Sprintf("basecamp show %s --type %s", id, showType) - } - breadcrumbs := []output.Breadcrumb{ {Action: "show", Cmd: showCmd, Description: "Show item"}, } @@ -183,7 +192,7 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { } downloadable := downloadableAttachments(attachments) if len(downloadable) > 0 { - breadcrumbs = append(breadcrumbs, attachmentBreadcrumb(id, len(downloadable))) + breadcrumbs = append(breadcrumbs, attachmentBreadcrumb(readRef, len(downloadable))) } respOpts := []output.ResponseOption{ @@ -232,10 +241,11 @@ Options: } // Resolve target — reuse the same URL/comment parsing as list - idStr, resolvedType, err := resolveAttachmentTarget(args[0], recordType) + target, err := resolveAttachmentTarget(args[0], recordType) if err != nil { return err } + idStr, resolvedType := target.id, target.recordType // Validate type early (same guard as list) if !isGenericType(resolvedType) && typeToEndpoint(resolvedType, idStr) == "" { @@ -246,13 +256,16 @@ Options: } // Fetch recording content - content, err := fetchItemContent(cmd, app, idStr, resolvedType) + data, err := fetchAttachmentItem(cmd, app, target) if err != nil { return err } + if target.occurrence != "" { + idStr = args[0] + } // Parse all attachments (same parser as list — consistent indexes) - allAttachments := richtext.ParseAttachments(content) + allAttachments := richtext.ParseAttachments(extractContentField(data)) if len(allAttachments) == 0 { return output.ErrUsageHint( "No downloadable attachments found", @@ -439,15 +452,20 @@ func shouldSuggestType(recordType string) bool { } } -// 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) { +// fetchAttachmentItem retrieves the item whose rich text holds the +// attachments. Uses the same recording-type discovery pattern as show.go. +func fetchAttachmentItem(cmd *cobra.Command, app *appctx.App, target attachmentTarget) (map[string]any, error) { + id, recordType := target.id, target.recordType + // If type is provided, go directly to the type-specific endpoint. // Caller validates the type before calling, so typeToEndpoint won't return "". var endpoint string - if !isGenericType(recordType) { + switch { + case target.occurrence != "": + endpoint = fmt.Sprintf("/schedule_entries/%s/occurrences/%s.json", id, target.occurrence) + case !isGenericType(recordType): endpoint = typeToEndpoint(recordType, id) - } else { + default: // Generic recording lookup first endpoint = fmt.Sprintf("/recordings/%s.json", id) } @@ -464,22 +482,22 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string if shouldSuggestType(recordType) { var sdkErr *basecamp.Error if errors.As(err, &sdkErr) && sdkErr.Code == basecamp.CodeNotFound { - return "", output.ErrUsageHint( + return nil, output.ErrUsageHint( fmt.Sprintf("Item %s not found or type required", id), genericLookupTypeHint, ) } } - return "", convertSDKError(err) + return nil, convertSDKError(err) } if resp.StatusCode == http.StatusNoContent { if shouldSuggestType(recordType) { - return "", output.ErrUsageHint( + return nil, output.ErrUsageHint( fmt.Sprintf("Item %s not found or type required", id), genericLookupTypeHint, ) } - return "", output.ErrNotFound("item", id) + return nil, output.ErrNotFound("item", id) } // UseNumber so parentRecordingID sees json.Number for large IDs @@ -488,7 +506,7 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string dec := json.NewDecoder(bytes.NewReader(resp.Data)) dec.UseNumber() if err := dec.Decode(&data); err != nil { - return "", fmt.Errorf("failed to parse response: %w", err) + return nil, fmt.Errorf("failed to parse response: %w", err) } // If we used generic recording, re-fetch via type-specific endpoint for full content @@ -506,8 +524,7 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string } } - // Extract content — different item types use different field names - return extractContentField(data), nil + return data, nil } // extractContentField extracts the rich-text HTML content from a recording's @@ -539,31 +556,47 @@ func extractContentField(data map[string]any) string { return content } -// resolveAttachmentTarget extracts the recording ID and type from a raw -// argument (plain ID or Basecamp URL) and an optional explicit type hint. -// Prefers CommentID when present and type is compatible. -func resolveAttachmentTarget(arg, recordType string) (id, resolvedType string, err error) { - id, err = extractID(arg) - if err != nil { - return "", "", err - } - resolvedType = recordType +// attachmentTarget is the item whose attachments list and download read. +// For one occurrence of a recurring event, id is the event's and occurrence +// is the date. +type attachmentTarget struct { + id string + recordType string + occurrence string +} - if parsed := urlarg.Parse(arg); parsed != nil { - if parsed.CommentID != "" && (resolvedType == "" || resolvedType == "comment" || resolvedType == "comments") { - id = parsed.CommentID - if resolvedType == "" { - resolvedType = "comment" - } - } else if parsed.RecordingID != "" { - id = parsed.RecordingID - } - if resolvedType == "" && parsed.Type != "" { - resolvedType = parsed.Type +// resolveAttachmentTarget reads the target from a raw argument (plain ID or +// Basecamp URL) and an optional explicit type hint. A comment fragment wins +// when the type allows a comment, even on an occurrence URL. +func resolveAttachmentTarget(arg, recordType string) (attachmentTarget, error) { + parsed := urlarg.Parse(arg) + if parsed != nil && parsed.CommentID != "" && (recordType == "" || recordType == "comment" || recordType == "comments") { + if recordType == "" { + recordType = "comment" } + return attachmentTarget{id: parsed.CommentID, recordType: recordType}, nil + } + if parsed != nil && recordType == "" { + recordType = parsed.Type + } + if parsed != nil && parsed.OccurrenceDate != "" && isScheduleEntryType(recordType) { + return attachmentTarget{id: parsed.RecordingID, recordType: recordType, occurrence: parsed.OccurrenceDate}, nil } - return id, resolvedType, nil + id, err := extractID(arg) + if err != nil { + return attachmentTarget{}, err + } + return attachmentTarget{id: id, recordType: recordType}, nil +} + +func isScheduleEntryType(recordType string) bool { + switch recordType { + case "schedule-entry", "schedule_entry", "schedule_entries": + return true + default: + return false + } } // normalizeShowType maps attachment type aliases to canonical types that diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index b383d52be..ad0683caa 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -72,10 +72,10 @@ func TestResolveAttachmentTarget(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - id, typ, err := resolveAttachmentTarget(tt.arg, tt.typeHint) + target, err := resolveAttachmentTarget(tt.arg, tt.typeHint) require.NoError(t, err) - assert.Equal(t, tt.wantID, id) - assert.Equal(t, tt.wantType, typ) + assert.Equal(t, tt.wantID, target.id) + assert.Equal(t, tt.wantType, target.recordType) }) } } @@ -475,11 +475,11 @@ func TestWriteBodyToFile(t *testing.T) { }) } -// TestFetchItemContentRefetchesLineWithLargeParentID verifies that -// fetchItemContent uses UseNumber when decoding the recording response, +// TestFetchAttachmentItemRefetchesLineWithLargeParentID verifies that +// fetchAttachmentItem uses UseNumber when decoding the recording response, // so parentRecordingID can build the correct refetch endpoint even when // the parent ID exceeds 2^53. -func TestFetchItemContentRefetchesLineWithLargeParentID(t *testing.T) { +func TestFetchAttachmentItemRefetchesLineWithLargeParentID(t *testing.T) { largeID := "9007199254740993" // 2^53 + 1 transport := &showTrackingTransport{ diff --git a/internal/commands/occurrence_url_test.go b/internal/commands/occurrence_url_test.go index df2705565..7ef953d8e 100644 --- a/internal/commands/occurrence_url_test.go +++ b/internal/commands/occurrence_url_test.go @@ -1,6 +1,8 @@ package commands import ( + "bytes" + "encoding/json" "errors" "fmt" "io" @@ -16,41 +18,111 @@ import ( "github.com/basecamp/basecamp-cli/internal/output" ) +// The series is entry 789. Its occurrence on 2026-10-22 is recording 1001, +// and comment 555 sits on that occurrence. const ( occurrenceURL = "https://3.basecamp.com/99999/buckets/123/schedule_entries/789/occurrences/20261022" occurrenceEditURL = occurrenceURL + "/edit" entryURL = "https://3.basecamp.com/99999/buckets/123/schedule_entries/789" ) -// occurrenceTransport records every request and answers each with entry 789, -// which is enough for a command to reach the API. -type occurrenceTransport struct { +const ( + occurrenceEntryJSON = `{"id": 789, "type": "Schedule::Entry", "summary": "Standup", "all_day": false, + "starts_at": "2026-10-20T15:00:00Z", "ends_at": "2026-10-20T16:00:00Z", + "description": "
Agenda
", + "bucket": {"id": 123}}` + occurrenceJSON = `{"id": 1001, "type": "Schedule::Entry::Occurrence", "summary": "Standup", + "starts_at": "2026-10-22T15:00:00Z", "ends_at": "2026-10-22T16:00:00Z", + "description": "
Agenda
", + "comments_count": 1, "parent": {"id": 789, "type": "Schedule::Entry"}, "bucket": {"id": 123}}` + occurrenceCommentJSON = `{"id": 555, "type": "Comment", + "content": "
Notes
", + "parent": {"id": 1001, "type": "Schedule::Entry::Occurrence"}, "bucket": {"id": 123}}` +) + +// occurrenceAPI answers the requests these commands make with the shape the +// SDK expects, and 404s anything else, so a command only succeeds when it +// asked for the right thing. +type occurrenceAPI struct { mu sync.Mutex requests []string } -func (t *occurrenceTransport) RoundTrip(req *http.Request) (*http.Response, error) { - t.mu.Lock() - t.requests = append(t.requests, req.Method+" "+req.URL.Path) - t.mu.Unlock() +func (a *occurrenceAPI) RoundTrip(req *http.Request) (*http.Response, error) { + a.mu.Lock() + a.requests = append(a.requests, req.Method+" "+req.URL.Path) + a.mu.Unlock() + + path := strings.TrimSuffix(strings.TrimPrefix(req.URL.Path, "/99999"), ".json") + status, body := occurrenceAPIResponse(req.Method, path) header := make(http.Header) header.Set("Content-Type", "application/json") + if path == "/recordings/1001/comments" { + header.Set("X-Total-Count", "1") + } return &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(strings.NewReader(`{"id": 789}`)), + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), Header: header, Request: req, }, nil } +func (a *occurrenceAPI) got() []string { + a.mu.Lock() + defer a.mu.Unlock() + return append([]string(nil), a.requests...) +} + +func occurrenceAPIResponse(method, path string) (int, string) { + switch method + " " + path { + case "GET /schedule_entries/789", "PUT /schedule_entries/789", "GET /recordings/789": + return http.StatusOK, occurrenceEntryJSON + case "GET /schedule_entries/789/occurrences/20261022": + return http.StatusOK, occurrenceJSON + case "PUT /recordings/789/status/trashed", "PUT /recordings/789/status/archived": + return http.StatusNoContent, "" + case "POST /recordings/789/comments", "POST /recordings/1001/comments": + return http.StatusCreated, `{"id": 556, "content": "
hello
"}` + case "GET /recordings/789/comments": + return http.StatusOK, `[]` + case "GET /recordings/1001/comments": + return http.StatusOK, `[{"id": 555, "content": "
Notes
"}]` + case "POST /recordings/789/boosts": + return http.StatusCreated, `{"id": 31, "content": "yes"}` + case "POST /recordings/789/subscription": + return http.StatusOK, `{"subscribed": true, "count": 1, "subscribers": []}` + case "GET /comments/555", "PUT /comments/555": + return http.StatusOK, occurrenceCommentJSON + case "GET /blobs/notes/download/notes.txt", "GET /blobs/agenda/download/agenda.pdf": + return http.StatusOK, "file body" + } + return http.StatusNotFound, `{"error": "not found"}` +} + +func runOccurrenceCommand(t *testing.T, cmd *cobra.Command, args ...string) ([]string, *bytes.Buffer, error) { + t.Helper() + api := &occurrenceAPI{} + app, out := setupMessagesMockApp(t, api) + app.Flags.Hints = true + err := executeMessagesCommand(cmd, app, args...) + return api.got(), out, err +} + type occurrenceCase struct { name string cmd func() *cobra.Command args string // %s is the target } +func (c occurrenceCase) run(t *testing.T, target string) ([]string, error) { + t.Helper() + requests, _, err := runOccurrenceCommand(t, c.cmd(), strings.Fields(fmt.Sprintf(c.args, target))...) + return requests, err +} + // Between them, these commands reach the entry through each shared extractor. -var occurrenceCases = []occurrenceCase{ +var refusedOccurrenceCases = []occurrenceCase{ {"schedule update", NewScheduleCmd, "update %s --summary X"}, {"recordings trash", NewRecordingsCmd, "trash %s"}, {"recordings archive", NewRecordingsCmd, "archive %s"}, @@ -58,24 +130,14 @@ var occurrenceCases = []occurrenceCase{ {"comments list", NewCommentsCmd, "list %s"}, {"boost create", NewBoostsCmd, "create %s yes"}, {"subscriptions subscribe", NewSubscriptionsCmd, "subscribe %s"}, - {"attachments list", NewAttachmentsCmd, "list %s"}, -} - -func runOccurrenceCase(t *testing.T, c occurrenceCase, target string) ([]string, error) { - t.Helper() - transport := &occurrenceTransport{} - app, _ := setupMessagesMockApp(t, transport) - err := executeMessagesCommand(c.cmd(), app, strings.Fields(fmt.Sprintf(c.args, target))...) - transport.mu.Lock() - defer transport.mu.Unlock() - return transport.requests, err + {"attachments list as a todo", NewAttachmentsCmd, "list %s --type todo"}, } func TestOccurrenceURLIsRefused(t *testing.T) { - for _, c := range occurrenceCases { + for _, c := range refusedOccurrenceCases { for _, target := range []string{occurrenceURL, occurrenceEditURL} { t.Run(c.name+" "+target, func(t *testing.T) { - requests, err := runOccurrenceCase(t, c, target) + requests, err := c.run(t, target) var outErr *output.Error require.True(t, errors.As(err, &outErr), "want a usage error, got %v", err) @@ -89,52 +151,153 @@ func TestOccurrenceURLIsRefused(t *testing.T) { } func TestEntryURLAndIDStillReachTheEntry(t *testing.T) { - for _, c := range occurrenceCases { + cases := []struct { + occurrenceCase + want []string + wantID []string // when the bare ID asks differently from the URL + }{ + {refusedOccurrenceCases[0], []string{"GET /99999/schedule_entries/789", "PUT /99999/schedule_entries/789"}, nil}, + {refusedOccurrenceCases[1], []string{"PUT /99999/recordings/789/status/trashed.json"}, nil}, + {refusedOccurrenceCases[2], []string{"PUT /99999/recordings/789/status/archived.json"}, nil}, + {refusedOccurrenceCases[3], []string{"POST /99999/recordings/789/comments.json"}, nil}, + {refusedOccurrenceCases[4], []string{"GET /99999/recordings/789/comments.json"}, nil}, + {refusedOccurrenceCases[5], []string{"POST /99999/recordings/789/boosts.json"}, nil}, + {refusedOccurrenceCases[6], []string{"POST /99999/recordings/789/subscription.json"}, nil}, + { + occurrenceCase{"attachments list", NewAttachmentsCmd, "list %s"}, + []string{"GET /99999/schedule_entries/789.json"}, + []string{"GET /99999/recordings/789.json", "GET /99999/schedule_entries/789.json"}, + }, + } + for _, c := range cases { for _, target := range []string{entryURL, "789"} { t.Run(c.name+" "+target, func(t *testing.T) { - requests, err := runOccurrenceCase(t, c, target) + requests, err := c.run(t, target) - var outErr *output.Error - if errors.As(err, &outErr) { - assert.NotContains(t, outErr.Message, "occurrence") + require.NoError(t, err, "requests: %v", requests) + want := c.want + if target == "789" && c.wantID != nil { + want = c.wantID } - assert.True(t, slicesContainSubstring(requests, "/789"), "no request for entry 789: %v", requests) + assert.Equal(t, want, requests) }) } } } func TestCommentFragmentOnOccurrenceURLNamesTheComment(t *testing.T) { - transport := &occurrenceTransport{} - app, _ := setupMessagesMockApp(t, transport) + commentURL := occurrenceURL + "#__recording_555" + cases := []struct { + name string + cmd func() *cobra.Command + args []string + want []string + }{ + {"comments update", NewCommentsCmd, []string{"update", commentURL, "edited"}, + []string{"PUT /99999/comments/555"}}, + {"attachments list", NewAttachmentsCmd, []string{"list", commentURL}, + []string{"GET /99999/comments/555.json"}}, + {"attachments list --type comment", NewAttachmentsCmd, []string{"list", commentURL, "--type", "comment"}, + []string{"GET /99999/comments/555.json"}}, + {"attachments download", NewAttachmentsCmd, []string{"download", commentURL, "--out", t.TempDir()}, + []string{"GET /99999/comments/555.json", "GET /99999/blobs/notes/download/notes.txt"}}, + {"attachments download --type comment", NewAttachmentsCmd, []string{"download", commentURL, "--type", "comment", "--out", t.TempDir()}, + []string{"GET /99999/comments/555.json", "GET /99999/blobs/notes/download/notes.txt"}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + requests, _, err := runOccurrenceCommand(t, c.cmd(), c.args...) - err := executeMessagesCommand(NewCommentsCmd(), app, "update", occurrenceURL+"#__recording_555", "edited") - require.NoError(t, err) - assert.True(t, slicesContainSubstring(transport.requests, "/comments/555"), "requests: %v", transport.requests) + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, c.want, requests) + }) + } } -func TestScheduleShowOccurrenceEditURL(t *testing.T) { - transport := &mockScheduleShowTransport{} - app, _ := setupMessagesMockApp(t, transport) +type occurrenceEnvelope struct { + Data json.RawMessage `json:"data"` + Summary string `json:"summary"` + Breadcrumbs []output.Breadcrumb `json:"breadcrumbs"` +} - err := executeMessagesCommand(NewScheduleCmd(), app, "show", occurrenceEditURL) - require.NoError(t, err) - assert.True(t, slicesContainSubstring(transport.requests, "/schedule_entries/789/occurrences/20261022"), - "requests: %v", transport.requests) +func decodeOccurrenceEnvelope(t *testing.T, out *bytes.Buffer) occurrenceEnvelope { + t.Helper() + var env occurrenceEnvelope + require.NoError(t, json.Unmarshal(out.Bytes(), &env), out.String()) + return env } -func TestShowOccurrenceEditURL(t *testing.T) { - reqs, err := runShowCmd(t, &showTrackingTransport{}, occurrenceEditURL) - require.NoError(t, err) - require.NotEmpty(t, reqs) - assert.Contains(t, reqs[0], "/schedule_entries/789/occurrences/20261022.json") +func (env occurrenceEnvelope) cmds() []string { + cmds := make([]string, 0, len(env.Breadcrumbs)) + for _, b := range env.Breadcrumbs { + cmds = append(cmds, b.Cmd) + } + return cmds } -func slicesContainSubstring(items []string, sub string) bool { - for _, item := range items { - if strings.Contains(item, sub) { - return true - } +// A read of an occurrence goes through the occurrence, and anything a +// breadcrumb suggests writing goes to the occurrence's own recording. +func TestAttachmentsListOccurrenceURL(t *testing.T) { + requests, out, err := runOccurrenceCommand(t, NewAttachmentsCmd(), "list", occurrenceURL) + + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, []string{"GET /99999/schedule_entries/789/occurrences/20261022.json"}, requests) + env := decodeOccurrenceEnvelope(t, out) + assert.Contains(t, string(env.Data), "agenda.pdf") + assert.Equal(t, []string{ + "basecamp show " + occurrenceURL, + "basecamp comments create 1001 ", + "basecamp attachments download " + occurrenceURL, + }, env.cmds()) +} + +func TestAttachmentsDownloadOccurrenceURL(t *testing.T) { + requests, _, err := runOccurrenceCommand(t, NewAttachmentsCmd(), "download", occurrenceEditURL, "--out", t.TempDir()) + + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, []string{ + "GET /99999/schedule_entries/789/occurrences/20261022.json", + "GET /99999/blobs/agenda/download/agenda.pdf", + }, requests) +} + +func TestScheduleShowOccurrenceURLReadsItsComments(t *testing.T) { + for _, target := range []string{occurrenceURL, occurrenceEditURL} { + t.Run(target, func(t *testing.T) { + requests, out, err := runOccurrenceCommand(t, NewScheduleCmd(), "show", target, "--comments") + + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, []string{ + "GET /99999/schedule_entries/789/occurrences/20261022", + "GET /99999/recordings/1001/comments.json", + }, requests) + env := decodeOccurrenceEnvelope(t, out) + assert.Equal(t, []string{ + "basecamp comments create 1001 ", + "basecamp schedule entries --project 123", + "basecamp comments list --all 1001", + }, env.cmds()) + }) + } +} + +func TestShowOccurrenceURLReadsItsComments(t *testing.T) { + for _, target := range []string{occurrenceURL, occurrenceEditURL} { + t.Run(target, func(t *testing.T) { + requests, out, err := runOccurrenceCommand(t, NewShowCmd(), target) + + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, []string{ + "GET /99999/schedule_entries/789/occurrences/20261022.json", + "GET /99999/recordings/1001/comments.json", + }, requests) + env := decodeOccurrenceEnvelope(t, out) + assert.Contains(t, env.Summary, "#1001") + assert.Contains(t, env.cmds(), "basecamp comments create 1001 ") + assert.Contains(t, env.cmds(), "basecamp comments list --all 1001") + for _, cmd := range env.cmds() { + assert.NotContains(t, cmd, "comments create 789", "a breadcrumb must not write to the series") + } + }) } - return false } diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index 9afb3ea21..b95e04025 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -336,7 +336,10 @@ func runScheduleEntryShow(cmd *cobra.Command, app *appctx.App, entryID, project, title = "Entry" } - enrichment := fetchCommentsForRecording(cmd.Context(), app, entryID, cf) + // The occurrence is a recording of its own, with its own comments. + // The API cannot update it, so there is no update breadcrumb. + occurrenceID := strconv.FormatInt(entry.ID, 10) + enrichment := fetchCommentsForRecording(cmd.Context(), app, occurrenceID, cf) data, commentOpts := enrichment.apply(entry, "") summary := fmt.Sprintf("%s: %s -> %s", title, entry.StartsAt.Format("2006-01-02 15:04"), entry.EndsAt.Format("2006-01-02 15:04")) @@ -345,9 +348,9 @@ func runScheduleEntryShow(cmd *cobra.Command, app *appctx.App, entryID, project, output.WithSummary(summary), output.WithBreadcrumbs( output.Breadcrumb{ - Action: "update", - Cmd: fmt.Sprintf("basecamp schedule update %s --summary \"...\" --project %s", entryID, resolvedProjectID), - Description: "Update entry", + Action: "comment", + Cmd: fmt.Sprintf("basecamp comments create %s ", occurrenceID), + Description: "Comment on this occurrence", }, output.Breadcrumb{ Action: "entries", diff --git a/internal/commands/show.go b/internal/commands/show.go index b86cd2341..13d530dad 100644 --- a/internal/commands/show.go +++ b/internal/commands/show.go @@ -257,10 +257,17 @@ You can also pass a Basecamp URL directly: return err } + // An occurrence URL carries its event's ID, but the occurrence is + // a recording of its own, with its own comments. + recordingID := id + if occurrenceDate != "" { + recordingID = fmt.Sprint(data["id"]) + } + // Skip comment fetch for non-commentable types. enrichment := &commentEnrichment{} if isCommentableShowType(recordType, data) { - enrichment = fetchRecordingComments(cmd.Context(), app, id, data, cf) + enrichment = fetchRecordingComments(cmd.Context(), app, recordingID, data, cf) if enrichment.Comments != nil { data["comments"] = enrichment.Comments } @@ -283,14 +290,14 @@ You can also pass a Basecamp URL directly: itemType = t } - summary := fmt.Sprintf("%s #%s: %s", itemType, id, title) + summary := fmt.Sprintf("%s #%s: %s", itemType, recordingID, title) if enrichment.CountLabel != "" { summary += fmt.Sprintf(" (%s)", enrichment.CountLabel) } breadcrumbs := make([]output.Breadcrumb, 0, 1+len(enrichment.Breadcrumbs)) breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "comment", - Cmd: fmt.Sprintf("basecamp comments create %s ", id), + Cmd: fmt.Sprintf("basecamp comments create %s ", recordingID), Description: "Add comment", }) breadcrumbs = append(breadcrumbs, enrichment.Breadcrumbs...) From c9ca892fc3e16769900a311df1c1d661cd3b265a Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 22:45:16 -0400 Subject: [PATCH 04/11] Suggest the occurrence, not its comment, after reading it by type With an occurrence URL carrying #__recording_555 and --type schedule-entry, attachments list and download read the occurrence, but their show and download breadcrumbs repeated the URL with its fragment. Run as suggested, those commands read comment 555 instead. The breadcrumbs now name the occurrence URL without the fragment. --- internal/commands/attachments.go | 14 ++++++----- internal/commands/occurrence_url_test.go | 32 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 3a5a3aa5f..19ebdcb27 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -131,8 +131,8 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error { } if target.occurrence != "" { id = fmt.Sprint(data["id"]) - readRef = arg - showCmd = fmt.Sprintf("basecamp show %s", arg) + readRef = target.url + showCmd = fmt.Sprintf("basecamp show %s", target.url) } // Styled TTY output @@ -261,7 +261,7 @@ Options: return err } if target.occurrence != "" { - idStr = args[0] + idStr = target.url } // Parse all attachments (same parser as list — consistent indexes) @@ -557,12 +557,13 @@ func extractContentField(data map[string]any) string { } // attachmentTarget is the item whose attachments list and download read. -// For one occurrence of a recurring event, id is the event's and occurrence -// is the date. +// For one occurrence of a recurring event, id is the event's, occurrence is +// the date, and url names the occurrence without any comment fragment. type attachmentTarget struct { id string recordType string occurrence string + url string } // resolveAttachmentTarget reads the target from a raw argument (plain ID or @@ -580,7 +581,8 @@ func resolveAttachmentTarget(arg, recordType string) (attachmentTarget, error) { recordType = parsed.Type } if parsed != nil && parsed.OccurrenceDate != "" && isScheduleEntryType(recordType) { - return attachmentTarget{id: parsed.RecordingID, recordType: recordType, occurrence: parsed.OccurrenceDate}, nil + url, _, _ := strings.Cut(arg, "#") + return attachmentTarget{id: parsed.RecordingID, recordType: recordType, occurrence: parsed.OccurrenceDate, url: url}, nil } id, err := extractID(arg) diff --git a/internal/commands/occurrence_url_test.go b/internal/commands/occurrence_url_test.go index 7ef953d8e..38a7a096c 100644 --- a/internal/commands/occurrence_url_test.go +++ b/internal/commands/occurrence_url_test.go @@ -261,6 +261,38 @@ func TestAttachmentsDownloadOccurrenceURL(t *testing.T) { }, requests) } +// With --type schedule-entry, a comment fragment does not pick the comment, +// so no breadcrumb may carry the fragment that would. +func TestOccurrenceBreadcrumbsDropCommentFragment(t *testing.T) { + commentURL := occurrenceURL + "#__recording_555" + cases := []struct { + name string + args []string + wantReqs []string + wantCrumb []string + }{ + {"attachments list", []string{"list", commentURL, "--type", "schedule-entry"}, + []string{"GET /99999/schedule_entries/789/occurrences/20261022.json"}, + []string{ + "basecamp show " + occurrenceURL, + "basecamp comments create 1001 ", + "basecamp attachments download " + occurrenceURL, + }}, + {"attachments download", []string{"download", commentURL, "--type", "schedule-entry", "--out", t.TempDir()}, + []string{"GET /99999/schedule_entries/789/occurrences/20261022.json", "GET /99999/blobs/agenda/download/agenda.pdf"}, + []string{"basecamp show " + occurrenceURL}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + requests, out, err := runOccurrenceCommand(t, NewAttachmentsCmd(), c.args...) + + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, c.wantReqs, requests) + assert.Equal(t, c.wantCrumb, decodeOccurrenceEnvelope(t, out).cmds()) + }) + } +} + func TestScheduleShowOccurrenceURLReadsItsComments(t *testing.T) { for _, target := range []string{occurrenceURL, occurrenceEditURL} { t.Run(target, func(t *testing.T) { From 8a751a0b3de9330a3128dd2acb4fc3c75f1943b3 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 22:54:32 -0400 Subject: [PATCH 05/11] Read an occurrence's attachments under --type recording attachments list and download read an occurrence URL with no --type, but --type recording or recordings sent it to the refusal instead, though neither names a type. Both now read the occurrence, as with no --type. A type that names something else, such as todo or line, is still refused. --- internal/commands/attachments.go | 20 ++++++----- internal/commands/occurrence_url_test.go | 46 +++++++++++++++--------- 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 19ebdcb27..68e0a36f0 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -438,12 +438,12 @@ 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 { +// namesNoType reports whether recordType leaves the item's type open: no +// --type, or one meaning any recording. The "line"/"lines"/"replies" aliases +// route through the same generic /recordings lookup for parent discovery, +// but they name a type, so telling those callers to "specify --type" is +// misleading. +func namesNoType(recordType string) bool { switch recordType { case "", "recording", "recordings": return true @@ -479,7 +479,7 @@ func fetchAttachmentItem(cmd *cobra.Command, app *appctx.App, target attachmentT // "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) { + if namesNoType(recordType) { var sdkErr *basecamp.Error if errors.As(err, &sdkErr) && sdkErr.Code == basecamp.CodeNotFound { return nil, output.ErrUsageHint( @@ -491,7 +491,7 @@ func fetchAttachmentItem(cmd *cobra.Command, app *appctx.App, target attachmentT return nil, convertSDKError(err) } if resp.StatusCode == http.StatusNoContent { - if shouldSuggestType(recordType) { + if namesNoType(recordType) { return nil, output.ErrUsageHint( fmt.Sprintf("Item %s not found or type required", id), genericLookupTypeHint, @@ -577,7 +577,9 @@ func resolveAttachmentTarget(arg, recordType string) (attachmentTarget, error) { } return attachmentTarget{id: parsed.CommentID, recordType: recordType}, nil } - if parsed != nil && recordType == "" { + // Left open, the type would be looked up by the event's ID and read the + // whole series, so an occurrence URL takes its type from the URL. + if parsed != nil && (recordType == "" || (parsed.OccurrenceDate != "" && namesNoType(recordType))) { recordType = parsed.Type } if parsed != nil && parsed.OccurrenceDate != "" && isScheduleEntryType(recordType) { diff --git a/internal/commands/occurrence_url_test.go b/internal/commands/occurrence_url_test.go index 38a7a096c..c7a0363e7 100644 --- a/internal/commands/occurrence_url_test.go +++ b/internal/commands/occurrence_url_test.go @@ -131,6 +131,7 @@ var refusedOccurrenceCases = []occurrenceCase{ {"boost create", NewBoostsCmd, "create %s yes"}, {"subscriptions subscribe", NewSubscriptionsCmd, "subscribe %s"}, {"attachments list as a todo", NewAttachmentsCmd, "list %s --type todo"}, + {"attachments list as a chat line", NewAttachmentsCmd, "list %s --type line"}, } func TestOccurrenceURLIsRefused(t *testing.T) { @@ -235,30 +236,41 @@ func (env occurrenceEnvelope) cmds() []string { return cmds } +// untypedHints leave the type open, so each reads the occurrence the URL names. +var untypedHints = [][]string{nil, {"--type", "recording"}, {"--type", "recordings"}} + // A read of an occurrence goes through the occurrence, and anything a // breadcrumb suggests writing goes to the occurrence's own recording. func TestAttachmentsListOccurrenceURL(t *testing.T) { - requests, out, err := runOccurrenceCommand(t, NewAttachmentsCmd(), "list", occurrenceURL) - - require.NoError(t, err, "requests: %v", requests) - assert.Equal(t, []string{"GET /99999/schedule_entries/789/occurrences/20261022.json"}, requests) - env := decodeOccurrenceEnvelope(t, out) - assert.Contains(t, string(env.Data), "agenda.pdf") - assert.Equal(t, []string{ - "basecamp show " + occurrenceURL, - "basecamp comments create 1001 ", - "basecamp attachments download " + occurrenceURL, - }, env.cmds()) + for _, hint := range untypedHints { + t.Run(strings.Join(hint, " "), func(t *testing.T) { + requests, out, err := runOccurrenceCommand(t, NewAttachmentsCmd(), append([]string{"list", occurrenceURL}, hint...)...) + + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, []string{"GET /99999/schedule_entries/789/occurrences/20261022.json"}, requests) + env := decodeOccurrenceEnvelope(t, out) + assert.Contains(t, string(env.Data), "agenda.pdf") + assert.Equal(t, []string{ + "basecamp show " + occurrenceURL, + "basecamp comments create 1001 ", + "basecamp attachments download " + occurrenceURL, + }, env.cmds()) + }) + } } func TestAttachmentsDownloadOccurrenceURL(t *testing.T) { - requests, _, err := runOccurrenceCommand(t, NewAttachmentsCmd(), "download", occurrenceEditURL, "--out", t.TempDir()) + for _, hint := range untypedHints { + t.Run(strings.Join(hint, " "), func(t *testing.T) { + requests, _, err := runOccurrenceCommand(t, NewAttachmentsCmd(), append([]string{"download", occurrenceEditURL, "--out", t.TempDir()}, hint...)...) - require.NoError(t, err, "requests: %v", requests) - assert.Equal(t, []string{ - "GET /99999/schedule_entries/789/occurrences/20261022.json", - "GET /99999/blobs/agenda/download/agenda.pdf", - }, requests) + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, []string{ + "GET /99999/schedule_entries/789/occurrences/20261022.json", + "GET /99999/blobs/agenda/download/agenda.pdf", + }, requests) + }) + } } // With --type schedule-entry, a comment fragment does not pick the comment, From 82e471c61d593629bdb117e40c2d6e0c1f76b927 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 23:03:43 -0400 Subject: [PATCH 06/11] Show an occurrence, not its series, under --type recording basecamp show --type recording (or recordings) read the series: it fetched recording 789 and the series' comments, and its breadcrumbs wrote to the series. Neither type names what the item is, so an occurrence URL now takes its type from the URL, as with no --type, and show reads the occurrence and its own comments. A type that names something else is still used as given. --- internal/commands/occurrence_url_test.go | 32 +++++++++++++----------- internal/commands/show.go | 6 +++-- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/internal/commands/occurrence_url_test.go b/internal/commands/occurrence_url_test.go index c7a0363e7..8e5917b2e 100644 --- a/internal/commands/occurrence_url_test.go +++ b/internal/commands/occurrence_url_test.go @@ -327,21 +327,23 @@ func TestScheduleShowOccurrenceURLReadsItsComments(t *testing.T) { func TestShowOccurrenceURLReadsItsComments(t *testing.T) { for _, target := range []string{occurrenceURL, occurrenceEditURL} { - t.Run(target, func(t *testing.T) { - requests, out, err := runOccurrenceCommand(t, NewShowCmd(), target) + for _, hint := range untypedHints { + t.Run(strings.Join(append([]string{target}, hint...), " "), func(t *testing.T) { + requests, out, err := runOccurrenceCommand(t, NewShowCmd(), append([]string{target}, hint...)...) - require.NoError(t, err, "requests: %v", requests) - assert.Equal(t, []string{ - "GET /99999/schedule_entries/789/occurrences/20261022.json", - "GET /99999/recordings/1001/comments.json", - }, requests) - env := decodeOccurrenceEnvelope(t, out) - assert.Contains(t, env.Summary, "#1001") - assert.Contains(t, env.cmds(), "basecamp comments create 1001 ") - assert.Contains(t, env.cmds(), "basecamp comments list --all 1001") - for _, cmd := range env.cmds() { - assert.NotContains(t, cmd, "comments create 789", "a breadcrumb must not write to the series") - } - }) + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, []string{ + "GET /99999/schedule_entries/789/occurrences/20261022.json", + "GET /99999/recordings/1001/comments.json", + }, requests) + env := decodeOccurrenceEnvelope(t, out) + assert.Contains(t, env.Summary, "#1001") + assert.Contains(t, env.cmds(), "basecamp comments create 1001 ") + assert.Contains(t, env.cmds(), "basecamp comments list --all 1001") + for _, cmd := range env.cmds() { + assert.NotContains(t, cmd, "comments create 789", "a breadcrumb must not write to the series") + } + }) + } } } diff --git a/internal/commands/show.go b/internal/commands/show.go index 13d530dad..e0c0f4319 100644 --- a/internal/commands/show.go +++ b/internal/commands/show.go @@ -88,8 +88,10 @@ You can also pass a Basecamp URL directly: ) } else if parsed.RecordingID != "" { id = parsed.RecordingID - // Auto-detect type from URL if not specified - if recordType == "" && parsed.Type != "" { + // Auto-detect type from URL if not specified. Left open, an + // occurrence's type would be looked up by the event's ID and + // read the whole series, so it takes its type from the URL. + if parsed.Type != "" && (recordType == "" || (parsed.OccurrenceDate != "" && namesNoType(recordType))) { recordType = parsed.Type untypedLookup = false // URL carries the type } From 92ea0e6a730fce986dcb5b2157439dd93d6b611f Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 03:44:39 -0400 Subject: [PATCH 07/11] Point occurrence URLs at the occurrence's own ID for comments The refusal said Basecamp's API cannot change a single occurrence and offered the series ID, so agents asked to comment on an occurrence either gave up or commented on the whole series. An occurrence takes comments and boosts under its own ID, which basecamp show prints; the hint now says so, and keeps the series ID for updates and trash. url parse reported an occurrence URL's series ID as recording_id and suggested commenting on it. It now adds occurrence_date, a notice that recording_id is the whole series, and suggests basecamp show . --- internal/commands/helpers.go | 8 +++++--- internal/commands/url.go | 29 +++++++++++++++++++++++++++++ internal/commands/url_test.go | 30 ++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 39006aa61..6f40d363e 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -586,13 +586,15 @@ func extractIDs(args []string) ([]string, error) { } // occurrenceUsage turns the extractors' refusal of an occurrence URL into a -// usage error that names the entry, the one thing the CLI can act on. +// usage error that names both IDs a caller can act on: the occurrence's own, +// which takes comments and boosts, and the entry's, which changes the series. func occurrenceUsage(err error) error { var occ *urlarg.OccurrenceError if errors.As(err, &occ) { return output.ErrUsageHint("That URL names one occurrence of a recurring event", - fmt.Sprintf("Basecamp's API cannot change a single occurrence. Pass the event's ID, %s, to act on the whole series, "+ - "or see the occurrence with basecamp show ", occ.EntryID)) + fmt.Sprintf("To comment on or boost just this occurrence, pass its own ID, which basecamp show prints. "+ + "Basecamp's API cannot change a single occurrence, so to update or trash the event, pass its ID, %s, "+ + "which acts on the whole series", occ.EntryID)) } return err } diff --git a/internal/commands/url.go b/internal/commands/url.go index 770dd2cb0..e32a3fe8a 100644 --- a/internal/commands/url.go +++ b/internal/commands/url.go @@ -20,6 +20,9 @@ type ParsedURL struct { TypeSingular *string `json:"type_singular"` RecordingID *string `json:"recording_id"` CommentID *string `json:"comment_id"` + // OccurrenceDate is set only for an occurrence URL, whose recording_id + // names the whole series. + OccurrenceDate *string `json:"occurrence_date,omitempty"` } // NewURLCmd creates the url command for parsing Basecamp URLs. @@ -142,6 +145,10 @@ func runURLParse(app *appctx.App, url string) error { if commentID != "" { result.CommentID = &commentID } + occurrenceDate := parsed.OccurrenceDate + if occurrenceDate != "" { + result.OccurrenceDate = &occurrenceDate + } // Build summary var summary string @@ -158,6 +165,9 @@ func runURLParse(app *appctx.App, url string) error { if commentID != "" { summary += fmt.Sprintf(", comment #%s", commentID) } + if occurrenceDate != "" { + summary += fmt.Sprintf(", occurrence %s", occurrenceDate) + } } else if bucketID != "" { if recordingType == "project" { summary = fmt.Sprintf("Project #%s", bucketID) @@ -172,6 +182,25 @@ func runURLParse(app *appctx.App, url string) error { // Build breadcrumbs var breadcrumbs []output.Breadcrumb + if occurrenceDate != "" { + return app.OK(result, + output.WithSummary(summary), + output.WithNotice("recording_id is the whole series. To comment on or boost just this occurrence, "+ + "use its own ID, which basecamp show prints"), + output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "show", + Cmd: fmt.Sprintf("basecamp show %s", url), + Description: "View the occurrence and its own ID", + }, + output.Breadcrumb{ + Action: "show-series", + Cmd: fmt.Sprintf("basecamp show %s %s --in %s", typeSingular, recordingID, bucketID), + Description: "View the whole series", + }, + ), + ) + } if recordingID != "" && bucketID != "" { // Special handling for card table types (column, step) switch typeSingular { diff --git a/internal/commands/url_test.go b/internal/commands/url_test.go index dfc479f63..5e5698085 100644 --- a/internal/commands/url_test.go +++ b/internal/commands/url_test.go @@ -413,3 +413,33 @@ func assertStringPtr(t *testing.T, got *string, want, name string) { require.NotNil(t, got, "%s is nil, want %q", name, want) assert.Equal(t, want, *got, "%s mismatch", name) } + +// An occurrence URL's recording_id is the whole series, so the response says +// so and points at the occurrence instead of suggesting a comment on the series. +func TestURLParseOccurrenceURL(t *testing.T) { + url := "https://3.basecamp.com/2914079/buckets/41746046/schedule_entries/789/occurrences/20261022" + resp, err := parseURLWithOutput(t, url) + require.NoError(t, err) + + parsed := getParsedURL(t, resp) + require.NotNil(t, parsed.OccurrenceDate) + assert.Equal(t, "20261022", *parsed.OccurrenceDate) + assert.Equal(t, "789", *parsed.RecordingID) + assert.Contains(t, resp.Notice, "whole series") + + cmds := make([]string, 0, len(resp.Breadcrumbs)) + for _, b := range resp.Breadcrumbs { + cmds = append(cmds, b.Cmd) + } + assert.Contains(t, cmds, "basecamp show "+url) + for _, c := range cmds { + assert.NotContains(t, c, "comments create 789", "a comment on the series ID would miss the occurrence") + } +} + +func TestURLParseOmitsOccurrenceDateForOtherURLs(t *testing.T) { + resp, err := parseURLWithOutput(t, "https://3.basecamp.com/2914079/buckets/41746046/schedule_entries/789") + require.NoError(t, err) + assert.Nil(t, getParsedURL(t, resp).OccurrenceDate) + assert.Empty(t, resp.Notice) +} From c41746a6d00cf5f5fa9bb3b53a5f6616511aca41 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 04:00:02 -0400 Subject: [PATCH 08/11] Send occurrence callers to schedule show, and drop the boost claim bc3 refuses a boost on an occurrence with a 403 (an occurrence has no content to boost), so the hint and url parse notice no longer offer it. basecamp show on an occurrence comment link resolves the comment first and prints the comment's ID. schedule show reads the occurrence from any form of its URL, so the hint, the notice and url parse's breadcrumb name it. The test that non-occurrence URLs omit occurrence_date now checks the decoded keys: an absent field and a null one unmarshal to the same nil. --- internal/commands/helpers.go | 9 +++++---- internal/commands/occurrence_url_test.go | 4 +++- internal/commands/url.go | 6 +++--- internal/commands/url_test.go | 20 ++++++++++++++++++-- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 6f40d363e..b32673cbf 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -587,14 +587,15 @@ func extractIDs(args []string) ([]string, error) { // occurrenceUsage turns the extractors' refusal of an occurrence URL into a // usage error that names both IDs a caller can act on: the occurrence's own, -// which takes comments and boosts, and the entry's, which changes the series. +// which takes comments, and the entry's, which acts on the series. bc3 refuses +// a boost on an occurrence with a 403. func occurrenceUsage(err error) error { var occ *urlarg.OccurrenceError if errors.As(err, &occ) { return output.ErrUsageHint("That URL names one occurrence of a recurring event", - fmt.Sprintf("To comment on or boost just this occurrence, pass its own ID, which basecamp show prints. "+ - "Basecamp's API cannot change a single occurrence, so to update or trash the event, pass its ID, %s, "+ - "which acts on the whole series", occ.EntryID)) + fmt.Sprintf("To comment on just this occurrence, pass its own ID, which basecamp schedule show prints. "+ + "For anything else, pass the event's ID, %s, which acts on the whole series. "+ + "Basecamp's API cannot change a single occurrence", occ.EntryID)) } return err } diff --git a/internal/commands/occurrence_url_test.go b/internal/commands/occurrence_url_test.go index 8e5917b2e..33998e216 100644 --- a/internal/commands/occurrence_url_test.go +++ b/internal/commands/occurrence_url_test.go @@ -305,8 +305,10 @@ func TestOccurrenceBreadcrumbsDropCommentFragment(t *testing.T) { } } +// A comment link reads the occurrence too, which is why url parse and the +// refusal hint send callers here for the occurrence's own ID. func TestScheduleShowOccurrenceURLReadsItsComments(t *testing.T) { - for _, target := range []string{occurrenceURL, occurrenceEditURL} { + for _, target := range []string{occurrenceURL, occurrenceEditURL, occurrenceURL + "#__recording_555"} { t.Run(target, func(t *testing.T) { requests, out, err := runOccurrenceCommand(t, NewScheduleCmd(), "show", target, "--comments") diff --git a/internal/commands/url.go b/internal/commands/url.go index e32a3fe8a..8f22f3a85 100644 --- a/internal/commands/url.go +++ b/internal/commands/url.go @@ -185,12 +185,12 @@ func runURLParse(app *appctx.App, url string) error { if occurrenceDate != "" { return app.OK(result, output.WithSummary(summary), - output.WithNotice("recording_id is the whole series. To comment on or boost just this occurrence, "+ - "use its own ID, which basecamp show prints"), + output.WithNotice("recording_id is the whole series. To comment on just this occurrence, "+ + "use its own ID, which basecamp schedule show prints"), output.WithBreadcrumbs( output.Breadcrumb{ Action: "show", - Cmd: fmt.Sprintf("basecamp show %s", url), + Cmd: fmt.Sprintf("basecamp schedule show %s", url), Description: "View the occurrence and its own ID", }, output.Breadcrumb{ diff --git a/internal/commands/url_test.go b/internal/commands/url_test.go index 5e5698085..0d5a2b0bf 100644 --- a/internal/commands/url_test.go +++ b/internal/commands/url_test.go @@ -431,7 +431,7 @@ func TestURLParseOccurrenceURL(t *testing.T) { for _, b := range resp.Breadcrumbs { cmds = append(cmds, b.Cmd) } - assert.Contains(t, cmds, "basecamp show "+url) + assert.Contains(t, cmds, "basecamp schedule show "+url) for _, c := range cmds { assert.NotContains(t, c, "comments create 789", "a comment on the series ID would miss the occurrence") } @@ -440,6 +440,22 @@ func TestURLParseOccurrenceURL(t *testing.T) { func TestURLParseOmitsOccurrenceDateForOtherURLs(t *testing.T) { resp, err := parseURLWithOutput(t, "https://3.basecamp.com/2914079/buckets/41746046/schedule_entries/789") require.NoError(t, err) - assert.Nil(t, getParsedURL(t, resp).OccurrenceDate) + data, ok := resp.Data.(map[string]any) + require.True(t, ok, "want an object, got %T", resp.Data) + assert.NotContains(t, data, "occurrence_date") assert.Empty(t, resp.Notice) } + +// show resolves a comment fragment first, so the occurrence breadcrumb must +// lead somewhere that reads the occurrence even from a comment link. +func TestURLParseOccurrenceCommentURL(t *testing.T) { + url := "https://3.basecamp.com/2914079/buckets/41746046/schedule_entries/789/occurrences/20261022#__recording_555" + resp, err := parseURLWithOutput(t, url) + require.NoError(t, err) + + parsed := getParsedURL(t, resp) + require.NotNil(t, parsed.CommentID) + assert.Equal(t, "555", *parsed.CommentID) + require.NotEmpty(t, resp.Breadcrumbs) + assert.Equal(t, "basecamp schedule show "+url, resp.Breadcrumbs[0].Cmd) +} From cb151f3422e834ff138493240ceab32d67f518c2 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 04:14:49 -0400 Subject: [PATCH 09/11] Name occurrence subscriptions in the hint, and drop the series breadcrumb An occurrence has its own comments and subscribers: unsubscribing from one on bc3 leaves the series subscription in place. The hint said anything but a comment goes through the event's ID, which subscribed to and listed the series. It now names comments, comment lists and subscriptions for the occurrence's ID, and updates, trash and boosts for the event's. url parse's series breadcrumb, show schedule_entry , shows the first occurrence, since bc3 redirects a recurring entry's GET there, and printed an empty --in for a URL with no project. It is gone. The notice now names the exact command: schedule show , or show when the URL names no project, since schedule show needs one. --- internal/commands/helpers.go | 10 +++++----- internal/commands/url.go | 27 +++++++++++++-------------- internal/commands/url_test.go | 15 +++++++++++++++ 3 files changed, 33 insertions(+), 19 deletions(-) diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index b32673cbf..792123d4d 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -587,15 +587,15 @@ func extractIDs(args []string) ([]string, error) { // occurrenceUsage turns the extractors' refusal of an occurrence URL into a // usage error that names both IDs a caller can act on: the occurrence's own, -// which takes comments, and the entry's, which acts on the series. bc3 refuses -// a boost on an occurrence with a 403. +// which has its own comments and subscribers, and the entry's, which acts on +// the series. bc3 refuses a boost on an occurrence with a 403. func occurrenceUsage(err error) error { var occ *urlarg.OccurrenceError if errors.As(err, &occ) { return output.ErrUsageHint("That URL names one occurrence of a recurring event", - fmt.Sprintf("To comment on just this occurrence, pass its own ID, which basecamp schedule show prints. "+ - "For anything else, pass the event's ID, %s, which acts on the whole series. "+ - "Basecamp's API cannot change a single occurrence", occ.EntryID)) + fmt.Sprintf("To comment on, list the comments of, or subscribe to just this occurrence, pass its own ID, "+ + "which basecamp schedule show prints. To update, trash or boost the event, pass its ID, %s, "+ + "which acts on the whole series. Basecamp's API cannot change a single occurrence", occ.EntryID)) } return err } diff --git a/internal/commands/url.go b/internal/commands/url.go index 8f22f3a85..da60d5e3c 100644 --- a/internal/commands/url.go +++ b/internal/commands/url.go @@ -183,22 +183,21 @@ func runURLParse(app *appctx.App, url string) error { // Build breadcrumbs var breadcrumbs []output.Breadcrumb if occurrenceDate != "" { + // schedule show reads the occurrence even from a comment link, where show + // would read the comment, but it needs the project the URL names. + showOccurrence := "basecamp schedule show " + url + if bucketID == "" { + showOccurrence = "basecamp show " + url + } return app.OK(result, output.WithSummary(summary), - output.WithNotice("recording_id is the whole series. To comment on just this occurrence, "+ - "use its own ID, which basecamp schedule show prints"), - output.WithBreadcrumbs( - output.Breadcrumb{ - Action: "show", - Cmd: fmt.Sprintf("basecamp schedule show %s", url), - Description: "View the occurrence and its own ID", - }, - output.Breadcrumb{ - Action: "show-series", - Cmd: fmt.Sprintf("basecamp show %s %s --in %s", typeSingular, recordingID, bucketID), - Description: "View the whole series", - }, - ), + output.WithNotice("recording_id is the whole series. To comment on or subscribe to just this occurrence, "+ + "use its own ID, which "+showOccurrence+" prints"), + output.WithBreadcrumbs(output.Breadcrumb{ + Action: "show", + Cmd: showOccurrence, + Description: "View the occurrence and its own ID", + }), ) } if recordingID != "" && bucketID != "" { diff --git a/internal/commands/url_test.go b/internal/commands/url_test.go index 0d5a2b0bf..048658b9f 100644 --- a/internal/commands/url_test.go +++ b/internal/commands/url_test.go @@ -426,6 +426,7 @@ func TestURLParseOccurrenceURL(t *testing.T) { assert.Equal(t, "20261022", *parsed.OccurrenceDate) assert.Equal(t, "789", *parsed.RecordingID) assert.Contains(t, resp.Notice, "whole series") + assert.Contains(t, resp.Notice, "basecamp schedule show "+url) cmds := make([]string, 0, len(resp.Breadcrumbs)) for _, b := range resp.Breadcrumbs { @@ -459,3 +460,17 @@ func TestURLParseOccurrenceCommentURL(t *testing.T) { require.NotEmpty(t, resp.Breadcrumbs) assert.Equal(t, "basecamp schedule show "+url, resp.Breadcrumbs[0].Cmd) } + +func TestURLParseAccountScopedOccurrenceURL(t *testing.T) { + url := "https://3.basecampapi.com/99999/schedule_entries/789/occurrences/20261022.json" + resp, err := parseURLWithOutput(t, url) + require.NoError(t, err) + + require.NotNil(t, getParsedURL(t, resp).OccurrenceDate) + cmds := make([]string, 0, len(resp.Breadcrumbs)) + for _, b := range resp.Breadcrumbs { + cmds = append(cmds, b.Cmd) + } + assert.Equal(t, []string{"basecamp show " + url}, cmds, "schedule show needs a project this URL does not name") + assert.Contains(t, resp.Notice, "basecamp show "+url) +} From 75a80bcc4a4904f0d6294d545352d38d7a5ad8db Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 04:22:05 -0400 Subject: [PATCH 10/11] Drop the comment fragment from url parse's occurrence suggestion For a URL with no project the suggestion falls back to show, which reads a comment fragment before the occurrence, so show #__recording_N printed the comment's ID. The suggested URL now drops the fragment in both forms; comment_id still reports it. --- internal/commands/url.go | 9 +++++---- internal/commands/url_test.go | 27 ++++++++++++++++----------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/internal/commands/url.go b/internal/commands/url.go index da60d5e3c..1c54d6c22 100644 --- a/internal/commands/url.go +++ b/internal/commands/url.go @@ -183,11 +183,12 @@ func runURLParse(app *appctx.App, url string) error { // Build breadcrumbs var breadcrumbs []output.Breadcrumb if occurrenceDate != "" { - // schedule show reads the occurrence even from a comment link, where show - // would read the comment, but it needs the project the URL names. - showOccurrence := "basecamp schedule show " + url + // Without the comment fragment, show reads the occurrence, not the + // comment. schedule show needs the project the URL names; show does not. + occurrenceURL, _, _ := strings.Cut(url, "#") + showOccurrence := "basecamp schedule show " + occurrenceURL if bucketID == "" { - showOccurrence = "basecamp show " + url + showOccurrence = "basecamp show " + occurrenceURL } return app.OK(result, output.WithSummary(summary), diff --git a/internal/commands/url_test.go b/internal/commands/url_test.go index 048658b9f..1e13d8cbe 100644 --- a/internal/commands/url_test.go +++ b/internal/commands/url_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -458,19 +459,23 @@ func TestURLParseOccurrenceCommentURL(t *testing.T) { require.NotNil(t, parsed.CommentID) assert.Equal(t, "555", *parsed.CommentID) require.NotEmpty(t, resp.Breadcrumbs) - assert.Equal(t, "basecamp schedule show "+url, resp.Breadcrumbs[0].Cmd) + assert.Equal(t, "basecamp schedule show "+strings.TrimSuffix(url, "#__recording_555"), resp.Breadcrumbs[0].Cmd) } func TestURLParseAccountScopedOccurrenceURL(t *testing.T) { - url := "https://3.basecampapi.com/99999/schedule_entries/789/occurrences/20261022.json" - resp, err := parseURLWithOutput(t, url) - require.NoError(t, err) - - require.NotNil(t, getParsedURL(t, resp).OccurrenceDate) - cmds := make([]string, 0, len(resp.Breadcrumbs)) - for _, b := range resp.Breadcrumbs { - cmds = append(cmds, b.Cmd) + const occurrence = "https://3.basecampapi.com/99999/schedule_entries/789/occurrences/20261022.json" + for _, url := range []string{occurrence, occurrence + "#__recording_555"} { + t.Run(url, func(t *testing.T) { + resp, err := parseURLWithOutput(t, url) + require.NoError(t, err) + + require.NotNil(t, getParsedURL(t, resp).OccurrenceDate) + cmds := make([]string, 0, len(resp.Breadcrumbs)) + for _, b := range resp.Breadcrumbs { + cmds = append(cmds, b.Cmd) + } + assert.Equal(t, []string{"basecamp show " + occurrence}, cmds, "schedule show needs a project this URL does not name") + assert.Contains(t, resp.Notice, "basecamp show "+occurrence+" prints") + }) } - assert.Equal(t, []string{"basecamp show " + url}, cmds, "schedule show needs a project this URL does not name") - assert.Contains(t, resp.Notice, "basecamp show "+url) } From 95c1d1a5765d6668396198bce7bf3fc86953305b Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Fri, 23 Oct 2026 04:33:31 -0400 Subject: [PATCH 11/11] Print the exact show command for an occurrence, from any form of its URL The refusal hint named schedule show , which needs a project, so for an occurrence URL without /buckets/ it failed before printing the ID. Each new URL form had needed its own branch. show reads the occurrence from every form once the comment fragment is dropped, and needs no project, so OccurrenceError now carries the URL and one helper prints show with the fragment removed. The refusal hint, url parse and spotlight's occurrence message all use it, and each prints the command with the caller's own URL. --- internal/commands/helpers.go | 14 ++++++++++++-- internal/commands/occurrence_url_test.go | 20 ++++++++++++++++++-- internal/commands/spotlight.go | 2 +- internal/commands/url.go | 11 ++--------- internal/commands/url_test.go | 12 ++++++------ internal/urlarg/urlarg.go | 12 +++++++----- 6 files changed, 46 insertions(+), 25 deletions(-) diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 792123d4d..8cbff8676 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -594,12 +594,22 @@ func occurrenceUsage(err error) error { if errors.As(err, &occ) { return output.ErrUsageHint("That URL names one occurrence of a recurring event", fmt.Sprintf("To comment on, list the comments of, or subscribe to just this occurrence, pass its own ID, "+ - "which basecamp schedule show prints. To update, trash or boost the event, pass its ID, %s, "+ - "which acts on the whole series. Basecamp's API cannot change a single occurrence", occ.EntryID)) + "which %s prints. To update, trash or boost the event, pass its ID, %s, "+ + "which acts on the whole series. Basecamp's API cannot change a single occurrence", + showOccurrence(occ.URL), occ.EntryID)) } return err } +// showOccurrence is the command that prints an occurrence's own recording ID. +// show reads a comment fragment before the occurrence, so the fragment goes; +// unlike schedule show, show needs no project, so it works for a URL without +// one. +func showOccurrence(url string) string { + url, _, _ = strings.Cut(url, "#") + return "basecamp show " + url +} + // requireOneParseableTarget rejects a recording argument whose every // comma-separated token fails to parse. Callers tolerate individual bad IDs so // a mixed batch still posts what it can, but an all-invalid argument creates diff --git a/internal/commands/occurrence_url_test.go b/internal/commands/occurrence_url_test.go index 33998e216..cb3d27471 100644 --- a/internal/commands/occurrence_url_test.go +++ b/internal/commands/occurrence_url_test.go @@ -145,6 +145,7 @@ func TestOccurrenceURLIsRefused(t *testing.T) { assert.Equal(t, output.CodeUsage, outErr.Code) assert.Contains(t, outErr.Message, "occurrence") assert.Contains(t, outErr.Hint, "789", "the hint names the event's ID") + assert.Contains(t, outErr.Hint, "basecamp show "+target+" prints", "the hint names the command that prints the occurrence's ID") assert.Empty(t, requests) }) } @@ -305,8 +306,7 @@ func TestOccurrenceBreadcrumbsDropCommentFragment(t *testing.T) { } } -// A comment link reads the occurrence too, which is why url parse and the -// refusal hint send callers here for the occurrence's own ID. +// A comment link reads the occurrence too. func TestScheduleShowOccurrenceURLReadsItsComments(t *testing.T) { for _, target := range []string{occurrenceURL, occurrenceEditURL, occurrenceURL + "#__recording_555"} { t.Run(target, func(t *testing.T) { @@ -349,3 +349,19 @@ func TestShowOccurrenceURLReadsItsComments(t *testing.T) { } } } + +// The hint's command has to work from any form of the URL: one with no +// project, where schedule show fails, and one with a comment fragment, which +// show would read instead of the occurrence. +func TestOccurrenceHintNamesAWorkingShowCommand(t *testing.T) { + const occurrence = "https://3.basecampapi.com/99999/schedule_entries/789/occurrences/20261022.json" + for _, target := range []string{occurrence, occurrence + "#__recording_555"} { + t.Run(target, func(t *testing.T) { + _, err := extractID(target) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "want a usage error, got %v", err) + assert.Contains(t, outErr.Hint, "basecamp show "+occurrence+" prints") + }) + } +} diff --git a/internal/commands/spotlight.go b/internal/commands/spotlight.go index 1cf29d746..35a6cc011 100644 --- a/internal/commands/spotlight.go +++ b/internal/commands/spotlight.go @@ -151,7 +151,7 @@ func spotlightTarget(cmd *cobra.Command, app *appctx.App, arg string) (int64, er // the whole series. The occurrence is a recording of its own. if parsed.OccurrenceDate != "" { return 0, spotlightUsage(cmd, "the URL names one occurrence of a recurring event", - "Pass the occurrence's own ID, which basecamp schedule show prints, or the event's ID to act on the whole series") + "Pass the occurrence's own ID, which "+showOccurrence(arg)+" prints, or the event's ID to act on the whole series") } idStr, urlAccountID = parsed.RecordingID, parsed.AccountID } diff --git a/internal/commands/url.go b/internal/commands/url.go index 1c54d6c22..52cdc50da 100644 --- a/internal/commands/url.go +++ b/internal/commands/url.go @@ -183,20 +183,13 @@ func runURLParse(app *appctx.App, url string) error { // Build breadcrumbs var breadcrumbs []output.Breadcrumb if occurrenceDate != "" { - // Without the comment fragment, show reads the occurrence, not the - // comment. schedule show needs the project the URL names; show does not. - occurrenceURL, _, _ := strings.Cut(url, "#") - showOccurrence := "basecamp schedule show " + occurrenceURL - if bucketID == "" { - showOccurrence = "basecamp show " + occurrenceURL - } return app.OK(result, output.WithSummary(summary), output.WithNotice("recording_id is the whole series. To comment on or subscribe to just this occurrence, "+ - "use its own ID, which "+showOccurrence+" prints"), + "use its own ID, which "+showOccurrence(url)+" prints"), output.WithBreadcrumbs(output.Breadcrumb{ Action: "show", - Cmd: showOccurrence, + Cmd: showOccurrence(url), Description: "View the occurrence and its own ID", }), ) diff --git a/internal/commands/url_test.go b/internal/commands/url_test.go index 1e13d8cbe..deb74fd8e 100644 --- a/internal/commands/url_test.go +++ b/internal/commands/url_test.go @@ -427,13 +427,13 @@ func TestURLParseOccurrenceURL(t *testing.T) { assert.Equal(t, "20261022", *parsed.OccurrenceDate) assert.Equal(t, "789", *parsed.RecordingID) assert.Contains(t, resp.Notice, "whole series") - assert.Contains(t, resp.Notice, "basecamp schedule show "+url) + assert.Contains(t, resp.Notice, "basecamp show "+url+" prints") cmds := make([]string, 0, len(resp.Breadcrumbs)) for _, b := range resp.Breadcrumbs { cmds = append(cmds, b.Cmd) } - assert.Contains(t, cmds, "basecamp schedule show "+url) + assert.Equal(t, []string{"basecamp show " + url}, cmds) for _, c := range cmds { assert.NotContains(t, c, "comments create 789", "a comment on the series ID would miss the occurrence") } @@ -448,8 +448,8 @@ func TestURLParseOmitsOccurrenceDateForOtherURLs(t *testing.T) { assert.Empty(t, resp.Notice) } -// show resolves a comment fragment first, so the occurrence breadcrumb must -// lead somewhere that reads the occurrence even from a comment link. +// show resolves a comment fragment first, so the occurrence breadcrumb drops +// it to read the occurrence. func TestURLParseOccurrenceCommentURL(t *testing.T) { url := "https://3.basecamp.com/2914079/buckets/41746046/schedule_entries/789/occurrences/20261022#__recording_555" resp, err := parseURLWithOutput(t, url) @@ -459,7 +459,7 @@ func TestURLParseOccurrenceCommentURL(t *testing.T) { require.NotNil(t, parsed.CommentID) assert.Equal(t, "555", *parsed.CommentID) require.NotEmpty(t, resp.Breadcrumbs) - assert.Equal(t, "basecamp schedule show "+strings.TrimSuffix(url, "#__recording_555"), resp.Breadcrumbs[0].Cmd) + assert.Equal(t, "basecamp show "+strings.TrimSuffix(url, "#__recording_555"), resp.Breadcrumbs[0].Cmd) } func TestURLParseAccountScopedOccurrenceURL(t *testing.T) { @@ -474,7 +474,7 @@ func TestURLParseAccountScopedOccurrenceURL(t *testing.T) { for _, b := range resp.Breadcrumbs { cmds = append(cmds, b.Cmd) } - assert.Equal(t, []string{"basecamp show " + occurrence}, cmds, "schedule show needs a project this URL does not name") + assert.Equal(t, []string{"basecamp show " + occurrence}, cmds) assert.Contains(t, resp.Notice, "basecamp show "+occurrence+" prints") }) } diff --git a/internal/urlarg/urlarg.go b/internal/urlarg/urlarg.go index 77332047e..fb594d9c5 100644 --- a/internal/urlarg/urlarg.go +++ b/internal/urlarg/urlarg.go @@ -255,6 +255,7 @@ func Parse(input string) *Parsed { type OccurrenceError struct { EntryID string Date string + URL string // the occurrence's URL, without any comment fragment } func (e *OccurrenceError) Error() string { @@ -263,9 +264,10 @@ func (e *OccurrenceError) Error() string { // recording returns the URL's recording ID, or an *OccurrenceError when the // URL names an occurrence of that recording. -func (p *Parsed) recording() (string, error) { +func (p *Parsed) recording(input string) (string, error) { if p.OccurrenceDate != "" { - return "", &OccurrenceError{EntryID: p.RecordingID, Date: p.OccurrenceDate} + url, _, _ := strings.Cut(input, "#") + return "", &OccurrenceError{EntryID: p.RecordingID, Date: p.OccurrenceDate, URL: url} } return p.RecordingID, nil } @@ -276,7 +278,7 @@ func (p *Parsed) recording() (string, error) { func ExtractID(arg string) (string, error) { if parsed := Parse(arg); parsed != nil { if parsed.RecordingID != "" { - return parsed.recording() + return parsed.recording(arg) } if parsed.ProjectID != "" { return parsed.ProjectID, nil @@ -302,7 +304,7 @@ func ExtractProjectID(arg string) string { // Returns (recordingID, projectID). If projectID is empty, it wasn't in the URL. func ExtractWithProject(arg string) (recordingID, projectID string, err error) { if parsed := Parse(arg); parsed != nil { - recordingID, err = parsed.recording() + recordingID, err = parsed.recording(arg) return recordingID, parsed.ProjectID, err } return arg, "", nil @@ -318,7 +320,7 @@ func ExtractCommentWithProject(arg string) (id, projectID string, err error) { if parsed.CommentID != "" { return parsed.CommentID, parsed.ProjectID, nil } - id, err = parsed.recording() + id, err = parsed.recording(arg) return id, parsed.ProjectID, err } return arg, "", nil