From 90809f6d39010aa889ae6bbc5b1ec2fe4afdc903 Mon Sep 17 00:00:00 2001 From: Chris Williams Date: Wed, 24 Oct 2018 19:41:32 -0700 Subject: [PATCH] [sqllab] more robust copy to clipboard (#6180) * [sqllab] implement more robust share query button * [sqllab] fix Hotkeys react warnings * [CopyToClipboard] don't bind in render functions * [explorer] fix short url button position * [sqllab] remove share query item in tab dropdown menu * [sqllab] reference new ShareSqlLabQuery component * [sqllab][share query robustness] clean up * [sqllab] fix tests * [sqllab][share query] add unit tests --- .../sqllab/CopyQueryTabUrl_spec.jsx | 15 --- .../sqllab/ShareSqlLabQuery_spec.jsx | 91 +++++++++++++++++++ .../src/SqlLab/components/CopyQueryTabUrl.jsx | 60 ------------ .../src/SqlLab/components/ShareQuery.jsx | 22 ----- .../SqlLab/components/ShareSqlLabQuery.jsx | 80 ++++++++++++++++ .../src/SqlLab/components/SqlEditor.jsx | 4 +- .../SqlLab/components/TabbedSqlEditors.jsx | 4 +- .../assets/src/components/CopyToClipboard.jsx | 13 +-- superset/assets/src/components/Hotkeys.jsx | 4 +- .../src/components/URLShortLinkButton.jsx | 1 + 10 files changed, 181 insertions(+), 113 deletions(-) delete mode 100644 superset/assets/spec/javascripts/sqllab/CopyQueryTabUrl_spec.jsx create mode 100644 superset/assets/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx delete mode 100644 superset/assets/src/SqlLab/components/CopyQueryTabUrl.jsx delete mode 100644 superset/assets/src/SqlLab/components/ShareQuery.jsx create mode 100644 superset/assets/src/SqlLab/components/ShareSqlLabQuery.jsx diff --git a/superset/assets/spec/javascripts/sqllab/CopyQueryTabUrl_spec.jsx b/superset/assets/spec/javascripts/sqllab/CopyQueryTabUrl_spec.jsx deleted file mode 100644 index dd2301617d..0000000000 --- a/superset/assets/spec/javascripts/sqllab/CopyQueryTabUrl_spec.jsx +++ /dev/null @@ -1,15 +0,0 @@ -import React from 'react'; -import { initialState } from './fixtures'; - -import CopyQueryTabUrl from '../../../src/SqlLab/components/CopyQueryTabUrl'; - -describe('CopyQueryTabUrl', () => { - const mockedProps = { - queryEditor: initialState.sqlLab.queryEditors[0], - }; - it('is valid with props', () => { - expect( - React.isValidElement(), - ).toBe(true); - }); -}); diff --git a/superset/assets/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx b/superset/assets/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx new file mode 100644 index 0000000000..4746a0b880 --- /dev/null +++ b/superset/assets/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx @@ -0,0 +1,91 @@ +import React from 'react'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { OverlayTrigger } from 'react-bootstrap'; +import fetchMock from 'fetch-mock'; +import { shallow } from 'enzyme'; + +import * as utils from '../../../src/utils/common'; +import Button from '../../../src/components/Button'; +import ShareSqlLabQuery from '../../../src/SqlLab/components/ShareSqlLabQuery'; + +const mockStore = configureStore([thunk]); +const store = mockStore(); + +describe('ShareSqlLabQuery', () => { + const storeQueryUrl = 'glob:*/kv/store/'; + const storeQueryMockId = '123'; + + beforeEach(() => { + fetchMock.post(storeQueryUrl, () => ({ id: storeQueryMockId }), { + overwriteRoutes: true, + }); + fetchMock.resetHistory(); + }); + + afterAll(fetchMock.reset); + + const defaultProps = { + queryEditor: { + dbId: 0, + title: 'query title', + schema: 'query_schema', + autorun: false, + sql: 'SELECT * FROM ...', + }, + }; + + function setup(overrideProps) { + const wrapper = shallow(, { + context: { store }, + }).dive(); // wrapped in withToasts HOC + + return wrapper; + } + + it('renders an OverlayTrigger with Button', () => { + const wrapper = setup(); + const trigger = wrapper.find(OverlayTrigger); + const button = trigger.find(Button); + + expect(trigger).toHaveLength(1); + expect(button).toHaveLength(1); + }); + + it('calls storeQuery() with the query when getCopyUrl() is called and saves the url', () => { + expect.assertions(4); + const storeQuerySpy = jest.spyOn(utils, 'storeQuery'); + + const wrapper = setup(); + const instance = wrapper.instance(); + + return instance.getCopyUrl().then(() => { + expect(storeQuerySpy.mock.calls).toHaveLength(1); + expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1); + expect(storeQuerySpy.mock.calls[0][0]).toMatchObject(defaultProps.queryEditor); + expect(instance.state.shortUrl).toContain(storeQueryMockId); + + return Promise.resolve(); + }); + }); + + it('dispatches an error toast upon fetching failure', () => { + expect.assertions(3); + const error = 'error'; + const addDangerToastSpy = jest.fn(); + fetchMock.post(storeQueryUrl, { throws: error }, { overwriteRoutes: true }); + const wrapper = setup(); + wrapper.setProps({ addDangerToast: addDangerToastSpy }); + + return wrapper + .instance() + .getCopyUrl() + .then(() => { + expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1); + expect(addDangerToastSpy.mock.calls).toHaveLength(1); + expect(addDangerToastSpy.mock.calls[0][0]).toBe(error); + + return Promise.resolve(); + }); + }); +}); diff --git a/superset/assets/src/SqlLab/components/CopyQueryTabUrl.jsx b/superset/assets/src/SqlLab/components/CopyQueryTabUrl.jsx deleted file mode 100644 index 704226898f..0000000000 --- a/superset/assets/src/SqlLab/components/CopyQueryTabUrl.jsx +++ /dev/null @@ -1,60 +0,0 @@ -/* eslint no-alert: 0 */ -import React from 'react'; -import PropTypes from 'prop-types'; -import CopyToClipboard from '../../components/CopyToClipboard'; -import { storeQuery } from '../../utils/common'; -import { t } from '../../locales'; - -const propTypes = { - queryEditor: PropTypes.object.isRequired, -}; - -export default class CopyQueryTabUrl extends React.PureComponent { - constructor(props) { - super(props); - this.getUrl = this.getUrl.bind(this); - } - - getUrl(callback) { - const qe = this.props.queryEditor; - const sharedQuery = { - dbId: qe.dbId, - title: qe.title, - schema: qe.schema, - autorun: qe.autorun, - sql: qe.sql, - }; - - // the fetch call to get a url is async, but execCommand('copy') must be sync - // get around this with 2 timeouts. calling a timeout from within a timeout is not considered - // a short-lived, user-initiated sync event - let url; - storeQuery(sharedQuery).then((shareUrl) => { url = shareUrl; }); - const longTimeout = setTimeout(() => { if (url) callback(url); }, 750); - setTimeout(() => { - if (url) { - callback(url); - clearTimeout(longTimeout); - } - }, 150); - - } - - render() { - return ( - - {t('share query')} - - } - tooltipText={t('copy URL to clipboard')} - shouldShowText={false} - getText={this.getUrl} - /> - ); - } -} - -CopyQueryTabUrl.propTypes = propTypes; diff --git a/superset/assets/src/SqlLab/components/ShareQuery.jsx b/superset/assets/src/SqlLab/components/ShareQuery.jsx deleted file mode 100644 index 48e8330fca..0000000000 --- a/superset/assets/src/SqlLab/components/ShareQuery.jsx +++ /dev/null @@ -1,22 +0,0 @@ -import React from 'react'; - -import CopyToClipboard from '../../components/CopyToClipboard'; -import CopyQueryTabUrl from './CopyQueryTabUrl'; -import Button from '../../components/Button'; -import { t } from '../../locales'; - -export default class ShareQuery extends CopyQueryTabUrl { - render() { - return ( - - {t('Share Query')} - - )} - tooltipText={t('copy URL to clipboard')} - shouldShowText={false} - getText={this.getUrl} - />); - } -} diff --git a/superset/assets/src/SqlLab/components/ShareSqlLabQuery.jsx b/superset/assets/src/SqlLab/components/ShareSqlLabQuery.jsx new file mode 100644 index 0000000000..55f8c8820d --- /dev/null +++ b/superset/assets/src/SqlLab/components/ShareSqlLabQuery.jsx @@ -0,0 +1,80 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { Popover, OverlayTrigger } from 'react-bootstrap'; + +import Button from '../../components/Button'; +import CopyToClipboard from '../../components/CopyToClipboard'; +import { storeQuery } from '../../utils/common'; +import { getClientErrorObject } from '../../modules/utils'; +import { t } from '../../locales'; +import withToasts from '../../messageToasts/enhancers/withToasts'; + +const propTypes = { + queryEditor: PropTypes.shape({ + dbId: PropTypes.number, + title: PropTypes.string, + schema: PropTypes.string, + autorun: PropTypes.bool, + sql: PropTypes.string, + }).isRequired, + addDangerToast: PropTypes.func.isRequired, +}; + +class ShareSqlLabQuery extends React.Component { + constructor(props) { + super(props); + this.state = { + shortUrl: 'Loading ...', + showOverlay: false, + }; + this.getCopyUrl = this.getCopyUrl.bind(this); + } + + getCopyUrl() { + const { dbId, title, schema, autorun, sql } = this.props.queryEditor; + const sharedQuery = { dbId, title, schema, autorun, sql }; + + return storeQuery(sharedQuery) + .then((shortUrl) => { + this.setState({ shortUrl }); + }) + .catch((response) => { + getClientErrorObject(response).then(({ error }) => { + this.props.addDangerToast(error); + this.setState({ shortUrl: t('Error') }); + }); + }); + } + + renderPopover() { + return ( + + } + /> + + ); + } + + render() { + return ( + + + + ); + } +} + +ShareSqlLabQuery.propTypes = propTypes; + +export default withToasts(ShareSqlLabQuery); diff --git a/superset/assets/src/SqlLab/components/SqlEditor.jsx b/superset/assets/src/SqlLab/components/SqlEditor.jsx index 3dd9d48584..7f82e02a6f 100644 --- a/superset/assets/src/SqlLab/components/SqlEditor.jsx +++ b/superset/assets/src/SqlLab/components/SqlEditor.jsx @@ -19,7 +19,7 @@ import Button from '../../components/Button'; import TemplateParamsEditor from './TemplateParamsEditor'; import SouthPane from './SouthPane'; import SaveQuery from './SaveQuery'; -import ShareQuery from './ShareQuery'; +import ShareSqlLabQuery from './ShareSqlLabQuery'; import Timer from '../../components/Timer'; import Hotkeys from '../../components/Hotkeys'; import SqlEditorLeftBar from './SqlEditorLeftBar'; @@ -235,7 +235,7 @@ class SqlEditor extends React.PureComponent { /> - + {ctasControls} diff --git a/superset/assets/src/SqlLab/components/TabbedSqlEditors.jsx b/superset/assets/src/SqlLab/components/TabbedSqlEditors.jsx index e693295aab..9bf3978592 100644 --- a/superset/assets/src/SqlLab/components/TabbedSqlEditors.jsx +++ b/superset/assets/src/SqlLab/components/TabbedSqlEditors.jsx @@ -7,7 +7,6 @@ import URI from 'urijs'; import * as Actions from '../actions'; import SqlEditor from './SqlEditor'; -import CopyQueryTabUrl from './CopyQueryTabUrl'; import { areArraysShallowEqual } from '../../reduxUtils'; import { t } from '../../locales'; import TabStatusIcon from './TabStatusIcon'; @@ -189,8 +188,7 @@ class TabbedSqlEditors extends React.PureComponent { {t('Rename tab')} - {qe && } - +
diff --git a/superset/assets/src/components/CopyToClipboard.jsx b/superset/assets/src/components/CopyToClipboard.jsx index 9514b18f1e..c2b9c573e4 100644 --- a/superset/assets/src/components/CopyToClipboard.jsx +++ b/superset/assets/src/components/CopyToClipboard.jsx @@ -32,6 +32,7 @@ export default class CopyToClipboard extends React.Component { this.copyToClipboard = this.copyToClipboard.bind(this); this.resetTooltipText = this.resetTooltipText.bind(this); this.onMouseOut = this.onMouseOut.bind(this); + this.onClick = this.onClick.bind(this); } onMouseOut() { @@ -105,7 +106,7 @@ export default class CopyToClipboard extends React.Component { overlay={this.renderTooltip()} trigger={['hover']} bsStyle="link" - onClick={this.onClick.bind(this)} + onClick={this.onClick} onMouseOut={this.onMouseOut} > {this.props.copyNode} @@ -119,7 +120,7 @@ export default class CopyToClipboard extends React.Component { {this.props.copyNode} @@ -138,13 +139,7 @@ export default class CopyToClipboard extends React.Component { } render() { - let html; - if (this.props.inMenu) { - html = this.renderInMenu(); - } else { - html = this.renderLink(); - } - return html; + return this.props.inMenu ? this.renderInMenu() : this.renderLink(); } } diff --git a/superset/assets/src/components/Hotkeys.jsx b/superset/assets/src/components/Hotkeys.jsx index fbecee8d8c..97d8c95d51 100644 --- a/superset/assets/src/components/Hotkeys.jsx +++ b/superset/assets/src/components/Hotkeys.jsx @@ -26,7 +26,7 @@ export default class Hotkeys extends React.PureComponent { const { header, hotkeys } = this.props; return ( - + @@ -36,7 +36,7 @@ export default class Hotkeys extends React.PureComponent { {hotkeys.map(({ key, descr }) => ( - + diff --git a/superset/assets/src/components/URLShortLinkButton.jsx b/superset/assets/src/components/URLShortLinkButton.jsx index b0570c0d10..47fd3329dc 100644 --- a/superset/assets/src/components/URLShortLinkButton.jsx +++ b/superset/assets/src/components/URLShortLinkButton.jsx @@ -54,6 +54,7 @@ class URLShortLinkButton extends React.Component {
{key} {descr}