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

Support multi-select in parameters #3952

Merged
merged 32 commits into from
Aug 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
113fa1a
Allow multiple values for enum parameter
gabrieldutra Jul 4, 2019
8c627f0
Allow multi-select for Query dropdown parameters
gabrieldutra Jul 5, 2019
25fed53
CR + make sure list values are allowed
gabrieldutra Jul 7, 2019
a98cb5a
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 9, 2019
293a83c
Add prefix, suffix and separator
gabrieldutra Jul 9, 2019
de31c67
Rename multipleValues and cast options as strings
gabrieldutra Jul 10, 2019
e4270cd
Replicate serialization logic on frontend
gabrieldutra Jul 12, 2019
a703ec0
Add Quote Option Select
gabrieldutra Jul 12, 2019
138ebab
Make sure it's enum or query before join
gabrieldutra Jul 12, 2019
286e9ce
Add a couple of tests
gabrieldutra Jul 12, 2019
403dd7d
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 15, 2019
a8a6413
Add help to quote option
gabrieldutra Jul 15, 2019
f5fc796
Add min-width and normalize empty array
gabrieldutra Jul 15, 2019
c25db79
Improve behavior when changing parameter settings
gabrieldutra Jul 15, 2019
c0d5297
Validate enum values on setValue + CodeClimate
gabrieldutra Jul 16, 2019
00ede72
Ran wording suggestions
gabrieldutra Jul 16, 2019
6234f86
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 17, 2019
0be0693
Updates after Apply Changes
gabrieldutra Jul 17, 2019
3b3d410
Fix failing Cypress tests
gabrieldutra Jul 17, 2019
a71bde9
Make sure enumOptions exists before split
gabrieldutra Jul 17, 2019
6b89b4c
Improve propTypes for QueyBasedParameterInput
gabrieldutra Jul 18, 2019
58c25aa
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 18, 2019
6ae403a
CR
gabrieldutra Jul 18, 2019
e2f3756
Cypress: Test for multi-select Enum
gabrieldutra Jul 18, 2019
96319d2
Merge branch 'multi-select-parameters' of github.com:getredash/redash…
gabrieldutra Jul 18, 2019
5487731
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 22, 2019
6cf7e5c
Fix multi-selection Cypress spec
gabrieldutra Jul 22, 2019
26e8274
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 26, 2019
8d127e4
Update Refresh Schedule
gabrieldutra Jul 29, 2019
2b85c79
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 29, 2019
1d209e2
Merge branch 'master' into multi-select-parameters
gabrieldutra Jul 30, 2019
52523c0
Merge branch 'master' into multi-select-parameters
gabrieldutra Aug 1, 2019
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
50 changes: 50 additions & 0 deletions client/app/components/EditParameterSettingsDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { includes, words, capitalize, clone, isNull } from 'lodash';
import React, { useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import Checkbox from 'antd/lib/checkbox';
import Modal from 'antd/lib/modal';
import Form from 'antd/lib/form';
import Button from 'antd/lib/button';
Expand All @@ -23,6 +24,13 @@ function isTypeDateRange(type) {
return /-range/.test(type);
}

function joinExampleList(multiValuesOptions) {
const { prefix, suffix } = multiValuesOptions;
return ['value1', 'value2', 'value3']
.map(value => `${prefix}${value}${suffix}`)
.join(',');
}

function NameInput({ name, type, onChange, existingNames, setValidation }) {
let helpText = '';
let validateStatus = '';
Expand Down Expand Up @@ -185,6 +193,48 @@ function EditParameterSettingsDialog(props) {
/>
</Form.Item>
)}
{(param.type === 'enum' || param.type === 'query') && (
<Form.Item className="m-b-0" label=" " colon={false} {...formItemProps}>
<Checkbox
defaultChecked={!!param.multiValuesOptions}
onChange={e => setParam({ ...param,
multiValuesOptions: e.target.checked ? {
prefix: '',
suffix: '',
separator: ',',
} : null })}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are already set as the defaults in query.js:Parameters, perhaps no need for it here?

const separator = get(multiValuesOptions, 'separator', ',');
const prefix = get(multiValuesOptions, 'prefix', '');
const suffix = get(multiValuesOptions, 'suffix', '');

Copy link
Member Author

@gabrieldutra gabrieldutra Jul 18, 2019

Choose a reason for hiding this comment

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

In EditParameterSettingsDialog it's used to manage the Quotation Select value. I was actually a bit "paranoid" with this, there's one in the backend as well 😆. In any case: the query.js one is only triggered for "text query" cases, where the backend is not aware of these settings and serialization logic is made by the frontend (yep, the same logic is duplicated to workaround this scenario).

Should probably add a comment in there to mention that. Edit: comment added

data-test="AllowMultipleValuesCheckbox"
>
Allow multiple values
</Checkbox>
</Form.Item>
)}
{(param.type === 'enum' || param.type === 'query') && param.multiValuesOptions && (
<Form.Item
label="Quotation"
help={(
<React.Fragment>
Placed in query as: <code>{joinExampleList(param.multiValuesOptions)}</code>
</React.Fragment>
)}
{...formItemProps}
>
<Select
value={param.multiValuesOptions.prefix}
onChange={quoteOption => setParam({ ...param,
multiValuesOptions: {
...param.multiValuesOptions,
prefix: quoteOption,
suffix: quoteOption,
} })}
data-test="QuotationSelect"
>
<Option value="">None (default)</Option>
<Option value="'">Single Quotation Mark</Option>
<Option value={'"'} data-test="DoubleQuotationMarkOption">Double Quotation Mark</Option>
</Select>
</Form.Item>
)}
</Form>
</Modal>
);
Expand Down
26 changes: 21 additions & 5 deletions client/app/components/ParameterValueInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,20 @@ import './ParameterValueInput.less';

