[Dashboard] fix a filter refresh bug and add Test (#3967)

superset/assets/javascripts/dashboard/components/CodeModal.jsx

@@ -21,7 +21,7 @@ export default class CodeModal extends React.PureComponent {
   }
   beforeOpen() {
     let code = this.props.code;
-    if (this.props.codeCallback) {
+    if (!code && this.props.codeCallback) {
       code = this.props.codeCallback();
     }
     this.setState({ code });

superset/assets/javascripts/dashboard/components/Controls.jsx

@@ -13,12 +13,12 @@ const $ = window.$ = require('jquery');
 
 const propTypes = {
   dashboard: PropTypes.object.isRequired,
+  filters: PropTypes.object.isRequired,
   slices: PropTypes.array,
   userId: PropTypes.string.isRequired,
   addSlicesToDashboard: PropTypes.func,
   onSave: PropTypes.func,
   onChange: PropTypes.func,
-  readFilters: PropTypes.func,
   renderSlices: PropTypes.func,
   serialize: PropTypes.func,
   startPeriodicRender: PropTypes.func,
@@ -92,8 +92,8 @@ class Controls extends React.PureComponent {
     this.props.onChange();
   }
   render() {
-    const { dashboard, userId,
-      addSlicesToDashboard, startPeriodicRender, readFilters,
+    const { dashboard, userId, filters,
+      addSlicesToDashboard, startPeriodicRender,
       serialize, onSave, editMode } = this.props;
     const emailBody = t('Checkout this dashboard: %s', window.location.href);
     const emailLink = 'mailto:?Subject=Superset%20Dashboard%20'
@@ -123,7 +123,7 @@ class Controls extends React.PureComponent {
           />
           <SaveModal
             dashboard={dashboard}
-            readFilters={readFilters}
+            filters={filters}
             serialize={serialize}
             onSave={onSave}
             css={this.state.css}

superset/assets/javascripts/dashboard/components/Dashboard.jsx

@@ -4,7 +4,6 @@ import PropTypes from 'prop-types';
 import AlertsWrapper from '../../components/AlertsWrapper';
 import GridLayout from './GridLayout';
 import Header from './Header';
-import { getExploreUrl } from '../../explore/exploreUtils';
 import { areObjectsEqual } from '../../reduxUtils';
 import { t } from '../../locales';
 
@@ -30,6 +29,7 @@ const defaultProps = {
   slices: {},
   datasources: {},
   filters: {},
+  refresh: false,
   timeout: 60,
   userId: '',
   isStarred: false,
@@ -50,7 +50,6 @@ class Dashboard extends React.PureComponent {
     this.onSave = this.onSave.bind(this);
     this.onChange = this.onChange.bind(this);
     this.serialize = this.serialize.bind(this);
-    this.readFilters = this.readFilters.bind(this);
     this.fetchAllSlices = this.fetchSlices.bind(this, this.getAllSlices());
     this.startPeriodicRender = this.startPeriodicRender.bind(this);
     this.addSlicesToDashboard = this.addSlicesToDashboard.bind(this);
@@ -69,14 +68,18 @@ class Dashboard extends React.PureComponent {
   }
 
   componentDidMount() {
-    this.loadPreSelectFilters();
     this.firstLoad = false;
     window.addEventListener('resize', this.rerenderCharts);
   }
 
   componentDidUpdate(prevProps) {
     if (!areObjectsEqual(prevProps.filters, this.props.filters) && this.props.refresh) {
-      Object.keys(this.props.filters).forEach(sliceId => (this.refreshExcept(sliceId)));
+      const currentFilterKeys = Object.keys(this.props.filters);
+      if (currentFilterKeys.length) {
+        currentFilterKeys.forEach(key => (this.refreshExcept(key)));
+      } else {
+        this.refreshExcept();
+      }
     }
   }
 
@@ -160,31 +163,15 @@ class Dashboard extends React.PureComponent {
     return f;
   }
 
-  jsonEndpoint(data, force = false) {
-    let endpoint = getExploreUrl(data, 'json', force);
-    if (endpoint.charAt(0) !== '/') {
-      // Known issue for IE <= 11:
-      // https://connect.microsoft.com/IE/feedbackdetail/view/1002846/pathname-incorrect-for-out-of-document-elements
-      endpoint = '/' + endpoint;
-    }
-    return endpoint;
-  }
-
-  loadPreSelectFilters() {
-    for (const key in this.props.filters) {
-      for (const col in this.props.filters[key]) {
-        const sliceId = parseInt(key, 10);
-        this.props.actions.addFilter(sliceId, col,
-          this.props.filters[key][col], false, false,
-        );
-      }
-    }
-  }
-
-  refreshExcept(sliceId) {
+  refreshExcept(filterKey) {
     const immune = this.props.dashboard.metadata.filter_immune_slices || [];
-    const slices = this.getAllSlices()
-      .filter(slice => slice.slice_id !== sliceId && immune.indexOf(slice.slice_id) === -1);
+    let slices = this.getAllSlices();
+    if (filterKey) {
+      slices = slices.filter(slice => (
+        String(slice.slice_id) !== filterKey &&
+        immune.indexOf(slice.slice_id) === -1
+      ));
+    }
     this.fetchSlices(slices);
   }
 
@@ -213,11 +200,6 @@ class Dashboard extends React.PureComponent {
     fetchAndRender();
   }
 
-  readFilters() {
-    // Returns a list of human readable active filters
-    return JSON.stringify(this.props.filters, null, '  ');
-  }
-
   updateDashboardTitle(title) {
     this.props.actions.updateDashboardTitle(title);
     this.onChange();
@@ -280,13 +262,13 @@ class Dashboard extends React.PureComponent {
           <Header
             dashboard={this.props.dashboard}
             unsavedChanges={this.state.unsavedChanges}
+            filters={this.props.filters}
             userId={this.props.userId}
             isStarred={this.props.isStarred}
             updateDashboardTitle={this.updateDashboardTitle}
             onSave={this.onSave}
             onChange={this.onChange}
             serialize={this.serialize}
-            readFilters={this.readFilters}
             fetchFaveStar={this.props.actions.fetchFaveStar}
             saveFaveStar={this.props.actions.saveFaveStar}
             renderSlices={this.fetchAllSlices}

superset/assets/javascripts/dashboard/components/DashboardContainer.jsx

@@ -13,7 +13,7 @@ function mapStateToProps({ charts, dashboard }) {
     slices: charts,
     datasources: dashboard.datasources,
     filters: dashboard.filters,
-    refresh: dashboard.refresh,
+    refresh: !!dashboard.refresh,
     userId: dashboard.userId,
     isStarred: !!dashboard.isStarred,
     editMode: dashboard.editMode,

superset/assets/javascripts/dashboard/components/Header.jsx

@@ -10,13 +10,13 @@ import { t } from '../../locales';
 
 const propTypes = {
   dashboard: PropTypes.object.isRequired,
+  filters: PropTypes.object.isRequired,
   userId: PropTypes.string.isRequired,
   isStarred: PropTypes.bool,
   addSlicesToDashboard: PropTypes.func,
   onSave: PropTypes.func,
   onChange: PropTypes.func,
   fetchFaveStar: PropTypes.func,
-  readFilters: PropTypes.func,
   renderSlices: PropTypes.func,
   saveFaveStar: PropTypes.func,
   serialize: PropTypes.func,
@@ -95,11 +95,11 @@ class Header extends React.PureComponent {
           {this.renderEditButton()}
           <Controls
             dashboard={dashboard}
+            filters={this.props.filters}
             userId={this.props.userId}
             addSlicesToDashboard={this.props.addSlicesToDashboard}
             onSave={this.props.onSave}
             onChange={this.props.onChange}
-            readFilters={this.props.readFilters}
             renderSlices={this.props.renderSlices}
             serialize={this.props.serialize}
             startPeriodicRender={this.props.startPeriodicRender}

superset/assets/javascripts/dashboard/components/SaveModal.jsx

@@ -13,7 +13,7 @@ const propTypes = {
   css: PropTypes.string,
   dashboard: PropTypes.object.isRequired,
   triggerNode: PropTypes.node.isRequired,
-  readFilters: PropTypes.func,
+  filters: PropTypes.object.isRequired,
   serialize: PropTypes.func,
   onSave: PropTypes.func,
 };
@@ -81,7 +81,7 @@ class SaveModal extends React.PureComponent {
       css: this.state.css,
       expanded_slices: dashboard.metadata.expanded_slices || {},
       dashboard_title: dashboard.dashboard_title,
-      default_filters: this.props.readFilters(),
+      default_filters: JSON.stringify(this.props.filters),
       duplicate_slices: this.state.duplicateSlices,
     };
     let url = null;

superset/assets/javascripts/dashboard/reducers.js

@@ -14,14 +14,10 @@ export function getInitialState(bootstrapData) {
   delete common.language_pack;
 
   const dashboard = { ...bootstrapData.dashboard_data };
-  const filters = {};
+  let filters = {};
   try {
     // allow request parameter overwrite dashboard metadata
-    const filterData = JSON.parse(getParam('preselect_filters') || dashboard.metadata.default_filters);
-    for (const key in filterData) {
-      const sliceId = parseInt(key, 10);
-      filters[sliceId] = filterData[key];
-    }
+    filters = JSON.parse(getParam('preselect_filters') || dashboard.metadata.default_filters);
   } catch (e) {
     //
   }
@@ -87,7 +83,7 @@ export function getInitialState(bootstrapData) {
   };
 }
 
-const dashboard = function (state = {}, action) {
+export const dashboard = function (state = {}, action) {
   const actionHandlers = {
     [actions.UPDATE_DASHBOARD_TITLE]() {
       const newDashboard = { ...state.dashboard, dashboard_title: action.title };
@@ -98,11 +94,22 @@ const dashboard = function (state = {}, action) {
       return { ...state, dashboard: newDashboard };
     },
     [actions.REMOVE_SLICE]() {
-      const newLayout = state.dashboard.layout.filter(function (reactPos) {
-        return reactPos.i !== String(action.slice.slice_id);
-      });
+      const key = String(action.slice.slice_id);
+      const newLayout = state.dashboard.layout.filter(reactPos => (reactPos.i !== key));
       const newDashboard = removeFromArr(state.dashboard, 'slices', action.slice, 'slice_id');
-      return { ...state, dashboard: { ...newDashboard, layout: newLayout } };
+      // if this slice is a filter
+      const newFilter = { ...state.filters };
+      let refresh = false;
+      if (state.filters[key]) {
+        delete newFilter[key];
+        refresh = true;
+      }
+      return {
+        ...state,
+        dashboard: { ...newDashboard, layout: newLayout },
+        filters: newFilter,
+        refresh,
+      };
     },
     [actions.TOGGLE_FAVE_STAR]() {
       return { ...state, isStarred: action.isStarred };
@@ -158,8 +165,7 @@ const dashboard = function (state = {}, action) {
     [actions.CLEAR_FILTER]() {
       const newFilters = { ...state.filters };
       delete newFilters[action.sliceId];
-
-      return { ...state.dashboard, filter: newFilters, refresh: true };
+      return { ...state, filter: newFilters, refresh: true };
     },
     [actions.REMOVE_FILTER]() {
       const newFilters = { ...state.filters };
@@ -176,7 +182,7 @@ const dashboard = function (state = {}, action) {
           newFilters[sliceId][col] = a;
         }
       }
-      return { ...state.dashboard, filter: newFilters, refresh: true };
+      return { ...state, filter: newFilters, refresh: true };
     },
 
     // slice reducer
@@ -185,7 +191,7 @@ const dashboard = function (state = {}, action) {
         state.dashboard, 'slices',
         action.slice, { slice_name: action.sliceName },
         'slice_id');
-      return { ...state.dashboard, dashboard: newDashboard };
+      return { ...state, dashboard: newDashboard };
     },
   };
 

superset/assets/spec/helpers/browser.js

@@ -37,4 +37,5 @@ global.assert = chai.assert;
 global.sinon.useFakeXMLHttpRequest();
 
 global.window.XMLHttpRequest = global.XMLHttpRequest;
+global.window.location = { href: 'about:blank' };
 global.$ = require('jquery')(global.window);

superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx

@@ -0,0 +1,89 @@
+import React from 'react';
+import { shallow } from 'enzyme';
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+import sinon from 'sinon';
+
+import * as dashboardActions from '../../../javascripts/dashboard/actions';
+import * as chartActions from '../../../javascripts/chart/chartAction';
+import Dashboard from '../../../javascripts/dashboard/components/Dashboard';
+import { defaultFilters, dashboard, charts } from './fixtures';
+
+describe('Dashboard', () => {
+  const mockedProps = {
+    actions: { ...chartActions, ...dashboardActions },
+    initMessages: [],
+    dashboard: dashboard.dashboard,
+    slices: charts,
+    filters: dashboard.filters,
+    datasources: dashboard.datasources,
+    refresh: false,
+    timeout: 60,
+    isStarred: false,
+    userId: dashboard.userId,
+  };
+
+  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);
+  });
+
+  it('should handle metadata default_filters', () => {
+    const wrapper = shallow(<Dashboard {...mockedProps} />);
+    expect(wrapper.instance().props.filters).deep.equal(defaultFilters);
+  });
+
+  describe('getFormDataExtra', () => {
+    let wrapper;
+    let selectedSlice;
+    beforeEach(() => {
+      wrapper = shallow(<Dashboard {...mockedProps} />);
+      selectedSlice = wrapper.instance().props.dashboard.slices[1];
+    });
+
+    it('should carry default_filters', () => {
+      const extraFilters = wrapper.instance().getFormDataExtra(selectedSlice).extra_filters;
+      expect(extraFilters[0]).to.deep.equal({ col: 'region', op: 'in', val: [] });
+      expect(extraFilters[1]).to.deep.equal({ col: 'country_name', op: 'in', val: ['United States'] });
+    });
+
+    it('should carry updated filter', () => {
+      wrapper.setProps({
+        filters: {
+          256: {
+            region: [],
+            country_name: ['France'],
+          },
+        },
+      });
+      const extraFilters = wrapper.instance().getFormDataExtra(selectedSlice).extra_filters;
+      expect(extraFilters[1]).to.deep.equal({ col: 'country_name', op: 'in', val: ['France'] });
+    });
+  });
+
+  describe('refreshExcept', () => {
+    let wrapper;
+    let spy;
+    beforeEach(() => {
+      wrapper = shallow(<Dashboard {...mockedProps} />);
+      spy = sinon.spy(wrapper.instance(), 'fetchSlices');
+    });
+    afterEach(() => {
+      spy.restore();
+    });
+
+    it('should not refresh filter slice', () => {
+      const filterKey = Object.keys(defaultFilters)[0];
+      wrapper.instance().refreshExcept(filterKey);
+      expect(spy.callCount).to.equal(1);
+      expect(spy.getCall(0).args[0].length).to.equal(1);
+    });
+
+    it('should refresh all slices', () => {
+      wrapper.instance().refreshExcept();
+      expect(spy.callCount).to.equal(1);
+      expect(spy.getCall(0).args[0].length).to.equal(2);
+    });
+  });
+});

superset/assets/spec/javascripts/dashboard/fixtures.jsx

@@ -1,70 +1,134 @@
-export const slice = {
-  token: 'token_089ec8c1',
-  csv_endpoint: '',
-  edit_url: '/slicemodelview/edit/39',
-  viz_name: 'filter_box',
-  json_endpoint: '',
-  slice_id: 39,
-  standalone_endpoint: '',
-  description_markeddown: '',
-  form_data: {
-    collapsed_fieldsets: null,
-    time_grain_sqla: 'Time Column',
-    granularity_sqla: 'year',
-    standalone: null,
-    date_filter: false,
-    until: '2014-01-02',
-    extra_filters: null,
-    force: null,
-    where: '',
-    since: '2014-01-01',
-    async: null,
-    slice_id: null,
-    json: null,
-    having: '',
-    flt_op_2: 'in',
-    previous_viz_type: 'filter_box',
-    groupby: [
-      'region',
-      'country_name',
-    ],
-    flt_col_7: '',
-    slice_name: null,
-    viz_type: 'filter_box',
-    metric: 'sum__SP_POP_TOTL',
-    flt_col_8: '',
+import { getInitialState } from '../../../javascripts/dashboard/reducers';
+
+export const defaultFilters = {
+  256: {
+    region: [],
+    country_name: ['United States'],
   },
-  slice_url: '',
-  slice_name: 'Region Filter',
-  description: null,
-  column_formats: {},
 };
-export const dashboardData = {
+export const regionFilter = {
+  datasource: null,
+  description: null,
+  description_markeddown: '',
+  edit_url: '/slicemodelview/edit/256',
+  form_data: {
+    datasource: '2__table',
+    date_filter: false,
+    filters: [{
+      col: 'country_name',
+      op: 'in',
+      val: ['United States', 'France', 'Japan'],
+    }],
+    granularity_sqla: null,
+    groupby: ['region', '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: 256,
+    time_grain_sqla: null,
+    until: 'now',
+    viz_type: 'filter_box',
+    where: '',
+  },
+  slice_id: 256,
+  slice_name: 'Region Filters',
+  slice_url: '/superset/explore/table/2/?form_data=%7B%22slice_id%22%3A%20256%7D',
+};
+export const slice = {
+  datasource: null,
+  description: null,
+  description_markeddown: '',
+  edit_url: '/slicemodelview/edit/248',
+  form_data: {
+    annotation_layers: [],
+    bottom_margin: 'auto',
+    color_scheme: 'bnbColors',
+    contribution: false,
+    datasource: '2__table',
+    filters: [],
+    granularity_sqla: null,
+    groupby: [],
+    having: '',
+    left_margin: 'auto',
+    limit: 50,
+    line_interpolation: 'linear',
+    metrics: ['sum__SP_POP_TOTL'],
+    num_period_compare: '',
+    order_desc: true,
+    period_ratio_type: 'growth',
+    resample_fillmethod: null,
+    resample_how: null,
+    resample_rule: null,
+    rich_tooltip: true,
+    rolling_type: 'None',
+    show_brush: false,
+    show_legend: true,
+    show_markers: false,
+    since: '1961-01-01T00:00:00',
+    slice_id: 248,
+    time_compare: null,
+    time_grain_sqla: null,
+    timeseries_limit_metric: null,
+    until: '2014-12-31T00:00:00',
+    viz_type: 'line',
+    where: '',
+    x_axis_format: 'smart_date',
+    x_axis_label: '',
+    x_axis_showminmax: true,
+    y_axis_bounds: [null, null],
+    y_axis_format: '.3s',
+    y_axis_label: '',
+    y_axis_showminmax: true,
+    y_log_scale: false,
+  },
+  slice_id: 248,
+  slice_name: 'Filtered Population',
+  slice_url: '/superset/explore/table/2/?form_data=%7B%22slice_id%22%3A%20248%7D',
+};
+
+const datasources = {};
+const mockDashboardData = {
   css: '',
+  dash_edit_perm: true,
+  dash_save_perm: true,
+  dashboard_title: 'Births',
+  id: 2,
   metadata: {
+    default_filters: JSON.stringify(defaultFilters),
     filter_immune_slices: [],
     timed_refresh_immune_slices: [],
     filter_immune_slice_fields: {},
     expanded_slices: {},
   },
-  slug: 'births',
   position_json: [
     {
-      size_x: 2,
-      slice_id: '52',
+      size_x: 4,
+      slice_id: '256',
       row: 0,
-      size_y: 2,
+      size_y: 4,
+      col: 5,
+    },
+    {
+      size_x: 4,
+      slice_id: '248',
+      row: 0,
+      size_y: 4,
       col: 1,
     },
   ],
-  id: 2,
-  slices: [slice],
-  dashboard_title: 'Births',
-};
-
-export const contextData = {
-  dash_save_perm: true,
+  slug: 'births',
+  slices: [regionFilter, slice],
   standalone_mode: false,
-  dash_edit_perm: true,
-  userId: '1',
 };
+export const { dashboard, charts } = getInitialState({
+  common: {},
+  dashboard_data: mockDashboardData,
+  datasources,
+  user_id: '1',
+});
+

superset/assets/spec/javascripts/dashboard/reducers_spec.js

@@ -0,0 +1,30 @@
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+
+import { dashboard as reducers } from '../../../javascripts/dashboard/reducers';
+import * as actions from '../../../javascripts/dashboard/actions';
+import { defaultFilters, dashboard as initState } from './fixtures';
+
+describe('Dashboard reducers', () => {
+  it('should remove slice', () => {
+    const action = {
+      type: actions.REMOVE_SLICE,
+      slice: initState.dashboard.slices[1],
+    };
+    const { dashboard, filters, refresh } = reducers(initState, action);
+    expect(dashboard.slices).to.have.length(1);
+    expect(filters).to.deep.equal(defaultFilters);
+    expect(refresh).to.equal(false);
+  });
+
+  it('should remove filter slice', () => {
+    const action = {
+      type: actions.REMOVE_SLICE,
+      slice: initState.dashboard.slices[0],
+    };
+    const { dashboard, filters, refresh } = reducers(initState, action);
+    expect(dashboard.slices).to.have.length(1);
+    expect(filters).to.deep.equal({});
+    expect(refresh).to.equal(true);
+  });
+});