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.

Sign up for GitHub

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): Don't discard controls with custom sql when changing datasource #20934

Merged
merged 5 commits into from
Oct 19, 2022
Merged

feat(explore): Don't discard controls with custom sql when changing datasource #20934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a25c08a
feat(explore): Don't discard controls with custom sql when changing d…
kgabryje Aug 1, 2022
8c55987
Fix lint
kgabryje Aug 1, 2022
1cb86c9
Fix UT
kgabryje Aug 5, 2022
0c3e6c7
Fix ut
kgabryje Oct 17, 2022
8eb5335
Fix label in adhoc metric not updating
kgabryje Oct 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset-frontend/packages/superset-ui-core/src/query/types/Column.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export interface AdhocColumn {
expressionType: 'SQL';
columnType?: 'BASE_AXIS' | 'SERIES';
timeGrain?: string;
datasourceWarning?: boolean;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-core/src/utils/isDefined.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@
* under the License.
*/

export default function isDefined(x: unknown) {
export default function isDefined<T>(x: T): x is NonNullable<T> {
return x !== null && x !== undefined;
}
41 changes: 36 additions & 5 deletions superset-frontend/src/explore/components/ControlPanelsContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
css,
SupersetTheme,
useTheme,
isDefined,
} from '@superset-ui/core';
import {
ControlPanelSectionConfig,
Expand All @@ -45,6 +46,9 @@ import {
ExpandedControlItem,
sections,
} from '@superset-ui/chart-controls';
import { useSelector } from 'react-redux';
import { rgba } from 'emotion-rgba';
import { kebabCase } from 'lodash';

import Collapse from 'src/components/Collapse';
import Tabs from 'src/components/Tabs';
Expand All @@ -57,9 +61,6 @@ import { ExploreActions } from 'src/explore/actions/exploreActions';
import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';

import { rgba } from 'emotion-rgba';
import { kebabCase } from 'lodash';
import ControlRow from './ControlRow';
import Control from './Control';
import { ExploreAlert } from './ExploreAlert';
Expand Down Expand Up @@ -269,6 +270,36 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {

const containerRef = useRef<HTMLDivElement>(null);

const controlsTransferred = useSelector<
ExplorePageState,
string[] | undefined
>(state => state.explore.controlsTransferred);

useEffect(() => {
if (props.chart.chartStatus === 'success') {
controlsTransferred?.forEach(controlName => {
const alteredControls = ensureIsArray(
props.controls[controlName].value,
).map(value => {
if (
typeof value === 'object' &&
isDefined(value) &&
'datasourceWarning' in value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor. Do you think we can have Typescript helping here or maybe setting it in a constant? I am not very fond of using a plain string for datasourceWarning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the typing of that area of code is very poor and value's type is basically any object, so addind datasourceWarning to typing here would require a lot of restructuring which is not a focus of this PR.
However, I think 'fieldName' in objectName is a rather common pattern, we use it in many places in the code, including typescript files

Copy link
Member Author

@kgabryje kgabryje Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be precise, there are 21 occurences across 9 typescript files of this pattern 🙂

) {
return { ...value, datasourceWarning: false };
}
return value;
});
props.actions.setControlValue(controlName, alteredControls);
});
}
}, [
controlsTransferred,
props.actions,
props.chart.chartStatus,
props.controls,
]);

useEffect(() => {
if (
prevDatasource &&
Expand Down Expand Up @@ -455,7 +486,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
>
<Icons.InfoCircleOutlined
css={css`
${iconStyles}
${iconStyles};
color: ${errorColor};
`}
/>
Expand Down Expand Up @@ -591,7 +622,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
>
<Icons.ExclamationCircleOutlined
css={css`
${iconStyles}
${iconStyles};
color: ${errorColor};
`}
/>
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ function ExploreViewContainer(props) {
!areObjectsEqual(
props.controls[key].value,
lastQueriedControls[key].value,
{ ignoreFields: ['datasourceWarning'] },
),
);

Expand Down
6 changes: 6 additions & 0 deletions ...-frontend/src/explore/components/controls/DndColumnSelectControl/DndAdhocFilterOption.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import React from 'react';
import { t } from '@superset-ui/core';
import { DndItemType } from 'src/explore/components/DndItemType';
import AdhocFilterPopoverTrigger from 'src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger';
import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
Expand Down Expand Up @@ -63,6 +64,11 @@ export default function DndAdhocFilterOption({
type={DndItemType.FilterOption}
withCaret
isExtra={adhocFilter.isExtra}
datasourceWarningMessage={
adhocFilter.datasourceWarning
? t('This filter might be incompatible with current dataset')
: undefined
}
/>
</AdhocFilterPopoverTrigger>
);
Expand Down
45 changes: 12 additions & 33 deletions superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import {
isFeatureEnabled,
tn,
QueryFormColumn,
t,
isAdhocColumn,
} from '@superset-ui/core';
import {
ColumnMeta,
Expand All @@ -35,7 +37,6 @@ import OptionWrapper from 'src/explore/components/controls/DndColumnSelectContro
import { OptionSelector } from 'src/explore/components/controls/DndColumnSelectControl/utils';
import { DatasourcePanelDndItem } from 'src/explore/components/DatasourcePanel/types';
import { DndItemType } from 'src/explore/components/DndItemType';
import { useComponentDidUpdate } from 'src/hooks/useComponentDidUpdate';
import ColumnSelectPopoverTrigger from './ColumnSelectPopoverTrigger';
import { DndControlProps } from './types';
import SelectControl from '../SelectControl';
Expand Down Expand Up @@ -68,34 +69,6 @@ function DndColumnSelect(props: DndColumnSelectProps) {
return new OptionSelector(optionsMap, multi, value);
}, [multi, options, value]);

// synchronize values in case of dataset changes
const handleOptionsChange = useCallback(() => {
const optionSelectorValues = optionSelector.getValues();
if (typeof value !== typeof optionSelectorValues) {
onChange(optionSelectorValues);
}
if (
typeof value === 'string' &&
typeof optionSelectorValues === 'string' &&
value !== optionSelectorValues
) {
onChange(optionSelectorValues);
}
if (
Array.isArray(optionSelectorValues) &&
Array.isArray(value) &&
(optionSelectorValues.length !== value.length ||
optionSelectorValues.every((val, index) => val === value[index]))
) {
onChange(optionSelectorValues);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [JSON.stringify(value), JSON.stringify(optionSelector.getValues())]);

// useComponentDidUpdate to avoid running this for the first render, to avoid
// calling onChange when the initial value is not valid for the dataset
useComponentDidUpdate(handleOptionsChange);

const onDrop = useCallback(
(item: DatasourcePanelDndItem) => {
const column = item.value as ColumnMeta;
Expand Down Expand Up @@ -142,8 +115,12 @@ function DndColumnSelect(props: DndColumnSelectProps) {

const valuesRenderer = useCallback(
() =>
optionSelector.values.map((column, idx) =>
clickEnabled ? (
optionSelector.values.map((column, idx) => {
const datasourceWarningMessage =
isAdhocColumn(column) && column.datasourceWarning
? t('This column might be incompatible with current dataset')
: undefined;
return clickEnabled ? (
<ColumnSelectPopoverTrigger
key={idx}
columns={options}
Expand All @@ -166,6 +143,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
type={`${DndItemType.ColumnOption}_${name}_${label}`}
canDelete={canDelete}
column={column}
datasourceWarningMessage={datasourceWarningMessage}
withCaret
/>
</ColumnSelectPopoverTrigger>
Expand All @@ -178,9 +156,10 @@ function DndColumnSelect(props: DndColumnSelectProps) {
type={`${DndItemType.ColumnOption}_${name}_${label}`}
canDelete={canDelete}
column={column}
datasourceWarningMessage={datasourceWarningMessage}
/>
),
),
);
}),
[
canDelete,
clickEnabled,
Expand Down
19 changes: 1 addition & 18 deletions ...-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ test('remove selected custom metric when metric gets removed from dataset', () =
);
expect(screen.getByText('metric_a')).toBeVisible();
expect(screen.queryByText('Metric B')).not.toBeInTheDocument();
expect(screen.queryByText('metric_b')).not.toBeInTheDocument();
expect(screen.getByText('SUM(column_a)')).toBeVisible();
expect(screen.getByText('SUM(Column B)')).toBeVisible();
});
Expand Down Expand Up @@ -171,15 +172,6 @@ test('remove selected custom metric when metric gets removed from dataset for si
],
};

// rerender twice - first to update columns, second to update value
rerender(
<DndMetricSelect
{...newPropsWithRemovedMetric}
value={metricValue}
onChange={onChange}
multi={false}
/>,
);
rerender(
<DndMetricSelect
{...newPropsWithRemovedMetric}
Expand Down Expand Up @@ -220,15 +212,6 @@ test('remove selected adhoc metric when column gets removed from dataset', async
],
};

// rerender twice - first to update columns, second to update value
rerender(
<DndMetricSelect
{...newPropsWithRemovedColumn}
value={metricValues}
onChange={onChange}
multi
/>,
);
rerender(
<DndMetricSelect
{...newPropsWithRemovedColumn}
Expand Down
Loading