rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3481832866
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.tsx:
##########
@@ -235,174 +214,205 @@ class AdhocFilterControl extends Component<
});
}
}
- }
+ }, [datasource]);
- componentDidUpdate(prevProps: AdhocFilterControlProps): void {
- if (this.props.columns !== prevProps.columns) {
- this.setState({ options: optionsForSelect(this.props) });
- }
- if (this.props.value !== prevProps.value) {
- this.setState({
- values: (this.props.value || []).map(filter =>
+ useEffect(() => {
+ if (value !== undefined) {
+ setValues(
+ (value || []).map(filter =>
isDictionaryForAdhocFilter(filter) ? new AdhocFilter(filter) :
filter,
),
- });
+ );
}
- }
+ }, [value]);
- removeFilter(index: number): void {
- const valuesCopy = [...this.state.values];
- valuesCopy.splice(index, 1);
- this.setState(prevState => ({
- ...prevState,
- values: valuesCopy,
- }));
- this.props.onChange?.(valuesCopy);
- }
+ const getMetricExpression = useCallback(
+ (savedMetricName: string): string => {
+ const metric = savedMetrics?.find(
+ savedMetric => savedMetric.metric_name === savedMetricName,
+ );
+ return metric?.expression ?? '';
+ },
+ [savedMetrics],
+ );
- onRemoveFilter(index: number): void {
- const { canDelete } = this.props;
- const { values } = this.state;
- const result = canDelete?.(values[index], values);
- if (typeof result === 'string') {
- warning({ title: t('Warning'), content: result });
- return;
- }
- this.removeFilter(index);
- }
+ const mapOption = useCallback(
+ (option: FilterOption | AdhocFilter): AdhocFilter | null => {
+ // already a AdhocFilter, skip
+ if (option instanceof AdhocFilter) {
+ return option;
+ }
+ // via datasource saved metric
+ if (option.saved_metric_name) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Sql,
+ subject: getMetricExpression(option.saved_metric_name),
+ operator:
+ OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
+ comparator: 0,
+ clause: Clauses.Having,
+ });
+ }
+ // has a custom label, meaning it's custom column
+ if (option.label) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Sql,
+ subject: new AdhocMetric(option).translateToSql(),
+ operator:
+ OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
+ comparator: 0,
+ clause: Clauses.Having,
+ });
+ }
+ // add a new filter item
+ if (option.column_name) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Simple,
+ subject: option.column_name,
+ operator: OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.Equals].operation,
+ comparator: '',
+ clause: Clauses.Where,
+ isNew: true,
+ });
+ }
+ return null;
+ },
+ [getMetricExpression],
+ );
- onNewFilter(newFilter: FilterOption | AdhocFilter): void {
- const mappedOption = this.mapOption(newFilter);
- if (mappedOption) {
- this.setState(
- prevState => ({
- ...prevState,
- values: [...prevState.values, mappedOption],
- }),
- () => {
- this.props.onChange?.(this.state.values);
- },
- );
- }
- }
+ const removeFilter = useCallback(
+ (index: number) => {
+ const valuesCopy = [...values];
+ valuesCopy.splice(index, 1);
+ setValues(valuesCopy);
+ onChange?.(valuesCopy);
+ },
+ [values, onChange],
+ );
- onFilterEdit(changedFilter: AdhocFilter): void {
- this.props.onChange?.(
- this.state.values.map(value => {
- if (value.filterOptionName === changedFilter.filterOptionName) {
- return changedFilter;
- }
- return value;
- }),
- );
- }
+ const onRemoveFilter = useCallback(
+ (index: number) => {
+ const result = canDelete?.(values[index], values);
+ if (typeof result === 'string') {
+ warning({ title: t('Warning'), content: result });
+ return;
+ }
+ removeFilter(index);
+ },
+ [canDelete, values, removeFilter],
+ );
- onChange(opts: FilterOption[] | null): void {
- const options = (opts || [])
- .map(option => this.mapOption(option))
- .filter((option): option is AdhocFilter => option !== null);
- this.props.onChange?.(options);
- }
+ const onFilterEdit = useCallback(
+ (changedFilter: AdhocFilter) => {
+ onChange?.(
+ values.map(val => {
+ if (val.filterOptionName === changedFilter.filterOptionName) {
+ return changedFilter;
+ }
+ return val;
+ }),
+ );
+ },
+ [values, onChange],
+ );
- getMetricExpression(savedMetricName: string): string {
- const metric = this.props.savedMetrics?.find(
- savedMetric => savedMetric.metric_name === savedMetricName,
- );
- return metric?.expression ?? '';
- }
+ const moveLabel = useCallback((dragIndex: number, hoverIndex: number) => {
+ setValues(prevValues => {
+ const newValues = [...prevValues];
+ [newValues[hoverIndex], newValues[dragIndex]] = [
+ newValues[dragIndex],
+ newValues[hoverIndex],
+ ];
+ return newValues;
+ });
+ }, []);
- moveLabel(dragIndex: number, hoverIndex: number): void {
- const { values } = this.state;
+ const onDropLabel = useCallback(() => {
+ onChange?.(values);
+ }, [onChange, values]);
- const newValues = [...values];
- [newValues[hoverIndex], newValues[dragIndex]] = [
- newValues[dragIndex],
- newValues[hoverIndex],
- ];
- this.setState({ values: newValues });
- }
+ const onNewFilter = useCallback(
+ (newFilter: FilterOption | AdhocFilter) => {
+ const mappedOption = mapOption(newFilter);
+ if (mappedOption) {
+ const newValues = [...values, mappedOption];
+ setValues(newValues);
+ onChange?.(newValues);
+ }
+ },
+ [mapOption, values, onChange],
+ );
- mapOption(option: FilterOption | AdhocFilter): AdhocFilter | null {
- // already a AdhocFilter, skip
- if (option instanceof AdhocFilter) {
- return option;
- }
- // via datasource saved metric
- if (option.saved_metric_name) {
- return new AdhocFilter({
- expressionType: ExpressionTypes.Sql,
- subject: this.getMetricExpression(option.saved_metric_name),
- operator:
- OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
- comparator: 0,
- clause: Clauses.Having,
- });
- }
- // has a custom label, meaning it's custom column
- if (option.label) {
- return new AdhocFilter({
- expressionType: ExpressionTypes.Sql,
- subject: new AdhocMetric(option).translateToSql(),
- operator:
- OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
- comparator: 0,
- clause: Clauses.Having,
- });
- }
- // add a new filter item
- if (option.column_name) {
- return new AdhocFilter({
- expressionType: ExpressionTypes.Simple,
- subject: option.column_name,
- operator: OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.Equals].operation,
- comparator: '',
- clause: Clauses.Where,
- isNew: true,
- });
- }
- return null;
- }
+ const valueRenderer = useCallback(
+ (adhocFilter: AdhocFilter, index: number) => (
+ <AdhocFilterOption
+ key={index}
Review Comment:
`key={index}` here matches master - the class render used the same index
key, so the DnD-identity concern predates this conversion. Worth its own PR if
we want to fix it properly.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -883,602 +869,701 @@ const mapStateToProps = (state: RootState) => ({
const connector = connect(mapStateToProps, mapDispatchToProps);
type PropsFromRedux = ConnectedProps<typeof connector>;
-type DatasourceEditorProps = DatasourceEditorOwnProps &
- PropsFromRedux & {
- theme?: SupersetTheme;
- };
-
-class DatasourceEditor extends PureComponent<
- DatasourceEditorProps,
- DatasourceEditorState
-> {
- private isComponentMounted: boolean;
+type DatasourceEditorProps = DatasourceEditorOwnProps & PropsFromRedux;
+
+function DatasourceEditor({
+ datasource: propsDatasource,
+ onChange = () => {},
+ addSuccessToast,
+ addDangerToast,
+ setIsEditing = () => {},
+ database,
+ runQuery,
+ resetQuery,
+ formatQuery: formatQueryAction,
+}: DatasourceEditorProps) {
+ const theme = useTheme();
+ const isComponentMounted = useRef(false);
+ const isInitialMount = useRef(true);
+ const prevPropsDatasourceRef = useRef(propsDatasource);
+ const isSyncingColumnsFromProps = useRef(false);
+ const abortControllers = useRef<AbortControllers>({
+ formatQuery: null,
+ formatSql: null,
+ syncMetadata: null,
+ fetchUsageData: null,
+ });
+
+ // Initialize datasource state with transformed owners and metrics
+ const [datasource, setDatasource] = useState<DatasourceObject>(() => ({
+ ...propsDatasource,
+ owners: propsDatasource.owners.map(owner => {
+ const ownerName = owner.label || `${owner.first_name}
${owner.last_name}`;
+ return {
+ value: owner.value || owner.id,
+ label: OwnerSelectLabel({
+ name: typeof ownerName === 'string' ? ownerName : '',
+ email: owner.email,
+ }),
+ [OWNER_TEXT_LABEL_PROP]: typeof ownerName === 'string' ? ownerName :
'',
+ [OWNER_EMAIL_PROP]: owner.email ?? '',
+ };
+ }),
+ metrics: propsDatasource.metrics?.map(metric => {
+ const {
+ certified_by: certifiedByMetric,
+ certification_details: certificationDetails,
+ } = metric;
+ const {
+ certification: {
+ details = undefined,
+ certified_by: certifiedBy = undefined,
+ } = {},
+ warning_markdown: warningMarkdown,
+ } = JSON.parse(metric.extra || '{}') || {};
+ return {
+ ...metric,
+ certification_details: certificationDetails || details,
+ warning_markdown: warningMarkdown || '',
+ certified_by: certifiedBy || certifiedByMetric,
+ };
+ }),
+ }));
- private abortControllers: AbortControllers;
+ const [errors, setErrors] = useState<string[]>([]);
+ const [isSqla] = useState(
+ propsDatasource.datasource_type === 'table' ||
+ propsDatasource.type === 'table',
+ );
+ const [isEditMode, setIsEditMode] = useState(false);
+ const [databaseColumns, setDatabaseColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !col.expression),
+ );
+ const [calculatedColumns, setCalculatedColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !!col.expression),
+ );
+ const [folders, setFolders] = useState<DatasourceFolder[]>(
+ propsDatasource.folders || [],
+ );
+ const [folderCount, setFolderCount] = useState(() => {
+ const savedFolders = propsDatasource.folders || [];
+ const savedCount = countAllFolders(savedFolders);
+ const hasDefaultsSaved = savedFolders.some(f => isDefaultFolder(f.uuid));
+ return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
+ });
+ const [metadataLoading, setMetadataLoading] = useState(false);
+ const [activeTabKey, setActiveTabKey] = useState(TABS_KEYS.SOURCE);
+ const [datasourceType, setDatasourceType] = useState(
+ propsDatasource.sql
+ ? DATASOURCE_TYPES.virtual.key
+ : DATASOURCE_TYPES.physical.key,
+ );
+ const [usageCharts, setUsageCharts] = useState<ChartUsageData[]>([]);
+ const [usageChartsCount, setUsageChartsCount] = useState(0);
+ const [metricSearchTerm, setMetricSearchTerm] = useState('');
+ const [columnSearchTerm, setColumnSearchTerm] = useState('');
+ const [calculatedColumnSearchTerm, setCalculatedColumnSearchTerm] =
+ useState('');
+
+ const findDuplicates = useCallback(
+ <T,>(arr: T[], accessor: (obj: T) => string): string[] => {
+ const seen: Record<string, null> = {};
+ const dups: string[] = [];
+ arr.forEach((obj: T) => {
+ const item = accessor(obj);
+ if (item in seen) {
+ dups.push(item);
+ } else {
+ seen[item] = null;
+ }
+ });
+ return dups;
+ },
+ [],
+ );
- static defaultProps = {
- onChange: () => {},
- setIsEditing: () => {},
- };
+ const validate = useCallback(
+ (callback: (validationErrors: string[]) => void) => {
+ let validationErrors: string[] = [];
+ let dups: string[];
- constructor(props: DatasourceEditorProps) {
- super(props);
- this.state = {
- datasource: {
- ...props.datasource,
- owners: props.datasource.owners.map(owner => {
- const ownerName =
- owner.label || `${owner.first_name} ${owner.last_name}`;
- return {
- value: owner.value || owner.id,
- label: OwnerSelectLabel({
- name: typeof ownerName === 'string' ? ownerName : '',
- email: owner.email,
- }),
- [OWNER_TEXT_LABEL_PROP]:
- typeof ownerName === 'string' ? ownerName : '',
- [OWNER_EMAIL_PROP]: owner.email ?? '',
- };
- }),
- metrics: props.datasource.metrics?.map(metric => {
- const {
- certified_by: certifiedByMetric,
- certification_details: certificationDetails,
- } = metric;
- const {
- certification: {
- details = undefined,
- certified_by: certifiedBy = undefined,
- } = {},
- warning_markdown: warningMarkdown,
- } = JSON.parse(metric.extra || '{}') || {};
- return {
- ...metric,
- certification_details: certificationDetails || details,
- warning_markdown: warningMarkdown || '',
- certified_by: certifiedBy || certifiedByMetric,
- };
- }),
- },
- errors: [],
- isSqla:
- props.datasource.datasource_type === 'table' ||
- props.datasource.type === 'table',
- isEditMode: false,
- databaseColumns: props.datasource.columns.filter(col => !col.expression),
- calculatedColumns: props.datasource.columns.filter(
- col => !!col.expression,
- ),
- folders: props.datasource.folders || [],
- folderCount: (() => {
- const savedFolders = props.datasource.folders || [];
- const savedCount = countAllFolders(savedFolders);
- const hasDefaultsSaved = savedFolders.some(f =>
- isDefaultFolder(f.uuid),
- );
- return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
- })(),
- metadataLoading: false,
- activeTabKey: TABS_KEYS.SOURCE,
- datasourceType: props.datasource.sql
- ? DATASOURCE_TYPES.virtual.key
- : DATASOURCE_TYPES.physical.key,
- usageCharts: [],
- usageChartsCount: 0,
- metricSearchTerm: '',
- columnSearchTerm: '',
- calculatedColumnSearchTerm: '',
- };
+ // Looking for duplicate column_name
+ dups = findDuplicates(datasource.columns, obj => obj.column_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Column name [%s] is duplicated', name)),
Review Comment:
The duplicate check reads `datasource.columns` exactly as the class version
on master did. Same validation source - not introduced by the port.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -84,714 +200,652 @@ interface SaveModalProps extends RouteComponentProps {
dashboardId: '' | number | null;
isVisible: boolean;
dispatch: Dispatch;
- theme: SupersetTheme;
metadata?: ExplorePageInitialData['metadata'];
}
-type SaveModalState = {
- newSliceName?: string;
- datasetName: string;
- action: SaveActionType;
- isLoading: boolean;
- saveStatus?: string | null;
- dashboard?: { label: string; value: string | number };
- selectedTab?: { label: string; value: string | number };
- tabsData: TabTreeNode[];
-};
-
export const StyledModal = styled(Modal)`
.ant-modal-body {
overflow: visible;
}
`;
-class SaveModal extends Component<SaveModalProps, SaveModalState> {
- constructor(props: SaveModalProps) {
- super(props);
- this.state = {
- newSliceName: props.sliceName,
- datasetName: props.datasource?.name,
- action: this.canOverwriteSlice()
- ? ChartStatusType.overwrite
- : ChartStatusType.saveas,
- isLoading: false,
- dashboard: undefined,
- tabsData: [],
- selectedTab: undefined,
- };
- this.onDashboardChange = this.onDashboardChange.bind(this);
- this.onSliceNameChange = this.onSliceNameChange.bind(this);
- this.changeAction = this.changeAction.bind(this);
- this.saveOrOverwrite = this.saveOrOverwrite.bind(this);
- this.isNewDashboard = this.isNewDashboard.bind(this);
- this.onHide = this.onHide.bind(this);
- }
+const SaveModal = ({
+ addDangerToast,
+ actions,
+ form_data,
+ user,
+ alert: alertProp,
+ sliceName = '',
+ slice,
+ can_overwrite,
+ datasource,
+ dashboardId: dashboardIdProp,
+ isVisible,
+ metadata,
+}: SaveModalProps) => {
+ const dispatch = useDispatch();
+ const history = useHistory();
+ const theme = useTheme();
+
+ const canOverwriteSlice = useCallback(
+ (): boolean =>
+ (can_overwrite ||
+ isUserAdmin(user) ||
+ slice?.owners?.includes(user.userId)) &&
+ !slice?.is_managed_externally,
+ [can_overwrite, slice, user],
+ );
- isNewDashboard(): boolean {
- const { dashboard } = this.state;
- return typeof dashboard?.value === 'string';
- }
+ const [newSliceName, setNewSliceName] = useState<string | undefined>(
+ sliceName,
+ );
+ const [datasetName, setDatasetName] = useState<string>(datasource?.name);
+ const [action, setAction] = useState<SaveActionType>(
+ canOverwriteSlice() ? ChartStatusType.overwrite : ChartStatusType.saveas,
+ );
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [dashboard, setDashboard] = useState<
+ { label: string; value: string | number } | undefined
+ >(undefined);
+ const [tabsData, setTabsData] = useState<TabTreeNode[]>([]);
+ const [selectedTab, setSelectedTab] = useState<
+ { label: string; value: string | number } | undefined
+ >(undefined);
+
+ const isNewDashboard = useCallback(
+ (): boolean => typeof dashboard?.value === 'string',
+ [dashboard?.value],
+ );
- canOverwriteSlice(): boolean {
- return (
- (this.props.can_overwrite ||
- isUserAdmin(this.props.user) ||
- this.props.slice?.owners?.includes(this.props.user.userId)) &&
- !this.props.slice?.is_managed_externally
- );
- }
+ const loadDashboard = useCallback(async (id: number) => {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${id}`,
+ });
+ return response.json.result;
+ }, []);
- async componentDidMount() {
- let { dashboardId } = this.props;
- if (!dashboardId) {
- let lastDashboard = null;
- try {
- lastDashboard = sessionStorage.getItem(SK_DASHBOARD_ID);
- } catch (error) {
- // continue regardless of error
- }
- dashboardId = lastDashboard && parseInt(lastDashboard, 10);
- }
- if (dashboardId) {
+ const loadTabs = useCallback(
+ async (dashboardId: number) => {
try {
- const result = (await this.loadDashboard(dashboardId)) as Dashboard;
- if (canUserEditDashboard(result, this.props.user)) {
- this.setState({
- dashboard: { label: result.dashboard_title, value: result.id },
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}/tabs`,
+ });
+
+ const { result } = response.json;
+ if (!result || !Array.isArray(result.tab_tree)) {
+ logging.warn('Invalid tabs response format');
+ setTabsData([]);
+ return [];
Review Comment:
This tab-reset behavior mirrors the class version on master - `selectedTab`
was handled the same way there, so the stale-reference concern predates this
conversion.
--
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]
