From 86e7139245e7444e749b5e8ad1d5167b4df53ce9 Mon Sep 17 00:00:00 2001 From: Vitor Avila <96086495+Vitor-Avila@users.noreply.github.com> Date: Mon, 9 Jun 2025 22:47:14 -0300 Subject: [PATCH] fix: Dataset currency (#33682) --- .../superset-ui-chart-controls/src/types.ts | 1 + .../components/Datasource/DatasourceModal.tsx | 5 +- .../explore/actions/hydrateExplore.test.ts | 48 ++++++++++ .../src/explore/actions/hydrateExplore.ts | 9 ++ .../src/hooks/apiResources/dashboards.test.ts | 89 +++++++++++++++++++ .../src/hooks/apiResources/dashboards.ts | 17 +++- superset/datasets/schemas.py | 7 +- ...vert_metric_currencies_from_str_to_json.py | 84 +++++++++++++++++ tests/integration_tests/datasets/api_tests.py | 23 +++++ 9 files changed, 277 insertions(+), 6 deletions(-) create mode 100644 superset-frontend/src/hooks/apiResources/dashboards.test.ts create mode 100644 superset/migrations/versions/2025-06-06_00-39_363a9b1e8992_convert_metric_currencies_from_str_to_json.py diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts index 71517884daa..418efe55fcd 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts @@ -69,6 +69,7 @@ export interface Dataset { columns: ColumnMeta[]; metrics: Metric[]; column_formats: Record; + currency_formats?: Record; verbose_map: Record; main_dttm_col: string; // eg. ['["ds", true]', 'ds [asc]'] diff --git a/superset-frontend/src/components/Datasource/DatasourceModal.tsx b/superset-frontend/src/components/Datasource/DatasourceModal.tsx index c89f588a0cb..e5c3e5c6cd9 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal.tsx @@ -21,7 +21,6 @@ import { useSelector } from 'react-redux'; import Alert from 'src/components/Alert'; import Button from 'src/components/Button'; import { - isDefined, styled, SupersetClient, getClientErrorObject, @@ -141,9 +140,7 @@ const DatasourceModal: FunctionComponent = ({ metric_name: metric.metric_name, metric_type: metric.metric_type, d3format: metric.d3format || null, - currency: !isDefined(metric.currency) - ? null - : JSON.stringify(metric.currency), + currency: metric.currency, verbose_name: metric.verbose_name, warning_text: metric.warning_text, uuid: metric.uuid, diff --git a/superset-frontend/src/explore/actions/hydrateExplore.test.ts b/superset-frontend/src/explore/actions/hydrateExplore.test.ts index cdbca5b94df..f0703699147 100644 --- a/superset-frontend/src/explore/actions/hydrateExplore.test.ts +++ b/superset-frontend/src/explore/actions/hydrateExplore.test.ts @@ -213,3 +213,51 @@ test('uses configured default time range if not set', () => { }), ); }); + +test('extracts currency formats from metrics in dataset', () => { + const dispatch = jest.fn(); + const getState = jest.fn(() => ({ + user: {}, + charts: {}, + datasources: {}, + common: {}, + explore: {}, + })); + + const datasetWithMetrics = { + ...exploreInitialData.dataset, + metrics: [ + { + metric_name: 'count', + currency: { symbol: 'GBP', symbolPosition: 'prefix' }, + }, + { + metric_name: 'revenue', + currency: { symbol: 'USD', symbolPosition: 'suffix' }, + }, + { metric_name: 'no_currency' }, + ], + }; + + // @ts-ignore + hydrateExplore({ ...exploreInitialData, dataset: datasetWithMetrics })( + dispatch, + // @ts-ignore + getState, + ); + + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + datasources: expect.objectContaining({ + '8__table': expect.objectContaining({ + currency_formats: { + count: { symbol: 'GBP', symbolPosition: 'prefix' }, + revenue: { symbol: 'USD', symbolPosition: 'suffix' }, + }, + }), + }), + }), + }), + ); +}); diff --git a/superset-frontend/src/explore/actions/hydrateExplore.ts b/superset-frontend/src/explore/actions/hydrateExplore.ts index 2ee46c1e93e..b8e0375a648 100644 --- a/superset-frontend/src/explore/actions/hydrateExplore.ts +++ b/superset-frontend/src/explore/actions/hydrateExplore.ts @@ -27,6 +27,7 @@ import { getChartKey } from 'src/explore/exploreUtils'; import { getControlsState } from 'src/explore/store'; import { Dispatch } from 'redux'; import { + Currency, ensureIsArray, getCategoricalSchemeRegistry, getColumnLabel, @@ -97,6 +98,14 @@ export const hydrateExplore = } const initialDatasource = dataset; + initialDatasource.currency_formats = Object.fromEntries( + (initialDatasource.metrics ?? []) + .filter(metric => !!metric.currency) + .map((metric): [string, Currency] => [ + metric.metric_name, + metric.currency!, + ]), + ); const initialExploreState = { form_data: initialFormData, diff --git a/superset-frontend/src/hooks/apiResources/dashboards.test.ts b/superset-frontend/src/hooks/apiResources/dashboards.test.ts new file mode 100644 index 00000000000..3a6265163e0 --- /dev/null +++ b/superset-frontend/src/hooks/apiResources/dashboards.test.ts @@ -0,0 +1,89 @@ +/** + * 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 { renderHook } from '@testing-library/react-hooks'; +import fetchMock from 'fetch-mock'; +import { useDashboardDatasets } from './dashboards'; + +describe('useDashboardDatasets', () => { + const mockDatasets = [ + { + id: 1, + metrics: [ + { + metric_name: 'count', + currency: { symbol: 'GBP', symbolPosition: 'prefix' }, + }, + { + metric_name: 'revenue', + currency: { symbol: 'USD', symbolPosition: 'suffix' }, + }, + { metric_name: 'no_currency' }, + ], + }, + { + id: 2, + metrics: [{ metric_name: 'no_currency' }], + }, + { + id: 3, + metrics: [ + { + metric_name: 'other_currency', + currency: { symbol: 'CNY', symbolPosition: 'suffix' }, + }, + ], + }, + ]; + + beforeEach(() => { + fetchMock.reset(); + }); + + it('adds currencyFormats to datasets', async () => { + fetchMock.get('glob:*/api/v1/dashboard/*/datasets', { + result: mockDatasets, + }); + + const { result, waitForNextUpdate } = renderHook(() => + useDashboardDatasets(1), + ); + await waitForNextUpdate(); + + const expectedContent = [ + { + ...mockDatasets[0], + currencyFormats: { + count: { symbol: 'GBP', symbolPosition: 'prefix' }, + revenue: { symbol: 'USD', symbolPosition: 'suffix' }, + }, + }, + { + ...mockDatasets[1], + currencyFormats: {}, + }, + { + ...mockDatasets[2], + currencyFormats: { + other_currency: { symbol: 'CNY', symbolPosition: 'suffix' }, + }, + }, + ]; + expect(result.current.result).toEqual(expectedContent); + }); +}); diff --git a/superset-frontend/src/hooks/apiResources/dashboards.ts b/superset-frontend/src/hooks/apiResources/dashboards.ts index 61896ba1309..65e4b1c0bdd 100644 --- a/superset-frontend/src/hooks/apiResources/dashboards.ts +++ b/superset-frontend/src/hooks/apiResources/dashboards.ts @@ -19,6 +19,7 @@ import { Dashboard, Datasource, EmbeddedDashboard } from 'src/dashboard/types'; import { Chart } from 'src/types/Chart'; +import { Currency } from '@superset-ui/core'; import { useApiV1Resource, useTransformedResource } from './apiResources'; export const useDashboard = (idOrSlug: string | number) => @@ -43,7 +44,21 @@ export const useDashboardCharts = (idOrSlug: string | number) => // important: this endpoint only returns the fields in the dataset // that are necessary for rendering the given dashboard export const useDashboardDatasets = (idOrSlug: string | number) => - useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/datasets`); + useTransformedResource( + useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/datasets`), + datasets => + datasets.map(dataset => ({ + ...dataset, + currencyFormats: Object.fromEntries( + (dataset.metrics ?? []) + .filter(metric => !!metric.currency) + .map((metric): [string, Currency] => [ + metric.metric_name, + metric.currency!, + ]), + ), + })), + ); export const useEmbeddedDashboard = (idOrSlug: string | number) => useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/embedded`); diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index 54ce4481737..405d0bab3dd 100644 --- a/superset/datasets/schemas.py +++ b/superset/datasets/schemas.py @@ -74,6 +74,11 @@ class DatasetColumnsPutSchema(Schema): uuid = fields.UUID(allow_none=True) +class DatasetMetricCurrencyPutSchema(Schema): + symbol = fields.String(validate=Length(1, 128)) + symbolPosition = fields.String(validate=Length(1, 128)) # noqa: N815 + + class DatasetMetricsPutSchema(Schema): id = fields.Integer() expression = fields.String(required=True) @@ -82,7 +87,7 @@ class DatasetMetricsPutSchema(Schema): metric_name = fields.String(required=True, validate=Length(1, 255)) metric_type = fields.String(allow_none=True, validate=Length(1, 32)) d3format = fields.String(allow_none=True, validate=Length(1, 128)) - currency = fields.String(allow_none=True, required=False, validate=Length(1, 128)) + currency = fields.Nested(DatasetMetricCurrencyPutSchema, allow_none=True) verbose_name = fields.String(allow_none=True, metadata={Length: (1, 1024)}) warning_text = fields.String(allow_none=True) uuid = fields.UUID(allow_none=True) diff --git a/superset/migrations/versions/2025-06-06_00-39_363a9b1e8992_convert_metric_currencies_from_str_to_json.py b/superset/migrations/versions/2025-06-06_00-39_363a9b1e8992_convert_metric_currencies_from_str_to_json.py new file mode 100644 index 00000000000..708e7c52dae --- /dev/null +++ b/superset/migrations/versions/2025-06-06_00-39_363a9b1e8992_convert_metric_currencies_from_str_to_json.py @@ -0,0 +1,84 @@ +# 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. +"""convert_metric_currencies_from_str_to_json + +Revision ID: 363a9b1e8992 +Revises: f1edd4a4d4f2 +Create Date: 2025-06-06 00:39:00.107746 + +""" + +import json +import logging + +from alembic import op +from sqlalchemy import Column, Integer, JSON, String +from sqlalchemy.ext.declarative import declarative_base + +from superset import db +from superset.migrations.shared.utils import paginated_update + +logger = logging.getLogger("alembic") +logger.setLevel(logging.INFO) + +# revision identifiers, used by Alembic. +revision = "363a9b1e8992" +down_revision = "f1edd4a4d4f2" + +Base = declarative_base() + + +class SqlMetric(Base): + __tablename__ = "sql_metrics" + + id = Column(Integer, primary_key=True) + metric_name = Column(String(512)) + currency = Column(JSON) + + +def upgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + currency_configs = session.query(SqlMetric).filter(SqlMetric.currency.isnot(None)) + for metric in paginated_update( + currency_configs, + lambda current, total: logger.info((f"Upgrading {current}/{total} metrics")), + ): + while True: + if isinstance(metric.currency, str): + try: + metric.currency = json.loads(metric.currency) + except Exception as e: + logger.error( + f"Error loading metric {metric.metric_name} as json: {e}" + ) + metric.currency = {} + break + else: + break + + +def downgrade(): + """ + No op downgrade. + + The downgrade could just do `metric.currency = json.dumps(metric.currency)`. However + this is happening after `f1edd4a4d4f2` which already converted the currency column + to JSON at the DB level, so we shouldn't have currencies in str anymore. It was the + case because the client was still stringifying it. + """ + pass diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index ab645d300e1..9d226f793dc 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -1462,6 +1462,29 @@ class TestDatasetApi(SupersetTestCase): assert data == expected_result self.items_to_delete = [dataset] + def test_update_dataset_update_metric_invalid_currency(self): + """ + Dataset API: Test update dataset metric with an invalid currency config + """ + + dataset = self.insert_default_dataset() + + self.login(ADMIN_USERNAME) + uri = f"api/v1/dataset/{dataset.id}" + data = { + "metrics": [ + { + "metric_name": "test", + "expression": "COUNT(*)", + "currency": '{"symbol": "USD", "symbolPosition": "suffix"}', + }, + ] + } + rv = self.put_assert_metric(uri, data, "put") + assert rv.status_code == 422 + + self.items_to_delete = [dataset] + def test_update_dataset_item_gamma(self): """ Dataset API: Test update dataset item gamma