graceguo-supercat closed pull request #4339: [bug] fix JS error when
interactive with loading filter_box
URL: https://github.com/apache/incubator-superset/pull/4339
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/superset/assets/javascripts/chart/Chart.jsx
b/superset/assets/javascripts/chart/Chart.jsx
index 111c48ee24..7bb71aca47 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 f12b9ab707..416e770295 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 c99c293604..bcc13778d2 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 f4754f477c..9d45e85c90 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 95f013f115..1ac495992a 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 7b95e7c655..1b9cf21b62 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 17b8f75df5..8022928ddf 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 c5a8ea7d45..488adebd5e 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;
}
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
