fix: set label on adhoc column should persist (#26154)

(cherry picked from commit b2ea97a)

superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.test.tsx

@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { render, fireEvent } from '@testing-library/react';
+import '@testing-library/jest-dom/extend-expect';
+import { Provider } from 'react-redux';
+import configureMockStore from 'redux-mock-store';
+import thunk from 'redux-thunk';
+import { supersetTheme, ThemeProvider } from '@superset-ui/core';
+import ColumnSelectPopover from 'src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover';
+
+const middlewares = [thunk];
+const mockStore = configureMockStore(middlewares);
+
+describe('ColumnSelectPopover - onTabChange function', () => {
+ it('updates adhocColumn when switching to sqlExpression tab with custom label', () => {
+ const mockColumns = [{ column_name: 'year' }];
+ const mockOnClose = jest.fn();
+ const mockOnChange = jest.fn();
+ const mockGetCurrentTab = jest.fn();
+ const mockSetDatasetModal = jest.fn();
+ const mockSetLabel = jest.fn();
+
+ const store = mockStore({ explore: { datasource: { type: 'table' } } });
+
+ const { container, getByText } = render(
+ <Provider store={store}>
+ <ThemeProvider theme={supersetTheme}>
+ <ColumnSelectPopover
+ columns={mockColumns}
+ editedColumn={mockColumns[0]}
+ getCurrentTab={mockGetCurrentTab}
+ hasCustomLabel
+ isTemporal
+ label="Custom Label"
+ onChange={mockOnChange}
+ onClose={mockOnClose}
+ setDatasetModal={mockSetDatasetModal}
+ setLabel={mockSetLabel}
+ />
+ </ThemeProvider>
+ </Provider>,
+ );
+
+ const sqlExpressionTab = container.querySelector(
+ '#adhoc-metric-edit-tabs-tab-sqlExpression',
+ );
+ expect(sqlExpressionTab).not.toBeNull();
+ fireEvent.click(sqlExpressionTab!);
+ expect(mockGetCurrentTab).toHaveBeenCalledWith('sqlExpression');
+
+ const saveButton = getByText('Save');
+ fireEvent.click(saveButton);
+ expect(mockOnChange).toHaveBeenCalledWith({
+ label: 'Custom Label',
+ sqlExpression: 'year',
+ expressionType: 'SQL',
+ });
+ });
+});

superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx

@@ -68,6 +68,7 @@ interface ColumnSelectPopoverProps {
  editedColumn?: ColumnMeta | AdhocColumn;
  onChange: (column: ColumnMeta | AdhocColumn) => void;
  onClose: () => void;
+ hasCustomLabel: boolean;
  setLabel: (title: string) => void;
  getCurrentTab: (tab: string) => void;
  label: string;
@@ -93,13 +94,14 @@ const getInitialColumnValues = (
 const ColumnSelectPopover = ({
  columns,
  editedColumn,
+ getCurrentTab,
+ hasCustomLabel,
+ isTemporal,
+ label,
  onChange,
  onClose,
  setDatasetModal,
  setLabel,
- getCurrentTab,
- label,
- isTemporal,
 }: ColumnSelectPopoverProps) => {
  const datasourceType = useSelector<ExplorePageState, string | undefined>(
  state => state.explore.datasource.type,
@@ -117,6 +119,7 @@ const ColumnSelectPopover = ({
  const [selectedSimpleColumn, setSelectedSimpleColumn] = useState<
  ColumnMeta | undefined
  >(initialSimpleColumn);
+ const [selectedTab, setSelectedTab] = useState<string | null>(null);
 
  const [resizeButton, width, height] = useResizeButton(
  POPOVER_INITIAL_WIDTH,
@@ -188,7 +191,34 @@ const ColumnSelectPopover = ({
 
  useEffect(() => {
  getCurrentTab(defaultActiveTabKey);
- }, [defaultActiveTabKey, getCurrentTab]);
+ setSelectedTab(defaultActiveTabKey);
+ }, [defaultActiveTabKey, getCurrentTab, setSelectedTab]);
+
+ useEffect(() => {
+ /* if the adhoc column is not set (because it was never edited) but the
+ * tab is selected and the label has changed, then we need to set the
+ * adhoc column manually */
+ if (
+ adhocColumn === undefined &&
+ selectedTab === 'sqlExpression' &&
+ hasCustomLabel
+ ) {
+ const sqlExpression =
+ selectedSimpleColumn?.column_name ||
+ selectedCalculatedColumn?.expression ||
+ '';
+ setAdhocColumn({ label, sqlExpression, expressionType: 'SQL' });
+ }
+ }, [
+ adhocColumn,
+ defaultActiveTabKey,
+ hasCustomLabel,
+ getCurrentTab,
+ label,
+ selectedCalculatedColumn,
+ selectedSimpleColumn,
+ selectedTab,
+ ]);
 
  const onSave = useCallback(() => {
  if (adhocColumn && adhocColumn.label !== label) {
@@ -225,6 +255,7 @@ const ColumnSelectPopover = ({
  const onTabChange = useCallback(
  tab => {
  getCurrentTab(tab);
+ setSelectedTab(tab);
  // @ts-ignore
  sqlEditorRef.current?.editor.focus();
  },

superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopoverTrigger.tsx

@@ -103,6 +103,7 @@ const ColumnSelectPopoverTrigger = ({
  setDatasetModal={setDatasetModal}
  onClose={handleClosePopover}
  onChange={onColumnEdit}
+ hasCustomLabel={hasCustomLabel}
  label={popoverLabel}
  setLabel={setPopoverLabel}
  getCurrentTab={getCurrentTab}
@@ -114,17 +115,21 @@ const ColumnSelectPopoverTrigger = ({
  columns,
  editedColumn,
  getCurrentTab,
+ hasCustomLabel,
  handleClosePopover,
  isTemporal,
  onColumnEdit,
  popoverLabel,
  ],
  );
 
- const onLabelChange = useCallback((e: any) => {
- setPopoverLabel(e.target.value);
- setHasCustomLabel(true);
- }, []);
+ const onLabelChange = useCallback(
+ (e: any) => {
+ setPopoverLabel(e.target.value);
+ setHasCustomLabel(true);
+ },
+ [setPopoverLabel, setHasCustomLabel],
+ );
 
  const popoverTitle = useMemo(
  () => (