Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(explore): Show confirmation modal if user exits Explore without saving changes #19993

Merged
merged 4 commits into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
});
});
});
42 changes: 2 additions & 40 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 { 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';
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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];
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down
24 changes: 23 additions & 1 deletion 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 } from './formData';
import { sanitizeFormData, isEqualish } from './formData';

test('sanitizeFormData removes temporary control values', () => {
expect(
Expand All @@ -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);
});
54 changes: 52 additions & 2 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 { 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;
Expand All @@ -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) {
Expand Down