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

Jump to bottom

fix(explore): Allow only saved metrics and columns #27539

Merged
merged 1 commit into from
Mar 18, 2024
Merged

fix(explore): Allow only saved metrics and columns #27539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
221 changes: 208 additions & 13 deletions ...-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/
import React from 'react';
import thunk from 'redux-thunk';
import { Provider } from 'react-redux';
import configureStore from 'redux-mock-store';

import {
Expand All @@ -29,7 +28,13 @@ import {
import { ColumnMeta } from '@superset-ui/chart-controls';
import { TimeseriesDefaultFormData } from '@superset-ui/plugin-chart-echarts';

import { render, screen } from 'spec/helpers/testing-library';
import {
fireEvent,
render,
screen,
within,
} from 'spec/helpers/testing-library';
import type { AsyncAceEditorProps } from 'src/components/AsyncAceEditor';
import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
import {
Expand All @@ -38,13 +43,21 @@ import {
} from 'src/explore/components/controls/DndColumnSelectControl/DndFilterSelect';
import { PLACEHOLDER_DATASOURCE } from 'src/dashboard/constants';
import { ExpressionTypes } from '../FilterControl/types';
import { Datasource } from '../../../types';
import { DndItemType } from '../../DndItemType';
import DatasourcePanelDragOption from '../../DatasourcePanel/DatasourcePanelDragOption';

jest.mock('src/components/AsyncAceEditor', () => ({
SQLEditor: (props: AsyncAceEditorProps) => (
<div data-test="react-ace">{props.value}</div>
),
}));

const defaultProps: DndFilterSelectProps = {
const defaultProps: Omit<DndFilterSelectProps, 'datasource'> = {
type: 'DndFilterSelect',
name: 'Filter',
value: [],
columns: [],
datasource: PLACEHOLDER_DATASOURCE,
formData: null,
savedMetrics: [],
selectedMetrics: [],
Expand All @@ -64,25 +77,26 @@ function setup({
value = undefined,
formData = baseFormData,
columns = [],
datasource = PLACEHOLDER_DATASOURCE,
}: {
value?: AdhocFilter;
formData?: QueryFormData;
columns?: ColumnMeta[];
datasource?: Datasource;
} = {}) {
return (
<Provider store={store}>
<DndFilterSelect
{...defaultProps}
value={ensureIsArray(value)}
formData={formData}
columns={columns}
/>
</Provider>
<DndFilterSelect
{...defaultProps}
datasource={datasource}
value={ensureIsArray(value)}
formData={formData}
columns={columns}
/>
);
}

test('renders with default props', async () => {
render(setup(), { useDnd: true });
render(setup(), { useDnd: true, store });
expect(
await screen.findByText('Drop columns/metrics here or click'),
).toBeInTheDocument();
Expand All @@ -95,6 +109,7 @@ test('renders with value', async () => {
});
render(setup({ value }), {
useDnd: true,
store,
});
expect(await screen.findByText('COUNT(*)')).toBeInTheDocument();
});
Expand All @@ -110,6 +125,7 @@ test('renders options with saved metric', async () => {
}),
{
useDnd: true,
store,
},
);
expect(
Expand All @@ -131,6 +147,7 @@ test('renders options with column', async () => {
}),
{
useDnd: true,
store,
},
);
expect(
Expand All @@ -153,9 +170,187 @@ test('renders options with adhoc metric', async () => {
}),
{
useDnd: true,
store,
},
);
expect(
await screen.findByText('Drop columns/metrics here or click'),
).toBeInTheDocument();
});

test('cannot drop a column that is not part of the simple column selection', () => {
Copy link
Member Author

@justinpark justinpark Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kgabryje I also added the spec for your case (#27209). Please let me know if you have any issues.

const adhocMetric = new AdhocMetric({
expression: 'AVG(birth_names.num)',
metric_name: 'avg__num',
});
const { getByTestId, getAllByTestId } = render(
<>
<DatasourcePanelDragOption
value={{ column_name: 'order_date' }}
type={DndItemType.Column}
/>
<DatasourcePanelDragOption
value={{ column_name: 'address_line1' }}
type={DndItemType.Column}
/>
<DatasourcePanelDragOption
value={{ metric_name: 'metric_a', expression: 'AGG(metric_a)' }}
type={DndItemType.Metric}
/>
{setup({
formData: {
...baseFormData,
...TimeseriesDefaultFormData,
metrics: [adhocMetric],
},
columns: [{ column_name: 'order_date' }],
})}
</>,
{
useDnd: true,
store,
},
);

const selections = getAllByTestId('DatasourcePanelDragOption');
const acceptableColumn = selections[0];
const unacceptableColumn = selections[1];
const metricType = selections[2];
const currentMetric = getByTestId('dnd-labels-container');

fireEvent.dragStart(unacceptableColumn);
fireEvent.dragOver(currentMetric);
fireEvent.drop(currentMetric);

expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument();

fireEvent.dragStart(acceptableColumn);
fireEvent.dragOver(currentMetric);
fireEvent.drop(currentMetric);

const filterConfigPopup = screen.getByTestId('filter-edit-popover');
expect(within(filterConfigPopup).getByText('order_date')).toBeInTheDocument();

fireEvent.keyDown(filterConfigPopup, {
key: 'Escape',
code: 'Escape',
keyCode: 27,
charCode: 27,
});
expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument();

fireEvent.dragStart(metricType);
fireEvent.dragOver(currentMetric);
fireEvent.drop(currentMetric);

expect(
within(screen.getByTestId('filter-edit-popover')).getByTestId('react-ace'),
).toHaveTextContent('AGG(metric_a)');
});

describe('when disallow_adhoc_metrics is set', () => {
test('can drop a column type from the simple column selection', () => {
const adhocMetric = new AdhocMetric({
expression: 'AVG(birth_names.num)',
metric_name: 'avg__num',
});
const { getByTestId } = render(
<>
<DatasourcePanelDragOption
value={{ column_name: 'column_b' }}
type={DndItemType.Column}
/>
{setup({
formData: {
...baseFormData,
...TimeseriesDefaultFormData,
metrics: [adhocMetric],
},
datasource: {
...PLACEHOLDER_DATASOURCE,
extra: '{ "disallow_adhoc_metrics": true }',
},
columns: [{ column_name: 'column_a' }, { column_name: 'column_b' }],
})}
</>,
{
useDnd: true,
store,
},
);

const acceptableColumn = getByTestId('DatasourcePanelDragOption');
const currentMetric = getByTestId('dnd-labels-container');

fireEvent.dragStart(acceptableColumn);
fireEvent.dragOver(currentMetric);
fireEvent.drop(currentMetric);

const filterConfigPopup = screen.getByTestId('filter-edit-popover');
expect(within(filterConfigPopup).getByText('column_b')).toBeInTheDocument();
});

test('cannot drop any other types of selections apart from simple column selection', () => {
const adhocMetric = new AdhocMetric({
expression: 'AVG(birth_names.num)',
metric_name: 'avg__num',
});
const { getByTestId, getAllByTestId } = render(
<>
<DatasourcePanelDragOption
value={{ column_name: 'column_c' }}
type={DndItemType.Column}
/>
<DatasourcePanelDragOption
value={{ metric_name: 'metric_a' }}
type={DndItemType.Metric}
/>
<DatasourcePanelDragOption
value={{ metric_name: 'avg__num' }}
type={DndItemType.AdhocMetricOption}
/>
{setup({
formData: {
...baseFormData,
...TimeseriesDefaultFormData,
metrics: [adhocMetric],
},
datasource: {
...PLACEHOLDER_DATASOURCE,
extra: '{ "disallow_adhoc_metrics": true }',
},
columns: [{ column_name: 'column_a' }, { column_name: 'column_c' }],
})}
</>,
{
useDnd: true,
store,
},
);

const selections = getAllByTestId('DatasourcePanelDragOption');
const acceptableColumn = selections[0];
const unacceptableMetric = selections[1];
const unacceptableType = selections[2];
const currentMetric = getByTestId('dnd-labels-container');

fireEvent.dragStart(unacceptableMetric);
fireEvent.dragOver(currentMetric);
fireEvent.drop(currentMetric);

expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument();

fireEvent.dragStart(unacceptableType);
fireEvent.dragOver(currentMetric);
fireEvent.drop(currentMetric);

expect(screen.queryByTestId('filter-edit-popover')).not.toBeInTheDocument();

fireEvent.dragStart(acceptableColumn);
fireEvent.dragOver(currentMetric);
fireEvent.drop(currentMetric);

const filterConfigPopup = screen.getByTestId('filter-edit-popover');
expect(within(filterConfigPopup).getByText('column_c')).toBeInTheDocument();
});
});
36 changes: 32 additions & 4 deletions superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,16 @@ const DndFilterSelect = (props: DndFilterSelectProps) => {
canDelete,
} = props;

const extra = useMemo<{ disallow_adhoc_metrics?: boolean }>(() => {
let extra = {};
if (datasource?.extra) {
try {
extra = JSON.parse(datasource.extra);
} catch {} // eslint-disable-line no-empty
}
return extra;
}, [datasource?.extra]);

const propsValues = Array.from(props.value ?? []);
const [values, setValues] = useState(
propsValues.map((filter: OptionValueType) =>
Expand Down Expand Up @@ -149,6 +159,17 @@ const DndFilterSelect = (props: DndFilterSelectProps) => {
optionsForSelect(props.columns, props.formData),
);

const availableColumnSet = useMemo(
() =>
new Set(
options.map(
({ column_name, filterOptionName }) =>
column_name ?? filterOptionName,
),
),
[options],
);

useEffect(() => {
if (datasource && datasource.type === 'table') {
const dbId = datasource.database?.id;
Expand Down Expand Up @@ -384,14 +405,21 @@ const DndFilterSelect = (props: DndFilterSelectProps) => {

const canDrop = useCallback(
(item: DatasourcePanelDndItem) => {
if (
extra.disallow_adhoc_metrics &&
(item.type !== DndItemType.Column ||
!availableColumnSet.has((item.value as ColumnMeta).column_name))
) {
return false;
}

if (item.type === DndItemType.Column) {
return props.columns.some(
col => col.column_name === (item.value as ColumnMeta).column_name,
);
const columnName = (item.value as ColumnMeta).column_name;
return availableColumnSet.has(columnName);
Copy link
Member Author

Choose a reason for hiding this comment

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

@kgabryje I made a change from #27209 to improve the array iteration check by utilizing a hashset, which offers better performance and optimization.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thank you!

}
return true;
},
[props.columns],
[availableColumnSet, extra],
);

const handleDrop = useCallback(
Expand Down
Loading
Loading