diff --git a/.surface b/.surface index 1e8e2333..ae0acb63 100644 --- a/.surface +++ b/.surface @@ -3046,10 +3046,12 @@ FLAG basecamp cards list --account type=string FLAG basecamp cards list --agent type=bool FLAG basecamp cards list --all type=bool FLAG basecamp cards list --all-projects type=bool +FLAG basecamp cards list --assignee type=stringArray FLAG basecamp cards list --cache-dir type=string FLAG basecamp cards list --card-table type=string FLAG basecamp cards list --column type=string FLAG basecamp cards list --count type=bool +FLAG basecamp cards list --due type=string FLAG basecamp cards list --help type=bool FLAG basecamp cards list --hints type=bool FLAG basecamp cards list --ids-only type=bool @@ -14272,10 +14274,11 @@ FLAG basecamp todos list --account type=string FLAG basecamp todos list --agent type=bool FLAG basecamp todos list --all type=bool FLAG basecamp todos list --all-projects type=bool -FLAG basecamp todos list --assignee type=string +FLAG basecamp todos list --assignee type=stringArray FLAG basecamp todos list --cache-dir type=string FLAG basecamp todos list --completed type=bool FLAG basecamp todos list --count type=bool +FLAG basecamp todos list --due type=string FLAG basecamp todos list --help type=bool FLAG basecamp todos list --hints type=bool FLAG basecamp todos list --ids-only type=bool diff --git a/.surface-breaking b/.surface-breaking index 4da82f27..4464719a 100644 --- a/.surface-breaking +++ b/.surface-breaking @@ -744,6 +744,7 @@ FLAG basecamp todos --page type=int FLAG basecamp todos --status type=string FLAG basecamp todos --todoset type=string FLAG basecamp todos create --content type=string +FLAG basecamp todos list --assignee type=string FLAG basecamp todosets --todoset type=string FLAG basecamp tools create --clone type=string FLAG basecamp tools create --source type=string diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index 2e0702dd..e6ddaa8a 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -106,9 +106,14 @@ account-wide. Reject by name, including aliases: `--list`/`--todolist`, `--todoset`, `--questionnaire`, `--event`, `--by`. A configured todolist is subject to the same rule as a configured project. -**Filters with no aggregate equivalent** (`--assignee`, unsupported `--status` -values) are rejected, pointing at the command that does answer the question -(e.g. `reports assigned`). +**Filters with no aggregate equivalent** (unsupported `--status` values) are +rejected, pointing at the command that does answer the question. + +`--assignee` used to be the example here, and no longer is. It was rejected +account-wide because the aggregates had no assignee parameter to map it onto; +SDK v0.12.0 added one, so the flag is answerable in both scopes and the +rejection is gone. What it costs differs sharply by scope — see the filter table +below. **No new flags** beyond `--all-projects`, the endpoint selectors the method matrix names, the `files list` filters, and the pagination flags the two @@ -138,6 +143,9 @@ flags added by this work — anything not listed here is reuse: | `cards list` | `--no-due-date` | `NoDueDateCards` | account-wide only | | `cards list` | `--not-now` | `NotNowCards` | account-wide only | | `cards list` | `--overdue` | `OverdueCards` | account-wide only | +| `todos list` | `--due` | filter on the todo aggregates | account-wide only | +| `cards list` | `--assignee` | filter on the card aggregates | account-wide only | +| `cards list` | `--due` | filter on the card aggregates | 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 | @@ -356,6 +364,64 @@ unrecognized `--kind` value is `ErrUsage` listing the accepted set. These filters are account-wide-only by nature rather than by policy: the project-scoped path has nothing to map them onto. +#### The task filters: `--assignee` and `--due` + +`EverythingTaskFilters` (SDK v0.12.0) is a trailing parameter on 11 of the 16 +aggregate methods — the nine paginated todo and card selectors plus the two +unpaginated overdue endpoints. Two flags map onto it: + +| Flag | Value | Maps to | +|---|---|---| +| `--assignee` | repeatable, and comma-separated within a value; name, email, ID, or `me`, resolved via `resolvePersonRoleIDs(ctx, app, input, "Assignee")` | `EverythingTaskFilters.AssigneeIDs` | +| `--due` | `with`, `without`, `overdue` | `EverythingTaskFilters.Due` | + +`--due` is account-wide-only on both groups, and `--assignee` is +account-wide-only on `cards`, which had none before. On `todos`, `--assignee` +already existed project-scoped and now works in both. + +**The same flag means two different things by scope, and that is worth saying +out loud rather than papering over.** Account-wide it is a real `assignee_ids[]` +query parameter: the server narrows the listing before it paginates, so the +filter never turns the bounded walk into a full crawl — the walk stays bounded +by the item cap exactly as it is unfiltered. + +It does not follow that the request count is identical. The cap counts *items*, +so a narrower filter returns fewer of them per page and can need one more page +to reach the same cap: soaked against account 2914079, `todos list +--all-projects` took 2 requests for 100 todos and `--assignee 3` took 3 for its +21. That is the walk working, not leaking. What must never happen is the filter +pushing the walk toward page 0, and a test pins that. + +Project-scoped there is no server-side assignee parameter at all, so +the filter runs client-side over an *unlimited* fetch — the project path already +disables its own limit whenever an assignee is set. Same spelling, same +semantics, very different cost. + +The semantics are matched deliberately: project-scoped `--assignee` now matches +**any** of the named people, the way `assignee_ids[]` does. Before it took one +value; widening it from `StringVar` to `StringArrayVar` changes the `.surface` +type line, and since `TestSurfaceSnapshot` compares whole lines, the old line +reads as a removal and is acknowledged in `.surface-breaking`. + +Carry the SDK's own caveat into help text: the filter matches the task's own +assignees, and **assignees on nested steps are not considered** — a card whose +step is assigned to someone does not match on that basis. + +**Two rejections, both before any request is issued:** + +- **`--assignee` with `--unassigned`.** The server builds that selector as + `todos_recordings.remaining.not_assigned` over a relation the assignee filter + has already narrowed + (`bc3:app/controllers/concerns/everything/todos/recordings.rb:24`). The + intersection is *necessarily* empty, so the combination would return zero rows + that look like a real answer. Same rule for the card selector. +- **`--due` with `--overdue` or `--no-due-date`.** Those two each select their + own endpoint on the same axis `--due` narrows, so combining them asks two + endpoints for one answer. + +`internal/dateparse` is deliberately not involved: these are category tokens, +not dates. An unrecognized `--due` value is `ErrUsage` naming the accepted set. + #### The `files` group's alias spellings `vaults` (aliases `vault`, `folders`) and `docs` (alias `documents`) are @@ -430,4 +496,17 @@ Attachment variants — every field read during flattening must be nil-checked. - The interactive project prompt no longer fires on these list commands when no project is configured; they list account-wide instead. - `todos list` with no project and `--overdue`/`--assignee` previously errored - with a redirect. `--overdue` now returns results; `--assignee` still errors. + with a redirect. Both now return results: `--overdue` since the bounded-walk + work, and `--assignee` since SDK v0.12.0 gave the aggregates an assignee + parameter. +- `todos list --assignee` is now **repeatable** and matches any of the named + people. It was single-valued; the `.surface` type line changes from `string` + to `stringArray`, acknowledged in `.surface-breaking`. +- `cards list` gains `--assignee` (account-wide only). The group's agent note + used to say cards do not support assignee filtering at all; that is now true + only of the project-scoped path. +- Both groups gain `--due with|without|overdue`, account-wide only, rejected + alongside `--overdue` and `--no-due-date`. +- `--assignee` with `--unassigned` is now a usage error on both groups. The + combination was previously reachable on `cards` only by not having the flag; + it is refused because the server makes it necessarily empty. diff --git a/e2e/smoke/smoke_core.bats b/e2e/smoke/smoke_core.bats index 307b84c1..ba0418e0 100644 --- a/e2e/smoke/smoke_core.bats +++ b/e2e/smoke/smoke_core.bats @@ -84,3 +84,15 @@ setup_file() { run_smoke basecamp todos create "smoke loose conflict" --loose --list 999999 --json assert_failure } + +@test "todos list rejects --assignee with --unassigned" { + # The server makes the intersection necessarily empty, so it is refused + # rather than answered with zero rows that look real. + run_smoke basecamp todos list --unassigned --assignee me --json + assert_failure +} + +@test "todos list rejects an unknown --due token" { + run_smoke basecamp todos list --due tomorrow --json + assert_failure +} diff --git a/internal/commands/accountwide.go b/internal/commands/accountwide.go index 6aabcda1..668c7dce 100644 --- a/internal/commands/accountwide.go +++ b/internal/commands/accountwide.go @@ -1,8 +1,10 @@ package commands import ( + "context" "fmt" "math" + "strings" "github.com/spf13/cobra" @@ -175,6 +177,151 @@ func accountWideCapNotice(capped bool, meta basecamp.ListMeta, count int, plural count, plural) } +// Account-wide task filters (--assignee, --due). +// +// These map onto EverythingTaskFilters, which the todo and card aggregates +// accept as a trailing parameter. They are genuinely server-side there: the +// request carries assignee_ids[] and due=, and the server narrows the listing +// before paginating. +// +// What that does *not* mean is a constant request count. The bounded walk's cap +// counts items, so a narrower filter can return fewer per page and need another +// page to reach the cap — a production soak measured 2 requests for 100 +// unfiltered todos against 3 for --assignee's 21. Filtering leaves the walk's +// algorithm untouched; the number of requests it takes is a property of the +// result, not of the filter. +// +// Project-scoped --assignee is a different animal — see the note on +// filterTodosByAssignees. + +// dueFilterValues are the tokens --due accepts. These are categories, not +// dates: internal/dateparse is deliberately not involved, since "overdue" is +// not a date and "with" is not a date range. +var dueFilterValues = []string{"with", "without", "overdue"} + +// rejectEmptyTaskFilterValues refuses an explicitly empty --due or --assignee. +// +// Every other check in this file tests the flag's *value*, which makes `--due=` +// indistinguishable from never passing --due: the project-scoped guard stops +// rejecting it, the account-wide path builds no filter, and the caller gets a +// full unfiltered listing believing they narrowed it. Presence is what makes it +// a request, so presence is what has to be tested — and it has to happen before +// account resolution, which can otherwise prompt on the way to a listing that +// was never going to be filtered. +// +// `--assignee=` is the same mistake in the other direction: StringArrayVar +// appends the empty string, so len(assignees) > 0 sends a filter that names +// nobody. +// It also validates the --due token here rather than only in +// validateAccountWideTaskFilters, which runs after ensureAccount. Neither the +// token set nor the emptiness check depends on the account or the scope, so +// leaving them late meant `todos list --due tomorrow` with no account +// configured hit account resolution first — an interactive session got the +// account picker and a noninteractive one got "--account is required", and the +// real error was never shown. A usage error that needs no account should not +// require one. +func validateTaskFilterValues(cmd *cobra.Command, due string, assignees []string) error { + if cmd.Flags().Changed("due") && due == "" { + return output.ErrUsageHint( + "--due needs a value", + fmt.Sprintf("Pass one of: %s", strings.Join(dueFilterValues, ", "))) + } + if err := validateDueFilter(due); err != nil { + return err + } + for _, assignee := range assignees { + if strings.TrimSpace(assignee) == "" { + return output.ErrUsageHint( + "--assignee needs a value", + "Pass a name or id, or drop the flag to list everyone's.") + } + } + return nil +} + +// validateDueFilter rejects an unknown --due token, naming the alternatives. +func validateDueFilter(due string) error { + if due == "" { + return nil + } + for _, valid := range dueFilterValues { + if due == valid { + return nil + } + } + return output.ErrUsageHint( + fmt.Sprintf("%q is not a valid --due filter", due), + "Pick one of: "+strings.Join(dueFilterValues, ", "), + ) +} + +// rejectAssigneeWithUnassigned refuses --assignee alongside the unassigned +// selector. +// +// The server builds that selector as todos_recordings.remaining.not_assigned, +// over a relation the assignee filter has already narrowed +// (bc3:app/controllers/concerns/everything/todos/recordings.rb:24). "Assigned +// to Ann" intersected with "assigned to nobody" is necessarily empty, so the +// combination cannot return a row. Refusing it beats returning zero results +// that look like a real answer. +func rejectAssigneeWithUnassigned(noun string) error { + return output.ErrUsageHint( + "--assignee and --unassigned cannot be combined (nothing can match both)", + fmt.Sprintf("Drop --unassigned to see that person's %s, or drop --assignee to see unassigned ones", noun), + ) +} + +// validateAccountWideTaskFilters enforces the combinations the filters cannot +// honor, before any request is issued. +// +// --due names the same axis as the dedicated due-date selectors: --overdue and +// --no-due-date each pick their own endpoint, and --due narrows a different +// one. Combining them asks two endpoints for one answer, so the flag that would +// be ignored is named instead. +func validateAccountWideTaskFilters(assignees []string, due string, unassigned, overdue, noDueDate bool, noun string) error { + if err := validateDueFilter(due); err != nil { + return err + } + if len(assignees) > 0 && unassigned { + return rejectAssigneeWithUnassigned(noun) + } + if due != "" { + switch { + case overdue: + return output.ErrUsageHint( + "--due and --overdue cannot be combined (each selects a different listing)", + "Use --overdue on its own, or --due overdue to narrow another listing") + case noDueDate: + return output.ErrUsageHint( + "--due and --no-due-date cannot be combined (each selects a different listing)", + "Use --no-due-date on its own, or --due without to narrow another listing") + } + } + return nil +} + +// accountWideTaskFilters resolves --assignee/--due into the SDK filter struct, +// returning nil when neither was passed so the call stays byte-identical to an +// unfiltered one. +func accountWideTaskFilters(ctx context.Context, app *appctx.App, assignees []string, due string) (*basecamp.EverythingTaskFilters, error) { + if len(assignees) == 0 && due == "" { + return nil, nil + } + + filters := &basecamp.EverythingTaskFilters{Due: due} + for _, assignee := range assignees { + // Each value may itself be a comma-separated list, so --assignee is + // repeatable and comma-separated both, matching how the other + // people-taking flags already behave. + ids, err := resolvePersonRoleIDs(ctx, app, assignee, "Assignee") + if err != nil { + return nil, err + } + filters.AssigneeIDs = append(filters.AssigneeIDs, ids...) + } + return filters, nil +} + // validateAccountWidePaginationFlags enforces the combination rules every // bounded account-wide listing shares: --all and --limit both answer "how much", // --page answers "which one", and mixing them asks for two different things at diff --git a/internal/commands/accountwide_filters_test.go b/internal/commands/accountwide_filters_test.go new file mode 100644 index 00000000..3c3c1e37 --- /dev/null +++ b/internal/commands/accountwide_filters_test.go @@ -0,0 +1,347 @@ +package commands + +import ( + "net/http" + "net/url" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/appctx" +) + +// Account-wide --assignee and --due. +// +// These are the feature the v0.12.0 signature change exists for. Two properties +// matter and are asserted separately: the filters must reach the wire as real +// query parameters, and, served identical pages, they must not change the shape +// of the bounded walk. +// +// The second is deliberately narrower than "the request count is the same". +// Against a real account it is not: the cap counts items, so a narrower filter +// can return fewer per page and need another page to reach it — a production +// soak measured 2 requests for 100 unfiltered todos against 3 for --assignee's +// 21. What is pinned here is the algorithm, on a same-fixture comparison. + +const ( + openTodosPath = "/99999/todos/open.json" + openCardsPath = "/99999/cards/open.json" +) + +func cardsAccountWideFilterRoute(path string) stubRoute { + body := `[{"bucket":{"id":977190,"name":"JD test proj","type":"Project"},"cards":[{"id":1,"title":"A card"}]}]` + return stubRoute{ + method: http.MethodGet, + path: path, + status: http.StatusOK, + body: body, + pages: []string{body}, + } +} + +// queryValues parses one recorded query string. +func queryValues(t *testing.T, raw string) url.Values { + t.Helper() + values, err := url.ParseQuery(raw) + require.NoError(t, err) + return values +} + +func TestTodosListAccountWideSendsAssigneeIDs(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, + "list", "--assignee", "42", "--assignee", "43")) + + queries := transport.queriesFor(openTodosPath) + require.NotEmpty(t, queries) + values := queryValues(t, queries[0]) + assert.Equal(t, []string{"42", "43"}, values["assignee_ids[]"], + "both people must reach the wire — --assignee is repeatable") +} + +// A single value may itself be comma-separated, matching how the other +// people-taking flags already behave. +func TestTodosListAccountWideAcceptsCommaSeparatedAssignees(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, "list", "--assignee", "42,43")) + + values := queryValues(t, transport.queriesFor(openTodosPath)[0]) + assert.Equal(t, []string{"42", "43"}, values["assignee_ids[]"]) +} + +func TestTodosListAccountWideSendsDue(t *testing.T) { + for _, due := range []string{"with", "without", "overdue"} { + t.Run(due, func(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, "list", "--due", due)) + + values := queryValues(t, transport.queriesFor(openTodosPath)[0]) + assert.Equal(t, due, values.Get("due")) + }) + } +} + +func TestCardsListAccountWideSendsFilters(t *testing.T) { + app, transport := setupRecordingTestApp(t, cardsAccountWideFilterRoute(openCardsPath)) + + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, + "list", "--all-projects", "--assignee", "42", "--due", "with")) + + queries := transport.queriesFor(openCardsPath) + require.NotEmpty(t, queries) + values := queryValues(t, queries[0]) + assert.Equal(t, []string{"42"}, values["assignee_ids[]"]) + assert.Equal(t, "with", values.Get("due")) +} + +// An unfiltered call must stay byte-identical to what it was before the filters +// existed: no filter passed means no filter parameter on the wire. +func TestAccountWideListingsOmitFilterParamsWhenUnused(t *testing.T) { + t.Run("todos", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, "list")) + + for _, q := range transport.queriesFor(openTodosPath) { + assert.NotContains(t, q, "assignee_ids") + assert.NotContains(t, q, "due=") + } + }) + + t.Run("cards", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, cardsAccountWideFilterRoute(openCardsPath)) + + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects")) + + for _, q := range transport.queriesFor(openCardsPath) { + assert.NotContains(t, q, "assignee_ids") + assert.NotContains(t, q, "due=") + } + }) +} + +// A server-side filter must not deepen the walk. Served the same page contents, +// a filtered listing walks exactly as far as an unfiltered one — the filter +// changes what the server selects, not how the client paginates. +// +// This is a same-fixture comparison on purpose. Against a real account the +// counts can differ by a page, because the cap counts items and a narrower +// filter returns fewer per page; that is the bounded walk working. What this +// pins is that filtering does not change the walk's shape. +func TestAccountWideFiltersDoNotDeepenTheWalk(t *testing.T) { + countRequests := func(t *testing.T, args ...string) int { + t.Helper() + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, args...)) + return len(transport.queriesFor(openTodosPath)) + } + + unfiltered := countRequests(t, "list") + filtered := countRequests(t, "list", "--assignee", "42", "--due", "with") + + assert.Equal(t, unfiltered, filtered, + "given identical page contents, filtering must not change the walk") +} + +// --assignee intersected with --unassigned is necessarily empty: the server +// builds the unassigned selector over a relation the assignee filter has +// already narrowed, so nothing can satisfy both. Returning zero rows would look +// like a real answer, so the combination is refused before any request. +func TestAccountWideRejectsAssigneeWithUnassigned(t *testing.T) { + t.Run("todos", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute("/99999/todos/unassigned.json", todosGroupsBody(1))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--unassigned", "--assignee", "42") + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, "--assignee and --unassigned") + assert.Empty(t, transport.recorded(), "an impossible query must not be issued") + }) + + t.Run("cards", func(t *testing.T) { + app, transport := setupRecordingTestApp(t, + cardsAccountWideFilterRoute("/99999/cards/unassigned.json")) + + err := executeRecordingCommand(NewCardsCmd(), app, + "list", "--all-projects", "--unassigned", "--assignee", "42") + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, "--assignee and --unassigned") + assert.Empty(t, transport.recorded(), "an impossible query must not be issued") + }) +} + +// --due names the same axis as the dedicated due-date selectors, each of which +// picks its own endpoint. Combining them asks two endpoints for one answer. +func TestAccountWideRejectsDueWithDueDateSelectors(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {"todos --due with --overdue", []string{"list", "--overdue", "--due", "with"}}, + {"todos --due with --no-due-date", []string{"list", "--no-due-date", "--due", "with"}}, + } { + t.Run(tc.name, func(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute("/99999/todos/overdue.json", todosGroupsBody(1)), + accountWideTodosRoute("/99999/todos/without_due_date.json", todosGroupsBody(1))) + + err := executeRecordingCommand(NewTodosCmd(), app, tc.args...) + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, "--due and") + assert.Empty(t, transport.recorded()) + }) + } +} + +func TestAccountWideRejectsUnknownDueToken(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + + err := executeRecordingCommand(NewTodosCmd(), app, "list", "--due", "tomorrow") + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, "tomorrow") + assert.Contains(t, outErr.Hint, "with, without, overdue", + "the hint must name the tokens, since these are categories rather than dates") + assert.Empty(t, transport.recorded()) +} + +// Both flags are account-wide only on cards, and --due is account-wide only on +// todos too. A project in scope makes them unanswerable, so they are refused by +// name rather than ignored. +func TestProjectScopedRejectsAccountWideOnlyFilters(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + wantMsg string + }{ + { + name: "cards --assignee", + cmd: NewCardsCmd, + args: []string{"list", "--in", "123", "--assignee", "42"}, + wantMsg: "--assignee filters the account-wide card listing only", + }, + { + name: "cards --due", + cmd: NewCardsCmd, + args: []string{"list", "--in", "123", "--due", "with"}, + wantMsg: "--due filters the account-wide card listing only", + }, + { + name: "todos --due", + cmd: NewTodosCmd, + args: []string{"list", "--in", "123", "--due", "with"}, + wantMsg: "--due filters the account-wide listing only", + }, + } { + t.Run(tc.name, func(t *testing.T) { + app, transport := setupRecordingTestApp(t, projectsRoute()) + + err := executeRecordingCommand(tc.cmd(), app, tc.args...) + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, tc.wantMsg) + + for _, call := range transport.recorded() { + assert.NotContains(t, strings.Join([]string{call.Path, call.Query}, "?"), "assignee_ids") + } + }) + } +} + +// `--due=` is a request that names nothing, and every other check in this file +// tests the flag's *value* — which makes an explicit empty string +// indistinguishable from never passing the flag at all. Left alone it slipped +// past the project-scoped guard, built no account-wide filter, and returned a +// full unfiltered listing to a caller who believed they had narrowed it. +// +// Both nouns and both scopes, because the two scopes reject on different paths: +// account-wide never had a guard, and the project-scoped one keys off a +// non-empty value. +func TestEmptyTaskFilterValuesAreRefusedBeforeAnyRequest(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + want string + }{ + {"todos account-wide --due=", NewTodosCmd, []string{"list", "--all-projects", "--due", ""}, "--due needs a value"}, + {"todos project-scoped --due=", NewTodosCmd, []string{"list", "--in", "123", "--due", ""}, "--due needs a value"}, + {"cards account-wide --due=", NewCardsCmd, []string{"list", "--all-projects", "--due", ""}, "--due needs a value"}, + {"cards project-scoped --due=", NewCardsCmd, []string{"list", "--in", "123", "--due", ""}, "--due needs a value"}, + {"todos --assignee=", NewTodosCmd, []string{"list", "--all-projects", "--assignee", ""}, "--assignee needs a value"}, + {"cards --assignee=", NewCardsCmd, []string{"list", "--all-projects", "--assignee", ""}, "--assignee needs a value"}, + } { + t.Run(tc.name, func(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + + err := executeRecordingCommand(tc.cmd(), app, tc.args...) + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, tc.want) + assert.Empty(t, transport.recorded(), + "an empty filter value must be refused before the account is resolved") + }) + } +} + +// setupNoAccountTestApp is setupRecordingTestApp with no configured account, so +// anything reaching ensureAccount fails with "--account is required". +func setupNoAccountTestApp(t *testing.T, routes ...stubRoute) (*appctx.App, *recordingTransport) { + t.Helper() + app, transport := setupRecordingTestApp(t, routes...) + app.Config.AccountID = "" + return app, transport +} + +// A filter usage error that needs no account must not demand one. +// +// The token and emptiness checks depend on neither the account nor the scope, +// but they used to run inside the account-wide path — after ensureAccount. With +// nothing configured, `todos list --due tomorrow` therefore hit account +// resolution first: an interactive session got the account picker, a +// noninteractive one got "--account is required", and the actual mistake was +// never reported. Asserting on the message is the point; asserting on the +// absence of requests alone would have passed before the fix too. +func TestTaskFilterUsageErrorsPrecedeAccountResolution(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + want string + }{ + {"todos unknown --due token", NewTodosCmd, []string{"list", "--due", "tomorrow"}, "tomorrow"}, + {"cards unknown --due token", NewCardsCmd, []string{"list", "--due", "tomorrow"}, "tomorrow"}, + {"todos empty --due", NewTodosCmd, []string{"list", "--due", ""}, "--due needs a value"}, + {"cards empty --assignee", NewCardsCmd, []string{"list", "--assignee", ""}, "--assignee needs a value"}, + } { + t.Run(tc.name, func(t *testing.T) { + app, transport := setupNoAccountTestApp(t, + accountWideTodosRoute(openTodosPath, todosGroupsBody(1))) + + err := executeRecordingCommand(tc.cmd(), app, tc.args...) + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, tc.want) + assert.NotContains(t, outErr.Message, "account is required", + "the filter mistake must be reported, not masked by account resolution") + assert.Empty(t, transport.recorded()) + }) + } +} diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 57de3c13..d53d1e88 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -29,7 +29,7 @@ func NewCardsCmd() *cobra.Command { Use: "cards", Short: "Manage cards in Card Tables", Long: "List, show, create, and manage cards in Card Tables (Kanban boards).", - Annotations: map[string]string{"agent_notes": "Cards do NOT support --assignee filtering like todos — fetch all and filter client-side\nIf a project has multiple card tables, you must specify --card-table \nAssign/unassign shortcuts work on cards: basecamp assign --to \nCross-project cards: basecamp recordings cards --json"}, + Annotations: map[string]string{"agent_notes": "--assignee filters the account-wide listing only; within a project, fetch all and filter client-side\nIf a project has multiple card tables, you must specify --card-table \nAssign/unassign shortcuts work on cards: basecamp assign --to \nCross-project cards: basecamp recordings cards --json"}, } cmd.PersistentFlags().StringVarP(&project, "project", "p", "", "Project ID or name") @@ -75,6 +75,8 @@ type cardsListOptions struct { noDueDate bool notNow bool overdue bool + assignees []string + due string } // Account-wide card listings, one per Everything endpoint. @@ -96,7 +98,12 @@ func newCardsListCmd(project, cardTable *string) *cobra.Command { Long: "List all cards in a project's card table, or across every project with --all-projects.\n\n" + "With --all-projects the listing comes from the account-wide card aggregates, grouped by\n" + "project. --status completed, --unassigned, --no-due-date, --not-now, and --overdue each\n" + - "select a different aggregate and are account-wide only.", + "select a different aggregate and are account-wide only.\n\n" + + "--assignee (repeatable) and --due (with, without, overdue) filter those aggregates and are\n" + + "account-wide only too — the project-scoped card listing has no equivalent for either.\n" + + "--assignee matches a card assigned to any of the named people; assignees on nested steps\n" + + "are not considered. --assignee cannot be combined with --unassigned (nothing can match\n" + + "both), and --due cannot be combined with --overdue or --no-due-date.", RunE: func(cmd *cobra.Command, args []string) error { opts.project = *project opts.cardTable = *cardTable @@ -116,6 +123,8 @@ func newCardsListCmd(project, cardTable *string) *cobra.Command { cmd.Flags().BoolVar(&opts.unassigned, "unassigned", false, "Account-wide only: cards with no assignee") cmd.Flags().BoolVar(&opts.noDueDate, "no-due-date", false, "Account-wide only: cards with no due date") cmd.Flags().BoolVar(&opts.notNow, "not-now", false, "Account-wide only: cards parked in Not now") + cmd.Flags().StringArrayVar(&opts.assignees, "assignee", nil, "Account-wide only: filter by assignee (repeatable)") + cmd.Flags().StringVar(&opts.due, "due", "", "Account-wide only: filter by due date (with, without, overdue)") cmd.Flags().BoolVar(&opts.overdue, "overdue", false, "Account-wide only: overdue cards, oldest due date first") return cmd @@ -139,6 +148,13 @@ func runCardsList(cmd *cobra.Command, opts cardsListOptions) error { } } + // Scope- and account-independent filter validation: an explicitly empty + // --due/--assignee, and an unknown --due token. Before ensureAccount, so a + // usage error that needs no account does not demand one. + if err := validateTaskFilterValues(cmd, opts.due, opts.assignees); err != nil { + return err + } + selector, selectorFlag, err := cardsAccountWideSelector(opts) if err != nil { return err @@ -165,6 +181,22 @@ func runCardsList(cmd *cobra.Command, opts cardsListOptions) error { return runCardsListAccountWide(cmd, app, opts, selector) } + // --assignee and --due are parameters on the account-wide aggregates. The + // project-scoped card listing has no equivalent for either, so they are + // refused by name rather than silently ignored. + if len(opts.assignees) > 0 { + return output.ErrUsageHint( + "--assignee filters the account-wide card listing only", + "Pass --all-projects to filter across every project, or drop --assignee to list this project's cards.", + ) + } + if opts.due != "" { + return output.ErrUsageHint( + "--due filters the account-wide card listing only", + "Pass --all-projects to filter across every project, or drop --due to list this project's cards.", + ) + } + // The endpoint selectors reach account-wide aggregates that have no // project-scoped equivalent, so a project in scope makes them unanswerable // rather than a no-op. @@ -300,6 +332,21 @@ func runCardsListAccountWide(cmd *cobra.Command, app *appctx.App, opts cardsList // 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. + if err := validateAccountWideTaskFilters(opts.assignees, opts.due, opts.unassigned, + opts.overdue, opts.noDueDate, "cards"); err != nil { + return err + } + + // Server-side: these become assignee_ids[] and due= on the request, so the + // server narrows the listing before it is paginated. The bounded walk's + // algorithm is unchanged, but its request count is not guaranteed to match + // an unfiltered run — the cap counts items, so a narrower result can need an + // extra page. See the note on accountWideTaskFilters. + taskFilters, err := accountWideTaskFilters(cmd.Context(), app, opts.assignees, opts.due) + if err != nil { + return err + } + limit := opts.limit if limit == 0 { limit = accountWideDefaultLimit @@ -313,15 +360,15 @@ func runCardsListAccountWide(cmd *cobra.Command, app *appctx.App, opts cardsList ) switch selector { case cardsSelectorCompleted: - groupsPage, err = everything.CompletedCards(cmd.Context(), page, nil) + groupsPage, err = everything.CompletedCards(cmd.Context(), page, taskFilters) case cardsSelectorUnassigned: - groupsPage, err = everything.UnassignedCards(cmd.Context(), page, nil) + groupsPage, err = everything.UnassignedCards(cmd.Context(), page, taskFilters) case cardsSelectorNoDueDate: - groupsPage, err = everything.NoDueDateCards(cmd.Context(), page, nil) + groupsPage, err = everything.NoDueDateCards(cmd.Context(), page, taskFilters) case cardsSelectorNotNow: - groupsPage, err = everything.NotNowCards(cmd.Context(), page, nil) + groupsPage, err = everything.NotNowCards(cmd.Context(), page, taskFilters) default: - groupsPage, err = everything.OpenCards(cmd.Context(), page, nil) + groupsPage, err = everything.OpenCards(cmd.Context(), page, taskFilters) } if err != nil { return nil, basecamp.ListMeta{}, convertSDKError(err) @@ -388,7 +435,17 @@ func runCardsListOverdue(cmd *cobra.Command, app *appctx.App, opts cardsListOpti return output.ErrUsage("--sort position requires --column (position is per-column)") } - cards, err := app.Account().Everything().OverdueCards(cmd.Context(), nil) + if err := validateAccountWideTaskFilters(opts.assignees, opts.due, opts.unassigned, + opts.overdue, opts.noDueDate, "cards"); err != nil { + return err + } + + taskFilters, err := accountWideTaskFilters(cmd.Context(), app, opts.assignees, opts.due) + if err != nil { + return err + } + + cards, err := app.Account().Everything().OverdueCards(cmd.Context(), taskFilters) if err != nil { return convertSDKError(err) } diff --git a/internal/commands/todos.go b/internal/commands/todos.go index f91a2f16..6850ed40 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -28,7 +28,8 @@ type todosListFlags struct { allProjects bool todolist string todoset string - assignee string + assignees []string + due string status string completed bool overdue bool @@ -47,7 +48,7 @@ func NewTodosCmd() *cobra.Command { Use: "todos", Short: "Manage todos", Long: "List, show, create, and manage Basecamp todos.", - Annotations: map[string]string{"agent_notes": "--assignee only works on todos, not cards or other content types\nbasecamp todos complete accepts multiple IDs: basecamp todos complete 1 2 3\nbasecamp todos list without a project lists every project's todos; --all-projects forces that over a configured default\n--assignee requires a project (--in, global flag, or config default); for cross-project use basecamp reports assigned"}, + Annotations: map[string]string{"agent_notes": "basecamp todos complete accepts multiple IDs: basecamp todos complete 1 2 3\nbasecamp todos list without a project lists every project's todos; --all-projects forces that over a configured default\n--assignee works in both scopes but differently: account-wide (--all-projects, or no project in scope) it is a server-side filter; inside a project it is applied client-side. --due is account-wide only"}, } cmd.AddCommand( @@ -78,7 +79,18 @@ func newTodosListCmd() *cobra.Command { With no project in scope, todos are listed across every project you can see. --all-projects forces that listing over a configured default project, and --unassigned/--no-due-date select account-wide filters that have no -project-scoped equivalent.`, +project-scoped equivalent. + +--assignee is repeatable and matches a todo assigned to any of the named +people. Account-wide it is a server-side filter; within a project the API has +no assignee parameter, so it is applied client-side over an unlimited fetch — +same results, very different cost. Assignees on nested steps are not +considered. + +--due (with, without, overdue) filters the account-wide listing only, and +cannot be combined with --overdue or --no-due-date, which each select their own +listing on that same axis. --assignee cannot be combined with --unassigned: +nothing can match both.`, RunE: func(cmd *cobra.Command, args []string) error { return runTodosList(cmd, flags) }, @@ -89,7 +101,11 @@ project-scoped equivalent.`, cmd.Flags().BoolVar(&flags.allProjects, "all-projects", false, "List todos across every project (overrides a configured project)") cmd.Flags().StringVarP(&flags.todolist, "list", "l", "", "Todolist ID") cmd.Flags().StringVarP(&flags.todoset, "todoset", "t", "", "Todoset ID (for projects with multiple todosets)") - cmd.Flags().StringVar(&flags.assignee, "assignee", "", "Filter by assignee") + // Repeatable, and account-wide it is a real server-side filter. Widening + // from StringVar changes the .surface type line, which reads as a removal — + // acknowledged in .surface-breaking. + cmd.Flags().StringArrayVar(&flags.assignees, "assignee", nil, "Filter by assignee (repeatable; account-wide it is server-side)") + cmd.Flags().StringVar(&flags.due, "due", "", "Filter by due date: with, without, overdue (account-wide only)") cmd.Flags().StringVarP(&flags.status, "status", "s", "", "Filter by status (completed, incomplete, archived, trashed)") cmd.Flags().BoolVar(&flags.completed, "completed", false, "Show completed todos (shorthand for --status completed)") cmd.Flags().BoolVar(&flags.overdue, "overdue", false, "Filter overdue todos") @@ -134,6 +150,13 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { } } + // Scope- and account-independent filter validation: an explicitly empty + // --due/--assignee, and an unknown --due token. Before ensureAccount, so a + // usage error that needs no account does not demand one. + if err := validateTaskFilterValues(cmd, flags.due, flags.assignees); err != nil { + return err + } + // Pick the scope before validating against it: the account-wide endpoints // take any positive page, while the project path only permits page 1. if flags.allProjects && (flags.project != "" || app.Flags.Project != "") { @@ -172,6 +195,15 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { return err } + // --due is a parameter on the account-wide aggregates only; the + // project-scoped listing has no equivalent, so it is refused rather than + // dropped. + if flags.due != "" { + return output.ErrUsageHint( + "--due filters the account-wide listing only", + "Drop --project/--in to filter across all projects, or use --overdue within this one") + } + // Use project from flag, global flag, or config. One of the three is set — // otherwise the account-wide branch above answered the listing, so there is // nothing left to prompt for. @@ -201,7 +233,7 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { // If todolist is specified, list todos in that list if todolist != "" { - return listTodosInList(cmd, app, project, todolist, flags.assignee, sdkStatus, sdkCompleted, flags.limit, flags.all, flags.sortField, flags.reverse) + return listTodosInList(cmd, app, project, todolist, flags.assignees, sdkStatus, sdkCompleted, flags.limit, flags.all, flags.sortField, flags.reverse) } // --page is not meaningful when aggregating across todolists @@ -211,7 +243,7 @@ func runTodosList(cmd *cobra.Command, flags todosListFlags) error { } // Otherwise, get all todos from project's todoset - return listAllTodos(cmd, app, project, flags.todoset, flags.assignee, sdkStatus, sdkCompleted, flags.overdue, flags.limit, flags.all, flags.sortField, flags.reverse) + return listAllTodos(cmd, app, project, flags.todoset, flags.assignees, sdkStatus, sdkCompleted, flags.overdue, flags.limit, flags.all, flags.sortField, flags.reverse) } // todosAccountWideFilter names the account-wide todo aggregate a listing maps @@ -240,6 +272,11 @@ func listTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags todosLis return err } + if err := validateAccountWideTaskFilters(flags.assignees, flags.due, flags.unassigned, + flags.overdue, flags.noDueDate, "todos"); err != nil { + return err + } + if flags.limit < 0 { return output.ErrUsage("--limit cannot be negative") } @@ -293,10 +330,6 @@ func rejectProjectScopedTodosFlags(app *appctx.App, flags todosListFlags) error return output.ErrUsageHint( "--todoset names a todoset inside one project, which has no meaning across all projects", fmt.Sprintf("List that todoset: basecamp todos list --in --todoset %s", flags.todoset)) - case flags.assignee != "": - return output.ErrUsageHint( - "--assignee has no account-wide equivalent", - "For cross-project assigned todos: basecamp reports assigned") } return nil } @@ -350,6 +383,16 @@ func selectAccountWideTodosFilter(flags todosListFlags) (todosAccountWideFilter, // listGroupedTodosAcrossProjects fetches one of the paginated aggregates, whose // payload is nested by project. func listGroupedTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags todosListFlags, filter todosAccountWideFilter) error { + // Server-side here, unlike the project-scoped path: these become + // assignee_ids[] and due= on the request, so the server narrows the listing + // before it is paginated. That does not fix the request count — the bounded + // walk's cap counts items, so a narrower result can take an extra page to + // fill it. See the note on accountWideTaskFilters. + taskFilters, err := accountWideTaskFilters(cmd.Context(), app, flags.assignees, flags.due) + if err != nil { + return err + } + limit := flags.limit if limit == 0 { limit = accountWideDefaultLimit @@ -364,14 +407,14 @@ func listGroupedTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t if err != nil { return err } - page, err := fetchAccountWideTodoGroups(cmd.Context(), app, filter, sdkPage) + page, err := fetchAccountWideTodoGroups(cmd.Context(), app, filter, sdkPage, taskFilters) if err != nil { return convertSDKError(err) } groups = page.Groups truncated = page.Meta.Truncated } else { - collected, more, err := collectAccountWideTodoGroups(cmd.Context(), app, filter, limit) + collected, more, err := collectAccountWideTodoGroups(cmd.Context(), app, filter, limit, taskFilters) if err != nil { return convertSDKError(err) } @@ -427,7 +470,12 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t return output.ErrUsage("--sort position requires --list (position is per-todolist)") } - todos, err := app.Account().Everything().OverdueTodos(cmd.Context(), nil) + taskFilters, err := accountWideTaskFilters(cmd.Context(), app, flags.assignees, flags.due) + if err != nil { + return err + } + + todos, err := app.Account().Everything().OverdueTodos(cmd.Context(), taskFilters) if err != nil { return convertSDKError(err) } @@ -481,10 +529,10 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t // requested number of todos, which is cheaper than fetching every page only to // truncate. The second return reports that collection stopped at the cap rather // than at the end of the listing. -func collectAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter todosAccountWideFilter, limit int) ([]basecamp.BucketTodosGroup, bool, error) { +func collectAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter todosAccountWideFilter, limit int, taskFilters *basecamp.EverythingTaskFilters) ([]basecamp.BucketTodosGroup, bool, error) { groups, capped, _, err := accountWideCollect( func(page int32) ([]basecamp.BucketTodosGroup, basecamp.ListMeta, error) { - result, err := fetchAccountWideTodoGroups(ctx, app, filter, page) + result, err := fetchAccountWideTodoGroups(ctx, app, filter, page, taskFilters) if err != nil { return nil, basecamp.ListMeta{}, err } @@ -498,17 +546,17 @@ func collectAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter t // fetchAccountWideTodoGroups calls the aggregate the filter selects. Page 0 // follows the Link header across every page. -func fetchAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter todosAccountWideFilter, page int32) (*basecamp.BucketTodosGroupsPage, error) { +func fetchAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter todosAccountWideFilter, page int32, taskFilters *basecamp.EverythingTaskFilters) (*basecamp.BucketTodosGroupsPage, error) { everything := app.Account().Everything() switch filter { case todosFilterCompleted: - return everything.CompletedTodos(ctx, page, nil) + return everything.CompletedTodos(ctx, page, taskFilters) case todosFilterUnassigned: - return everything.UnassignedTodos(ctx, page, nil) + return everything.UnassignedTodos(ctx, page, taskFilters) case todosFilterNoDueDate: - return everything.NoDueDateTodos(ctx, page, nil) + return everything.NoDueDateTodos(ctx, page, taskFilters) default: - return everything.OpenTodos(ctx, page, nil) + return everything.OpenTodos(ctx, page, taskFilters) } } @@ -721,7 +769,7 @@ func fetchTodosIncludingGroups(ctx context.Context, app *appctx.App, todolistID return result, totalCount, nil } -func listTodosInList(cmd *cobra.Command, app *appctx.App, project, todolist, assignee, sdkStatus string, sdkCompleted bool, limit int, all bool, sortField string, reverse bool) error { +func listTodosInList(cmd *cobra.Command, app *appctx.App, project, todolist string, assignees []string, sdkStatus string, sdkCompleted bool, limit int, all bool, sortField string, reverse bool) error { resolvedTodolist, _, err := app.Names.ResolveTodolist(cmd.Context(), todolist, project) if err != nil { return err @@ -740,7 +788,7 @@ func listTodosInList(cmd *cobra.Command, app *appctx.App, project, todolist, ass // When assignee filtering is active, fetch all so client-side filtering // doesn't miss matches beyond the default cap. sdkLimit := 0 // SDK default - if all || assignee != "" { + if all || len(assignees) > 0 { sdkLimit = -1 } else if limit > 0 { sdkLimit = limit @@ -751,21 +799,20 @@ func listTodosInList(cmd *cobra.Command, app *appctx.App, project, todolist, ass return convertSDKError(err) } - // Filter by assignee client-side (API has no server-side assignee filter) - if assignee != "" { - resolvedID, _, err := app.Names.ResolvePerson(cmd.Context(), assignee) + // Project-scoped --assignee is a client-side filter: this endpoint has no + // server-side assignee parameter, which is why the fetch above is unlimited + // whenever one is set. Account-wide the same flag is a real assignee_ids[] + // query parameter — same spelling, very different cost. + if len(assignees) > 0 { + assigneeIDs, err := resolveAssigneeFilterIDs(cmd.Context(), app, assignees) if err != nil { - return fmt.Errorf("failed to resolve assignee '%s': %w", assignee, err) + return err } - assigneeID, _ := strconv.ParseInt(resolvedID, 10, 64) - if assigneeID != 0 { + if len(assigneeIDs) > 0 { filtered := todos[:0] for _, todo := range todos { - for _, a := range todo.Assignees { - if a.ID == assigneeID { - filtered = append(filtered, todo) - break - } + if todoMatchesAnyAssignee(todo, assigneeIDs) { + filtered = append(filtered, todo) } } todos = filtered @@ -775,7 +822,7 @@ func listTodosInList(cmd *cobra.Command, app *appctx.App, project, todolist, ass // Apply --limit after client-side filtering so the cap reflects // the filtered set, not the pre-filter fetch. - if assignee != "" && !all && limit > 0 && len(todos) > limit { + if len(assignees) > 0 && !all && limit > 0 && len(todos) > limit { todos = todos[:limit] } @@ -808,7 +855,36 @@ func listTodosInList(cmd *cobra.Command, app *appctx.App, project, todolist, ass return app.OK(todos, respOpts...) } -func listAllTodos(cmd *cobra.Command, app *appctx.App, project, todosetFlag, assignee, sdkStatus string, sdkCompleted bool, overdue bool, limit int, all bool, sortField string, reverse bool) error { +// resolveAssigneeFilterIDs resolves the repeatable --assignee into person ids. +// Each value may itself be comma-separated, so both spellings work. +func resolveAssigneeFilterIDs(ctx context.Context, app *appctx.App, assignees []string) ([]int64, error) { + ids := make([]int64, 0, len(assignees)) + for _, assignee := range assignees { + resolved, err := resolvePersonRoleIDs(ctx, app, assignee, "Assignee") + if err != nil { + return nil, err + } + ids = append(ids, resolved...) + } + return ids, nil +} + +// todoMatchesAnyAssignee reports whether the todo is assigned to any of the +// given people. Any rather than all: --assignee ann --assignee bob asks for +// what either of them is on, matching the server-side assignee_ids[] semantics +// the account-wide path gets for free. +func todoMatchesAnyAssignee(todo basecamp.Todo, assigneeIDs []int64) bool { + for _, a := range todo.Assignees { + for _, id := range assigneeIDs { + if a.ID == id { + return true + } + } + } + return false +} + +func listAllTodos(cmd *cobra.Command, app *appctx.App, project, todosetFlag string, assignees []string, sdkStatus string, sdkCompleted bool, overdue bool, limit int, all bool, sortField string, reverse bool) error { // Position is only meaningful within a single todolist — reject before // the --all check so users get the right error message. if sortField == "position" { @@ -819,17 +895,17 @@ func listAllTodos(cmd *cobra.Command, app *appctx.App, project, todosetFlag, ass // (assignee/overdue) forces an unlimited per-list fetch below. Otherwise // results are sampled per-todolist using default SDK paging and a sort // would be misleading. - if sortField != "" && !all && assignee == "" && !overdue { + if sortField != "" && !all && len(assignees) == 0 && !overdue { return output.ErrUsage("--sort requires --all (or --assignee/--overdue) when listing across todolists (results are otherwise sampled per list)") } - // Resolve assignee name to ID if provided - var assigneeID int64 - if assignee != "" { - resolvedID, _, err := app.Names.ResolvePerson(cmd.Context(), assignee) - if err != nil { - return fmt.Errorf("failed to resolve assignee '%s': %w", assignee, err) + // Resolve assignee names to IDs if provided. Client-side again: a todo + // matches when any one of the named people is on it. + var assigneeIDs []int64 + if len(assignees) > 0 { + var err error + if assigneeIDs, err = resolveAssigneeFilterIDs(cmd.Context(), app, assignees); err != nil { + return err } - assigneeID, _ = strconv.ParseInt(resolvedID, 10, 64) } // Get todoset ID from project dock (with interactive fallback for multi-todoset projects) @@ -854,7 +930,7 @@ func listAllTodos(cmd *cobra.Command, app *appctx.App, project, todosetFlag, ass // doesn't miss matches beyond the default cap — mirroring the single-list // path. Any explicit --limit is then applied after filtering, below. sdkLimit := 0 // SDK default - if all || assignee != "" || overdue { + if all || len(assignees) > 0 || overdue { sdkLimit = -1 } else if limit > 0 { sdkLimit = limit @@ -888,18 +964,9 @@ func listAllTodos(cmd *cobra.Command, app *appctx.App, project, todosetFlag, ass // Apply filters var result []basecamp.Todo for _, todo := range allTodos { - // Filter by assignee (using resolved ID) - if assigneeID != 0 { - found := false - for _, a := range todo.Assignees { - if a.ID == assigneeID { - found = true - break - } - } - if !found { - continue - } + // Filter by assignee (any of the resolved IDs) + if len(assigneeIDs) > 0 && !todoMatchesAnyAssignee(todo, assigneeIDs) { + continue } // Filter overdue - check if due date is in the past and not completed @@ -920,7 +987,7 @@ func listAllTodos(cmd *cobra.Command, app *appctx.App, project, todosetFlag, ass // When a client-side filter forced an unlimited fetch above, apply the // explicit --limit after filtering so the cap reflects the filtered set // rather than the pre-filter fetch (mirrors the single-list path). - if (assignee != "" || overdue) && !all && limit > 0 && len(result) > limit { + if (len(assignees) > 0 || overdue) && !all && limit > 0 && len(result) > limit { result = result[:limit] } diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 4402c45a..5219f550 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -422,17 +422,17 @@ func TestTodosCreateContentIsPlainText(t *testing.T) { "--notify-on-completion must map to completion_subscriber_ids") } -func TestTodosListAssigneeWithoutProjectErrors(t *testing.T) { - app, _ := setupTodosTestApp(t) +// --assignee used to be rejected account-wide, because the aggregates had no +// assignee parameter to map it onto. SDK v0.12.0 added one, so the flag now +// works in both scopes and the rejection is gone. +func TestTodosListAssigneeWithoutProjectIsAccountWide(t *testing.T) { + app, transport := setupRecordingTestApp(t, + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(1))) - cmd := NewTodosCmd() - err := executeTodosCommand(cmd, app, "list", "--assignee", "me") - require.Error(t, err) + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, "list", "--assignee", "42")) - var e *output.Error - require.True(t, errors.As(err, &e)) - assert.Contains(t, e.Message, "--assignee has no account-wide equivalent") - assert.Contains(t, e.Hint, "reports assigned") + assert.Equal(t, "/99999/todos/open.json", transport.last(t).Path) + assert.Contains(t, transport.last(t).Query, "assignee_ids") } // TestTodosListOverdueWithoutProjectListsAcrossProjects covers the behavior @@ -3190,7 +3190,6 @@ func TestTodosListAccountWideRejectsProjectOnlyFilters(t *testing.T) { app, _ := setupRecordingTestApp(t) requireTodosListUsageError(t, app, "--todoset names a todoset inside one project", "--todoset", "789") - requireTodosListUsageError(t, app, "--assignee has no account-wide equivalent", "--assignee", "me") requireTodosListUsageError(t, app, "--status archived has no account-wide equivalent", "--status", "archived") requireTodosListUsageError(t, app, "--status trashed has no account-wide equivalent", "--status", "trashed") requireTodosListUsageError(t, app, `unknown --status value "nonsense"`, "--status", "nonsense") diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 66a8b391..d7f4e93b 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -146,7 +146,7 @@ basecamp todos --agent --help "inherited_flags":[{"name":"json","shorthand":"j","type":"bool","default":"false","usage":"..."}]} ``` -Walk the tree: start at `basecamp --agent --help` for top-level commands, then drill into any subcommand. Commands include `notes` with domain-specific agent hints (e.g., "Cards do NOT support --assignee filtering"). +Walk the tree: start at `basecamp --agent --help` for top-level commands, then drill into any subcommand. Commands carry domain-specific agent hints (e.g., "`--assignee` filters the account-wide listing only; within a project, fetch all and filter client-side"). ### Pagination @@ -161,7 +161,8 @@ basecamp --page 1 # First page only, no auto-pagination ### Smart Defaults - `--assignee me` resolves to current user -- `--due tomorrow` / `--due +3` / `--due "next week"` - natural date parsing +- `--due tomorrow` / `--due +3` / `--due "next week"` — natural date parsing, **when setting a due date** (`todos create`, `todos update`, `cards create`, and so on) +- `--due` on a **listing** is a different flag and does not take dates: it accepts only `with`, `without`, or `overdue`, and only account-wide. `basecamp todos list --due tomorrow` is rejected. For date-based listing use `--overdue`, `--no-due-date`, or `basecamp assignments due ` - Project from `.basecamp/config.json` if `--in` not specified - Multiple identities use named profiles: `basecamp profile create `, then select one with global `--profile ` or `BASECAMP_PROFILE=`. @@ -181,6 +182,10 @@ basecamp --page 1 # First page only, no auto-pagination | Overdue todos (in project) | `basecamp todos list --overdue --in --json` | | Overdue todos (cross-project) | `basecamp todos list --all-projects --overdue --json` (flat, oldest first) or `basecamp reports overdue --json` (bucketed by lateness) | | All cards (cross-project) | `basecamp cards list --all-projects --json` (grouped by project) | +| Someone's todos (cross-project) | `basecamp todos list --all-projects --assignee "Ann" --json` (server-side filter) | +| Two people's todos (cross-project) | `basecamp todos list --all-projects --assignee ann --assignee bob --json` (matches either) | +| Someone's cards (cross-project) | `basecamp cards list --all-projects --assignee "Ann" --json` | +| Todos with no due date set (cross-project) | `basecamp todos list --all-projects --due without --json` | | My bookmarks | `basecamp bookmarks list --json` | | Bookmark something | `basecamp bookmarks add --json` | | Is it bookmarked? | `basecamp bookmarks check --json` (always exits 0) | @@ -511,7 +516,7 @@ basecamp todos update --notify-on-completion "Jane" # Set who's notified o basecamp todos update --no-notify-on-completion # Clear completion notifications ``` -**Flags:** `--assignee` (todos only - not available on cards/messages), `--status` (completed/incomplete/archived/trashed), `--overdue`, `--list`, `--due`, `--limit`, `--all` +**Flags:** `--assignee` (repeatable; server-side account-wide, client-side within a project; also on `cards list` account-wide, but not on messages), `--status` (completed/incomplete/archived/trashed), `--overdue`, `--list`, `--due` (**listing filter: `with`/`without`/`overdue` only, account-wide only** — not a date; see Smart Defaults), `--limit`, `--all` **Completion subscribers** ("When done, notify…"): set with `--notify-on-completion ` on `todos create` and @@ -612,7 +617,7 @@ the same todoset, top to bottom. It always places them at the top. ### Cards (Kanban) -**Note:** Cards do NOT support `--assignee` filtering like todos. Fetch all cards and filter client-side if needed. If a project has multiple card tables, you must specify `--card-table `. When you get an "Ambiguous card table" error, the hint shows available table IDs and names. +**Note:** `--assignee` on `cards list` is **account-wide only** — pass `--all-projects` (or have no project in scope) and it becomes a real server-side filter. Within a single project cards have no assignee filter: fetch all and filter client-side. `--due with|without|overdue` is account-wide only on cards too. If a project has multiple card tables, you must specify `--card-table `. When you get an "Ambiguous card table" error, the hint shows available table IDs and names. ```bash basecamp cards list --in --json # All cards @@ -963,6 +968,28 @@ basecamp assignments due due_later_this_week --json # Due later this week **Scopes:** overdue, due_today, due_tomorrow, due_later_this_week, due_next_week, due_later. +**Cross-project assignee filtering:** `basecamp todos list --all-projects +--assignee ` and `basecamp cards list --all-projects --assignee ` +filter server-side across every project. Both are repeatable and match a task +assigned to **any** of the named people. Assignees on nested steps are not +considered, so a card whose step is assigned to someone does not match on that +basis. + +**Always pass `--all-projects` when you mean every project.** Without it these +listings are account-wide *only* when no project is in scope — and a configured +default project counts as in scope. With one configured, `--assignee` silently +degrades to a client-side filter over that single project, and `--due` is +rejected outright as account-wide-only. `--all-projects` is what overrides a +configured default, so a recipe that omits it returns different results +depending on the reader's config. + +Within a project `--assignee` still works on todos, but there is no server-side +filter, so it fetches everything and narrows client-side. Cards have no +project-scoped `--assignee` at all. `--due with|without|overdue` is account-wide +only on both, and conflicts with `--overdue` and `--no-due-date`, which select +their own listings on the same axis. `--assignee` with `--unassigned` is refused +— the server makes that combination necessarily empty. + **Up Next** — reorder the priority list: ```bash