diff --git a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx index a460e8193953..7501ce6382a4 100644 --- a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx +++ b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx @@ -308,4 +308,34 @@ describe('AlteredSliceTag', () => { ).toBe(expected); }); }); + describe('isEqualish', () => { + it('considers null, undefined, {} and [] as equal', () => { + const inst = wrapper.instance(); + expect(inst.isEqualish(null, undefined)).toBe(true); + expect(inst.isEqualish(null, [])).toBe(true); + expect(inst.isEqualish(null, {})).toBe(true); + expect(inst.isEqualish(undefined, {})).toBe(true); + }); + it('considers empty strings are the same as null', () => { + const inst = wrapper.instance(); + expect(inst.isEqualish(undefined, '')).toBe(true); + expect(inst.isEqualish(null, '')).toBe(true); + }); + it('considers deeply equal objects as equal', () => { + const inst = wrapper.instance(); + expect(inst.isEqualish('', '')).toBe(true); + expect(inst.isEqualish({ a: 1, b: 2, c: 3 }, { a: 1, b: 2, c: 3 })).toBe( + true, + ); + // Out of order + expect(inst.isEqualish({ a: 1, b: 2, c: 3 }, { b: 2, a: 1, c: 3 })).toBe( + true, + ); + + // Actually not equal + expect(inst.isEqualish({ a: 1, b: 2, z: 9 }, { a: 1, b: 2, c: 3 })).toBe( + false, + ); + }); + }); }); diff --git a/superset-frontend/src/components/AlteredSliceTag/index.jsx b/superset-frontend/src/components/AlteredSliceTag/index.jsx index ecae44ad0c81..dd5dfb3c8786 100644 --- a/superset-frontend/src/components/AlteredSliceTag/index.jsx +++ b/superset-frontend/src/components/AlteredSliceTag/index.jsx @@ -20,7 +20,7 @@ import React from 'react'; import PropTypes from 'prop-types'; import { isEqual, isEmpty } from 'lodash'; import { styled, t } from '@superset-ui/core'; -import { getFormDataDiffs } from 'src/explore/exploreUtils/formData'; +import { sanitizeFormData } from 'src/explore/exploreUtils/formData'; import getControlsForVizType from 'src/utils/getControlsForVizType'; import { safeStringify } from 'src/utils/safeStringify'; import { Tooltip } from 'src/components/Tooltip'; @@ -44,6 +44,24 @@ const StyledLabel = styled.span` `} `; +function alterForComparison(value) { + // Considering `[]`, `{}`, `null` and `undefined` as identical + // for this purpose + if (value === undefined || value === null || value === '') { + return null; + } + if (typeof value === 'object') { + if (Array.isArray(value) && value.length === 0) { + return null; + } + const keys = Object.keys(value); + if (keys && keys.length === 0) { + return null; + } + } + return value; +} + export default class AlteredSliceTag extends React.Component { constructor(props) { super(props); @@ -77,7 +95,27 @@ export default class AlteredSliceTag extends React.Component { getDiffs(props) { // Returns all properties that differ in the // current form data and the saved form data - return getFormDataDiffs(props.origFormData, props.currentFormData); + const ofd = sanitizeFormData(props.origFormData); + const cfd = sanitizeFormData(props.currentFormData); + + const fdKeys = Object.keys(cfd); + const diffs = {}; + fdKeys.forEach(fdKey => { + if (!ofd[fdKey] && !cfd[fdKey]) { + return; + } + if (['filters', 'having', 'having_filters', 'where'].includes(fdKey)) { + return; + } + if (!this.isEqualish(ofd[fdKey], cfd[fdKey])) { + diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] }; + } + }); + return diffs; + } + + isEqualish(val1, val2) { + return isEqual(alterForComparison(val1), alterForComparison(val2)); } formatValue(value, key, controlsMap) { diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index 354e95b5ab2d..97e30d335b7a 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -17,18 +17,12 @@ * under the License. */ /* eslint camelcase: 0 */ -import React, { - useCallback, - useEffect, - useMemo, - useRef, - useState, -} from 'react'; +import React, { useCallback, useEffect, useMemo, useState } from 'react'; import PropTypes from 'prop-types'; import { bindActionCreators } from 'redux'; import { connect } from 'react-redux'; import { styled, t, css, useTheme, logging } from '@superset-ui/core'; -import { debounce, pick, isEmpty } from 'lodash'; +import { debounce, pick } from 'lodash'; import { Resizable } from 're-resizable'; import { useChangeEffect } from 'src/hooks/useChangeEffect'; import { usePluginContext } from 'src/components/DynamicPlugins'; @@ -49,11 +43,7 @@ import * as chartActions from 'src/components/Chart/chartAction'; import { fetchDatasourceMetadata } from 'src/dashboard/actions/datasources'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import { mergeExtraFormData } from 'src/dashboard/components/nativeFilters/utils'; -import { - getFormDataDiffs, - postFormData, - putFormData, -} from 'src/explore/exploreUtils/formData'; +import { postFormData, putFormData } from 'src/explore/exploreUtils/formData'; import { useTabId } from 'src/hooks/useTabId'; import ExploreChartPanel from '../ExploreChartPanel'; import ConnectedControlPanelsContainer from '../ControlPanelsContainer'; @@ -226,11 +216,6 @@ const updateHistory = debounce( 1000, ); -const handleUnloadEvent = e => { - e.preventDefault(); - e.returnValue = 'Controls changed'; -}; - function ExploreViewContainer(props) { const dynamicPluginContext = usePluginContext(); const dynamicPlugin = dynamicPluginContext.dynamicPlugins[props.vizType]; @@ -251,9 +236,6 @@ function ExploreViewContainer(props) { const theme = useTheme(); - const isBeforeUnloadActive = useRef(false); - const initialFormData = useRef(props.form_data); - const defaultSidebarsWidth = { controls_width: 320, datasource_width: 300, @@ -383,27 +365,6 @@ function ExploreViewContainer(props) { }; }, [handleKeydown, previousHandleKeyDown]); - useEffect(() => { - const formDataChanged = !isEmpty( - getFormDataDiffs(initialFormData.current, props.form_data), - ); - if (formDataChanged && !isBeforeUnloadActive.current) { - window.addEventListener('beforeunload', handleUnloadEvent); - isBeforeUnloadActive.current = true; - } - if (!formDataChanged && isBeforeUnloadActive.current) { - window.removeEventListener('beforeunload', handleUnloadEvent); - isBeforeUnloadActive.current = false; - } - }, [props.form_data]); - - // cleanup beforeunload event listener - // we use separate useEffect to call it only on component unmount instead of on every form data change - useEffect( - () => () => window.removeEventListener('beforeunload', handleUnloadEvent), - [], - ); - useEffect(() => { if (wasDynamicPluginLoading && !isDynamicPluginLoading) { // reload the controls now that we actually have the control config diff --git a/superset-frontend/src/explore/exploreUtils/formData.test.ts b/superset-frontend/src/explore/exploreUtils/formData.test.ts index cfe583dd62b2..f14455b51bd8 100644 --- a/superset-frontend/src/explore/exploreUtils/formData.test.ts +++ b/superset-frontend/src/explore/exploreUtils/formData.test.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { sanitizeFormData, isEqualish } from './formData'; +import { sanitizeFormData } from './formData'; test('sanitizeFormData removes temporary control values', () => { expect( @@ -26,25 +26,3 @@ test('sanitizeFormData removes temporary control values', () => { }), ).toEqual({ metrics: ['foo', 'bar'] }); }); - -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); - - // considers empty strings are the same as null - expect(isEqualish(undefined, '')).toBe(true); - expect(isEqualish(null, '')).toBe(true); - - // considers deeply equal objects as equal - expect(isEqualish('', '')).toBe(true); - expect(isEqualish({ a: 1, b: 2, c: 3 }, { a: 1, b: 2, c: 3 })).toBe(true); - - // Out of order - expect(isEqualish({ a: 1, b: 2, c: 3 }, { b: 2, a: 1, c: 3 })).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/explore/exploreUtils/formData.ts b/superset-frontend/src/explore/exploreUtils/formData.ts index eedfb62d8fb8..9987b5d8cfa7 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 { isEqual, omit } from 'lodash'; -import { SupersetClient, JsonObject, JsonValue } from '@superset-ui/core'; +import { omit } from 'lodash'; +import { SupersetClient, JsonObject } from '@superset-ui/core'; type Payload = { dataset_id: number; @@ -30,56 +30,6 @@ const TEMPORARY_CONTROLS = ['url_params']; export const sanitizeFormData = (formData: JsonObject): JsonObject => omit(formData, TEMPORARY_CONTROLS); -export const alterForComparison = (value: JsonValue | undefined) => { - // Considering `[]`, `{}`, `null` and `undefined` as identical - // for this purpose - if ( - value === undefined || - value === null || - value === '' || - (Array.isArray(value) && value.length === 0) || - (typeof value === 'object' && Object.keys(value).length === 0) - ) { - return null; - } - if (Array.isArray(value)) { - // omit prototype for comparison of class instances with json objects - return value.map(v => (typeof v === 'object' ? omit(v, ['__proto__']) : v)); - } - if (typeof value === 'object') { - return omit(value, ['__proto__']); - } - return value; -}; - -export const isEqualish = ( - val1: JsonValue | undefined, - val2: JsonValue | undefined, -) => isEqual(alterForComparison(val1), alterForComparison(val2)); - -export const getFormDataDiffs = ( - formData1: JsonObject, - formData2: JsonObject, -) => { - const ofd = sanitizeFormData(formData1); - const cfd = sanitizeFormData(formData2); - - const fdKeys = Object.keys(cfd); - const diffs = {}; - fdKeys.forEach(fdKey => { - if (!ofd[fdKey] && !cfd[fdKey]) { - return; - } - if (['filters', 'having', 'having_filters', 'where'].includes(fdKey)) { - return; - } - if (!isEqualish(ofd[fdKey], cfd[fdKey])) { - diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] }; - } - }); - return diffs; -}; - const assembleEndpoint = (key?: string, tabId?: string) => { let endpoint = 'api/v1/explore/form_data'; if (key) {