Support multi-select in parameters #3952

gabrieldutra · 2019-07-04T14:40:59Z

What type of PR is this? (check all applicable)

Feature

Description

This one is to add an option to allow multiple values in Dropdown parameters (see preview below :)).

Make 'quote each value' an option (and let the user customize the separator?)
Apply this to Query based Dropdown parameters
Styling updates (select maxWidth, etc)
Add tests
Refresh Schedule

Related Tickets & Documents

Closes #3053

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Dropdown

Query Based Dropdown

gabrieldutra · 2019-07-04T15:02:14Z

There are currently two places that invoke mustache_render, one is the ParametizedQuery, and the other is this task:

redash/redash/tasks/queries.py

Lines 192 to 197 in 45a3b72

    
           if query.options and len(query.options.get('parameters', [])) > 0: 
        
               query_params = {p['name']: p.get('value') 
        
                               for p in query.options['parameters']} 
        
               query_text = mustache_render(query.query_text, query_params) 
        
           else: 
        
               query_text = query.query_text

The List logic is in the ParametizedQuery, so I was thinking about turn the above to use it instead of a direct call to mustache_render. This way the ParametizedQuery would centralize logic related to Queries with parameters 😁.

As I'm not that familiar with the backend, where would you put this resolve_parameter_values logic @arikfr or @rauchy, is the Parametized Query model a good place?

ranbena · 2019-07-05T06:35:39Z

Since this component's dimensions could be as high and wide as the content it holds, we should limit it so layout doesn't break. Here are some suggestions:

CSS limits

{
  max-width: 350px;
  max-height: 69px;
  overflow: auto;
}

