Merge pull request #14 from dimaanj/make-adhoc-logic-more-explicit

[Discover] Make adhoc data view logic more explicit
kertal · Nov 3, 2022 · 0fd1b90 · 0fd1b90
2 parents 9404801 + badaf79
commit 0fd1b90
Show file tree

Hide file tree

Showing 10 changed files with 127 additions and 69 deletions.
diff --git a/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx b/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx
@@ -181,12 +181,15 @@ export function DiscoverLayout({
   const contentCentered = resultState === 'uninitialized' || resultState === 'none';
   const onDataViewCreated = useCallback(
     (nextDataView: DataView) => {
+      if (!nextDataView.isPersisted()) {
+        stateContainer.actions.appendAdHocDataView(nextDataView);
+      }
       if (nextDataView.id) {
         onChangeDataView(nextDataView.id);
       }
       savedSearchRefetch$.next('reset');
     },
-    [onChangeDataView, savedSearchRefetch$]
+    [onChangeDataView, savedSearchRefetch$, stateContainer]
   );
 
   const savedSearchTitle = useRef<HTMLHeadingElement>(null);

diff --git a/src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx b/src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx
@@ -128,13 +128,16 @@ export const DiscoverTopNav = ({
   const createNewDataView = useCallback(() => {
     closeDataViewEditor.current = dataViewEditor.openEditor({
       onSave: async (dataViewToSave) => {
+        if (!dataViewToSave.isPersisted()) {
+          stateContainer.actions.appendAdHocDataView(dataViewToSave);
+        }
         if (dataViewToSave.id) {
           onChangeDataView(dataViewToSave.id);
         }
       },
       allowAdHocDataView: true,
     });
-  }, [dataViewEditor, onChangeDataView]);
+  }, [dataViewEditor, onChangeDataView, stateContainer.actions]);
 
   const onCreateDefaultAdHocDataView = useCallback(
     async (pattern: string) => {
@@ -144,9 +147,11 @@ export const DiscoverTopNav = ({
       if (newDataView.fields.getByName('@timestamp')?.type === 'date') {
         newDataView.timeFieldName = '@timestamp';
       }
+
+      stateContainer.actions.appendAdHocDataView(newDataView);
       onChangeDataView(newDataView.id!);
     },
-    [dataViews, onChangeDataView]
+    [dataViews, onChangeDataView, stateContainer.actions]
   );
 
   const topNavMenu = useMemo(

diff --git a/src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.test.tsx b/src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.test.tsx
@@ -130,9 +130,7 @@ describe('useAdHocDataViews', () => {
       updatedDataView = await hook.result.current.updateAdHocDataViewId(mockDataView);
     });
 
-    expect(dataViewsCreateMock).toHaveBeenCalledWith({
-      id: undefined,
-    });
+    expect(mockDiscoverServices.dataViews.clearInstanceCache).toHaveBeenCalledWith(mockDataView.id);
     expect(updatedDataView!.id).toEqual('updated-mock-id');
   });
 });
diff --git a/src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.ts b/src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.ts
@@ -7,19 +7,15 @@
  */
 
 import { useCallback } from 'react';
+import uuid from 'uuid/v4';
 import type { DataView, DataViewsContract } from '@kbn/data-views-plugin/public';
 import { SavedSearch } from '@kbn/saved-search-plugin/public';
-import {
-  UPDATE_FILTER_REFERENCES_ACTION,
-  UPDATE_FILTER_REFERENCES_TRIGGER,
-} from '@kbn/unified-search-plugin/public';
-import { ActionExecutionContext } from '@kbn/ui-actions-plugin/public';
 import type { FilterManager } from '@kbn/data-plugin/public';
 import type { ToastsStart } from '@kbn/core-notifications-browser';
-import { getUiActions } from '../../../kibana_services';
 import { useConfirmPersistencePrompt } from '../../../hooks/use_confirm_persistence_prompt';
 import { DiscoverStateContainer } from '../services/discover_state';
 import { useFiltersValidation } from './use_filters_validation';
+import { updateFiltersReferences } from '../utils/update_filter_references';
 
 export const useAdHocDataViews = ({
   savedSearch,
@@ -46,50 +42,40 @@ export const useAdHocDataViews = ({
    * This is to prevent duplicate ids messing with our system
    */
   const updateAdHocDataViewId = useCallback(
-    async (dataViewToUpdate: DataView) => {
-      const adHocDataViewList = stateContainer.internalState.getState().dataViewsAdHoc;
-      const newDataView = await dataViews.create({ ...dataViewToUpdate.toSpec(), id: undefined });
+    async (prevDataView: DataView) => {
+      const newDataView = await dataViews.create({ ...prevDataView.toSpec(), id: uuid() });
+      dataViews.clearInstanceCache(prevDataView.id);
 
-      dataViews.clearInstanceCache(dataViewToUpdate.id);
-      const nextAdHocDataViewList = adHocDataViewList.filter(
-        (d: DataView) => d.id && dataViewToUpdate.id && d.id !== dataViewToUpdate.id
-      );
-      stateContainer.internalState.transitions.setDataViewsAdHoc(nextAdHocDataViewList);
+      updateFiltersReferences(prevDataView, newDataView);
 
-      const uiActions = await getUiActions();
-      const trigger = uiActions.getTrigger(UPDATE_FILTER_REFERENCES_TRIGGER);
-      const action = uiActions.getAction(UPDATE_FILTER_REFERENCES_ACTION);
-
-      // execute shouldn't be awaited, this is important for pending history push cancellation
-      action?.execute({
-        trigger,
-        fromDataView: dataViewToUpdate.id,
-        toDataView: newDataView.id,
-        usedDataViews: [],
-      } as ActionExecutionContext);
-      stateContainer.actions.setDataView(newDataView);
+      stateContainer.actions.replaceAdHocDataViewWithId(prevDataView.id!, newDataView);
       await stateContainer.replaceUrlAppState({ index: newDataView.id });
+
       setUrlTracking(newDataView);
       return newDataView;
     },
     [dataViews, setUrlTracking, stateContainer]
   );
 
-  const { openConfirmSavePrompt, updateSavedSearch } =
-    useConfirmPersistencePrompt(updateAdHocDataViewId);
+  const { openConfirmSavePrompt, updateSavedSearch } = useConfirmPersistencePrompt(stateContainer);
   const persistDataView = useCallback(async () => {
     const currentDataView = savedSearch.searchSource.getField('index')!;
-    if (currentDataView && !currentDataView.isPersisted()) {
-      const createdDataView = await openConfirmSavePrompt(currentDataView);
+    if (!currentDataView || currentDataView.isPersisted()) {
+      return currentDataView;
+    }
 
+    const createdDataView = await openConfirmSavePrompt(currentDataView);
+    if (!createdDataView) {
+      return currentDataView; // persistance cancelled
+    }
+
+    if (savedSearch.id) {
       // update saved search with saved data view
-      if (createdDataView && savedSearch.id) {
-        const currentState = stateContainer.appState.getState();
-        await updateSavedSearch({ savedSearch, dataView: createdDataView, state: currentState });
-      }
-      return createdDataView;
+      const currentState = stateContainer.appState.getState();
+      await updateSavedSearch({ savedSearch, dataView: createdDataView, state: currentState });
     }
-    return currentDataView;
+
+    return createdDataView;
   }, [stateContainer, openConfirmSavePrompt, savedSearch, updateSavedSearch]);
 
   return { persistDataView, updateAdHocDataViewId };

diff --git a/src/plugins/discover/public/application/main/hooks/use_discover_state.ts b/src/plugins/discover/public/application/main/hooks/use_discover_state.ts
@@ -63,7 +63,8 @@ export function useDiscoverState({
     });
     container.actions.setDataView(dataView);
     return container;
-  }, [history, savedSearch, services, dataView]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [history, savedSearch, services]);
 
   const { setUrlTracking } = useUrlTracking(savedSearch, dataView);
 

diff --git a/src/plugins/discover/public/application/main/services/discover_internal_state_container.ts b/src/plugins/discover/public/application/main/services/discover_internal_state_container.ts
@@ -20,7 +20,11 @@ export interface InternalState {
 
 interface InternalStateTransitions {
   setDataView: (state: InternalState) => (dataView: DataView) => InternalState;
-  setDataViewsAdHoc: (state: InternalState) => (dataViews: DataView[]) => InternalState;
+  appendAdHocDataView: (state: InternalState) => (dataView: DataView) => InternalState;
+  removeAdHocDataViewById: (state: InternalState) => (id: string) => InternalState;
+  replaceAdHocDataViewWithId: (
+    state: InternalState
+  ) => (id: string, dataView: DataView) => InternalState;
 }
 
 export type InternalStateContainer = ReduxLikeStateContainer<
@@ -42,10 +46,21 @@ export function getInternalStateContainer() {
         ...prevState,
         dataView: nextDataView,
       }),
-      setDataViewsAdHoc: (prevState: InternalState) => (dataViewsAdHoc: DataView[]) => ({
+      appendAdHocDataView: (prevState: InternalState) => (dataViewAdHoc: DataView) => ({
         ...prevState,
-        dataViewsAdHoc,
+        dataViewsAdHoc: prevState.dataViewsAdHoc.concat(dataViewAdHoc),
       }),
+      removeAdHocDataViewById: (prevState: InternalState) => (id: string) => ({
+        ...prevState,
+        dataViewsAdHoc: prevState.dataViewsAdHoc.filter((dataView) => dataView.id !== id),
+      }),
+      replaceAdHocDataViewWithId:
+        (prevState: InternalState) => (prevId: string, newDataView: DataView) => ({
+          ...prevState,
+          dataViewsAdHoc: prevState.dataViewsAdHoc.map((dataView) =>
+            dataView.id === prevId ? newDataView : dataView
+          ),
+        }),
     },
     {},
     { freeze: (state) => state }

diff --git a/src/plugins/discover/public/application/main/services/discover_state.ts b/src/plugins/discover/public/application/main/services/discover_state.ts
@@ -128,6 +128,12 @@ export interface DiscoverStateContainer {
      * Set the currently selected data view
      */
     setDataView: (dataView: DataView) => void;
+    /**
+     * Manage AdHoc data views
+     */
+    appendAdHocDataView: (dataView: DataView) => void;
+    removeAdHocDataViewById: (id: string) => void;
+    replaceAdHocDataViewWithId: (id: string, dataView: DataView) => void;
   };
 }
 
@@ -210,16 +216,14 @@ export function getDiscoverStateContainer({
     }
   };
 
-  const setDataView = (dataView: DataView) => {
+  const setDataView = (dataView: DataView) =>
     internalStateContainer.transitions.setDataView(dataView);
-    if (!dataView.isPersisted()) {
-      const adHocDataViewList = internalStateContainer.getState().dataViewsAdHoc;
-      const existing = adHocDataViewList.find((prevDataView) => prevDataView.id === dataView.id);
-      if (!existing) {
-        internalStateContainer.transitions.setDataViewsAdHoc([...adHocDataViewList, dataView]);
-      }
-    }
-  };
+  const appendAdHocDataView = (dataView: DataView) =>
+    internalStateContainer.transitions.appendAdHocDataView(dataView);
+  const replaceAdHocDataViewWithId = (id: string, dataView: DataView) =>
+    internalStateContainer.transitions.replaceAdHocDataViewWithId(id, dataView);
+  const removeAdHocDataViewById = (id: string) =>
+    internalStateContainer.transitions.removeAdHocDataViewById(id);
 
   return {
     kbnUrlStateStorage: stateStorage,
@@ -302,6 +306,9 @@ export function getDiscoverStateContainer({
     },
     actions: {
       setDataView,
+      appendAdHocDataView,
+      replaceAdHocDataViewWithId,
+      removeAdHocDataViewById,
     },
   };
 }

diff --git a/src/plugins/discover/public/application/main/utils/update_filter_references.ts b/src/plugins/discover/public/application/main/utils/update_filter_references.ts
@@ -0,0 +1,27 @@
+/*
+ * 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 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import {
+  UPDATE_FILTER_REFERENCES_ACTION,
+  UPDATE_FILTER_REFERENCES_TRIGGER,
+} from '@kbn/unified-search-plugin/public';
+import { ActionExecutionContext } from '@kbn/ui-actions-plugin/public';
+import type { DataView } from '@kbn/data-views-plugin/public';
+import { getUiActions } from '../../../kibana_services';
+
+export const updateFiltersReferences = (prevDataView: DataView, nextDataView: DataView) => {
+  const uiActions = getUiActions();
+  const trigger = uiActions.getTrigger(UPDATE_FILTER_REFERENCES_TRIGGER);
+  const action = uiActions.getAction(UPDATE_FILTER_REFERENCES_ACTION);
+  action?.execute({
+    trigger,
+    fromDataView: prevDataView.id,
+    toDataView: nextDataView.id,
+    usedDataViews: [],
+  } as ActionExecutionContext);
+};
diff --git a/src/plugins/discover/public/hooks/use_confirm_persistence_prompt.test.tsx b/src/plugins/discover/public/hooks/use_confirm_persistence_prompt.test.tsx
@@ -10,8 +10,11 @@ import React from 'react';
 import type { DataView } from '@kbn/data-views-plugin/public';
 import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
 import { renderHook } from '@testing-library/react-hooks';
+import type { UiActionsStart } from '@kbn/ui-actions-plugin/public';
 import { discoverServiceMock as mockDiscoverServices } from '../__mocks__/services';
 import { useConfirmPersistencePrompt } from './use_confirm_persistence_prompt';
+import { getDiscoverStateMock } from '../__mocks__/discover_state.mock';
+import { setUiActions } from '../kibana_services';
 
 jest.mock('./show_confirm_panel', () => {
   return {
@@ -28,11 +31,16 @@ const mockDataView = {
   toSpec: () => ({}),
 } as DataView;
 
+setUiActions({
+  getTrigger: jest.fn(() => {}),
+  getAction: jest.fn(() => ({ execute: jest.fn() })),
+} as unknown as UiActionsStart);
+
 describe('useConfirmPersistencePrompt', () => {
   it('should save data view correctly', async () => {
     mockDiscoverServices.dataViews.createAndSave = jest.fn().mockResolvedValue(mockDataView);
     const hook = renderHook(
-      (d: DataView) => useConfirmPersistencePrompt(() => Promise.resolve(mockDataView)),
+      (d: DataView) => useConfirmPersistencePrompt(getDiscoverStateMock({})),
       {
         initialProps: mockDataView,
         wrapper: ({ children }) => (
@@ -52,7 +60,7 @@ describe('useConfirmPersistencePrompt', () => {
       .fn()
       .mockRejectedValue(new Error('failed to save'));
     const hook = renderHook(
-      (d: DataView) => useConfirmPersistencePrompt(() => Promise.resolve(mockDataView)),
+      (d: DataView) => useConfirmPersistencePrompt(getDiscoverStateMock({})),
       {
         initialProps: mockDataView,
         wrapper: ({ children }) => (

diff --git a/src/plugins/discover/public/hooks/use_confirm_persistence_prompt.ts b/src/plugins/discover/public/hooks/use_confirm_persistence_prompt.ts
@@ -7,31 +7,39 @@
  */
 
 import { useCallback } from 'react';
+import uuid from 'uuid/v4';
 import { i18n } from '@kbn/i18n';
 import type { DataView } from '@kbn/data-views-plugin/public';
 import { SavedSearch } from '@kbn/saved-search-plugin/public';
 import { useDiscoverServices } from './use_discover_services';
 import { showConfirmPanel } from './show_confirm_panel';
 import { persistSavedSearch } from '../application/main/utils/persist_saved_search';
+import { DiscoverStateContainer } from '../application/main/services/discover_state';
+import { updateFiltersReferences } from '../application/main/utils/update_filter_references';
 
-export const useConfirmPersistencePrompt = (
-  updateAdHocDataViewId: (dataView: DataView) => Promise<DataView>
-) => {
+export const useConfirmPersistencePrompt = (stateContainer: DiscoverStateContainer) => {
   const services = useDiscoverServices();
 
-  const persistDataView: (dataView: DataView) => Promise<DataView> = useCallback(
-    async (dataView) => {
+  const persistDataView: (adHocDataView: DataView) => Promise<DataView> = useCallback(
+    async (adHocDataView) => {
       try {
-        const updatedDataView = await updateAdHocDataViewId(dataView);
+        const persistedDataView = await services.dataViews.createAndSave({
+          ...adHocDataView.toSpec(),
+          id: uuid(),
+        });
+        services.dataViews.clearInstanceCache(adHocDataView.id);
+
+        updateFiltersReferences(adHocDataView, persistedDataView);
 
-        const response = await services.dataViews.createAndSave(updatedDataView.toSpec());
+        stateContainer.actions.removeAdHocDataViewById(adHocDataView.id!);
+        await stateContainer.replaceUrlAppState({ index: persistedDataView.id });
 
         const message = i18n.translate('discover.dataViewPersist.message', {
           defaultMessage: "Saved '{dataViewName}'",
-          values: { dataViewName: response.getName() },
+          values: { dataViewName: persistedDataView.getName() },
         });
         services.toastNotifications.addSuccess(message);
-        return response;
+        return persistedDataView;
       } catch (error) {
         services.toastNotifications.addDanger({
           title: i18n.translate('discover.dataViewPersistError.title', {
@@ -42,7 +50,7 @@ export const useConfirmPersistencePrompt = (
         throw new Error(error);
       }
     },
-    [services.dataViews, services.toastNotifications, updateAdHocDataViewId]
+    [services.dataViews, services.toastNotifications, stateContainer]
   );
 
   const openConfirmSavePrompt: (dataView: DataView) => Promise<DataView | undefined> = useCallback(
@@ -61,7 +69,7 @@ export const useConfirmPersistencePrompt = (
   );
 
   const onUpdateSuccess = useCallback(
-    (id: string, savedSearch: SavedSearch) => {
+    (savedSearch: SavedSearch) => {
       services.toastNotifications.addSuccess({
         title: i18n.translate('discover.notifications.updateSavedSearchTitle', {
           defaultMessage: `Search '{savedSearchTitle}' updated with saved data view`,
@@ -94,7 +102,7 @@ export const useConfirmPersistencePrompt = (
     ({ savedSearch, dataView, state }) => {
       return persistSavedSearch(savedSearch, {
         dataView,
-        onSuccess: (id) => onUpdateSuccess(id, savedSearch),
+        onSuccess: () => onUpdateSuccess(savedSearch),
         onError: (error) => onUpdateError(error, savedSearch),
         state,
         saveOptions: {},