This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 4.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit a37a1da40a8c93811636697d43d9a758f3d493ba 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 | 74 --------------------- .../AdhocFilterEditPopoverSqlTabContent.test.tsx | 76 ++++++++++++++++++++++ .../AdhocFilterEditPopoverSqlTabContent/index.jsx | 26 +++----- .../AdhocMetricEditPopover.test.tsx | 7 +- .../features/rls/RowLevelSecurityModal.test.tsx | 31 +++------ 9 files changed, 133 insertions(+), 145 deletions(-) diff --git a/superset-frontend/spec/helpers/testing-library.tsx b/superset-frontend/spec/helpers/testing-library.tsx index 9a8649a862..8564e62ab8 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 React, { 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 eef5d1fa07..3cc45f8168 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 @@ -19,7 +19,13 @@ import React from 'react'; 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, @@ -200,21 +206,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', @@ -251,14 +243,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 94b04b1114..9333bfb5db 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 9bdd20c0f0..0000000000 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.jsx +++ /dev/null @@ -1,74 +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 React from 'react'; -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..0bd8c59c2b --- /dev/null +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/AdhocFilterEditPopoverSqlTabContent.test.tsx @@ -0,0 +1,76 @@ +/** + * 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, 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 852bfc38b3..d6261e11f7 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 React.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 React.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 React.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 React.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 78add77814..a74e4c6d31 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 @@ -18,7 +18,7 @@ */ import userEvent from '@testing-library/user-event'; import React from 'react'; -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 '.'; @@ -133,10 +133,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 735e5fa756..85c5e2b0a5 100644 --- a/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx +++ b/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx @@ -18,7 +18,12 @@ */ import React from 'react'; 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, { @@ -231,17 +236,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'); @@ -258,17 +253,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"');
