Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dashboard De-Angular] Fix dashboard save and back button functional test #4491

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/plugins/dashboard/common/migrate_to_730_panels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
);
}

Expand Down Expand Up @@ -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,
};
}

Expand Down
25 changes: 2 additions & 23 deletions src/plugins/dashboard/public/application/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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')]}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
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 {
Expand All @@ -33,7 +33,11 @@
dashboardIdFromUrl
);

const { appState } = useDashboardAppState(services, eventEmitter, savedDashboardInstance);
const { appState } = useDashboardAppAndGlobalState(

Check warning on line 36 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_editor.tsx#L36

Added line #L36 was not covered by tests
services,
eventEmitter,
savedDashboardInstance
);

const { dashboardContainer } = useDashboardContainer(
services,
Expand All @@ -54,23 +58,23 @@
);

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)
);
}
}
}, [appState?.getState(), savedDashboardInstance, chrome]);

Check failure on line 77 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

React Hook useEffect has missing dependencies: 'appState' and 'dashboard'. Either include them or remove the dependency array

Check failure on line 77 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

React Hook useEffect has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked

Check failure on line 77 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

React Hook useEffect has missing dependencies: 'appState' and 'dashboard'. Either include them or remove the dependency array

Check failure on line 77 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

React Hook useEffect has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked

useEffect(() => {
// clean up all registered listeners if any is left
Expand All @@ -79,14 +83,16 @@
};
}, [eventEmitter]);

console.log('savedDashboardInstance', savedDashboardInstance);

Check failure on line 86 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 86 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement
console.log('dashboard', dashboard);

Check failure on line 87 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 87 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement
console.log('appState', appState);

Check failure on line 88 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 88 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement
console.log('appStateData', appState?.getState());

Check failure on line 89 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 89 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement
console.log('currentAppState', currentAppState);

Check failure on line 90 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 90 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement
console.log('isEmbeddableRendered', isEmbeddableRendered);

Check failure on line 91 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 91 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement
console.log('app state isDirty', appState?.getState().isDirty);
if (dashboard) {
console.log('isDirty', dashboard.isDirty);

Check warning on line 93 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_editor.tsx#L93

Added line #L93 was not covered by tests

Check failure on line 93 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 93 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement
}
console.log('dashboardContainer', dashboardContainer);

Check failure on line 95 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Windows

Unexpected console statement

Check failure on line 95 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View workflow job for this annotation

GitHub Actions / Build and Verify on Linux

Unexpected console statement

return (
<div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
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 = '';

Expand All @@ -32,6 +33,8 @@
notifications,
savedDashboards,
dashboardProviders,
data: { query },
osdUrlStateStorage,
},
} = useOpenSearchDashboards<DashboardServices>();

Expand All @@ -40,6 +43,16 @@
const initialFiltersFromURL = queryParameters.get('filter');
const [initialFilter, setInitialFilter] = useState<string | null>(initialFiltersFromURL);

useEffect(() => {

Check warning on line 46 in src/plugins/dashboard/public/application/components/dashboard_listing.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_listing.tsx#L46

Added line #L46 was not covered by tests
// syncs `_g` portion of url with query services
const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);

Check warning on line 48 in src/plugins/dashboard/public/application/components/dashboard_listing.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_listing.tsx#L48

Added line #L48 was not covered by tests

return () => stop();

Check warning on line 50 in src/plugins/dashboard/public/application/components/dashboard_listing.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_listing.tsx#L50

Added line #L50 was not covered by tests

// 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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ const TopNav = ({
savedDashboardInstance,
stateContainer,
isEmbeddableRendered,
dashboard
dashboard,
]);

