diff --git a/superset/assets/javascripts/chart/Chart.jsx b/superset/assets/javascripts/chart/Chart.jsx index 2824b255540e0..a7bff52b34c6a 100644 --- a/superset/assets/javascripts/chart/Chart.jsx +++ b/superset/assets/javascripts/chart/Chart.jsx @@ -180,7 +180,7 @@ class Chart extends React.PureComponent { /> } - {!this.props.chartAlert && + {!isLoading && !this.props.chartAlert && ); diff --git a/superset/assets/javascripts/dashboard/components/Dashboard.jsx b/superset/assets/javascripts/dashboard/components/Dashboard.jsx index c99c293604ad9..bcc13778d2fe1 100644 --- a/superset/assets/javascripts/dashboard/components/Dashboard.jsx +++ b/superset/assets/javascripts/dashboard/components/Dashboard.jsx @@ -93,12 +93,21 @@ class Dashboard extends React.PureComponent { } componentDidUpdate(prevProps) { - if (!areObjectsEqual(prevProps.filters, this.props.filters) && this.props.refresh) { - const currentFilterKeys = Object.keys(this.props.filters); - if (currentFilterKeys.length) { - currentFilterKeys.forEach(key => (this.refreshExcept(key))); - } else { - this.refreshExcept(); + if (this.props.refresh) { + let changedFilterKey; + const prevFiltersKeySet = new Set(Object.keys(prevProps.filters)); + Object.keys(this.props.filters).some((key) => { + prevFiltersKeySet.delete(key); + if (prevProps.filters[key] === undefined || + !areObjectsEqual(prevProps.filters[key], this.props.filters[key])) { + changedFilterKey = key; + return true; + } + return false; + }); + // has changed filter or removed a filter? + if (!!changedFilterKey || prevFiltersKeySet.size) { + this.refreshExcept(changedFilterKey); } } } diff --git a/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx b/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx index 30b47994eb3bf..0faf164df4cf9 100644 --- a/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx +++ b/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx @@ -74,7 +74,7 @@ class ExploreChartHeader extends React.PureComponent { json_endpoint: getExploreUrl(formData, 'json'), standalone_endpoint: getExploreUrl(formData, 'standalone'), }; - + const chartSucceeded = ['success', 'rendered'].indexOf(this.props.chart.chartStatus) > 0; return (
}
- {this.props.chart.chartStatus === 'success' && queryResponse && + {chartSucceeded && queryResponse && - } - {this.props.chart.chartStatus === 'success' && - queryResponse && - queryResponse.is_cached && - - - } + />} + {chartSucceeded && queryResponse && queryResponse.is_cached && + } { const chart = { @@ -30,15 +32,20 @@ describe('Chart', () => { }, }; + let wrapper; + beforeEach(() => { + wrapper = shallow( + , + ); + }); describe('renderViz', () => { - let wrapper; let stub; beforeEach(() => { - wrapper = shallow( - , - ); stub = sinon.stub(wrapper.instance(), 'renderViz'); }); + afterEach(() => { + stub.restore(); + }); it('should not call when loading', () => { const prevProp = wrapper.props(); @@ -68,4 +75,11 @@ describe('Chart', () => { expect(stub.callCount).to.equals(1); }); }); + + describe('render', () => { + it('should render ChartBody after loading is completed', () => { + expect(wrapper.find(Loading)).to.have.length(1); + expect(wrapper.find(ChartBody)).to.have.length(0); + }); + }); }); diff --git a/superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx b/superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx index 95f013f115080..1ac495992a630 100644 --- a/superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx +++ b/superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx @@ -26,7 +26,7 @@ describe('Dashboard', () => { it('should render', () => { const wrapper = shallow(); expect(wrapper.find('#dashboard-container')).to.have.length(1); - expect(wrapper.instance().getAllSlices()).to.have.length(2); + expect(wrapper.instance().getAllSlices()).to.have.length(3); }); it('should handle metadata default_filters', () => { @@ -51,10 +51,8 @@ describe('Dashboard', () => { it('should carry updated filter', () => { wrapper.setProps({ filters: { - 256: { - region: [], - country_name: ['France'], - }, + 256: { region: [] }, + 257: { country_name: ['France'] }, }, }); const extraFilters = wrapper.instance().getFormDataExtra(selectedSlice).extra_filters; @@ -74,7 +72,7 @@ describe('Dashboard', () => { }); it('should not refresh filter slice', () => { - const filterKey = Object.keys(defaultFilters)[0]; + const filterKey = Object.keys(defaultFilters)[1]; wrapper.instance().refreshExcept(filterKey); expect(spy.callCount).to.equal(1); expect(spy.getCall(0).args[0].length).to.equal(1); @@ -83,7 +81,102 @@ describe('Dashboard', () => { it('should refresh all slices', () => { wrapper.instance().refreshExcept(); expect(spy.callCount).to.equal(1); - expect(spy.getCall(0).args[0].length).to.equal(2); + expect(spy.getCall(0).args[0].length).to.equal(3); + }); + }); + + describe('componentDidUpdate', () => { + let wrapper; + let refreshExceptSpy; + let fetchSlicesStub; + let prevProp; + beforeEach(() => { + wrapper = shallow(); + prevProp = wrapper.instance().props; + refreshExceptSpy = sinon.spy(wrapper.instance(), 'refreshExcept'); + fetchSlicesStub = sinon.stub(wrapper.instance(), 'fetchSlices'); + }); + afterEach(() => { + fetchSlicesStub.restore(); + refreshExceptSpy.restore(); + }); + + describe('should check if filter has change', () => { + beforeEach(() => { + refreshExceptSpy.reset(); + }); + it('no change', () => { + wrapper.setProps({ + refresh: true, + filters: { + 256: { region: [] }, + 257: { country_name: ['United States'] }, + }, + }); + wrapper.instance().componentDidUpdate(prevProp); + expect(refreshExceptSpy.callCount).to.equal(0); + }); + + it('remove filter', () => { + wrapper.setProps({ + refresh: true, + filters: { + 256: { region: [] }, + }, + }); + wrapper.instance().componentDidUpdate(prevProp); + expect(refreshExceptSpy.callCount).to.equal(1); + }); + + it('change filter', () => { + wrapper.setProps({ + refresh: true, + filters: { + 256: { region: [] }, + 257: { country_name: ['Canada'] }, + }, + }); + wrapper.instance().componentDidUpdate(prevProp); + expect(refreshExceptSpy.callCount).to.equal(1); + }); + + it('add filter', () => { + wrapper.setProps({ + refresh: true, + filters: { + 256: { region: [] }, + 257: { country_name: ['Canada'] }, + 258: { another_filter: ['new'] }, + }, + }); + wrapper.instance().componentDidUpdate(prevProp); + expect(refreshExceptSpy.callCount).to.equal(1); + }); + }); + + it('should refresh if refresh flag is true', () => { + wrapper.setProps({ + refresh: true, + filters: { + 256: { region: ['Asian'] }, + }, + }); + wrapper.instance().componentDidUpdate(prevProp); + const fetchArgs = fetchSlicesStub.lastCall.args[0]; + expect(fetchArgs).to.have.length(2); + }); + + it('should not refresh filter_immune_slices', () => { + wrapper.setProps({ + refresh: true, + filters: { + 256: { region: [] }, + 257: { country_name: ['Canada'] }, + }, + }); + wrapper.instance().componentDidUpdate(prevProp); + const fetchArgs = fetchSlicesStub.lastCall.args[0]; + expect(fetchArgs).to.have.length(1); }); }); }); diff --git a/superset/assets/spec/javascripts/dashboard/fixtures.jsx b/superset/assets/spec/javascripts/dashboard/fixtures.jsx index 7b95e7c65533c..1b9cf21b62c1b 100644 --- a/superset/assets/spec/javascripts/dashboard/fixtures.jsx +++ b/superset/assets/spec/javascripts/dashboard/fixtures.jsx @@ -1,10 +1,8 @@ import { getInitialState } from '../../../javascripts/dashboard/reducers'; export const defaultFilters = { - 256: { - region: [], - country_name: ['United States'], - }, + 256: { region: [] }, + 257: { country_name: ['United States'] }, }; export const regionFilter = { datasource: null, @@ -39,6 +37,35 @@ export const regionFilter = { slice_name: 'Region Filters', slice_url: '/superset/explore/table/2/?form_data=%7B%22slice_id%22%3A%20256%7D', }; +export const countryFilter = { + datasource: null, + description: null, + description_markeddown: '', + edit_url: '/slicemodelview/edit/257', + form_data: { + datasource: '2__table', + date_filter: false, + filters: [], + granularity_sqla: null, + groupby: ['country_name'], + having: '', + instant_filtering: true, + metric: 'sum__SP_POP_TOTL', + show_druid_time_granularity: false, + show_druid_time_origin: false, + show_sqla_time_column: false, + show_sqla_time_granularity: false, + since: '100 years ago', + slice_id: 257, + time_grain_sqla: null, + until: 'now', + viz_type: 'filter_box', + where: '', + }, + slice_id: 257, + slice_name: 'Country Filters', + slice_url: '/superset/explore/table/2/?form_data=%7B%22slice_id%22%3A%20257%7D', +}; export const slice = { datasource: null, description: null, @@ -100,7 +127,7 @@ const mockDashboardData = { id: 2, metadata: { default_filters: JSON.stringify(defaultFilters), - filter_immune_slices: [], + filter_immune_slices: [256], timed_refresh_immune_slices: [], filter_immune_slice_fields: {}, expanded_slices: {}, @@ -122,7 +149,7 @@ const mockDashboardData = { }, ], slug: 'births', - slices: [regionFilter, slice], + slices: [regionFilter, slice, countryFilter], standalone_mode: false, }; export const { dashboard, charts } = getInitialState({ diff --git a/superset/assets/spec/javascripts/dashboard/reducers_spec.js b/superset/assets/spec/javascripts/dashboard/reducers_spec.js index 17b8f75df54c3..8022928ddf4b5 100644 --- a/superset/assets/spec/javascripts/dashboard/reducers_spec.js +++ b/superset/assets/spec/javascripts/dashboard/reducers_spec.js @@ -11,8 +11,10 @@ describe('Dashboard reducers', () => { type: actions.REMOVE_SLICE, slice: initState.dashboard.slices[1], }; + expect(initState.dashboard.slices).to.have.length(3); + const { dashboard, filters, refresh } = reducers(initState, action); - expect(dashboard.slices).to.have.length(1); + expect(dashboard.slices).to.have.length(2); expect(filters).to.deep.equal(defaultFilters); expect(refresh).to.equal(false); }); @@ -22,9 +24,12 @@ describe('Dashboard reducers', () => { type: actions.REMOVE_SLICE, slice: initState.dashboard.slices[0], }; + const initFilters = Object.keys(initState.filters); + expect(initFilters).to.have.length(2); + const { dashboard, filters, refresh } = reducers(initState, action); - expect(dashboard.slices).to.have.length(1); - expect(filters).to.deep.equal({}); + expect(dashboard.slices).to.have.length(2); + expect(Object.keys(filters)).to.have.length(1); expect(refresh).to.equal(true); }); }); diff --git a/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx b/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx new file mode 100644 index 0000000000000..2875e8398eac4 --- /dev/null +++ b/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx @@ -0,0 +1,40 @@ +import React from 'react'; +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { shallow } from 'enzyme'; + +import ExploreChartHeader from '../../../../javascripts/explore/components/ExploreChartHeader'; +import ExploreActionButtons from '../../../../javascripts/explore/components/ExploreActionButtons'; +import EditableTitle from '../../../../javascripts/components/EditableTitle'; + +const mockProps = { + actions: {}, + can_overwrite: true, + can_download: true, + isStarred: true, + slice: {}, + table_name: 'foo', + form_data: {}, + timeout: 1000, + chart: { + queryResponse: {}, + }, +}; + +describe('ExploreChartHeader', () => { + let wrapper; + beforeEach(() => { + wrapper = shallow(); + }); + + it('is valid', () => { + expect( + React.isValidElement(), + ).to.equal(true); + }); + + it('renders', () => { + expect(wrapper.find(EditableTitle)).to.have.lengthOf(1); + expect(wrapper.find(ExploreActionButtons)).to.have.lengthOf(1); + }); +}); diff --git a/superset/assets/stylesheets/superset.less b/superset/assets/stylesheets/superset.less index c5a8ea7d4576f..488adebd5ed62 100644 --- a/superset/assets/stylesheets/superset.less +++ b/superset/assets/stylesheets/superset.less @@ -200,6 +200,7 @@ div.widget { .stack-trace-container, .slice_container { opacity: 0.5; + position: relative; } } } diff --git a/superset/db_engine_specs.py b/superset/db_engine_specs.py index d26f633bbd266..e55fa9482902a 100644 --- a/superset/db_engine_specs.py +++ b/superset/db_engine_specs.py @@ -133,13 +133,14 @@ def _allowed_file(filename): 'table': table, 'df': df, 'name': form.name.data, - 'con': create_engine(form.con.data, echo=False), + 'con': create_engine(form.con.data.sqlalchemy_uri, echo=False), 'schema': form.schema.data, 'if_exists': form.if_exists.data, 'index': form.index.data, 'index_label': form.index_label.data, 'chunksize': 10000, } + BaseEngineSpec.df_to_db(**df_to_db_kwargs) @classmethod diff --git a/superset/forms.py b/superset/forms.py index a07790440ffde..cacb9067eb81b 100644 --- a/superset/forms.py +++ b/superset/forms.py @@ -10,14 +10,20 @@ from flask_wtf.file import FileAllowed, FileField, FileRequired from wtforms import ( BooleanField, IntegerField, SelectField, StringField) +from wtforms.ext.sqlalchemy.fields import QuerySelectField from wtforms.validators import DataRequired, NumberRange, Optional -from superset import app +from superset import app, db +from superset.models import core as models config = app.config class CsvToDatabaseForm(DynamicForm): + # pylint: disable=E0211 + def all_db_items(): + return db.session.query(models.Database) + name = StringField( _('Table Name'), description=_('Name of table to be created from csv data.'), @@ -28,12 +34,9 @@ class CsvToDatabaseForm(DynamicForm): description=_('Select a CSV file to be uploaded to a database.'), validators=[ FileRequired(), FileAllowed(['csv'], _('CSV Files Only!'))]) - - con = SelectField( - _('Database'), - description=_('database in which to add above table.'), - validators=[DataRequired()], - choices=[]) + con = QuerySelectField( + query_factory=all_db_items, + get_pk=lambda a: a.id, get_label=lambda a: a.database_name) sep = StringField( _('Delimiter'), description=_('Delimiter used by CSV file (for whitespace use \s+).'), @@ -49,7 +52,6 @@ class CsvToDatabaseForm(DynamicForm): ('fail', _('Fail')), ('replace', _('Replace')), ('append', _('Append'))], validators=[DataRequired()]) - schema = StringField( _('Schema'), description=_('Specify a schema (if database flavour supports this).'), diff --git a/superset/views/core.py b/superset/views/core.py index ccf6b484bdab0..41effd9bc27ad 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -24,10 +24,11 @@ from flask_babel import gettext as __ from flask_babel import lazy_gettext as _ import pandas as pd +from six import text_type import sqlalchemy as sqla from sqlalchemy import create_engine from sqlalchemy.engine.url import make_url -from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import IntegrityError, OperationalError from unidecode import unidecode from werkzeug.routing import BaseConverter from werkzeug.utils import secure_filename @@ -163,8 +164,6 @@ def apply(self, query, func): # noqa return query - - class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin): # noqa datamodel = SQLAInterface(models.Database) @@ -319,49 +318,36 @@ def form_get(self, form): form.infer_datetime_format.data = True form.decimal.data = '.' form.if_exists.data = 'append' - all_datasources = ( - db.session.query( - models.Database.sqlalchemy_uri, - models.Database.database_name) - .all() - ) - form.con.choices += all_datasources def form_post(self, form): - def _upload_file(csv_file): - if csv_file and csv_file.filename: - filename = secure_filename(csv_file.filename) - csv_file.save(os.path.join(config['UPLOAD_FOLDER'], filename)) - return filename - csv_file = form.csv_file.data - _upload_file(csv_file) - table = SqlaTable(table_name=form.name.data) - database = ( - db.session.query(models.Database) - .filter_by(sqlalchemy_uri=form.data.get('con')) - .one() - ) - table.database = database - table.database_id = database.id + form.csv_file.data.filename = secure_filename(form.csv_file.data.filename) + csv_filename = form.csv_file.data.filename try: - database.db_engine_spec.create_table_from_csv(form, table) + csv_file.save(os.path.join(config['UPLOAD_FOLDER'], csv_filename)) + table = SqlaTable(table_name=form.name.data) + table.database = form.data.get('con') + table.database_id = table.database.id + table.database.db_engine_spec.create_table_from_csv(form, table) except Exception as e: - os.remove(os.path.join(config['UPLOAD_FOLDER'], csv_file.filename)) - flash(e, 'error') - return redirect('/tablemodelview/list/') + try: + os.remove(os.path.join(config['UPLOAD_FOLDER'], csv_filename)) + except OSError: + pass + message = u'Table name {} already exists. Please pick another'.format( + form.name.data) if isinstance(e, IntegrityError) else text_type(e) + flash( + message, + 'danger') + return redirect('/csvtodatabaseview/form') - os.remove(os.path.join(config['UPLOAD_FOLDER'], csv_file.filename)) + os.remove(os.path.join(config['UPLOAD_FOLDER'], csv_filename)) # Go back to welcome page / splash screen - db_name = ( - db.session.query(models.Database.database_name) - .filter_by(sqlalchemy_uri=form.data.get('con')) - .one() - ) - message = _('CSV file "{0}" uploaded to table "{1}" in ' - 'database "{2}"'.format(form.csv_file.data.filename, + db_name = table.database.database_name + message = _(u'CSV file "{0}" uploaded to table "{1}" in ' + 'database "{2}"'.format(csv_filename, form.name.data, - db_name[0])) + db_name)) flash(message, 'info') return redirect('/tablemodelview/list/') diff --git a/tests/core_tests.py b/tests/core_tests.py index 8415465ae6d86..9cb48b042f3e2 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -804,20 +804,22 @@ def test_import_csv(self): test_file.write('john,1\n') test_file.write('paul,2\n') test_file.close() - main_db_uri = db.session.query( - models.Database.sqlalchemy_uri)\ - .filter_by(database_name='main').all() + main_db_uri = ( + db.session.query(models.Database) + .filter_by(database_name='main') + .all() + ) test_file = open(filename, 'rb') form_data = { 'csv_file': test_file, 'sep': ',', 'name': table_name, - 'con': main_db_uri[0][0], + 'con': main_db_uri[0].id, 'if_exists': 'append', 'index_label': 'test_label', - 'mangle_dupe_cols': False} - + 'mangle_dupe_cols': False, + } url = '/databaseview/list/' add_datasource_page = self.get_resp(url) assert 'Upload a CSV' in add_datasource_page