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

[Security Solution] Add 3rd level breadcrumb to admin page #71275

Merged
merged 11 commits into from
Jul 14, 2020

Conversation

parkiino
Copy link
Contributor

@parkiino parkiino commented Jul 9, 2020

Summary

  • Adds additional breadcrumbs for the administration subapp (hosts and policies)
  • Additionally renames some more things from management to admin

image

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@parkiino parkiino marked this pull request as ready for review July 13, 2020 20:37
@parkiino parkiino requested review from a team as code owners July 13, 2020 20:37
@parkiino parkiino added Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Management v7.9.0 v8.0.0 labels Jul 13, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-management (Team:Endpoint Management)

Copy link

@aisantos aisantos left a comment

Choose a reason for hiding this comment

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

lgtm;

@kevinlog
Copy link
Contributor

this looks good to me, but I want @paul-tavares to review before merging this in for 7.9. Worst case, we just merge in 8.0 and backport after the 7.9 branch is cut.

Thanks for putting up the PR @parkiino !

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 825 +3 822

History

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

const isAdminRoutes = (spyState: RouteSpyState): spyState is AdministrationRouteSpyState =>
spyState != null && spyState.pageName === SecurityPageName.administration;

// eslint-disable-next-line complexity
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what the ESLint error was here.

@@ -30,20 +30,20 @@ export const ManagementPageView = memo<Omit<PageViewProps, 'tabs'>>((options) =>
}
return [
{
name: i18n.translate('xpack.securitySolution.managementTabs.endpoints', {
name: i18n.translate('xpack.securitySolution.managementTabs.hosts', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you created a translations.ts module under management/common/ where both Host and Policies tab names are defined - should they be used here as well?

@@ -106,7 +106,7 @@ export const HostDetails = memo(({ details }: { details: HostMetadata }) => {
path: agentDetailsWithFlyoutPath,
state: {
onDoneNavigateTo: [
'securitySolution:management',
'securitySolution:administration',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually be replaced with the constant we have in management/common/constants: MANAGEMENT_APP_ID

@@ -127,12 +127,12 @@ export const HostList = () => {
}`,
state: {
onCancelNavigateTo: [
'securitySolution:management',
'securitySolution:administration',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here and other places below - use MANAGEMENT_APP_ID

@@ -172,7 +172,7 @@ describe('Policy Details', () => {
cancelbutton.simulate('click', { button: 0 });
const navigateToAppMockedCalls = coreStart.application.navigateToApp.mock.calls;
expect(navigateToAppMockedCalls[navigateToAppMockedCalls.length - 1]).toEqual([
'securitySolution:management',
'securitySolution:administration',
Copy link
Contributor

Choose a reason for hiding this comment

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

:-)
Here as well (maybe global search and replace under security_solution/public dir

let breadcrumb = [
{
text: ADMINISTRATION,
href: getUrlForApp(`${APP_ID}:${SecurityPageName.administration}`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

use MANAGEMENT_APP_ID instead here

@@ -281,7 +281,7 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S
});

core.application.register({
id: `${APP_ID}:${SecurityPageName.management}`,
id: `${APP_ID}:${SecurityPageName.administration}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should use the management const here too MANAGEMENT_APP_ID

@@ -80,7 +80,7 @@ const securitySubPlugins = [
`${APP_ID}:${SecurityPageName.network}`,
`${APP_ID}:${SecurityPageName.timelines}`,
`${APP_ID}:${SecurityPageName.case}`,
`${APP_ID}:${SecurityPageName.management}`,
`${APP_ID}:${SecurityPageName.administration}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

and here :)

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Nice job 👍 ⭐

All of my comment can be done in a separate PR as they are minor and really should not hold this up.
I would suggest that the description of this PR (and ultimately the Squash and Merge commit message) be clearer around the changes made. I think the bulk of the changes were refactoring ++ bug fix (I think?) around the renaming of the Management tab to Administration (Do I have that right?)

@parkiino parkiino changed the title Task/admin breadcrumbs [Security Solution] Add 3rd level breadcrumb to admin page Jul 14, 2020
@parkiino parkiino merged commit b7a6cff into elastic:master Jul 14, 2020
@parkiino parkiino deleted the task/admin-breadcrumbs branch July 14, 2020 04:00
parkiino added a commit to parkiino/kibana that referenced this pull request Jul 14, 2020
…1275)

[Endpoint Security] Add 3rd level (hosts / policies) breadcrumb to admin page
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (314 commits)
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540)
  initial telemetry setup (elastic#69330)
  [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027)
  Search across spaces (elastic#67644)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 14, 2020
…t-apps-page-titles

* 'master' of github.com:elastic/kibana: (88 commits)
  [ML] Functional tests - disable DFA creation and cloning tests
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540)
  initial telemetry setup (elastic#69330)
  [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027)
  ...

# Conflicts:
#	x-pack/plugins/index_management/public/application/index.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (72 commits)
  [test] Skips test preventing promotion of ES snapshot elastic#71612
  [Logs UI] Remove UUID from Alert Instances (elastic#71340)
  [Metrics UI] Remove UUID from Alert Instance IDs (elastic#71335)
  [ML] Functional tests - disable DFA creation and cloning tests
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  ...
parkiino added a commit that referenced this pull request Jul 14, 2020
…71592)

[Endpoint Security] Add 3rd level (hosts / policies) breadcrumb to admin page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants