Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .surface
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions .surface-breaking
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
87 changes: 83 additions & 4 deletions ACCOUNT-WIDE-LISTINGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 |

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
12 changes: 12 additions & 0 deletions e2e/smoke/smoke_core.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
147 changes: 147 additions & 0 deletions internal/commands/accountwide.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package commands

import (
"context"
"fmt"
"math"
"strings"

"github.com/spf13/cobra"

Expand Down Expand Up @@ -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
}
Comment thread
jeremy marked this conversation as resolved.
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
Expand Down
Loading
Loading