abhinav-phi commented on code in PR #44815:
URL: https://github.com/apache/superset/pull/44815#discussion_r4143762655
##########
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:
Agreed, the fixture was repeated inline. The metric-collapse test now
derives from the shared constant instead:
```ts
// Dropping the trailing grand-total record from the shared fixture is
exactly
// that case.
const pivotData = buildPivot(SINGLE_METRIC_DATA.slice(0, -1));
```
That is the same data with the explicit `rows=[]/columns=[]` record removed,
which is the whole point of that test — the grand total is then fed only by the
metric-collapse mirror. One definition now, and the difference between the two
cases is a single `.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]
