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

feat(widget-builder): Add legend aliases to visualize #82313

Merged
merged 2 commits into from
Dec 19, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,34 @@ describe('Visualize', () => {
).toBeDisabled();
});

it('does not show the legend alias input for chart widgets', async () => {
render(
<WidgetBuilderProvider>
<Visualize />
</WidgetBuilderProvider>,
{
organization,
router: RouterFixture({
location: LocationFixture({
query: {
dataset: WidgetType.TRANSACTIONS,
displayType: DisplayType.LINE,
yAxis: ['p90(transaction.duration)'],
},
}),
}),
}
);

expect(
await screen.findByRole('button', {name: 'Column Selection'})
).toHaveTextContent('transaction.duration');
expect(
await screen.findByRole('button', {name: 'Aggregate Selection'})
).toHaveTextContent('p90');
expect(screen.queryByLabelText('Legend Alias')).not.toBeInTheDocument();
});

describe('spans', () => {
beforeEach(() => {
jest.mocked(useSpanTags).mockImplementation((type?: 'string' | 'number') => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -425,13 +425,23 @@ function Visualize() {
</Fragment>
)}
</FieldBar>
<FieldExtras>
<LegendAliasInput
type="text"
name="name"
placeholder={t('Add Alias')}
onChange={() => {}}
/>
<FieldExtras isChartWidget={isChartWidget}>
{!isChartWidget && (
<LegendAliasInput
type="text"
name="name"
placeholder={t('Add Alias')}
value={field.alias}
onChange={e => {
const newFields = cloneDeep(fields);
newFields[index].alias = e.target.value;
dispatch({
type: updateAction,
payload: newFields,
});
}}
/>
)}
<StyledDeleteButton
borderless
icon={<IconDelete />}
Expand Down Expand Up @@ -628,11 +638,11 @@ const FieldRow = styled('div')`

const StyledDeleteButton = styled(Button)``;

const FieldExtras = styled('div')`
const FieldExtras = styled('div')<{isChartWidget: boolean}>`
display: flex;
flex-direction: row;
gap: ${space(1)};
flex: 1;
flex: ${p => (p.isChartWidget ? '0' : '1')};
`;

const AddButton = styled(Button)`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ import {LocationFixture} from 'sentry-fixture/locationFixture';

import {act, renderHook} from 'sentry-test/reactTestingLibrary';

import type {Column} from 'sentry/utils/discover/fields';
import {useLocation} from 'sentry/utils/useLocation';
import {useNavigate} from 'sentry/utils/useNavigate';
import {DisplayType, WidgetType} from 'sentry/views/dashboards/types';
import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext';
import useWidgetBuilderState, {
BuilderStateAction,
serializeFields,
} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState';
import {FieldValueKind} from 'sentry/views/discover/table/types';

jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/useNavigate');
Expand Down Expand Up @@ -497,6 +500,44 @@ describe('useWidgetBuilderState', () => {
},
]);
});

it('decodes both JSON formatted fields and non-JSON formatted fields', () => {
mockedUsedLocation.mockReturnValue(
LocationFixture({
query: {
field: [
'{"field": "event.type", "alias": "test"}',
'p90(transaction.duration)',
],
},
})
);

const {result} = renderHook(() => useWidgetBuilderState(), {
wrapper: WidgetBuilderProvider,
});

expect(result.current.state.fields).toEqual([
{field: 'event.type', alias: 'test', kind: 'field'},
{
function: ['p90', 'transaction.duration', undefined, undefined],
alias: undefined,
kind: 'function',
},
]);
});

it('encodes fields to JSON when they have aliases', () => {
const fields = [
{field: 'event.type', alias: 'test', kind: FieldValueKind.FIELD},
{field: 'event.type', alias: undefined, kind: FieldValueKind.FIELD},
] as Column[];
const encodedFields = serializeFields(fields);
expect(encodedFields).toEqual([
'{"field":"event.type","alias":"test"}',
'event.type',
]);
});
});

describe('yAxis', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,15 +237,30 @@ function deserializeDataset(value: string): WidgetType {
* them into a list of fields and functions
*/
function deserializeFields(fields: string[]): Column[] {
return fields.map(field => explodeField({field}));
return fields.map(stringifiedField => {
try {
const {field, alias} = JSON.parse(stringifiedField);
return explodeField({field, alias});
} catch (error) {
return explodeField({field: stringifiedField, alias: undefined});
}
});
}

/**
* Takes fields in the field and function format and coverts
* them into a list of strings compatible with query params
*/
function serializeFields(fields: Column[]): string[] {
return fields.map(generateFieldAsString);
export function serializeFields(fields: Column[]): string[] {
return fields.map(field => {
if (field.alias) {
return JSON.stringify({
field: generateFieldAsString(field),
alias: field.alias,
});
}
return generateFieldAsString(field);
});
}

function serializeSorts(sorts: Sort[]): string[] {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {DisplayType, WidgetType} from 'sentry/views/dashboards/types';
import type {WidgetBuilderState} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState';
import {convertBuilderStateToWidget} from 'sentry/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget';
import {FieldValueKind} from 'sentry/views/discover/table/types';

describe('convertBuilderStateToWidget', function () {
it('returns the widget with the provided widget queries state', function () {
Expand Down Expand Up @@ -36,7 +37,7 @@ describe('convertBuilderStateToWidget', function () {
queries: [
{
fields: ['geo.country', 'count()', 'count_unique(user)'],
fieldAliases: [],
fieldAliases: ['', '', ''],
aggregates: ['count()'],
columns: ['geo.country'],
conditions: '',
Expand Down Expand Up @@ -70,4 +71,18 @@ describe('convertBuilderStateToWidget', function () {
expect(widget.queries[0].orderby).toEqual('-count()');
expect(widget.queries[1].orderby).toEqual('-count()');
});

it('adds aliases to the widget queries', function () {
const mockState: WidgetBuilderState = {
fields: [
{field: 'geo.country', alias: 'test', kind: FieldValueKind.FIELD},
{field: 'geo.country', alias: undefined, kind: FieldValueKind.FIELD},
{field: 'geo.country', alias: 'another one', kind: FieldValueKind.FIELD},
],
};

const widget = convertBuilderStateToWidget(mockState);

expect(widget.queries[0].fieldAliases).toEqual(['test', '', 'another one']);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export function convertBuilderStateToWidget(state: WidgetBuilderState): Widget {
const queries = defined(state.query) && state.query.length > 0 ? state.query : [''];

const fields = state.fields?.map(generateFieldAsString);
const fieldAliases = state.fields?.map(field => field.alias ?? '');
const aggregates =
(state.yAxis?.length ?? 0) > 0
? state.yAxis?.map(generateFieldAsString)
Expand Down Expand Up @@ -47,6 +48,7 @@ export function convertBuilderStateToWidget(state: WidgetBuilderState): Widget {
columns: columns ?? [],
conditions: query,
orderby: sort,
fieldAliases: fieldAliases ?? [],
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,24 @@ describe('convertWidgetToBuilderStateParams', () => {
const params = convertWidgetToBuilderStateParams(widget);
expect(params.yAxis).toEqual([]);
});

it('stringifies the fields when converting a table to builder params', () => {
const widget = {
...getDefaultWidget(WidgetType.ERRORS),
queries: [
{
aggregates: [],
columns: [],
conditions: '',
name: '',
orderby: '',

fields: ['geo.country'],
fieldAliases: ['test'],
},
],
};
const params = convertWidgetToBuilderStateParams(widget);
expect(params.field).toEqual(['{"field":"geo.country","alias":"test"}']);
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,24 @@
import {DisplayType, type Widget, WidgetType} from 'sentry/views/dashboards/types';
import type {WidgetBuilderStateQueryParams} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState';
import {explodeField} from 'sentry/utils/discover/fields';
import {
DisplayType,
type Widget,
type WidgetQuery,
WidgetType,
} from 'sentry/views/dashboards/types';
import {
serializeFields,
type WidgetBuilderStateQueryParams,
} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState';

function stringifyFields(
query: WidgetQuery,
fieldKey: 'fields' | 'columns' | 'aggregates'
) {
const fields = query[fieldKey]?.map((field, index) =>
explodeField({field, alias: query.fieldAliases?.[index]})
);
return fields ? serializeFields(fields) : [];
}

/**
* Converts a widget to a set of query params that can be used to
Expand All @@ -17,10 +36,12 @@ export function convertWidgetToBuilderStateParams(
widget.displayType === DisplayType.TABLE ||
widget.displayType === DisplayType.BIG_NUMBER
) {
field = widget.queries.flatMap(q => q.fields ?? []);
field = widget.queries.flatMap(widgetQuery => stringifyFields(widgetQuery, 'fields'));
yAxis = [];
} else {
field = widget.queries.flatMap(q => q.columns);
field = widget.queries.flatMap(widgetQuery =>
stringifyFields(widgetQuery, 'columns')
);
}

return {
Expand Down
Loading