Browse Source

ref(alerts): Refactor isIssueAlert to use type property (#75080)

Most of the calls to `isIssueAlert` are with a rule object that has a
`type` property reflecting exactly what type of alert rule we are
dealing with. Let's go ahead and use that to make the logic more
explicit and clean up the types.

The one call site that had to be changed is in the metric alert details
header. However the header's usage of `isIssueAlert` is unnecessary
because we already know we are dealing with a metric alert.
David Wang 7 months ago
parent
commit
2376089d6d

+ 133 - 94
static/app/views/alerts/list/rules/alertRulesList.spec.tsx

@@ -19,7 +19,7 @@ import {
 import OrganizationStore from 'sentry/stores/organizationStore';
 import ProjectsStore from 'sentry/stores/projectsStore';
 import TeamStore from 'sentry/stores/teamStore';
-import {IncidentStatus} from 'sentry/views/alerts/types';
+import {CombinedAlertType, IncidentStatus} from 'sentry/views/alerts/types';
 
 import AlertRulesList from './alertRulesList';
 
@@ -42,24 +42,33 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        ProjectAlertRuleFixture({
-          id: '123',
-          name: 'First Issue Alert',
-          projects: ['earth'],
-          createdBy: {name: 'Samwise', id: 1, email: ''},
-        }),
-        MetricRuleFixture({
-          id: '345',
-          projects: ['earth'],
-          latestIncident: IncidentFixture({
-            status: IncidentStatus.CRITICAL,
+        {
+          ...ProjectAlertRuleFixture({
+            id: '123',
+            name: 'First Issue Alert',
+            projects: ['earth'],
+            createdBy: {name: 'Samwise', id: 1, email: ''},
           }),
-        }),
-        MetricRuleFixture({
-          id: '678',
-          projects: ['earth'],
-          latestIncident: null,
-        }),
+          type: CombinedAlertType.ISSUE,
+        },
+        {
+          ...MetricRuleFixture({
+            id: '345',
+            projects: ['earth'],
+            latestIncident: IncidentFixture({
+              status: IncidentStatus.CRITICAL,
+            }),
+          }),
+          type: CombinedAlertType.METRIC,
+        },
+        {
+          ...MetricRuleFixture({
+            id: '678',
+            projects: ['earth'],
+            latestIncident: null,
+          }),
+          type: CombinedAlertType.METRIC,
+        },
       ],
     });
 
@@ -178,12 +187,15 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        ProjectAlertRuleFixture({
-          id: '123',
-          name: deletedRuleName,
-          projects: ['earth'],
-          createdBy: {name: 'Samwise', id: 1, email: ''},
-        }),
+        {
+          ...ProjectAlertRuleFixture({
+            id: '123',
+            name: deletedRuleName,
+            projects: ['earth'],
+            createdBy: {name: 'Samwise', id: 1, email: ''},
+          }),
+          type: CombinedAlertType.ISSUE,
+        },
       ],
     });
     const deleteMock = MockApiClient.addMockResponse({
@@ -343,35 +355,41 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        MetricRuleFixture({
-          id: '1',
-          projects: ['earth'],
-          name: 'Active Activated Alert',
-          monitorType: 1,
-          activationCondition: 0,
-          activations: [
-            {
-              alertRuleId: '1',
-              dateCreated: '2021-08-01T00:00:00Z',
-              finishedAt: '',
-              id: '1',
-              isComplete: false,
-              querySubscriptionId: '1',
-              activator: '123',
-              conditionType: '0',
-            },
-          ],
-          latestIncident: IncidentFixture({
-            status: IncidentStatus.CRITICAL,
+        {
+          ...MetricRuleFixture({
+            id: '1',
+            projects: ['earth'],
+            name: 'Active Activated Alert',
+            monitorType: 1,
+            activationCondition: 0,
+            activations: [
+              {
+                alertRuleId: '1',
+                dateCreated: '2021-08-01T00:00:00Z',
+                finishedAt: '',
+                id: '1',
+                isComplete: false,
+                querySubscriptionId: '1',
+                activator: '123',
+                conditionType: '0',
+              },
+            ],
+            latestIncident: IncidentFixture({
+              status: IncidentStatus.CRITICAL,
+            }),
           }),
-        }),
-        MetricRuleFixture({
-          id: '2',
-          projects: ['earth'],
-          name: 'Ready Activated Alert',
-          monitorType: 1,
-          activationCondition: 0,
-        }),
+          type: CombinedAlertType.METRIC,
+        },
+        {
+          ...MetricRuleFixture({
+            id: '2',
+            projects: ['earth'],
+            name: 'Ready Activated Alert',
+            monitorType: 1,
+            activationCondition: 0,
+          }),
+          type: CombinedAlertType.METRIC,
+        },
       ],
     });
     const {router, organization} = initializeOrg({organization: defaultOrg});
@@ -392,11 +410,14 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        ProjectAlertRuleFixture({
-          name: 'First Issue Alert',
-          projects: ['earth'],
-          status: 'disabled',
-        }),
+        {
+          ...ProjectAlertRuleFixture({
+            name: 'First Issue Alert',
+            projects: ['earth'],
+            status: 'disabled',
+          }),
+          type: CombinedAlertType.ISSUE,
+        },
       ],
     });
     const {router, organization} = initializeOrg({organization: defaultOrg});
