-
Notifications
You must be signed in to change notification settings - Fork 50
Filtering: NonApplicability improvements #1373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ab898b3
7413638
33b4091
bea96d7
e6e7296
3f5d04a
25b514e
5c146bc
b24b42f
90de9f9
d510a18
22231aa
76582e9
1ea1e59
5d7ed6e
0b5806a
cfbfa10
78beba4
d7f4cb5
dc5fd3c
cdae7e7
6723f29
e32af32
65b52e4
200f5fc
5769a1f
7543750
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| import { AdHocFiltersVariable, AdHocFilterWithLabels } from './adhoc/AdHocFiltersVariable'; | ||
| import { DrilldownDependenciesManager } from './DrilldownDependenciesManager'; | ||
| import { GroupByVariable } from './groupby/GroupByVariable'; | ||
| import { VariableDependencyConfig } from './VariableDependencyConfig'; | ||
|
|
||
| function createManager(opts: { adhocVar?: AdHocFiltersVariable; groupByVar?: GroupByVariable }) { | ||
| const mockDependencyConfig = { setVariableNames: jest.fn() } as unknown as VariableDependencyConfig<any>; | ||
| const manager = new DrilldownDependenciesManager(mockDependencyConfig); | ||
|
|
||
| if (opts.adhocVar) { | ||
| manager['_adhocFiltersVar'] = opts.adhocVar; | ||
| } | ||
| if (opts.groupByVar) { | ||
| manager['_groupByVar'] = opts.groupByVar; | ||
| } | ||
|
|
||
| return manager; | ||
| } | ||
|
|
||
| function createAdhocVar( | ||
| filters: AdHocFilterWithLabels[], | ||
| originFilters?: AdHocFilterWithLabels[], | ||
| applicabilityEnabled?: boolean | ||
| ) { | ||
| return new AdHocFiltersVariable({ | ||
| datasource: { uid: 'my-ds-uid' }, | ||
| name: 'filters', | ||
| filters, | ||
| originFilters, | ||
| applicabilityEnabled, | ||
| }); | ||
| } | ||
|
|
||
| function createGroupByVar(value: string[], keysApplicability?: any[], applicabilityEnabled?: boolean) { | ||
| return new GroupByVariable({ | ||
| datasource: { uid: 'my-ds-uid' }, | ||
| name: 'groupby', | ||
| key: 'testGroupBy', | ||
| value, | ||
| text: value, | ||
| keysApplicability, | ||
| applicabilityEnabled, | ||
| }); | ||
| } | ||
|
|
||
| describe('DrilldownDependenciesManager', () => { | ||
| describe('getFilters', () => { | ||
| it('should return undefined when no adhocFiltersVar', () => { | ||
| const manager = createManager({}); | ||
| expect(manager.getFilters()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should return all complete filters when no applicability results', () => { | ||
| const filters: AdHocFilterWithLabels[] = [ | ||
| { key: 'env', value: 'prod', operator: '=' }, | ||
| { key: 'cluster', value: 'us', operator: '=' }, | ||
| ]; | ||
|
|
||
| const manager = createManager({ adhocVar: createAdhocVar(filters) }); | ||
|
|
||
| const result = manager.getFilters() ?? []; | ||
| expect(result).toHaveLength(2); | ||
| expect(result[0].key).toBe('env'); | ||
| expect(result[1].key).toBe('cluster'); | ||
| }); | ||
|
|
||
| it('should exclude incomplete filters', () => { | ||
| const filters: AdHocFilterWithLabels[] = [ | ||
| { key: 'env', value: 'prod', operator: '=' }, | ||
| { key: '', value: '', operator: '' }, | ||
| ]; | ||
|
|
||
| const manager = createManager({ adhocVar: createAdhocVar(filters) }); | ||
|
|
||
| const result = manager.getFilters() ?? []; | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].key).toBe('env'); | ||
| }); | ||
|
|
||
| it('should exclude variable-level nonApplicable filters', () => { | ||
| const filters: AdHocFilterWithLabels[] = [ | ||
| { key: 'env', value: 'prod', operator: '=' }, | ||
| { key: 'pod', value: 'abc', operator: '=', nonApplicable: true }, | ||
| ]; | ||
|
|
||
| const manager = createManager({ adhocVar: createAdhocVar(filters) }); | ||
|
|
||
| const result = manager.getFilters() ?? []; | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].key).toBe('env'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getGroupByKeys', () => { | ||
| it('should return undefined when no groupByVar', () => { | ||
| const manager = createManager({}); | ||
| expect(manager.getGroupByKeys()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should return all applicable keys', () => { | ||
| const manager = createManager({ groupByVar: createGroupByVar(['ns', 'pod']) }); | ||
| expect(manager.getGroupByKeys()).toEqual(['ns', 'pod']); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,15 +81,21 @@ export class DrilldownDependenciesManager<TState extends SceneObjectState> { | |
| } | ||
|
|
||
| public getFilters(): AdHocFilterWithLabels[] | undefined { | ||
| return this._adhocFiltersVar | ||
| ? [...(this._adhocFiltersVar.state.originFilters ?? []), ...this._adhocFiltersVar.state.filters].filter( | ||
| (f) => isFilterComplete(f) && isFilterApplicable(f) | ||
| ) | ||
| : undefined; | ||
| if (!this._adhocFiltersVar) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const stateFilters = this._adhocFiltersVar.state.filters; | ||
| const originFilters = this._adhocFiltersVar.state.originFilters ?? []; | ||
| const allFilters = [...originFilters, ...stateFilters]; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sanity check - does it matter that in the adhoc filters we create
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call, yeah this should be consistent and originfilters should be first by convention
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've modified the adhoc to be consistent, the origin filters were put last mainly because it was easier and order didnt matter -- it still doesnt for origin ones vs non origin ones -- it matters mostly for same type of filters with same key (e.g. I have 2 user filters env=prod and then env=dev, the latter one should take priority and overwrite the first, this doesnt happen for origin and user ones because of the origin property delimitation) |
||
| return allFilters.filter((f) => isFilterComplete(f) && isFilterApplicable(f)); | ||
| } | ||
|
|
||
| public getGroupByKeys(): string[] | undefined { | ||
| return this._groupByVar ? this._groupByVar.getApplicableKeys() : undefined; | ||
| if (!this._groupByVar) { | ||
| return undefined; | ||
| } | ||
| return this._groupByVar.getApplicableKeys(); | ||
| } | ||
|
|
||
| public cleanup(): void { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this entire manager will get very thin after the unification, basically it would just deal with adhoc subscription and fetching filters... might not even need it at all we'll see