Skip to content

[ML] Fix regression with some links not opening in new tab #80785

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 10 commits into from
Oct 19, 2020
Merged

[ML] Fix regression with some links not opening in new tab #80785

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
65e86d4
[ML] Fix so View series will open page in new tab instead of re-direc…
qn895 Oct 15, 2020
257835a
[ML] Open link in annotation in new tab
qn895 Oct 15, 2020
b8052fe
[ML] Fix links in data visualizer to allow to open in new tab
qn895 Oct 15, 2020
36cc49a
[ML] Fix broken calendar link in management
qn895 Oct 15, 2020
9e68a05
Revert "[ML] Open link in annotation in new tab"
qn895 Oct 15, 2020
66e775a
[ML] Fix single explorer chart link
qn895 Oct 15, 2020
b5418b6
[ML] Clean up console
qn895 Oct 15, 2020
0064254
[ML] Add checks to data visualizer results, change jobs_list_page to …
qn895 Oct 16, 2020
ac8ffcc
Merge remote-tracking branch 'upstream/master' into ml-fix-link-regre…
qn895 Oct 16, 2020
df14a3a
Merge branch 'master' into ml-fix-link-regression
kibanamachine Oct 19, 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
24 changes: 15 additions & 9 deletions x-pack/plugins/ml/public/application/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import { AppMountParameters, CoreStart, HttpStart } from 'kibana/public';

import { Storage } from '../../../../../src/plugins/kibana_utils/public';

import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public';
import {
KibanaContextProvider,
RedirectAppLinks,
} from '../../../../../src/plugins/kibana_react/public';
import { setDependencyCache, clearCache } from './util/dependency_cache';
import { setLicenseCache } from './license';
import { MlSetupDependencies, MlStartDependencies } from '../plugin';
Expand All @@ -21,7 +24,6 @@ import { MlRouter } from './routing';
import { mlApiServicesProvider } from './services/ml_api_service';
import { HttpService } from './services/http_service';
import { ML_APP_URL_GENERATOR, ML_PAGES } from '../../common/constants/ml_url_generator';

export type MlDependencies = Omit<MlSetupDependencies, 'share' | 'indexPatternManagement'> &
MlStartDependencies;

Expand Down Expand Up @@ -80,13 +82,17 @@ const App: FC<AppProps> = ({ coreStart, deps, appMountParams }) => {

const I18nContext = coreStart.i18n.Context;
return (
<I18nContext>
<KibanaContextProvider
services={{ ...services, mlServices: getMlGlobalServices(coreStart.http) }}
>
<MlRouter pageDeps={pageDeps} />
</KibanaContextProvider>
</I18nContext>
/** RedirectAppLinks intercepts all <a> tags to use navigateToUrl
* avoiding full page reload **/
<RedirectAppLinks application={coreStart.application}>
<I18nContext>
<KibanaContextProvider
services={{ ...services, mlServices: getMlGlobalServices(coreStart.http) }}
>
<MlRouter pageDeps={pageDeps} />
</KibanaContextProvider>
</I18nContext>
</RedirectAppLinks>
);
};

