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

fix(explore): Prevent shared controls from checking feature flags outside React render #21315

Merged
merged 3 commits into from
Sep 14, 2022
Merged

fix(explore): Prevent shared controls from checking feature flags outside React render #21315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5964987
WIP on DnD fix.
codyml Sep 2, 2022
ad0b10a
Fix Table chart, trigger CI, trigger CI.
codyml Sep 2, 2022
3d47b7d
Add explanatory comment.
codyml Sep 12, 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
6 changes: 1 addition & 5 deletions superset-frontend/packages/superset-ui-chart-controls/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@ export * from './components/ColumnTypeLabel/ColumnTypeLabel';
export * from './components/MetricOption';

// React control components
export {
sharedControls,
dndEntity,
dndColumnsControl,
} from './shared-controls';
export { default as sharedControls, withDndFallback } from './shared-controls';
export { default as sharedControlComponents } from './shared-controls/components';
export { legacySortBy } from './shared-controls/legacySortBy';
export * from './shared-controls/emitFilterControl';
Expand Down
167 changes: 116 additions & 51 deletions superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useMemo } from 'react';
import {
FeatureFlag,
isFeatureEnabled,
Expand All @@ -25,43 +26,83 @@ import {
t,
validateNonEmpty,
} from '@superset-ui/core';
import { ExtraControlProps, SharedControlConfig, Dataset } from '../types';
import {
ExtraControlProps,
SharedControlConfig,
Dataset,
Metric,
} from '../types';
import { DATASET_TIME_COLUMN_OPTION, TIME_FILTER_LABELS } from '../constants';
import { QUERY_TIME_COLUMN_OPTION, defineSavedMetrics } from '..';
import {
QUERY_TIME_COLUMN_OPTION,
defineSavedMetrics,
ColumnOption,
ColumnMeta,
FilterOption,
} from '..';
import { xAxisControlConfig } from './constants';

export const dndGroupByControl: SharedControlConfig<'DndColumnSelect'> = {
type Control = {
savedMetrics?: Metric[] | null;
default?: unknown;
};

/*
* Note: Previous to the commit that introduced this comment, the shared controls module
* would check feature flags at module execution time and expose a different control
* configuration (component + props) depending on the status of drag-and-drop feature
* flags. This commit combines those configs, merging the required props for both the
* drag-and-drop and non-drag-and-drop components, and renders a wrapper component that
* checks feature flags at component render time to avoid race conditions between when
* feature flags are set and when they're checked.
*/

export const dndGroupByControl: SharedControlConfig<
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment that will explain the idea behind using props from non-dnd component in dnd control definition? Might be useful for other contributors in the future

'DndColumnSelect' | 'SelectControl',
ColumnMeta
> = {
type: 'DndColumnSelect',
label: t('Dimensions'),
multi: true,
freeForm: true,
clearable: true,
default: [],
includeTime: false,
description: t(
'One or many columns to group by. High cardinality groupings should include a series limit ' +
'to limit the number of fetched and rendered series.',
'One or many columns to group by. High cardinality groupings should include a sort by metric ' +
'and series limit to limit the number of fetched and rendered series.',
),
mapStateToProps(state, { includeTime }) {
optionRenderer: (c: ColumnMeta) => <ColumnOption showType column={c} />,
valueRenderer: (c: ColumnMeta) => <ColumnOption column={c} />,
valueKey: 'column_name',
allowAll: true,
filterOption: ({ data: opt }: FilterOption<ColumnMeta>, text: string) =>
(opt.column_name &&
opt.column_name.toLowerCase().includes(text.toLowerCase())) ||
(opt.verbose_name &&
opt.verbose_name.toLowerCase().includes(text.toLowerCase())) ||
false,
promptTextCreator: (label: unknown) => label,
mapStateToProps(state, controlState) {
const newState: ExtraControlProps = {};
const { datasource } = state;
if (datasource?.columns[0]?.hasOwnProperty('groupby')) {
const options = (datasource as Dataset).columns.filter(c => c.groupby);
if (includeTime) {
if (controlState?.includeTime) {
options.unshift(DATASET_TIME_COLUMN_OPTION);
}
newState.options = Object.fromEntries(
options.map(option => [option.column_name, option]),
);
newState.options = options;
newState.savedMetrics = (datasource as Dataset).metrics || [];
} else {
const options = datasource?.columns;
if (includeTime) {
(options as QueryColumn[])?.unshift(QUERY_TIME_COLUMN_OPTION);
const options = (datasource?.columns as QueryColumn[]) || [];
if (controlState?.includeTime) {
options.unshift(QUERY_TIME_COLUMN_OPTION);
}
newState.options = Object.fromEntries(
(options as QueryColumn[])?.map(option => [option.name, option]),
);
newState.options = datasource?.columns;
newState.options = options;
zhaoyongjie marked this conversation as resolved.
Show resolved Hide resolved
}
return newState;
},
commaChoosesOption: false,
};

export const dndColumnsControl: typeof dndGroupByControl = {
Expand All @@ -70,7 +111,7 @@ export const dndColumnsControl: typeof dndGroupByControl = {
description: t('One or many columns to pivot as columns'),
};

export const dndSeries: typeof dndGroupByControl = {
export const dndSeriesControl: typeof dndGroupByControl = {
...dndGroupByControl,
label: t('Dimension'),
multi: false,
Expand All @@ -82,7 +123,7 @@ export const dndSeries: typeof dndGroupByControl = {
),
};

export const dndEntity: typeof dndGroupByControl = {
export const dndEntityControl: typeof dndGroupByControl = {
...dndGroupByControl,
label: t('Entity'),
default: null,
Expand All @@ -91,7 +132,9 @@ export const dndEntity: typeof dndGroupByControl = {
description: t('This defines the element to be plotted on the chart'),
};

export const dnd_adhoc_filters: SharedControlConfig<'DndFilterSelect'> = {
export const dndAdhocFilterControl: SharedControlConfig<
'DndFilterSelect' | 'AdhocFilterControl'
> = {
type: 'DndFilterSelect',
label: t('Filters'),
default: [],
Expand All @@ -109,7 +152,9 @@ export const dnd_adhoc_filters: SharedControlConfig<'DndFilterSelect'> = {
provideFormDataToProps: true,
};

export const dnd_adhoc_metrics: SharedControlConfig<'DndMetricSelect'> = {
export const dndAdhocMetricsControl: SharedControlConfig<
'DndMetricSelect' | 'MetricsControl'
> = {
type: 'DndMetricSelect',
multi: true,
label: t('Metrics'),
Expand All @@ -123,20 +168,23 @@ export const dnd_adhoc_metrics: SharedControlConfig<'DndMetricSelect'> = {
description: t('One or many metrics to display'),
};

export const dnd_adhoc_metric: SharedControlConfig<'DndMetricSelect'> = {
...dnd_adhoc_metrics,
export const dndAdhocMetricControl: typeof dndAdhocMetricsControl = {
...dndAdhocMetricsControl,
multi: false,
label: t('Metric'),
description: t('Metric'),
};

export const dnd_adhoc_metric_2: SharedControlConfig<'DndMetricSelect'> = {
...dnd_adhoc_metric,
export const dndAdhocMetricControl2: typeof dndAdhocMetricControl = {
...dndAdhocMetricControl,
label: t('Right Axis Metric'),
clearable: true,
description: t('Choose a metric for right axis'),
};

export const dnd_sort_by: SharedControlConfig<'DndMetricSelect'> = {
export const dndSortByControl: SharedControlConfig<
'DndMetricSelect' | 'MetricsControl'
> = {
type: 'DndMetricSelect',
label: t('Sort by'),
default: null,
Expand All @@ -152,33 +200,37 @@ export const dnd_sort_by: SharedControlConfig<'DndMetricSelect'> = {
}),
};

export const dnd_size: SharedControlConfig<'DndMetricSelect'> = {
...dnd_adhoc_metric,
export const dndSizeControl: typeof dndAdhocMetricControl = {
...dndAdhocMetricControl,
label: t('Bubble Size'),
description: t('Metric used to calculate bubble size'),
default: null,
};

export const dnd_x: SharedControlConfig<'DndMetricSelect'> = {
...dnd_adhoc_metric,
export const dndXControl: typeof dndAdhocMetricControl = {
...dndAdhocMetricControl,
label: t('X Axis'),
description: t('Metric assigned to the [X] axis'),
default: null,
};

export const dnd_y: SharedControlConfig<'DndMetricSelect'> = {
...dnd_adhoc_metric,
export const dndYControl: typeof dndAdhocMetricControl = {
...dndAdhocMetricControl,
label: t('Y Axis'),
description: t('Metric assigned to the [Y] axis'),
default: null,
};

export const dnd_secondary_metric: SharedControlConfig<'DndMetricSelect'> = {
...dnd_adhoc_metric,
export const dndSecondaryMetricControl: typeof dndAdhocMetricControl = {
...dndAdhocMetricControl,
label: t('Color Metric'),
default: null,
validators: [],
description: t('A metric to use for color'),
};

export const dnd_granularity_sqla: typeof dndGroupByControl = {
...dndSeries,
export const dndGranularitySqlaControl: typeof dndSeriesControl = {
...dndSeriesControl,
label: TIME_FILTER_LABELS.granularity_sqla,
description: t(
'The time column for the visualization. Note that you ' +
Expand All @@ -187,44 +239,57 @@ export const dnd_granularity_sqla: typeof dndGroupByControl = {
'filter below is applied against this column or ' +
'expression',
),
default: (c: Control) => c.default,
clearable: false,
canDelete: false,
ghostButtonText: t(
isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX)
? 'Drop a temporal column here or click'
: 'Drop temporal column here',
),
ghostButtonText: t('Drop temporal column here'),
clickEnabledGhostButtonText: t('Drop a temporal column here or click'),
optionRenderer: (c: ColumnMeta) => <ColumnOption showType column={c} />,
valueRenderer: (c: ColumnMeta) => <ColumnOption column={c} />,
valueKey: 'column_name',
mapStateToProps: ({ datasource }) => {
if (datasource?.columns[0]?.hasOwnProperty('column_name')) {
const temporalColumns =
(datasource as Dataset)?.columns?.filter(c => c.is_dttm) ?? [];
const options = Object.fromEntries(
temporalColumns.map(option => [option.column_name, option]),
);
return {
options,
options: temporalColumns,
default:
(datasource as Dataset)?.main_dttm_col ||
temporalColumns[0]?.column_name ||
null,
isTemporal: true,
};
}

const sortedQueryColumns = (datasource as QueryResponse)?.columns?.sort(
query => (query?.is_dttm ? -1 : 1),
);
const options = Object.fromEntries(
sortedQueryColumns.map(option => [option.name, option]),
);
return {
options,
options: sortedQueryColumns,
default: sortedQueryColumns[0]?.name || null,
isTemporal: true,
};
},
};

export const dnd_x_axis: SharedControlConfig<'DndColumnSelect'> = {
export const dndXAxisControl: typeof dndGroupByControl = {
...dndGroupByControl,
...xAxisControlConfig,
};

export function withDndFallback(
DndComponent: React.ComponentType<any>,
FallbackComponent: React.ComponentType<any>,
) {
return function DndControl(props: any) {
const enableExploreDnd = useMemo(
() => isFeatureEnabled(FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP),
[],
);

return enableExploreDnd ? (
<DndComponent {...props} />
) : (
<FallbackComponent {...props} />
);
};
}
Loading