From b303aaa67cee6f7bfd9b6a9d292f361fefd30e7b Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Mon, 9 May 2022 15:38:53 +0200 Subject: [PATCH 1/4] feat(explore): Show confirmation modal if user exits Explore without saving changes --- .../AlteredSliceTag/AlteredSliceTag.test.jsx | 30 ----------- .../src/components/AlteredSliceTag/index.jsx | 42 +--------------- .../components/ExploreViewContainer/index.jsx | 37 ++++++++++++-- .../src/explore/exploreUtils/formData.test.ts | 24 ++++++++- .../src/explore/exploreUtils/formData.ts | 50 ++++++++++++++++++- 5 files changed, 107 insertions(+), 76 deletions(-) diff --git a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx index 7501ce6382a4c..a460e81939534 100644 --- a/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx +++ b/superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx @@ -308,34 +308,4 @@ 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 2faa62c8908db..26e72915e6973 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 { sanitizeFormData } from 'src/explore/exploreUtils/formData'; +import { getFormDataDiffs } from 'src/explore/exploreUtils/formData'; import getControlsForVizType from 'src/utils/getControlsForVizType'; import { safeStringify } from 'src/utils/safeStringify'; import { Tooltip } from 'src/components/Tooltip'; @@ -44,24 +44,6 @@ 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); @@ -95,27 +77,7 @@ export default class AlteredSliceTag extends React.Component { getDiffs(props) { // 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 = {}; - 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)); + return getFormDataDiffs(props.origFormData, props.currentFormData); } 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 7e27015e3f050..f0d69c0d5caaf 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -17,12 +17,18 @@ * under the License. */ /* eslint camelcase: 0 */ -import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import React, { + useCallback, + useEffect, + useMemo, + useRef, + 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 } from 'lodash'; +import { debounce, pick, isEmpty } from 'lodash'; import { Resizable } from 're-resizable'; import { useChangeEffect } from 'src/hooks/useChangeEffect'; import { usePluginContext } from 'src/components/DynamicPlugins'; @@ -43,7 +49,11 @@ 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 { postFormData, putFormData } from 'src/explore/exploreUtils/formData'; +import { + getFormDataDiffs, + postFormData, + putFormData, +} from 'src/explore/exploreUtils/formData'; import { useTabId } from 'src/hooks/useTabId'; import ExploreChartPanel from '../ExploreChartPanel'; import ConnectedControlPanelsContainer from '../ControlPanelsContainer'; @@ -236,6 +246,8 @@ function ExploreViewContainer(props) { const theme = useTheme(); + const isBeforeUnloadActive = useRef(false); + const defaultSidebarsWidth = { controls_width: 320, datasource_width: 300, @@ -365,6 +377,25 @@ function ExploreViewContainer(props) { }; }, [handleKeydown, previousHandleKeyDown]); + useEffect(() => { + const handleCloseEvent = e => { + e.preventDefault(); + e.returnValue = 'Controls changed'; + }; + const formDataChanged = !isEmpty( + getFormDataDiffs(props.chart.sliceFormData, props.form_data), + ); + if (formDataChanged && !isBeforeUnloadActive.current) { + window.addEventListener('beforeunload', handleCloseEvent); + isBeforeUnloadActive.current = true; + } + if (!formDataChanged && isBeforeUnloadActive.current) { + window.removeEventListener('beforeunload', handleCloseEvent); + isBeforeUnloadActive.current = false; + } + return () => window.removeEventListener('beforeunload', handleCloseEvent); + }, [props.chart.sliceFormData, props.form_data]); + 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 f14455b51bd8e..cfe583dd62b27 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 } from './formData'; +import { sanitizeFormData, isEqualish } from './formData'; test('sanitizeFormData removes temporary control values', () => { expect( @@ -26,3 +26,25 @@ 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 9987b5d8cfa76..24b483f1089d4 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 { isEqual, omit } from 'lodash'; +import { SupersetClient, JsonObject, JsonValue } from '@superset-ui/core'; type Payload = { dataset_id: number; @@ -30,6 +30,52 @@ const TEMPORARY_CONTROLS = ['url_params']; export const sanitizeFormData = (formData: JsonObject): JsonObject => omit(formData, TEMPORARY_CONTROLS); +export const getNullIfEmpty = (value: JsonValue | undefined) => { + // 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 const isEqualish = ( + val1: JsonValue | undefined, + val2: JsonValue | undefined, +) => isEqual(getNullIfEmpty(val1), getNullIfEmpty(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 (!isEqual(getNullIfEmpty(ofd[fdKey]), getNullIfEmpty(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) { From b061dd45dd9081aa6c68386e135f9421d4648008 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Tue, 10 May 2022 14:32:06 +0200 Subject: [PATCH 2/4] Fix calling cleanup func unnecessarily --- .../components/ExploreViewContainer/index.jsx | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index f0d69c0d5caaf..dd136ef0148b9 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -226,6 +226,11 @@ 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]; @@ -378,24 +383,26 @@ function ExploreViewContainer(props) { }, [handleKeydown, previousHandleKeyDown]); useEffect(() => { - const handleCloseEvent = e => { - e.preventDefault(); - e.returnValue = 'Controls changed'; - }; const formDataChanged = !isEmpty( getFormDataDiffs(props.chart.sliceFormData, props.form_data), ); if (formDataChanged && !isBeforeUnloadActive.current) { - window.addEventListener('beforeunload', handleCloseEvent); + window.addEventListener('beforeunload', handleUnloadEvent); isBeforeUnloadActive.current = true; } if (!formDataChanged && isBeforeUnloadActive.current) { - window.removeEventListener('beforeunload', handleCloseEvent); + window.removeEventListener('beforeunload', handleUnloadEvent); isBeforeUnloadActive.current = false; } - return () => window.removeEventListener('beforeunload', handleCloseEvent); }, [props.chart.sliceFormData, 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 From ed6e5f99d829206ead248763ae79006423553446 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Tue, 10 May 2022 14:32:38 +0200 Subject: [PATCH 3/4] Fix comparing AdhocMetric instance with JSON object --- .../src/explore/exploreUtils/formData.ts | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/superset-frontend/src/explore/exploreUtils/formData.ts b/superset-frontend/src/explore/exploreUtils/formData.ts index 24b483f1089d4..eedfb62d8fb8c 100644 --- a/superset-frontend/src/explore/exploreUtils/formData.ts +++ b/superset-frontend/src/explore/exploreUtils/formData.ts @@ -30,20 +30,24 @@ const TEMPORARY_CONTROLS = ['url_params']; export const sanitizeFormData = (formData: JsonObject): JsonObject => omit(formData, TEMPORARY_CONTROLS); -export const getNullIfEmpty = (value: JsonValue | undefined) => { +export const alterForComparison = (value: JsonValue | undefined) => { // Considering `[]`, `{}`, `null` and `undefined` as identical // for this purpose - if (value === undefined || value === null || value === '') { + 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') { - if (Array.isArray(value) && value.length === 0) { - return null; - } - const keys = Object.keys(value); - if (keys && keys.length === 0) { - return null; - } + return omit(value, ['__proto__']); } return value; }; @@ -51,7 +55,7 @@ export const getNullIfEmpty = (value: JsonValue | undefined) => { export const isEqualish = ( val1: JsonValue | undefined, val2: JsonValue | undefined, -) => isEqual(getNullIfEmpty(val1), getNullIfEmpty(val2)); +) => isEqual(alterForComparison(val1), alterForComparison(val2)); export const getFormDataDiffs = ( formData1: JsonObject, @@ -69,7 +73,7 @@ export const getFormDataDiffs = ( if (['filters', 'having', 'having_filters', 'where'].includes(fdKey)) { return; } - if (!isEqual(getNullIfEmpty(ofd[fdKey]), getNullIfEmpty(cfd[fdKey]))) { + if (!isEqualish(ofd[fdKey], cfd[fdKey])) { diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] }; } }); From 33690f94f5c1ee6393e7edb9f473bcf2f287daec Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 11 May 2022 14:45:35 +0200 Subject: [PATCH 4/4] Replace sliceFormData with the initial form data --- .../src/explore/components/ExploreViewContainer/index.jsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index dd136ef0148b9..2715141a104c5 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -252,6 +252,7 @@ function ExploreViewContainer(props) { const theme = useTheme(); const isBeforeUnloadActive = useRef(false); + const initialFormData = useRef(props.form_data); const defaultSidebarsWidth = { controls_width: 320, @@ -384,7 +385,7 @@ function ExploreViewContainer(props) { useEffect(() => { const formDataChanged = !isEmpty( - getFormDataDiffs(props.chart.sliceFormData, props.form_data), + getFormDataDiffs(initialFormData.current, props.form_data), ); if (formDataChanged && !isBeforeUnloadActive.current) { window.addEventListener('beforeunload', handleUnloadEvent); @@ -394,7 +395,7 @@ function ExploreViewContainer(props) { window.removeEventListener('beforeunload', handleUnloadEvent); isBeforeUnloadActive.current = false; } - }, [props.chart.sliceFormData, props.form_data]); + }, [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