[Discover] Update data view id on adhoc data view change (#142069)

* [Discover] update id on adhoc data view change * [Discover] fix ci * [Discover] fix ci * Update src/plugins/discover/public/application/main/components/layout/discover_layout.tsx Co-authored-by: Matthias Wilhelm <[email protected]> * [Discover] add filters validation * [Discover] fix ci * [Discover] fix checks * [Discover] fix ci * Update src/plugins/discover/public/application/main/hooks/use_filters_validation.ts Co-authored-by: Matthias Wilhelm <[email protected]> * Update src/plugins/discover/public/application/main/hooks/use_filters_validation.ts Co-authored-by: Matthias Wilhelm <[email protected]> * [Discover] apply suggestions * [Discover] update wording in tests * [Discover] try to fix functional * [Discover] change test impl to get rid of flakiness * [Discover] remove flaky test Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Joe Reuter <[email protected]>
elastic · Oct 12, 2022 · 5a0e61b · 5a0e61b
1 parent 7ed9067
commit 5a0e61b
Show file tree

Hide file tree

Showing 18 changed files with 256 additions and 73 deletions.
diff --git a/src/plugins/discover/public/__mocks__/services.ts b/src/plugins/discover/public/__mocks__/services.ts
@@ -26,10 +26,12 @@ import { FORMATS_UI_SETTINGS } from '@kbn/field-formats-plugin/common';
 import { LocalStorageMock } from './local_storage_mock';
 import { fieldFormatsMock } from '@kbn/field-formats-plugin/common/mocks';
 import { dataViewsMock } from './data_views';
+import { Observable, of } from 'rxjs';
 const dataPlugin = dataPluginMock.createStartContract();
 const expressionsPlugin = expressionsPluginMock.createStartContract();
 
 dataPlugin.query.filterManager.getFilters = jest.fn(() => []);
+dataPlugin.query.filterManager.getUpdates$ = jest.fn(() => of({}) as unknown as Observable<void>);
 
 export const discoverServiceMock = {
   core: coreMock.createStart(),

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
@@ -152,9 +152,13 @@ export function DiscoverLayout({
     [filterManager, dataView, dataViews, trackUiMetric, capabilities]
   );
 
-  const onFieldEdited = useCallback(() => {
+  const onFieldEdited = useCallback(async () => {
+    if (!dataView.isPersisted()) {
+      await updateAdHocDataViewId(dataView);
+      return;
+    }
     savedSearchRefetch$.next('reset');
-  }, [savedSearchRefetch$]);
+  }, [dataView, savedSearchRefetch$, updateAdHocDataViewId]);
 
   const onDisableFilters = useCallback(() => {
     const disabledFilters = filterManager

diff --git a/src/plugins/discover/public/application/main/components/layout/discover_main_content.tsx b/src/plugins/discover/public/application/main/components/layout/discover_main_content.tsx
@@ -52,7 +52,7 @@ export interface DiscoverMainContentProps {
   isTimeBased: boolean;
   viewMode: VIEW_MODE;
   onAddFilter: DocViewFilterFn | undefined;
-  onFieldEdited: () => void;
+  onFieldEdited: () => Promise<void>;
   columns: string[];
   resizeRef: RefObject<HTMLDivElement>;
 }

diff --git a/src/plugins/discover/public/application/main/components/sidebar/discover_sidebar.tsx b/src/plugins/discover/public/application/main/components/sidebar/discover_sidebar.tsx
@@ -283,7 +283,7 @@ export function DiscoverSidebarComponent({
               },
               fieldName,
               onDelete: async () => {
-                onFieldEdited();
+                await onFieldEdited();
               },
             });
             if (setFieldEditorRef) {

diff --git a/...ugins/discover/public/application/main/components/sidebar/discover_sidebar_responsive.tsx b/...ugins/discover/public/application/main/components/sidebar/discover_sidebar_responsive.tsx
@@ -93,7 +93,7 @@ export interface DiscoverSidebarResponsiveProps {
   /**
    * callback to execute on edit runtime field
    */
-  onFieldEdited: () => void;
+  onFieldEdited: () => Promise<void>;
   /**
    * callback to execute on create dataview
    */
@@ -220,7 +220,7 @@ export function DiscoverSidebarResponsive(props: DiscoverSidebarResponsiveProps)
               },
               fieldName,
               onSave: async () => {
-                onFieldEdited();
+                await onFieldEdited();
               },
             });
             if (setFieldEditorRef) {

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
@@ -34,7 +34,7 @@ export type DiscoverTopNavProps = Pick<
   onChangeDataView: (dataView: string) => void;
   isPlainRecord: boolean;
   textBasedLanguageModeErrors?: Error;
-  onFieldEdited: () => void;
+  onFieldEdited: () => Promise<void>;
   persistDataView: (dataView: DataView) => Promise<DataView | undefined>;
   updateAdHocDataViewId: (dataView: DataView) => Promise<DataView>;
   adHocDataViewList: DataView[];
@@ -111,7 +111,7 @@ export const DiscoverTopNav = ({
                 },
                 fieldName,
                 onSave: async () => {
-                  onFieldEdited();
+                  await onFieldEdited();
                 },
               });
             }

diff --git a/src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.test.ts b/src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.test.ts
@@ -75,12 +75,18 @@ describe('useAdHocDataViews', () => {
     const hook = renderHook((d: DataView) =>
       useAdHocDataViews({
         dataView: mockDataView,
-        dataViews: mockDiscoverServices.dataViews,
         savedSearch: savedSearchMock,
         stateContainer: {
           appStateContainer: { getState: jest.fn().mockReturnValue({}) },
+          replaceUrlAppState: jest.fn(),
+          kbnUrlStateStorage: {
+            kbnUrlControls: { flush: jest.fn() },
+          },
         } as unknown as GetStateReturn,
-        onChangeDataView: jest.fn(),
+        setUrlTracking: jest.fn(),
+        dataViews: mockDiscoverServices.dataViews,
+        filterManager: mockDiscoverServices.filterManager,
+        toastNotifications: mockDiscoverServices.toastNotifications,
       })
     );
 
@@ -101,12 +107,18 @@ describe('useAdHocDataViews', () => {
     const hook = renderHook((d: DataView) =>
       useAdHocDataViews({
         dataView: mockDataView,
-        dataViews: mockDiscoverServices.dataViews,
         savedSearch: savedSearchMock,
         stateContainer: {
           appStateContainer: { getState: jest.fn().mockReturnValue({}) },
+          replaceUrlAppState: jest.fn(),
+          kbnUrlStateStorage: {
+            kbnUrlControls: { flush: jest.fn() },
+          },
         } as unknown as GetStateReturn,
-        onChangeDataView: jest.fn(),
+        setUrlTracking: jest.fn(),
+        dataViews: mockDiscoverServices.dataViews,
+        filterManager: mockDiscoverServices.filterManager,
+        toastNotifications: mockDiscoverServices.toastNotifications,
       })
     );
 

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,29 +7,36 @@
  */
 
 import { useCallback, useEffect, useState } from 'react';
-import { DataViewsContract, type DataView } from '@kbn/data-views-plugin/public';
+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 { getUiActions, getUrlTracker } from '../../../kibana_services';
 import { GetStateReturn } from '../services/discover_state';
+import { useFiltersValidation } from './use_filters_validation';
 
 export const useAdHocDataViews = ({
   dataView,
   savedSearch,
-  dataViews,
   stateContainer,
-  onChangeDataView,
+  setUrlTracking,
+  filterManager,
+  dataViews,
+  toastNotifications,
 }: {
   dataView: DataView;
   savedSearch: SavedSearch;
-  dataViews: DataViewsContract;
   stateContainer: GetStateReturn;
-  onChangeDataView: (dataViewId: string) => Promise<void>;
+  setUrlTracking: (dataView: DataView) => void;
+  dataViews: DataViewsContract;
+  filterManager: FilterManager;
+  toastNotifications: ToastsStart;
 }) => {
   const [adHocDataViewList, setAdHocDataViewList] = useState<DataView[]>(
     !dataView.isPersisted() ? [dataView] : []
@@ -44,6 +51,11 @@ export const useAdHocDataViews = ({
     }
   }, [dataView]);
 
+  /**
+   * Takes care of checking data view id references in filters
+   */
+  useFiltersValidation({ savedSearch, filterManager, toastNotifications });
+
   /**
    * When saving a saved search with an ad hoc data view, a new id needs to be generated for the data view
    * This is to prevent duplicate ids messing with our system
@@ -57,22 +69,23 @@ export const useAdHocDataViews = ({
         prev.filter((d) => d.id && dataViewToUpdate.id && d.id !== dataViewToUpdate.id)
       );
 
-      savedSearch.searchSource.setField('index', newDataView);
-
-      // update filters references
       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.replaceUrlAppState({ index: newDataView.id });
+      setUrlTracking(newDataView);
       return newDataView;
     },
-    [dataViews, savedSearch.searchSource]
+    [dataViews, setUrlTracking, stateContainer]
   );
 
   const { openConfirmSavePrompt, updateSavedSearch } =
@@ -81,22 +94,16 @@ export const useAdHocDataViews = ({
     const currentDataView = savedSearch.searchSource.getField('index')!;
     if (currentDataView && !currentDataView.isPersisted()) {
       const createdDataView = await openConfirmSavePrompt(currentDataView);
-      if (createdDataView) {
-        savedSearch.searchSource.setField('index', createdDataView);
-        await onChangeDataView(createdDataView.id!);
 
-        // update saved search with saved data view
-        if (savedSearch.id) {
-          const currentState = stateContainer.appStateContainer.getState();
-          await updateSavedSearch({ savedSearch, dataView: createdDataView, state: currentState });
-        }
-        getUrlTracker().setTrackingEnabled(true);
-        return createdDataView;
+      // update saved search with saved data view
+      if (createdDataView && savedSearch.id) {
+        const currentState = stateContainer.appStateContainer.getState();
+        await updateSavedSearch({ savedSearch, dataView: createdDataView, state: currentState });
       }
-      return undefined;
+      return createdDataView;
     }
     return currentDataView;
-  }, [stateContainer, onChangeDataView, openConfirmSavePrompt, savedSearch, updateSavedSearch]);
+  }, [stateContainer, openConfirmSavePrompt, savedSearch, updateSavedSearch]);
 
   return { adHocDataViewList, persistDataView, updateAdHocDataViewId };
 };
diff --git a/...ion/main/hooks/use_discover_state.test.ts → ...on/main/hooks/use_discover_state.test.tsx b/...ion/main/hooks/use_discover_state.test.ts → ...on/main/hooks/use_discover_state.test.tsx
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
@@ -44,7 +44,7 @@ export function useDiscoverState({
   setExpandedDoc: (doc?: DataTableRecord) => void;
   dataViewList: DataViewListItem[];
 }) {
-  const { uiSettings, data, filterManager, dataViews } = services;
+  const { uiSettings, data, filterManager, dataViews, toastNotifications } = services;
   const useNewFieldsApi = useMemo(() => !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE), [uiSettings]);
   const { timefilter } = data.query.timefilter;
 
@@ -126,10 +126,12 @@ export function useDiscoverState({
    */
   const { adHocDataViewList, persistDataView, updateAdHocDataViewId } = useAdHocDataViews({
     dataView,
-    dataViews,
     stateContainer,
     savedSearch,
-    onChangeDataView,
+    setUrlTracking,
+    dataViews,
+    toastNotifications,
+    filterManager,
   });
 
   /**

diff --git a/src/plugins/discover/public/application/main/hooks/use_filters_validation.ts b/src/plugins/discover/public/application/main/hooks/use_filters_validation.ts
@@ -0,0 +1,56 @@
+/*
+ * 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 { IToasts, ToastsStart } from '@kbn/core/public';
+import { FilterManager } from '@kbn/data-plugin/public';
+import { i18n } from '@kbn/i18n';
+import { SavedSearch } from '@kbn/saved-search-plugin/public';
+import { useEffect } from 'react';
+import { debounceTime } from 'rxjs';
+
+const addInvalidFiltersWarn = (toastNotifications: IToasts) => {
+  const warningTitle = i18n.translate('discover.invalidFiltersWarnToast.title', {
+    defaultMessage: 'Different index references',
+  });
+  toastNotifications.addWarning({
+    title: warningTitle,
+    text: i18n.translate('discover.invalidFiltersWarnToast.description', {
+      defaultMessage:
+        'Data view id references in some of the applied filters differ from the current data view.',
+    }),
+    'data-test-subj': 'invalidFiltersWarnToast',
+  });
+};
+
+export const useFiltersValidation = ({
+  savedSearch,
+  filterManager,
+  toastNotifications,
+}: {
+  savedSearch: SavedSearch;
+  filterManager: FilterManager;
+  toastNotifications: ToastsStart;
+}) => {
+  useEffect(() => {
+    const subscription = filterManager
+      .getUpdates$()
+      .pipe(debounceTime(500))
+      .subscribe(() => {
+        const currentFilters = filterManager.getFilters();
+        const dataView = savedSearch.searchSource.getField('index');
+        const areFiltersInvalid =
+          dataView &&
+          !dataView.isPersisted() &&
+          !currentFilters.every((current) => current.meta.index === dataView.id);
+        if (areFiltersInvalid) {
+          addInvalidFiltersWarn(toastNotifications);
+        }
+      });
+    return () => subscription.unsubscribe();
+  }, [filterManager, savedSearch.searchSource, toastNotifications]);
+};
diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx
@@ -376,7 +376,7 @@ export const DiscoverGrid = ({
               },
               fieldName,
               onSave: async () => {
-                onFieldEdited();
+                await onFieldEdited();
               },
             });
           }

diff --git a/test/functional/apps/discover/group1/_discover.ts b/test/functional/apps/discover/group1/_discover.ts
@@ -364,20 +364,16 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
     });
 
     describe('URL state', () => {
-      const getCurrentDataViewId = (currentUrl: string) => {
-        const [indexSubstring] = currentUrl.match(/index:[^,]*/)!;
-        const dataViewId = indexSubstring.replace('index:', '');
-        return dataViewId;
-      };
-
       it('should show a warning and fall back to the default data view when navigating to a URL with an invalid data view ID', async () => {
         await PageObjects.common.navigateToApp('discover');
         await PageObjects.timePicker.setDefaultAbsoluteRange();
         await PageObjects.header.waitUntilLoadingHasFinished();
+        const dataViewId = await PageObjects.discover.getCurrentDataViewId();
+
         const originalUrl = await browser.getCurrentUrl();
-        const dataViewId = getCurrentDataViewId(originalUrl);
         const newUrl = originalUrl.replace(dataViewId, 'invalid-data-view-id');
         await browser.get(newUrl);
+
         await PageObjects.header.waitUntilLoadingHasFinished();
         expect(await browser.getCurrentUrl()).to.be(originalUrl);
         expect(await testSubjects.exists('dscDataViewNotFoundShowDefaultWarning')).to.be(true);
@@ -387,10 +383,12 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         await PageObjects.common.navigateToApp('discover');
         await PageObjects.timePicker.setDefaultAbsoluteRange();
         const originalHash = await browser.execute<[], string>('return window.location.hash');
-        const dataViewId = getCurrentDataViewId(originalHash);
+        const dataViewId = await PageObjects.discover.getCurrentDataViewId();
+
         const newHash = originalHash.replace(dataViewId, 'invalid-data-view-id');
         await browser.execute(`window.location.hash = "${newHash}"`);
         await PageObjects.header.waitUntilLoadingHasFinished();
+
         const currentHash = await browser.execute<[], string>('return window.location.hash');
         expect(currentHash).to.be(originalHash);
         expect(await testSubjects.exists('dscDataViewNotFoundShowSavedWarning')).to.be(true);