diff --git a/skills/Jira/CliReference.md b/skills/Jira/CliReference.md index 41d6595f..7623b7f6 100644 --- a/skills/Jira/CliReference.md +++ b/skills/Jira/CliReference.md @@ -29,7 +29,7 @@ Prompts for: Atlassian instance URL, email, API token (from https://id.atlassian | Flag | Description | |------|-------------| -| `--fulltext` | Disable truncation of descriptions and comments (`--no-truncate` is a deprecated alias kept during migration) | +| `--fulltext` | Disable truncation of descriptions and comments without changing list output shape | | `--id` | Emit only the primary identifier (useful for scripting). Takes precedence over `--fulltext` | | `--no-color` | Disable colored output | | `-v, --verbose` | Enable verbose output | @@ -150,7 +150,7 @@ Common transition names: "To Do", "In Progress", "In Review", "Done" (instance-d ## Sprints -`--board` and sprint positional arguments accept either a numeric ID or a name (resolved via cache — see SKILL.md Cache Warming). +`--board` and sprint positional arguments accept either a numeric ID or a name (resolved via cache — see SKILL.md Cache Warming). Sprint names must resolve uniquely; ambiguity reports candidate IDs, while unresolved names require a cache refresh or numeric ID. | Command | Description | |---------|-------------| diff --git a/skills/Jira/SKILL.md b/skills/Jira/SKILL.md index 74b83f37..cc9bf49c 100644 --- a/skills/Jira/SKILL.md +++ b/skills/Jira/SKILL.md @@ -87,7 +87,7 @@ Use `jtk refresh --status` to inspect cache freshness without fetching anything. `jtk` commands follow a text-first output model (per the repo's [Output Artifact Contract](https://github.com/open-cli-collective/atlassian-cli/blob/main/docs/ARTIFACT_CONTRACT.md)): - **`--fields `** — explicitly selects supported fields on commands that expose field projection. -- **`--fulltext`** — disables truncation of descriptions and comments. Use when the user needs full body content (e.g., "show the full description"). `--no-truncate` is a deprecated alias kept during the migration; prefer `--fulltext`. +- **`--fulltext`** — disables truncation of descriptions and comments without changing list output shape. Use when the user needs full body content (e.g., "show the full description"). - **`--id`** — emits only the primary identifier (issue key, account ID, etc.) and takes precedence over `--fulltext` and `--fields` (projection is bypassed entirely). Stdout is identifier-only, one per line, safe to pipe; any continuation notice goes to stderr. `automation export` is the only resource command that emits JSON (`set-credential --json` is the control-plane exception). For every other command, use `--id` for scripting composition. diff --git a/skills/Jira/Workflows/ManageComments.md b/skills/Jira/Workflows/ManageComments.md index c84f0e35..39f2a379 100644 --- a/skills/Jira/Workflows/ManageComments.md +++ b/skills/Jira/Workflows/ManageComments.md @@ -20,7 +20,7 @@ View and add comments on Jira issues. jtk comments list PROJ-123 ``` -Optional: `--max N` to control page size (default 50). Use `--fulltext` global flag to disable truncation of long comment bodies. +Optional: `--max N` to control page size (default 50). Use `--fulltext` to disable truncation of long comment bodies without changing the table shape. ### Add Comment @@ -55,7 +55,7 @@ The `COMMENT_ID` is a numeric ID (e.g., `12345`). If the user refers to the comm ## Output Format -- **List comments:** Show each comment with author, timestamp, and body +- **List comments:** A table with ID, author, timestamp, and body; `--fulltext` preserves these columns - **Add comment:** Confirm comment was added with issue key and a snippet of the comment text ## Post-Action diff --git a/tools/jtk/CHANGELOG.md b/tools/jtk/CHANGELOG.md index 30faf1c1..9f63cbb2 100644 --- a/tools/jtk/CHANGELOG.md +++ b/tools/jtk/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Breaking:** Removed global `--extended` and `issues list/search --all-fields`. Use explicit `--fields`; `--fulltext` controls truncation only. ([#455](https://github.com/open-cli-collective/atlassian-cli/issues/455)) - **Breaking:** Pagination and continuation prose now goes to stderr, keeping `--id` stdout identifier-only. `--max` is a positive single-page size; issue list/search no longer auto-exhaust pages. ([#455](https://github.com/open-cli-collective/atlassian-cli/issues/455)) +- **Breaking:** `comments list --fulltext` now preserves table shape and only disables truncation; deprecated `--no-truncate` aliases were removed. Sprint-name filters now fail closed on ambiguous or unresolved names. Automation export now validates and deterministically formats API JSON. ([#455](https://github.com/open-cli-collective/atlassian-cli/issues/455)) - **Breaking:** Short alias for `--file` renamed from `-f` to `-F` on `attachments add`, `automation create`, and `automation update`. `-f` continues to mean `--field` on field-setting commands (`issues create`/`update`, `transitions do`). No back-compat alias. ([#339](https://github.com/open-cli-collective/atlassian-cli/issues/339)) - Default page size for paginated commands converged to 50: `issues list` and `issues search` were 25, `users search` was 10. `users search` and `dashboards list` also gain the `-m` short alias for `--max`. ([#340](https://github.com/open-cli-collective/atlassian-cli/issues/340)) - `links types`, `issues types`, `boards list`, `sprints list`, and `users get` now serve from the local instance cache by default — removes per-command API calls in the most common paths. Run `jtk refresh` to update. ([#328](https://github.com/open-cli-collective/atlassian-cli/pull/328), [#329](https://github.com/open-cli-collective/atlassian-cli/pull/329), [#330](https://github.com/open-cli-collective/atlassian-cli/pull/330)) diff --git a/tools/jtk/README.md b/tools/jtk/README.md index 1595cff7..8d67da63 100644 --- a/tools/jtk/README.md +++ b/tools/jtk/README.md @@ -293,6 +293,8 @@ jtk issues list --project MYPROJECT --fields summary,status,customfield_10005 | `--fields` | | | Comma-separated display columns (headers, Jira field IDs, or human names) | | `--next-page-token` | | | Token for next page of results | +Sprint names must resolve uniquely from the cache; ambiguous names report candidate IDs, and unresolved names require a cache refresh or numeric sprint ID. + --- ### `jtk issues get [issue-key...]` @@ -757,6 +759,8 @@ jtk comments list PROJ-123 --fields ID,AUTHOR | `--fulltext` | | `false` | Show full comment bodies without truncation (global) | | `--fields` | | | Comma-separated display fields | +`--fulltext` preserves the table columns and row shape; it only disables body truncation. + **Arguments:** - `` - The issue key (**required**) @@ -1215,12 +1219,13 @@ jtk automation export 123 > rule-backup.json | Flag | Default | Description | |------|---------|-------------| -| `--compact` | `false` | Output minified JSON | +| `--compact` | `false` | Output whitespace-normalized minified JSON | **Arguments:** - `` - The rule ID (**required**) > Note: Output is always JSON — this is the only resource command that emits JSON directly (the control-plane `set-credential --json` envelope is the other exception). +> Invalid JSON returned by the API fails with empty stdout. --- diff --git a/tools/jtk/internal/cmd/OUTPUT_SPEC.md b/tools/jtk/internal/cmd/OUTPUT_SPEC.md index c4230273..32cc6dfe 100644 --- a/tools/jtk/internal/cmd/OUTPUT_SPEC.md +++ b/tools/jtk/internal/cmd/OUTPUT_SPEC.md @@ -98,6 +98,8 @@ All entity-reference flags (`--assignee`, `--project`, `--board`, `--sprint`, li - No match + looks like a raw ID → pass through unchanged - No match + looks like a name → fail with suggestion to `jtk refresh ` +Sprint names must resolve to exactly one cached sprint ID. Ambiguous names list candidate IDs; unresolved names fail with refresh and numeric-ID guidance. `issues list --sprint` never falls back to broader name-based JQL. + ``` $ jtk issues assign MON-4820 "John Smith" Ambiguous user "John Smith" — 3 matches: @@ -295,18 +297,11 @@ ID | AUTHOR | CREATED | BODY 21242 | Aaron Wong | 2026-04-16 | Short audit conclusion after the current code changes: The major source-level accessibility findings on CapOne-specific surfaces appear to be addressed or materially improv... ``` -**`comments list MON-4810 --fulltext`:** one block per comment: +**`comments list MON-4810 --fulltext`:** the same table with body truncation disabled: ``` -ID: 21242 -Author: Aaron Wong -Created: 2026-04-16 -Body: -Short audit conclusion after the current code changes: -The major source-level accessibility findings on CapOne-specific surfaces -appear to be addressed or materially improved: -- loading / redirect states now expose accessible status messaging -- the unsupported-package modal now exposes both title and description correctly -... +ID | AUTHOR | CREATED | BODY +21242 | Aaron Wong | 2026-04-16 | Short audit conclusion after the current code changes: +The major source-level accessibility findings on CapOne-specific surfaces appear to be addressed or materially improved: ... ``` ### `links` @@ -406,7 +401,11 @@ Description: Creates Tasks when a new Onboarding Epic is created **`automation get --show-components`:** dumps the full component tree as indented text (trigger → conditions → actions). +<<<<<<< HEAD +**`automation export `:** validates and emits the rule definition as pretty-printed JSON to stdout. This is the round-trip format consumed by `automation create --from-file`. `--compact` emits whitespace-normalized minified JSON. Invalid API JSON is an error and emits nothing. This command bypasses the global flag system. +======= **`automation export `:** emits the rule definition as pretty-printed JSON to stdout. This is the round-trip format consumed by `automation create --file`. `--compact` minifies. This command bypasses the global flag system. +>>>>>>> 455-jtk-pagination ### `dashboards` diff --git a/tools/jtk/internal/cmd/automation/export.go b/tools/jtk/internal/cmd/automation/export.go index 6f40f4be..16171534 100644 --- a/tools/jtk/internal/cmd/automation/export.go +++ b/tools/jtk/internal/cmd/automation/export.go @@ -19,8 +19,8 @@ func newExportCmd(opts *root.Options) *cobra.Command { Short: "Export automation rule as JSON", Long: `Export the full automation rule definition as JSON. -This outputs the exact JSON returned by the API, suitable for editing -and re-importing via 'jtk auto update'. Output is always JSON. +This validates and formats the JSON returned by the API, suitable for +editing and re-importing via 'jtk auto update'. Output is always JSON. RECOMMENDED WORKFLOW: jtk auto export > rule.json @@ -51,17 +51,14 @@ func runExport(ctx context.Context, opts *root.Options, ruleID string, compact b return err } + var buf bytes.Buffer if compact { - _, err = fmt.Fprintln(opts.Stdout, string(raw)) - return err + err = json.Compact(&buf, raw) + } else { + err = json.Indent(&buf, raw, "", " ") } - - // Pretty-print the JSON - var buf bytes.Buffer - if err := json.Indent(&buf, raw, "", " "); err != nil { - // If indenting fails, output raw - _, err = fmt.Fprintln(opts.Stdout, string(raw)) - return err + if err != nil { + return fmt.Errorf("formatting automation rule JSON: %w", err) } _, err = fmt.Fprintln(opts.Stdout, buf.String()) diff --git a/tools/jtk/internal/cmd/automation/export_test.go b/tools/jtk/internal/cmd/automation/export_test.go new file mode 100644 index 00000000..9ab68f84 --- /dev/null +++ b/tools/jtk/internal/cmd/automation/export_test.go @@ -0,0 +1,78 @@ +package automation + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/open-cli-collective/atlassian-go/testutil" + + "github.com/open-cli-collective/jira-ticket-cli/api" + "github.com/open-cli-collective/jira-ticket-cli/internal/cmd/root" +) + +func TestRunExportFormatsJSONDeterministically(t *testing.T) { + t.Parallel() + tests := []struct { + name string + raw string + compact bool + want string + }{ + {"pretty input defaults to pretty", "{\n \"name\": \"rule\",\n \"enabled\": true\n}", false, "{\n \"name\": \"rule\",\n \"enabled\": true\n}\n"}, + {"compact input defaults to pretty", `{"name":"rule","enabled":true}`, false, "{\n \"name\": \"rule\",\n \"enabled\": true\n}\n"}, + {"compact normalizes whitespace", " { \"name\" : \"rule\", \"enabled\" : true } ", true, `{"name":"rule","enabled":true}` + "\n"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := automationExportServer(tt.raw) + defer server.Close() + + opts, stdout := exportTestOptions(t, server) + testutil.RequireNoError(t, runExport(context.Background(), opts, "rule-1", tt.compact)) + if stdout.String() != tt.want { + t.Fatalf("output mismatch:\n got %q\nwant %q", stdout.String(), tt.want) + } + }) + } +} + +func TestRunExportMalformedJSONLeavesStdoutEmpty(t *testing.T) { + t.Parallel() + for _, compact := range []bool{false, true} { + server := automationExportServer(`{"broken":`) + opts, stdout := exportTestOptions(t, server) + err := runExport(context.Background(), opts, "rule-1", compact) + server.Close() + + if err == nil { + t.Fatalf("compact=%v: expected malformed JSON error", compact) + } + if stdout.Len() != 0 { + t.Fatalf("compact=%v: stdout must stay empty, got %q", compact, stdout.String()) + } + } +} + +func automationExportServer(raw string) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/_edge/tenant_info" { + _, _ = w.Write([]byte(`{"cloudId":"test-cloud"}`)) + return + } + _, _ = w.Write([]byte(raw)) + })) +} + +func exportTestOptions(t *testing.T, server *httptest.Server) (*root.Options, *bytes.Buffer) { + t.Helper() + client, err := api.New(api.ClientConfig{URL: server.URL, Email: "t@x.com", APIToken: "tok"}) + testutil.RequireNoError(t, err) + stdout := &bytes.Buffer{} + opts := &root.Options{Stdout: stdout, Stderr: &bytes.Buffer{}} + opts.SetAPIClient(client) + return opts, stdout +} diff --git a/tools/jtk/internal/cmd/comments/comments.go b/tools/jtk/internal/cmd/comments/comments.go index 950eb293..875bcd31 100644 --- a/tools/jtk/internal/cmd/comments/comments.go +++ b/tools/jtk/internal/cmd/comments/comments.go @@ -7,8 +7,6 @@ import ( "github.com/spf13/cobra" - "github.com/open-cli-collective/atlassian-go/present" - "github.com/open-cli-collective/jira-ticket-cli/api" "github.com/open-cli-collective/jira-ticket-cli/internal/cmd/root" jtkpresent "github.com/open-cli-collective/jira-ticket-cli/internal/present" @@ -42,7 +40,6 @@ func Register(parent *cobra.Command, opts *root.Options) { func newListCmd(opts *root.Options) *cobra.Command { var maxResults int var nextPageToken string - var noTruncate bool var fieldsFlag string cmd := &cobra.Command{ @@ -61,14 +58,12 @@ func newListCmd(opts *root.Options) *cobra.Command { return jtkpresent.ValidateMax(maxResults) }, RunE: func(cmd *cobra.Command, args []string) error { - return runList(cmd.Context(), opts, args[0], maxResults, nextPageToken, noTruncate || opts.IsFullText(), fieldsFlag) + return runList(cmd.Context(), opts, args[0], maxResults, nextPageToken, opts.IsFullText(), fieldsFlag) }, } cmd.Flags().IntVarP(&maxResults, "max", "m", 50, "Page size") cmd.Flags().StringVar(&nextPageToken, "next-page-token", "", "Decimal startAt for the next page") - cmd.Flags().BoolVar(&noTruncate, "no-truncate", false, "Show full comment bodies without truncation") - _ = cmd.Flags().MarkDeprecated("no-truncate", "use --fulltext instead") cmd.Flags().StringVar(&fieldsFlag, "fields", "", "Comma-separated display fields (labels)") return cmd @@ -92,9 +87,6 @@ func runList(ctx context.Context, opts *root.Options, issueKey string, maxResult var selected []projection.ColumnSpec var projected bool spec := jtkpresent.CommentListSpec - if noTruncate { - spec = jtkpresent.CommentDetailSpec - } if !idOnly { selected, projected, err = projection.Resolve( ctx, @@ -133,33 +125,19 @@ func runList(ctx context.Context, opts *root.Options, issueKey string, maxResult return jtkpresent.Emit(opts, model) } - optional := projection.HasOptionalFields(selected, spec) - var model *present.OutputModel - if noTruncate { - model = jtkpresent.CommentPresenter{}.PresentListFullWithPagination(result.Comments, optional, hasMore, nextToken) - if projected { - projectAllDetailSectionsInModel(model, selected) - } - } else { - model = jtkpresent.CommentPresenter{}.PresentListWithPagination(result.Comments, optional, hasMore, nextToken) - if projected { - projection.ApplyToTableInModel(model, selected) - } + model := jtkpresent.CommentPresenter{}.PresentListWithPagination( + result.Comments, + projection.HasOptionalFields(selected, spec), + noTruncate, + hasMore, + nextToken, + ) + if projected { + projection.ApplyToTableInModel(model, selected) } return jtkpresent.Emit(opts, model) } -// projectAllDetailSectionsInModel rewrites every DetailSection of model -// to the selected fields, leaving non-Detail sections (e.g. the -// pagination MessageSection) untouched. -func projectAllDetailSectionsInModel(model *present.OutputModel, selected []projection.ColumnSpec) { - for i, s := range model.Sections { - if ds, ok := s.(*present.DetailSection); ok { - model.Sections[i] = projection.ProjectDetail(ds, selected) - } - } -} - // commentsHasMore computes pagination using the authoritative API metadata, // falling back to a full-page heuristic when Total is unavailable (Jira Cloud // occasionally returns Total=0). diff --git a/tools/jtk/internal/cmd/comments/comments_fields_test.go b/tools/jtk/internal/cmd/comments/comments_fields_test.go index e4dd8278..f50fd0a3 100644 --- a/tools/jtk/internal/cmd/comments/comments_fields_test.go +++ b/tools/jtk/internal/cmd/comments/comments_fields_test.go @@ -65,54 +65,50 @@ func TestRunList_Fields_TableMode_BodyTruncatedWithoutFullText(t *testing.T) { testutil.NotContains(t, output, longBodyText) } -// AC2 (block mode): --fields ID,Author with --fulltext drops Body from each -// per-comment DetailSection. -func TestRunList_Fields_BlockMode_DropsBodyField(t *testing.T) { +// --fields remains a table projection under --fulltext. +func TestRunList_Fields_FullTextDropsBodyColumn(t *testing.T) { t.Parallel() server := newTestCommentsServer(t, []api.Comment{longBodyComment("1", "Alice")}) defer server.Close() opts, stdout, _ := newCommentsOpts(t, server) - err := runList(context.Background(), opts, "TEST-1", 50, "", true, "ID,Author") + err := runList(context.Background(), opts, "TEST-1", 50, "", true, "ID,AUTHOR") testutil.RequireNoError(t, err) output := stdout.String() - testutil.Contains(t, output, "ID:") - testutil.Contains(t, output, "Author:") - if strings.Contains(output, "Body:") { - t.Errorf("Body label should be absent: %q", output) + testutil.Contains(t, output, "ID | AUTHOR") + if strings.Contains(output, "BODY") { + t.Errorf("BODY column should be absent: %q", output) } if strings.Contains(output, "BBB") { t.Errorf("body text leaked into projected block output: %q", output) } } -// AC1+AC3 (block mode): --fields Body with --fulltext renders the full body -// without the truncation marker. -func TestRunList_Fields_BlockMode_BodySelectedWithFullText(t *testing.T) { +func TestRunList_Fields_BodySelectedWithFullText(t *testing.T) { t.Parallel() server := newTestCommentsServer(t, []api.Comment{longBodyComment("1", "Alice")}) defer server.Close() opts, stdout, _ := newCommentsOpts(t, server) - err := runList(context.Background(), opts, "TEST-1", 50, "", true, "Body") + err := runList(context.Background(), opts, "TEST-1", 50, "", true, "BODY") testutil.RequireNoError(t, err) output := stdout.String() - testutil.Contains(t, output, "Body:") + testutil.Contains(t, output, "BODY") testutil.Contains(t, output, longBodyText) testutil.NotContains(t, output, "[truncated") } // AC3 (block mode): --fulltext is a no-op for unselected fields. Even with // fulltext on, body text MUST NOT appear when Body is not in --fields. -func TestRunList_Fields_BlockMode_FullTextNoOp_WhenBodyNotSelected(t *testing.T) { +func TestRunList_Fields_FullTextNoOp_WhenBodyNotSelected(t *testing.T) { t.Parallel() server := newTestCommentsServer(t, []api.Comment{longBodyComment("1", "Alice")}) defer server.Close() opts, stdout, _ := newCommentsOpts(t, server) - err := runList(context.Background(), opts, "TEST-1", 50, "", true, "ID,Author") + err := runList(context.Background(), opts, "TEST-1", 50, "", true, "ID,AUTHOR") testutil.RequireNoError(t, err) output := stdout.String() @@ -121,23 +117,19 @@ func TestRunList_Fields_BlockMode_FullTextNoOp_WhenBodyNotSelected(t *testing.T) } } -// AC2 + helper coverage: in block mode with hasMore=true, projecting Body -// out of every comment must not strip the trailing pagination MessageSection. -// Guards against projectAllDetailSectionsInModel accidentally rewriting -// non-Detail sections. -func TestRunList_Fields_BlockMode_PreservesPaginationHint(t *testing.T) { +func TestRunList_Fields_FullTextPreservesPaginationHint(t *testing.T) { t.Parallel() // Total=2 with one comment returned forces commentsHasMore to true. server := commentsServerWithTotal([]api.Comment{longBodyComment("1", "Alice")}, 2) defer server.Close() opts, stdout, stderr := newCommentsOpts(t, server) - err := runList(context.Background(), opts, "TEST-1", 50, "", true, "ID,Author") + err := runList(context.Background(), opts, "TEST-1", 50, "", true, "ID,AUTHOR") testutil.RequireNoError(t, err) output := stdout.String() - if strings.Contains(output, "Body:") { - t.Errorf("Body label should be projected away: %q", output) + if strings.Contains(output, "BODY") { + t.Errorf("BODY column should be projected away: %q", output) } testutil.NotContains(t, output, "More results available") testutil.Contains(t, stderr.String(), "More results available") @@ -229,7 +221,8 @@ func TestRunList_IDOnly_OverridesFields(t *testing.T) { opts, stdout, _ := newCommentsOpts(t, server) opts.IDOnly = true - err := runList(context.Background(), opts, "TEST-1", 50, "", false, "Body") + opts.FullText = true + err := runList(context.Background(), opts, "TEST-1", 50, "", true, "BODY") testutil.RequireNoError(t, err) if stdout.String() != "1\n" { diff --git a/tools/jtk/internal/cmd/comments/comments_test.go b/tools/jtk/internal/cmd/comments/comments_test.go index 3c8dc746..2914d0aa 100644 --- a/tools/jtk/internal/cmd/comments/comments_test.go +++ b/tools/jtk/internal/cmd/comments/comments_test.go @@ -23,10 +23,9 @@ func TestNewListCmd(t *testing.T) { testutil.Equal(t, cmd.Use, "list ") - // Check that no-truncate flag exists - noTruncateFlag := cmd.Flags().Lookup("no-truncate") - testutil.NotNil(t, noTruncateFlag) - testutil.Equal(t, noTruncateFlag.DefValue, "false") + if cmd.Flags().Lookup("no-truncate") != nil { + t.Fatal("deprecated --no-truncate flag must be removed") + } // Check that max flag exists maxFlag := cmd.Flags().Lookup("max") @@ -161,15 +160,11 @@ func TestRunList_FullCommentBody(t *testing.T) { output := stdout.String() testutil.Contains(t, output, longText) testutil.NotContains(t, output, "[truncated") - // Full mode uses key-value layout - testutil.Contains(t, output, "ID:") - testutil.Contains(t, output, "Author:") - testutil.Contains(t, output, "Body:") + testutil.Contains(t, output, "ID | AUTHOR | CREATED | BODY") } // TestNewListCmd_FullTextRoutesFromRoot verifies that --fulltext on the root -// Options flows through the RunE wrapper to disable truncation, even when the -// local --no-truncate flag is not set. +// Options flows through the RunE wrapper to disable truncation. func TestNewListCmd_FullTextRoutesFromRoot(t *testing.T) { t.Parallel() longText := strings.Repeat("B", 200) @@ -212,60 +207,7 @@ func TestNewListCmd_FullTextRoutesFromRoot(t *testing.T) { opts.SetAPIClient(client) cmd := newListCmd(opts) - cmd.SetArgs([]string{"TEST-1"}) // no --no-truncate locally - testutil.RequireNoError(t, cmd.Execute()) - - output := stdout.String() - testutil.Contains(t, output, longText) - testutil.NotContains(t, output, "[truncated") -} - -// TestNewListCmd_NoTruncateAndFullTextBothSet guards the OR-combined path: -// both the local --no-truncate flag and the global --fulltext must produce -// the same result when set together (prevents accidental && regression). -func TestNewListCmd_NoTruncateAndFullTextBothSet(t *testing.T) { - t.Parallel() - longText := strings.Repeat("B", 200) - comments := []api.Comment{ - { - ID: "1", - Author: api.User{DisplayName: "Alice"}, - Body: &api.ADFDocument{ - Type: "doc", - Version: 1, - Content: []*api.ADFNode{ - { - Type: "paragraph", - Content: []*api.ADFNode{ - {Type: "text", Text: longText}, - }, - }, - }, - }, - Created: "2024-01-15T10:00:00.000Z", - }, - } - - server := newTestCommentsServer(t, comments) - defer server.Close() - - client, err := api.New(api.ClientConfig{ - URL: server.URL, - Email: "test@example.com", - APIToken: "token", - }) - testutil.RequireNoError(t, err) - - var stdout bytes.Buffer - opts := &root.Options{ - FullText: true, - Stdout: &stdout, - Stderr: &bytes.Buffer{}, - } - opts.SetAPIClient(client) - - cmd := newListCmd(opts) - cmd.SetArgs([]string{"TEST-1", "--no-truncate"}) + cmd.SetArgs([]string{"TEST-1"}) testutil.RequireNoError(t, cmd.Execute()) output := stdout.String() @@ -383,7 +325,7 @@ func newCommentsOpts(t *testing.T, server *httptest.Server) (*root.Options, *byt return opts, &stdout, &stderr } -func TestRunList_FullTextBlockSpacing(t *testing.T) { +func TestRunList_FullTextRemainsTable(t *testing.T) { t.Parallel() comments := []api.Comment{ plainComment("11", "Alice", "First comment body"), @@ -397,11 +339,10 @@ func TestRunList_FullTextBlockSpacing(t *testing.T) { testutil.RequireNoError(t, err) out := stdout.String() - // Each comment block ends with "Body: \n" and the second block starts with "ID: 22". - // A blank line between blocks means "Body: First comment body\n\nID: 22" appears. - if !strings.Contains(out, "First comment body\n\nID: 22") { - t.Errorf("expected blank line between comment blocks; got:\n%s", out) - } + testutil.Contains(t, out, "ID | AUTHOR | CREATED | BODY") + testutil.NotContains(t, out, "ID:") + testutil.Contains(t, out, "First comment body") + testutil.Contains(t, out, "Second comment body") } func TestRunList_FullTextPaginationOnStderr(t *testing.T) { @@ -633,7 +574,7 @@ func TestRunList_MultipleCommentsFullMode(t *testing.T) { output := stdout.String() testutil.Contains(t, output, "First comment") testutil.Contains(t, output, "Second comment") - // Comments are now rendered as DetailSections with blank line separators (renderer-owned) + testutil.Contains(t, output, "ID | AUTHOR | CREATED | BODY") } func TestRunAdd_IDOnly(t *testing.T) { diff --git a/tools/jtk/internal/cmd/issues/get.go b/tools/jtk/internal/cmd/issues/get.go index 09a5d8ef..3ed2383e 100644 --- a/tools/jtk/internal/cmd/issues/get.go +++ b/tools/jtk/internal/cmd/issues/get.go @@ -16,7 +16,6 @@ import ( ) func newGetCmd(opts *root.Options) *cobra.Command { - var noTruncate bool var fieldsFlag string var customFields bool @@ -42,12 +41,10 @@ func newGetCmd(opts *root.Options) *cobra.Command { } return runGetMulti(cmd.Context(), opts, args) } - return runGet(cmd.Context(), opts, args[0], noTruncate || opts.IsFullText(), fieldsFlag, customFields) + return runGet(cmd.Context(), opts, args[0], opts.IsFullText(), fieldsFlag, customFields) }, } - cmd.Flags().BoolVar(&noTruncate, "no-truncate", false, "Show full description without truncation") - _ = cmd.Flags().MarkDeprecated("no-truncate", "use --fulltext instead") cmd.Flags().StringVar(&fieldsFlag, "fields", "", "Comma-separated display fields (labels, Jira field IDs, or human names)") cmd.Flags().BoolVar(&customFields, "custom-fields", false, "Append custom fields section to output") diff --git a/tools/jtk/internal/cmd/issues/get_test.go b/tools/jtk/internal/cmd/issues/get_test.go index c351b182..75e4caa1 100644 --- a/tools/jtk/internal/cmd/issues/get_test.go +++ b/tools/jtk/internal/cmd/issues/get_test.go @@ -24,10 +24,9 @@ func TestNewGetCmd(t *testing.T) { testutil.Equal(t, cmd.Use, "get [issue-key...]") testutil.Equal(t, cmd.Short, "Get issue details") - // Check that no-truncate flag exists - noTruncateFlag := cmd.Flags().Lookup("no-truncate") - testutil.NotNil(t, noTruncateFlag) - testutil.Equal(t, noTruncateFlag.DefValue, "false") + if cmd.Flags().Lookup("no-truncate") != nil { + t.Fatal("deprecated --no-truncate flag must be removed") + } } func newTestIssueServer(_ *testing.T, issue api.Issue) *httptest.Server { @@ -154,7 +153,7 @@ func TestRunGet_FullText_KeepsCompactSections(t *testing.T) { // TestNewGetCmd_FullTextRoutesFromRoot verifies that when --fulltext is set on // the root Options (as the persistent --fulltext flag does), runGet is invoked -// with noTruncate=true even though the local --no-truncate flag is not set. +// with truncation disabled. func TestNewGetCmd_FullTextRoutesFromRoot(t *testing.T) { t.Parallel() longText := strings.Repeat("A", 300) @@ -187,50 +186,7 @@ func TestNewGetCmd_FullTextRoutesFromRoot(t *testing.T) { opts.SetAPIClient(client) cmd := newGetCmd(opts) - cmd.SetArgs([]string{"TEST-1"}) // no --no-truncate locally - testutil.RequireNoError(t, cmd.Execute()) - - output := stdout.String() - testutil.Contains(t, output, longText) - testutil.NotContains(t, output, "[truncated") -} - -// TestNewGetCmd_NoTruncateAndFullTextBothSet guards the OR-combined path: -// both the local --no-truncate flag and the global --fulltext must produce -// the same result when set together (prevents accidental && regression). -func TestNewGetCmd_NoTruncateAndFullTextBothSet(t *testing.T) { - t.Parallel() - longText := strings.Repeat("A", 300) - issue := api.Issue{ - Key: "TEST-1", - Fields: api.IssueFields{ - Summary: "Test issue", - Description: &api.Description{Text: longText}, - Status: &api.Status{Name: "Open"}, - IssueType: &api.IssueType{Name: "Task"}, - }, - } - - server := newTestIssueServer(t, issue) - defer server.Close() - - client, err := api.New(api.ClientConfig{ - URL: server.URL, - Email: "test@example.com", - APIToken: "token", - }) - testutil.RequireNoError(t, err) - - var stdout bytes.Buffer - opts := &root.Options{ - FullText: true, - Stdout: &stdout, - Stderr: &bytes.Buffer{}, - } - opts.SetAPIClient(client) - - cmd := newGetCmd(opts) - cmd.SetArgs([]string{"TEST-1", "--no-truncate"}) + cmd.SetArgs([]string{"TEST-1"}) testutil.RequireNoError(t, cmd.Execute()) output := stdout.String() diff --git a/tools/jtk/internal/cmd/issues/list.go b/tools/jtk/internal/cmd/issues/list.go index b3f6d566..99c992c0 100644 --- a/tools/jtk/internal/cmd/issues/list.go +++ b/tools/jtk/internal/cmd/issues/list.go @@ -2,15 +2,12 @@ package issues import ( "context" - "errors" "fmt" "strconv" "strings" "github.com/spf13/cobra" - "github.com/open-cli-collective/atlassian-go/present" - "github.com/open-cli-collective/jira-ticket-cli/api" "github.com/open-cli-collective/jira-ticket-cli/internal/cmd/root" jtkpresent "github.com/open-cli-collective/jira-ticket-cli/internal/present" @@ -51,7 +48,7 @@ func newListCmd(opts *root.Options) *cobra.Command { } cmd.Flags().StringVarP(&project, "project", "p", "", "Filter by project key or name") - cmd.Flags().StringVarP(&sprint, "sprint", "s", "", "Filter by sprint name, numeric ID, or 'current'") + cmd.Flags().StringVarP(&sprint, "sprint", "s", "", "Filter by unique sprint name, numeric ID, or 'current'") cmd.Flags().IntVarP(&maxResults, "max", "m", 50, "Page size (maximum 100)") cmd.Flags().StringVar(&nextPageToken, "next-page-token", "", "Token for next page of results") cmd.Flags().StringVar(&fieldsFlag, "fields", "", "Comma-separated display columns (headers, Jira field IDs, or human names)") @@ -102,13 +99,10 @@ func runList(ctx context.Context, opts *root.Options, project, sprint string, ma } if sprint != "" { - sprintClause, warning, err := buildSprintClause(ctx, resolver, sprint) + sprintClause, err := buildSprintClause(ctx, resolver, sprint) if err != nil { return err } - if warning != nil { - _ = jtkpresent.Emit(opts, emitSprintWarning(warning)) - } if jql != "" { jql += " AND " + sprintClause } else { @@ -160,80 +154,30 @@ func runList(ctx context.Context, opts *root.Options, project, sprint string, ma return jtkpresent.Emit(opts, model) } -type sprintWarningKind int - -const ( - sprintWarningAmbiguity sprintWarningKind = iota - sprintWarningCacheMiss - sprintWarningResolverError - sprintWarningSynthetic -) - -type sprintWarning struct { - Kind sprintWarningKind - SprintName string - Err error -} - -// emitSprintWarning maps a sprintWarning to the appropriate presenter method. -func emitSprintWarning(w *sprintWarning) *present.OutputModel { - p := jtkpresent.SprintPresenter{} - switch w.Kind { - case sprintWarningAmbiguity: - return p.PresentResolutionAmbiguity(w.SprintName) - case sprintWarningCacheMiss: - return p.PresentResolutionCacheMiss(w.SprintName) - case sprintWarningResolverError: - return p.PresentResolutionError(w.SprintName, w.Err) - case sprintWarningSynthetic: - return p.PresentResolutionSynthetic(w.SprintName) - default: - panic(fmt.Sprintf("unhandled sprintWarningKind %d", w.Kind)) - } -} - // buildSprintClause builds the JQL `sprint` clause. Rules: // // - "current" → sprint in openSprints() // - numeric input → sprint = (passed straight through, no cache hit // needed to validate; Jira rejects bad IDs) -// - name input → try the resolver for a canonical ID; on ambiguity or -// not-found, fall through to a quoted name clause so Jira's own JQL -// engine can resolve it (the pre-resolver behavior). The resolver's -// global unique-match requirement is too strict for JQL — names that -// repeat across boards are legal JQL targets and Jira handles them -// natively in the project/board context. -// -// Returns structured warning metadata when a fallback fires; the caller -// maps it to the appropriate presenter method. -func buildSprintClause(ctx context.Context, resolver *resolve.Resolver, sprint string) (string, *sprintWarning, error) { +// - name input → resolve one canonical ID or fail closed. +func buildSprintClause(ctx context.Context, resolver *resolve.Resolver, sprint string) (string, error) { if sprint == "current" { - return "sprint in openSprints()", nil, nil + return "sprint in openSprints()", nil } if n, err := strconv.Atoi(sprint); err == nil { if n <= 0 { - return "", nil, fmt.Errorf("--sprint numeric ID must be positive (got %s)", sprint) + return "", fmt.Errorf("--sprint numeric ID must be positive (got %s)", sprint) } - return fmt.Sprintf("sprint = %d", n), nil, nil + return fmt.Sprintf("sprint = %d", n), nil } resolved, err := resolver.Sprint(ctx, sprint, 0) - if err == nil && resolved.ID != 0 { - return fmt.Sprintf("sprint = %d", resolved.ID), nil, nil + if err != nil { + return "", fmt.Errorf("resolve sprint %q: %w; refresh the sprint cache or pass a numeric sprint ID", sprint, err) } - var w *sprintWarning - var amb *resolve.AmbiguousMatchError - var nf *resolve.NotFoundError - switch { - case errors.As(err, &amb): - w = &sprintWarning{Kind: sprintWarningAmbiguity, SprintName: sprint} - case errors.As(err, &nf): - w = &sprintWarning{Kind: sprintWarningCacheMiss, SprintName: sprint} - case err != nil: - w = &sprintWarning{Kind: sprintWarningResolverError, SprintName: sprint, Err: err} - case resolved.ID == 0: - w = &sprintWarning{Kind: sprintWarningSynthetic, SprintName: sprint} + if resolved.ID == 0 { + return "", fmt.Errorf("resolve sprint %q: resolver returned no ID; refresh the sprint cache or pass a numeric sprint ID", sprint) } - return fmt.Sprintf(`sprint = "%s"`, jqlEscape(sprint)), w, nil + return fmt.Sprintf("sprint = %d", resolved.ID), nil } // jqlEscape makes a string safe to embed between JQL double quotes. JQL diff --git a/tools/jtk/internal/cmd/issues/list_test.go b/tools/jtk/internal/cmd/issues/list_test.go index ba38a506..87037e56 100644 --- a/tools/jtk/internal/cmd/issues/list_test.go +++ b/tools/jtk/internal/cmd/issues/list_test.go @@ -78,6 +78,32 @@ func TestRunList_SprintNumericPassThrough(t *testing.T) { } } +func TestRunList_AmbiguousSprintDoesNotSearch(t *testing.T) { + seedCacheForIssues(t) + testutil.RequireNoError(t, seedSprints(map[int][]api.Sprint{ + 11: {{ID: 100, Name: "Duplicated Sprint"}}, + 22: {{ID: 200, Name: "Duplicated Sprint"}}, + })) + + searchCalls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/search/jql") { + searchCalls++ + } + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + opts, stdout, _ := newListOpts(t, server) + err := runList(context.Background(), opts, "PROJ", "Duplicated Sprint", 25, "", "") + if err == nil || !strings.Contains(err.Error(), "100") || !strings.Contains(err.Error(), "200") { + t.Fatalf("expected candidate IDs, got %v", err) + } + if searchCalls != 0 || stdout.Len() != 0 { + t.Fatalf("ambiguous sprint must fail before search/output: calls=%d stdout=%q", searchCalls, stdout.String()) + } +} + func TestRunList_SprintCurrentUsesOpenSprints(t *testing.T) { seedCacheForIssues(t) @@ -561,57 +587,44 @@ func TestJqlEscape(t *testing.T) { } } -func TestBuildSprintClause_WarnBranches(t *testing.T) { +func TestBuildSprintClause_FailsClosed(t *testing.T) { seedCacheForIssues(t) - // Seed two boards with a sprint of the same name on both, so name resolution - // is ambiguous. testutil.RequireNoError(t, seedSprints(map[int][]api.Sprint{ 11: {{ID: 100, Name: "Duplicated Sprint", State: "active"}}, 22: {{ID: 200, Name: "Duplicated Sprint", State: "closed"}}, })) - // Hermetic httptest server — prevents any accidental resolver refresh from - // reaching a real host (CI outbound-blocked envs would otherwise time out). server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) + w.WriteHeader(http.StatusInternalServerError) })) defer server.Close() client, err := api.New(api.ClientConfig{URL: server.URL, Email: "e", APIToken: "t"}) testutil.RequireNoError(t, err) - t.Run("ambiguous_returns_warning", func(t *testing.T) { - clause, warning, err := buildSprintClause(context.Background(), resolve.New(client), "Duplicated Sprint") - testutil.RequireNoError(t, err) - if !strings.Contains(clause, `sprint = "Duplicated Sprint"`) { - t.Fatalf("want quoted JQL fallback, got %q", clause) - } - if warning == nil || warning.Kind != sprintWarningAmbiguity { - t.Errorf("want ambiguity warning, got: %v", warning) + t.Run("same name on multiple boards lists candidate IDs", func(t *testing.T) { + clause, err := buildSprintClause(context.Background(), resolve.New(client), "Duplicated Sprint") + if err == nil || !strings.Contains(err.Error(), "100") || !strings.Contains(err.Error(), "200") { + t.Fatalf("want ambiguity with candidate IDs, got clause=%q err=%v", clause, err) } }) - t.Run("unresolvable_name_always_returns_some_warning", func(t *testing.T) { - clause, warning, err := buildSprintClause(context.Background(), resolve.New(client), "Nonexistent Sprint Name") - testutil.RequireNoError(t, err) - if !strings.Contains(clause, `sprint = "Nonexistent Sprint Name"`) { - t.Fatalf("want quoted JQL fallback, got %q", clause) - } - if warning == nil { - t.Error("want some warning, got nil") + t.Run("resolver failure gives refresh and ID guidance", func(t *testing.T) { + clause, err := buildSprintClause(context.Background(), resolve.New(client), "Nonexistent Sprint Name") + if err == nil || !strings.Contains(err.Error(), "refresh") || !strings.Contains(err.Error(), "numeric sprint ID") { + t.Fatalf("want fail-closed guidance, got clause=%q err=%v", clause, err) } }) t.Run("negative_numeric_rejected", func(t *testing.T) { - _, _, err := buildSprintClause(context.Background(), resolve.New(client), "-5") + _, err := buildSprintClause(context.Background(), resolve.New(client), "-5") if err == nil || !strings.Contains(err.Error(), "must be positive") { t.Errorf("want positive-only error, got %v", err) } }) t.Run("zero_numeric_rejected", func(t *testing.T) { - _, _, err := buildSprintClause(context.Background(), resolve.New(client), "0") + _, err := buildSprintClause(context.Background(), resolve.New(client), "0") if err == nil || !strings.Contains(err.Error(), "must be positive") { t.Errorf("want positive-only error, got %v", err) } diff --git a/tools/jtk/internal/present/comment.go b/tools/jtk/internal/present/comment.go index 588a94ee..b403aeec 100644 --- a/tools/jtk/internal/present/comment.go +++ b/tools/jtk/internal/present/comment.go @@ -28,21 +28,9 @@ var CommentListSpec = projection.Registry{ {Header: "BODY"}, } -// CommentDetailSpec declares the Fields emitted by PresentListFull / -// PresentListFullWithPagination. Order MUST match the per-comment field -// order in PresentListFull. -var CommentDetailSpec = projection.Registry{ - {Header: "ID", Identity: true}, - {Header: "Author"}, - {Header: "Created"}, - {Header: "Updated", Optional: true}, - {Header: "Visibility", Optional: true}, - {Header: "Body"}, -} - // PresentList creates a table view for a list of comments. Optional -// adds UPDATED column with full timestamp. -func (CommentPresenter) PresentList(comments []api.Comment, includeOptional bool) *present.OutputModel { +// adds UPDATED and VISIBILITY columns. Fulltext disables body truncation. +func (CommentPresenter) PresentList(comments []api.Comment, includeOptional, fulltext bool) *present.OutputModel { var headers []string if includeOptional { headers = []string{"ID", "AUTHOR", "CREATED", "UPDATED", "VISIBILITY", "BODY"} @@ -59,7 +47,7 @@ func (CommentPresenter) PresentList(comments []api.Comment, includeOptional bool body := "" if c.Body != nil { body = c.Body.ToPlainText() - if len(body) > 100 { + if !fulltext && len(body) > 100 { body = body[:100] + "..." } } @@ -80,48 +68,10 @@ func (CommentPresenter) PresentList(comments []api.Comment, includeOptional bool } } -// PresentListFull creates detail views for comments without truncation. -// Each comment becomes a DetailSection. Optional adds Updated field. -func (CommentPresenter) PresentListFull(comments []api.Comment, includeOptional bool) *present.OutputModel { - sections := make([]present.Section, len(comments)) - for i, c := range comments { - author := "Unknown" - if c.Author.DisplayName != "" { - author = c.Author.DisplayName - } - body := "" - if c.Body != nil { - body = strings.TrimRight(c.Body.ToPlainText(), "\n") - } - fields := []present.Field{ - {Label: "ID", Value: c.ID}, - {Label: "Author", Value: author}, - {Label: "Created", Value: FormatTime(c.Created)}, - } - if includeOptional { - fields = append(fields, - present.Field{Label: "Updated", Value: OrDash(c.Updated)}, - present.Field{Label: "Visibility", Value: formatVisibility(c.Visibility)}, - ) - } - fields = append(fields, present.Field{Label: "Body", Value: body}) - sections[i] = &present.DetailSection{Fields: fields} - } - return &present.OutputModel{Sections: sections} -} - // PresentListWithPagination wraps PresentList and appends a stderr-bound // pagination hint when hasMore is true. -func (p CommentPresenter) PresentListWithPagination(comments []api.Comment, includeOptional bool, hasMore bool, nextToken string) *present.OutputModel { - model := p.PresentList(comments, includeOptional) - model.Sections = AppendPaginationHintWithToken(model.Sections, hasMore, nextToken) - return model -} - -// PresentListFullWithPagination wraps PresentListFull and appends a -// stderr-bound pagination hint when hasMore is true. -func (p CommentPresenter) PresentListFullWithPagination(comments []api.Comment, includeOptional bool, hasMore bool, nextToken string) *present.OutputModel { - model := p.PresentListFull(comments, includeOptional) +func (p CommentPresenter) PresentListWithPagination(comments []api.Comment, includeOptional, fulltext, hasMore bool, nextToken string) *present.OutputModel { + model := p.PresentList(comments, includeOptional, fulltext) model.Sections = AppendPaginationHintWithToken(model.Sections, hasMore, nextToken) return model } diff --git a/tools/jtk/internal/present/comment_test.go b/tools/jtk/internal/present/comment_test.go index fb92b026..67637921 100644 --- a/tools/jtk/internal/present/comment_test.go +++ b/tools/jtk/internal/present/comment_test.go @@ -1,14 +1,39 @@ package present import ( + "reflect" + "strings" "testing" "github.com/open-cli-collective/atlassian-go/present" - "github.com/open-cli-collective/atlassian-go/testutil" "github.com/open-cli-collective/jira-ticket-cli/api" ) +func TestCommentPresenter_FullTextShapeStable(t *testing.T) { + t.Parallel() + long := strings.Repeat("body ", 30) + comments := []api.Comment{{ + ID: "42", Author: api.User{DisplayName: "Alice"}, Created: "2024-01-15T10:00:00.000Z", + Body: &api.ADFDocument{Type: "doc", Version: 1, Content: []*api.ADFNode{ + {Type: "paragraph", Content: []*api.ADFNode{{Type: "text", Text: long}}}, + {Type: "paragraph", Content: []*api.ADFNode{{Type: "text", Text: "second line"}}}, + }}, + }} + + compact := CommentPresenter{}.PresentList(comments, false, false).Sections[0].(*present.TableSection) + full := CommentPresenter{}.PresentList(comments, false, true).Sections[0].(*present.TableSection) + if !reflect.DeepEqual(compact.Headers, full.Headers) || !reflect.DeepEqual(compact.Rows[0].Cells[:3], full.Rows[0].Cells[:3]) { + t.Fatalf("--fulltext changed table shape: compact=%#v full=%#v", compact, full) + } + if !strings.HasSuffix(compact.Rows[0].Cells[3], "...") { + t.Fatalf("default body was not truncated: %q", compact.Rows[0].Cells[3]) + } + if !strings.Contains(full.Rows[0].Cells[3], long) || !strings.Contains(full.Rows[0].Cells[3], "second line") { + t.Fatalf("fulltext did not preserve multiline body: %q", full.Rows[0].Cells[3]) + } +} + func singleComment() []api.Comment { return []api.Comment{ { @@ -38,9 +63,9 @@ func TestCommentListSpec_MatchesPresentListHeaders(t *testing.T) { name string model *present.OutputModel }{ - {"PresentList", CommentPresenter{}.PresentList(comments, false)}, - {"PresentListWithPagination_NoMore", CommentPresenter{}.PresentListWithPagination(comments, false, false, "token")}, - {"PresentListWithPagination_HasMore", CommentPresenter{}.PresentListWithPagination(comments, false, true, "token")}, + {"PresentList", CommentPresenter{}.PresentList(comments, false, false)}, + {"PresentListWithPagination_NoMore", CommentPresenter{}.PresentListWithPagination(comments, false, false, false, "token")}, + {"PresentListWithPagination_HasMore", CommentPresenter{}.PresentListWithPagination(comments, false, false, true, "token")}, } for _, tc := range cases { @@ -102,7 +127,7 @@ func TestCommentPresenter_PresentList_ExtendedVisibility(t *testing.T) { }, } - model := CommentPresenter{}.PresentList(comments, true) + model := CommentPresenter{}.PresentList(comments, true, false) table := model.Sections[0].(*present.TableSection) expectedHeaders := []string{"ID", "AUTHOR", "CREATED", "UPDATED", "VISIBILITY", "BODY"} @@ -132,60 +157,12 @@ func TestCommentPresenter_PresentList_ExtendedVisibility(t *testing.T) { } } -func TestCommentPresenter_PresentListFull_ExtendedVisibility(t *testing.T) { - t.Parallel() - comments := []api.Comment{ - { - ID: "100", - Author: api.User{DisplayName: "Alice"}, - Body: &api.ADFDocument{ - Type: "doc", Version: 1, - Content: []*api.ADFNode{{Type: "paragraph", Content: []*api.ADFNode{{Type: "text", Text: "public"}}}}, - }, - Created: "2024-01-15T10:00:00.000Z", - }, - { - ID: "101", - Author: api.User{DisplayName: "Bob"}, - Body: &api.ADFDocument{ - Type: "doc", Version: 1, - Content: []*api.ADFNode{{Type: "paragraph", Content: []*api.ADFNode{{Type: "text", Text: "restricted"}}}}, - }, - Created: "2024-01-15T11:00:00.000Z", - Visibility: &api.CommentVisibility{Type: "role", Value: "Administrators"}, - }, - } - - model := CommentPresenter{}.PresentListFull(comments, true) - - ds0 := model.Sections[0].(*present.DetailSection) - ds1 := model.Sections[1].(*present.DetailSection) - - var vis0, vis1 string - for _, f := range ds0.Fields { - if f.Label == "Visibility" { - vis0 = f.Value - } - } - for _, f := range ds1.Fields { - if f.Label == "Visibility" { - vis1 = f.Value - } - } - if vis0 != "-" { - t.Errorf("comment 0 Visibility: expected '-', got %q", vis0) - } - if vis1 != "Administrators" { - t.Errorf("comment 1 Visibility: expected 'Administrators', got %q", vis1) - } -} - func TestCommentListSpec_ExtendedMatchesPresentListHeaders(t *testing.T) { t.Parallel() comments := singleComment() extendedSpec := CommentListSpec.ForMode(true) - model := CommentPresenter{}.PresentList(comments, true) + model := CommentPresenter{}.PresentList(comments, true, false) var table *present.TableSection for _, s := range model.Sections { @@ -206,138 +183,3 @@ func TestCommentListSpec_ExtendedMatchesPresentListHeaders(t *testing.T) { } } } - -func TestCommentDetailSpec_ExtendedMatchesPresentDetailLabels(t *testing.T) { - t.Parallel() - comments := singleComment() - - extendedSpec := CommentDetailSpec.ForMode(true) - model := CommentPresenter{}.PresentListFull(comments, true) - - var detail *present.DetailSection - for _, s := range model.Sections { - if ds, ok := s.(*present.DetailSection); ok { - detail = ds - break - } - } - if detail == nil { - t.Fatal("no DetailSection in includeOptional PresentListFull output") - } - - renderedLabels := make(map[string]bool, len(detail.Fields)) - for _, f := range detail.Fields { - renderedLabels[f.Label] = true - } - for _, spec := range extendedSpec { - if !renderedLabels[spec.Header] { - t.Errorf("spec Header %q not emitted in includeOptional mode", spec.Header) - } - } - - specLabels := make(map[string]bool, len(extendedSpec)) - for _, spec := range extendedSpec { - specLabels[spec.Header] = true - } - mismatch := false - for _, f := range detail.Fields { - if !specLabels[f.Label] { - t.Errorf("rendered field %q has no matching CommentDetailSpec entry in includeOptional mode", f.Label) - mismatch = true - } - } - if mismatch { - t.Fatal("reverse-direction check failed; skipping order check") - } - - specOrder := make([]string, 0, len(extendedSpec)) - for _, spec := range extendedSpec { - specOrder = append(specOrder, spec.Header) - } - renderedOrder := make([]string, 0, len(detail.Fields)) - for _, f := range detail.Fields { - renderedOrder = append(renderedOrder, f.Label) - } - if len(specOrder) != len(renderedOrder) { - t.Fatalf("includeOptional spec has %d entries, rendered has %d", len(specOrder), len(renderedOrder)) - } - for i := range specOrder { - if specOrder[i] != renderedOrder[i] { - t.Errorf("includeOptional order mismatch at index %d: spec=%q rendered=%q", i, specOrder[i], renderedOrder[i]) - } - } -} - -// TestCommentDetailSpec_MatchesPresentDetailLabels locks CommentDetailSpec -// against the Field labels emitted by PresentListFull, both directions: -// - Every spec entry must appear as a rendered Field label. -// - Every rendered Field label must have a matching spec entry — otherwise -// --fields projection would silently drop that field. -// -// Order is checked too: ProjectDetail relies on the spec order being the same -// as the presenter's Field order for deterministic projection output. -func TestCommentDetailSpec_MatchesPresentDetailLabels(t *testing.T) { - t.Parallel() - comments := singleComment() - - cases := []struct { - name string - model *present.OutputModel - }{ - {"PresentListFull", CommentPresenter{}.PresentListFull(comments, false)}, - {"PresentListFullWithPagination_NoMore", CommentPresenter{}.PresentListFullWithPagination(comments, false, false, "token")}, - {"PresentListFullWithPagination_HasMore", CommentPresenter{}.PresentListFullWithPagination(comments, false, true, "token")}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - var detail *present.DetailSection - for _, s := range tc.model.Sections { - if ds, ok := s.(*present.DetailSection); ok { - detail = ds - break - } - } - if detail == nil { - t.Fatalf("no DetailSection in %s output", tc.name) - } - - activeSpec := CommentDetailSpec.ForMode(false) - - renderedLabels := make(map[string]bool, len(detail.Fields)) - for _, f := range detail.Fields { - renderedLabels[f.Label] = true - } - for _, spec := range activeSpec { - if !renderedLabels[spec.Header] { - t.Errorf("spec Header %q not emitted by %s", spec.Header, tc.name) - } - } - - specLabels := make(map[string]bool, len(activeSpec)) - for _, spec := range activeSpec { - specLabels[spec.Header] = true - } - for _, f := range detail.Fields { - if !specLabels[f.Label] { - t.Errorf("rendered field %q has no matching CommentDetailSpec entry", f.Label) - } - } - - specOrder := make([]string, 0, len(activeSpec)) - for _, spec := range activeSpec { - specOrder = append(specOrder, spec.Header) - } - renderedOrder := make([]string, 0, len(detail.Fields)) - for _, f := range detail.Fields { - renderedOrder = append(renderedOrder, f.Label) - } - testutil.Equal(t, len(specOrder), len(renderedOrder)) - for i := range specOrder { - if specOrder[i] != renderedOrder[i] { - t.Errorf("order mismatch at index %d: spec=%q rendered=%q", i, specOrder[i], renderedOrder[i]) - } - } - }) - } -}