Repository: getredash/redash
Pull request: Fix query based dropdown not adding quote marks correctly
## What type of PR is this? (check all applicable)
<!-- Please leave only what's applicable -->

- [x] Bug Fix

## Description
This is not perfect yet, I still wanna explore a few more options without having the Dropdown Options in the Parameter structure context (that's the only difference from the regular Dropdown code - what handles if the value is valid is the Input).

## Related Tickets & Documents
Fixes #4184

## Mobile & Desktop Screenshots/Recordings (if there are UI changes)
--

Problem statement:
Query with a drop down list based on query (Allow multiple values with quotation mark) not adding quotation mark correctly
### Issue Summary

Query with a drop down list based on query (Allow multiple values with quotation mark) not adding quotation mark correctly.

### Steps to Reproduce

1. Create a query with a parameter with query based dropdown list.
2. Select allow multiple values, double quotation mark.
3. Execute query, when the parameter select only 1 choice and the parameter is text, it didn't add double quotation mark and the query returns an error.

### Technical details:

* Redash Version: 8.0 Beta 2
* Browser/OS: CentOS 7
* How did you install Redash: Docker

Hints from issue discussion:
@gabrieldutra is this expected or a bug?
@ranbena it should have the quote mark in any case when you set them.

@myonlylonely I tried it in our [netlify preview](https://redash-preview.netlify.com/queries/123/source?p_test=%5B%22MrCirca%22%5D#219) and it seems to be adding the quotes as expected:

<img src="https://user-images.githubusercontent.com/3356951/65598030-e7374100-df70-11e9-8c64-1da35be2b5c5.png" width="50%" />

But I did notice a different issue that may be what's going on here: what happens if you remove the value the querie has, run the query, add it back and run the query again?
I created a new query "Search Album New Sample" on netlify preview.
![image](https://user-images.githubusercontent.com/8188177/65599780-25644d00-dfd1-11e9-992f-669e8793984f.png)

But when I choose 2 items in the dropdown list, It returns the results.
![image](https://user-images.githubusercontent.com/8188177/65599825-4a58c000-dfd1-11e9-8be5-12add83df6ba.png)

When I delete 2 items to 1 item, It returns the results correctly.
![image](https://user-images.githubusercontent.com/8188177/65599860-652b3480-dfd1-11e9-9e66-7fb3f64b8aa0.png)

For the first time I created a new query with a drop down list based on query (Allow multiple values with quotation mark), it always not adding the quotation mark, and it returns an error.
Thanks @myonlylonely, that's exactly what I was thinking before :+1:.

It seems it's having trouble to handle empty values (for Multiple values it should allow you to leave it empty). I'll take a deeper look into it today and submit a fix PR, but a workaround for you to have this working is to save the query right after you have it working as it should.

Commit message: Normalize array in parameter structure

Patch to review:
diff --git a/client/app/components/QueryBasedParameterInput.jsx b/client/app/components/QueryBasedParameterInput.jsx
index d55f5aa2b6..6b69559c31 100644
--- a/client/app/components/QueryBasedParameterInput.jsx
+++ b/client/app/components/QueryBasedParameterInput.jsx
@@ -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';
@@ -29,6 +29,7 @@ export class QueryBasedParameterInput extends React.Component {
     super(props);
     this.state = {
       options: [],
+      value: null,
       loading: false,
     };
   }
@@ -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) {
@@ -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);
           }
-        }
+        });
       }
     }
   }
@@ -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"
diff --git a/client/app/services/query.js b/client/app/services/query.js
index 9b14581f6f..f6ca0c34f5 100644
--- a/client/app/services/query.js
+++ b/client/app/services/query.js
@@ -253,6 +253,12 @@ export class Parameter {
       }
     }
 
+    if (this.type === 'query' && !isEmptyValue(value)) {
+      if (this.multiValuesOptions && !isArray(value) && value !== null) {
+        value = [value];
+      }
+    }
+
     if (isDateRangeParameter(this.type)) {
       this.value = null;
       this.$$value = null;
@@ -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]);
           }


Code review comments:
diff_hunk: @@ -253,6 +253,12 @@ export class Parameter {
       }
     }
 
+    if (this.type === 'query' && !isEmptyValue(value)) {
+      if (this.multiValuesOptions && !isArray(value) && value !== null) {
path: client/app/services/query.js
text: @user1:
Don't think you need to check `value !== null` cause it's checked the line before.

@author:
Yeah, I was a bit paranoid on this one :sweat_smile: