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

Supports aliases for fields. I do this by modifying the serialize/deserialize methods to allow for a JSON string that represents the field with the alias. It only applies the JSON string if it has an alias, otherwise we just put it in as normal. This is for convenience so we don't have to update all of the tests to account for this. The deserialize method will try to parse the field as JSON, and if that doesn't work, fallback to the old method of exploding the string. Also adds the correct handling to take a widget and turn it into params, or take the state and convert it into a widget (specifically, passing the aliases around in the `WidgetQuery` data structure) Closes #82368
getsentry · Dec 19, 2024 · 00ffeb5 · 00ffeb5
1 parent f138600
commit 00ffeb5
Show file tree

Hide file tree

Showing 8 changed files with 169 additions and 17 deletions.
diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize.spec.tsx
@@ -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') => {

diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize.tsx
@@ -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 />}
@@ -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)`

diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx
@@ -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');
@@ -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', () => {

diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx
@@ -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[] {

diff --git a/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.spec.tsx b/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.spec.tsx
@@ -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 () {
@@ -36,7 +37,7 @@ describe('convertBuilderStateToWidget', function () {
       queries: [
         {
           fields: ['geo.country', 'count()', 'count_unique(user)'],
-          fieldAliases: [],
+          fieldAliases: ['', '', ''],
           aggregates: ['count()'],
           columns: ['geo.country'],
           conditions: '',
@@ -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']);
+  });
 });
diff --git a/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.ts b/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.ts
@@ -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)
@@ -47,6 +48,7 @@ export function convertBuilderStateToWidget(state: WidgetBuilderState): Widget {
       columns: columns ?? [],
       conditions: query,
       orderby: sort,
+      fieldAliases: fieldAliases ?? [],
     };
   });
 

diff --git a/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.spec.tsx b/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.spec.tsx
@@ -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"}']);
+  });
 });
diff --git a/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.ts b/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.ts
@@ -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
@@ -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 {