From d006178c03abd117ea1e61919d0d8a82099d0f1c Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Mon, 5 Apr 2021 16:57:40 -0400 Subject: [PATCH] refactor: move CTAS/CVAS field II (#13877) Co-authored-by: lyndsiWilliams --- .../integration/chart_list/list_view.test.ts | 4 +- .../dashboard_list/list_view.test.ts | 4 +- .../spec/javascripts/sqllab/fixtures.ts | 2 + .../CRUD/data/database/DatabaseModal_spec.jsx | 238 ++++++++++++--- .../src/components/IndeterminateCheckbox.tsx | 51 +++- .../CRUD/data/database/DatabaseModal.tsx | 273 +++++++++++------- 6 files changed, 409 insertions(+), 163 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/integration/chart_list/list_view.test.ts b/superset-frontend/cypress-base/cypress/integration/chart_list/list_view.test.ts index 5d4e4fcfe572d..915bf15f0ae83 100644 --- a/superset-frontend/cypress-base/cypress/integration/chart_list/list_view.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/chart_list/list_view.test.ts @@ -51,8 +51,8 @@ describe('chart list view', () => { it('should bulk delete correctly', () => { cy.get('[data-test="listview-table"]').should('be.visible'); cy.get('[data-test="bulk-select"]').eq(0).click(); - cy.get('[data-test="checkbox-off"]').eq(1).click(); - cy.get('[data-test="checkbox-off"]').eq(2).click(); + cy.get('[data-test="checkbox-off"]').eq(1).siblings('input').click(); + cy.get('[data-test="checkbox-off"]').eq(2).siblings('input').click(); cy.get('[data-test="bulk-select-action"]').eq(0).click(); cy.get('[data-test="delete-modal-input"]').eq(0).type('DELETE'); cy.get('[data-test="modal-confirm-button"]').eq(0).click(); diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard_list/list_view.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard_list/list_view.test.ts index c5f25a0504953..924cf84b6e462 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard_list/list_view.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard_list/list_view.test.ts @@ -51,8 +51,8 @@ describe('dashboard list view', () => { it('should bulk delete correctly', () => { cy.get('[data-test="listview-table"]').should('be.visible'); cy.get('[data-test="bulk-select"]').eq(0).click(); - cy.get('[data-test="checkbox-off"]').eq(1).click(); - cy.get('[data-test="checkbox-off"]').eq(2).click(); + cy.get('[data-test="checkbox-off"]').eq(1).siblings('input').click(); + cy.get('[data-test="checkbox-off"]').eq(2).siblings('input').click(); cy.get('[data-test="bulk-select-action"]').eq(0).click(); cy.get('[data-test="delete-modal-input"]').eq(0).type('DELETE'); cy.get('[data-test="modal-confirm-button"]').eq(0).click(); diff --git a/superset-frontend/spec/javascripts/sqllab/fixtures.ts b/superset-frontend/spec/javascripts/sqllab/fixtures.ts index f4ab24dd558ce..fa3209cddd70b 100644 --- a/superset-frontend/spec/javascripts/sqllab/fixtures.ts +++ b/superset-frontend/spec/javascripts/sqllab/fixtures.ts @@ -480,6 +480,8 @@ export const initialState = { DEFAULT_SQLLAB_LIMIT: 1000, SQL_MAX_ROW: 100000, DISPLAY_MAX_ROW: 100, + SQLALCHEMY_DOCS_URL: 'test_SQLALCHEMY_DOCS_URL', + SQLALCHEMY_DISPLAY_TEXT: 'test_SQLALCHEMY_DISPLAY_TEXT', }, }, }; diff --git a/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx b/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx index 901b18a67e361..213f7be744b96 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx +++ b/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx @@ -19,71 +19,243 @@ import React from 'react'; import thunk from 'redux-thunk'; import configureStore from 'redux-mock-store'; +import * as redux from 'react-redux'; import { styledMount as mount } from 'spec/helpers/theming'; +import { render, screen } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; +import { supersetTheme, ThemeProvider } from '@superset-ui/core'; +import { Provider } from 'react-redux'; import DatabaseModal from 'src/views/CRUD/data/database/DatabaseModal'; import Modal from 'src/common/components/Modal'; import Tabs from 'src/common/components/Tabs'; import fetchMock from 'fetch-mock'; import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint'; +import { initialState } from 'spec/javascripts/sqllab/fixtures'; // store needed for withToasts(DatabaseModal) const mockStore = configureStore([thunk]); const store = mockStore({}); - const mockedProps = { show: true, }; - const dbProps = { show: true, database: { id: 10, database_name: 'test', sqlalchemy_uri: 'sqllite:///user:pw/test', + expose_in_sqllab: true, }, }; -jest.mock('react-redux', () => ({ - ...jest.requireActual('react-redux'), - useSelector: jest.fn(), -})); - const DATABASE_ENDPOINT = 'glob:*/api/v1/database/*'; - fetchMock.get(DATABASE_ENDPOINT, {}); describe('DatabaseModal', () => { - const wrapper = mount(); - - it('renders', () => { - expect(wrapper.find(DatabaseModal)).toExist(); + describe('enzyme', () => { + let wrapper; + let spyOnUseSelector; + beforeAll(() => { + spyOnUseSelector = jest.spyOn(redux, 'useSelector'); + spyOnUseSelector.mockReturnValue(initialState.common.conf); + }); + beforeEach(() => { + wrapper = mount( + + + , + ); + }); + afterEach(() => { + wrapper.unmount(); + }); + it('renders', () => { + expect(wrapper.find(DatabaseModal)).toExist(); + }); + it('renders a Modal', () => { + expect(wrapper.find(Modal)).toExist(); + }); + it('renders "Add database" header when no database is included', () => { + expect(wrapper.find('h4').text()).toEqual('Add database'); + }); + it('renders "Edit database" header when database prop is included', () => { + const editWrapper = mount(); + waitForComponentToPaint(editWrapper); + expect(editWrapper.find('h4').text()).toEqual('Edit database'); + editWrapper.unmount(); + }); + it('renders a Tabs menu', () => { + expect(wrapper.find(Tabs)).toExist(); + }); + it('renders five TabPanes', () => { + expect(wrapper.find(Tabs.TabPane)).toExist(); + expect(wrapper.find(Tabs.TabPane)).toHaveLength(5); + }); + it('renders input elements for Connection section', () => { + expect(wrapper.find('input[name="database_name"]')).toExist(); + expect(wrapper.find('input[name="sqlalchemy_uri"]')).toExist(); + }); }); - it('renders a Modal', () => { - expect(wrapper.find(Modal)).toExist(); - }); + describe('RTL', () => { + describe('initial load', () => { + it('hides the forms from the db when not selected', () => { + render( + + + + + , + ); + // Select SQL Lab settings tab + const sqlLabSettingsTab = screen.getByRole('tab', { + name: /sql lab settings/i, + }); + userEvent.click(sqlLabSettingsTab); - it('renders "Add database" header when no database is included', () => { - expect(wrapper.find('h4').text()).toEqual('Add database'); - }); + const exposeInSqlLab = screen.getByText('Expose in SQL Lab'); + const exposeChoicesForm = exposeInSqlLab.parentElement.nextSibling; + const schemaField = screen.getByText('CTAS & CVAS SCHEMA') + .parentElement; + expect(exposeChoicesForm).not.toHaveClass('open'); + expect(schemaField).not.toHaveClass('open'); + }); + }); + it('renders all settings when "Expose in SQL Lab" is checked', () => { + render( + + + + + , + ); - it('renders "Edit database" header when database prop is included', () => { - const editWrapper = mount(); - waitForComponentToPaint(editWrapper); - expect(editWrapper.find('h4').text()).toEqual('Edit database'); - }); + // Select SQL Lab settings tab + const sqlLabSettingsTab = screen.getByRole('tab', { + name: /sql lab settings/i, + }); - it('renders a Tabs menu', () => { - expect(wrapper.find(Tabs)).toExist(); - }); + userEvent.click(sqlLabSettingsTab); + // Grab all SQL Lab Settings by their labels + // const exposeInSqlLab = screen.getByText('Expose in SQL Lab'); + const exposeInSqlLab = screen.getByRole('checkbox', { + name: /expose in sql lab/i, + }); - it('renders five TabPanes', () => { - expect(wrapper.find(Tabs.TabPane)).toExist(); - expect(wrapper.find(Tabs.TabPane)).toHaveLength(5); - }); + // While 'Expose in SQL Lab' is checked, all settings should display + expect(exposeInSqlLab).not.toBeChecked(); + + // When clicked, "Expose in SQL Lab" becomes unchecked + userEvent.click(exposeInSqlLab); + + // While checked make sure all checkboxes are showing + expect(exposeInSqlLab).toBeChecked(); + const checkboxes = screen + .getAllByRole('checkbox') + .filter(checkbox => !checkbox.checked); + + expect(checkboxes.length).toEqual(4); + }); + + it('renders the schema field when allowCTAS is checked', () => { + render( + + + + + , + ); + + // Select SQL Lab settings tab + const sqlLabSettingsTab = screen.getByRole('tab', { + name: /sql lab settings/i, + }); + userEvent.click(sqlLabSettingsTab); + // Grab CTAS & schema field by their labels + const allowCTAS = screen.getByLabelText('Allow CREATE TABLE AS'); + const schemaField = screen.getByText('CTAS & CVAS SCHEMA').parentElement; + + // While CTAS & CVAS are unchecked, schema field is not visible + expect(schemaField).not.toHaveClass('open'); + + // Check "Allow CTAS" to reveal schema field + userEvent.click(allowCTAS); + expect(schemaField).toHaveClass('open'); + + // Uncheck "Allow CTAS" to hide schema field again + userEvent.click(allowCTAS); + expect(schemaField).not.toHaveClass('open'); + }); + + it('renders the schema field when allowCVAS is checked', () => { + render( + + + + + , + ); + + // Select SQL Lab settings tab + const sqlLabSettingsTab = screen.getByRole('tab', { + name: /sql lab settings/i, + }); + userEvent.click(sqlLabSettingsTab); + // Grab CVAS by it's label & schema field + const allowCVAS = screen.getByText('Allow CREATE VIEW AS'); + const schemaField = screen.getByText('CTAS & CVAS SCHEMA').parentElement; + + // While CTAS & CVAS are unchecked, schema field is not visible + expect(schemaField).not.toHaveClass('open'); + + // Check "Allow CVAS" to reveal schema field + userEvent.click(allowCVAS); + expect(schemaField).toHaveClass('open'); + + // Uncheck "Allow CVAS" to hide schema field again + userEvent.click(allowCVAS); + expect(schemaField).not.toHaveClass('open'); + }); + + it('renders the schema field when both allowCTAS and allowCVAS are checked', () => { + render( + + + + + , + ); + + // Select SQL Lab settings tab + const sqlLabSettingsTab = screen.getByRole('tab', { + name: /sql lab settings/i, + }); + userEvent.click(sqlLabSettingsTab); + // Grab CTAS and CVAS by their labels, & schema field + const allowCTAS = screen.getByText('Allow CREATE TABLE AS'); + const allowCVAS = screen.getByText('Allow CREATE VIEW AS'); + const schemaField = screen.getByText('CTAS & CVAS SCHEMA').parentElement; + + // While CTAS & CVAS are unchecked, schema field is not visible + expect(schemaField).not.toHaveClass('open'); + + // Check both "Allow CTAS" and "Allow CVAS" to reveal schema field + userEvent.click(allowCTAS); + userEvent.click(allowCVAS); + expect(schemaField).toHaveClass('open'); + // Uncheck both "Allow CTAS" and "Allow CVAS" to hide schema field again + userEvent.click(allowCTAS); + userEvent.click(allowCVAS); - it('renders input elements for Connection section', () => { - expect(wrapper.find('input[name="database_name"]')).toExist(); - expect(wrapper.find('input[name="sqlalchemy_uri"]')).toExist(); + // Both checkboxes go unchecked, so the field should no longer render + expect(schemaField).not.toHaveClass('open'); + }); }); }); diff --git a/superset-frontend/src/components/IndeterminateCheckbox.tsx b/superset-frontend/src/components/IndeterminateCheckbox.tsx index 7133f89f719d4..aa706fa7c151f 100644 --- a/superset-frontend/src/components/IndeterminateCheckbox.tsx +++ b/superset-frontend/src/components/IndeterminateCheckbox.tsx @@ -26,19 +26,35 @@ interface IndeterminateCheckboxProps { checked: boolean; onChange: React.EventHandler>; title?: string; + labelText?: string; } const CheckboxLabel = styled.label` cursor: pointer; + display: inline-block; margin-bottom: 0; `; const IconWithColor = styled(Icon)` color: ${({ theme }) => theme.colors.primary.dark1}; + cursor: pointer; `; const HiddenInput = styled.input` - visibility: none; + &[type='checkbox'] { + cursor: pointer; + opacity: 0; + position: absolute; + left: 3px; + margin: 0; + top: 4px; + } +`; + +const InputContainer = styled.div` + cursor: pointer; + display: inline-block; + position: relative; `; const IndeterminateCheckbox = React.forwardRef( @@ -49,6 +65,7 @@ const IndeterminateCheckbox = React.forwardRef( checked, onChange, title = '', + labelText = '', }: IndeterminateCheckboxProps, ref: React.MutableRefObject, ) => { @@ -60,20 +77,24 @@ const IndeterminateCheckbox = React.forwardRef( }, [resolvedRef, indeterminate]); return ( - - {indeterminate && } - {!indeterminate && checked && } - {!indeterminate && !checked && } - - + <> + + {indeterminate && } + {!indeterminate && checked && } + {!indeterminate && !checked && } + + + + {labelText} + + ); }, ); diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx index cded9c88c9d58..beb062526a45e 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx @@ -43,6 +43,9 @@ interface DatabaseModalProps { } const DEFAULT_TAB_KEY = '1'; +const EXPOSE_SQLLAB_FORM_HEIGHT = '270px'; +const CTAS_CVAS_SCHEMA_FORM_HEIGHT = '94px'; + const StyledIcon = styled(Icon)` margin: auto ${({ theme }) => theme.gridUnit * 2}px auto 0; `; @@ -54,6 +57,17 @@ const StyledInputContainer = styled.div` padding-top: 8px; } + &.expandable { + height: 0; + overflow: hidden; + transition: height 0.25s; + margin-left: ${({ theme }) => theme.gridUnit * 8}px; + padding: 0; + &.open { + height: ${CTAS_CVAS_SCHEMA_FORM_HEIGHT}; + } + } + .helper { display: block; padding: ${({ theme }) => theme.gridUnit}px 0; @@ -69,11 +83,14 @@ const StyledInputContainer = styled.div` .input-container { display: flex; - align-items: center; + align-items: top; label { display: flex; - margin-right: ${({ theme }) => theme.gridUnit * 2}px; + margin-left: ${({ theme }) => theme.gridUnit * 2}px; + margin-top: ${({ theme }) => theme.gridUnit * 0.75}px; + font-family: ${({ theme }) => theme.typography.families.sansSerif}; + font-size: ${({ theme }) => theme.typography.sizes.m}px; } i { @@ -122,6 +139,23 @@ const StyledJsonEditor = styled(JsonEditor)` border-radius: ${({ theme }) => theme.gridUnit}px; `; +const StyledExpandableForm = styled.div` + padding-top: ${({ theme }) => theme.gridUnit}px; + .input-container { + padding-top: ${({ theme }) => theme.gridUnit}px; + padding-bottom: ${({ theme }) => theme.gridUnit}px; + } + &.expandable { + height: 0; + overflow: hidden; + transition: height 0.25s; + margin-left: ${({ theme }) => theme.gridUnit * 7}px; + &.open { + height: ${EXPOSE_SQLLAB_FORM_HEIGHT}; + } + } +`; + const DatabaseModal: FunctionComponent = ({ addDangerToast, addSuccessToast, @@ -221,16 +255,17 @@ const DatabaseModal: FunctionComponent = ({ const onInputChange = (event: React.ChangeEvent) => { const { target } = event; + const { checked, name, value, type } = target; const data = { - database_name: db ? db.database_name : '', - sqlalchemy_uri: db ? db.sqlalchemy_uri : '', + database_name: db?.database_name || '', + sqlalchemy_uri: db?.sqlalchemy_uri || '', ...db, }; - if (target.type === 'checkbox') { - data[target.name] = target.checked; + if (type === 'checkbox') { + data[name] = checked; } else { - data[target.name] = target.value; + data[name] = value; } setDB(data); @@ -238,20 +273,21 @@ const DatabaseModal: FunctionComponent = ({ const onTextChange = (event: React.ChangeEvent) => { const { target } = event; + const { name, value } = target; const data = { - database_name: db ? db.database_name : '', - sqlalchemy_uri: db ? db.sqlalchemy_uri : '', + database_name: db?.database_name || '', + sqlalchemy_uri: db?.sqlalchemy_uri || '', ...db, }; - data[target.name] = target.value; + data[name] = value; setDB(data); }; const onEditorChange = (json: string, name: string) => { const data = { - database_name: db ? db.database_name : '', - sqlalchemy_uri: db ? db.sqlalchemy_uri : '', + database_name: db?.database_name || '', + sqlalchemy_uri: db?.sqlalchemy_uri || '', ...db, }; @@ -275,9 +311,9 @@ const DatabaseModal: FunctionComponent = ({ // Initialize if ( isEditMode && - (!db || !db.id || (database && database.id !== db.id) || (isHidden && show)) + (!db || !db.id || database?.id !== db.id || (isHidden && show)) ) { - if (database && database.id !== null && !dbLoading) { + if (database?.id && !dbLoading) { const id = database.id || 0; setTabKey(DEFAULT_TAB_KEY); @@ -285,11 +321,11 @@ const DatabaseModal: FunctionComponent = ({ .then(() => { setDB(dbFetched); }) - .catch(errMsg => + .catch(e => addDangerToast( t( 'Sorry there was an error fetching database information: %s', - errMsg.message, + e.message, ), ), ); @@ -305,7 +341,7 @@ const DatabaseModal: FunctionComponent = ({ // Validation useEffect(() => { validate(); - }, [db ? db.database_name : null, db ? db.sqlalchemy_uri : null]); + }, [db?.database_name || null, db?.sqlalchemy_uri || null]); // Show/hide if (isHidden && show) { @@ -316,6 +352,9 @@ const DatabaseModal: FunctionComponent = ({ setTabKey(key); }; + const expandableModalIsOpen = !!db?.expose_in_sqllab; + const createAsOpen = !!(db?.allow_ctas || db?.allow_cvas); + return ( = ({ @@ -371,7 +410,7 @@ const DatabaseModal: FunctionComponent = ({ = ({ @@ -420,10 +459,10 @@ const DatabaseModal: FunctionComponent = ({ -
{t('Asynchronous query execution')}
= ({ -
{t('Expose in SQL Lab')}
+ + +
+ + +
+ + +
+ + +
+ +
+ {t('CTAS & CVAS SCHEMA')} +
+
+ +
+
+ {t( + 'When allowing CREATE TABLE AS option in SQL Lab, this option ' + + 'forces the table to be created in this schema.', + )} +
+ + + +
+ + +
+ + +
+ + +
+ + - -
- -
{t('Allow CREATE TABLE AS')}
- -
- - -
- -
{t('Allow CREATE VIEW AS')}
- -
- - -
- -
{t('Allow DML')}
- -
- - -
- -
{t('Allow multi schema metadata fetch')}
- -
- - - -
{t('CTAS schema')}
-
- -
-
- {t( - 'When allowing CREATE TABLE AS option in SQL Lab, this option ' + - 'forces the table to be created in this schema.', - )} -
{t('Security')}} key="4"> @@ -539,7 +590,7 @@ const DatabaseModal: FunctionComponent = ({
onEditorChange(json, 'encrypted_extra') @@ -568,7 +619,7 @@ const DatabaseModal: FunctionComponent = ({