or Ant options (I think it's better):

<Select
  maxTagCount="3"
  maxTagTextLength="10"
  maxTagPlaceholder={num => `+${num.length} more`}
/>

arikfr · 2019-07-05T10:32:57Z

This way the ParametizedQuery would centralize logic related to Queries with parameters 😁.

Sounds like a good idea 👍 But probably worth waiting to hear what @rauchy thinks.

or Ant options (I think it's better):

Sounds like the better option. Not sure we should change the default, btw. And we should reuse this for multi-filters as well (but in a separate PR).

redash/models/parameterized_query.py

client/app/components/ParameterValueInput.jsx

gabrieldutra

Thanks for the comments!

@rauchy, I'll finish with the backend part + add some tests, ping you back when done :)

client/app/components/ParameterValueInput.jsx

redash/models/parameterized_query.py

gabrieldutra · 2019-07-10T20:06:52Z

Taking another step and considering Serialization Settings I wanted to make it custom (separator, prefix and suffix to each value options at least backend-wise for future support to any UI). While exploring options I realized there is another case that will need to be handled:

When running adhoc queries (or running a modified unsaved existing one) there is no information about parameters (only their values), thus no information about the Serialization options. As it's a "trusted" route for queries I think I'll just replicate serialization logic in the frontend and send the value as string for such cases.

An alternative is to always serialize this on frontend and have a "reverse operation" to validate the values in backend based on settings.

Edit:
The reverse operation is actually more interesting if I can reliably recreate the array, would be sth like:

regex = "(?:{}|^)(?:{})(.*?)(?:{})(?={}|$)".format(re.escape(separator), re.escape(prefix),
                                                   re.escape(suffix), re.escape(separator))
if not re.match('^(' + regex +')*$', value):
     return False
values = re.findall(regex, value)
return set(map(unicode, values)).issubset(set(dropdown_options))

@arikfr WDYT? (I couldn't think of a simpler way to do that without regex)

arikfr · 2019-07-11T07:09:14Z

Taking another step and considering Serialization Settings I wanted to make it custom (separator, prefix and suffix to each value options at least backend-wise for future support to any UI)

In what cases one would want different prefix and suffix?

I'm not sure if at this stage we should let the user control the separator anyway, but we can have a setting for it just in case without visible UI.

Also, if the user picks quotes they need to pick the escaping form. Most SQL implementation escape a single quote by double it (select '''' returns '). But in other cases you might want to prefix with a slash.

An alternative is to always serialize this on frontend and have a "reverse operation" to validate the values in backend based on settings.

The regex seems to work, but this is tricky and can lead to various issues down the road.

Maybe instead we can just skip validation in such case (after confirming the user has access to the data source), like we do with dropdowns in general?

gabrieldutra · 2019-07-11T13:35:49Z

In what cases one would want different prefix and suffix?

I have no idea, but could be a use case at some point, my plan was to have it only on backend for now 😅. The same for the separator.

For UI I was thinking about showing quote options in a Select.

The regex seems to work, but this is tricky and can lead to various issues down the road.

That was sort of my concern too, a Regex that takes strings on it doesn't seem to be something that reliable.

Maybe instead we can just skip validation in such case (after confirming the user has access to the data source), like we do with dropdowns in general?

For such cases that you only have the parameter values the validation is already skipped the problem is that you'd only have the list of values, not information on how to serialize data.

As the validation is already skipped I think the best option will be to replicate the serialization logic on frontend and just send it as String.

- Set parameter value again to pass through checks - Add setValue check for multi values

gabrieldutra · 2019-07-16T01:11:58Z

@arikfr, @ranbena this is what I was thinking for the Parameter Settings Dialog, you can give your comments 🙂

Also, if the user picks quotes they need to pick the escaping form.

Why is that 🤔? Isn't the purpose of the quotes to allow scenarios like this one:

?

ranbena · 2019-07-16T09:28:58Z

Very nice @gabrieldutra. I have a few wording suggestions.

None (default)
Single quotation mark
Double quotation mark

The hint at the bottom is essential to understanding the feature and its result.
A small change to the sentence:

gabrieldutra

Notes

I wanted to let options available on Backend (prefix, suffix and separator), even though not showed in UI just to handle possible future cases;
I tried to handle parameter issues when changing their settings, the result is a more complex Parameter Structure (query.js file), which will be even more complex after Add Dynamic Values to Date and Date Range Parameters #3904. A refactor for that is on my plans once both are merged;
I want to add Cypress tests here too, but it's probably better to wait Parameter “Apply Changes” button #3907 to avoid rework.

Caveats

Needed to replicate the logic for the List Join on frontend for cases where the Parameter schema is not present (joinListValue = true). In that case value is sent as String as in backend validation is already skipped (it's a "trusted" route).

gabrieldutra · 2019-07-16T13:59:02Z

client/app/components/parameters.js

@@ -38,7 +38,7 @@ function ParametersDirective($location) {
        EditParameterSettingsDialog
          .showModal({ parameter })
          .result.then((updated) => {
-            scope.parameters[index] = extend(parameter, updated);
+            scope.parameters[index] = extend(parameter, updated).setValue(updated.value);


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.

Cool cool cool

gabrieldutra · 2019-07-16T14:02:25Z

client/app/components/ParameterValueInput.jsx

        disabled={enumOptionsArray.length === 0}
-        defaultValue={value}
+        value={value}


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.

gabrieldutra · 2019-07-16T14:26:23Z

redash/models/parameterized_query.py

@@ -105,7 +126,7 @@ def apply(self, parameters):
            raise InvalidParameterError(invalid_parameter_names)
        else:
            self.parameters.update(parameters)
-            self.query = mustache_render(self.template, self.parameters)
+            self.query = mustache_render(self.template, join_parameter_list_values(parameters, self.schema))


Needed to provide the schema to get the prefix, suffix and separator options.

arikfr · 2019-07-17T16:41:56Z

And no easy solution, for each data source is a different thing. This is probably together with "make parameters safe".

The easy solution in this case is just to let the user select from a few options. For some it might be a trial and error process to find the right setting, but it's a reasonable thing.

gabrieldutra · 2019-07-17T23:34:48Z

client/cypress/integration/query/parameter_spec.js

@@ -129,14 +129,14 @@ describe('Parameter', () => {
        .find('.ant-select')
        .click();

-      cy.contains('li.ant-select-dropdown-menu-item', 'value1')
+      cy.contains('li.ant-select-dropdown-menu-item', 'value2')


Needed to change to value2 since now I make sure the first dropdown option is selected when value is invalid or empty.

ranbena

Great job. Works perfect.
Few code questions and suggestions.

ranbena · 2019-07-18T10:38:26Z

client/app/components/EditParameterSettingsDialog.jsx

+                  prefix: '',
+                  suffix: '',
+                  separator: ',',
+                } : null })}


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

redash/client/app/services/query.js

Lines 107 to 109 in a71bde9

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

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

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

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

ranbena · 2019-07-18T10:48:00Z

client/app/components/ParameterValueInput.jsx

@@ -22,6 +23,7 @@ export class ParameterValueInput extends React.Component {
    enumOptions: PropTypes.string,
    queryId: PropTypes.number,
    parameter: PropTypes.any, // eslint-disable-line react/forbid-prop-types
+    allowMultipleValues: PropTypes.bool,


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

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.

ranbena · 2019-07-18T10:52:48Z

client/app/components/ParameterValueInput.less

@@ -80,3 +80,7 @@
    }
  }
 }
+
+.parameter-multi-select {
+  min-width: 195px;


Can you elaborate on the reasoning for this?

It gets too short when it has no selected values. I let it with the same value as Text Parameters, LMK if you have other suggestions.

(Also should add a comment here in the code 😅) Edit: comment added

client/app/components/ParameterValueInput.jsx

client/app/components/QueryBasedParameterInput.jsx

ranbena · 2019-07-18T11:09:47Z

client/app/components/QueryBasedParameterInput.jsx

+        if (this.props.mode === 'multiple' && isArray(this.props.value)) {
+          const optionValues = map(options, option => option.value);
+          const validValues = intersection(this.props.value, optionValues);
+          if (validValues.length !== this.props.value.length) {


Not sure what this means, can you explain or add a comment?

In this case the if is equivalent to !isEqual, I'll just change to it as it's easier to understand 😅. Overall it checks if all values are valid (if there's any that's not on the options), in case the validValues list is not the same as the value provided, it forces selection of only the valid ones.

ranbena · 2019-07-18T11:13:27Z

client/app/components/parameters.js

@@ -38,7 +38,7 @@ function ParametersDirective($location) {
        EditParameterSettingsDialog
          .showModal({ parameter })
          .result.then((updated) => {
-            scope.parameters[index] = extend(parameter, updated);
+            scope.parameters[index] = extend(parameter, updated).setValue(updated.value);


Cool cool cool

ranbena · 2019-07-18T11:16:05Z

client/app/services/query.js

-    const isEmptyValue = isNull(value) || isUndefined(value) || (value === '');
+  static getValue(param, joinListValue = false) {
+    const { value, type, useCurrentDateTime, multiValuesOptions } = param;
+    const isEmptyValue = isNull(value) || isUndefined(value) || (value === '') || (isArray(value) && value.length === 0);


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

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

client/app/services/query.js

ranbena · 2019-07-18T11:29:36Z

client/cypress/integration/query/parameter_spec.js

@@ -145,7 +145,7 @@ describe('Parameter', () => {
          .find('.ant-select')
          .click();

-        cy.contains('li.ant-select-dropdown-menu-item', 'value1')
+        cy.contains('li.ant-select-dropdown-menu-item', 'value2')
          .click();
      });
    });


No new tests?

I was waiting for the Apply All 😝, I will add Cypress tests today.

Co-Authored-By: Ran Byron <[email protected]>

… into multi-select-parameters

gabrieldutra · 2019-07-22T15:38:31Z

Important note I forgot 😬: I didn't code the "Refresh Schedule" part, because I couldn't get Parameterized Queries to be scheduled (Python debugger + logs to track), I suspect this is a bug, but I'm not 100% aware of how it's supposed to work ^^ (if you schedule a parameterized query on preview it won't update the results).

shenoyroopesh · 2019-07-28T18:11:12Z

How could this work for selecting multiple dates as well?

gabrieldutra · 2019-07-29T15:35:51Z

How could this work for selecting multiple dates as well?

Hi @shenoyroopesh 🙂, this will work for the existing Dropdown and Query Based Dropdown parameters. Dates won't be supported directly, but Query Based Dropdown parameters make any type of data possible with this workaround (with actual dates and not intervals). Unfortunately, it would still not be that convenient.

arikfr · 2019-07-29T15:39:47Z

@shenoyroopesh can you share where you might want to select multiple dates but a date range won't be applicable? (just trying to understand the use case)

gabrieldutra · 2019-07-29T23:53:54Z

redash/tasks/queries.py

@@ -193,7 +193,7 @@ def refresh_queries():
                if query.options and len(query.options.get('parameters', [])) > 0:
                    query_params = {p['name']: p.get('value')
                                    for p in query.options['parameters']}
-                    query_text = mustache_render(query.query_text, query_params)
+                    query_text = query.parameterized.apply(query_params).query


Refresh schedule updated. Tested that by changing the outdated_queries method locally so all queries were outdated 🙂.

gabrieldutra · 2019-08-01T10:01:28Z

@arikfr this one is ready to me, but it's worth to give a check on the backend part :)

arikfr · 2019-08-04T12:47:44Z

🎉

* Allow multiple values for enum parameter * Allow multi-select for Query dropdown parameters * CR + make sure list values are allowed * Add prefix, suffix and separator * Rename multipleValues and cast options as strings * Replicate serialization logic on frontend * Add Quote Option Select * Make sure it's enum or query before join * Add a couple of tests * Add help to quote option * Add min-width and normalize empty array * Improve behavior when changing parameter settings - Set parameter value again to pass through checks - Add setValue check for multi values * Validate enum values on setValue + CodeClimate * Ran wording suggestions * Updates after Apply Changes * Fix failing Cypress tests * Make sure enumOptions exists before split * Improve propTypes for QueyBasedParameterInput Co-Authored-By: Ran Byron <[email protected]> * CR * Cypress: Test for multi-select Enum * Fix multi-selection Cypress spec * Update Refresh Schedule

Allow multiple values for enum parameter

113fa1a

gabrieldutra self-assigned this Jul 4, 2019

Allow multi-select for Query dropdown parameters

8c627f0

rauchy requested changes Jul 7, 2019

View reviewed changes

redash/models/parameterized_query.py Outdated Show resolved Hide resolved

redash/models/parameterized_query.py Show resolved Hide resolved

redash/models/parameterized_query.py Outdated Show resolved Hide resolved

client/app/components/ParameterValueInput.jsx Show resolved Hide resolved

CR + make sure list values are allowed

25fed53

gabrieldutra commented Jul 7, 2019

View reviewed changes

client/app/components/ParameterValueInput.jsx Show resolved Hide resolved

redash/models/parameterized_query.py Outdated Show resolved Hide resolved

redash/models/parameterized_query.py Show resolved Hide resolved

gabrieldutra added 2 commits July 9, 2019 14:55

Merge branch 'master' into multi-select-parameters

a98cb5a

Add prefix, suffix and separator

293a83c

gabrieldutra force-pushed the multi-select-parameters branch from 80be362 to 293a83c Compare July 9, 2019 21:54

Rename multipleValues and cast options as strings

de31c67

gabrieldutra added 9 commits July 12, 2019 14:29

Replicate serialization logic on frontend

e4270cd

Add Quote Option Select

a703ec0

Make sure it's enum or query before join

138ebab

Add a couple of tests

286e9ce

Merge branch 'master' into multi-select-parameters

403dd7d

Add help to quote option

a8a6413

Add min-width and normalize empty array

f5fc796

Improve behavior when changing parameter settings

c25db79

- Set parameter value again to pass through checks - Add setValue check for multi values

Validate enum values on setValue + CodeClimate

c0d5297

Ran wording suggestions

00ede72

gabrieldutra commented Jul 16, 2019

View reviewed changes

gabrieldutra changed the title ~~WIP: Support multi-select in parameters~~ Support multi-select in parameters Jul 16, 2019

Fix failing Cypress tests

3b3d410

gabrieldutra commented Jul 17, 2019

View reviewed changes

Make sure enumOptions exists before split

a71bde9

ranbena suggested changes Jul 18, 2019

View reviewed changes

Improve propTypes for QueyBasedParameterInput

6b89b4c

Co-Authored-By: Ran Byron <[email protected]>

arikfr mentioned this pull request Jul 18, 2019

Provide settings to control multifilter initial selection #3905

Closed

4 tasks

gabrieldutra added 4 commits July 18, 2019 18:26

Merge branch 'master' into multi-select-parameters

58c25aa

CR

6ae403a

Cypress: Test for multi-select Enum

e2f3756

Merge branch 'multi-select-parameters' of github.com:getredash/redash…

96319d2

… into multi-select-parameters

gabrieldutra mentioned this pull request Jul 22, 2019

Add Dynamic Values to Date and Date Range Parameters #3904

Merged

1 task

Merge branch 'master' into multi-select-parameters

5487731

gabrieldutra added 2 commits July 22, 2019 13:21

Fix multi-selection Cypress spec

6cf7e5c

Merge branch 'master' into multi-select-parameters

26e8274

gabrieldutra added 2 commits July 29, 2019 19:20

Update Refresh Schedule

8d127e4

Merge branch 'master' into multi-select-parameters

2b85c79

gabrieldutra commented Jul 29, 2019

View reviewed changes

Merge branch 'master' into multi-select-parameters

1d209e2

ranbena approved these changes Aug 1, 2019

View reviewed changes

Merge branch 'master' into multi-select-parameters

52523c0

arikfr merged commit f0576a3 into master Aug 4, 2019

arikfr deleted the multi-select-parameters branch August 4, 2019 12:47

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

Support multi-select in parameters #3952

Support multi-select in parameters #3952

Conversation

gabrieldutra commented Jul 4, 2019 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

gabrieldutra commented Jul 4, 2019

ranbena commented Jul 5, 2019

arikfr commented Jul 5, 2019

gabrieldutra left a comment

Choose a reason for hiding this comment

gabrieldutra commented Jul 10, 2019 • edited Loading

arikfr commented Jul 11, 2019

gabrieldutra commented Jul 11, 2019

gabrieldutra commented Jul 16, 2019

ranbena commented Jul 16, 2019

gabrieldutra left a comment

Choose a reason for hiding this comment

gabrieldutra Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr commented Jul 17, 2019

Choose a reason for hiding this comment

ranbena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Jul 22, 2019 • edited Loading

shenoyroopesh commented Jul 28, 2019 • edited Loading

gabrieldutra commented Jul 29, 2019

arikfr commented Jul 29, 2019

Choose a reason for hiding this comment

gabrieldutra commented Aug 1, 2019

arikfr commented Aug 4, 2019

gabrieldutra commented Jul 4, 2019 •

edited

Loading

gabrieldutra commented Jul 10, 2019 •

edited

Loading

gabrieldutra Jul 16, 2019 •

edited

Loading

gabrieldutra Jul 18, 2019 •

edited

Loading

gabrieldutra Jul 18, 2019 •

edited

Loading

gabrieldutra Jul 18, 2019 •

edited

Loading

gabrieldutra commented Jul 22, 2019 •

edited

Loading

shenoyroopesh commented Jul 28, 2019 •

edited

Loading