rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3484281008
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -74,270 +81,317 @@ function createKeyedCollection(arr: Array<object>) {
};
}
-export default class CRUDCollection extends PureComponent<
- CRUDCollectionProps,
- CRUDCollectionState
-> {
- constructor(props: CRUDCollectionProps) {
- super(props);
-
- const { collection, collectionArray } = createKeyedCollection(
- props.collection,
- );
-
- // Get initial page size from pagination prop
- const initialPageSize =
- typeof props.pagination === 'object' && props.pagination?.pageSize
- ? props.pagination.pageSize
- : 10;
-
- this.state = {
- expandedColumns: {},
- collection,
- collectionArray,
- sortColumn: '',
- sort: 0,
- currentPage: 1,
- pageSize: initialPageSize,
- };
- this.onAddItem = this.onAddItem.bind(this);
- this.renderExpandableSection = this.renderExpandableSection.bind(this);
- this.getLabel = this.getLabel.bind(this);
- this.onFieldsetChange = this.onFieldsetChange.bind(this);
- this.changeCollection = this.changeCollection.bind(this);
- this.handleTableChange = this.handleTableChange.bind(this);
- this.buildTableColumns = this.buildTableColumns.bind(this);
- this.toggleExpand = this.toggleExpand.bind(this);
+export default function CRUDCollection({
+ allowAddItem = false,
+ allowDeletes = false,
+ collection: propsCollection,
+ columnLabels,
+ columnLabelTooltips,
+ emptyMessage = t('No items'),
+ expandFieldset,
+ itemGenerator,
+ itemCellProps,
+ itemRenderers,
+ onChange,
+ tableColumns,
+ sortColumns = [],
+ stickyHeader = false,
+ pagination = false,
+ filterTerm,
+ filterFields,
+}: CRUDCollectionProps) {
+ const [expandedColumns, setExpandedColumns] = useState<
+ Record<PropertyKey, boolean>
+ >({});
+ // Seed both pieces of state from a single createKeyedCollection() pass so
+ // that items lacking an `id` get one consistent set of synthetic ids
+ // (matching the prior class component, which keyed the collection once).
+ const initialKeyed = useRef<ReturnType<typeof createKeyedCollection>>();
+ if (!initialKeyed.current) {
+ initialKeyed.current = createKeyedCollection(propsCollection);
}
+ const [collection, setCollection] = useState<
+ Record<PropertyKey, CollectionItem>
+ >(() => initialKeyed.current!.collection);
+ const [collectionArray, setCollectionArray] = useState<CollectionItem[]>(
+ () => initialKeyed.current!.collectionArray,
+ );
+ const [sortColumn, setSortColumn] = useState<string>('');
+ const [sort, setSort] = useState<SortOrderEnum>(SortOrderEnum.Unsorted);
+ // Controlled pagination: tracked so that filtering can clamp currentPage
+ // back to a valid page (avoids the user being stranded on an empty page
+ // when filterTerm shrinks the result set).
+ const [pageSize, setPageSize] = useState<number>(() =>
+ typeof pagination === 'object' && pagination?.pageSize
+ ? pagination.pageSize
+ : 10,
+ );
+ const [currentPage, setCurrentPage] = useState<number>(1);
+
+ // Sync with props.collection changes
+ useEffect(() => {
+ const { collection: newCollection, collectionArray: newCollectionArray } =
+ createKeyedCollection(propsCollection);
+ setCollection(newCollection);
+ setCollectionArray(newCollectionArray);
+ }, [propsCollection]);
+
+ const onCellChange = useCallback(
+ (id: string | number, col: string, val: unknown) => {
+ setCollection(prevCollection => {
+ const updatedCollection = {
+ ...prevCollection,
+ [id]: {
+ ...prevCollection[id],
+ [col]: val,
+ },
+ };
+ return updatedCollection;
+ });
- componentDidUpdate(prevProps: CRUDCollectionProps) {
- if (this.props.collection !== prevProps.collection) {
- const { collection, collectionArray } = createKeyedCollection(
- this.props.collection,
- );
+ setCollectionArray(prevCollectionArray => {
+ const updatedCollectionArray = prevCollectionArray.map(item => {
+ if (item.id === id) {
+ return {
+ ...item,
+ [col]: val,
+ };
+ }
+ return item;
+ });
- this.setState(prevState => ({
- collection,
- collectionArray,
- expandedColumns: prevState.expandedColumns,
- }));
- }
- }
+ if (onChange) {
+ onChange(updatedCollectionArray);
+ }
- onCellChange(id: string | number, col: string, val: unknown) {
- this.setState(prevState => {
- const updatedCollection = {
- ...prevState.collection,
- [id]: {
- ...prevState.collection[id],
- [col]: val,
- },
- };
- const updatedCollectionArray = prevState.collectionArray.map(item =>
- item.id === id ? updatedCollection[id] : item,
- );
+ return updatedCollectionArray;
+ });
+ },
+ [onChange],
+ );
- if (this.props.onChange) {
- this.props.onChange(updatedCollectionArray);
+ const changeCollection = useCallback(
+ (
+ newCollection: Record<PropertyKey, CollectionItem>,
+ currentCollectionArray: CollectionItem[],
+ ) => {
+ // Preserve existing order instead of recreating from Object.keys()
+ const existingIds = new Set(currentCollectionArray.map(item => item.id));
+ const newCollectionArray: CollectionItem[] = [];
+
+ // First pass: preserve existing order and update items
+ for (const existingItem of currentCollectionArray) {
+ if (newCollection[existingItem.id]) {
+ newCollectionArray.push(newCollection[existingItem.id]);
+ }
}
- return {
- collection: updatedCollection,
- collectionArray: updatedCollectionArray,
- };
- });
- }
- onAddItem() {
- if (this.props.itemGenerator) {
- let newItem = this.props.itemGenerator();
- const shouldStartExpanded = newItem.expanded === true;
- if (!newItem.id) {
- newItem = { ...newItem, id: nanoid() };
+ // Second pass: add new items
+ for (const item of Object.values(newCollection)) {
+ if (!existingIds.has(item.id)) {
+ newCollectionArray.push(item);
+ }
}
- delete newItem.expanded;
- this.setState(
- prevState => {
- const newCollection = {
- ...prevState.collection,
- [newItem.id]: newItem,
- };
- const newExpandedColumns = shouldStartExpanded
- ? { ...prevState.expandedColumns, [newItem.id]: true }
- : prevState.expandedColumns;
- const newCollectionArray = [newItem, ...prevState.collectionArray];
-
- return {
- collection: newCollection,
- collectionArray: newCollectionArray,
- expandedColumns: newExpandedColumns,
- };
- },
- () => {
- if (this.props.onChange) {
- this.props.onChange(this.state.collectionArray);
- }
- },
- );
- }
- }
+ setCollection(newCollection);
+ setCollectionArray(newCollectionArray);
- onFieldsetChange(item: any) {
- this.changeCollection({
- ...this.state.collection,
- [item.id]: item,
- });
- }
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
+ },
+ [onChange],
+ );
- getLabel(col: any): string {
- const { columnLabels } = this.props;
- let label = columnLabels?.[col] ? columnLabels[col] : col;
- if (label.startsWith('__')) {
- label = '';
- }
- return label;
- }
+ const deleteItem = useCallback(
+ (id: string | number) => {
+ setCollection(prevCollection => {
+ const newColl = { ...prevCollection };
+ delete newColl[id];
+ return newColl;
+ });
- getTooltip(col: string): string | undefined {
- const { columnLabelTooltips } = this.props;
- return columnLabelTooltips?.[col];
- }
+ setCollectionArray(prevCollectionArray => {
+ const newCollectionArray = prevCollectionArray.filter(
+ item => item.id !== id,
+ );
- changeCollection(collection: any) {
- // Preserve existing order instead of recreating from Object.keys()
- const existingIds = new Set(
- this.state.collectionArray.map(item => item.id),
- );
- const newCollectionArray: CollectionItem[] = [];
-
- // First pass: preserve existing order and update items
- for (const existingItem of this.state.collectionArray) {
- if (collection[existingItem.id]) {
- newCollectionArray.push(collection[existingItem.id]);
- }
- }
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
- // Second pass: add new items
- for (const item of Object.values(collection) as CollectionItem[]) {
- if (!existingIds.has(item.id)) {
- newCollectionArray.push(item);
- }
- }
+ return newCollectionArray;
+ });
+ },
+ [onChange],
+ );
- this.setState({ collection, collectionArray: newCollectionArray });
+ const onAddItem = useCallback(() => {
+ if (itemGenerator) {
+ let newItem = itemGenerator() as CollectionItem;
+ const shouldStartExpanded = newItem.expanded === true;
+ if (!newItem.id) {
+ newItem = { ...newItem, id: nanoid() };
+ }
Review Comment:
Good catch - `createKeyedCollection` already moved to `id != null`, so I
aligned `onAddItem` to match. Pushed.
##########
superset-frontend/src/components/CopyToClipboard/index.tsx:
##########
@@ -16,129 +16,156 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Component, cloneElement, ReactElement } from 'react';
+import {
+ cloneElement,
+ isValidElement,
+ type KeyboardEvent,
+ ReactElement,
+ useCallback,
+} from 'react';
import { t } from '@apache-superset/core/translation';
import { css, SupersetTheme } from '@apache-superset/core/theme';
import copyTextToClipboard from 'src/utils/copy';
import { Tooltip } from '@superset-ui/core/components';
import withToasts from '../MessageToasts/withToasts';
import type { CopyToClipboardProps } from './types';
-const defaultProps: Partial<CopyToClipboardProps> = {
- copyNode: <span>{t('Copy')}</span>,
- onCopyEnd: () => {},
- shouldShowText: true,
- wrapped: true,
- tooltipText: t('Copy to clipboard'),
- hideTooltip: false,
-};
+function CopyToClip({
+ copyNode = <span>{t('Copy')}</span>,
+ onCopyEnd = () => {},
+ shouldShowText = true,
+ wrapped = true,
+ tooltipText = t('Copy to clipboard'),
+ hideTooltip = false,
+ disabled,
+ getText,
+ text,
+ addSuccessToast,
+ addDangerToast,
+}: CopyToClipboardProps) {
+ const copyToClipboard = useCallback(
+ (textToCopy: Promise<string>) => {
+ copyTextToClipboard(() => textToCopy)
+ .then(() => {
+ addSuccessToast(t('Copied to clipboard!'));
+ })
+ .catch(() => {
+ addDangerToast(
+ t(
+ 'Sorry, your browser does not support copying. Use Ctrl / Cmd +
C!',
+ ),
+ );
+ })
+ .finally(() => {
+ if (onCopyEnd) onCopyEnd();
+ });
+ },
+ [addSuccessToast, addDangerToast, onCopyEnd],
+ );
-class CopyToClip extends Component<CopyToClipboardProps> {
- static defaultProps = defaultProps;
-
- constructor(props: CopyToClipboardProps) {
- super(props);
- this.copyToClipboard = this.copyToClipboard.bind(this);
- this.onClick = this.onClick.bind(this);
- }
-
- onClick() {
- if (this.props.disabled) {
+ const onClick = useCallback(() => {
+ if (disabled) {
return;
}
- if (this.props.getText) {
- this.props.getText((d: string) => {
- this.copyToClipboard(Promise.resolve(d));
+ if (getText) {
+ getText((d: string) => {
+ copyToClipboard(Promise.resolve(d));
});
} else {
- this.copyToClipboard(Promise.resolve(this.props.text || ''));
+ copyToClipboard(Promise.resolve(text || ''));
}
- }
-
- getDecoratedCopyNode() {
- const copyNode = this.props.copyNode as ReactElement;
- const { disabled } = this.props;
- return cloneElement(copyNode, {
- style: {
- ...copyNode.props.style,
- cursor: disabled ? 'not-allowed' : 'pointer',
- },
- onClick: disabled ? undefined : this.onClick,
- 'aria-disabled': disabled || undefined,
- tabIndex: disabled ? -1 : copyNode.props.tabIndex,
- });
- }
+ }, [disabled, getText, text, copyToClipboard]);
- copyToClipboard(textToCopy: Promise<string>) {
- copyTextToClipboard(() => textToCopy)
- .then(() => {
- this.props.addSuccessToast(t('Copied to clipboard!'));
- })
- .catch(() => {
- this.props.addDangerToast(
- t(
- 'Sorry, your browser does not support copying. Use Ctrl / Cmd +
C!',
- ),
- );
- })
- .finally(() => {
- if (this.props.onCopyEnd) this.props.onCopyEnd();
+ const getDecoratedCopyNode = useCallback(() => {
+ const cursor = disabled ? 'not-allowed' : 'pointer';
+ if (isValidElement(copyNode)) {
+ const node = copyNode as ReactElement;
Review Comment:
This `as ReactElement` cast is verbatim from master - the FC port just
carried it over, so it predates this conversion rather than being introduced
here.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2345,299 +2359,289 @@ class DatasourceEditor extends PureComponent<
/>
</div>
);
- }
+ }, [datasource, sortMetrics, onDatasourcePropChange, metricSearchTerm]);
- render() {
- const { datasource, activeTabKey } = this.state;
- const { metrics } = datasource;
- const sortedMetrics = metrics?.length ? this.sortMetrics(metrics) : [];
+ const sortedMetrics = useMemo(
+ () => (datasource.metrics?.length ? sortMetrics(datasource.metrics) : []),
+ [datasource.metrics, sortMetrics],
+ );
- return (
- <DatasourceContainer data-test="datasource-editor">
- {this.renderErrors()}
- <Alert
- css={theme => ({ marginBottom: theme.sizeUnit * 4 })}
- type="warning"
- message={
- <>
- {' '}
- <strong>{t('Be careful.')} </strong>
- {t(
- 'Changing these settings will affect all charts using this
dataset, including charts owned by other people.',
- )}
- </>
- }
+ // Retained to mirror the canonical (class-based) component on master: the
+ // Spatial tab definition is kept available even though it is not wired into
+ // the rendered tab list. Removing it would also drop its translatable
strings
+ // and regress existing translations. It is referenced in the `tabItems`
+ // dependency list below so it is not reported as an unused local.
+ const renderSpatialTab = useCallback(() => {
+ const { spatials, all_cols: allCols } = datasource;
+
+ return {
+ key: TABS_KEYS.SPATIAL,
+ label: <CollectionTabTitle collection={spatials} title={t('Spatial')} />,
+ children: (
+ <CollectionTable
+ tableColumns={['name', 'config']}
+ sortColumns={['name']}
+ onChange={value => onDatasourcePropChange('spatials', value)}
+ itemGenerator={() => ({
+ name: t('<new spatial>'),
+ type: t('<no type>'),
+ config: null,
+ })}
+ collection={spatials ?? []}
+ allowDeletes
+ itemRenderers={{
+ name: (d, onChange) => (
+ <EditableTitle
+ canEdit
+ title={d as string}
+ onSaveTitle={onChange}
+ />
+ ),
+ config: (v, onChange) => (
+ <SpatialControl
+ value={
+ v as {
+ type: 'latlong' | 'delimited' | 'geohash';
+ }
+ }
+ onChange={onChange}
+ choices={allCols?.map(col => [col, col] as [string, string])}
+ />
+ ),
+ }}
/>
- <StyledTableTabs
- id="table-tabs"
- data-test="edit-dataset-tabs"
- onChange={this.handleTabSelect}
- defaultActiveKey={activeTabKey}
- items={[
- {
- key: TABS_KEYS.SOURCE,
- label: t('Source'),
- children: this.renderSourceFieldset(),
- },
- {
- key: TABS_KEYS.METRICS,
- label: (
- <CollectionTabTitle
- collection={sortedMetrics}
- title={t('Metrics')}
- />
- ),
- children: this.renderMetricCollection(),
- },
- {
- key: TABS_KEYS.COLUMNS,
- label: (
- <CollectionTabTitle
- collection={this.state.databaseColumns}
- title={t('Columns')}
- />
- ),
- children: (
- <StyledTableTabWrapper>
- {this.renderDefaultColumnSettings()}
- <ColumnButtonWrapper>
- <StyledButtonWrapper>
- <Button
- buttonSize="small"
- buttonStyle="tertiary"
- onClick={this.syncMetadata}
- className="sync-from-source"
- disabled={this.state.isEditMode}
- >
- <Icons.DatabaseOutlined iconSize="m" />
- {t('Sync columns from source')}
- </Button>
- </StyledButtonWrapper>
- </ColumnButtonWrapper>
- <Input.Search
- placeholder={t('Search columns by name')}
- value={this.state.columnSearchTerm}
- onChange={e =>
- this.setState({ columnSearchTerm: e.target.value })
- }
- style={{ marginBottom: 16, width: 300 }}
- allowClear
- />
- <ColumnCollectionTable
- className="columns-table"
- columns={this.state.databaseColumns}
- filterTerm={this.state.columnSearchTerm}
- filterFields={['column_name']}
- datasource={datasource}
- onColumnsChange={databaseColumns =>
- this.setColumns({ databaseColumns })
- }
- onDatasourceChange={this.onDatasourceChange}
- />
- {this.state.metadataLoading && <Loading />}
- </StyledTableTabWrapper>
- ),
- },
+ ),
+ };
+ }, [datasource, onDatasourcePropChange]);
+
+ const tabItems = useMemo(
+ () => [
+ {
+ key: TABS_KEYS.SOURCE,
+ label: t('Source'),
+ children: renderSourceFieldset(),
+ },
+ {
+ key: TABS_KEYS.METRICS,
+ label: (
+ <CollectionTabTitle collection={sortedMetrics} title={t('Metrics')}
/>
+ ),
+ children: renderMetricCollection(),
+ },
+ {
+ key: TABS_KEYS.COLUMNS,
+ label: (
+ <CollectionTabTitle
+ collection={databaseColumns}
+ title={t('Columns')}
+ />
+ ),
+ children: (
+ <StyledTableTabWrapper>
+ {renderDefaultColumnSettings()}
+ <DefaultColumnSettingsTitle>
+ {t('Column Settings')}
+ </DefaultColumnSettingsTitle>
+ <ColumnButtonWrapper>
+ <StyledButtonWrapper>
+ <Button
+ buttonSize="small"
+ buttonStyle="tertiary"
+ onClick={syncMetadata}
+ className="sync-from-source"
+ disabled={isEditMode}
Review Comment:
`disabled={isEditMode}` matches master verbatim (`this.state.isEditMode`) -
the port preserved it, so this gating predates the conversion rather than being
introduced here.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -71,7 +74,120 @@ import { CHART_WIDTH, CHART_HEIGHT } from
'src/dashboard/constants';
// Session storage key for recent dashboard
const SK_DASHBOARD_ID = 'save_chart_recent_dashboard';
-interface SaveModalProps extends RouteComponentProps {
+/**
+ * Creates URLSearchParams with save action and slice ID, removing
form_data_key.
+ * Exported for testing purposes.
+ */
+export const createRedirectParams = (
+ windowLocationSearch: string,
+ chart: { id: number },
+ action: string,
+): URLSearchParams => {
+ const searchParams = new URLSearchParams(windowLocationSearch);
+ searchParams.set('save_action', action);
+ searchParams.delete('form_data_key');
+ searchParams.set('slice_id', chart.id.toString());
+ return searchParams;
+};
+
+/**
+ * Adds a chart to a dashboard tab by updating the position_json.
+ * Exported for testing purposes.
+ */
+export const addChartToDashboard = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ sliceNameParam: string | undefined,
+): Promise<void> => {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboardData = dashboardResponse.json.result;
+
+ let positionJson = dashboardData.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const chartKey = `CHART-${chartId}`;
+
+ // Find a row in the tab with available space
+ const tabChildren = positionJson[tabId]?.children || [];
+ let targetRowKey: string | null = null;
+
+ for (const childKey of tabChildren) {
+ const child = positionJson[childKey];
+ if (child?.type === 'ROW') {
+ const rowChildren = child.children || [];
+ const totalWidth = rowChildren.reduce((sum: number, key: string) => {
+ const component = positionJson[key];
+ return sum + (component?.meta?.width || 0);
+ }, 0);
+
+ if (totalWidth + CHART_WIDTH <= GRID_COLUMN_COUNT) {
+ targetRowKey = childKey;
+ break;
+ }
+ }
+ }
+
+ const updatedPositionJson = { ...positionJson };
+
+ // Create a new row if no existing row has space
+ if (!targetRowKey) {
+ targetRowKey = `ROW-${nanoid()}`;
+ updatedPositionJson[targetRowKey] = {
+ type: 'ROW',
+ id: targetRowKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', tabId],
+ meta: {
+ background: 'BACKGROUND_TRANSPARENT',
+ },
+ };
+
+ if (positionJson[tabId]) {
+ updatedPositionJson[tabId] = {
+ ...positionJson[tabId],
+ children: [...(positionJson[tabId].children || []), targetRowKey],
+ };
+ } else {
+ throw new Error(`Tab ${tabId} not found in positionJson`);
+ }
+ }
+
+ updatedPositionJson[chartKey] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', tabId, targetRowKey],
Review Comment:
This `parents` chain is copied verbatim from master - the port did not touch
the ancestry logic, so it is pre-existing rather than introduced here.
##########
superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx:
##########
@@ -237,413 +217,388 @@ const preventRouterLinkWhileMetaClicked = (evt:
React.MouseEvent) => {
}
};
-class DatasourceControl extends PureComponent<
- DatasourceControlProps,
- DatasourceControlState
-> {
- static defaultProps = defaultProps;
-
- constructor(props: DatasourceControlProps) {
- super(props);
- this.state = {
- showEditDatasourceModal: false,
- showChangeDatasourceModal: false,
- showSaveDatasetModal: false,
- };
- }
-
- onDatasourceSave = (datasource: Datasource) => {
- // Cast to ExtendedDatasource for the component's internal use
- this.props.actions.changeDatasource(datasource as ExtendedDatasource);
- // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
- const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
- datasource as Parameters<typeof getTemporalColumns>[0],
- );
- const { columns } = datasource;
- // the current granularity_sqla might not be a temporal column anymore
- const timeCol = this.props.form_data?.granularity_sqla;
- const isGranularitySqlaTemporal = columns.find(
- ({ column_name }) => column_name === timeCol,
- )?.is_dttm;
- // the current main_dttm_col might not be a temporal column anymore
- const isDefaultTemporal = columns.find(
- ({ column_name }) => column_name === defaultTemporalColumn,
- )?.is_dttm;
-
- // if the current granularity_sqla is empty or it is not a temporal column
anymore
- // let's update the control value
- if (datasource.type === 'table' && !isGranularitySqlaTemporal) {
- const temporalColumn = isDefaultTemporal
- ? defaultTemporalColumn
- : temporalColumns?.[0];
- this.props.actions.setControlValue(
- 'granularity_sqla',
- temporalColumn || null,
+export default function DatasourceControl({
+ actions,
+ onChange = () => {},
+ value = null,
+ datasource,
+ form_data,
+ isEditable = true,
+ onDatasourceSave = null,
+ user,
+}: DatasourceControlProps) {
+ const theme = useTheme();
+
+ const [showEditDatasourceModal, setShowEditDatasourceModal] =
useState(false);
+ const [showChangeDatasourceModal, setShowChangeDatasourceModal] =
+ useState(false);
+ const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
+
+ const handleDatasourceSave = useCallback(
+ (savedDatasource: Datasource) => {
+ // Cast to ExtendedDatasource for the component's internal use
+ actions.changeDatasource(savedDatasource as ExtendedDatasource);
+ // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
+ const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
+ savedDatasource as Parameters<typeof getTemporalColumns>[0],
);
- }
+ const { columns } = savedDatasource;
+ // the granularity_sqla might not be a temporal column anymore
+ const timeCol = form_data?.granularity_sqla;
+ const isGranularitySqlaTemporal = columns.find(
+ ({ column_name }) => column_name === timeCol,
+ )?.is_dttm;
+ // the main_dttm_col might not be a temporal column anymore
+ const isDefaultTemporal = columns.find(
+ ({ column_name }) => column_name === defaultTemporalColumn,
+ )?.is_dttm;
+
+ // if granularity_sqla is empty or it is not a temporal column anymore
+ // let's update the control value
+ if (savedDatasource.type === 'table' && !isGranularitySqlaTemporal) {
+ const temporalColumn = isDefaultTemporal
+ ? defaultTemporalColumn
+ : temporalColumns?.[0];
+ actions.setControlValue('granularity_sqla', temporalColumn || null);
+ }
- if (this.props.onDatasourceSave) {
- this.props.onDatasourceSave(datasource);
- }
- };
+ if (onDatasourceSave) {
+ onDatasourceSave(savedDatasource);
+ }
+ },
+ [actions, form_data?.granularity_sqla, onDatasourceSave],
+ );
- toggleShowDatasource = () => {
- this.setState(({ showDatasource }) => ({
- showDatasource: !showDatasource,
- }));
- };
+ const toggleChangeDatasourceModal = useCallback(() => {
+ setShowChangeDatasourceModal(prev => !prev);
+ }, []);
+
+ const toggleEditDatasourceModal = useCallback(() => {
+ setShowEditDatasourceModal(prev => !prev);
+ }, []);
+
+ const toggleSaveDatasetModal = useCallback(() => {
+ setShowSaveDatasetModal(prev => !prev);
+ }, []);
+
+ const handleMenuItemClick = useCallback(
+ ({ key }: { key: string }) => {
+ switch (key) {
+ case CHANGE_DATASET:
+ toggleChangeDatasourceModal();
+ break;
+
+ case EDIT_DATASET:
+ toggleEditDatasourceModal();
+ break;
+
+ case VIEW_IN_SQL_LAB:
+ {
+ const payload = {
+ datasourceKey: `${datasource.id}__${datasource.type}`,
+ sql: datasource.sql,
+ };
+ SupersetClient.postForm('/sqllab/', {
+ form_data: safeStringify(payload),
+ });
+ }
+ break;
+
+ case SAVE_AS_DATASET:
+ toggleSaveDatasetModal();
+ break;
+
+ default:
+ break;
+ }
+ },
+ [
+ datasource,
+ toggleChangeDatasourceModal,
+ toggleEditDatasourceModal,
+ toggleSaveDatasetModal,
+ ],
+ );
- toggleChangeDatasourceModal = () => {
- this.setState(({ showChangeDatasourceModal }) => ({
- showChangeDatasourceModal: !showChangeDatasourceModal,
- }));
- };
+ let extra;
+ if (datasource?.extra) {
+ if (typeof datasource.extra === 'string') {
+ try {
+ extra = JSON.parse(datasource.extra);
+ } catch {} // eslint-disable-line no-empty
+ } else {
+ extra = datasource.extra; // eslint-disable-line prefer-destructuring
+ }
+ }
+ const isMissingDatasource = !datasource?.id || Boolean(extra?.error);
+ let isMissingParams = false;
+ if (isMissingDatasource) {
+ const datasourceId = getUrlParam(URL_PARAMS.datasourceId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId);
+
+ if (!datasourceId && !sliceId) {
+ isMissingParams = true;
+ }
+ }
- toggleEditDatasourceModal = () => {
- this.setState(({ showEditDatasourceModal }) => ({
- showEditDatasourceModal: !showEditDatasourceModal,
- }));
- };
+ const allowEdit =
+ datasource.owners?.map(o => o.id || o.value).includes(user.userId) ||
+ isUserAdmin(user);
- toggleSaveDatasetModal = () => {
- this.setState(({ showSaveDatasetModal }) => ({
- showSaveDatasetModal: !showSaveDatasetModal,
- }));
- };
+ const canAccessSqlLab = userHasPermission(user, 'SQL Lab', 'menu_access');
- handleMenuItemClick = ({ key }: { key: string }) => {
- switch (key) {
- case CHANGE_DATASET:
- this.toggleChangeDatasourceModal();
- break;
-
- case EDIT_DATASET:
- this.toggleEditDatasourceModal();
- break;
-
- case VIEW_IN_SQL_LAB:
- {
- const { datasource } = this.props;
- const payload = {
- datasourceKey: `${datasource.id}__${datasource.type}`,
- sql: datasource.sql,
- };
- SupersetClient.postForm('/sqllab/', {
- form_data: safeStringify(payload),
- });
- }
- break;
-
- case SAVE_AS_DATASET:
- this.toggleSaveDatasetModal();
- break;
-
- default:
- break;
- }
+ const editText = t('Edit %s', datasetLabelLower());
+ const requestedQuery = {
+ datasourceKey: `${datasource.id}__${datasource.type}`,
+ sql: datasource.sql,
};
+ const defaultDatasourceMenuItems = [];
+ if (isEditable && !isMissingDatasource) {
+ defaultDatasourceMenuItems.push({
+ key: EDIT_DATASET,
+ label: !allowEdit ? (
+ <Tooltip
+ title={t(
+ 'You must be a %s owner in order to edit. Please reach out to a %s
owner to request modifications or edit access.',
+ datasetLabelLower(),
+ datasetLabelLower(),
+ )}
+ >
+ {editText}
+ </Tooltip>
+ ) : (
+ editText
+ ),
+ disabled: !allowEdit,
+ 'data-test': 'edit-dataset',
+ });
+ }
- render() {
- const {
- showChangeDatasourceModal,
- showEditDatasourceModal,
- showSaveDatasetModal,
- } = this.state;
- const { datasource, onChange, theme } = this.props;
- let extra;
- if (datasource?.extra) {
- if (typeof datasource.extra === 'string') {
- try {
- extra = JSON.parse(datasource.extra);
- } catch {} // eslint-disable-line no-empty
- } else {
- extra = datasource.extra; // eslint-disable-line prefer-destructuring
- }
- }
- const isMissingDatasource = !datasource?.id || Boolean(extra?.error);
- let isMissingParams = false;
- if (isMissingDatasource) {
- const datasourceId = getUrlParam(URL_PARAMS.datasourceId);
- const sliceId = getUrlParam(URL_PARAMS.sliceId);
-
- if (!datasourceId && !sliceId) {
- isMissingParams = true;
- }
- }
-
- const { user } = this.props;
- const allowEdit =
- datasource.owners?.map(o => o.id || o.value).includes(user.userId) ||
- isUserAdmin(user);
-
- const canAccessSqlLab = userHasPermission(user, 'SQL Lab', 'menu_access');
-
- const editText = t('Edit %s', datasetLabelLower());
- const requestedQuery = {
- datasourceKey: `${datasource.id}__${datasource.type}`,
- sql: datasource.sql,
- };
- const defaultDatasourceMenuItems = [];
- if (this.props.isEditable && !isMissingDatasource) {
- defaultDatasourceMenuItems.push({
- key: EDIT_DATASET,
- label: !allowEdit ? (
- <Tooltip
- title={t(
- 'You must be a %s owner in order to edit. Please reach out to a
%s owner to request modifications or edit access.',
- datasetLabelLower(),
- datasetLabelLower(),
- )}
- >
- {editText}
- </Tooltip>
- ) : (
- editText
- ),
- disabled: !allowEdit,
- 'data-test': 'edit-dataset',
- });
- }
+ defaultDatasourceMenuItems.push({
+ key: CHANGE_DATASET,
+ label: t('Swap %s', datasetLabelLower()),
+ });
+ if (!isMissingDatasource && canAccessSqlLab) {
defaultDatasourceMenuItems.push({
- key: CHANGE_DATASET,
- label: t('Swap %s', datasetLabelLower()),
+ key: VIEW_IN_SQL_LAB,
+ label: (
+ <Link
+ to={{
+ pathname: '/sqllab',
+ state: { requestedQuery },
+ }}
+ onClick={preventRouterLinkWhileMetaClicked}
+ >
+ {t('View in SQL Lab')}
+ </Link>
+ ),
});
+ }
- if (!isMissingDatasource && canAccessSqlLab) {
- defaultDatasourceMenuItems.push({
- key: VIEW_IN_SQL_LAB,
- label: (
- <Link
- to={{
- pathname: '/sqllab',
- state: { requestedQuery },
- }}
- onClick={preventRouterLinkWhileMetaClicked}
- >
- {t('View in SQL Lab')}
- </Link>
- ),
- });
- }
-
- const defaultDatasourceMenu = (
- <Menu
- onClick={this.handleMenuItemClick}
- items={defaultDatasourceMenuItems}
- />
- );
-
- const queryDatasourceMenuItems = [
- {
- key: QUERY_PREVIEW,
- label: (
- <ModalTrigger
- triggerNode={
- <div data-test="view-query-menu-item">{t('Query preview')}</div>
- }
- modalTitle={t('Query preview')}
- modalBody={
- <ViewQuery
- sql={datasource?.sql || datasource?.select_star || ''}
- datasource={`${datasource.id}__${datasource.type}`}
- />
- }
- modalFooter={
- <ViewQueryModalFooter
- changeDatasource={this.toggleSaveDatasetModal}
- datasource={{
- id: String(datasource.id),
- sql: datasource.sql || '',
- type: datasource.type,
- }}
- />
- }
- draggable={false}
- resizable={false}
- responsive
- />
- ),
- },
- ];
-
- if (canAccessSqlLab) {
- queryDatasourceMenuItems.push({
- key: VIEW_IN_SQL_LAB,
- label: (
- <Link
- to={{
- pathname: '/sqllab',
- state: { requestedQuery },
- }}
- onClick={preventRouterLinkWhileMetaClicked}
- >
- {t('View in SQL Lab')}
- </Link>
- ),
- });
- }
+ const defaultDatasourceMenu = (
+ <Menu onClick={handleMenuItemClick} items={defaultDatasourceMenuItems} />
+ );
+ const queryDatasourceMenuItems = [
+ {
+ key: QUERY_PREVIEW,
+ label: (
+ <ModalTrigger
+ triggerNode={
+ <div data-test="view-query-menu-item">{t('Query preview')}</div>
+ }
+ modalTitle={t('Query preview')}
+ modalBody={
+ <ViewQuery
+ sql={datasource?.sql || datasource?.select_star || ''}
+ datasource={`${datasource.id}__${datasource.type}`}
+ />
+ }
+ modalFooter={
+ <ViewQueryModalFooter
+ changeDatasource={toggleSaveDatasetModal}
+ datasource={{
+ id: String(datasource.id),
+ sql: datasource.sql || '',
+ type: datasource.type,
+ }}
+ />
+ }
+ draggable={false}
+ resizable={false}
+ responsive
+ />
+ ),
+ },
+ ];
+
+ if (canAccessSqlLab) {
queryDatasourceMenuItems.push({
- key: SAVE_AS_DATASET,
- label: <span>{t('Save as %s', datasetLabelLower())}</span>,
+ key: VIEW_IN_SQL_LAB,
+ label: (
+ <Link
+ to={{
+ pathname: '/sqllab',
+ state: { requestedQuery },
+ }}
+ onClick={preventRouterLinkWhileMetaClicked}
+ >
+ {t('View in SQL Lab')}
+ </Link>
+ ),
});
+ }
- const queryDatasourceMenu = (
- <Menu
- onClick={this.handleMenuItemClick}
- items={queryDatasourceMenuItems}
- />
- );
-
- const { health_check_message: healthCheckMessage } = datasource;
-
- const titleText =
- isMissingDatasource && !datasource.name
- ? t('Missing %s', datasetLabelLower())
- : getDatasourceTitle(datasource);
-
- const tooltip = titleText;
-
- return (
- <Styles data-test="datasource-control" className="DatasourceControl">
- <div className="data-container">
- {datasourceIconLookup[getDatasetType(datasource)]}
- {renderDatasourceTitle(titleText, tooltip)}
- {healthCheckMessage && (
- <Tooltip title={healthCheckMessage}>
- <Icons.WarningOutlined
- css={css`
- margin-left: ${theme.sizeUnit * 2}px;
- `}
- iconColor={theme.colorWarning}
- />
- </Tooltip>
- )}
- {extra?.warning_markdown && (
- <WarningIconWithTooltip warningMarkdown={extra.warning_markdown} />
- )}
- <Dropdown
- popupRender={() =>
- datasource.type === DatasourceType.Query
- ? queryDatasourceMenu
- : defaultDatasourceMenu
- }
- trigger={['click']}
- data-test="datasource-menu"
- >
- <Icons.MoreOutlined
- iconSize="xl"
- iconColor={theme.colorPrimary}
- className="datasource-modal-trigger"
- data-test="datasource-menu-trigger"
- />
- </Dropdown>
- </div>
- {/* missing dataset */}
- {isMissingDatasource && isMissingParams && (
- <div className="error-alert">
- <ErrorAlert
- type="warning"
- message={t('Missing URL parameters')}
- description={t(
- 'The URL is missing the dataset_id or slice_id parameters.',
- )}
+ queryDatasourceMenuItems.push({
+ key: SAVE_AS_DATASET,
+ label: <span>{t('Save as %s', datasetLabelLower())}</span>,
+ });
+
+ const queryDatasourceMenu = (
+ <Menu onClick={handleMenuItemClick} items={queryDatasourceMenuItems} />
+ );
+
+ const { health_check_message: healthCheckMessage } = datasource;
+
+ const titleText =
+ isMissingDatasource && !datasource.name
+ ? t('Missing %s', datasetLabelLower())
+ : getDatasourceTitle(datasource);
+
+ const tooltip = titleText;
+
+ return (
+ <Styles data-test="datasource-control" className="DatasourceControl">
+ <div className="data-container">
+ {datasourceIconLookup[getDatasetType(datasource)]}
+ {renderDatasourceTitle(titleText, tooltip)}
+ {healthCheckMessage && (
+ <Tooltip title={healthCheckMessage}>
+ <Icons.WarningOutlined
+ css={css`
+ margin-left: ${theme.sizeUnit * 2}px;
+ `}
+ iconColor={theme.colorWarning}
/>
- </div>
+ </Tooltip>
)}
- {isMissingDatasource && !isMissingParams && (
- <div className="error-alert">
- {extra?.error ? (
- <ErrorMessageWithStackTrace
- title={extra.error.statusText || extra.error.message}
- subtitle={
- extra.error.statusText ? extra.error.message : undefined
- }
- error={extra.error}
- source="explore"
- />
- ) : (
- <ErrorAlert
- type="warning"
- message={t('Missing %s', datasetLabelLower())}
- descriptionPre={false}
- descriptionDetailsCollapsed={false}
- descriptionDetails={
- <>
- <p>
- {t(
- 'The %s linked to this chart may have been deleted.',
- datasetLabelLower(),
- )}
- </p>
- <p>
- <Button
- buttonStyle="primary"
- onClick={() =>
- this.handleMenuItemClick({ key: CHANGE_DATASET })
- }
- >
- {t('Swap %s', datasetLabelLower())}
- </Button>
- </p>
- </>
- }
- />
- )}
- </div>
+ {extra?.warning_markdown && (
+ <WarningIconWithTooltip warningMarkdown={extra.warning_markdown} />
)}
- {showEditDatasourceModal &&
- (String(datasource.type) === 'semantic_view' ? (
- <SemanticViewEditModal
- show={showEditDatasourceModal}
- onHide={this.toggleEditDatasourceModal}
- onSave={() => this.onDatasourceSave(datasource)}
- semanticView={{
- id: datasource.id,
- table_name: datasource.name,
- description: datasource.description,
- cache_timeout: datasource.cache_timeout,
- }}
+ <Dropdown
+ popupRender={() =>
+ datasource.type === DatasourceType.Query
+ ? queryDatasourceMenu
+ : defaultDatasourceMenu
+ }
+ trigger={['click']}
+ data-test="datasource-menu"
+ >
+ <Icons.MoreOutlined
+ iconSize="xl"
+ iconColor={theme.colorPrimary}
+ className="datasource-modal-trigger"
+ data-test="datasource-menu-trigger"
+ />
+ </Dropdown>
+ </div>
+ {/* missing dataset */}
+ {isMissingDatasource && isMissingParams && (
+ <div className="error-alert">
+ <ErrorAlert
+ type="warning"
+ message={t('Missing URL parameters')}
+ description={t(
+ 'The URL is missing the dataset_id or slice_id parameters.',
+ )}
+ />
+ </div>
+ )}
+ {isMissingDatasource && !isMissingParams && (
+ <div className="error-alert">
+ {extra?.error ? (
+ <ErrorMessageWithStackTrace
+ title={extra.error.statusText || extra.error.message}
+ subtitle={
+ extra.error.statusText ? extra.error.message : undefined
+ }
+ error={extra.error}
+ source="explore"
/>
) : (
- <DatasourceModal
- datasource={datasource}
- show={showEditDatasourceModal}
- onDatasourceSave={this.onDatasourceSave}
- onHide={this.toggleEditDatasourceModal}
+ <ErrorAlert
+ type="warning"
+ message={t('Missing %s', datasetLabelLower())}
+ descriptionPre={false}
+ descriptionDetailsCollapsed={false}
+ descriptionDetails={
+ <>
+ <p>
+ {t(
+ 'The %s linked to this chart may have been deleted.',
+ datasetLabelLower(),
+ )}
+ </p>
+ <p>
+ <Button
+ buttonStyle="primary"
+ onClick={() =>
+ handleMenuItemClick({ key: CHANGE_DATASET })
+ }
+ >
+ {t('Swap %s', datasetLabelLower())}
+ </Button>
+ </p>
+ </>
+ }
/>
- ))}
- {showChangeDatasourceModal && (
- <ChangeDatasourceModal
- onDatasourceSave={this.onDatasourceSave}
- onHide={this.toggleChangeDatasourceModal}
- show={showChangeDatasourceModal}
- onChange={onChange}
+ )}
+ </div>
+ )}
+ {showEditDatasourceModal &&
+ (String(datasource.type) === 'semantic_view' ? (
+ <SemanticViewEditModal
+ show={showEditDatasourceModal}
+ onHide={toggleEditDatasourceModal}
+ onSave={() => handleDatasourceSave(datasource)}
+ semanticView={{
+ id: datasource.id,
+ table_name: datasource.name,
+ description: datasource.description,
+ cache_timeout: datasource.cache_timeout,
+ }}
/>
Review Comment:
`onSave={() => handleDatasourceSave(datasource)}` matches master
line-for-line - the port preserved it, so this behavior predates the conversion
rather than being introduced here.
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -59,10 +72,16 @@ afterEach(() => {
}
} finally {
fetchMock.clearHistory().removeRoutes();
- jest.restoreAllMocks();
+ jest.clearAllMocks(); // Clears mock history but keeps spy in place
}
});
Review Comment:
The module-scope `get` spy is intentionally restored only in `afterAll`, so
a blanket `restoreAllMocks` here would clobber it, and `afterEach` already
reassigns `window.location`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
