fix(explore): don't discard controls on deprecated (#30447)

apache · Oct 4, 2024 · b627011 · b627011
1 parent 2aa9348
commit b627011
Show file tree

Hide file tree

Showing 10 changed files with 141 additions and 17 deletions.
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx
@@ -17,7 +17,7 @@
  * under the License.
  */
 import { useState, ReactNode, useLayoutEffect, RefObject } from 'react';
-import { css, styled, SupersetTheme } from '@superset-ui/core';
+import { css, SafeMarkdown, styled, SupersetTheme } from '@superset-ui/core';
 import { Tooltip } from './Tooltip';
 import { ColumnTypeLabel } from './ColumnTypeLabel/ColumnTypeLabel';
 import CertifiedIconWithTooltip from './CertifiedIconWithTooltip';
@@ -28,6 +28,7 @@ import {
   getColumnTypeTooltipNode,
 } from './labelUtils';
 import { SQLPopover } from './SQLPopover';
+import InfoTooltipWithTrigger from './InfoTooltipWithTrigger';
 
 export type ColumnOptionProps = {
   column: ColumnMeta;
@@ -50,6 +51,8 @@ export function ColumnOption({
 }: ColumnOptionProps) {
   const { expression, column_name, type_generic } = column;
   const hasExpression = expression && expression !== column_name;
+  const warningMarkdown =
+    column.warning_markdown || column.warning_text || column.error_text;
   const type = hasExpression ? 'expression' : type_generic;
   const [tooltipText, setTooltipText] = useState<ReactNode>(column.column_name);
   const [columnTypeTooltipText, setcolumnTypeTooltipText] = useState<ReactNode>(
@@ -94,6 +97,19 @@ export function ColumnOption({
           details={column.certification_details}
         />
       )}
+      {warningMarkdown && (
+        <InfoTooltipWithTrigger
+          className="text-warning"
+          icon="warning"
+          tooltip={<SafeMarkdown source={warningMarkdown} />}
+          label={`warn-${column.column_name}`}
+          iconsStyle={{ marginLeft: 0 }}
+          {...(column.error_text && {
+            className: 'text-danger',
+            icon: 'exclamation-circle',
+          })}
+        />
+      )}
     </StyleOverrides>
   );
 }

diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/components/MetricOption.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/components/MetricOption.tsx
@@ -81,7 +81,8 @@ export function MetricOption({
     </span>
   );
 
-  const warningMarkdown = metric.warning_markdown || metric.warning_text;
+  const warningMarkdown =
+    metric.warning_markdown || metric.warning_text || metric.error_text;
 
   const [tooltipText, setTooltipText] = useState<ReactNode>(metric.metric_name);
 
@@ -116,6 +117,10 @@ export function MetricOption({
           tooltip={<SafeMarkdown source={warningMarkdown} />}
           label={`warn-${metric.metric_name}`}
           iconsStyle={{ marginLeft: 0 }}
+          {...(metric.error_text && {
+            className: 'text-danger',
+            icon: 'exclamation-circle',
+          })}
         />
       )}
     </FlexRowContainer>

diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/components/ColumnOption.test.tsx b/superset-frontend/packages/superset-ui-chart-controls/test/components/ColumnOption.test.tsx
@@ -34,6 +34,9 @@ jest.mock('../../src/components/ColumnTypeLabel/ColumnTypeLabel', () => ({
     <div data-test="mock-column-type-label">{type}</div>
   ),
 }));
+jest.mock('../../src/components/InfoTooltipWithTrigger', () => () => (
+  <div data-test="mock-info-tooltip-with-trigger" />
+));
 
 const defaultProps: ColumnOptionProps = {
   column: {
@@ -111,3 +114,17 @@ test('dttm column has correct column label if showType is true', () => {
     String(GenericDataType.Temporal),
   );
 });
+test('doesnt show InfoTooltipWithTrigger when no warning', () => {
+  const { queryByText } = setup();
+  expect(queryByText('mock-info-tooltip-with-trigger')).not.toBeInTheDocument();
+});
+test('shows a warning with InfoTooltipWithTrigger when it contains warning', () => {
+  const { getByTestId } = setup({
+    ...defaultProps,
+    column: {
+      ...defaultProps.column,
+      warning_text: 'This is a warning',
+    },
+  });
+  expect(getByTestId('mock-info-tooltip-with-trigger')).toBeInTheDocument();
+});
diff --git a/superset-frontend/packages/superset-ui-core/src/query/types/Metric.ts b/superset-frontend/packages/superset-ui-core/src/query/types/Metric.ts
@@ -71,6 +71,7 @@ export interface Metric {
   verbose_name?: string;
   warning_markdown?: Maybe<string>;
   warning_text?: Maybe<string>;
+  error_text?: string;
 }
 
 export function isSavedMetric(metric: any): metric is SavedMetric {

diff --git a/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.test.tsx b/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.test.tsx
@@ -16,7 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { render, screen } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { render, screen, within } from 'spec/helpers/testing-library';
 import {
   DndColumnSelect,
   DndColumnSelectProps,
@@ -63,3 +64,52 @@ test('renders adhoc column', async () => {
   expect(await screen.findByText('adhoc column')).toBeVisible();
   expect(screen.getByLabelText('calculator')).toBeVisible();
 });
+
+test('warn selected custom metric when metric gets removed from dataset', async () => {
+  const columnValues = ['column1', 'column2'];
+
+  const { rerender, container } = render(
+    <DndColumnSelect
+      {...defaultProps}
+      options={[
+        {
+          column_name: 'column1',
+        },
+        {
+          column_name: 'column2',
+        },
+      ]}
+      value={columnValues}
+    />,
+    {
+      useDnd: true,
+      useRedux: true,
+    },
+  );
+
+  rerender(
+    <DndColumnSelect
+      {...defaultProps}
+      options={[
+        {
+          column_name: 'column3',
+        },
+        {
+          column_name: 'column2',
+        },
+      ]}
+      value={columnValues}
+    />,
+  );
+  expect(screen.getByText('column2')).toBeVisible();
+  expect(screen.queryByText('column1')).toBeInTheDocument();
+  const warningIcon = within(
+    screen.getByText('column1').parentElement ?? container,
+  ).getByRole('button');
+  expect(warningIcon).toBeInTheDocument();
+  userEvent.hover(warningIcon);
+  const warningTooltip = await screen.findByText(
+    'This column might be incompatible with current dataset',
+  );
+  expect(warningTooltip).toBeInTheDocument();
+});
diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
@@ -109,6 +109,8 @@ function DndColumnSelect(props: DndColumnSelectProps) {
           isAdhocColumn(column) && column.datasourceWarning
             ? t('This column might be incompatible with current dataset')
             : undefined;
+        const withCaret = isAdhocColumn(column) || !column.error_text;
+
         return (
           <ColumnSelectPopoverTrigger
             key={idx}
@@ -134,7 +136,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
               canDelete={canDelete}
               column={column}
               datasourceWarningMessage={datasourceWarningMessage}
-              withCaret
+              withCaret={withCaret}
             />
           </ColumnSelectPopoverTrigger>
         );

diff --git a/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx b/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
@@ -92,13 +92,13 @@ test('render selected metrics correctly', () => {
   expect(screen.getByText('SUM(Column B)')).toBeVisible();
 });
 
-test('remove selected custom metric when metric gets removed from dataset', () => {
+test('warn selected custom metric when metric gets removed from dataset', async () => {
   let metricValues = ['metric_a', 'metric_b', adhocMetricA, adhocMetricB];
   const onChange = (val: any[]) => {
     metricValues = val;
   };
 
-  const { rerender } = render(
+  const { rerender, container } = render(
     <DndMetricSelect
       {...defaultProps}
       value={metricValues}
@@ -129,19 +129,28 @@ test('remove selected custom metric when metric gets removed from dataset', () =
   );
   expect(screen.getByText('metric_a')).toBeVisible();
   expect(screen.queryByText('Metric B')).not.toBeInTheDocument();
-  expect(screen.queryByText('metric_b')).not.toBeInTheDocument();
+  expect(screen.queryByText('metric_b')).toBeInTheDocument();
+  const warningIcon = within(
+    screen.getByText('metric_b').parentElement ?? container,
+  ).getByRole('button');
+  expect(warningIcon).toBeInTheDocument();
+  userEvent.hover(warningIcon);
+  const warningTooltip = await screen.findByText(
+    'This metric might be incompatible with current dataset',
+  );
+  expect(warningTooltip).toBeInTheDocument();
   expect(screen.getByText('SUM(column_a)')).toBeVisible();
   expect(screen.getByText('SUM(Column B)')).toBeVisible();
 });
 
-test('remove selected custom metric when metric gets removed from dataset for single-select metric control', () => {
+test('warn selected custom metric when metric gets removed from dataset for single-select metric control', async () => {
   let metricValue = 'metric_b';
 
   const onChange = (val: any) => {
     metricValue = val;
   };
 
-  const { rerender } = render(
+  const { rerender, container } = render(
     <DndMetricSelect
       {...defaultProps}
       value={metricValue}
@@ -178,7 +187,19 @@ test('remove selected custom metric when metric gets removed from dataset for si
   );
 
   expect(screen.queryByText('Metric B')).not.toBeInTheDocument();
-  expect(screen.getByText('Drop a column/metric here or click')).toBeVisible();
+  expect(
+    screen.queryByText('Drop a column/metric here or click'),
+  ).not.toBeInTheDocument();
+  expect(screen.queryByText('metric_b')).toBeInTheDocument();
+  const warningIcon = within(
+    screen.getByText('metric_b').parentElement ?? container,
+  ).getByRole('button');
+  expect(warningIcon).toBeInTheDocument();
+  userEvent.hover(warningIcon);
+  const warningTooltip = await screen.findByText(
+    'This metric might be incompatible with current dataset',
+  );
+  expect(warningTooltip).toBeInTheDocument();
 });
 
 test('remove selected adhoc metric when column gets removed from dataset', async () => {

diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx
@@ -60,11 +60,6 @@ const coerceMetrics = (
   }
   const metricsCompatibleWithDataset = ensureIsArray(addedMetrics).filter(
     metric => {
-      if (isSavedMetric(metric)) {
-        return savedMetrics.some(
-          savedMetric => savedMetric.metric_name === metric,
-        );
-      }
       if (isAdhocMetricSimple(metric)) {
         return columns.some(
           column => column.column_name === metric.column.column_name,
@@ -75,6 +70,15 @@ const coerceMetrics = (
   );
 
   return metricsCompatibleWithDataset.map(metric => {
+    if (
+      isSavedMetric(metric) &&
+      !savedMetrics.some(savedMetric => savedMetric.metric_name === metric)
+    ) {
+      return {
+        metric_name: metric,
+        error_text: t('This metric might be incompatible with current dataset'),
+      };
+    }
     if (!isDictionaryForAdhocMetric(metric)) {
       return metric;
     }

diff --git a/...t-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts b/...t-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
@@ -22,6 +22,7 @@ import {
   ensureIsArray,
   QueryFormColumn,
   isPhysicalColumn,
+  t,
 } from '@superset-ui/core';
 
 const getColumnNameOrAdhocColumn = (
@@ -55,7 +56,13 @@ export class OptionSelector {
         if (!isPhysicalColumn(value)) {
           return value;
         }
-        return null;
+        return {
+          type_generic: 'UNKNOWN',
+          column_name: value,
+          error_text: t(
+            'This column might be incompatible with current dataset',
+          ),
+        };
       })
       .filter(Boolean) as ColumnMeta[];
   }

diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricOption.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricOption.jsx
@@ -67,6 +67,7 @@ class AdhocMetricOption extends PureComponent {
       multi,
       datasourceWarningMessage,
     } = this.props;
+    const withCaret = !savedMetric.error_text;
 
     return (
       <AdhocMetricPopoverTrigger
@@ -86,7 +87,7 @@ class AdhocMetricOption extends PureComponent {
           onDropLabel={onDropLabel}
           index={index}
           type={type ?? DndItemType.AdhocMetricOption}
-          withCaret
+          withCaret={withCaret}
           isFunction
           multi={multi}
           datasourceWarningMessage={datasourceWarningMessage}