diff --git a/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx index afbf578f4..4f61423a3 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/Dashboard_spec.jsx @@ -62,10 +62,10 @@ describe('Dashboard', () => { // activeFilters map use id_column) as key const OVERRIDE_FILTERS = { - '1_region': [], - '2_country_name': ['USA'], - '3_region': [], - '3_country_name': ['USA'], + '1_region': { values: [], scope: [1] }, + '2_country_name': { values: ['USA'], scope: [1, 2] }, + '3_region': { values: [], scope: [1] }, + '3_country_name': { values: ['USA'], scope: [] }, }; it('should render a DashboardBuilder', () => { @@ -143,19 +143,13 @@ describe('Dashboard', () => { it('should call refresh if a filter is added', () => { const newFilter = { - gender: ['boy', 'girl'], + gender: { values: ['boy', 'girl'], scope: [1] }, }; wrapper.setProps({ - activeFilters: { - ...OVERRIDE_FILTERS, - ...newFilter, - }, + activeFilters: newFilter, }); expect(refreshSpy.callCount).toBe(1); - expect(wrapper.instance().appliedFilters).toEqual({ - ...OVERRIDE_FILTERS, - ...newFilter, - }); + expect(wrapper.instance().appliedFilters).toEqual(newFilter); }); it('should call refresh if a filter is removed', () => { @@ -167,17 +161,55 @@ describe('Dashboard', () => { }); it('should call refresh if a filter is changed', () => { + const newFilters = { + ...OVERRIDE_FILTERS, + '1_region': { values: ['Canada'], scope: [1] }, + }; wrapper.setProps({ - activeFilters: { - ...OVERRIDE_FILTERS, - '1_region': ['Canada'], - }, + activeFilters: newFilters, }); expect(refreshSpy.callCount).toBe(1); - expect(wrapper.instance().appliedFilters).toEqual({ + expect(wrapper.instance().appliedFilters).toEqual(newFilters); + expect(refreshSpy.getCall(0).args[0]).toEqual([1]); + }); + + it('should call refresh with multiple chart ids', () => { + const newFilters = { ...OVERRIDE_FILTERS, - '1_region': ['Canada'], + '2_country_name': { values: ['New Country'], scope: [1, 2] }, + }; + wrapper.setProps({ + activeFilters: newFilters, }); + expect(refreshSpy.callCount).toBe(1); + expect(wrapper.instance().appliedFilters).toEqual(newFilters); + expect(refreshSpy.getCall(0).args[0]).toEqual([1, 2]); + }); + + it('should call refresh if a filter scope is changed', () => { + const newFilters = { + ...OVERRIDE_FILTERS, + '3_country_name': { values: ['USA'], scope: [2] }, + }; + + wrapper.setProps({ + activeFilters: newFilters, + }); + expect(refreshSpy.callCount).toBe(1); + expect(refreshSpy.getCall(0).args[0]).toEqual([2]); + }); + + it('should call refresh with empty [] if a filter is changed but scope is not applicable', () => { + const newFilters = { + ...OVERRIDE_FILTERS, + '3_country_name': { values: ['CHINA'], scope: [] }, + }; + + wrapper.setProps({ + activeFilters: newFilters, + }); + expect(refreshSpy.callCount).toBe(1); + expect(refreshSpy.getCall(0).args[0]).toEqual([]); }); }); }); diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx index 34e0f125e..e870c9f7c 100644 --- a/superset-frontend/src/dashboard/components/Dashboard.jsx +++ b/superset-frontend/src/dashboard/components/Dashboard.jsx @@ -145,31 +145,7 @@ class Dashboard extends React.PureComponent { const { activeFilters } = this.props; // do not apply filter when dashboard in edit mode if (!editMode && !areObjectsEqual(appliedFilters, activeFilters)) { - // refresh charts if a filter was removed, added, or changed - const currFilterKeys = Object.keys(activeFilters); - const appliedFilterKeys = Object.keys(appliedFilters); - - const allKeys = new Set(currFilterKeys.concat(appliedFilterKeys)); - const affectedChartIds = []; - [...allKeys].forEach(filterKey => { - if (!currFilterKeys.includes(filterKey)) { - // removed filter? - [].push.apply(affectedChartIds, appliedFilters[filterKey].scope); - } else if (!appliedFilterKeys.includes(filterKey)) { - // added filter? - [].push.apply(affectedChartIds, activeFilters[filterKey].scope); - } else { - // changed filter field value or scope? - const affectedScope = (activeFilters[filterKey].scope || []).concat( - appliedFilters[filterKey].scope || [], - ); - [].push.apply(affectedChartIds, affectedScope); - } - }); - - const idSet = new Set(affectedChartIds); - this.refreshCharts([...idSet]); - this.appliedFilters = activeFilters; + this.applyFilters(); } if (hasUnsavedChanges) { @@ -205,6 +181,56 @@ class Dashboard extends React.PureComponent { return Object.values(this.props.charts); } + applyFilters() { + const appliedFilters = this.appliedFilters; + const { activeFilters } = this.props; + + // refresh charts if a filter was removed, added, or changed + const currFilterKeys = Object.keys(activeFilters); + const appliedFilterKeys = Object.keys(appliedFilters); + + const allKeys = new Set(currFilterKeys.concat(appliedFilterKeys)); + const affectedChartIds = []; + [...allKeys].forEach(filterKey => { + if (!currFilterKeys.includes(filterKey)) { + // filterKey is removed? + affectedChartIds.push(...appliedFilters[filterKey].scope); + } else if (!appliedFilterKeys.includes(filterKey)) { + // filterKey is newly added? + affectedChartIds.push(...activeFilters[filterKey].scope); + } else { + // if filterKey changes value, + // update charts in its scope + if ( + !areObjectsEqual( + appliedFilters[filterKey].values, + activeFilters[filterKey].values, + ) + ) { + affectedChartIds.push(...activeFilters[filterKey].scope); + } + + // if filterKey changes scope, + // update all charts in its scope + if ( + !areObjectsEqual( + appliedFilters[filterKey].scope, + activeFilters[filterKey].scope, + ) + ) { + const chartsInScope = (activeFilters[filterKey].scope || []).concat( + appliedFilters[filterKey].scope || [], + ); + affectedChartIds.push(...chartsInScope); + } + } + }); + + // remove dup in affectedChartIds + this.refreshCharts([...new Set(affectedChartIds)]); + this.appliedFilters = activeFilters; + } + refreshCharts(ids) { ids.forEach(id => { this.props.actions.triggerQuery(true, id);