@@ -410,13 +431,16 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        ProjectAlertRuleFixture({
-          name: 'First Issue Alert',
-          projects: ['earth'],
-          // both disabled and muted
-          status: 'disabled',
-          snooze: true,
-        }),
+        {
+          ...ProjectAlertRuleFixture({
+            name: 'First Issue Alert',
+            projects: ['earth'],
+            // both disabled and muted
+            status: 'disabled',
+            snooze: true,
+          }),
+          type: CombinedAlertType.ISSUE,
+        },
       ],
     });
     const {router, organization} = initializeOrg({organization: defaultOrg});
@@ -431,11 +455,14 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        ProjectAlertRuleFixture({
-          name: 'First Issue Alert',
-          projects: ['earth'],
-          snooze: true,
-        }),
+        {
+          ...ProjectAlertRuleFixture({
+            name: 'First Issue Alert',
+            projects: ['earth'],
+            snooze: true,
+          }),
+          type: CombinedAlertType.ISSUE,
+        },
       ],
     });
     const {router, organization} = initializeOrg({organization: defaultOrg});
@@ -449,10 +476,13 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        MetricRuleFixture({
-          projects: ['earth'],
-          snooze: true,
-        }),
+        {
+          ...MetricRuleFixture({
+            projects: ['earth'],
+            snooze: true,
+          }),
+          type: CombinedAlertType.METRIC,
+        },
       ],
     });
     const {router, organization} = initializeOrg({organization: defaultOrg});
