bito-code-review[bot] commented on code in PR #44815:
URL: https://github.com/apache/superset/pull/44815#discussion_r4190024982
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1130,6 +1133,24 @@ class PivotData {
return fmtAggs[groupName][String(groupValue)] || this.aggregator;
}
+ /*
+ * Push a record into the grand-total slot, creating the slot on first use.
+ *
+ * The grand total has no key of its own, so -- exactly like rowTotals and
+ * colTotals -- its aggregator is built from the first record that reaches
it,
+ * which lets it pick up that record's per-metric formatter. Building it in
the
+ * constructor from `this.aggregator` instead (as this used to) pins it to
+ * `defaultFormatter`, so a metric configured with a currency or custom d3
+ * format had its total render as a bare number while its body cells rendered
+ * as money.
+ */
+ pushAllTotal(record: PivotRecord) {
Review Comment:
<!-- Bito Reply -->
The addition of the `: void` return type to the `pushAllTotal` method is
appropriate. It ensures consistency with the existing class structure and
matches the return type annotations used in sibling methods like `sortKeys()`
and `processRecord()`.
**superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts**
```
pushAllTotal(record: PivotRecord): void {
```
##########
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 update to use `SINGLE_METRIC_DATA.slice(0, -1)` is a clean and effective
way to address the duplication while maintaining the necessary test coverage
for the metric-collapse scenario. This approach correctly isolates the
difference between the two test cases to a single line, improving
maintainability as discussed.
**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]
