From 4ed6382e143d57762c648a9ec9b5e98c13b0b181 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Fri, 1 Dec 2017 10:58:55 -0800 Subject: [PATCH] [Dashboard] fix a filter refresh bug and add Test (#3967) --- .../dashboard/components/CodeModal.jsx | 2 +- .../dashboard/components/Controls.jsx | 8 +- .../dashboard/components/Dashboard.jsx | 50 ++---- .../components/DashboardContainer.jsx | 2 +- .../dashboard/components/Header.jsx | 4 +- .../dashboard/components/SaveModal.jsx | 4 +- .../assets/javascripts/dashboard/reducers.js | 36 ++-- superset/assets/spec/helpers/browser.js | 1 + .../javascripts/dashboard/Dashboard_spec.jsx | 89 ++++++++++ .../spec/javascripts/dashboard/fixtures.jsx | 158 ++++++++++++------ .../javascripts/dashboard/reducers_spec.js | 30 ++++ 11 files changed, 278 insertions(+), 106 deletions(-) create mode 100644 superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx create mode 100644 superset/assets/spec/javascripts/dashboard/reducers_spec.js diff --git a/superset/assets/javascripts/dashboard/components/CodeModal.jsx b/superset/assets/javascripts/dashboard/components/CodeModal.jsx index f9c1535a7708a..0e84ad1bab606 100644 --- a/superset/assets/javascripts/dashboard/components/CodeModal.jsx +++ b/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 }); diff --git a/superset/assets/javascripts/dashboard/components/Controls.jsx b/superset/assets/javascripts/dashboard/components/Controls.jsx index ead2c9af6d1e5..caf04ca46b951 100644 --- a/superset/assets/javascripts/dashboard/components/Controls.jsx +++ b/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 { /> (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 {
(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 }; }, }; diff --git a/superset/assets/spec/helpers/browser.js b/superset/assets/spec/helpers/browser.js index a74ce3ef5ccc4..638a63dcc8d6d 100644 --- a/superset/assets/spec/helpers/browser.js +++ b/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); diff --git a/superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx b/superset/assets/spec/javascripts/dashboard/Dashboard_spec.jsx new file mode 100644 index 0000000000000..95f013f115080 --- /dev/null +++ b/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(); + 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(); + expect(wrapper.instance().props.filters).deep.equal(defaultFilters); + }); + + describe('getFormDataExtra', () => { + let wrapper; + let selectedSlice; + beforeEach(() => { + wrapper = shallow(); + 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(); + 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); + }); + }); +}); diff --git a/superset/assets/spec/javascripts/dashboard/fixtures.jsx b/superset/assets/spec/javascripts/dashboard/fixtures.jsx index be515e39a8ef0..7b95e7c65533c 100644 --- a/superset/assets/spec/javascripts/dashboard/fixtures.jsx +++ b/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: '', +import { getInitialState } from '../../../javascripts/dashboard/reducers'; + +export const defaultFilters = { + 256: { + region: [], + country_name: ['United States'], + }, +}; +export const regionFilter = { + datasource: null, + description: null, description_markeddown: '', + edit_url: '/slicemodelview/edit/256', form_data: { - collapsed_fieldsets: null, - time_grain_sqla: 'Time Column', - granularity_sqla: 'year', - standalone: null, + datasource: '2__table', date_filter: false, - until: '2014-01-02', - extra_filters: null, - force: null, - where: '', - since: '2014-01-01', - async: null, - slice_id: null, - json: null, + filters: [{ + col: 'country_name', + op: 'in', + val: ['United States', 'France', 'Japan'], + }], + granularity_sqla: null, + groupby: ['region', 'country_name'], having: '', - flt_op_2: 'in', - previous_viz_type: 'filter_box', - groupby: [ - 'region', - 'country_name', - ], - flt_col_7: '', - slice_name: null, - viz_type: 'filter_box', + instant_filtering: true, metric: 'sum__SP_POP_TOTL', - flt_col_8: '', + 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_url: '', - slice_name: 'Region Filter', + 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, - column_formats: {}, + 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', }; -export const dashboardData = { + +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: 4, + col: 5, + }, + { + size_x: 4, + slice_id: '248', row: 0, - size_y: 2, + 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', +}); + diff --git a/superset/assets/spec/javascripts/dashboard/reducers_spec.js b/superset/assets/spec/javascripts/dashboard/reducers_spec.js new file mode 100644 index 0000000000000..17b8f75df54c3 --- /dev/null +++ b/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); + }); +});