Skip to content

Commit

Permalink
Fix query based dropdown not adding quote marks correctly (#4186)
Browse files Browse the repository at this point in the history
* Handle non-array in multi-value QueryBasedParameter

* Use state value in QueryBasedParameterInput

* Normalize array in parameter structure

* Add Multi-selection test

* Remove unnecessary not null check
  • Loading branch information
gabrieldutra authored and arikfr committed Oct 6, 2019
1 parent 648847d commit d8a0af1
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 17 deletions.
41 changes: 26 additions & 15 deletions client/app/components/QueryBasedParameterInput.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { find, isFunction, isArray, isEqual, toString, map, intersection } from 'lodash';
import { find, isArray, map, intersection, isEqual } from 'lodash';
import React from 'react';
import PropTypes from 'prop-types';
import { react2angular } from 'react2angular';
Expand Down Expand Up @@ -29,6 +29,7 @@ export class QueryBasedParameterInput extends React.Component {
super(props);
this.state = {
options: [],
value: null,
loading: false,
};
}
Expand All @@ -41,6 +42,24 @@ export class QueryBasedParameterInput extends React.Component {
if (this.props.queryId !== prevProps.queryId) {
this._loadOptions(this.props.queryId);
}
if (this.props.value !== prevProps.value) {
this.setValue(this.props.value);
}
}

setValue(value) {
const { options } = this.state;
if (this.props.mode === 'multiple') {
value = isArray(value) ? value : [value];
const optionValues = map(options, option => option.value);
const validValues = intersection(value, optionValues);
this.setState({ value: validValues });
return validValues;
}
const found = find(options, option => option.value === this.props.value) !== undefined;
value = found ? value : options[0].value;
this.setState({ value });
return value;
}

async _loadOptions(queryId) {
Expand All @@ -50,20 +69,12 @@ export class QueryBasedParameterInput extends React.Component {

// stale queryId check
if (this.props.queryId === queryId) {
this.setState({ options, loading: false });

if (this.props.mode === 'multiple' && isArray(this.props.value)) {
const optionValues = map(options, option => option.value);
const validValues = intersection(this.props.value, optionValues);
if (!isEqual(this.props.value, validValues)) {
this.props.onSelect(validValues);
}
} else {
const found = find(options, option => option.value === this.props.value) !== undefined;
if (!found && isFunction(this.props.onSelect)) {
this.props.onSelect(options[0].value);
this.setState({ options, loading: false }, () => {
const updatedValue = this.setValue(this.props.value);
if (!isEqual(updatedValue, this.props.value)) {
this.props.onSelect(updatedValue);
}
}
});
}
}
}
Expand All @@ -78,7 +89,7 @@ export class QueryBasedParameterInput extends React.Component {
disabled={loading || (options.length === 0)}
loading={loading}
mode={mode}
value={isArray(value) ? value : toString(value)}
value={this.state.value}
onChange={onSelect}
dropdownMatchSelectWidth={false}
optionFilterProp="children"
Expand Down
9 changes: 8 additions & 1 deletion client/app/services/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,12 @@ export class Parameter {
}
}

if (this.type === 'query' && !isEmptyValue(value)) {
if (this.multiValuesOptions && !isArray(value)) {
value = [value];
}
}

if (isDateRangeParameter(this.type)) {
this.value = null;
this.$$value = null;
Expand Down Expand Up @@ -387,7 +393,8 @@ export class Parameter {
if (has(query, key)) {
if (this.multiValuesOptions) {
try {
this.setValue(JSON.parse(query[key]));
const valueFromJson = JSON.parse(query[key]);
this.setValue(isArray(valueFromJson) ? valueFromJson : query[key]);
} catch (e) {
this.setValue(query[key]);
}
Expand Down
65 changes: 64 additions & 1 deletion client/cypress/integration/query/parameter_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ describe('Parameter', () => {
describe('Dropdown Parameter', () => {
beforeEach(() => {
const queryData = {
name: 'Number Parameter',
name: 'Dropdown Parameter',
query: "SELECT '{{test-parameter}}' AS parameter",
options: {
parameters: [
Expand Down Expand Up @@ -180,6 +180,69 @@ describe('Parameter', () => {
});
});

describe('Query Based Dropdown Parameter', () => {
beforeEach(() => {
const dropdownQueryData = {
name: 'Dropdown Query',
query: `SELECT 'value1' AS name, 1 AS value UNION ALL
SELECT 'value2' AS name, 2 AS value UNION ALL
SELECT 'value3' AS name, 3 AS value`,
};
createQuery(dropdownQueryData, true).then((dropdownQuery) => {
const queryData = {
name: 'Query Based Dropdown Parameter',
query: "SELECT '{{test-parameter}}' AS parameter",
options: {
parameters: [
{ name: 'test-parameter',
title: 'Test Parameter',
type: 'query',
queryId: dropdownQuery.id },
],
},
};

cy.visit(`/queries/${dropdownQuery.id}`);
cy.getByTestId('ExecuteButton').click();
cy.getByTestId('TableVisualization')
.should('contain', 'value1')
.and('contain', 'value2')
.and('contain', 'value3');

createQuery(queryData, false)
.then(({ id }) => cy.visit(`/queries/${id}/source`));
});
});

it('supports multi-selection', () => {
cy.clickThrough(`
ParameterSettings-test-parameter
AllowMultipleValuesCheckbox
QuotationSelect
DoubleQuotationMarkOption
SaveParameterSettings
`);

cy.getByTestId('ParameterName-test-parameter')
.find('.ant-select')
.click();

// make sure all options are unselected and select all
cy.get('li.ant-select-dropdown-menu-item').each(($option) => {
expect($option).not.to.have.class('ant-select-dropdown-menu-item-selected');
cy.wrap($option).click();
});

cy.getByTestId('QueryEditor').click(); // just to close the select menu

cy.getByTestId('ParameterApplyButton')
.click();

cy.getByTestId('TableVisualization')
.should('contain', '"1","2","3"');
});
});

describe('Date Parameter', () => {
const selectCalendarDate = (date) => {
cy.getByTestId('ParameterName-test-parameter')
Expand Down

0 comments on commit d8a0af1

Please sign in to comment.