diff --git a/README.md b/README.md index 6d46a28..a4b4aa4 100644 --- a/README.md +++ b/README.md @@ -929,6 +929,17 @@ abs metrics list --owners "jane@example.com" --teams Growth # by name/email abs metrics list --review-status pending abs metrics list --sort name --asc abs metrics list --ids 1,2,3 + +# Client-side filters (applied across all pages) +abs metrics list --metric-type goal_ratio,custom_sql +abs metrics list --goal purchase # name or ID, incl. denominator +abs metrics list --outlier-limiting # has limiting; --no-outlier-limiting for none +abs metrics list --outlier-method quantile,stdev +abs metrics list --has-property-filter # --no-property-filter for none +abs metrics list --property-filter-path page_name +abs metrics list --property-filter-contains BookingFullDetails +abs metrics list --impact-direction positive,negative +abs metrics list --cuped # --no-cuped for none abs metrics get 123 abs metrics create --name "Revenue" --type count abs metrics update 123 --description "Updated" @@ -953,6 +964,22 @@ abs metrics access grant-user 123 --user 1 --role 2 abs metrics access revoke-user 123 --user 1 --role 2 ``` +#### Metric list filters (client-side) + +These filters run client-side: when any is set, the CLI fetches every metric and filters locally. They combine with AND across flags and OR within a comma-separated list. Because the full set is fetched and filtered, `--items`/`--page` do not apply while filtering — every match is shown. (Server-side equivalents are a planned follow-up.) + +| Filter | Description | +|---|---| +| `--metric-type ` | `goal_count`, `goal_unique_count`, `goal_time_to_achievement`, `goal_property`, `goal_property_unique_count`, `goal_ratio`, `goal_retention`, `goal_activity_period_count`, `custom_sql` | +| `--goal ` | Goal name (substring) or ID; matches numerator and denominator goals | +| `--outlier-limiting` / `--no-outlier-limiting` | Has / does not have outlier limiting (method other than `unlimited`) | +| `--outlier-method ` | `unlimited`, `quantile`, `stdev`, `fixed` | +| `--has-property-filter` / `--no-property-filter` | Has / does not have a goal property filter | +| `--property-filter-path ` | Property path(s) referenced inside the property filter (substring) | +| `--property-filter-contains ` | Substring anywhere in the serialized property filter | +| `--impact-direction ` | `positive`, `negative`, `unknown` | +| `--cuped` / `--no-cuped` | Has / does not have CUPED (variance reduction) enabled | + ### Teams Aliases: `teams`, `team` diff --git a/src/commands/metrics/index.ts b/src/commands/metrics/index.ts index e599f71..5a9e106 100644 --- a/src/commands/metrics/index.ts +++ b/src/commands/metrics/index.ts @@ -14,7 +14,13 @@ import { summarizeMetricRow, } from '../../api-client/entity-summary.js'; import { createListCommand } from '../../lib/utils/list-command.js'; -import { listMetrics as coreListMetrics } from '../../core/metrics/list.js'; +import { listMetrics as coreListMetrics, listAllMetrics } from '../../core/metrics/list.js'; +import { + parseMetricFilters, + validateMetricFilters, + hasActiveMetricFilters, + filterMetrics, +} from '../../core/metrics/filter.js'; import { getMetric } from '../../core/metrics/get.js'; import { createMetric } from '../../core/metrics/create.js'; import { updateMetric } from '../../core/metrics/update.js'; @@ -33,7 +39,7 @@ const listCommand = createListCommand({ description: 'List all metrics', defaultItems: 100, fetch: async (client, options) => { - const result = await coreListMetrics(client, { + const baseParams = { items: options.items as number, page: options.page as number, archived: options.archived as boolean, @@ -45,16 +51,62 @@ const listCommand = createListCommand({ owners: options.owners as string | undefined, teams: options.teams as string | undefined, reviewStatus: options.reviewStatus as string | undefined, - }); + }; + + const filters = parseMetricFilters(options); + if (hasActiveMetricFilters(filters)) { + validateMetricFilters(options, filters); + // Client-side filters need the full set, not a single page. + const all = await listAllMetrics(client, baseParams); + return filterMetrics(all.data as Array>, filters); + } + + const result = await coreListMetrics(client, baseParams); return result.data; }, summarizeRow: summarizeMetricRow, + isClientFiltered: (options) => hasActiveMetricFilters(parseMetricFilters(options)), extraOptions: (cmd) => cmd .option('--include-drafts', 'include draft (non-activated) metrics') .option('--owners ', 'filter by owners (comma-separated IDs, names, or emails)') .option('--teams ', 'filter by teams (comma-separated IDs or names)') - .option('--review-status ', 'filter by review status (pending, approved, none)'), + .option('--review-status ', 'filter by review status (pending, approved, none)') + // Client-side filters (applied across all pages). API-side equivalents are a follow-up. + .option( + '--metric-type ', + 'filter by metric type(s), comma-separated (goal_count, goal_unique_count, goal_time_to_achievement, goal_property, goal_property_unique_count, goal_ratio, goal_retention, goal_activity_period_count, custom_sql)' + ) + .option( + '--goal ', + 'filter by goal name or ID, comma-separated (matches numerator and denominator goals)' + ) + // positive flag declared before negative so the boolean stays tri-state + .option( + '--outlier-limiting', + 'only metrics that have outlier limiting (method other than unlimited)' + ) + .option('--no-outlier-limiting', 'only metrics without outlier limiting') + .option( + '--outlier-method ', + 'filter by outlier limit method(s), comma-separated (unlimited, quantile, stdev, fixed)' + ) + .option('--has-property-filter', 'only metrics that have a goal property filter') + .option('--no-property-filter', 'only metrics without a goal property filter') + .option( + '--property-filter-path ', + 'filter by property path(s) referenced in the property filter, comma-separated (substring match)' + ) + .option( + '--property-filter-contains ', + 'filter by a substring anywhere in the serialized property filter' + ) + .option( + '--impact-direction ', + 'filter by impact direction(s), comma-separated (positive, negative, unknown)' + ) + .option('--cuped', 'only metrics with CUPED (variance reduction) enabled') + .option('--no-cuped', 'only metrics without CUPED'), }); const getCommand = new Command('get') diff --git a/src/commands/metrics/metrics.test.ts b/src/commands/metrics/metrics.test.ts index 90d6474..f221fc0 100644 --- a/src/commands/metrics/metrics.test.ts +++ b/src/commands/metrics/metrics.test.ts @@ -201,6 +201,131 @@ describe('metrics command', () => { expect(printFormatted).toHaveBeenCalled(); }); + const richMetrics = [ + { + id: 1, + name: 'a', + type: 'goal_count', + effect: 'positive', + goal_id: 1, + goal: { id: 1, name: 'page_view' }, + outlier_limit_method: 'unlimited', + vr_lookback_interval: null, + property_filter: null, + }, + { + id: 2, + name: 'b', + type: 'goal_ratio', + effect: 'negative', + goal_id: 2, + goal: { id: 2, name: 'checkout' }, + outlier_limit_method: 'quantile', + vr_lookback_interval: '2w', + property_filter: '{"filter":{"and":[{"var":{"path":"page_name"}}]}}', + }, + { + id: 3, + name: 'c', + type: 'custom_sql', + effect: 'unknown', + goal_id: 3, + goal: { id: 3, name: 'purchase' }, + outlier_limit_method: 'unlimited', + vr_lookback_interval: null, + property_filter: null, + }, + ]; + + it('filters metrics list by --metric-type (client-side)', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + await metricsCommand.parseAsync([ + 'node', + 'test', + 'list', + '--metric-type', + 'goal_ratio,custom_sql', + ]); + + const printed = vi.mocked(printFormatted).mock.calls.at(-1)?.[0] as Array<{ id: number }>; + expect(printed.map((m) => m.id)).toEqual([2, 3]); + }); + + it('filters metrics list by --cuped (client-side)', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + await metricsCommand.parseAsync(['node', 'test', 'list', '--cuped']); + + const printed = vi.mocked(printFormatted).mock.calls.at(-1)?.[0] as Array<{ id: number }>; + expect(printed.map((m) => m.id)).toEqual([2]); + }); + + it('filters metrics list by --no-cuped (client-side)', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + await metricsCommand.parseAsync(['node', 'test', 'list', '--no-cuped']); + + const printed = vi.mocked(printFormatted).mock.calls.at(-1)?.[0] as Array<{ id: number }>; + expect(printed.map((m) => m.id)).toEqual([1, 3]); + }); + + it('filters metrics list by --has-property-filter (client-side)', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + await metricsCommand.parseAsync(['node', 'test', 'list', '--has-property-filter']); + + const printed = vi.mocked(printFormatted).mock.calls.at(-1)?.[0] as Array<{ id: number }>; + expect(printed.map((m) => m.id)).toEqual([2]); + }); + + it('filters metrics list by --no-property-filter (client-side)', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + await metricsCommand.parseAsync(['node', 'test', 'list', '--no-property-filter']); + + const printed = vi.mocked(printFormatted).mock.calls.at(-1)?.[0] as Array<{ id: number }>; + expect(printed.map((m) => m.id)).toEqual([1, 3]); + }); + + it('rejects combining --has-property-filter with --no-property-filter', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + try { + await metricsCommand.parseAsync([ + 'node', + 'test', + 'list', + '--has-property-filter', + '--no-property-filter', + ]); + throw new Error('Should have thrown'); + } catch (error) { + if (!(error as Error).message.startsWith('process.exit')) throw error; + const errorOutput = consoleErrorSpy.mock.calls.flat().join(' '); + expect(errorOutput).toContain('property-filter'); + } + }); + + it('filters metrics list by --goal name (client-side)', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + await metricsCommand.parseAsync(['node', 'test', 'list', '--goal', 'checkout']); + + const printed = vi.mocked(printFormatted).mock.calls.at(-1)?.[0] as Array<{ id: number }>; + expect(printed.map((m) => m.id)).toEqual([2]); + }); + + it('rejects an invalid --impact-direction value', async () => { + mockClient.listMetrics.mockResolvedValue(richMetrics); + try { + await metricsCommand.parseAsync(['node', 'test', 'list', '--impact-direction', 'sideways']); + throw new Error('Should have thrown'); + } catch (error) { + if (!(error as Error).message.startsWith('process.exit')) throw error; + const errorOutput = consoleErrorSpy.mock.calls.flat().join(' '); + expect(errorOutput).toContain('impact-direction'); + } + }); + + it('does not client-filter (single page) when no new filters are passed', async () => { + await metricsCommand.parseAsync(['node', 'test', 'list']); + expect(mockClient.listMetrics).toHaveBeenCalledWith(defaultListParams); + }); + it('should get metric by id', async () => { await metricsCommand.parseAsync(['node', 'test', 'get', '1']); diff --git a/src/core/metrics/filter.test.ts b/src/core/metrics/filter.test.ts new file mode 100644 index 0000000..f3ad2f0 --- /dev/null +++ b/src/core/metrics/filter.test.ts @@ -0,0 +1,284 @@ +import { describe, it, expect } from 'vitest'; +import { + parseMetricFilters, + validateMetricFilters, + hasActiveMetricFilters, + filterMetrics, +} from './filter.js'; + +describe('parseMetricFilters', () => { + it('splits comma-separated values and trims them', () => { + const f = parseMetricFilters({ + metricType: 'goal_count, goal_ratio', + goal: '1, purchase', + outlierMethod: 'quantile,stdev', + impactDirection: 'positive', + propertyFilterPath: 'page_name , order', + propertyFilterContains: ' BookingFullDetails ', + }); + expect(f.metricType).toEqual(['goal_count', 'goal_ratio']); + expect(f.goal).toEqual(['1', 'purchase']); + expect(f.outlierMethod).toEqual(['quantile', 'stdev']); + expect(f.impactDirection).toEqual(['positive']); + expect(f.propertyFilterPath).toEqual(['page_name', 'order']); + expect(f.propertyFilterContains).toBe('BookingFullDetails'); + }); + + it('returns undefined for absent / empty values', () => { + const f = parseMetricFilters({}); + expect(f.metricType).toBeUndefined(); + expect(f.goal).toBeUndefined(); + expect(f.outlierLimiting).toBeUndefined(); + expect(f.hasPropertyFilter).toBeUndefined(); + expect(f.cuped).toBeUndefined(); + expect(f.propertyFilterContains).toBeUndefined(); + }); + + it('reads tri-state booleans for outlier limiting and cuped', () => { + expect(parseMetricFilters({ outlierLimiting: true }).outlierLimiting).toBe(true); + expect(parseMetricFilters({ outlierLimiting: false }).outlierLimiting).toBe(false); + expect(parseMetricFilters({ cuped: true }).cuped).toBe(true); + expect(parseMetricFilters({ cuped: false }).cuped).toBe(false); + }); + + it('collapses the property-filter dest pair into a tri-state', () => { + // neither flag: commander leaves propertyFilter at its default true; we ignore it + expect(parseMetricFilters({ propertyFilter: true }).hasPropertyFilter).toBeUndefined(); + // --has-property-filter + expect( + parseMetricFilters({ hasPropertyFilter: true, propertyFilter: true }).hasPropertyFilter + ).toBe(true); + // --no-property-filter + expect(parseMetricFilters({ propertyFilter: false }).hasPropertyFilter).toBe(false); + }); +}); + +describe('hasActiveMetricFilters', () => { + it('is false when nothing is set', () => { + expect(hasActiveMetricFilters(parseMetricFilters({}))).toBe(false); + }); + it('is true when any filter is set, including false booleans', () => { + expect(hasActiveMetricFilters(parseMetricFilters({ metricType: 'goal_count' }))).toBe(true); + expect(hasActiveMetricFilters(parseMetricFilters({ outlierLimiting: false }))).toBe(true); + expect(hasActiveMetricFilters(parseMetricFilters({ propertyFilter: false }))).toBe(true); + expect(hasActiveMetricFilters(parseMetricFilters({ cuped: true }))).toBe(true); + }); +}); + +describe('validateMetricFilters', () => { + it('rejects unknown outlier methods', () => { + const opts = { outlierMethod: 'bogus' }; + expect(() => validateMetricFilters(opts, parseMetricFilters(opts))).toThrow(/outlier.*method/i); + }); + it('rejects unknown impact directions', () => { + const opts = { impactDirection: 'sideways' }; + expect(() => validateMetricFilters(opts, parseMetricFilters(opts))).toThrow( + /impact.*direction/i + ); + }); + it('rejects passing both --has-property-filter and --no-property-filter', () => { + const opts = { hasPropertyFilter: true, propertyFilter: false }; + expect(() => validateMetricFilters(opts, parseMetricFilters(opts))).toThrow(/property-filter/i); + }); + it('accepts valid values', () => { + const opts = { outlierMethod: 'quantile,stdev', impactDirection: 'positive,negative' }; + expect(() => validateMetricFilters(opts, parseMetricFilters(opts))).not.toThrow(); + }); +}); + +type M = Record; +const metric = (over: M = {}): M => ({ + id: 1, + name: 'm', + type: 'goal_count', + effect: 'positive', + goal_id: 1, + goal: { id: 1, name: 'page_view' }, + denominator_goal_id: null, + denominator_goal: null, + outlier_limit_method: 'unlimited', + denominator_outlier_limit_method: null, + vr_lookback_interval: null, + denominator_vr_lookback_interval: null, + property_filter: null, + denominator_property_filter: null, + ...over, +}); + +describe('filterMetrics - type / impact / goal', () => { + it('returns all metrics when no filters are active', () => { + const data = [metric(), metric({ id: 2 })]; + expect(filterMetrics(data, parseMetricFilters({}))).toHaveLength(2); + }); + + it('filters by metric type (case-insensitive, OR within list)', () => { + const data = [ + metric({ id: 1, type: 'goal_count' }), + metric({ id: 2, type: 'goal_ratio' }), + metric({ id: 3, type: 'custom_sql' }), + ]; + const out = filterMetrics(data, parseMetricFilters({ metricType: 'GOAL_COUNT,goal_ratio' })); + expect(out.map((m) => m.id)).toEqual([1, 2]); + }); + + it('filters by impact direction', () => { + const data = [ + metric({ id: 1, effect: 'positive' }), + metric({ id: 2, effect: 'negative' }), + metric({ id: 3, effect: 'unknown' }), + ]; + const out = filterMetrics(data, parseMetricFilters({ impactDirection: 'negative,unknown' })); + expect(out.map((m) => m.id)).toEqual([2, 3]); + }); + + it('filters by goal numeric id (numerator or denominator)', () => { + const data = [ + metric({ id: 1, goal_id: 1 }), + metric({ id: 2, goal_id: 9, denominator_goal_id: 1 }), + metric({ id: 3, goal_id: 5 }), + ]; + const out = filterMetrics(data, parseMetricFilters({ goal: '1' })); + expect(out.map((m) => m.id)).toEqual([1, 2]); + }); + + it('filters by goal name substring (numerator or denominator)', () => { + const data = [ + metric({ id: 1, goal: { id: 1, name: 'page_view' } }), + metric({ + id: 2, + goal: { id: 2, name: 'checkout' }, + denominator_goal: { id: 3, name: 'page_view_all' }, + }), + metric({ id: 3, goal: { id: 4, name: 'purchase' } }), + ]; + const out = filterMetrics(data, parseMetricFilters({ goal: 'page_view' })); + expect(out.map((m) => m.id)).toEqual([1, 2]); + }); +}); + +describe('filterMetrics - outlier / cuped', () => { + it('--outlier-limiting keeps only metrics with limiting (numerator or denominator)', () => { + const data = [ + metric({ id: 1, outlier_limit_method: 'unlimited' }), + metric({ id: 2, outlier_limit_method: 'quantile' }), + metric({ + id: 3, + outlier_limit_method: 'unlimited', + denominator_outlier_limit_method: 'stdev', + }), + ]; + const out = filterMetrics(data, parseMetricFilters({ outlierLimiting: true })); + expect(out.map((m) => m.id)).toEqual([2, 3]); + }); + + it('--no-outlier-limiting keeps only metrics without limiting', () => { + const data = [ + metric({ id: 1, outlier_limit_method: 'unlimited' }), + metric({ id: 2, outlier_limit_method: 'fixed' }), + ]; + const out = filterMetrics(data, parseMetricFilters({ outlierLimiting: false })); + expect(out.map((m) => m.id)).toEqual([1]); + }); + + it('filters by outlier method (numerator or denominator, OR within list)', () => { + const data = [ + metric({ id: 1, outlier_limit_method: 'quantile' }), + metric({ + id: 2, + outlier_limit_method: 'unlimited', + denominator_outlier_limit_method: 'fixed', + }), + metric({ id: 3, outlier_limit_method: 'stdev' }), + ]; + const out = filterMetrics(data, parseMetricFilters({ outlierMethod: 'quantile,fixed' })); + expect(out.map((m) => m.id)).toEqual([1, 2]); + }); + + it('--cuped keeps only metrics with a lookback interval', () => { + const data = [ + metric({ id: 1, vr_lookback_interval: null }), + metric({ id: 2, vr_lookback_interval: '2w' }), + metric({ id: 3, vr_lookback_interval: null, denominator_vr_lookback_interval: '1w' }), + ]; + const out = filterMetrics(data, parseMetricFilters({ cuped: true })); + expect(out.map((m) => m.id)).toEqual([2, 3]); + }); + + it('--no-cuped keeps only metrics without a lookback interval', () => { + const data = [ + metric({ id: 1, vr_lookback_interval: '4w' }), + metric({ id: 2, vr_lookback_interval: '' }), + metric({ id: 3, vr_lookback_interval: null }), + ]; + const out = filterMetrics(data, parseMetricFilters({ cuped: false })); + expect(out.map((m) => m.id)).toEqual([2, 3]); + }); +}); + +const PF = JSON.stringify({ + filter: { and: [{ in: [{ value: ['BookingFullDetails'] }, { var: { path: 'page_name' } }] }] }, +}); +const PF2 = JSON.stringify({ filter: { and: [{ '==': [{ var: { path: 'currency' } }, 'USD'] }] } }); + +describe('filterMetrics - property filter', () => { + it('--has-property-filter keeps only metrics with a non-empty filter', () => { + const data = [ + metric({ id: 1, property_filter: null }), + metric({ id: 2, property_filter: '' }), + metric({ id: 3, property_filter: '{}' }), + metric({ id: 4, property_filter: PF }), + metric({ id: 5, property_filter: null, denominator_property_filter: PF2 }), + ]; + const out = filterMetrics( + data, + parseMetricFilters({ hasPropertyFilter: true, propertyFilter: true }) + ); + expect(out.map((m) => m.id)).toEqual([4, 5]); + }); + + it('--no-property-filter keeps only metrics without a filter', () => { + const data = [metric({ id: 1, property_filter: null }), metric({ id: 2, property_filter: PF })]; + const out = filterMetrics(data, parseMetricFilters({ propertyFilter: false })); + expect(out.map((m) => m.id)).toEqual([1]); + }); + + it('--property-filter-path matches var.path substrings (numerator or denominator)', () => { + const data = [ + metric({ id: 1, property_filter: PF }), // page_name + metric({ id: 2, property_filter: null, denominator_property_filter: PF2 }), // currency + metric({ id: 3, property_filter: null }), + ]; + const out = filterMetrics(data, parseMetricFilters({ propertyFilterPath: 'PAGE,currency' })); + expect(out.map((m) => m.id)).toEqual([1, 2]); + }); + + it('--property-filter-contains matches anywhere in the serialized filter (incl. values)', () => { + const data = [ + metric({ id: 1, property_filter: PF }), // contains BookingFullDetails + metric({ id: 2, property_filter: PF2 }), // contains USD + ]; + const out = filterMetrics(data, parseMetricFilters({ propertyFilterContains: 'bookingfull' })); + expect(out.map((m) => m.id)).toEqual([1]); + }); + + it('handles property_filter supplied as an object, not just a string', () => { + const data = [metric({ id: 1, property_filter: { filter: { var: { path: 'amount' } } } })]; + const out = filterMetrics(data, parseMetricFilters({ propertyFilterPath: 'amount' })); + expect(out.map((m) => m.id)).toEqual([1]); + }); + + it('treats an empty filter wrapper (empty and/or array) as no property filter', () => { + const data = [ + metric({ id: 1, property_filter: JSON.stringify({ filter: { and: [] } }) }), + metric({ id: 2, property_filter: JSON.stringify({ filter: { or: [] } }) }), + metric({ id: 3, property_filter: PF }), + metric({ id: 4, property_filter: '[]' }), + metric({ id: 5, property_filter: '[{}]' }), + ]; + const out = filterMetrics( + data, + parseMetricFilters({ hasPropertyFilter: true, propertyFilter: true }) + ); + expect(out.map((m) => m.id)).toEqual([3]); + }); +}); diff --git a/src/core/metrics/filter.ts b/src/core/metrics/filter.ts new file mode 100644 index 0000000..5d0a690 --- /dev/null +++ b/src/core/metrics/filter.ts @@ -0,0 +1,293 @@ +// Client-side filtering for `metrics list`. Every field referenced here is +// already present in the GET /metrics list response, so no extra API calls are +// needed. Server-side equivalents are tracked as a follow-up to FT-1966. + +export interface MetricFilters { + metricType?: string[] | undefined; + goal?: string[] | undefined; + outlierLimiting?: boolean | undefined; // tri-state: undefined = no filter + outlierMethod?: string[] | undefined; + hasPropertyFilter?: boolean | undefined; // tri-state + propertyFilterPath?: string[] | undefined; + propertyFilterContains?: string | undefined; + impactDirection?: string[] | undefined; + cuped?: boolean | undefined; // tri-state +} + +export const OUTLIER_METHODS = ['unlimited', 'quantile', 'stdev', 'fixed'] as const; +export const IMPACT_DIRECTIONS = ['positive', 'negative', 'unknown'] as const; + +function splitCsv(value: unknown): string[] | undefined { + if (typeof value !== 'string') return undefined; + const parts = value + .split(',') + .map((s) => s.trim()) + .filter((s) => s !== ''); + return parts.length > 0 ? parts : undefined; +} + +function trimmedString(value: unknown): string | undefined { + if (typeof value !== 'string') return undefined; + const t = value.trim(); + return t === '' ? undefined : t; +} + +export function parseMetricFilters(options: Record): MetricFilters { + // Property-filter presence is split across two commander dests because the + // requested flag names (--has-property-filter / --no-property-filter) do not + // share a base name. --no-property-filter declared alone defaults + // `propertyFilter` to true, so we only treat an explicit `=== false` as the + // "absent" filter and let --has-property-filter own the "present" filter. + const hasPropertyFilter = + options.hasPropertyFilter === true + ? true + : options.propertyFilter === false + ? false + : undefined; + + return { + metricType: splitCsv(options.metricType), + goal: splitCsv(options.goal), + outlierLimiting: + typeof options.outlierLimiting === 'boolean' ? options.outlierLimiting : undefined, + outlierMethod: splitCsv(options.outlierMethod), + hasPropertyFilter, + propertyFilterPath: splitCsv(options.propertyFilterPath), + propertyFilterContains: trimmedString(options.propertyFilterContains), + impactDirection: splitCsv(options.impactDirection), + cuped: typeof options.cuped === 'boolean' ? options.cuped : undefined, + }; +} + +export function hasActiveMetricFilters(f: MetricFilters): boolean { + return ( + f.metricType !== undefined || + f.goal !== undefined || + f.outlierLimiting !== undefined || + f.outlierMethod !== undefined || + f.hasPropertyFilter !== undefined || + f.propertyFilterPath !== undefined || + f.propertyFilterContains !== undefined || + f.impactDirection !== undefined || + f.cuped !== undefined + ); +} + +export function validateMetricFilters( + options: Record, + filters: MetricFilters +): void { + if (options.hasPropertyFilter === true && options.propertyFilter === false) { + throw new Error('Cannot combine --has-property-filter with --no-property-filter.'); + } + + if (filters.outlierMethod) { + const allowed = OUTLIER_METHODS as readonly string[]; + const bad = filters.outlierMethod.filter((m) => !allowed.includes(m.toLowerCase())); + if (bad.length > 0) { + throw new Error( + `Invalid --outlier-method value(s): ${bad.join(', ')}. Must be one of: ${OUTLIER_METHODS.join(', ')}.` + ); + } + } + + if (filters.impactDirection) { + const allowed = IMPACT_DIRECTIONS as readonly string[]; + const bad = filters.impactDirection.filter((d) => !allowed.includes(d.toLowerCase())); + if (bad.length > 0) { + throw new Error( + `Invalid --impact-direction value(s): ${bad.join(', ')}. Must be one of: ${IMPACT_DIRECTIONS.join(', ')}.` + ); + } + } +} + +type Metric = Record; + +const lc = (v: unknown): string => String(v ?? '').toLowerCase(); + +function byType(m: Metric, f: MetricFilters): boolean { + if (!f.metricType) return true; + const want = f.metricType.map((t) => t.toLowerCase()); + return want.includes(lc(m.type)); +} + +function byImpactDirection(m: Metric, f: MetricFilters): boolean { + if (!f.impactDirection) return true; + const want = f.impactDirection.map((d) => d.toLowerCase()); + return want.includes(lc(m.effect)); +} + +function byGoal(m: Metric, f: MetricFilters): boolean { + if (!f.goal) return true; + const goal = m.goal as Metric | null | undefined; + const denomGoal = m.denominator_goal as Metric | null | undefined; + const ids = [m.goal_id, m.denominator_goal_id] + .filter((v) => v !== null && v !== undefined) + .map((v) => String(v)); + const names = [goal?.name, denomGoal?.name] + .filter((v): v is string => typeof v === 'string') + .map((n) => n.toLowerCase()); + return f.goal.some((token) => { + if (/^\d+$/.test(token)) return ids.includes(token); + const t = token.toLowerCase(); + return names.some((n) => n.includes(t)); + }); +} + +function isLimited(method: unknown): boolean { + const m = lc(method); + return m !== '' && m !== 'unlimited'; +} + +function metricHasOutlierLimiting(m: Metric): boolean { + return isLimited(m.outlier_limit_method) || isLimited(m.denominator_outlier_limit_method); +} + +function byOutlierLimiting(m: Metric, f: MetricFilters): boolean { + if (f.outlierLimiting === undefined) return true; + return metricHasOutlierLimiting(m) === f.outlierLimiting; +} + +function byOutlierMethod(m: Metric, f: MetricFilters): boolean { + if (!f.outlierMethod) return true; + const want = f.outlierMethod.map((x) => x.toLowerCase()); + const num = lc(m.outlier_limit_method); + const den = lc(m.denominator_outlier_limit_method); + return (num !== '' && want.includes(num)) || (den !== '' && want.includes(den)); +} + +function nonEmpty(value: unknown): boolean { + return value !== null && value !== undefined && String(value).trim() !== ''; +} + +function metricHasCuped(m: Metric): boolean { + return nonEmpty(m.vr_lookback_interval) || nonEmpty(m.denominator_vr_lookback_interval); +} + +function byCuped(m: Metric, f: MetricFilters): boolean { + if (f.cuped === undefined) return true; + return metricHasCuped(m) === f.cuped; +} + +function propertyFilterText(raw: unknown): string { + if (raw === null || raw === undefined) return ''; + if (typeof raw === 'string') return raw; + try { + return JSON.stringify(raw); + } catch { + return ''; + } +} + +function hasLeafContent(node: unknown): boolean { + if (node === null || node === undefined) return false; + if (Array.isArray(node)) return node.some(hasLeafContent); + if (typeof node === 'object') return Object.values(node).some(hasLeafContent); + return true; // a primitive leaf (string/number/boolean) = real content +} + +function hasMeaningfulPropertyFilter(raw: unknown): boolean { + const t = propertyFilterText(raw).trim(); + if (t === '' || t === 'null') return false; + let parsed: unknown; + try { + parsed = JSON.parse(t); + } catch { + // Not JSON but a non-empty string — treat the raw value as a present filter. + return true; + } + // Unwrap the {"filter": ...} envelope when present, then check for any leaf. + const inner = + parsed !== null && + typeof parsed === 'object' && + !Array.isArray(parsed) && + 'filter' in (parsed as Record) + ? (parsed as Record).filter + : parsed; + return hasLeafContent(inner); +} + +function metricHasPropertyFilter(m: Metric): boolean { + return ( + hasMeaningfulPropertyFilter(m.property_filter) || + hasMeaningfulPropertyFilter(m.denominator_property_filter) + ); +} + +function collectVarPaths(node: unknown, out: string[]): void { + if (Array.isArray(node)) { + for (const child of node) collectVarPaths(child, out); + return; + } + if (node !== null && typeof node === 'object') { + for (const [key, value] of Object.entries(node)) { + if (key === 'var') { + if (typeof value === 'string') out.push(value); + else if ( + value !== null && + typeof value === 'object' && + typeof (value as Record).path === 'string' + ) { + out.push((value as Record).path as string); + } + } + collectVarPaths(value, out); + } + } +} + +function extractPropertyFilterPaths(raw: unknown): string[] { + const text = propertyFilterText(raw); + if (text === '') return []; + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + return []; + } + const out: string[] = []; + collectVarPaths(parsed, out); + return out; +} + +function byPropertyFilterPresence(m: Metric, f: MetricFilters): boolean { + if (f.hasPropertyFilter === undefined) return true; + return metricHasPropertyFilter(m) === f.hasPropertyFilter; +} + +function byPropertyFilterPath(m: Metric, f: MetricFilters): boolean { + if (!f.propertyFilterPath) return true; + const paths = [ + ...extractPropertyFilterPaths(m.property_filter), + ...extractPropertyFilterPaths(m.denominator_property_filter), + ].map((p) => p.toLowerCase()); + const want = f.propertyFilterPath.map((t) => t.toLowerCase()); + return want.some((t) => paths.some((p) => p.includes(t))); +} + +function byPropertyFilterContains(m: Metric, f: MetricFilters): boolean { + if (!f.propertyFilterContains) return true; + const hay = ( + propertyFilterText(m.property_filter) + + ' ' + + propertyFilterText(m.denominator_property_filter) + ).toLowerCase(); + return hay.includes(f.propertyFilterContains.toLowerCase()); +} + +const PREDICATES: Array<(m: Metric, f: MetricFilters) => boolean> = [ + byType, + byImpactDirection, + byGoal, + byOutlierLimiting, + byOutlierMethod, + byCuped, + byPropertyFilterPresence, + byPropertyFilterPath, + byPropertyFilterContains, +]; + +export function filterMetrics(metrics: Metric[], f: MetricFilters): Metric[] { + return metrics.filter((m) => PREDICATES.every((p) => p(m, f))); +} diff --git a/src/core/metrics/list.ts b/src/core/metrics/list.ts index e199d56..26f6865 100644 --- a/src/core/metrics/list.ts +++ b/src/core/metrics/list.ts @@ -44,3 +44,61 @@ export async function listMetrics( pagination: { page: params.page, items: params.items, hasMore: data.length >= params.items }, }; } + +const FETCH_ALL_PAGE_SIZE = 200; +const FETCH_ALL_MAX_PAGES = 100; + +/** + * Fetch every metric across all pages, resolving owners/teams a single time. + * Used by `metrics list` when client-side filters are active so filtering sees + * the full set rather than a single page. + */ +export async function listAllMetrics( + client: APIClient, + params: ListMetricsParams, + pageSize: number = FETCH_ALL_PAGE_SIZE, + maxPages: number = FETCH_ALL_MAX_PAGES +): Promise> { + const ownerIds = params.owners ? await resolveOwnerIds(client, params.owners) : undefined; + const teamIds = params.teams ? await resolveTeamIds(client, params.teams) : undefined; + + const all: unknown[] = []; + // Stays true if we exhaust `maxPages` without seeing a short (final) page, + // meaning there may be more metrics we did not fetch. + let truncated = true; + for (let page = 1; page <= maxPages; page++) { + const data = await client.listMetrics({ + items: pageSize, + page, + archived: params.archived, + include_drafts: params.include_drafts, + search: params.search, + sort: params.sort, + sort_asc: params.sortAsc, + ids: params.ids, + owners: ownerIds, + teams: teamIds, + review_status: params.reviewStatus, + }); + all.push(...data); + if (data.length < pageSize) { + truncated = false; + break; + } + } + + // Don't silently drop metrics: warn (stderr) and report hasMore honestly when + // the page cap is reached. Client-side filtering would otherwise look + // complete while missing matches beyond the cap. + if (truncated) { + console.error( + `Warning: reached the ${maxPages}-page fetch cap (${all.length} metrics); results may be incomplete. ` + + 'Narrow the set with server-side filters (--search, --owners, --teams, --ids) for complete client-side filtering.' + ); + } + + return { + data: all, + pagination: { page: 1, items: all.length, hasMore: truncated }, + }; +} diff --git a/src/core/metrics/metrics.test.ts b/src/core/metrics/metrics.test.ts index ae4bf05..eb5ffe6 100644 --- a/src/core/metrics/metrics.test.ts +++ b/src/core/metrics/metrics.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { listMetrics, resolveOwnerIds, resolveTeamIds } from './list.js'; +import { listMetrics, listAllMetrics, resolveOwnerIds, resolveTeamIds } from './list.js'; import { getMetric } from './get.js'; import { createMetric } from './create.js'; import { updateMetric } from './update.js'; @@ -811,3 +811,86 @@ describe('validateMetricFields', () => { ).toThrow(/--value-source-property is required/); }); }); + +describe('listAllMetrics', () => { + it('pages through results until a short page is returned', async () => { + const client = { + listMetrics: vi.fn(), + } as any; + // pageSize 2: page1 full (2), page2 full (2), page3 short (1) -> stop + client.listMetrics + .mockResolvedValueOnce([{ id: 1 }, { id: 2 }]) + .mockResolvedValueOnce([{ id: 3 }, { id: 4 }]) + .mockResolvedValueOnce([{ id: 5 }]); + + const result = await listAllMetrics(client, { items: 100, page: 1 }, 2); + + expect(result.data.map((m: any) => m.id)).toEqual([1, 2, 3, 4, 5]); + expect(client.listMetrics).toHaveBeenCalledTimes(3); + expect(client.listMetrics).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ items: 2, page: 1 }) + ); + expect(client.listMetrics).toHaveBeenNthCalledWith( + 3, + expect.objectContaining({ items: 2, page: 3 }) + ); + }); + + it('stops after a single page when fewer than pageSize are returned', async () => { + const client = { listMetrics: vi.fn().mockResolvedValue([{ id: 1 }]) } as any; + const result = await listAllMetrics(client, { items: 100, page: 1 }, 2); + expect(result.data).toHaveLength(1); + expect(client.listMetrics).toHaveBeenCalledTimes(1); + }); + + it('resolves owners and teams once, then forwards their IDs on every page', async () => { + const client = { + listMetrics: vi.fn().mockResolvedValue([]), + resolveUsers: vi.fn().mockResolvedValue([{ id: 10, email: 'jane@example.com' }]), + resolveTeams: vi.fn().mockResolvedValue([{ id: 5, name: 'Growth' }]), + } as any; + + await listAllMetrics( + client, + { items: 100, page: 1, owners: 'jane@example.com', teams: 'Growth' }, + 2 + ); + + expect(client.resolveUsers).toHaveBeenCalledTimes(1); + expect(client.resolveTeams).toHaveBeenCalledTimes(1); + expect(client.listMetrics).toHaveBeenCalledWith( + expect.objectContaining({ owners: '10', teams: '5' }) + ); + }); + + it('warns and reports hasMore when the page cap is reached (truncation)', async () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + // Every page is full (pageSize 2), so the loop never breaks early and hits the cap. + const client = { listMetrics: vi.fn().mockResolvedValue([{ id: 1 }, { id: 2 }]) } as any; + + const result = await listAllMetrics(client, { items: 100, page: 1 }, 2, 3); + + expect(client.listMetrics).toHaveBeenCalledTimes(3); // maxPages + expect(result.data).toHaveLength(6); + expect(result.pagination?.hasMore).toBe(true); + expect(errSpy).toHaveBeenCalledWith(expect.stringContaining('fetch cap')); + errSpy.mockRestore(); + }); + + it('does not warn and reports hasMore false when a short page ends the fetch', async () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const client = { + listMetrics: vi + .fn() + .mockResolvedValueOnce([{ id: 1 }, { id: 2 }]) + .mockResolvedValueOnce([{ id: 3 }]), + } as any; + + const result = await listAllMetrics(client, { items: 100, page: 1 }, 2, 3); + + expect(result.pagination?.hasMore).toBe(false); + expect(errSpy).not.toHaveBeenCalled(); + errSpy.mockRestore(); + }); +}); diff --git a/src/lib/utils/list-command.test.ts b/src/lib/utils/list-command.test.ts index 7526e16..9626b81 100644 --- a/src/lib/utils/list-command.test.ts +++ b/src/lib/utils/list-command.test.ts @@ -3,6 +3,7 @@ import { createListCommand } from './list-command.js'; import { getAPIClientFromOptions, getGlobalOptions, printFormatted } from './api-helper.js'; import { resetCommand } from '../../test/helpers/command-reset.js'; import { setTTYOverride } from './stdin.js'; +import { printPaginationFooter } from './pagination.js'; vi.mock('./api-helper.js', async (importOriginal) => { const actual = await importOriginal(); @@ -70,6 +71,32 @@ describe('createListCommand', () => { expect(printFormatted).toHaveBeenCalled(); }); + it('prints a filtered total footer (not the pagination footer) when isClientFiltered returns true', async () => { + const fetch = vi.fn().mockResolvedValue([{ id: 1 }, { id: 2 }]); + const cmd = createListCommand({ + description: 'List items', + fetch, + isClientFiltered: () => true, + }); + resetCommand(cmd); + + await cmd.parseAsync(['node', 'test']); + + const output = consoleSpy.mock.calls.flat().join('\n'); + expect(output).toContain('(filtered)'); + expect(printPaginationFooter).not.toHaveBeenCalled(); + }); + + it('uses the pagination footer when isClientFiltered is absent', async () => { + const fetch = vi.fn().mockResolvedValue([{ id: 1 }]); + const cmd = createListCommand({ description: 'List items', fetch }); + resetCommand(cmd); + + await cmd.parseAsync(['node', 'test']); + + expect(printPaginationFooter).toHaveBeenCalled(); + }); + describe('output behavior under piping', () => { afterEach(() => { setTTYOverride({ stdin: true, stdout: true }); diff --git a/src/lib/utils/list-command.ts b/src/lib/utils/list-command.ts index 2d20e85..5f9971e 100644 --- a/src/lib/utils/list-command.ts +++ b/src/lib/utils/list-command.ts @@ -6,7 +6,7 @@ import { shouldOutputIdsOnly, withErrorHandling, } from './api-helper.js'; -import { addPaginationOptions, printPaginationFooter } from './pagination.js'; +import { addPaginationOptions, printPaginationFooter, printFilteredFooter } from './pagination.js'; import { applyShowExclude } from '../../api-client/entity-summary.js'; import type { APIClient } from '../../api-client/api-client.js'; @@ -16,6 +16,12 @@ export interface ListCommandOptions { fetch: (client: APIClient, options: Record) => Promise; summarizeRow?: (item: Record) => Record; extraOptions?: (cmd: Command) => Command; + /** + * When this returns true for the parsed options, `fetch` is expected to have + * already returned the COMPLETE client-filtered result set, so a total-count + * footer is printed instead of the page-based pagination footer. + */ + isClientFiltered?: (options: Record) => boolean; } export function createListCommand(opts: ListCommandOptions): Command { @@ -65,12 +71,16 @@ export function createListCommand(opts: ListCommandOptions): Command { } printFormatted(data, globalOptions); - printPaginationFooter( - items.length, - options.items, - options.page, - globalOptions.output as string - ); + if (opts.isClientFiltered?.(options)) { + printFilteredFooter(items.length, globalOptions.output as string); + } else { + printPaginationFooter( + items.length, + options.items, + options.page, + globalOptions.output as string + ); + } }) ); diff --git a/src/lib/utils/pagination.ts b/src/lib/utils/pagination.ts index d67be86..bbbfd40 100644 --- a/src/lib/utils/pagination.ts +++ b/src/lib/utils/pagination.ts @@ -20,3 +20,8 @@ export function printPaginationFooter( : `Page ${page} (${count} results).`; console.log(chalk.gray(footer)); } + +export function printFilteredFooter(count: number, outputFormat?: string): void { + if (outputFormat === 'json' || outputFormat === 'yaml') return; + console.log(chalk.gray(`${count} result${count === 1 ? '' : 's'} (filtered).`)); +}