From ca9766c109ae0849748e791405554f54e5d13249 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 12 May 2022 12:24:29 +0200 Subject: [PATCH] feat(explore): Show confirmation modal if user exits Explore without saving changes (#19993) * feat(explore): Show confirmation modal if user exits Explore without saving changes * Fix calling cleanup func unnecessarily * Fix comparing AdhocMetric instance with JSON object * Replace sliceFormData with the initial form data --- .../AlteredSliceTag/AlteredSliceTag.test.jsx | 30 ----------- .../src/components/AlteredSliceTag/index.jsx | 42 +-------------- .../components/ExploreViewContainer/index.jsx | 45 ++++++++++++++-- .../src/explore/exploreUtils/formData.test.ts | 24 ++++++++- .../src/explore/exploreUtils/formData.ts | 54 ++++++++++++++++++- 5 files changed, 119 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..2715141a104c5 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'; @@ -216,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]; @@ -236,6 +251,9 @@ function ExploreViewContainer(props) { const theme = useTheme(); + const isBeforeUnloadActive = useRef(false); + const initialFormData = useRef(props.form_data); + const defaultSidebarsWidth = { controls_width: 320, datasource_width: 300, @@ -365,6 +383,27 @@ 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 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..eedfb62d8fb8c 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,56 @@ 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) {