Expand Down
9 changes: 2 additions & 7 deletions x-pack/plugins/ml/public/application/components/anomalies_table/links_menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import { formatHumanReadableDateTimeSeconds } from '../../../../common/util/date
import { getIndexPatternIdFromName } from '../../util/index_utils';
import { replaceStringTokens } from '../../util/string_utils';
import { ML_APP_URL_GENERATOR, ML_PAGES } from '../../../../common/constants/ml_url_generator';
import { PLUGIN_ID } from '../../../../common/constants/app';
/*
* Component for rendering the links menu inside a cell in the anomalies table.
*/
Expand Down Expand Up @@ -147,8 +146,6 @@ class LinksMenuUI extends Component {
viewSeries = async () => {
const {
services: {
application: { navigateToApp },

share: {
urlGenerators: { getUrlGenerator },
},
Expand Down Expand Up @@ -185,7 +182,7 @@ class LinksMenuUI extends Component {
}

const singleMetricViewerLink = await mlUrlGenerator.createUrl({
excludeBasePath: true,
excludeBasePath: false,
page: ML_PAGES.SINGLE_METRIC_VIEWER,
pageState: {
jobIds: [record.job_id],
Expand All @@ -211,9 +208,7 @@ class LinksMenuUI extends Component {
},
},
});
await navigateToApp(PLUGIN_ID, {
path: singleMetricViewerLink,
});
window.open(singleMetricViewerLink, '_blank');
Copy link
Contributor

@darnautov darnautov Oct 16, 2020

Choose a reason for hiding this comment

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

shall we use MlHref instead of window.open?

Copy link
Member Author

@qn895 qn895 Oct 16, 2020

Choose a reason for hiding this comment

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

We'll keep this one here for now for 7.10.0.

};

viewExamples = () => {
Expand Down
157 changes: 83 additions & 74 deletions ...l/public/application/datavisualizer/file_based/components/results_links/results_links.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { FC, useState, useEffect, useCallback } from 'react';
import React, { FC, useState, useEffect } from 'react';
import moment from 'moment';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiFlexGroup, EuiFlexItem, EuiCard, EuiIcon } from '@elastic/eui';
import { ml } from '../../../../services/ml_api_service';
import { isFullLicense } from '../../../../license';
import { checkPermission } from '../../../../capabilities/check_capabilities';
import { mlNodesAvailable } from '../../../../ml_nodes_check/check_ml_nodes';
import { useMlKibana, useMlUrlGenerator, useNavigateToPath } from '../../../../contexts/kibana';
import { useMlKibana, useMlUrlGenerator } from '../../../../contexts/kibana';
import { ML_PAGES } from '../../../../../../common/constants/ml_url_generator';
import { MlCommonGlobalState } from '../../../../../../common/types/ml_url_generator';
import {
Expand Down Expand Up @@ -45,12 +45,16 @@ export const ResultsLinks: FC<Props> = ({
const [globalState, setGlobalState] = useState<MlCommonGlobalState | undefined>();

const [discoverLink, setDiscoverLink] = useState('');
const [indexManagementLink, setIndexManagementLink] = useState('');
const [indexPatternManagementLink, setIndexPatternManagementLink] = useState('');
const [dataVisualizerLink, setDataVisualizerLink] = useState('');
const [createJobsSelectTypePage, setCreateJobsSelectTypePage] = useState('');

const mlUrlGenerator = useMlUrlGenerator();
const navigateToPath = useNavigateToPath();

const {
services: {
application: { navigateToApp, navigateToUrl },
application: { getUrlForApp },
share: {
urlGenerators: { getUrlGenerator },
},
Expand Down Expand Up @@ -84,35 +88,52 @@ export const ResultsLinks: FC<Props> = ({
setDiscoverLink(discoverUrl);
}
};

const getDataVisualizerLink = async (): Promise<void> => {
const _dataVisualizerLink = await mlUrlGenerator.createUrl({
page: ML_PAGES.DATA_VISUALIZER_INDEX_VIEWER,
pageState: {
index: indexPatternId,
globalState,
},
});
if (!unmounted) {
setDataVisualizerLink(_dataVisualizerLink);
}
};
const getADCreateJobsSelectTypePage = async (): Promise<void> => {
const _createJobsSelectTypePage = await mlUrlGenerator.createUrl({
page: ML_PAGES.ANOMALY_DETECTION_CREATE_JOB_SELECT_TYPE,
pageState: {
index: indexPatternId,
globalState,
},
});
if (!unmounted) {
setCreateJobsSelectTypePage(_createJobsSelectTypePage);
}
};

getDiscoverUrl();
getDataVisualizerLink();
getADCreateJobsSelectTypePage();

if (!unmounted) {
setIndexManagementLink(
getUrlForApp('management', { path: '/data/index_management/indices' })
);
setIndexPatternManagementLink(
getUrlForApp('management', {
path: `/kibana/indexPatterns${createIndexPattern ? `/patterns/${indexPatternId}` : ''}`,
})
);
}

return () => {
unmounted = true;
};
}, [indexPatternId, getUrlGenerator]);

const openInDataVisualizer = useCallback(async () => {
const path = await mlUrlGenerator.createUrl({
page: ML_PAGES.DATA_VISUALIZER_INDEX_VIEWER,
pageState: {
index: indexPatternId,
globalState,
},
});
await navigateToPath(path);
}, [indexPatternId, globalState]);

const redirectToADCreateJobsSelectTypePage = useCallback(async () => {
const path = await mlUrlGenerator.createUrl({
page: ML_PAGES.ANOMALY_DETECTION_CREATE_JOB_SELECT_TYPE,
pageState: {
index: indexPatternId,
globalState,
},
});
await navigateToPath(path);
}, [indexPatternId, globalState]);

useEffect(() => {
setShowCreateJobLink(checkPermission('canCreateJob') && mlNodesAvailable());
updateTimeValues();
Expand Down Expand Up @@ -148,23 +169,6 @@ export const ResultsLinks: FC<Props> = ({
}
}

function openInDiscover(e: React.MouseEvent<HTMLButtonElement>) {
e.preventDefault();
navigateToUrl(discoverLink);
}

function openIndexManagement(e: React.MouseEvent<HTMLButtonElement>) {
e.preventDefault();
navigateToApp('management', { path: '/data/index_management/indices' });
}

function openIndexPatternManagement(e: React.MouseEvent<HTMLButtonElement>) {
e.preventDefault();
navigateToApp('management', {
path: `/kibana/indexPatterns${createIndexPattern ? `/patterns/${indexPatternId}` : ''}`,
});
}

return (
<EuiFlexGroup gutterSize="l">
{createIndexPattern && discoverLink && (
Copy link
Contributor

@darnautov darnautov Oct 16, 2020

Choose a reason for hiding this comment

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

do you need to add a check for all the other links as well?

Copy link
Member Author

@qn895 qn895 Oct 16, 2020

Choose a reason for hiding this comment

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

Updated here 0064254

Expand All @@ -178,15 +182,16 @@ export const ResultsLinks: FC<Props> = ({
/>
}
description=""
onClick={openInDiscover}
href={discoverLink}
/>
</EuiFlexItem>
)}

{isFullLicense() === true &&
timeFieldName !== undefined &&
showCreateJobLink &&
createIndexPattern && (
createIndexPattern &&
createJobsSelectTypePage && (
<EuiFlexItem>
<EuiCard
icon={<EuiIcon size="xxl" type={`machineLearningApp`} />}
Expand All @@ -197,12 +202,12 @@ export const ResultsLinks: FC<Props> = ({
/>
}
description=""
onClick={redirectToADCreateJobsSelectTypePage}
href={createJobsSelectTypePage}
/>
</EuiFlexItem>
)}

{createIndexPattern && (
{createIndexPattern && dataVisualizerLink && (
<EuiFlexItem>
<EuiCard
icon={<EuiIcon size="xxl" type={`dataVisualizer`} />}
Expand All @@ -213,38 +218,42 @@ export const ResultsLinks: FC<Props> = ({
/>
}
description=""
onClick={openInDataVisualizer}
href={dataVisualizerLink}
/>
</EuiFlexItem>
)}

<EuiFlexItem>
<EuiCard
icon={<EuiIcon size="xxl" type={`managementApp`} />}
title={
<FormattedMessage
id="xpack.ml.fileDatavisualizer.resultsLinks.indexManagementTitle"
defaultMessage="Index Management"
/>
}
description=""
onClick={openIndexManagement}
/>
</EuiFlexItem>
{indexManagementLink && (
<EuiFlexItem>
<EuiCard
icon={<EuiIcon size="xxl" type={`managementApp`} />}
title={
<FormattedMessage
id="xpack.ml.fileDatavisualizer.resultsLinks.indexManagementTitle"
defaultMessage="Index Management"
/>
}
description=""
href={indexManagementLink}
/>
</EuiFlexItem>
)}

<EuiFlexItem>
<EuiCard
icon={<EuiIcon size="xxl" type={`managementApp`} />}
title={
<FormattedMessage
id="xpack.ml.fileDatavisualizer.resultsLinks.indexPatternManagementTitle"
defaultMessage="Index Pattern Management"
/>
}
description=""
onClick={openIndexPatternManagement}
/>
</EuiFlexItem>
{indexPatternManagementLink && (
<EuiFlexItem>
<EuiCard
icon={<EuiIcon size="xxl" type={`managementApp`} />}
title={
<FormattedMessage
id="xpack.ml.fileDatavisualizer.resultsLinks.indexPatternManagementTitle"
defaultMessage="Index Pattern Management"
/>
}
description=""
href={indexPatternManagementLink}
/>
</EuiFlexItem>
)}
<EuiFlexItem>
<EuiCard
icon={<EuiIcon size="xxl" type={`filebeatApp`} />}
Expand Down
14 changes: 2 additions & 12 deletions x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_charts_container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import { FormattedMessage } from '@kbn/i18n/react';
import { MlTooltipComponent } from '../../components/chart_tooltip';
import { withKibana } from '../../../../../../../src/plugins/kibana_react/public';
import { ML_APP_URL_GENERATOR } from '../../../../common/constants/ml_url_generator';
import { PLUGIN_ID } from '../../../../common/constants/app';
import { addItemToRecentlyAccessed } from '../../util/recently_accessed';

const textTooManyBuckets = i18n.translate('xpack.ml.explorer.charts.tooManyBucketsDescription', {
Expand All @@ -55,21 +54,12 @@ function getChartId(series) {
}

// Wrapper for a single explorer chart
function ExplorerChartContainer({
series,
severity,
tooManyBuckets,
wrapLabel,
navigateToApp,
mlUrlGenerator,
}) {
function ExplorerChartContainer({ series, severity, tooManyBuckets, wrapLabel, mlUrlGenerator }) {
const redirectToSingleMetricViewer = useCallback(async () => {
const singleMetricViewerLink = await getExploreSeriesLink(mlUrlGenerator, series);
addItemToRecentlyAccessed('timeseriesexplorer', series.jobId, singleMetricViewerLink);

await navigateToApp(PLUGIN_ID, {
path: singleMetricViewerLink,
});
window.open(singleMetricViewerLink, '_blank');
}, [mlUrlGenerator]);

const { detectorLabel, entityFields } = series;
Expand Down
8 changes: 5 additions & 3 deletions ...lugins/ml/public/application/jobs/jobs_list/components/job_details/extract_job_details.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import React from 'react';
import { detectorToString } from '../../../../util/string_utils';
import { formatValues, filterObjects } from './format_values';
import { i18n } from '@kbn/i18n';
import { Link } from 'react-router-dom';
import { EuiLink } from '@elastic/eui';

export function extractJobDetails(job) {
export function extractJobDetails(job, basePath) {
if (Object.keys(job).length === 0) {
return {};
}
Expand Down Expand Up @@ -61,7 +61,9 @@ export function extractJobDetails(job) {
if (job.calendars) {
calendars.items = job.calendars.map((c) => [
'',
<Link to={`/settings/calendars_list/edit_calendar/${c}?_g=()`}>{c}</Link>,
<EuiLink href={basePath.prepend(`/app/ml/settings/calendars_list/edit_calendar/${c}?_g=()`)}>
Copy link
Member

@jgowdyelastic jgowdyelastic Oct 16, 2020

Choose a reason for hiding this comment

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

rather than using Link or EuiLink here, is there a better way to create this link?
i'm asking because we don't need basePath anywhere else in this PR, so why here?

Copy link
Member Author

@qn895 qn895 Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think moving forward we will have to use the url with basePath for all the href. The EuiLink work here without causing a full page reload because we are wrapping the ml application inside RedirectAppLinks. Alternatively, we can use it as a button and navigate the user to the new page but that means user will no longer be able to ctrl+click or right click to open in new tab.

RedirectAppLinks intercepts any tag and override the onClick behavior to use navigatetoUrl, and therefore requires the href to have the full base path https://github.com/smith/kibana/blob/765a70f7cc2583d394b96015a6fd7f5ab44662ba/docs/developer/best-practices/navigation.asciidoc#L21

Also, in this particular component the basePath is needed so the link will work in the management section. If not the href would be like abc/app/management/insightsAndAlerting/jobsListLink/settings/calendars_list/edit_calendar/nov4?_g=() which is invalid. So that would mean we have to check if the table is being rendered in the ml plugin or the management section, and either prepend or not prepend the path for it to work properly.

{c}
</EuiLink>,
]);
// remove the calendars list from the general section
// so not to show it twice.
Expand Down
Loading