diff --git a/.surface b/.surface index 678fd3d11..8ce5ca25b 100644 --- a/.surface +++ b/.surface @@ -19,12 +19,12 @@ ARG basecamp bonfire split 00 ARG basecamp boost create 00 ARG basecamp boost create 01 ARG basecamp boost delete 00 -ARG basecamp boost list 00 +ARG basecamp boost list 00 [id|url] ARG basecamp boost show 00 ARG basecamp boosts create 00 ARG basecamp boosts create 01 ARG basecamp boosts delete 00 -ARG basecamp boosts list 00 +ARG basecamp boosts list 00 [id|url] ARG basecamp boosts show 00 ARG basecamp campfire delete 00 ARG basecamp campfire line 00 @@ -76,7 +76,7 @@ ARG basecamp checkin answer create 01 ARG basecamp checkin answer show 00 ARG basecamp checkin answer update 00 ARG basecamp checkin answer update 01 -ARG basecamp checkin answers 00 +ARG basecamp checkin answers 00 [question_id|url] ARG basecamp checkin question 00 ARG basecamp checkin question create 00 ARG basecamp checkin question show 00 <id|url> @@ -88,7 +88,7 @@ ARG basecamp checkins answer create 01 <content> ARG basecamp checkins answer show 00 <id|url> ARG basecamp checkins answer update 00 <id|url> ARG basecamp checkins answer update 01 <content> -ARG basecamp checkins answers 00 <question_id|url> +ARG basecamp checkins answers 00 [question_id|url] ARG basecamp checkins question 00 <id|url> ARG basecamp checkins question create 00 <title> ARG basecamp checkins question show 00 <id|url> @@ -97,7 +97,7 @@ ARG basecamp checkins question update 01 [title] ARG basecamp comments archive 00 <id|url> ARG basecamp comments create 00 <id|url> ARG basecamp comments create 01 <content> -ARG basecamp comments list 00 <id|url> +ARG basecamp comments list 00 [id|url] ARG basecamp comments restore 00 <id|url> ARG basecamp comments show 00 <id|url> ARG basecamp comments thread 00 <comment-id|comment-url> @@ -2061,6 +2061,7 @@ FLAG basecamp boost delete --todolist type=string FLAG basecamp boost delete --verbose type=count FLAG basecamp boost list --account type=string FLAG basecamp boost list --agent type=bool +FLAG basecamp boost list --all-projects type=bool FLAG basecamp boost list --cache-dir type=string FLAG basecamp boost list --count type=bool FLAG basecamp boost list --event type=string @@ -2168,6 +2169,7 @@ FLAG basecamp boosts delete --todolist type=string FLAG basecamp boosts delete --verbose type=count FLAG basecamp boosts list --account type=string FLAG basecamp boosts list --agent type=bool +FLAG basecamp boosts list --all-projects type=bool FLAG basecamp boosts list --cache-dir type=string FLAG basecamp boosts list --count type=bool FLAG basecamp boosts list --event type=string @@ -2764,6 +2766,7 @@ FLAG basecamp cards done --verbose type=count FLAG basecamp cards list --account type=string FLAG basecamp cards list --agent type=bool FLAG basecamp cards list --all type=bool +FLAG basecamp cards list --all-projects type=bool FLAG basecamp cards list --cache-dir type=string FLAG basecamp cards list --card-table type=string FLAG basecamp cards list --column type=string @@ -2777,8 +2780,11 @@ FLAG basecamp cards list --json type=bool FLAG basecamp cards list --limit type=int FLAG basecamp cards list --markdown type=bool FLAG basecamp cards list --md type=bool +FLAG basecamp cards list --no-due-date type=bool FLAG basecamp cards list --no-hints type=bool FLAG basecamp cards list --no-stats type=bool +FLAG basecamp cards list --not-now type=bool +FLAG basecamp cards list --overdue type=bool FLAG basecamp cards list --page type=int FLAG basecamp cards list --profile type=string FLAG basecamp cards list --project type=string @@ -2786,8 +2792,10 @@ FLAG basecamp cards list --quiet type=bool FLAG basecamp cards list --reverse type=bool FLAG basecamp cards list --sort type=string FLAG basecamp cards list --stats type=bool +FLAG basecamp cards list --status type=string FLAG basecamp cards list --styled type=bool FLAG basecamp cards list --todolist type=string +FLAG basecamp cards list --unassigned type=bool FLAG basecamp cards list --verbose type=count FLAG basecamp cards move --account type=string FLAG basecamp cards move --agent type=bool @@ -3614,6 +3622,7 @@ FLAG basecamp checkin answer update --verbose type=count FLAG basecamp checkin answers --account type=string FLAG basecamp checkin answers --agent type=bool FLAG basecamp checkin answers --all type=bool +FLAG basecamp checkin answers --all-projects type=bool FLAG basecamp checkin answers --by type=string FLAG basecamp checkin answers --cache-dir type=string FLAG basecamp checkin answers --count type=bool @@ -3884,6 +3893,7 @@ FLAG basecamp checkins answer update --verbose type=count FLAG basecamp checkins answers --account type=string FLAG basecamp checkins answers --agent type=bool FLAG basecamp checkins answers --all type=bool +FLAG basecamp checkins answers --all-projects type=bool FLAG basecamp checkins answers --by type=string FLAG basecamp checkins answers --cache-dir type=string FLAG basecamp checkins answers --count type=bool @@ -4143,6 +4153,7 @@ FLAG basecamp comments create --verbose type=count FLAG basecamp comments list --account type=string FLAG basecamp comments list --agent type=bool FLAG basecamp comments list --all type=bool +FLAG basecamp comments list --all-projects type=bool FLAG basecamp comments list --cache-dir type=string FLAG basecamp comments list --count type=bool FLAG basecamp comments list --help type=bool @@ -5051,6 +5062,7 @@ FLAG basecamp docs folders list --vault type=string FLAG basecamp docs folders list --verbose type=count FLAG basecamp docs list --account type=string FLAG basecamp docs list --agent type=bool +FLAG basecamp docs list --all-projects type=bool FLAG basecamp docs list --cache-dir type=string FLAG basecamp docs list --count type=bool FLAG basecamp docs list --folder type=string @@ -5060,10 +5072,12 @@ FLAG basecamp docs list --ids-only type=bool FLAG basecamp docs list --in type=string FLAG basecamp docs list --jq type=string FLAG basecamp docs list --json type=bool +FLAG basecamp docs list --kind type=string FLAG basecamp docs list --markdown type=bool FLAG basecamp docs list --md type=bool FLAG basecamp docs list --no-hints type=bool FLAG basecamp docs list --no-stats type=bool +FLAG basecamp docs list --person type=stringArray FLAG basecamp docs list --profile type=string FLAG basecamp docs list --project type=string FLAG basecamp docs list --quiet type=bool @@ -5959,6 +5973,7 @@ FLAG basecamp documents folders list --vault type=string FLAG basecamp documents folders list --verbose type=count FLAG basecamp documents list --account type=string FLAG basecamp documents list --agent type=bool +FLAG basecamp documents list --all-projects type=bool FLAG basecamp documents list --cache-dir type=string FLAG basecamp documents list --count type=bool FLAG basecamp documents list --folder type=string @@ -5968,10 +5983,12 @@ FLAG basecamp documents list --ids-only type=bool FLAG basecamp documents list --in type=string FLAG basecamp documents list --jq type=string FLAG basecamp documents list --json type=bool +FLAG basecamp documents list --kind type=string FLAG basecamp documents list --markdown type=bool FLAG basecamp documents list --md type=bool FLAG basecamp documents list --no-hints type=bool FLAG basecamp documents list --no-stats type=bool +FLAG basecamp documents list --person type=stringArray FLAG basecamp documents list --profile type=string FLAG basecamp documents list --project type=string FLAG basecamp documents list --quiet type=bool @@ -6870,6 +6887,7 @@ FLAG basecamp file folders list --vault type=string FLAG basecamp file folders list --verbose type=count FLAG basecamp file list --account type=string FLAG basecamp file list --agent type=bool +FLAG basecamp file list --all-projects type=bool FLAG basecamp file list --cache-dir type=string FLAG basecamp file list --count type=bool FLAG basecamp file list --folder type=string @@ -6879,10 +6897,12 @@ FLAG basecamp file list --ids-only type=bool FLAG basecamp file list --in type=string FLAG basecamp file list --jq type=string FLAG basecamp file list --json type=bool +FLAG basecamp file list --kind type=string FLAG basecamp file list --markdown type=bool FLAG basecamp file list --md type=bool FLAG basecamp file list --no-hints type=bool FLAG basecamp file list --no-stats type=bool +FLAG basecamp file list --person type=stringArray FLAG basecamp file list --profile type=string FLAG basecamp file list --project type=string FLAG basecamp file list --quiet type=bool @@ -7757,6 +7777,7 @@ FLAG basecamp files folders list --vault type=string FLAG basecamp files folders list --verbose type=count FLAG basecamp files list --account type=string FLAG basecamp files list --agent type=bool +FLAG basecamp files list --all-projects type=bool FLAG basecamp files list --cache-dir type=string FLAG basecamp files list --count type=bool FLAG basecamp files list --folder type=string @@ -7766,10 +7787,12 @@ FLAG basecamp files list --ids-only type=bool FLAG basecamp files list --in type=string FLAG basecamp files list --jq type=string FLAG basecamp files list --json type=bool +FLAG basecamp files list --kind type=string FLAG basecamp files list --markdown type=bool FLAG basecamp files list --md type=bool FLAG basecamp files list --no-hints type=bool FLAG basecamp files list --no-stats type=bool +FLAG basecamp files list --person type=stringArray FLAG basecamp files list --profile type=string FLAG basecamp files list --project type=string FLAG basecamp files list --quiet type=bool @@ -8644,6 +8667,7 @@ FLAG basecamp folders folders list --vault type=string FLAG basecamp folders folders list --verbose type=count FLAG basecamp folders list --account type=string FLAG basecamp folders list --agent type=bool +FLAG basecamp folders list --all-projects type=bool FLAG basecamp folders list --cache-dir type=string FLAG basecamp folders list --count type=bool FLAG basecamp folders list --folder type=string @@ -8653,10 +8677,12 @@ FLAG basecamp folders list --ids-only type=bool FLAG basecamp folders list --in type=string FLAG basecamp folders list --jq type=string FLAG basecamp folders list --json type=bool +FLAG basecamp folders list --kind type=string FLAG basecamp folders list --markdown type=bool FLAG basecamp folders list --md type=bool FLAG basecamp folders list --no-hints type=bool FLAG basecamp folders list --no-stats type=bool +FLAG basecamp folders list --person type=stringArray FLAG basecamp folders list --profile type=string FLAG basecamp folders list --project type=string FLAG basecamp folders list --quiet type=bool @@ -9116,6 +9142,7 @@ FLAG basecamp forwards inbox --verbose type=count FLAG basecamp forwards list --account type=string FLAG basecamp forwards list --agent type=bool FLAG basecamp forwards list --all type=bool +FLAG basecamp forwards list --all-projects type=bool FLAG basecamp forwards list --cache-dir type=string FLAG basecamp forwards list --count type=bool FLAG basecamp forwards list --help type=bool @@ -9805,6 +9832,7 @@ FLAG basecamp messages create --visible-to-clients type=bool FLAG basecamp messages list --account type=string FLAG basecamp messages list --agent type=bool FLAG basecamp messages list --all type=bool +FLAG basecamp messages list --all-projects type=bool FLAG basecamp messages list --cache-dir type=string FLAG basecamp messages list --count type=bool FLAG basecamp messages list --help type=bool @@ -10216,6 +10244,7 @@ FLAG basecamp msgs create --visible-to-clients type=bool FLAG basecamp msgs list --account type=string FLAG basecamp msgs list --agent type=bool FLAG basecamp msgs list --all type=bool +FLAG basecamp msgs list --all-projects type=bool FLAG basecamp msgs list --cache-dir type=string FLAG basecamp msgs list --count type=bool FLAG basecamp msgs list --help type=bool @@ -13606,6 +13635,7 @@ FLAG basecamp todos create --verbose type=count FLAG basecamp todos list --account type=string FLAG basecamp todos list --agent type=bool FLAG basecamp todos list --all type=bool +FLAG basecamp todos list --all-projects type=bool FLAG basecamp todos list --assignee type=string FLAG basecamp todos list --cache-dir type=string FLAG basecamp todos list --completed type=bool @@ -13620,6 +13650,7 @@ FLAG basecamp todos list --limit type=int FLAG basecamp todos list --list type=string FLAG basecamp todos list --markdown type=bool FLAG basecamp todos list --md type=bool +FLAG basecamp todos list --no-due-date type=bool FLAG basecamp todos list --no-hints type=bool FLAG basecamp todos list --no-stats type=bool FLAG basecamp todos list --overdue type=bool @@ -13634,6 +13665,7 @@ FLAG basecamp todos list --status type=string FLAG basecamp todos list --styled type=bool FLAG basecamp todos list --todolist type=string FLAG basecamp todos list --todoset type=string +FLAG basecamp todos list --unassigned type=bool FLAG basecamp todos list --verbose type=count FLAG basecamp todos move --account type=string FLAG basecamp todos move --agent type=bool @@ -14876,6 +14908,7 @@ FLAG basecamp vault folders list --vault type=string FLAG basecamp vault folders list --verbose type=count FLAG basecamp vault list --account type=string FLAG basecamp vault list --agent type=bool +FLAG basecamp vault list --all-projects type=bool FLAG basecamp vault list --cache-dir type=string FLAG basecamp vault list --count type=bool FLAG basecamp vault list --folder type=string @@ -14885,10 +14918,12 @@ FLAG basecamp vault list --ids-only type=bool FLAG basecamp vault list --in type=string FLAG basecamp vault list --jq type=string FLAG basecamp vault list --json type=bool +FLAG basecamp vault list --kind type=string FLAG basecamp vault list --markdown type=bool FLAG basecamp vault list --md type=bool FLAG basecamp vault list --no-hints type=bool FLAG basecamp vault list --no-stats type=bool +FLAG basecamp vault list --person type=stringArray FLAG basecamp vault list --profile type=string FLAG basecamp vault list --project type=string FLAG basecamp vault list --quiet type=bool @@ -15763,6 +15798,7 @@ FLAG basecamp vaults folders list --vault type=string FLAG basecamp vaults folders list --verbose type=count FLAG basecamp vaults list --account type=string FLAG basecamp vaults list --agent type=bool +FLAG basecamp vaults list --all-projects type=bool FLAG basecamp vaults list --cache-dir type=string FLAG basecamp vaults list --count type=bool FLAG basecamp vaults list --folder type=string @@ -15772,10 +15808,12 @@ FLAG basecamp vaults list --ids-only type=bool FLAG basecamp vaults list --in type=string FLAG basecamp vaults list --jq type=string FLAG basecamp vaults list --json type=bool +FLAG basecamp vaults list --kind type=string FLAG basecamp vaults list --markdown type=bool FLAG basecamp vaults list --md type=bool FLAG basecamp vaults list --no-hints type=bool FLAG basecamp vaults list --no-stats type=bool +FLAG basecamp vaults list --person type=stringArray FLAG basecamp vaults list --profile type=string FLAG basecamp vaults list --project type=string FLAG basecamp vaults list --quiet type=bool diff --git a/.surface-breaking b/.surface-breaking index efae949ea..8539d5563 100644 --- a/.surface-breaking +++ b/.surface-breaking @@ -1,13 +1,18 @@ ARG basecamp assign 00 <id> ARG basecamp assign 00 <todo_id> +ARG basecamp boost list 00 <id|url> +ARG basecamp boosts list 00 <id|url> ARG basecamp card 00 <title> ARG basecamp card 01 [body] ARG basecamp card move 00 <id|url> ARG basecamp card mv 00 <id|url> ARG basecamp card update 00 <id|url> +ARG basecamp checkin answers 00 <question_id|url> +ARG basecamp checkins answers 00 <question_id|url> ARG basecamp comment 00 <id|url> ARG basecamp comment 01 <content> +ARG basecamp comments list 00 <id|url> ARG basecamp done 00 <id|url>... ARG basecamp message 00 <title> ARG basecamp message 01 [body] diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md new file mode 100644 index 000000000..7712404cf --- /dev/null +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -0,0 +1,313 @@ +# Account-Wide Listings + +Authoritative contract for exposing the SDK's `EverythingService` aggregates +(basecamp/basecamp-sdk#435, #438) through the CLI. This supersedes the "open +design question" framing in basecamp/basecamp-cli#585 and the "deliberately not +wired" note in PR #584. + +## Shape + +The aggregates are **not** a new command group. `Everything` is Basecamp web-UI +vocabulary; these are the account-wide variants of resources the CLI already +owns. Each aggregate lands on the group that owns its noun, reached by the +group's existing leaf list command. + +## Method matrix + +All 17 methods, and the invocation that reaches each. + +| Command | SDK method | Payload | Paginated | +|---|---|---|---| +| `messages list --all-projects` | `Messages` | `.Recordings` | yes | +| `comments list --all-projects` | `Comments` | `.Recordings` | yes | +| `checkins answers --all-projects` | `Checkins` | `.Recordings` | yes | +| `forwards list --all-projects` | `Forwards` | `.Recordings` | yes | +| `boost list --all-projects` | `Boosts` | `.Boosts` | yes | +| `files list --all-projects` | `Files` | `.Files` | yes | +| `todos list --all-projects` | `OpenTodos` | `.Groups` | yes | +| `todos list --all-projects --status completed` | `CompletedTodos` | `.Groups` | yes | +| `todos list --all-projects --unassigned` | `UnassignedTodos` | `.Groups` | yes | +| `todos list --all-projects --no-due-date` | `NoDueDateTodos` | `.Groups` | yes | +| `todos list --all-projects --overdue` | `OverdueTodos` | flat `[]Todo` | **no** | +| `cards list --all-projects` | `OpenCards` | `.Groups` | yes | +| `cards list --all-projects --status completed` | `CompletedCards` | `.Groups` | yes | +| `cards list --all-projects --unassigned` | `UnassignedCards` | `.Groups` | yes | +| `cards list --all-projects --no-due-date` | `NoDueDateCards` | `.Groups` | yes | +| `cards list --all-projects --not-now` | `NotNowCards` | `.Groups` | yes | +| `cards list --all-projects --overdue` | `OverdueCards` | flat `[]Card` | **no** | + +`reports overdue` is **not** replaced or deprecated. It is a lateness-bucketed +report (under a week / over a week / over a month / over three months); +`todos list --overdue` is a flat oldest-first aggregate. Both stay, and the +distinction is documented rather than resolved. + +## Invariants + +### I1 — Scope is selected before scope-specific validation + +Determine the scope first, then validate against that scope. Validating a flag +under project rules before the account-wide branch is reached is a defect: the +paginated aggregates accept **any positive page**, while several project-scoped +commands only permit page 1. + +Conversely `--page` must return `ErrUsage` against the **unpaginated** overdue +endpoints, which take no page argument at all. + +### I2 — Scope precedence + +Three inputs, in order: + +1. **Explicit project** — `--project`, `-p`, or `--in`, accepted both at the + root (`basecamp --project X todos list`, parsed in `internal/cli/root.go` + into `app.Flags.Project`) and after the group noun. Detection must not rely + on `cmd.Flags().Changed` alone, which misses the root-level form. +2. **Configured project** — `app.Config.ProjectID`. +3. **Nothing** — account-wide. + +`--all-projects` **overrides** a configured project and **conflicts** with an +explicit one (`ErrUsage`). Without it, a user with a configured project could +never reach these listings, and identical scripts would change behavior with +ambient config. + +#### Per-item dispatch + +The per-item groups (`comments`, `boost`, `checkins answers`) list the children +of one recording, so a project alone cannot produce a listing — only an item ID +can. Their dispatch is therefore its own truth table, and it overrides the +general precedence above: + +| ID/URL | Explicit project | `--all-projects` | Result | +|---|---|---|---| +| present | any | absent | item-scoped | +| present | any | present | **ErrUsage** (conflict) | +| absent | present | present | **ErrUsage** (conflict) — the general I2 rule still applies | +| absent | present | absent | **ErrUsage** — ask for an ID | +| absent | absent (configured only) | absent | account-wide; ambient config ignored because it cannot scope this operation | +| absent | absent | present | account-wide, intent pinned | + +The fourth row is the deliberate exception to "a configured project selects +project scope": for these three commands a configured project is not a scope +that can be honored, so it is ignored rather than turned into an error. + +### I3 — No flag is silently ignored + +Every accepted flag must either affect the account-wide result or return an +actionable `ErrUsage`. This is the primary defect class. + +**Scope-child flags** name a container inside one project and are meaningless +account-wide. Reject by name, including aliases: +`--message-board`, `--inbox`, `--vault`/`--folder`, `--card-table`, `--column`, +`--list`/`--todolist`, `--todoset`, `--questionnaire`, `--event`, `--by`. +A configured todolist is subject to the same rule as a configured project. + +**Filters with no aggregate equivalent** (`--assignee`, unsupported `--status` +values) are rejected, pointing at the command that does answer the question +(e.g. `reports assigned`). + +**No new flags** beyond `--all-projects`, the endpoint selectors the method +matrix names, and one recorded exception. The account-wide path reuses the +filter, sort, and pagination flags a command already has; it does not grow a +parallel flag surface. The exception is `files list`, which today has no filters +at all and gains two account-wide-only ones — see I5. + +#### Endpoint selectors + +The method matrix reaches eleven distinct todo/card endpoints, and the flags +that pick among them do not all exist yet. This is the complete inventory of +flags added by this work — anything not listed here is reuse: + +| Command | New flag | Selects | Scope | +|---|---|---|---| +| all eight | `--all-projects` | account-wide | — | +| `todos list` | `--unassigned` | `UnassignedTodos` | account-wide only | +| `todos list` | `--no-due-date` | `NoDueDateTodos` | account-wide only | +| `cards list` | `--status` (only `completed`) | `CompletedCards` | account-wide only | +| `cards list` | `--unassigned` | `UnassignedCards` | account-wide only | +| `cards list` | `--no-due-date` | `NoDueDateCards` | account-wide only | +| `cards list` | `--not-now` | `NotNowCards` | account-wide only | +| `cards list` | `--overdue` | `OverdueCards` | account-wide only | +| `files list` | `--kind`, `--person` | filters on `Files` | account-wide only — see I5 | + +**Account-wide only** means exactly what it means for the `files list` filters: +the project-scoped path has no equivalent, so passing one with a project in +scope is `ErrUsage`, not a silent no-op. `todos list --status completed`, +`--completed`, and `--overdue` already exist project-scoped and keep working +there; they merely gain an account-wide meaning. + +The selectors are **mutually exclusive** — each picks one endpoint, and there is +no endpoint that combines two. Passing more than one is `ErrUsage` naming the +pair. + +### I4 — Sorting + +**Sorting flags are reused where they exist and never added.** Verified current +state: among these commands only `messages list`, `todos list`, and `cards list` +expose `--sort`/`--reverse`. `comments list`, `checkins answers`, +`forwards list`, `boost list`, and bare `files list` expose neither and gain +neither — an unsorted account-wide feed is the honest result, not a gap to +paper over. + +`--sort`/`--reverse` are **rejected for the grouped todo and card aggregates**. +The payload is nested by project, and raw grouped machine output cannot +represent a globally sorted cross-project list; sorting within each group is a +different contract from the existing global `--sort` and must not be passed off +as it. + +They are **allowed for the flat results** — overdue todos/cards (flat `[]Todo`, +`[]Card`) and `messages list --all-projects` (flat `[]Recording`) — where a +client-side sort helper exists or can be adapted. + +Everywhere: `--reverse` without `--sort` is an error, and **sorting precedes +truncation** — truncating first would sort only the surviving window. + +### I5 — Pagination and limits + +The aggregates take a single page number: `0` follows every page, `N` returns +exactly page `N`. + +| Flag | Mapping | +|---|---| +| `--all` | page 0 | +| `--page N` | page N (any positive N) | +| `--limit N` | client-side truncation to N, with an honest truncation notice | +| default | the command's documented default, per the table below | + +#### Per-command defaults + +"The command's documented default" is not left to interpretation. Each +command's account-wide default matches what it already does project-scoped: + +| Command | Existing default | Account-wide contract | +|---|---|---| +| `messages list` | 100 | cap 100 | +| `comments list` | 100 | cap 100 | +| `checkins answers` | all | all | +| `forwards list` | all | all | +| `boost list` | no paging flags | first page only | +| `files list` | no paging flags | all pages | +| `todos list` | 100 | cap 100 | +| `cards list` | all | all | + +The default must remain the documented default. Degrading a documented cap to +"one raw SDK page" is a silent contract change; so is promoting an unpaged +command to a capped one. + +#### Rules + +- **No new pagination flags where none exist.** `boost list` and bare + `files list` have no `--limit`/`--page`/`--all` today and gain none. Their + account-wide behavior is fixed by the table above. +- **`--limit N` on grouped todo/card responses counts inner todos/cards, not + outer project groups.** Truncating groups would silently drop whole projects. +- **`Meta.TotalCount` counts groups, not items,** for the grouped responses. + `accountWideRespOpts` therefore must **not** be used for grouped item-count + notices — the command computes its own item total and builds its own summary. +- **Explicit `--page 0`, a negative `--page`, and a negative `--limit` return + `ErrUsage`.** `--page 0` means "unset" to Cobra's default but "follow every + page" to the SDK; accepting it silently would hand the user a full-account + crawl they did not ask for, and that is the same no-silent-flags defect I3 + names. `--all` is the spelling for every page. + +#### The `files list` filter exception + +`EverythingFilesOptions` carries filters the project-scoped path has no +equivalent for: `Vaults`, `Uploads`, and `Documents` `List()` take neither a +kind nor a creator. Rather than leave adopted SDK surface unreachable, bare +`files list` gains two flags: + +| Flag | Value | Maps to | +|---|---|---| +| `--kind` | `all`, `images`, `pdfs`, `documents`, `videos` | `EverythingFilesOptions.Kind` | +| `--person` | repeatable; name, email, ID, or `me`, resolved via `resolveAssigneeID` | `EverythingFilesOptions.PeopleIDs` | + +Both are **account-wide-only**. Passing either with a project in scope — +explicit, configured, or via `--vault`/`--folder` — is `ErrUsage`, because the +project-scoped path would have to ignore them, and I3 forbids that. An +unrecognized `--kind` value is `ErrUsage` listing the accepted set. + +This is the one deliberate exception to I3's "no new flags"; it is recorded here +so it stays an exception rather than a precedent. + +#### The `files` group's alias spellings + +`vaults` (aliases `vault`, `folders`) and `docs` (alias `documents`) are +`NewFilesCmd` under different names, so they share this leaf. The account-wide feed +is not the same listing the project-scoped path returns: it carries Uploads, +Documents, and Attachments, and **no folder variant at all**. + +Sharing the leaf unchanged would make `folders list --all-projects` return a +listing containing none of the thing the command is named for. Each spelling +therefore gets its own account-wide meaning: + +| Spelling | Account-wide behavior | +|---|---| +| `files list` | the whole feed, `--kind` free | +| `vaults` / `folders list` | **ErrUsage** — folders have no account-wide listing; points at the project-scoped form and at `files list --all-projects` | +| `docs` / `documents list` | the feed pinned to `--kind documents`; an explicit `--kind` is ErrUsage, since the command name already chose | + +The project-scoped behavior of all three is unchanged. + +**Accepted tradeoff.** Honoring a 100-item cap by fetching page 0 downloads +every page before truncating, which is correct but potentially expensive on +large accounts. Where sorting is not in play, a bounded loop over positive pages +that stops once the limit is collected is the cheaper equivalent and is +preferred. Recorded here so the cost is a decision rather than an accident. + +### I6 — Output contract + +Project-scoped paths return concrete types (`[]Message`, `[]Todo`). Aggregate +paths return `[]Recording`, `[]EverythingFile`, or **nested** bucket groups. +Handing nested groups to the styled renderer produces unreadable nested cells. + +Branch on `app.Output.EffectiveFormat()`, following the `humanizeSearchResults` +precedent in `internal/commands/search.go`: + +- **machine formats** (`--json`, `--agent`, `--md`): raw SDK payload, grouping + preserved. +- **styled**: flattened rows carrying at least project name, id, and + title/subject, plus status and due date where applicable. + +Which payloads actually need flattening, so the eight commands do not each +answer this differently: + +| Payload | Commands | Styled treatment | +|---|---|---| +| `[]Recording` | messages, comments, checkins answers, forwards | flatten — the generic renderer drops the nested `bucket`, and a project column is exactly what an account-wide row needs | +| `[]Todo`, `[]Card` (flat overdue) | todos, cards `--overdue` | flatten — same reason as `[]Recording`; the items come from every project and `bucket` is skipped by name | +| `[]EverythingBoost` | boost | flatten — the boosted `*Recording` is nested | +| `[]EverythingFile` | files | flatten — all-pointer superset, too wide to render raw | +| `[]BucketTodosGroup`, `[]BucketCardsGroup` | todos, cards | flatten — nested groups render as unreadable cells | + +**Every** account-wide payload flattens. The rule is not about nesting depth — +it is that `internal/output/render.go` skips `bucket` by name in both generic +renderers, so any aggregate handed over raw loses the one column that makes a +cross-project row attributable. "Renders fine project-scoped" is never the test: +project-scoped output needs no project column. `recordings list` hands +`[]Recording` over raw for exactly that reason, and it is not a precedent here. + +The same goes for `WithEntity`. A schema that renders a task list has no column +for a project, so the account-wide overdue todo listing drops the entity rather +than lose the attribution. + +**Mechanism.** Flattening is supplied through `output.WithDisplayData`, not by +branching on `EffectiveFormat()`. `Data` stays the raw SDK payload so `--json` +and `--agent` keep it; `DisplayData` carries the flat rows that styled, +`--md`, `--ids`, and `--count` all read. Branching on the format instead leaves +the other three reading the nested payload — where `--count` counts project +groups and `--ids` finds no ids at all, both silently. + +`EverythingFile` is an all-pointer superset over the Upload, Document, and +Attachment variants — every field read during flattening must be nil-checked. +`EverythingBoost.Booster` and `.Recording` are pointers too. + +### I7 — No interactive prompt + +`ensureProject()` is unreachable on the account-wide path. The bare command +**group** still shows help; only the leaf list invocation lists account-wide. + +## Behavior changes to release-note + +- The interactive project prompt no longer fires on these list commands when no + project is configured; they list account-wide instead. +- `todos list` with no project and `--overdue`/`--assignee` previously errored + with a redirect. `--overdue` now returns results; `--assignee` still errors. diff --git a/API-COVERAGE.md b/API-COVERAGE.md index aa4c1176a..4fcf31958 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -11,14 +11,98 @@ Coverage of Basecamp 3 API endpoints. Source: [bc3-api/sections](https://github. | **Total tracked** | **49** | **179** | **100% coverage of tracked in-scope API** (167/167 endpoints). This is not a -complete bc-api parity figure; the other five BC5 sections introduced by -bc-api#410 remain untracked and outside this coverage matrix. +complete bc-api parity figure. The other five BC5 sections introduced by +bc-api#410 remain untracked and outside this coverage matrix. The pinned SDK's +`EverythingService` is now fully reached — see [Account-wide +aggregates](#account-wide-aggregates). Out-of-scope sections are excluded from parity totals and scripts: chatbots (different auth), legacy Clientside (deprecated) > Note: the per-row `Endpoints` column in the Coverage by Section table sums higher than the Summary totals above. The discrepancy predates the BC5 baseline; the row count (48 sections) is authoritative for the `Since` column. Reconciling endpoint counts is pre-existing maintenance, tracked separately. -**SDK version:** v0.9.1-0.20260727173625-bb363c847b92 — reshapes `MessageTypes` to the bucket-scoped categories API (`/buckets/{id}/categories…`, basecamp/basecamp-sdk#393), consumed by the now project-scoped `messagetypes` commands; adds the Bubble Ups successor surface (`MyNotifications.BubbleUps` via `GET /my/readings/bubble_ups.json` + `GetWithOptions`/`WithLimitBubbleUps`, basecamp/basecamp-sdk#426) behind `notifications bubbleups` and `notifications list --limit-bubble-ups`; adds create-time `visible_to_clients` on `Tools.Create` (basecamp/basecamp-sdk#419) behind `tools create --visible-to-clients`; and carries model-only Door fields on Recording (basecamp/basecamp-sdk#431) and activity-timeline event fields (basecamp/basecamp-sdk#424) that flow through `--json`. API date 2026-07-26. +**SDK version:** v0.10.0 — adds `EverythingService` (`AccountClient.Everything()`, +basecamp/basecamp-sdk#435 and #438), a 17-method account-wide aggregate family +covering cross-project messages, comments, checkins, forwards, boosts, files, and +the open/completed/unassigned/overdue/no-due-date todo and card rollups. All 17 +are reached from the CLI — see [Account-wide +aggregates](#account-wide-aggregates). They are not a new command group and add +no endpoints to the tracked totals above: each aggregate is the account-wide +variant of a listing the CLI already owned, reached through that group's +existing leaf command. The contract is `ACCOUNT-WIDE-LISTINGS.md`. + +Model and transport changes riding along: + +- `UpdateCardRequest.Title`/`.Content`/`.DueOn` became `*string`, nil meaning + "leave unchanged", for merge-safe partial updates (#489). +- `SearchResult.Content`/`.Description` became `*string`, and the excerpt moved + to `PlainTextContent`/`PlainTextDescription` (#487). Those two are **HTML + fragments despite the name** — BC3 wraps each query match in + `<mark class="circled-text">` — so any consumer must strip markup before + display. +- `BubbleUpURL` spread to `Recording`, `SearchResult`, `Todolist`, and + `TodolistGroup` (#488); it previously existed only on `BubbleUp`. On `Todolist` + and `TodolistGroup` the tag carries no `omitempty`, so the key is always + present in machine output. +- HTTP 400 now maps to the `validation` error code rather than `api_error` (#482). + Since `convertSDKError` passes the SDK code straight through, **a 400's exit + code moves from 7 to 9**; 422 was already validation. +- Retry behavior: per-operation `retry.max` is honored as a ceiling (#483), + `*WithBody` request bodies replay across retries (#481), and the declared + `retry_on` status set is honored (#486). +- Provenance repinned to current bc3 HEAD, pinning the `participant_ids` + contract (#491). + +**Machine-output contract change.** `search` serializes raw SDK structs for +`--json`/`--agent`/`--md` (only the styled path is humanized), so these model +changes reach users directly: `content` and `description` now serialize as +explicit `null` (the pointer fields carry no `omitempty`, where the old empty +strings were omitted), and `plain_text_content`/`plain_text_description` plus +`bubble_up_url` appear when populated. Styled output is unaffected. + +API date 2026-07-28. + +## Account-wide aggregates + +`EverythingService` answers, across every accessible project, the same questions +the project-scoped listings answer within one. All 17 methods are reachable. + +These rows are **not** added to the totals above. They are not new endpoints in +the tracked matrix — they are the account-wide variant of listings already +counted, reached through the owning group's existing leaf command rather than a +new `everything` group. `--all-projects` pins the intent and overrides a +configured project; with nothing in scope the same command lists account-wide +instead of prompting for a project. + +| Invocation | SDK method | Payload | +|------------|-----------|---------| +| `messages list --all-projects` | `Messages` | `[]Recording` | +| `comments list --all-projects` | `Comments` | `[]Recording` | +| `checkins answers --all-projects` | `Checkins` | `[]Recording` | +| `forwards list --all-projects` | `Forwards` | `[]Recording` | +| `boost list --all-projects` | `Boosts` | `[]EverythingBoost` | +| `files list --all-projects` | `Files` | `[]EverythingFile` | +| `todos list --all-projects` | `OpenTodos` | bucket groups | +| `todos list --all-projects --status completed` | `CompletedTodos` | bucket groups | +| `todos list --all-projects --unassigned` | `UnassignedTodos` | bucket groups | +| `todos list --all-projects --no-due-date` | `NoDueDateTodos` | bucket groups | +| `todos list --all-projects --overdue` | `OverdueTodos` | flat `[]Todo` | +| `cards list --all-projects` | `OpenCards` | bucket groups | +| `cards list --all-projects --status completed` | `CompletedCards` | bucket groups | +| `cards list --all-projects --unassigned` | `UnassignedCards` | bucket groups | +| `cards list --all-projects --no-due-date` | `NoDueDateCards` | bucket groups | +| `cards list --all-projects --not-now` | `NotNowCards` | bucket groups | +| `cards list --all-projects --overdue` | `OverdueCards` | flat `[]Card` | + +`files list` additionally exposes the feed's own filters, `--kind` +(all/images/pdfs/documents/videos) and repeatable `--person`. Both are +account-wide-only: the project-scoped path has no equivalent filter, so passing +either with a project in scope is a usage error rather than a silent no-op. + +`reports overdue` is neither replaced nor deprecated. It is a lateness-bucketed +report; `todos list --all-projects --overdue` is a flat oldest-first aggregate. + +Design discussion: basecamp/basecamp-cli#585. Contract and invariants: +`ACCOUNT-WIDE-LISTINGS.md`. ## Coverage by Section diff --git a/e2e/checkins.bats b/e2e/checkins.bats index 0ae6a143f..28ecb0a75 100644 --- a/e2e/checkins.bats +++ b/e2e/checkins.bats @@ -55,13 +55,24 @@ load test_helper assert_json_value '.code' 'usage' } -@test "checkins answers without question id shows error" { +@test "checkins answers with a project but no question id shows error" { create_credentials - create_global_config '{"account_id": 99999, "project_id": 123}' + create_global_config '{"account_id": 99999}' + + run basecamp checkins answers --in 123 + assert_failure + assert_output_contains "A project alone cannot select check-in answers" + assert_json_value '.code' 'usage' +} - run basecamp checkins answers +@test "checkins answers rejects --all-projects alongside a question id" { + create_credentials + create_global_config '{"account_id": 99999}' + + run basecamp checkins answers 789 --all-projects assert_failure - assert_output_contains "ID required" + assert_output_contains "cannot be combined with a question ID" + assert_json_value '.code' 'usage' } @test "checkins answer without id shows error" { diff --git a/go.mod b/go.mod index bf3e2bda0..a01c6f406 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( charm.land/bubbles/v2 v2.1.1 charm.land/bubbletea/v2 v2.0.8 charm.land/lipgloss/v2 v2.0.5 - github.com/basecamp/basecamp-sdk/go v0.9.1-0.20260727173625-bb363c847b92 + github.com/basecamp/basecamp-sdk/go v0.10.0 github.com/basecamp/cli v0.2.2-0.20260728023309-04e401b12c6c github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/glamour v1.0.0 diff --git a/go.sum b/go.sum index 11a120dd6..679c4f37a 100644 --- a/go.sum +++ b/go.sum @@ -23,8 +23,8 @@ github.com/aymanbagabas/go-udiff v0.4.1 h1:OEIrQ8maEeDBXQDoGCbbTTXYJMYRCRO1fnodZ github.com/aymanbagabas/go-udiff v0.4.1/go.mod h1:0L9PGwj20lrtmEMeyw4WKJ/TMyDtvAoK9bf2u/mNo3w= github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuPk= github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= -github.com/basecamp/basecamp-sdk/go v0.9.1-0.20260727173625-bb363c847b92 h1:4gQJTR8e5kBvXHDHLvef+Q00XY7KzWTxnQSLWMSPcyA= -github.com/basecamp/basecamp-sdk/go v0.9.1-0.20260727173625-bb363c847b92/go.mod h1:r83ralDQ0q9vbAby5qQ5x9hgCgUdJLDLHYpiU6jaFjE= +github.com/basecamp/basecamp-sdk/go v0.10.0 h1:5mK+2Z2XmFCVWsl1pIxdOAF45M47gWtr0+/zgCZgFtY= +github.com/basecamp/basecamp-sdk/go v0.10.0/go.mod h1:r83ralDQ0q9vbAby5qQ5x9hgCgUdJLDLHYpiU6jaFjE= github.com/basecamp/cli v0.2.2-0.20260728023309-04e401b12c6c h1:+5sQBl8sqYoD1Qhwsibn8sBCKWPyZ9NDez6mnuo9Afo= github.com/basecamp/cli v0.2.2-0.20260728023309-04e401b12c6c/go.mod h1:EK1Dba6DEw8ZAilVBpf/jri3ONDV7LQkLACSDe73f/c= github.com/bmatcuk/doublestar v1.1.1/go.mod h1:UD6OnuiIn0yFxxA2le/rnRU1G4RaI4UvFv1sNto9p6w= diff --git a/internal/commands/accountwide.go b/internal/commands/accountwide.go new file mode 100644 index 000000000..482e14f93 --- /dev/null +++ b/internal/commands/accountwide.go @@ -0,0 +1,128 @@ +package commands + +import ( + "fmt" + "math" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// Account-wide listings. +// +// The resource groups are project-scoped: they resolve a project from +// --project/--in, then the global flag, then config, and otherwise prompt for +// one. When none of those supply a project, the SDK's account-wide aggregate +// endpoints answer the same question across every accessible project, so the +// groups list account-wide instead of prompting. +// +// Groups keep their own pagination flags rather than gaining new ones; each +// maps them onto the page number these endpoints take, where 0 means "follow +// the Link header across every page". + +// projectKnown reports whether a project is available without prompting. +func projectKnown(app *appctx.App, projectFlag string) bool { + if app == nil { + return false + } + return projectFlag != "" || app.Flags.Project != "" || app.Config.ProjectID != "" +} + +// accountWidePage maps a group's existing --page/--all pair onto the page +// number the account-wide endpoints take. Groups that only support page 1 +// pass their validated page through unchanged. +// +// A page beyond int32 is a usage error rather than a clamp. Clamping would +// serve a different page than the one asked for, and I3 forbids silently +// altering a flag just as much as silently dropping it. +func accountWidePage(page int, all bool) (int32, error) { + if all { + return 0, nil + } + if page < 1 { + return 1, nil + } + if page > math.MaxInt32 { + return 0, output.ErrUsage("--page is out of range") + } + return int32(page), nil +} + +// flattenAccountWideRecordings builds the display rows for the four aggregate +// feeds that return []Recording. The renderer's generic column detection drops +// the nested bucket, which on an account-wide listing removes the one thing +// that makes a row actionable — the project it came from. Title also falls +// back to Subject, since a Comment's own title is the generic type name while +// its subject carries the thread. +func flattenAccountWideRecordings(recordings []basecamp.Recording) []map[string]any { + rows := make([]map[string]any, 0, len(recordings)) + for _, r := range recordings { + title := r.Title + if r.Subject != nil && *r.Subject != "" { + title = *r.Subject + } + row := map[string]any{ + "id": r.ID, + "title": title, + "type": r.Type, + "created": r.CreatedAt, + } + if r.Bucket != nil { + row["project"] = r.Bucket.Name + } + if r.Creator != nil { + row["creator"] = r.Creator.Name + } + rows = append(rows, row) + } + return rows +} + +// rejectAccountWideTodolist refuses the root-level --todolist on an +// account-wide listing. It names a container inside one project, so it can no +// more narrow an account-wide feed than a project can — and being a root +// global rather than a group flag, it reaches every one of these commands +// whether or not the command has any notion of a todolist. +func rejectAccountWideTodolist(app *appctx.App, noun string) error { + if app == nil || app.Flags.Todolist == "" { + return nil + } + return output.ErrUsageHint( + fmt.Sprintf("--todolist cannot scope an account-wide %s listing", noun), + "Drop --todolist, or pass --project/--in to list within that project.") +} + +// accountWideRespOpts builds the summary and truncation notice shared by the +// account-wide listings. singular/plural are the noun in both forms, since a +// one-item account-wide listing is common enough that "1 boosts" shows up. +// +// moreFlag names the flag that widens the listing, or is empty for the +// commands I5 gives no pagination flags at all — recommending --all to a +// command that has no --all is worse than saying nothing. +// +// explicitLimit reports whether the user asked for the cap. It changes the +// recovery advice rather than the fact: these commands reject --all alongside +// --limit, so telling someone who passed --limit to add --all names a +// combination the command refuses. +func accountWideRespOpts(count int, singular, plural string, meta basecamp.ListMeta, moreFlag string, explicitLimit bool) []output.ResponseOption { + noun := plural + if count == 1 { + noun = singular + } + respOpts := []output.ResponseOption{ + output.WithSummary(fmt.Sprintf("%d %s across all projects", count, noun)), + } + if meta.TotalCount > count { + notice := fmt.Sprintf("Showing %d of %d results", count, meta.TotalCount) + switch { + case explicitLimit: + notice += " (raise or drop --limit for more)" + case moreFlag != "": + notice += fmt.Sprintf(" (use %s for the complete list)", moreFlag) + } + respOpts = append(respOpts, output.WithNotice(notice)) + } + return respOpts +} diff --git a/internal/commands/accountwide_test.go b/internal/commands/accountwide_test.go new file mode 100644 index 000000000..5294d7374 --- /dev/null +++ b/internal/commands/accountwide_test.go @@ -0,0 +1,164 @@ +package commands + +import ( + "bytes" + "encoding/json" + "errors" + "math" + "net/http" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// The root --todolist is a global, so it reaches every account-wide listing +// whether or not the command has any notion of a todolist. I3 lists it among +// the scope-child flags that must be rejected by name rather than dropped. +func TestAccountWideListingsRejectRootTodolist(t *testing.T) { + cases := []struct { + name string + cmd func() *cobra.Command + args []string + }{ + {"messages", NewMessagesCmd, []string{"list"}}, + {"comments", NewCommentsCmd, []string{"list"}}, + {"boost", NewBoostsCmd, []string{"list"}}, + {"forwards", NewForwardsCmd, []string{"list"}}, + {"checkins", NewCheckinsCmd, []string{"answers"}}, + {"files", NewFilesCmd, []string{"list"}}, + {"cards", NewCardsCmd, []string{"list"}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app, transport := setupRecordingTestApp(t) + app.Flags.Todolist = "456" + + err := executeRecordingCommand(tc.cmd(), app, tc.args...) + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T", err) + assert.Contains(t, e.Message, "--todolist") + assert.Empty(t, transport.recorded(), "must reject before any request") + }) + } +} + +// The grouped aggregates nest their items inside project groups. --count and +// --ids read the display rows so they report todos rather than projects, and +// --md renders rows at all instead of a heading with no table. +func TestAccountWideGroupedListingsFeedEveryOutputMode(t *testing.T) { + body := `[{"bucket":{"id":1,"name":"Alpha"},"todos":[{"id":11,"title":"A1"},{"id":12,"title":"A2"}]}, + {"bucket":{"id":2,"name":"Beta"},"todos":[{"id":21,"title":"B1"}]}]` + + run := func(t *testing.T, format output.Format) string { + t.Helper() + app, _ := setupRecordingTestApp(t, stubRoute{ + method: http.MethodGet, path: "/99999/todos/open.json", status: http.StatusOK, body: body, + }) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: format, Writer: buf}) + require.NoError(t, executeRecordingCommand(newTodosListCmd(), app, "--all-projects", "--page", "1")) + return buf.String() + } + + assert.Equal(t, "3\n", run(t, output.FormatCount), "counts todos, not project groups") + assert.Equal(t, "11\n12\n21\n", run(t, output.FormatIDs)) + + md := run(t, output.FormatMarkdown) + assert.Contains(t, md, "| Alpha |") + assert.Contains(t, md, "A1") + + // --json keeps the grouping the SDK returned. + app, _ := setupRecordingTestApp(t, stubRoute{ + method: http.MethodGet, path: "/99999/todos/open.json", status: http.StatusOK, body: body, + }) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf}) + require.NoError(t, executeRecordingCommand(newTodosListCmd(), app, "--all-projects", "--page", "1")) + + var envelope struct { + Data []struct { + Bucket struct { + Name string `json:"name"` + } `json:"bucket"` + } `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + require.Len(t, envelope.Data, 2) + assert.Equal(t, "Alpha", envelope.Data[0].Bucket.Name) +} + +// An account-wide row the user cannot attribute to a project is not actionable, +// so the recording feeds carry the bucket name into their display rows. +func TestAccountWideRecordingFeedsCarryProject(t *testing.T) { + body := `[{"id":7,"title":"Message","subject":"Ship it","type":"Message", + "bucket":{"id":1,"name":"Alpha"},"created_at":"2026-01-01T00:00:00Z"}]` + + app, _ := setupRecordingTestApp(t, stubRoute{ + method: http.MethodGet, path: "/99999/messages.json", status: http.StatusOK, body: body, + }) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatMarkdown, Writer: buf}) + require.NoError(t, executeRecordingCommand(NewMessagesCmd(), app, "list", "--all-projects", "--page", "1")) + + out := buf.String() + assert.Contains(t, out, "Alpha", "styled/markdown rows must name the project") + assert.Contains(t, out, "Ship it", "subject wins over the generic recording title") + assert.True(t, strings.Contains(out, "| Project |") || strings.Contains(out, "Project"), + "expected a project column, got:\n%s", out) +} + +// The flat overdue aggregates return items from every project with the project +// in a nested bucket, which both generic renderers skip by name. Without +// display rows, two otherwise identical overdue todos cannot be told apart. +func TestAccountWideOverdueListingsNameTheirProject(t *testing.T) { + t.Run("todos", func(t *testing.T) { + body := `[{"id":11,"title":"Ship it","due_on":"2020-01-01","bucket":{"id":1,"name":"Alpha"}}]` + app, _ := setupRecordingTestApp(t, stubRoute{ + method: http.MethodGet, path: "/99999/todos/overdue.json", status: http.StatusOK, body: body, + }) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatMarkdown, Writer: buf}) + require.NoError(t, executeRecordingCommand(newTodosListCmd(), app, "--all-projects", "--overdue")) + assert.Contains(t, buf.String(), "Alpha") + assert.Contains(t, buf.String(), "Ship it") + }) + + t.Run("cards", func(t *testing.T) { + body := `[{"id":21,"title":"Fix it","due_on":"2020-01-01","bucket":{"id":2,"name":"Beta"}}]` + app, _ := setupRecordingTestApp(t, stubRoute{ + method: http.MethodGet, path: "/99999/cards/overdue.json", status: http.StatusOK, body: body, + }) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatMarkdown, Writer: buf}) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--overdue")) + assert.Contains(t, buf.String(), "Beta") + assert.Contains(t, buf.String(), "Fix it") + }) +} + +// A page beyond int32 used to clamp, serving a different page than the one +// asked for. I3 forbids silently altering a flag as much as dropping it. +func TestAccountWidePageRejectsOutOfRange(t *testing.T) { + _, err := accountWidePage(math.MaxInt32+1, false) + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "--page is out of range") + + within, err := accountWidePage(math.MaxInt32, false) + require.NoError(t, err) + assert.Equal(t, int32(math.MaxInt32), within) + + all, err := accountWidePage(9999999999, true) + require.NoError(t, err, "--all wins before the range check") + assert.Equal(t, int32(0), all) +} diff --git a/internal/commands/boost.go b/internal/commands/boost.go index 92b8e892d..14f111cf1 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -7,6 +7,8 @@ import ( "github.com/spf13/cobra" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" ) @@ -48,33 +50,70 @@ Tip: In the TUI, press 'b' on any item to boost interactively.`, func newBoostListCmd(project *string) *cobra.Command { var eventID string + var allProjects bool cmd := &cobra.Command{ - Use: "list <id|url>", - Short: "List boosts on an item", + Use: "list [id|url]", + Short: "List boosts on an item, or across every project", Long: `List boosts on an item. You can pass either an ID or a Basecamp URL: basecamp boost list 789 --project my-project basecamp boost list https://3.basecamp.com/123/buckets/456/todos/789 -Use --event to list boosts on a specific event within the item.`, - Args: cobra.ExactArgs(1), +Use --event to list boosts on a specific event within the item. + +Without an ID, boosts from every accessible project are listed, newest +first — the first page of the feed: + basecamp boost list + basecamp boost list --all-projects`, + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { return err } - return runBoostList(cmd, app, args[0], *project, eventID) + recording := "" + if len(args) > 0 { + recording = args[0] + } + return runBoostList(cmd, app, recording, *project, eventID, allProjects) }, } cmd.Flags().StringVar(&eventID, "event", "", "Event ID (for event-specific boosts)") + cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List boosts across every project") return cmd } -func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string) error { +func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string, allProjects bool) error { + // Boosts hang off a single recording, so a project cannot scope this + // listing on its own — only an item ID can. Without one, the account-wide + // feed answers instead, and a configured project is ignored rather than + // turned into an error, since it could never have produced a listing here. + explicitProject := project + if explicitProject == "" { + explicitProject = app.Flags.Project + } + + if recording == "" { + switch { + case explicitProject != "" && allProjects: + return output.ErrUsageHint("Cannot combine --all-projects with --project", + "--all-projects lists boosts from every project; drop --project, or pass an item ID to list one item's boosts") + case explicitProject != "": + return output.ErrUsageHint("Boosts belong to an item, so --project alone cannot list them", + fmt.Sprintf("Pass the item's ID or URL: basecamp boost list <id> --project %s", explicitProject)) + } + return runBoostListAccountWide(cmd, app, eventID) + } + + if allProjects { + return output.ErrUsageHint("Cannot combine --all-projects with an item ID", + "Drop the ID to list boosts across every project, or drop --all-projects to list that item's boosts") + } + recordingID, urlProjectID := extractWithProject(recording) projectID := project @@ -148,6 +187,72 @@ func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, event ) } +// runBoostListAccountWide lists boosts across every accessible project. The +// feed takes a page number and boost list has no paging flags to map onto it, +// so it stays on the first page rather than growing a parallel flag surface. +func runBoostListAccountWide(cmd *cobra.Command, app *appctx.App, eventID string) error { + if err := rejectAccountWideTodolist(app, "boost"); err != nil { + return err + } + if eventID != "" { + return output.ErrUsageHint("--event names an event inside one item, which the account-wide feed has no equivalent for", + "Pass the item's ID alongside --event, or drop --event to list boosts across every project") + } + + result, err := app.Account().Everything().Boosts(cmd.Context(), 1) + if err != nil { + return convertSDKError(err) + } + + // The boosted recording is nested, so every consumer but --json and + // --agent reads the flat rows: the generic renderer skips nested maps, + // which would drop the project and the item title that make a boost row + // mean anything. + respOpts := accountWideRespOpts(len(result.Boosts), "boost", "boosts", result.Meta, "", false) + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideBoosts(result.Boosts))) + respOpts = append(respOpts, output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "show", + Cmd: "basecamp boost show <boost-id>", + Description: "Show a boost", + }, + )) + + return app.OK(result.Boosts, respOpts...) +} + +// flattenAccountWideBoosts turns the account-wide feed into flat rows for the +// styled renderer: each boost nests the recording it sits on, which renders as +// an unreadable cell. Machine formats keep the nested payload. Booster and +// Recording are both optional, so every row carries the same keys whether or +// not the feed populated them. +func flattenAccountWideBoosts(boosts []basecamp.EverythingBoost) []map[string]any { + rows := make([]map[string]any, 0, len(boosts)) + for _, boost := range boosts { + booster := "" + if boost.Booster != nil { + booster = boost.Booster.Name + } + project, title, recordingType := "", "", "" + if boost.Recording != nil { + title = boost.Recording.Title + recordingType = simplifyType(boost.Recording.Type) + if boost.Recording.Bucket != nil { + project = boost.Recording.Bucket.Name + } + } + rows = append(rows, map[string]any{ + "id": boost.ID, + "project": project, + "booster": booster, + "content": boost.Content, + "title": title, + "type": recordingType, + }) + } + return rows +} + func newBoostShowCmd(project *string) *cobra.Command { cmd := &cobra.Command{ Use: "show <boost-id|url>", diff --git a/internal/commands/boost_test.go b/internal/commands/boost_test.go index 0d5720b54..9cbdfa106 100644 --- a/internal/commands/boost_test.go +++ b/internal/commands/boost_test.go @@ -215,6 +215,210 @@ func TestBoostCreateAcceptsMaxContent(t *testing.T) { assert.Equal(t, "POST", transport.capturedMethod) } +// --- account-wide boost listing --- + +// accountWideBoostsRoute serves the /boosts.json aggregate feed. +func accountWideBoostsRoute() stubRoute { + return stubRoute{ + method: http.MethodGet, + path: "/99999/boosts.json", + status: http.StatusOK, + body: `[{"id":1,"content":"🎉","created_at":"2024-01-01T00:00:00Z", + "booster":{"id":10,"name":"Alice"}, + "recording":{"id":456,"title":"Ship it","type":"Todo", + "bucket":{"id":123,"name":"Test Project","type":"Project"}}}]`, + } +} + +// recordingBoostsRoute serves the item-scoped boost listing. +func recordingBoostsRoute() stubRoute { + return stubRoute{ + method: http.MethodGet, + path: "/99999/recordings/456/boosts.json", + status: http.StatusOK, + body: `[{"id":1,"content":"🎉","created_at":"2024-01-01T00:00:00Z"}]`, + } +} + +// setupBoostListTest builds a command and app wired to the boost routes. +func setupBoostListTest(t *testing.T, buf *bytes.Buffer) (*cobra.Command, *appctx.App, *recordingTransport) { + t.Helper() + + app, transport := setupRecordingTestApp(t, projectsRoute(), accountWideBoostsRoute(), recordingBoostsRoute()) + if buf != nil { + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf}) + } + return NewBoostsCmd(), app, transport +} + +// requireBoostUsageError asserts that err is a usage error mentioning want. +func requireBoostUsageError(t *testing.T, err error, want string) { + t.Helper() + + require.Error(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, want) +} + +// TestBoostListWithIDStaysItemScoped verifies that passing an ID still lists +// that item's boosts, unchanged by the account-wide path. +func TestBoostListWithIDStaysItemScoped(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + + require.NoError(t, executeBoostCommand(cmd, app, "list", "456", "--project", "123")) + assert.Equal(t, "/99999/recordings/456/boosts.json", transport.last(t).Path) +} + +// TestBoostListWithoutIDListsAccountWide verifies that a bare list with no +// project anywhere reaches the account-wide feed instead of prompting. +func TestBoostListWithoutIDListsAccountWide(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + + require.NoError(t, executeBoostCommand(cmd, app, "list")) + + last := transport.last(t) + assert.Equal(t, "/99999/boosts.json", last.Path) + assert.Equal(t, "page=1", last.Query, "boost list has no paging flags, so it stays on the first page") +} + +// TestBoostListIgnoresConfiguredProject verifies that a configured project — +// which cannot scope a per-item listing — is ignored rather than errored on. +func TestBoostListIgnoresConfiguredProject(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + app.Config.ProjectID = "123" + + require.NoError(t, executeBoostCommand(cmd, app, "list")) + assert.Equal(t, "/99999/boosts.json", transport.last(t).Path) +} + +// TestBoostListAllProjectsOverridesConfiguredProject verifies that +// --all-projects pins account-wide intent over ambient config. +func TestBoostListAllProjectsOverridesConfiguredProject(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + app.Config.ProjectID = "123" + + require.NoError(t, executeBoostCommand(cmd, app, "list", "--all-projects")) + assert.Equal(t, "/99999/boosts.json", transport.last(t).Path) +} + +// TestBoostListExplicitProjectWithoutIDAsksForID verifies that an explicit +// project without an ID is a usage error — through --project, its --in alias, +// and the root-level form that lands in app.Flags.Project. +func TestBoostListExplicitProjectWithoutIDAsksForID(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--project", "123"), + "--project alone cannot list them") + + cmd, app, _ = setupBoostListTest(t, nil) + requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--in", "123"), + "--project alone cannot list them") + + cmd, app, _ = setupBoostListTest(t, nil) + app.Flags.Project = "123" + requireBoostUsageError(t, executeBoostCommand(cmd, app, "list"), + "--project alone cannot list them") + + assert.Empty(t, transport.recorded(), "a usage error must not reach the API") +} + +// TestBoostListExplicitProjectWithAllProjectsConflicts verifies that +// --all-projects conflicts with an explicitly named project. +func TestBoostListExplicitProjectWithAllProjectsConflicts(t *testing.T) { + cmd, app, _ := setupBoostListTest(t, nil) + requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--project", "123", "--all-projects"), + "Cannot combine --all-projects with --project") + + cmd, app, _ = setupBoostListTest(t, nil) + app.Flags.Project = "123" + requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--all-projects"), + "Cannot combine --all-projects with --project") +} + +// TestBoostListIDWithAllProjectsConflicts verifies that an item ID and +// --all-projects name two different listings. +func TestBoostListIDWithAllProjectsConflicts(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + + requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "456", "--all-projects"), + "Cannot combine --all-projects with an item ID") + assert.Empty(t, transport.recorded()) +} + +// TestBoostListAccountWideRejectsEvent verifies that --event, which names an +// event inside one item, is rejected rather than silently ignored. +func TestBoostListAccountWideRejectsEvent(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + + requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--event", "5"), "--event") + assert.Empty(t, transport.recorded()) +} + +// TestBoostListHasNoPaginationFlags verifies that the account-wide path did not +// grow a parallel pagination surface. +func TestBoostListHasNoPaginationFlags(t *testing.T) { + list, _, err := NewBoostsCmd().Find([]string{"list"}) + require.NoError(t, err) + + for _, name := range []string{"limit", "page", "all"} { + assert.Nil(t, list.Flags().Lookup(name), "boost list must not gain --%s", name) + } +} + +// TestBoostListAccountWideMachineOutputKeepsPayload verifies that machine +// formats get the raw feed, nesting intact. +func TestBoostListAccountWideMachineOutputKeepsPayload(t *testing.T) { + buf := &bytes.Buffer{} + cmd, app, _ := setupBoostListTest(t, buf) + + require.NoError(t, executeBoostCommand(cmd, app, "list")) + + var envelope struct { + Data []struct { + ID int64 `json:"id"` + Recording *struct { + Title string `json:"title"` + } `json:"recording"` + } `json:"data"` + Summary string `json:"summary"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + require.Len(t, envelope.Data, 1) + require.NotNil(t, envelope.Data[0].Recording) + assert.Equal(t, "Ship it", envelope.Data[0].Recording.Title) + assert.Equal(t, "1 boost across all projects", envelope.Summary) +} + +// TestBoostListAccountWideStyledOutputFlattens verifies that styled output gets +// flat rows rather than a nested recording cell. +func TestBoostListAccountWideStyledOutputFlattens(t *testing.T) { + buf := &bytes.Buffer{} + cmd, app, _ := setupBoostListTest(t, nil) + app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: buf}) + + require.NoError(t, executeBoostCommand(cmd, app, "list")) + + rendered := buf.String() + assert.Contains(t, rendered, "Test Project") + assert.Contains(t, rendered, "Alice") + assert.Contains(t, rendered, "Ship it") + assert.NotContains(t, rendered, "Recording", "the nested recording must be flattened away, not rendered as a cell") +} + +// TestFlattenAccountWideBoostsNilPointers verifies that a boost missing its +// booster and recording still yields a row with every column. +func TestFlattenAccountWideBoostsNilPointers(t *testing.T) { + rows := flattenAccountWideBoosts([]basecamp.EverythingBoost{{ID: 7, Content: "👍"}}) + + require.Len(t, rows, 1) + assert.Equal(t, int64(7), rows[0]["id"]) + assert.Equal(t, "👍", rows[0]["content"]) + assert.Equal(t, "", rows[0]["booster"]) + assert.Equal(t, "", rows[0]["project"]) + assert.Equal(t, "", rows[0]["title"]) + assert.Equal(t, "", rows[0]["type"]) +} + // mockBoostNilBoosterTransport returns a boost with no booster field. type mockBoostNilBoosterTransport struct{} diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 4c54ad2a8..21d30e84f 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "strconv" "strings" @@ -55,71 +56,370 @@ func NewCardsCmd() *cobra.Command { return cmd } +// cardsListOptions carries the flags `cards list` accepts, including the +// group's persistent --project/--card-table, so the project-scoped and +// account-wide branches read the same values. +type cardsListOptions struct { + project string + column string + cardTable string + limit int + page int + pageSet bool + all bool + allProjects bool + sortField string + reverse bool + status string + unassigned bool + noDueDate bool + notNow bool + overdue bool +} + +// Account-wide card listings, one per Everything endpoint. +const ( + cardsSelectorOpen = "open" + cardsSelectorCompleted = "completed" + cardsSelectorUnassigned = "unassigned" + cardsSelectorNoDueDate = "no-due-date" + cardsSelectorNotNow = "not-now" + cardsSelectorOverdue = "overdue" +) + func newCardsListCmd(project, cardTable *string) *cobra.Command { - var column string - var limit int - var page int - var all bool - var sortField string - var reverse bool + var opts cardsListOptions cmd := &cobra.Command{ Use: "list", Short: "List cards", - Long: "List all cards in a project's card table.", + Long: "List all cards in a project's card table, or across every project with --all-projects.\n\n" + + "With --all-projects the listing comes from the account-wide card aggregates, grouped by\n" + + "project. --status completed, --unassigned, --no-due-date, --not-now, and --overdue each\n" + + "select a different aggregate and are account-wide only.", RunE: func(cmd *cobra.Command, args []string) error { - return runCardsList(cmd, *project, column, *cardTable, limit, page, all, sortField, reverse) + opts.project = *project + opts.cardTable = *cardTable + opts.pageSet = cmd.Flags().Changed("page") + return runCardsList(cmd, opts) }, } - cmd.Flags().StringVarP(&column, "column", "c", "", "Filter by column ID or name") - cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of cards to fetch (0 = all)") - cmd.Flags().BoolVar(&all, "all", false, "Fetch all cards (no limit)") - cmd.Flags().IntVar(&page, "page", 0, "Fetch a single page (use --all for everything)") - cmd.Flags().StringVar(&sortField, "sort", "", "Sort by field (title, created, updated, position, due)") - cmd.Flags().BoolVar(&reverse, "reverse", false, "Reverse sort order") + cmd.Flags().StringVarP(&opts.column, "column", "c", "", "Filter by column ID or name") + cmd.Flags().IntVarP(&opts.limit, "limit", "n", 0, "Maximum number of cards to fetch (0 = all)") + cmd.Flags().BoolVar(&opts.all, "all", false, "Fetch all cards (no limit)") + cmd.Flags().IntVar(&opts.page, "page", 0, "Fetch a single page (use --all for everything)") + cmd.Flags().StringVar(&opts.sortField, "sort", "", "Sort by field (title, created, updated, position, due)") + cmd.Flags().BoolVar(&opts.reverse, "reverse", false, "Reverse sort order") + cmd.Flags().BoolVar(&opts.allProjects, "all-projects", false, "List cards across every project") + cmd.Flags().StringVar(&opts.status, "status", "", "Account-wide only: completed") + cmd.Flags().BoolVar(&opts.unassigned, "unassigned", false, "Account-wide only: cards with no assignee") + cmd.Flags().BoolVar(&opts.noDueDate, "no-due-date", false, "Account-wide only: cards with no due date") + cmd.Flags().BoolVar(&opts.notNow, "not-now", false, "Account-wide only: cards parked in Not now") + cmd.Flags().BoolVar(&opts.overdue, "overdue", false, "Account-wide only: overdue cards, oldest due date first") return cmd } -func runCardsList(cmd *cobra.Command, project, column, cardTable string, limit, page int, all bool, sortField string, reverse bool) error { +func runCardsList(cmd *cobra.Command, opts cardsListOptions) error { app := appctx.FromContext(cmd.Context()) - // Validate flag combinations - if all && limit > 0 { - return output.ErrUsage("--all and --limit are mutually exclusive") - } - if page > 0 && (all || limit > 0) { - return output.ErrUsage("--page cannot be combined with --all or --limit") + // Scope-independent flag validation. Anything that depends on the scope + // waits until the scope is known, because the two scopes disagree about + // which pages and sorts are legal. + if opts.reverse && opts.sortField == "" { + return output.ErrUsageHint("--reverse requires --sort", "Pick a field to sort by, e.g. --sort due --reverse.") } - if page > 1 { - return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") - } - if sortField != "" { + if opts.sortField != "" { // Validate against the superset of all allowed fields early, before any // API calls. Context-specific restrictions (e.g. no position in aggregate) // are enforced at each branch below. - if err := validateSortField(sortField, []string{"title", "created", "updated", "position", "due"}); err != nil { + if err := validateSortField(opts.sortField, []string{"title", "created", "updated", "position", "due"}); err != nil { return err } } - // Pagination flags only make sense when listing a single column - // When aggregating across columns, pagination is per-column which is confusing - if column == "" && (page > 0 || limit > 0 || all) { + selector, selectorFlag, err := cardsAccountWideSelector(opts) + if err != nil { + return err + } + + // Scope precedence: an explicit project wins, then a configured one, then + // account-wide. --all-projects overrides a configured project and conflicts + // with an explicit one. + explicitProject := opts.project != "" || app.Flags.Project != "" + if opts.allProjects && explicitProject { return output.ErrUsageHint( - "Pagination flags require --column", - "When listing all columns, pagination applies per-column. Use --column <id> to paginate a single column.", + "--all-projects cannot be combined with --project", + "Drop one: --project scopes the listing to a project, --all-projects spans every project.", ) } + accountWide := opts.allProjects || !projectKnown(app, opts.project) // Resolve account (enables interactive prompt if needed) if err := ensureAccount(cmd, app); err != nil { return err } + if accountWide { + return runCardsListAccountWide(cmd, app, opts, selector) + } + + // The endpoint selectors reach account-wide aggregates that have no + // project-scoped equivalent, so a project in scope makes them unanswerable + // rather than a no-op. + if selectorFlag != "" { + return output.ErrUsageHint( + fmt.Sprintf("%s lists cards across every project and cannot be combined with a project", selectorFlag), + "Pass --all-projects (which overrides a configured project), or drop the flag to list this project's cards.", + ) + } + + return runCardsListInProject(cmd, app, opts) +} + +// cardsAccountWideSelector maps the endpoint-selector flags onto the aggregate +// each one picks. The selectors are mutually exclusive: no endpoint combines +// two. The returned flag spelling is empty when the caller passed none. +func cardsAccountWideSelector(opts cardsListOptions) (selector, flag string, err error) { + var chosen []string + if opts.status != "" { + if !strings.EqualFold(strings.TrimSpace(opts.status), cardsSelectorCompleted) { + return "", "", output.ErrUsageHint( + fmt.Sprintf("--status %s is not a card listing", opts.status), + "Only --status completed is supported; open cards are the default.", + ) + } + chosen = append(chosen, "--status completed") + } + for _, sel := range []struct { + on bool + flag string + name string + }{ + {opts.unassigned, "--unassigned", cardsSelectorUnassigned}, + {opts.noDueDate, "--no-due-date", cardsSelectorNoDueDate}, + {opts.notNow, "--not-now", cardsSelectorNotNow}, + {opts.overdue, "--overdue", cardsSelectorOverdue}, + } { + if sel.on { + chosen = append(chosen, sel.flag) + } + } + + if len(chosen) > 1 { + return "", "", output.ErrUsage(fmt.Sprintf( + "%s and %s are mutually exclusive; each selects a different card listing", + chosen[0], chosen[1])) + } + if len(chosen) == 0 { + return cardsSelectorOpen, "", nil + } + + switch chosen[0] { + case "--status completed": + return cardsSelectorCompleted, chosen[0], nil + case "--unassigned": + return cardsSelectorUnassigned, chosen[0], nil + case "--no-due-date": + return cardsSelectorNoDueDate, chosen[0], nil + case "--not-now": + return cardsSelectorNotNow, chosen[0], nil + default: + return cardsSelectorOverdue, chosen[0], nil + } +} + +// runCardsListAccountWide lists cards across every accessible project. The +// paginated aggregates come back grouped by project; --overdue comes back flat +// and unpaginated. +func runCardsListAccountWide(cmd *cobra.Command, app *appctx.App, opts cardsListOptions, selector string) error { + if err := rejectAccountWideTodolist(app, "card"); err != nil { + return err + } + // Scope-child flags name a container inside one project, so they cannot + // narrow a listing that spans every project. --card-table also arrives via + // the group's persistent flag. + if opts.column != "" { + return output.ErrUsageHint( + "--column names a column in one project's card table and cannot narrow an account-wide listing", + "Scope the listing with --project <id> to filter by column.", + ) + } + if opts.cardTable != "" { + return output.ErrUsageHint( + "--card-table names one project's card table and cannot narrow an account-wide listing", + "Scope the listing with --project <id> to pick a card table.", + ) + } + + if opts.limit < 0 { + return output.ErrUsage("--limit cannot be negative") + } + if opts.pageSet && opts.page == 0 { + return output.ErrUsageHint( + "--page 0 is not a page number", + "Use --all to follow every page, or --page 1 for the first one.", + ) + } + if opts.page < 0 { + return output.ErrUsage("--page cannot be negative") + } + if opts.page > math.MaxInt32 { + return output.ErrUsage("--page is out of range") + } + if opts.all && opts.limit > 0 { + return output.ErrUsage("--all and --limit are mutually exclusive") + } + if opts.page > 0 && (opts.all || opts.limit > 0) { + return output.ErrUsage("--page cannot be combined with --all or --limit") + } + + if selector == cardsSelectorOverdue { + if opts.pageSet || opts.all { + return output.ErrUsageHint( + "--overdue returns every overdue card in one unpaginated list", + "Drop --page/--all; use --limit to cap the result.", + ) + } + return runCardsListOverdue(cmd, app, opts) + } + + // The grouped payloads nest cards under their project, and raw grouped + // output cannot represent a globally sorted list. + if opts.sortField != "" { + return output.ErrUsageHint( + "--sort is not supported for grouped account-wide card listings", + "These aggregates are grouped by project; --overdue returns a flat, sortable list.", + ) + } + + // `cards list` fetches everything by default, so the account-wide default + // follows every page. accountWidePage is not used here: it defaults to page + // 1, which would silently cap a listing documented as unbounded. + apiPage := int32(0) + if opts.page > 0 { + apiPage = int32(opts.page) //nolint:gosec // bounded above by the --page range check + } + + everything := app.Account().Everything() + var ( + groupsPage *basecamp.BucketCardsGroupsPage + err error + ) + switch selector { + case cardsSelectorCompleted: + groupsPage, err = everything.CompletedCards(cmd.Context(), apiPage) + case cardsSelectorUnassigned: + groupsPage, err = everything.UnassignedCards(cmd.Context(), apiPage) + case cardsSelectorNoDueDate: + groupsPage, err = everything.NoDueDateCards(cmd.Context(), apiPage) + case cardsSelectorNotNow: + groupsPage, err = everything.NotNowCards(cmd.Context(), apiPage) + default: + groupsPage, err = everything.OpenCards(cmd.Context(), apiPage) + } + if err != nil { + return convertSDKError(err) + } + + // --limit counts cards, not project groups: truncating groups would drop + // whole projects. Meta.TotalCount counts groups, so the item total is ours + // to compute. + groups := groupsPage.Groups + total := countAccountWideCards(groups) + shown := total + if opts.limit > 0 && total > opts.limit { + groups = truncateAccountWideCards(groups, opts.limit) + shown = opts.limit + } + + respOpts := []output.ResponseOption{ + output.WithSummary(fmt.Sprintf("%d %s cards across %d projects", shown, selector, len(groups))), + output.WithBreadcrumbs(cardsAccountWideBreadcrumbs()...), + } + switch { + case shown < total: + respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( + "Showing the first %d of %d cards; drop --limit to see them all", shown, total))) + case groupsPage.Meta.Truncated: + respOpts = append(respOpts, output.WithNotice("More pages are available; results were truncated")) + } + + // --json and --agent keep the grouping the SDK returned; every other + // consumer gets flat rows. Nested groups have no id and no title of their + // own, so a renderer handed them produces unreadable cells, and --ids and + // --count read right past the cards to count projects. + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideCards(groups))) + + return app.OK(groups, respOpts...) +} + +// runCardsListOverdue lists overdue cards across every project. The endpoint +// returns a flat, oldest-due-first array, so it renders like the +// project-scoped path and accepts the sort flags. +func runCardsListOverdue(cmd *cobra.Command, app *appctx.App, opts cardsListOptions) error { + if opts.sortField == "position" { + return output.ErrUsage("--sort position requires --column (position is per-column)") + } + + cards, err := app.Account().Everything().OverdueCards(cmd.Context()) + if err != nil { + return convertSDKError(err) + } + + // Sort before truncating: truncating first would sort only the survivors. + if opts.sortField != "" { + sortCards(cards, opts.sortField, opts.reverse) + } + + total := len(cards) + if opts.limit > 0 && total > opts.limit { + cards = cards[:opts.limit] + } + + respOpts := []output.ResponseOption{ + output.WithSummary(fmt.Sprintf("%d overdue cards across all projects", len(cards))), + output.WithBreadcrumbs(cardsAccountWideBreadcrumbs()...), + output.WithDisplayData(flattenOverdueCards(cards)), + } + if len(cards) < total { + respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( + "Showing the first %d of %d cards; drop --limit to see them all", len(cards), total))) + } + + return app.OK(cards, respOpts...) +} + +func runCardsListInProject(cmd *cobra.Command, app *appctx.App, opts cardsListOptions) error { + // Validate flag combinations + if opts.all && opts.limit > 0 { + return output.ErrUsage("--all and --limit are mutually exclusive") + } + if opts.page > 0 && (opts.all || opts.limit > 0) { + return output.ErrUsage("--page cannot be combined with --all or --limit") + } + if opts.page > 1 { + return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") + } + if opts.page < 0 { + return output.ErrUsage("--page cannot be negative") + } + if opts.limit < 0 { + return output.ErrUsage("--limit cannot be negative") + } + + // Pagination flags only make sense when listing a single column + // When aggregating across columns, pagination is per-column which is confusing + if opts.column == "" && (opts.page > 0 || opts.limit > 0 || opts.all) { + return output.ErrUsageHint( + "Pagination flags require --column", + "When listing all columns, pagination applies per-column. Use --column <id> to paginate a single column.", + ) + } + // Resolve project from CLI flags and config, with interactive fallback - projectID := project + projectID := opts.project if projectID == "" { projectID = app.Flags.Project } @@ -137,7 +437,7 @@ func runCardsList(cmd *cobra.Command, project, column, cardTable string, limit, // Column name (non-numeric) requires --card-table for resolution // Numeric column IDs can be used directly without discovery - if column != "" && !isNumericID(column) && cardTable == "" { + if opts.column != "" && !isNumericID(opts.column) && opts.cardTable == "" { return output.ErrUsage("--card-table is required when using --column with a name") } @@ -147,31 +447,31 @@ func runCardsList(cmd *cobra.Command, project, column, cardTable string, limit, } // Build pagination options - opts := &basecamp.CardListOptions{} - if all { - opts.Limit = -1 // SDK treats -1 as "fetch all" - } else if limit > 0 { - opts.Limit = limit + listOpts := &basecamp.CardListOptions{} + if opts.all { + listOpts.Limit = -1 // SDK treats -1 as "fetch all" + } else if opts.limit > 0 { + listOpts.Limit = opts.limit } - if page > 0 { - opts.Page = page + if opts.page > 0 { + listOpts.Page = opts.page } // Optimization: If column is a numeric ID, skip card table discovery // and fetch cards directly from the column endpoint - if column != "" && isNumericID(column) { - columnID, err := strconv.ParseInt(column, 10, 64) + if opts.column != "" && isNumericID(opts.column) { + columnID, err := strconv.ParseInt(opts.column, 10, 64) if err != nil { return output.ErrUsage("Invalid column ID") } - cardsResult, err := app.Account().Cards().List(cmd.Context(), columnID, opts) + cardsResult, err := app.Account().Cards().List(cmd.Context(), columnID, listOpts) if err != nil { return convertSDKError(err) } - if sortField != "" { - sortCards(cardsResult.Cards, sortField, reverse) + if opts.sortField != "" { + sortCards(cardsResult.Cards, opts.sortField, opts.reverse) } return app.OK(cardsResult.Cards, @@ -181,7 +481,7 @@ func runCardsList(cmd *cobra.Command, project, column, cardTable string, limit, } // Get card table ID from project dock - cardTableID, err := getCardTableID(cmd, app, resolvedProjectID, cardTable) + cardTableID, err := getCardTableID(cmd, app, resolvedProjectID, opts.cardTable) if err != nil { return err } @@ -199,27 +499,27 @@ func runCardsList(cmd *cobra.Command, project, column, cardTable string, limit, // Get cards from all columns or specific column var allCards []basecamp.Card - if column != "" { + if opts.column != "" { // Find column by ID or name - columnID := resolveColumn(cardTableData.Lists, column) + columnID := resolveColumn(cardTableData.Lists, opts.column) if columnID == 0 { return output.ErrUsageHint( - fmt.Sprintf("Column '%s' not found", column), + fmt.Sprintf("Column '%s' not found", opts.column), "Use column ID or exact name", ) } - cardsResult, err := app.Account().Cards().List(cmd.Context(), columnID, opts) + cardsResult, err := app.Account().Cards().List(cmd.Context(), columnID, listOpts) if err != nil { return convertSDKError(err) } allCards = cardsResult.Cards - if sortField != "" { - sortCards(allCards, sortField, reverse) + if opts.sortField != "" { + sortCards(allCards, opts.sortField, opts.reverse) } } else { // No position in aggregate — it's only meaningful within a single column - if sortField == "position" { + if opts.sortField == "position" { return output.ErrUsage("--sort position requires --column (position is per-column)") } @@ -232,8 +532,8 @@ func runCardsList(cmd *cobra.Command, project, column, cardTable string, limit, allCards = append(allCards, cardsResult.Cards...) } - if sortField != "" { - sortCards(allCards, sortField, reverse) + if opts.sortField != "" { + sortCards(allCards, opts.sortField, opts.reverse) } } @@ -249,6 +549,79 @@ func runCardsList(cmd *cobra.Command, project, column, cardTable string, limit, ) } +// countAccountWideCards totals the cards inside the project groups. +func countAccountWideCards(groups []basecamp.BucketCardsGroup) int { + total := 0 + for _, group := range groups { + total += len(group.Cards) + } + return total +} + +// truncateAccountWideCards keeps the first limit cards, trimming groups from +// the tail rather than dropping cards out of the middle of a project. +func truncateAccountWideCards(groups []basecamp.BucketCardsGroup, limit int) []basecamp.BucketCardsGroup { + kept := make([]basecamp.BucketCardsGroup, 0, len(groups)) + remaining := limit + for _, group := range groups { + if remaining <= 0 { + break + } + if len(group.Cards) > remaining { + group.Cards = group.Cards[:remaining] + } + remaining -= len(group.Cards) + kept = append(kept, group) + } + return kept +} + +// flattenAccountWideCards turns the project groups into one row per card, so +// styled output renders a flat table instead of nested cells. +func flattenAccountWideCards(groups []basecamp.BucketCardsGroup) []map[string]any { + rows := make([]map[string]any, 0, countAccountWideCards(groups)) + for _, group := range groups { + for _, card := range group.Cards { + rows = append(rows, map[string]any{ + "id": card.ID, + "title": card.Title, + "project": group.Bucket.Name, + "status": card.Status, + "due": card.DueOn, + }) + } + } + return rows +} + +// flattenOverdueCards builds display rows for the flat overdue aggregate. The +// cards arrive with a nested bucket, which both generic renderers skip by name, +// so without this an account-wide overdue listing gives no way to tell which +// project a card belongs to. +func flattenOverdueCards(cards []basecamp.Card) []map[string]any { + rows := make([]map[string]any, 0, len(cards)) + for _, card := range cards { + row := map[string]any{ + "id": card.ID, + "title": card.Title, + "status": card.Status, + "due": card.DueOn, + } + if card.Bucket != nil { + row["project"] = card.Bucket.Name + } + rows = append(rows, row) + } + return rows +} + +func cardsAccountWideBreadcrumbs() []output.Breadcrumb { + return []output.Breadcrumb{ + {Action: "show", Cmd: "basecamp cards show <id>", Description: "Show card details"}, + {Action: "scope", Cmd: "basecamp cards list --in <project_id>", Description: "List one project's cards"}, + } +} + func cardsListBreadcrumbs(resolvedProjectID string) []output.Breadcrumb { return []output.Breadcrumb{ {Action: "create", Cmd: fmt.Sprintf("basecamp cards create <title> --in %s", resolvedProjectID), Description: "Create card"}, @@ -633,7 +1006,7 @@ You can pass either a card ID or a Basecamp URL: req := &basecamp.UpdateCardRequest{} if title != "" { - req.Title = title + req.Title = &title } var mentionNotice string var html string @@ -661,10 +1034,11 @@ You can pass either a card ID or a Basecamp URL: } if html != "" { - req.Content = html + req.Content = &html } if due != "" { - req.DueOn = dateparse.Parse(due) + dueOn := dateparse.Parse(due) + req.DueOn = &dueOn } if cmd.Flags().Changed("assignee") { assigneeID, err := resolveAssigneeID(cmd.Context(), app, assignee) diff --git a/internal/commands/cards_test.go b/internal/commands/cards_test.go index ada7940ea..ddf81db6f 100644 --- a/internal/commands/cards_test.go +++ b/internal/commands/cards_test.go @@ -325,19 +325,16 @@ func TestCardsStepMoveRequiresPosition(t *testing.T) { } // TestCardsCmdRequiresProject tests that Project ID required when not in config. -func TestCardsCmdRequiresProject(t *testing.T) { - app, _ := setupTestApp(t) +// TestCardsCmdWithoutProjectListsAccountWide tests that a bare `cards list` +// with no project anywhere lists account-wide rather than prompting. +func TestCardsCmdWithoutProjectListsAccountWide(t *testing.T) { + app, transport := setupRecordingTestApp(t, cardsOpenRoute()) // No project in config cmd := NewCardsCmd() - err := executeCommand(cmd, app, "list") - require.NotNil(t, err, "expected error, got nil") - - var e *output.Error - if assert.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) { - assert.Equal(t, "Project ID required", e.Message) - } + require.NoError(t, executeRecordingCommand(cmd, app, "list")) + assert.Equal(t, "/99999/cards/open.json", transport.last(t).Path) } // TestCardsListColumnNameRequiresCardTable tests that column name requires --card-table. @@ -2424,3 +2421,352 @@ func TestCardsWormholesUpdateBreadcrumbPinsCardTable(t *testing.T) { require.Len(t, envelope.Breadcrumbs, 1) assert.Contains(t, envelope.Breadcrumbs[0].Cmd, "cards wormholes list --in 123 --card-table 555") } + +// --- account-wide card listings --- + +// cardsGroupsFixture is a two-project grouped response, two cards each. +const cardsGroupsFixture = `[ + {"bucket":{"id":1,"name":"Alpha","type":"Project"}, + "cards":[{"id":11,"title":"Alpha one","status":"active","due_on":"2026-01-01"}, + {"id":12,"title":"Alpha two","status":"active"}]}, + {"bucket":{"id":2,"name":"Beta","type":"Project"}, + "cards":[{"id":21,"title":"Beta one","status":"active"}, + {"id":22,"title":"Beta two","status":"active","due_on":"2026-03-01"}]} +]` + +// cardsOverdueFixture is the flat, unpaginated overdue payload. +const cardsOverdueFixture = `[ + {"id":31,"title":"Zeta late","status":"active","due_on":"2025-02-01"}, + {"id":32,"title":"Alpha late","status":"active","due_on":"2025-01-01"} +]` + +func cardsAggregateRoute(name, body string) stubRoute { + return stubRoute{ + method: http.MethodGet, + path: fmt.Sprintf("/99999/cards/%s.json", name), + status: http.StatusOK, + body: body, + } +} + +func cardsOpenRoute() stubRoute { return cardsAggregateRoute("open", cardsGroupsFixture) } + +// cardsAllAggregateRoutes serves every account-wide card endpoint, so a test +// that dispatches to the wrong one fails on the recorded path rather than on a +// missing stub. +func cardsAllAggregateRoutes() []stubRoute { + return []stubRoute{ + cardsAggregateRoute("open", cardsGroupsFixture), + cardsAggregateRoute("completed", cardsGroupsFixture), + cardsAggregateRoute("unassigned", cardsGroupsFixture), + cardsAggregateRoute("no_due_date", cardsGroupsFixture), + cardsAggregateRoute("not_now", cardsGroupsFixture), + cardsAggregateRoute("overdue", cardsOverdueFixture), + } +} + +// setupCardsAccountWideApp wires the recording harness to an output writer the +// test can read back. +func setupCardsAccountWideApp(t *testing.T, format output.Format, routes ...stubRoute) (*appctx.App, *recordingTransport, *bytes.Buffer) { + t.Helper() + app, transport := setupRecordingTestApp(t, routes...) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: format, Writer: buf}) + return app, transport, buf +} + +// cardsUsageError asserts the command failed with a usage error and returns it. +func cardsUsageError(t *testing.T, err error) *output.Error { + t.Helper() + require.Error(t, err) + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) + return e +} + +// TestCardsListProjectScopedUnchanged pins that a project in scope still lists +// through the project-scoped column endpoint. +func TestCardsListProjectScopedUnchanged(t *testing.T) { + app, transport := setupRecordingTestApp(t, + projectsRoute(), + stubRoute{http.MethodGet, "/99999/card_tables/lists/12345/cards.json", http.StatusOK, `[]`}, + ) + + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--project", "123", "--column", "12345")) + assert.Equal(t, "/99999/card_tables/lists/12345/cards.json", transport.last(t).Path) +} + +// TestCardsListAllProjectsOverridesConfiguredProject tests that --all-projects +// beats an ambient configured project rather than conflicting with it. +func TestCardsListAllProjectsOverridesConfiguredProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, cardsOpenRoute()) + app.Config.ProjectID = "123" + + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects")) + assert.Equal(t, "/99999/cards/open.json", transport.last(t).Path) +} + +// TestCardsListAllProjectsConflictsWithExplicitProject tests the conflict for +// both the after-the-noun and root-level spellings of an explicit project. +func TestCardsListAllProjectsConflictsWithExplicitProject(t *testing.T) { + for _, spelling := range []string{"--project", "--in"} { + app, _ := setupRecordingTestApp(t) + err := executeRecordingCommand(NewCardsCmd(), app, "list", spelling, "123", "--all-projects") + assert.Equal(t, "--all-projects cannot be combined with --project", cardsUsageError(t, err).Message, spelling) + } + + app, _ := setupRecordingTestApp(t) + app.Flags.Project = "123" // basecamp --project 123 cards list + err := executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects") + assert.Equal(t, "--all-projects cannot be combined with --project", cardsUsageError(t, err).Message) +} + +// TestCardsListAccountWideSelectorEndpoints tests that each selector flag +// reaches its own endpoint. +func TestCardsListAccountWideSelectorEndpoints(t *testing.T) { + cases := []struct { + args []string + path string + }{ + {[]string{"--all-projects"}, "/99999/cards/open.json"}, + {[]string{"--all-projects", "--status", "completed"}, "/99999/cards/completed.json"}, + {[]string{"--all-projects", "--unassigned"}, "/99999/cards/unassigned.json"}, + {[]string{"--all-projects", "--no-due-date"}, "/99999/cards/no_due_date.json"}, + {[]string{"--all-projects", "--not-now"}, "/99999/cards/not_now.json"}, + {[]string{"--all-projects", "--overdue"}, "/99999/cards/overdue.json"}, + } + for _, tc := range cases { + app, transport := setupRecordingTestApp(t, cardsAllAggregateRoutes()...) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, tc.args...)...), tc.args) + assert.Equal(t, tc.path, transport.last(t).Path, tc.args) + } +} + +// TestCardsListSelectorsRejectedWithProject tests that the account-wide-only +// selectors are a usage error whenever a project is in scope — explicit, +// root-level, or configured. +func TestCardsListSelectorsRejectedWithProject(t *testing.T) { + selectors := [][]string{ + {"--status", "completed"}, + {"--unassigned"}, + {"--no-due-date"}, + {"--not-now"}, + {"--overdue"}, + } + for _, selector := range selectors { + // Explicit --project after the group noun. + app, _ := setupRecordingTestApp(t, projectsRoute()) + err := executeRecordingCommand(NewCardsCmd(), app, append([]string{"list", "--project", "123"}, selector...)...) + assert.Contains(t, cardsUsageError(t, err).Message, "cannot be combined with a project", selector) + + // Root-level --project, which lands in app.Flags.Project. + app, _ = setupRecordingTestApp(t, projectsRoute()) + app.Flags.Project = "123" + err = executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, selector...)...) + assert.Contains(t, cardsUsageError(t, err).Message, "cannot be combined with a project", selector) + + // Configured project. + app, _ = setupRecordingTestApp(t, projectsRoute()) + app.Config.ProjectID = "123" + err = executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, selector...)...) + assert.Contains(t, cardsUsageError(t, err).Message, "cannot be combined with a project", selector) + } +} + +// TestCardsListSelectorsMutuallyExclusive tests that two selectors name the +// conflicting pair rather than silently picking one. +func TestCardsListSelectorsMutuallyExclusive(t *testing.T) { + app, _ := setupRecordingTestApp(t) + err := executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--unassigned", "--overdue") + msg := cardsUsageError(t, err).Message + assert.Contains(t, msg, "--unassigned") + assert.Contains(t, msg, "--overdue") + assert.Contains(t, msg, "mutually exclusive") +} + +// TestCardsListStatusRejectsUnsupportedValue tests that only completed is a +// recognized --status. +func TestCardsListStatusRejectsUnsupportedValue(t *testing.T) { + app, _ := setupRecordingTestApp(t) + err := executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--status", "archived") + assert.Contains(t, cardsUsageError(t, err).Message, "--status archived is not a card listing") +} + +// TestCardsListAccountWideRejectsScopeChildren tests that --column and +// --card-table are rejected account-wide, including via the shorthand and the +// group's persistent flag. +func TestCardsListAccountWideRejectsScopeChildren(t *testing.T) { + cases := []struct { + args []string + message string + }{ + {[]string{"list", "--all-projects", "--column", "9"}, "--column names a column"}, + {[]string{"list", "--all-projects", "-c", "9"}, "--column names a column"}, + {[]string{"list", "--all-projects", "--card-table", "7"}, "--card-table names one project's"}, + {[]string{"--card-table", "7", "list", "--all-projects"}, "--card-table names one project's"}, + {[]string{"list", "--column", "9"}, "--column names a column"}, + } + for _, tc := range cases { + app, _ := setupRecordingTestApp(t) + err := executeRecordingCommand(NewCardsCmd(), app, tc.args...) + assert.Contains(t, cardsUsageError(t, err).Message, tc.message, tc.args) + } +} + +// TestCardsListAccountWidePagination tests the page mapping: the default and +// --all follow every page, --page N asks for exactly N. +func TestCardsListAccountWidePagination(t *testing.T) { + cases := []struct { + args []string + query string + }{ + {[]string{"--all-projects"}, ""}, + {[]string{"--all-projects", "--all"}, ""}, + {[]string{"--all-projects", "--page", "3"}, "page=3"}, + {[]string{"--all-projects", "--limit", "2"}, ""}, + } + for _, tc := range cases { + app, transport := setupRecordingTestApp(t, cardsOpenRoute()) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, tc.args...)...), tc.args) + assert.Equal(t, tc.query, transport.last(t).Query, tc.args) + } +} + +// TestCardsListAccountWideRejectsBadPaging tests that the page and limit values +// with no account-wide meaning are usage errors rather than surprises. +func TestCardsListAccountWideRejectsBadPaging(t *testing.T) { + cases := []struct { + args []string + message string + }{ + {[]string{"--all-projects", "--page", "0"}, "--page 0 is not a page number"}, + {[]string{"--all-projects", "--page", "-1"}, "--page cannot be negative"}, + {[]string{"--all-projects", "--limit", "-1"}, "--limit cannot be negative"}, + {[]string{"--all-projects", "--all", "--limit", "2"}, "--all and --limit are mutually exclusive"}, + {[]string{"--all-projects", "--page", "2", "--limit", "2"}, "--page cannot be combined with --all or --limit"}, + } + for _, tc := range cases { + app, _ := setupRecordingTestApp(t, cardsOpenRoute()) + err := executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, tc.args...)...) + assert.Equal(t, tc.message, cardsUsageError(t, err).Message, tc.args) + } +} + +// TestCardsListOverdueRejectsPaging tests that the unpaginated endpoint refuses +// page flags instead of accepting and dropping them. +func TestCardsListOverdueRejectsPaging(t *testing.T) { + for _, args := range [][]string{ + {"--all-projects", "--overdue", "--page", "1"}, + {"--all-projects", "--overdue", "--all"}, + } { + app, _ := setupRecordingTestApp(t, cardsAllAggregateRoutes()...) + err := executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, args...)...) + assert.Contains(t, cardsUsageError(t, err).Message, "one unpaginated list", args) + } +} + +// TestCardsListAccountWideSorting tests that sorting is rejected for the +// grouped aggregates and honored for the flat overdue list. +func TestCardsListAccountWideSorting(t *testing.T) { + app, _ := setupRecordingTestApp(t, cardsOpenRoute()) + err := executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--sort", "due") + assert.Contains(t, cardsUsageError(t, err).Message, "--sort is not supported for grouped") + + app, _ = setupRecordingTestApp(t, cardsAllAggregateRoutes()...) + err = executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--overdue", "--sort", "position") + assert.Contains(t, cardsUsageError(t, err).Message, "--sort position requires --column") + + // Sorting precedes truncation: the earliest-titled card survives --limit 1. + app, _, buf := setupCardsAccountWideApp(t, output.FormatJSON, cardsAllAggregateRoutes()...) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, + "list", "--all-projects", "--overdue", "--sort", "title", "--limit", "1")) + + var envelope struct { + Data []basecamp.Card `json:"data"` + Notice string `json:"notice"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + require.Len(t, envelope.Data, 1) + assert.Equal(t, "Alpha late", envelope.Data[0].Title) + assert.Contains(t, envelope.Notice, "first 1 of 2") +} + +// TestCardsListReverseRequiresSort tests that --reverse alone is an error +// rather than a no-op. +func TestCardsListReverseRequiresSort(t *testing.T) { + app, _ := setupRecordingTestApp(t, cardsOpenRoute()) + err := executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--reverse") + assert.Equal(t, "--reverse requires --sort", cardsUsageError(t, err).Message) +} + +// TestCardsListAccountWideLimitCountsCards tests that --limit truncates inner +// cards, keeping the groups that hold them, rather than dropping projects. +func TestCardsListAccountWideLimitCountsCards(t *testing.T) { + app, _, buf := setupCardsAccountWideApp(t, output.FormatJSON, cardsOpenRoute()) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--limit", "3")) + + var envelope struct { + Data []basecamp.BucketCardsGroup `json:"data"` + Summary string `json:"summary"` + Notice string `json:"notice"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + require.Len(t, envelope.Data, 2, "both projects survive a card-counted limit") + assert.Equal(t, 3, countAccountWideCards(envelope.Data)) + assert.Equal(t, "Alpha", envelope.Data[0].Bucket.Name) + assert.Len(t, envelope.Data[1].Cards, 1) + assert.Contains(t, envelope.Summary, "3 open cards across 2 projects") + assert.Contains(t, envelope.Notice, "first 3 of 4") +} + +// TestCardsListAccountWideOutputBranches tests that machine formats keep the +// grouping and styled output gets flattened rows. +func TestCardsListAccountWideOutputBranches(t *testing.T) { + app, _, buf := setupCardsAccountWideApp(t, output.FormatJSON, cardsOpenRoute()) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects")) + + var envelope struct { + Data []basecamp.BucketCardsGroup `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + require.Len(t, envelope.Data, 2) + assert.Equal(t, 4, countAccountWideCards(envelope.Data)) + + app, _, styled := setupCardsAccountWideApp(t, output.FormatStyled, cardsOpenRoute()) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects")) + rendered := styled.String() + assert.Contains(t, rendered, "Alpha") + assert.Contains(t, rendered, "Beta") + assert.Contains(t, rendered, "Alpha one") + assert.NotContains(t, rendered, "bucket") +} + +// TestFlattenAccountWideCards tests that flattening carries the project name +// alongside each card. +func TestFlattenAccountWideCards(t *testing.T) { + groups := []basecamp.BucketCardsGroup{ + {Bucket: basecamp.Bucket{ID: 1, Name: "Alpha"}, Cards: []basecamp.Card{{ID: 11, Title: "One", Status: "active", DueOn: "2026-01-01"}}}, + {Bucket: basecamp.Bucket{ID: 2, Name: "Beta"}}, + } + + rows := flattenAccountWideCards(groups) + require.Len(t, rows, 1) + assert.Equal(t, map[string]any{ + "id": int64(11), "title": "One", "project": "Alpha", "status": "active", "due": "2026-01-01", + }, rows[0]) +} + +// TestTruncateAccountWideCards tests that truncation trims from the tail and +// never keeps more cards than asked for. +func TestTruncateAccountWideCards(t *testing.T) { + groups := []basecamp.BucketCardsGroup{ + {Bucket: basecamp.Bucket{Name: "Alpha"}, Cards: []basecamp.Card{{ID: 1}, {ID: 2}}}, + {Bucket: basecamp.Bucket{Name: "Beta"}, Cards: []basecamp.Card{{ID: 3}, {ID: 4}}}, + } + + assert.Equal(t, 1, countAccountWideCards(truncateAccountWideCards(groups, 1))) + assert.Len(t, truncateAccountWideCards(groups, 1), 1) + assert.Len(t, truncateAccountWideCards(groups, 2), 1) + assert.Len(t, truncateAccountWideCards(groups, 3), 2) + assert.Equal(t, 4, countAccountWideCards(truncateAccountWideCards(groups, 9))) +} diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index 37f28ae88..544121220 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -39,7 +39,7 @@ on a schedule (e.g., "What did you work on today?").`, cmd.AddCommand( newCheckinsQuestionsCmd(&project, &questionnaireID), newCheckinsQuestionCmd(&project), - newCheckinsAnswersCmd(&project), + newCheckinsAnswersCmd(&project, &questionnaireID), newCheckinsAnswerCmd(&project), ) @@ -534,15 +534,16 @@ You can pass either a question ID or a Basecamp URL: return cmd } -func newCheckinsAnswersCmd(project *string) *cobra.Command { +func newCheckinsAnswersCmd(project, questionnaireID *string) *cobra.Command { var limit int var page int var all bool + var allProjects bool var by string cmd := &cobra.Command{ - Use: "answers <question_id|url>", - Short: "List answers for a question", + Use: "answers [question_id|url]", + Short: "List answers for a question, or across every project", Long: `List answers for a check-in question. You can pass either a question ID or a Basecamp URL: @@ -551,122 +552,241 @@ You can pass either a question ID or a Basecamp URL: Use --by to filter answers by a specific person (name, email, ID, or "me"): basecamp checkins answers 789 --by me --in my-project - basecamp checkins answers 789 --by "Alice Smith" --in my-project`, - Args: cobra.ExactArgs(1), + basecamp checkins answers 789 --by "Alice Smith" --in my-project + +Omit the question ID to list check-in answers across every project you can +reach, newest first. A configured project is ignored there, because a project +on its own cannot name a question; --all-projects states that intent outright: + basecamp checkins answers + basecamp checkins answers --all-projects`, + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - // Validate flag combinations - if all && limit > 0 { - return output.ErrUsage("--all and --limit are mutually exclusive") - } - if page > 0 && (all || limit > 0) { - return output.ErrUsage("--page cannot be combined with --all or --limit") - } - if page > 1 { - return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") - } - if err := ensureAccount(cmd, app); err != nil { return err } - // Extract ID and project from URL if provided - questionIDStr, urlProjectID := extractWithProject(args[0]) - - // Resolve project - use URL > flag > config, with interactive fallback - projectID := *project - if projectID == "" && urlProjectID != "" { - projectID = urlProjectID - } - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err + // Pick the scope before validating anything against it: the + // account-wide feed serves any positive page, while the + // per-question listing only serves page 1. + // + // Answers belong to one question, so a project can never select + // this listing on its own. That makes an explicit project without + // a question ID a request we cannot answer, and a merely + // configured project something to ignore rather than honor. + explicitProject := *project != "" || app.Flags.Project != "" + + switch { + case len(args) > 0 && allProjects: + return output.ErrUsageHint("--all-projects cannot be combined with a question ID", + "Drop the ID to list answers across every project, or drop --all-projects to list that question's answers") + case len(args) > 0: + // A question ID already names its questionnaire, so + // --questionnaire cannot narrow anything here. It was + // accepted and dropped on this branch; say so instead. + if *questionnaireID != "" { + return output.ErrUsageHint( + "--questionnaire cannot narrow a question's answers", + "The question ID already selects its questionnaire. Drop --questionnaire.") } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) - if err != nil { - return err - } - - questionID, err := strconv.ParseInt(questionIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid question ID") + return runCheckinsAnswers(cmd, app, *project, args[0], by, limit, page, all) + case explicitProject && allProjects: + return output.ErrUsageHint("--all-projects cannot be combined with --project", + "Drop --project to list answers across every project") + case explicitProject: + return output.ErrUsageHint("A project alone cannot select check-in answers", + "Pass a question ID: basecamp checkins answers <question_id>") } - // Build pagination options - opts := &basecamp.AnswerListOptions{} - if all { - opts.Limit = -1 // SDK treats -1 as "fetch all" - } else if limit > 0 { - opts.Limit = limit - } - if page > 0 { - opts.Page = page - } - - trimmedBy := strings.TrimSpace(by) - if cmd.Flags().Changed("by") && trimmedBy == "" { - return output.ErrUsage("--by value cannot be blank") - } - - var answers []basecamp.QuestionAnswer - if trimmedBy != "" { - personIDStr, _, err := app.Names.ResolvePerson(cmd.Context(), trimmedBy) - if err != nil { - return err - } - personID, err := strconv.ParseInt(personIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid person ID") - } - answersResult, err := app.Account().Checkins().ListAnswersByPerson(cmd.Context(), questionID, personID, opts) - if err != nil { - return convertSDKError(err) - } - answers = answersResult.Answers - } else { - answersResult, err := app.Account().Checkins().ListAnswers(cmd.Context(), questionID, opts) - if err != nil { - return convertSDKError(err) - } - answers = answersResult.Answers - } - - return app.OK(answers, - output.WithSummary(fmt.Sprintf("%d answers", len(answers))), - output.WithBreadcrumbs( - output.Breadcrumb{ - Action: "answer", - Cmd: fmt.Sprintf("basecamp checkins answer <id> --in %s", resolvedProjectID), - Description: "View answer details", - }, - output.Breadcrumb{ - Action: "question", - Cmd: fmt.Sprintf("basecamp checkins question %s --in %s", questionIDStr, resolvedProjectID), - Description: "View question", - }, - ), - ) + return runCheckinsAnswersAccountWide(cmd, app, *questionnaireID, limit, page, all) }, } cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of answers to fetch (0 = all)") cmd.Flags().BoolVar(&all, "all", false, "Fetch all answers (no limit)") cmd.Flags().IntVar(&page, "page", 0, "Fetch a single page (use --all for everything)") + cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List check-in answers across every project (no question ID)") cmd.Flags().StringVar(&by, "by", "", "Filter answers by person (name, email, ID, or \"me\")") return cmd } +// runCheckinsAnswers lists the answers to a single question. +func runCheckinsAnswers(cmd *cobra.Command, app *appctx.App, project, questionArg, by string, limit, page int, all bool) error { + // Validate flag combinations + if all && limit > 0 { + return output.ErrUsage("--all and --limit are mutually exclusive") + } + if page > 0 && (all || limit > 0) { + return output.ErrUsage("--page cannot be combined with --all or --limit") + } + if page > 1 { + return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") + } + + // Extract ID and project from URL if provided + questionIDStr, urlProjectID := extractWithProject(questionArg) + + // Resolve project - use URL > flag > config, with interactive fallback + projectID := project + if projectID == "" && urlProjectID != "" { + projectID = urlProjectID + } + if projectID == "" { + projectID = app.Flags.Project + } + if projectID == "" { + projectID = app.Config.ProjectID + } + if projectID == "" { + if err := ensureProject(cmd, app); err != nil { + return err + } + projectID = app.Config.ProjectID + } + + resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + if err != nil { + return err + } + + questionID, err := strconv.ParseInt(questionIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid question ID") + } + + // Build pagination options + opts := &basecamp.AnswerListOptions{} + if all { + opts.Limit = -1 // SDK treats -1 as "fetch all" + } else if limit > 0 { + opts.Limit = limit + } + if page > 0 { + opts.Page = page + } + + trimmedBy := strings.TrimSpace(by) + if cmd.Flags().Changed("by") && trimmedBy == "" { + return output.ErrUsage("--by value cannot be blank") + } + + var answers []basecamp.QuestionAnswer + if trimmedBy != "" { + personIDStr, _, err := app.Names.ResolvePerson(cmd.Context(), trimmedBy) + if err != nil { + return err + } + personID, err := strconv.ParseInt(personIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid person ID") + } + answersResult, err := app.Account().Checkins().ListAnswersByPerson(cmd.Context(), questionID, personID, opts) + if err != nil { + return convertSDKError(err) + } + answers = answersResult.Answers + } else { + answersResult, err := app.Account().Checkins().ListAnswers(cmd.Context(), questionID, opts) + if err != nil { + return convertSDKError(err) + } + answers = answersResult.Answers + } + + return app.OK(answers, + output.WithSummary(fmt.Sprintf("%d answers", len(answers))), + output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "answer", + Cmd: fmt.Sprintf("basecamp checkins answer <id> --in %s", resolvedProjectID), + Description: "View answer details", + }, + output.Breadcrumb{ + Action: "question", + Cmd: fmt.Sprintf("basecamp checkins question %s --in %s", questionIDStr, resolvedProjectID), + Description: "View question", + }, + ), + ) +} + +// runCheckinsAnswersAccountWide lists check-in answers across every project the +// account can reach. The listing is a flat []Recording, the same payload +// `recordings list` already hands the styled renderer, so it needs no +// format-dependent flattening. +func runCheckinsAnswersAccountWide(cmd *cobra.Command, app *appctx.App, questionnaireID string, limit, page int, all bool) error { + if err := rejectAccountWideTodolist(app, "check-in answer"); err != nil { + return err + } + // A questionnaire is a container inside one project and a person filter + // has no aggregate endpoint, so neither can narrow this feed. Accepting + // either would answer a different question than the one asked. + if questionnaireID != "" { + return output.ErrUsageHint("--questionnaire names a check-in container inside one project", + "Pass a question ID to list that question's answers, or drop --questionnaire") + } + if cmd.Flags().Changed("by") { + return output.ErrUsageHint("--by has no account-wide equivalent", + "Filter by person within one question: basecamp checkins answers <question_id> --by <person>") + } + + if limit < 0 { + return output.ErrUsage("--limit cannot be negative") + } + if page < 0 { + return output.ErrUsage("--page cannot be negative") + } + if cmd.Flags().Changed("page") && page == 0 { + return output.ErrUsage("--page 0 is not a page; use --all to fetch every page") + } + if all && limit > 0 { + return output.ErrUsage("--all and --limit are mutually exclusive") + } + if page > 0 && (all || limit > 0) { + return output.ErrUsage("--page cannot be combined with --all or --limit") + } + + // The project-scoped default is "0 = all", so the account-wide default + // follows every page too. Only an explicit --page narrows it to one. + sdkPage, err := accountWidePage(page, all || page == 0) + if err != nil { + return err + } + answersPage, err := app.Account().Everything().Checkins(cmd.Context(), sdkPage) + if err != nil { + return convertSDKError(err) + } + + answers := answersPage.Recordings + fetched := len(answers) + if limit > 0 && fetched > limit { + answers = answers[:limit] + } + + respOpts := accountWideRespOpts(len(answers), "check-in answer", "check-in answers", answersPage.Meta, "--all", limit > 0) + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(answers))) + if len(answers) < fetched { + respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( + "Showing %d of %d fetched check-in answers (--limit); drop --limit for all of them", len(answers), fetched))) + } + respOpts = append(respOpts, output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "answer", + Cmd: "basecamp checkins answer <id>", + Description: "View answer details", + }, + output.Breadcrumb{ + Action: "answers", + Cmd: "basecamp checkins answers <question_id>", + Description: "View one question's answers", + }, + )) + + return app.OK(answers, respOpts...) +} + func newCheckinsAnswerCmd(project *string) *cobra.Command { cmd := &cobra.Command{ Use: "answer <id|url>", diff --git a/internal/commands/checkins_test.go b/internal/commands/checkins_test.go index fadc5c3e4..16ff220b6 100644 --- a/internal/commands/checkins_test.go +++ b/internal/commands/checkins_test.go @@ -1,6 +1,7 @@ package commands import ( + "bytes" "encoding/json" "fmt" "io" @@ -9,8 +10,11 @@ import ( "testing" "time" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" ) type mockCheckinsAnswersByPersonTransport struct { @@ -66,7 +70,8 @@ func TestCheckinsAnswersByPersonFlag(t *testing.T) { app.Config.ProjectID = "123" project := "" - cmd := newCheckinsAnswersCmd(&project) + questionnaire := "" + cmd := newCheckinsAnswersCmd(&project, &questionnaire) err := executeCommand(cmd, app, "789", "--by", "Alice Smith") require.NoError(t, err) @@ -84,7 +89,8 @@ func TestCheckinsAnswersByBlankValue(t *testing.T) { app.Config.ProjectID = "123" project := "" - cmd := newCheckinsAnswersCmd(&project) + questionnaire := "" + cmd := newCheckinsAnswersCmd(&project, &questionnaire) err := executeCommand(cmd, app, "789", "--by", blank) require.Error(t, err) @@ -272,3 +278,220 @@ func TestCheckinsAnswerCreatePreservesExplicitDate(t *testing.T) { require.NotNil(t, transport.recordedBody) assert.Equal(t, "2026-03-25", transport.recordedBody["group_on"]) } + +// Account-wide check-in answers. +// +// `checkins answers` lists the children of one question, so a project alone +// cannot select a listing. Dropping the question ID lists every project's +// answers through the account-wide aggregate instead. + +const checkinsAccountWidePath = "/99999/checkins.json" + +const checkinsAccountWideBody = `[ + {"id":1,"title":"Monday","type":"Question::Answer","bucket":{"id":123,"name":"Test Project"}}, + {"id":2,"title":"Tuesday","type":"Question::Answer","bucket":{"id":456,"name":"Other Project"}} +]` + +func checkinsAccountWideRoute() stubRoute { + return stubRoute{ + method: http.MethodGet, + path: checkinsAccountWidePath, + status: http.StatusOK, + body: checkinsAccountWideBody, + } +} + +func checkinsQuestionAnswersRoute() stubRoute { + return stubRoute{ + method: http.MethodGet, + path: "/99999/questions/789/answers.json", + status: http.StatusOK, + body: `[{"id":11,"content":"<p>done</p>"}]`, + } +} + +// newCheckinsAnswersTestCmd builds the answers command with its own copies of +// the group's persistent flag targets. +func newCheckinsAnswersTestCmd() *cobra.Command { + project := "" + questionnaire := "" + return newCheckinsAnswersCmd(&project, &questionnaire) +} + +// runCheckinsAnswersAccountWideCmd runs the answers command against a stub +// serving the account-wide aggregate. +func runCheckinsAnswersAccountWideCmd(t *testing.T, args ...string) (*recordingTransport, error) { + t.Helper() + app, transport := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + return transport, executeRecordingCommand(newCheckinsAnswersTestCmd(), app, args...) +} + +func TestCheckinsAnswersQuestionScopedStillHitsQuestionEndpoint(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute(), checkinsQuestionAnswersRoute()) + app.Config.ProjectID = "123" + + require.NoError(t, executeRecordingCommand(newCheckinsAnswersTestCmd(), app, "789")) + assert.Equal(t, "/99999/questions/789/answers.json", transport.last(t).Path) +} + +func TestCheckinsAnswersWithoutQuestionListsAccountWide(t *testing.T) { + transport, err := runCheckinsAnswersAccountWideCmd(t) + require.NoError(t, err) + + last := transport.last(t) + assert.Equal(t, checkinsAccountWidePath, last.Path) + assert.Empty(t, last.Query, "the default follows every page, which sends no page param") +} + +// A configured project cannot scope a question's answers, so it is ignored +// rather than turned into an error or a project lookup. +func TestCheckinsAnswersIgnoresConfiguredProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + app.Config.ProjectID = "123" + + require.NoError(t, executeRecordingCommand(newCheckinsAnswersTestCmd(), app)) + + for _, req := range transport.recorded() { + assert.NotEqual(t, "/99999/projects.json", req.Path, "must not resolve the configured project") + } + assert.Equal(t, checkinsAccountWidePath, transport.last(t).Path) +} + +func TestCheckinsAnswersAllProjectsOverridesConfiguredProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + app.Config.ProjectID = "123" + + require.NoError(t, executeRecordingCommand(newCheckinsAnswersTestCmd(), app, "--all-projects")) + assert.Equal(t, checkinsAccountWidePath, transport.last(t).Path) +} + +func TestCheckinsAnswersExplicitProjectWithoutQuestionIsUsage(t *testing.T) { + assertUsage := func(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + assert.Contains(t, err.Error(), "A project alone cannot select check-in answers") + } + + t.Run("group --in", func(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + assertUsage(t, executeRecordingCommand(NewCheckinsCmd(), app, "answers", "--in", "123")) + }) + + t.Run("group --project", func(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + assertUsage(t, executeRecordingCommand(NewCheckinsCmd(), app, "answers", "--project", "123")) + }) + + // The root-level form lands in app.Flags.Project, not cmd.Flags().Changed. + t.Run("root --project", func(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + app.Flags.Project = "123" + assertUsage(t, executeRecordingCommand(newCheckinsAnswersTestCmd(), app)) + }) +} + +func TestCheckinsAnswersAllProjectsConflicts(t *testing.T) { + t.Run("with a question ID", func(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + err := executeRecordingCommand(newCheckinsAnswersTestCmd(), app, "789", "--all-projects") + require.Error(t, err) + assert.Contains(t, err.Error(), "--all-projects cannot be combined with a question ID") + }) + + t.Run("with an explicit project", func(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + err := executeRecordingCommand(NewCheckinsCmd(), app, "answers", "--in", "123", "--all-projects") + require.Error(t, err) + assert.Contains(t, err.Error(), "--all-projects cannot be combined with --project") + }) + + t.Run("with a root-level project", func(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + app.Flags.Project = "123" + err := executeRecordingCommand(newCheckinsAnswersTestCmd(), app, "--all-projects") + require.Error(t, err) + assert.Contains(t, err.Error(), "--all-projects cannot be combined with --project") + }) +} + +func TestCheckinsAnswersAccountWideRejectsBy(t *testing.T) { + transport, err := runCheckinsAnswersAccountWideCmd(t, "--by", "me") + require.Error(t, err) + assert.Contains(t, err.Error(), "--by has no account-wide equivalent") + assert.Empty(t, transport.recorded(), "must not call the aggregate endpoint") +} + +// --questionnaire is a persistent flag on the group, so it only reaches the +// answers command through the parent. +func TestCheckinsAnswersAccountWideRejectsQuestionnaire(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + + err := executeRecordingCommand(NewCheckinsCmd(), app, "answers", "--questionnaire", "555") + require.Error(t, err) + assert.Contains(t, err.Error(), "--questionnaire names a check-in container inside one project") + assert.Empty(t, transport.recorded(), "must not call the aggregate endpoint") +} + +func TestCheckinsAnswersAccountWidePagination(t *testing.T) { + t.Run("--page N asks for that page", func(t *testing.T) { + transport, err := runCheckinsAnswersAccountWideCmd(t, "--page", "3") + require.NoError(t, err) + assert.Equal(t, "page=3", transport.last(t).Query) + }) + + t.Run("--all follows every page", func(t *testing.T) { + transport, err := runCheckinsAnswersAccountWideCmd(t, "--all") + require.NoError(t, err) + assert.Empty(t, transport.last(t).Query) + }) + + t.Run("explicit --page 0 is usage", func(t *testing.T) { + _, err := runCheckinsAnswersAccountWideCmd(t, "--page", "0") + require.Error(t, err) + assert.Contains(t, err.Error(), "--page 0 is not a page") + }) + + t.Run("negative --page is usage", func(t *testing.T) { + _, err := runCheckinsAnswersAccountWideCmd(t, "--page", "-1") + require.Error(t, err) + assert.Contains(t, err.Error(), "--page cannot be negative") + }) + + t.Run("negative --limit is usage", func(t *testing.T) { + _, err := runCheckinsAnswersAccountWideCmd(t, "--limit", "-1") + require.Error(t, err) + assert.Contains(t, err.Error(), "--limit cannot be negative") + }) +} + +func TestCheckinsAnswersAccountWideLimitTruncatesWithNotice(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf}) + + require.NoError(t, executeRecordingCommand(newCheckinsAnswersTestCmd(), app, "--limit", "1")) + + var resp struct { + Data []map[string]any `json:"data"` + Summary string `json:"summary"` + Notice string `json:"notice"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &resp)) + assert.Len(t, resp.Data, 1) + assert.Equal(t, "1 check-in answer across all projects", resp.Summary) + assert.Contains(t, resp.Notice, "Showing 1 of 2 fetched check-in answers") +} + +// []Recording is what `recordings list` already hands the styled renderer, so +// the account-wide payload needs no format-dependent flattening. +func TestCheckinsAnswersAccountWideStyledRendersRecordings(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute(), checkinsAccountWideRoute()) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: buf}) + + require.NoError(t, executeRecordingCommand(newCheckinsAnswersTestCmd(), app)) + + rendered := buf.String() + assert.Contains(t, rendered, "Monday") + assert.Contains(t, rendered, "Tuesday") +} diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 684c9eee1..1172187ce 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -41,7 +41,7 @@ func NewCommentsCmd() *cobra.Command { cmd.PersistentFlags().StringVar(&project, "in", "", "Project ID (alias for --project)") cmd.AddCommand( - newCommentsListCmd(), + newCommentsListCmd(&project), newCommentsShowCmd(), newCommentsThreadCmd(), newCommentsCreateCmd(), @@ -54,45 +54,91 @@ func NewCommentsCmd() *cobra.Command { return cmd } -func newCommentsListCmd() *cobra.Command { +func newCommentsListCmd(project *string) *cobra.Command { var limit, page int - var all bool + var all, allProjects bool cmd := &cobra.Command{ - Use: "list <id|url>", - Short: "List comments on an item", - Long: "List all comments on an item.", + Use: "list [id|url]", + Short: "List comments on an item, or across every project", + Long: `List all comments on an item. + +Without an item, lists every comment across all accessible projects, +newest first. Comments hang off a single item rather than off a project, +so a project cannot narrow that listing: a configured project is ignored +and an explicit --project asks for an item instead. Pass --all-projects to +state the account-wide intent outright.`, + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - if len(args) == 0 { - return missingArg(cmd, "<id|url>") + recordingArg := "" + if len(args) > 0 { + recordingArg = args[0] } - return runCommentsList(cmd, args[0], limit, page, all) + return runCommentsList(cmd, recordingArg, *project, limit, page, all, allProjects) }, } cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of comments to fetch (0 = default 100)") cmd.Flags().BoolVar(&all, "all", false, "Fetch all comments (no limit)") cmd.Flags().IntVar(&page, "page", 0, "Fetch a single page (use --all for everything)") + cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List comments across every project instead of one item") return cmd } -func runCommentsList(cmd *cobra.Command, recordingID string, limit, page int, all bool) error { +// runCommentsList picks the scope before validating against it. Comments are +// per-item, so a project alone cannot produce a listing and the usual +// flag > config > prompt precedence does not apply: only an item ID scopes the +// item feed, and everything else lands on the account-wide one. +func runCommentsList(cmd *cobra.Command, recordingArg, project string, limit, page int, all, allProjects bool) error { app := appctx.FromContext(cmd.Context()) - // Validate flag combinations + // Combinations that are wrong under either scope. if all && limit > 0 { return output.ErrUsage("--all and --limit are mutually exclusive") } if page > 0 && (all || limit > 0) { return output.ErrUsage("--page cannot be combined with --all or --limit") } + + // --project/-p/--in is explicit whether it is given after the group noun + // (bound to project here) or at the root, where it lands on app.Flags + // instead — so cmd.Flags().Changed would miss half the forms. + explicitProject := project != "" || app.Flags.Project != "" + + switch { + case allProjects && recordingArg != "": + return output.ErrUsageHint( + "--all-projects cannot be combined with an item ID", + "Drop the ID to list every comment in the account, or drop --all-projects to list one item's comments.") + case allProjects && explicitProject: + return output.ErrUsageHint( + "--all-projects cannot be combined with --project", + "Account-wide comments span every project. Drop --project, or drop --all-projects and pass an item ID.") + case allProjects: + return runCommentsListAccountWide(cmd, app, limit, page, all) + case recordingArg != "": + return runCommentsListForItem(cmd, app, recordingArg, limit, page, all) + case explicitProject: + return output.ErrUsageHint( + "--project cannot scope a comment listing; an item ID can", + "Comments belong to one item: basecamp comments list <id|url>. "+ + "To list every comment in the account, drop --project and pass --all-projects.") + default: + // No item and no explicit project. A configured project is not a scope + // this listing could honor even if we wanted to, so it is ignored rather + // than turned into an error, and the account-wide feed answers instead. + return runCommentsListAccountWide(cmd, app, limit, page, all) + } +} + +func runCommentsListForItem(cmd *cobra.Command, app *appctx.App, recordingArg string, limit, page int, all bool) error { if page > 1 { return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") } // Extract recording ID from URL if provided - recordingID = extractID(recordingID) + recordingID := extractID(recordingArg) if err := ensureAccount(cmd, app); err != nil { return err @@ -146,6 +192,120 @@ func runCommentsList(cmd *cobra.Command, recordingID string, limit, page int, al return app.OK(comments, respOpts...) } +// defaultAccountWideCommentLimit caps the account-wide feed at the same 100 +// comments `comments list <id>` defaults to. Dropping the item must not +// silently promote a bounded command into a full-account crawl. +const defaultAccountWideCommentLimit = 100 + +// runCommentsListAccountWide lists every comment in the account, newest first. +// The payload is []Recording, which machine formats get raw. Human-facing +// output gets flattened rows instead: the generic renderers skip the nested +// bucket by name, and on a cross-project listing the project is the column +// that makes a row worth reading. +func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page int, all bool) error { + // A todolist names a container inside one project, so it cannot narrow an + // account-wide feed any more than a project can. Reject the explicit flag + // rather than accept and drop it; an ambient configured todolist is ignored + // on the same grounds as an ambient configured project. + if app.Flags.Todolist != "" { + return output.ErrUsageHint( + "--todolist cannot scope an account-wide comment listing", + "Drop --todolist, or pass an item ID to list that item's comments.") + } + if limit < 0 { + return output.ErrUsage("--limit must be a positive number") + } + // --page 0 is Cobra's "unset" but the SDK's "follow every page", so an + // explicit 0 would hand back a full-account crawl nobody asked for. --all is + // the spelling for that. + if cmd.Flags().Changed("page") && page < 1 { + return output.ErrUsage("--page must be a positive page number; use --all to fetch every page") + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + + var ( + comments []basecamp.Recording + meta basecamp.ListMeta + ) + switch { + case all || page > 0: + // accountWidePage maps --all onto page 0 ("follow the Link header") and + // otherwise passes the requested page straight through: the aggregate + // accepts any positive page, unlike the item feed. + sdkPage, err := accountWidePage(page, all) + if err != nil { + return err + } + result, err := app.Account().Everything().Comments(cmd.Context(), sdkPage) + if err != nil { + return convertSDKError(err) + } + comments, meta = result.Recordings, result.Meta + default: + effectiveLimit := defaultAccountWideCommentLimit + if limit > 0 { + effectiveLimit = limit + } + var err error + comments, meta, err = fetchAccountWideComments(cmd, app, effectiveLimit) + if err != nil { + return err + } + } + + respOpts := append(accountWideRespOpts(len(comments), "comment", "comments", meta, "--all", limit > 0), + output.WithDisplayData(flattenAccountWideRecordings(comments)), + output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "show", + Cmd: "basecamp comments show <id>", + Description: "Show comment", + }, + output.Breadcrumb{ + Action: "thread", + Cmd: "basecamp comments thread <id>", + Description: "Show a comment with its discussion", + }, + )) + + return app.OK(comments, respOpts...) +} + +// fetchAccountWideComments collects up to limit comments by walking positive +// pages, stopping as soon as the limit is met or a page comes back empty. +// Asking for page 0 would follow the Link header to the end of the account +// before truncating — correct, but it downloads every comment to keep 100. +// Each iteration adds at least one comment, so the walk always terminates. +func fetchAccountWideComments(cmd *cobra.Command, app *appctx.App, limit int) ([]basecamp.Recording, basecamp.ListMeta, error) { + var ( + comments []basecamp.Recording + meta basecamp.ListMeta + ) + for page := int32(1); len(comments) < limit; page++ { + result, err := app.Account().Everything().Comments(cmd.Context(), page) + if err != nil { + return nil, meta, convertSDKError(err) + } + if page == 1 { + // X-Total-Count is the account-wide total on every page; take it + // from the first so the truncation notice is honest about how much + // this walk left behind. + meta = result.Meta + } + if len(result.Recordings) == 0 { + break + } + comments = append(comments, result.Recordings...) + } + if len(comments) > limit { + comments = comments[:limit] + } + return comments, meta, nil +} + func newCommentsShowCmd() *cobra.Command { cmd := &cobra.Command{ Use: "show <id|url>", diff --git a/internal/commands/comment_test.go b/internal/commands/comment_test.go index cf7511c8a..a44f8c4f5 100644 --- a/internal/commands/comment_test.go +++ b/internal/commands/comment_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "os" @@ -478,3 +479,279 @@ func TestCommentsShowFlagAccountEchoedInBreadcrumb(t *testing.T) { } assert.Contains(t, replyCmd, "--account 99999") } + +// --- Account-wide comment listing ------------------------------------------- + +// accountWideCommentsRoute serves n comments on the account-wide feed. The stub +// matches on path only, so every page of a walk sees the same body — enough to +// prove the walk and the truncation without pretending to be a real cursor. +func accountWideCommentsRoute(n int) stubRoute { + items := make([]string, 0, n) + for i := 1; i <= n; i++ { + items = append(items, fmt.Sprintf( + `{"id":%d,"type":"Comment","title":"Re: thing %d","content":"<p>c%d</p>",`+ + `"bucket":{"id":7,"name":"Test Project","type":"Project"}}`, 1000+i, i, i)) + } + return stubRoute{ + method: http.MethodGet, + path: "/99999/comments.json", + status: http.StatusOK, + body: "[" + strings.Join(items, ",") + "]", + } +} + +func runCommentsListCmd(t *testing.T, app *appctx.App, args ...string) error { + t.Helper() + return executeRecordingCommand(NewCommentsCmd(), app, append([]string{"list"}, args...)...) +} + +type commentsListEnvelope struct { + Data []map[string]any `json:"data"` + Summary string `json:"summary"` + Notice string `json:"notice"` +} + +// runCommentsListJSON runs `comments list` and returns the JSON envelope. +func runCommentsListJSON(t *testing.T, app *appctx.App, args ...string) commentsListEnvelope { + t.Helper() + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf}) + require.NoError(t, runCommentsListCmd(t, app, args...)) + + var env commentsListEnvelope + require.NoError(t, json.Unmarshal(buf.Bytes(), &env)) + return env +} + +// assertCommentsListUsage asserts the invocation is rejected as an actionable +// usage error before any request goes out. +func assertCommentsListUsage(t *testing.T, app *appctx.App, transport *recordingTransport, wantMsg string, args ...string) { + t.Helper() + outErr := assertCommentsListUsageCode(t, app, transport, wantMsg, args...) + assert.NotEmpty(t, outErr.Hint, "a rejected flag must say what to do instead") +} + +// assertCommentsListUsageCode is assertCommentsListUsage without the hint +// requirement, for the terse pre-existing usage errors. +func assertCommentsListUsageCode(t *testing.T, app *appctx.App, transport *recordingTransport, wantMsg string, args ...string) *output.Error { + t.Helper() + err := runCommentsListCmd(t, app, args...) + require.Error(t, err) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T: %v", err, err) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Message, wantMsg) + assert.Empty(t, transport.recorded(), "a rejected invocation must not reach the API") + return outErr +} + +// assertCommentsNoProjectLookup proves ensureProject never ran: it resolves through the +// project list, so a single /projects.json fetch would give it away. +func assertCommentsNoProjectLookup(t *testing.T, transport *recordingTransport) { + t.Helper() + for _, req := range transport.recorded() { + assert.NotContains(t, req.Path, "/projects.json", "account-wide listing must not resolve a project") + } +} + +// TestCommentsListItemScopedUnchanged pins the item-scoped path: an ID still +// reaches the per-recording endpoint, and it still permits only page 1. +func TestCommentsListItemScopedUnchanged(t *testing.T) { + itemRoute := stubRoute{ + method: http.MethodGet, + path: "/99999/recordings/789/comments.json", + status: http.StatusOK, + body: `[{"id":1,"content":"<p>hi</p>"}]`, + } + + t.Run("bare ID", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, itemRoute) + require.NoError(t, runCommentsListCmd(t, app, "789")) + assert.Equal(t, "/99999/recordings/789/comments.json", transport.last(t).Path) + }) + + t.Run("a configured project does not divert it", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, itemRoute, projectsRoute()) + app.Config.ProjectID = "123" + require.NoError(t, runCommentsListCmd(t, app, "789")) + assert.Equal(t, "/99999/recordings/789/comments.json", transport.last(t).Path) + }) + + t.Run("still only page 1", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, itemRoute) + assertCommentsListUsageCode(t, app, transport, "only --page 1 is supported", "789", "--page", "2") + }) +} + +// TestCommentsListReachesAccountWide covers the three dispatch rows that end +// account-wide: nothing in scope, --all-projects over a configured project, and +// a configured project alone — which cannot scope a per-item listing and is +// therefore ignored rather than turned into an error. +func TestCommentsListReachesAccountWide(t *testing.T) { + t.Run("no project anywhere", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideCommentsRoute(120)) + require.NoError(t, runCommentsListCmd(t, app)) + assert.Equal(t, "/99999/comments.json", transport.last(t).Path) + assertCommentsNoProjectLookup(t, transport) + }) + + t.Run("--all-projects overrides a configured project", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideCommentsRoute(120), projectsRoute()) + app.Config.ProjectID = "123" + require.NoError(t, runCommentsListCmd(t, app, "--all-projects")) + assert.Equal(t, "/99999/comments.json", transport.last(t).Path) + assertCommentsNoProjectLookup(t, transport) + }) + + t.Run("a configured project alone is ignored", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideCommentsRoute(120), projectsRoute()) + app.Config.ProjectID = "123" + app.Config.TodolistID = "456" + require.NoError(t, runCommentsListCmd(t, app)) + assert.Equal(t, "/99999/comments.json", transport.last(t).Path) + assertCommentsNoProjectLookup(t, transport) + }) +} + +// TestCommentsListRejectsUnhonorableScopeFlags covers every way a scope that +// cannot narrow an account-wide comment feed can arrive: after the group noun, +// by the --in alias, at the root, and as a todolist. +func TestCommentsListRejectsUnhonorableScopeFlags(t *testing.T) { + const cannotScope = "cannot scope a comment listing" + + t.Run("--project without an ID asks for an ID", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute()) + assertCommentsListUsage(t, app, transport, cannotScope, "--project", "123") + }) + + t.Run("-p without an ID asks for an ID", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute()) + assertCommentsListUsage(t, app, transport, cannotScope, "-p", "123") + }) + + t.Run("--in alias without an ID asks for an ID", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute()) + assertCommentsListUsage(t, app, transport, cannotScope, "--in", "123") + }) + + t.Run("root-level --project without an ID asks for an ID", func(t *testing.T) { + // The root form never sets cmd.Flags().Changed — it lands here instead. + app, transport := setupRecordingTestApp(t, projectsRoute()) + app.Flags.Project = "123" + assertCommentsListUsage(t, app, transport, cannotScope) + }) + + t.Run("--all-projects conflicts with an explicit project", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute()) + assertCommentsListUsage(t, app, transport, + "--all-projects cannot be combined with --project", "--all-projects", "--project", "123") + }) + + t.Run("--all-projects conflicts with the root-level project", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute()) + app.Flags.Project = "123" + assertCommentsListUsage(t, app, transport, + "--all-projects cannot be combined with --project", "--all-projects") + }) + + t.Run("--all-projects conflicts with an item ID", func(t *testing.T) { + app, transport := setupRecordingTestApp(t) + assertCommentsListUsage(t, app, transport, + "--all-projects cannot be combined with an item ID", "--all-projects", "789") + }) + + t.Run("root-level --todolist is rejected by name", func(t *testing.T) { + app, transport := setupRecordingTestApp(t) + app.Flags.Todolist = "456" + assertCommentsListUsage(t, app, transport, "--todolist cannot scope an account-wide comment listing") + }) + + t.Run("root-level --todolist is rejected under --all-projects too", func(t *testing.T) { + app, transport := setupRecordingTestApp(t) + app.Flags.Todolist = "456" + assertCommentsListUsage(t, app, transport, + "--todolist cannot scope an account-wide comment listing", "--all-projects") + }) +} + +// TestCommentsListAccountWidePagination pins the row of the pagination contract +// this command owns: default cap 100, --all follows every page, and any +// positive --page is accepted where the item feed permits only page 1. +func TestCommentsListAccountWidePagination(t *testing.T) { + t.Run("default caps at 100", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideCommentsRoute(120)) + env := runCommentsListJSON(t, app) + assert.Len(t, env.Data, 100) + assert.Equal(t, "100 comments across all projects", env.Summary) + + reqs := transport.recorded() + require.Len(t, reqs, 1, "one full page already covers the cap") + assert.Equal(t, "page=1", reqs[0].Query) + }) + + t.Run("--limit walks positive pages until it is met", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideCommentsRoute(3)) + env := runCommentsListJSON(t, app, "--limit", "5") + assert.Len(t, env.Data, 5) + assert.Equal(t, "5 comments across all projects", env.Summary) + + reqs := transport.recorded() + require.Len(t, reqs, 2, "three per page, so five needs two pages") + assert.Equal(t, "page=1", reqs[0].Query) + assert.Equal(t, "page=2", reqs[1].Query) + }) + + t.Run("-n is the same flag", func(t *testing.T) { + app, _ := setupRecordingTestApp(t, accountWideCommentsRoute(9)) + env := runCommentsListJSON(t, app, "-n", "4") + assert.Len(t, env.Data, 4) + }) + + t.Run("--all follows every page", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideCommentsRoute(3)) + env := runCommentsListJSON(t, app, "--all") + assert.Len(t, env.Data, 3) + + reqs := transport.recorded() + require.Len(t, reqs, 1) + assert.Empty(t, reqs[0].Query, "page 0 is spelled as no page parameter") + }) + + t.Run("any positive --page is accepted", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideCommentsRoute(3)) + env := runCommentsListJSON(t, app, "--page", "7") + assert.Len(t, env.Data, 3) + + reqs := transport.recorded() + require.Len(t, reqs, 1) + assert.Equal(t, "page=7", reqs[0].Query) + }) + + t.Run("explicit --page 0 is rejected", func(t *testing.T) { + app, transport := setupRecordingTestApp(t) + assertCommentsListUsageCode(t, app, transport, "--page must be a positive page number", "--page", "0") + }) + + t.Run("negative --page is rejected", func(t *testing.T) { + app, transport := setupRecordingTestApp(t) + assertCommentsListUsageCode(t, app, transport, "--page must be a positive page number", "--page=-1") + }) + + t.Run("negative --limit is rejected", func(t *testing.T) { + app, transport := setupRecordingTestApp(t) + assertCommentsListUsageCode(t, app, transport, "--limit must be a positive number", "--limit=-1") + }) +} + +// TestCommentsListAccountWideRendersRecordingsAsIs proves the []Recording +// payload needs no flattening branch: the styled renderer takes it unchanged, +// exactly as `recordings list` already hands it over. +func TestCommentsListAccountWideRendersRecordingsAsIs(t *testing.T) { + app, _ := setupRecordingTestApp(t, accountWideCommentsRoute(3)) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: buf}) + + require.NoError(t, runCommentsListCmd(t, app)) + assert.Contains(t, buf.String(), "Re: thing 1") +} diff --git a/internal/commands/files.go b/internal/commands/files.go index 281734143..910f9f571 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -6,6 +6,7 @@ import ( "net/url" "os" "path/filepath" + "slices" "strconv" "strings" @@ -96,26 +97,115 @@ func NewUploadsCmd() *cobra.Command { return cmd } +// filesAccountWideKinds is the accepted --kind vocabulary, in the order the +// usage error lists it. It mirrors EverythingFilesOptions.Kind. +var filesAccountWideKinds = []string{"all", "images", "pdfs", "documents", "videos"} + +const filesKindDocuments = "documents" + +// The three group spellings that reach this leaf. `vaults`/`folders` and +// `docs`/`documents` are built from NewFilesCmd, so the leaf is shared and the +// group's own name is the only thing that says which listing the user asked +// for. +const ( + filesGroupFiles = iota + filesGroupFolders + filesGroupDocuments +) + +// filesGroupSpelling reports which group noun invoked this list command. Name() +// returns the group's canonical Use rather than the alias typed, so `folders` +// and `vault` both resolve through `vaults`. +func filesGroupSpelling(cmd *cobra.Command) int { + parent := cmd.Parent() + if parent == nil { + return filesGroupFiles + } + switch parent.Name() { + case "vaults": + return filesGroupFolders + case "docs": + return filesGroupDocuments + default: + return filesGroupFiles + } +} + func newFilesListCmd(project, vaultID *string) *cobra.Command { - return &cobra.Command{ + var allProjects bool + var kind string + var people []string + + cmd := &cobra.Command{ Use: "list", Short: "List all items in a folder", - Long: "List all folders, documents, and uploads in a folder.", + Long: `List all folders, documents, and uploads in a folder. + +With no project in scope, lists every file across all accessible projects +instead of asking which project to use. Pass --all-projects to list +account-wide even when a project is configured. + +--kind and --person filter the account-wide listing only; the project-scoped +folder listing has no equivalent for them.`, + Example: ` basecamp files list --in my-project + basecamp files list --all-projects --kind images + basecamp files list --all-projects --person me`, RunE: func(cmd *cobra.Command, args []string) error { - return runFilesList(cmd, *project, *vaultID) + return runFilesList(cmd, *project, *vaultID, allProjects, kind, people) }, } + + cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List files across every accessible project") + cmd.Flags().StringVar(&kind, "kind", "", fmt.Sprintf("Account-wide only: filter by file kind (%s)", strings.Join(filesAccountWideKinds, ", "))) + cmd.Flags().StringArrayVar(&people, "person", nil, "Account-wide only: filter by creator (name, email, ID, or \"me\"; repeatable)") + + return cmd } -func runFilesList(cmd *cobra.Command, project, vaultID string) error { +func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, kind string, people []string) error { app := appctx.FromContext(cmd.Context()) + // Scope is settled before any scope-specific validation, and an explicit + // project counts whether it arrived after the group noun or at the root. + explicitProject := project != "" || app.Flags.Project != "" + if allProjects && explicitProject { + return output.ErrUsageHint( + "--all-projects conflicts with --project/--in", + "Drop --all-projects to list that project's folder, or drop --project/--in to list every project.", + ) + } + // Resolve account (enables interactive prompt if needed) if err := ensureAccount(cmd, app); err != nil { return err } - // Resolve project from CLI flags and config, with interactive fallback + // --all-projects overrides a configured project; otherwise a project from + // any layer keeps the folder listing. With nothing to scope to, list + // account-wide rather than prompting. + if allProjects || !projectKnown(app, project) { + return runFilesListAccountWide(cmd, app, vaultID, kind, people) + } + + // The account-wide filters have no project-scoped equivalent, so a project + // in scope rejects them instead of quietly dropping them. Detection is by + // flag presence, not by value: --kind "" is still the user asking for a + // filter, and dropping it because the value is empty is the same silent + // ignore as dropping it because a project is set. + if cmd.Flags().Changed("kind") { + return output.ErrUsageHint( + "--kind only applies to the account-wide file listing", + "A project's folder listing has no kind filter. Drop --project/--in (and pass --all-projects if a project is configured) to filter across every project.", + ) + } + if len(people) > 0 { + return output.ErrUsageHint( + "--person only applies to the account-wide file listing", + "A project's folder listing has no creator filter. Drop --project/--in (and pass --all-projects if a project is configured) to filter across every project.", + ) + } + + // Resolve project from CLI flags and config projectID := project if projectID == "" { projectID = app.Flags.Project @@ -124,14 +214,6 @@ func runFilesList(cmd *cobra.Command, project, vaultID string) error { projectID = app.Config.ProjectID } - // If no project specified, try interactive resolution - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) if err != nil { return err @@ -235,6 +317,141 @@ func runFilesList(cmd *cobra.Command, project, vaultID string) error { return app.OK(items, respOpts...) } +// runFilesListAccountWide lists every file across all accessible projects. +// The bare files listing has no --limit/--page/--all project-scoped and gains +// none here, so it always follows the Link header across every page. +func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind string, people []string) error { + if err := rejectAccountWideTodolist(app, "file"); err != nil { + return err + } + + // vaults/folders and docs/documents are the same command under different + // names, but the account-wide feed is not the same listing. It carries + // Uploads, Documents, and Attachments — no folder variant at all — so the + // folder spellings would return a listing with none of the thing they are + // named for. Answer honestly instead, and let the document spellings mean + // what they say by pinning the kind they already name. + switch filesGroupSpelling(cmd) { + case filesGroupFolders: + return output.ErrUsageHint( + "folders have no account-wide listing", + "Folders exist inside one project: basecamp folders list --in <project>. For files across every project: basecamp files list --all-projects") + case filesGroupDocuments: + if cmd.Flags().Changed("kind") { + return output.ErrUsageHint( + "--kind cannot narrow an account-wide document listing", + "This command already lists documents. For other kinds: basecamp files list --all-projects --kind <kind>") + } + kind = filesKindDocuments + } + // --vault/--folder names a folder inside one project. Account-wide has no + // such container, and ignoring the flag would hand back a listing of + // something else entirely. + if vaultID != "" { + return output.ErrUsageHint( + "--vault/--folder names a folder inside one project, which an account-wide listing has no equivalent for", + "Pass --project/--in to list that folder's contents, or drop --vault/--folder to list files across every project.", + ) + } + + opts, err := filesAccountWideOptions(cmd, app, kind, people) + if err != nil { + return err + } + + page, err := app.Account().Everything().Files(cmd.Context(), 0, opts) + if err != nil { + return convertSDKError(err) + } + + // EverythingFile is an all-pointer superset far too wide to render raw, and + // its bucket is nested, so every consumer but --json and --agent reads the + // flat rows. + respOpts := accountWideRespOpts(len(page.Files), "file", "files", page.Meta, "", false) + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideFiles(page.Files))) + respOpts = append(respOpts, output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "kind", + Cmd: "basecamp files list --all-projects --kind images", + Description: "Filter by file kind", + }, + output.Breadcrumb{ + Action: "project", + Cmd: "basecamp files list --in <project>", + Description: "List one project's folder", + }, + )) + + return app.OK(page.Files, respOpts...) +} + +// filesAccountWideOptions validates --kind and resolves --person into the +// creator IDs the files feed filters on. +func filesAccountWideOptions(cmd *cobra.Command, app *appctx.App, kind string, people []string) (*basecamp.EverythingFilesOptions, error) { + opts := &basecamp.EverythingFilesOptions{} + + // Presence, not value: --kind "" is still the user asking for a filter, and + // dropping it because the value is empty is a silent ignore. A kind the + // command pinned for itself (the docs spelling) arrives non-empty with the + // flag unchanged, so it applies without being re-validated as user input. + if cmd.Flags().Changed("kind") { + normalized := strings.ToLower(strings.TrimSpace(kind)) + if !slices.Contains(filesAccountWideKinds, normalized) { + return nil, output.ErrUsageHint( + fmt.Sprintf("Invalid --kind value %q", kind), + fmt.Sprintf("Use one of: %s", strings.Join(filesAccountWideKinds, ", ")), + ) + } + opts.Kind = normalized + } else if kind != "" { + opts.Kind = kind + } + + for _, person := range people { + id, err := resolvePersonRoleID(cmd.Context(), app, person, "Person") + if err != nil { + return nil, err + } + opts.PeopleIDs = append(opts.PeopleIDs, id) + } + + return opts, nil +} + +// flattenAccountWideFiles reduces the files feed to styled table rows. +// EverythingFile is an all-pointer superset over the Upload, Document, and +// Attachment variants, so every field is nil-checked and an absent one stays +// absent rather than rendering a fabricated zero. +func flattenAccountWideFiles(files []basecamp.EverythingFile) []map[string]any { + rows := make([]map[string]any, 0, len(files)) + for _, f := range files { + row := map[string]any{} + if f.ID != nil { + row["id"] = *f.ID + } + if f.Bucket != nil { + row["project"] = f.Bucket.Name + } + switch { + case f.Title != nil && *f.Title != "": + row["name"] = *f.Title + case f.Filename != nil: + row["name"] = *f.Filename + } + if f.Type != nil { + row["type"] = *f.Type + } + if f.ByteSize != nil { + row["size"] = humanSize(*f.ByteSize) + } + if f.CreatedAt != nil { + row["created"] = relativeTime(*f.CreatedAt) + } + rows = append(rows, row) + } + return rows +} + func newFoldersCmd(project, vaultID *string) *cobra.Command { var limit int var page int diff --git a/internal/commands/files_test.go b/internal/commands/files_test.go index 67e4e0083..4557e4bde 100644 --- a/internal/commands/files_test.go +++ b/internal/commands/files_test.go @@ -13,6 +13,7 @@ import ( "strings" "testing" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -911,3 +912,275 @@ func TestUploadShortcutNestedVaultVisibleToClientsRejected(t *testing.T) { require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) assert.Empty(t, transport.postPaths, "the shortcut must also stage no POSTs on rejection") } + +// --- files list: account-wide ----------------------------------------------- + +// filesAccountWideRoute serves the account-wide files feed. +func filesAccountWideRoute(body string) stubRoute { + return stubRoute{ + method: http.MethodGet, + path: "/99999/files.json", + status: http.StatusOK, + body: body, + } +} + +// filesProjectScopedRoutes serves everything the project-scoped folder listing +// touches: the dock lookup that finds the root vault, then its three children. +func filesProjectScopedRoutes() []stubRoute { + return []stubRoute{ + projectsRoute(), + {http.MethodGet, "/99999/projects/123.json", http.StatusOK, + `{"id":123,"name":"Test Project","dock":[{"id":555,"name":"vault","title":"Docs & Files","enabled":true}]}`}, + {http.MethodGet, "/99999/vaults/555", http.StatusOK, `{"id":555,"title":"Docs & Files"}`}, + {http.MethodGet, "/99999/vaults/555/vaults.json", http.StatusOK, `[]`}, + {http.MethodGet, "/99999/vaults/555/uploads.json", http.StatusOK, `[]`}, + {http.MethodGet, "/99999/vaults/555/documents.json", http.StatusOK, `[]`}, + } +} + +func runFilesListCmd(t *testing.T, app *appctx.App, args ...string) error { + t.Helper() + return executeRecordingCommand(NewFilesCmd(), app, append([]string{"list"}, args...)...) +} + +func requireFilesUsageError(t *testing.T, err error) *output.Error { + t.Helper() + require.Error(t, err) + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) + assert.Equal(t, output.CodeUsage, e.Code) + return e +} + +func filesRequestPaths(calls []recordedCall) []string { + paths := make([]string, 0, len(calls)) + for _, c := range calls { + paths = append(paths, c.Path) + } + return paths +} + +func TestFilesListWithConfiguredProjectStaysProjectScoped(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesProjectScopedRoutes()...) + app.Config.ProjectID = "123" + + require.NoError(t, runFilesListCmd(t, app)) + paths := filesRequestPaths(transport.recorded()) + assert.NotContains(t, paths, "/99999/files.json") + assert.Contains(t, paths, "/99999/vaults/555/documents.json") +} + +func TestFilesListWithoutProjectListsAccountWide(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + require.NoError(t, runFilesListCmd(t, app)) + assert.Equal(t, "/99999/files.json", transport.last(t).Path) +} + +func TestFilesListAllProjectsOverridesConfiguredProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + app.Config.ProjectID = "123" + + require.NoError(t, runFilesListCmd(t, app, "--all-projects")) + assert.Equal(t, "/99999/files.json", transport.last(t).Path) +} + +func TestFilesListAllProjectsConflictsWithExplicitProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--all-projects", "--in", "123")) + assert.Contains(t, e.Message, "--all-projects conflicts with --project/--in") + assert.Empty(t, transport.recorded(), "a scope conflict must not reach the network") +} + +func TestFilesListAllProjectsConflictsWithRootLevelProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + app.Flags.Project = "123" + + requireFilesUsageError(t, runFilesListCmd(t, app, "--all-projects")) + assert.Empty(t, transport.recorded()) +} + +func TestFilesListAccountWideRejectsVault(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--vault", "555")) + assert.Contains(t, e.Message, "--vault/--folder") + assert.Empty(t, filesRequestPaths(transport.recorded()), "a rejected scope-child flag lists nothing") +} + +func TestFilesListAccountWideRejectsFolderAlias(t *testing.T) { + app, _ := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--folder", "555")) + assert.Contains(t, e.Message, "--vault/--folder") +} + +func TestFilesListKindRejectedWithExplicitProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesProjectScopedRoutes()...) + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--in", "123", "--kind", "images")) + assert.Contains(t, e.Message, "--kind only applies to the account-wide file listing") + assert.Empty(t, filesRequestPaths(transport.recorded()), "a rejected filter must not list the project") +} + +func TestFilesListKindRejectedWithRootLevelProject(t *testing.T) { + app, _ := setupRecordingTestApp(t, filesProjectScopedRoutes()...) + app.Flags.Project = "123" + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--kind", "images")) + assert.Contains(t, e.Message, "--kind") +} + +func TestFilesListKindRejectedWithConfiguredProject(t *testing.T) { + app, _ := setupRecordingTestApp(t, filesProjectScopedRoutes()...) + app.Config.ProjectID = "123" + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--kind", "images")) + assert.Contains(t, e.Message, "--kind") +} + +func TestFilesListPersonRejectedWithConfiguredProject(t *testing.T) { + app, _ := setupRecordingTestApp(t, filesProjectScopedRoutes()...) + app.Config.ProjectID = "123" + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--person", "me")) + assert.Contains(t, e.Message, "--person only applies to the account-wide file listing") +} + +func TestFilesListInvalidKindListsAcceptedValues(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + e := requireFilesUsageError(t, runFilesListCmd(t, app, "--kind", "spreadsheets")) + assert.Contains(t, e.Message, "spreadsheets") + assert.Contains(t, e.Hint, "all, images, pdfs, documents, videos") + assert.Empty(t, filesRequestPaths(transport.recorded())) +} + +func TestFilesListKindReachesTheQuery(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + require.NoError(t, runFilesListCmd(t, app, "--kind", "IMAGES")) + assert.Contains(t, transport.last(t).Query, "kind=images") +} + +func TestFilesListPersonResolvesAndReachesTheQuery(t *testing.T) { + app, transport := setupRecordingTestApp(t, + filesAccountWideRoute(`[]`), + stubRoute{http.MethodGet, "/99999/people.json", http.StatusOK, + `[{"id":77,"name":"Ann Perkins"},{"id":88,"name":"Bo Diaz"}]`}, + ) + + require.NoError(t, runFilesListCmd(t, app, "--person", "Ann Perkins", "--person", "88")) + query := transport.last(t).Query + assert.Contains(t, query, "77") + assert.Contains(t, query, "88") +} + +func TestFilesListAccountWideFollowsEveryPage(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + require.NoError(t, runFilesListCmd(t, app)) + assert.NotContains(t, transport.last(t).Query, "page=", + "bare files list is pinned at every page, so it sends no page argument") +} + +func TestFilesListGainsNoPaginationFlags(t *testing.T) { + listCmd, _, err := NewFilesCmd().Find([]string{"list"}) + require.NoError(t, err) + + for _, name := range []string{"limit", "page", "all"} { + assert.Nil(t, listCmd.Flags().Lookup(name), "files list must not grow --%s", name) + } +} + +func TestFilesListMachineFormatKeepsRawPayload(t *testing.T) { + buf := &bytes.Buffer{} + app, _ := setupRecordingTestApp(t, filesAccountWideRoute(filesAccountWideFixture)) + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf}) + + require.NoError(t, runFilesListCmd(t, app)) + assert.Contains(t, buf.String(), "attachable_sgid", "machine output keeps the SDK payload") +} + +func TestFilesListStyledFormatFlattensRows(t *testing.T) { + buf := &bytes.Buffer{} + app, _ := setupRecordingTestApp(t, filesAccountWideRoute(filesAccountWideFixture)) + app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: buf}) + + require.NoError(t, runFilesListCmd(t, app)) + rendered := buf.String() + assert.NotContains(t, rendered, "attachable_sgid", "styled output renders flattened rows") + assert.Contains(t, rendered, "Test Project") + assert.Contains(t, rendered, "report.pdf") +} + +// filesAccountWideFixture carries one file per wire variant: a full upload, a +// document with no blob metadata, and a bare attachment with no title. +const filesAccountWideFixture = `[ + {"id":1,"title":"report.pdf","type":"Upload","byte_size":2048,"filename":"report.pdf","created_at":"2026-07-01T10:00:00.000Z","bucket":{"id":123,"name":"Test Project","type":"Project"}}, + {"id":2,"title":"Spec","type":"Document","content":"<div>hi</div>","bucket":{"id":123,"name":"Test Project","type":"Project"}}, + {"type":"Attachment","attachable_sgid":"sgid-abc","filename":"pasted.png","byte_size":17} +]` + +func TestFlattenAccountWideFilesNilChecksEveryField(t *testing.T) { + rows := flattenAccountWideFiles([]basecamp.EverythingFile{{}}) + + require.Len(t, rows, 1) + assert.Empty(t, rows[0], "an all-nil file must fabricate no columns") +} + +func TestFlattenAccountWideFilesFallsBackToFilename(t *testing.T) { + name := "pasted.png" + size := int64(17) + rows := flattenAccountWideFiles([]basecamp.EverythingFile{{Filename: &name, ByteSize: &size}}) + + require.Len(t, rows, 1) + assert.Equal(t, "pasted.png", rows[0]["name"]) + assert.Equal(t, "17b", rows[0]["size"]) +} + +// vaults/folders and docs/documents share files list's leaf, but the +// account-wide feed carries no folder variant, so the folder spellings must +// say so rather than return a folder-less listing under a folder name. +func TestFilesGroupSpellingsGetHonestAccountWideSemantics(t *testing.T) { + t.Run("folders refuse account-wide", func(t *testing.T) { + for _, args := range [][]string{{"list", "--all-projects"}, {"list"}} { + app, transport := setupRecordingTestApp(t) + err := executeRecordingCommand(NewVaultsCmd(), app, args...) + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T", err) + assert.Contains(t, e.Message, "folders have no account-wide listing") + assert.Empty(t, transport.recorded(), "must refuse before any request") + } + }) + + t.Run("docs pin the documents kind", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, stubRoute{ + method: http.MethodGet, path: "/99999/files.json", status: http.StatusOK, body: `[]`, + }) + require.NoError(t, executeRecordingCommand(NewDocsCmd(), app, "list", "--all-projects")) + assert.Contains(t, transport.last(t).Query, "kind=documents") + }) + + t.Run("docs reject a contradicting kind", func(t *testing.T) { + app, _ := setupRecordingTestApp(t) + err := executeRecordingCommand(NewDocsCmd(), app, "list", "--all-projects", "--kind", "images") + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T", err) + assert.Contains(t, e.Message, "--kind cannot narrow an account-wide document listing") + }) + + t.Run("files stays unfiltered", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, stubRoute{ + method: http.MethodGet, path: "/99999/files.json", status: http.StatusOK, body: `[]`, + }) + require.NoError(t, executeRecordingCommand(NewFilesCmd(), app, "list", "--all-projects")) + assert.NotContains(t, transport.last(t).Query, "kind=") + }) +} diff --git a/internal/commands/forwards.go b/internal/commands/forwards.go index 894601c3c..cce352b6b 100644 --- a/internal/commands/forwards.go +++ b/internal/commands/forwards.go @@ -51,26 +51,47 @@ func newForwardsListCmd(project, inboxID *string) *cobra.Command { var limit int var page int var all bool + var allProjects bool cmd := &cobra.Command{ Use: "list", Short: "List forwards in project inbox", - Long: "List all email forwards in the project inbox.", + Long: `List all email forwards in the project inbox. + +With no project in scope — none passed, none configured, or --all-projects to +override a configured one — lists forwards across every accessible project.`, RunE: func(cmd *cobra.Command, args []string) error { - return runForwardsList(cmd, *project, *inboxID, limit, page, all) + return runForwardsList(cmd, *project, *inboxID, limit, page, all, allProjects) }, } cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of forwards to fetch (0 = all)") cmd.Flags().BoolVar(&all, "all", false, "Fetch all forwards (no limit)") cmd.Flags().IntVar(&page, "page", 0, "Fetch a single page (use --all for everything)") + cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List forwards across every project") return cmd } -func runForwardsList(cmd *cobra.Command, project, inboxID string, limit, page int, all bool) error { +func runForwardsList(cmd *cobra.Command, project, inboxID string, limit, page int, all, allProjects bool) error { app := appctx.FromContext(cmd.Context()) + // Select the scope before validating against it: the account-wide endpoint + // serves any positive page, while the project-scoped one only serves the + // first. --all-projects overrides a configured project and conflicts with + // an explicit one, which arrives either after the group noun or at the + // root (basecamp --project X forwards list). + explicitProject := project != "" || app.Flags.Project != "" + if allProjects && explicitProject { + return output.ErrUsageHint( + "--all-projects conflicts with --project/--in", + "Drop one: --all-projects lists every project, --project lists one", + ) + } + if allProjects || !projectKnown(app, project) { + return runForwardsListEverywhere(cmd, app, inboxID, limit, page, all) + } + // Validate flag combinations if all && limit > 0 { return output.ErrUsage("--all and --limit are mutually exclusive") @@ -86,7 +107,9 @@ func runForwardsList(cmd *cobra.Command, project, inboxID string, limit, page in return err } - // Resolve project, with interactive fallback + // Resolve project. No interactive fallback: reaching here means projectKnown + // found one of these three sources set, and an empty scope listed + // account-wide instead of prompting. projectID := project if projectID == "" { projectID = app.Flags.Project @@ -94,12 +117,6 @@ func runForwardsList(cmd *cobra.Command, project, inboxID string, limit, page in if projectID == "" { projectID = app.Config.ProjectID } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) if err != nil { @@ -161,6 +178,83 @@ func runForwardsList(cmd *cobra.Command, project, inboxID string, limit, page in return app.OK(forwards, respOpts...) } +// runForwardsListEverywhere lists forwards across every accessible project via +// the account-wide aggregate endpoint. No project is in scope here, so every +// flag that names something inside one project is rejected rather than ignored. +func runForwardsListEverywhere(cmd *cobra.Command, app *appctx.App, inboxID string, limit, page int, all bool) error { + if err := rejectAccountWideTodolist(app, "forward"); err != nil { + return err + } + if inboxID != "" { + return output.ErrUsageHint( + "--inbox names an inbox inside one project and has no account-wide meaning", + "Pass --project/--in to list that project's inbox, or drop --inbox", + ) + } + if all && limit > 0 { + return output.ErrUsage("--all and --limit are mutually exclusive") + } + if page > 0 && (all || limit > 0) { + return output.ErrUsage("--page cannot be combined with --all or --limit") + } + if cmd.Flags().Changed("page") && page < 1 { + return output.ErrUsageHint( + "--page must be a positive page number", + "Omit --page, or pass --all, to follow every page", + ) + } + if limit < 0 { + return output.ErrUsage("--limit must be zero or positive") + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + + // The endpoint spells "follow every page" as page 0, which is where both + // --all and the default land: this command's project-scoped default is + // already "0 = all", so listing account-wide does not silently start + // capping. Only an explicit positive --page narrows to a single page. + var sdkPage int32 + if page > 0 { + var err error + if sdkPage, err = accountWidePage(page, all); err != nil { + return err + } + } + + result, err := app.Account().Everything().Forwards(cmd.Context(), sdkPage) + if err != nil { + return convertSDKError(err) + } + + // --limit truncates client-side; accountWideRespOpts reports the shortfall + // against the server's total so the trim is visible. + forwards := result.Recordings + if limit > 0 && len(forwards) > limit { + forwards = forwards[:limit] + } + + respOpts := accountWideRespOpts(len(forwards), "forward", "forwards", result.Meta, "--all", limit > 0) + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(forwards))) + respOpts = append(respOpts, + output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "show", + Cmd: "basecamp forwards show <id> --in <project>", + Description: "View a forward", + }, + output.Breadcrumb{ + Action: "list", + Cmd: "basecamp forwards list --in <project>", + Description: "List one project's inbox", + }, + ), + ) + + return app.OK(forwards, respOpts...) +} + func newForwardsShowCmd(project *string) *cobra.Command { var cf *commentFlags diff --git a/internal/commands/forwards_test.go b/internal/commands/forwards_test.go new file mode 100644 index 000000000..f0a0820b5 --- /dev/null +++ b/internal/commands/forwards_test.go @@ -0,0 +1,301 @@ +package commands + +import ( + "bytes" + "encoding/json" + "errors" + "io" + "net/http" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/names" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// forwardsAccountWidePath is the account-wide aggregate feed for the harness +// account; forwardsInboxPath is the project-scoped inbox listing. +const ( + forwardsAccountWidePath = "/99999/forwards.json" + forwardsInboxPath = "/99999/inboxes/555/forwards.json" +) + +// forwardsFeedBody renders n forwards as the account-wide feed serves them. +func forwardsFeedBody(n int) string { + items := make([]string, 0, n) + for i := 1; i <= n; i++ { + items = append(items, `{"id":`+strconv.Itoa(i)+`,"type":"Inbox::Forward","title":"Forward","subject":"Subject","status":"active","bucket":{"id":123,"name":"Test Project","type":"Project"},"creator":{"id":1,"name":"A"}}`) + } + return "[" + strings.Join(items, ",") + "]" +} + +func forwardsAccountWideRoute(n int) stubRoute { + return stubRoute{ + method: http.MethodGet, + path: forwardsAccountWidePath, + status: http.StatusOK, + body: forwardsFeedBody(n), + } +} + +func forwardsInboxRoute() stubRoute { + return stubRoute{ + method: http.MethodGet, + path: forwardsInboxPath, + status: http.StatusOK, + body: forwardsFeedBody(1), + } +} + +// requireForwardsUsageError asserts err is an actionable usage error rather +// than a silently accepted flag. +func requireForwardsUsageError(t *testing.T, err error) *output.Error { + t.Helper() + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeUsage, outErr.Code) + return outErr +} + +// Scope selection. + +func TestForwardsListProjectScopedStillUsesInbox(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute(), forwardsInboxRoute()) + + err := executeRecordingCommand(NewForwardsCmd(), app, "list", "--in", "123", "--inbox", "555") + + require.NoError(t, err) + assert.Equal(t, forwardsInboxPath, transport.last(t).Path) +} + +func TestForwardsListConfiguredProjectStaysProjectScoped(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute(), forwardsInboxRoute()) + app.Config.ProjectID = "123" + + err := executeRecordingCommand(NewForwardsCmd(), app, "list", "--inbox", "555") + + require.NoError(t, err) + assert.Equal(t, forwardsInboxPath, transport.last(t).Path) +} + +func TestForwardsListWithoutProjectListsAccountWide(t *testing.T) { + app, transport := setupRecordingTestApp(t, forwardsAccountWideRoute(2)) + + err := executeRecordingCommand(NewForwardsCmd(), app, "list") + + require.NoError(t, err) + last := transport.last(t) + assert.Equal(t, forwardsAccountWidePath, last.Path) + assert.Empty(t, last.Query, "the default follows every page, which the endpoint spells as an absent page") +} + +func TestForwardsListAllProjectsOverridesConfiguredProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, forwardsAccountWideRoute(2)) + app.Config.ProjectID = "123" + + err := executeRecordingCommand(NewForwardsCmd(), app, "list", "--all-projects") + + require.NoError(t, err) + assert.Equal(t, forwardsAccountWidePath, transport.last(t).Path) +} + +func TestForwardsListAllProjectsConflictsWithExplicitProject(t *testing.T) { + assertConflict := func(t *testing.T, configure func(*appctx.App), args ...string) { + t.Helper() + app, transport := setupRecordingTestApp(t, forwardsAccountWideRoute(2), forwardsInboxRoute()) + configure(app) + + err := executeRecordingCommand(NewForwardsCmd(), app, args...) + + outErr := requireForwardsUsageError(t, err) + assert.Contains(t, outErr.Message, "--all-projects") + assert.Empty(t, transport.recorded(), "a conflicting scope must not reach the API") + } + + t.Run("--in", func(t *testing.T) { + assertConflict(t, func(*appctx.App) {}, "list", "--all-projects", "--in", "123") + }) + t.Run("-p", func(t *testing.T) { + assertConflict(t, func(*appctx.App) {}, "list", "--all-projects", "-p", "123") + }) + t.Run("root --project", func(t *testing.T) { + assertConflict(t, func(app *appctx.App) { app.Flags.Project = "123" }, "list", "--all-projects") + }) +} + +// Rejected flags. Every one of these would otherwise be silently ignored. + +func TestForwardsListAccountWideRejectsInbox(t *testing.T) { + assertRejected := func(t *testing.T, args ...string) { + t.Helper() + app, transport := setupRecordingTestApp(t, forwardsAccountWideRoute(2)) + + err := executeRecordingCommand(NewForwardsCmd(), app, args...) + + outErr := requireForwardsUsageError(t, err) + assert.Contains(t, outErr.Message, "--inbox") + assert.Contains(t, outErr.Hint, "--project") + assert.Empty(t, transport.recorded()) + } + + t.Run("after the subcommand", func(t *testing.T) { + assertRejected(t, "list", "--inbox", "555") + }) + t.Run("before the subcommand", func(t *testing.T) { + assertRejected(t, "--inbox", "555", "list") + }) + t.Run("with --all-projects", func(t *testing.T) { + assertRejected(t, "list", "--all-projects", "--inbox", "555") + }) +} + +func TestForwardsListAccountWideRejectsUnusablePaging(t *testing.T) { + assertRejected := func(t *testing.T, wantFragment string, args ...string) { + t.Helper() + app, transport := setupRecordingTestApp(t, forwardsAccountWideRoute(2)) + + err := executeRecordingCommand(NewForwardsCmd(), app, args...) + + outErr := requireForwardsUsageError(t, err) + assert.Contains(t, outErr.Message, wantFragment) + assert.Empty(t, transport.recorded()) + } + + t.Run("explicit --page 0", func(t *testing.T) { + assertRejected(t, "--page", "list", "--page", "0") + }) + t.Run("negative --page", func(t *testing.T) { + assertRejected(t, "--page", "list", "--page=-1") + }) + t.Run("negative --limit", func(t *testing.T) { + assertRejected(t, "--limit", "list", "--limit=-1") + }) + t.Run("--all with --limit", func(t *testing.T) { + assertRejected(t, "--limit", "list", "--all", "--limit", "5") + }) + t.Run("--page with --limit", func(t *testing.T) { + assertRejected(t, "--page", "list", "--page", "2", "--limit", "5") + }) +} + +// Pagination contract: the default is every page, and any positive page is +// addressable. + +func TestForwardsListAccountWidePageContract(t *testing.T) { + assertQuery := func(t *testing.T, wantQuery string, args ...string) { + t.Helper() + app, transport := setupRecordingTestApp(t, forwardsAccountWideRoute(2)) + + err := executeRecordingCommand(NewForwardsCmd(), app, args...) + + require.NoError(t, err) + last := transport.last(t) + assert.Equal(t, forwardsAccountWidePath, last.Path) + assert.Equal(t, wantQuery, last.Query) + } + + t.Run("--all follows every page", func(t *testing.T) { + assertQuery(t, "", "list", "--all") + }) + t.Run("--page 1", func(t *testing.T) { + assertQuery(t, "page=1", "list", "--page", "1") + }) + t.Run("--page beyond the first", func(t *testing.T) { + assertQuery(t, "page=7", "list", "--page", "7") + }) +} + +// Output: []Recording goes to the renderer as-is, and --limit trims it with a +// notice that names the full total. + +func TestForwardsListAccountWideLimitTruncatesWithNotice(t *testing.T) { + app, out := setupForwardsCountingApp(t, forwardsFeedBody(3), 3) + + err := executeRecordingCommand(NewForwardsCmd(), app, "list", "--limit", "2") + + require.NoError(t, err) + envelope := decodeForwardsEnvelope(t, out) + assert.Len(t, envelope.Data, 2) + assert.Equal(t, "2 forwards across all projects", envelope.Summary) + assert.Contains(t, envelope.Notice, "Showing 2 of 3 results") +} + +func TestForwardsListAccountWideUntruncatedHasNoNotice(t *testing.T) { + app, out := setupForwardsCountingApp(t, forwardsFeedBody(3), 3) + + err := executeRecordingCommand(NewForwardsCmd(), app, "list") + + require.NoError(t, err) + envelope := decodeForwardsEnvelope(t, out) + assert.Len(t, envelope.Data, 3) + assert.Equal(t, "3 forwards across all projects", envelope.Summary) + assert.Empty(t, envelope.Notice) + require.NotEmpty(t, envelope.Data) + assert.Equal(t, "Test Project", envelope.Data[0].Bucket.Name, "each row carries its project") +} + +// forwardsCountingTransport serves the account-wide feed with an X-Total-Count +// header, which the shared stub route table cannot express. +type forwardsCountingTransport struct { + body string + totalCount string +} + +func (t *forwardsCountingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "X-Total-Count": []string{t.totalCount}, + }, + Body: io.NopCloser(strings.NewReader(t.body)), + Request: req, + }, nil +} + +// setupForwardsCountingApp returns an app whose SDK sees a feed with a total +// count, plus the buffer its JSON envelope is written to. +func setupForwardsCountingApp(t *testing.T, body string, totalCount int) (*appctx.App, *bytes.Buffer) { + t.Helper() + + app, _ := setupRecordingTestApp(t) + transport := &forwardsCountingTransport{body: body, totalCount: strconv.Itoa(totalCount)} + app.SDK = basecamp.NewClient( + &basecamp.Config{BaseURL: "https://3.basecampapi.com"}, + recordingTestTokenProvider{}, + basecamp.WithTransport(transport), + basecamp.WithMaxRetries(1), + ) + app.Names = names.NewResolver(app.SDK, app.Auth, app.Config.AccountID) + + out := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: out}) + return app, out +} + +// forwardsEnvelope is the slice of the JSON success envelope these tests read. +type forwardsEnvelope struct { + Data []struct { + ID int64 `json:"id"` + Bucket struct { + Name string `json:"name"` + } `json:"bucket"` + } `json:"data"` + Summary string `json:"summary"` + Notice string `json:"notice"` +} + +func decodeForwardsEnvelope(t *testing.T, out *bytes.Buffer) forwardsEnvelope { + t.Helper() + var envelope forwardsEnvelope + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope)) + return envelope +} diff --git a/internal/commands/messages.go b/internal/commands/messages.go index aa9b11474..aca72e030 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -1,8 +1,11 @@ package commands import ( + "context" "fmt" "os" + "slices" + "sort" "strconv" "strings" @@ -55,40 +58,46 @@ use --message-board <id> to specify which one.`, func newMessagesListCmd(project *string, messageBoard *string) *cobra.Command { var limit, page int var all bool + var allProjects bool var sortField string var reverse bool cmd := &cobra.Command{ Use: "list", Short: "List messages", - Long: "List all messages in a project's message board.", + Long: `List all messages in a project's message board. + +With no project in scope, lists every message across all accessible projects, +newest first. --all-projects asks for that listing explicitly, overriding a +configured project.`, RunE: func(cmd *cobra.Command, args []string) error { - return runMessagesList(cmd, *project, *messageBoard, limit, page, all, sortField, reverse) + return runMessagesList(cmd, *project, *messageBoard, limit, page, all, allProjects, sortField, reverse) }, } cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of messages to fetch (0 = default 100)") cmd.Flags().BoolVar(&all, "all", false, "Fetch all messages (no limit)") cmd.Flags().IntVar(&page, "page", 0, "Fetch a single page (use --all for everything)") + cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List messages across every accessible project") cmd.Flags().StringVar(&sortField, "sort", "", "Sort by field (title, created, updated)") cmd.Flags().BoolVar(&reverse, "reverse", false, "Reverse sort order") return cmd } -func runMessagesList(cmd *cobra.Command, project string, messageBoard string, limit, page int, all bool, sortField string, reverse bool) error { +func runMessagesList(cmd *cobra.Command, project string, messageBoard string, limit, page int, all, allProjects bool, sortField string, reverse bool) error { app := appctx.FromContext(cmd.Context()) - // Validate flag combinations + // Flag combinations that mean the same thing in either scope. The page + // rules are scope-specific and live on the branches below: a project's + // message board only serves page 1, while the account-wide feed serves any + // positive page. if all && limit > 0 { return output.ErrUsage("--all and --limit are mutually exclusive") } if page > 0 && (all || limit > 0) { return output.ErrUsage("--page cannot be combined with --all or --limit") } - if page > 1 { - return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") - } if sortField != "" { if err := validateSortField(sortField, []string{"title", "created", "updated"}); err != nil { return err @@ -100,23 +109,32 @@ func runMessagesList(cmd *cobra.Command, project string, messageBoard string, li return err } - // Resolve project from CLI flags and config, with interactive fallback - projectID := project - if projectID == "" { - projectID = app.Flags.Project + // Select the scope before validating against it. An explicit project wins, + // then a configured one; with neither, the account-wide feed answers the + // same question across every project rather than prompting for one. + // --all-projects pins that intent, so it overrides a configured project and + // conflicts with an explicit one. + explicitProject := project + if explicitProject == "" { + explicitProject = app.Flags.Project } - if projectID == "" { - projectID = app.Config.ProjectID + switch { + case allProjects && explicitProject != "": + return output.ErrUsageHint("--all-projects conflicts with --project/--in", + "drop one: --all-projects lists every project, --project lists one") + case allProjects, !projectKnown(app, project): + return runMessagesListAccountWide(cmd, app, messageBoard, limit, page, all, sortField, reverse) } - // If no project specified, try interactive resolution + projectID := explicitProject if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } projectID = app.Config.ProjectID } + if page > 1 { + return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") + } + resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) if err != nil { return err @@ -179,6 +197,152 @@ func messagesListBreadcrumbs(resolvedProjectID string) []output.Breadcrumb { } } +// messagesAccountWideLimit is the account-wide default cap. It matches what the +// project-scoped list already returns by default, so dropping the project does +// not quietly change how much a caller gets back. +const messagesAccountWideLimit = 100 + +// runMessagesListAccountWide lists every message across all accessible +// projects. The feed is flat []Recording, and each item carries its own +// bucket — which the generic renderers skip by name, so human-facing output +// reads flattened rows that keep the project while machine formats get the +// raw payload. +func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoard string, limit, page int, all bool, sortField string, reverse bool) error { + if err := rejectAccountWideTodolist(app, "message"); err != nil { + return err + } + // --message-board names one project's board, so it cannot narrow a listing + // that spans every project. It reaches here from the group's persistent + // flag, whichever side of the subcommand it was written on. + if messageBoard != "" { + return output.ErrUsageHint("--message-board applies to a single project's message board", + "drop --message-board, or pass --project/--in to list that board") + } + if reverse && sortField == "" { + return output.ErrUsage("--reverse requires --sort") + } + if limit < 0 { + return output.ErrUsage("--limit must be a positive number of messages") + } + if page < 0 || (page == 0 && cmd.Flags().Changed("page")) { + return output.ErrUsageHint("--page must be a positive page number", + "use --all to follow every page") + } + + capped := !all && page == 0 + wanted := limit + if wanted == 0 { + wanted = messagesAccountWideLimit + } + + recordings, meta, err := messagesAccountWideFetch(cmd.Context(), app, wanted, page, all, sortField != "") + if err != nil { + return err + } + + // Sort before truncating: capping first would sort only the window that + // happened to survive. + if sortField != "" { + sortMessagesAccountWide(recordings, sortField, reverse) + } + if capped && len(recordings) > wanted { + recordings = recordings[:wanted] + } + + respOpts := accountWideRespOpts(len(recordings), "message", "messages", meta, "--all", limit > 0) + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(recordings))) + respOpts = append(respOpts, output.WithBreadcrumbs(messagesAccountWideBreadcrumbs()...)) + + return app.OK(recordings, respOpts...) +} + +// messagesAccountWideFetch collects the account-wide feed, which takes a single +// page number: N returns exactly page N, and 0 follows the Link header across +// every page. +// +// A capped listing collects one page at a time and stops once it has enough, +// rather than crawling the whole account and discarding most of it. A sorted +// listing cannot do that — the cap applies after the sort, so every page has to +// be in hand first. +func messagesAccountWideFetch(ctx context.Context, app *appctx.App, wanted, page int, all, sorted bool) ([]basecamp.Recording, basecamp.ListMeta, error) { + everything := app.Account().Everything() + + fetch := func(p int32) ([]basecamp.Recording, basecamp.ListMeta, error) { + result, err := everything.Messages(ctx, p) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return result.Recordings, result.Meta, nil + } + + if page > 0 || all { + sdkPage, err := accountWidePage(page, all) + if err != nil { + return nil, basecamp.ListMeta{}, err + } + return fetch(sdkPage) + } + if sorted { + return fetch(0) + } + + var recordings []basecamp.Recording + var meta basecamp.ListMeta + for p := int32(1); len(recordings) < wanted; p++ { + pageRecordings, pageMeta, err := fetch(p) + if err != nil { + return nil, basecamp.ListMeta{}, err + } + if p == 1 { + meta = pageMeta + } + if len(pageRecordings) == 0 { + break + } + recordings = append(recordings, pageRecordings...) + } + return recordings, meta, nil +} + +func messagesAccountWideBreadcrumbs() []output.Breadcrumb { + return []output.Breadcrumb{ + {Action: "show", Cmd: "basecamp messages show <id>", Description: "Show message details"}, + {Action: "project", Cmd: "basecamp messages list --in <project>", Description: "List one project's message board"}, + {Action: "search", Cmd: "basecamp search <query> --type message", Description: "Search messages"}, + } +} + +// messagesAccountWideTitle returns the display title for a message recording. +// The /messages.json feed renders the message partial on top of the base +// recording projection, so Subject is the message's own title; Title is the +// generic fallback. +func messagesAccountWideTitle(r basecamp.Recording) string { + if r.Subject != nil && *r.Subject != "" { + return *r.Subject + } + return r.Title +} + +// sortMessagesAccountWide sorts account-wide message recordings, matching the +// fields and default directions sortMessages applies to project-scoped +// messages. +func sortMessagesAccountWide(recordings []basecamp.Recording, field string, reverse bool) { + sort.SliceStable(recordings, func(i, j int) bool { + switch field { + case "title": + return strings.ToLower(messagesAccountWideTitle(recordings[i])) < strings.ToLower(messagesAccountWideTitle(recordings[j])) + case "created": + return recordings[i].CreatedAt.After(recordings[j].CreatedAt) + case "updated": + return recordings[i].UpdatedAt.After(recordings[j].UpdatedAt) + } + return false + }) + if reverse { + slices.Reverse(recordings) + } +} + func newMessagesShowCmd() *cobra.Command { cmd := &cobra.Command{ Use: "show <id|url>", diff --git a/internal/commands/messages_test.go b/internal/commands/messages_test.go index 3d3d7fb5b..bb15bb506 100644 --- a/internal/commands/messages_test.go +++ b/internal/commands/messages_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "strings" @@ -96,14 +97,16 @@ func TestMessagesShowsHelp(t *testing.T) { assert.NoError(t, err) } -// TestMessagesListRequiresProject tests that messages list requires --project. -func TestMessagesListRequiresProject(t *testing.T) { +// TestMessagesCreateRequiresProject tests that a project-only command still +// asks for a project when none is in scope. messages list no longer does — +// it lists account-wide instead. +func TestMessagesCreateRequiresProject(t *testing.T) { app, _ := setupMessagesTestApp(t) // No project in config cmd := NewMessagesCmd() - err := executeMessagesCommand(cmd, app, "list") + err := executeMessagesCommand(cmd, app, "create", "Title", "Body") require.Error(t, err) var e *output.Error @@ -581,3 +584,312 @@ func TestMessagesListAgentModeTruncationSilent(t *testing.T) { assert.Empty(t, stderr.String(), "truncation notices should not appear on stderr in quiet mode") } + +// --- account-wide listing --- + +const messagesAccountWidePath = "/99999/messages.json" + +// messagesFeedRoute serves the account-wide feed with n message recordings, +// titled by the given subjects (cycled) so ordering is assertable. +func messagesFeedRoute(subjects ...string) stubRoute { + items := make([]string, 0, len(subjects)) + for i, subject := range subjects { + items = append(items, fmt.Sprintf( + `{"id":%d,"type":"Message","title":%q,"subject":%q,"bucket":{"id":%d,"name":"Project %d","type":"Project"}}`, + i+1, subject, subject, 100+i, i)) + } + return stubRoute{ + method: http.MethodGet, + path: messagesAccountWidePath, + status: http.StatusOK, + body: "[" + strings.Join(items, ",") + "]", + } +} + +// messagesFeedRouteOfSize serves n identical message recordings. +func messagesFeedRouteOfSize(n int) stubRoute { + subjects := make([]string, n) + for i := range subjects { + subjects[i] = fmt.Sprintf("Message %d", i) + } + return messagesFeedRoute(subjects...) +} + +// runMessagesListJSON runs messages list and returns the decoded envelope data. +func runMessagesListJSON(t *testing.T, app *appctx.App, args ...string) []map[string]any { + t.Helper() + + var buf bytes.Buffer + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: &buf}) + + require.NoError(t, executeRecordingCommand(NewMessagesCmd(), app, append([]string{"list"}, args...)...)) + + var envelope struct { + Data []map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + return envelope.Data +} + +// messagesRequestsTo returns the requests the transport saw for the given path. +func messagesRequestsTo(transport *recordingTransport, path string) []recordedCall { + var matched []recordedCall + for _, call := range transport.recorded() { + if call.Path == path { + matched = append(matched, call) + } + } + return matched +} + +// requireMessagesUsageError asserts the command failed with a usage error whose +// message contains want. +func requireMessagesUsageError(t *testing.T, err error, want string) { + t.Helper() + + require.Error(t, err) + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) + assert.Contains(t, e.Message, want) +} + +// TestMessagesListProjectScopedUnchanged verifies a configured project still +// reaches the project's message board, not the account-wide feed. +func TestMessagesListProjectScopedUnchanged(t *testing.T) { + app, transport := setupRecordingTestApp(t, + projectsRoute(), + stubRoute{http.MethodGet, "/99999/projects/123.json", http.StatusOK, + `{"id":123,"dock":[{"name":"message_board","id":777,"enabled":true}]}`}, + stubRoute{http.MethodGet, "/99999/message_boards/777/messages.json", http.StatusOK, + `[{"id":1,"subject":"Board message"}]`}, + ) + app.Config.ProjectID = "123" + + require.NoError(t, executeRecordingCommand(NewMessagesCmd(), app, "list")) + + assert.Equal(t, "/99999/message_boards/777/messages.json", transport.last(t).Path) + assert.Empty(t, messagesRequestsTo(transport, messagesAccountWidePath)) +} + +// TestMessagesListAccountWideWithoutAnyProject verifies that with no project +// anywhere the list goes account-wide instead of prompting for one (I7). +func TestMessagesListAccountWideWithoutAnyProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, messagesFeedRouteOfSize(messagesAccountWideLimit)) + + data := runMessagesListJSON(t, app) + + require.Len(t, data, messagesAccountWideLimit) + require.Len(t, messagesRequestsTo(transport, messagesAccountWidePath), 1) + assert.Empty(t, messagesRequestsTo(transport, "/99999/projects.json"), + "account-wide listing should not resolve a project") +} + +// TestMessagesListAllProjectsOverridesConfiguredProject verifies --all-projects +// wins over ambient config rather than being ignored. +func TestMessagesListAllProjectsOverridesConfiguredProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute(), messagesFeedRouteOfSize(messagesAccountWideLimit)) + app.Config.ProjectID = "123" + + data := runMessagesListJSON(t, app, "--all-projects") + + require.Len(t, data, messagesAccountWideLimit) + require.Len(t, messagesRequestsTo(transport, messagesAccountWidePath), 1) + assert.Empty(t, messagesRequestsTo(transport, "/99999/projects/123.json"), + "--all-projects should override the configured project") +} + +// requireMessagesAllProjectsConflict runs messages list and asserts the +// explicit-project conflict was caught. +func requireMessagesAllProjectsConflict(t *testing.T, rootProject string, args ...string) { + t.Helper() + + app, _ := setupRecordingTestApp(t, projectsRoute(), messagesFeedRoute("Kickoff")) + app.Flags.Project = rootProject + + err := executeRecordingCommand(NewMessagesCmd(), app, args...) + requireMessagesUsageError(t, err, "--all-projects conflicts with --project/--in") +} + +// TestMessagesListAllProjectsConflictsWithExplicitProject verifies the conflict +// is caught however the explicit project arrives — by each alias, on either side +// of the subcommand, and from the root-level flag that never sets Changed. +func TestMessagesListAllProjectsConflictsWithExplicitProject(t *testing.T) { + requireMessagesAllProjectsConflict(t, "", "list", "--all-projects", "--project", "123") + requireMessagesAllProjectsConflict(t, "", "list", "--all-projects", "-p", "123") + requireMessagesAllProjectsConflict(t, "", "list", "--all-projects", "--in", "123") + requireMessagesAllProjectsConflict(t, "", "--in", "123", "list", "--all-projects") + requireMessagesAllProjectsConflict(t, "123", "list", "--all-projects") +} + +// TestMessagesListAccountWideRejectsMessageBoard verifies the scope-child flag +// is rejected rather than ignored, on both sides of the subcommand. +func TestMessagesListAccountWideRejectsMessageBoard(t *testing.T) { + for _, args := range [][]string{ + {"list", "--message-board", "777"}, + {"--message-board", "777", "list"}, + {"list", "--all-projects", "--message-board", "777"}, + } { + app, _ := setupRecordingTestApp(t, messagesFeedRoute("Kickoff")) + + err := executeRecordingCommand(NewMessagesCmd(), app, args...) + requireMessagesUsageError(t, err, "--message-board applies to a single project") + } +} + +// TestMessagesListAccountWideRejectsBadPaging verifies the pagination inputs the +// feed cannot express are errors, not silent no-ops. +func TestMessagesListAccountWideRejectsBadPaging(t *testing.T) { + cases := []struct { + args []string + want string + }{ + {[]string{"--page", "0"}, "--page must be a positive page number"}, + {[]string{"--page", "-1"}, "--page must be a positive page number"}, + {[]string{"--limit", "-5"}, "--limit must be a positive number of messages"}, + {[]string{"--reverse"}, "--reverse requires --sort"}, + } + + for _, tc := range cases { + app, _ := setupRecordingTestApp(t, messagesFeedRoute("Kickoff")) + + err := executeRecordingCommand(NewMessagesCmd(), app, append([]string{"list"}, tc.args...)...) + requireMessagesUsageError(t, err, tc.want) + } +} + +// TestMessagesListAccountWideAcceptsAnyPositivePage verifies the project path's +// "only --page 1" rule does not reach the account-wide branch (I1). +func TestMessagesListAccountWideAcceptsAnyPositivePage(t *testing.T) { + app, transport := setupRecordingTestApp(t, messagesFeedRoute("Kickoff")) + + data := runMessagesListJSON(t, app, "--page", "7") + + require.Len(t, data, 1) + calls := messagesRequestsTo(transport, messagesAccountWidePath) + require.Len(t, calls, 1) + assert.Contains(t, calls[0].Query, "page=7") +} + +// TestMessagesListProjectScopedStillRejectsPageTwo verifies the project path +// keeps its own page rule. +func TestMessagesListProjectScopedStillRejectsPageTwo(t *testing.T) { + app, _ := setupRecordingTestApp(t, projectsRoute()) + app.Config.ProjectID = "123" + + err := executeRecordingCommand(NewMessagesCmd(), app, "list", "--page", "2") + requireMessagesUsageError(t, err, "only --page 1 is supported") +} + +// TestMessagesListAccountWideAllFollowsEveryPage verifies --all maps to page 0, +// which the SDK follows across every page. +func TestMessagesListAccountWideAllFollowsEveryPage(t *testing.T) { + app, transport := setupRecordingTestApp(t, messagesFeedRouteOfSize(3)) + + data := runMessagesListJSON(t, app, "--all") + + require.Len(t, data, 3) + calls := messagesRequestsTo(transport, messagesAccountWidePath) + require.Len(t, calls, 1) + assert.NotContains(t, calls[0].Query, "page=") +} + +// TestMessagesListAccountWideDefaultCapsAt100 verifies the documented default +// cap survives the move account-wide, and that the cap is collected a page at a +// time rather than by crawling the account. +func TestMessagesListAccountWideDefaultCapsAt100(t *testing.T) { + app, transport := setupRecordingTestApp(t, messagesFeedRouteOfSize(60)) + + data := runMessagesListJSON(t, app) + + assert.Len(t, data, messagesAccountWideLimit) + calls := messagesRequestsTo(transport, messagesAccountWidePath) + require.Len(t, calls, 2, "expected the cap to be filled from two pages") + assert.Contains(t, calls[0].Query, "page=1") + assert.Contains(t, calls[1].Query, "page=2") +} + +// TestMessagesListAccountWideLimitTruncates verifies --limit caps the feed. +func TestMessagesListAccountWideLimitTruncates(t *testing.T) { + app, transport := setupRecordingTestApp(t, messagesFeedRouteOfSize(10)) + + data := runMessagesListJSON(t, app, "--limit", "4") + + assert.Len(t, data, 4) + assert.Len(t, messagesRequestsTo(transport, messagesAccountWidePath), 1) +} + +// TestMessagesListAccountWideSortsBeforeTruncating verifies a sorted listing +// sees every page before the cap is applied — truncating first would sort only +// the surviving window. +func TestMessagesListAccountWideSortsBeforeTruncating(t *testing.T) { + app, transport := setupRecordingTestApp(t, messagesFeedRoute("Charlie", "Alpha", "Bravo")) + + data := runMessagesListJSON(t, app, "--sort", "title", "--limit", "2") + + require.Len(t, data, 2) + assert.Equal(t, "Alpha", data[0]["subject"]) + assert.Equal(t, "Bravo", data[1]["subject"]) + + calls := messagesRequestsTo(transport, messagesAccountWidePath) + require.Len(t, calls, 1) + assert.NotContains(t, calls[0].Query, "page=", + "a sorted listing must fetch every page before capping") +} + +// TestMessagesListAccountWideReverseSorts verifies --reverse flips the order. +func TestMessagesListAccountWideReverseSorts(t *testing.T) { + app, _ := setupRecordingTestApp(t, messagesFeedRoute("Charlie", "Alpha", "Bravo")) + + data := runMessagesListJSON(t, app, "--sort", "title", "--reverse") + + require.Len(t, data, 3) + assert.Equal(t, "Charlie", data[0]["subject"]) + assert.Equal(t, "Alpha", data[2]["subject"]) +} + +// TestMessagesListAccountWideRejectsUnknownSortField verifies the sort field set +// is the same one the project path validates. +func TestMessagesListAccountWideRejectsUnknownSortField(t *testing.T) { + app, _ := setupRecordingTestApp(t, messagesFeedRoute("Kickoff")) + + err := executeRecordingCommand(NewMessagesCmd(), app, "list", "--sort", "due") + requireMessagesUsageError(t, err, "invalid sort field") +} + +// TestMessagesListAccountWideStyledRendersFlatRows verifies []Recording needs no +// flattening: the styled renderer already gives it one row per message, the way +// recordings list does. +func TestMessagesListAccountWideStyledRendersFlatRows(t *testing.T) { + app, _ := setupRecordingTestApp(t, messagesFeedRoute("Kickoff", "Retro")) + + var buf bytes.Buffer + app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: &buf}) + + require.NoError(t, executeRecordingCommand(NewMessagesCmd(), app, "list", "--all")) + + rendered := buf.String() + assert.Contains(t, rendered, "2 messages across all projects") + assert.Contains(t, rendered, "Kickoff") + assert.Contains(t, rendered, "Retro") + assert.NotContains(t, rendered, "map[", "nested cells would mean the payload needs flattening") +} + +// TestMessagesAccountWideTitlePrefersSubject verifies the message feed's own +// subject wins over the generic recording title. +func TestMessagesAccountWideTitlePrefersSubject(t *testing.T) { + subject := "Kickoff" + empty := "" + + assert.Equal(t, "Kickoff", messagesAccountWideTitle(basecamp.Recording{Title: "generic", Subject: &subject})) + assert.Equal(t, "generic", messagesAccountWideTitle(basecamp.Recording{Title: "generic", Subject: &empty})) + assert.Equal(t, "generic", messagesAccountWideTitle(basecamp.Recording{Title: "generic"})) +} + +// TestMessagesListHasAllProjectsFlag verifies the flag is on the list command. +func TestMessagesListHasAllProjectsFlag(t *testing.T) { + cmd := NewMessagesCmd() + listCmd, _, err := cmd.Find([]string{"list"}) + require.NoError(t, err) + + require.NotNil(t, listCmd.Flags().Lookup("all-projects")) +} diff --git a/internal/commands/todos.go b/internal/commands/todos.go index fb74d4467..7264bc3ff 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "sort" "strconv" "strings" @@ -23,18 +24,21 @@ import ( // todosListFlags holds the flags for the todos list command. type todosListFlags struct { - project string - todolist string - todoset string - assignee string - status string - completed bool - overdue bool - limit int - page int - all bool - sortField string - reverse bool + project string + allProjects bool + todolist string + todoset string + assignee string + status string + completed bool + overdue bool + unassigned bool + noDueDate bool + limit int + page int + all bool + sortField string + reverse bool } // NewTodosCmd creates the todos command group. @@ -43,7 +47,7 @@ func NewTodosCmd() *cobra.Command { Use: "todos", Short: "Manage todos", Long: "List, show, create, and manage Basecamp todos.", - Annotations: map[string]string{"agent_notes": "--assignee only works on todos, not cards or other content types\nbasecamp todos complete accepts multiple IDs: basecamp todos complete 1 2 3\n--assignee and --overdue require a project (--in, global flag, or config default); for cross-project use basecamp reports assigned/overdue"}, + Annotations: map[string]string{"agent_notes": "--assignee only works on todos, not cards or other content types\nbasecamp todos complete accepts multiple IDs: basecamp todos complete 1 2 3\nbasecamp todos list without a project lists every project's todos; --all-projects forces that over a configured default\n--assignee requires a project (--in, global flag, or config default); for cross-project use basecamp reports assigned"}, } cmd.AddCommand( @@ -69,7 +73,12 @@ func newTodosListCmd() *cobra.Command { cmd := &cobra.Command{ Use: "list", Short: "List todos", - Long: "List todos in a project or todolist.", + Long: `List todos in a project or todolist. + +With no project in scope, todos are listed across every project you can see. +--all-projects forces that listing over a configured default project, and +--unassigned/--no-due-date select account-wide filters that have no +project-scoped equivalent.`, RunE: func(cmd *cobra.Command, args []string) error { return runTodosList(cmd, flags) }, @@ -77,12 +86,15 @@ func newTodosListCmd() *cobra.Command { // Note: can't use -a for assignee since it conflicts with global -a for account cmd.Flags().StringVar(&flags.project, "in", "", "Project ID or name") + cmd.Flags().BoolVar(&flags.allProjects, "all-projects", false, "List todos across every project (overrides a configured project)") cmd.Flags().StringVarP(&flags.todolist, "list", "l", "", "Todolist ID") cmd.Flags().StringVarP(&flags.todoset, "todoset", "t", "", "Todoset ID (for projects with multiple todosets)") cmd.Flags().StringVar(&flags.assignee, "assignee", "", "Filter by assignee") cmd.Flags().StringVarP(&flags.status, "status", "s", "", "Filter by status (completed, incomplete, archived, trashed)") cmd.Flags().BoolVar(&flags.completed, "completed", false, "Show completed todos (shorthand for --status completed)") cmd.Flags().BoolVar(&flags.overdue, "overdue", false, "Filter overdue todos") + cmd.Flags().BoolVar(&flags.unassigned, "unassigned", false, "Unassigned todos (across all projects only)") + cmd.Flags().BoolVar(&flags.noDueDate, "no-due-date", false, "Todos with no due date (across all projects only)") cmd.Flags().IntVarP(&flags.limit, "limit", "n", 0, "Maximum number of todos to fetch (0 = default 100)") cmd.Flags().BoolVar(&flags.all, "all", false, "Fetch all todos (no limit)") cmd.Flags().IntVar(&flags.page, "page", 0, "Fetch a single page (use --all for everything)") @@ -103,7 +115,7 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { return fmt.Errorf("app not initialized") } - // Validate flag combinations + // Validate the flag combinations that hold under either scope. if flags.completed && flags.status != "" { return output.ErrUsage("--completed and --status are mutually exclusive") } @@ -116,44 +128,53 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { if flags.page > 0 && (flags.all || flags.limit > 0) { return output.ErrUsage("--page cannot be combined with --all or --limit") } - if flags.page > 1 { - return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") - } if flags.sortField != "" { if err := validateSortField(flags.sortField, []string{"title", "created", "updated", "position", "due"}); err != nil { return err } } - sdkStatus, sdkCompleted, err := resolveStatusFilter(flags.status) - if err != nil { - return err + // Pick the scope before validating against it: the account-wide endpoints + // take any positive page, while the project path only permits page 1. + if flags.allProjects && (flags.project != "" || app.Flags.Project != "") { + return output.ErrUsageHint( + "--all-projects cannot be combined with a project (--in/--project)", + "Drop one: --all-projects lists every project, --in lists one.") } + accountWide := flags.allProjects || !projectKnown(app, flags.project) // Resolve account (enables interactive prompt if needed) if err := ensureAccount(cmd, app); err != nil { return err } - // --assignee and --overdue filter within a single project. When no - // project is set anywhere (flag, global flag, config), the interactive - // picker would silently scope results to one arbitrary project. Error - // early and point to the Reports API for cross-project queries. - projectKnown := flags.project != "" || app.Flags.Project != "" || app.Config.ProjectID != "" - if !projectKnown { - if flags.assignee != "" { - return output.ErrUsageHint( - "--assignee requires a project (--in or default config)", - "For cross-project assigned todos: basecamp reports assigned") - } - if flags.overdue { - return output.ErrUsageHint( - "--overdue requires a project (--in or default config)", - "For cross-project overdue todos: basecamp reports overdue") - } + if accountWide { + return listTodosAcrossProjects(cmd, app, flags) + } + + // Project scope from here down. + if flags.unassigned { + return output.ErrUsageHint( + "--unassigned lists across all projects and has no project-scoped equivalent", + "Drop the project to list unassigned todos across every project") + } + if flags.noDueDate { + return output.ErrUsageHint( + "--no-due-date lists across all projects and has no project-scoped equivalent", + "Drop the project to list todos with no due date across every project") + } + if flags.page > 1 { + return output.ErrUsage("only --page 1 is supported; use --all to fetch everything") } - // Use project from flag or config, with interactive fallback + sdkStatus, sdkCompleted, err := resolveStatusFilter(flags.status) + if err != nil { + return err + } + + // Use project from flag, global flag, or config. One of the three is set — + // otherwise the account-wide branch above answered the listing, so there is + // nothing left to prompt for. project := flags.project if project == "" { project = app.Flags.Project @@ -162,14 +183,6 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { project = app.Config.ProjectID } - // If no project specified, try interactive resolution - if project == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - project = app.Config.ProjectID - } - // Resolve project name to ID resolvedProject, _, err := app.Names.ResolveProject(cmd.Context(), project) if err != nil { @@ -201,6 +214,404 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { return listAllTodos(cmd, app, project, flags.todoset, flags.assignee, sdkStatus, sdkCompleted, flags.overdue, flags.limit, flags.all, flags.sortField, flags.reverse) } +// todosAccountWideFilter names the account-wide todo aggregate a listing maps +// onto. Each is a distinct endpoint, so exactly one can be selected per run. +type todosAccountWideFilter int + +const ( + todosFilterOpen todosAccountWideFilter = iota + todosFilterCompleted + todosFilterUnassigned + todosFilterNoDueDate + todosFilterOverdue +) + +// todosAccountWideDefaultLimit mirrors the project-scoped default: 100 todos +// unless --limit or --all says otherwise. +const todosAccountWideDefaultLimit = 100 + +// listTodosAcrossProjects answers `todos list` from the account-wide aggregates +// when no project is in scope, or when --all-projects overrides a configured +// one. Flags that only mean something inside one project are rejected here +// rather than quietly dropped. +func listTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags todosListFlags) error { + if err := rejectProjectScopedTodosFlags(app, flags); err != nil { + return err + } + + filter, err := selectAccountWideTodosFilter(flags) + if err != nil { + return err + } + + if flags.limit < 0 { + return output.ErrUsage("--limit cannot be negative") + } + if flags.reverse && flags.sortField == "" { + return output.ErrUsage("--reverse requires --sort") + } + + if filter == todosFilterOverdue { + return listOverdueTodosAcrossProjects(cmd, app, flags) + } + + // An explicit --page 0 means "unset" to Cobra but "every page" to the API; + // --all is the spelling for that. + if cmd.Flags().Changed("page") && flags.page < 1 { + return output.ErrUsage("--page must be 1 or greater; use --all to fetch every page") + } + // The endpoints take an int32 page. Clamping a larger value would serve a + // page the user did not ask for, so say it is out of range instead. + if flags.page > math.MaxInt32 { + return output.ErrUsage("--page is out of range") + } + if flags.sortField != "" { + return output.ErrUsageHint( + "--sort is not supported when listing across all projects (results are grouped by project)", + fmt.Sprintf("Sort within one project: basecamp todos list --in <project> --sort %s", flags.sortField)) + } + + return listGroupedTodosAcrossProjects(cmd, app, flags, filter) +} + +// rejectProjectScopedTodosFlags returns a usage error for each flag that names +// something inside a single project. A configured todolist is treated the same +// way a configured project is: --all-projects overrides it, but without that +// the user never said to ignore it, and silently dropping it would hand back a +// listing they did not ask for. +func rejectProjectScopedTodosFlags(app *appctx.App, flags todosListFlags) error { + switch { + case flags.todolist != "": + return output.ErrUsageHint( + "--list names a todolist inside one project, which has no meaning across all projects", + fmt.Sprintf("List that todolist: basecamp todos list --in <project> --list %s", flags.todolist)) + case app.Flags.Todolist != "": + return output.ErrUsageHint( + "--todolist names a todolist inside one project, which has no meaning across all projects", + "Drop --todolist to list todos across every project") + case app.Config.TodolistID != "" && !flags.allProjects: + return output.ErrUsageHint( + "a default todolist is configured, which has no meaning across all projects", + "Pass --all-projects to ignore it, or clear it with: basecamp config unset todolist_id") + case flags.todoset != "": + return output.ErrUsageHint( + "--todoset names a todoset inside one project, which has no meaning across all projects", + fmt.Sprintf("List that todoset: basecamp todos list --in <project> --todoset %s", flags.todoset)) + case flags.assignee != "": + return output.ErrUsageHint( + "--assignee has no account-wide equivalent", + "For cross-project assigned todos: basecamp reports assigned") + } + return nil +} + +// selectAccountWideTodosFilter maps the endpoint selectors onto the aggregate +// they pick. The selectors are mutually exclusive — no endpoint combines two. +func selectAccountWideTodosFilter(flags todosListFlags) (todosAccountWideFilter, error) { + completedSpelling := "--status completed" + if flags.completed { + completedSpelling = "--completed" + } + + switch flags.status { + case "", "incomplete", "completed": + case "archived", "trashed": + return todosFilterOpen, output.ErrUsageHint( + fmt.Sprintf("--status %s has no account-wide equivalent", flags.status), + fmt.Sprintf("List them in one project: basecamp todos list --in <project> --status %s", flags.status)) + default: + return todosFilterOpen, output.ErrUsage( + fmt.Sprintf("unknown --status value %q (expected completed, incomplete, archived, or trashed)", flags.status)) + } + + selectors := []struct { + name string + selected bool + filter todosAccountWideFilter + }{ + {completedSpelling, flags.status == "completed", todosFilterCompleted}, + {"--unassigned", flags.unassigned, todosFilterUnassigned}, + {"--no-due-date", flags.noDueDate, todosFilterNoDueDate}, + {"--overdue", flags.overdue, todosFilterOverdue}, + } + + filter := todosFilterOpen + chosen := "" + for _, s := range selectors { + if !s.selected { + continue + } + if chosen != "" { + return todosFilterOpen, output.ErrUsage( + fmt.Sprintf("%s and %s are mutually exclusive (each selects a different listing)", chosen, s.name)) + } + chosen, filter = s.name, s.filter + } + + return filter, nil +} + +// listGroupedTodosAcrossProjects fetches one of the paginated aggregates, whose +// payload is nested by project. +func listGroupedTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags todosListFlags, filter todosAccountWideFilter) error { + limit := flags.limit + if limit == 0 { + limit = todosAccountWideDefaultLimit + } + + var groups []basecamp.BucketTodosGroup + capped := false + truncated := false + + if flags.all || flags.page > 0 { + sdkPage, err := accountWidePage(flags.page, flags.all) + if err != nil { + return err + } + page, err := fetchAccountWideTodoGroups(cmd.Context(), app, filter, sdkPage) + if err != nil { + return convertSDKError(err) + } + groups = page.Groups + truncated = page.Meta.Truncated + } else { + collected, more, err := collectAccountWideTodoGroups(cmd.Context(), app, filter, limit) + if err != nil { + return convertSDKError(err) + } + groups, capped = truncateAccountWideTodoGroups(collected, limit), more + } + + // Meta.TotalCount counts project groups rather than todos, so the item + // total and its notice are computed here instead of from the SDK's meta. + count := countAccountWideTodos(groups) + + // --json and --agent keep the grouping the SDK returned; every other + // consumer gets flat rows. Nested groups have no id and no title of their + // own, so a renderer handed them produces unreadable cells, and --ids and + // --count read right past the todos to count projects. + respOpts := []output.ResponseOption{ + output.WithDisplayData(flattenAccountWideTodos(groups)), + output.WithSummary(fmt.Sprintf("%d todos across %d projects", count, len(groups))), + output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "show", + Cmd: "basecamp todos show <id>", + Description: "Show todo details", + }, + output.Breadcrumb{ + Action: "list", + Cmd: "basecamp todos list --in <project>", + Description: "List one project's todos", + }, + ), + } + switch { + case capped: + respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( + "Showing the first %d todos; more may exist (use --all for every page, or --limit to raise the cap)", count))) + case truncated: + // The SDK stopped following pages before the listing ran out. Saying + // nothing would present a partial result as a complete one. + respOpts = append(respOpts, output.WithNotice("More pages are available; results were truncated")) + } + + return app.OK(groups, respOpts...) +} + +// listOverdueTodosAcrossProjects fetches the overdue aggregate, which is a flat +// oldest-due-first array rather than a paginated, project-grouped listing. +func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags todosListFlags) error { + if cmd.Flags().Changed("page") { + return output.ErrUsageHint( + "--page is not supported with --overdue (the overdue listing is not paginated)", + "Cap the results instead: basecamp todos list --overdue --limit <n>") + } + if flags.all { + return output.ErrUsageHint( + "--all is not supported with --overdue (the overdue listing is already complete)", + "Raise the cap instead: basecamp todos list --overdue --limit <n>") + } + if flags.sortField == "position" { + return output.ErrUsage("--sort position requires --list (position is per-todolist)") + } + + todos, err := app.Account().Everything().OverdueTodos(cmd.Context()) + if err != nil { + return convertSDKError(err) + } + total := len(todos) + + // Sort before truncating — truncating first would sort only the survivors. + if flags.sortField != "" { + sortTodos(todos, flags.sortField, flags.reverse) + } + + limit := flags.limit + if limit == 0 { + limit = todosAccountWideDefaultLimit + } + if len(todos) > limit { + todos = todos[:limit] + } + + // No WithEntity here: the todo schema renders a task list, which has no + // column for a project, and the cards arrive from every project. Flat rows + // carrying the bucket name are what makes an account-wide overdue listing + // attributable — the generic renderers skip a nested bucket by name. + respOpts := []output.ResponseOption{ + output.WithDisplayData(flattenOverdueTodos(todos)), + output.WithSummary(fmt.Sprintf("%d overdue todos across all projects", len(todos))), + output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "show", + Cmd: "basecamp todos show <id>", + Description: "Show todo details", + }, + output.Breadcrumb{ + Action: "complete", + Cmd: "basecamp todos complete <id>", + Description: "Complete a todo", + }, + ), + } + if total > len(todos) { + respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( + "Showing %d of %d overdue todos (use --limit to raise the cap)", len(todos), total))) + } + + return app.OK(todos, respOpts...) +} + +// collectAccountWideTodoGroups walks positive pages until it has collected the +// requested number of todos, which is cheaper than fetching every page only to +// truncate. The second return reports that collection stopped at the cap rather +// than at the end of the listing. +func collectAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter todosAccountWideFilter, limit int) ([]basecamp.BucketTodosGroup, bool, error) { + var groups []basecamp.BucketTodosGroup + count := 0 + + for page := int32(1); ; page++ { + result, err := fetchAccountWideTodoGroups(ctx, app, filter, page) + if err != nil { + return nil, false, err + } + if len(result.Groups) == 0 { + return groups, false, nil + } + + groups = append(groups, result.Groups...) + fetched := countAccountWideTodos(groups) + if fetched == count { + // A page of empty groups makes no progress toward the cap; + // stop rather than request the same page shape forever. + return groups, false, nil + } + count = fetched + + if count >= limit { + return groups, true, nil + } + // TotalCount counts groups, so it bounds the walk even though it + // cannot bound the todos. + if result.Meta.TotalCount > 0 && len(groups) >= result.Meta.TotalCount { + return groups, false, nil + } + } +} + +// fetchAccountWideTodoGroups calls the aggregate the filter selects. Page 0 +// follows the Link header across every page. +func fetchAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter todosAccountWideFilter, page int32) (*basecamp.BucketTodosGroupsPage, error) { + everything := app.Account().Everything() + switch filter { + case todosFilterCompleted: + return everything.CompletedTodos(ctx, page) + case todosFilterUnassigned: + return everything.UnassignedTodos(ctx, page) + case todosFilterNoDueDate: + return everything.NoDueDateTodos(ctx, page) + default: + return everything.OpenTodos(ctx, page) + } +} + +// truncateAccountWideTodoGroups caps a grouped listing at limit todos. The cap +// counts todos rather than groups — truncating groups would drop whole projects +// from the listing. +func truncateAccountWideTodoGroups(groups []basecamp.BucketTodosGroup, limit int) []basecamp.BucketTodosGroup { + kept := make([]basecamp.BucketTodosGroup, 0, len(groups)) + remaining := limit + + for _, group := range groups { + if remaining <= 0 { + break + } + if len(group.Todos) > remaining { + group.Todos = group.Todos[:remaining] + } + remaining -= len(group.Todos) + kept = append(kept, group) + } + + return kept +} + +// countAccountWideTodos totals the todos inside a grouped listing. +func countAccountWideTodos(groups []basecamp.BucketTodosGroup) int { + count := 0 + for _, group := range groups { + count += len(group.Todos) + } + return count +} + +// flattenAccountWideTodos turns the project-grouped payload into flat rows for +// styled output, which renders nested groups as unreadable cells. Machine +// formats keep the grouping. +// flattenOverdueTodos builds display rows for the flat overdue aggregate, which +// returns todos from every project rather than groups. +func flattenOverdueTodos(todos []basecamp.Todo) []map[string]any { + rows := make([]map[string]any, 0, len(todos)) + for _, todo := range todos { + status := "incomplete" + if todo.Completed { + status = "completed" + } + row := map[string]any{ + "id": todo.ID, + "title": todo.Title, + "status": status, + "due": todo.DueOn, + } + if todo.Bucket != nil { + row["project"] = todo.Bucket.Name + } + rows = append(rows, row) + } + return rows +} + +func flattenAccountWideTodos(groups []basecamp.BucketTodosGroup) []map[string]any { + rows := make([]map[string]any, 0, countAccountWideTodos(groups)) + for _, group := range groups { + for _, todo := range group.Todos { + status := "incomplete" + if todo.Completed { + status = "completed" + } + rows = append(rows, map[string]any{ + "project": group.Bucket.Name, + "id": todo.ID, + "title": todo.Title, + "status": status, + "due": todo.DueOn, + }) + } + } + return rows +} + // resolveStatusFilter maps the user-facing --status value to the SDK's // (Status, Completed) pair. Status is lifecycle-only ("archived", "trashed", // or empty); Completed handles the completion filter. The empty/"incomplete" diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index ffe2f1b3c..3cc46906b 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -98,19 +98,17 @@ func TestTodosShowsHelp(t *testing.T) { assert.NoError(t, err) } -// TestTodosListRequiresProject tests that todos list requires --project. -func TestTodosListRequiresProject(t *testing.T) { - app, _ := setupTodosTestApp(t) - // No project in config - - cmd := NewTodosCmd() +// TestTodosListWithoutProjectListsAcrossProjects tests that todos list with no +// project anywhere lists account-wide instead of prompting for one. +func TestTodosListWithoutProjectListsAcrossProjects(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(todosAccountWideDefaultLimit))) - err := executeTodosCommand(cmd, app, "list") - require.Error(t, err) + err := executeRecordingCommand(NewTodosCmd(), app, "list") + require.NoError(t, err) - var e *output.Error - require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) - assert.Equal(t, "Project ID required", e.Message) + require.Len(t, transport.recorded(), 1, "the default cap of 100 is met by the first page") + assert.Equal(t, "/99999/todos/open.json", transport.last(t).Path) } // TestTodosCreateRequiresContent tests that todos create requires content. @@ -433,21 +431,21 @@ func TestTodosListAssigneeWithoutProjectErrors(t *testing.T) { var e *output.Error require.True(t, errors.As(err, &e)) - assert.Contains(t, e.Message, "--assignee requires a project") + assert.Contains(t, e.Message, "--assignee has no account-wide equivalent") assert.Contains(t, e.Hint, "reports assigned") } -func TestTodosListOverdueWithoutProjectErrors(t *testing.T) { - app, _ := setupTodosTestApp(t) +// TestTodosListOverdueWithoutProjectListsAcrossProjects covers the behavior +// change: --overdue without a project used to redirect to reports overdue, and +// now answers from the account-wide overdue listing. +func TestTodosListOverdueWithoutProjectListsAcrossProjects(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute("/99999/todos/overdue.json", `[{"id":1,"title":"Late","due_on":"2020-01-01"}]`)) - cmd := NewTodosCmd() - err := executeTodosCommand(cmd, app, "list", "--overdue") - require.Error(t, err) + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--overdue") + require.NoError(t, err) - var e *output.Error - require.True(t, errors.As(err, &e)) - assert.Contains(t, e.Message, "--overdue requires a project") - assert.Contains(t, e.Hint, "reports overdue") + assert.Equal(t, "/99999/todos/overdue.json", transport.last(t).Path) } func TestTodosListAssigneeWithConfigDefaultProceeds(t *testing.T) { @@ -3040,3 +3038,328 @@ func TestTodosShowJSONSurfacesSDKFields(t *testing.T) { assert.Equal(t, "spec.pdf", attachments[0].Filename) assert.Equal(t, "https://example.com/spec.pdf", attachments[0].DownloadURL) } + +// --- account-wide listings --------------------------------------------------- + +// accountWideTodosRoute stubs one of the account-wide todo aggregate endpoints. +func accountWideTodosRoute(path, body string) stubRoute { + return stubRoute{method: http.MethodGet, path: path, status: http.StatusOK, body: body} +} + +// todosGroupsBody builds a project-grouped aggregate payload with todosPerGroup +// todos in each named project (one "Test Project" group by default). +func todosGroupsBody(todosPerGroup int, projects ...string) string { + if len(projects) == 0 { + projects = []string{"Test Project"} + } + + id := 0 + groups := make([]string, 0, len(projects)) + for i, name := range projects { + todos := make([]string, 0, todosPerGroup) + for range todosPerGroup { + id++ + todos = append(todos, fmt.Sprintf( + `{"id":%d,"title":"Todo %d","due_on":"2026-01-01","completed":false}`, id, id)) + } + groups = append(groups, fmt.Sprintf( + `{"bucket":{"id":%d,"name":%q,"type":"Project"},"todos":[%s]}`, 100+i, name, strings.Join(todos, ","))) + } + return "[" + strings.Join(groups, ",") + "]" +} + +// requireTodosListUsageError runs todos list and asserts it failed with a usage +// error naming the flag, rather than silently ignoring it. The app is stubbed +// with no matching routes, so a flag that leaks through to the network fails too. +func requireTodosListUsageError(t *testing.T, app *appctx.App, wantMessage string, args ...string) { + t.Helper() + + err := executeRecordingCommand(NewTodosCmd(), app, append([]string{"list"}, args...)...) + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) + assert.Contains(t, e.Message, wantMessage) +} + +// decodeTodosEnvelope decodes the JSON response envelope a list command wrote. +func decodeTodosEnvelope(t *testing.T, buf *bytes.Buffer) map[string]any { + t.Helper() + + var envelope map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + return envelope +} + +// setupAccountWideTodosApp stubs the given routes and captures output in the +// requested format. +func setupAccountWideTodosApp(t *testing.T, format output.Format, routes ...stubRoute) (*appctx.App, *recordingTransport, *bytes.Buffer) { + t.Helper() + + app, transport := setupRecordingTestApp(t, routes...) + buf := &bytes.Buffer{} + app.Output = output.New(output.Options{Format: format, Writer: buf}) + return app, transport, buf +} + +func TestTodosListWithProjectStaysProjectScoped(t *testing.T) { + app, transport := setupRecordingTestApp(t, + projectsRoute(), + stubRoute{method: http.MethodGet, path: "/99999/projects/123.json", status: http.StatusOK, + body: `{"id":123,"name":"Test Project","dock":[{"name":"todoset","id":700,"enabled":true}]}`}, + stubRoute{method: http.MethodGet, path: "/99999/todosets/700/todolists.json", status: http.StatusOK, + body: `[{"id":456,"name":"My List"}]`}, + stubRoute{method: http.MethodGet, path: "/99999/todolists/456/groups.json", status: http.StatusOK, body: `[]`}, + stubRoute{method: http.MethodGet, path: "/99999/todolists/456/todos.json", status: http.StatusOK, + body: `[{"id":9,"title":"In a list"}]`}, + ) + app.Config.ProjectID = "123" + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--list", "456") + require.NoError(t, err) + + assert.Equal(t, "/99999/todolists/456/todos.json", transport.last(t).Path) + for _, req := range transport.recorded() { + assert.NotContains(t, req.Path, "/todos/open.json") + } +} + +func TestTodosListAllProjectsOverridesConfiguredProject(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(todosAccountWideDefaultLimit))) + app.Config.ProjectID = "123" + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--all-projects") + require.NoError(t, err) + + assert.Equal(t, "/99999/todos/open.json", transport.last(t).Path) +} + +func TestTodosListAllProjectsConflictsWithExplicitProject(t *testing.T) { + app, _ := setupRecordingTestApp(t) + requireTodosListUsageError(t, app, "--all-projects cannot be combined with a project", "--all-projects", "--in", "123") + + rootFlagApp, _ := setupRecordingTestApp(t) + rootFlagApp.Flags.Project = "123" + requireTodosListUsageError(t, rootFlagApp, "--all-projects cannot be combined with a project", "--all-projects") +} + +func TestTodosListAccountWideSelectsEndpointPerFilter(t *testing.T) { + assertEndpoint := func(path string, args ...string) { + t.Helper() + + app, transport := setupRecordingTestApp(t, accountWideTodosRoute(path, todosGroupsBody(todosAccountWideDefaultLimit))) + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, append([]string{"list"}, args...)...)) + assert.Equal(t, path, transport.last(t).Path) + } + + assertEndpoint("/99999/todos/open.json") + assertEndpoint("/99999/todos/open.json", "--status", "incomplete") + assertEndpoint("/99999/todos/completed.json", "--completed") + assertEndpoint("/99999/todos/completed.json", "--status", "completed") + assertEndpoint("/99999/todos/unassigned.json", "--unassigned") + assertEndpoint("/99999/todos/no_due_date.json", "--no-due-date") +} + +func TestTodosListAccountWideRejectsTodolistScope(t *testing.T) { + flagApp, _ := setupRecordingTestApp(t) + requireTodosListUsageError(t, flagApp, "--list names a todolist inside one project", "--list", "456") + + rootFlagApp, _ := setupRecordingTestApp(t) + rootFlagApp.Flags.Todolist = "456" + requireTodosListUsageError(t, rootFlagApp, "--todolist names a todolist inside one project") + + configApp, _ := setupRecordingTestApp(t) + configApp.Config.TodolistID = "456" + requireTodosListUsageError(t, configApp, "a default todolist is configured") +} + +// --all-projects overrides a configured todolist the same way it overrides a +// configured project: the flag is the user saying to ignore ambient scope. +func TestTodosListAllProjectsOverridesConfiguredTodolist(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(1))) + app.Config.TodolistID = "456" + + cmd := newTodosListCmd() + require.NoError(t, executeRecordingCommand(cmd, app, "--all-projects")) + assert.Equal(t, "/99999/todos/open.json", transport.last(t).Path) +} + +func TestTodosListAccountWideRejectsProjectOnlyFilters(t *testing.T) { + app, _ := setupRecordingTestApp(t) + + requireTodosListUsageError(t, app, "--todoset names a todoset inside one project", "--todoset", "789") + requireTodosListUsageError(t, app, "--assignee has no account-wide equivalent", "--assignee", "me") + requireTodosListUsageError(t, app, "--status archived has no account-wide equivalent", "--status", "archived") + requireTodosListUsageError(t, app, "--status trashed has no account-wide equivalent", "--status", "trashed") + requireTodosListUsageError(t, app, `unknown --status value "nonsense"`, "--status", "nonsense") +} + +func TestTodosListAccountWideOnlyFiltersRejectedWithProject(t *testing.T) { + configApp, _ := setupRecordingTestApp(t) + configApp.Config.ProjectID = "123" + requireTodosListUsageError(t, configApp, "--unassigned lists across all projects", "--unassigned") + requireTodosListUsageError(t, configApp, "--no-due-date lists across all projects", "--no-due-date") + + flagApp, _ := setupRecordingTestApp(t) + requireTodosListUsageError(t, flagApp, "--unassigned lists across all projects", "--in", "123", "--unassigned") + + rootFlagApp, _ := setupRecordingTestApp(t) + rootFlagApp.Flags.Project = "123" + requireTodosListUsageError(t, rootFlagApp, "--no-due-date lists across all projects", "--no-due-date") +} + +func TestTodosListAccountWideRejectsCombinedSelectors(t *testing.T) { + app, _ := setupRecordingTestApp(t) + + requireTodosListUsageError(t, app, "--unassigned and --no-due-date are mutually exclusive", "--unassigned", "--no-due-date") + requireTodosListUsageError(t, app, "--completed and --overdue are mutually exclusive", "--completed", "--overdue") + requireTodosListUsageError(t, app, "--status completed and --unassigned are mutually exclusive", "--status", "completed", "--unassigned") +} + +func TestTodosListAccountWideRejectsSorting(t *testing.T) { + app, _ := setupRecordingTestApp(t) + + requireTodosListUsageError(t, app, "--sort is not supported when listing across all projects", "--sort", "title") + requireTodosListUsageError(t, app, "--reverse requires --sort", "--reverse") +} + +func TestTodosListAccountWideRejectsInvalidPagination(t *testing.T) { + app, _ := setupRecordingTestApp(t) + + requireTodosListUsageError(t, app, "--page must be 1 or greater", "--page", "0") + requireTodosListUsageError(t, app, "--page must be 1 or greater", "--page=-2") + requireTodosListUsageError(t, app, "--limit cannot be negative", "--limit=-5") +} + +func TestTodosListAccountWideAcceptsAnyPositivePage(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(1))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--page", "3") + require.NoError(t, err) + + require.Len(t, transport.recorded(), 1) + assert.Contains(t, transport.last(t).Query, "page=3") +} + +func TestTodosListAccountWideAllFetchesEveryPage(t *testing.T) { + app, transport := setupRecordingTestApp(t, accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(2))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--all") + require.NoError(t, err) + + require.Len(t, transport.recorded(), 1) + assert.Empty(t, transport.last(t).Query, "--all maps to page 0, which sends no page parameter") +} + +func TestTodosListAccountWideLimitCountsTodosNotGroups(t *testing.T) { + app, transport, buf := setupAccountWideTodosApp(t, output.FormatJSON, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(2, "Alpha", "Beta"))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--limit", "3") + require.NoError(t, err) + + // The single page carried 4 todos, so the walk stopped there and capped at + // 3 — keeping both project groups rather than dropping a whole project. + require.Len(t, transport.recorded(), 1) + + envelope := decodeTodosEnvelope(t, buf) + assert.Equal(t, "3 todos across 2 projects", envelope["summary"]) + assert.Contains(t, envelope["notice"], "Showing the first 3 todos") + + groups, ok := envelope["data"].([]any) + require.True(t, ok) + require.Len(t, groups, 2) + assert.Len(t, groups[0].(map[string]any)["todos"], 2) + assert.Len(t, groups[1].(map[string]any)["todos"], 1) +} + +func TestTodosListAccountWideWalksPagesUntilLimitIsMet(t *testing.T) { + app, transport, buf := setupAccountWideTodosApp(t, output.FormatJSON, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(2))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--limit", "4") + require.NoError(t, err) + + requests := transport.recorded() + require.Len(t, requests, 2, "two todos per page, so a limit of 4 needs a second page") + assert.Contains(t, requests[0].Query, "page=1") + assert.Contains(t, requests[1].Query, "page=2") + + assert.Equal(t, "4 todos across 2 projects", decodeTodosEnvelope(t, buf)["summary"]) +} + +func TestTodosListAccountWideKeepsGroupingForMachineFormats(t *testing.T) { + app, _, buf := setupAccountWideTodosApp(t, output.FormatJSON, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(1))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--limit", "1") + require.NoError(t, err) + + groups, ok := decodeTodosEnvelope(t, buf)["data"].([]any) + require.True(t, ok) + require.Len(t, groups, 1) + + group, ok := groups[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, "Test Project", group["bucket"].(map[string]any)["name"]) + assert.Len(t, group["todos"], 1) +} + +func TestTodosListAccountWideFlattensForStyledOutput(t *testing.T) { + app, _, buf := setupAccountWideTodosApp(t, output.FormatStyled, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(1))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--limit", "1") + require.NoError(t, err) + + rendered := buf.String() + assert.Contains(t, rendered, "Test Project") + assert.Contains(t, rendered, "Todo 1") + assert.NotContains(t, rendered, "bucket", "styled output renders flat rows, not the nested groups") +} + +func TestFlattenAccountWideTodosCarriesProjectAndStatus(t *testing.T) { + rows := flattenAccountWideTodos([]basecamp.BucketTodosGroup{{ + Bucket: basecamp.Bucket{ID: 1, Name: "Alpha"}, + Todos: []basecamp.Todo{ + {ID: 7, Title: "Open one", DueOn: "2026-02-01"}, + {ID: 8, Title: "Done one", Completed: true}, + }, + }}) + + require.Len(t, rows, 2) + assert.Equal(t, map[string]any{ + "project": "Alpha", "id": int64(7), "title": "Open one", "status": "incomplete", "due": "2026-02-01", + }, rows[0]) + assert.Equal(t, "completed", rows[1]["status"]) +} + +func TestTodosListAccountWideOverdueIsUnpaginated(t *testing.T) { + app, _ := setupRecordingTestApp(t) + + requireTodosListUsageError(t, app, "--page is not supported with --overdue", "--overdue", "--page", "2") + requireTodosListUsageError(t, app, "--all is not supported with --overdue", "--overdue", "--all") +} + +func TestTodosListAccountWideOverdueSortsBeforeTruncating(t *testing.T) { + body := `[{"id":1,"title":"Bravo","due_on":"2020-03-01"}, + {"id":2,"title":"Alpha","due_on":"2020-01-01"}, + {"id":3,"title":"Charlie","due_on":"2020-02-01"}]` + app, _, buf := setupAccountWideTodosApp(t, output.FormatJSON, + accountWideTodosRoute("/99999/todos/overdue.json", body)) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--overdue", "--sort", "title", "--limit", "2") + require.NoError(t, err) + + envelope := decodeTodosEnvelope(t, buf) + assert.Equal(t, "2 overdue todos across all projects", envelope["summary"]) + assert.Contains(t, envelope["notice"], "Showing 2 of 3 overdue todos") + + todos, ok := envelope["data"].([]any) + require.True(t, ok) + require.Len(t, todos, 2) + assert.Equal(t, "Alpha", todos[0].(map[string]any)["title"]) + assert.Equal(t, "Bravo", todos[1].(map[string]any)["title"]) +} diff --git a/internal/output/envelope.go b/internal/output/envelope.go index fa83ca6df..59b83fb28 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -454,6 +454,32 @@ func (w *Writer) writeQuiet(v any) error { return w.writeJSON(NormalizeData(v)) } +// displayResponse swaps DisplayData in for Data so the generic renderers see +// the same rows the schema-aware presenters already prefer. Without this, a +// command that sets DisplayData gets it honored only when it also sets an +// entity the presenter recognizes, and falls back to rendering its +// machine-shaped Data everywhere else. +func displayResponse(resp *Response) *Response { + if resp == nil || resp.DisplayData == nil { + return resp + } + shallow := *resp + shallow.Data = resp.DisplayData + return &shallow +} + +// itemData returns the rows --ids and --count should read. A command that sets +// DisplayData is saying its Data is shaped for machines rather than for +// enumeration — the account-wide todo and card listings nest their items inside +// project groups, where counting Data counts projects and no group carries an +// id at all. DisplayData is the flat, per-item view those two flags want. +func itemData(resp *Response) any { + if resp.DisplayData != nil { + return resp.DisplayData + } + return resp.Data +} + func (w *Writer) writeIDs(v any) error { resp, ok := v.(*Response) if !ok { @@ -461,7 +487,7 @@ func (w *Writer) writeIDs(v any) error { } // Normalize data to []map[string]any or map[string]any - data := NormalizeData(resp.Data) + data := NormalizeData(itemData(resp)) // Handle slice of objects with ID field switch d := data.(type) { @@ -498,7 +524,7 @@ func (w *Writer) writeCount(v any) error { } // Normalize data to a standard type - data := NormalizeData(resp.Data) + data := NormalizeData(itemData(resp)) switch d := data.(type) { case []any: @@ -525,7 +551,7 @@ func (w *Writer) writeStyled(v any) error { r := NewRenderer(w.opts.Writer, true) // Force styled switch resp := v.(type) { case *Response: - return r.RenderResponse(w.opts.Writer, resp) + return r.RenderResponse(w.opts.Writer, displayResponse(resp)) case *ErrorResponse: return r.RenderError(w.opts.Writer, resp) default: @@ -545,7 +571,7 @@ func (w *Writer) writeLiteralMarkdown(v any) error { r := NewMarkdownRenderer(w.opts.Writer) switch resp := v.(type) { case *Response: - return r.RenderResponse(w.opts.Writer, resp) + return r.RenderResponse(w.opts.Writer, displayResponse(resp)) case *ErrorResponse: return r.RenderError(w.opts.Writer, resp) default: diff --git a/internal/tui/workspace/views/detail.go b/internal/tui/workspace/views/detail.go index aa2961ddb..6a68b3a26 100644 --- a/internal/tui/workspace/views/detail.go +++ b/internal/tui/workspace/views/detail.go @@ -883,7 +883,7 @@ func (v *Detail) setDetailDueDate(dueOn string) tea.Cmd { switch rt { case "card": err = hub.UpdateCard(ctx, scope.AccountID, scope.ProjectID, recordingID, - &basecamp.UpdateCardRequest{DueOn: dueOn}) + &basecamp.UpdateCardRequest{DueOn: &dueOn}) default: err = hub.UpdateTodo(ctx, scope.AccountID, scope.ProjectID, recordingID, &basecamp.UpdateTodoRequest{DueOn: dueOn}) @@ -1064,7 +1064,7 @@ func (v *Detail) submitEditTitle(title string) tea.Cmd { &basecamp.UpdateTodoRequest{Content: title}) case "card": err = hub.UpdateCard(ctx, scope.AccountID, scope.ProjectID, recordingID, - &basecamp.UpdateCardRequest{Title: title}) + &basecamp.UpdateCardRequest{Title: &title}) default: err = fmt.Errorf("editing %s titles is not supported", recordType) } diff --git a/internal/tui/workspace/views/search.go b/internal/tui/workspace/views/search.go index 9bbcd625a..748b9e770 100644 --- a/internal/tui/workspace/views/search.go +++ b/internal/tui/workspace/views/search.go @@ -515,10 +515,15 @@ func searchResultToInfo(r basecamp.SearchResult, accountID, accountName string) title = r.Subject } - excerpt := r.Description - if excerpt == "" && r.Content != "" { - excerpt = truncateExcerpt(r.Content, 120) - } + // SDK v0.10.0 made Content and Description nullable and moved the + // highlighted excerpt to the plain-text variants. Those are HTML fragments + // despite the name — BC3 wraps each query match in <mark> — so both + // branches have to go through truncateExcerpt, which strips the markup. + excerpt := r.PlainTextDescription + if excerpt == "" { + excerpt = r.PlainTextContent + } + excerpt = truncateExcerpt(excerpt, 120) return workspace.SearchResultInfo{ ID: r.ID, diff --git a/internal/tui/workspace/views/search_test.go b/internal/tui/workspace/views/search_test.go index 27525be50..fd7e7a878 100644 --- a/internal/tui/workspace/views/search_test.go +++ b/internal/tui/workspace/views/search_test.go @@ -9,6 +9,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/tui" "github.com/basecamp/basecamp-cli/internal/tui/workspace" "github.com/basecamp/basecamp-cli/internal/tui/workspace/widget" @@ -286,6 +288,46 @@ func TestTruncateExcerpt(t *testing.T) { assert.Equal(t, "clean text", truncateExcerpt("<b>clean</b> text", 20)) } +// The plain-text search fields are HTML fragments despite their name: BC3 +// wraps each query match in <mark class="circled-text">. Both the description +// and the content fallback have to be stripped before they reach the list. +func TestSearchResultToInfo_StripsMarkupFromDescription(t *testing.T) { + r := basecamp.SearchResult{ + PlainTextDescription: `the <mark class="circled-text"><span></span>login</mark> form crashes`, + } + + info := searchResultToInfo(r, "1", "Acme") + + assert.Equal(t, "the login form crashes", info.Excerpt) +} + +func TestSearchResultToInfo_StripsMarkupFromContentFallback(t *testing.T) { + r := basecamp.SearchResult{ + PlainTextContent: `a <mark class="circled-text"><span></span>login</mark> attempt failed`, + } + + info := searchResultToInfo(r, "1", "Acme") + + assert.Equal(t, "a login attempt failed", info.Excerpt) +} + +func TestSearchResultToInfo_PrefersDescriptionOverContent(t *testing.T) { + r := basecamp.SearchResult{ + PlainTextDescription: "from description", + PlainTextContent: "from content", + } + + info := searchResultToInfo(r, "1", "Acme") + + assert.Equal(t, "from description", info.Excerpt) +} + +func TestSearchResultToInfo_NoExcerptFields(t *testing.T) { + info := searchResultToInfo(basecamp.SearchResult{}, "1", "Acme") + + assert.Empty(t, info.Excerpt) +} + // --- FocusMsg --- func TestSearch_FocusMsg_RefocusesInput(t *testing.T) { diff --git a/internal/version/sdk-provenance.json b/internal/version/sdk-provenance.json index 2c47d43af..fa4df9b73 100644 --- a/internal/version/sdk-provenance.json +++ b/internal/version/sdk-provenance.json @@ -1,13 +1,13 @@ { "sdk": { "module": "github.com/basecamp/basecamp-sdk/go", - "version": "v0.9.1-0.20260727173625-bb363c847b92", - "revision": "bb363c847b92", - "updated_at": "2026-07-27T17:36:25Z" + "version": "v0.10.0", + "revision": "d1978c4ec8ca", + "updated_at": "2026-07-29T02:40:23Z" }, "api": { "repo": "basecamp/bc3", - "revision": "c308693171680e8ec9d3ae9e68181ef2ea80f077", - "synced_at": "2026-07-26" + "revision": "dffa7e11b3337b17454d9a82301be3e94226a858", + "synced_at": "2026-07-28" } } diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index d5c6d772e..0b146de05 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -103,7 +103,8 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, ```bash printf '%s\n' '海报 mockup 方向稿：' '' '<bc-attachment ...>' | basecamp comments create <recording_id> - --in <project> --json ``` -6. **Project scope is mandatory for most commands** — via `--in <project>` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings <type>` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges. +6. **Project scope is mandatory for most commands** — via `--in <project>` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings <type>` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the eight list commands covered in item 7. +7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, `boost list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. ### Output Modes @@ -160,6 +161,8 @@ basecamp <cmd> --page 1 # First page only, no auto-pagination ## Quick Reference > **Note:** Most queries require project scope (via `--in <project>` or `.basecamp/config.json`). Cross-project exceptions: `basecamp reports assigned`, `basecamp assignments`, `basecamp reports overdue`, `basecamp reports schedule`, `basecamp recordings <type>`, `basecamp notifications`, `basecamp gauges list`. +> +> Eight list commands also list account-wide: `basecamp todos list --all-projects --json`, and likewise `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, `boost list`, and `checkins answers`. | Task | Command | |------|---------| @@ -167,9 +170,10 @@ basecamp <cmd> --page 1 # First page only, no auto-pagination | My todos (in project) | `basecamp todos list --assignee me --in <project> --json` | | My todos (cross-project) | `basecamp reports assigned --json` (defaults to "me") | | My schedule (cross-project) | `basecamp reports schedule --json` (upcoming events across all projects) | -| All todos (cross-project) | `basecamp recordings todos --json` (no assignee data — cannot filter by person) | +| All todos (cross-project) | `basecamp todos list --all-projects --json` (grouped by project) | | Overdue todos (in project) | `basecamp todos list --overdue --in <project> --json` | -| Overdue todos (cross-project) | `basecamp reports overdue --json` | +| Overdue todos (cross-project) | `basecamp todos list --all-projects --overdue --json` (flat, oldest first) or `basecamp reports overdue --json` (bucketed by lateness) | +| All cards (cross-project) | `basecamp cards list --all-projects --json` (grouped by project) | | Assign todo | `basecamp assign <id> [id...] --to <person> --in <project> --json` | | Assign card | `basecamp assign <id> [id...] --card --to <person> --in <project> --json` | | Assign card step | `basecamp assign <id> [id...] --step --to <person> --in <project> --json` |