This is an automated email from the ASF dual-hosted git repository.
johnbodley pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 16c449748a fix: Custom SQL filter control (#29260)
16c449748a is described below
commit 16c449748a4b1a0811285ef5c8765cc8b447907b
Author: Michael S. Molina <[email protected]>
AuthorDate: Fri Jun 14 14:26:58 2024 -0300
fix: Custom SQL filter control (#29260)
---
superset-frontend/spec/helpers/testing-library.tsx | 24 ++++++-
.../ScopingModal/ScopingModal.test.tsx | 34 +++-------
.../controls/FilterControl/AdhocFilter/index.js | 1 +
.../FilterControl/AdhocFilterEditPopover/index.jsx | 5 +-
.../AdhocFilterEditPopoverSqlTabContent.test.jsx | 73 ---------------------
.../AdhocFilterEditPopoverSqlTabContent.test.tsx | 75 ++++++++++++++++++++++
.../AdhocFilterEditPopoverSqlTabContent/index.jsx | 26 +++-----
.../AdhocMetricEditPopover.test.tsx | 7 +-
.../features/rls/RowLevelSecurityModal.test.tsx | 31 +++------
9 files changed, 132 insertions(+), 144 deletions(-)
diff --git a/superset-frontend/spec/helpers/testing-library.tsx
b/superset-frontend/spec/helpers/testing-library.tsx
index 61ccc7fb0d..625531f926 100644
--- a/superset-frontend/spec/helpers/testing-library.tsx
+++ b/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);
+}
diff --git
a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ScopingModal.test.tsx
b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ScopingModal.test.tsx
index 9411d3e7a4..392db3ae09 100644
---
a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ScopingModal.test.tsx
+++
b/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'),
);
diff --git
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js
b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js
index 627cc50c44..92cb69eebc 100644
---
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.js
+++
b/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 &&
diff --git
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
index a0e8f33b94..01169ff187 100644
---
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
+++
b/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 {
diff --git
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.jsx
b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.jsx
deleted file mode 100644
index 7366e7e0ef..0000000000
---
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.jsx
+++ /dev/null
@@ -1,73 +0,0 @@
-/**
- * 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.
- */
-/* eslint-disable no-unused-expressions */
-import sinon from 'sinon';
-import { shallow } from 'enzyme';
-
-import AdhocFilter from
'src/explore/components/controls/FilterControl/AdhocFilter';
-import AdhocFilterEditPopoverSqlTabContent from '.';
-import { Clauses, ExpressionTypes } from '../types';
-
-const sqlAdhocFilter = new AdhocFilter({
- expressionType: ExpressionTypes.Sql,
- sqlExpression: 'value > 10',
- clause: Clauses.Where,
-});
-
-function setup(overrides) {
- const onChange = sinon.spy();
- const props = {
- adhocFilter: sqlAdhocFilter,
- onChange,
- options: [],
- height: 100,
- ...overrides,
- };
- const wrapper = shallow(<AdhocFilterEditPopoverSqlTabContent {...props} />);
- return { wrapper, onChange };
-}
-
-describe('AdhocFilterEditPopoverSqlTabContent', () => {
- it('renders the sql tab form', () => {
- const { wrapper } = setup();
- expect(wrapper).toExist();
- });
-
- it('passes the new clause to onChange after onSqlExpressionClauseChange', ()
=> {
- const { wrapper, onChange } = setup();
- wrapper.instance().onSqlExpressionClauseChange(Clauses.Having);
- expect(onChange.calledOnce).toBe(true);
- expect(
- onChange.lastCall.args[0].equals(
- sqlAdhocFilter.duplicateWith({ clause: Clauses.Having }),
- ),
- ).toBe(true);
- });
-
- it('passes the new query to onChange after onSqlExpressionChange', () => {
- const { wrapper, onChange } = setup();
- wrapper.instance().onSqlExpressionChange('value < 5');
- expect(onChange.calledOnce).toBe(true);
- expect(
- onChange.lastCall.args[0].equals(
- sqlAdhocFilter.duplicateWith({ sqlExpression: 'value < 5' }),
- ),
- ).toBe(true);
- });
-});
diff --git
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.tsx
b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.tsx
new file mode 100644
index 0000000000..1d518a5c63
--- /dev/null
+++
b/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 }),
+ );
+});
diff --git
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/index.jsx
b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/index.jsx
index 2ae0fd6164..f24dff5aab 100644
---
a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/index.jsx
+++
b/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,7 +105,7 @@ 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,
}));
@@ -123,11 +113,15 @@ export default class AdhocFilterEditPopoverSqlTabContent
extends Component {
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 />
diff --git
a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx
b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx
index f0ebd1fc14..1aa7e3acaa 100644
---
a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx
+++
b/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);
diff --git a/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx
b/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx
index b5954c8d96..dfd22abc72 100644
--- a/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx
+++ b/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx
@@ -17,7 +17,12 @@
* under the License.
*/
import fetchMock from 'fetch-mock';
-import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
+import {
+ render,
+ screen,
+ selectOption,
+ waitFor,
+} from 'spec/helpers/testing-library';
import { act } from 'react-dom/test-utils';
import userEvent from '@testing-library/user-event';
import RowLevelSecurityModal, {
@@ -230,17 +235,7 @@ describe('Rule modal', () => {
expect(addButton).toBeDisabled();
- const getSelect = () => screen.getByRole('combobox', { name: 'Tables' });
- const getElementByClassName = (className: string) =>
- document.querySelector(className)! as HTMLElement;
-
- const findSelectOption = (text: string) =>
- waitFor(() =>
- within(getElementByClassName('.rc-virtual-list')).getByText(text),
- );
- const open = () => waitFor(() => userEvent.click(getSelect()));
- await open();
- userEvent.click(await findSelectOption('birth_names'));
+ await selectOption('birth_names', 'Tables');
expect(addButton).toBeDisabled();
const clause = await screen.findByTestId('clause-test');
@@ -257,17 +252,7 @@ describe('Rule modal', () => {
const nameTextBox = screen.getByTestId('rule-name-test');
userEvent.type(nameTextBox, 'name');
- const getSelect = () => screen.getByRole('combobox', { name: 'Tables' });
- const getElementByClassName = (className: string) =>
- document.querySelector(className)! as HTMLElement;
-
- const findSelectOption = (text: string) =>
- waitFor(() =>
- within(getElementByClassName('.rc-virtual-list')).getByText(text),
- );
- const open = () => waitFor(() => userEvent.click(getSelect()));
- await open();
- userEvent.click(await findSelectOption('birth_names'));
+ await selectOption('birth_names', 'Tables');
const clause = await screen.findByTestId('clause-test');
userEvent.type(clause, 'gender="girl"');
