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..68e0a36f0 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,7 +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 := 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) == "" { @@ -111,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 = target.url + showCmd = fmt.Sprintf("basecamp show %s", target.url) + } // Styled TTY output if app.Output.EffectiveFormat() == output.FormatStyled { @@ -156,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"}, } @@ -180,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{ @@ -229,7 +241,11 @@ Options: } // Resolve target — reuse the same URL/comment parsing as list - idStr, resolvedType := 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) == "" { @@ -240,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 = target.url + } // 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", @@ -419,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 @@ -433,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) } @@ -455,25 +479,25 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string // "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 "", 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( + if namesNoType(recordType) { + 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 @@ -482,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 @@ -500,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 @@ -533,28 +556,51 @@ 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) { - id = extractID(arg) - resolvedType = recordType - - 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 +// attachmentTarget is the item whose attachments list and download read. +// 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 +// 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 + } + // 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) { + url, _, _ := strings.Cut(arg, "#") + return attachmentTarget{id: parsed.RecordingID, recordType: recordType, occurrence: parsed.OccurrenceDate, url: url}, nil } - return id, resolvedType + 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 2816a1cfe..ad0683caa 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -72,9 +72,10 @@ func TestResolveAttachmentTarget(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - id, typ := resolveAttachmentTarget(tt.arg, tt.typeHint) - assert.Equal(t, tt.wantID, id) - assert.Equal(t, tt.wantType, typ) + target, err := resolveAttachmentTarget(tt.arg, tt.typeHint) + require.NoError(t, err) + assert.Equal(t, tt.wantID, target.id) + assert.Equal(t, tt.wantType, target.recordType) }) } } @@ -474,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/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..8cbff8676 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -558,27 +558,56 @@ 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 both IDs a caller can act on: the occurrence's own, +// 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, list the comments of, or subscribe to just this occurrence, pass its own ID, "+ + "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 @@ -587,7 +616,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 +844,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/occurrence_url_test.go b/internal/commands/occurrence_url_test.go new file mode 100644 index 000000000..cb3d27471 --- /dev/null +++ b/internal/commands/occurrence_url_test.go @@ -0,0 +1,367 @@ +package commands + +import ( + "bytes" + "encoding/json" + "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" +) + +// 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" +) + +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 (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: 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 refusedOccurrenceCases = []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 as a todo", NewAttachmentsCmd, "list %s --type todo"}, + {"attachments list as a chat line", NewAttachmentsCmd, "list %s --type line"}, +} + +func TestOccurrenceURLIsRefused(t *testing.T) { + for _, c := range refusedOccurrenceCases { + for _, target := range []string{occurrenceURL, occurrenceEditURL} { + t.Run(c.name+" "+target, func(t *testing.T) { + requests, err := c.run(t, 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.Contains(t, outErr.Hint, "basecamp show "+target+" prints", "the hint names the command that prints the occurrence's ID") + assert.Empty(t, requests) + }) + } + } +} + +func TestEntryURLAndIDStillReachTheEntry(t *testing.T) { + 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 := c.run(t, target) + + require.NoError(t, err, "requests: %v", requests) + want := c.want + if target == "789" && c.wantID != nil { + want = c.wantID + } + assert.Equal(t, want, requests) + }) + } + } +} + +func TestCommentFragmentOnOccurrenceURLNamesTheComment(t *testing.T) { + 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...) + + require.NoError(t, err, "requests: %v", requests) + assert.Equal(t, c.want, requests) + }) + } +} + +type occurrenceEnvelope struct { + Data json.RawMessage `json:"data"` + Summary string `json:"summary"` + Breadcrumbs []output.Breadcrumb `json:"breadcrumbs"` +} + +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 (env occurrenceEnvelope) cmds() []string { + cmds := make([]string, 0, len(env.Breadcrumbs)) + for _, b := range env.Breadcrumbs { + cmds = append(cmds, b.Cmd) + } + 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) { + 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) { + 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) + }) + } +} + +// 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()) + }) + } +} + +// 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) { + 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} { + 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") + } + }) + } + } +} + +// 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/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..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", @@ -639,7 +642,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/show.go b/internal/commands/show.go index b86cd2341..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 } @@ -257,10 +259,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 +292,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...) 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/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/commands/url.go b/internal/commands/url.go index 770dd2cb0..52cdc50da 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,18 @@ 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 subscribe to just this occurrence, "+ + "use its own ID, which "+showOccurrence(url)+" prints"), + output.WithBreadcrumbs(output.Breadcrumb{ + Action: "show", + Cmd: showOccurrence(url), + Description: "View the occurrence and its own ID", + }), + ) + } 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..deb74fd8e 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" @@ -413,3 +414,68 @@ 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") + 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.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") + } +} + +func TestURLParseOmitsOccurrenceDateForOtherURLs(t *testing.T) { + resp, err := parseURLWithOutput(t, "https://3.basecamp.com/2914079/buckets/41746046/schedule_entries/789") + require.NoError(t, err) + 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 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) + 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 show "+strings.TrimSuffix(url, "#__recording_555"), resp.Breadcrumbs[0].Cmd) +} + +func TestURLParseAccountScopedOccurrenceURL(t *testing.T) { + 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) + assert.Contains(t, resp.Notice, "basecamp show "+occurrence+" prints") + }) + } +} diff --git a/internal/urlarg/urlarg.go b/internal/urlarg/urlarg.go index 4380eb8d2..fb594d9c5 100644 --- a/internal/urlarg/urlarg.go +++ b/internal/urlarg/urlarg.go @@ -248,22 +248,46 @@ 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 + URL string // the occurrence's URL, without any comment fragment +} + +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(input string) (string, error) { + if p.OccurrenceDate != "" { + url, _, _ := strings.Cut(input, "#") + return "", &OccurrenceError{EntryID: p.RecordingID, Date: p.OccurrenceDate, URL: url} + } + 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(arg) } 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 +302,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(arg) + 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(arg) + 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) }