Skip to content

[Index Management] Fix broken app links#71007

Merged
alisonelizabeth merged 3 commits intoelastic:masterfrom
alisonelizabeth:bugfix/im-broken-links
Jul 9, 2020
Merged

[Index Management] Fix broken app links#71007
alisonelizabeth merged 3 commits intoelastic:masterfrom
alisonelizabeth:bugfix/im-broken-links

Conversation

@alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Jul 7, 2020

This PR fixes some broken cross-app linking in Index Management.

ILM
ilm

CCR
ccr


Other changes:

I also noticed we have both a navigation and routing file. I combined these into one file; let me know if there are any objections.

@alisonelizabeth alisonelizabeth added Feature:Index Management Index and index templates UI v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 7, 2020
@alisonelizabeth alisonelizabeth marked this pull request as ready for review July 8, 2020 00:46
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner July 8, 2020 00:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

I didn't really review the code, just glanced through out of curiosity and had one comment.

<EuiDescriptionListDescription>
{ilmPolicy && ilmPolicy.name ? (
<EuiLink href={getILMPolicyPath(ilmPolicy.name)}>{ilmPolicy.name}</EuiLink>
<EuiLink
Copy link
Contributor

@cjcenizal cjcenizal Jul 8, 2020

Choose a reason for hiding this comment

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

Just a note that we need an href here too, so users can copy the url or open the link in a new tab. If you grep for reactRouterNavigate you can see how we use this utility in various plugins to provide both an href and an onClick.

Copy link
Contributor Author

@alisonelizabeth alisonelizabeth Jul 8, 2020

Choose a reason for hiding this comment

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

@cjcenizal thanks for pointing this out! From what I can see, reactRouterNavigate doesn't work when navigating to other apps. I came across #67697, which suggests using history.parentHistory. This would work, however, it is private in ScopedHistory. I will change it back to a href for now, but there will be a full page refresh.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Tested locally and code changes look good! Great work @alisonelizabeth !

In ILM and CCR, you should be able to view related indices. This appears to be a regression of #67940.

I'm not 100% on how the regression came from that particular PR - looks like I'm missing something! Was it because of the introduction of includeHiddenIndices?

@alisonelizabeth
Copy link
Contributor Author

Thanks for the review @jloleysens!

I'm not 100% on how the regression came from that particular PR - looks like I'm missing something! Was it because of the introduction of includeHiddenIndices?

It looks like you changed the way we get the filter value. So instead of /indices/filter/my_filter, the UI expects /indices?includeHiddenIndices=true&filter=my_filter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Index Management Index and index templates UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.9.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants