feat(plugin-chart-echarts): add more robust formatting for series (#783)

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-echarts/src/Pie/transformProps.ts

@@ -72,12 +72,12 @@ export default function transformProps(chartProps: ChartProps): EchartsProps {
   } = formData as PieChartFormData;
   const { label: metricLabel } = convertMetric(metric);
 
-  const keys = data.map(datum => extractGroupbyLabel(datum, groupby));
+  const keys = data.map(datum => extractGroupbyLabel({ datum, groupby }));
   const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
   const numberFormatter = getNumberFormatter(numberFormat);
 
   const transformedData = data.map(datum => {
-    const name = extractGroupbyLabel(datum, groupby);
+    const name = extractGroupbyLabel({ datum, groupby });
     return {
       value: datum[metricLabel],
       name,

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-echarts/src/constants.ts

@@ -0,0 +1,20 @@
+/**
+ * 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.
+ */
+// eslint-disable-next-line import/prefer-default-export
+export const NULL_STRING = '<NULL>';

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-echarts/src/utils/series.ts

@@ -17,13 +17,19 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { DataRecord, TimeseriesDataRecord } from '@superset-ui/core';
+import {
+  DataRecord,
+  DataRecordValue,
+  NumberFormatter,
+  TimeFormatter,
+  TimeseriesDataRecord,
+} from '@superset-ui/core';
+import { NULL_STRING } from '../constants';
 
 export function extractTimeseriesSeries(
   data: TimeseriesDataRecord[],
 ): echarts.EChartOption.Series[] {
   if (data.length === 0) return [];
-  // eslint-disable-next-line @typescript-eslint/no-unsafe-return
   const rows = data.map(datum => ({
     ...datum,
     __timestamp: datum.__timestamp || datum.__timestamp === 0 ? new Date(datum.__timestamp) : null,
@@ -39,7 +45,43 @@ export function extractTimeseriesSeries(
     }));
 }
 
-export function extractGroupbyLabel(datum: DataRecord, groupby: string[]): string {
-  // TODO: apply formatting to dates and numbers
-  return groupby.map(val => `${datum[val]}`).join(', ');
+export function formatSeriesName(
+  name: DataRecordValue | undefined,
+  {
+    numberFormatter,
+    timeFormatter,
+  }: {
+    numberFormatter?: NumberFormatter;
+    timeFormatter?: TimeFormatter;
+  } = {},
+): string {
+  if (name === undefined || name === null) {
+    return NULL_STRING;
+  }
+  if (typeof name === 'number') {
+    return numberFormatter ? numberFormatter(name) : name.toString();
+  }
+  if (typeof name === 'boolean') {
+    return name.toString();
+  }
+  if (name instanceof Date) {
+    return timeFormatter ? timeFormatter(name) : name.toISOString();
+  }
+  return name;
+}
+
+export function extractGroupbyLabel({
+  datum,
+  groupby,
+  numberFormatter,
+  timeFormatter,
+}: {
+  datum: DataRecord;
+  groupby: string[];
+  numberFormatter?: NumberFormatter;
+  timeFormatter?: TimeFormatter;
+}): string {
+  return groupby
+    .map(val => formatSeriesName(datum[val], { numberFormatter, timeFormatter }))
+    .join(', ');
 }

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-echarts/test/utils/series.test.ts

@@ -16,7 +16,12 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { extractGroupbyLabel, extractTimeseriesSeries } from '../../src/utils/series';
+import { getNumberFormatter, getTimeFormatter } from '@superset-ui/core';
+import {
+  extractGroupbyLabel,
+  extractTimeseriesSeries,
+  formatSeriesName,
+} from '../../src/utils/series';
 
 describe('extractTimeseriesSeries', () => {
   it('should generate a valid ECharts timeseries series object', () => {
@@ -60,20 +65,56 @@ describe('extractTimeseriesSeries', () => {
 
 describe('extractGroupbyLabel', () => {
   it('should join together multiple groupby labels', () => {
-    expect(extractGroupbyLabel({ a: 'abc', b: 'qwerty' }, ['a', 'b'])).toEqual('abc, qwerty');
+    expect(extractGroupbyLabel({ datum: { a: 'abc', b: 'qwerty' }, groupby: ['a', 'b'] })).toEqual(
+      'abc, qwerty',
+    );
   });
 
   it('should handle a single groupby', () => {
-    expect(extractGroupbyLabel({ xyz: 'qqq' }, ['xyz'])).toEqual('qqq');
+    expect(extractGroupbyLabel({ datum: { xyz: 'qqq' }, groupby: ['xyz'] })).toEqual('qqq');
   });
 
   it('should handle mixed types', () => {
     expect(
-      extractGroupbyLabel({ strcol: 'abc', intcol: 123, floatcol: 0.123 }, [
-        'strcol',
-        'intcol',
-        'floatcol',
-      ]),
-    ).toEqual('abc, 123, 0.123');
+      extractGroupbyLabel({
+        datum: { strcol: 'abc', intcol: 123, floatcol: 0.123, boolcol: true },
+        groupby: ['strcol', 'intcol', 'floatcol', 'boolcol'],
+      }),
+    ).toEqual('abc, 123, 0.123, true');
+  });
+});
+
+describe('formatSeriesName', () => {
+  const numberFormatter = getNumberFormatter();
+  const timeFormatter = getTimeFormatter();
+  it('should handle missing values properly', () => {
+    expect(formatSeriesName(undefined)).toEqual('<NULL>');
+    expect(formatSeriesName(null)).toEqual('<NULL>');
+  });
+
+  it('should handle string values properly', () => {
+    expect(formatSeriesName('abc XYZ!')).toEqual('abc XYZ!');
+  });
+
+  it('should handle boolean values properly', () => {
+    expect(formatSeriesName(true)).toEqual('true');
+  });
+
+  it('should use default formatting for numeric values without formatter', () => {
+    expect(formatSeriesName(12345678.9)).toEqual('12345678.9');
+  });
+
+  it('should use numberFormatter for numeric values when formatter is provided', () => {
+    expect(formatSeriesName(12345678.9, { numberFormatter })).toEqual('12.3M');
+  });
+
+  it('should use default formatting for for date values without formatter', () => {
+    expect(formatSeriesName(new Date('2020-09-11'))).toEqual('2020-09-11T00:00:00.000Z');
+  });
+
+  it('should use timeFormatter for date values when formatter is provided', () => {
+    expect(formatSeriesName(new Date('2020-09-11'), { timeFormatter })).toEqual(
+      '2020-09-11 00:00:00',
+    );
   });
 });