msyavuz commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2675507676
##########
superset-frontend/packages/superset-ui-core/test/currency-format/normalizeCurrency.test.ts:
##########
Review Comment:
Should we merge these tests in a single utils.test file?
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -63,18 +97,47 @@ class CurrencyFormatter extends ExtensibleFunction {
return this.d3Format.replace(/\$|%/g, '');
}
- format(value: number) {
+ format(value: number, rowData?: RowData, currencyColumn?: string): string {
const formattedValue = getNumberFormatter(this.getNormalizedD3Format())(
value,
);
- if (!this.hasValidCurrency()) {
+
+ const isAutoMode = this.currency?.symbol === AUTO_CURRENCY_SYMBOL;
+
+ if (!this.hasValidCurrency() && !isAutoMode) {
return formattedValue as string;
}
- if (this.currency.symbolPosition === 'prefix') {
- return `${getCurrencySymbol(this.currency)} ${formattedValue}`;
+ if (isAutoMode) {
+ if (rowData && currencyColumn && rowData[currencyColumn]) {
+ const rawCurrency = rowData[currencyColumn];
+ const normalizedCurrency = normalizeCurrency(rawCurrency);
+
+ if (normalizedCurrency) {
+ try {
+ const symbol = getCurrencySymbol({ symbol: normalizedCurrency });
+ if (symbol) {
+ return this.currency.symbolPosition === 'prefix'
+ ? `${symbol} ${formattedValue}`
+ : `${formattedValue} ${symbol}`;
+ }
+ } catch {
+ // Invalid currency code - fall through to neutral format
+ }
Review Comment:
Or even return the formattedValue without falling through?
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -63,18 +97,47 @@ class CurrencyFormatter extends ExtensibleFunction {
return this.d3Format.replace(/\$|%/g, '');
}
- format(value: number) {
+ format(value: number, rowData?: RowData, currencyColumn?: string): string {
const formattedValue = getNumberFormatter(this.getNormalizedD3Format())(
value,
);
- if (!this.hasValidCurrency()) {
+
+ const isAutoMode = this.currency?.symbol === AUTO_CURRENCY_SYMBOL;
+
+ if (!this.hasValidCurrency() && !isAutoMode) {
return formattedValue as string;
}
- if (this.currency.symbolPosition === 'prefix') {
- return `${getCurrencySymbol(this.currency)} ${formattedValue}`;
+ if (isAutoMode) {
+ if (rowData && currencyColumn && rowData[currencyColumn]) {
+ const rawCurrency = rowData[currencyColumn];
+ const normalizedCurrency = normalizeCurrency(rawCurrency);
+
+ if (normalizedCurrency) {
+ try {
+ const symbol = getCurrencySymbol({ symbol: normalizedCurrency });
+ if (symbol) {
+ return this.currency.symbolPosition === 'prefix'
+ ? `${symbol} ${formattedValue}`
+ : `${formattedValue} ${symbol}`;
+ }
+ } catch {
+ // Invalid currency code - fall through to neutral format
+ }
Review Comment:
Can we add a log for this. This just swallows the error now right?
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js:
##########
@@ -47,20 +47,29 @@ export default function transformProps(chartProps) {
currencyFormat,
} = formData;
const { r, g, b } = colorPicker;
- const { currencyFormats = {}, columnFormats = {} } = datasource;
+ const {
+ currencyFormats = {},
+ columnFormats = {},
+ currencyCodeColumn,
+ } = datasource;
+ const { data, detected_currency: detectedCurrency } = queriesData[0];
const formatter = getValueFormatter(
metric,
currencyFormats,
columnFormats,
yAxisFormat,
currencyFormat,
+ undefined,
Review Comment:
Are we intending to pass undefined by default for key?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/types.ts:
##########
@@ -31,7 +31,7 @@ import { ColorFormatters } from '@superset-ui/chart-controls';
import { BaseChartProps, Refs } from '../types';
export interface BigNumberDatum {
- [key: string]: number | null;
+ [key: string]: number | string | null;
Review Comment:
Why do we accept a string as well now?
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -63,18 +97,47 @@ class CurrencyFormatter extends ExtensibleFunction {
return this.d3Format.replace(/\$|%/g, '');
}
- format(value: number) {
+ format(value: number, rowData?: RowData, currencyColumn?: string): string {
const formattedValue = getNumberFormatter(this.getNormalizedD3Format())(
value,
);
- if (!this.hasValidCurrency()) {
+
+ const isAutoMode = this.currency?.symbol === AUTO_CURRENCY_SYMBOL;
+
+ if (!this.hasValidCurrency() && !isAutoMode) {
return formattedValue as string;
}
- if (this.currency.symbolPosition === 'prefix') {
- return `${getCurrencySymbol(this.currency)} ${formattedValue}`;
+ if (isAutoMode) {
+ if (rowData && currencyColumn && rowData[currencyColumn]) {
+ const rawCurrency = rowData[currencyColumn];
+ const normalizedCurrency = normalizeCurrency(rawCurrency);
+
+ if (normalizedCurrency) {
+ try {
+ const symbol = getCurrencySymbol({ symbol: normalizedCurrency });
+ if (symbol) {
+ return this.currency.symbolPosition === 'prefix'
Review Comment:
symbolPosition is string right? So if it's not 'prefix' we are treating it
as suffix?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts:
##########
@@ -92,6 +100,10 @@ export default function transformProps(
columnFormats,
metricEntry?.d3format || yAxisFormat,
currencyFormat,
+ undefined,
Review Comment:
Maybe just move the key argument to last?
##########
superset-frontend/plugins/plugin-chart-echarts/test/BigNumber/transformProps.test.ts:
##########
@@ -492,3 +492,61 @@ describe('BigNumberWithTrendline - Aggregation Tests', ()
=> {
expect(transformed.bigNumber).toStrictEqual(10);
});
});
+
+describe('BigNumberWithTrendline - AUTO Mode Currency', () => {
Review Comment:
Let's not add immediate tech debt here and use a flat testing structure
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +737,129 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('EchartsTimeseries AUTO Mode Currency', () => {
Review Comment:
ditto
--
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]
