[Dashboard De-Angular] Fix dashboard save and back button functional …

…test (#4491) * fix copy on save and functional test 5 Signed-off-by: abbyhu2000 <[email protected]> * Fix back button navigation -- Fix version migration for panels -- Fix conversions between saved panel and container panel type -- Fix redundant browser update by re-structure app state and global state sync logic in order for back button to work, also fix the corresponding functional test Signed-off-by: abbyhu2000 <[email protected]> * migration version Signed-off-by: abbyhu2000 <[email protected]> * Add initialization dirty flag and fix full mode filter bar Signed-off-by: abbyhu2000 <[email protected]> * Add initialization dirty flag and fix full mode filter bar Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]>
opensearch-project · Jul 5, 2023 · 444ab79 · 444ab79
1 parent e43170f
commit 444ab79
Show file tree

Hide file tree

Showing 15 changed files with 118 additions and 81 deletions.
diff --git a/src/plugins/dashboard/common/migrate_to_730_panels.ts b/src/plugins/dashboard/common/migrate_to_730_panels.ts
@@ -87,8 +87,11 @@ function is640To720Panel(
   panel: unknown | RawSavedDashboardPanel640To720
 ): panel is RawSavedDashboardPanel640To720 {
   return (
-    semver.satisfies((panel as RawSavedDashboardPanel630).version, '>6.3') &&
-    semver.satisfies((panel as RawSavedDashboardPanel630).version, '<7.3')
+    semver.satisfies(
+      semver.coerce((panel as RawSavedDashboardPanel630).version)!.version,
+      '>6.3'
+    ) &&
+    semver.satisfies(semver.coerce((panel as RawSavedDashboardPanel630).version)!.version, '<7.3')
   );
 }
 
@@ -273,10 +276,12 @@ function migrate640To720PanelsToLatest(
   version: string
 ): RawSavedDashboardPanel730ToLatest {
   const panelIndex = panel.panelIndex ? panel.panelIndex.toString() : uuid.v4();
+  const embeddableConfig = panel.embeddableConfig ?? {};
   return {
     ...panel,
     version,
     panelIndex,
+    embeddableConfig,
   };
 }
 

diff --git a/src/plugins/dashboard/public/application/app.tsx b/src/plugins/dashboard/public/application/app.tsx
@@ -11,37 +11,16 @@
 
 import './app.scss';
 import { AppMountParameters } from 'opensearch-dashboards/public';
-import React, { useEffect } from 'react';
-import { Route, Switch, useLocation } from 'react-router-dom';
+import React from 'react';
+import { Route, Switch } from 'react-router-dom';
 import { DashboardConstants, createDashboardEditUrl } from '../dashboard_constants';
 import { DashboardEditor, DashboardListing, DashboardNoMatch } from './components';
-import { useOpenSearchDashboards } from '../../../opensearch_dashboards_react/public';
-import { DashboardServices } from '../types';
-import { syncQueryStateWithUrl } from '../../../data/public';
 
 export interface DashboardAppProps {
   onAppLeave: AppMountParameters['onAppLeave'];
 }
 
 export const DashboardApp = ({ onAppLeave }: DashboardAppProps) => {
-  const {
-    services: {
-      data: { query },
-      osdUrlStateStorage,
-    },
-  } = useOpenSearchDashboards<DashboardServices>();
-  const { pathname } = useLocation();
-
-  useEffect(() => {
-    // syncs `_g` portion of url with query services
-    const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);
-
-    return () => stop();
-
-    // this effect should re-run when pathname is changed to preserve querystring part,
-    // so the global state is always preserved
-  }, [query, osdUrlStateStorage, pathname]);
-
   return (
     <Switch>
       <Route path={[DashboardConstants.CREATE_NEW_DASHBOARD_URL, createDashboardEditUrl(':id')]}>

diff --git a/src/plugins/dashboard/public/application/components/dashboard_editor.tsx b/src/plugins/dashboard/public/application/components/dashboard_editor.tsx
@@ -11,7 +11,7 @@ import { useChromeVisibility } from '../utils/use/use_chrome_visibility';
 import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
 import { useSavedDashboardInstance } from '../utils/use/use_saved_dashboard_instance';
 import { DashboardServices } from '../../types';
-import { useDashboardAppState } from '../utils/use/use_dashboard_app_state';
+import { useDashboardAppAndGlobalState } from '../utils/use/use_dashboard_app_state';
 import { useDashboardContainer } from '../utils/use/use_dashboard_container';
 import { useEditorUpdates } from '../utils/use/use_editor_updates';
 import {
@@ -33,7 +33,11 @@ export const DashboardEditor = () => {
     dashboardIdFromUrl
   );
 
-  const { appState } = useDashboardAppState(services, eventEmitter, savedDashboardInstance);
+  const { appState } = useDashboardAppAndGlobalState(
+    services,
+    eventEmitter,
+    savedDashboardInstance
+  );
 
   const { dashboardContainer } = useDashboardContainer(
     services,
@@ -54,19 +58,19 @@ export const DashboardEditor = () => {
   );
 
   useEffect(() => {
-    if (appState) {
+    if (appState && dashboard) {
       if (savedDashboardInstance?.id) {
         chrome.setBreadcrumbs(
           setBreadcrumbsForExistingDashboard(
             savedDashboardInstance.title,
             appState?.getState().viewMode,
-            appState?.getState().isDirty
+            dashboard.isDirty
           )
         );
         chrome.docTitle.change(savedDashboardInstance.title);
       } else {
         chrome.setBreadcrumbs(
-          setBreadcrumbsForNewDashboard(appState?.getState().viewMode, appState?.getState().isDirty)
+          setBreadcrumbsForNewDashboard(appState?.getState().viewMode, dashboard.isDirty)
         );
       }
     }
@@ -85,7 +89,9 @@ export const DashboardEditor = () => {
   console.log('appStateData', appState?.getState());
   console.log('currentAppState', currentAppState);
   console.log('isEmbeddableRendered', isEmbeddableRendered);
-  console.log('app state isDirty', appState?.getState().isDirty);
+  if (dashboard) {
+    console.log('isDirty', dashboard.isDirty);
+  }
   console.log('dashboardContainer', dashboardContainer);
 
   return (

diff --git a/src/plugins/dashboard/public/application/components/dashboard_listing.tsx b/src/plugins/dashboard/public/application/components/dashboard_listing.tsx
@@ -16,6 +16,7 @@ import { DashboardConstants, createDashboardEditUrl } from '../../dashboard_cons
 import { DashboardServices } from '../../types';
 import { getTableColumns } from '../utils/get_table_columns';
 import { getNoItemsMessage } from '../utils/get_no_items_message';
+import { syncQueryStateWithUrl } from '../../../../data/public';
 
 export const EMPTY_FILTER = '';
 
@@ -32,6 +33,8 @@ export const DashboardListing = () => {
       notifications,
       savedDashboards,
       dashboardProviders,
+      data: { query },
+      osdUrlStateStorage,
     },
   } = useOpenSearchDashboards<DashboardServices>();
 
@@ -40,6 +43,16 @@ export const DashboardListing = () => {
   const initialFiltersFromURL = queryParameters.get('filter');
   const [initialFilter, setInitialFilter] = useState<string | null>(initialFiltersFromURL);
 
+  useEffect(() => {
+    // syncs `_g` portion of url with query services
+    const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);
+
+    return () => stop();
+
+    // this effect should re-run when pathname is changed to preserve querystring part,
+    // so the global state is always preserved
+  }, [query, osdUrlStateStorage, location]);
+
   useEffect(() => {
     const getDashboardsBasedOnUrl = async () => {
       const title = queryParameters.get('title');

diff --git a/src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx b/src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx
@@ -105,7 +105,7 @@ const TopNav = ({
     savedDashboardInstance,
     stateContainer,
     isEmbeddableRendered,
-    dashboard
+    dashboard,
   ]);
 
   useEffect(() => {
@@ -140,7 +140,7 @@ const TopNav = ({
   }, [dashboardContainer, stateContainer, currentAppState, services.data.indexPatterns]);
 
   const shouldShowFilterBar = (forceHide: boolean): boolean =>
-    !forceHide && (filters!.length > 0 || !currentAppState?.fullScreenMode);
+    !forceHide && (currentAppState.filters!.length > 0 || !currentAppState?.fullScreenMode);
 
   const forceShowTopNavMenu = shouldForceDisplay(UrlParams.SHOW_TOP_MENU);
   const forceShowQueryInput = shouldForceDisplay(UrlParams.SHOW_QUERY_INPUT);

diff --git a/src/plugins/dashboard/public/application/lib/embeddable_saved_object_converters.ts b/src/plugins/dashboard/public/application/lib/embeddable_saved_object_converters.ts
@@ -52,17 +52,18 @@ export function convertPanelStateToSavedDashboardPanel(
   panelState: DashboardPanelState,
   version: string
 ): SavedDashboardPanel {
-  const customTitle: string | undefined = panelState.explicitInput.title
-    ? (panelState.explicitInput.title as string)
-    : undefined;
+  const customTitle: string | undefined =
+    panelState.explicitInput.title !== undefined
+      ? (panelState.explicitInput.title as string)
+      : undefined;
   const savedObjectId = (panelState.explicitInput as SavedObjectEmbeddableInput).savedObjectId;
   return {
     version,
     type: panelState.type,
     gridData: panelState.gridData,
     panelIndex: panelState.explicitInput.id,
-    embeddableConfig: omit(panelState.explicitInput, ['id', 'savedObjectId']),
-    ...(customTitle && { title: customTitle }),
+    embeddableConfig: omit(panelState.explicitInput, ['id', 'savedObjectId', 'title']),
+    ...(customTitle !== undefined && { title: customTitle }),
     ...(savedObjectId !== undefined && { id: savedObjectId }),
   };
 }
diff --git a/src/plugins/dashboard/public/application/lib/get_app_state_defaults.ts b/src/plugins/dashboard/public/application/lib/get_app_state_defaults.ts
@@ -46,6 +46,5 @@ export function getAppStateDefaults(
     query: savedDashboard.getQuery(),
     filters: savedDashboard.getFilters(),
     viewMode: savedDashboard.id || hideWriteControls ? ViewMode.VIEW : ViewMode.EDIT,
-    isDirty: false,
   };
 }
diff --git a/src/plugins/dashboard/public/application/lib/migrate_app_state.ts b/src/plugins/dashboard/public/application/lib/migrate_app_state.ts
@@ -79,7 +79,15 @@ export function migrateAppState(
       usageCollection.reportUiStats('DashboardPanelVersionInUrl', METRIC_TYPE.LOADED, `${version}`);
     }
 
-    return semver.satisfies(version, '<7.3');
+    // Adding this line to parse versions such as "7.0.0-alpha1"
+    const cleanVersion = semver.coerce(version);
+    if (cleanVersion?.version) {
+      // Only support migration for version 6.0 - 7.2
+      // We do not need to migrate OpenSearch version 1.x, 2.x, or 3.x since the panel structure
+      // is the same as previous version 7.3
+      return semver.satisfies(cleanVersion, '<7.3') && semver.satisfies(cleanVersion, '>6.0');
+    }
+    return true;
   });
 
   if (panelNeedsMigration) {
@@ -98,5 +106,9 @@ export function migrateAppState(
     delete appState.uiState;
   }
 
+  appState.panels.forEach((panel) => {
+    panel.version = opensearchDashboardsVersion;
+  });
+
   return appState;
 }
diff --git a/src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx b/src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx
@@ -17,8 +17,9 @@ import {
 } from '../../types';
 import { ViewMode } from '../../embeddable_plugin';
 import { getDashboardIdFromUrl } from '../lib';
+import { syncQueryStateWithUrl } from '../../../../data/public';
 
-const STATE_STORAGE_KEY = '_a';
+const APP_STATE_STORAGE_KEY = '_a';
 
 interface Arguments {
   osdUrlStateStorage: IOsdUrlStateStorage;
@@ -27,14 +28,27 @@ interface Arguments {
   instance: any;
 }
 
-export const createDashboardAppState = ({
+export const createDashboardGlobalAndAppState = ({
   stateDefaults,
   osdUrlStateStorage,
   services,
   instance,
 }: Arguments) => {
-  const urlState = osdUrlStateStorage.get<DashboardAppState>(STATE_STORAGE_KEY);
-  const { opensearchDashboardsVersion, usageCollection, history } = services;
+  const urlState = osdUrlStateStorage.get<DashboardAppState>(APP_STATE_STORAGE_KEY);
+  const {
+    opensearchDashboardsVersion,
+    usageCollection,
+    history,
+    data: { query },
+  } = services;
+
+  /* 
+  Function migrateAppState() does two things
+  1. Migrate panel before version 7.3.0 to the 7.3.0 panel structure.
+     There are no changes to the panel structure after version 7.3.0 to the current 
+     OpenSearch version so no need to migrate panels that are version 7.3.0 or higher
+  2. Update the version number on each panel to the current version.
+  */
   const initialState = migrateAppState(
     {
       ...stateDefaults,
@@ -55,14 +69,6 @@ export const createDashboardAppState = ({
     }),
     // setDashboard: (state)
   } as DashboardAppStateTransitions;
-  /*
-     make sure url ('_a') matches initial state
-     Initializing appState does two things - first it translates the defaults into AppState,
-     second it updates appState based on the url (the url trumps the defaults). This means if
-     we update the state format at all and want to handle BWC, we must not only migrate the
-     data stored with saved vis, but also any old state in the url.
-   */
-  osdUrlStateStorage.set(STATE_STORAGE_KEY, initialState, { replace: true });
 
   const stateContainer = createStateContainer<DashboardAppState, DashboardAppStateTransitions>(
     initialState,
@@ -78,7 +84,7 @@ export const createDashboardAppState = ({
   };
 
   const { start: startStateSync, stop: stopStateSync } = syncState({
-    storageKey: STATE_STORAGE_KEY,
+    storageKey: APP_STATE_STORAGE_KEY,
     stateContainer: {
       ...stateContainer,
       get: () => toUrlState(stateContainer.get()),
@@ -112,7 +118,27 @@ export const createDashboardAppState = ({
     stateStorage: osdUrlStateStorage,
   });
 
+  // starts syncing `_g` portion of url with query services
+  // it is important to start this syncing after we set the time filter if timeRestore = true
+  // otherwise it will case redundant browser history records and browser navigation like going back will not work correctly
+  const { stop: stopSyncingQueryServiceStateWithUrl } = syncQueryStateWithUrl(
+    query,
+    osdUrlStateStorage
+  );
+
+  /*
+     make sure url ('_a') matches initial state
+     Initializing appState does two things - first it translates the defaults into AppState,
+     second it updates appState based on the url (the url trumps the defaults). This means if
+     we update the state format at all and want to handle BWC, we must not only migrate the
+     data stored with saved vis, but also any old state in the url.
+   */
+  osdUrlStateStorage.set(APP_STATE_STORAGE_KEY, toUrlState(initialState), { replace: true });
+
+  // immediately forces scheduled updates and changes location
+  osdUrlStateStorage.flush({ replace: true });
+
   // start syncing the appState with the ('_a') url
   startStateSync();
-  return { stateContainer, stopStateSync };
+  return { stateContainer, stopStateSync, stopSyncingQueryServiceStateWithUrl };
 };
diff --git a/src/plugins/dashboard/public/application/utils/get_nav_actions.tsx b/src/plugins/dashboard/public/application/utils/get_nav_actions.tsx
@@ -99,7 +99,7 @@ export const getNavActions = (
       stateContainer.transitions.set('title', newTitle);
       stateContainer.transitions.set('description', newDescription);
       stateContainer.transitions.set('timeRestore', newTimeRestore);
-      // dashboardStateManager.savedDashboard.copyOnSave = newCopyOnSave;
+      savedDashboard.copyOnSave = newCopyOnSave;
 
       const saveOptions = {
         confirmOverwrite: false,
@@ -114,8 +114,8 @@ export const getNavActions = (
           stateContainer.transitions.set('timeRestore', currentTimeRestore);
         }
 
-        // If the save was successfull, then set the app state isDirty back to false
-        stateContainer.transitions.set('isDirty', false);
+        // If the save was successfull, then set the dashboard isDirty back to false
+        dashboard.isDirty = false;
         return response;
       });
     };
@@ -283,7 +283,7 @@ export const getNavActions = (
         sharingData: {
           title: savedDashboard.title,
         },
-        isDirty: false, // TODO
+        isDirty: dashboard.isDirty,
         embedUrlParamExtensions: [
           {
             paramName: 'embed',
@@ -297,7 +297,7 @@ export const getNavActions = (
   function onChangeViewMode(newMode: ViewMode) {
     const isPageRefresh = newMode === appState.viewMode;
     const isLeavingEditMode = !isPageRefresh && newMode === ViewMode.VIEW;
-    const willLoseChanges = isLeavingEditMode && stateContainer.getState().isDirty === true;
+    const willLoseChanges = isLeavingEditMode && dashboard.isDirty === true;
 
     // If there are no changes, do not show the discard window
     if (!willLoseChanges) {
@@ -340,7 +340,7 @@ export const getNavActions = (
       }
 
       // Set the isDirty flag back to false since we discard all the changes
-      stateContainer.transitions.set('isDirty', false);
+      dashboard.isDirty = false;
     }
 
     overlays