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

Disable url tracking for dashboard #55818

Merged
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
3ac4119
Remove GlobalState from Dashboard App
Dosant Jan 21, 2020
b343638
move filters syncing utility to data plugin
Dosant Jan 22, 2020
79850bf
improve
Dosant Jan 22, 2020
7189cf6
test
Dosant Jan 22, 2020
4ad51c3
fix bug
Dosant Jan 22, 2020
74acc33
add setGlobalFilters & setGlobalFilters methods
Dosant Jan 23, 2020
f5affd5
Merge branch 'dev/filter-manager-set-app-set-global-filters' of githu…
Dosant Jan 23, 2020
4844420
introduce flag for disabling sub url tracking
flash1293 Jan 23, 2020
9c8678d
Merge remote-tracking branch 'upstream/master' into exclude-sub-url-t…
flash1293 Jan 23, 2020
e41b273
improve
Dosant Jan 23, 2020
8d0d0df
fix lint
Dosant Jan 23, 2020
df90a14
Merge branch 'exclude-sub-url-tracking' into disable-url-tracking-for…
flash1293 Jan 23, 2020
4fb7b78
start implementing local handling for sub url tracking
flash1293 Jan 24, 2020
5200092
Merge remote-tracking branch 'upstream/master' into exclude-sub-url-t…
flash1293 Jan 24, 2020
374d8b2
re-generate documentation and extend ui_app
flash1293 Jan 24, 2020
0b03d6f
Merge remote-tracking branch 'upstream/master' into exclude-sub-url-t…
flash1293 Jan 24, 2020
0a744f4
fix ui_nav_link test
flash1293 Jan 24, 2020
4b2de2a
Merge branch 'exclude-sub-url-tracking' into disable-url-tracking-for…
flash1293 Jan 24, 2020
e72edd0
move everything into one helper
flash1293 Jan 27, 2020
d671a36
continue fleshing out functionalirty
flash1293 Jan 27, 2020
9777074
Merge branch 'master' into exclude-sub-url-tracking
elasticmachine Jan 27, 2020
2a4e7ca
Merge remote-tracking branch 'upstream/master' into disable-url-track…
flash1293 Jan 28, 2020
8232a49
Merge remote-tracking branch 'upstream/master' into exclude-sub-url-t…
flash1293 Jan 28, 2020
1d77849
Merge branch 'exclude-sub-url-tracking' of github.com:flash1293/kiban…
flash1293 Jan 28, 2020
3e7c1a5
Merge branch 'exclude-sub-url-tracking' into disable-url-tracking-for…
flash1293 Jan 28, 2020
9958a9a
make it work in dashboards
flash1293 Jan 28, 2020
e1d5932
Merge remote-tracking branch 'upstream/master' into disable-url-track…
flash1293 Jan 28, 2020
44234a8
revert standard url tracker changes
flash1293 Jan 28, 2020
4921fda
Merge remote-tracking branch 'upstream/master' into disable-url-track…
flash1293 Jan 30, 2020
b28cbbd
fix types and add tests
flash1293 Jan 30, 2020
2d29fbb
fix type
flash1293 Jan 30, 2020
f63f589
dont parse URL to avoid decoding url params
flash1293 Jan 30, 2020
6d26b89
Merge remote-tracking branch 'upstream/master' into disable-url-track…
flash1293 Jan 31, 2020
25087e9
fix hashed urls
flash1293 Jan 31, 2020
508f9da
Merge remote-tracking branch 'upstream/master' into disable-url-track…
flash1293 Feb 3, 2020
9e8ef0b
PR review comments
flash1293 Feb 3, 2020
9bf5a59
Merge remote-tracking branch 'upstream/master' into disable-url-track…
flash1293 Feb 5, 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
7 changes: 1 addition & 6 deletions src/legacy/core_plugins/kibana/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,8 @@ export default function(kibana) {
}),
order: -1001,
url: `${kbnBaseUrl}#/dashboards`,
// The subUrlBase is the common substring of all urls for this app. If not given, it defaults to the url
// above. This app has to use a different subUrlBase, in addition to the url above, because "#/dashboard"
// routes to a page that creates a new dashboard. When we introduced a landing page, we needed to change
// the url above in order to preserve the original url for BWC. The subUrlBase helps the Chrome api nav
// to determine what url to use for the app link.
subUrlBase: `${kbnBaseUrl}#/dashboard`,
euiIconType: 'dashboardApp',
disableSubUrlTracking: true,
category: DEFAULT_APP_CATEGORIES.analyze,
},
{
Expand Down
70 changes: 41 additions & 29 deletions src/legacy/core_plugins/kibana/public/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,15 @@
* under the License.
*/

const topLevelConfig = require('../../../../../.eslintrc.js');
const path = require('path');

const topLevelRestricedZones = topLevelConfig.overrides.find(
override =>
override.files[0] === '**/*.{js,ts,tsx}' &&
Object.keys(override.rules)[0] === '@kbn/eslint/no-restricted-paths'
).rules['@kbn/eslint/no-restricted-paths'][1].zones;

/**
* Builds custom restricted paths configuration for the shimmed plugins within the kibana plugin.
* These custom rules extend the default checks in the top level `eslintrc.js` by also checking two other things:
Expand All @@ -28,34 +35,37 @@ const path = require('path');
* @returns zones configuration for the no-restricted-paths linter
*/
function buildRestrictedPaths(shimmedPlugins) {
return shimmedPlugins.map(shimmedPlugin => ([{
target: [
`src/legacy/core_plugins/kibana/public/${shimmedPlugin}/np_ready/**/*`,
],
from: [
'ui/**/*',
'src/legacy/ui/**/*',
'src/legacy/core_plugins/kibana/public/**/*',
'src/legacy/core_plugins/data/public/**/*',
'!src/legacy/core_plugins/data/public/index.ts',
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/**/*`,
],
allowSameFolder: false,
errorMessage: `${shimmedPlugin} is a shimmed plugin that is not allowed to import modules from the legacy platform. If you need legacy modules for the transition period, import them either in the legacy_imports, kibana_services or index module.`,
}, {
target: [
'src/**/*',
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/**/*`,
'x-pack/**/*',
],
from: [
`src/legacy/core_plugins/kibana/public/${shimmedPlugin}/**/*`,
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/index.ts`,
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/legacy.ts`,
],
allowSameFolder: false,
errorMessage: `kibana/public/${shimmedPlugin} is behaving like a NP plugin and does not allow deep imports. If you need something from within ${shimmedPlugin} in another plugin, consider re-exporting it from the top level index module`,
}])).reduce((acc, part) => [...acc, ...part], []);
return shimmedPlugins
.map(shimmedPlugin => [
{
target: [`src/legacy/core_plugins/kibana/public/${shimmedPlugin}/np_ready/**/*`],
from: [
'ui/**/*',
'src/legacy/ui/**/*',
'src/legacy/core_plugins/kibana/public/**/*',
'src/legacy/core_plugins/data/public/**/*',
'!src/legacy/core_plugins/data/public/index.ts',
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/**/*`,
],
allowSameFolder: false,
errorMessage: `${shimmedPlugin} is a shimmed plugin that is not allowed to import modules from the legacy platform. If you need legacy modules for the transition period, import them either in the legacy_imports, kibana_services or index module.`,
},
{
target: [
'src/**/*',
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/**/*`,
'x-pack/**/*',
],
from: [
`src/legacy/core_plugins/kibana/public/${shimmedPlugin}/**/*`,
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/index.ts`,
`!src/legacy/core_plugins/kibana/public/${shimmedPlugin}/legacy.ts`,
],
allowSameFolder: false,
errorMessage: `kibana/public/${shimmedPlugin} is behaving like a NP plugin and does not allow deep imports. If you need something from within ${shimmedPlugin} in another plugin, consider re-exporting it from the top level index module`,
},
])
.reduce((acc, part) => [...acc, ...part], []);
}

module.exports = {
Expand All @@ -66,7 +76,9 @@ module.exports = {
'error',
{
basePath: path.resolve(__dirname, '../../../../../'),
zones: buildRestrictedPaths(['visualize', 'discover', 'dashboard', 'devTools', 'home']),
zones: topLevelRestricedZones.concat(
buildRestrictedPaths(['visualize', 'discover', 'dashboard', 'devTools', 'home'])
),
},
],
},
Expand Down
1 change: 1 addition & 0 deletions src/legacy/core_plugins/kibana/public/dashboard/legacy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ async function getAngularDependencies(): Promise<LegacyAngularInjectedDependenci
const instance = plugin({} as PluginInitializerContext);
instance.setup(npSetup.core, {
...npSetup.plugins,
npData: npSetup.plugins.data,
__LEGACY: {
getAngularDependencies,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ export const renderApp = (element: HTMLElement, appBasePath: string, deps: Rende
angularModuleInstance = createLocalAngularModule(deps.core, deps.navigation);
// global routing stuff
configureAppAngularModule(angularModuleInstance, deps.core as LegacyCoreStart, true);
// custom routing stuff
initDashboardApp(angularModuleInstance, deps);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { searchSourceMock } from '../../../../../../../plugins/data/public/search/search_source/mocks';
import { searchSourceMock } from '../../../../../../../plugins/data/public/mocks';
import { SavedObjectDashboard } from '../../saved_dashboard/saved_dashboard';

export function getSavedDashboardMock(
Expand Down
65 changes: 60 additions & 5 deletions src/legacy/core_plugins/kibana/public/dashboard/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/

import { BehaviorSubject } from 'rxjs';
import {
App,
CoreSetup,
Expand All @@ -28,7 +29,10 @@ import {
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 {
DataPublicPluginStart as NpDataStart,
DataPublicPluginSetup as NpDataSetup,
} from '../../../../../plugins/data/public';
import { IEmbeddableStart } from '../../../../../plugins/embeddable/public';
import { Storage } from '../../../../../plugins/kibana_utils/public';
import { NavigationPublicPluginStart as NavigationStart } from '../../../../../plugins/navigation/public';
Expand All @@ -38,8 +42,13 @@ import {
HomePublicPluginSetup,
} from '../../../../../plugins/home/public';
import { SharePluginStart } from '../../../../../plugins/share/public';
import { KibanaLegacySetup } from '../../../../../plugins/kibana_legacy/public';
import {
AngularRenderedAppUpdater,
KibanaLegacySetup,
} from '../../../../../plugins/kibana_legacy/public';
import { createSavedDashboardLoader } from './saved_dashboard/saved_dashboards';
import { createKbnUrlTracker } from '../../../../../plugins/kibana_utils/public';
import { getQueryStateContainer } from '../../../../../plugins/data/public';

export interface LegacyAngularInjectedDependencies {
dashboardConfig: any;
Expand All @@ -59,6 +68,7 @@ export interface DashboardPluginSetupDependencies {
};
home: HomePublicPluginSetup;
kibana_legacy: KibanaLegacySetup;
npData: NpDataSetup;
}

export class DashboardPlugin implements Plugin {
Expand All @@ -70,17 +80,46 @@ export class DashboardPlugin implements Plugin {
share: SharePluginStart;
} | null = null;

private appStateUpdater = new BehaviorSubject<AngularRenderedAppUpdater>(() => ({}));
private stopUrlTracking: (() => void) | undefined = undefined;

public setup(
core: CoreSetup,
{ __LEGACY: { getAngularDependencies }, home, kibana_legacy }: DashboardPluginSetupDependencies
{
__LEGACY: { getAngularDependencies },
home,
kibana_legacy,
npData,
}: DashboardPluginSetupDependencies
) {
const { querySyncStateContainer, stop: stopQuerySyncStateContainer } = getQueryStateContainer(
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we will have to add this to data plugin as part of the api. Because with current setup, if to apply this for multiple apps: each app will create a state container, which will all the time independently orchestrate data.query changes.
So we should add 1 state container on a data plugin, which apps could reuse and receive the updates.

cc @lizozom, I think you've mentioned something like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's even worse than that - the syncState helper is also creating its own state container, so even the dashboard plugin alone will have two of them running while the app is mounted. +1 for exposing it once via setup contract and using it everywhere from there.

npData.query
);
const { appMounted, appUnMounted, stop: stopUrlTracker } = createKbnUrlTracker({
baseUrl: core.http.basePath.prepend('/app/kibana'),
defaultSubUrl: '#/dashboards',
storageKey: 'lastUrl:dashboard',
navLinkUpdater$: this.appStateUpdater,
toastNotifications: core.notifications.toasts,
stateParams: [
{
kbnUrlKey: '_g',
stateUpdate$: querySyncStateContainer.state$,
},
],
});
this.stopUrlTracking = () => {
stopQuerySyncStateContainer();
stopUrlTracker();
};
const app: App = {
id: '',
title: 'Dashboards',
mount: async ({ core: contextCore }, params) => {
if (this.startDependencies === null) {
throw new Error('not started yet');
}
appMounted();
const {
savedObjectsClient,
embeddables,
Expand Down Expand Up @@ -113,10 +152,20 @@ export class DashboardPlugin implements Plugin {
localStorage: new Storage(localStorage),
};
const { renderApp } = await import('./np_ready/application');
return renderApp(params.element, params.appBasePath, deps);
const unmount = renderApp(params.element, params.appBasePath, deps);
return () => {
unmount();
appUnMounted();
};
},
};
kibana_legacy.registerLegacyApp({ ...app, id: 'dashboard' });
kibana_legacy.registerLegacyApp({
...app,
id: 'dashboard',
// only register the updater in once app, otherwise all updates would happen twice
updater$: this.appStateUpdater.asObservable(),
navLinkId: 'kibana:dashboard',
});
kibana_legacy.registerLegacyApp({ ...app, id: 'dashboards' });

home.featureCatalogue.register({
Expand Down Expand Up @@ -146,4 +195,10 @@ export class DashboardPlugin implements Plugin {
share,
};
}

stop() {
if (this.stopUrlTracking) {
this.stopUrlTracking();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import PropTypes from 'prop-types';
import { Instruction } from './instruction';
import { ParameterForm } from './parameter_form';
import { Content } from './content';
import { getDisplayText } from '../../../../../../../../plugins/home/server/tutorials/instructions/instruction_variant';
import { getDisplayText } from '../../../../../../../../plugins/home/public';
import {
EuiTabs,
EuiTab,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,17 @@ export class LocalApplicationService {
})();
},
});

if (app.updater$) {
app.updater$.subscribe(updater => {
Dosant marked this conversation as resolved.
Show resolved Hide resolved
const updatedFields = updater(app);
if (updatedFields && updatedFields.activeUrl) {
npStart.core.chrome.navLinks.update(app.navLinkId || app.id, {
url: updatedFields.activeUrl,
});
}
});
}
});

npStart.plugins.kibana_legacy.getForwards().forEach(({ legacyAppId, newAppId, keepPrefix }) => {
Expand Down
2 changes: 1 addition & 1 deletion src/legacy/ui/public/chrome/api/nav.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export function initChromeNavApi(chrome: any, internals: NavInternals) {
// link.active and link.lastUrl properties
coreNavLinks
.getAll()
.filter(link => link.subUrlBase)
.filter(link => link.subUrlBase && !link.disableSubUrlTracking)
.forEach(link => {
coreNavLinks.update(link.id, {
subUrlBase: relativeToAbsolute(chrome.addBasePath(link.subUrlBase)),
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/data/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ const createStartContract = (): Start => {
return startContract;
};

export { searchSourceMock } from './search/mocks';

export const dataPluginMock = {
createSetupContract,
createStartContract,
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/public/query/state_sync/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@
* under the License.
*/

export { syncQuery } from './sync_query';
export { syncQuery, getQueryStateContainer } from './sync_query';
export { syncAppFilters } from './sync_app_filters';
67 changes: 66 additions & 1 deletion src/plugins/data/public/query/state_sync/sync_query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
import { QueryService, QueryStart } from '../query_service';
import { StubBrowserStorage } from 'test_utils/stub_browser_storage';
import { TimefilterContract } from '../timefilter';
import { QuerySyncState, syncQuery } from './sync_query';
import { getQueryStateContainer, QuerySyncState, syncQuery } from './sync_query';

const setupMock = coreMock.createSetup();
const startMock = coreMock.createStart();
Expand Down Expand Up @@ -163,4 +163,69 @@ describe('sync_query', () => {
expect(spy).not.toBeCalled();
stop();
});

describe('getQueryStateContainer', () => {
test('state is initialized with state from query service', () => {
const { stop, querySyncStateContainer, initialState } = getQueryStateContainer(
queryServiceStart
);
expect(querySyncStateContainer.getState()).toMatchInlineSnapshot(`
Object {
"filters": Array [],
"refreshInterval": Object {
"pause": true,
"value": 0,
},
"time": Object {
"from": "now-15m",
"to": "now",
},
}
`);
expect(initialState).toEqual(querySyncStateContainer.getState());
stop();
});

test('state takes initial overrides into account', () => {
const { stop, querySyncStateContainer, initialState } = getQueryStateContainer(
queryServiceStart,
{
time: { from: 'now-99d', to: 'now' },
}
);
expect(querySyncStateContainer.getState().time).toEqual({
from: 'now-99d',
to: 'now',
});
expect(initialState).toEqual(querySyncStateContainer.getState());
stop();
});

test('when filters change, state container contains updated global filters', () => {
const { stop, querySyncStateContainer } = getQueryStateContainer(queryServiceStart);
filterManager.setFilters([gF, aF]);
expect(querySyncStateContainer.getState().filters).toHaveLength(1);
stop();
});

test('when time range changes, state container contains updated time range', () => {
const { stop, querySyncStateContainer } = getQueryStateContainer(queryServiceStart);
timefilter.setTime({ from: 'now-30m', to: 'now' });
expect(querySyncStateContainer.getState().time).toEqual({
from: 'now-30m',
to: 'now',
});
stop();
});

test('when refresh interval changes, state container contains updated refresh interval', () => {
const { stop, querySyncStateContainer } = getQueryStateContainer(queryServiceStart);
timefilter.setRefreshInterval({ pause: true, value: 100 });
expect(querySyncStateContainer.getState().refreshInterval).toEqual({
pause: true,
value: 100,
});
stop();
});
});
});
Loading