feat(widget-builder): Support aggregates with no arguments in visuali…

…ze (#81967) Handles aggregates without any arguments by implementing the following behaviour: - If an aggregate with no parameters is selected (e.g. `count()` in transactions/errors) then the column is wiped clear and disabled - After that, if an aggregate _with_ a column is selected, set the column to the default value - If an aggregate is selected that allows for a column, and a column is already set, persist that column
getsentry · Dec 11, 2024 · f750cb0 · f750cb0
1 parent a4422b0
commit f750cb0
Show file tree

Hide file tree

Showing 2 changed files with 132 additions and 13 deletions.
diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize.spec.tsx
@@ -91,6 +91,98 @@ describe('Visualize', () => {
     expect(screen.queryAllByRole('button', {name: 'Remove field'})[0]).toBeDisabled();
   });
 
+  it('disables the column selection when the aggregate has no parameters', async () => {
+    render(
+      <WidgetBuilderProvider>
+        <Visualize />
+      </WidgetBuilderProvider>,
+      {
+        organization,
+        router: RouterFixture({
+          location: LocationFixture({
+            query: {
+              yAxis: ['max(spans.db)'],
+              dataset: WidgetType.TRANSACTIONS,
+              displayType: DisplayType.LINE,
+            },
+          }),
+        }),
+      }
+    );
+
+    expect(screen.getByRole('button', {name: 'Column Selection'})).toBeEnabled();
+    await userEvent.click(screen.getByRole('button', {name: 'Aggregate Selection'}));
+    await userEvent.click(screen.getByRole('option', {name: 'count'}));
+
+    expect(screen.getByRole('button', {name: 'Column Selection'})).toBeDisabled();
+    expect(screen.getByRole('button', {name: 'Aggregate Selection'})).toBeEnabled();
+
+    expect(screen.getByRole('button', {name: 'Column Selection'})).toHaveValue('');
+  });
+
+  it('adds the default value for the column selection when the aggregate has parameters', async () => {
+    render(
+      <WidgetBuilderProvider>
+        <Visualize />
+      </WidgetBuilderProvider>,
+      {
+        organization,
+        router: RouterFixture({
+          location: LocationFixture({
+            query: {
+              yAxis: ['count()'],
+              dataset: WidgetType.TRANSACTIONS,
+              displayType: DisplayType.LINE,
+            },
+          }),
+        }),
+      }
+    );
+
+    expect(screen.getByRole('button', {name: 'Column Selection'})).toBeDisabled();
+
+    await userEvent.click(screen.getByRole('button', {name: 'Aggregate Selection'}));
+    await userEvent.click(screen.getByRole('option', {name: 'p95'}));
+
+    expect(screen.getByRole('button', {name: 'Column Selection'})).toBeEnabled();
+    expect(screen.getByRole('button', {name: 'Column Selection'})).toHaveTextContent(
+      'transaction.duration'
+    );
+    expect(screen.getByRole('button', {name: 'Aggregate Selection'})).toHaveTextContent(
+      'p95'
+    );
+  });
+
+  it('maintains the selected aggregate when the column selection is changed and there are parameters', async () => {
+    render(
+      <WidgetBuilderProvider>
+        <Visualize />
+      </WidgetBuilderProvider>,
+      {
+        organization,
+        router: RouterFixture({
+          location: LocationFixture({
+            query: {
+              yAxis: ['max(spans.db)'],
+              dataset: WidgetType.TRANSACTIONS,
+              displayType: DisplayType.LINE,
+            },
+          }),
+        }),
+      }
+    );
+
+    await userEvent.click(screen.getByRole('button', {name: 'Aggregate Selection'}));
+    await userEvent.click(screen.getByRole('option', {name: 'p95'}));
+
+    expect(screen.getByRole('button', {name: 'Column Selection'})).toHaveTextContent(
+      'spans.db'
+    );
+    expect(screen.getByRole('button', {name: 'Aggregate Selection'})).toHaveTextContent(
+      'p95'
+    );
+  });
+
   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
@@ -53,10 +53,6 @@ function Visualize() {
     [organization, tags, customMeasurements, datasetConfig]
   );
 
-  // TODO: no parameters should show up as primary options?
-  // let aggregateOptions = Object.values(fieldOptions).filter(
-  //   option => option.value.kind === 'function' && option.value.meta.parameters.length > 0
-  // );
   const aggregateOptions = useMemo(
     () =>
       Object.values(fieldOptions).filter(option =>
@@ -86,7 +82,6 @@ function Visualize() {
           const columnOptions = Object.values(fieldOptions)
             .filter(option => {
               return (
-                // TODO: This should allow for aggregates without parameters
                 option.value.kind !== FieldValueKind.FUNCTION &&
                 (datasetConfig.filterYAxisAggregateParams?.(
                   field,
@@ -103,6 +98,18 @@ function Visualize() {
                   : option.value.meta.name,
             }));
 
+          let matchingAggregate;
+          if (
+            fields[index].kind === FieldValueKind.FUNCTION &&
+            FieldValueKind.FUNCTION in fields[index]
+          ) {
+            matchingAggregate = aggregateOptions.find(
+              option =>
+                option.value.meta.name ===
+                parseFunction(stringFields?.[index] ?? '')?.name
+            );
+          }
+
           return (
             <FieldRow key={index}>
               <FieldBar data-testid={'field-bar'}>
@@ -113,7 +120,7 @@ function Visualize() {
                   onChange={newField => {
                     // TODO: Handle scalars (i.e. no aggregate, for tables)
                     // Update the current field's aggregate with the new aggregate
-                    if (field.kind === 'function') {
+                    if (field.kind === FieldValueKind.FUNCTION) {
                       field.function[1] = newField.value as string;
                     }
                     dispatch({
@@ -124,23 +131,43 @@ function Visualize() {
                   triggerProps={{
                     'aria-label': t('Column Selection'),
                   }}
+                  disabled={
+                    fields[index].kind === FieldValueKind.FUNCTION &&
+                    matchingAggregate?.value.meta.parameters.length === 0
+                  }
                 />
                 {/* TODO: Add equation options */}
-                {/* TODO: Handle aggregates with no parameters */}
                 {/* TODO: Handle aggregates with multiple parameters */}
                 <AggregateCompactSelect
                   options={aggregateOptions.map(option => ({
                     value: option.value.meta.name,
                     label: option.value.meta.name,
                   }))}
                   value={parseFunction(stringFields?.[index] ?? '')?.name ?? ''}
-                  onChange={newAggregate => {
+                  onChange={aggregateSelection => {
                     // TODO: Handle scalars (i.e. no aggregate, for tables)
                     // Update the current field's aggregate with the new aggregate
-                    const currentField = fields?.[index];
-                    if (currentField.kind === 'function') {
-                      currentField.function[0] =
-                        newAggregate.value as AggregationKeyWithAlias;
+                    if (field.kind === FieldValueKind.FUNCTION) {
+                      field.function[0] =
+                        aggregateSelection.value as AggregationKeyWithAlias;
+                      const newAggregate = aggregateOptions.find(
+                        option => option.value.meta.name === aggregateSelection.value
+                      );
+                      if (
+                        newAggregate?.value.meta &&
+                        'parameters' in newAggregate.value.meta
+                      ) {
+                        // There are aggregates that have no parameters, so wipe out the argument
+                        // if it's supposed to be empty
+                        if (newAggregate.value.meta.parameters.length === 0) {
+                          field.function[1] = '';
+                        } else {
+                          field.function[1] =
+                            (field.function[1] ||
+                              newAggregate.value.meta.parameters[0].defaultValue) ??
+                            '';
+                        }
+                      }
                     }
                     dispatch({
                       type: updateAction,
@@ -190,7 +217,7 @@ function Visualize() {
                 // TODO: Define a default aggregate/field for the datasets?
                 {
                   function: ['count', '', undefined, undefined],
-                  kind: 'function',
+                  kind: FieldValueKind.FUNCTION,
                 },
               ],
             })