fix(explore): clean data when hidding control (#19039)

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx

@@ -34,6 +34,7 @@
  * control interface.
  */
 import React from 'react';
+import { isEmpty } from 'lodash';
 import {
   FeatureFlag,
   t,
@@ -43,7 +44,6 @@ import {
   SequentialScheme,
   legacyValidateInteger,
   validateNonEmpty,
-  JsonArray,
   ComparisionType,
 } from '@superset-ui/core';
 
@@ -352,7 +352,7 @@ const order_desc: SharedControlConfig<'CheckboxControl'> = {
   visibility: ({ controls }) =>
     Boolean(
       controls?.timeseries_limit_metric.value &&
-        (controls?.timeseries_limit_metric.value as JsonArray).length,
+        !isEmpty(controls?.timeseries_limit_metric.value),
     ),
 };
 

superset-frontend/src/explore/components/Control.test.tsx

@@ -0,0 +1,94 @@
+/**
+ * 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 { mount } from 'enzyme';
+import {
+  ThemeProvider,
+  supersetTheme,
+  promiseTimeout,
+} from '@superset-ui/core';
+import React from 'react';
+import { render, screen } from 'spec/helpers/testing-library';
+import Control, { ControlProps } from 'src/explore/components/Control';
+
+const defaultProps: ControlProps = {
+  type: 'CheckboxControl',
+  name: 'checkbox',
+  value: true,
+  actions: {
+    setControlValue: jest.fn(),
+  },
+};
+
+const setup = (overrides = {}) => (
+  <ThemeProvider theme={supersetTheme}>
+    <Control {...defaultProps} {...overrides} />
+  </ThemeProvider>
+);
+
+describe('Control', () => {
+  it('render a control', () => {
+    render(setup());
+
+    const checkbox = screen.getByRole('checkbox');
+    expect(checkbox).toBeVisible();
+  });
+
+  it('render null if type is not exit', () => {
+    render(
+      setup({
+        type: undefined,
+      }),
+    );
+    expect(screen.queryByRole('checkbox')).not.toBeInTheDocument();
+  });
+
+  it('render null if type is not valid', () => {
+    render(
+      setup({
+        type: 'UnkownControl',
+      }),
+    );
+    expect(screen.queryByRole('checkbox')).not.toBeInTheDocument();
+  });
+
+  it('render null if isVisible is false', () => {
+    render(
+      setup({
+        isVisible: false,
+      }),
+    );
+    expect(screen.queryByRole('checkbox')).not.toBeInTheDocument();
+  });
+
+  it('call setControlValue if isVisible is false', () => {
+    const wrapper = mount(
+      setup({
+        isVisible: true,
+        default: false,
+      }),
+    );
+    wrapper.setProps({
+      isVisible: false,
+      default: false,
+    });
+    promiseTimeout(() => {
+      expect(defaultProps.actions.setControlValue).toBeCalled();
+    }, 100);
+  });
+});

superset-frontend/src/explore/components/Control.tsx

@@ -16,12 +16,14 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { ReactNode, useCallback, useState } from 'react';
+import React, { ReactNode, useCallback, useState, useEffect } from 'react';
+import { isEqual } from 'lodash';
 import {
   ControlType,
   ControlComponentProps as BaseControlComponentProps,
 } from '@superset-ui/chart-controls';
 import { styled, JsonValue, QueryFormData } from '@superset-ui/core';
+import { usePrevious } from 'src/hooks/usePrevious';
 import ErrorBoundary from 'src/components/ErrorBoundary';
 import { ExploreActions } from 'src/explore/actions/exploreActions';
 import controlMap from './controls';
@@ -42,6 +44,8 @@ export type ControlProps = {
   validationErrors?: any[];
   hidden?: boolean;
   renderTrigger?: boolean;
+  default?: JsonValue;
+  isVisible?: boolean;
 };
 
 /**
@@ -60,15 +64,36 @@ export default function Control(props: ControlProps) {
     name,
     type,
     hidden,
+    isVisible,
   } = props;
 
   const [hovered, setHovered] = useState(false);
+  const wasVisible = usePrevious(isVisible);
   const onChange = useCallback(
     (value: any, errors: any[]) => setControlValue(name, value, errors),
     [name, setControlValue],
   );
 
-  if (!type) return null;
+  useEffect(() => {
+    if (
+      wasVisible === true &&
+      isVisible === false &&
+      props.default !== undefined &&
+      !isEqual(props.value, props.default)
+    ) {
+      // reset control value if setting to invisible
+      setControlValue?.(name, props.default);
+    }
+  }, [
+    name,
+    wasVisible,
+    isVisible,
+    setControlValue,
+    props.value,
+    props.default,
+  ]);
+
+  if (!type || isVisible === false) return null;
 
   const ControlComponent = typeof type === 'string' ? controlMap[type] : type;
   if (!ControlComponent) {

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

@@ -283,16 +283,17 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
       validationErrors?: any[];
     };
 
-    // if visibility check says the config is not visible, don't render it
-    if (visibility && !visibility.call(config, props, controlData)) {
-      return null;
-    }
+    const isVisible = visibility
+      ? visibility.call(config, props, controlData)
+      : undefined;
+
     return (
       <Control
         key={`control-${name}`}
         name={name}
         validationErrors={validationErrors}
         actions={props.actions}
+        isVisible={isVisible}
         {...restProps}
       />
     );

superset-frontend/src/explore/components/ControlRow.test.tsx

@@ -17,20 +17,51 @@
  * under the License.
  */
 import React from 'react';
-import { render } from 'spec/helpers/testing-library';
+import { render, screen } from 'spec/helpers/testing-library';
 import ControlSetRow from 'src/explore/components/ControlRow';
 
+const MockControl = (props: {
+  children: React.ReactElement;
+  type?: string;
+  isVisible?: boolean;
+}) => <div>{props.children}</div>;
 describe('ControlSetRow', () => {
   it('renders a single row with one element', () => {
-    const { getAllByText } = render(
-      <ControlSetRow controls={[<p>My Control 1</p>]} />,
-    );
-    expect(getAllByText('My Control 1').length).toBe(1);
+    render(<ControlSetRow controls={[<p>My Control 1</p>]} />);
+    expect(screen.getAllByText('My Control 1').length).toBe(1);
   });
   it('renders a single row with two elements', () => {
-    const { getAllByText } = render(
+    render(
       <ControlSetRow controls={[<p>My Control 1</p>, <p>My Control 2</p>]} />,
     );
-    expect(getAllByText(/My Control/)).toHaveLength(2);
+    expect(screen.getAllByText(/My Control/)).toHaveLength(2);
+  });
+
+  it('renders a single row with one elements if is HiddenControl', () => {
+    render(
+      <ControlSetRow
+        controls={[
+          <p>My Control 1</p>,
+          <MockControl type="HiddenControl">
+            <p>My Control 2</p>
+          </MockControl>,
+        ]}
+      />,
+    );
+    expect(screen.getAllByText(/My Control/)).toHaveLength(2);
+  });
+
+  it('renders a single row with one elements if is invisible', () => {
+    render(
+      <ControlSetRow
+        controls={[
+          <p>My Control 1</p>,
+          <MockControl isVisible={false}>
+            <p>My Control 2</p>
+          </MockControl>,
+        ]}
+      />,
+    );
+    expect(screen.getAllByText(/My Control/)).toHaveLength(2);
   });
 });

superset-frontend/src/explore/components/ControlRow.tsx

@@ -16,26 +16,34 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React from 'react';
+import React, { useCallback } from 'react';
 
 const NUM_COLUMNS = 12;
 
 type Control = React.ReactElement | null;
 
 export default function ControlRow({ controls }: { controls: Control[] }) {
-  // ColorMapControl renders null and should not be counted
+  const isHiddenControl = useCallback(
+    (control: Control) =>
+      control?.props.type === 'HiddenControl' ||
+      control?.props.isVisible === false,
+    [],
+  );
+  // Invisible control should not be counted
   // in the columns number
   const countableControls = controls.filter(
-    control => !['ColorMapControl'].includes(control?.props.type),
+    control => !isHiddenControl(control),
   );
-  const colSize = NUM_COLUMNS / countableControls.length;
+  const colSize = countableControls.length
+    ? NUM_COLUMNS / countableControls.length
+    : NUM_COLUMNS;
   return (
     <div className="row">
       {controls.map((control, i) => (
         <div
           className={`col-lg-${colSize} col-xs-12`}
           style={{
-            display: control?.props.type === 'HiddenControl' ? 'none' : 'block',
+            display: isHiddenControl(control) ? 'none' : 'block',
           }}
           key={i}
         >