fix(plugin-chart-table): always sort descending by first metric (#935)

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/query/buildQueryObject.ts

@@ -47,22 +47,24 @@ export default function buildQueryObject<T extends QueryFormData>(
   });
 
   let queryObject: QueryObject = {
-    time_range,
-    since,
-    until,
-    granularity,
+    // fallback `null` to `undefined` so they won't be sent to the backend
+    // (JSON.strinify will ignore `undefined`.)
+    time_range: time_range || undefined,
+    since: since || undefined,
+    until: until || undefined,
+    granularity: granularity || undefined,
     ...extras,
     ...extrasAndfilters,
-    annotation_layers,
     columns,
     metrics,
     orderby,
+    annotation_layers,
     row_limit: row_limit == null || Number.isNaN(numericRowLimit) ? undefined : numericRowLimit,
     row_offset: row_offset == null || Number.isNaN(numericRowOffset) ? undefined : numericRowOffset,
     timeseries_limit: limit ? Number(limit) : 0,
-    timeseries_limit_metric,
+    timeseries_limit_metric: timeseries_limit_metric || undefined,
     order_desc: typeof order_desc === 'undefined' ? true : order_desc,
-    url_params,
+    url_params: url_params || undefined,
   };
   // append and override extra form data used by native filters
   queryObject = appendExtraFormData(queryObject, append_form_data);

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/utils/ensureIsArray.ts

@@ -16,16 +16,14 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { QueryFormMetric } from '@superset-ui/core';
 
-export function extractTimeseriesLimitMetric(
-  timeSeriesLimitMetric?: QueryFormMetric[] | QueryFormMetric | null,
-): QueryFormMetric[] {
-  if (timeSeriesLimitMetric === undefined || timeSeriesLimitMetric === null) {
+/**
+ * Ensure a nullable value input is an array. Useful when consolidating
+ * input format from a select control.
+ */
+export default function ensureIsArray<T>(value?: T[] | T | null): T[] {
+  if (value === undefined || value === null) {
     return [];
   }
-  if (Array.isArray(timeSeriesLimitMetric)) {
-    return timeSeriesLimitMetric;
-  }
-  return [timeSeriesLimitMetric];
+  return Array.isArray(value) ? value : [value];
 }

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/utils/index.ts

@@ -17,6 +17,7 @@
  * under the License.
  */
 export { default as convertKeysToCamelCase } from './convertKeysToCamelCase';
+export { default as ensureIsArray } from './ensureIsArray';
 export { default as isDefined } from './isDefined';
 export { default as isRequired } from './isRequired';
 export { default as makeSingleton } from './makeSingleton';

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/query/buildQueryObject.test.ts

@@ -201,13 +201,21 @@ describe('buildQueryObject', () => {
   });
 
   it('should populate url_params', () => {
-    const urlParams = { abc: '123' };
-    query = buildQueryObject({
-      datasource: '5__table',
-      granularity_sqla: 'ds',
-      viz_type: 'table',
-      url_params: urlParams,
-    });
-    expect(query.url_params).toEqual(urlParams);
+    expect(
+      buildQueryObject({
+        datasource: '5__table',
+        granularity_sqla: 'ds',
+        viz_type: 'table',
+        url_params: { abc: '123' },
+      }).url_params,
+    ).toEqual({ abc: '123' });
+    expect(
+      buildQueryObject({
+        datasource: '5__table',
+        granularity_sqla: 'ds',
+        viz_type: 'table',
+        url_params: (null as unknown) as undefined,
+      }).url_params,
+    ).toBeUndefined();
   });
 });

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/utils/ensureIsArray.test.ts

@@ -16,18 +16,15 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { extractTimeseriesLimitMetric } from '../src/utils/extractOrderby';
+import { ensureIsArray } from '../../src';
 
