fix: Custom SQL filter control (#29260)

superset-frontend/spec/helpers/testing-library.tsx

@@ -18,7 +18,13 @@
  */
 import '@testing-library/jest-dom/extend-expect';
 import { ReactNode, ReactElement } from 'react';
-import { render, RenderOptions } from '@testing-library/react';
+import {
+  render,
+  RenderOptions,
+  screen,
+  waitFor,
+  within,
+} from '@testing-library/react';
 import { ThemeProvider, supersetTheme } from '@superset-ui/core';
 import { BrowserRouter } from 'react-router-dom';
 import { Provider } from 'react-redux';
@@ -28,6 +34,7 @@ import reducerIndex from 'spec/helpers/reducerIndex';
 import { QueryParamProvider } from 'use-query-params';
 import { configureStore, Store } from '@reduxjs/toolkit';
 import { api } from 'src/hooks/apiResources/queryApi';
+import userEvent from '@testing-library/user-event';
 
 type Options = Omit<RenderOptions, 'queries'> & {
   useRedux?: boolean;
@@ -102,3 +109,18 @@ export function sleep(time: number) {
 
 export * from '@testing-library/react';
 export { customRender as render };
+
+export async function selectOption(option: string, selectName?: string) {
+  const select = screen.getByRole(
+    'combobox',
+    selectName ? { name: selectName } : {},
+  );
+  await userEvent.click(select);
+  const item = await waitFor(() =>
+    within(
+      // eslint-disable-next-line testing-library/no-node-access
+      document.querySelector('.rc-virtual-list')!,
+    ).getByText(option),
+  );
+  await userEvent.click(item);
+}

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ScopingModal.test.tsx

@@ -18,7 +18,13 @@
  */
 import fetchMock from 'fetch-mock';
 import userEvent from '@testing-library/user-event';
-import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
+import {
+  render,
+  screen,
+  selectOption,
+  waitFor,
+  within,
+} from 'spec/helpers/testing-library';
 import {
   CHART_TYPE,
   DASHBOARD_ROOT_TYPE,
@@ -199,21 +205,7 @@ it('add new custom scoping', async () => {
   expect(screen.getByText('[new custom scoping]')).toBeInTheDocument();
   expect(screen.getByText('[new custom scoping]')).toHaveClass('active');
 
-  await waitFor(() =>
-    userEvent.click(screen.getByRole('combobox', { name: 'Select chart' })),
-  );
-  await waitFor(() => {
-    userEvent.click(
-      within(document.querySelector('.rc-virtual-list')!).getByText('chart 1'),
-    );
-  });
-
-  expect(
-    within(document.querySelector('.ant-select-selection-item')!).getByText(
-      'chart 1',
-    ),
-  ).toBeInTheDocument();
-
+  await selectOption('chart 1', 'Select chart');
   expect(
     document.querySelectorAll(
       '[data-test="scoping-tree-panel"] .ant-tree-checkbox-checked',
@@ -250,14 +242,8 @@ it('edit scope and save', async () => {
 
   // create custom scoping for chart 1 with unselected chart 2 (from global) and chart 4
   userEvent.click(screen.getByText('Add custom scoping'));
-  await waitFor(() =>
-    userEvent.click(screen.getByRole('combobox', { name: 'Select chart' })),
-  );
-  await waitFor(() => {
-    userEvent.click(
-      within(document.querySelector('.rc-virtual-list')!).getByText('chart 1'),
-    );
-  });
+  await selectOption('chart 1', 'Select chart');
+
   userEvent.click(
     within(document.querySelector('.ant-tree')!).getByText('chart 4'),
   );

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js

@@ -92,6 +92,7 @@ export default class AdhocFilter {
 
   equals(adhocFilter) {
     return (
+      adhocFilter.clause === this.clause &&
       adhocFilter.expressionType === this.expressionType &&
       adhocFilter.sqlExpression === this.sqlExpression &&
       adhocFilter.operator === this.operator &&

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx

@@ -73,11 +73,12 @@ const FilterPopoverContentContainer = styled.div`
 
   .filter-edit-clause-info {
     font-size: ${({ theme }) => theme.typography.sizes.xs}px;
-    padding-left: ${({ theme }) => theme.gridUnit}px;
   }
 
   .filter-edit-clause-section {
-    display: inline-flex;
+    display: flex;
+    flex-direction: row;
+    gap: ${({ theme }) => theme.gridUnit * 5}px;
   }
 
   .adhoc-filter-simple-column-dropdown {

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.tsx

@@ -0,0 +1,75 @@
+/**
+ * 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 { render, screen, selectOption } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { IAceEditorProps } from 'react-ace';
+import AdhocFilter from '../AdhocFilter';
+import { Clauses, ExpressionTypes } from '../types';
+import AdhocFilterEditPopoverSqlTabContent from '.';
+
+jest.mock('src/components/AsyncAceEditor', () => ({
+  SQLEditor: ({ value, onChange }: IAceEditorProps) => (
+    <textarea
+      defaultValue={value}
+      onChange={value => onChange?.(value.target.value)}
+    />
+  ),
+}));
+
+const adhocFilter = new AdhocFilter({
+  expressionType: ExpressionTypes.Sql,
+  sqlExpression: 'value > 10',
+  clause: Clauses.Where,
+});
+
+test('calls onChange when the SQL clause changes', async () => {
+  const onChange = jest.fn();
+  render(
+    <AdhocFilterEditPopoverSqlTabContent
+      adhocFilter={adhocFilter}
+      onChange={onChange}
+      options={[]}
+      height={100}
+    />,
+  );
+  await selectOption(Clauses.Having);
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ clause: Clauses.Having }),
+  );
+});
+
+test('calls onChange when the SQL expression changes', () => {
+  const onChange = jest.fn();
+  const input = 'value < 20';
+  render(
+    <AdhocFilterEditPopoverSqlTabContent
+      adhocFilter={adhocFilter}
+      onChange={onChange}
+      options={[]}
+      height={100}
+    />,
+  );
+  const sqlEditor = screen.getByRole('textbox');
+  expect(sqlEditor).toBeInTheDocument();
+  userEvent.clear(sqlEditor);
+  userEvent.paste(sqlEditor, input);
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ sqlExpression: input }),
+  );
+});

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/index.jsx

@@ -39,7 +39,6 @@ const propTypes = {
     ]),
   ).isRequired,
   height: PropTypes.number.isRequired,
-  activeKey: PropTypes.string.isRequired,
 };
 
 const StyledSelect = styled(Select)`
@@ -56,10 +55,6 @@ export default class AdhocFilterEditPopoverSqlTabContent extends Component {
     this.onSqlExpressionClauseChange =
       this.onSqlExpressionClauseChange.bind(this);
     this.handleAceEditorRef = this.handleAceEditorRef.bind(this);
-
-    this.selectProps = {
-      ariaLabel: t('Select column'),
-    };
   }
 
   componentDidUpdate() {
@@ -95,11 +90,6 @@ export default class AdhocFilterEditPopoverSqlTabContent extends Component {
   render() {
     const { adhocFilter, height, options } = this.props;
 
-    const clauseSelectProps = {
-      placeholder: t('choose WHERE or HAVING...'),
-      value: adhocFilter.clause,
-      onChange: this.onSqlExpressionClauseChange,
-    };
     const keywords = sqlKeywords.concat(
       options
         .map(option => {
@@ -115,19 +105,23 @@ export default class AdhocFilterEditPopoverSqlTabContent extends Component {
         })
         .filter(Boolean),
     );
-    const selectOptions = Object.keys(Clauses).map(clause => ({
+    const selectOptions = Object.values(Clauses).map(clause => ({
       label: clause,
       value: clause,
     }));
 
     return (
       <span>
         <div className="filter-edit-clause-section">
-          <StyledSelect
-            options={selectOptions}
-            {...this.selectProps}
-            {...clauseSelectProps}
-          />
+          <div>
+            <StyledSelect
+              options={selectOptions}
+              ariaLabel={t('Select column')}
+              placeholder={t('choose WHERE or HAVING...')}
+              value={adhocFilter.clause}
+              onChange={this.onSqlExpressionClauseChange}
+            />
+          </div>
           <span className="filter-edit-clause-info">
             <strong>WHERE</strong> {t('Filters by columns')}
             <br />

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx

@@ -17,7 +17,7 @@
  * under the License.
  */
 import userEvent from '@testing-library/user-event';
-import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
+import { render, screen, selectOption } from 'spec/helpers/testing-library';
 import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
 import AdhocMetricEditPopover from '.';
 
@@ -132,10 +132,7 @@ test('Clicking on "Save" should call onChange and onClose', async () => {
       name: 'Select saved metrics',
     }),
   );
-  const sumOption = await waitFor(() =>
-    within(document.querySelector('.rc-virtual-list')!).getByText('sum'),
-  );
-  userEvent.click(sumOption);
+  await selectOption('sum');
   userEvent.click(screen.getByRole('button', { name: 'Save' }));
   expect(props.onChange).toBeCalledTimes(1);
   expect(props.onClose).toBeCalledTimes(1);