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

fix(alerts): edit alert modal fixes #27135

Merged
merged 5 commits into from
Jan 2, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use getDisplayNameFromEntityNode to get a formatted name that uses readable names, such as "Pageview" instead of "$pageview".

}`,
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
Loading