bito-code-review[bot] commented on code in PR #44815:
URL: https://github.com/apache/superset/pull/44815#discussion_r4140519935
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated test data constant</b></div>
<div id="fix">
The same constant `SINGLE_METRIC_DATA` is defined twice in this test file,
leading to code duplication. Please consolidate to a single definition to avoid
maintenance issues. The duplicate block is at lines 151-174, while the original
is at lines 110-133.
</div>
</div>
<small><i>Code Review Run #f6fa46</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing return type</b></div>
<div id="fix">
The new `pushAllTotal` method omits an explicit return type, while sibling
methods in this class annotate theirs (`sortKeys(): void` at line 1177,
`processRecord(...): void` at line 1247). BITO.md adaptive rule 7819 requires
explicit return type hints on all methods. Add `: void` for consistency and
static-check coverage.
</div>
</div>
<small><i>Code Review Run #f6fa46</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
