feat: Implement currencies formatter for saved metrics #24517

kgabryje · 2023-06-26T15:47:13Z

SUMMARY

WARNING: This PR might be considered a breaking change. Currently the d3formats defined in dataset editor for saved metrics worked only in Table and Pivot Table charts. Now they will also work in viz types listed below and they take priority over d3 formats defined in control panel. That means that charts that use metrics that have custom formats defined on dataset level might change their displayed number formats.

Enables users to set a currency formatter for saved metrics in Datasource edit modal.

Currently supported currencies are: "USD", "EUR", "GBP", "INR", "MXN", "JPY", "CNY" (+ user can manually type other currency codes in the currency picker). More currency codes can be added in config.py by modifying array CURRENCIES.

Currently supported charts are: Table, Pivot Table, Big Number (+ with trendline), (Time)Series Echarts, Pie, Gauge, Funnel, Treemap.

The number is formatted using the d3 format specified either for saved metric in datasource editor (takes priority) or in control panel. If user picks a d3 formatter that adds % or currency signs, the special signs are ignored.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2023-06-26.at.17.45.13.mov

TESTING INSTRUCTIONS

If you're running this PR locally, run the migration first
Open 1 of the supported charts
Open datasource editor, add/edit a saved metric and set a currency
Use the metric with currency in the chart
Verify that values are formatted with currency sign

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided:
  Current: 0.13 s
  10+: 0.12 s
  100+: 0.12 s
  1000+: 0.13 s
Introduces new feature or API
Removes existing feature or API

CC @yousoph

superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts

codecov · 2023-06-26T16:42:29Z

Codecov Report

Merging #24517 (adc04ee) into master (a846e8a) will increase coverage by 0.02%.
The diff coverage is 71.54%.

❗ Current head adc04ee differs from pull request most recent head d59b9de. Consider uploading reports for the commit d59b9de to get more accurate results

@@            Coverage Diff             @@
##           master   #24517      +/-   ##
==========================================
+ Coverage   58.22%   58.24%   +0.02%     
==========================================
  Files        1903     1906       +3     
  Lines       74026    74129     +103     
  Branches     8118     8155      +37     
==========================================
+ Hits        43100    43178      +78     
- Misses      28815    28832      +17     
- Partials     2111     2119       +8

Flag	Coverage Δ
hive	`53.90% <85.00%> (+0.01%)`	⬆️
javascript	`55.78% <68.93%> (+0.03%)`	⬆️
presto	`53.80% <85.00%> (+0.01%)`	⬆️
python	`60.91% <85.00%> (+0.01%)`	⬆️
unit	`54.66% <85.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ackages/superset-ui-chart-controls/src/fixtures.ts	`100.00% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...ges/superset-ui-core/src/query/types/Datasource.ts	`100.00% <ø> (ø)`
...ackages/superset-ui-core/src/query/types/Metric.ts	`100.00% <ø> (ø)`
...rts/src/BigNumber/BigNumberTotal/transformProps.ts	`0.00% <0.00%> (ø)`
...lugin-chart-echarts/src/Timeseries/transformers.ts	`56.20% <ø> (ø)`
...plugins/plugin-chart-echarts/src/utils/forecast.ts	`93.22% <ø> (ø)`
...d/plugins/plugin-chart-echarts/src/utils/series.ts	`85.48% <ø> (ø)`
...s/plugin-chart-pivot-table/src/PivotTableChart.tsx	`0.00% <0.00%> (ø)`
...gin-chart-pivot-table/src/plugin/transformProps.ts	`42.30% <0.00%> (ø)`
... and 29 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

villebro

First pass comments. Really neat feature, I think this must be in the top 10 list of feature requests of all time, thanks for working on this! 🙂

villebro · 2023-06-27T05:33:25Z

superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts

+  d3Format: string | undefined,
+  labelMap: Record<string, string[]> = {},
+  seriesNames: string[] = [],
+) => {


It seems the return type is { [key: string]: CurrencyFormatter | NumberFormatter }, could we add that explicitly here to make it easier to read?

villebro · 2023-06-27T05:33:42Z

superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts

+  customFormatters: Record<string, NumberFormatter | CurrencyFormatter>,
+  metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+  key?: string,
+) => {


villebro · 2023-06-27T05:37:09Z

superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts

+    getCustomFormatter(
+      buildCustomFormatters(
+        metric,
+        currencyFormats,
+        columnFormats,
+        numberFormat,
+      ),
+      metric,
+    ) ?? getNumberFormatter(numberFormat);


At the risk of over-abstraction, would it make sense to have yet one more helper that simplifies this call chain? This code block seems to be repeated a few times so abstracting it would make it slightly more DRY and less heavy on the eyes. Something like

const numberFormatter = getFormatter( metric, currencyFormats, columnFormats, numberFormat, );

villebro

LGTM with some optional comments

villebro · 2023-06-27T16:17:28Z

superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts

+  hasValidCurrency() {
+    return this.currency?.symbol;
+  }


I'm going on about return types again 😆 To be precise, shouldn't this be

hasValidCurrency(): boolean { return Boolean(this.currency?.symbol); }

villebro · 2023-06-27T16:21:58Z

superset-frontend/packages/superset-ui-core/test/currency-format/CurrencyFormatter.test.ts

+    // @ts-ignore
+    currency: { symbol: 'USD' },
+  });
+  expect(currencyFormatterWithoutPosition.hasValidCurrency()).toBeTruthy();


