From aef6223219ff6ff120f2715abdf9654db44c4c43 Mon Sep 17 00:00:00 2001 From: Liza Katz Date: Wed, 3 Jul 2019 19:50:34 +0300 Subject: [PATCH] Bugfix: Fix TS to avoid assigning filters to undefined (#40249) * Define getAppState TS correctly, to avoid filter assignment into an undefined state * Added tests for the case where appState is undefined --- .../filter_state_manager.test.ts | 149 +++++++++++------- .../filter_manager/filter_state_manager.ts | 12 +- 2 files changed, 103 insertions(+), 58 deletions(-) diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts index c32570500c305..b413efc0ba0f5 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts @@ -58,16 +58,6 @@ describe('filter_state_manager', () => { globalStateStub = new StubState(); const indexPatterns = new StubIndexPatterns(); filterManager = new FilterManager(indexPatterns); - - // FilterStateManager is tested indirectly. - // Therefore, we don't need it's instance. - new FilterStateManager( - globalStateStub, - () => { - return appStateStub; - }, - filterManager - ); }); afterEach(() => { @@ -76,51 +66,102 @@ describe('filter_state_manager', () => { } }); - test('should update filter manager global filters', done => { - const f1 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); - globalStateStub.filters.push(f1); - - setTimeout(() => { - expect(filterManager.getGlobalFilters()).toHaveLength(1); - done(); - }, 100); - }); - - test('should update filter manager app filters', done => { - expect(filterManager.getAppFilters()).toHaveLength(0); - - const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); - appStateStub.filters.push(f1); - - setTimeout(() => { - expect(filterManager.getAppFilters()).toHaveLength(1); - done(); - }, 100); + describe('app_state_undefined', () => { + beforeEach(() => { + // FilterStateManager is tested indirectly. + // Therefore, we don't need it's instance. + new FilterStateManager( + globalStateStub, + () => { + return undefined; + }, + filterManager + ); + }); + + test('should NOT watch state until both app and global state are defined', done => { + const f1 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); + globalStateStub.filters.push(f1); + + setTimeout(() => { + expect(filterManager.getGlobalFilters()).toHaveLength(0); + done(); + }, 100); + }); + + test('should NOT update app URL when filter manager filters are set', async () => { + appStateStub.save = sinon.stub(); + globalStateStub.save = sinon.stub(); + + const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); + const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); + + await filterManager.setFilters([f1, f2]); + + sinon.assert.notCalled(appStateStub.save); + sinon.assert.calledOnce(globalStateStub.save); + }); }); - test('should update URL when filter manager filters are set', async () => { - appStateStub.save = sinon.stub(); - globalStateStub.save = sinon.stub(); - - const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); - const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); - - await filterManager.setFilters([f1, f2]); - - sinon.assert.calledOnce(appStateStub.save); - sinon.assert.calledOnce(globalStateStub.save); - }); - - test('should update URL when filter manager filters are added', async () => { - appStateStub.save = sinon.stub(); - globalStateStub.save = sinon.stub(); - - const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); - const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); - - await filterManager.addFilters([f1, f2]); - - sinon.assert.calledOnce(appStateStub.save); - sinon.assert.calledOnce(globalStateStub.save); + describe('app_state_defined', () => { + beforeEach(() => { + // FilterStateManager is tested indirectly. + // Therefore, we don't need it's instance. + new FilterStateManager( + globalStateStub, + () => { + return appStateStub; + }, + filterManager + ); + }); + + test('should update filter manager global filters', done => { + const f1 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); + globalStateStub.filters.push(f1); + + setTimeout(() => { + expect(filterManager.getGlobalFilters()).toHaveLength(1); + done(); + }, 100); + }); + + test('should update filter manager app filters', done => { + expect(filterManager.getAppFilters()).toHaveLength(0); + + const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); + appStateStub.filters.push(f1); + + setTimeout(() => { + expect(filterManager.getAppFilters()).toHaveLength(1); + done(); + }, 100); + }); + + test('should update URL when filter manager filters are set', async () => { + appStateStub.save = sinon.stub(); + globalStateStub.save = sinon.stub(); + + const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); + const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); + + await filterManager.setFilters([f1, f2]); + + sinon.assert.calledOnce(appStateStub.save); + sinon.assert.calledOnce(globalStateStub.save); + }); + + test('should update URL when filter manager filters are added', async () => { + appStateStub.save = sinon.stub(); + globalStateStub.save = sinon.stub(); + + const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); + const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); + + await filterManager.addFilters([f1, f2]); + + sinon.assert.calledOnce(appStateStub.save); + sinon.assert.calledOnce(globalStateStub.save); + }); }); }); diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts index 4fec67df919f4..032233cbc0a8b 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts @@ -23,6 +23,8 @@ import _ from 'lodash'; import { State } from 'ui/state_management/state'; import { FilterManager } from './filter_manager'; +type GetAppStateFunc = () => State | undefined | null; + /** * FilterStateManager is responsible for watching for filter changes * and syncing with FilterManager, as well as syncing FilterManager changes @@ -31,12 +33,12 @@ import { FilterManager } from './filter_manager'; export class FilterStateManager { filterManager: FilterManager; globalState: State; - getAppState: () => State; + getAppState: GetAppStateFunc; prevGlobalFilters: Filter[] | undefined; prevAppFilters: Filter[] | undefined; interval: NodeJS.Timeout | undefined; - constructor(globalState: State, getAppState: () => State, filterManager: FilterManager) { + constructor(globalState: State, getAppState: GetAppStateFunc, filterManager: FilterManager) { this.getAppState = getAppState; this.globalState = globalState; this.filterManager = filterManager; @@ -63,7 +65,7 @@ export class FilterStateManager { if (stateUndefined) return; const globalFilters = this.globalState.filters || []; - const appFilters = appState.filters || []; + const appFilters = (appState && appState.filters) || []; const globalFilterChanged = !( this.prevGlobalFilters && _.isEqual(this.prevGlobalFilters, globalFilters) @@ -96,7 +98,9 @@ export class FilterStateManager { // Update Angular state before saving State objects (which save it to URL) const partitionedFilters = this.filterManager.getPartitionedFilters(); const appState = this.getAppState(); - appState.filters = partitionedFilters.appFilters; + if (appState) { + appState.filters = partitionedFilters.appFilters; + } this.globalState.filters = partitionedFilters.globalFilters; this.saveState(); }