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

[ML] Fix navigation for the Basic licence #139469

Merged
merged 6 commits into from
Aug 26, 2022

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Aug 25, 2022

Summary

Fixes #139279

Previously when running with a basic licence, when you landed on the ML app from certain apps, e.g. Discover or Stack Monitoring, it would get stuck in a redirect loop showing the Page Not Found warning message.

Checklist

@darnautov darnautov self-assigned this Aug 25, 2022
@darnautov darnautov requested a review from a team as a code owner August 25, 2022 11:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@darnautov darnautov requested a review from pheyos August 25, 2022 12:52
@darnautov darnautov requested a review from pheyos August 25, 2022 14:21
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional tests LGTM

@@ -287,7 +288,7 @@ export async function cloneJob(jobId) {
);
}

window.location.href = '#/jobs/new_job';
getApplication().navigateToApp('ml', { path: ML_PAGES.ANOMALY_DETECTION_CREATE_JOB });
Copy link
Member

Choose a reason for hiding this comment

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

PLUGIN_ID could be used here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in e8767d4

}

private redirectToBasic() {
return this.application.navigateToApp('ml', { path: ML_PAGES.DATA_VISUALIZER });
Copy link
Member

Choose a reason for hiding this comment

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

PLUGIN_ID could be used here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in e8767d4

super();
}

private redirectToKibana() {
Copy link
Member

Choose a reason for hiding this comment

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

I believe these should still return Promise.reject(); as the originals did.
They are used in the page loading resolver, if any of the promises reject at he beginning, the function will return early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in e8767d4

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM. Did not hit the redirect loop with any of the navigation between apps that was happening previously.

@darnautov darnautov enabled auto-merge (squash) August 26, 2022 12:29
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.3MB 3.3MB +244.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 40.2KB 40.4KB +147.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @darnautov

@darnautov darnautov merged commit 62578bd into elastic:main Aug 26, 2022
@darnautov darnautov deleted the ml-139279-fix-nagivation branch August 26, 2022 13:26
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 26, 2022
* fix redirect to the data vis

* update jest tests

* remove window.location.href from the clone action

* assert the redirect

* update power user tests

* use plugin id const, return promise reject

(cherry picked from commit 62578bd)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

darnautov added a commit that referenced this pull request Aug 29, 2022
* fix redirect to the data vis

* update jest tests

* remove window.location.href from the clone action

* assert the redirect

* update power user tests

* use plugin id const, return promise reject

(cherry picked from commit 62578bd)

Co-authored-by: Dima Arnautov <[email protected]>
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
* fix redirect to the data vis

* update jest tests

* remove window.location.href from the clone action

* assert the redirect

* update power user tests

* use plugin id const, return promise reject
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Fix navigation for the Basic licence
7 participants