From 462d00458691475b3168e5b993ebb066a2e2aee9 Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 19 Jul 2023 15:58:34 -0700 Subject: [PATCH 1/3] fix(native filter): clean deleted parent filter ids --- .../FiltersConfigModal.test.tsx | 85 ++++++++++++++++++- .../FiltersConfigModal/FiltersConfigModal.tsx | 37 ++++---- 2 files changed, 104 insertions(+), 18 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx index 58ebcea548bc4..5e6f1f5658744 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx @@ -353,10 +353,91 @@ test('filters are draggable', async () => { adds a new time grain filter type with all fields filled collapsible controls opens by default when it is checked advanced section opens by default when it has an option checked - deletes a filter - disables the default value when default to first item is checked + disables the default value when default to first item is checked changes the default value options when the column changes switches to configuration tab when validation fails displays cancel message when there are pending operations do not displays cancel message when there are no pending operations */ + +test('deletes a filter', async () => { + const nativeFilterState = [ + buildNativeFilter('NATIVE_FILTER-1', 'state', ['NATIVE_FILTER-2']), + buildNativeFilter('NATIVE_FILTER-2', 'country', []), + buildNativeFilter('NATIVE_FILTER-3', 'product', []), + ]; + const state = { + ...defaultState(), + dashboardInfo: { + metadata: { native_filter_configuration: nativeFilterState }, + }, + dashboardLayout, + }; + const onSave = jest.fn(); + defaultRender(state, { + ...props, + createNewOnOpen: false, + onSave, + }); + const removeButtons = screen.getAllByRole('img', { name: 'trash' }); + // remove NATIVE_FILTER-3 which isn't a dependancy of any other filter + userEvent.click(removeButtons[2]); + userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); + await waitFor(() => + expect(onSave).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + type: 'NATIVE_FILTER', + id: 'NATIVE_FILTER-1', + cascadeParentIds: ['NATIVE_FILTER-2'], + }), + expect.objectContaining({ + type: 'NATIVE_FILTER', + id: 'NATIVE_FILTER-2', + cascadeParentIds: [], + }), + ]), + ), + ); +}); + +test('deletes a filter including dependencies', async () => { + const nativeFilterState = [ + buildNativeFilter('NATIVE_FILTER-1', 'state', ['NATIVE_FILTER-2']), + buildNativeFilter('NATIVE_FILTER-2', 'country', []), + buildNativeFilter('NATIVE_FILTER-3', 'product', []), + ]; + const state = { + ...defaultState(), + dashboardInfo: { + metadata: { native_filter_configuration: nativeFilterState }, + }, + dashboardLayout, + }; + const onSave = jest.fn(); + defaultRender(state, { + ...props, + createNewOnOpen: false, + onSave, + }); + const removeButtons = screen.getAllByRole('img', { name: 'trash' }); + // remove NATIVE_FILTER-2 which is a dependancy of NATIVE_FILTER-1 + userEvent.click(removeButtons[1]); + userEvent.click(screen.getByRole('button', { name: SAVE_REGEX })); + await waitFor(() => + expect(onSave).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + type: 'NATIVE_FILTER', + id: 'NATIVE_FILTER-1', + cascadeParentIds: [], + }), + expect.objectContaining({ + type: 'NATIVE_FILTER', + id: 'NATIVE_FILTER-3', + cascadeParentIds: [], + }), + ]), + ), + ); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 0abca30a303f2..29833af580eca 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -306,21 +306,25 @@ function FiltersConfigModal({ ); const cleanDeletedParents = (values: NativeFiltersForm | null) => { - Object.keys(filterConfigMap).forEach(key => { - const filter = filterConfigMap[key]; - if (!('cascadeParentIds' in filter)) { - return; - } - const { cascadeParentIds } = filter; - if (cascadeParentIds) { - dispatch( - updateCascadeParentIds( - key, - cascadeParentIds.filter(id => canBeUsedAsDependency(id)), - ), + const updatedFilterConfigMap = Object.keys(filterConfigMap).reduce( + (acc, key) => { + const filter = filterConfigMap[key]; + const cascadeParentIds = filter.cascadeParentIds?.filter(id => + canBeUsedAsDependency(id), ); - } - }); + if (cascadeParentIds) { + dispatch(updateCascadeParentIds(key, cascadeParentIds)); + } + return { + ...acc, + [key]: { + ...filter, + cascadeParentIds, + }, + }; + }, + {}, + ); const filters = values?.filters; if (filters) { @@ -337,6 +341,7 @@ function FiltersConfigModal({ } }); } + return updatedFilterConfigMap; }; const handleErroredFilters = useCallback(() => { @@ -375,9 +380,9 @@ function FiltersConfigModal({ handleErroredFilters(); if (values) { - cleanDeletedParents(values); + const updatedFilterConfigMap = cleanDeletedParents(values); createHandleSave( - filterConfigMap, + updatedFilterConfigMap, orderedFilters, removedFilters, onSave, From 36897125d80ddd64d27643eb79316c73eb32b20b Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 19 Jul 2023 16:18:07 -0700 Subject: [PATCH 2/3] remove space --- .../FiltersConfigModal/FiltersConfigModal.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx index 5e6f1f5658744..3070e6d0bceda 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx @@ -353,7 +353,7 @@ test('filters are draggable', async () => { adds a new time grain filter type with all fields filled collapsible controls opens by default when it is checked advanced section opens by default when it has an option checked - disables the default value when default to first item is checked + disables the default value when default to first item is checked changes the default value options when the column changes switches to configuration tab when validation fails displays cancel message when there are pending operations From b2903416d2259937e6c34a57fdd082568cef0083 Mon Sep 17 00:00:00 2001 From: John Bodley Date: Wed, 19 Jul 2023 17:17:41 -0700 Subject: [PATCH 3/3] Add migration --- ...280_cleanup_erroneous_parent_filter_ids.py | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 superset/migrations/versions/2023-07-19_16-48_a23c6f8b1280_cleanup_erroneous_parent_filter_ids.py diff --git a/superset/migrations/versions/2023-07-19_16-48_a23c6f8b1280_cleanup_erroneous_parent_filter_ids.py b/superset/migrations/versions/2023-07-19_16-48_a23c6f8b1280_cleanup_erroneous_parent_filter_ids.py new file mode 100644 index 0000000000000..17ad592b22fdc --- /dev/null +++ b/superset/migrations/versions/2023-07-19_16-48_a23c6f8b1280_cleanup_erroneous_parent_filter_ids.py @@ -0,0 +1,82 @@ +# 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. +"""cleanup erroneous parent filter IDs + +Revision ID: a23c6f8b1280 +Revises: 863adcf72773 +Create Date: 2023-07-19 16:48:05.571149 + +""" + +# revision identifiers, used by Alembic. +revision = "a23c6f8b1280" +down_revision = "863adcf72773" + + +import json +import logging + +from alembic import op +from sqlalchemy import Column, Integer, Text +from sqlalchemy.ext.declarative import declarative_base + +from superset import db + +Base = declarative_base() + + +class Dashboard(Base): + __tablename__ = "dashboards" + + id = Column(Integer, primary_key=True) + json_metadata = Column(Text) + + +def upgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + + for dashboard in session.query(Dashboard).all(): + if dashboard.json_metadata: + updated = False + + try: + json_metadata = json.loads(dashboard.json_metadata) + + if filters := json_metadata.get("native_filter_configuration"): + filter_ids = {fltr["id"] for fltr in filters} + + for fltr in filters: + for parent_id in fltr["cascadeParentIds"][:]: + if parent_id not in filter_ids: + fltr["cascadeParentIds"].remove(parent_id) + updated = True + + if updated: + dashboard.json_metadata = json.dumps(json_metadata) + + except Exception: + logging.exception( + f"Unable to parse JSON metadata for dashboard {dashboard.id}" + ) + + session.commit() + session.close() + + +def downgrade(): + pass