diff --git a/.surface b/.surface index 8ce5ca25..c0273e93 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 [id|url] +ARG basecamp boost list 00 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 [id|url] +ARG basecamp boosts list 00 ARG basecamp boosts show 00 ARG basecamp campfire delete 00 ARG basecamp campfire line 00 @@ -2061,7 +2061,6 @@ 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 @@ -2169,7 +2168,6 @@ 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 @@ -5062,6 +5060,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 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 @@ -5073,10 +5072,12 @@ 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 --limit type=int 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 --page type=int FLAG basecamp docs list --person type=stringArray FLAG basecamp docs list --profile type=string FLAG basecamp docs list --project type=string @@ -5973,6 +5974,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 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 @@ -5984,10 +5986,12 @@ 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 --limit type=int 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 --page type=int FLAG basecamp documents list --person type=stringArray FLAG basecamp documents list --profile type=string FLAG basecamp documents list --project type=string @@ -6887,6 +6891,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 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 @@ -6898,10 +6903,12 @@ 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 --limit type=int 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 --page type=int FLAG basecamp file list --person type=stringArray FLAG basecamp file list --profile type=string FLAG basecamp file list --project type=string @@ -7777,6 +7784,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 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 @@ -7788,10 +7796,12 @@ 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 --limit type=int 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 --page type=int FLAG basecamp files list --person type=stringArray FLAG basecamp files list --profile type=string FLAG basecamp files list --project type=string @@ -8667,6 +8677,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 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 @@ -8678,10 +8689,12 @@ 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 --limit type=int 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 --page type=int FLAG basecamp folders list --person type=stringArray FLAG basecamp folders list --profile type=string FLAG basecamp folders list --project type=string @@ -14908,6 +14921,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 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 @@ -14919,10 +14933,12 @@ 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 --limit type=int 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 --page type=int FLAG basecamp vault list --person type=stringArray FLAG basecamp vault list --profile type=string FLAG basecamp vault list --project type=string @@ -15798,6 +15814,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 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 @@ -15809,10 +15826,12 @@ 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 --limit type=int 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 --page type=int FLAG basecamp vaults list --person type=stringArray FLAG basecamp vaults list --profile type=string FLAG basecamp vaults list --project type=string diff --git a/.surface-breaking b/.surface-breaking index 8539d556..4da82f27 100644 --- a/.surface-breaking +++ b/.surface-breaking @@ -1,8 +1,7 @@ - ARG basecamp assign 00 ARG basecamp assign 00 -ARG basecamp boost list 00 -ARG basecamp boosts list 00 +ARG basecamp boost list 00 [id|url] +ARG basecamp boosts list 00 [id|url] ARG basecamp card 00 ARG basecamp card 01 [body] ARG basecamp card move 00 <id|url> @@ -134,6 +133,8 @@ CMD basecamp uploads vault list CMD basecamp uploads vaults CMD basecamp uploads vaults create CMD basecamp uploads vaults list +FLAG basecamp boost list --all-projects type=bool +FLAG basecamp boosts list --all-projects type=bool FLAG basecamp campfire --account type=string FLAG basecamp campfire --agent type=bool FLAG basecamp campfire --cache-dir type=string diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index 7712404c..03012781 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -22,7 +22,6 @@ All 17 methods, and the invocation that reaches each. | `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 | @@ -71,10 +70,10 @@ 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: +The per-item groups (`comments`, `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 | |---|---|---|---| @@ -86,8 +85,13 @@ general precedence above: | 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. +project scope": for these two commands a configured project is not a scope that +can be honored, so it is ignored rather than turned into an error. + +`boost list` is per-item in the same way but has **no account-wide row at all** +— the aggregate behind it is being withdrawn server-side. Every absent-ID case +asks for an ID, and it carries no `--all-projects`. See "`boost list` — +withdrawn" under I5. ### I3 — No flag is silently ignored @@ -105,10 +109,16 @@ 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. +matrix names, the `files list` filters, and the pagination flags the two +flagless commands needed. The account-wide path reuses the filter and sort flags +a command already has; it does not grow a parallel flag surface for its own +sake. + +Pagination is the one place where "reuse only" turned out to be wrong. A command +with no `--limit`/`--page`/`--all` has no way to recover from a server error +mid-crawl and no way to reach past a bounded default, so bare `files list` +gained all three — see I5. (`boost list` gained them too, then lost them along +with the account-wide boost feed itself — see below.) #### Endpoint selectors @@ -118,7 +128,7 @@ flags added by this work — anything not listed here is reuse: | Command | New flag | Selects | Scope | |---|---|---|---| -| all eight | `--all-projects` | account-wide | — | +| all seven | `--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 | @@ -127,6 +137,7 @@ flags added by this work — anything not listed here is reuse: | `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 | +| `files list` | `--limit`/`-n`, `--page`, `--all` | pagination 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 @@ -143,7 +154,7 @@ pair. **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 +`forwards 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. @@ -167,36 +178,84 @@ exactly page `N`. | Flag | Mapping | |---|---| -| `--all` | page 0 | +| `--all` | page 0 — the whole account | | `--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 | +| `--limit N` | **bounds the fetch**: walks positive pages until N items are collected, then trims to exactly N | +| default | a bounded cap, per the table below | + +`--limit` bounding the fetch is the point. Fetching page 0 and then truncating +is correct and useless: it downloads the entire account to keep the first +hundred rows, which is what made `cards list --limit 3` take 16s while +`todos list --limit 3` took none. `accountWideCollect` is the shared walk. + +**Three deliberate exceptions**, each with a reason that is not "we didn't get +to it": + +1. **Sorted `messages list`** fetches every page before capping. The cap applies + after the sort (I4), so every page has to be in hand first. Pinned by + `TestMessagesListAccountWideSortsBeforeTruncating`. +2. **The overdue endpoints** (`todos list --overdue`, `cards list --overdue`) + are unpaginated — one request returns everything — so `--limit` trims + locally. There is no walk to bound. +3. ~~**`boost list`** keeps a first-page default.~~ Withdrawn — there is no + account-wide boost listing at all now. See 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: +Pinning each account-wide default to its project-scoped default was a mistake, +and it is the mistake this section exists to correct. Project-scoped "all" is +one project's items; account-wide "all" is the whole account. The same word +describes two very different amounts of work, and on a ~80-project account the +account-wide reading of it timed out. -| Command | Existing default | Account-wide contract | +**The default is a uniform cap of 100** (`accountWideDefaultLimit`), with two +rows that differ for stated reasons. `--all` is how you ask for the account. + +| Command | Account-wide default | Notes | |---|---|---| -| `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. +| `messages list` | cap 100 | unchanged | +| `comments list` | cap 100 | unchanged | +| `todos list` | cap 100 | unchanged | +| `cards list` | cap 100 | **changed** from "all pages" | +| `checkins answers` | cap 100 | **changed** from "all pages" | +| `forwards list` | cap 100 | **changed** from "all pages" | +| `files list` | cap 100 | **changed** from "all pages" | +| `todos list --overdue` | cap 100 | unpaginated endpoint; accepts `--all`, rejects `--page` | +| `cards list --overdue` | cap 100 | **changed** from uncapped; same rules as above | + +Project-scoped defaults are untouched throughout. + +**The two overdue rows.** Both endpoints are unpaginated, so `--page` has +nothing to address and stays an error. `--all` is a different question: it means +"skip the cap", and since the complete array is already in hand it costs no +extra request. Previously `todos --overdue` capped at 100 *and* rejected `--all`, +which left item 101 unreachable by any flag combination — capped with no escape +hatch. `cards --overdue` was uncapped and also rejected `--all`. + +**Changing a default is allowed when the old one was wrong.** The earlier +version of this section forbade moving a default in either direction. That rule +protected a contract that could not be honored at scale. What must not happen is +a default changing *silently*: each row marked **changed** above is a documented +behavior change and belongs in the release notes. #### 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. +- **Every account-wide listing needs a bounded default and a way to ask for + more.** A listing with no escape hatch cannot recover from a server error + mid-crawl: when `files list --all-projects` returned a 500 partway through the + full-account crawl, there was no flag that could step around the failing page, + because the command had none. A bounded default without a way past it is the + same defect from the other side. +- **A registered flag must work on every path that accepts it.** Where a path + genuinely has no pagination, it rejects all three by name rather than + accepting and ignoring them (`rejectScopedPaginationFlags`): + - **Project-scoped `files list`** rejects `--limit`/`--page`/`--all`. That + path passes `nil` to `Vaults().List`, `Uploads().List`, and + `Documents().List` — three unpaginated calls with nowhere to put a page. + - **`boost list`** carries none of the three at all. The SDK documents + `BoostListOptions.Page` as not honoring the page number — setting `Page=2` + does not fetch page 2 — and the account-wide feed they were added for is + gone. - **`--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. @@ -208,6 +267,48 @@ command to a capped one. crawl they did not ask for, and that is the same no-silent-flags defect I3 names. `--all` is the spelling for every page. +#### `boost list` — withdrawn + +There is **no account-wide boost listing.** `boost list` requires an item ID. + +The `/boosts.json` aggregate this section used to document was an easter egg — +unlinked from the Basecamp web UI, ~2,250 requests per 30 days globally, and +this CLI its only known consumer. BC5 has **withdrawn it** (bc3#12464); the +path 404s on both the web and API hosts once that deploys, so the CLI stops +calling it. + +The cause, from BC3's own diagnosis rather than inference: the query's cost is +proportional to the account's **accessible recordings, not its boosts**. That +is why it measured ~44s on the largest account and why page 40 cost the same as +page 1 (bc3#12458 has the timings). + +**The withdrawal is temporary.** The feed is expected back on a +boost-proportional query, via a `boosts.bucket_id` denormalization, with the +design record in bc3#12463. Public notice: basecamp/bc-api#427. So this section +describes a listing that is gone for now, not one that was judged a bad idea. + +What that means here: + +- `boost list` takes an item ID, and rejecting a bare invocation is the honest + answer rather than a fallback to an endpoint that will 404. +- It carries no `--all-projects`, `--limit`, `--page`, or `--all`. An item's + boosts arrive in one unpaginated response, and the SDK documents + `BoostListOptions.Page` as not honoring a page number, so there would be + nothing for those flags to address even if the aggregate had survived. +- **`Everything().Boosts()` is being removed from the SDK too.** The initial + brief asked that it stay — churning every generated client twice for a + temporary withdrawal seemed worse than letting it 404 — but the endpoint is + going away for real on the server, so the SDK drops it rather than ship an + operation that cannot work. That is the SDK's call; this repo only has to not + call it, which it already does. The CLI carries **zero** references to the + aggregate, so the SDK bump that removes it needs no CLI change. + +Everything else is untouched: the other Everything feeds, the bucket-scoped +boosts endpoints, and the `boosts_count`/`boosts_url` recording attributes. + +Seven commands list account-wide, not eight — until the feed returns, at which +point this is the section to revisit. + #### The `files list` filter exception `EverythingFilesOptions` carries filters the project-scoped path has no @@ -218,15 +319,15 @@ kind nor a creator. Rather than leave adopted SDK surface unreachable, bare | Flag | Value | Maps to | |---|---|---| | `--kind` | `all`, `images`, `pdfs`, `documents`, `videos` | `EverythingFilesOptions.Kind` | -| `--person` | repeatable; name, email, ID, or `me`, resolved via `resolveAssigneeID` | `EverythingFilesOptions.PeopleIDs` | +| `--person` | repeatable; name, email, ID, or `me`, resolved via `resolvePersonRoleID(ctx, app, person, "Person")` | `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. +These filters are account-wide-only by nature rather than by policy: the +project-scoped path has nothing to map them onto. #### The `files` group's alias spellings @@ -241,18 +342,12 @@ therefore gets its own account-wide meaning: | Spelling | Account-wide behavior | |---|---| -| `files list` | the whole feed, `--kind` free | +| `files list` | the feed, `--kind` free, bounded by the cap and its pagination flags | | `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 @@ -267,14 +362,13 @@ precedent in `internal/commands/search.go`: - **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 +Which payloads actually need flattening, so the seven 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 | @@ -298,7 +392,6 @@ 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 diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 4fcf3195..59d6432a 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -23,9 +23,17 @@ Out-of-scope sections are excluded from parity totals and scripts: chatbots (dif **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 +the open/completed/unassigned/overdue/no-due-date todo and card rollups. **16 of +the 17 are reached from the CLI** — see [Account-wide +aggregates](#account-wide-aggregates). `Everything().Boosts()` is not called: +BC5 has withdrawn the `/boosts.json` aggregate behind it (basecamp/bc3#12464), +because its cost was proportional to the account's accessible recordings rather +than its boosts (~44s per page — basecamp/bc3#12458). The feed is expected back +later on a boost-proportional query (basecamp/bc3#12463), but the endpoint is +genuinely gone server-side in the meantime, so **the SDK is dropping the +operation as well**. The CLI holds no references to it, so that SDK removal +lands here as a no-op. Once the CLI pins an SDK without it, this line becomes +16 of 16. 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`. @@ -79,7 +87,6 @@ instead of prompting for a project. | `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 | @@ -126,7 +133,7 @@ The **Since** column tags each row with the Basecamp version that introduced its | message_types | 5 | `messagetypes` | ✅ | BC4 | - | list, show, create, update, delete. Bucket-scoped (`/buckets/{id}/categories…`); commands are project-scoped via `--in`/`--project` | | campfires | 14 | `chat` | ✅ | BC4 | - | list, messages, post, line show/update/delete. @mentions in content | | comments | 8 | `comment`, `comments` | ✅ | BC4 | - | list, show, thread, create, update. @mentions in content. `show` surfaces `reply_target` + paste-ready `mention` from its single Get (no new calls). `thread` composes Get + parent recording (via type endpoint) + List into a deterministic reply-ready context (no new endpoints) | -| boosts | 6 | `boost`, `react` | ✅ | BC4 | - | list (recording + event), show, create (recording + event), delete | +| boosts | 6 | `boost`, `react` | ✅ | BC4 | - | list (recording + event), show, create (recording + event), delete. No account-wide listing — BC5 withdrew `/boosts.json` (basecamp/bc3#12464); temporary, returns via basecamp/bc3#12463 | | notifications | 2 | `notifications` | ✅ | BC4 | - | list, mark as read (BC5: `bubble_ups`/`scheduled_bubble_ups` sections; `memories` is BC4-only) | | bubble_ups | 1 | `notifications bubbleups` | ✅ | BC5 | - | Dedicated Bubble Ups list (`GET /my/readings/bubble_ups.json`, paginated) plus the `limit_bubble_ups` variant behind `notifications list --limit-bubble-ups` | | **Cards (Kanban)** | diff --git a/internal/commands/accountwide.go b/internal/commands/accountwide.go index 482e14f9..3647b3a4 100644 --- a/internal/commands/accountwide.go +++ b/internal/commands/accountwide.go @@ -4,6 +4,8 @@ import ( "fmt" "math" + "github.com/spf13/cobra" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/basecamp/basecamp-cli/internal/appctx" @@ -18,9 +20,15 @@ import ( // 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". +// Every account-wide listing needs a bounded default and a way to ask for more. +// A listing with no escape hatch cannot recover from a server error mid-crawl, +// which is why the two commands that once had no pagination flags now have +// them. Each command maps --page/--all onto the page number these endpoints +// take, where 0 means "follow the Link header across every page". +// +// The default is bounded because project-scoped "all" is one project's items +// while account-wide "all" is the whole account: the same number means very +// different work. --all is how you ask for the account. // projectKnown reports whether a project is available without prompting. func projectKnown(app *appctx.App, projectFlag string) bool { @@ -50,6 +58,139 @@ func accountWidePage(page int, all bool) (int32, error) { return int32(page), nil } +// accountWideDefaultLimit bounds every paginated account-wide listing that has +// no reason to differ. Project-scoped defaults are untouched: capping one +// project's items and capping the whole account are not the same promise. +const accountWideDefaultLimit = 100 + +// accountWideMaxPages bounds the walk so a server that never returns an empty +// page cannot spin forever. It is a runaway backstop, not a tuning knob: the +// default cap of 100 items is met within a handful of pages, and hitting this +// ceiling is reported as a capped result rather than a complete one. +const accountWideMaxPages = 1000 + +// accountWideCollect walks positive pages until it has collected at least limit +// items, which is far cheaper than fetching every page only to throw most of it +// away. It is the shared engine behind every bounded account-wide listing. +// +// fetch returns one page. count reports how many items a collected slice holds: +// for a flat payload that is len, but for a payload grouped by project it is +// the number of items nested inside the groups, since capping groups would drop +// whole projects from the listing. +// +// The walk stops on the first of: an empty page, reaching the cap, exhausting +// the server's reported total, or the page ceiling. +// +// Two things callers must know: +// +// - Meta.TotalCount is compared against len(items) — the top-level elements, +// which for a grouped payload are the groups, never the inner-item counter. +// The two counters measure different things and mixing them ends the walk +// early on grouped feeds. +// - This does not trim. It stops at the first page boundary at or past the +// cap, so it can overshoot by up to one page. Every caller trims exactly +// afterwards. +// +// capped reports that collection stopped at the cap rather than at the end of +// the listing, which is what lets a caller say "more may exist" honestly. meta +// is the first page's, whose X-Total-Count is the account-wide total. +func accountWideCollect[T any]( + fetch func(page int32) ([]T, basecamp.ListMeta, error), + count func([]T) int, + limit int, +) ([]T, bool, basecamp.ListMeta, error) { + // Non-nil so an empty first page renders as [] rather than null. The + // SDK-backed --all path returns an empty slice, and consumers iterating + // .data[] should not have to handle both. + items := make([]T, 0) + var meta basecamp.ListMeta + + for page := int32(1); page <= accountWideMaxPages; page++ { + pageItems, pageMeta, err := fetch(page) + if err != nil { + return nil, false, basecamp.ListMeta{}, err + } + if page == 1 { + meta = pageMeta + } + // An empty page is the server saying the listing has ended. It is the + // only content-based stop: a page carrying groups that happen to hold + // no items is sparse, not terminal, and later pages may still have + // results. Stopping on one used to drop every project after it. + if len(pageItems) == 0 { + return items, false, meta, nil + } + + items = append(items, pageItems...) + collected := count(items) + + // Exhaustion is tested before the cap, and against the first page's + // total rather than this page's. + // + // Order matters: a listing that ends exactly at the cap is complete, + // not truncated, and checking the cap first would report "more may + // exist" about a listing with nothing left in it. + // + // The total comes from meta because the first page's is the one this + // helper documents as authoritative. Reading it from pageMeta let a + // later page that omits X-Total-Count silently switch the bound off + // and walk past the declared end of the listing. + if meta.TotalCount > 0 && len(items) >= meta.TotalCount { + // Everything the listing has is in hand. The caller still trims to + // the cap, so it is only complete if the trim drops nothing. + return items, collected > limit, meta, nil + } + if collected >= limit { + return items, true, meta, nil + } + } + + // Ran out of page budget with the server still handing back items. Report + // it as capped: whatever remains is unreached, and saying so beats + // presenting a bounded walk as a complete listing. + return items, true, meta, nil +} + +// accountWideFlatCount is the count function for the flat listings, where an +// item is its own unit and the top-level length is the item total. +func accountWideFlatCount[T any](items []T) int { return len(items) } + +// accountWideCapNotice reports that the bounded walk stopped at the cap rather +// than at the end of the listing. Without it a trimmed listing reads as a +// complete one. +// +// It defers to accountWideRespOpts whenever the server reported a total, since +// "N of M" is strictly more informative than "more may exist". The walk stops +// early by design, so on the feeds that withhold X-Total-Count the count is all +// there is to say. +func accountWideCapNotice(capped bool, meta basecamp.ListMeta, count int, plural string) string { + // Any server-reported total wins, not just one larger than the count. + // When the total equals or trails the count there is demonstrably nothing + // more to show, and "more may exist" would contradict the server. + if !capped || meta.TotalCount > 0 { + return "" + } + return fmt.Sprintf( + "Showing the first %d %s; more may exist (use --all for every page, or --limit to raise the cap)", + count, plural) +} + +// rejectScopedPaginationFlags refuses --limit/--page/--all on a path that has +// no pagination to thread them onto. +// +// A flag registered on a command but ignored on one of its paths is exactly the +// defect this contract exists to prevent: the operator gets a listing that +// silently disregards what they asked for. Saying so by name costs one error +// and keeps the flag honest everywhere it is accepted. +func rejectScopedPaginationFlags(cmd *cobra.Command, scope, hint string) error { + for _, name := range []string{"limit", "page", "all"} { + if cmd.Flags().Changed(name) { + return output.ErrUsageHint(fmt.Sprintf("--%s does not apply to %s", name, scope), hint) + } + } + return 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 @@ -98,15 +239,16 @@ func rejectAccountWideTodolist(app *appctx.App, noun string) error { // 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. +// Every account-wide listing now has an --all, so the recovery advice can name +// it unconditionally. It did not always: the two flagless commands used to get +// no advice at all, since recommending --all to a command that has none 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 { +func accountWideRespOpts(count int, singular, plural string, meta basecamp.ListMeta, explicitLimit bool) []output.ResponseOption { noun := plural if count == 1 { noun = singular @@ -116,11 +258,12 @@ func accountWideRespOpts(count int, singular, plural string, meta basecamp.ListM } 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) + if explicitLimit { + // Not "drop --limit": with an explicit limit above the default, + // dropping it falls back to 100 and returns fewer. + notice += " (raise --limit, or use --all for the complete list)" + } else { + notice += " (use --all for the complete list)" } respOpts = append(respOpts, output.WithNotice(notice)) } diff --git a/internal/commands/accountwide_test.go b/internal/commands/accountwide_test.go index 5294d737..b5218e5f 100644 --- a/internal/commands/accountwide_test.go +++ b/internal/commands/accountwide_test.go @@ -13,12 +13,17 @@ 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/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. +// +// boost list is absent: it has no account-wide listing to scope, so a bare +// invocation asks for an item ID before any flag rejection is reached. func TestAccountWideListingsRejectRootTodolist(t *testing.T) { cases := []struct { name string @@ -27,7 +32,6 @@ func TestAccountWideListingsRejectRootTodolist(t *testing.T) { }{ {"messages", NewMessagesCmd, []string{"list"}}, {"comments", NewCommentsCmd, []string{"list"}}, - {"boost", NewBoostsCmd, []string{"list"}}, {"forwards", NewForwardsCmd, []string{"list"}}, {"checkins", NewCheckinsCmd, []string{"answers"}}, {"files", NewFilesCmd, []string{"list"}}, @@ -162,3 +166,269 @@ func TestAccountWidePageRejectsOutOfRange(t *testing.T) { require.NoError(t, err, "--all wins before the range check") assert.Equal(t, int32(0), all) } + +// --- accountWideCollect ------------------------------------------------- +// +// The bounded walk is the whole reason the account-wide listings are usable at +// account scale, so each guard gets its own test. The request count is asserted +// as often as the payload: "stopped early" and "returned the right items" are +// different claims, and only the first one is what makes these commands fast. + +// collectGroup stands in for the project-grouped payloads, where the top-level +// element is a project and the items nest inside it. +type collectGroup struct{ items []int } + +func countCollectGroups(groups []collectGroup) int { + total := 0 + for _, g := range groups { + total += len(g.items) + } + return total +} + +// collectPager serves canned pages and records how many were asked for. +type collectPager[T any] struct { + pages [][]T + metas []basecamp.ListMeta + err error + errOn int32 + requests int + asked []int32 +} + +func (p *collectPager[T]) fetch(page int32) ([]T, basecamp.ListMeta, error) { + p.requests++ + p.asked = append(p.asked, page) + if p.err != nil && page == p.errOn { + return nil, basecamp.ListMeta{}, p.err + } + idx := int(page - 1) + var meta basecamp.ListMeta + if idx < len(p.metas) { + meta = p.metas[idx] + } + if idx >= len(p.pages) { + return nil, meta, nil + } + return p.pages[idx], meta, nil +} + +func TestAccountWideCollectStopsOnEmptyPage(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1, 2, 3}, {}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, []int{1, 2, 3}, items) + assert.False(t, capped, "running out of listing is not hitting the cap") + assert.Equal(t, 2, pager.requests, "stops at the empty page rather than walking on") +} + +// A page can be non-empty at the top level while adding no items, when every +// group on it happens to be empty. That is sparsity, not the end of the +// listing, and the walk used to stop dead on it — silently dropping every +// project on every later page. +func TestAccountWideCollectContinuesPastAPageWithNoInnerItems(t *testing.T) { + pager := &collectPager[collectGroup]{pages: [][]collectGroup{ + {{items: []int{1, 2}}}, + {{items: nil}}, + {{items: []int{3}}}, + }} + + items, capped, _, err := accountWideCollect(pager.fetch, countCollectGroups, 100) + + require.NoError(t, err) + assert.Equal(t, 3, countCollectGroups(items), "the item on page 3 survives the empty group on page 2") + assert.Len(t, items, 3, "all three groups are kept") + assert.False(t, capped, "the listing ran out; it was not truncated") + assert.Equal(t, 4, pager.requests, "walks to the empty page that actually ends the listing") +} + +// The empty page is the only content-based stop. Everything else — cap, server +// total, page ceiling — is a bound we impose. +func TestAccountWideCollectStopsOnlyOnATrulyEmptyPage(t *testing.T) { + pager := &collectPager[collectGroup]{pages: [][]collectGroup{ + {{items: nil}}, + {{items: nil}}, + {{items: []int{1}}}, + }} + + items, _, _, err := accountWideCollect(pager.fetch, countCollectGroups, 100) + + require.NoError(t, err) + assert.Equal(t, 1, countCollectGroups(items), + "two consecutive item-less pages must not hide the third") +} + +// A server that never returns an empty page must not spin forever. The ceiling +// is a runaway backstop, and hitting it is reported as capped rather than +// passed off as a complete listing. +func TestAccountWideCollectStopsAtThePageCeiling(t *testing.T) { + pager := &endlessPager{} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 1<<30) + + require.NoError(t, err) + assert.True(t, capped, "the walk was cut short, so more may exist") + assert.Equal(t, accountWideMaxPages, pager.requests) + assert.Len(t, items, accountWideMaxPages) +} + +// endlessPager always returns one item and never an empty page. +type endlessPager struct{ requests int } + +func (p *endlessPager) fetch(int32) ([]int, basecamp.ListMeta, error) { + p.requests++ + return []int{p.requests}, basecamp.ListMeta{}, nil +} + +// An empty first page must render as [] rather than null: the SDK-backed --all +// path returns an empty slice, and consumers iterating .data[] should not have +// to handle both shapes. +func TestAccountWideCollectReturnsEmptySliceNotNil(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + require.NotNil(t, items, "a nil slice marshals to null") + assert.Empty(t, items) + assert.False(t, capped) + + encoded, err := json.Marshal(items) + require.NoError(t, err) + assert.Equal(t, "[]", string(encoded)) +} + +// The helper deliberately does not trim: it stops at the first page boundary at +// or past the cap and leaves the exact cut to the caller. +func TestAccountWideCollectOvershootsToThePageBoundary(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1, 2, 3}, {4, 5, 6}, {7, 8, 9}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 4) + + require.NoError(t, err) + assert.True(t, capped) + assert.Equal(t, []int{1, 2, 3, 4, 5, 6}, items, "overshoots by up to one page; the caller trims") + assert.Equal(t, 2, pager.requests, "stops as soon as the cap is met") +} + +func TestAccountWideCollectStopsExactlyAtAPageBoundary(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1, 2, 3, 4, 5}, {6, 7, 8, 9, 10}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 5) + + require.NoError(t, err) + assert.True(t, capped) + assert.Len(t, items, 5) + assert.Equal(t, 1, pager.requests, "a cap met by page 1 must not fetch page 2") +} + +// TotalCount bounds the walk against len(items) — the top-level elements. On a +// grouped feed those are groups, not the todos or cards inside them. Comparing +// it against the inner counter would end the walk on the first page here. +func TestAccountWideCollectBoundsTotalCountAgainstTopLevelLength(t *testing.T) { + meta := basecamp.ListMeta{TotalCount: 2} + pager := &collectPager[collectGroup]{ + pages: [][]collectGroup{ + {{items: []int{1, 2, 3, 4, 5}}}, + {{items: []int{6, 7, 8, 9, 10}}}, + {{items: []int{11}}}, + }, + metas: []basecamp.ListMeta{meta, meta, meta}, + } + + items, capped, _, err := accountWideCollect(pager.fetch, countCollectGroups, 100) + + require.NoError(t, err) + assert.False(t, capped, "exhausting the listing is not hitting the cap") + assert.Len(t, items, 2, "two groups is the whole listing TotalCount promised") + assert.Equal(t, 10, countCollectGroups(items)) + assert.Equal(t, 2, pager.requests, + "TotalCount counts groups; comparing it against the 5 items on page 1 would stop a page early") +} + +// X-Total-Count is the account-wide total on every page, so the first page's +// meta is what the truncation notice should report. +func TestAccountWideCollectReportsFirstPageMeta(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2, 3}, {}}, + metas: []basecamp.ListMeta{{TotalCount: 42}, {TotalCount: 7}}, + } + + _, _, meta, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, 42, meta.TotalCount, "later pages must not overwrite the account-wide total") +} + +func TestAccountWideCollectWalksPositivePagesOnly(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1}, {2}, {3}, {}}} + + _, _, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, []int32{1, 2, 3, 4}, pager.asked, + "page 0 is the full-account crawl this helper exists to avoid") +} + +func TestAccountWideCollectPropagatesFetchError(t *testing.T) { + boom := errors.New("boom") + pager := &collectPager[int]{pages: [][]int{{1, 2, 3}, {4}}, err: boom, errOn: 2} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.ErrorIs(t, err, boom) + assert.Nil(t, items) + assert.False(t, capped) +} + +// A listing that ends exactly at the cap is complete, not truncated. Checking +// the cap before exhaustion reported capped=true here, and the caller turned +// that into "more may exist" about a listing with nothing left in it. +func TestAccountWideCollectExactCapIsNotCapped(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2, 3, 4, 5}}, + metas: []basecamp.ListMeta{{TotalCount: 5}}, + } + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 5) + + require.NoError(t, err) + assert.Len(t, items, 5) + assert.False(t, capped, "TotalCount == limit means the cap and the end of the listing coincide") + assert.Equal(t, 1, pager.requests) +} + +// Exhaustion still has to lose to the trim: holding the whole listing does not +// make a result complete when the caller is about to cut it down to the cap. +func TestAccountWideCollectExhaustedButOvershootingIsCapped(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2, 3, 4, 5}}, + metas: []basecamp.ListMeta{{TotalCount: 5}}, + } + + _, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 3) + + require.NoError(t, err) + assert.True(t, capped, "the caller's trim to 3 drops two items that exist") +} + +// X-Total-Count is the account-wide total on every page that carries it, but +// not every page carries it. The bound comes from page 1 so that a later page +// omitting the header cannot switch it off mid-walk. +func TestAccountWideCollectBoundsOnFirstPageTotalOnly(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2}, {3, 4}, {5, 6}, {7, 8}}, + metas: []basecamp.ListMeta{{TotalCount: 4}, {}, {}, {}}, + } + + items, capped, meta, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, []int{1, 2, 3, 4}, items, "page 1 declared four items; the walk stops there") + assert.False(t, capped) + assert.Equal(t, 4, meta.TotalCount) + assert.Equal(t, 2, pager.requests, + "pages 3 and 4 omit the header; reading the total from them would walk past the declared end") +} diff --git a/internal/commands/boost.go b/internal/commands/boost.go index 14f111cf..3d70a0e4 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -7,8 +7,6 @@ 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" ) @@ -50,11 +48,10 @@ 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, or across every project", + Use: "list <id|url>", + Short: "List boosts on an item", Long: `List boosts on an item. You can pass either an ID or a Basecamp URL: @@ -63,57 +60,39 @@ You can pass either an ID or a Basecamp URL: 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), +Boosts hang off a single item, so an item ID is required: there is no +account-wide boost listing. An item's boosts arrive in one unpaginated +response, so there is nothing to page through either.`, + Example: ` basecamp boost list 789 --project my-project`, + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + if len(args) == 0 { + return missingArg(cmd, "<id|url>") + } if err := ensureAccount(cmd, app); err != nil { return err } - recording := "" - if len(args) > 0 { - recording = args[0] - } - return runBoostList(cmd, app, recording, *project, eventID, allProjects) + return runBoostList(cmd, app, args[0], *project, eventID) }, } 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, 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") - } - +func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string) error { + // Boosts hang off a single recording, so only an item ID can scope this + // listing — a project cannot, and there is no account-wide equivalent to + // fall back to: BC5 withdrew /boosts.json (basecamp/bc3#12464) because its + // cost scaled with accessible recordings rather than with boosts. + // + // The feed is expected back later on a boost-proportional query + // (basecamp/bc3#12463), but the endpoint is genuinely gone in the meantime + // and the SDK is dropping Everything().Boosts() with it. When it returns, + // this is where the account-wide branch goes back, along with + // --all-projects. recordingID, urlProjectID := extractWithProject(recording) projectID := project @@ -187,72 +166,6 @@ 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 9cbdfa10..79023dda 100644 --- a/internal/commands/boost_test.go +++ b/internal/commands/boost_test.go @@ -215,20 +215,7 @@ 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"}}}]`, - } -} +// --- item-scoped boost listing --- // recordingBoostsRoute serves the item-scoped boost listing. func recordingBoostsRoute() stubRoute { @@ -244,7 +231,7 @@ func recordingBoostsRoute() stubRoute { func setupBoostListTest(t *testing.T, buf *bytes.Buffer) (*cobra.Command, *appctx.App, *recordingTransport) { t.Helper() - app, transport := setupRecordingTestApp(t, projectsRoute(), accountWideBoostsRoute(), recordingBoostsRoute()) + app, transport := setupRecordingTestApp(t, projectsRoute(), recordingBoostsRoute()) if buf != nil { app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf}) } @@ -261,8 +248,8 @@ func requireBoostUsageError(t *testing.T, err error, want string) { assert.Contains(t, e.Message, want) } -// TestBoostListWithIDStaysItemScoped verifies that passing an ID still lists -// that item's boosts, unchanged by the account-wide path. +// TestBoostListWithIDStaysItemScoped verifies that passing an ID lists that +// item's boosts. func TestBoostListWithIDStaysItemScoped(t *testing.T) { cmd, app, transport := setupBoostListTest(t, nil) @@ -270,153 +257,71 @@ func TestBoostListWithIDStaysItemScoped(t *testing.T) { 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") -} +// Boosts hang off a single item, so an ID is required. The account-wide feed +// that used to answer a bare `boost list` was an unlinked easter egg on the +// web side and BC5 has withdrawn it (basecamp/bc3#12464), so there is nothing +// to fall back to. If it returns (basecamp/bc3#12463), this test is the one +// that says so. +// +// A machine-output invocation gets a structured usage error; an interactive one +// gets help. Either way the point is that nothing is fetched — silently listing +// something else is the failure mode worth pinning. +func TestBoostListWithoutIDAsksForAnID(t *testing.T) { + // missingArg shows help interactively and errors otherwise; pin the + // non-interactive branch so the assertion is about the contract, not + // about whether the test process happens to look like a terminal. + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + buf := &bytes.Buffer{} + cmd, app, transport := setupBoostListTest(t, buf) -// 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" + err := executeBoostCommand(cmd, app, "list") - require.NoError(t, executeBoostCommand(cmd, app, "list")) - assert.Equal(t, "/99999/boosts.json", transport.last(t).Path) + requireBoostUsageError(t, err, "<id|url> required") + assert.Empty(t, transport.recorded(), "a missing argument must not reach the API") } -// TestBoostListAllProjectsOverridesConfiguredProject verifies that -// --all-projects pins account-wide intent over ambient config. -func TestBoostListAllProjectsOverridesConfiguredProject(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) +// A configured project cannot scope a per-item listing, so it must not be +// silently promoted into one. +func TestBoostListConfiguredProjectStillNeedsAnID(t *testing.T) { + // missingArg shows help interactively and errors otherwise; pin the + // non-interactive branch so the assertion is about the contract, not + // about whether the test process happens to look like a terminal. + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + buf := &bytes.Buffer{} + cmd, app, transport := setupBoostListTest(t, buf) 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") + err := executeBoostCommand(cmd, app, "list") - 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") + requireBoostUsageError(t, err, "<id|url> required") 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. +// The pagination flags existed only for the account-wide feed. With that gone +// they must not linger: an item's boosts arrive in one unpaginated response, +// and the SDK documents BoostListOptions.Page as not honoring a page number. 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) + for _, name := range []string{"limit", "page", "all", "all-projects"} { + assert.Nil(t, list.Flags().Lookup(name), "boost list must not carry --%s", name) } } -// TestBoostListAccountWideMachineOutputKeepsPayload verifies that machine -// formats get the raw feed, nesting intact. -func TestBoostListAccountWideMachineOutputKeepsPayload(t *testing.T) { +// --event names an event inside the item, so it still needs that item's ID. +func TestBoostListEventWithoutIDAsksForAnID(t *testing.T) { + // missingArg shows help interactively and errors otherwise; pin the + // non-interactive branch so the assertion is about the contract, not + // about whether the test process happens to look like a terminal. + t.Setenv("BASECAMP_NONINTERACTIVE", "1") 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) -} + cmd, app, transport := setupBoostListTest(t, buf) -// 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}) + err := executeBoostCommand(cmd, app, "list", "--event", "999") - 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"]) + requireBoostUsageError(t, err, "<id|url> required") + assert.Empty(t, transport.recorded()) } // mockBoostNilBoosterTransport returns a boost with no booster field. diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 21d30e84..95facaef 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -106,7 +106,7 @@ func newCardsListCmd(project, cardTable *string) *cobra.Command { } 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().IntVarP(&opts.limit, "limit", "n", 0, "Maximum number of cards to fetch (0 = all in one project, 100 account-wide; use --all for every page)") 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)") @@ -276,10 +276,13 @@ func runCardsListAccountWide(cmd *cobra.Command, app *appctx.App, opts cardsList } if selector == cardsSelectorOverdue { - if opts.pageSet || opts.all { + // --page has nothing to address on an unpaginated endpoint, but --all + // does: the listing is capped by default, and --all is how you reach + // past the cap. Rejecting both would leave card 101 unreachable. + if opts.pageSet { return output.ErrUsageHint( - "--overdue returns every overdue card in one unpaginated list", - "Drop --page/--all; use --limit to cap the result.", + "--page is not supported with --overdue (the overdue listing is not paginated)", + "Cap the results instead: basecamp cards list --overdue --limit <n>", ) } return runCardsListOverdue(cmd, app, opts) @@ -294,55 +297,77 @@ func runCardsListAccountWide(cmd *cobra.Command, app *appctx.App, opts cardsList ) } - // `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 + // Account-wide "all" is the whole account, not one project's cards, so the + // default is bounded and --all is how you ask for the rest. Walking pages + // to the cap beats fetching every page and discarding most of it. + limit := opts.limit + if limit == 0 { + limit = accountWideDefaultLimit } everything := app.Account().Everything() + fetch := func(page int32) ([]basecamp.BucketCardsGroup, basecamp.ListMeta, error) { + var ( + groupsPage *basecamp.BucketCardsGroupsPage + err error + ) + switch selector { + case cardsSelectorCompleted: + groupsPage, err = everything.CompletedCards(cmd.Context(), page) + case cardsSelectorUnassigned: + groupsPage, err = everything.UnassignedCards(cmd.Context(), page) + case cardsSelectorNoDueDate: + groupsPage, err = everything.NoDueDateCards(cmd.Context(), page) + case cardsSelectorNotNow: + groupsPage, err = everything.NotNowCards(cmd.Context(), page) + default: + groupsPage, err = everything.OpenCards(cmd.Context(), page) + } + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return groupsPage.Groups, groupsPage.Meta, nil + } + var ( - groupsPage *basecamp.BucketCardsGroupsPage - err error + groups []basecamp.BucketCardsGroup + capped bool + truncated bool ) - 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) + if opts.all || opts.page > 0 { + sdkPage, err := accountWidePage(opts.page, opts.all) + if err != nil { + return err + } + pageGroups, meta, err := fetch(sdkPage) + if err != nil { + return err + } + groups, truncated = pageGroups, meta.Truncated + } else { + // --limit counts cards, not project groups: truncating groups would + // drop whole projects. The walk stops at a page boundary, so trim to + // the exact cap afterwards. + collected, more, _, err := accountWideCollect(fetch, countAccountWideCards, limit) + if err != nil { + return err + } + groups, capped = truncateAccountWideCards(collected, limit), more } - // --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 - } + // Meta.TotalCount counts project groups rather than cards, so the item + // total is ours to compute. + count := countAccountWideCards(groups) respOpts := []output.ResponseOption{ - output.WithSummary(fmt.Sprintf("%d %s cards across %d projects", shown, selector, len(groups))), + output.WithSummary(fmt.Sprintf("%d %s cards across %d projects", count, selector, len(groups))), output.WithBreadcrumbs(cardsAccountWideBreadcrumbs()...), } switch { - case shown < total: + case capped: 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: + "Showing the first %d cards; more may exist (use --all for every page, or --limit to raise the cap)", count))) + case truncated: respOpts = append(respOpts, output.WithNotice("More pages are available; results were truncated")) } @@ -373,9 +398,15 @@ func runCardsListOverdue(cmd *cobra.Command, app *appctx.App, opts cardsListOpti sortCards(cards, opts.sortField, opts.reverse) } + // The endpoint is unpaginated, so the complete array is already in hand and + // --all costs nothing beyond skipping the cap. --limit trims locally. total := len(cards) - if opts.limit > 0 && total > opts.limit { - cards = cards[:opts.limit] + limit := opts.limit + if limit == 0 { + limit = accountWideDefaultLimit + } + if !opts.all && total > limit { + cards = cards[:limit] } respOpts := []output.ResponseOption{ @@ -385,7 +416,8 @@ func runCardsListOverdue(cmd *cobra.Command, app *appctx.App, opts cardsListOpti } 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))) + "Showing the first %d of %d overdue cards (use --all for the complete list, or --limit to raise the cap)", + len(cards), total))) } return app.OK(cards, respOpts...) diff --git a/internal/commands/cards_test.go b/internal/commands/cards_test.go index ddf81db6..c13dd7d7 100644 --- a/internal/commands/cards_test.go +++ b/internal/commands/cards_test.go @@ -2446,9 +2446,19 @@ func cardsAggregateRoute(name, body string) stubRoute { path: fmt.Sprintf("/99999/cards/%s.json", name), status: http.StatusOK, body: body, + // The whole fixture is page 1; page 2 is where the listing runs out. + // Without a page-aware route the bounded walk would never see an empty + // page and would keep asking until it hit the cap. + pages: []string{body}, } } +// cardsAggregatePath is the path cardsAggregateRoute serves, for tests that +// assert on the request sequence. +func cardsAggregatePath(name string) string { + return fmt.Sprintf("/99999/cards/%s.json", name) +} + func cardsOpenRoute() stubRoute { return cardsAggregateRoute("open", cardsGroupsFixture) } // cardsAllAggregateRoutes serves every account-wide card endpoint, so a test @@ -2489,7 +2499,7 @@ func cardsUsageError(t *testing.T, err error) *output.Error { func TestCardsListProjectScopedUnchanged(t *testing.T) { app, transport := setupRecordingTestApp(t, projectsRoute(), - stubRoute{http.MethodGet, "/99999/card_tables/lists/12345/cards.json", http.StatusOK, `[]`}, + stubRoute{method: http.MethodGet, path: "/99999/card_tables/lists/12345/cards.json", status: http.StatusOK, body: `[]`}, ) require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--project", "123", "--column", "12345")) @@ -2613,22 +2623,44 @@ func TestCardsListAccountWideRejectsScopeChildren(t *testing.T) { } } -// TestCardsListAccountWidePagination tests the page mapping: the default and -// --all follow every page, --page N asks for exactly N. +// TestCardsListAccountWidePagination pins the request sequence, not just the +// last call. The old default asked for page 0 — one request that made the +// server walk the whole account — and --limit did the same before throwing most +// of the result away. Both now walk positive pages and stop as soon as they +// can, so the sequence is the behavior under test. func TestCardsListAccountWidePagination(t *testing.T) { cases := []struct { - args []string - query string + name string + args []string + queries []string }{ - {[]string{"--all-projects"}, ""}, - {[]string{"--all-projects", "--all"}, ""}, - {[]string{"--all-projects", "--page", "3"}, "page=3"}, - {[]string{"--all-projects", "--limit", "2"}, ""}, + { + "the default walks positive pages until the listing runs out", + []string{"--all-projects"}, + []string{"page=1", "page=2"}, + }, + { + "--all is the only spelling that asks for the full crawl", + []string{"--all-projects", "--all"}, + []string{""}, + }, + { + "--page N asks for exactly N", + []string{"--all-projects", "--page", "3"}, + []string{"page=3"}, + }, + { + "--limit stops at the first page that satisfies it", + []string{"--all-projects", "--limit", "2"}, + []string{"page=1"}, + }, } 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) + t.Run(tc.name, func(t *testing.T) { + app, transport := setupRecordingTestApp(t, cardsOpenRoute()) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, tc.args...)...)) + assert.Equal(t, tc.queries, transport.queriesFor(cardsAggregatePath("open"))) + }) } } @@ -2652,17 +2684,32 @@ func TestCardsListAccountWideRejectsBadPaging(t *testing.T) { } } -// 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) +// TestCardsListOverdueRejectsPage tests that --page is refused rather than +// accepted and dropped: there is no page to address on an unpaginated endpoint. +func TestCardsListOverdueRejectsPage(t *testing.T) { + app, _ := setupRecordingTestApp(t, cardsAllAggregateRoutes()...) + err := executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--overdue", "--page", "1") + assert.Contains(t, cardsUsageError(t, err).Message, "--page is not supported with --overdue") +} + +// --all is a different question from --page. The overdue listing is capped at +// 100 by default, so rejecting --all as well would leave card 101 unreachable — +// capped with no escape hatch is the defect, not the fix. The endpoint is +// unpaginated, so honoring --all costs nothing: the complete array is already +// in hand. +func TestCardsListOverdueAcceptsAll(t *testing.T) { + app, transport, buf := setupCardsAccountWideApp(t, output.FormatJSON, cardsAllAggregateRoutes()...) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--overdue", "--all")) + + var envelope struct { + Data []basecamp.Card `json:"data"` + Notice string `json:"notice"` } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.Len(t, envelope.Data, 2, "--all returns the complete array") + assert.Empty(t, envelope.Notice, "nothing was withheld, so there is nothing to warn about") + assert.Len(t, transport.queriesFor(cardsAggregatePath("overdue")), 1, + "--all costs no extra request on an unpaginated endpoint") } // TestCardsListAccountWideSorting tests that sorting is rejected for the @@ -2716,7 +2763,10 @@ func TestCardsListAccountWideLimitCountsCards(t *testing.T) { 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") + // The walk stops at the cap, so the account-wide total is unknown by + // construction — claiming "of 4" would be reporting the size of the one + // page that happened to be fetched. + assert.Contains(t, envelope.Notice, "Showing the first 3 cards; more may exist") } // TestCardsListAccountWideOutputBranches tests that machine formats keep the diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index 54412122..fc423fbc 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -603,7 +603,7 @@ on its own cannot name a question; --all-projects states that intent outright: }, } - cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of answers to fetch (0 = all)") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of answers to fetch (0 = all for one question, 100 account-wide; use --all for every page)") 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)") @@ -748,29 +748,49 @@ func runCheckinsAnswersAccountWide(cmd *cobra.Command, app *appctx.App, question 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) + fetch := func(p int32) ([]basecamp.Recording, basecamp.ListMeta, error) { + answersPage, err := app.Account().Everything().Checkins(cmd.Context(), p) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return answersPage.Recordings, answersPage.Meta, nil } - answers := answersPage.Recordings - fetched := len(answers) - if limit > 0 && fetched > limit { - answers = answers[:limit] + // Account-wide "all" is every answer in the account, which on a large one + // is a crawl nobody asked for. Bound the default and let --all say it. + var ( + answers []basecamp.Recording + meta basecamp.ListMeta + capped bool + ) + if all || page > 0 { + sdkPage, err := accountWidePage(page, all) + if err != nil { + return err + } + if answers, meta, err = fetch(sdkPage); err != nil { + return err + } + } else { + effectiveLimit := limit + if effectiveLimit == 0 { + effectiveLimit = accountWideDefaultLimit + } + var err error + if answers, capped, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], effectiveLimit); err != nil { + return err + } + // The walk stops at a page boundary, so trim to the exact cap. + if len(answers) > effectiveLimit { + answers = answers[:effectiveLimit] + } } - 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 := accountWideRespOpts(len(answers), "check-in answer", "check-in answers", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(answers), "check-in answers"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) } + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(answers))) respOpts = append(respOpts, output.WithBreadcrumbs( output.Breadcrumb{ Action: "answer", diff --git a/internal/commands/checkins_test.go b/internal/commands/checkins_test.go index 16ff220b..ea8c2fa9 100644 --- a/internal/commands/checkins_test.go +++ b/internal/commands/checkins_test.go @@ -298,6 +298,10 @@ func checkinsAccountWideRoute() stubRoute { path: checkinsAccountWidePath, status: http.StatusOK, body: checkinsAccountWideBody, + // The bounded default walks positive pages until one comes back empty, + // so the fixture has to have an end. Serving the same body for every + // page would walk to the cap instead. + pages: []string{checkinsAccountWideBody}, } } @@ -338,9 +342,10 @@ 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") + // The default used to send no page param at all, which asked the server to + // crawl the whole account — the shape that timed out in production. It now + // walks positive pages and stops at the first empty one. + assert.Equal(t, []string{"page=1", "page=2"}, transport.queriesFor(checkinsAccountWidePath)) } // A configured project cannot scope a question's answers, so it is ignored @@ -479,7 +484,10 @@ func TestCheckinsAnswersAccountWideLimitTruncatesWithNotice(t *testing.T) { 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") + // "of 2 fetched" was an artifact of fetching everything and then trimming. + // The walk now stops at the cap, so what it can honestly report is that + // more may exist — not a total it never saw. + assert.Contains(t, resp.Notice, "Showing the first 1 check-in answers; more may exist") } // []Recording is what `recordings list` already hands the styled renderer, so diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 1172187c..05ec3656 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -192,11 +192,6 @@ func runCommentsListForItem(cmd *cobra.Command, app *appctx.App, recordingArg st 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 @@ -229,6 +224,7 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page var ( comments []basecamp.Recording meta basecamp.ListMeta + capped bool ) switch { case all || page > 0: @@ -245,18 +241,22 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page } comments, meta = result.Recordings, result.Meta default: - effectiveLimit := defaultAccountWideCommentLimit + effectiveLimit := accountWideDefaultLimit if limit > 0 { effectiveLimit = limit } var err error - comments, meta, err = fetchAccountWideComments(cmd, app, effectiveLimit) + comments, capped, meta, err = fetchAccountWideComments(cmd, app, effectiveLimit) if err != nil { return err } } - respOpts := append(accountWideRespOpts(len(comments), "comment", "comments", meta, "--all", limit > 0), + respOpts := accountWideRespOpts(len(comments), "comment", "comments", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(comments), "comments"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) + } + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(comments)), output.WithBreadcrumbs( output.Breadcrumb{ @@ -279,31 +279,26 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page // 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 +func fetchAccountWideComments(cmd *cobra.Command, app *appctx.App, limit int) ([]basecamp.Recording, bool, basecamp.ListMeta, error) { + comments, capped, meta, err := accountWideCollect( + func(page int32) ([]basecamp.Recording, basecamp.ListMeta, error) { + result, err := app.Account().Everything().Comments(cmd.Context(), page) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return result.Recordings, result.Meta, nil + }, + accountWideFlatCount[basecamp.Recording], + limit, ) - 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 err != nil { + return nil, false, basecamp.ListMeta{}, err } + // The walk stops at a page boundary, so trim to the exact cap. if len(comments) > limit { comments = comments[:limit] } - return comments, meta, nil + return comments, capped, meta, nil } func newCommentsShowCmd() *cobra.Command { diff --git a/internal/commands/files.go b/internal/commands/files.go index 910f9f57..1f42f742 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -3,6 +3,7 @@ package commands import ( "fmt" "io" + "math" "net/url" "os" "path/filepath" @@ -135,6 +136,8 @@ func newFilesListCmd(project, vaultID *string) *cobra.Command { var allProjects bool var kind string var people []string + var limit, page int + var all bool cmd := &cobra.Command{ Use: "list", @@ -146,23 +149,31 @@ 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.`, +folder listing has no equivalent for them. + +--limit/--page/--all page through the account-wide listing, which returns +the first 100 files by default. A project's folder listing is a single +unpaginated response, so it rejects all three.`, Example: ` basecamp files list --in my-project basecamp files list --all-projects --kind images - basecamp files list --all-projects --person me`, + basecamp files list --all-projects --person me + basecamp files list --all-projects --limit 500`, RunE: func(cmd *cobra.Command, args []string) error { - return runFilesList(cmd, *project, *vaultID, allProjects, kind, people) + return runFilesList(cmd, *project, *vaultID, allProjects, kind, people, limit, page, all) }, } 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)") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Account-wide only: maximum files to fetch (0 = default 100)") + cmd.Flags().IntVar(&page, "page", 0, "Account-wide only: fetch a single page (use --all for everything)") + cmd.Flags().BoolVar(&all, "all", false, "Account-wide only: fetch every page") return cmd } -func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, kind string, people []string) error { +func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, kind string, people []string, limit, page int, all bool) error { app := appctx.FromContext(cmd.Context()) // Scope is settled before any scope-specific validation, and an explicit @@ -184,7 +195,16 @@ func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, // 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) + return runFilesListAccountWide(cmd, app, vaultID, kind, people, limit, page, all) + } + + // A project's folder listing is three unpaginated calls — Vaults, Uploads, + // and Documents each get a nil options struct, so there is no page number + // to thread. Accepting a cap here and ignoring it is the defect these + // scope rules exist to prevent. + if err := rejectScopedPaginationFlags(cmd, "a project's folder listing", + "Drop --project/--in (and pass --all-projects if a project is configured) to page through files across every project."); err != nil { + return err } // The account-wide filters have no project-scoped equivalent, so a project @@ -320,7 +340,7 @@ func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, // 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 { +func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind string, people []string, limit, page int, all bool) error { if err := rejectAccountWideTodolist(app, "file"); err != nil { return err } @@ -354,21 +374,77 @@ func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind ) } + if limit < 0 { + return output.ErrUsage("--limit must be zero or positive") + } + 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 page > math.MaxInt32 { + return output.ErrUsage("--page is out of range") + } + 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) + fetch := func(p int32) ([]basecamp.EverythingFile, basecamp.ListMeta, error) { + result, err := app.Account().Everything().Files(cmd.Context(), p, opts) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return result.Files, result.Meta, nil + } + + // The default used to follow the Link header across the whole account, + // which on a large one is both slow and fragile: a single failing page + // takes down the entire listing, and before these flags existed there was + // no way to step around it. Bound the default and let --page/--all reach + // the rest. + var ( + files []basecamp.EverythingFile + meta basecamp.ListMeta + capped bool + ) + if all || page > 0 { + sdkPage, err := accountWidePage(page, all) + if err != nil { + return err + } + if files, meta, err = fetch(sdkPage); err != nil { + return err + } + } else { + effectiveLimit := limit + if effectiveLimit == 0 { + effectiveLimit = accountWideDefaultLimit + } + if files, capped, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.EverythingFile], effectiveLimit); err != nil { + return err + } + // The walk stops at a page boundary, so trim to the exact cap. + if len(files) > effectiveLimit { + files = files[:effectiveLimit] + } } // 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 := accountWideRespOpts(len(files), "file", "files", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(files), "files"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) + } + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideFiles(files))) respOpts = append(respOpts, output.WithBreadcrumbs( output.Breadcrumb{ Action: "kind", @@ -382,7 +458,7 @@ func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind }, )) - return app.OK(page.Files, respOpts...) + return app.OK(files, respOpts...) } // filesAccountWideOptions validates --kind and resolves --person into the diff --git a/internal/commands/files_test.go b/internal/commands/files_test.go index 4557e4bd..78f881bb 100644 --- a/internal/commands/files_test.go +++ b/internal/commands/files_test.go @@ -922,6 +922,8 @@ func filesAccountWideRoute(body string) stubRoute { path: "/99999/files.json", status: http.StatusOK, body: body, + // The bounded default walks positive pages until one comes back empty. + pages: []string{body}, } } @@ -930,12 +932,12 @@ func filesAccountWideRoute(body string) stubRoute { 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, `[]`}, + {method: http.MethodGet, path: "/99999/projects/123.json", status: http.StatusOK, + body: `{"id":123,"name":"Test Project","dock":[{"id":555,"name":"vault","title":"Docs & Files","enabled":true}]}`}, + {method: http.MethodGet, path: "/99999/vaults/555", status: http.StatusOK, body: `{"id":555,"title":"Docs & Files"}`}, + {method: http.MethodGet, path: "/99999/vaults/555/vaults.json", status: http.StatusOK, body: `[]`}, + {method: http.MethodGet, path: "/99999/vaults/555/uploads.json", status: http.StatusOK, body: `[]`}, + {method: http.MethodGet, path: "/99999/vaults/555/documents.json", status: http.StatusOK, body: `[]`}, } } @@ -1068,8 +1070,7 @@ func TestFilesListKindReachesTheQuery(t *testing.T) { 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"}]`}, + stubRoute{method: http.MethodGet, path: "/99999/people.json", status: http.StatusOK, body: `[{"id":77,"name":"Ann Perkins"},{"id":88,"name":"Bo Diaz"}]`}, ) require.NoError(t, runFilesListCmd(t, app, "--person", "Ann Perkins", "--person", "88")) @@ -1078,20 +1079,71 @@ func TestFilesListPersonResolvesAndReachesTheQuery(t *testing.T) { assert.Contains(t, query, "88") } -func TestFilesListAccountWideFollowsEveryPage(t *testing.T) { +// The old default asked for page 0 — the full-account crawl — and that is the +// request that returned a 500 mid-listing with no flag available to step around +// it. The default is now a bounded walk over positive pages. +func TestFilesListAccountWideDefaultWalksBoundedPages(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") + assert.Equal(t, []string{"page=1"}, transport.queriesFor("/99999/files.json"), + "an empty first page ends the walk immediately") } -func TestFilesListGainsNoPaginationFlags(t *testing.T) { - listCmd, _, err := NewFilesCmd().Find([]string{"list"}) - require.NoError(t, err) +// --all is the one spelling that still asks for the whole account. +func TestFilesListAccountWideAllFollowsEveryPage(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + require.NoError(t, runFilesListCmd(t, app, "--all")) + assert.Equal(t, []string{""}, transport.queriesFor("/99999/files.json"), + "the endpoint spells the full crawl as an absent page") +} + +func TestFilesListAccountWidePageAsksForExactlyThatPage(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + require.NoError(t, runFilesListCmd(t, app, "--page", "4")) + assert.Equal(t, []string{"page=4"}, transport.queriesFor("/99999/files.json")) +} + +// Every account-wide listing needs a bounded default and a way to ask for more. +// files list had neither, so a 500 mid-crawl left no way to step around the bad +// page. The three group constructors share newFilesListCmd, so the flags have +// to be present under all six spellings, not just the one they were added for. +func TestFilesListHasPaginationFlagsUnderEverySpelling(t *testing.T) { + spellings := map[string]func() *cobra.Command{ + "files": NewFilesCmd, + "vaults": NewVaultsCmd, + "docs": NewDocsCmd, + } - for _, name := range []string{"limit", "page", "all"} { - assert.Nil(t, listCmd.Flags().Lookup(name), "files list must not grow --%s", name) + for spelling, ctor := range spellings { + t.Run(spelling, func(t *testing.T) { + listCmd, _, err := ctor().Find([]string{"list"}) + require.NoError(t, err) + + for _, name := range []string{"limit", "page", "all"} { + assert.NotNil(t, listCmd.Flags().Lookup(name), "%s list must carry --%s", spelling, name) + } + assert.Equal(t, "n", listCmd.Flags().Lookup("limit").Shorthand) + }) + } +} + +// The flags are account-wide only: a project's folder listing is three +// unpaginated SDK calls with nowhere to put a page number. Accepting a cap and +// then ignoring it is precisely the defect the contract forbids. +func TestFilesListProjectScopedRejectsPaginationFlags(t *testing.T) { + for _, flag := range []string{"--limit=5", "--page=2", "--all"} { + t.Run(flag, func(t *testing.T) { + app, _ := setupMessagesTestApp(t) + cmd := NewFilesCmd() + + err := executeMessagesCommand(cmd, app, "list", "--in", "123", flag) + + require.Error(t, err, "a project-scoped folder listing has no pagination to thread") + assert.Contains(t, err.Error(), "does not apply to a project's folder listing") + }) } } diff --git a/internal/commands/forwards.go b/internal/commands/forwards.go index cce352b6..dbed16eb 100644 --- a/internal/commands/forwards.go +++ b/internal/commands/forwards.go @@ -65,7 +65,7 @@ override a configured one — lists forwards across every accessible project.`, }, } - cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of forwards to fetch (0 = all)") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of forwards to fetch (0 = all in one project, 100 account-wide; use --all for every page)") 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") @@ -211,31 +211,50 @@ func runForwardsListEverywhere(cmd *cobra.Command, app *appctx.App, inboxID stri 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 + fetch := func(p int32) ([]basecamp.Recording, basecamp.ListMeta, error) { + result, err := app.Account().Everything().Forwards(cmd.Context(), p) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) } + return result.Recordings, result.Meta, nil } - result, err := app.Account().Everything().Forwards(cmd.Context(), sdkPage) - if err != nil { - return convertSDKError(err) + // The endpoint spells "follow every page" as page 0, which is where --all + // lands. The default no longer goes there: one project's inbox and every + // project's inboxes are different amounts of work, so the account-wide + // default is bounded and --all is how you ask for the rest. + var ( + forwards []basecamp.Recording + meta basecamp.ListMeta + capped bool + ) + if all || page > 0 { + sdkPage, err := accountWidePage(page, all) + if err != nil { + return err + } + if forwards, meta, err = fetch(sdkPage); err != nil { + return err + } + } else { + effectiveLimit := limit + if effectiveLimit == 0 { + effectiveLimit = accountWideDefaultLimit + } + var err error + if forwards, capped, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], effectiveLimit); err != nil { + return err + } + // The walk stops at a page boundary, so trim to the exact cap. + if len(forwards) > effectiveLimit { + forwards = forwards[:effectiveLimit] + } } - // --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", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(forwards), "forwards"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) } - - respOpts := accountWideRespOpts(len(forwards), "forward", "forwards", result.Meta, "--all", limit > 0) respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(forwards))) respOpts = append(respOpts, output.WithBreadcrumbs( diff --git a/internal/commands/forwards_test.go b/internal/commands/forwards_test.go index f0a0820b..4192bc24 100644 --- a/internal/commands/forwards_test.go +++ b/internal/commands/forwards_test.go @@ -42,6 +42,8 @@ func forwardsAccountWideRoute(n int) stubRoute { path: forwardsAccountWidePath, status: http.StatusOK, body: forwardsFeedBody(n), + // The bounded default walks positive pages until one comes back empty. + pages: []string{forwardsFeedBody(n)}, } } @@ -92,9 +94,9 @@ func TestForwardsListWithoutProjectListsAccountWide(t *testing.T) { 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") + // The default is bounded now: one project's inbox and every project's + // inboxes are different amounts of work, so only --all asks for the latter. + assert.Equal(t, []string{"page=1", "page=2"}, transport.queriesFor(forwardsAccountWidePath)) } func TestForwardsListAllProjectsOverridesConfiguredProject(t *testing.T) { diff --git a/internal/commands/messages.go b/internal/commands/messages.go index aca72e03..82014f67 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -197,11 +197,6 @@ 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 @@ -229,13 +224,17 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar "use --all to follow every page") } - capped := !all && page == 0 + // bounded reports that this invocation applies a cap at all; capped + // reports that the walk actually stopped short of the listing. They are + // different questions — --all is unbounded, and a bounded walk that + // reaches the end of a short listing is not truncated. + bounded := !all && page == 0 wanted := limit if wanted == 0 { - wanted = messagesAccountWideLimit + wanted = accountWideDefaultLimit } - recordings, meta, err := messagesAccountWideFetch(cmd.Context(), app, wanted, page, all, sortField != "") + recordings, capped, meta, err := messagesAccountWideFetch(cmd.Context(), app, wanted, page, all, sortField != "") if err != nil { return err } @@ -245,11 +244,17 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar if sortField != "" { sortMessagesAccountWide(recordings, sortField, reverse) } - if capped && len(recordings) > wanted { + if bounded && len(recordings) > wanted { + // A sorted listing arrives whole and is capped here, so the trim is + // itself the truncation the notice has to report. + capped = true recordings = recordings[:wanted] } - respOpts := accountWideRespOpts(len(recordings), "message", "messages", meta, "--all", limit > 0) + respOpts := accountWideRespOpts(len(recordings), "message", "messages", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(recordings), "messages"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) + } respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(recordings))) respOpts = append(respOpts, output.WithBreadcrumbs(messagesAccountWideBreadcrumbs()...)) @@ -264,7 +269,7 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar // 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) { +func messagesAccountWideFetch(ctx context.Context, app *appctx.App, wanted, page int, all, sorted bool) ([]basecamp.Recording, bool, basecamp.ListMeta, error) { everything := app.Account().Everything() fetch := func(p int32) ([]basecamp.Recording, basecamp.ListMeta, error) { @@ -275,33 +280,27 @@ func messagesAccountWideFetch(ctx context.Context, app *appctx.App, wanted, page return result.Recordings, result.Meta, nil } + // An explicit page or --all is exactly what was asked for, and a sorted + // listing needs every page before the cap can apply (I4). Neither is a + // bounded walk, so neither can report one stopping short. if page > 0 || all { sdkPage, err := accountWidePage(page, all) if err != nil { - return nil, basecamp.ListMeta{}, err + return nil, false, basecamp.ListMeta{}, err } - return fetch(sdkPage) + recordings, meta, err := fetch(sdkPage) + return recordings, false, meta, err } if sorted { - return fetch(0) + recordings, meta, err := fetch(0) + return recordings, false, meta, err } - 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...) + recordings, capped, meta, err := accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], wanted) + if err != nil { + return nil, false, basecamp.ListMeta{}, err } - return recordings, meta, nil + return recordings, capped, meta, nil } func messagesAccountWideBreadcrumbs() []output.Breadcrumb { diff --git a/internal/commands/messages_test.go b/internal/commands/messages_test.go index bb15bb50..6e47a1fb 100644 --- a/internal/commands/messages_test.go +++ b/internal/commands/messages_test.go @@ -658,10 +658,8 @@ func requireMessagesUsageError(t *testing.T, err error, want string) { 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"}]`}, + stubRoute{method: http.MethodGet, path: "/99999/projects/123.json", status: http.StatusOK, body: `{"id":123,"dock":[{"name":"message_board","id":777,"enabled":true}]}`}, + stubRoute{method: http.MethodGet, path: "/99999/message_boards/777/messages.json", status: http.StatusOK, body: `[{"id":1,"subject":"Board message"}]`}, ) app.Config.ProjectID = "123" @@ -674,11 +672,11 @@ func TestMessagesListProjectScopedUnchanged(t *testing.T) { // 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)) + app, transport := setupRecordingTestApp(t, messagesFeedRouteOfSize(accountWideDefaultLimit)) data := runMessagesListJSON(t, app) - require.Len(t, data, messagesAccountWideLimit) + require.Len(t, data, accountWideDefaultLimit) require.Len(t, messagesRequestsTo(transport, messagesAccountWidePath), 1) assert.Empty(t, messagesRequestsTo(transport, "/99999/projects.json"), "account-wide listing should not resolve a project") @@ -687,12 +685,12 @@ func TestMessagesListAccountWideWithoutAnyProject(t *testing.T) { // 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, transport := setupRecordingTestApp(t, projectsRoute(), messagesFeedRouteOfSize(accountWideDefaultLimit)) app.Config.ProjectID = "123" data := runMessagesListJSON(t, app, "--all-projects") - require.Len(t, data, messagesAccountWideLimit) + require.Len(t, data, accountWideDefaultLimit) require.Len(t, messagesRequestsTo(transport, messagesAccountWidePath), 1) assert.Empty(t, messagesRequestsTo(transport, "/99999/projects/123.json"), "--all-projects should override the configured project") @@ -801,7 +799,7 @@ func TestMessagesListAccountWideDefaultCapsAt100(t *testing.T) { data := runMessagesListJSON(t, app) - assert.Len(t, data, messagesAccountWideLimit) + assert.Len(t, data, accountWideDefaultLimit) 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") diff --git a/internal/commands/recording_test.go b/internal/commands/recording_test.go index 71ddd08a..0deced55 100644 --- a/internal/commands/recording_test.go +++ b/internal/commands/recording_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "strconv" "strings" "sync" "testing" @@ -41,6 +42,28 @@ type stubRoute struct { path string status int body string + + // pages, when set, makes the route page-aware: page=N serves pages[N-1] + // and anything past the end serves an empty array, while an absent or zero + // page serves body — the whole listing, which is what the real endpoints + // return for the "follow every page" form. + // + // Account-wide fixtures need this. The bounded walk stops on the first + // empty page, so a route that served the same body for every page number + // would walk all the way to the cap and hide the behavior under test. + pages []string +} + +// pagedBody picks the response for a page-aware route. +func pagedBody(route stubRoute, rawPage string) string { + if rawPage == "" || rawPage == "0" { + return route.body + } + n, err := strconv.Atoi(rawPage) + if err != nil || n < 1 || n > len(route.pages) { + return "[]" + } + return route.pages[n-1] } // recordingTransport is an http.RoundTripper that records every request and @@ -75,10 +98,14 @@ func (t *recordingTransport) RoundTrip(req *http.Request) (*http.Response, error for _, route := range t.routes { if route.method == req.Method && route.path == req.URL.Path { + responseBody := route.body + if route.pages != nil { + responseBody = pagedBody(route, req.URL.Query().Get("page")) + } return &http.Response{ StatusCode: route.status, Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: io.NopCloser(strings.NewReader(route.body)), + Body: io.NopCloser(strings.NewReader(responseBody)), Request: req, }, nil } @@ -100,6 +127,20 @@ func (t *recordingTransport) recorded() []recordedCall { return append([]recordedCall(nil), t.requests...) } +// queriesFor returns the raw query string of every recorded request to path, in +// order. The account-wide tests assert on the whole sequence rather than the +// last call: the request count, and whether any request omitted page= at all, +// are exactly what separates a bounded walk from a full-account crawl. +func (t *recordingTransport) queriesFor(path string) []string { + queries := []string{} + for _, call := range t.recorded() { + if call.Path == path { + queries = append(queries, call.Query) + } + } + return queries +} + // last returns the most recent request, failing the test if none were made. func (t *recordingTransport) last(tb testing.TB) recordedCall { tb.Helper() diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 7264bc3f..b0003186 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -226,10 +226,6 @@ const ( 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 @@ -356,7 +352,7 @@ func selectAccountWideTodosFilter(flags todosListFlags) (todosAccountWideFilter, func listGroupedTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags todosListFlags, filter todosAccountWideFilter) error { limit := flags.limit if limit == 0 { - limit = todosAccountWideDefaultLimit + limit = accountWideDefaultLimit } var groups []basecamp.BucketTodosGroup @@ -427,11 +423,6 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t "--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)") } @@ -447,11 +438,13 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t sortTodos(todos, flags.sortField, flags.reverse) } + // The endpoint is unpaginated, so the complete array is already in hand and + // --all costs nothing beyond skipping the cap. --limit trims locally. limit := flags.limit if limit == 0 { - limit = todosAccountWideDefaultLimit + limit = accountWideDefaultLimit } - if len(todos) > limit { + if !flags.all && len(todos) > limit { todos = todos[:limit] } @@ -477,7 +470,8 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t } 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))) + "Showing %d of %d overdue todos (use --all for the complete list, or --limit to raise the cap)", + len(todos), total))) } return app.OK(todos, respOpts...) @@ -488,36 +482,18 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t // 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 - } - } + groups, capped, _, err := accountWideCollect( + func(page int32) ([]basecamp.BucketTodosGroup, basecamp.ListMeta, error) { + result, err := fetchAccountWideTodoGroups(ctx, app, filter, page) + if err != nil { + return nil, basecamp.ListMeta{}, err + } + return result.Groups, result.Meta, nil + }, + countAccountWideTodos, + limit, + ) + return groups, capped, err } // fetchAccountWideTodoGroups calls the aggregate the filter selects. Page 0 diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 3cc46906..4402c45a 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -102,7 +102,7 @@ func TestTodosShowsHelp(t *testing.T) { // 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))) + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(accountWideDefaultLimit))) err := executeRecordingCommand(NewTodosCmd(), app, "list") require.NoError(t, err) @@ -3126,7 +3126,7 @@ func TestTodosListWithProjectStaysProjectScoped(t *testing.T) { func TestTodosListAllProjectsOverridesConfiguredProject(t *testing.T) { app, transport := setupRecordingTestApp(t, - accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(todosAccountWideDefaultLimit))) + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(accountWideDefaultLimit))) app.Config.ProjectID = "123" err := executeRecordingCommand(NewTodosCmd(), app, "list", "--all-projects") @@ -3148,7 +3148,7 @@ func TestTodosListAccountWideSelectsEndpointPerFilter(t *testing.T) { assertEndpoint := func(path string, args ...string) { t.Helper() - app, transport := setupRecordingTestApp(t, accountWideTodosRoute(path, todosGroupsBody(todosAccountWideDefaultLimit))) + app, transport := setupRecordingTestApp(t, accountWideTodosRoute(path, todosGroupsBody(accountWideDefaultLimit))) require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, append([]string{"list"}, args...)...)) assert.Equal(t, path, transport.last(t).Path) } @@ -3340,7 +3340,24 @@ 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") +} + +// --all is a different question from --page. The overdue listing caps at 100 by +// default, so refusing --all too would leave todo 101 unreachable. The endpoint +// is unpaginated, so honoring it costs nothing: the array is already in hand. +func TestTodosListAccountWideOverdueAcceptsAll(t *testing.T) { + body := `[{"id":1,"title":"Bravo","due_on":"2020-03-01"}, + {"id":2,"title":"Alpha","due_on":"2020-01-01"}]` + app, transport, buf := setupAccountWideTodosApp(t, output.FormatJSON, + accountWideTodosRoute("/99999/todos/overdue.json", body)) + + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, "list", "--overdue", "--all")) + + envelope := decodeTodosEnvelope(t, buf) + assert.Equal(t, "2 overdue todos across all projects", envelope["summary"]) + assert.Empty(t, envelope["notice"], "nothing was withheld, so there is nothing to warn about") + assert.Len(t, transport.queriesFor("/99999/todos/overdue.json"), 1, + "--all costs no extra request on an unpaginated endpoint") } func TestTodosListAccountWideOverdueSortsBeforeTruncating(t *testing.T) { diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 0b146de0..fbf4a58c 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -103,8 +103,10 @@ 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, 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. +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 seven 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`, 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. + Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected) or `--all` for everything. `--page N` fetches exactly one page, but only on the paginated listings. + The two overdue variants — `basecamp todos list --all-projects --overdue` and `basecamp cards list --all-projects --overdue` — come from unpaginated endpoints. They accept `--limit` and `--all` but **reject `--page`**, so do not generate `--page` against them. ### Output Modes @@ -162,7 +164,7 @@ basecamp <cmd> --page 1 # First page only, no auto-pagination > **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`. +> Seven list commands also list account-wide: `basecamp todos list --all-projects --json`, and likewise `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, and `checkins answers`. | Task | Command | |------|---------| @@ -720,6 +722,10 @@ basecamp comments update <id> "Updated" --in <project> ```bash basecamp files list --in <project> --json # List all (folders, files, docs) basecamp files list --vault <folder_id> --in <project> # List folder contents +basecamp files list --all-projects --json # Across every project (first 100) +basecamp files list --all-projects --limit 500 # Walk pages until 500 collected +basecamp files list --all-projects --page 2 # Exactly page 2 +basecamp files list --all-projects --all # Every page (slow on big accounts) basecamp files show <id> --in <project> # Show item (auto-detects type) basecamp files download <id> --in <project> # Download file basecamp files download <id> --out ./dir # Download to specific dir