Skip to content

Commit

Permalink
fix(alerts): edit alert modal fixes (#27135)
Browse files Browse the repository at this point in the history
  • Loading branch information
anirudhpillai authored Jan 2, 2025
1 parent 3aa25f3 commit 96da932
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 24 deletions.
15 changes: 10 additions & 5 deletions frontend/src/lib/components/Alerts/alertFormLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ export type AlertFormType = Pick<
insight?: QueryBasedInsightModel['id']
}

export function canCheckOngoingInterval(alert?: AlertType | AlertFormType): boolean {
return (
(alert?.condition.type === AlertConditionType.ABSOLUTE_VALUE ||
alert?.condition.type === AlertConditionType.RELATIVE_INCREASE) &&
alert?.threshold.configuration.bounds?.upper != null &&
!isNaN(alert?.threshold.configuration.bounds.upper)
)
}

export interface AlertFormLogicProps {
alert: AlertType | null
insightId: QueryBasedInsightModel['id']
Expand Down Expand Up @@ -85,11 +94,7 @@ export const alertFormLogic = kea<alertFormLogicType>([
// can only check ongoing interval for absolute value/increase alerts with upper threshold
config: {
...alert.config,
check_ongoing_interval:
(alert.condition.type === AlertConditionType.ABSOLUTE_VALUE ||
alert.condition.type === AlertConditionType.RELATIVE_INCREASE) &&
alert.threshold.configuration.bounds?.upper != null &&
alert.config.check_ongoing_interval,
check_ongoing_interval: canCheckOngoingInterval(alert) && alert.config.check_ongoing_interval,
},
}

Expand Down
40 changes: 25 additions & 15 deletions frontend/src/lib/components/Alerts/views/EditAlertModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ import { LemonButton } from 'lib/lemon-ui/LemonButton'
import { LemonField } from 'lib/lemon-ui/LemonField'
import { LemonModal } from 'lib/lemon-ui/LemonModal'
import { alphabet, formatDate } from 'lib/utils'
import { useCallback } from 'react'
import { trendsDataLogic } from 'scenes/trends/trendsDataLogic'

import { AlertCalculationInterval, AlertConditionType, AlertState, InsightThresholdType } from '~/queries/schema'
import { InsightShortId, QueryBasedInsightModel } from '~/types'

import { alertFormLogic } from '../alertFormLogic'
import { alertFormLogic, canCheckOngoingInterval } from '../alertFormLogic'
import { alertLogic } from '../alertLogic'
import { SnoozeButton } from '../SnoozeButton'
import { AlertType } from '../types'
Expand Down Expand Up @@ -87,9 +88,17 @@ export function EditAlertModal({
onClose,
onEditSuccess,
}: EditAlertModalProps): JSX.Element {
const { alert, alertLoading } = useValues(alertLogic({ alertId }))
const _alertLogic = alertLogic({ alertId })
const { alert, alertLoading } = useValues(_alertLogic)
const { loadAlert } = useActions(_alertLogic)

const formLogicProps = { alert, insightId, onEditSuccess }
// need to reload edited alert as well
const _onEditSuccess = useCallback(() => {
loadAlert()
onEditSuccess()
}, [loadAlert, onEditSuccess])

const formLogicProps = { alert, insightId, onEditSuccess: _onEditSuccess }
const formLogic = alertFormLogic(formLogicProps)
const { alertForm, isAlertFormSubmitting, alertFormChanged } = useValues(formLogic)
const { deleteAlert, snoozeAlert, clearSnooze } = useActions(formLogic)
Expand All @@ -100,10 +109,7 @@ export function EditAlertModal({

const creatingNewAlert = alertForm.id === undefined
// can only check ongoing interval for absolute value/increase alerts with upper threshold
const can_check_ongoing_interval =
(alertForm?.condition.type === AlertConditionType.ABSOLUTE_VALUE ||
alertForm?.condition.type === AlertConditionType.RELATIVE_INCREASE) &&
alertForm.threshold.configuration.bounds?.upper != null
const can_check_ongoing_interval = canCheckOngoingInterval(alertForm)

return (
<LemonModal onClose={onClose} isOpen={isOpen} width={600} simple title="">
Expand Down Expand Up @@ -167,14 +173,18 @@ export function EditAlertModal({
<LemonSelect
fullWidth
data-attr="alertForm-series-index"
options={alertSeries?.map(({ event }, index) => ({
label: isBreakdownValid
? 'any breakdown value'
: formula
? `Formula (${formula})`
: `${alphabet[index]} - ${event}`,
value: isBreakdownValid || formula ? 0 : index,
}))}
options={alertSeries?.map(
({ custom_name, name, event }, index) => ({
label: isBreakdownValid
? 'any breakdown value'
: formula
? `Formula (${formula})`
: `${alphabet[index]} - ${
custom_name ?? name ?? event
}`,
value: isBreakdownValid || formula ? 0 : index,
})
)}
disabledReason={
(isBreakdownValid &&
`For trends with breakdown, the alert will fire if any of the breakdown
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ export function AlertListItem({ alert, onClick }: AlertListItemProps): JSX.Eleme

{alert.enabled ? (
<div className="text-muted pl-3">
{bounds?.lower !== undefined &&
{bounds?.lower != null &&
`Low ${isPercentage ? bounds.lower * 100 : bounds.lower}${isPercentage ? '%' : ''}`}
{bounds?.lower !== undefined && bounds?.upper ? ' · ' : ''}
{bounds?.upper !== undefined &&
{bounds?.lower != null && bounds?.upper != null ? ' · ' : ''}
{bounds?.upper != null &&
`High ${isPercentage ? bounds.upper * 100 : bounds.upper}${isPercentage ? '%' : ''}`}
</div>
) : (
Expand Down
4 changes: 4 additions & 0 deletions posthog/tasks/alerts/checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@ def check_alert(alert_id: str, capture_ph_event: Callable = lambda *args, **kwar
"Skipping alert check because weekend checking is disabled",
alert=alert,
)

# ignore alert check until due again
alert.next_check_at = next_check_time(alert)
alert.save()
return

if alert.snoozed_until:
Expand Down
5 changes: 5 additions & 0 deletions posthog/tasks/alerts/test/test_alert_checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,11 @@ def test_alert_is_not_checked_on_weekend_when_skip_weekends_is_true(
checks = AlertCheck.objects.filter(alert_configuration=self.alert["id"])
assert len(checks) == 0

def test_alert_is_checked_on_weekday_when_skip_weekends_is_true(
self, mock_send_notifications_for_breaches: MagicMock, mock_send_errors: MagicMock
) -> None:
self.skip_weekend(True)

# run on week day
with freeze_time("2024-12-19T07:55:00.000Z"):
check_alert(self.alert["id"])
Expand Down
2 changes: 1 addition & 1 deletion posthog/tasks/alerts/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def skip_because_of_weekend(alert: AlertConfiguration) -> bool:
team_timezone = pytz.timezone(alert.team.timezone)

now_local = now.astimezone(team_timezone)
return now_local.isoweekday() in [5, 6]
return now_local.isoweekday() in [6, 7]


def next_check_time(alert: AlertConfiguration) -> datetime:
Expand Down

0 comments on commit 96da932

Please sign in to comment.