From c6e0abbe13d8f4045639e3c61dc0aa534e3ace4c Mon Sep 17 00:00:00 2001 From: Radovenchyk Date: Wed, 19 Mar 2025 19:42:47 +0200 Subject: [PATCH 1/8] chore: replaced the workflow badge link (#32749) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 984d3d63fd5..e0bb2915399 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ under the License. [![License](https://img.shields.io/badge/License-Apache%202.0-blue.svg)](https://opensource.org/license/apache-2-0) [![GitHub release (latest SemVer)](https://img.shields.io/github/v/release/apache/superset?sort=semver)](https://github.com/apache/superset/tree/latest) -[![Build Status](https://github.com/apache/superset/workflows/Python/badge.svg)](https://github.com/apache/superset/actions) +[![Build Status](https://github.com/apache/superset/actions/workflows/superset-python-unittest.yml/badge.svg)](https://github.com/apache/superset/actions) [![PyPI version](https://badge.fury.io/py/apache-superset.svg)](https://badge.fury.io/py/apache-superset) [![Coverage Status](https://codecov.io/github/apache/superset/coverage.svg?branch=master)](https://codecov.io/github/apache/superset) [![PyPI](https://img.shields.io/pypi/pyversions/apache-superset.svg?maxAge=2592000)](https://pypi.python.org/pypi/apache-superset) From e9d507998636066721b1d4347e6e3ce78f2cf23d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 19 Mar 2025 11:24:24 -0700 Subject: [PATCH 2/8] =?UTF-8?q?chore(=F0=9F=A6=BE):=20bump=20python=20flas?= =?UTF-8?q?k-appbuilder=20subpackage(s)=20(#32744)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: GitHub Action Co-authored-by: Maxime Beauchemin --- requirements/base.in | 7 +++++++ requirements/base.txt | 14 ++++++-------- requirements/development.txt | 13 ++++--------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/requirements/base.in b/requirements/base.in index accdd2fce91..56085768b8f 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -27,3 +27,10 @@ async_timeout>=4.0.0,<5.0.0 # Known issue with 6.7.0 breaking a unit test, probably easy to fix, but will require # a bit of attention to bump. apispec>=6.0.0,<6.7.0 + +# 1.4.0 appears to use much more memory, where the python test suite runs out of memory +# causing CI to fail. 1.3.0 is the last version that works. +# This is probably related to the changes around PickleType +# https://marshmallow-sqlalchemy.readthedocs.io/en/latest/changelog.html#id3 +# Opened this issue https://github.com/marshmallow-code/marshmallow-sqlalchemy/issues/665 +marshmallow-sqlalchemy>=1.3.0,<1.4.0 diff --git a/requirements/base.txt b/requirements/base.txt index 2ccec3a9a06..e3a92219e1a 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -128,7 +128,7 @@ flask-compress==1.17 # via apache-superset (pyproject.toml) flask-jwt-extended==4.7.1 # via flask-appbuilder -flask-limiter==3.11.0 +flask-limiter==3.12 # via flask-appbuilder flask-login==0.6.3 # via @@ -176,8 +176,6 @@ idna==3.10 # trio importlib-metadata==8.6.1 # via apache-superset (pyproject.toml) -importlib-resources==6.5.2 - # via limits isodate==0.7.2 # via apache-superset (pyproject.toml) itsdangerous==2.2.0 @@ -198,7 +196,7 @@ kombu==5.5.0 # via celery korean-lunar-calendar==0.3.1 # via holidays -limits==3.13.0 +limits==4.4.1 # via flask-limiter mako==1.3.9 # via @@ -218,8 +216,10 @@ marshmallow==3.26.1 # via # flask-appbuilder # marshmallow-sqlalchemy -marshmallow-sqlalchemy==0.28.2 - # via flask-appbuilder +marshmallow-sqlalchemy==1.3.0 + # via + # -r requirements/base.in + # flask-appbuilder mdurl==0.1.2 # via markdown-it-py msgpack==1.0.8 @@ -253,7 +253,6 @@ packaging==24.2 # gunicorn # limits # marshmallow - # marshmallow-sqlalchemy # shillelagh pandas==2.0.3 # via apache-superset (pyproject.toml) @@ -394,7 +393,6 @@ typing-extensions==4.12.2 # apache-superset (pyproject.toml) # alembic # cattrs - # flask-limiter # limits # pyopenssl # referencing diff --git a/requirements/development.txt b/requirements/development.txt index 63453cb5a66..68ec6a17c37 100644 --- a/requirements/development.txt +++ b/requirements/development.txt @@ -224,7 +224,7 @@ flask-jwt-extended==4.7.1 # via # -c requirements/base.txt # flask-appbuilder -flask-limiter==3.11.0 +flask-limiter==3.12 # via # -c requirements/base.txt # flask-appbuilder @@ -360,10 +360,7 @@ importlib-metadata==8.6.1 # -c requirements/base.txt # apache-superset importlib-resources==6.5.2 - # via - # -c requirements/base.txt - # limits - # prophet + # via prophet iniconfig==2.0.0 # via pytest isodate==0.7.2 @@ -409,7 +406,7 @@ korean-lunar-calendar==0.3.1 # holidays lazy-object-proxy==1.10.0 # via openapi-spec-validator -limits==3.13.0 +limits==4.4.1 # via # -c requirements/base.txt # flask-limiter @@ -438,7 +435,7 @@ marshmallow==3.26.1 # -c requirements/base.txt # flask-appbuilder # marshmallow-sqlalchemy -marshmallow-sqlalchemy==0.28.2 +marshmallow-sqlalchemy==1.3.0 # via # -c requirements/base.txt # flask-appbuilder @@ -511,7 +508,6 @@ packaging==24.2 # gunicorn # limits # marshmallow - # marshmallow-sqlalchemy # matplotlib # pytest # shillelagh @@ -849,7 +845,6 @@ typing-extensions==4.12.2 # alembic # apache-superset # cattrs - # flask-limiter # limits # pyopenssl # referencing From 78efb62781f3f55f54d941c8fbb7a2df044c46b8 Mon Sep 17 00:00:00 2001 From: Giampaolo Capelli Date: Wed, 19 Mar 2025 19:38:53 +0100 Subject: [PATCH 3/8] fix: Changing language doesn't affect echarts charts (#31751) Co-authored-by: Giampaolo Capelli --- .../plugins/plugin-chart-echarts/package.json | 3 +- .../src/components/Echart.tsx | 86 ++++++++++++------- .../plugin-chart-echarts/src/constants.ts | 2 + 3 files changed, 59 insertions(+), 32 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/package.json b/superset-frontend/plugins/plugin-chart-echarts/package.json index f84ecd0ed4a..64da86afdcc 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/package.json +++ b/superset-frontend/plugins/plugin-chart-echarts/package.json @@ -26,7 +26,8 @@ "dependencies": { "d3-array": "^1.2.0", "lodash": "^4.17.21", - "dayjs": "^1.11.13" + "dayjs": "^1.11.13", + "@types/react-redux": "^7.1.10" }, "peerDependencies": { "@superset-ui/chart-controls": "*", diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx index 995e3a53513..5f5f71c14bd 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx @@ -27,8 +27,10 @@ import { Ref, } from 'react'; +import { useSelector } from 'react-redux'; + import { styled } from '@superset-ui/core'; -import { use, init, EChartsType } from 'echarts/core'; +import { use, init, EChartsType, registerLocale } from 'echarts/core'; import { SankeyChart, PieChart, @@ -60,6 +62,15 @@ import { } from 'echarts/components'; import { LabelLayout } from 'echarts/features'; import { EchartsHandler, EchartsProps, EchartsStylesProps } from '../types'; +import { DEFAULT_LOCALE } from '../constants'; + +// Define this interface here to avoid creating a dependency back to superset-frontend, +// TODO: to move the type to @superset-ui/core +interface ExplorePageState { + common: { + locale: string; + }; +} const Styles = styled.div` height: ${({ height }) => height}; @@ -123,24 +134,52 @@ function Echart( getEchartInstance: () => chartRef.current, })); + const locale = useSelector( + (state: ExplorePageState) => state?.common?.locale ?? DEFAULT_LOCALE, + ).toUpperCase(); + + const handleSizeChange = useCallback( + ({ width, height }: { width: number; height: number }) => { + if (chartRef.current) { + chartRef.current.resize({ width, height }); + } + }, + [], + ); + useEffect(() => { - if (!divRef.current) return; - if (!chartRef.current) { - chartRef.current = init(divRef.current); - } + const loadLocaleAndInitChart = async () => { + if (!divRef.current) return; - Object.entries(eventHandlers || {}).forEach(([name, handler]) => { - chartRef.current?.off(name); - chartRef.current?.on(name, handler); - }); + const lang = await import(`echarts/lib/i18n/lang${locale}`).catch(e => { + console.error(`Locale ${locale} not supported in ECharts`, e); + }); + if (lang?.default) { + registerLocale(locale, lang.default); + } - Object.entries(zrEventHandlers || {}).forEach(([name, handler]) => { - chartRef.current?.getZr().off(name); - chartRef.current?.getZr().on(name, handler); - }); + if (!chartRef.current) { + chartRef.current = init(divRef.current, null, { locale }); + } - chartRef.current.setOption(echartOptions, true); - }, [echartOptions, eventHandlers, zrEventHandlers]); + Object.entries(eventHandlers || {}).forEach(([name, handler]) => { + chartRef.current?.off(name); + chartRef.current?.on(name, handler); + }); + + Object.entries(zrEventHandlers || {}).forEach(([name, handler]) => { + chartRef.current?.getZr().off(name); + chartRef.current?.getZr().on(name, handler); + }); + + chartRef.current.setOption(echartOptions, true); + + // did mount + handleSizeChange({ width, height }); + }; + + loadLocaleAndInitChart(); + }, [echartOptions, eventHandlers, zrEventHandlers, locale]); // highlighting useEffect(() => { @@ -158,22 +197,7 @@ function Echart( }); } previousSelection.current = currentSelection; - }, [currentSelection]); - - const handleSizeChange = useCallback( - ({ width, height }: { width: number; height: number }) => { - if (chartRef.current) { - chartRef.current.resize({ width, height }); - } - }, - [], - ); - - // did mount - useEffect(() => { - handleSizeChange({ width, height }); - return () => chartRef.current?.dispose(); - }, []); + }, [currentSelection, chartRef.current]); useLayoutEffect(() => { handleSizeChange({ width, height }); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts index 65ea1679e2c..fb6221342a4 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts @@ -121,3 +121,5 @@ export const TOOLTIP_POINTER_MARGIN = 10; // If no satisfactory position can be found, how far away // from the edge of the window should the tooltip be kept export const TOOLTIP_OVERFLOW_MARGIN = 5; + +export const DEFAULT_LOCALE = 'en'; From 6042ea8f282b0f24a7b63af6c9cda95cd2239596 Mon Sep 17 00:00:00 2001 From: Vitor Avila <96086495+Vitor-Avila@users.noreply.github.com> Date: Wed, 19 Mar 2025 15:44:07 -0300 Subject: [PATCH 4/8] feat(embedded): Force a specific referrerPolicy for the iframe request (#32735) --- superset-embedded-sdk/README.md | 12 +++++++++++- superset-embedded-sdk/src/index.ts | 9 ++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/superset-embedded-sdk/README.md b/superset-embedded-sdk/README.md index 377720dd3b9..63d8e706183 100644 --- a/superset-embedded-sdk/README.md +++ b/superset-embedded-sdk/README.md @@ -60,7 +60,9 @@ embedDashboard({ } }, // optional additional iframe sandbox attributes - iframeSandboxExtras: ['allow-top-navigation', 'allow-popups-to-escape-sandbox'] + iframeSandboxExtras: ['allow-top-navigation', 'allow-popups-to-escape-sandbox'], + // optional config to enforce a particular referrerPolicy + referrerPolicy: "same-origin" }); ``` @@ -146,3 +148,11 @@ To pass additional sandbox attributes you can use `iframeSandboxExtras`: // optional additional iframe sandbox attributes iframeSandboxExtras: ['allow-top-navigation', 'allow-popups-to-escape-sandbox'] ``` + +### Enforcing a ReferrerPolicy on the request triggered by the iframe + +By default, the Embedded SDK creates an `iframe` element without a `referrerPolicy` value enforced. This means that a policy defined for `iframe` elements at the host app level would reflect to it. + +This can be an issue as during the embedded enablement for a dashboard it's possible to specify which domain(s) are allowed to embed the dashboard, and this validation happens throuth the `Referrer` header. That said, in case the hosting app has a more restrictive policy that would omit this header, this validation would fail. + +Use the `referrerPolicy` parameter in the `embedDashboard` method to specify [a particular policy](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Referrer-Policy) that works for your implementation. diff --git a/superset-embedded-sdk/src/index.ts b/superset-embedded-sdk/src/index.ts index 063db77fb70..7bcf5dcd2e9 100644 --- a/superset-embedded-sdk/src/index.ts +++ b/superset-embedded-sdk/src/index.ts @@ -64,6 +64,8 @@ export type EmbedDashboardParams = { iframeTitle?: string /** additional iframe sandbox attributes ex (allow-top-navigation, allow-popups-to-escape-sandbox) **/ iframeSandboxExtras?: string[] + /** force a specific refererPolicy to be used in the iframe request **/ + referrerPolicy?: ReferrerPolicy } export type Size = { @@ -88,7 +90,8 @@ export async function embedDashboard({ dashboardUiConfig, debug = false, iframeTitle = "Embedded Dashboard", - iframeSandboxExtras = [] + iframeSandboxExtras = [], + referrerPolicy, }: EmbedDashboardParams): Promise { function log(...info: unknown[]) { if (debug) { @@ -142,6 +145,10 @@ export async function embedDashboard({ iframeSandboxExtras.forEach((key: string) => { iframe.sandbox.add(key); }); + // force a specific refererPolicy to be used in the iframe request + if(referrerPolicy) { + iframe.referrerPolicy = referrerPolicy; + } // add the event listener before setting src, to be 100% sure that we capture the load event iframe.addEventListener('load', () => { From 376a1f49d339c393af678b12ad74ae91a72c94fc Mon Sep 17 00:00:00 2001 From: Antonio Rivero <38889534+Antonio-RiveroMartnez@users.noreply.github.com> Date: Wed, 19 Mar 2025 22:47:26 +0100 Subject: [PATCH 5/8] fix(migrations): fix foreign keys to match FAB 4.6.0 tables (#32759) --- superset/migrations/shared/utils.py | 81 +++++++++++-- ...bf93dfe2a4_add_on_cascade_in_fab_tables.py | 111 ++++++++++++++++++ 2 files changed, 184 insertions(+), 8 deletions(-) create mode 100644 superset/migrations/versions/2025-03-19_17-46_32bf93dfe2a4_add_on_cascade_in_fab_tables.py diff --git a/superset/migrations/shared/utils.py b/superset/migrations/shared/utils.py index 0cddd323949..32e7710729d 100644 --- a/superset/migrations/shared/utils.py +++ b/superset/migrations/shared/utils.py @@ -193,12 +193,16 @@ def has_table(table_name: str) -> bool: return table_exists -def drop_fks_for_table(table_name: str) -> None: +def drop_fks_for_table( + table_name: str, foreign_key_names: list[str] | None = None +) -> None: """ - Drop all foreign key constraints for a table if it exist and the database - is not sqlite. + Drop specific or all foreign key constraints for a table + if they exist and the database is not sqlite. - :param table_name: The table name to drop foreign key constraints for + :param table_name: The table name to drop foreign key constraints from + :param foreign_key_names: Optional list of specific foreign key names to drop. + If None is provided, all will be dropped. """ connection = op.get_bind() inspector = Inspector.from_engine(connection) @@ -207,12 +211,19 @@ def drop_fks_for_table(table_name: str) -> None: return # sqlite doesn't like constraints if has_table(table_name): - foreign_keys = inspector.get_foreign_keys(table_name) - for fk in foreign_keys: + existing_fks = {fk["name"] for fk in inspector.get_foreign_keys(table_name)} + + # What to delete based on whether the list was passed + if foreign_key_names is not None: + foreign_key_names = list(set(foreign_key_names) & existing_fks) + else: + foreign_key_names = list(existing_fks) + + for fk_name in foreign_key_names: logger.info( - f"Dropping foreign key {GREEN}{fk['name']}{RESET} from table {GREEN}{table_name}{RESET}..." # noqa: E501 + f"Dropping foreign key {GREEN}{fk_name}{RESET} from table {GREEN}{table_name}{RESET}..." # noqa: E501 ) - op.drop_constraint(fk["name"], table_name, type_="foreignkey") + op.drop_constraint(fk_name, table_name, type_="foreignkey") def create_table(table_name: str, *columns: SchemaItem) -> None: @@ -406,3 +417,57 @@ def drop_index(table_name: str, index_name: str) -> None: ) op.drop_index(table_name=table_name, index_name=index_name) + + +def create_fks_for_table( + foreign_key_name: str, + table_name: str, + referenced_table: str, + local_cols: list[str], + remote_cols: list[str], + ondelete: Optional[str] = None, +) -> None: + """ + Create a foreign key constraint for a table, ensuring compatibility with sqlite. + + :param foreign_key_name: Foreign key constraint name. + :param table_name: The name of the table where the foreign key will be created. + :param referenced_table: The table the FK references. + :param local_cols: Column names in the current table. + :param remote_cols: Column names in the referenced table. + :param ondelete: (Optional) The ON DELETE action (e.g., "CASCADE", "SET NULL"). + """ + connection = op.get_bind() + + if not has_table(table_name): + logger.warning( + f"Table {LRED}{table_name}{RESET} does not exist. Skipping foreign key creation." # noqa: E501 + ) + return + + if isinstance(connection.dialect, SQLiteDialect): + # SQLite requires batch mode since ALTER TABLE is limited + with op.batch_alter_table(table_name) as batch_op: + logger.info( + f"Creating foreign key {GREEN}{foreign_key_name}{RESET} on table {GREEN}{table_name}{RESET} (SQLite mode)..." # noqa: E501 + ) + batch_op.create_foreign_key( + foreign_key_name, + referenced_table, + local_cols, + remote_cols, + ondelete=ondelete, + ) + else: + # Standard FK creation for other databases + logger.info( + f"Creating foreign key {GREEN}{foreign_key_name}{RESET} on table {GREEN}{table_name}{RESET}..." # noqa: E501 + ) + op.create_foreign_key( + foreign_key_name, + table_name, + referenced_table, + local_cols, + remote_cols, + ondelete=ondelete, + ) diff --git a/superset/migrations/versions/2025-03-19_17-46_32bf93dfe2a4_add_on_cascade_in_fab_tables.py b/superset/migrations/versions/2025-03-19_17-46_32bf93dfe2a4_add_on_cascade_in_fab_tables.py new file mode 100644 index 00000000000..e7a9a3d75cd --- /dev/null +++ b/superset/migrations/versions/2025-03-19_17-46_32bf93dfe2a4_add_on_cascade_in_fab_tables.py @@ -0,0 +1,111 @@ +# 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. +"""Add on cascade to foreign keys in ab_permission_view_role and ab_user_role + +Revision ID: 32bf93dfe2a4 +Revises: 94e7a3499973 +Create Date: 2025-03-19 17:46:25.702610 + +""" + +from superset.migrations.shared.utils import create_fks_for_table, drop_fks_for_table + +# revision identifiers, used by Alembic. +revision = "32bf93dfe2a4" +down_revision = "94e7a3499973" + + +def upgrade(): + drop_fks_for_table( + "ab_permission_view_role", + [ + "ab_permission_view_role_permission_view_id_fkey", + "ab_permission_view_role_role_id_fkey", + ], + ) + create_fks_for_table( + "ab_permission_view_role_role_id_fkey", + "ab_permission_view_role", + "ab_role", + ["role_id"], + ["id"], + ondelete="CASCADE", + ) + create_fks_for_table( + "ab_permission_view_role_permission_view_id_fkey", + "ab_permission_view_role", + "ab_permission_view", + ["permission_view_id"], + ["id"], + ondelete="CASCADE", + ) + + drop_fks_for_table( + "ab_user_role", + ["ab_user_role_user_id_fkey", "ab_user_role_role_id_fkey"], + ) + create_fks_for_table( + "ab_user_role_user_id_fkey", + "ab_user_role", + "ab_user", + ["user_id"], + ["id"], + ondelete="CASCADE", + ) + create_fks_for_table( + "ab_user_role_role_id_fkey", + "ab_user_role", + "ab_role", + ["role_id"], + ["id"], + ondelete="CASCADE", + ) + + +def downgrade(): + drop_fks_for_table( + "ab_permission_view_role", + [ + "ab_permission_view_role_permission_view_id_fkey", + "ab_permission_view_role_role_id_fkey", + ], + ) + create_fks_for_table( + "ab_permission_view_role_permission_view_id_fkey", + "ab_permission_view_role", + "ab_permission_view", + ["permission_view_id"], + ["id"], + ) + create_fks_for_table( + "ab_permission_view_role_role_id_fkey", + "ab_permission_view_role", + "ab_role", + ["role_id"], + ["id"], + ) + + drop_fks_for_table( + "ab_user_role", + ["ab_user_role_user_id_fkey", "ab_user_role_role_id_fkey"], + ) + create_fks_for_table( + "ab_user_role_user_id_fkey", "ab_user_role", "ab_user", ["user_id"], ["id"] + ) + create_fks_for_table( + "ab_user_role_role_id_fkey", "ab_user_role", "ab_role", ["role_id"], ["id"] + ) From 89ce7ba0b0dc83e44166672136fd62d58aef3f08 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Wed, 19 Mar 2025 17:33:28 -0700 Subject: [PATCH 6/8] fix: do not add calculated columns when syncing (#32761) --- .../src/components/Datasource/DatasourceEditor.jsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx index 94620563908..057cff7b346 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx @@ -715,7 +715,11 @@ class DatasourceEditor extends PureComponent { newCols, this.props.addSuccessToast, ); - this.setColumns({ databaseColumns: columnChanges.finalColumns }); + this.setColumns({ + databaseColumns: columnChanges.finalColumns.filter( + col => !col.expression, // remove calculated columns + ), + }); this.props.addSuccessToast(t('Metadata has been synced')); this.setState({ metadataLoading: false }); } catch (error) { From 5392bafe28760ac62cce0a7298351940c9a50501 Mon Sep 17 00:00:00 2001 From: Alexandru Soare <37236580+alexandrusoare@users.noreply.github.com> Date: Thu, 20 Mar 2025 14:28:25 +0200 Subject: [PATCH 7/8] feat(FormModal): Specialized Modal component for forms (#32721) --- .../src/components/Form/Form.tsx | 10 +- .../src/components/Modal/FormModal.test.tsx | 115 ++++++++++++++++ .../src/components/Modal/FormModal.tsx | 126 ++++++++++++++++++ 3 files changed, 250 insertions(+), 1 deletion(-) create mode 100644 superset-frontend/src/components/Modal/FormModal.test.tsx create mode 100644 superset-frontend/src/components/Modal/FormModal.tsx diff --git a/superset-frontend/src/components/Form/Form.tsx b/superset-frontend/src/components/Form/Form.tsx index 66a9f58d7f2..d827dd5871a 100644 --- a/superset-frontend/src/components/Form/Form.tsx +++ b/superset-frontend/src/components/Form/Form.tsx @@ -29,8 +29,16 @@ const StyledForm = styled(AntdForm)` } `; -export default function Form(props: FormProps) { +function Form(props: FormProps) { return ; } +export default Object.assign(Form, { + useForm: AntdForm.useForm, + Item: AntdForm.Item, + List: AntdForm.List, + ErrorList: AntdForm.ErrorList, + Provider: AntdForm.Provider, +}); + export type { FormProps }; diff --git a/superset-frontend/src/components/Modal/FormModal.test.tsx b/superset-frontend/src/components/Modal/FormModal.test.tsx new file mode 100644 index 00000000000..c061e83acd5 --- /dev/null +++ b/superset-frontend/src/components/Modal/FormModal.test.tsx @@ -0,0 +1,115 @@ +/** + * 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 { + render, + fireEvent, + screen, + userEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import FormModal, { FormModalProps } from 'src/components/Modal/FormModal'; +import { FormItem } from 'src/components/Form'; +import { Input } from 'src/components/Input'; + +describe('FormModal Component', () => { + const children = ( + <> + + + + + + + + ); + + const mockedProps: FormModalProps = { + show: true, + onHide: jest.fn(), + title: 'Test Form Modal', + onSave: jest.fn(), + formSubmitHandler: jest.fn().mockResolvedValue(undefined), + initialValues: { name: '', email: '' }, + requiredFields: ['name'], + children, + }; + + const renderComponent = () => render(); + + it('should render the modal with two input fields', () => { + renderComponent(); + + expect(screen.getByLabelText('Name')).toBeInTheDocument(); + expect(screen.getByLabelText('Email')).toBeInTheDocument(); + }); + + it('should disable Save button when required fields are empty', async () => { + renderComponent(); + + const saveButton = screen.getByTestId('form-modal-save-button'); + expect(saveButton).toBeDisabled(); + }); + + it('should enable Save button only when the required field is filled', async () => { + renderComponent(); + + const nameInput = screen.getByPlaceholderText('Enter your name'); + userEvent.type(nameInput, 'Jane Doe'); + + await waitFor(() => { + expect(screen.getByTestId('form-modal-save-button')).toBeEnabled(); + }); + }); + + it('should keep Save button disabled when only the optional field is filled', async () => { + renderComponent(); + + const emailInput = screen.getByPlaceholderText('Enter your email'); + userEvent.type(emailInput, 'test@example.com'); + + await waitFor(() => { + expect(screen.getByTestId('form-modal-save-button')).toBeDisabled(); + }); + }); + + it('should call formSubmitHandler with correct values when submitted', async () => { + renderComponent(); + + userEvent.type(screen.getByPlaceholderText('Enter your name'), 'Jane Doe'); + userEvent.type( + screen.getByPlaceholderText('Enter your email'), + 'test@example.com', + ); + + fireEvent.click(screen.getByText('Save')); + + await waitFor(() => { + expect(mockedProps.formSubmitHandler).toHaveBeenCalledWith({ + name: 'Jane Doe', + email: 'test@example.com', + }); + expect(mockedProps.onSave).toHaveBeenCalled(); + }); + }); +}); diff --git a/superset-frontend/src/components/Modal/FormModal.tsx b/superset-frontend/src/components/Modal/FormModal.tsx new file mode 100644 index 00000000000..6141f7806e6 --- /dev/null +++ b/superset-frontend/src/components/Modal/FormModal.tsx @@ -0,0 +1,126 @@ +/** + * 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 Modal, { ModalProps } from 'src/components/Modal'; +import Button from 'src/components/Button'; +import { Form } from 'src/components/Form'; +import { useState, useCallback } from 'react'; +import { t } from '@superset-ui/core'; + +export interface FormModalProps extends ModalProps { + initialValues: Object; + formSubmitHandler: (values: Object) => Promise; + onSave: () => void; + requiredFields: string[]; +} + +function FormModal({ + show, + onHide, + title, + onSave, + children, + initialValues = {}, + formSubmitHandler, + bodyStyle = {}, + requiredFields = [], +}: FormModalProps) { + const [form] = Form.useForm(); + const [isSaving, setIsSaving] = useState(false); + const resetForm = useCallback(() => { + form.resetFields(); + setIsSaving(false); + }, [form]); + const [submitDisabled, setSubmitDisabled] = useState(true); + + const handleClose = useCallback(() => { + resetForm(); + onHide(); + }, [onHide, resetForm]); + + const handleSave = useCallback(() => { + resetForm(); + onSave(); + }, [onSave, resetForm]); + + const handleFormSubmit = useCallback( + async values => { + try { + setIsSaving(true); + await formSubmitHandler(values); + handleSave(); + } catch (err) { + console.error(err); + } finally { + setIsSaving(false); + } + }, + [formSubmitHandler, handleSave], + ); + + const onFormChange = () => { + const hasErrors = form.getFieldsError().some(({ errors }) => errors.length); + + const values = form.getFieldsValue(); + const hasEmptyRequired = requiredFields.some(field => !values[field]); + + setSubmitDisabled(hasErrors || hasEmptyRequired); + }; + + return ( + + + + + } + > +
+ {typeof children === 'function' ? children(form) : children} +
+ + ); +} + +export default FormModal; From d319543377f69ec6f0848e0393033283b39b5733 Mon Sep 17 00:00:00 2001 From: Vladislav Korenkov <73882772+Quatters@users.noreply.github.com> Date: Thu, 20 Mar 2025 23:14:09 +1000 Subject: [PATCH 8/8] fix(chart control): Change default of "Y Axis Title Margin" (#32720) --- .../cypress/e2e/dashboard/drillby.test.ts | 36 +++++++++---------- .../e2e/dashboard/drilltodetail.test.ts | 16 ++++----- .../src/sections/chartTitle.tsx | 2 +- .../Timeseries/Regular/Bar/controlPanel.tsx | 2 +- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts index 2586fbd4381..2f8254310ca 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts @@ -510,29 +510,29 @@ describe('Drill by modal', () => { it('Line chart', () => { testEchart('echarts_timeseries_line', 'Line Chart', [ - [70, 93], - [70, 93], + [85, 93], + [85, 93], ]); }); it('Area Chart', () => { testEchart('echarts_area', 'Area Chart', [ - [70, 93], - [70, 93], + [85, 93], + [85, 93], ]); }); it('Scatter Chart', () => { testEchart('echarts_timeseries_scatter', 'Scatter Chart', [ - [70, 93], - [70, 93], + [85, 93], + [85, 93], ]); }); it('Bar Chart', () => { testEchart('echarts_timeseries_bar', 'Bar Chart', [ - [70, 94], - [362, 68], + [85, 94], + [490, 68], ]); }); @@ -565,22 +565,22 @@ describe('Drill by modal', () => { it('Generic Chart', () => { testEchart('echarts_timeseries', 'Generic Chart', [ - [70, 93], - [70, 93], + [85, 93], + [85, 93], ]); }); it('Smooth Line Chart', () => { testEchart('echarts_timeseries_smooth', 'Smooth Line Chart', [ - [70, 93], - [70, 93], + [85, 93], + [85, 93], ]); }); it('Step Line Chart', () => { testEchart('echarts_timeseries_step', 'Step Line Chart', [ - [70, 93], - [70, 93], + [85, 93], + [85, 93], ]); }); @@ -616,8 +616,8 @@ describe('Drill by modal', () => { cy.get('[data-test-viz-type="mixed_timeseries"] canvas').then($canvas => { // click 'boy' cy.wrap($canvas).scrollIntoView(); - cy.wrap($canvas).trigger('mouseover', 70, 93); - cy.wrap($canvas).rightclick(70, 93); + cy.wrap($canvas).trigger('mouseover', 85, 93); + cy.wrap($canvas).rightclick(85, 93); drillBy('name').then(intercepted => { const { queries } = intercepted.request.body; @@ -650,8 +650,8 @@ describe('Drill by modal', () => { cy.get(`[data-test="drill-by-chart"] canvas`).then($canvas => { // click second query cy.wrap($canvas).scrollIntoView(); - cy.wrap($canvas).trigger('mouseover', 246, 114); - cy.wrap($canvas).rightclick(246, 114); + cy.wrap($canvas).trigger('mouseover', 261, 114); + cy.wrap($canvas).rightclick(261, 114); drillBy('ds').then(intercepted => { const { queries } = intercepted.request.body; diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/drilltodetail.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/drilltodetail.test.ts index 1063689edce..f6fd4b6a5c2 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/drilltodetail.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/drilltodetail.test.ts @@ -95,24 +95,24 @@ function testTimeChart(vizType: string) { cy.get(`[data-test-viz-type='${vizType}'] canvas`).then($canvas => { cy.wrap($canvas).scrollIntoView(); - cy.wrap($canvas).trigger('mousemove', 70, 93); - cy.wrap($canvas).rightclick(70, 93); + cy.wrap($canvas).trigger('mousemove', 85, 93); + cy.wrap($canvas).rightclick(85, 93); drillToDetailBy('Drill to detail by 1965'); cy.getBySel('filter-val').should('contain', '1965'); closeModal(); cy.wrap($canvas).scrollIntoView(); - cy.wrap($canvas).trigger('mousemove', 70, 93); - cy.wrap($canvas).rightclick(70, 93); + cy.wrap($canvas).trigger('mousemove', 85, 93); + cy.wrap($canvas).rightclick(85, 93); drillToDetailBy('Drill to detail by boy'); cy.getBySel('filter-val').should('contain', 'boy'); closeModal(); cy.wrap($canvas).scrollIntoView(); - cy.wrap($canvas).trigger('mousemove', 70, 93); - cy.wrap($canvas).rightclick(70, 93); + cy.wrap($canvas).trigger('mousemove', 85, 93); + cy.wrap($canvas).rightclick(85, 93); drillToDetailBy('Drill to detail by all'); cy.getBySel('filter-val').first().should('contain', '1965'); @@ -434,7 +434,7 @@ describe('Drill to detail modal', () => { SUPPORTED_TIER2_CHARTS.forEach(waitForChartLoad); }); - describe('Modal actions', () => { + describe.only('Modal actions', () => { it('clears filters', () => { interceptSamples(); @@ -442,7 +442,7 @@ describe('Drill to detail modal', () => { cy.get("[data-test-viz-type='box_plot'] canvas").then($canvas => { const canvasWidth = $canvas.width() || 0; const canvasHeight = $canvas.height() || 0; - const canvasCenterX = canvasWidth / 3; + const canvasCenterX = canvasWidth / 3 + 15; const canvasCenterY = (canvasHeight * 5) / 6; cy.wrap($canvas).scrollIntoView(); diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx index 6c8d08364cc..43ad46ba7f7 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx @@ -84,7 +84,7 @@ export const titleControls: ControlPanelSectionConfig = { clearable: true, label: t('Y Axis Title Margin'), renderTrigger: true, - default: TITLE_MARGIN_OPTIONS[0], + default: TITLE_MARGIN_OPTIONS[1], choices: formatSelectOptions(TITLE_MARGIN_OPTIONS), description: t('Changing this control takes effect instantly'), }, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx index ee2d9a6c5cf..55cd48736a0 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx @@ -122,7 +122,7 @@ function createAxisTitleControl(axis: 'x' | 'y'): ControlSetRow[] { clearable: true, label: t('AXIS TITLE MARGIN'), renderTrigger: true, - default: sections.TITLE_MARGIN_OPTIONS[0], + default: sections.TITLE_MARGIN_OPTIONS[1], choices: formatSelectOptions(sections.TITLE_MARGIN_OPTIONS), description: t('Changing this control takes effect instantly'), visibility: ({ controls }: ControlPanelsContainerProps) =>