Fix query based param with no results crashing page (#4707)

* Fix query based param with no results crashing page * Add message for empty dropdown parameters * Handle 500 no results case with empty result set * Cypress: Make sure it shows the message * Use .ant-select-selection to open dropdown
getredash · Mar 24, 2020 · 8db1612 · 8db1612
1 parent fabaf73
commit 8db1612
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 49 deletions.
diff --git a/client/app/components/ParameterValueInput.jsx b/client/app/components/ParameterValueInput.jsx
@@ -1,4 +1,4 @@
-import { isEqual } from "lodash";
+import { isEqual, isEmpty } from "lodash";
 import React from "react";
 import PropTypes from "prop-types";
 import Select from "antd/lib/select";
@@ -103,14 +103,13 @@ class ParameterValueInput extends React.Component {
         className={this.props.className}
         mode={parameter.multiValuesOptions ? "multiple" : "default"}
         optionFilterProp="children"
-        disabled={enumOptionsArray.length === 0}
         value={normalize(value)}
         onChange={this.onSelect}
         dropdownMatchSelectWidth={false}
         showSearch
         showArrow
         style={{ minWidth: 60 }}
-        notFoundContent={null}
+        notFoundContent={isEmpty(enumOptionsArray) ? "No options available" : null}
         {...multipleValuesProps}>
         {enumOptionsArray.map(option => (
           <Option key={option} value={option}>

diff --git a/client/app/components/QueryBasedParameterInput.jsx b/client/app/components/QueryBasedParameterInput.jsx
@@ -1,4 +1,4 @@
-import { find, isArray, map, intersection, isEqual } from "lodash";
+import { find, isArray, get, first, map, intersection, isEqual, isEmpty } from "lodash";
 import React from "react";
 import PropTypes from "prop-types";
 import Select from "antd/lib/select";
@@ -56,7 +56,7 @@ export default class QueryBasedParameterInput extends React.Component {
       return validValues;
     }
     const found = find(options, option => option.value === this.props.value) !== undefined;
-    value = found ? value : options[0].value;
+    value = found ? value : get(first(options), "value");
     this.setState({ value });
     return value;
   }
@@ -85,7 +85,7 @@ export default class QueryBasedParameterInput extends React.Component {
       <span>
         <Select
           className={className}
-          disabled={loading || options.length === 0}
+          disabled={loading}
           loading={loading}
           mode={mode}
           value={this.state.value}
@@ -94,7 +94,7 @@ export default class QueryBasedParameterInput extends React.Component {
           optionFilterProp="children"
           showSearch
           showArrow
-          notFoundContent={null}
+          notFoundContent={isEmpty(options) ? "No options available" : null}
           {...otherProps}>
           {options.map(option => (
             <Option value={option.value} key={option.value}>

diff --git a/client/app/services/parameters/QueryBasedDropdownParameter.js b/client/app/services/parameters/QueryBasedDropdownParameter.js
@@ -67,10 +67,12 @@ class QueryBasedDropdownParameter extends Parameter {
 
   loadDropdownValues() {
     if (this.parentQueryId) {
-      return Query.associatedDropdown({ queryId: this.parentQueryId, dropdownQueryId: this.queryId });
+      return Query.associatedDropdown({ queryId: this.parentQueryId, dropdownQueryId: this.queryId }).catch(() =>
+        Promise.resolve([])
+      );
     }
 
-    return Query.asDropdown({ id: this.queryId });
+    return Query.asDropdown({ id: this.queryId }).catch(Promise.resolve([]));
   }
 }
 

diff --git a/client/cypress/integration/query/parameter_spec.js b/client/cypress/integration/query/parameter_spec.js
@@ -161,57 +161,91 @@ 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 }],
-          },
+    context("based on a query with no results", () => {
+      beforeEach(() => {
+        const dropdownQueryData = {
+          name: "Dropdown Query",
+          query: "",
         };
+        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 },
+              ],
+            },
+          };
+
+          createQuery(queryData, false).then(({ id }) => cy.visit(`/queries/${id}/source`));
+        });
+      });
 
-        cy.visit(`/queries/${dropdownQuery.id}`);
-        cy.getByTestId("ExecuteButton").click();
-        cy.getByTestId("TableVisualization")
-          .should("contain", "value1")
-          .and("contain", "value2")
-          .and("contain", "value3");
+      it("should show a 'No options available' message when you click", () => {
+        cy.getByTestId("ParameterName-test-parameter")
+          .find(".ant-select-selection")
+          .click();
 
-        createQuery(queryData, false).then(({ id }) => cy.visit(`/queries/${id}/source`));
+        cy.contains("li.ant-select-dropdown-menu-item", "No options available");
       });
     });
 
-    it("supports multi-selection", () => {
-      cy.clickThrough(`
-        ParameterSettings-test-parameter
-        AllowMultipleValuesCheckbox
-        QuotationSelect
-        DoubleQuotationMarkOption
-        SaveParameterSettings
-      `);
+    context("based on a query with 3 results", () => {
+      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`));
+        });
+      });
 
-      cy.getByTestId("ParameterName-test-parameter")
-        .find(".ant-select")
-        .click();
+      it("supports multi-selection", () => {
+        cy.clickThrough(`
+          ParameterSettings-test-parameter
+          AllowMultipleValuesCheckbox
+          QuotationSelect
+          DoubleQuotationMarkOption
+          SaveParameterSettings
+        `);
 
-      // 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("ParameterName-test-parameter")
+          .find(".ant-select")
+          .click();
 
-      cy.getByTestId("QueryEditor").click(); // just to close the select menu
+        // 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("ParameterApplyButton").click();
+        cy.getByTestId("QueryEditor").click(); // just to close the select menu
 
-      cy.getByTestId("TableVisualization").should("contain", '"1","2","3"');
+        cy.getByTestId("ParameterApplyButton").click();
+
+        cy.getByTestId("TableVisualization").should("contain", '"1","2","3"');
+      });
     });
   });