You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by gr...@apache.org on 2018/02/05 18:21:21 UTC
[incubator-superset] branch master updated: 1. fix check filters
change logic (#4339)
This is an automated email from the ASF dual-hosted git repository.
graceguo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new e965f95 1. fix check filters change logic (#4339)
e965f95 is described below
commit e965f954775c702703f1a0878fba04aa54a73159
Author: Grace Guo <gr...@airbnb.com>
AuthorDate: Mon Feb 5 10:21:17 2018 -0800
1. fix check filters change logic (#4339)
2. should show chart after loading completed
---
superset/assets/javascripts/chart/Chart.jsx | 2 +-
superset/assets/javascripts/components/Loading.jsx | 1 +
.../javascripts/dashboard/components/Dashboard.jsx | 21 ++--
.../assets/spec/javascripts/chart/Chart_spec.jsx | 22 ++++-
.../spec/javascripts/dashboard/Dashboard_spec.jsx | 107 +++++++++++++++++++--
.../assets/spec/javascripts/dashboard/fixtures.jsx | 39 ++++++--
.../spec/javascripts/dashboard/reducers_spec.js | 11 ++-
superset/assets/stylesheets/superset.less | 1 +
8 files changed, 177 insertions(+), 27 deletions(-)
diff --git a/superset/assets/javascripts/chart/Chart.jsx b/superset/assets/javascripts/chart/Chart.jsx
index 111c48e..7bb71ac 100644
--- a/superset/assets/javascripts/chart/Chart.jsx
+++ b/superset/assets/javascripts/chart/Chart.jsx
@@ -207,7 +207,7 @@ class Chart extends React.PureComponent {
/>
}
- {!this.props.chartAlert &&
+ {!isLoading && !this.props.chartAlert &&
<ChartBody
containerId={this.containerId}
vizType={this.props.vizType}
diff --git a/superset/assets/javascripts/components/Loading.jsx b/superset/assets/javascripts/components/Loading.jsx
index f12b9ab..416e770 100644
--- a/superset/assets/javascripts/components/Loading.jsx
+++ b/superset/assets/javascripts/components/Loading.jsx
@@ -19,6 +19,7 @@ export default function Loading(props) {
height: props.size,
padding: 0,
margin: 0,
+ position: 'absolute',
}}
/>
);
diff --git a/superset/assets/javascripts/dashboard/components/Dashboard.jsx b/superset/assets/javascripts/dashboard/components/Dashboard.jsx
index c99c293..bcc1377 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/spec/javascripts/chart/Chart_spec.jsx b/superset/assets/spec/javascripts/chart/Chart_spec.jsx
index f4754f4..9d45e85 100644
--- a/superset/assets/spec/javascripts/chart/Chart_spec.jsx
+++ b/superset/assets/spec/javascripts/chart/Chart_spec.jsx
@@ -6,6 +6,8 @@ import sinon from 'sinon';
import { chart as initChart } from '../../../javascripts/chart/chartReducer';
import Chart from '../../../javascripts/chart/Chart';
+import ChartBody from '../../../javascripts/chart/ChartBody';
+import Loading from '../../../javascripts/components/Loading';
describe('Chart', () => {
const chart = {
@@ -30,15 +32,20 @@ describe('Chart', () => {
},
};
+ let wrapper;
+ beforeEach(() => {
+ wrapper = shallow(
+ <Chart {...mockedProps} />,
+ );
+ });
describe('renderViz', () => {
- let wrapper;
let stub;
beforeEach(() => {
- wrapper = shallow(
- <Chart {...mockedProps} />,
- );
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 95f013f..1ac4959 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(<Dashboard {...mockedProps} />);
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(<Dashboard {...mockedProps} />);
+ 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 7b95e7c..1b9cf21 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 17b8f75..8022928 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/stylesheets/superset.less b/superset/assets/stylesheets/superset.less
index c5a8ea7..488adeb 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;
}
}
}
--
To stop receiving notification emails like this one, please contact
graceguo@apache.org.