Skip to content
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

GroupByVariable: Sync label to URL #705

Merged
merged 7 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { SceneObjectUrlSyncHandler, SceneObjectUrlValue, SceneObjectUrlValues } from '../../core/types';
import { AdHocFiltersVariable, AdHocFilterWithLabels, isFilterComplete } from './AdHocFiltersVariable';
import { escapeUrlPipeDelimiters, toUrlCommaDelimitedString, unescapeUrlDelimiters } from '../utils';

export class AdHocFiltersVariableUrlSyncHandler implements SceneObjectUrlSyncHandler {
public constructor(private _variable: AdHocFiltersVariable) {}
Expand All @@ -19,7 +20,7 @@ export class AdHocFiltersVariableUrlSyncHandler implements SceneObjectUrlSyncHan
return { [this.getKey()]: [''] };
}

const value = filters.filter(isFilterComplete).map((filter) => toArray(filter).map(escapePipeDelimiters).join('|'));
const value = filters.filter(isFilterComplete).map((filter) => toArray(filter).map(escapeUrlPipeDelimiters).join('|'));
return { [this.getKey()]: value };
}

Expand All @@ -45,52 +46,14 @@ function deserializeUrlToFilters(value: SceneObjectUrlValue): AdHocFilterWithLab
return filter === null ? [] : [filter];
}

function escapePipeDelimiters(value: string | undefined): string {
if (value === null || value === undefined) {
return '';
}

// Replace the pipe due to using it as a filter separator
return (value = /\|/g[Symbol.replace](value, '__gfp__'));
}

function escapeCommaDelimiters(value: string | undefined): string {
if (value === null || value === undefined) {
return '';
}

// Replace the comma due to using it as a value/label separator
return /,/g[Symbol.replace](value, '__gfc__');
}

function unescapeDelimiters(value: string | undefined): string {
if (value === null || value === undefined) {
return '';
}

value = /__gfp__/g[Symbol.replace](value, '|');
value = /__gfc__/g[Symbol.replace](value, ',');

return value;
}

function toArray(filter: AdHocFilterWithLabels): string[] {
return [
toCommaDelimitedString(filter.key, filter.keyLabel),
toUrlCommaDelimitedString(filter.key, filter.keyLabel),
filter.operator,
toCommaDelimitedString(filter.value, filter.valueLabel),
toUrlCommaDelimitedString(filter.value, filter.valueLabel),
];
}

function toCommaDelimitedString(key: string, label?: string): string {
// Omit for identical key/label or when label is not set at all
if (!label || key === label) {
return escapeCommaDelimiters(key);
}

return [key, label].map(escapeCommaDelimiters).join(',');
}

function toFilter(urlValue: string | number | boolean | undefined | null): AdHocFilterWithLabels | null {
if (typeof urlValue !== 'string' || urlValue.length === 0) {
return null;
Expand All @@ -105,7 +68,7 @@ function toFilter(urlValue: string | number | boolean | undefined | null): AdHoc

return acc;
}, [])
.map(unescapeDelimiters);
.map(unescapeUrlDelimiters);

return {
key,
Expand Down
119 changes: 105 additions & 14 deletions packages/scenes/src/variables/groupby/GroupByVariable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,27 +46,118 @@ describe('GroupByVariable', () => {
});
});

