You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kgabryje (via GitHub)" <gi...@apache.org> on 2023/06/26 15:47:15 UTC
[GitHub] [superset] kgabryje opened a new pull request, #24517: feat: Implement currencies formatter for saved metrics
kgabryje opened a new pull request, #24517:
URL: https://github.com/apache/superset/pull/24517
### SUMMARY
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)
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
<!--- Skip this if not applicable -->
https://github.com/apache/superset/assets/15073128/281ec0b5-3506-4dbb-a709-50080fa9906b
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24517:
URL: https://github.com/apache/superset/pull/24517#discussion_r1243567703
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -141,7 +148,17 @@ export default function transformProps(
};
const refs: Refs = {};
const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
- const numberFormatter = getNumberFormatter(numberFormat);
+ const numberFormatter =
+ getCustomFormatter(
+ buildCustomFormatters(
+ metric,
+ currencyFormats,
+ columnFormats,
+ numberFormat,
+ ),
+ metric,
+ ) ?? getNumberFormatter(numberFormat);
Review Comment:
💯
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts:
##########
@@ -0,0 +1,74 @@
+import {
+ Currency,
+ CurrencyFormatter,
+ ensureIsArray,
+ getNumberFormatter,
+ isDefined,
+ isSavedMetric,
+ NumberFormatter,
+ QueryFormMetric,
+} from '@superset-ui/core';
+
+export const buildCustomFormatters = (
+ metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+ currencyFormats: Record<string, Currency>,
+ columnFormats: Record<string, string>,
+ d3Format: string | undefined,
+ labelMap: Record<string, string[]> = {},
+ seriesNames: string[] = [],
+) => {
Review Comment:
Good point
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #24517:
URL: https://github.com/apache/superset/pull/24517#issuecomment-1624035290
Ok, perfect!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24517:
URL: https://github.com/apache/superset/pull/24517#issuecomment-1607844218
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24517](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (69855d8) into [master](https://app.codecov.io/gh/apache/superset/commit/ba3bdc077cfe1a017105332dc688b9d7faef6795?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ba3bdc0) will **decrease** coverage by `11.00%`.
> The diff coverage is `85.00%`.
> :exclamation: Current head 69855d8 differs from pull request most recent head 6a6e36d. Consider uploading reports for the commit 6a6e36d to get more accurate results
```diff
@@ Coverage Diff @@
## master #24517 +/- ##
===========================================
- Coverage 69.06% 58.07% -11.00%
===========================================
Files 1901 1901
Lines 74019 74017 -2
Branches 8116 8110 -6
===========================================
- Hits 51121 42984 -8137
- Misses 20787 28923 +8136
+ Partials 2111 2110 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.81% <85.00%> (+0.01%)` | :arrow_up: |
| python | `60.61% <85.00%> (-22.88%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `54.67% <85.00%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...d/packages/superset-ui-chart-controls/src/types.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...ges/superset-ui-core/src/query/types/Datasource.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvRGF0YXNvdXJjZS50cw==) | `100.00% <ø> (ø)` | |
| [...ackages/superset-ui-core/src/query/types/Metric.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvTWV0cmljLnRz) | `100.00% <ø> (ø)` | |
| [...i-core/src/time-format/utils/normalizeTimestamp.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdGltZS1mb3JtYXQvdXRpbHMvbm9ybWFsaXplVGltZXN0YW1wLnRz) | `100.00% <ø> (ø)` | |
| [...rts/src/BigNumber/BigNumberTotal/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlclRvdGFsL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <ø> (ø)` | |
| [...BigNumber/BigNumberWithTrendline/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvdHJhbnNmb3JtUHJvcHMudHM=) | `49.25% <ø> (+1.42%)` | :arrow_up: |
| [.../plugin-chart-echarts/src/Funnel/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvRnVubmVsL3RyYW5zZm9ybVByb3BzLnRz) | `72.34% <ø> (-0.58%)` | :arrow_down: |
| [...s/plugin-chart-echarts/src/Gauge/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvR2F1Z2UvdHJhbnNmb3JtUHJvcHMudHM=) | `77.21% <ø> (-0.57%)` | :arrow_down: |
| [...ins/plugin-chart-echarts/src/Pie/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvUGllL3RyYW5zZm9ybVByb3BzLnRz) | `55.07% <ø> (-0.65%)` | :arrow_down: |
| [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | `56.03% <ø> (-0.75%)` | :arrow_down: |
| ... and [23 more](https://app.codecov.io/gh/apache/superset/pull/24517?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [293 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24517/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24517:
URL: https://github.com/apache/superset/pull/24517#discussion_r1243162274
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts:
##########
@@ -0,0 +1,74 @@
+import {
+ Currency,
+ CurrencyFormatter,
+ ensureIsArray,
+ getNumberFormatter,
+ isDefined,
+ isSavedMetric,
+ NumberFormatter,
+ QueryFormMetric,
+} from '@superset-ui/core';
+
+export const buildCustomFormatters = (
+ metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+ currencyFormats: Record<string, Currency>,
+ columnFormats: Record<string, string>,
+ d3Format: string | undefined,
+ labelMap: Record<string, string[]> = {},
+ seriesNames: string[] = [],
+) => {
+ const metricsArray = ensureIsArray(metrics);
+ if (metricsArray.length === 1) {
+ const metric = metricsArray[0];
+ if (isSavedMetric(metric)) {
+ return {
+ [metric]: currencyFormats[metric]
+ ? new CurrencyFormatter({
+ d3Format: columnFormats[metric] ?? d3Format,
+ currency: currencyFormats[metric],
+ })
+ : getNumberFormatter(columnFormats[metric] ?? d3Format),
+ };
+ }
+ return {};
+ }
+ return seriesNames.reduce((acc, name) => {
+ if (!isDefined(name)) {
+ return acc;
+ }
+
+ const metricName = labelMap[name]?.[0];
+ const isSaved =
+ metricName &&
+ // string equality checks if it is a saved metric
+ metricsArray?.some(metric => metric === metricName);
+ const actualD3Format = isSaved
+ ? columnFormats[metricName] ?? d3Format
+ : d3Format;
+ if (isSaved && currencyFormats[metricName]) {
+ return {
+ ...acc,
+ [name]: new CurrencyFormatter({
+ d3Format: actualD3Format,
+ currency: currencyFormats[metricName],
+ }),
+ };
+ }
+ return {
+ ...acc,
+ [name]: getNumberFormatter(actualD3Format),
+ };
+ }, {});
+};
+
+export const getCustomFormatter = (
+ customFormatters: Record<string, NumberFormatter | CurrencyFormatter>,
+ metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+ key?: string,
+) => {
Review Comment:
same here
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -141,7 +148,17 @@ export default function transformProps(
};
const refs: Refs = {};
const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
- const numberFormatter = getNumberFormatter(numberFormat);
+ const numberFormatter =
+ getCustomFormatter(
+ buildCustomFormatters(
+ metric,
+ currencyFormats,
+ columnFormats,
+ numberFormat,
+ ),
+ metric,
+ ) ?? getNumberFormatter(numberFormat);
Review Comment:
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
```ts
const numberFormatter =
getFormatter(
metric,
currencyFormats,
columnFormats,
numberFormat,
);
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts:
##########
@@ -0,0 +1,74 @@
+import {
+ Currency,
+ CurrencyFormatter,
+ ensureIsArray,
+ getNumberFormatter,
+ isDefined,
+ isSavedMetric,
+ NumberFormatter,
+ QueryFormMetric,
+} from '@superset-ui/core';
+
+export const buildCustomFormatters = (
+ metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+ currencyFormats: Record<string, Currency>,
+ columnFormats: Record<string, string>,
+ d3Format: string | undefined,
+ labelMap: Record<string, string[]> = {},
+ seriesNames: string[] = [],
+) => {
Review Comment:
It seems the return type is `{ [key: string]: CurrencyFormatter | NumberFormatter }`, could we add that explicitly here to make it easier to read?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje merged pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje merged PR #24517:
URL: https://github.com/apache/superset/pull/24517
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24517:
URL: https://github.com/apache/superset/pull/24517#discussion_r1244022658
##########
superset-frontend/packages/superset-ui-core/test/currency-format/CurrencyFormatter.test.ts:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import {
+ CurrencyFormatter,
+ getCurrencySymbol,
+ NumberFormats,
+} from '@superset-ui/core';
+
+it('getCurrencySymbol', () => {
+ expect(
+ getCurrencySymbol({ symbol: 'PLN', symbolPosition: 'prefix' }),
+ ).toEqual('PLN');
+ expect(
+ getCurrencySymbol({ symbol: 'USD', symbolPosition: 'prefix' }),
+ ).toEqual('$');
+
+ expect(() =>
+ getCurrencySymbol({ symbol: 'INVALID_CODE', symbolPosition: 'prefix' }),
+ ).toThrow(RangeError);
+});
+
+it('CurrencyFormatter object fields', () => {
+ const defaultCurrencyFormatter = new CurrencyFormatter({
+ currency: { symbol: 'USD', symbolPosition: 'prefix' },
+ });
+ expect(defaultCurrencyFormatter.d3Format).toEqual(NumberFormats.SMART_NUMBER);
+ expect(defaultCurrencyFormatter.locale).toEqual('en-US');
+ expect(defaultCurrencyFormatter.currency).toEqual({
+ symbol: 'USD',
+ symbolPosition: 'prefix',
+ });
+
+ const currencyFormatter = new CurrencyFormatter({
+ currency: { symbol: 'PLN', symbolPosition: 'suffix' },
+ locale: 'pl-PL',
+ d3Format: ',.1f',
+ });
+ expect(currencyFormatter.d3Format).toEqual(',.1f');
+ expect(currencyFormatter.locale).toEqual('pl-PL');
+ expect(currencyFormatter.currency).toEqual({
+ symbol: 'PLN',
+ symbolPosition: 'suffix',
+ });
+});
+
+it('CurrencyFormatter:hasValidCurrency', () => {
+ const currencyFormatter = new CurrencyFormatter({
+ currency: { symbol: 'USD', symbolPosition: 'prefix' },
+ });
+ expect(currencyFormatter.hasValidCurrency()).toBeTruthy();
+
+ const currencyFormatterWithoutPosition = new CurrencyFormatter({
+ // @ts-ignore
+ currency: { symbol: 'USD' },
+ });
+ expect(currencyFormatterWithoutPosition.hasValidCurrency()).toBeTruthy();
Review Comment:
...and if we made that change up there, this would be
```suggestion
expect(currencyFormatterWithoutPosition.hasValidCurrency()).toBe(true);
```
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ExtensibleFunction } from '../models';
+import { getNumberFormatter, NumberFormats } from '../number-format';
+import { Currency } from '../query';
+
+interface CurrencyFormatterConfig {
+ d3Format?: string;
+ currency: Currency;
+ locale?: string;
+}
+
+interface CurrencyFormatter {
+ (value: number | null | undefined): string;
+}
+
+export const getCurrencySymbol = (currency: Currency) =>
+ new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: currency.symbol,
+ })
+ .formatToParts(1)
+ .find(x => x.type === 'currency')?.value;
+
+class CurrencyFormatter extends ExtensibleFunction {
+ d3Format: string;
+
+ locale: string;
+
+ currency: Currency;
+
+ constructor(config: CurrencyFormatterConfig) {
+ super((value: number) => this.format(value));
+ this.d3Format = config.d3Format || NumberFormats.SMART_NUMBER;
+ this.currency = config.currency;
+ this.locale = config.locale || 'en-US';
+ }
+
+ hasValidCurrency() {
+ return this.currency?.symbol;
+ }
Review Comment:
I'm going on about return types again 😆 To be precise, shouldn't this be
```ts
hasValidCurrency(): boolean {
return Boolean(this.currency?.symbol);
}
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts:
##########
@@ -0,0 +1,96 @@
+import {
+ Currency,
+ CurrencyFormatter,
+ ensureIsArray,
+ getNumberFormatter,
+ isDefined,
+ isSavedMetric,
+ QueryFormMetric,
+ ValueFormatter,
+} from '@superset-ui/core';
+
+export const buildCustomFormatters = (
+ metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+ currencyFormats: Record<string, Currency>,
+ columnFormats: Record<string, string>,
+ d3Format: string | undefined,
+ labelMap: Record<string, string[]> = {},
+ seriesNames: string[] = [],
+) => {
+ const metricsArray = ensureIsArray(metrics);
+ if (metricsArray.length === 1) {
+ const metric = metricsArray[0];
+ if (isSavedMetric(metric)) {
+ return {
+ [metric]: currencyFormats[metric]
+ ? new CurrencyFormatter({
+ d3Format: columnFormats[metric] ?? d3Format,
+ currency: currencyFormats[metric],
+ })
+ : getNumberFormatter(columnFormats[metric] ?? d3Format),
+ };
+ }
+ return {};
+ }
+ return seriesNames.reduce((acc, name) => {
+ if (!isDefined(name)) {
+ return acc;
+ }
+
+ const metricName = labelMap[name]?.[0];
+ const isSaved =
+ metricName &&
+ // string equality checks if it is a saved metric
+ metricsArray?.some(metric => metric === metricName);
+ const actualD3Format = isSaved
+ ? columnFormats[metricName] ?? d3Format
+ : d3Format;
+ if (isSaved && currencyFormats[metricName]) {
+ return {
+ ...acc,
+ [name]: new CurrencyFormatter({
+ d3Format: actualD3Format,
+ currency: currencyFormats[metricName],
+ }),
+ };
+ }
+ return {
+ ...acc,
+ [name]: getNumberFormatter(actualD3Format),
+ };
+ }, {});
+};
+
+export const getCustomFormatter = (
+ customFormatters: Record<string, ValueFormatter>,
+ metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+ key?: string,
+) => {
+ const metricsArray = ensureIsArray(metrics);
+ if (metricsArray.length === 1 && isSavedMetric(metricsArray[0])) {
+ return customFormatters[metricsArray[0]];
+ }
+ return key ? customFormatters[key] : undefined;
+};
+
+export const getFormatter = (
Review Comment:
For the sake of consistency, should this be `getValueFormatter`?
##########
superset-frontend/src/explore/actions/hydrateExplore.ts:
##########
@@ -96,6 +96,8 @@ export const hydrateExplore =
if (dashboardId) {
initialFormData.dashboardId = dashboardId;
}
+
+ // const initialDatasource = dataset;
Review Comment:
I assume this is redundant
```suggestion
// const initialDatasource = dataset;
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24517:
URL: https://github.com/apache/superset/pull/24517#issuecomment-1611916534
Ephemeral environment shutdown and build artifacts deleted.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24517:
URL: https://github.com/apache/superset/pull/24517#discussion_r1244897247
##########
superset-frontend/src/explore/actions/hydrateExplore.ts:
##########
@@ -96,6 +96,8 @@ export const hydrateExplore =
if (dashboardId) {
initialFormData.dashboardId = dashboardId;
}
+
+ // const initialDatasource = dataset;
Review Comment:
🤦
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #24517:
URL: https://github.com/apache/superset/pull/24517#issuecomment-1611015977
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-code-scanning[bot] commented on a diff in pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #24517:
URL: https://github.com/apache/superset/pull/24517#discussion_r1242409199
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ExtensibleFunction } from '../models';
+import { getNumberFormatter, NumberFormats } from '../number-format';
+import { Currency } from '../query';
+
+interface CurrencyFormatterConfig {
+ d3Format?: string;
+ currency: Currency;
+ locale?: string;
+}
+
+interface CurrencyFormatter {
+ (value: number | null | undefined): string;
+}
+
+export const getCurrencySymbol = (currency: Currency) =>
+ new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: currency.symbol,
+ })
+ .formatToParts(1)
+ .find(x => x.type === 'currency')?.value;
+
+class CurrencyFormatter extends ExtensibleFunction {
+ d3Format: string;
+
+ locale: string;
+
+ currency: Currency;
+
+ constructor(config: CurrencyFormatterConfig) {
+ super((value: number) => this.format(value));
+ this.d3Format = config.d3Format || NumberFormats.SMART_NUMBER;
+ this.currency = config.currency;
+ this.locale = config.locale || 'en-US';
+ }
+
+ hasValidCurrency() {
+ return this.currency?.symbol;
+ }
+
+ getNormalizedD3Format() {
+ return this.d3Format.replace('$', '').replace('%', '');
Review Comment:
## Incomplete string escaping or encoding
This replaces only the first occurrence of '$'.
[Show more details](https://github.com/apache/superset/security/code-scanning/1211)
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ExtensibleFunction } from '../models';
+import { getNumberFormatter, NumberFormats } from '../number-format';
+import { Currency } from '../query';
+
+interface CurrencyFormatterConfig {
+ d3Format?: string;
+ currency: Currency;
+ locale?: string;
+}
+
+interface CurrencyFormatter {
+ (value: number | null | undefined): string;
+}
+
+export const getCurrencySymbol = (currency: Currency) =>
+ new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: currency.symbol,
+ })
+ .formatToParts(1)
+ .find(x => x.type === 'currency')?.value;
+
+class CurrencyFormatter extends ExtensibleFunction {
+ d3Format: string;
+
+ locale: string;
+
+ currency: Currency;
+
+ constructor(config: CurrencyFormatterConfig) {
+ super((value: number) => this.format(value));
+ this.d3Format = config.d3Format || NumberFormats.SMART_NUMBER;
+ this.currency = config.currency;
+ this.locale = config.locale || 'en-US';
+ }
+
+ hasValidCurrency() {
+ return this.currency?.symbol;
+ }
+
+ getNormalizedD3Format() {
+ return this.d3Format.replace('$', '').replace('%', '');
Review Comment:
## Incomplete string escaping or encoding
This replaces only the first occurrence of '%'.
[Show more details](https://github.com/apache/superset/security/code-scanning/1210)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24517:
URL: https://github.com/apache/superset/pull/24517#issuecomment-1611018332
@kgabryje Ephemeral environment spinning up at http://18.237.178.241:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24517:
URL: https://github.com/apache/superset/pull/24517#issuecomment-1623535819
> @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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #24517: feat: Implement currencies formatter for saved metrics
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #24517:
URL: https://github.com/apache/superset/pull/24517#issuecomment-1622674042
@kgabryje given that this could be considered a breaking change, wdyt about trying to cherry this PR into 3.0, along with https://github.com/apache/superset/pull/24594? cc @michael-s-molina and @rusackas
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org