bito-code-review[bot] commented on code in PR #44815:
URL: https://github.com/apache/superset/pull/44815#discussion_r4143764039
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts:
##########
@@ -59,3 +59,182 @@ test('Minimum and Maximum still compute extremes regardless
of order', () => {
expect(aggregate('Minimum', records)).toBe(1);
expect(aggregate('Maximum', records)).toBe(5);
});
+
+/*
+ * #44724 -- per-metric formatters must reach the aggregation (total) cells
too.
+ *
+ * A metric can be configured with its own value format (currency, d3 format,
+ * decimal precision). `PivotData` receives those as `customFormatters`, keyed
by
+ * the metric pseudo-dimension and then by metric name, and resolves them
through
+ * `getFormattedAggregator`. Every slot must go through that resolution,
otherwise
+ * a total silently renders with the chart-level `defaultFormatter` instead --
so
+ * a $300 total shows up as a bare "300.00".
+ *
+ * Two metrics with deliberately different formatters make a wrong pick
obvious:
+ * the assertions below read correctly only when each cell uses its own
metric's
+ * formatter rather than the other metric's or the default.
+ */
+const currencyFmt = (x: number) => `$${x.toFixed(2)}`;
+const rateFmt = (x: number) => `${x.toFixed(3)} r`;
+const defaultFmt = (x: number) => x.toFixed(2);
+
+// Keyed exactly as PivotTableChart builds `metricFormatters`: METRIC_KEY
first,
+// then metric name.
+const perMetricFormatters = {
+ Metric: { sales: currencyFmt, rate: rateFmt },
+};
+
+const buildPivot = (data: Record<string, unknown>[]) =>
+ new PivotData(
+ {
+ data,
+ rows: ['color'],
+ cols: ['Metric'],
+ vals: ['value'],
+ defaultFormatter: defaultFmt,
+ customFormatters: perMetricFormatters,
+ } as unknown as Record<string, unknown>,
+ { colEnabled: true, rowEnabled: true },
+ );
+
+const rendered = (pivotData: PivotData, rowKey: string[], colKey: string[]) =>
{
+ const agg = pivotData.getAggregator(rowKey, colKey);
+ return agg.format(agg.value(), agg);
+};
+
+/**
+ * Single metric, so every total unambiguously belongs to `sales` and must be
+ * currency-formatted. Multi-metric grand totals are a separate question (which
+ * metric a cross-metric total belongs to is #44725, not this issue).
+ */
+const SINGLE_METRIC_DATA = [
+ {
+ color: 'blue',
+ Metric: 'sales',
+ value: 100,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ color: 'red',
+ Metric: 'sales',
+ value: 200,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ Metric: 'sales',
+ value: 300,
+ __rows: [],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ { Metric: 'sales', value: 300, __rows: [], __columns: [] },
+];
+
+test('grand total uses the metric formatter, not the default formatter', () =>
{
+ const pivotData = buildPivot(SINGLE_METRIC_DATA);
+
+ // The body cell and the column total already resolve per-metric...
+ expect(rendered(pivotData, ['blue'], ['sales'])).toBe('$100.00');
+ expect(rendered(pivotData, [], ['sales'])).toBe('$300.00');
+ // ...and the grand total is the same `sales` value, so it must match.
+ // Before the fix this was "300.00" -- defaultFormatter, the reported bug.
+ expect(rendered(pivotData, [], [])).toBe('$300.00');
+});
+
+test('metric-collapse total uses the metric formatter', () => {
+ // No rollup level produces an empty key on the metric axis, so the collapsed
+ // total is mirrored into rowTotals/allTotal from the metric-only records.
+ const pivotData = buildPivot([
Review Comment:
<!-- Bito Reply -->
The approach of deriving the test data from the shared constant using
`.slice(0, -1)` is a clean way to handle the metric-collapse case while
maintaining consistency with the original fixture. This change effectively
isolates the difference between the two test scenarios to a single line, which
improves maintainability and clarifies the intent of the test.
**superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts**
```
// Dropping the trailing grand-total record from the shared fixture is
exactly
// that case.
const pivotData = buildPivot(SINGLE_METRIC_DATA.slice(0, -1));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
