feat(generic-x-axis): add x sorting on series limit metric (#23274)

superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts

@@ -24,12 +24,16 @@ import {
   getXAxisLabel,
 } from '@superset-ui/core';
 import { PostProcessingFactory } from './types';
+import { extractExtraMetrics } from './utils';
 
 export const pivotOperator: PostProcessingFactory<PostProcessingPivot> = (
   formData,
   queryObject,
 ) => {
-  const metricLabels = ensureIsArray(queryObject.metrics).map(getMetricLabel);
+  const metricLabels = [
+    ...ensureIsArray(queryObject.metrics),
+    ...extractExtraMetrics(formData),
+  ].map(getMetricLabel);
   const xAxisLabel = getXAxisLabel(formData);
   const columns = queryObject.series_columns || queryObject.columns;
 

superset-frontend/packages/superset-ui-chart-controls/src/operators/sortOperator.ts

@@ -26,6 +26,7 @@ import {
   PostProcessingSort,
 } from '@superset-ui/core';
 import { PostProcessingFactory } from './types';
+import { extractExtraMetrics } from './utils';
 
 export const sortOperator: PostProcessingFactory<PostProcessingSort> = (
   formData,
@@ -34,7 +35,8 @@ export const sortOperator: PostProcessingFactory<PostProcessingSort> = (
   // the sortOperator only used in the barchart v2
   const sortableLabels = [
     getXAxisLabel(formData),
-    ...ensureIsArray(formData.metrics).map(metric => getMetricLabel(metric)),
+    ...ensureIsArray(formData.metrics).map(getMetricLabel),
+    ...extractExtraMetrics(formData).map(getMetricLabel),
   ].filter(Boolean);
 
   if (

superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts

@@ -0,0 +1,38 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+  getMetricLabel,
+  QueryFormData,
+  QueryFormMetric,
+} from '@superset-ui/core';
+
+export function extractExtraMetrics(
+  formData: QueryFormData,
+): QueryFormMetric[] {
+  const { groupby, timeseries_limit_metric, x_axis_sort } = formData;
+  const extra_metrics: QueryFormMetric[] = [];
+  if (
+    !(groupby || []).length &&
+    timeseries_limit_metric &&
+    getMetricLabel(timeseries_limit_metric) === x_axis_sort
+  ) {
+    extra_metrics.push(timeseries_limit_metric);
+  }
+  return extra_metrics;
+}

superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts

@@ -20,4 +20,5 @@
 export { getMetricOffsetsMap } from './getMetricOffsetsMap';
 export { isTimeComparison } from './isTimeComparison';
 export { isDerivedSeries } from './isDerivedSeries';
+export { extractExtraMetrics } from './extractExtraMetrics';
 export { TIME_COMPARISON_SEPARATOR } from './constants';

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx

@@ -23,12 +23,16 @@ import {
   getColumnLabel,
   getMetricLabel,
   isDefined,
-  isEqualArray,
   QueryFormColumn,
   QueryFormMetric,
   t,
 } from '@superset-ui/core';
-import { ControlPanelState, ControlState, ControlStateMapping } from '../types';
+import {
+  ControlPanelState,
+  ControlState,
+  ControlStateMapping,
+  isDataset,
+} from '../types';
 import { isTemporalColumn } from '../utils';
 
 export const contributionModeControl = {
@@ -59,39 +63,42 @@ export const xAxisSortControl = {
   name: 'x_axis_sort',
   config: {
     type: 'XAxisSortControl',
-    label: t('X-Axis Sort By'),
-    description: t('Whether to sort descending or ascending on the X-Axis.'),
-    shouldMapStateToProps: (
-      prevState: ControlPanelState,
-      state: ControlPanelState,
-    ) => {
-      const prevOptions = [
-        getColumnLabel(prevState?.controls?.x_axis?.value as QueryFormColumn),
-        ...ensureIsArray(prevState?.controls?.metrics?.value).map(metric =>
-          getMetricLabel(metric as QueryFormMetric),
-        ),
-      ];
-      const currOptions = [
-        getColumnLabel(state?.controls?.x_axis?.value as QueryFormColumn),
-        ...ensureIsArray(state?.controls?.metrics?.value).map(metric =>
-          getMetricLabel(metric as QueryFormMetric),
-        ),
-      ];
-      return !isEqualArray(prevOptions, currOptions);
-    },
-    mapStateToProps: (
-      { controls }: { controls: ControlStateMapping },
-      controlState: ControlState,
-    ) => {
-      const choices = [
-        getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
-        ...ensureIsArray(controls?.metrics?.value).map(metric =>
-          getMetricLabel(metric as QueryFormMetric),
-        ),
+    label: (state: ControlPanelState) =>
+      state.form_data?.orientation === 'horizontal'
+        ? t('Y-Axis Sort By')
+        : t('X-Axis Sort By'),
+    description: t('Decides which column to sort the base axis by.'),
+    shouldMapStateToProps: () => true,
+    mapStateToProps: (state: ControlPanelState, controlState: ControlState) => {
+      const { controls, datasource } = state;
+      const dataset = isDataset(datasource) ? datasource : undefined;
+      const columns = [controls?.x_axis?.value as QueryFormColumn].filter(
+        Boolean,
+      );
+      const metrics = [
+        ...ensureIsArray(controls?.metrics?.value as QueryFormMetric),
+        controls?.timeseries_limit_metric?.value as QueryFormMetric,
       ].filter(Boolean);
+      const options = [
+        ...columns.map(column => {
+          const value = getColumnLabel(column);
+          return {
+            value,
+            label: dataset?.verbose_map?.[value] || value,
+          };
+        }),
+        ...metrics.map(metric => {
+          const value = getMetricLabel(metric);
+          return {
+            value,
+            label: dataset?.verbose_map?.[value] || value,
+          };
+        }),
+      ];
+
       const shouldReset = !(
         typeof controlState.value === 'string' &&
-        choices.includes(controlState.value) &&
+        options.map(option => option.value).includes(controlState.value) &&
         !isTemporalColumn(
           getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
           controls?.datasource?.datasource,
@@ -100,10 +107,7 @@ export const xAxisSortControl = {
 
       return {
         shouldReset,
-        options: choices.map(entry => ({
-          value: entry,
-          label: entry,
-        })),
+        options,
       };
     },
     visibility: xAxisSortVisibility,
@@ -114,9 +118,12 @@ export const xAxisSortAscControl = {
   name: 'x_axis_sort_asc',
   config: {
     type: 'CheckboxControl',
-    label: t('X-Axis Sort Ascending'),
+    label: (state: ControlPanelState) =>
+      state.form_data?.orientation === 'horizontal'
+        ? t('Y-Axis Sort Ascending')
+        : t('X-Axis Sort Ascending'),
     default: true,
-    description: t('Whether to sort descending or ascending on the X-Axis.'),
+    description: t('Whether to sort ascending or descending on the base Axis.'),
     visibility: xAxisSortVisibility,
   },
 };

superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts

@@ -185,3 +185,33 @@ test('pivot by adhoc x_axis', () => {
     },
   });
 });
+
+test('pivot by x_axis with extra metrics', () => {
+  expect(
+    pivotOperator(
+      {
+        ...formData,
+        x_axis: 'foo',
+        x_axis_sort: 'bar',
+        groupby: [],
+        timeseries_limit_metric: 'bar',
+      },
+      {
+        ...queryObject,
+        series_columns: [],
+      },
+    ),
+  ).toEqual({
+    operation: 'pivot',
+    options: {
+      index: ['foo'],
+      columns: [],
+      aggregates: {
+        'count(*)': { operator: 'mean' },
+        'sum(val)': { operator: 'mean' },
+        bar: { operator: 'mean' },
+      },
+      drop_missing_columns: false,
+    },
+  });
+});

superset-frontend/packages/superset-ui-chart-controls/test/operators/sortOperator.test.ts

@@ -146,3 +146,28 @@ test('should sort by axis', () => {
     },
   });
 });
+
+test('should sort by extra metric', () => {
+  Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', {
+    value: true,
+  });
+  expect(
+    sortOperator(
+      {
+        ...formData,
+        x_axis_sort: 'my_limit_metric',
+        x_axis_sort_asc: true,
+        x_axis: 'Categorical Column',
+        groupby: [],
+        timeseries_limit_metric: 'my_limit_metric',
+      },
+      queryObject,
+    ),
+  ).toEqual({
+    operation: 'sort',
+    options: {
+      by: 'my_limit_metric',
+      ascending: true,
+    },
+  });
+});

superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/extractExtraMetrics.test.ts

@@ -0,0 +1,94 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { QueryFormData, QueryFormMetric } from '@superset-ui/core';
+import { extractExtraMetrics } from '@superset-ui/chart-controls';
+
+const baseFormData: QueryFormData = {
+  datasource: 'dummy',
+  viz_type: 'table',
+  metrics: ['a', 'b'],
+  columns: ['foo', 'bar'],
+  limit: 100,
+  metrics_b: ['c', 'd'],
+  columns_b: ['hello', 'world'],
+  limit_b: 200,
+};
+
+const metric: QueryFormMetric = {
+  expressionType: 'SQL',
+  sqlExpression: 'case when 1 then 1 else 2 end',
+  label: 'foo',
+};
+
+test('returns empty array if relevant controls missing', () => {
+  expect(
+    extractExtraMetrics({
+      ...baseFormData,
+    }),
+  ).toEqual([]);
+});
+
+test('returns empty array if x_axis_sort is not same as timeseries_limit_metric', () => {
+  expect(
+    extractExtraMetrics({
+      ...baseFormData,
+      timeseries_limit_metric: 'foo',
+      x_axis_sort: 'bar',
+    }),
+  ).toEqual([]);
+});
+
+test('returns correct column if sort columns match', () => {
+  expect(
+    extractExtraMetrics({
+      ...baseFormData,
+      timeseries_limit_metric: 'foo',
+      x_axis_sort: 'foo',
+    }),
+  ).toEqual(['foo']);
+});
+
+test('handles adhoc metrics correctly', () => {
+  expect(
+    extractExtraMetrics({
+      ...baseFormData,
+      timeseries_limit_metric: metric,
+      x_axis_sort: 'foo',
+    }),
+  ).toEqual([metric]);
+
+  expect(
+    extractExtraMetrics({
+      ...baseFormData,
+      timeseries_limit_metric: metric,
+      x_axis_sort: 'bar',
+    }),
+  ).toEqual([]);
+});
+
+test('returns empty array if groupby populated', () => {
+  expect(
+    extractExtraMetrics({
+      ...baseFormData,
+      groupby: ['bar'],
+      timeseries_limit_metric: 'foo',
+      x_axis_sort: 'foo',
+    }),
+  ).toEqual([]);
+});