Skip to content

Commit

Permalink
Revert "feat(explore): Show confirmation modal if user exits Explore …
Browse files Browse the repository at this point in the history
…without saving changes (#19993)" (#20092)

This reverts commit ca9766c.
  • Loading branch information
kgabryje committed May 17, 2022
1 parent e69f629 commit 9cdaa28
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 119 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
});
});
});
42 changes: 40 additions & 2 deletions superset-frontend/src/components/AlteredSliceTag/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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];
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down
24 changes: 1 addition & 23 deletions superset-frontend/src/explore/exploreUtils/formData.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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);
});
54 changes: 2 additions & 52 deletions superset-frontend/src/explore/exploreUtils/formData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down

0 comments on commit 9cdaa28

Please sign in to comment.