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

Redirect to full path when calling navigateToApp to a legacy app #65112

Merged
merged 1 commit into from
May 7, 2020

Conversation

joshdover
Copy link
Contributor

Summary

This fixes a bug where calling navigateToApp from a KP application to redirect to a legacy application would result in the path being lost.

This was happening because the "mounter" constructed for legacy applications always redirected to the legacy app's base route rather than the path that was specified in the navigateToApp call.

Dev Docs

A bug was fixed where calling core.application.navigateToApp to a legacy application would not retain the path specified.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.8.0 labels May 4, 2020
@joshdover joshdover requested a review from flash1293 May 4, 2020 16:49
@joshdover joshdover requested a review from a team as a code owner May 4, 2020 16:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@joshdover
Copy link
Contributor Author

Clearly this has broken some things. Putting back into draft.

@joshdover joshdover marked this pull request as draft May 4, 2020 22:11
@flash1293
Copy link
Contributor

flash1293 commented May 5, 2020

I haven't tested the PR, but at least the spaces error (ci group 4) could be caused by the fact that now, if navigateToApp is used in the legacy platform to go to the legacy platform, the page is always reloaded, even if it's the same legacy app.

That wasn't the case before, because if the user is on app/kibana#/discover and navigateToApp('kibana', { path: '#/management/spaces' }) is called, it would just do a hash change, not a reload. I think the test fails because it assumes this navigation is fast - the error screenshot shows the regular Kibana loading spinner. It should be enough to check whether the app actually changes by the navigateToApp call and only call window.location.reload(); when that's the case.

@joshdover
Copy link
Contributor Author

I haven't tested the PR, but at least the spaces error (ci group 4) could be caused by the fact that now, if navigateToApp is used in the legacy platform to go to the legacy platform, the page is always reloaded, even if it's the same legacy app.

That wasn't the case before, because if the user is on app/kibana#/discover and navigateToApp('kibana', { path: '#/management/spaces' }) is called, it would just do a hash change, not a reload. I think the test fails because it assumes this navigation is fast - the error screenshot shows the regular Kibana loading spinner. It should be enough to check whether the app actually changes by the navigateToApp call and only call window.location.reload(); when that's the case.

Yep you are right 😄 I will have to think of a different solution. Honestly how this was originally implemented was better since it did not require this sort of loopback from history -> router -> app container -> legacy "mounter"

@joshdover joshdover force-pushed the np/fix-redirect-to-legacy branch from 2e7df63 to 76600a4 Compare May 6, 2020 21:59
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #45124 failed 2e7df63733429db1e85bc237d5885c8e161e81f5

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Chrome - the "Manage spaces" button works fine with this PR and just redirects to home without it when clicked on a new platform app. LGTM

No thorough code review

@joshdover joshdover marked this pull request as ready for review May 7, 2020 13:44
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linking to Visualize from Metrics Explorer redirects to Kibana home
5 participants