diff --git a/superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx b/superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx index 9be6a15dd11..4bd716f1f13 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx @@ -228,13 +228,16 @@ const CustomModal = ({ }; let FooterComponent; - if (isValidElement(footer)) { + + // This safely avoids injecting "closeModal" into native elements like
or + if (isValidElement(footer) && typeof footer.type === 'function') // If a footer component is provided inject a closeModal function // so the footer can provide a "close" button if desired FooterComponent = cloneElement(footer, { closeModal: handleOnHide, } as Partial); - } + else FooterComponent = footer; + const modalFooter = isNil(FooterComponent) ? [
+ } + footer={ +
+ + {t('Discard')} + + + {t('Save')} + +
+ } + > + {body} + +); diff --git a/superset-frontend/packages/superset-ui-core/src/components/index.ts b/superset-frontend/packages/superset-ui-core/src/components/index.ts index 6e5348f320d..f7eda5e3332 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/components/index.ts @@ -163,5 +163,6 @@ export * from './Steps'; export * from './Table'; export * from './TableView'; export * from './Tag'; +export * from './UnsavedChangesModal'; export * from './constants'; export * from './Result'; diff --git a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx deleted file mode 100644 index 267b50291ca..00000000000 --- a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx +++ /dev/null @@ -1,279 +0,0 @@ -/** - * 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, screen, userEvent } from 'spec/helpers/testing-library'; -import { - AlteredSliceTag, - alterForComparison, - formatValueHandler, - isEqualish, -} from '.'; -import { defaultProps } from './AlteredSliceTagMocks'; - -const controlsMap = { - b: { type: 'BoundsControl', label: 'Bounds' }, - column_collection: { type: 'CollectionControl', label: 'Collection' }, - metrics: { type: 'MetricsControl', label: 'Metrics' }, - adhoc_filters: { type: 'AdhocFilterControl', label: 'Adhoc' }, - other_control: { type: 'OtherControl', label: 'Other' }, -}; - -test('renders the "Altered" label', () => { - render( - , - ); - - const alteredLabel = screen.getByText('Altered'); - expect(alteredLabel).toBeInTheDocument(); -}); - -test('opens the modal on click', () => { - render( - , - ); - - const alteredLabel = screen.getByText('Altered'); - userEvent.click(alteredLabel); - - const modalTitle = screen.getByText('Chart changes'); - expect(modalTitle).toBeInTheDocument(); -}); - -test('displays the differences in the modal', () => { - render( - , - ); - - const alteredLabel = screen.getByText('Altered'); - userEvent.click(alteredLabel); - - const beforeValue = screen.getByText('1, 2, 3, 4'); - const afterValue = screen.getByText('a, b, c, d'); - expect(beforeValue).toBeInTheDocument(); - expect(afterValue).toBeInTheDocument(); -}); - -test('does not render anything if there are no differences', () => { - render( - , - ); - - const alteredLabel = screen.queryByText('Altered'); - expect(alteredLabel).not.toBeInTheDocument(); -}); - -test('alterForComparison returns null for undefined value', () => { - expect(alterForComparison(undefined)).toBeNull(); -}); - -test('alterForComparison returns null for null value', () => { - expect(alterForComparison(null)).toBeNull(); -}); - -test('alterForComparison returns null for empty string value', () => { - expect(alterForComparison('')).toBeNull(); -}); - -test('alterForComparison returns null for empty array value', () => { - expect(alterForComparison([])).toBeNull(); -}); - -test('alterForComparison returns null for empty object value', () => { - expect(alterForComparison({})).toBeNull(); -}); - -test('alterForComparison returns value for non-empty array', () => { - const value = [1, 2, 3]; - expect(alterForComparison(value)).toEqual(value); -}); - -test('alterForComparison returns value for non-empty object', () => { - const value = { key: 'value' }; - expect(alterForComparison(value)).toEqual(value); -}); - -test('formatValueHandler handles undefined value', () => { - const value = undefined; - const key = 'b'; - const formattedValue = formatValueHandler(value, key, controlsMap); - expect(formattedValue).toBe('N/A'); -}); - -test('formatValueHandler handles null value', () => { - const value = null; - const key = 'b'; - const formattedValue = formatValueHandler(value, key, controlsMap); - expect(formattedValue).toBe('null'); -}); - -test('formatValueHandler returns "[]" for empty filters', () => { - const value = []; - const key = 'adhoc_filters'; - const formattedValue = formatValueHandler(value, key, controlsMap); - expect(formattedValue).toBe('[]'); -}); - -test('formatValueHandler formats filters with array values', () => { - const filters = [ - { - clause: 'WHERE', - comparator: ['1', 'g', '7', 'ho'], - expressionType: 'SIMPLE', - operator: 'IN', - subject: 'a', - }, - { - clause: 'WHERE', - comparator: ['hu', 'ho', 'ha'], - expressionType: 'SIMPLE', - operator: 'NOT IN', - subject: 'b', - }, - ]; - const key = 'adhoc_filters'; - const formattedValue = formatValueHandler(filters, key, controlsMap); - const expected = 'a IN [1, g, 7, ho], b NOT IN [hu, ho, ha]'; - expect(formattedValue).toBe(expected); -}); - -test('formatValueHandler formats filters with string values', () => { - const filters = [ - { - clause: 'WHERE', - comparator: 'gucci', - expressionType: 'SIMPLE', - operator: '==', - subject: 'a', - }, - { - clause: 'WHERE', - comparator: 'moshi moshi', - expressionType: 'SIMPLE', - operator: 'LIKE', - subject: 'b', - }, - ]; - const key = 'adhoc_filters'; - const expected = 'a == gucci, b LIKE moshi moshi'; - const formattedValue = formatValueHandler(filters, key, controlsMap); - expect(formattedValue).toBe(expected); -}); - -test('formatValueHandler formats "Min" and "Max" for BoundsControl', () => { - const value = [1, 2]; - const key = 'b'; - const result = formatValueHandler(value, key, controlsMap); - expect(result).toEqual('Min: 1, Max: 2'); -}); - -test('formatValueHandler formats stringified objects for CollectionControl', () => { - const value = [{ a: 1 }, { b: 2 }]; - const key = 'column_collection'; - const result = formatValueHandler(value, key, controlsMap); - expect(result).toEqual( - `${JSON.stringify(value[0])}, ${JSON.stringify(value[1])}`, - ); -}); - -test('formatValueHandler formats MetricsControl values correctly', () => { - const value = [{ label: 'SUM(Sales)' }, { label: 'Metric2' }]; - const key = 'metrics'; - const result = formatValueHandler(value, key, controlsMap); - expect(result).toEqual('SUM(Sales), Metric2'); -}); - -test('formatValueHandler formats boolean values as string', () => { - const value1 = true; - const value2 = false; - const key = 'b'; - const formattedValue1 = formatValueHandler(value1, key, controlsMap); - const formattedValue2 = formatValueHandler(value2, key, controlsMap); - expect(formattedValue1).toBe('true'); - expect(formattedValue2).toBe('false'); -}); - -test('formatValueHandler formats array values correctly', () => { - const value = [ - { label: 'Label1' }, - { label: 'Label2' }, - 5, - 6, - 7, - 8, - 'hello', - 'goodbye', - ]; - const result = formatValueHandler(value, undefined, controlsMap); - const expected = 'Label1, Label2, 5, 6, 7, 8, hello, goodbye'; - expect(result).toEqual(expected); -}); - -test('formatValueHandler formats string values correctly', () => { - const value = 'test'; - const key = 'other_control'; - const result = formatValueHandler(value, key, controlsMap); - expect(result).toEqual('test'); -}); - -test('formatValueHandler formats number values correctly', () => { - const value = 123; - const key = 'other_control'; - const result = formatValueHandler(value, key, controlsMap); - expect(result).toEqual(123); -}); - -test('formatValueHandler formats object values correctly', () => { - const value = { 1: 2, alpha: 'bravo' }; - const key = 'other_control'; - const expected = '{"1":2,"alpha":"bravo"}'; - const result = formatValueHandler(value, key, controlsMap); - expect(result).toEqual(expected); -}); - -test('isEqualish considers null, undefined, {} and [] as equal', () => { - expect(isEqualish(null, undefined)).toBe(true); - expect(isEqualish(null, [])).toBe(true); - expect(isEqualish(null, {})).toBe(true); - expect(isEqualish(undefined, {})).toBe(true); -}); - -test('isEqualish considers empty strings equal to null', () => { - expect(isEqualish(undefined, '')).toBe(true); - expect(isEqualish(null, '')).toBe(true); -}); - -test('isEqualish considers deeply equal objects equal', () => { - const obj1 = { a: { b: { c: 1 } } }; - const obj2 = { a: { b: { c: 1 } } }; - expect(isEqualish('', '')).toBe(true); - expect(isEqualish(obj1, obj2)).toBe(true); - // Actually not equal - expect(isEqualish({ a: 1, b: 2, z: 9 }, { a: 1, b: 2, c: 3 })).toBe(false); -}); diff --git a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.tsx b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.tsx new file mode 100644 index 00000000000..515b6d9fc0b --- /dev/null +++ b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.tsx @@ -0,0 +1,97 @@ +/** + * 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, screen, userEvent } from 'spec/helpers/testing-library'; +import { QueryFormData } from '@superset-ui/core'; +import { AlteredSliceTag } from '.'; +import { defaultProps, expectedDiffs } from './AlteredSliceTagMocks'; + +test('renders the "Altered" label', () => { + render( + , + ); + + const alteredLabel: HTMLElement = screen.getByText('Altered'); + expect(alteredLabel).toBeInTheDocument(); +}); + +test('opens the modal on click', () => { + render( + , + ); + + const alteredLabel: HTMLElement = screen.getByText('Altered'); + userEvent.click(alteredLabel); + + const modalTitle: HTMLElement = screen.getByText('Chart changes'); + + expect(modalTitle).toBeInTheDocument(); +}); + +test('displays the differences in the modal', () => { + render( + , + ); + + const alteredLabel: HTMLElement = screen.getByText('Altered'); + userEvent.click(alteredLabel); + + const beforeValue: HTMLElement = screen.getByText('1, 2, 3, 4'); + const afterValue: HTMLElement = screen.getByText('a, b, c, d'); + + expect(beforeValue).toBeInTheDocument(); + expect(afterValue).toBeInTheDocument(); +}); + +test('does not render anything if there are no differences', () => { + render( + , + ); + + const alteredLabel: HTMLElement | null = screen.queryByText('Altered'); + + expect(alteredLabel).not.toBeInTheDocument(); +}); + +test('does not render the altered label if diffs is empty', () => { + render( + , + ); + + expect(screen.queryByText('Altered')).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTagMocks.ts b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTagMocks.ts index c307eace575..1f54bdfbdea 100644 --- a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTagMocks.ts +++ b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTagMocks.ts @@ -18,10 +18,16 @@ */ import { QueryFormData } from '@superset-ui/core'; import { ControlPanelConfig } from '@superset-ui/chart-controls'; -import type { DiffType, RowType } from './types'; +import type { DiffType } from 'src/types/DiffType'; +import { getChartFormDiffs } from 'src/utils/getChartFormDiffs'; +import type { RowType } from './types'; -export const defaultProps: Record> = { +export const defaultProps: { + origFormData: QueryFormData; + currentFormData: QueryFormData; +} = { origFormData: { + datasource: '123__table', viz_type: 'altered_slice_tag_spec', adhoc_filters: [ { @@ -40,7 +46,10 @@ export const defaultProps: Record> = { never: 5, ever: { a: 'b', c: 'd' }, }, + currentFormData: { + datasource: '123__table', + viz_type: 'altered_slice_tag_spec', adhoc_filters: [ { clause: 'WHERE', @@ -106,6 +115,7 @@ export const expectedDiffs: Record = { after: { x: 'y', z: 'z' }, }, }; + export const expectedRows: RowType[] = [ { control: 'Fake Filters', @@ -131,6 +141,7 @@ export const expectedRows: RowType[] = [ after: '{"x":"y","z":"z"}', }, ]; + export const fakePluginControls: ControlPanelConfig = { controlPanelSections: [ { @@ -175,3 +186,6 @@ export const fakePluginControls: ControlPanelConfig = { }, ], }; + +export const createDiffs = (original: QueryFormData, current: QueryFormData) => + getChartFormDiffs(original, current); diff --git a/superset-frontend/src/components/AlteredSliceTag/index.tsx b/superset-frontend/src/components/AlteredSliceTag/index.tsx index 43fd0d7a4f9..b9025fd2d26 100644 --- a/superset-frontend/src/components/AlteredSliceTag/index.tsx +++ b/superset-frontend/src/components/AlteredSliceTag/index.tsx @@ -16,13 +16,10 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useMemo, useState, FC } from 'react'; - -import { isEqual, isEmpty } from 'lodash'; +import { useEffect, useMemo, useState, FC } from 'react'; +import { isEmpty } from 'lodash'; import { t } from '@superset-ui/core'; -import { sanitizeFormData } from 'src/explore/exploreUtils/formData'; import getControlsForVizType from 'src/utils/getControlsForVizType'; -import { safeStringify } from 'src/utils/safeStringify'; import { Label, Icons, @@ -30,130 +27,21 @@ import { ModalTrigger, TableView, } from '@superset-ui/core/components'; -import type { - AlteredSliceTagProps, - ControlMap, - DiffItemType, - DiffType, - FilterItemType, - RowType, -} from './types'; - -export const alterForComparison = ( - value?: string | null | [], -): string | null => { - // Treat `null`, `undefined`, and empty strings as equivalent - if (value === undefined || value === null || value === '') { - return null; - } - // Treat empty arrays and objects as equivalent to null - if (Array.isArray(value) && value.length === 0) { - return null; - } - if (typeof value === 'object' && Object.keys(value).length === 0) { - return null; - } - return value; -}; - -export const formatValueHandler = ( - value: DiffItemType, - key: string, - controlsMap: ControlMap, -): string | number => { - if (value === undefined) { - return 'N/A'; - } - if (value === null) { - return 'null'; - } - if (typeof value === 'boolean') { - return value ? 'true' : 'false'; - } - if (controlsMap[key]?.type === 'AdhocFilterControl' && Array.isArray(value)) { - if (!value.length) { - return '[]'; - } - return value - .map((v: FilterItemType) => { - const filterVal = - v.comparator && v.comparator.constructor === Array - ? `[${v.comparator.join(', ')}]` - : v.comparator; - return `${v.subject} ${v.operator} ${filterVal}`; - }) - .join(', '); - } - if (controlsMap[key]?.type === 'BoundsControl' && Array.isArray(value)) { - return `Min: ${value[0]}, Max: ${value[1]}`; - } - if (controlsMap[key]?.type === 'CollectionControl' && Array.isArray(value)) { - return value.map((v: FilterItemType) => safeStringify(v)).join(', '); - } - if ( - controlsMap[key]?.type === 'MetricsControl' && - value.constructor === Array - ) { - const formattedValue = value.map((v: FilterItemType) => v?.label ?? v); - return formattedValue.length ? formattedValue.join(', ') : '[]'; - } - if (Array.isArray(value)) { - const formattedValue = value.map((v: FilterItemType) => v?.label ?? v); - return formattedValue.length ? formattedValue.join(', ') : '[]'; - } - if (typeof value === 'string' || typeof value === 'number') { - return value; - } - return safeStringify(value); -}; - -export const getRowsFromDiffs = ( - diffs: { [key: string]: DiffType }, - controlsMap: ControlMap, -): RowType[] => - Object.entries(diffs).map(([key, diff]) => ({ - control: controlsMap[key]?.label || key, - before: formatValueHandler(diff.before, key, controlsMap), - after: formatValueHandler(diff.after, key, controlsMap), - })); - -export const isEqualish = (val1: string, val2: string): boolean => - isEqual(alterForComparison(val1), alterForComparison(val2)); +import type { AlteredSliceTagProps, ControlMap, RowType } from './types'; +import { getRowsFromDiffs } from './utils'; export const AlteredSliceTag: FC = props => { const [rows, setRows] = useState([]); const [hasDiffs, setHasDiffs] = useState(false); - const getDiffs = useCallback(() => { - // Returns all properties that differ in the - // current form data and the saved form data - const ofd = sanitizeFormData(props.origFormData); - const cfd = sanitizeFormData(props.currentFormData); - - const fdKeys = Object.keys(cfd); - const diffs: { [key: string]: DiffType } = {}; - fdKeys.forEach(fdKey => { - if (!ofd[fdKey] && !cfd[fdKey]) { - return; - } - if (['filters', 'having', 'where'].includes(fdKey)) { - return; - } - if (!isEqualish(ofd[fdKey], cfd[fdKey])) { - diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] }; - } - }); - return diffs; - }, [props.currentFormData, props.origFormData]); - useEffect(() => { - const diffs = getDiffs(); const controlsMap = getControlsForVizType( props.origFormData?.viz_type, ) as ControlMap; - setRows(getRowsFromDiffs(diffs, controlsMap)); - setHasDiffs(!isEmpty(diffs)); - }, [getDiffs, props.origFormData?.viz_type]); + + setRows(getRowsFromDiffs(props.diffs, controlsMap)); + setHasDiffs(!isEmpty(props.diffs)); + }, [props.diffs, props.origFormData?.viz_type]); const modalBody = useMemo(() => { const columns = [ diff --git a/superset-frontend/src/components/AlteredSliceTag/types.ts b/superset-frontend/src/components/AlteredSliceTag/types.ts index e1f61086088..44c6796e361 100644 --- a/superset-frontend/src/components/AlteredSliceTag/types.ts +++ b/superset-frontend/src/components/AlteredSliceTag/types.ts @@ -17,8 +17,11 @@ * under the License. */ import type { QueryFormData } from '@superset-ui/core'; +import { DiffType } from 'src/types/DiffType'; export interface AlteredSliceTagProps { + className?: string; + diffs: Record; origFormData: QueryFormData; currentFormData: QueryFormData; } @@ -30,29 +33,6 @@ export interface ControlMap { }; } -export type FilterItemType = { - comparator?: string | string[]; - subject: string; - operator: string; - label?: string; -}; - -export type DiffItemType< - T = FilterItemType | number | string | Record, -> = - | T[] - | boolean - | number - | string - | Record - | null - | undefined; - -export type DiffType = { - before: DiffItemType; - after: DiffItemType; -}; - export type RowType = { before: string | number; after: string | number; diff --git a/superset-frontend/src/components/AlteredSliceTag/utils/index.ts b/superset-frontend/src/components/AlteredSliceTag/utils/index.ts new file mode 100644 index 00000000000..482a4332621 --- /dev/null +++ b/superset-frontend/src/components/AlteredSliceTag/utils/index.ts @@ -0,0 +1,107 @@ +/** + * 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 { DiffItemType, DiffType, FilterItemType } from 'src/types/DiffType'; +import { safeStringify } from 'src/utils/safeStringify'; +import { ControlMap, RowType } from '../types'; + +export const alterForComparison = (value?: unknown): unknown => { + // Treat `null`, `undefined`, and empty strings as equivalent + if (value === undefined || value === null || value === '') return null; + + // Treat empty arrays and objects as equivalent to null + if (Array.isArray(value) && value.length === 0) return null; + + if (typeof value === 'object' && Object.keys(value).length === 0) return null; + + return value; +}; + +export const formatValueHandler = ( + value: DiffItemType, + key = '', + controlsMap: ControlMap, +): string | number => { + if (value === undefined) return 'N/A'; + + if (value === null) return 'null'; + + if (typeof value === 'boolean') return value ? 'true' : 'false'; + + if (controlsMap[key]?.type === 'AdhocFilterControl' && Array.isArray(value)) { + if (!value.length) return '[]'; + + return value + .map((v: FilterItemType): string => { + const filterVal: string | string[] | undefined = + v.comparator && v.comparator.constructor === Array + ? `[${v.comparator.join(', ')}]` + : v.comparator; + return `${v.subject} ${v.operator} ${filterVal}`; + }) + .join(', '); + } + + if (controlsMap[key]?.type === 'BoundsControl' && Array.isArray(value)) + return `Min: ${value[0]}, Max: ${value[1]}`; + + if (controlsMap[key]?.type === 'CollectionControl' && Array.isArray(value)) + return value + .map((v: FilterItemType): string => safeStringify(v)) + .join(', '); + + if ( + controlsMap[key]?.type === 'MetricsControl' && + value.constructor === Array + ) { + const formattedValue: (string | FilterItemType)[] = value.map( + (v: FilterItemType): string | FilterItemType => v?.label ?? v, + ); + return formattedValue.length ? formattedValue.join(', ') : '[]'; + } + + if (Array.isArray(value)) { + const formattedValue = value.map((v: any) => { + if ( + typeof v === 'object' && + v !== null && + 'label' in v && + typeof v.label === 'string' + ) + return v.label; + + return String(v); + }); + + return formattedValue.length ? formattedValue.join(', ') : '[]'; + } + + if (typeof value === 'string' || typeof value === 'number') return value; + + return safeStringify(value); +}; + +export const getRowsFromDiffs = ( + diffs: { [key: string]: DiffType }, + controlsMap: ControlMap, +): RowType[] => + Object.entries(diffs).map(([key, diff]: [string, DiffType]) => ({ + control: controlsMap[key]?.label || key, + before: formatValueHandler(diff.before, key, controlsMap), + after: formatValueHandler(diff.after, key, controlsMap), + })); diff --git a/superset-frontend/src/components/AlteredSliceTag/utils/utils.test.ts b/superset-frontend/src/components/AlteredSliceTag/utils/utils.test.ts new file mode 100644 index 00000000000..861686e7c46 --- /dev/null +++ b/superset-frontend/src/components/AlteredSliceTag/utils/utils.test.ts @@ -0,0 +1,291 @@ +/** + * 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 { alterForComparison, formatValueHandler, getRowsFromDiffs } from '.'; +import { RowType } from '../types'; + +describe('alterForComparison', () => { + it('returns null for undefined value', () => { + expect(alterForComparison(undefined)).toBeNull(); + }); + + it('returns null for null value', () => { + expect(alterForComparison(null)).toBeNull(); + }); + + it('returns null for empty string value', () => { + expect(alterForComparison('')).toBeNull(); + }); + + it('returns null for empty array value', () => { + expect(alterForComparison([])).toBeNull(); + }); + + it('returns null for empty object value', () => { + expect(alterForComparison({})).toBeNull(); + }); + + it('returns value for non-empty array', () => { + const value = [1, 2, 3]; + expect(alterForComparison(value)).toEqual(value); + }); + + it('returns value for non-empty object', () => { + const value = { key: 'value' }; + expect(alterForComparison(value)).toEqual(value); + }); +}); + +describe('formatValueHandler', () => { + const controlsMap = { + b: { type: 'BoundsControl', label: 'Bounds' }, + column_collection: { type: 'CollectionControl', label: 'Collection' }, + metrics: { type: 'MetricsControl', label: 'Metrics' }, + adhoc_filters: { type: 'AdhocFilterControl', label: 'Adhoc' }, + other_control: { type: 'OtherControl', label: 'Other' }, + }; + + it('handles undefined value', () => { + const value = undefined; + const key = 'b'; + + const formattedValue: string | number = formatValueHandler( + value, + key, + controlsMap, + ); + + expect(formattedValue).toBe('N/A'); + }); + + it('handles null value', () => { + const value = null; + const key = 'b'; + + const formattedValue: string | number = formatValueHandler( + value, + key, + controlsMap, + ); + + expect(formattedValue).toBe('null'); + }); + + it('returns "[]" for empty filters', () => { + const value: unknown[] = []; + const key = 'adhoc_filters'; + + const formattedValue: string | number = formatValueHandler( + value, + key, + controlsMap, + ); + + expect(formattedValue).toBe('[]'); + }); + + it('formats filters with array values', () => { + const filters = [ + { + clause: 'WHERE', + comparator: ['1', 'g', '7', 'ho'], + expressionType: 'SIMPLE', + operator: 'IN', + subject: 'a', + }, + { + clause: 'WHERE', + comparator: ['hu', 'ho', 'ha'], + expressionType: 'SIMPLE', + operator: 'NOT IN', + subject: 'b', + }, + ]; + const key = 'adhoc_filters'; + + const expected = 'a IN [1, g, 7, ho], b NOT IN [hu, ho, ha]'; + const formattedValue: string | number = formatValueHandler( + filters, + key, + controlsMap, + ); + + expect(formattedValue).toBe(expected); + }); + + it('formats filters with string values', () => { + const filters = [ + { + clause: 'WHERE', + comparator: 'gucci', + expressionType: 'SIMPLE', + operator: '==', + subject: 'a', + }, + { + clause: 'WHERE', + comparator: 'moshi moshi', + expressionType: 'SIMPLE', + operator: 'LIKE', + subject: 'b', + }, + ]; + + const key = 'adhoc_filters'; + const expected = 'a == gucci, b LIKE moshi moshi'; + const formattedValue: string | number = formatValueHandler( + filters, + key, + controlsMap, + ); + + expect(formattedValue).toBe(expected); + }); + + it('formats "Min" and "Max" for BoundsControl', () => { + const value: number[] = [1, 2]; + const key = 'b'; + + const result: string | number = formatValueHandler(value, key, controlsMap); + + expect(result).toEqual('Min: 1, Max: 2'); + }); + + it('formats stringified objects for CollectionControl', () => { + const value = [{ a: 1 }, { b: 2 }]; + const key = 'column_collection'; + + const result: string | number = formatValueHandler(value, key, controlsMap); + + expect(result).toEqual( + `${JSON.stringify(value[0])}, ${JSON.stringify(value[1])}`, + ); + }); + + it('formats MetricsControl values correctly', () => { + const value = [{ label: 'SUM(Sales)' }, { label: 'Metric2' }]; + const key = 'metrics'; + + const result: string | number = formatValueHandler(value, key, controlsMap); + + expect(result).toEqual('SUM(Sales), Metric2'); + }); + + it('formats boolean values as string', () => { + const value1 = true; + const value2 = false; + const key = 'b'; + + const formattedValue1: string | number = formatValueHandler( + value1, + key, + controlsMap, + ); + const formattedValue2: string | number = formatValueHandler( + value2, + key, + controlsMap, + ); + + expect(formattedValue1).toBe('true'); + expect(formattedValue2).toBe('false'); + }); + + it('formats array values correctly', () => { + const value = [ + { label: 'Label1' }, + { label: 'Label2' }, + 5, + 6, + 7, + 8, + 'hello', + 'goodbye', + ]; + + const expected = 'Label1, Label2, 5, 6, 7, 8, hello, goodbye'; + const result: string | number = formatValueHandler( + value, + undefined, + controlsMap, + ); + + expect(result).toEqual(expected); + }); + + it('formats string values correctly', () => { + const value = 'test'; + const key = 'other_control'; + + const result: string | number = formatValueHandler(value, key, controlsMap); + + expect(result).toEqual('test'); + }); + + it('formats number values correctly', () => { + const value = 123; + const key = 'other_control'; + + const result: string | number = formatValueHandler(value, key, controlsMap); + + expect(result).toEqual(123); + }); + + it('formats object values correctly', () => { + const value = { 1: 2, alpha: 'bravo' }; + const key = 'other_control'; + const expected = '{"1":2,"alpha":"bravo"}'; + + const result: string | number = formatValueHandler(value, key, controlsMap); + + expect(result).toEqual(expected); + }); +}); + +describe('getRowsFromDiffs', () => { + it('returns formatted rows for diffs', () => { + const diffs = { + metric: { before: [{ label: 'old' }], after: [{ label: 'new' }] }, + limit: { before: 10, after: 20 }, + }; + + const controlsMap = { + metric: { label: 'Metric', type: 'MetricsControl' }, + limit: { label: 'Row Limit', type: 'TextControl' }, + }; + + const rows: RowType[] = getRowsFromDiffs(diffs, controlsMap); + + expect(rows).toEqual([ + { control: 'Metric', before: 'old', after: 'new' }, + { control: 'Row Limit', before: 10, after: 20 }, + ]); + }); + + it('falls back to key if label is missing', () => { + const diffs = { + unknown: { before: 'a', after: 'b' }, + }; + + const controlsMap = {}; + const rows: RowType[] = getRowsFromDiffs(diffs, controlsMap); + + expect(rows).toEqual([{ control: 'unknown', before: 'a', after: 'b' }]); + }); +}); diff --git a/superset-frontend/src/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx index ac567ad6fcf..36ee0c7696a 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -17,11 +17,13 @@ * under the License. */ import * as redux from 'redux'; +import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt'; import { render, screen, fireEvent, userEvent, + within, } from 'spec/helpers/testing-library'; import fetchMock from 'fetch-mock'; import { getExtensionsRegistry, JsonObject } from '@superset-ui/core'; @@ -166,6 +168,10 @@ const onRefresh = jest.fn(); const dashboardInfoChanged = jest.fn(); const dashboardTitleChanged = jest.fn(); +jest.mock('src/hooks/useUnsavedChangesPrompt', () => ({ + useUnsavedChangesPrompt: jest.fn(), +})); + beforeAll(() => { jest.spyOn(redux, 'bindActionCreators').mockImplementation(() => ({ addSuccessToast, @@ -195,9 +201,14 @@ beforeAll(() => { beforeEach(() => { jest.clearAllMocks(); -}); -beforeEach(() => { + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + }); + window.history.pushState({}, 'Test page', '/dashboard?standalone=1'); }); @@ -484,3 +495,91 @@ test('should render MetadataBar when not in edit mode and not embedded', () => { screen.getByText(state.dashboardInfo.changed_on_delta_humanized), ).toBeInTheDocument(); }); + +test('should show UnsavedChangesModal when there are unsaved changes and user tries to navigate', async () => { + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + }); + + setup({ ...editableState }); + + const modalTitle: HTMLElement = await screen.findByText( + 'Save changes to your dashboard?', + ); + + const modalBody: HTMLElement = await screen.findByText( + "If you don't save, changes will be lost.", + ); + + expect(modalTitle).toBeInTheDocument(); + expect(modalBody).toBeInTheDocument(); +}); + +test('should call handleSaveAndCloseModal when Save is clicked in UnsavedChangesModal', async () => { + const handleSaveAndCloseModal = jest.fn(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal, + }); + + setup({ ...editableState }); + + const modal: HTMLElement = await screen.findByRole('dialog'); + const saveButton: HTMLElement = within(modal).getByRole('button', { + name: /save/i, + }); + + userEvent.click(saveButton); + + expect(handleSaveAndCloseModal).toHaveBeenCalled(); +}); + +test('should call handleConfirmNavigation when user confirms navigation in UnsavedChangesModal', async () => { + const handleConfirmNavigation = jest.fn(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal: jest.fn(), + handleConfirmNavigation, + handleSaveAndCloseModal: jest.fn(), + }); + + setup({ ...editableState }); + + const modal: HTMLElement = await screen.findByRole('dialog'); + const discardButton: HTMLElement = within(modal).getByRole('button', { + name: /discard/i, + }); + + userEvent.click(discardButton); + + expect(handleConfirmNavigation).toHaveBeenCalled(); +}); + +test('should call setShowUnsavedChangesModal(false) on cancel', async () => { + const setShowModal = jest.fn(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal, + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + }); + + setup({ ...editableState }); + + const modal: HTMLElement = await screen.findByRole('dialog'); + const closeButton: HTMLElement = within(modal).getByRole('button', { + name: /close/i, + }); + + userEvent.click(closeButton); + + expect(setShowModal).toHaveBeenCalledWith(false); +}); diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index b677a8c5ece..f4535c4de39 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -36,7 +36,12 @@ import { LOG_ACTIONS_TOGGLE_EDIT_DASHBOARD, } from 'src/logger/LogUtils'; import { Icons } from '@superset-ui/core/components/Icons'; -import { Button, Tooltip, DeleteModal } from '@superset-ui/core/components'; +import { + Button, + Tooltip, + DeleteModal, + UnsavedChangesModal, +} from '@superset-ui/core/components'; import { findPermission } from 'src/utils/findPermission'; import { safeStringify } from 'src/utils/safeStringify'; import PublishedStatus from 'src/dashboard/components/PublishedStatus'; @@ -54,6 +59,7 @@ import setPeriodicRunner, { import ReportModal from 'src/features/reports/ReportModal'; import { deleteActiveReport } from 'src/features/reports/ReportModal/actions'; import { PageHeaderWithActions } from '@superset-ui/core/components/PageHeaderWithActions'; +import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt'; import DashboardEmbedModal from '../EmbeddedModal'; import OverwriteConfirm from '../OverwriteConfirm'; import { @@ -460,6 +466,16 @@ const Header = () => { slug, ]); + const { + showModal: showUnsavedChangesModal, + setShowModal: setShowUnsavedChangesModal, + handleConfirmNavigation, + handleSaveAndCloseModal, + } = useUnsavedChangesPrompt({ + hasUnsavedChanges, + onSave: overwriteDashboard, + }); + const showPropertiesModal = useCallback(() => { setShowingPropertiesModal(true); }, []); @@ -816,6 +832,15 @@ const Header = () => { } `} /> + + setShowUnsavedChangesModal(false)} + onConfirmNavigation={handleConfirmNavigation} + handleSave={handleSaveAndCloseModal} + /> ); }; 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 4f993d0158a..886aee28d6f 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx @@ -305,12 +305,19 @@ test.skip('validates the default value', async () => { }); test('validates the pre-filter value', async () => { + jest.useFakeTimers(); + defaultRender(); + userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX)); userEvent.click(getCheckbox(PRE_FILTER_REGEX)); - expect( - await screen.findByText(PRE_FILTER_REQUIRED_REGEX), - ).toBeInTheDocument(); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + + await waitFor(() => { + expect(screen.getByText(PRE_FILTER_REQUIRED_REGEX)).toBeInTheDocument(); + }); }); // eslint-disable-next-line jest/no-disabled-tests diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx index 9888493cf9c..6f083fc9bf7 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx @@ -23,6 +23,7 @@ import { screen, userEvent, waitFor, + within, } from 'spec/helpers/testing-library'; import fetchMock from 'fetch-mock'; import * as chartAction from 'src/components/Chart/chartAction'; @@ -30,6 +31,7 @@ import * as saveModalActions from 'src/explore/actions/saveModalActions'; import * as downloadAsImage from 'src/utils/downloadAsImage'; import * as exploreUtils from 'src/explore/exploreUtils'; import { FeatureFlag, VizType } from '@superset-ui/core'; +import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt'; import ExploreHeader from '.'; const chartEndpoint = 'glob:*api/v1/chart/*'; @@ -40,6 +42,10 @@ window.featureFlags = { [FeatureFlag.EmbeddableCharts]: true, }; +jest.mock('src/hooks/useUnsavedChangesPrompt', () => ({ + useUnsavedChangesPrompt: jest.fn(), +})); + const createProps = (additionalProps = {}) => ({ chart: { id: 1, @@ -134,6 +140,18 @@ fetchMock.post( describe('ExploreChartHeader', () => { jest.setTimeout(15000); // ✅ Applies to all tests in this suite + beforeEach(() => { + jest.clearAllMocks(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave: jest.fn(), + }); + }); + test('Cancelling changes to the properties should reset previous properties', async () => { const props = createProps(); render(, { useRedux: true }); @@ -201,35 +219,173 @@ describe('ExploreChartHeader', () => { }); test('Save chart', async () => { - const setSaveChartModalVisibility = jest.spyOn( + const setSaveChartModalVisibilitySpy = jest.spyOn( saveModalActions, 'setSaveChartModalVisibility', ); + + const setSaveChartModalVisibilityMock = + setSaveChartModalVisibilitySpy as jest.Mock; + + const triggerManualSave = jest.fn(() => { + setSaveChartModalVisibilityMock(true); + }); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave, + }); + const props = createProps(); render(, { useRedux: true }); - expect(await screen.findByText('Save')).toBeInTheDocument(); - userEvent.click(screen.getByText('Save')); - expect(setSaveChartModalVisibility).toHaveBeenCalledWith(true); - setSaveChartModalVisibility.mockClear(); + + const saveButton: HTMLElement = await screen.findByRole('button', { + name: /save/i, + }); + + userEvent.click(saveButton); + + expect(triggerManualSave).toHaveBeenCalled(); + expect(setSaveChartModalVisibilityMock).toHaveBeenCalledWith(true); + + setSaveChartModalVisibilityMock.mockClear(); }); test('Save disabled', async () => { - const setSaveChartModalVisibility = jest.spyOn( - saveModalActions, - 'setSaveChartModalVisibility', - ); + const triggerManualSave = jest.fn(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave, + }); + const props = createProps(); render(, { useRedux: true }); - expect(await screen.findByText('Save')).toBeInTheDocument(); - userEvent.click(screen.getByText('Save')); - expect(setSaveChartModalVisibility).not.toHaveBeenCalled(); - setSaveChartModalVisibility.mockClear(); + + const saveButton: HTMLElement = await screen.findByRole('button', { + name: /save/i, + }); + + expect(saveButton).toBeDisabled(); + + userEvent.click(saveButton); + + expect(triggerManualSave).not.toHaveBeenCalled(); + }); + + test('should render UnsavedChangesModal when showModal is true', async () => { + const props = createProps(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave: jest.fn(), + }); + + render(, { useRedux: true }); + + expect(await screen.findByRole('dialog')).toBeInTheDocument(); + expect( + await screen.findByText('Save changes to your chart?'), + ).toBeInTheDocument(); + expect( + await screen.findByText("If you don't save, changes will be lost."), + ).toBeInTheDocument(); + }); + + test('should call handleSaveAndCloseModal when clicking Save in UnsavedChangesModal', async () => { + const handleSaveAndCloseModal = jest.fn(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal, + triggerManualSave: jest.fn(), + }); + + const props = createProps(); + render(, { useRedux: true }); + + const modal: HTMLElement = await screen.findByRole('dialog'); + const saveButton: HTMLElement = within(modal).getByRole('button', { + name: /save/i, + }); + + userEvent.click(saveButton); + + expect(handleSaveAndCloseModal).toHaveBeenCalled(); + }); + + test('should call handleConfirmNavigation when clicking Discard in UnsavedChangesModal', async () => { + const handleConfirmNavigation = jest.fn(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal: jest.fn(), + handleConfirmNavigation, + handleSaveAndCloseModal: jest.fn(), + triggerManualSave: jest.fn(), + }); + + const props = createProps(); + render(, { useRedux: true }); + + const modal: HTMLElement = await screen.findByRole('dialog'); + const discardButton: HTMLElement = within(modal).getByRole('button', { + name: /discard/i, + }); + + userEvent.click(discardButton); + + expect(handleConfirmNavigation).toHaveBeenCalled(); + }); + + test('should call setShowModal(false) when clicking close button in UnsavedChangesModal', async () => { + const setShowModal = jest.fn(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: true, + setShowModal, + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave: jest.fn(), + }); + + const props = createProps(); + render(, { useRedux: true }); + + const closeButton: HTMLElement = await screen.findByRole('button', { + name: /close/i, + }); + + userEvent.click(closeButton); + + expect(setShowModal).toHaveBeenCalledWith(false); }); }); describe('Additional actions tests', () => { jest.setTimeout(15000); // ✅ Applies to all tests in this suite + beforeEach(() => { + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave: jest.fn(), + }); + }); + test('Should render a button', async () => { const props = createProps(); render(, { useRedux: true }); @@ -356,6 +512,14 @@ describe('Additional actions tests', () => { beforeEach(() => { spyDownloadAsImage = sinon.spy(downloadAsImage, 'default'); spyExportChart = sinon.spy(exploreUtils, 'exportChart'); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + triggerManualSave: jest.fn(), + }); }); afterEach(async () => { diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 2492d94a2e6..f90f37c49c4 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -16,11 +16,16 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; import { useHistory } from 'react-router-dom'; import { useDispatch } from 'react-redux'; import PropTypes from 'prop-types'; -import { Tooltip, Button, DeleteModal } from '@superset-ui/core/components'; +import { + Tooltip, + Button, + DeleteModal, + UnsavedChangesModal, +} from '@superset-ui/core/components'; import { AlteredSliceTag } from 'src/components'; import { css, logging, SupersetClient, t } from '@superset-ui/core'; import { chartPropShape } from 'src/dashboard/util/propShapes'; @@ -32,6 +37,8 @@ import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalAction import { applyColors, resetColors } from 'src/utils/colorScheme'; import ReportModal from 'src/features/reports/ReportModal'; import { deleteActiveReport } from 'src/features/reports/ReportModal/actions'; +import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt'; +import { getChartFormDiffs } from 'src/utils/getChartFormDiffs'; import { useExploreAdditionalActionsMenu } from '../useExploreAdditionalActionsMenu'; import { useExploreMetadataBar } from './useExploreMetadataBar'; @@ -50,6 +57,7 @@ const propTypes = { timeout: PropTypes.number, chart: chartPropShape, saveDisabled: PropTypes.bool, + isSaveModalVisible: PropTypes.bool, }; const saveButtonStyles = theme => css` @@ -83,13 +91,16 @@ export const ExploreChartHeader = ({ sliceName, saveDisabled, metadata, + isSaveModalVisible, }) => { const dispatch = useDispatch(); const { latestQueryFormData, sliceFormData } = chart; const [isPropertiesModalOpen, setIsPropertiesModalOpen] = useState(false); const [isReportModalOpen, setIsReportModalOpen] = useState(false); const [currentReportDeleting, setCurrentReportDeleting] = useState(null); - const updateCategoricalNamespace = async () => { + const [shouldForceCloseModal, setShouldForceCloseModal] = useState(false); + + const updateCategoricalNamespace = useCallback(async () => { const { dashboards } = metadata || {}; const dashboard = dashboardId && dashboards && dashboards.find(d => d.id === dashboardId); @@ -117,11 +128,11 @@ export const ExploreChartHeader = ({ logging.info(t('Unable to retrieve dashboard colors')); } } - }; + }, [dashboardColorScheme, dashboardId, metadata]); useEffect(() => { updateCategoricalNamespace(); - }, []); + }, [updateCategoricalNamespace]); const openPropertiesModal = () => { setIsPropertiesModalOpen(true); @@ -139,10 +150,6 @@ export const ExploreChartHeader = ({ setIsReportModalOpen(false); }; - const showModal = useCallback(() => { - dispatch(setSaveChartModalVisibility(true)); - }, [dispatch]); - const updateSlice = useCallback( slice => { dispatch(sliceUpdated(slice)); @@ -179,8 +186,53 @@ export const ExploreChartHeader = ({ ); const metadataBar = useExploreMetadataBar(metadata, slice); - const oldSliceName = slice?.slice_name; + + const originalFormData = useMemo(() => { + if (!sliceFormData) return {}; + return { + ...sliceFormData, + chartTitle: oldSliceName, + }; + }, [sliceFormData, oldSliceName]); + + const currentFormData = useMemo( + () => ({ ...formData, chartTitle: sliceName }), + [formData, sliceName], + ); + + const formDiffs = useMemo( + () => getChartFormDiffs(originalFormData, currentFormData), + [originalFormData, currentFormData], + ); + + const { + showModal: showUnsavedChangesModal, + setShowModal: setShowUnsavedChangesModal, + handleConfirmNavigation, + handleSaveAndCloseModal, + triggerManualSave, + } = useUnsavedChangesPrompt({ + hasUnsavedChanges: Object.keys(formDiffs).length > 0, + onSave: () => dispatch(setSaveChartModalVisibility(true)), + isSaveModalVisible, + manualSaveOnUnsavedChanges: true, + }); + + const showModal = useCallback(() => { + triggerManualSave(); + }, [triggerManualSave]); + + useEffect(() => { + if (!isSaveModalVisible) setShouldForceCloseModal(true); + }, [isSaveModalVisible, setShowUnsavedChangesModal]); + + useEffect(() => { + if (!showUnsavedChangesModal && shouldForceCloseModal) { + setShouldForceCloseModal(false); + } + }, [showUnsavedChangesModal, shouldForceCloseModal]); + return ( <> ) : null} {metadataBar} @@ -286,6 +336,15 @@ export const ExploreChartHeader = ({ title={t('Delete Report?')} /> )} + + setShowUnsavedChangesModal(false)} + onConfirmNavigation={handleConfirmNavigation} + handleSave={handleSaveAndCloseModal} + /> ); }; diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index 497cb487308..6f4b01c2cbe 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -593,6 +593,7 @@ function ExploreViewContainer(props) { reports={props.reports} saveDisabled={errorMessage || props.chart.chartStatus === 'loading'} metadata={props.metadata} + isSaveModalVisible={props.isSaveModalVisible} /> { - expect( - sanitizeFormData({ - url_params: { foo: 'bar' }, - metrics: ['foo', 'bar'], - }), - ).toEqual({ metrics: ['foo', 'bar'] }); +jest.mock('@superset-ui/core', () => ({ + SupersetClient: { + post: jest.fn(), + put: jest.fn(), + }, +})); + +test('postFormData should call SupersetClient.post with correct payload and return key', async () => { + const mockKey = '123abc'; + const mockResponse = { json: { key: mockKey } }; + + (SupersetClient.post as jest.Mock).mockResolvedValue(mockResponse); + + const result = await postFormData( + 1, + 'table', + { some: 'form', data: true }, + 42, + 'tab-7', + ); + + expect(SupersetClient.post).toHaveBeenCalledWith({ + endpoint: 'api/v1/explore/form_data?tab_id=tab-7', + jsonPayload: { + datasource_id: 1, + datasource_type: 'table', + form_data: JSON.stringify({ some: 'form', data: true }), // assuming sanitizeFormData is a passthrough + chart_id: 42, + }, + }); + + expect(result).toBe(mockKey); +}); + +test('putFormData should call SupersetClient.put with correct payload and return message', async () => { + const mockMessage = 'Form data updated'; + const mockResponse = { json: { message: mockMessage } }; + + (SupersetClient.put as jest.Mock).mockResolvedValue(mockResponse); + + const result = await putFormData( + 10, + 'druid', + 'abc123', + { another: 'value' }, + undefined, + 'tab-5', + ); + + expect(SupersetClient.put).toHaveBeenCalledWith({ + endpoint: 'api/v1/explore/form_data/abc123?tab_id=tab-5', + jsonPayload: { + datasource_id: 10, + datasource_type: 'druid', + form_data: JSON.stringify({ another: 'value' }), // again, assuming sanitizeFormData is passthrough + }, + }); + + expect(result).toBe(mockMessage); +}); + +test('postFormData without optional params should work', async () => { + (SupersetClient.post as jest.Mock).mockResolvedValue({ + json: { key: 'abc' }, + }); + + await postFormData(2, 'table', { foo: 'bar' }); + + expect(SupersetClient.post).toHaveBeenCalledWith({ + endpoint: 'api/v1/explore/form_data', + jsonPayload: { + datasource_id: 2, + datasource_type: 'table', + form_data: JSON.stringify({ foo: 'bar' }), + }, + }); }); diff --git a/superset-frontend/src/explore/exploreUtils/formData.ts b/superset-frontend/src/explore/exploreUtils/formData.ts index 36de6640a5c..9a83d8fd8da 100644 --- a/superset-frontend/src/explore/exploreUtils/formData.ts +++ b/superset-frontend/src/explore/exploreUtils/formData.ts @@ -16,8 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { omit } from 'lodash'; -import { SupersetClient, JsonObject } from '@superset-ui/core'; +import { SupersetClient, JsonObject, JsonResponse } from '@superset-ui/core'; +import { sanitizeFormData } from 'src/utils/sanitizeFormData'; type Payload = { datasource_id: number; @@ -26,19 +26,12 @@ type Payload = { chart_id?: number; }; -const TEMPORARY_CONTROLS = ['url_params']; - -export const sanitizeFormData = (formData: JsonObject): JsonObject => - omit(formData, TEMPORARY_CONTROLS); - const assembleEndpoint = (key?: string, tabId?: string) => { let endpoint = 'api/v1/explore/form_data'; - if (key) { - endpoint = endpoint.concat(`/${key}`); - } - if (tabId) { - endpoint = endpoint.concat(`?tab_id=${tabId}`); - } + + if (key) endpoint = endpoint.concat(`/${key}`); + if (tabId) endpoint = endpoint.concat(`?tab_id=${tabId}`); + return endpoint; }; @@ -47,15 +40,15 @@ const assemblePayload = ( datasourceType: string, formData: JsonObject, chartId?: number, -) => { +): Payload => { const payload: Payload = { datasource_id: datasourceId, datasource_type: datasourceType, form_data: JSON.stringify(sanitizeFormData(formData)), }; - if (chartId) { - payload.chart_id = chartId; - } + + if (chartId) payload.chart_id = chartId; + return payload; }; @@ -74,7 +67,7 @@ export const postFormData = ( formData, chartId, ), - }).then(r => r.json.key); + }).then((r: JsonResponse) => r.json.key); export const putFormData = ( datasourceId: number, @@ -92,4 +85,4 @@ export const putFormData = ( formData, chartId, ), - }).then(r => r.json.message); + }).then((r: JsonResponse) => r.json.message); diff --git a/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts new file mode 100644 index 00000000000..b09adece6af --- /dev/null +++ b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts @@ -0,0 +1,124 @@ +/** + * 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 { getClientErrorObject, t } from '@superset-ui/core'; +import { useEffect, useRef, useCallback, useState } from 'react'; +import { useHistory } from 'react-router-dom'; + +type UseUnsavedChangesPromptProps = { + hasUnsavedChanges: boolean; + onSave: () => Promise | void; + isSaveModalVisible?: boolean; + manualSaveOnUnsavedChanges?: boolean; +}; + +export const useUnsavedChangesPrompt = ({ + hasUnsavedChanges, + onSave, + isSaveModalVisible = false, + manualSaveOnUnsavedChanges = false, +}: UseUnsavedChangesPromptProps) => { + const history = useHistory(); + const [showModal, setShowModal] = useState(false); + + const confirmNavigationRef = useRef<(() => void) | null>(null); + const unblockRef = useRef<() => void>(() => {}); + const manualSaveRef = useRef(false); // Track if save was user-initiated (not via navigation) + + const handleConfirmNavigation = useCallback(() => { + confirmNavigationRef.current?.(); + }, []); + + const handleSaveAndCloseModal = useCallback(async () => { + try { + if (manualSaveOnUnsavedChanges) manualSaveRef.current = true; + + await onSave(); + setShowModal(false); + } catch (err) { + const clientError = await getClientErrorObject(err); + throw new Error( + clientError.message || + clientError.error || + t('Sorry, an error occurred'), + { cause: err }, + ); + } + }, [manualSaveOnUnsavedChanges, onSave]); + + const triggerManualSave = useCallback(() => { + manualSaveRef.current = true; + onSave(); + }, [onSave]); + + const blockCallback = useCallback( + ({ pathname }: { pathname: string }) => { + if (manualSaveRef.current) { + manualSaveRef.current = false; + return undefined; + } + + confirmNavigationRef.current = () => { + unblockRef.current?.(); + history.push(pathname); + }; + + setShowModal(true); + return false; + }, + [history], + ); + + useEffect(() => { + if (!hasUnsavedChanges) return undefined; + + const unblock = history.block(blockCallback); + unblockRef.current = unblock; + + return () => unblock(); + }, [blockCallback, hasUnsavedChanges, history]); + + useEffect(() => { + const handleBeforeUnload = (event: BeforeUnloadEvent) => { + if (!hasUnsavedChanges) return; + event.preventDefault(); + + // Most browsers require a "returnValue" set to empty string + const evt = event as any; + evt.returnValue = ''; + }; + + window.addEventListener('beforeunload', handleBeforeUnload); + return () => window.removeEventListener('beforeunload', handleBeforeUnload); + }, [hasUnsavedChanges]); + + useEffect(() => { + if (!isSaveModalVisible && manualSaveRef.current) { + setShowModal(false); + manualSaveRef.current = false; + } + }, [isSaveModalVisible]); + + return { + showModal, + setShowModal, + handleConfirmNavigation, + handleSaveAndCloseModal, + triggerManualSave, + }; +}; diff --git a/superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx b/superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx new file mode 100644 index 00000000000..bc42cdcfc91 --- /dev/null +++ b/superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx @@ -0,0 +1,106 @@ +/** + * 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 { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt'; +import { Router } from 'react-router-dom'; +import { createMemoryHistory } from 'history'; +import { act } from 'spec/helpers/testing-library'; + +const history = createMemoryHistory({ + initialEntries: ['/dashboard'], +}); + +const wrapper = ({ children }: { children: React.ReactNode }) => ( + {children} +); + +describe('useUnsavedChangesPrompt', () => { + it('should not show modal initially', () => { + const { result } = renderHook( + () => + useUnsavedChangesPrompt({ + hasUnsavedChanges: true, + onSave: jest.fn(), + }), + { wrapper }, + ); + + expect(result.current.showModal).toBe(false); + }); + + it('should block navigation and show modal if there are unsaved changes', () => { + const { result } = renderHook( + () => + useUnsavedChangesPrompt({ + hasUnsavedChanges: true, + onSave: jest.fn(), + }), + { wrapper }, + ); + + // Simulate blocked navigation + act(() => { + const unblock = history.block((tx: any) => tx); + unblock(); + history.push('/another-page'); + }); + + expect(result.current.showModal).toBe(true); + }); + + it('should trigger onSave and hide modal on handleSaveAndCloseModal', async () => { + const onSave = jest.fn().mockResolvedValue(undefined); + + const { result } = renderHook( + () => + useUnsavedChangesPrompt({ + hasUnsavedChanges: true, + onSave, + }), + { wrapper }, + ); + + await act(async () => { + await result.current.handleSaveAndCloseModal(); + }); + + expect(onSave).toHaveBeenCalled(); + expect(result.current.showModal).toBe(false); + }); + + it('should trigger manual save and not show modal again', async () => { + const onSave = jest.fn().mockResolvedValue(undefined); + + const { result } = renderHook( + () => + useUnsavedChangesPrompt({ + hasUnsavedChanges: true, + onSave, + }), + { wrapper }, + ); + + act(() => { + result.current.triggerManualSave(); + }); + + expect(onSave).toHaveBeenCalled(); + expect(result.current.showModal).toBe(false); + }); +}); diff --git a/superset-frontend/src/pages/Chart/Chart.test.tsx b/superset-frontend/src/pages/Chart/Chart.test.tsx index 2fe7599211f..6b0e9f820a3 100644 --- a/superset-frontend/src/pages/Chart/Chart.test.tsx +++ b/superset-frontend/src/pages/Chart/Chart.test.tsx @@ -30,9 +30,12 @@ import { LocalStorageKeys } from 'src/utils/localStorageHelpers'; import getFormDataWithExtraFilters from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; import { URL_PARAMS } from 'src/constants'; import { JsonObject, VizType } from '@superset-ui/core'; - +import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt'; import ChartPage from '.'; +jest.mock('src/hooks/useUnsavedChangesPrompt', () => ({ + useUnsavedChangesPrompt: jest.fn(), +})); jest.mock('re-resizable', () => ({ Resizable: () =>
, })); @@ -48,6 +51,17 @@ jest.mock( jest.mock('src/dashboard/util/charts/getFormDataWithExtraFilters'); describe('ChartPage', () => { + beforeEach(() => { + jest.clearAllMocks(); + + (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({ + showModal: false, + setShowModal: jest.fn(), + handleConfirmNavigation: jest.fn(), + handleSaveAndCloseModal: jest.fn(), + }); + }); + afterEach(() => { fetchMock.reset(); }); diff --git a/superset-frontend/src/types/DiffType.ts b/superset-frontend/src/types/DiffType.ts new file mode 100644 index 00000000000..e146df35be2 --- /dev/null +++ b/superset-frontend/src/types/DiffType.ts @@ -0,0 +1,40 @@ +/** + * 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. + */ +export type FilterItemType = { + comparator?: string | string[]; + subject: string; + operator: string; + label?: string; +}; + +export type DiffItemType< + T = FilterItemType | number | string | Record, +> = + | T[] + | boolean + | number + | string + | Record + | null + | undefined; + +export type DiffType = { + before: DiffItemType; + after: DiffItemType; +}; diff --git a/superset-frontend/src/utils/getChartFormDiffs/getChartFormDiffs.test.ts b/superset-frontend/src/utils/getChartFormDiffs/getChartFormDiffs.test.ts new file mode 100644 index 00000000000..6b7680ec1c6 --- /dev/null +++ b/superset-frontend/src/utils/getChartFormDiffs/getChartFormDiffs.test.ts @@ -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 { JsonObject } from '@superset-ui/core'; +import { alterForComparison, getChartFormDiffs, isEqualish } from '.'; + +jest.mock('../sanitizeFormData', () => ({ + sanitizeFormData: (fd: JsonObject): JsonObject => ({ + ...fd, + _sanitized: true, + }), +})); + +describe('alterForComparison', () => { + it.each([ + [null, null], + ['', null], + [[], null], + [{}, null], + [ + [1, 2], + [1, 2], + ], + [{ a: 1 }, { a: 1 }], + ['foo', 'foo'], + ])('normalizes %p to %p', (input, expected) => { + expect(alterForComparison(input)).toEqual(expected); + }); +}); + +describe('isEqualish', () => { + it('returns true for semantically equal values with different formats', () => { + expect(isEqualish('', null)).toBe(true); + expect(isEqualish([], null)).toBe(true); + expect(isEqualish({}, null)).toBe(true); + expect(isEqualish([1], [1])).toBe(true); + }); + + it('returns false for clearly different values', () => { + expect(isEqualish([1], [2])).toBe(false); + expect(isEqualish({ a: 1 }, { a: 2 })).toBe(false); + expect(isEqualish('foo', 'bar')).toBe(false); + }); +}); + +describe('getChartFormDiffs', () => { + it('returns diffs for changed values', () => { + const original = { metric: 'count', adhoc_filters: [] }; + const current = { metric: 'sum__num', adhoc_filters: [] }; + + const diffs = getChartFormDiffs(original, current); + + expect(diffs).toHaveProperty('metric'); + expect(diffs.metric).toEqual({ + before: 'count', + after: 'sum__num', + }); + }); + + it('ignores noisy keys', () => { + const original = { where: 'a = 1', metric: 'count' }; + const current = { where: 'a = 2', metric: 'sum__num' }; + + const diffs = getChartFormDiffs(original, current); + + expect(diffs).not.toHaveProperty('where'); + expect(diffs).toHaveProperty('metric'); + }); + + it('does not include values that are equalish', () => { + const original = { filters: [], metric: 'count' }; + const current = { filters: null, metric: 'count' }; + + const diffs = getChartFormDiffs(original, current); + + expect(diffs).toEqual({}); + }); + + it('handles missing keys in original or current gracefully', () => { + const original = { metric: 'count' }; + const current = { metric: 'count', new_field: 'value' }; + + const diffs = getChartFormDiffs(original, current); + + expect(diffs).toHaveProperty('new_field'); + expect(diffs.new_field).toEqual({ + before: undefined, + after: 'value', + }); + }); + + it('ignores keys that are missing in current and not explicitly changed', () => { + const original = { metric: 'count', removed_field: 'gone' }; + const current = { metric: 'count' }; + + const diffs = getChartFormDiffs(original, current); + + expect(diffs).not.toHaveProperty('removed_field'); + }); +}); diff --git a/superset-frontend/src/utils/getChartFormDiffs/index.ts b/superset-frontend/src/utils/getChartFormDiffs/index.ts new file mode 100644 index 00000000000..6c89b4fef31 --- /dev/null +++ b/superset-frontend/src/utils/getChartFormDiffs/index.ts @@ -0,0 +1,66 @@ +/** + * 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 { isEqual } from 'lodash'; +import { DiffType } from 'src/types/DiffType'; +import { JsonObject } from '@superset-ui/core'; +import { sanitizeFormData } from '../sanitizeFormData'; + +export const noisyKeys = new Set(['filters', 'having', 'where']); + +export const alterForComparison = (value: unknown): unknown => { + if (value == null || value === '') return null; + if (Array.isArray(value) && value.length === 0) return null; + if (typeof value === 'object' && value && Object.keys(value).length === 0) + return null; + + return value; +}; + +export const isEqualish = (a: unknown, b: unknown): boolean => + isEqual(alterForComparison(a), alterForComparison(b)); + +export const getChartFormDiffs = ( + originalFormData: Record, + currentFormData: Record, +): Record => { + const ofd: JsonObject = sanitizeFormData(originalFormData); + const cfd: JsonObject = sanitizeFormData(currentFormData); + + const keys = new Set([...Object.keys(ofd), ...Object.keys(cfd)]); + const diffs: Record = {}; + + keys.forEach((key: string) => { + if (noisyKeys.has(key)) return; + + const original = ofd[key]; + const current = cfd[key]; + + const currentHasKey = Object.prototype.hasOwnProperty.call(cfd, key); + const originalHasKey = Object.prototype.hasOwnProperty.call(ofd, key); + + const bothExplicit = currentHasKey && originalHasKey; + + if (!bothExplicit && !currentHasKey) return; + + if (!isEqualish(original, current)) + diffs[key] = { before: original, after: current }; + }); + + return diffs; +}; diff --git a/superset-frontend/src/utils/sanitizeFormData/index.ts b/superset-frontend/src/utils/sanitizeFormData/index.ts new file mode 100644 index 00000000000..9c55cfc6cf4 --- /dev/null +++ b/superset-frontend/src/utils/sanitizeFormData/index.ts @@ -0,0 +1,25 @@ +/** + * 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 { JsonObject } from '@superset-ui/core'; +import { omit } from 'lodash'; + +const TEMPORARY_CONTROLS: string[] = ['url_params']; + +export const sanitizeFormData = (formData: JsonObject): JsonObject => + omit(formData, TEMPORARY_CONTROLS); diff --git a/superset-frontend/src/utils/sanitizeFormData/sanitizeFormData.test.ts b/superset-frontend/src/utils/sanitizeFormData/sanitizeFormData.test.ts new file mode 100644 index 00000000000..98da5c86984 --- /dev/null +++ b/superset-frontend/src/utils/sanitizeFormData/sanitizeFormData.test.ts @@ -0,0 +1,28 @@ +/** + * 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 { sanitizeFormData } from '.'; + +test('sanitizeFormData removes temporary control values', () => { + expect( + sanitizeFormData({ + url_params: { foo: 'bar' }, + metrics: ['foo', 'bar'], + }), + ).toEqual({ metrics: ['foo', 'bar'] }); +});