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
apache · May 12, 2022 · ca9766c · ca9766c
1 parent 26c81a7
commit ca9766c
Show file tree

Hide file tree

Showing 5 changed files with 119 additions and 76 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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) {