-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Refactor Discover v2 rules menu: merge alerts into legacy-rules submenu #259649
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
Changes from all commits
e42b6d2
aed1b57
225f55f
74577b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,12 +12,10 @@ import type { AggregateQuery } from '@kbn/es-query'; | |
| import { i18n } from '@kbn/i18n'; | ||
| import type { DiscoverAppMenuItemType, DiscoverAppMenuPopoverItem } from '@kbn/discover-utils'; | ||
| import { AppMenuActionId } from '@kbn/discover-utils'; | ||
| import { ES_QUERY_ID } from '@kbn/rule-data-utils'; | ||
| import type { DiscoverInternalState } from '../../../state_management/redux'; | ||
| import { selectTab } from '../../../state_management/redux/selectors'; | ||
| import type { AppMenuDiscoverParams } from './types'; | ||
| import type { DiscoverServices } from '../../../../../build_services'; | ||
| import { CreateAlertFlyout, getManageRulesUrl, getTimeField } from './get_alerts'; | ||
|
|
||
| export function CreateESQLRuleFlyout({ | ||
| services, | ||
|
|
@@ -75,59 +73,11 @@ export const getCreateRuleMenuItem = ({ | |
| tabId: string; | ||
| getState: () => DiscoverInternalState; | ||
| }): DiscoverAppMenuItemType => { | ||
| const { dataView, isEsqlMode } = discoverParams; | ||
| const timeField = getTimeField(dataView); | ||
| const hasTimeFieldName = !isEsqlMode ? Boolean(dataView?.timeFieldName) : Boolean(timeField); | ||
|
|
||
| const legacyItems: DiscoverAppMenuPopoverItem[] = []; | ||
|
|
||
| if (services.capabilities.management?.insightsAndAlerting?.triggersActions) { | ||
| if (discoverParams.authorizedRuleTypeIds.includes(ES_QUERY_ID)) { | ||
| legacyItems.push({ | ||
| id: 'legacy-search-threshold', | ||
| order: 1, | ||
| label: i18n.translate('discover.localMenu.legacySearchThresholdTitle', { | ||
| defaultMessage: 'Search threshold rule', | ||
| }), | ||
| iconType: 'bell', | ||
| testId: 'discoverLegacySearchThresholdButton', | ||
| disableButton: !hasTimeFieldName, | ||
| tooltipContent: hasTimeFieldName | ||
| ? undefined | ||
| : i18n.translate('discover.localMenu.legacyMissedTimeFieldToolTip', { | ||
| defaultMessage: 'Data view does not have a time field.', | ||
| }), | ||
| run: ({ context: { onFinishAction } }) => { | ||
| return ( | ||
| <CreateAlertFlyout | ||
| onFinishAction={onFinishAction} | ||
| discoverParams={discoverParams} | ||
| services={services} | ||
| tabId={tabId} | ||
| getState={getState} | ||
| /> | ||
| ); | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| legacyItems.push({ | ||
| id: 'manage-rules-connectors', | ||
| order: Number.MAX_SAFE_INTEGER, | ||
| label: i18n.translate('discover.localMenu.manageRulesAndConnectors', { | ||
| defaultMessage: 'Manage rules and connectors', | ||
| }), | ||
| iconType: 'tableOfContents', | ||
| testId: 'discoverManageRulesButton', | ||
| href: getManageRulesUrl(services), | ||
| }); | ||
| } | ||
|
|
||
| const createRuleItem: DiscoverAppMenuPopoverItem = { | ||
| id: 'create-rule', | ||
| order: 1, | ||
| label: i18n.translate('discover.localMenu.createRuleTitle', { | ||
| defaultMessage: 'Create rule', | ||
| defaultMessage: 'Create v2 ES|QL rule', | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will be explicit for M1, decide on much better naming and labeling for M2 |
||
| }), | ||
| iconType: 'bell', | ||
| testId: 'discoverCreateRuleButton', | ||
|
|
@@ -148,17 +98,13 @@ export const getCreateRuleMenuItem = ({ | |
| id: 'legacy-rules', | ||
| order: 2, | ||
| label: i18n.translate('discover.localMenu.legacyRulesTitle', { | ||
| defaultMessage: 'Create legacy rules', | ||
| defaultMessage: 'Create v1 rules', | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must avoid the word "legacy" for v1 rules (v1 is also terrible but we will fix for M2) |
||
| }), | ||
| testId: 'discoverLegacyRulesButton', | ||
| items: legacyItems, | ||
| items: [], | ||
| }; | ||
|
|
||
| const items: DiscoverAppMenuPopoverItem[] = [createRuleItem]; | ||
|
|
||
| if (legacyItems.length > 0) { | ||
| items.push(legacyRulesItem); | ||
| } | ||
| const items: DiscoverAppMenuPopoverItem[] = [createRuleItem, legacyRulesItem]; | ||
|
|
||
| return { | ||
| id: AppMenuActionId.createRule, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,11 +127,6 @@ export const useTopNavLinks = ({ | |
| const inspectAppMenuItem = getInspectAppMenuItem({ onOpenInspector }); | ||
| items.push(inspectAppMenuItem); | ||
|
|
||
| const showLegacyAlerts = | ||
| services.triggersActionsUi && | ||
| discoverParams.authorizedRuleTypeIds.length && | ||
| (!canCreateESQLRule || !discoverParams.isEsqlMode); | ||
|
|
||
| if (showCreateRuleV2) { | ||
| const createRuleV2 = getCreateRuleMenuItem({ | ||
| discoverParams, | ||
|
|
@@ -142,7 +137,7 @@ export const useTopNavLinks = ({ | |
| items.push(createRuleV2); | ||
| } | ||
|
|
||
| if (showLegacyAlerts) { | ||
| if (services.triggersActionsUi && discoverParams.authorizedRuleTypeIds.length) { | ||
| const alertsAppMenuItem = getAlertsAppMenuItem({ | ||
| discoverParams, | ||
| services, | ||
|
|
@@ -233,7 +228,6 @@ export const useTopNavLinks = ({ | |
| hasUnsavedChanges, | ||
| totalHitsState, | ||
| intl, | ||
| canCreateESQLRule, | ||
| showCreateRuleV2, | ||
| ]); | ||
|
|
||
|
|
@@ -248,20 +242,6 @@ export const useTopNavLinks = ({ | |
|
|
||
| newAppMenuRegistry.registerItems(appMenuItems); | ||
|
|
||
| // Register legacyRules as a top-level item when v2 rules are enabled | ||
| // This allows profile extensions to add items to it via registerPopoverItem | ||
| // The items are then merged into the createRule menu's legacy submenu | ||
| if (showCreateRuleV2) { | ||
| newAppMenuRegistry.registerItem({ | ||
| id: AppMenuActionId.legacyRules, | ||
| label: '', // Not displayed directly - items are pulled into the v2 menu's submenu | ||
| iconType: 'empty', | ||
| order: Number.MAX_SAFE_INTEGER, | ||
| hidden: 'all', // Hide at all breakpoints since this is just a container for registry items | ||
| items: [], | ||
| }); | ||
| } | ||
|
|
||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to register this "dummy" item, we re-use the alerts one, then remove/delete it after we've merged its contents into the sub-nav. |
||
| // Only show the ES|QL button in classic mode (not in ES|QL mode) | ||
| // The "Switch to Classic" option is now in the tab menu when in ES|QL mode | ||
| if (services.uiSettings.get(ENABLE_ESQL) && !isEsqlMode) { | ||
|
|
@@ -404,14 +384,16 @@ export const useTopNavLinks = ({ | |
|
|
||
| const registry = getAppMenu(discoverParams).appMenuRegistry(newAppMenuRegistry); | ||
|
|
||
| // Merge items from legacyRules into the createRule menu's legacy-rules submenu | ||
| // This allows profile extensions to add rule types to the v2 rules menu | ||
| // When v2 rules are enabled, profile extensions have registered their rule types | ||
| // into the alerts menu as usual. Move those items into the v2 createRule menu's | ||
| // legacy-rules submenu, then remove the alerts menu since v2 replaces it. | ||
| if (showCreateRuleV2) { | ||
| registry.mergePopoverItems( | ||
| AppMenuActionId.createRule, | ||
| 'legacy-rules', | ||
| AppMenuActionId.legacyRules | ||
| AppMenuActionId.alerts | ||
| ); | ||
| registry.deleteItem(AppMenuActionId.alerts); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new line to remove the old "alerts" menu item, we've merged its contents into the v2 Rules sub-nav item for M1
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: is there any benefit in exposing the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, I think I like how explicit it is here, so we can see the flow in line. |
||
| } | ||
|
|
||
| return registry; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to re-craft this manually, it comes over when we merge in the Alerts menu into the v2 Rules sub-menu