const { Option } = Select;

const multipleValuesProps = {
maxTagCount: 3,
maxTagTextLength: 10,
maxTagPlaceholder: num => `+${num.length} more`,
};

export class ParameterValueInput extends React.Component {
static propTypes = {
type: PropTypes.string,
value: PropTypes.any, // eslint-disable-line react/forbid-prop-types
enumOptions: PropTypes.string,
queryId: PropTypes.number,
parameter: PropTypes.any, // eslint-disable-line react/forbid-prop-types
allowMultipleValues: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

@gabrieldutra do you also feel this file (and perhaps query.js:Parameters) would be better off with some inheritance pattern (e.g. hoc, extends, ..)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like everything related to Parameters should be on an inheritance pattern. This included. This is all on the "Parameters Design" thought. Once both this and the Dynamic Values are merged I have a refactoring in mind.

onSelect: PropTypes.func,
className: PropTypes.string,
};
Expand All @@ -30,6 +37,7 @@ export class ParameterValueInput extends React.Component {
enumOptions: '',
queryId: null,
parameter: null,
allowMultipleValues: false,
onSelect: () => {},
className: '',
};
Expand Down Expand Up @@ -88,36 +96,43 @@ export class ParameterValueInput extends React.Component {
}

renderEnumInput() {
const { value, enumOptions } = this.props;
const { enumOptions, allowMultipleValues } = this.props;
const { value } = this.state;
const enumOptionsArray = enumOptions.split('\n').filter(v => v !== '');
return (
<Select
className={this.props.className}
mode={allowMultipleValues ? 'multiple' : 'default'}
optionFilterProp="children"
disabled={enumOptionsArray.length === 0}
defaultValue={value}
value={value}
Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise the changes from setValue wouldn't affect here.

Other inputs remain the same as is, one need is to see the behavior after #3907.

onChange={this.onSelect}
dropdownMatchSelectWidth={false}
dropdownClassName="ant-dropdown-in-bootstrap-modal"
showSearch
style={{ minWidth: 60 }}
optionFilterProp="children"
style={{ minWidth: allowMultipleValues ? 195 : 60 }}
notFoundContent={null}
{...multipleValuesProps}
>
{enumOptionsArray.map(option => (<Option key={option} value={option}>{ option }</Option>))}
</Select>
);
}

renderQueryBasedInput() {
const { queryId, parameter } = this.props;
const { queryId, parameter, allowMultipleValues } = this.props;
const { value } = this.state;
return (
<QueryBasedParameterInput
className={this.props.className}
mode={allowMultipleValues ? 'multiple' : 'default'}
optionFilterProp="children"
parameter={parameter}
value={value}
queryId={queryId}
onSelect={this.onSelect}
style={{ minWidth: allowMultipleValues ? 195 : 60 }}
{...multipleValuesProps}
/>
);
}
Expand Down Expand Up @@ -187,6 +202,7 @@ export default function init(ngModule) {
parameter="$ctrl.param"
enum-options="$ctrl.param.enumOptions"
query-id="$ctrl.param.queryId"
allow-multiple-values="!!$ctrl.param.multiValuesOptions"
on-select="$ctrl.setValue"
></parameter-value-input-impl>
`,
Expand Down
27 changes: 19 additions & 8 deletions client/app/components/QueryBasedParameterInput.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { find, isFunction, toString } from 'lodash';
import { find, isFunction, isArray, isEqual, toString, map, intersection } from 'lodash';
import React from 'react';
import PropTypes from 'prop-types';
import { react2angular } from 'react2angular';
Expand All @@ -10,13 +10,15 @@ export class QueryBasedParameterInput extends React.Component {
static propTypes = {
parameter: PropTypes.any, // eslint-disable-line react/forbid-prop-types
value: PropTypes.any, // eslint-disable-line react/forbid-prop-types
mode: PropTypes.oneOf(['default', 'multiple']),
queryId: PropTypes.number,
onSelect: PropTypes.func,
className: PropTypes.string,
};

static defaultProps = {
value: null,
mode: 'default',
parameter: null,
queryId: null,
onSelect: () => {},
Expand Down Expand Up @@ -50,31 +52,40 @@ export class QueryBasedParameterInput extends React.Component {
if (this.props.queryId === queryId) {
this.setState({ options, loading: false });

const found = find(options, option => option.value === this.props.value) !== undefined;
if (!found && isFunction(this.props.onSelect)) {
this.props.onSelect(options[0].value);
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);
}
}
}
}
}

render() {
const { className, value, onSelect } = this.props;
const { className, value, mode, onSelect, ...otherProps } = this.props;
const { loading, options } = this.state;
return (
<span>
<Select
className={className}
disabled={loading || (options.length === 0)}
loading={loading}
value={toString(value)}
mode={mode}
value={isArray(value) ? value : toString(value)}
onChange={onSelect}
dropdownMatchSelectWidth={false}
dropdownClassName="ant-dropdown-in-bootstrap-modal"
showSearch
style={{ minWidth: 60 }}
optionFilterProp="children"
showSearch
notFoundContent={null}
{...otherProps}
>
{options.map(option => (<Option value={option.value} key={option.value}>{option.name}</Option>))}
</Select>
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/parameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function ParametersDirective($location, KeyboardShortcuts) {
EditParameterSettingsDialog
.showModal({ parameter })
.result.then((updated) => {
scope.parameters[index] = extend(parameter, updated);
scope.parameters[index] = extend(parameter, updated).setValue(updated.value);
Copy link
Member Author

@gabrieldutra gabrieldutra Jul 16, 2019

Choose a reason for hiding this comment

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

This makes Parameter settings edits safer. setValue seems to be the centered place to process parameter values, so when a parameter changes it either adapts the value or sets it to empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool cool cool

scope.onUpdated();
});
};
Expand Down
61 changes: 47 additions & 14 deletions client/app/services/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ import moment from 'moment';
import debug from 'debug';
import Mustache from 'mustache';
import {
zipObject, isEmpty, map, filter, includes, union, uniq, has,
isNull, isUndefined, isArray, isObject, identity, extend, each,
startsWith, some,
zipObject, isEmpty, map, filter, includes, union, uniq, has, get, intersection,
isNull, isUndefined, isArray, isObject, identity, extend, each, join, some, startsWith,
} from 'lodash';

Mustache.escape = identity; // do not html-escape values
Expand Down Expand Up @@ -138,6 +137,7 @@ export class Parameter {
this.useCurrentDateTime = parameter.useCurrentDateTime;
this.global = parameter.global; // backward compatibility in Widget service
this.enumOptions = parameter.enumOptions;
this.multiValuesOptions = parameter.multiValuesOptions;
this.queryId = parameter.queryId;
this.parentQueryId = parentQueryId;

Expand All @@ -164,6 +164,10 @@ export class Parameter {
return isNull(this.getValue());
}

getValue(extra = {}) {
return this.constructor.getValue(this, extra);
}

get hasDynamicValue() {
if (isDateParameter(this.type)) {
return isDynamicDate(this.value);
Expand All @@ -184,13 +188,9 @@ export class Parameter {
return false;
}

getValue() {
return this.constructor.getValue(this);
}

static getValue(param) {
const { value, type, useCurrentDateTime } = param;
const isEmptyValue = isNull(value) || isUndefined(value) || (value === '');
static getValue(param, extra = {}) {
const { value, type, useCurrentDateTime, multiValuesOptions } = param;
const isEmptyValue = isNull(value) || isUndefined(value) || (value === '') || (isArray(value) && value.length === 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is now exactly how _.isEmpty() works 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Almost there 😂, _.isEmpty considers Numbers as empty. It may be interesting to have the "Empty Parameter" rule explicit, though.

if (isDateRangeParameter(type) && param.hasDynamicValue) {
const { dynamicValue } = param;
if (dynamicValue) {
Expand Down Expand Up @@ -224,10 +224,32 @@ export class Parameter {
if (type === 'number') {
return normalizeNumericValue(value, null); // normalize empty value
}

// join array in frontend when query is executed as a text
const { joinListValues } = extra;
if (includes(['enum', 'query'], type) && multiValuesOptions && isArray(value) && joinListValues) {
const separator = get(multiValuesOptions, 'separator', ',');
const prefix = get(multiValuesOptions, 'prefix', '');
const suffix = get(multiValuesOptions, 'suffix', '');
const parameterValues = map(value, v => `${prefix}${v}${suffix}`);
return join(parameterValues, separator);
}
return value;
}

setValue(value) {
if (this.type === 'enum') {
const enumOptionsArray = this.enumOptions && this.enumOptions.split('\n') || [];
if (this.multiValuesOptions) {
if (!isArray(value)) {
value = [value];
}
value = intersection(value, enumOptionsArray);
} else if (!value || isArray(value) || !includes(enumOptionsArray, value)) {
value = enumOptionsArray[0];
}
}

if (isDateRangeParameter(this.type)) {
this.value = null;
this.$$value = null;
Expand Down Expand Up @@ -331,6 +353,9 @@ export class Parameter {
[`${prefix}${this.name}`]: null,
};
}
if (this.multiValuesOptions && isArray(this.value)) {
return { [`${prefix}${this.name}`]: JSON.stringify(this.value) };
}
return {
[`${prefix}${this.name}`]: this.value,
[`${prefix}${this.name}.start`]: null,
Expand All @@ -352,7 +377,15 @@ export class Parameter {
} else {
const key = `${prefix}${this.name}`;
if (has(query, key)) {
this.setValue(query[key]);
if (this.multiValuesOptions) {
try {
this.setValue(JSON.parse(query[key]));
} catch (e) {
this.setValue(query[key]);
}
} else {
this.setValue(query[key]);
}
}
}
}
Expand Down Expand Up @@ -460,9 +493,9 @@ class Parameters {
return !isEmpty(this.get());
}

getValues() {
getValues(extra = {}) {
const params = this.get();
return zipObject(map(params, i => i.name), map(params, i => i.getValue()));
return zipObject(map(params, i => i.name), map(params, i => i.getValue(extra)));
}

hasPendingValues() {
Expand Down Expand Up @@ -710,7 +743,7 @@ function QueryResource(
return new QueryResultError("Can't execute empty query.");
}

const parameters = this.getParameters().getValues();
const parameters = this.getParameters().getValues({ joinListValues: true });
const execute = () => QueryResult.get(this.data_source_id, queryText, parameters, maxAge, this.id);
return this.prepareQueryResultExecution(execute, maxAge);
};
Expand Down
Loading