ref(new-widget-builder-experience): hide sort by step if no group by value is selected (#33953)

Hides the sort field unless viewing a table, top-n, or timeseries with a grouping.
Also fixes a number of issues related to setting the expected sort by when
options are removed or changed.

static/app/views/dashboardsV2/widgetBuilder/utils.tsx

@@ -123,6 +123,16 @@ export function normalizeQueries({
         query.fields = fields.filter(field => !columns.includes(field));
       }
 
+      if (
+        getIsTimeseriesChart(displayType) &&
+        !query.columns.filter(column => !!column).length
+      ) {
+        // The orderby is only applicable for timeseries charts when there's a
+        // grouping selected, if all fields are empty then we also reset the orderby
+        query.orderby = '';
+        return query;
+      }
+
       const queryOrderBy =
         widgetType === WidgetType.METRICS
           ? stripDerivedMetricsPrefix(queries[0].orderby)
@@ -340,7 +350,7 @@ export function filterPrimaryOptions({
   );
 }
 
-export function getResultsLimit(numQueries, numYAxes) {
+export function getResultsLimit(numQueries: number, numYAxes: number) {
   if (numQueries === 0 || numYAxes === 0) {
     return DEFAULT_RESULTS_LIMIT;
   }

static/app/views/dashboardsV2/widgetBuilder/widgetBuilder.tsx

@@ -64,6 +64,7 @@ import {
 import {IssueSortOptions} from 'sentry/views/issueList/utils';
 
 import {DEFAULT_STATS_PERIOD} from '../data';
+import {getNumEquations} from '../utils';
 
 import {ColumnsStep} from './buildSteps/columnsStep';
 import {DashboardStep} from './buildSteps/dashboardStep';
@@ -190,12 +191,6 @@ function WidgetBuilder({
     location.query.defaultWidgetQuery
   );
 
-  useEffect(() => {
-    if (objectIsEmpty(tags)) {
-      loadOrganizationTags(api, organization.slug, selection);
-    }
-  }, []);
-
   // Feature flag for new widget builder design. This feature is still a work in progress and not yet available internally.
   const widgetBuilderNewDesign = organization.features.includes(
     'new-widget-builder-experience-design'
@@ -272,6 +267,10 @@ function WidgetBuilder({
       new_widget: !isEditing,
     });
 
+    if (objectIsEmpty(tags)) {
+      loadOrganizationTags(api, organization.slug, selection);
+    }
+
     if (isEditing && isValidWidgetIndex) {
       const widgetFromDashboard = filteredDashboardWidgets[widgetIndexNum];
       setState({
@@ -433,7 +432,10 @@ function WidgetBuilder({
       set(newState, 'queries', normalized);
 
       if (widgetBuilderNewDesign) {
-        if (getIsTimeseriesChart(newDisplayType) && normalized[0].columns.length) {
+        if (
+          getIsTimeseriesChart(newDisplayType) &&
+          normalized[0].columns.filter(column => !!column).length
+        ) {
           // If a limit already exists (i.e. going between timeseries) then keep it,
           // otherwise calculate a limit
           newState.limit =
@@ -566,7 +568,7 @@ function WidgetBuilder({
 
     const newQueries = state.queries.map(query => {
       const isDescending = query.orderby.startsWith('-');
-      const orderbyAggregateAliasField = trimStart(query.orderby, '-');
+      const rawOrderby = trimStart(query.orderby, '-');
       const prevAggregateAliasFieldStrings = query.aggregates.map(aggregate =>
         state.dataSet === DataSet.RELEASE
           ? stripDerivedMetricsPrefix(aggregate)
@@ -601,32 +603,36 @@ function WidgetBuilder({
         newQuery.columns = columnsAndAggregates?.columns ?? [];
       }
 
-      if (
-        !aggregateAliasFieldStrings.includes(orderbyAggregateAliasField) &&
-        query.orderby !== ''
-      ) {
-        if (prevAggregateAliasFieldStrings.length === newFields.length) {
-          // The Field that was used in orderby has changed. Get the new field.
+      if (!aggregateAliasFieldStrings.includes(rawOrderby) && query.orderby !== '') {
+        if (
+          prevAggregateAliasFieldStrings.length === newFields.length &&
+          prevAggregateAliasFieldStrings.includes(rawOrderby)
+        ) {
+          // The aggregate that was used in orderby has changed. Get the new field.
           let newOrderByValue =
             aggregateAliasFieldStrings[
-              prevAggregateAliasFieldStrings.indexOf(orderbyAggregateAliasField)
+              prevAggregateAliasFieldStrings.indexOf(rawOrderby)
             ];
 
           if (!stripEquationPrefix(newOrderByValue ?? '')) {
             newOrderByValue = '';
           }
 
-          if (isDescending) {
-            newQuery.orderby = `-${newOrderByValue}`;
-          } else {
-            newQuery.orderby = newOrderByValue;
-          }
+          newQuery.orderby = `${isDescending ? '-' : ''}${newOrderByValue}`;
         } else {
+          const isFromAggregates = aggregateAliasFieldStrings.includes(rawOrderby);
+          const isCustomEquation = isEquation(rawOrderby);
+          const isUsedInGrouping = newQuery.columns.includes(rawOrderby);
+
+          const keepCurrentOrderby =
+            isFromAggregates || isCustomEquation || isUsedInGrouping;
+          const firstAggregateAlias = isEquation(aggregateAliasFieldStrings[0])
+            ? `equation[${getNumEquations(aggregateAliasFieldStrings) - 1}]`
+            : aggregateAliasFieldStrings[0];
+
           newQuery.orderby = widgetBuilderNewDesign
-            ? ((aggregateAliasFieldStrings.includes(orderbyAggregateAliasField) ||
-                isEquation(orderbyAggregateAliasField)) &&
-                newQuery.orderby) ||
-              `${isDescending ? '-' : ''}${aggregateAliasFieldStrings[0]}`
+            ? (keepCurrentOrderby && newQuery.orderby) ||
+              `${isDescending ? '-' : ''}${firstAggregateAlias}`
             : '';
         }
       }
@@ -670,28 +676,47 @@ function WidgetBuilder({
     const newQueries = state.queries.map(query => {
       const newQuery = cloneDeep(query);
       newQuery.columns = fieldStrings;
+      const orderby = trimStart(newQuery.orderby, '-');
+      const aggregateAliasFieldStrings = newQuery.aggregates.map(getAggregateAlias);
+
+      if (!fieldStrings.length) {
+        // The grouping was cleared, so clear the orderby
+        newQuery.orderby = '';
+      } else if (
+        aggregateAliasFieldStrings.length &&
+        !aggregateAliasFieldStrings.includes(orderby) &&
+        !newQuery.columns.includes(orderby) &&
+        !isEquation(orderby)
+      ) {
+        // If the orderby isn't contained in either aggregates or columns, choose the first aggregate
+        const isDescending = newQuery.orderby.startsWith('-');
+        const prefix = orderby && !isDescending ? '' : '-';
+        const firstAggregateAlias = isEquation(aggregateAliasFieldStrings[0])
+          ? `equation[${getNumEquations(aggregateAliasFieldStrings) - 1}]`
+          : aggregateAliasFieldStrings[0];
+        newQuery.orderby = `${prefix}${firstAggregateAlias}`;
+      }
       return newQuery;
     });
 
     set(newState, 'userHasModified', true);
     set(newState, 'queries', newQueries);
 
-    if (widgetBuilderNewDesign && isTimeseriesChart) {
-      const groupByFields = newState.queries[0].columns.filter(
-        field => !(field === 'equation|')
+    const groupByFields = newState.queries[0].columns.filter(
+      field => !(field === 'equation|')
+    );
+
+    if (groupByFields.length === 0) {
+      set(newState, 'limit', undefined);
+    } else {
+      set(
+        newState,
+        'limit',
+        Math.min(
+          newState.limit ?? DEFAULT_RESULTS_LIMIT,
+          getResultsLimit(newQueries.length, newQueries[0].aggregates.length)
+        )
       );
-      if (groupByFields.length === 0) {
-        set(newState, 'limit', undefined);
-      } else {
-        set(
-          newState,
-          'limit',
-          Math.min(
-            newState.limit ?? DEFAULT_RESULTS_LIMIT,
-            getResultsLimit(newQueries.length, newQueries[0].aggregates.length)
-          )
-        );
-      }
     }
 
     setState(newState);
@@ -920,6 +945,16 @@ function WidgetBuilder({
     return false;
   }
 
+  if (isEditing && !isValidWidgetIndex) {
+    return (
+      <SentryDocumentTitle title={dashboard.title} orgSlug={orgSlug}>
+        <PageContent>
+          <LoadingError message={t('The widget you want to edit was not found.')} />
+        </PageContent>
+      </SentryDocumentTitle>
+    );
+  }
+
   const canAddSearchConditions =
     [DisplayType.LINE, DisplayType.AREA, DisplayType.BAR].includes(state.displayType) &&
     state.queries.length < 3;
@@ -950,15 +985,16 @@ function WidgetBuilder({
     ? fields.map((field, index) => explodeField({field, alias: fieldAliases[index]}))
     : [...explodedColumns, ...explodedAggregates];
 
-  if (isEditing && !isValidWidgetIndex) {
-    return (
-      <SentryDocumentTitle title={dashboard.title} orgSlug={orgSlug}>
-        <PageContent>
-          <LoadingError message={t('The widget you want to edit was not found.')} />
-        </PageContent>
-      </SentryDocumentTitle>
-    );
-  }
+  const groupByValueSelected = currentWidget.queries.some(query => {
+    const noEmptyColumns = query.columns.filter(column => !!column);
+    return noEmptyColumns.length > 0;
+  });
+
+  // The SortBy field shall only be displayed in tabular visualizations or
+  // on time-series visualizations when at least one groupBy value is selected
+  const displaySortByStep =
+    (widgetBuilderNewDesign && isTimeseriesChart && groupByValueSelected) ||
+    isTabularChart;
 
   return (
     <SentryDocumentTitle title={dashboard.title} orgSlug={orgSlug}>
@@ -1053,7 +1089,7 @@ function WidgetBuilder({
                       dataSet={state.dataSet}
                     />
                   )}
-                  {((widgetBuilderNewDesign && isTimeseriesChart) || isTabularChart) && (
+                  {displaySortByStep && (
                     <SortByStep
                       limit={state.limit}
                       displayType={state.displayType}

tests/js/spec/views/dashboardsV2/widgetBuilder/widgetBuilder.spec.tsx

@@ -1575,6 +1575,51 @@ describe('WidgetBuilder', function () {
       });
     });
 
+    it('sortBy is only visible on tabular visualizations or when there is a groupBy value selected on time-series visualizations', async function () {
+      renderTestComponent({
+        orgFeatures: [...defaultOrgFeatures, 'new-widget-builder-experience-design'],
+      });
+
+      // Sort by shall be visible on table visualization
+      expect(await screen.findByText('Sort by a column')).toBeInTheDocument();
+
+      // Update visualization to be a time-series
+      userEvent.click(screen.getByText('Table'));
+      userEvent.click(screen.getByText('Line Chart'));
+
+      // Time-series visualizations display GroupBy step
+      expect(await screen.findByText('Group your results')).toBeInTheDocument();
+
+      // Do not show sortBy when empty columns (groupBys) are added
+      userEvent.click(screen.getByText('Add Group'));
+      expect(screen.getAllByText('Select group')).toHaveLength(2);
+
+      // SortBy step shall not be visible
+      expect(screen.queryByText('Sort by a y-axis')).not.toBeInTheDocument();
+
+      // Select GroupBy value
+      await selectEvent.select(screen.getAllByText('Select group')[0], 'project');
+
+      // Now that at least one groupBy value is selected, the SortBy step shall be visible
+      expect(screen.getByText('Sort by a y-axis')).toBeInTheDocument();
+
+      // Remove selected GroupBy value
+      userEvent.click(screen.getAllByLabelText('Remove group')[0]);
+
+      // SortBy step shall no longer be visible
+      expect(screen.queryByText('Sort by a y-axis')).not.toBeInTheDocument();
+
+      // Update visualization to be "Top 5 Events"
+      userEvent.click(screen.getByText('Line Chart'));
+      userEvent.click(screen.getByText('Top 5 Events'));
+
+      // Tabular visualizations display "Choose your columns" step
+      expect(await screen.findByText('Choose your columns')).toBeInTheDocument();
+
+      // SortBy step shall be visible
+      expect(screen.getByText('Sort by a y-axis')).toBeInTheDocument();
+    });
+
     it('allows for sorting by a custom equation', async function () {
       renderTestComponent({
         orgFeatures: [...defaultOrgFeatures, 'new-widget-builder-experience-design'],
@@ -1775,6 +1820,90 @@ describe('WidgetBuilder', function () {
 
       expect(screen.queryByPlaceholderText('Enter Equation')).not.toBeInTheDocument();
     });
+
+    it('persists a sort by a grouping when changing y-axes', async function () {
+      renderTestComponent({
+        orgFeatures: [...defaultOrgFeatures, 'new-widget-builder-experience-design'],
+        query: {
+          source: DashboardWidgetSource.DASHBOARDS,
+          displayType: DisplayType.LINE,
+        },
+      });
+
+      await selectEvent.select(await screen.findByText('count()'), /count_unique/);
+      await selectEvent.select(screen.getByText('Select group'), 'project');
+      await selectEvent.select(screen.getByText('count_unique(user)'), 'project');
+
+      await selectEvent.select(screen.getByText('count_unique(…)'), 'count()');
+
+      // project should appear in the group by field, as well as the sort field
+      expect(screen.getAllByText('project')).toHaveLength(2);
+    });
+
+    it('persists sort by a y-axis when grouping changes', async function () {
+      renderTestComponent({
+        orgFeatures: [...defaultOrgFeatures, 'new-widget-builder-experience-design'],
+        query: {
+          source: DashboardWidgetSource.DASHBOARDS,
+          displayType: DisplayType.LINE,
+        },
+      });
+
+      userEvent.click(await screen.findByText('Add Overlay'));
+      await selectEvent.select(screen.getByText('(Required)'), /count_unique/);
+      await selectEvent.select(screen.getByText('Select group'), 'project');
+
+      // Change the sort by to count_unique
+      await selectEvent.select(screen.getAllByText('count()')[1], /count_unique/);
+
+      // Change the grouping
+      await selectEvent.select(screen.getByText('project'), 'environment');
+
+      // count_unique(user) should still be the sorting field
+      expect(await screen.findByText('count_unique(user)')).toBeInTheDocument();
+    });
+
+    it('falls back to the first aggregate to sort by when the sorting aggregate is removed', async function () {
+      renderTestComponent({
+        orgFeatures: [...defaultOrgFeatures, 'new-widget-builder-experience-design'],
+        query: {
+          source: DashboardWidgetSource.DASHBOARDS,
+          displayType: DisplayType.LINE,
+        },
+      });
+
+      userEvent.click(await screen.findByText('Add Overlay'));
+      await selectEvent.select(screen.getByText('(Required)'), /count_unique/);
+      await selectEvent.select(screen.getByText('Select group'), 'project');
+
+      userEvent.click(screen.getAllByLabelText('Remove this Y-Axis')[0]);
+
+      // count_unique(user) should now be the sorting field
+      expect(screen.getByText('count_unique(user)')).toBeInTheDocument();
+    });
+
+    it('does not remove the Custom Equation field if a grouping is updated', async function () {
+      renderTestComponent({
+        orgFeatures: [...defaultOrgFeatures, 'new-widget-builder-experience-design'],
+        query: {
+          source: DashboardWidgetSource.DASHBOARDS,
+          displayType: DisplayType.LINE,
+        },
+      });
+
+      await selectEvent.select(await screen.findByText('Select group'), 'project');
+      await selectEvent.select(screen.getAllByText('count()')[1], 'Custom Equation');
+      userEvent.paste(
+        screen.getByPlaceholderText('Enter Equation'),
+        'count_unique(user) * 2'
+      );
+      userEvent.keyboard('{enter}');
+
+      userEvent.click(screen.getByText('Add Group'));
+      expect(screen.getByPlaceholderText('Enter Equation')).toHaveValue(
+        'count_unique(user) * 2'
+      );
+    });
   });
 
   describe('Widget creation coming from other verticals', function () {
@@ -1957,6 +2086,7 @@ describe('WidgetBuilder', function () {
     userEvent.click(await screen.findByText('Table'));
     userEvent.click(screen.getByText('Line Chart'));
     await selectEvent.select(screen.getAllByText('count()')[0], 'count_unique(…)');
+    await selectEvent.select(screen.getByText('Select group'), 'project');
 
     MockApiClient.clearMockResponses();
     eventsStatsMock = MockApiClient.addMockResponse({
@@ -2255,7 +2385,6 @@ describe('WidgetBuilder', function () {
             query: {
               environment: [],
               field: [`sum(session)`],
-              orderBy: '-sum(session)',
               groupBy: [],
               interval: '1h',
               project: [],