...and if we made that change up there, this would be

Suggested change

expect(currencyFormatterWithoutPosition.hasValidCurrency()).toBeTruthy();

expect(currencyFormatterWithoutPosition.hasValidCurrency()).toBe(true);

villebro · 2023-06-27T16:24:11Z

superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts

+  return key ? customFormatters[key] : undefined;
+};
+
+export const getFormatter = (


For the sake of consistency, should this be getValueFormatter?

villebro · 2023-06-27T16:32:21Z

superset-frontend/src/explore/actions/hydrateExplore.ts

+
+    // const initialDatasource = dataset;


I assume this is redundant

Suggested change

// const initialDatasource = dataset;

// const initialDatasource = dataset;

kgabryje · 2023-06-28T08:47:30Z

/testenv up

github-actions · 2023-06-28T08:49:17Z

@kgabryje Ephemeral environment spinning up at http://18.237.178.241:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro

Restamping, looks great!

github-actions · 2023-06-28T18:52:13Z

Ephemeral environment shutdown and build artifacts deleted.

eschutho · 2023-07-05T23:27:06Z

@kgabryje given that this could be considered a breaking change, wdyt about trying to cherry this PR into 3.0, along with #24594? cc @michael-s-molina and @rusackas

michael-s-molina · 2023-07-06T11:41:56Z

@kgabryje given that this could be considered a breaking change, wdyt about trying to cherry this PR into 3.0, along with #24594? cc @michael-s-molina and @rusackas

@eschutho This is already included in 3.0

eschutho · 2023-07-06T17:17:33Z

Ok, perfect!

kgabryje requested a review from a team as a code owner June 26, 2023 15:47

pull-request-size bot added the size/XL label Jun 26, 2023

kgabryje force-pushed the feat/metrics-currencies branch from 46f4905 to 57b9590 Compare June 26, 2023 15:48

github-advanced-security bot found potential problems Jun 26, 2023

View reviewed changes

superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts Fixed Show fixed Hide fixed

superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts Fixed Show fixed Hide fixed

villebro reviewed Jun 27, 2023

View reviewed changes

kgabryje requested review from eschutho and geido June 27, 2023 15:06

villebro approved these changes Jun 27, 2023

View reviewed changes

villebro approved these changes Jun 28, 2023

View reviewed changes

kgabryje added the risk:breaking-change Issues or PRs that will introduce breaking changes label Jun 28, 2023

kgabryje added 12 commits June 28, 2023 14:11

feat: Implement currencies formatter for saved metrics

03fccbb

fix lint

4082c65

fix migration

20f29b5

fix test

4cc48c1

fix python test

3fb2b7b

Address comments

affafe7

Add UTs

4dd5448

More UTs

1f06442

UTs for BigNumber and Table

bb90d62

Address comments

67bada5

Add error validation

2bee58b

fix timeseries

a40f28e

kgabryje force-pushed the feat/metrics-currencies branch from 346032c to a40f28e Compare June 28, 2023 12:39

kgabryje added 2 commits June 28, 2023 16:29

fix

f869484

fix verbose labels

d59b9de

kgabryje merged commit 83ff4cd into apache:master Jun 28, 2023
29 checks passed

kgabryje mentioned this pull request Jul 5, 2023

feat: Implement support for currencies in more charts #24594

Merged

9 tasks

kgabryje mentioned this pull request Jul 17, 2023

feat: Add currencies controls in control panels #24718

Merged

9 tasks

yousoph mentioned this pull request Jul 26, 2023

Chart d3-format currency configuration #24274

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement currencies formatter for saved metrics #24517

feat: Implement currencies formatter for saved metrics #24517

kgabryje commented Jun 26, 2023 •

edited

codecov bot commented Jun 26, 2023 •

edited

villebro left a comment

villebro Jun 27, 2023

kgabryje Jun 27, 2023

villebro Jun 27, 2023

villebro Jun 27, 2023

kgabryje Jun 27, 2023

villebro left a comment

villebro Jun 27, 2023

villebro Jun 27, 2023

villebro Jun 27, 2023

villebro Jun 27, 2023

kgabryje Jun 28, 2023

kgabryje commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

villebro left a comment

github-actions bot commented Jun 28, 2023

eschutho commented Jul 5, 2023

michael-s-molina commented Jul 6, 2023

eschutho commented Jul 6, 2023

	expect(currencyFormatterWithoutPosition.hasValidCurrency()).toBeTruthy();
	expect(currencyFormatterWithoutPosition.hasValidCurrency()).toBe(true);


	// const initialDatasource = dataset;

	// const initialDatasource = dataset;

feat: Implement currencies formatter for saved metrics #24517

feat: Implement currencies formatter for saved metrics #24517

Conversation

kgabryje commented Jun 26, 2023 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jun 26, 2023 • edited

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

villebro left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 28, 2023

eschutho commented Jul 5, 2023

michael-s-molina commented Jul 6, 2023

eschutho commented Jul 6, 2023

kgabryje commented Jun 26, 2023 •

edited

codecov bot commented Jun 26, 2023 •

edited