From e028177a45cf8c13f65a1921b3593184c056c99b Mon Sep 17 00:00:00 2001 From: justin-park Date: Tue, 1 Nov 2022 18:49:37 -0700 Subject: [PATCH 1/6] fix(sqllab): inconsistent addNewQueryEditor behavior --- .../src/SqlLab/actions/sqlLab.js | 35 +++++++++++++++++-- .../src/SqlLab/components/SqlEditor/index.jsx | 2 +- .../components/TabbedSqlEditors/index.jsx | 22 +----------- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index a2bdd65d5c03e..964d567219837 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -600,16 +600,45 @@ export function addQueryEditor(queryEditor) { }; } -export function addNewQueryEditor(queryEditor) { +export function addNewQueryEditor() { return function (dispatch, getState) { const { - sqlLab: { queryEditors }, + sqlLab: { + queryEditors, + tabHistory, + unsavedQueryEditor, + defaultDbId, + databases, + }, + common, } = getState(); + const sourceQueryEditor = queryEditors.find( + qe => qe.id === tabHistory[tabHistory.length - 1], + ); + const firstDbId = Math.min( + ...Object.values(databases).map(database => database.id), + ); + const queryEditor = { + ...queryEditors[0], + ...sourceQueryEditor, + ...(unsavedQueryEditor.id === sourceQueryEditor?.id && + unsavedQueryEditor), + }; + const warning = isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) + ? '' + : t( + '-- Note: Unless you save your query, these tabs will NOT persist if you clear your cookies or change browsers.\n\n', + ); + const name = newQueryTabName(queryEditors || []); return dispatch( addQueryEditor({ - ...queryEditor, + dbId: queryEditor.dbId || defaultDbId || firstDbId, + schema: queryEditor.schema, + autorun: false, + sql: `${warning}SELECT ...`, + queryLimit: queryEditor.queryLimit || common.conf.DEFAULT_SQLLAB_LIMIT, name, }), ); diff --git a/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx b/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx index 99e7ddb8f2515..d7626c8cbf842 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx +++ b/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx @@ -319,7 +319,7 @@ const SqlEditor = ({ key: userOS === 'Windows' ? 'ctrl+q' : 'ctrl+t', descr: t('New tab'), func: () => { - dispatch(addNewQueryEditor(queryEditor)); + dispatch(addNewQueryEditor()); }, }, { diff --git a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx index 43259a37eedb9..1b3e3a999d778 100644 --- a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx +++ b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx @@ -201,27 +201,7 @@ class TabbedSqlEditors extends React.PureComponent { } newQueryEditor() { - const activeQueryEditor = this.activeQueryEditor(); - const firstDbId = Math.min( - ...Object.values(this.props.databases).map(database => database.id), - ); - const warning = isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) - ? '' - : t( - '-- Note: Unless you save your query, these tabs will NOT persist if you clear your cookies or change browsers.\n\n', - ); - - const qe = { - dbId: - activeQueryEditor && activeQueryEditor.dbId - ? activeQueryEditor.dbId - : this.props.defaultDbId || firstDbId, - schema: activeQueryEditor ? activeQueryEditor.schema : null, - autorun: false, - sql: `${warning}SELECT ...`, - queryLimit: this.props.defaultQueryLimit, - }; - this.props.actions.addNewQueryEditor(qe); + this.props.actions.addNewQueryEditor(); } handleSelect(key) { From 60908ad51b36ab7e807c9e49abc16759cf97fd99 Mon Sep 17 00:00:00 2001 From: justin-park Date: Tue, 1 Nov 2022 18:59:59 -0700 Subject: [PATCH 2/6] rename variable for activeQueryEditor --- superset-frontend/src/SqlLab/actions/sqlLab.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index 964d567219837..ae42876632187 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -612,7 +612,7 @@ export function addNewQueryEditor() { }, common, } = getState(); - const sourceQueryEditor = queryEditors.find( + const activeQueryEditor = queryEditors.find( qe => qe.id === tabHistory[tabHistory.length - 1], ); const firstDbId = Math.min( @@ -620,8 +620,8 @@ export function addNewQueryEditor() { ); const queryEditor = { ...queryEditors[0], - ...sourceQueryEditor, - ...(unsavedQueryEditor.id === sourceQueryEditor?.id && + ...activeQueryEditor, + ...(unsavedQueryEditor.id === activeQueryEditor?.id && unsavedQueryEditor), }; const warning = isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) From 5815ad8796a75ce6c329a786dbc978e928929da5 Mon Sep 17 00:00:00 2001 From: justin-park Date: Wed, 2 Nov 2022 10:06:31 -0700 Subject: [PATCH 3/6] update spec --- superset-frontend/src/SqlLab/actions/sqlLab.js | 15 ++++++++------- .../src/SqlLab/actions/sqlLab.test.js | 10 ++++++++-- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index ae42876632187..af828604d0e6a 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -615,10 +615,11 @@ export function addNewQueryEditor() { const activeQueryEditor = queryEditors.find( qe => qe.id === tabHistory[tabHistory.length - 1], ); - const firstDbId = Math.min( - ...Object.values(databases).map(database => database.id), - ); - const queryEditor = { + const firstDbId = + databases.length > 0 + ? Math.min(...Object.values(databases).map(database => database.id)) + : undefined; + const { dbId, schema, queryLimit } = { ...queryEditors[0], ...activeQueryEditor, ...(unsavedQueryEditor.id === activeQueryEditor?.id && @@ -634,11 +635,11 @@ export function addNewQueryEditor() { return dispatch( addQueryEditor({ - dbId: queryEditor.dbId || defaultDbId || firstDbId, - schema: queryEditor.schema, + dbId: dbId || defaultDbId || firstDbId, + schema, autorun: false, sql: `${warning}SELECT ...`, - queryLimit: queryEditor.queryLimit || common.conf.DEFAULT_SQLLAB_LIMIT, + queryLimit: queryLimit || common.conf.DEFAULT_SQLLAB_LIMIT, name, }), ); diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.test.js index ca1e68b5b2879..4ff56512b6319 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.test.js @@ -480,15 +480,21 @@ describe('async actions', () => { { type: actions.ADD_QUERY_EDITOR, queryEditor: { - ...defaultQueryEditor, id: 'abcd', + sql: expect.stringContaining('SELECT ...'), name: `Untitled Query ${ store.getState().sqlLab.queryEditors.length + 1 }`, + dbId: defaultQueryEditor.dbId, + schema: defaultQueryEditor.schema, + autorun: false, + queryLimit: + defaultQueryEditor.queryLimit || + initialState.common.conf.DEFAULT_SQLLAB_LIMIT, }, }, ]; - const request = actions.addNewQueryEditor(defaultQueryEditor); + const request = actions.addNewQueryEditor(); return request(store.dispatch, store.getState).then(() => { expect(store.getActions()).toEqual(expectedActions); }); From 32bc98f2dc17dd17fdd07c7a27b5aa0bffbed215 Mon Sep 17 00:00:00 2001 From: justin-park Date: Wed, 2 Nov 2022 13:10:23 -0700 Subject: [PATCH 4/6] update fallback value --- superset-frontend/src/SqlLab/actions/sqlLab.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index af828604d0e6a..924ce79eff3f3 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -619,7 +619,7 @@ export function addNewQueryEditor() { databases.length > 0 ? Math.min(...Object.values(databases).map(database => database.id)) : undefined; - const { dbId, schema, queryLimit } = { + const { dbId, schema, queryLimit, autorun } = { ...queryEditors[0], ...activeQueryEditor, ...(unsavedQueryEditor.id === activeQueryEditor?.id && @@ -636,8 +636,8 @@ export function addNewQueryEditor() { return dispatch( addQueryEditor({ dbId: dbId || defaultDbId || firstDbId, - schema, - autorun: false, + schema: schema ?? null, + autorun: autorun ?? false, sql: `${warning}SELECT ...`, queryLimit: queryLimit || common.conf.DEFAULT_SQLLAB_LIMIT, name, From 9bb6b7c84f352d3aa721a16d0875cf009ec92669 Mon Sep 17 00:00:00 2001 From: justin-park Date: Wed, 2 Nov 2022 13:51:14 -0700 Subject: [PATCH 5/6] fix the firstDbId logic --- superset-frontend/src/SqlLab/actions/sqlLab.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index 924ce79eff3f3..56d94b974828f 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -615,10 +615,8 @@ export function addNewQueryEditor() { const activeQueryEditor = queryEditors.find( qe => qe.id === tabHistory[tabHistory.length - 1], ); - const firstDbId = - databases.length > 0 - ? Math.min(...Object.values(databases).map(database => database.id)) - : undefined; + const dbIds = Object.values(databases).map(database => database.id); + const firstDbId = dbIds.length > 0 ? Math.min(...dbIds) : undefined; const { dbId, schema, queryLimit, autorun } = { ...queryEditors[0], ...activeQueryEditor, From 9d779e7b610dfc9193cee6c41faae95e0389bed5 Mon Sep 17 00:00:00 2001 From: justin-park Date: Thu, 5 Jan 2023 10:52:37 -0800 Subject: [PATCH 6/6] add spec for addNewQueryEditor behavior --- .../cypress/integration/sqllab/tabs.test.ts | 56 ++++++++++++++++++- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/integration/sqllab/tabs.test.ts b/superset-frontend/cypress-base/cypress/integration/sqllab/tabs.test.ts index 0f08593022a6d..d31ef3e8c2f3e 100644 --- a/superset-frontend/cypress-base/cypress/integration/sqllab/tabs.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/sqllab/tabs.test.ts @@ -22,9 +22,10 @@ describe('SqlLab query tabs', () => { cy.visit('/superset/sqllab'); }); + const tablistSelector = '[data-test="sql-editor-tabs"] > [role="tablist"]'; + const tabSelector = `${tablistSelector} [role="tab"]`; + it('allows you to create and close a tab', () => { - const tablistSelector = '[data-test="sql-editor-tabs"] > [role="tablist"]'; - const tabSelector = `${tablistSelector} [role="tab"]`; cy.get(tabSelector).then(tabs => { const initialTabCount = tabs.length; const initialUntitledCount = Math.max( @@ -59,4 +60,55 @@ describe('SqlLab query tabs', () => { cy.get(tabSelector).should('have.length', initialTabCount); }); }); + + it('opens a new tab by a button and a shortcut', () => { + const editorContent = '#ace-editor .ace_content'; + const editorInput = '#ace-editor textarea'; + const queryLimitSelector = '#js-sql-toolbar .limitDropdown'; + cy.get(tabSelector).then(tabs => { + const initialTabCount = tabs.length; + const initialUntitledCount = Math.max( + 0, + ...tabs + .map( + (i, tabItem) => + Number(tabItem.textContent?.match(/Untitled Query (\d+)/)?.[1]) || + 0, + ) + .toArray(), + ); + + // configure some editor settings + cy.get(editorInput).type('some random query string', { force: true }); + cy.get(queryLimitSelector).parent().click({ force: true }); + cy.get('.ant-dropdown-menu') + .last() + .find('.ant-dropdown-menu-item') + .first() + .click({ force: true }); + + // open a new tab by a button + cy.get('[data-test="add-tab-icon"]:visible:last').click({ force: true }); + cy.contains('[role="tab"]', `Untitled Query ${initialUntitledCount + 1}`); + cy.get(tabSelector).should('have.length', initialTabCount + 1); + cy.get(editorContent).contains('SELECT ...'); + cy.get(queryLimitSelector).contains('10'); + + // close the tab + cy.get(`${tabSelector}:last [data-test="dropdown-trigger"]`).click({ + force: true, + }); + cy.get(`${tablistSelector} [aria-label="remove"]:last`).click({ + force: true, + }); + cy.get(tabSelector).should('have.length', initialTabCount); + + // open a new tab by a shortcut + cy.get('body').type('{ctrl}t'); + cy.get(tabSelector).should('have.length', initialTabCount + 1); + cy.contains('[role="tab"]', `Untitled Query ${initialUntitledCount + 1}`); + cy.get(editorContent).contains('SELECT ...'); + cy.get(queryLimitSelector).contains('10'); + }); + }); });