abhinav-phi commented on code in PR #44815:
URL: https://github.com/apache/superset/pull/44815#discussion_r4143756143
##########
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) {
+ if (!this.allTotal) {
+ this.allTotal = this.getFormattedAggregator(record)(this, [], []);
+ }
+ this.allTotal.push(record);
+ }
Review Comment:
Good catch — this was a real bug in my first pass, not just a theoretical
one, and I fixed it rather than deferring it.
`cellValue.push()` overwrites `this.val`, so a slot built once from the
first metric went on to render the *last* metric's value in the *first*
metric's format. I confirmed it by reverting to the sticky version and running
the new test:
```
× multi-metric grand total ... (sales first)
Expected: "5.000 r"
Received: "$5.00"
× multi-metric grand total ... (rate first)
Expected: "$300.00"
Received: "300.000 r"
```
A rate of 5 shown as `$5.00`, and $300 shown as `300.000 r` — the same class
of defect this issue is about, just harder to spot, so fixing it here was the
right call.
The slot now tracks which formatter built it and rebuilds when a record
resolves to a different one. `getFormattedAggregator` returns a stable function
per metric, so an identity check is enough to detect the switch, and records
resolving to the same formatter still accumulate into the same slot as before:
```ts
pushAllTotal(record: PivotRecord): void {
const formatter = this.getFormattedAggregator(record);
if (!this.allTotal || formatter !== this.allTotalFormatter) {
this.allTotalFormatter = formatter;
this.allTotal = formatter(this, [], []);
}
this.allTotal.push(record);
}
```
`test.each` covers both record orders.
To be clear about what this does *not* settle: *which* metric a multi-metric
grand total belongs to is still "last one wins". That is #44725's question, and
this patch only guarantees the weaker invariant — whatever value the slot shows
is formatted as the metric it actually came from.
--
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]
