[sql lab] improvements to the left panel (#2709)

* [sql lab] improvements to the left panel * better error handling with error notifications * table is added instantly with a loading image * Fixing tests * Fixed tests
2024-09-12 00:29:39 -04:00 · 2017-05-09 13:36:10 -07:00 · 2017-05-09 13:36:10 -07:00 · a471afe206
commit a471afe206
parent 5d0a01d0d0
8 changed files with 194 additions and 119 deletions
--- a/superset/assets/javascripts/SqlLab/actions.js
+++ b/superset/assets/javascripts/SqlLab/actions.js
@ -247,6 +247,17 @@ export function mergeTable(table, query) {

 export function addTable(query, tableName, schemaName) {
  return function (dispatch) {
+    let table = {
+      dbId: query.dbId,
+      queryEditorId: query.id,
+      schema: schemaName,
+      name: tableName,
+      isMetadataLoading: true,
+      isExtraMetadataLoading: true,
+      expanded: false,
+    };
+    dispatch(mergeTable(table));
+
    let url = `/superset/table/${query.dbId}/${tableName}/${schemaName}/`;
    $.get(url, (data) => {
      const dataPreviewQuery = {
@ -260,35 +271,21 @@ export function addTable(query, tableName, schemaName) {
        ctas: false,
      };
      // Merge table to tables in state
-      dispatch(mergeTable(
-        Object.assign(data, {
-          dbId: query.dbId,
-          queryEditorId: query.id,
-          schema: schemaName,
-          expanded: true,
-        }), dataPreviewQuery),
-      );
+      table = Object.assign({}, table, data, {
+        expanded: true,
+        isMetadataLoading: false,
+      });
+      dispatch(mergeTable(table, dataPreviewQuery));
      // Run query to get preview data for table
      dispatch(runQuery(dataPreviewQuery));
    })
    .fail(() => {
-      dispatch(
-        addAlert({
-          msg: 'Error occurred while fetching metadata',
-          bsStyle: 'danger',
-        }),
-      );
+      notify.error('Error occurred while fetching table metadata');
    });

    url = `/superset/extra_table_metadata/${query.dbId}/${tableName}/${schemaName}/`;
    $.get(url, (data) => {
-      const table = {
-        dbId: query.dbId,
-        queryEditorId: query.id,
-        schema: schemaName,
-        name: tableName,
-      };
-      Object.assign(table, data);
+      table = Object.assign({}, table, data, { isExtraMetadataLoading: false });
      dispatch(mergeTable(table));
    });
  };
--- a/superset/assets/javascripts/SqlLab/components/SqlEditorLeftBar.jsx
+++ b/superset/assets/javascripts/SqlLab/components/SqlEditorLeftBar.jsx
@ -1,3 +1,4 @@
+/* global notify */
 import React from 'react';
 import PropTypes from 'prop-types';
 import { Button } from 'react-bootstrap';
@ -82,6 +83,10 @@ class SqlEditorLeftBar extends React.PureComponent {
          tableOptions: data.options,
          tableLength: data.tableLength,
        });
+      })
+      .fail(() => {
+        this.setState({ tableLoading: false, tableOptions: [], tableLength: 0 });
+        notify.error('Error while fetching table list');
      });
    } else {
      this.setState({ tableLoading: false, tableOptions: [], filterOptions: null });
@ -104,11 +109,7 @@ class SqlEditorLeftBar extends React.PureComponent {
      this.props.actions.queryEditorSetSchema(this.props.queryEditor, schemaName);
      this.fetchTables(this.props.queryEditor.dbId, schemaName);
    }
-    this.setState({ tableLoading: true });
-    // TODO: handle setting the tableLoading state depending on success or
-    //       failure of the addTable async call in the action.
    this.props.actions.addTable(this.props.queryEditor, tableName, schemaName);
-    this.setState({ tableLoading: false });
  }
  changeSchema(schemaOpt) {
    const schema = (schemaOpt) ? schemaOpt.value : null;
@ -122,8 +123,11 @@ class SqlEditorLeftBar extends React.PureComponent {
      const url = `/superset/schemas/${actualDbId}/`;
      $.get(url, (data) => {
        const schemaOptions = data.schemas.map(s => ({ value: s, label: s }));
-        this.setState({ schemaOptions });
-        this.setState({ schemaLoading: false });
+        this.setState({ schemaOptions, schemaLoading: false });
+      })
+      .fail(() => {
+        this.setState({ schemaLoading: false, schemaOptions: [] });
+        notify.error('Error while fetching schema list');
      });
    }
  }
@ -145,6 +149,7 @@ class SqlEditorLeftBar extends React.PureComponent {
              '_od_DatabaseView=asc'
            }
            onChange={this.onDatabaseChange.bind(this)}
+            onAsyncError={() => notify.error('Error while fetching database list')}
            value={this.props.queryEditor.dbId}
            databaseId={this.props.queryEditor.dbId}
            actions={this.props.actions}
--- a/superset/assets/javascripts/SqlLab/components/TableElement.jsx
+++ b/superset/assets/javascripts/SqlLab/components/TableElement.jsx
@ -8,6 +8,7 @@ import CopyToClipboard from '../../components/CopyToClipboard';
 import Link from './Link';
 import ColumnElement from './ColumnElement';
 import ModalTrigger from '../../components/ModalTrigger';
+import Loading from '../../components/Loading';

 const propTypes = {
  table: PropTypes.object,
@ -62,7 +63,7 @@ class TableElement extends React.PureComponent {
    this.props.actions.removeTable(this.props.table);
  }

-  renderHeader() {
+  renderWell() {
    const table = this.props.table;
    let header;
    if (table.partitions) {
@ -97,37 +98,9 @@ class TableElement extends React.PureComponent {
    }
    return header;
  }
-  renderMetadata() {
-    const table = this.props.table;
-    let cols;
-    if (table.columns) {
-      cols = table.columns.slice();
-      if (this.state.sortColumns) {
-        cols.sort((a, b) => a.name.toUpperCase() > b.name.toUpperCase());
-      }
-    }
-    const metadata = (
-      <Collapse
-        in={table.expanded}
-        timeout={this.props.timeout}
-      >
-        <div>
-          {this.renderHeader()}
-          <div className="table-columns">
-            {cols && cols.map(col => (
-              <ColumnElement column={col} key={col.name} />
-            ))}
-            <hr />
-          </div>
-        </div>
-      </Collapse>
-    );
-    return metadata;
-  }
-
-  render() {
-    const table = this.props.table;
+  renderControls() {
    let keyLink;
+    const table = this.props.table;
    if (table.indexes && table.indexes.length > 0) {
      keyLink = (
        <ModalTrigger
@ -148,6 +121,94 @@ class TableElement extends React.PureComponent {
        />
      );
    }
+    return (
+      <ButtonGroup className="ws-el-controls pull-right">
+        {keyLink}
+        <Link
+          className={
+            `fa fa-sort-${!this.state.sortColumns ? 'alpha' : 'numeric'}-asc ` +
+            'pull-left sort-cols m-l-2'}
+          onClick={this.toggleSortColumns.bind(this)}
+          tooltip={
+            !this.state.sortColumns ?
+            'Sort columns alphabetically' :
+            'Original table column order'}
+          href="#"
+        />
+        {table.selectStar &&
+          <CopyToClipboard
+            copyNode={
+              <a className="fa fa-clipboard pull-left m-l-2" />
+            }
+            text={table.selectStar}
+            shouldShowText={false}
+            tooltipText="Copy SELECT statement to clipboard"
+          />
+        }
+        <Link
+          className="fa fa-times table-remove pull-left m-l-2"
+          onClick={this.removeTable.bind(this)}
+          tooltip="Remove table preview"
+          href="#"
+        />
+      </ButtonGroup>
+    );
+  }
+  renderHeader() {
+    const table = this.props.table;
+    return (
+      <div className="clearfix">
+        <div className="pull-left">
+          <a
+            href="#"
+            className="table-name"
+            onClick={(e) => { this.toggleTable(e); }}
+          >
+            <strong>{table.name}</strong>
+            <small className="m-l-5">
+              <i className={`fa fa-${table.expanded ? 'minus' : 'plus'}-square-o`} />
+            </small>
+          </a>
+        </div>
+        <div className="pull-right">
+          {table.isMetadataLoading || table.isExtraMetadataLoading ?
+            <Loading size={20} />
+            :
+            this.renderControls()
+          }
+        </div>
+      </div>
+    );
+  }
+  renderBody() {
+    const table = this.props.table;
+    let cols;
+    if (table.columns) {
+      cols = table.columns.slice();
+      if (this.state.sortColumns) {
+        cols.sort((a, b) => a.name.toUpperCase() > b.name.toUpperCase());
+      }
+    }
+    const metadata = (
+      <Collapse
+        in={table.expanded}
+        timeout={this.props.timeout}
+      >
+        <div>
+          {this.renderWell()}
+          <div className="table-columns">
+            {cols && cols.map(col => (
+              <ColumnElement column={col} key={col.name} />
+            ))}
+            <hr />
+          </div>
+        </div>
+      </Collapse>
+    );
+    return metadata;
+  }
+
+  render() {
    return (
      <Collapse
        in={this.state.expanded}
@ -156,54 +217,9 @@ class TableElement extends React.PureComponent {
        onExited={this.removeFromStore.bind(this)}
      >
        <div className="TableElement">
-          <div className="clearfix">
-            <div className="pull-left">
-              <a
-                href="#"
-                className="table-name"
-                onClick={(e) => { this.toggleTable(e); }}
-              >
-                <strong>{table.name}</strong>
-                <small className="m-l-5">
-                  <i className={`fa fa-${table.expanded ? 'minus' : 'plus'}-square-o`} />
-                </small>
-              </a>
-            </div>
-            <div className="pull-right">
-              <ButtonGroup className="ws-el-controls pull-right">
-                {keyLink}
-                <Link
-                  className={
-                    `fa fa-sort-${!this.state.sortColumns ? 'alpha' : 'numeric'}-asc ` +
-                    'pull-left sort-cols m-l-2'}
-                  onClick={this.toggleSortColumns.bind(this)}
-                  tooltip={
-                    !this.state.sortColumns ?
-                    'Sort columns alphabetically' :
-                    'Original table column order'}
-                  href="#"
-                />
-                {table.selectStar &&
-                  <CopyToClipboard
-                    copyNode={
-                      <a className="fa fa-clipboard pull-left m-l-2" />
-                    }
-                    text={table.selectStar}
-                    shouldShowText={false}
-                    tooltipText="Copy SELECT statement to clipboard"
-                  />
-                }
-                <Link
-                  className="fa fa-trash table-remove pull-left m-l-2"
-                  onClick={this.removeTable.bind(this)}
-                  tooltip="Remove table preview"
-                  href="#"
-                />
-              </ButtonGroup>
-            </div>
-          </div>
+          {this.renderHeader()}
          <div>
-            {this.renderMetadata()}
+            {this.renderBody()}
          </div>
        </div>
      </Collapse>
--- a/superset/assets/javascripts/components/AsyncSelect.jsx
+++ b/superset/assets/javascripts/components/AsyncSelect.jsx
@ -8,6 +8,7 @@ const propTypes = {
  dataEndpoint: PropTypes.string.isRequired,
  onChange: PropTypes.func.isRequired,
  mutator: PropTypes.func.isRequired,
+  onAsyncError: PropTypes.func,
  value: PropTypes.number,
  valueRenderer: PropTypes.func,
  placeholder: PropTypes.string,
@ -17,6 +18,7 @@ const propTypes = {
 const defaultProps = {
  placeholder: 'Select ...',
  valueRenderer: o => (<div>{o.label}</div>),
+  onAsyncError: () => {},
 };

 class AsyncSelect extends React.PureComponent {
@ -42,6 +44,12 @@ class AsyncSelect extends React.PureComponent {
      if (this.props.autoSelect && this.state.options.length) {
        this.onChange(this.state.options[0]);
      }
+    })
+    .fail(() => {
+      this.props.onAsyncError();
+    })
+    .always(() => {
+      this.setState({ isLoading: false });
    });
  }
  render() {
--- a/superset/assets/javascripts/components/Loading.jsx
+++ b/superset/assets/javascripts/components/Loading.jsx
@ -0,0 +1,27 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+
+const propTypes = {
+  size: PropTypes.number,
+};
+const defaultProps = {
+  size: 25,
+};
+
+export default function Loading(props) {
+  return (
+    <img
+      className="loading"
+      alt="Loading..."
+      src="/static/assets/images/loading.gif"
+      style={{
+        width: props.size,
+        height: props.size,
+        padding: 0,
+        margin: 0,
+      }}
+    />
+  );
+}
+Loading.propTypes = propTypes;
+Loading.defaultProps = defaultProps;
--- a/superset/assets/spec/helpers/browser.js
+++ b/superset/assets/spec/helpers/browser.js
@ -1,12 +1,14 @@
 /* eslint no-undef: 0, no-native-reassign: 0 */
+import 'babel-polyfill';
+import chai from 'chai';
+import jsdom from 'jsdom';

 require('babel-register')();

-const jsdom = require('jsdom').jsdom;
-
 const exposedProperties = ['window', 'navigator', 'document'];

-global.document = jsdom('');
+global.jsdom = jsdom.jsdom;
+global.document = global.jsdom('<!doctype html><html><body></body></html>');
 global.window = document.defaultView;
 Object.keys(document.defaultView).forEach((property) => {
  if (typeof global[property] === 'undefined') {
@ -20,3 +22,19 @@ global.navigator = {
  platform: 'linux',
  appName: 'Netscape',
 };
+
+// Configuration copied from https://github.com/sinonjs/sinon/issues/657
+// allowing for sinon.fakeServer to work
+
+global.window = global.document.defaultView;
+global.XMLHttpRequest = global.window.XMLHttpRequest;
+
+global.sinon = require('sinon');
+
+global.expect = chai.expect;
+global.assert = chai.assert;
+
+global.sinon.useFakeXMLHttpRequest();
+
+global.window.XMLHttpRequest = global.XMLHttpRequest;
+global.$ = require('jquery')(global.window);
--- a/superset/assets/spec/javascripts/components/AsyncSelect_spec.jsx
+++ b/superset/assets/spec/javascripts/components/AsyncSelect_spec.jsx
@ -1,10 +1,9 @@
 import React from 'react';
 import Select from 'react-select';
-import { shallow } from 'enzyme';
+import { mount, shallow } from 'enzyme';
 import { describe, it } from 'mocha';
 import { expect } from 'chai';
 import sinon from 'sinon';
-import $ from 'jquery';

 import AsyncSelect from '../../../javascripts/components/AsyncSelect';

@ -39,30 +38,32 @@ describe('AsyncSelect', () => {
  });

  describe('auto select', () => {
-    let stub;
+    let server;
    beforeEach(() => {
-      stub = sinon.stub($, 'get');
-      stub.yields();
+      server = sinon.fakeServer.create();
+      server.respondWith([
+        200, { 'Content-Type': 'application/json' }, JSON.stringify({}),
+      ]);
    });
    afterEach(() => {
-      stub.restore();
+      server.restore();
    });
    it('should be off by default', () => {
-      const wrapper = shallow(
+      const wrapper = mount(
        <AsyncSelect {...mockedProps} />,
      );
      const spy = sinon.spy(wrapper.instance(), 'onChange');
-
-      wrapper.instance().fetchOptions();
      expect(spy.callCount).to.equal(0);
    });
    it('should auto select first option', () => {
-      const wrapper = shallow(
+      const wrapper = mount(
        <AsyncSelect {...mockedProps} autoSelect />,
      );
      const spy = sinon.spy(wrapper.instance(), 'onChange');

-      wrapper.instance().fetchOptions();
+      server.respond();
+
+      expect(spy.callCount).to.equal(1);
      expect(spy.calledWith(wrapper.instance().state.options[0])).to.equal(true);
    });
  });
--- a/superset/assets/spec/javascripts/sqllab/SqlEditorLeftBar_spec.jsx
+++ b/superset/assets/spec/javascripts/sqllab/SqlEditorLeftBar_spec.jsx
@ -7,6 +7,9 @@ import { table, defaultQueryEditor } from './fixtures';
 import SqlEditorLeftBar from '../../../javascripts/SqlLab/components/SqlEditorLeftBar';
 import TableElement from '../../../javascripts/SqlLab/components/TableElement';

+global.notify = {
+  error: () => {},
+};

 describe('SqlEditorLeftBar', () => {
  const mockedProps = {