@@ -503,28 +533,37 @@ describe('AlertRulesList', () => {
       url: '/organizations/org-slug/combined-rules/',
       headers: {Link: pageLinks},
       body: [
-        ProjectAlertRuleFixture({
-          id: '123',
-          name: 'First Issue Alert',
-          projects: ['earth'],
-          createdBy: {name: 'Samwise', id: 1, email: ''},
-        }),
-        MetricRuleFixture({
-          id: '345',
-          projects: ['earth'],
-          name: 'activated Test Metric Alert',
-          monitorType: 1,
-          latestIncident: IncidentFixture({
-            status: IncidentStatus.CRITICAL,
+        {
+          ...ProjectAlertRuleFixture({
+            id: '123',
+            name: 'First Issue Alert',
+            projects: ['earth'],
+            createdBy: {name: 'Samwise', id: 1, email: ''},
           }),
-        }),
-        MetricRuleFixture({
-          id: '678',
-          name: 'Test Metric Alert 2',
-          monitorType: 0,
-          projects: ['earth'],
-          latestIncident: null,
-        }),
+          type: CombinedAlertType.ISSUE,
+        },
+        {
+          ...MetricRuleFixture({
+            id: '345',
+            projects: ['earth'],
+            name: 'activated Test Metric Alert',
+            monitorType: 1,
+            latestIncident: IncidentFixture({
+              status: IncidentStatus.CRITICAL,
+            }),
+          }),
+          type: CombinedAlertType.METRIC,
+        },
+        {
+          ...MetricRuleFixture({
+            id: '678',
+            name: 'Test Metric Alert 2',
+            monitorType: 0,
+            projects: ['earth'],
+            latestIncident: null,
+          }),
+          type: CombinedAlertType.METRIC,
+        },
       ],
     });
 

+ 1 - 5
static/app/views/alerts/rules/metric/details/header.tsx

@@ -15,8 +15,6 @@ import type {Project} from 'sentry/types/project';
 import type {MetricRule} from 'sentry/views/alerts/rules/metric/types';
 import {getAlertRuleActionCategory} from 'sentry/views/alerts/rules/utils';
 
-import {isIssueAlert} from '../../../utils';
-
 type Props = {
   hasMetricRuleDetailsError: boolean;
   onSnooze: (nextState: {
@@ -40,9 +38,7 @@ function DetailsHeader({
   const ruleTitle = rule && !hasMetricRuleDetailsError ? rule.name : '';
   const settingsLink =
     rule &&
-    `/organizations/${organization.slug}/alerts/${
-      isIssueAlert(rule) ? 'rules' : 'metric-rules'
-    }/${project?.slug ?? rule?.projects?.[0]}/${rule.id}/`;
+    `/organizations/${organization.slug}/alerts/metric-rules/${project?.slug ?? rule?.projects?.[0]}/${rule.id}/`;
 
   const duplicateLink = {
     pathname: `/organizations/${organization.slug}/alerts/new/metric/`,

+ 4 - 8
static/app/views/alerts/utils/index.tsx

@@ -2,7 +2,6 @@ import round from 'lodash/round';
 
 import {t} from 'sentry/locale';
 import {SessionFieldWithOperation} from 'sentry/types';
-import type {IssueAlertRule} from 'sentry/types/alerts';
 import type {Organization} from 'sentry/types/organization';
 import {defined} from 'sentry/utils';
 import toArray from 'sentry/utils/array/toArray';
@@ -11,7 +10,6 @@ import {axisLabelFormatter, tooltipFormatter} from 'sentry/utils/discover/charts
 import {aggregateOutputType} from 'sentry/utils/discover/fields';
 import {formatMetricUsingFixedUnit} from 'sentry/utils/metrics/formatters';
 import {parseField, parseMRI} from 'sentry/utils/metrics/mri';
-import type {MetricRule, SavedMetricRule} from 'sentry/views/alerts/rules/metric/types';
 import {
   Dataset,
   Datasource,
@@ -20,8 +18,8 @@ import {
 } from 'sentry/views/alerts/rules/metric/types';
 import {isCustomMetricAlert} from 'sentry/views/alerts/rules/metric/utils/isCustomMetricAlert';
 
-import type {Incident, IncidentStats} from '../types';
-import {AlertRuleStatus} from '../types';
+import type {CombinedMetricIssueAlerts, Incident, IncidentStats} from '../types';
+import {AlertRuleStatus, CombinedAlertType} from '../types';
 
 /**
  * Gets start and end date query parameters from stats
@@ -35,10 +33,8 @@ export function getStartEndFromStats(stats: IncidentStats) {
   return {start, end};
 }
 
-export function isIssueAlert(
-  data: IssueAlertRule | SavedMetricRule | MetricRule
-): data is IssueAlertRule {
-  return !data.hasOwnProperty('triggers');
+export function isIssueAlert(data: CombinedMetricIssueAlerts) {
+  return data.type === CombinedAlertType.ISSUE;
 }
 
 export const DATA_SOURCE_LABELS = {