-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat: Add Activated Alert Rules to alert rule index #69124
Changes from 1 commit
292129f
5edcf36
5a8db4f
e72d775
3d54f15
aa40fdb
902efd6
4c830cd
b0449f3
09c8c78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import {useState} from 'react'; | ||
import {useMemo, useState} from 'react'; | ||
import styled from '@emotion/styled'; | ||
|
||
import Access from 'sentry/components/acl/access'; | ||
|
@@ -32,7 +32,7 @@ import { | |
} from 'sentry/views/alerts/rules/metric/types'; | ||
|
||
import type {CombinedMetricIssueAlerts, MetricAlert} from '../../types'; | ||
import {CombinedAlertType, IncidentStatus} from '../../types'; | ||
import {ActivationStatus, CombinedAlertType, IncidentStatus} from '../../types'; | ||
|
||
type Props = { | ||
hasEditAccess: boolean; | ||
|
@@ -59,6 +59,12 @@ function ActivatedRuleListRow({ | |
}: Props) { | ||
const {teams: userTeams} = useUserTeams(); | ||
const [assignee, setAssignee] = useState<string>(''); | ||
const isWaiting = useMemo( | ||
() => | ||
!rule.activations?.length || | ||
(rule.activations?.length && rule.activations[0].isComplete), | ||
[rule] | ||
); | ||
|
||
function renderLatestActivation(): React.ReactNode { | ||
if (!rule.activations?.length) { | ||
|
@@ -83,8 +89,6 @@ function ActivatedRuleListRow({ | |
} | ||
|
||
function renderAlertRuleStatus(): React.ReactNode { | ||
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. I'd recommend decomposing this quite a bit.. you could really simplify this by pulling the alert status out as a different component.. then applying SOLID programming principles split this into it's most basic pieces.. each alert threshold state. Then have the AlertRuleStatus simply look at the state and decide which component to render. That would simplify the UI components, decouple your logic from presentation, and simplify testing. |
||
// const isActive = !(rule.activations?.length && rule.activations[0].isComplete); | ||
|
||
if (rule.snooze) { | ||
return renderSnoozeStatus(); | ||
} | ||
|
@@ -160,13 +164,6 @@ function ActivatedRuleListRow({ | |
|
||
const canEdit = ownerId ? userTeams.some(team => team.id === ownerId) : true; | ||
|
||
const IssueStatusText: Record<IncidentStatus, string> = { | ||
[IncidentStatus.CRITICAL]: t('Critical'), | ||
[IncidentStatus.WARNING]: t('Warning'), | ||
[IncidentStatus.CLOSED]: t('Resolved'), | ||
[IncidentStatus.OPENED]: t('Resolved'), | ||
}; | ||
|
||
const actions: MenuItemProps[] = [ | ||
{ | ||
key: 'edit', | ||
|
@@ -264,11 +261,15 @@ function ActivatedRuleListRow({ | |
<FlexCenter> | ||
<Tooltip | ||
title={tct('Metric Alert Status: [status]', { | ||
status: | ||
IssueStatusText[rule?.latestIncident?.status ?? IncidentStatus.CLOSED], | ||
status: isWaiting ? 'Ready to monitor' : 'Monitoring', | ||
})} | ||
> | ||
<AlertBadge status={rule?.latestIncident?.status} /> | ||
<AlertBadge | ||
status={rule?.latestIncident?.status} | ||
activationStatus={ | ||
isWaiting ? ActivationStatus.WAITING : ActivationStatus.MONITORING | ||
} | ||
/> | ||
</Tooltip> | ||
</FlexCenter> | ||
<AlertNameAndStatus> | ||
|
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.
Nit: recommend decomposing each of the render calls. If you're accessing things like the
rule
it'd be extra cool to usereact-query
to avoid prop drilling too.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.
hmmm how does react-query addres prop drilling?
Or did you mean context?
for now i think i'll keep these patterns to match
rules.tsx
😅I'll create a follow up ticket to clean both of these up though
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.
ticket here: https://github.com/orgs/getsentry/projects/199/views/3?pane=issue&itemId=60130307
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.
react-query uses context to store api data for you. (it also handles automatic updates and syncing data across all uses of the "store").
for example, if you wanted to fetch a specific
rule
you could douseRule(ruleId)
-- then that hook uses the api react-query hook to fetch the data (which updates the value in the store for rule etc).If you haven't used it before.. it's kinda a mind meld and we should def wait until later to update. i'm planning on using it for the API requests with project settings pages, so we could also take a look at it when that comes up.