1. fix check filters change logic (apache#4339)

2. should show chart after loading completed
graceguo-supercat · Feb 5, 2018 · e965f95 · e965f95
1 parent ad21227
commit e965f95
Show file tree

Hide file tree

Showing 8 changed files with 177 additions and 27 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -200,6 +200,7 @@ div.widget {
     .stack-trace-container,
     .slice_container {
       opacity: 0.5;
+      position: relative;
     }
   }
 }