Skip to content

[Bugfix][State Management] 'state:storeInSessionStorage' doesn’t take effect for dashboard without full page reload #56284

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 30, 2020
Merged

[Bugfix][State Management] 'state:storeInSessionStorage' doesn’t take effect for dashboard without full page reload #56284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35db869
bugfix: 'state:storeInSessionStorage' doesn’t take effect for dashboa…
Dosant Jan 29, 2020
d010abc
fix typescript
Dosant Jan 29, 2020
e663e83
add test “changing 'state:storeInSessionStorage' also takes effect wi…
Dosant Jan 29, 2020
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
5 changes: 1 addition & 4 deletions src/legacy/core_plugins/kibana/public/dashboard/np_ready/application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import { EuiConfirmModal, EuiIcon } from '@elastic/eui';
import angular, { IModule } from 'angular';
import { History } from 'history';
import { i18nDirective, i18nFilter, I18nProvider } from '@kbn/i18n/angular';
import {
AppMountContext,
Expand All @@ -28,7 +27,7 @@ import {
LegacyCoreStart,
SavedObjectsClientContract,
} from 'kibana/public';
import { IKbnUrlStateStorage, Storage } from '../../../../../../plugins/kibana_utils/public';
import { Storage } from '../../../../../../plugins/kibana_utils/public';
import {
configureAppAngularModule,
confirmModalFactory,
Expand Down Expand Up @@ -66,8 +65,6 @@ export interface RenderDeps {
embeddables: IEmbeddableStart;
localStorage: Storage;
share: SharePluginStart;
history: History;
kbnUrlStateStorage: IKbnUrlStateStorage;
}

let angularModuleInstance: IModule | null = null;
Expand Down
8 changes: 7 additions & 1 deletion src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import moment from 'moment';
import { Subscription } from 'rxjs';
import { History } from 'history';

import { IInjector } from '../legacy_imports';

Expand All @@ -35,6 +36,7 @@ import {

import { DashboardAppController } from './dashboard_app_controller';
import { RenderDeps } from './application';
import { IKbnUrlStateStorage } from '../../../../../../plugins/kibana_utils/public/';

export interface DashboardAppScope extends ng.IScope {
dash: SavedObjectDashboard;
Expand Down Expand Up @@ -96,7 +98,9 @@ export function initDashboardAppDirective(app: any, deps: RenderDeps) {
$route: any,
$routeParams: {
id?: string;
}
},
kbnUrlStateStorage: IKbnUrlStateStorage,
history: History
) =>
new DashboardAppController({
$route,
Expand All @@ -105,6 +109,8 @@ export function initDashboardAppDirective(app: any, deps: RenderDeps) {
config,
confirmModal,
indexPatterns: deps.npDataStart.indexPatterns,
kbnUrlStateStorage,
history,
...deps,
}),
};
Expand Down
9 changes: 8 additions & 1 deletion src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import angular from 'angular';

import { Subscription } from 'rxjs';
import { map } from 'rxjs/operators';
import { History } from 'history';
import { DashboardEmptyScreen, DashboardEmptyScreenProps } from './dashboard_empty_screen';

import {
Expand Down Expand Up @@ -77,7 +78,11 @@ import {
SavedObjectFinderProps,
SavedObjectFinderUi,
} from '../../../../../../plugins/kibana_react/public';
import { removeQueryParam, unhashUrl } from '../../../../../../plugins/kibana_utils/public';
import {
IKbnUrlStateStorage,
removeQueryParam,
unhashUrl,
} from '../../../../../../plugins/kibana_utils/public';

export interface DashboardAppControllerDependencies extends RenderDeps {
$scope: DashboardAppScope;
Expand All @@ -87,6 +92,8 @@ export interface DashboardAppControllerDependencies extends RenderDeps {
dashboardConfig: any;
config: any;
confirmModal: ConfirmModalFn;
history: History;
kbnUrlStateStorage: IKbnUrlStateStorage;
}

export class DashboardAppController {
Expand Down
17 changes: 13 additions & 4 deletions src/legacy/core_plugins/kibana/public/dashboard/np_ready/legacy_app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ import { i18n } from '@kbn/i18n';

import dashboardTemplate from './dashboard_app.html';
import dashboardListingTemplate from './listing/dashboard_listing_ng_wrapper.html';
import { createHashHistory } from 'history';

import { ensureDefaultIndexPattern } from '../legacy_imports';
import { initDashboardAppDirective } from './dashboard_app';
import { createDashboardEditUrl, DashboardConstants } from './dashboard_constants';
import {
createKbnUrlStateStorage,
InvalidJSONProperty,
SavedObjectNotFound,
} from '../../../../../../plugins/kibana_utils/public';
Expand Down Expand Up @@ -62,6 +64,14 @@ export function initDashboardApp(app, deps) {
stateManagementConfigProvider.disable();
});

app.factory('history', () => createHashHistory());
Copy link
Contributor

@flash1293 flash1293 Jan 30, 2020

Choose a reason for hiding this comment

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

nit: It's not a big deal, but as we are trying to get away from angular we could already do that step here and call createKbnUrlStorage directly within the controller instead of letting angular handle this. Not blocking this merge, we can also tackle that later.

Copy link
Contributor Author

@Dosant Dosant Jan 30, 2020

Choose a reason for hiding this comment

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

I needed to make sure that Listing page and Dashboard page are using the same instance of kbnUrlStateStorage.

When we clean up angular from here, these 2 just should move back somewhere to the root of the plugin and then passed down to react components.

Copy link
Contributor

@flash1293 flash1293 Jan 30, 2020

Choose a reason for hiding this comment

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

Why is this required? I'm asking because the dashboard listing page and the actual dashboard page are two separate apps - if you are switching between the two it is unmounting and re-mounting the application anyway (new angular context and so on). https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/dashboard/plugin.ts#L128

That was necessary because of the different urls - the listing page is the dashboards app, the individual dashboard is the dashboard app.

app.factory('kbnUrlStateStorage', history =>
createKbnUrlStateStorage({
history,
useHash: deps.uiSettings.get('state:storeInSessionStorage'),
})
);

app.config(function($routeProvider) {
const defaults = {
reloadOnSearch: false,
Expand All @@ -87,15 +97,15 @@ export function initDashboardApp(app, deps) {
.when(DashboardConstants.LANDING_PAGE_PATH, {
...defaults,
template: dashboardListingTemplate,
controller($injector, $location, $scope) {
controller($injector, $location, $scope, kbnUrlStateStorage) {
const service = deps.savedDashboards;
const kbnUrl = $injector.get('kbnUrl');
const dashboardConfig = deps.dashboardConfig;

// syncs `_g` portion of url with query services
const { stop: stopSyncingGlobalStateWithUrl } = syncQuery(
deps.npDataStart.query,
deps.kbnUrlStateStorage
kbnUrlStateStorage
);

$scope.listingLimit = deps.uiSettings.get('savedObjects:listingLimit');
Expand Down Expand Up @@ -189,7 +199,7 @@ export function initDashboardApp(app, deps) {
template: dashboardTemplate,
controller: createNewDashboardCtrl,
resolve: {
dash: function($rootScope, $route, redirectWhenMissing, kbnUrl) {
dash: function($rootScope, $route, redirectWhenMissing, kbnUrl, history) {
const id = $route.current.params.id;

return ensureDefaultIndexPattern(deps.core, deps.npDataStart, $rootScope, kbnUrl)
Expand All @@ -216,7 +226,6 @@ export function initDashboardApp(app, deps) {
// See https://github.com/elastic/kibana/issues/10951 for more context.
if (error instanceof SavedObjectNotFound && id === 'create') {
// Note preserve querystring part is necessary so the state is preserved through the redirect.
const history = deps.history;
history.replace({
...history.location, // preserve query,
pathname: DashboardConstants.CREATE_NEW_DASHBOARD_URL,
Expand Down
11 changes: 1 addition & 10 deletions src/legacy/core_plugins/kibana/public/dashboard/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,12 @@ import {
Plugin,
SavedObjectsClientContract,
} from 'kibana/public';
import { createHashHistory } from 'history';
import { i18n } from '@kbn/i18n';
import { RenderDeps } from './np_ready/application';
import { DataStart } from '../../../data/public';
import { DataPublicPluginStart as NpDataStart } from '../../../../../plugins/data/public';
import { IEmbeddableStart } from '../../../../../plugins/embeddable/public';
import { createKbnUrlStateStorage, Storage } from '../../../../../plugins/kibana_utils/public';
import { Storage } from '../../../../../plugins/kibana_utils/public';
import { NavigationPublicPluginStart as NavigationStart } from '../../../../../plugins/navigation/public';
import { DashboardConstants } from './np_ready/dashboard_constants';
import {
Expand Down Expand Up @@ -97,12 +96,6 @@ export class DashboardPlugin implements Plugin {
overlays: contextCore.overlays,
});

const history = createHashHistory();
const kbnUrlStateStorage = createKbnUrlStateStorage({
history,
useHash: core.uiSettings.get('state:storeInSessionStorage'),
});

const deps: RenderDeps = {
core: contextCore as LegacyCoreStart,
...angularDependencies,
Expand All @@ -118,8 +111,6 @@ export class DashboardPlugin implements Plugin {
embeddables,
dashboardCapabilities: contextCore.application.capabilities.dashboard,
localStorage: new Storage(localStorage),
history,
kbnUrlStateStorage,
};
const { renderApp } = await import('./np_ready/application');
return renderApp(params.element, params.appBasePath, deps);
Expand Down
25 changes: 16 additions & 9 deletions test/functional/apps/management/_kibana_settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import expect from '@kbn/expect';
export default function({ getService, getPageObjects }) {
const kibanaServer = getService('kibanaServer');
const browser = getService('browser');
const PageObjects = getPageObjects(['settings', 'common', 'dashboard', 'timePicker']);
const PageObjects = getPageObjects(['settings', 'common', 'dashboard', 'timePicker', 'header']);

describe('kibana settings', function describeIndexTests() {
before(async function() {
Expand Down Expand Up @@ -94,14 +94,21 @@ export default function({ getService, getPageObjects }) {
expect(appState.length).to.be.lessThan(20);
});

after(
'navigate to settings page and turn state:storeInSessionStorage back to false',
async () => {
await PageObjects.settings.navigateTo();
await PageObjects.settings.clickKibanaSettings();
await PageObjects.settings.toggleAdvancedSettingCheckbox('state:storeInSessionStorage');
}
);
it("changing 'state:storeInSessionStorage' also takes effect without full page reload", async () => {
await PageObjects.dashboard.preserveCrossAppState();
await PageObjects.header.clickStackManagement();
await PageObjects.settings.clickKibanaSettings();
await PageObjects.settings.toggleAdvancedSettingCheckbox('state:storeInSessionStorage');
await PageObjects.header.clickDashboard();
const currentUrl = await browser.getCurrentUrl();
const urlPieces = currentUrl.match(/(.*)?_g=(.*)&_a=(.*)/);
const globalState = urlPieces[2];
const appState = urlPieces[3];
// We don't have to be exact, just need to ensure it's greater than when the hashed variation is being used,
// which is less than 20 characters.
expect(globalState.length).to.be.greaterThan(20);
expect(appState.length).to.be.greaterThan(20);
});
});

after(async function() {
Expand Down