Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add separate limit setting for SqlLab #4941

Merged
merged 3 commits into from
Nov 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions superset/assets/spec/javascripts/sqllab/LimitControl_spec.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import React from 'react';
import { shallow } from 'enzyme';

import { Label } from 'react-bootstrap';
import LimitControl from '../../../src/SqlLab/components/LimitControl';
import ControlHeader from '../../../src/explore/components/ControlHeader';

describe('LimitControl', () => {
const defaultProps = {
value: 0,
defaultQueryLimit: 1000,
maxRow: 100000,
onChange: () => {},
};
let wrapper;
const factory = o => <LimitControl {...o} />;
beforeEach(() => {
wrapper = shallow(factory(defaultProps));
});
it('is a valid element', () => {
expect(React.isValidElement(<LimitControl {...defaultProps} />)).toEqual(true);
});
it('renders a Label', () => {
expect(wrapper.find(Label)).toHaveLength(1);
});
it('loads the correct state', () => {
const value = 100;
wrapper = shallow(factory({ ...defaultProps, value }));
expect(wrapper.state().textValue).toEqual(value.toString());
wrapper.find(Label).first().simulate('click');
expect(wrapper.state().showOverlay).toBe(true);
expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(0);
});
it('handles invalid value', () => {
wrapper.find(Label).first().simulate('click');
wrapper.setState({ textValue: 'invalid' });
expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(1);
});
it('handles negative value', () => {
wrapper.find(Label).first().simulate('click');
wrapper.setState({ textValue: '-1' });
expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(1);
});
it('handles value above max row', () => {
wrapper.find(Label).first().simulate('click');
wrapper.setState({ textValue: (defaultProps.maxRow + 1).toString() });
expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(1);
});
it('opens and closes', () => {
wrapper.find(Label).first().simulate('click');
expect(wrapper.state().showOverlay).toBe(true);
wrapper.find('.ok').first().simulate('click');
expect(wrapper.state().showOverlay).toBe(false);
});
it('resets and closes', () => {
const value = 100;
wrapper = shallow(factory({ ...defaultProps, value }));
wrapper.find(Label).first().simulate('click');
expect(wrapper.state().textValue).toEqual(value.toString());
wrapper.find('.reset').simulate('click');
expect(wrapper.state().textValue).toEqual(defaultProps.defaultQueryLimit.toString());
});
});
19 changes: 18 additions & 1 deletion superset/assets/spec/javascripts/sqllab/SqlEditor_spec.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React from 'react';
import { shallow } from 'enzyme';

import { initialState, queries, table } from './fixtures';
import { defaultQueryEditor, initialState, queries, table } from './fixtures';
import LimitControl from '../../../src/SqlLab/components/LimitControl';
import SqlEditor from '../../../src/SqlLab/components/SqlEditor';
import SqlEditorLeftBar from '../../../src/SqlLab/components/SqlEditorLeftBar';

Expand All @@ -16,6 +17,8 @@ describe('SqlEditor', () => {
getHeight: () => ('100px'),
editorQueries: [],
dataPreviewQueries: [],
defaultQueryLimit: 1000,
maxRow: 100000,
};
it('is valid', () => {
expect(
Expand All @@ -26,4 +29,18 @@ describe('SqlEditor', () => {
const wrapper = shallow(<SqlEditor {...mockedProps} />);
expect(wrapper.find(SqlEditorLeftBar)).toHaveLength(1);
});
it('render a LimitControl with default limit', () => {
const defaultQueryLimit = 101;
const updatedProps = { ...mockedProps, defaultQueryLimit };
const wrapper = shallow(<SqlEditor {...updatedProps} />);
expect(wrapper.find(LimitControl)).toHaveLength(1);
expect(wrapper.find(LimitControl).props().value).toEqual(defaultQueryLimit);
});
it('render a LimitControl with existing limit', () => {
const queryEditor = { ...defaultQueryEditor, queryLimit: 101 };
const updatedProps = { ...mockedProps, queryEditor };
const wrapper = shallow(<SqlEditor {...updatedProps} />);
expect(wrapper.find(LimitControl)).toHaveLength(1);
expect(wrapper.find(LimitControl).props().value).toEqual(queryEditor.queryLimit);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ describe('TabbedSqlEditors', () => {
editorHeight: '',
getHeight: () => ('100px'),
database: {},
defaultQueryLimit: 1000,
maxRow: 100000,
};
const getWrapper = () => (
shallow(<TabbedSqlEditors {...mockedProps} />, {
Expand Down
6 changes: 6 additions & 0 deletions superset/assets/spec/javascripts/sqllab/fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,12 @@ export const initialState = {
activeSouthPaneTab: 'Results',
},
messageToasts: [],
common: {
conf: {
DEFAULT_SQLLAB_LIMIT: 1000,
SQL_MAX_ROW: 100000,
},
},
};

export const query = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ describe('sqlLabReducer', () => {
newState = sqlLabReducer(newState, actions.queryEditorSetSql(qe, sql));
expect(newState.queryEditors[1].sql).toBe(sql);
});
it('should not fail while setting queryLimit', () => {
const queryLimit = 101;
newState = sqlLabReducer(newState, actions.queryEditorSetQueryLimit(qe, queryLimit));
expect(newState.queryEditors[1].queryLimit).toEqual(queryLimit);
});
it('should set selectedText', () => {
const selectedText = 'TEST';
expect(newState.queryEditors[0].selectedText).toBeNull();
Expand Down
7 changes: 7 additions & 0 deletions superset/assets/src/SqlLab/actions/sqlLab.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const QUERY_EDITOR_SET_SCHEMA = 'QUERY_EDITOR_SET_SCHEMA';
export const QUERY_EDITOR_SET_TITLE = 'QUERY_EDITOR_SET_TITLE';
export const QUERY_EDITOR_SET_AUTORUN = 'QUERY_EDITOR_SET_AUTORUN';
export const QUERY_EDITOR_SET_SQL = 'QUERY_EDITOR_SET_SQL';
export const QUERY_EDITOR_SET_QUERY_LIMIT = 'QUERY_EDITOR_SET_QUERY_LIMIT';
export const QUERY_EDITOR_SET_TEMPLATE_PARAMS = 'QUERY_EDITOR_SET_TEMPLATE_PARAMS';
export const QUERY_EDITOR_SET_SELECTED_TEXT = 'QUERY_EDITOR_SET_SELECTED_TEXT';
export const QUERY_EDITOR_PERSIST_HEIGHT = 'QUERY_EDITOR_PERSIST_HEIGHT';
Expand Down Expand Up @@ -141,6 +142,7 @@ export function runQuery(query) {
tmp_table_name: query.tempTableName,
select_as_cta: query.ctas,
templateParams: query.templateParams,
queryLimit: query.queryLimit,
};

return SupersetClient.post({
Expand Down Expand Up @@ -230,6 +232,10 @@ export function queryEditorSetSql(queryEditor, sql) {
return { type: QUERY_EDITOR_SET_SQL, queryEditor, sql };
}

export function queryEditorSetQueryLimit(queryEditor, queryLimit) {
return { type: QUERY_EDITOR_SET_QUERY_LIMIT, queryEditor, queryLimit };
}

export function queryEditorSetTemplateParams(queryEditor, templateParams) {
return { type: QUERY_EDITOR_SET_TEMPLATE_PARAMS, queryEditor, templateParams };
}
Expand Down Expand Up @@ -325,6 +331,7 @@ export function reFetchQueryResults(query) {
tab: '',
runAsync: false,
ctas: false,
queryLimit: query.queryLimit,
};
dispatch(runQuery(newQuery));
dispatch(changeDataPreviewId(query.id, newQuery));
Expand Down
126 changes: 126 additions & 0 deletions superset/assets/src/SqlLab/components/LimitControl.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import React from 'react';
import PropTypes from 'prop-types';
import {
Button,
Label,
FormGroup,
FormControl,
Overlay,
Popover,
} from 'react-bootstrap';
import { t } from '@superset-ui/translation';

import ControlHeader from '../../explore/components/ControlHeader';

const propTypes = {
value: PropTypes.number,
defaultQueryLimit: PropTypes.number.isRequired,
maxRow: PropTypes.number.isRequired,
onChange: PropTypes.func.isRequired,
};

export default class LimitControl extends React.PureComponent {
constructor(props) {
super(props);
const { value, defaultQueryLimit } = props;
this.state = {
textValue: value.toString() || defaultQueryLimit.toString(),
showOverlay: false,
};
this.handleHide = this.handleHide.bind(this);
this.handleToggle = this.handleToggle.bind(this);
this.submitAndClose = this.submitAndClose.bind(this);
}

setValueAndClose(val) {
this.setState({ textValue: val }, this.submitAndClose);
}

submitAndClose() {
const value = parseInt(this.state.textValue, 10) || this.props.defaultQueryLimit;
this.props.onChange(value);
this.setState({ showOverlay: false });
}

isValidLimit(limit) {
const value = parseInt(limit, 10);
return !(Number.isNaN(value) || value <= 0 || (this.props.maxRow && value > this.props.maxRow));
}

handleToggle() {
this.setState({ showOverlay: !this.state.showOverlay });
}

handleHide() {
this.setState({ showOverlay: false });
}

renderPopover() {
const textValue = this.state.textValue;
const isValid = this.isValidLimit(textValue);
const errorMsg = 'Row limit must be positive integer' +
(this.props.maxRow ? ` and not greater than ${this.props.maxRow}` : '');
return (
<Popover id="sqllab-limit-results">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the id necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is primarily for getting rid of the warning. Do we want to remove it?

<div style={{ width: '100px' }}>
<ControlHeader
label={t('Row limit')}
validationErrors={!isValid ? [t(errorMsg)] : []}
/>
<FormGroup>
<FormControl
type="text"
value={textValue}
placeholder={t(`Max: ${this.props.maxRow}`)}
bsSize="small"
onChange={e => this.setState({ textValue: e.target.value })}
/>
</FormGroup>
<div className="clearfix">
<Button
bsSize="small"
bsStyle="primary"
className="float-left ok"
disabled={!isValid}
onClick={this.submitAndClose}
>
Ok
</Button>
<Button
bsSize="small"
className="float-right reset"
onClick={this.setValueAndClose.bind(this, this.props.defaultQueryLimit.toString())}
>
Reset
</Button>
</div>
</div>
</Popover>
);
}

render() {
return (
<div>
<Label
style={{ cursor: 'pointer' }}
onClick={this.handleToggle}
>
LIMIT {this.props.value || this.props.maxRow}
</Label>
<Overlay
rootClose
show={this.state.showOverlay}
onHide={this.handleHide}
trigger="click"
placement="right"
target={this}
>
{this.renderPopover()}
</Overlay>
</div>
);
}
}

LimitControl.propTypes = propTypes;
20 changes: 19 additions & 1 deletion superset/assets/src/SqlLab/components/SqlEditor.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import SplitPane from 'react-split-pane';
import { t } from '@superset-ui/translation';

import Button from '../../components/Button';
import LimitControl from './LimitControl';
import TemplateParamsEditor from './TemplateParamsEditor';
import SouthPane from './SouthPane';
import SaveQuery from './SaveQuery';
Expand All @@ -38,6 +39,8 @@ const propTypes = {
dataPreviewQueries: PropTypes.array.isRequired,
queryEditor: PropTypes.object.isRequired,
hideLeftBar: PropTypes.bool,
defaultQueryLimit: PropTypes.number.isRequired,
maxRow: PropTypes.number.isRequired,
};

const defaultProps = {
Expand Down Expand Up @@ -130,6 +133,9 @@ class SqlEditor extends React.PureComponent {
setQueryEditorSql(sql) {
this.props.actions.queryEditorSetSql(this.props.queryEditor, sql);
}
setQueryLimit(queryLimit) {
this.props.actions.queryEditorSetQueryLimit(this.props.queryEditor, queryLimit);
}
runQuery() {
this.startQuery(!(this.props.database || {}).allow_run_sync);
}
Expand All @@ -143,6 +149,7 @@ class SqlEditor extends React.PureComponent {
schema: qe.schema,
tempTableName: ctas ? this.state.ctas : '',
templateParams: qe.templateParams,
queryLimit: qe.queryLimit,
runAsync,
ctas,
};
Expand Down Expand Up @@ -236,7 +243,18 @@ class SqlEditor extends React.PureComponent {
<span className="m-r-5">
<ShareSqlLabQuery queryEditor={qe} />
</span>
{ctasControls}
<span className="m-r-5">
{ctasControls}
</span>
<span className="inlineBlock m-r-5">
<LimitControl
value={(this.props.queryEditor.queryLimit !== undefined) ?
this.props.queryEditor.queryLimit : this.props.defaultQueryLimit}
defaultQueryLimit={this.props.defaultQueryLimit}
maxRow={this.props.maxRow}
onChange={this.setQueryLimit.bind(this)}
/>
</span>
<span className="m-l-5">
<Hotkeys
header="Keyboard shortcuts"
Expand Down
8 changes: 7 additions & 1 deletion superset/assets/src/SqlLab/components/TabbedSqlEditors.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import TabStatusIcon from './TabStatusIcon';
const propTypes = {
actions: PropTypes.object.isRequired,
defaultDbId: PropTypes.number,
defaultQueryLimit: PropTypes.number.isRequired,
maxRow: PropTypes.number.isRequired,
databases: PropTypes.object.isRequired,
queries: PropTypes.object.isRequired,
queryEditors: PropTypes.array,
Expand Down Expand Up @@ -212,6 +214,8 @@ class TabbedSqlEditors extends React.PureComponent {
database={database}
actions={this.props.actions}
hideLeftBar={this.state.hideLeftBar}
defaultQueryLimit={this.props.defaultQueryLimit}
maxRow={this.props.maxRow}
/>
)}
</div>
Expand Down Expand Up @@ -243,7 +247,7 @@ class TabbedSqlEditors extends React.PureComponent {
TabbedSqlEditors.propTypes = propTypes;
TabbedSqlEditors.defaultProps = defaultProps;

function mapStateToProps({ sqlLab }) {
function mapStateToProps({ sqlLab, common }) {
return {
databases: sqlLab.databases,
queryEditors: sqlLab.queryEditors,
Expand All @@ -252,6 +256,8 @@ function mapStateToProps({ sqlLab }) {
tables: sqlLab.tables,
defaultDbId: sqlLab.defaultDbId,
offline: sqlLab.offline,
defaultQueryLimit: common.conf.DEFAULT_SQLLAB_LIMIT,
maxRow: common.conf.SQL_MAX_ROW,
};
}
function mapDispatchToProps(dispatch) {
Expand Down
1 change: 1 addition & 0 deletions superset/assets/src/SqlLab/reducers/getInitialState.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export default function getInitialState({ defaultDbId, ...restBootstrapData }) {
latestQueryId: null,
autorun: false,
dbId: defaultDbId,
queryLimit: restBootstrapData.common.conf.DEFAULT_SQLLAB_LIMIT,
};

return {
Expand Down
Loading