[explore] fix query text modal while loading (#2596)

* [explore] fix and clean Currently it's not possible to view queries while they are running, the spinner appears endlessly. I decided to rearrange things a bit while debugging so I could see clearly through it. * Adding NotImplemented methods to base classes * Fixing * Piling up
apache · Apr 13, 2017 · db02b33 · db02b33
1 parent 2df6baa
commit db02b33
Show file tree

Hide file tree

Showing 9 changed files with 133 additions and 61 deletions.
diff --git a/superset/assets/javascripts/components/ModalTrigger.jsx b/superset/assets/javascripts/components/ModalTrigger.jsx
@@ -4,6 +4,7 @@ import cx from 'classnames';
 import Button from './Button';
 
 const propTypes = {
+  animation: PropTypes.bool,
   triggerNode: PropTypes.node.isRequired,
   modalTitle: PropTypes.node.isRequired,
   modalBody: PropTypes.node,  // not required because it can be generated by beforeOpen
@@ -17,6 +18,7 @@ const propTypes = {
 };
 
 const defaultProps = {
+  animation: true,
   beforeOpen: () => {},
   onExit: () => {},
   isButton: false,
@@ -46,6 +48,7 @@ export default class ModalTrigger extends React.Component {
   renderModal() {
     return (
       <Modal
+        animation={this.props.animation}
         show={this.state.showModal}
         onHide={this.close}
         onExit={this.props.onExit}
@@ -73,7 +76,11 @@ export default class ModalTrigger extends React.Component {
     });
     if (this.props.isButton) {
       return (
-        <Button tooltip={this.props.tooltip} onClick={this.open}>
+        <Button
+          className="modal-trigger"
+          tooltip={this.props.tooltip}
+          onClick={this.open}
+        >
           {this.props.triggerNode}
           {this.renderModal()}
         </Button>

diff --git a/superset/assets/javascripts/explorev2/components/ChartContainer.jsx b/superset/assets/javascripts/explorev2/components/ChartContainer.jsx
@@ -228,7 +228,6 @@ class ChartContainer extends React.PureComponent {
       return this.renderChart();
     }
     const queryResponse = this.props.queryResponse;
-    const query = queryResponse && queryResponse.query ? queryResponse.query : null;
     return (
       <div className="chart-container">
         <Panel
@@ -266,14 +265,14 @@ class ChartContainer extends React.PureComponent {
                 {this.props.chartStatus === 'success' &&
                  this.props.queryResponse &&
                  this.props.queryResponse.is_cached &&
-                 <TooltipWrapper
-                   tooltip="Loaded from cache. Click to force refresh"
-                   label="cache-desc"
-                 >
-                   <Label
-                     style={{ fontSize: '10px', marginRight: '5px', cursor: 'pointer' }}
-                     onClick={this.runQuery.bind(this)}
-                   >
+                  <TooltipWrapper
+                    tooltip="Loaded from cache. Click to force refresh"
+                    label="cache-desc"
+                  >
+                    <Label
+                      style={{ fontSize: '10px', marginRight: '5px', cursor: 'pointer' }}
+                      onClick={this.runQuery.bind(this)}
+                    >
                       cached
                     </Label>
                  </TooltipWrapper>
@@ -288,7 +287,8 @@ class ChartContainer extends React.PureComponent {
                 <ExploreActionButtons
                   slice={this.state.mockSlice}
                   canDownload={this.props.can_download}
-                  query={query}
+                  chartStatus={this.props.chartStatus}
+                  queryResponse={queryResponse}
                   queryEndpoint={getExploreUrl(this.props.latestQueryFormData, 'query')}
                 />
               </div>

diff --git a/superset/assets/javascripts/explorev2/components/DisplayQueryButton.jsx b/superset/assets/javascripts/explorev2/components/DisplayQueryButton.jsx
@@ -7,62 +7,91 @@ import ModalTrigger from './../../components/ModalTrigger';
 const $ = window.$ = require('jquery');
 
 const propTypes = {
-  query: PropTypes.string,
+  animation: PropTypes.bool,
+  queryResponse: PropTypes.object,
+  chartStatus: PropTypes.string,
   queryEndpoint: PropTypes.string.isRequired,
 };
+const defaultProps = {
+  animation: true,
+};
 
 export default class DisplayQueryButton extends React.PureComponent {
   constructor(props) {
     super(props);
     this.state = {
-      modalBody: <pre />,
+      language: null,
+      query: null,
+      isLoading: false,
+      error: null,
     };
+    this.beforeOpen = this.beforeOpen.bind(this);
+    this.fetchQuery = this.fetchQuery.bind(this);
   }
-  beforeOpen() {
+  fetchQuery() {
+    this.setState({ isLoading: true });
+    $.ajax({
+      type: 'GET',
+      url: this.props.queryEndpoint,
+      success: data => {
+        this.setState({
+          language: data.language,
+          query: data.query,
+          isLoading: false,
+        });
+      },
+      error: data => {
+        this.setState({
+          error: data.error,
+          isLoading: false,
+        });
+      },
+    });
+  }
+  setStateFromQueryResponse() {
+    const qr = this.props.queryResponse;
     this.setState({
-      modalBody:
-        (<img
-          className="loading"
-          alt="Loading..."
-          src="/static/assets/images/loading.gif"
-        />),
+      language: qr.language,
+      query: qr.query,
+      isLoading: false,
     });
-    if (this.props.query) {
-      const modalBody = (
-        <pre>{this.props.query}</pre>
-      );
-      this.setState({ modalBody });
+  }
+  beforeOpen() {
+    if (this.props.chartStatus === 'loading' || this.props.chartStatus === null) {
+      this.fetchQuery();
     } else {
-      $.ajax({
-        type: 'GET',
-        url: this.props.queryEndpoint,
-        success: (data) => {
-          const modalBody = data.language ?
-            (<SyntaxHighlighter language={data.language} style={github}>
-              {data.query}
-            </SyntaxHighlighter>)
-            :
-            <pre>{data.query}</pre>;
-          this.setState({ modalBody });
-        },
-        error(data) {
-          this.setState({ modalBody: (<pre>{data.error}</pre>) });
-        },
-      });
+      this.setStateFromQueryResponse();
+    }
+  }
+  renderModalBody() {
+    if (this.state.isLoading) {
+      return (<img
+        className="loading"
+        alt="Loading..."
+        src="/static/assets/images/loading.gif"
+      />);
+    } else if (this.state.error) {
+      return <pre>{this.state.error}</pre>;
     }
+    return (
+      <SyntaxHighlighter language={this.state.language} style={github}>
+        {this.state.query}
+      </SyntaxHighlighter>);
   }
   render() {
     return (
       <ModalTrigger
+        animation={this.props.animation}
         isButton
         triggerNode={<span>Query</span>}
         modalTitle="Query"
         bsSize="large"
-        beforeOpen={this.beforeOpen.bind(this)}
-        modalBody={this.state.modalBody}
+        beforeOpen={this.beforeOpen}
+        modalBody={this.renderModalBody()}
       />
     );
   }
 }
 
 DisplayQueryButton.propTypes = propTypes;
+DisplayQueryButton.defaultProps = defaultProps;
diff --git a/superset/assets/javascripts/explorev2/components/ExploreActionButtons.jsx b/superset/assets/javascripts/explorev2/components/ExploreActionButtons.jsx
@@ -8,10 +8,12 @@ const propTypes = {
   canDownload: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]).isRequired,
   slice: PropTypes.object,
   queryEndpoint: PropTypes.string,
-  query: PropTypes.string,
+  queryResponse: PropTypes.object,
+  chartStatus: PropTypes.string,
 };
 
-export default function ExploreActionButtons({ canDownload, slice, query, queryEndpoint }) {
+export default function ExploreActionButtons({
+    chartStatus, canDownload, slice, queryResponse, queryEndpoint }) {
   const exportToCSVClasses = cx('btn btn-default btn-sm', {
     'disabled disabledButton': !canDownload,
   });
@@ -43,8 +45,9 @@ export default function ExploreActionButtons({ canDownload, slice, query, queryE
         </a>
 
         <DisplayQueryButton
-          query={query}
+          queryResponse={queryResponse}
           queryEndpoint={queryEndpoint}
+          chartStatus={chartStatus}
         />
       </div>
     );

diff --git a/superset/assets/spec/javascripts/explorev2/components/DisplayQueryButton_spec.jsx b/superset/assets/spec/javascripts/explorev2/components/DisplayQueryButton_spec.jsx
@@ -1,17 +1,30 @@
 import React from 'react';
 import { expect } from 'chai';
 import { describe, it } from 'mocha';
+import { mount } from 'enzyme';
+import { Modal } from 'react-bootstrap';
+import ModalTrigger from './../../../../javascripts/components/ModalTrigger.jsx';
 
 import DisplayQueryButton from '../../../../javascripts/explorev2/components/DisplayQueryButton';
 
 describe('DisplayQueryButton', () => {
   const defaultProps = {
-    slice: {
-      viewSqlQuery: 'sql query string',
+    animation: false,
+    queryResponse: {
+      query: 'SELECT * FROM foo',
+      language: 'sql',
     },
+    chartStatus: 'success',
+    queryEndpoint: 'localhost',
   };
 
-  it('renders', () => {
+  it('is valid', () => {
     expect(React.isValidElement(<DisplayQueryButton {...defaultProps} />)).to.equal(true);
   });
+  it('renders a button and a modal', () => {
+    const wrapper = mount(<DisplayQueryButton {...defaultProps} />);
+    expect(wrapper.find(ModalTrigger)).to.have.lengthOf(1);
+    wrapper.find('.modal-trigger').simulate('click');
+    expect(wrapper.find(Modal)).to.have.lengthOf(1);
+  });
 });
diff --git a/superset/connectors/base.py b/superset/connectors/base.py
@@ -120,6 +120,28 @@ def data(self):
 
         return d
 
+    def get_query_str(self, query_obj):
+        """Returns a query as a string
+
+        This is used to be displayed to the user so that she/he can
+        understand what is taking place behind the scene"""
+        raise NotImplementedError()
+
+    def query(self, query_obj):
+        """Executes the query and returns a dataframe
+
+        query_obj is a dictionary representing Superset's query interface.
+        Should return a ``superset.models.helpers.QueryResult``
+        """
+        raise NotImplementedError()
+
+    def values_for_column(self, column_name, limit=10000):
+        """Given a column, returns an iterable of distinct values
+
+        This is used to populate the dropdown showing a list of
+        values in filters in the explore view"""
+        raise NotImplementedError()
+
 
 class BaseColumn(AuditMixinNullable, ImportMixin):
     """Interface for column"""

diff --git a/superset/connectors/druid/models.py b/superset/connectors/druid/models.py
@@ -696,7 +696,10 @@ def values_for_column(self,
         df = client.export_pandas()
         return [row[column_name] for row in df.to_records(index=False)]
 
-    def get_query_str(  # noqa / druid
+    def get_query_str(self, query_obj, phase=1, client=None):
+        return self.run_query(client=client, phase=phase, **query_obj)
+
+    def run_query(  # noqa / druid
             self,
             groupby, metrics,
             granularity,
@@ -712,8 +715,6 @@ def get_query_str(  # noqa / druid
             select=None,  # noqa
             columns=None, phase=2, client=None, form_data=None):
         """Runs a query against Druid and returns a dataframe.
-
-        This query interface is common to SqlAlchemy and Druid
         """
         # TODO refactor into using a TBD Query object
         client = client or self.cluster.get_pydruid_client()
@@ -917,7 +918,8 @@ def recursive_get_fields(_conf):
     def query(self, query_obj):
         qry_start_dttm = datetime.now()
         client = self.cluster.get_pydruid_client()
-        query_str = self.get_query_str(client=client, **query_obj)
+        query_str = self.get_query_str(
+            client=client, query_obj=query_obj, phase=2)
         df = client.export_pandas()
 
         if df is None or df.size == 0:

diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
@@ -312,9 +312,9 @@ def get_template_processor(self, **kwargs):
         return get_template_processor(
             table=self, database=self.database, **kwargs)
 
-    def get_query_str(self, **kwargs):
+    def get_query_str(self, query_obj):
         engine = self.database.get_sqla_engine()
-        qry = self.get_sqla_query(**kwargs)
+        qry = self.get_sqla_query(**query_obj)
         sql = str(
             qry.compile(
                 engine,
@@ -538,7 +538,7 @@ def query(self, query_obj):
         qry_start_dttm = datetime.now()
         engine = self.database.get_sqla_engine()
         qry = self.get_sqla_query(**query_obj)
-        sql = self.get_query_str(**query_obj)
+        sql = self.get_query_str(query_obj)
         status = QueryStatus.SUCCESS
         error_message = None
         df = None

diff --git a/superset/views/core.py b/superset/views/core.py
@@ -923,11 +923,7 @@ def explore_json(self, datasource_type, datasource_id):
         if request.args.get("query") == "true":
             try:
                 query_obj = viz_obj.query_obj()
-                if datasource_type == 'druid':
-                    # only retrive first phase query for druid
-                    query_obj['phase'] = 1
-                query = viz_obj.datasource.get_query_str(
-                    datetime.now(), **query_obj)
+                query = viz_obj.datasource.get_query_str(query_obj)
             except Exception as e:
                 return json_error_response(e)
             return Response(