From a423a049a2c6f83fdbb9105953a1460a924e608e Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 22 Jun 2021 12:52:34 -0700 Subject: [PATCH 1/5] Set up isEngineEmpty and isEngineSchemaEmpty selectors + update selector tests with mock engines to just an `engine` var (I saw Jason do this down below for `searchKey` and liked it, so I copied it) + update Documents & Search UI pages to new selectors --- .../components/documents/documents.test.tsx | 1 - .../components/documents/documents.tsx | 4 +- .../components/engine/engine_logic.test.ts | 102 +++++++++++++----- .../components/engine/engine_logic.ts | 7 ++ .../components/search_ui/search_ui.tsx | 4 +- 5 files changed, 89 insertions(+), 29 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.test.tsx index b5b6dd453c9df1..7e1b2acc81d182 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.test.tsx @@ -22,7 +22,6 @@ import { Documents } from '.'; describe('Documents', () => { const values = { isMetaEngine: false, - engine: { document_count: 1 }, myRole: { canManageEngineDocuments: true }, }; diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.tsx index 62c7759757bda8..75044bfcc8fb7e 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/documents.tsx @@ -21,7 +21,7 @@ import { DOCUMENTS_TITLE } from './constants'; import { SearchExperience } from './search_experience'; export const Documents: React.FC = () => { - const { isMetaEngine, engine } = useValues(EngineLogic); + const { isMetaEngine, isEngineEmpty } = useValues(EngineLogic); const { myRole } = useValues(AppLogic); return ( @@ -32,7 +32,7 @@ export const Documents: React.FC = () => { rightSideItems: myRole.canManageEngineDocuments && !isMetaEngine ? [] : [], }} - isEmptyState={!engine.document_count} + isEmptyState={isEngineEmpty} emptyState={} > {isMetaEngine && ( diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts index 2b60193d4f7d3d..3c3248158efb0d 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts @@ -9,6 +9,7 @@ import { LogicMounter, mockHttpValues } from '../../../__mocks__/kea_logic'; import { nextTick } from '@kbn/test/jest'; +import { SchemaType } from '../../../shared/schema/types'; import { ApiTokenTypes } from '../credentials/constants'; import { EngineTypes } from './types'; @@ -34,7 +35,7 @@ describe('EngineLogic', () => { sample: false, isMeta: false, invalidBoosts: false, - schema: {}, + schema: { test: SchemaType.Text }, apiTokens: [], apiKey: 'some-key', }; @@ -43,6 +44,8 @@ describe('EngineLogic', () => { dataLoading: true, engine: {}, engineName: '', + isEngineEmpty: true, + isEngineSchemaEmpty: true, isMetaEngine: false, isSampleEngine: false, hasSchemaErrors: false, @@ -52,6 +55,13 @@ describe('EngineLogic', () => { searchKey: '', }; + const DEFAULT_VALUES_WITH_ENGINE = { + ...DEFAULT_VALUES, + engine: mockEngineData, + isEngineEmpty: false, + isEngineSchemaEmpty: false, + }; + beforeEach(() => { jest.clearAllMocks(); }); @@ -69,7 +79,7 @@ describe('EngineLogic', () => { EngineLogic.actions.setEngineData(mockEngineData); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, + ...DEFAULT_VALUES_WITH_ENGINE, engine: mockEngineData, dataLoading: false, }); @@ -184,14 +194,58 @@ describe('EngineLogic', () => { }); describe('selectors', () => { + describe('isEngineEmpty', () => { + it('returns true if the engine contains no documents', () => { + const engine = { ...mockEngineData, document_count: 0 }; + mount({ engine }); + + expect(EngineLogic.values).toEqual({ + ...DEFAULT_VALUES_WITH_ENGINE, + engine, + isEngineEmpty: true, + }); + }); + + it('returns true if the engine is not yet initialized', () => { + mount({ engine: {} }); + + expect(EngineLogic.values).toEqual({ + ...DEFAULT_VALUES, + isEngineEmpty: true, + }); + }); + }); + + describe('isEngineSchemaEmpty', () => { + it('returns true if the engine schema contains no fields', () => { + const engine = { ...mockEngineData, schema: {} }; + mount({ engine }); + + expect(EngineLogic.values).toEqual({ + ...DEFAULT_VALUES_WITH_ENGINE, + engine, + isEngineSchemaEmpty: true, + }); + }); + + it('returns true if the engine is not yet initialized', () => { + mount({ engine: {} }); + + expect(EngineLogic.values).toEqual({ + ...DEFAULT_VALUES, + isEngineSchemaEmpty: true, + }); + }); + }); + describe('isSampleEngine', () => { it('should be set based on engine.sample', () => { - const mockSampleEngine = { ...mockEngineData, sample: true }; - mount({ engine: mockSampleEngine }); + const engine = { ...mockEngineData, sample: true }; + mount({ engine }); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, - engine: mockSampleEngine, + ...DEFAULT_VALUES_WITH_ENGINE, + engine, isSampleEngine: true, }); }); @@ -199,12 +253,12 @@ describe('EngineLogic', () => { describe('isMetaEngine', () => { it('should be set based on engine.type', () => { - const mockMetaEngine = { ...mockEngineData, type: EngineTypes.meta }; - mount({ engine: mockMetaEngine }); + const engine = { ...mockEngineData, type: EngineTypes.meta }; + mount({ engine }); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, - engine: mockMetaEngine, + ...DEFAULT_VALUES_WITH_ENGINE, + engine, isMetaEngine: true, }); }); @@ -212,17 +266,17 @@ describe('EngineLogic', () => { describe('hasSchemaErrors', () => { it('should be set based on engine.activeReindexJob.numDocumentsWithErrors', () => { - const mockSchemaEngine = { + const engine = { ...mockEngineData, activeReindexJob: { numDocumentsWithErrors: 10, }, }; - mount({ engine: mockSchemaEngine }); + mount({ engine }); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, - engine: mockSchemaEngine, + ...DEFAULT_VALUES_WITH_ENGINE, + engine, hasSchemaErrors: true, }); }); @@ -230,7 +284,7 @@ describe('EngineLogic', () => { describe('hasSchemaConflicts', () => { it('should be set based on engine.schemaConflicts', () => { - const mockSchemaEngine = { + const engine = { ...mockEngineData, schemaConflicts: { someSchemaField: { @@ -241,11 +295,11 @@ describe('EngineLogic', () => { }, }, }; - mount({ engine: mockSchemaEngine }); + mount({ engine }); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, - engine: mockSchemaEngine, + ...DEFAULT_VALUES_WITH_ENGINE, + engine, hasSchemaConflicts: true, }); }); @@ -253,15 +307,15 @@ describe('EngineLogic', () => { describe('hasUnconfirmedSchemaFields', () => { it('should be set based on engine.unconfirmedFields', () => { - const mockUnconfirmedFieldsEngine = { + const engine = { ...mockEngineData, unconfirmedFields: ['new_field_1', 'new_field_2'], }; - mount({ engine: mockUnconfirmedFieldsEngine }); + mount({ engine }); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, - engine: mockUnconfirmedFieldsEngine, + ...DEFAULT_VALUES_WITH_ENGINE, + engine, hasUnconfirmedSchemaFields: true, }); }); @@ -292,7 +346,7 @@ describe('EngineLogic', () => { mount({ engine }); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, + ...DEFAULT_VALUES_WITH_ENGINE, engine, searchKey: 'search-123xyz', }); @@ -312,7 +366,7 @@ describe('EngineLogic', () => { mount({ engine }); expect(EngineLogic.values).toEqual({ - ...DEFAULT_VALUES, + ...DEFAULT_VALUES_WITH_ENGINE, engine, searchKey: '', }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts index 5cbe89b364859e..0b171447c18c9a 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts @@ -17,6 +17,8 @@ interface EngineValues { dataLoading: boolean; engine: Partial; engineName: string; + isEngineEmpty: boolean; + isEngineSchemaEmpty: boolean; isMetaEngine: boolean; isSampleEngine: boolean; hasSchemaErrors: boolean; @@ -74,6 +76,11 @@ export const EngineLogic = kea>({ ], }, selectors: ({ selectors }) => ({ + isEngineEmpty: [() => [selectors.engine], (engine) => !engine.document_count], + isEngineSchemaEmpty: [ + () => [selectors.engine], + (engine) => Object.keys(engine.schema || {}).length === 0, + ], isMetaEngine: [() => [selectors.engine], (engine) => engine?.type === EngineTypes.meta], isSampleEngine: [() => [selectors.engine], (engine) => !!engine?.sample], // Indexed engines diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/search_ui/search_ui.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/search_ui/search_ui.tsx index 0ac59a33068baf..9f84bf4bd3b755 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/search_ui/search_ui.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/search_ui/search_ui.tsx @@ -24,7 +24,7 @@ import { SearchUILogic } from './search_ui_logic'; export const SearchUI: React.FC = () => { const { loadFieldData } = useActions(SearchUILogic); - const { engine } = useValues(EngineLogic); + const { isEngineSchemaEmpty } = useValues(EngineLogic); useEffect(() => { loadFieldData(); @@ -34,7 +34,7 @@ export const SearchUI: React.FC = () => { } > From c6da0d8cd4b276342a8bfed9af3ebcea9b121f44 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 22 Jun 2021 13:07:02 -0700 Subject: [PATCH 2/5] Update EngineOverview to use isEngineEmpty + remove polling - Per Casey, polling is primarily only needed to dynamically update from empty state to non-empty state, which we're going to handle at the engine level in the next commit --- .../engine_overview/engine_overview.test.tsx | 24 ++----- .../engine_overview/engine_overview.tsx | 21 ++---- .../engine_overview_logic.test.ts | 69 +++---------------- .../engine_overview/engine_overview_logic.ts | 49 ++++--------- .../engine_overview_metrics.test.tsx | 20 ++++++ .../engine_overview_metrics.tsx | 14 +++- 6 files changed, 63 insertions(+), 134 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx index edacd74e046a28..a2e0ba4fcd44df 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx @@ -5,8 +5,7 @@ * 2.0. */ -import '../../../__mocks__/shallow_useeffect.mock'; -import { setMockValues, setMockActions } from '../../../__mocks__/kea_logic'; +import { setMockValues } from '../../../__mocks__/kea_logic'; import React from 'react'; @@ -20,18 +19,14 @@ import { EngineOverview } from './'; describe('EngineOverview', () => { const values = { dataLoading: false, - documentCount: 0, myRole: {}, + isEngineEmpty: true, isMetaEngine: false, }; - const actions = { - pollForOverviewMetrics: jest.fn(), - }; beforeEach(() => { jest.clearAllMocks(); setMockValues(values); - setMockActions(actions); }); it('renders', () => { @@ -39,21 +34,10 @@ describe('EngineOverview', () => { expect(wrapper.find('[data-test-subj="EngineOverview"]')).toHaveLength(1); }); - it('initializes data on mount', () => { - shallow(); - expect(actions.pollForOverviewMetrics).toHaveBeenCalledTimes(1); - }); - - it('renders a loading page template if async data is still loading', () => { - setMockValues({ ...values, dataLoading: true }); - const wrapper = shallow(); - expect(wrapper.prop('isLoading')).toEqual(true); - }); - describe('EmptyEngineOverview', () => { it('renders when the engine has no documents & the user can add documents', () => { const myRole = { canManageEngineDocuments: true, canViewEngineCredentials: true }; - setMockValues({ ...values, myRole, documentCount: 0 }); + setMockValues({ ...values, myRole }); const wrapper = shallow(); expect(wrapper.find(EmptyEngineOverview)).toHaveLength(1); }); @@ -61,7 +45,7 @@ describe('EngineOverview', () => { describe('EngineOverviewMetrics', () => { it('renders when the engine has documents', () => { - setMockValues({ ...values, documentCount: 1 }); + setMockValues({ ...values, isEngineEmpty: false }); const wrapper = shallow(); expect(wrapper.find(EngineOverviewMetrics)).toHaveLength(1); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx index 4c15ffd8b7f947..a3f98d8c13e8e4 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx @@ -5,38 +5,25 @@ * 2.0. */ -import React, { useEffect } from 'react'; +import React from 'react'; -import { useActions, useValues } from 'kea'; +import { useValues } from 'kea'; import { AppLogic } from '../../app_logic'; import { EngineLogic } from '../engine'; -import { AppSearchPageTemplate } from '../layout'; import { EmptyEngineOverview } from './engine_overview_empty'; import { EngineOverviewMetrics } from './engine_overview_metrics'; -import { EngineOverviewLogic } from './'; - export const EngineOverview: React.FC = () => { const { myRole: { canManageEngineDocuments, canViewEngineCredentials }, } = useValues(AppLogic); - const { isMetaEngine } = useValues(EngineLogic); - - const { pollForOverviewMetrics } = useActions(EngineOverviewLogic); - const { dataLoading, documentCount } = useValues(EngineOverviewLogic); - - useEffect(() => { - pollForOverviewMetrics(); - }, []); - - if (dataLoading) return ; + const { isEngineEmpty, isMetaEngine } = useValues(EngineLogic); - const engineHasDocuments = documentCount > 0; const canAddDocuments = canManageEngineDocuments && canViewEngineCredentials; - const showEngineOverview = engineHasDocuments || !canAddDocuments || isMetaEngine; + const showEngineOverview = !isEngineEmpty || !canAddDocuments || isMetaEngine; return (
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.test.ts index c9c1defd46032b..cc677d2642702b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.test.ts @@ -20,7 +20,7 @@ import { nextTick } from '@kbn/test/jest'; import { EngineOverviewLogic } from './'; describe('EngineOverviewLogic', () => { - const { mount, unmount } = new LogicMounter(EngineOverviewLogic); + const { mount } = new LogicMounter(EngineOverviewLogic); const { http } = mockHttpValues; const { flashAPIErrors } = mockFlashMessageHelpers; @@ -41,7 +41,6 @@ describe('EngineOverviewLogic', () => { queriesPerDay: [], totalClicks: 0, totalQueries: 0, - timeoutId: null, }; beforeEach(() => { @@ -54,10 +53,10 @@ describe('EngineOverviewLogic', () => { }); describe('actions', () => { - describe('setPolledData', () => { + describe('onOverviewMetricsLoad', () => { it('should set all received data as top-level values and set dataLoading to false', () => { mount(); - EngineOverviewLogic.actions.setPolledData(mockEngineMetrics); + EngineOverviewLogic.actions.onOverviewMetricsLoad(mockEngineMetrics); expect(EngineOverviewLogic.values).toEqual({ ...DEFAULT_VALUES, @@ -66,34 +65,20 @@ describe('EngineOverviewLogic', () => { }); }); }); - - describe('setTimeoutId', () => { - describe('timeoutId', () => { - it('should be set to the provided value', () => { - mount(); - EngineOverviewLogic.actions.setTimeoutId(123); - - expect(EngineOverviewLogic.values).toEqual({ - ...DEFAULT_VALUES, - timeoutId: 123, - }); - }); - }); - }); }); describe('listeners', () => { - describe('pollForOverviewMetrics', () => { - it('fetches data and calls onPollingSuccess', async () => { + describe('loadOverviewMetrics', () => { + it('fetches data and calls onOverviewMetricsLoad', async () => { mount(); - jest.spyOn(EngineOverviewLogic.actions, 'onPollingSuccess'); + jest.spyOn(EngineOverviewLogic.actions, 'onOverviewMetricsLoad'); http.get.mockReturnValueOnce(Promise.resolve(mockEngineMetrics)); - EngineOverviewLogic.actions.pollForOverviewMetrics(); + EngineOverviewLogic.actions.loadOverviewMetrics(); await nextTick(); expect(http.get).toHaveBeenCalledWith('/api/app_search/engines/some-engine/overview'); - expect(EngineOverviewLogic.actions.onPollingSuccess).toHaveBeenCalledWith( + expect(EngineOverviewLogic.actions.onOverviewMetricsLoad).toHaveBeenCalledWith( mockEngineMetrics ); }); @@ -102,47 +87,11 @@ describe('EngineOverviewLogic', () => { mount(); http.get.mockReturnValue(Promise.reject('An error occurred')); - EngineOverviewLogic.actions.pollForOverviewMetrics(); + EngineOverviewLogic.actions.loadOverviewMetrics(); await nextTick(); expect(flashAPIErrors).toHaveBeenCalledWith('An error occurred'); }); }); - - describe('onPollingSuccess', () => { - it('starts a polling timeout and sets data', async () => { - mount(); - jest.useFakeTimers(); - jest.spyOn(EngineOverviewLogic.actions, 'setTimeoutId'); - jest.spyOn(EngineOverviewLogic.actions, 'setPolledData'); - - EngineOverviewLogic.actions.onPollingSuccess(mockEngineMetrics); - - expect(setTimeout).toHaveBeenCalledWith( - EngineOverviewLogic.actions.pollForOverviewMetrics, - 5000 - ); - expect(EngineOverviewLogic.actions.setTimeoutId).toHaveBeenCalledWith(expect.any(Number)); - expect(EngineOverviewLogic.actions.setPolledData).toHaveBeenCalledWith(mockEngineMetrics); - }); - }); - }); - - describe('unmount', () => { - beforeEach(() => { - jest.useFakeTimers(); - mount(); - }); - - it('clears existing polling timeouts on unmount', () => { - EngineOverviewLogic.actions.setTimeoutId(123); - unmount(); - expect(clearTimeout).toHaveBeenCalled(); - }); - - it("does not clear timeout if one hasn't been set", () => { - unmount(); - expect(clearTimeout).not.toHaveBeenCalled(); - }); }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts index 78d5358fc49096..3f9c2e43a332b9 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts @@ -11,8 +11,6 @@ import { flashAPIErrors } from '../../../shared/flash_messages'; import { HttpLogic } from '../../../shared/http'; import { EngineLogic } from '../engine'; -const POLLING_DURATION = 5000; - interface EngineOverviewApiData { documentCount: number; startDate: string; @@ -23,95 +21,74 @@ interface EngineOverviewApiData { } interface EngineOverviewValues extends EngineOverviewApiData { dataLoading: boolean; - timeoutId: number | null; } interface EngineOverviewActions { - setPolledData(engineMetrics: EngineOverviewApiData): EngineOverviewApiData; - setTimeoutId(timeoutId: number): { timeoutId: number }; - pollForOverviewMetrics(): void; - onPollingSuccess(engineMetrics: EngineOverviewApiData): EngineOverviewApiData; + loadOverviewMetrics(): void; + onOverviewMetricsLoad(response: EngineOverviewApiData): EngineOverviewApiData; } export const EngineOverviewLogic = kea>({ path: ['enterprise_search', 'app_search', 'engine_overview_logic'], actions: () => ({ - setPolledData: (engineMetrics) => engineMetrics, - setTimeoutId: (timeoutId) => ({ timeoutId }), - pollForOverviewMetrics: true, - onPollingSuccess: (engineMetrics) => engineMetrics, + loadOverviewMetrics: true, + onOverviewMetricsLoad: (engineMetrics) => engineMetrics, }), reducers: () => ({ dataLoading: [ true, { - setPolledData: () => false, + onOverviewMetricsLoad: () => false, }, ], startDate: [ '', { - setPolledData: (_, { startDate }) => startDate, + onOverviewMetricsLoad: (_, { startDate }) => startDate, }, ], queriesPerDay: [ [], { - setPolledData: (_, { queriesPerDay }) => queriesPerDay, + onOverviewMetricsLoad: (_, { queriesPerDay }) => queriesPerDay, }, ], operationsPerDay: [ [], { - setPolledData: (_, { operationsPerDay }) => operationsPerDay, + onOverviewMetricsLoad: (_, { operationsPerDay }) => operationsPerDay, }, ], totalQueries: [ 0, { - setPolledData: (_, { totalQueries }) => totalQueries, + onOverviewMetricsLoad: (_, { totalQueries }) => totalQueries, }, ], totalClicks: [ 0, { - setPolledData: (_, { totalClicks }) => totalClicks, + onOverviewMetricsLoad: (_, { totalClicks }) => totalClicks, }, ], documentCount: [ 0, { - setPolledData: (_, { documentCount }) => documentCount, - }, - ], - timeoutId: [ - null, - { - setTimeoutId: (_, { timeoutId }) => timeoutId, + onOverviewMetricsLoad: (_, { documentCount }) => documentCount, }, ], }), listeners: ({ actions }) => ({ - pollForOverviewMetrics: async () => { + loadOverviewMetrics: async () => { const { http } = HttpLogic.values; const { engineName } = EngineLogic.values; try { const response = await http.get(`/api/app_search/engines/${engineName}/overview`); - actions.onPollingSuccess(response); + actions.onOverviewMetricsLoad(response); } catch (e) { flashAPIErrors(e); } }, - onPollingSuccess: (engineMetrics) => { - const timeoutId = window.setTimeout(actions.pollForOverviewMetrics, POLLING_DURATION); - actions.setTimeoutId(timeoutId); - actions.setPolledData(engineMetrics); - }, - }), - events: ({ values }) => ({ - beforeUnmount() { - if (values.timeoutId !== null) clearTimeout(values.timeoutId); - }, }), }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.test.tsx index 620d913c5f9a7d..14f182463d8375 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.test.tsx @@ -5,6 +5,8 @@ * 2.0. */ +import { setMockValues, setMockActions } from '../../../__mocks__/kea_logic'; +import '../../../__mocks__/shallow_useeffect.mock'; import '../../__mocks__/engine_logic.mock'; import React from 'react'; @@ -17,6 +19,19 @@ import { TotalStats, TotalCharts, RecentApiLogs } from './components'; import { EngineOverviewMetrics } from './engine_overview_metrics'; describe('EngineOverviewMetrics', () => { + const values = { + dataLoading: false, + }; + const actions = { + loadOverviewMetrics: jest.fn(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + setMockValues(values); + setMockActions(actions); + }); + it('renders', () => { const wrapper = shallow(); @@ -25,4 +40,9 @@ describe('EngineOverviewMetrics', () => { expect(wrapper.find(TotalCharts)).toHaveLength(1); expect(wrapper.find(RecentApiLogs)).toHaveLength(1); }); + + it('initializes data on mount', () => { + shallow(); + expect(actions.loadOverviewMetrics).toHaveBeenCalledTimes(1); + }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.tsx index b47ae21104ae96..3cc7138623735c 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview_metrics.tsx @@ -5,7 +5,9 @@ * 2.0. */ -import React from 'react'; +import React, { useEffect } from 'react'; + +import { useActions, useValues } from 'kea'; import { EuiFlexGroup, EuiFlexItem, EuiSpacer } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; @@ -15,7 +17,16 @@ import { AppSearchPageTemplate } from '../layout'; import { TotalStats, TotalCharts, RecentApiLogs } from './components'; +import { EngineOverviewLogic } from './'; + export const EngineOverviewMetrics: React.FC = () => { + const { loadOverviewMetrics } = useActions(EngineOverviewLogic); + const { dataLoading } = useValues(EngineOverviewLogic); + + useEffect(() => { + loadOverviewMetrics(); + }, []); + return ( { defaultMessage: 'Engine overview', }), }} + isLoading={dataLoading} > From fba82ef8ca202d05b5bd9ec9e34317a224ed0d42 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 22 Jun 2021 14:39:59 -0700 Subject: [PATCH 3/5] Add empty engine polling behavior - Now that both Engines Overview, Documents, and Search UI views have empty states that are hooked up to EngineLogic, this poll will update all of the above pages automatically when a new document comes in! --- .../app_search/components/engine/constants.ts | 20 +++ .../components/engine/engine_logic.test.ts | 137 +++++++++++++++++- .../components/engine/engine_logic.ts | 55 ++++++- .../components/engine/engine_router.test.tsx | 14 +- .../components/engine/engine_router.tsx | 11 +- 5 files changed, 226 insertions(+), 11 deletions(-) create mode 100644 x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/constants.ts diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/constants.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/constants.ts new file mode 100644 index 00000000000000..9102f706fdbed4 --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/constants.ts @@ -0,0 +1,20 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { i18n } from '@kbn/i18n'; + +export const POLLING_DURATION = 5000; + +export const POLLING_ERROR_TITLE = i18n.translate( + 'xpack.enterpriseSearch.appSearch.engine.pollingErrorMessage', + { defaultMessage: 'Could not fetch engine data' } +); + +export const POLLING_ERROR_TEXT = i18n.translate( + 'xpack.enterpriseSearch.appSearch.engine.pollingErrorDescription', + { defaultMessage: 'Please check your connection or manually reload the page.' } +); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts index 3c3248158efb0d..4b3314d5a59229 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts @@ -5,7 +5,11 @@ * 2.0. */ -import { LogicMounter, mockHttpValues } from '../../../__mocks__/kea_logic'; +import { + LogicMounter, + mockHttpValues, + mockFlashMessageHelpers, +} from '../../../__mocks__/kea_logic'; import { nextTick } from '@kbn/test/jest'; @@ -17,8 +21,9 @@ import { EngineTypes } from './types'; import { EngineLogic } from './'; describe('EngineLogic', () => { - const { mount } = new LogicMounter(EngineLogic); + const { mount, unmount } = new LogicMounter(EngineLogic); const { http } = mockHttpValues; + const { flashErrorToast } = mockFlashMessageHelpers; const mockEngineData = { name: 'some-engine', @@ -53,6 +58,7 @@ describe('EngineLogic', () => { hasUnconfirmedSchemaFields: false, engineNotFound: false, searchKey: '', + intervalId: null, }; const DEFAULT_VALUES_WITH_ENGINE = { @@ -164,6 +170,34 @@ describe('EngineLogic', () => { }); }); }); + + describe('onPollStart', () => { + it('should set intervalId', () => { + mount({ intervalId: null }); + EngineLogic.actions.onPollStart(123); + + expect(EngineLogic.values).toEqual({ + ...DEFAULT_VALUES, + intervalId: 123, + }); + }); + + describe('onPollStop', () => { + // Note: This does have to be a separate action following stopPolling(), rather + // than using stopPolling: () => null as a reducer. If you do that, then the ID + // gets cleared before the actual poll interval does & the poll interval never clears :doh: + + it('should reset intervalId', () => { + mount({ intervalId: 123 }); + EngineLogic.actions.onPollStop(); + + expect(EngineLogic.values).toEqual({ + ...DEFAULT_VALUES, + intervalId: null, + }); + }); + }); + }); }); describe('listeners', () => { @@ -180,16 +214,100 @@ describe('EngineLogic', () => { expect(EngineLogic.actions.setEngineData).toHaveBeenCalledWith(mockEngineData); }); - it('handles errors', async () => { + it('handles 4xx errors', async () => { mount(); jest.spyOn(EngineLogic.actions, 'setEngineNotFound'); - http.get.mockReturnValue(Promise.reject('An error occured')); + http.get.mockReturnValue(Promise.reject({ response: { status: 404 } })); EngineLogic.actions.initializeEngine(); await nextTick(); expect(EngineLogic.actions.setEngineNotFound).toHaveBeenCalledWith(true); }); + + it('handles 5xx errors', async () => { + mount(); + jest.spyOn(EngineLogic.actions, 'setEngineNotFound'); + http.get.mockReturnValue(Promise.reject('An error occured')); + + EngineLogic.actions.initializeEngine(); + await nextTick(); + + expect(flashErrorToast).toHaveBeenCalledWith('Could not fetch engine data', { + text: expect.stringContaining('Please check your connection'), + toastLifeTimeMs: 3750, + }); + }); + }); + + describe('pollEmptyEngine', () => { + beforeEach(() => jest.useFakeTimers()); + afterEach(() => jest.clearAllTimers()); + + it('starts a poll', () => { + mount(); + jest.spyOn(global, 'setInterval'); + jest.spyOn(EngineLogic.actions, 'onPollStart'); + + EngineLogic.actions.pollEmptyEngine(); + + expect(global.setInterval).toHaveBeenCalled(); + expect(EngineLogic.actions.onPollStart).toHaveBeenCalled(); + }); + + it('polls for engine data if the current engine is empty', () => { + mount({ engine: {} }); + jest.spyOn(EngineLogic.actions, 'initializeEngine'); + + EngineLogic.actions.pollEmptyEngine(); + + jest.advanceTimersByTime(5000); + expect(EngineLogic.actions.initializeEngine).toHaveBeenCalledTimes(1); + jest.advanceTimersByTime(5000); + expect(EngineLogic.actions.initializeEngine).toHaveBeenCalledTimes(2); + }); + + it('cancels the poll if the current engine changed from empty to non-empty', () => { + mount({ engine: mockEngineData }); + jest.spyOn(EngineLogic.actions, 'stopPolling'); + jest.spyOn(EngineLogic.actions, 'initializeEngine'); + + EngineLogic.actions.pollEmptyEngine(); + + jest.advanceTimersByTime(5000); + expect(EngineLogic.actions.stopPolling).toHaveBeenCalled(); + expect(EngineLogic.actions.initializeEngine).not.toHaveBeenCalled(); + }); + + it('does not create new polls if one already exists', () => { + jest.spyOn(global, 'setInterval'); + mount({ intervalId: 123 }); + + EngineLogic.actions.pollEmptyEngine(); + + expect(global.setInterval).not.toHaveBeenCalled(); + }); + }); + + describe('stopPolling', () => { + it('clears the poll interval and unsets the intervalId', () => { + jest.spyOn(global, 'clearInterval'); + mount({ intervalId: 123 }); + + EngineLogic.actions.stopPolling(); + + expect(global.clearInterval).toHaveBeenCalledWith(123); + expect(EngineLogic.values.intervalId).toEqual(null); + }); + + it('does not clearInterval if a poll has not been started', () => { + jest.spyOn(global, 'clearInterval'); + mount({ intervalId: null }); + + EngineLogic.actions.stopPolling(); + + expect(global.clearInterval).not.toHaveBeenCalled(); + }); }); }); @@ -373,4 +491,15 @@ describe('EngineLogic', () => { }); }); }); + + describe('events', () => { + it('calls stopPolling before unmount', () => { + mount(); + // Has to be a const to check state after unmount + const stopPollingSpy = jest.spyOn(EngineLogic.actions, 'stopPolling'); + + unmount(); + expect(stopPollingSpy).toHaveBeenCalled(); + }); + }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts index 0b171447c18c9a..bfa77450176f64 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts @@ -7,10 +7,12 @@ import { kea, MakeLogicType } from 'kea'; +import { flashErrorToast } from '../../../shared/flash_messages'; import { HttpLogic } from '../../../shared/http'; import { ApiTokenTypes } from '../credentials/constants'; import { ApiToken } from '../credentials/types'; +import { POLLING_DURATION, POLLING_ERROR_TITLE, POLLING_ERROR_TEXT } from './constants'; import { EngineDetails, EngineTypes } from './types'; interface EngineValues { @@ -26,6 +28,7 @@ interface EngineValues { hasUnconfirmedSchemaFields: boolean; engineNotFound: boolean; searchKey: string; + intervalId: number | null; } interface EngineActions { @@ -34,6 +37,10 @@ interface EngineActions { setEngineNotFound(notFound: boolean): { notFound: boolean }; clearEngine(): void; initializeEngine(): void; + pollEmptyEngine(): void; + onPollStart(intervalId: number): { intervalId: number }; + stopPolling(): void; + onPollStop(): void; } export const EngineLogic = kea>({ @@ -44,6 +51,10 @@ export const EngineLogic = kea>({ setEngineNotFound: (notFound) => ({ notFound }), clearEngine: true, initializeEngine: true, + pollEmptyEngine: true, + onPollStart: (intervalId) => ({ intervalId }), + stopPolling: true, + onPollStop: true, }, reducers: { dataLoading: [ @@ -74,6 +85,13 @@ export const EngineLogic = kea>({ clearEngine: () => false, }, ], + intervalId: [ + null, + { + onPollStart: (_, { intervalId }) => intervalId, + onPollStop: () => null, + }, + ], }, selectors: ({ selectors }) => ({ isEngineEmpty: [() => [selectors.engine], (engine) => !engine.document_count], @@ -107,7 +125,9 @@ export const EngineLogic = kea>({ ], }), listeners: ({ actions, values }) => ({ - initializeEngine: async () => { + initializeEngine: async (_, breakpoint) => { + breakpoint(); // Prevents errors if logic unmounts while fetching + const { engineName } = values; const { http } = HttpLogic.values; @@ -115,8 +135,39 @@ export const EngineLogic = kea>({ const response = await http.get(`/api/app_search/engines/${engineName}`); actions.setEngineData(response); } catch (error) { - actions.setEngineNotFound(true); + if (error?.response?.status >= 400 && error?.response?.status < 500) { + actions.setEngineNotFound(true); + } else { + flashErrorToast(POLLING_ERROR_TITLE, { + text: POLLING_ERROR_TEXT, + toastLifeTimeMs: POLLING_DURATION * 0.75, + }); + } } }, + pollEmptyEngine: () => { + if (values.intervalId) return; // Ensure we only have one poll at a time + + const id = window.setInterval(() => { + if (values.isEngineEmpty && values.isEngineSchemaEmpty) { + actions.initializeEngine(); // Re-fetch engine data when engine is empty + } else { + actions.stopPolling(); + } + }, POLLING_DURATION); + + actions.onPollStart(id); + }, + stopPolling: () => { + if (values.intervalId !== null) { + clearInterval(values.intervalId); + actions.onPollStop(); + } + }, + }), + events: ({ actions }) => ({ + beforeUnmount: () => { + actions.stopPolling(); + }, }), }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.test.tsx index ee1c0578debfc0..ed35bfbe978428 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.test.tsx @@ -41,7 +41,13 @@ describe('EngineRouter', () => { engineNotFound: false, myRole: {}, }; - const actions = { setEngineName: jest.fn(), initializeEngine: jest.fn(), clearEngine: jest.fn() }; + const actions = { + setEngineName: jest.fn(), + initializeEngine: jest.fn(), + pollEmptyEngine: jest.fn(), + stopPolling: jest.fn(), + clearEngine: jest.fn(), + }; beforeEach(() => { setMockValues(values); @@ -58,12 +64,14 @@ describe('EngineRouter', () => { expect(actions.setEngineName).toHaveBeenCalledWith('some-engine'); }); - it('initializes/fetches engine API data', () => { + it('initializes/fetches engine API data and starts a poll for empty engines', () => { expect(actions.initializeEngine).toHaveBeenCalled(); + expect(actions.pollEmptyEngine).toHaveBeenCalled(); }); - it('clears engine on unmount and on update', () => { + it('clears engine and stops polling on unmount / on engine change', () => { unmountHandler(); + expect(actions.stopPolling).toHaveBeenCalled(); expect(actions.clearEngine).toHaveBeenCalled(); }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.tsx index 59535fb737fa6f..6fc40ee8503f2f 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.tsx @@ -68,12 +68,19 @@ export const EngineRouter: React.FC = () => { const { engineName: engineNameFromUrl } = useParams() as { engineName: string }; const { engineName, dataLoading, engineNotFound } = useValues(EngineLogic); - const { setEngineName, initializeEngine, clearEngine } = useActions(EngineLogic); + const { setEngineName, initializeEngine, pollEmptyEngine, stopPolling, clearEngine } = useActions( + EngineLogic + ); useEffect(() => { setEngineName(engineNameFromUrl); initializeEngine(); - return clearEngine; + pollEmptyEngine(); + + return () => { + stopPolling(); + clearEngine(); + }; }, [engineNameFromUrl]); if (engineNotFound) { From fd6287204c031bc7eb18181c830a52d97106d384 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 22 Jun 2021 14:44:23 -0700 Subject: [PATCH 4/5] [Misc UI polish] Add (+) icon to Index documents button - to match other create page header buttons --- .../documents/components/document_creation_button.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/components/document_creation_button.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/components/document_creation_button.tsx index cded18094c5f2f..482ee282cf4648 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/components/document_creation_button.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/documents/components/document_creation_button.tsx @@ -21,7 +21,7 @@ export const DocumentCreationButton: React.FC = () => { <> From 308a8804329fd8beb1a8d63c7570d1d42110a685 Mon Sep 17 00:00:00 2001 From: Constance Date: Wed, 23 Jun 2021 11:31:35 -0700 Subject: [PATCH 5/5] [PR feedback] Test improvements Co-authored-by: Jason Stoltzfus --- .../app_search/components/engine/engine_logic.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts index 4b3314d5a59229..0189edbbf871f7 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts @@ -227,7 +227,6 @@ describe('EngineLogic', () => { it('handles 5xx errors', async () => { mount(); - jest.spyOn(EngineLogic.actions, 'setEngineNotFound'); http.get.mockReturnValue(Promise.reject('An error occured')); EngineLogic.actions.initializeEngine(); @@ -243,6 +242,7 @@ describe('EngineLogic', () => { describe('pollEmptyEngine', () => { beforeEach(() => jest.useFakeTimers()); afterEach(() => jest.clearAllTimers()); + afterAll(() => jest.useRealTimers()); it('starts a poll', () => { mount();