-describe('utils', () => {
-  it('should add post-processing in aggregate mode', () => {
-    expect(extractTimeseriesLimitMetric(undefined)).toEqual([]);
-    expect(extractTimeseriesLimitMetric(null)).toEqual([]);
-    expect(extractTimeseriesLimitMetric([])).toEqual([]);
-    expect(extractTimeseriesLimitMetric('my_metric')).toEqual(['my_metric']);
-    expect(extractTimeseriesLimitMetric(['my_metric'])).toEqual(['my_metric']);
-    expect(extractTimeseriesLimitMetric(['my_metric_1', 'my_metric_2'])).toEqual([
-      'my_metric_1',
-      'my_metric_2',
-    ]);
+describe('ensureIsArray', () => {
+  it('handle inputs correctly', () => {
+    expect(ensureIsArray(undefined)).toEqual([]);
+    expect(ensureIsArray(null)).toEqual([]);
+    expect(ensureIsArray([])).toEqual([]);
+    expect(ensureIsArray('my_metric')).toEqual(['my_metric']);
+    expect(ensureIsArray(['my_metric'])).toEqual(['my_metric']);
+    expect(ensureIsArray(['my_metric_1', 'my_metric_2'])).toEqual(['my_metric_1', 'my_metric_2']);
   });
 });

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-table/src/buildQuery.ts

@@ -16,29 +16,48 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { buildQueryContext, getMetricLabel, QueryMode, removeDuplicates } from '@superset-ui/core';
+import {
+  buildQueryContext,
+  getMetricLabel,
+  QueryMode,
+  removeDuplicates,
+  ensureIsArray,
+} from '@superset-ui/core';
 import { PostProcessingRule } from '@superset-ui/core/src/query/types/PostProcessing';
 import { TableChartFormData } from './types';
-import { extractTimeseriesLimitMetric } from './utils/extractOrderby';
+
+/**
+ * Infer query mode from form data. If `all_columns` is set, then raw records mode,
+ * otherwise defaults to aggregation mode.
+ *
+ * The same logic is used in `controlPanel` with control values as well.
+ */
+export function getQueryMode(formData: TableChartFormData) {
+  const { query_mode: mode } = formData;
+  if (mode === QueryMode.aggregate || mode === QueryMode.raw) {
+    return mode;
+  }
+  const rawColumns = formData?.all_columns;
+  const hasRawColumns = rawColumns && rawColumns.length > 0;
+  return hasRawColumns ? QueryMode.raw : QueryMode.aggregate;
+}
 
 export default function buildQuery(formData: TableChartFormData) {
-  const { percent_metrics: percentMetrics, order_desc: orderDesc = null } = formData;
-  let { query_mode: queryMode } = formData;
-  const timeseriesLimitMetric = extractTimeseriesLimitMetric(formData.timeseries_limit_metric);
+  const { percent_metrics: percentMetrics, order_desc: orderDesc = false } = formData;
+  const queryMode = getQueryMode(formData);
+  const sortByMetric = ensureIsArray(formData.timeseries_limit_metric)[0];
   return buildQueryContext(formData, baseQueryObject => {
     let { metrics, orderby } = baseQueryObject;
-    if (queryMode === undefined && metrics.length > 0) {
-      queryMode = QueryMode.aggregate;
-    }
     let postProcessing: PostProcessingRule[] = [];
 
     if (queryMode === QueryMode.aggregate) {
       // orverride orderby with timeseries metric when in aggregation mode
-      if (timeseriesLimitMetric.length > 0 && orderDesc != null) {
-        orderby = [[timeseriesLimitMetric[0], !orderDesc]];
-      } else if (timeseriesLimitMetric.length === 0 && metrics?.length > 0 && orderDesc != null) {
-        // default to ordering by first metric when no sort order has been specified
-        orderby = [[metrics[0], !orderDesc]];
+      if (sortByMetric) {
+        orderby = [[sortByMetric, !orderDesc]];
+      } else if (metrics?.length > 0) {
+        // default to ordering by first metric in descending order
+        // when no "sort by" metric is set (regargless if "SORT DESC" is set to true)
+        orderby = [[metrics[0], false]];
       }
       // add postprocessing for percent metrics only when in aggregation mode
       if (percentMetrics && percentMetrics.length > 0) {

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-table/src/controlPanel.tsx

@@ -24,6 +24,7 @@ import {
   addLocaleData,
   smartDateFormatter,
   QueryMode,
+  QueryFormColumn,
 } from '@superset-ui/core';
 import {
   formatSelectOptions,
@@ -60,8 +61,8 @@ function getQueryMode(controls: ControlStateMapping): QueryMode {
   if (mode === QueryMode.aggregate || mode === QueryMode.raw) {
     return mode as QueryMode;
   }
-  const rawColumns = controls?.all_columns?.value;
-  const hasRawColumns = rawColumns && (rawColumns as string[])?.length > 0;
+  const rawColumns: QueryFormColumn[] | undefined = controls?.all_columns?.value;
+  const hasRawColumns = rawColumns && rawColumns.length > 0;
   return hasRawColumns ? QueryMode.raw : QueryMode.aggregate;
 }