it('url sync works', async () => {
const { variable } = setupTest({
defaultOptions: [
{ text: 'a', value: 'a' },
{ text: 'b', value: 'b' },
],
});
describe('url sync', () => {
it('should work with default options', () => {
const { variable } = setupTest({
defaultOptions: [
{ text: 'a', value: 'a' },
{ text: 'b', value: 'b' },
],
});

expect(variable.state.value).toEqual('');
expect(variable.state.text).toEqual('');

expect(variable.state.value).toEqual('');
act(() => {
variable.changeValueTo(['a']);
});

act(() => {
variable.changeValueTo(['a']);
expect(locationService.getLocation().search).toBe('?var-test=a');

act(() => {
locationService.push('/?var-test=a&var-test=b');
});

expect(variable.state.value).toEqual(['a', 'b']);
expect(variable.state.text).toEqual(['a', 'b']);

act(() => {
locationService.push('/?var-test=a&var-test=b&var-test=c');
});

expect(variable.state.value).toEqual(['a', 'b', 'c']);
expect(variable.state.text).toEqual(['a', 'b', 'c']);
});

expect(locationService.getLocation().search).toBe('?var-test=a');
it('should work with received options', async () => {
const { variable } = setupTest({
getTagKeysProvider: () => {
return Promise.resolve({
replace: true,
values: [
{ text: 'A', value: 'a' },
{ text: 'b', value: 'b' },
{ text: 'C', value: 'c' },
],
});
},
});

await act(async () => {
await lastValueFrom(variable.validateAndUpdate());
});

act(() => {
locationService.push('/?var-test=a&var-test=b');
expect(variable.state.value).toEqual('');
expect(variable.state.text).toEqual('');

act(() => {
variable.changeValueTo(['a']);
});

expect(locationService.getLocation().search).toBe('?var-test=a,A');

act(() => {
locationService.push('/?var-test=a,A&var-test=b');
});

expect(variable.state.value).toEqual(['a', 'b']);
expect(variable.state.text).toEqual(['A', 'b']);

act(() => {
locationService.push('/?var-test=a,A&var-test=b&var-test=c');
});

expect(variable.state.value).toEqual(['a', 'b', 'c']);
expect(variable.state.text).toEqual(['A', 'b', 'c']);
});

expect(variable.state.value).toEqual(['a', 'b']);
it('should work with commas', async () => {
const { variable } = setupTest({
defaultOptions: [
{ text: 'A,something', value: 'a' },
{ text: 'B', value: 'b,something' },
],
});

expect(variable.state.value).toEqual('');
expect(variable.state.text).toEqual('');

await act(async () => {
await lastValueFrom(variable.validateAndUpdate());
});

act(() => {
variable.changeValueTo(['a']);
});

expect(locationService.getLocation().search).toBe('?var-test=a,A__gfc__something');

act(() => {
locationService.push('/?var-test=a,A__gfc__something&var-test=b__gfc__something');
});

expect(variable.state.value).toEqual(['a', 'b,something']);
expect(variable.state.text).toEqual(['A,something', 'b,something']);

act(() => {
locationService.push(
'/?var-test=a,A__gfc__something&var-test=b__gfc__something&var-test=c__gfc__something,C__gfc__something'
);
});

expect(variable.state.value).toEqual(['a', 'b,something', 'c,something']);
expect(variable.state.text).toEqual(['A,something', 'b,something', 'C,something']);
});
});

it('Can override and replace getTagKeys', async () => {
Expand Down
32 changes: 23 additions & 9 deletions packages/scenes/src/variables/groupby/GroupByVariable.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React, { useEffect, useMemo, useState } from 'react';
import { AdHocVariableFilter, DataSourceApi, MetricFindValue } from '@grafana/data';
import { AdHocVariableFilter, DataSourceApi, MetricFindValue, SelectableValue } from '@grafana/data';
import { allActiveGroupByVariables } from './findActiveGroupByVariablesByUid';
import { DataSourceRef, VariableType } from '@grafana/schema';
import { SceneComponentProps, ControlsLayout } from '../../core/types';
import { SceneComponentProps, ControlsLayout, SceneObjectUrlSyncHandler } from '../../core/types';
import { sceneGraph } from '../../core/sceneGraph';
import { ValidateAndUpdateResult, VariableValueOption, VariableValueSingle } from '../types';
import { MultiValueVariable, MultiValueVariableState, VariableGetOptionsArgs } from '../variants/MultiValueVariable';
Expand All @@ -12,6 +12,7 @@ import { InputActionMeta, MultiSelect } from '@grafana/ui';
import { isArray } from 'lodash';
import { getQueriesForVariables } from '../utils';
import { OptionWithCheckbox } from '../components/VariableValueSelect';
import { GroupByVariableUrlSyncHandler } from './GroupByVariableUrlSyncHandler';

export interface GroupByVariableState extends MultiValueVariableState {
/** Defaults to "Group" */
Expand Down Expand Up @@ -57,6 +58,8 @@ export class GroupByVariable extends MultiValueVariable<GroupByVariableState> {
static Component = GroupByVariableRenderer;
isLazy = true;

protected _urlSync: SceneObjectUrlSyncHandler = new GroupByVariableUrlSyncHandler(this);

public validateAndUpdate(): Observable<ValidateAndUpdateResult> {
return this.getValueOptions({}).pipe(
map((options) => {
Expand Down Expand Up @@ -182,19 +185,27 @@ export class GroupByVariable extends MultiValueVariable<GroupByVariableState> {
}
}
export function GroupByVariableRenderer({ model }: SceneComponentProps<MultiValueVariable>) {
const { value, key, maxVisibleValues, noValueOnClear } = model.useState();
const arrayValue = useMemo(() => (isArray(value) ? value : [value]), [value]);
const { value, text, key, maxVisibleValues, noValueOnClear } = model.useState();
const values = useMemo<Array<SelectableValue<VariableValueSingle>>>(() => {
const arrayValue = isArray(value) ? value : [value];
const arrayText = isArray(text) ? text : [text];

return arrayValue.map((value, idx) => ({
value,
label: String(arrayText[idx] ?? value),
}));
}, [value, text]);
const [isFetchingOptions, setIsFetchingOptions] = useState(false);
const [isOptionsOpen, setIsOptionsOpen] = useState(false);
const [inputValue, setInputValue] = useState('');

// To not trigger queries on every selection we store this state locally here and only update the variable onBlur
const [uncommittedValue, setUncommittedValue] = useState(arrayValue);
const [uncommittedValue, setUncommittedValue] = useState(values);

// Detect value changes outside
useEffect(() => {
setUncommittedValue(arrayValue);
}, [arrayValue]);
setUncommittedValue(values);
}, [values]);

const onInputChange = (value: string, { action }: InputActionMeta) => {
if (action === 'input-change') {
Expand Down Expand Up @@ -237,13 +248,16 @@ export function GroupByVariableRenderer({ model }: SceneComponentProps<MultiValu
components={{ Option: OptionWithCheckbox }}
onInputChange={onInputChange}
onBlur={() => {
model.changeValueTo(uncommittedValue);
model.changeValueTo(
uncommittedValue.map((x) => x.value!),
uncommittedValue.map((x) => x.label!)
);
}}
onChange={(newValue, action) => {
if (action.action === 'clear' && noValueOnClear) {
model.changeValueTo([]);
}
setUncommittedValue(newValue.map((x) => x.value!));
setUncommittedValue(newValue);
}}
onOpenMenu={async () => {
setIsFetchingOptions(true);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { SceneObjectUrlSyncHandler, SceneObjectUrlValues } from '../../core/types';
import { GroupByVariable } from './GroupByVariable';
import { toUrlCommaDelimitedString, unescapeUrlDelimiters } from '../utils';
import { VariableValue } from '../types';

export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can reuse MultiValueUrlSyncHandler but with an option to sync label? since they are so close to identical

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I can merge them into one. I'll implement it in a subsequent commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started working on this but to be honest it kinda makes it weird to bring urlOptions to multi-value variable. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bfmatei I think the whole concept of urlOptions is strange, need to figure out a way we can avoid it.

the URL state should only be concerned with setting the current value/text not the options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing an update without it now.

public constructor(private _sceneObject: GroupByVariable) {}

private getKey(): string {
return `var-${this._sceneObject.state.name}`;
}

public getKeys(): string[] {
if (this._sceneObject.state.skipUrlSync) {
return [];
}

return [this.getKey()];
}

public getUrlState(): SceneObjectUrlValues {
if (this._sceneObject.state.skipUrlSync) {
return {};
}

return { [this.getKey()]: toUrlValues(this._sceneObject.state.value, this._sceneObject.state.text) };
}

public updateFromUrl(values: SceneObjectUrlValues): void {
let urlValue = values[this.getKey()];

if (urlValue != null) {
/**
* Initial URL Sync happens before scene objects are activated.
* We need to skip validation in this case to make sure values set via URL are maintained.
*/
if (!this._sceneObject.isActive) {
this._sceneObject.skipNextValidation = true;
}

const { values, texts } = fromUrlValues(urlValue);

this._sceneObject.changeValueTo(values, texts);
}
}
}

function toUrlValues(values: VariableValue, texts: VariableValue): string[] {
values = Array.isArray(values) ? values : [values];
texts = Array.isArray(texts) ? texts : [texts];

return values.map((value, idx) => {
if (value === undefined || value === null) {
return '';
}

value = String(value);

let text = texts[idx];
text = text === undefined || text === null ? value : String(text);

return toUrlCommaDelimitedString(value, text);
});
}

function fromUrlValues(urlValues: string | string[]): { values: string[]; texts: string[] } {
urlValues = Array.isArray(urlValues) ? urlValues : [urlValues];

return urlValues.reduce<{ values: string[]; texts: string[] }>(
(acc, urlValue) => {
const [value, label] = (urlValue ?? '').split(',');

acc.values.push(unescapeUrlDelimiters(value));
acc.texts.push(unescapeUrlDelimiters(label ?? value));

return acc;
},
{
values: [],
texts: [],
}
);
}