useEffect(() => {
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,5 @@ export function getAppStateDefaults(
query: savedDashboard.getQuery(),
filters: savedDashboard.getFilters(),
viewMode: savedDashboard.id || hideWriteControls ? ViewMode.VIEW : ViewMode.EDIT,
isDirty: false,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,15 @@
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;

Check warning on line 90 in src/plugins/dashboard/public/application/lib/migrate_app_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/lib/migrate_app_state.ts#L90

Added line #L90 was not covered by tests
});

if (panelNeedsMigration) {
Expand All @@ -98,5 +106,9 @@
delete appState.uiState;
}

appState.panels.forEach((panel) => {
panel.version = opensearchDashboardsVersion;
});

return appState;
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
} 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;
Expand All @@ -27,14 +28,27 @@
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);

Check warning on line 37 in src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx#L37

Added line #L37 was not covered by tests
const {
opensearchDashboardsVersion,
usageCollection,
history,
data: { query },
} = services;

Check warning on line 43 in src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx#L43

Added line #L43 was not covered by tests

/*
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,
Expand All @@ -55,14 +69,6 @@
}),
// 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,
Expand All @@ -78,7 +84,7 @@
};

const { start: startStateSync, stop: stopStateSync } = syncState({
storageKey: STATE_STORAGE_KEY,
storageKey: APP_STATE_STORAGE_KEY,
stateContainer: {
...stateContainer,
get: () => toUrlState(stateContainer.get()),
Expand Down Expand Up @@ -112,7 +118,27 @@
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(

Check warning on line 124 in src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx#L124

Added line #L124 was not covered by tests
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 });

Check warning on line 136 in src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx#L136

Added line #L136 was not covered by tests

// immediately forces scheduled updates and changes location
osdUrlStateStorage.flush({ replace: true });

Check warning on line 139 in src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx#L139

Added line #L139 was not covered by tests

// start syncing the appState with the ('_a') url
startStateSync();
return { stateContainer, stopStateSync };
return { stateContainer, stopStateSync, stopSyncingQueryServiceStateWithUrl };

Check warning on line 143 in src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx#L143

Added line #L143 was not covered by tests
};
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
stateContainer.transitions.set('title', newTitle);
stateContainer.transitions.set('description', newDescription);
stateContainer.transitions.set('timeRestore', newTimeRestore);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not specifically related to this PR but in types we can add to the DashboardAppStateTransitions

setDashboard: (state: DashboardAppState) => (dashboard: Partial<DashboardAppState>) => DashboardAppState

which should hopefully improve the run time of the application, since we listen to state changes everywhere and so each one of these cause the app state to be updated and then trigger any listeners to app state updates. So I believe in this instance it's updating the state 3 times and triggering listeners to react 3 times.

vs just adding the above and doing:

stateContainer.transitions.setDashboard({
  title: newTitle,
  description: newDescription,
  timeRestore: newTimeRestore
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea agree, was originally planning to clean that part after i test out the functionality

// dashboardStateManager.savedDashboard.copyOnSave = newCopyOnSave;
savedDashboard.copyOnSave = newCopyOnSave;

Check warning on line 102 in src/plugins/dashboard/public/application/utils/get_nav_actions.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/get_nav_actions.tsx#L102

Added line #L102 was not covered by tests

const saveOptions = {
confirmOverwrite: false,
Expand All @@ -114,8 +114,8 @@
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;

Check warning on line 118 in src/plugins/dashboard/public/application/utils/get_nav_actions.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/get_nav_actions.tsx#L118

Added line #L118 was not covered by tests
return response;
});
};
Expand Down Expand Up @@ -283,7 +283,7 @@
sharingData: {
title: savedDashboard.title,
},
isDirty: false, // TODO
isDirty: dashboard.isDirty,
embedUrlParamExtensions: [
{
paramName: 'embed',
Expand All @@ -297,7 +297,7 @@
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) {
Expand Down Expand Up @@ -340,7 +340,7 @@
}

// Set the isDirty flag back to false since we discard all the changes
stateContainer.transitions.set('isDirty', false);
dashboard.isDirty = false;

Check warning on line 343 in src/plugins/dashboard/public/application/utils/get_nav_actions.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/utils/get_nav_actions.tsx#L343

Added line #L343 was not covered by tests
}

overlays
Expand Down
Loading
Loading