[Security Solution][List Details Page]: Fix exception list details page route and adding breadcrumb
#145605
Conversation
WafaaNasr
added
release_note:fix
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
WafaaNasr
removed
the
Team: SecuritySolution
WafaaNasr
changed the title
WafaaNasr
changed the title
exception shared list details page issues
|
@elasticmachine merge upstream |
…shared-page-details
|
@elasticmachine merge upstream |
x-pack/plugins/security_solution/public/app/deep_links/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi! Thanks for this PR,
I see one problem that was carried from a previous PR already merged.
The problem is exceptionListDetails link should not exist as a SecurityPageName . The SecurityPageName have main sections only, the ones that have a static link. This new path (/exceptions/details/:exceptionListId) is dynamic, so it should be considered just a part of the SecurityPageName.exceptions (/exceptions) page.
It seems there's been some confusion lately about static and dynamic paths and the links config. We will create a new doc page explaining that soon.
Changes
The exceptionListDetails should not exist in the management links config. The same for the old config deep_links, though this one will be removed shortly.
The exceptionListDetails id should not exist as a SecurityPageName.
The Page Title will be Exceptions if we use <SpyRoute pageName={SecurityPageName.exceptions} />.
Could you please do these changes?
| deepLinkId: SecurityPageName.exceptionListDetails, | ||
| path: `/exceptions/details/${exceptionsList.list_id}`, |
There was a problem hiding this comment.
| deepLinkId: SecurityPageName.exceptionListDetails, | |
| path: `/exceptions/details/${exceptionsList.list_id}`, | |
| deepLinkId: SecurityPageName.exceptions, | |
| path: `/details/${exceptionsList.list_id}`, |
| <ListsDetailView /> | ||
| <SpyRoute pageName={SecurityPageName.sharedExceptionListDetails} /> | ||
| <SpyRoute pageName={SecurityPageName.exceptionListDetails} /> |
There was a problem hiding this comment.
This pageName should not be used here, since it is a dynamic page, we should keep using pageName={SecurityPageName.exception}.
If we want to generate a trailing breadcrumb with the exceptionList name we could add a state prop:
<SpyRoute pageName={SecurityPageName.exceptions} state={{ exceptionListName }}/>
(not here, somewhere we have the exceptionList name defined)
And create a trailing breadcrumbs module similar to:
WafaaNasr
changed the title
exception shared list details page issuesexception shared list details page name
…lement breadcrumb
|
@elasticmachine merge upstream |
|
merge conflict between base and head |
WafaaNasr
changed the title
exception shared list details page nameexception list details page route and adding breadcrumb
There was a problem hiding this comment.
@WafaaNasr The links implementation LGTM! 🚀
Thanks for doing the changes. I left just one NIT.
| href: getSecuritySolutionUrl({ | ||
| deepLinkId: SecurityPageName.exceptions, | ||
| path: '', | ||
| }), |
There was a problem hiding this comment.
NIT: This href prop is not strictly correct, it points to the main /exceptions path instead of the list detail page. Since this is the last breadcrumb, it is not clickable, the href prop is not needed, so we could just remove it by now.
However, if someday we need to append another breadcrumb after it, we will have to define the list detail href to make this breadcrumb (not the last one anymore) clickable. To do so, we would need to get the exception list id and generate the /detail/[id] path.
The generic way to do it would be to rename the /:exceptionListId parameter in the path by the generic /:detailName param name, this way we could generate the detail/${params.detailName} here.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @WafaaNasr |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
WafaaNasr
added
the
backport:prev-minor
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ails` page route and adding breadcrumb (#145605) (#145960) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Solution][List Details Page]: Fix `exception list details` page route and adding breadcrumb (#145605)](#145605) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Wafaa Nasr","email":"wafaa.nasr@elastic.co"},"sourceCommit":{"committedDate":"2022-11-21T15:38:33Z","message":"[Security Solution][List Details Page]: Fix `exception list details` page route and adding breadcrumb (#145605)\n\n## Summary\r\n\r\nAs per this\r\n[discussion](https://github.com/elastic/kibana/pull/145605#pullrequestreview-1186305242)\r\n\r\n- Remove the `Exception-List-details` definition from the [management\r\nlinks](https://github.com/elastic/kibana/blob/bb775883505bc0d81e487261244feab0a2011f6f/x-pack/plugins/security_solution/public/management/links.ts)\r\nand\r\n[deep_links](https://github.com/elastic/kibana/blob/bb775883505bc0d81e487261244feab0a2011f6f/x-pack/plugins/security_solution/public/app/deep_links/index.ts)\r\nbecause it is a dynamic route\r\n\r\n- Use the `Rule Exceptions` page title for list details\r\n- Add breadcrumb for the details list page\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"62d9dffbb2d806f35e76dff361fcf9df0b9ce90b","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Security Solution Platform","backport:prev-minor","v8.6.0","v8.7.0"],"number":145605,"url":"https://github.com/elastic/kibana/pull/145605","mergeCommit":{"message":"[Security Solution][List Details Page]: Fix `exception list details` page route and adding breadcrumb (#145605)\n\n## Summary\r\n\r\nAs per this\r\n[discussion](https://github.com/elastic/kibana/pull/145605#pullrequestreview-1186305242)\r\n\r\n- Remove the `Exception-List-details` definition from the [management\r\nlinks](https://github.com/elastic/kibana/blob/bb775883505bc0d81e487261244feab0a2011f6f/x-pack/plugins/security_solution/public/management/links.ts)\r\nand\r\n[deep_links](https://github.com/elastic/kibana/blob/bb775883505bc0d81e487261244feab0a2011f6f/x-pack/plugins/security_solution/public/app/deep_links/index.ts)\r\nbecause it is a dynamic route\r\n\r\n- Use the `Rule Exceptions` page title for list details\r\n- Add breadcrumb for the details list page\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"62d9dffbb2d806f35e76dff361fcf9df0b9ce90b"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145605","number":145605,"mergeCommit":{"message":"[Security Solution][List Details Page]: Fix `exception list details` page route and adding breadcrumb (#145605)\n\n## Summary\r\n\r\nAs per this\r\n[discussion](https://github.com/elastic/kibana/pull/145605#pullrequestreview-1186305242)\r\n\r\n- Remove the `Exception-List-details` definition from the [management\r\nlinks](https://github.com/elastic/kibana/blob/bb775883505bc0d81e487261244feab0a2011f6f/x-pack/plugins/security_solution/public/management/links.ts)\r\nand\r\n[deep_links](https://github.com/elastic/kibana/blob/bb775883505bc0d81e487261244feab0a2011f6f/x-pack/plugins/security_solution/public/app/deep_links/index.ts)\r\nbecause it is a dynamic route\r\n\r\n- Use the `Rule Exceptions` page title for list details\r\n- Add breadcrumb for the details list page\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"62d9dffbb2d806f35e76dff361fcf9df0b9ce90b"}}]}] BACKPORT-->
Summary
As per this discussion
Remove the
Exception-List-detailsdefinition from the management links and deep_links because it is a dynamic routeUse the
Rule Exceptionspage title for list detailsAdd breadcrumb for the details list page